Repository navigation
fix(perf): batch temporal ID lookups and eliminate duplicate history preparation - #1714
Conversation
Codecov Results 📊✅ Patch coverage is 99.32%. Project has 8163 uncovered lines. Files with missing lines (3)
Coverage diff@@ Coverage Diff @@
## main #PR +/-##
==========================================
+ Coverage 80.81% 80.87% +0.06%
==========================================
Files 224 227 +3
Lines 42544 42661 +117
Branches 33821 33882 +61
==========================================
+ Hits 34377 34498 +121
- Misses 8167 8163 -4
- Partials 3739 3740 +1Generated by Codecov Action |
There was a problem hiding this comment.
🟡 Changes recommended
Snapshotting changes persisted message timestamps and can alter temporal ordering and distillation boundaries.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Optimizes temporal preparation for long resumed transcripts by batching identity lookups, avoiding duplicate history conversion, and adding performance telemetry and benchmarks.
Changes:
- Batch temporal-ID resolution in bounded SQLite queries.
- Snapshot only the latest user message for post-response storage.
- Add telemetry, regression tests, and benchmark fixtures/results.
File summaries
| File | Description |
|---|---|
quality/benchmarks/temporal-preparation-1712.md |
Documents benchmark methodology and results. |
quality/benchmarks/temporal-preparation-1712.json |
Stores raw benchmark samples. |
packages/gateway/test/store-turn-temporal.test.ts |
Updates temporal-storage tests for snapshots. |
packages/gateway/test/semantic-preparation.test.ts |
Tests batching, telemetry, provenance, and retries. |
packages/gateway/test/pipeline-streaming.test.ts |
Adapts Sentry span mocks. |
packages/gateway/test/pipeline-semantic-preparation.test.ts |
Verifies single full-history conversion. |
packages/gateway/test/fixtures/semantic-history.ts |
Generates the large synthetic transcript. |
packages/gateway/src/turn-temporal.ts |
Implements narrow post-response storage snapshots. |
packages/gateway/src/temporal-adapter.ts |
Supports legacy indices for sliced conversion. |
packages/gateway/src/semantic-preparation.ts |
Centralizes preparation, batching, and telemetry. |
packages/gateway/src/pipeline.ts |
Integrates optimized preparation and storage. |
packages/core/test/temporal-batch-identity.test.ts |
Covers batch identity correctness and query bounds. |
packages/core/src/temporal.ts |
Adds batched temporal-ID resolution. |
Review details
- Files reviewed: 13/13 changed files
- Comments generated: 2
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
BYK
left a comment
There was a problem hiding this comment.
Fresh independent adversarial review completed on 7caf3e5 (tree a7646a454ba3070337ef66c1d9ad45841fa8e8a3). Verdict: MERGE once final-head CI and hosted review gates pass; no outstanding correctness findings.
- PASS: scalar identity parity including planner-dependent ambiguity; tenant/project/session isolation; restored primary-key probes and bounded queries.
- PASS: owned latest-user snapshot, absolute/legacy indices, rollback/retry, no-store behavior, and every streaming/provisional post-response path.
- PASS: persistence-time timestamps, wire/provenance equivalence and bounded telemetry labels. The timestamp fix was independently mutation-verified; removing it reproduces the twenty-minute timestamp error.
- FIXED: the reviewer caught a flaky test that sorted equal timestamps by hashed IDs. The helper now uses insertion order for ties and the test freezes Date to exercise them. This was a test-ordering defect; production storage behavior is unchanged.
Validation: final typecheck, lint, format and bundle pass; ten sync-property runs pass all five properties each. Timestamp, snapshot and ambiguity mutants all fail at the intended assertions. Root focused run: 19 passed, 1 benchmark skipped. Full independent local suite: 9,522 passed, 7 documented base/environment failures, 228 skipped (455 files, 686 seconds). The seven failures are cache-stability (1), compact-endpoint (2), installer process/ps behavior (3) and optional GPU assets (1), matching the prior environment and previously passing hosted CI. Final test-helper change was separately retested; source stayed unchanged during the full suite.
Both Copilot threads are addressed and resolved. Final working tree is clean and hash-verified; disposable mutation worktree removed. Hosted CI is rerunning on the final head.
|
Long resumed transcripts synchronously queried SQLite once per tool-result placeholder before forwarding upstream, then converted and resolved the complete history again after the response.
This change batches owned current/legacy/restored ID lookups in groups of 100 and captures an independent latest-user snapshot for transactional post-response storage. Restored candidates use primary-key probes; genuinely ambiguous identities retain the original scalar selection in a bounded fallback. No-store preparation remains read-only, and original tool outputs, recall IDs and Responses provenance are preserved.
Adds fixed-label Sentry stage wall/CPU timings, event-loop delay, message/tool/placeholder counts and memory measurements. The committed synthetic Responses/Codex fixture contains 5,580 messages and 2,789 tool-result placeholders.
Benchmark medians (three alternating samples):
The snapshot retains one user message, 5,230 serialized bytes on the large fixture. This isolates semantic preparation; it does not claim to reproduce the production 74-second stall. Full production turn-to-upstream telemetry will show any remaining dominant stage. Reproduction commands, stage breakdown and raw samples: benchmark report.
Validation: focused identity/storage/provenance/retry/no-store/telemetry and both streaming/buffered pipeline regressions pass; gateway bundle, typecheck, lint and formatting pass; sync property battery passes 10 runs. Separate adversarial and security reviews addressed concurrent cleanup and restored-query-plan findings; cleanup regression was mutation-verified. Full local suite encountered environment/baseline failures in cache-stability, compact-endpoint, installer and optional GPU assets, also documented by base commit 7bb87a9. The initial head passed hosted CI, including full coverage and npm/Linux/Windows/macOS smoke tests. Copilot follow-up preserves post-response persistence timestamps without mutating the snapshot and corrects the telemetry description. A fresh adversarial review additionally found and fixed an equal-timestamp ordering assumption in the test helper. The final head is rerunning CI; focused regressions, typecheck, lint and formatting pass.
Fixes #1712