fix(codex): make history no-op detection atomic - #1294
Conversation
📝 WalkthroughWalkthroughHistory migration now performs read-only no-op verification for canonical database and backup state. The worker returns proof-bearing converged results. Job validation checks proof metadata. The guardian relies on worker verification, and transitions persist verified counts. ChangesHistory no-op migration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Guardian
participant HistoryWorker
participant HistoryProvider
participant HistoryTransition
Guardian->>HistoryWorker: attempt migration
HistoryWorker->>HistoryProvider: snapshot canonical history state
HistoryProvider-->>HistoryWorker: verified no-op proof or pending state
HistoryWorker-->>Guardian: converged result with proof
Guardian->>HistoryTransition: persist migration outcome
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/codex/history-migration-guardian.ts (1)
88-104: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winA converged-but-unproven result now burns the whole tick budget and ends with a false "DB stayed locked" warning.
Line 91 makes
moved > 0 || verifiedNoopthe only stop condition. The previous recount-based stop was removed. Consider a database that is already fully onopenaibut whose backup manifest the provider cannot certify — for example a manifest written before thestateDbPathfield existed.inspectBackupForNoopinsrc/codex/history-provider.tsline 272 requirestypeof manifest.stateDbPath === "string"and otherwise returnsmanifest-schema, sosnapshotCodexHistoryNoopreturnsunknown, andsrc/codex/history-worker.tsline 144 falls through towriteHistoryProviderTransition.The resulting sequence on every tick:
migrateFn()returns{ rows: 0, files: 0 }withfailedunset.movedis 0 andverifiedNoopis false, so line 91 does not stop.- Line 104 reschedules.
This repeats for
maxTicksticks. Each tick acquires H and runs a full provider transition write, so the guardian performs repeated writes against a database that needs no work. At exhaustion, line 101 logs "Codex history DB stayed locked; legacy threads not yet migrated" and directs the user to close the Codex app and runocx sync. No lock contention occurred and no legacy threads exist. The message is wrong and the recommended action is useless.Separate the two exit conditions. Track whether any attempt was actually blocked or failed, and only emit the lock warning in that case. Better still, propagate the snapshot
reasonso the guardian can stop on a stable non-raceunknown.🩹 Minimal fix: do not claim a lock that never happened
let stopped = false; let pending: { cancel(): void } | undefined; let ticks = 0; + let sawObstruction = false; const tick = async () => { ... const result = await migrateFn(); + if (result.failed) sawObstruction = true; if (!result.failed) { ... } } catch { + sawObstruction = true; /* hard errors are not retryable state — fall through to the tick budget */ } if (ticks >= maxTicks) { stopped = true; - log.log("⚠️ history-migration: Codex history DB stayed locked; legacy threads not yet migrated. Close the Codex app and run 'ocx sync' (or check 'ocx doctor')."); + if (sawObstruction) { + log.log("⚠️ history-migration: Codex history DB stayed locked; legacy threads not yet migrated. Close the Codex app and run 'ocx sync' (or check 'ocx doctor')."); + } return; }The existing test at
tests/history-migration-guardian.test.tslines 125-144 already models the unverified zero-row path, so extend it with a run that never returnsverifiedNoop: trueand assert the warning is absent.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/codex/history-migration-guardian.ts` around lines 88 - 104, Update the guardian loop around migrateFn and the maxTicks handling to distinguish genuine lock/attempt failures from a stable zero-work result that lacks verifiedNoop. Stop cleanly for converged unverified results, or propagate the snapshot reason as needed, and emit the “DB stayed locked” warning only when an attempt was actually blocked or failed. Extend the existing history-migration guardian test to cover repeated unverified zero-row results and assert that no lock warning is logged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/codex/history-job.ts`:
- Around line 112-134: Update the worker-result validators around
isPlausibleWorkerResult and isPlausibleWorkerResultForTests to require
target.operation and accept proof objects only when it equals "migrate-openai";
preserve proofless done-result validation for other operations. Propagate the
full request operation from the existing caller, widen both target types
accordingly, and add a boundary test rejecting proofs on non-migrate-openai
requests.
In `@src/codex/history-migration-guardian.ts`:
- Around line 76-81: Remove the unused countFn probe and its dependency from the
history migration guardian, then update tests/history-migration-guardian.test.ts
to stop supplying countFn and asserting its invocations. Replace the stale
“Locked probe or pending work” comment with wording that reflects the
unconditional migration-pass attempt.
In `@src/codex/history-transition.ts`:
- Around line 36-40: Update the comment above pendingRows and backupEntries to
reflect that proof counts are verified worker results, including valid zero
values, while null is used only when no proof is available. Keep the existing
nullish-coalescing assignments unchanged and explicitly avoid describing zero as
manufactured or implying this path skips the relevant verification.
In `@tests/codex-history-provider.test.ts`:
- Around line 366-410: Ensure backup artifacts are cleaned up in both affected
test locations: in tests/codex-history-provider.test.ts lines 366-410, wrap each
test body in try/finally and remove backupPath and dir with forced cleanup,
matching the existing WAL-test pattern; in tests/codex-history-worker.test.ts
line 67, register historyBackupPathFor(stateDb) for teardown alongside the
sandbox root and verify getConfigDir() is consistent in the test process and
child processes using fixture.env.
---
Outside diff comments:
In `@src/codex/history-migration-guardian.ts`:
- Around line 88-104: Update the guardian loop around migrateFn and the maxTicks
handling to distinguish genuine lock/attempt failures from a stable zero-work
result that lacks verifiedNoop. Stop cleanly for converged unverified results,
or propagate the snapshot reason as needed, and emit the “DB stayed locked”
warning only when an attempt was actually blocked or failed. Extend the existing
history-migration guardian test to cover repeated unverified zero-row results
and assert that no lock warning is logged.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 78aa40d4-1dbd-4b71-9216-52ae7e0892b3
📒 Files selected for processing (10)
src/codex/history-job.tssrc/codex/history-migration-guardian.tssrc/codex/history-provider.tssrc/codex/history-transition.tssrc/codex/history-worker.tstests/codex-history-provider.test.tstests/codex-history-worker-boundary.test.tstests/codex-history-worker.test.tstests/codex-transition-state.test.tstests/history-migration-guardian.test.ts
|
@lidge-jun The review fixes are complete on exact head Author-side verification: Bun 1.3.14 related set 48/48 (210 assertions), transition mapping 2/2 (7 assertions), typecheck, privacy scan, and diff check passed. The remaining gates require maintainer approval to run: I am keeping the PR in draft until exact-head CI can run and report. |
|
Approved the pending Cross-platform CI run at this exact head, and it came back green. Worth explaining the silence beforehand: contributor PRs queue their workflow runs as To be precise about what this does and does not do: it only unblocks CI. It does not make this mergeable. The four-box readiness checklist in the description is your attestation, and the gate keeps the PR in draft until you complete it. With CI now green at your head, box 1 is provable. If anything in the run looks wrong to you, say so and I will dig into it rather than leaving you to guess. |
402a260 to
c4e9919
Compare
|
@lidge-jun #1294 has been rebased and revalidated on the current policy baseline. The new exact head is Local exact-head verification passed: Bun 1.3.14 proof 1/1, related set 48/48 (210 assertions), transition mapping 2/2 (7 assertions), typecheck, privacy scan, and diff check. The new fork workflow runs are awaiting approval: I will complete the four-box readiness section after exact-head CI and automated review finish. |
0236a60 to
deaf978
Compare
|
@lidge-jun Updated #1294 to exact head Exact-head Bun 1.3.14 verification is green: proof 1/1, related set 48/48 (210 assertions), transition 2/2 (7 assertions), typecheck, privacy, and diff check. CodeRabbit and target checks are green; unresolved review threads remain zero. Current exact-head workflows awaiting approval: I will check the final two readiness boxes once both runs are green. |
deaf978 to
cbd5304
Compare
|
✅ Deterministic PR hygiene checks passed. |
⏳ DRAFT
What to do
Review readiness checklist
2/4 boxes ticked. This PR stays in draft until every box above is ticked. |
|
Landed on Verified before merge: An independent reviewer went looking specifically for a missed correctness regression in the concurrent-migration design and did not find one. Full suite green on the rebased head (10199 pass, 0 fail). Thanks — 500 lines is a lot for a no-op check, but the WAL, malformed-manifest, forged-proof, and guardian-retry cases each earn their place. |
2 participants
Summary
data_versionfence that detects WAL commits.Closes #1183.
Verification
cbd53046d0617ee4fed03c52d3bb56a5de3eb261rebased and validated ondev@8e6d661b2340352219d5e3d694965a61244d91d1; livedev@a9838c1a353cab772265b65c14a473cde5c4441awas five disjoint commits ahead at publication time.bun x tsc --noEmit: passed.bun scripts/privacy-scan.ts: passed.git diff --check 8e6d661b2340352219d5e3d694965a61244d91d1...HEAD: passed.Checklist
Summary by CodeRabbit
Bug Fixes
Tests
Review readiness checklist
This PR stays in draft until every box below is ticked. Tick all four boxes once the requirements are met:
All CI tests are green on my local testing.
I pushed my PR to the latest dev commit.
I resolved all correct Codex and CodeRabbit findings.
My PR is ready for review.