Skip to content

perf(server): read only checkpoint capture baselines - #32114

Open
macodev00 wants to merge 4 commits into
pingdotgg:mainfrom
macodev00:perf/checkpoint-baseline-ordinals
Open

macodev00 wants to merge 4 commits into
pingdotgg:mainfrom
macodev00:perf/checkpoint-baseline-ordinals

Conversation

@macodev00

Copy link
Copy Markdown
Contributor

Checkpoint capture reads every ready checkpoint ordinal in its scope, although it only checks ordinal 0 and max(0, run.ordinal - 1). The work grows with checkpoint history on every capture. This is the smaller checkpoint-read issue reported in #24895.

The production capture caller now opts into baseline selection. The store derives the two ordinals from the decoded run inside the existing transaction and uses the existing unique scope/ordinal index. Calls without that option still return every ready ordinal, including the cancelled ordinal-one context's [0, 1] result.

This is a focused redundant-read defect under CONTRIBUTING.md's obvious-bug exception: unused historical rows are materialized and discarded, and the fix preserves capture decisions and events. No product behavior, public schema, migration, provider ownership, workspace selection, locking, or finalization changes. Current patches and discussions for #31012, #29521, #30862, #31349, #30875 and the closed duplicate #31844 were checked; their work is not included here.

Verification

Base and policy revision: 6577c6b6185167c0e502fa01f9e8e8f2dee6ff25.

  • Extended the existing capture test at its owning service/store layer. Before the fix, tracing the real SQLite statement's successful result records all 128 ready rows instead of [0, 127]; the ordinal-one cases also fail the bounded-index-plan assertion. After the fix, all 8 capture tests pass. The default full-context assertion, cancellation [0, 1], missing-baseline, rollback, and repeat-capture checks pass.
  • vp test run apps/server/src/orchestration-v2/CheckpointCaptureService.test.ts: 8 passed.
  • vp run check:changed upstream/main --concurrency 1: format, lint, both knip checks, generated GraphQL, 3 affected package typechecks, and 63 tests in 3 files passed. No repository-wide test run.
  • Independent read-only Codex review found no actionable findings. The reviewer independently compared event payloads and checked the SQLite plans with bound values.

Runtime transcript

Ran the actual CheckpointCaptureService, CheckpointService, EventSink, SQLite reader worker, and Git driver with isolated file-backed SQLite databases and disposable real Git repositories on macOS, Node 26.13.0, SQLite 3.54.0, Git 2.50.1. Each fixture seeded 4,000 checkpoint metadata records and created the real baseline refs used by capture. No user repositories, live T3 state, paid provider turns, or running application server were used.

Scenario Actual rows before → after Selected after Persisted events
Waiting run 4,000, both baselines ready 4,000 → 2 [0, 3999] 4
Both baseline rows marked missing 3,998 → 0 [] 6
Cancelled run 1 with later checkpoint history 4,000 → 1 [0] 3
Rolled-back run 4,000 → 2 [0, 3999] 0

Observed SQLite plans for the actual statement:

before: SEARCH orchestration_v2_projection_checkpoints
        USING INDEX orchestration_v2_projection_checkpoints_thread_idx (thread_id=?)
after:  SEARCH orchestration_v2_projection_checkpoints
        USING INDEX orchestration_v2_projection_checkpoints_scope_ordinal_idx
        (scope_id=? AND ordinal_within_scope=?)

The after trace identifies the reader connection. Before/after event types and payloads match after normalizing capture-clock timestamps; random event IDs are excluded from that comparison. Capture still records value.txt as one addition/one deletion, and git show <captured-ref>:value.txt returns the expected edited contents. Metadata marked missing produces the same baseline events. Cancellation stays cancelled, rollback emits nothing, and a repeated capture emits no additional events. Default full-context counts remain identical before/after.

This demonstrates bounded row selection and equivalent service behavior on synthetic history. It does not claim end-to-end latency, production-database benchmarking, or web/desktop/mobile/provider coverage.

Implemented and independently reviewed by GPT-6-based Codex agents using the Codex harness.

@github-actions github-actions Bot added vouch:unvouched Author is not yet trusted in the VOUCHED list. size:S 10-29 changed lines (additions + deletions). labels Nov 6, 2026
@macroscopeapp

macroscopeapp Bot commented Nov 6, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Approved at 2cbedc1

Macroscope's review found this PR approvable — Checkpoint capture reads only the two baseline ordinals it uses instead of loading checkpoint history. Existing capture behavior and the store’s default full-context reads remain unchanged, keeping the change narrowly scoped.

You can add or adjust custom eligibility rules. Learn more.

@coderabbitai

coderabbitai Bot commented Nov 6, 2026 •

Copy link
Copy Markdown
Contributor

Explain Change →

