Skip to content

fix(perf): batch temporal ID lookups and eliminate duplicate history preparation - #1714

Merged
BYK merged 5 commits into
mainfrom
fix/1712-temporal-id-batching
Sep 7, 2026
Merged

BYK merged 5 commits into
mainfrom
fix/1712-temporal-id-batching

Conversation

@BYK

@BYK BYK commented Sep 7, 2026 •

Copy link
Copy Markdown
Owner

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):

Workload Preparation before → after Temporal-ID statements Post-response before → after
5,580 messages, mixed IDs 2,277 → 220 ms (10.4×) 2,789 → 28 2,284 → 4.2 ms
5,580 messages, restored IDs 1,226 → 193 ms 2,789 → 28 1,270 → 4.0 ms
20 messages 1.75 → 2.12 ms 9 → 1 2.43 → 1.24 ms

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

@github-actions

github-actions Bot commented Sep 7, 2026 •

Copy link
Copy Markdown
Contributor

Codecov Results 📊

✅ Patch coverage is 99.32%. Project has 8163 uncovered lines.
✅ Project coverage is 80.87%. Comparing base (57e9ef0) to head (7caf3e5).

Files with missing lines (3)
File Patch % Lines
packages/gateway/test/fixtures/semantic-history.ts 90.00% ⚠️ 1 Missing and 1 partials
packages/core/src/temporal.ts 100.00% ⚠️ 1 partials
packages/gateway/src/turn-temporal.ts 100.00% ⚠️ 1 partials
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        +1

Generated by Codecov Action

@BYK
BYK marked this pull request as ready for review September 7, 2026 16:39
@BYK
BYK requested a balanced review from Copilot September 7, 2026 17:07

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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.

Comment thread packages/gateway/src/turn-temporal.ts
Comment thread quality/benchmarks/temporal-preparation-1712.md Outdated

@BYK BYK left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@BYK
BYK merged commit 4f92eaa into main Sep 7, 2026
25 checks passed
@BYK
BYK deleted the fix/1712-temporal-id-batching branch September 7, 2026 17:55
@github-actions

github-actions Bot commented Sep 7, 2026

Copy link
Copy Markdown
Contributor
PR Preview Action v1.8.1
Preview removed because the pull request was closed.
2026-09-07 17:55 UTC

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Quick win: eliminate full-history temporal-ID N+1 work before upstream

2 participants