No actionable comments were generated in the recent review.

Walkthrough

Checkpoint capture now requests baseline checkpoints from the projection store. The store can limit returned checkpoints to ordinal 0 and the preceding run ordinal. Tests cover this behavior for run ordinals 1 and 128.

Changes: Checkpoint capture

Layer / File(s) Summary
Ready baseline selection
apps/server/src/orchestration-v2/ProjectionStore.ts
getCheckpointCaptureContext accepts an option that limits returned checkpoints to ordinal 0 and max(0, run.ordinal - 1). Without the option, it applies no ordinal restriction.
Capture integration and validation
apps/server/src/orchestration-v2/CheckpointCaptureService.ts, apps/server/src/orchestration-v2/CheckpointCaptureService.test.ts
The capture service requests baseline checkpoints. Tests cover ordinals 1 and 128 and verify the preserved baseline ordinals.

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Refactor

Suggested reviewers: t3dotgg, revens2, juliusmarminge

Merge Risk: Minimal · up to 0c10f

This change narrows which checkpoint rows the capture path reads. Default behavior is preserved and the tests cover the ordinal edge cases. No concrete merge-blocking risk was identified.

Pre-merge checks: Passed 4

Check name Status Explanation
Title check Passed The title "perf(server): read only checkpoint capture baselines" matches the main change: the production capture caller now requests only the baseline checkpoint ordinals (0 and max(0, run.ordinal - 1…
Description check Passed The description follows the template's main sections: Problem, Change, Scope, and Verification. It explains the redundant checkpoint read, the baseline-only fix, the bug-fix scope rationale with a lin…
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Full details: Title check

Explanation

The title "perf(server): read only checkpoint capture baselines" matches the main change: the production capture caller now requests only the baseline checkpoint ordinals (0 and max(0, run.ordinal - 1)) instead of every ready checkpoint in the scope. It is a conventional-commit title with a concise, specific summary of the primary change.

Full details: Description check

Explanation

The description follows the template's main sections: Problem, Change, Scope, and Verification. It explains the redundant checkpoint read, the baseline-only fix, the bug-fix scope rationale with a linked issue, and detailed verification with commands, test counts, and a runtime transcript. The template's optional agent-model footer is present at the end ("GPT-6-based Codex agents using the Codex harness"). The description uses a non-template layout with no explicit Problem or Change headings, so the structure differs from the template, but the required information is present.

Finishing Touches

Generate unit tests (beta)

  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Recent review info

Run configuration

Commits

Reviewing files that changed from the base of the PR and between 97e8a95 and 0c10f05.

Files selected for processing: 2

  • apps/server/src/orchestration-v2/CheckpointCaptureService.test.ts
  • apps/server/src/orchestration-v2/ProjectionStore.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 8 remain after this review.

@maria-rcks

Copy link
Copy Markdown
Collaborator

Note

Written by gpt-6.1-sol on behalf of Maria

merged main without rebasing and resolved the checkpoint conflicts while preserving run-owned missing baselines. capture still reads only ordinal 0 and the previous run; default context reads remain complete. the diff stays at three files.

verified head 97e8a95d40cc9da58363e8eddd0726961ee4273c: all 31 reported checks pass, no conflicts, and two independent code reviews found no blockers. blacksmith vp run check:changed github/main passes on node 26.12.1, including lint, typecheck, and all 63 tests. no actionable bot findings remain. no live provider/client run or performance measurement was performed.

@navidnadali

Copy link
Copy Markdown
Contributor

+1

@juliusmarminge juliusmarminge added the takeover:in-progress A takeover lead is working on this PR label Nov 15, 2026
juliusmarminge and others added 2 commits November 15, 2026 04:34
Replace the tracer-captured SQL text and EXPLAIN QUERY PLAN assertions
with a direct check of the rows the baselines-only read returns.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@juliusmarminge

Copy link
Copy Markdown
Member

The automated pipeline stopped on this pull request and will not merge it:

the fix is written and reviewed, but proving it needs something no worker has:

  • Permanent blocker: the requested same-platform comparison needs macOS outside the device pool, which is unavailable under this brief.
  • Reduced baseline row reads, unchanged checkpoint decisions and persisted events, and unchanged default full-context reads were not demonstrated in the real app.
  • No stable capture latency medians, run spreads, captures, or uploaded driver were produced.

It stays open for a person to finish or close.

@juliusmarminge juliusmarminge added takeover:blocked Takeover blocked; reason in the PR body and removed takeover:in-progress A takeover lead is working on this PR labels Nov 15, 2026

This branch has not been deployed

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

Labels

size:S 10-29 changed lines (additions + deletions). takeover:blocked Takeover blocked; reason in the PR body vouch:unvouched Author is not yet trusted in the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants