fix(history): resolve a rollout's session_meta by thread id (#3026) - #3103
Conversation
A forked or branched session appends the SOURCE thread's session_meta after its own. codex-rs `apply_session_meta_from_item` discards any record whose payload id is not the canonical thread id, so that trailing line is ordinary rollout content -- but three call sites folded the file with the id-agnostic `readLatestSessionMeta` and answered with the foreign thread's provider. The writer skipped such a file outright, while the database row still flipped. That pair is unrestorable: the manifest expects an OpenCodex post-image the file never received, so `snapshotRolloutForRestore` throws history_backup_rollout_postimage_mismatch. `preflightRestoreTargets` is all-or-nothing, so a handful of forked threads blocks every entry, `ocx stop` exits 1 on every invocation, and `ocx update` aborts at its gate. The state never clears on retry. Resolve the record by id in the writer and in both validators. The append now carries this thread's own metadata, which the app honors, instead of cloning a foreign record it would discard. Fixes #3026
Carries @ntdat812's 25ce1ed for the id-aware half of #3026 and adds the second half the issue reports. has_user_event has two writers, so a final state cannot establish who wrote it. Routing derives the post-image value from first_user_message AT SNAPSHOT TIME, but restore recomputed it from the message as it is now. A row routed while its message was null, then given a first message by the user, looked exactly like OpenCodex's own write - and restore put it back to 0. The manifest entry now records whether the message was non-empty when the snapshot was taken. A boolean only, never the text: the manifest is a file on disk and the message is user content. Ownership is decided by which tuple the row wears. The recorded original and the expected post-image are unambiguous. Drift from 0 to 1 on the routed tuple is the user's, because a row wearing that tuple was written by OpenCodex and anything on top of it followed. Drift on the original tuple is decided by a per-entry relabel marker written before the routing write and resolved after it, so a crash between the two leaves an honest unknown that the observed row resolves. One cell genuinely cannot be decided: an OpenAI-origin entry whose routing would have produced a 1, wearing the original tuple, with the marker still pending. Routing-never-landed-plus-activity and routing-landed-then-legacy- recovery produce an identical row and nothing durable separates them. That refuses, which is what dev already does there. A guess would either erase real activity or fabricate it. Manifests written before these fields exist keep the current-row reading and restore exactly as they do today - refusing them would brick the population the fix exists to repair.
Review of the has_user_event work found three holes. A surviving manifest means a previous route/restore cycle did not consume it, and its relabel marker describes that attempt. Carrying a stale "committed" into a new routing attempt let a later restore treat it as proof that OpenCodex authored an event flag the user had since set. Every new attempt now reopens the marker as pending; the recorded snapshot itself stays, since it is the original provenance. Manifest v2 existed only in the type. The validator still rejected version 2 and the writer still emitted version 1, so the new fields were being written into a manifest that claimed not to have them. The validator now accepts 1 and 2 and checks both optional fields when present - an unvalidated hadFirstUserMessage: "false" is truthy and would have selected the wrong restore verdict. The classifier's D branch matched any opencodex row, so C and D both matched an exec-origin row and D won only by being tested first. D is now the tuple routing actually produces: routeOpenai keeps the source, routeExec moves exec to cli. The plan asked for a state-transition matrix and I had shipped two cases. There are now sixteen, one per cell, and writing them found the exec-origin post-image case where routeExec always writes 1 - so that row is B, not D.
Review found the reopen fix incomplete and its regression vacuous. The regression let an ordinary restore consume the manifest, so the second route saw no existing entry and the reopen code never ran. It now forces the consume to fail with setBeforeHistoryBackupConsumeForTests, which is the only way a manifest actually survives into a second routing attempt. That exposed the real gap. Reopening the marker was not enough: the entry also carries hadFirstUserMessage and hasUserEvent, and both described the PREVIOUS attempt. The stale message flag made the new routed row match the expected post-image, and the stale event value restored the thread to a state two events old - a restore that had already happened, plus whatever the user did afterwards. Both are re-recorded for the new attempt. The provenance tuple stays, because it is the original and a routed row must never overwrite it; the event baseline is only refreshed when the row is back at that tuple. Manifest promotion was also incomplete: only a brand-new manifest started at version 2, so an existing v1 kept writing the new fields under a schema that did not declare them. Any manifest written forward is now v2. The regression asserts the whole sequence: route, failed consume, user types, route again, restore - and the user's first message survives. Red without the refreshed baseline.
…line Third review round found the baseline refresh resting on the same inference the classifier already refuses: tuple equality does not establish ownership. Route to opencodex, legacy recovery back to openai, and the row wears the original tuple with a 1 that OpenCodex wrote - refreshing there adopts that as the user's baseline and the next restore keeps an event the user never generated. So the refresh now requires proof, and the proof is a relabel marker reading "none". Nothing wrote that value before; a restore that lands and passes its readback now records it on the surviving manifest, which is exactly the state where a later attempt needs to tell "already restored" from "still mid-route". Absent the proof the recorded baseline is kept: one event stale still restores to a state the user was in, while a wrong refresh silently rewrites what OpenCodex owns. The write happens after finalization fails rather than before consuming it - rewriting the manifest ahead of the consume changes the fingerprint the consume checks, which broke twelve tests when I tried it that way. Adds the legacy-return row to the classifier matrix, which is the history the audits kept circling: committed marker, expected-event-1 route, original tuple, restores to 0.
Fourth review round: the "none" proof is best-effort, so a failed write leaves the entry reading committed while the row has drifted. Keeping the recorded baseline erases the user's event; refreshing it preserves one OpenCodex authored. That is another undecidable cell, and the reviewer was right that neither reading is the safe default. A reroute in that state now refuses with history_apply_ambiguous_reroute rather than committing to an interpretation. Nothing is written, the manifest stays for a human, and the refusal surfaces as an integrity failure with zero rows applied. Adds the end-to-end legacy-return regression the reviewer asked for: route writes opencodex/vscode/1, legacy recovery pulls it back to openai/vscode/1, and the row now wears its original tuple carrying an event OpenCodex wrote. The classifier row alone could not express this history - it needed the real route and the real recovery. The assertion is the contract rather than one implementation: the 1 must not survive as the user's, so either restore returns it to 0 or the layer refuses and keeps the manifest. Drops the duplicate classifier row that added no coverage over its neighbour.
Fifth review round found the refusal too broad and the legacy-return test green for the wrong reason. The guard rejected every unproven event mismatch, including the case where the previous route would have written 0 - there an observed 1 can only be the user's, which is the expected-event-0 cell the classifier deliberately preserves. Refusal is now conditioned on whether the prior route could itself have authored the difference. The end-to-end legacy-return test passed because the fixture's rollout omits source, so restore refused during rollout preflight before the classifier mattered - a broken classifier would have passed it too. It now writes the matching session_meta so the real path runs, and asserts the exact outcome: the 1 returns to 0 and the manifest is consumed. Two smaller things: leftover debug logging is gone, and the specific integrity code now travels with the result. failureReason: "integrity" alone reads as "retry or run doctor", and history_apply_ambiguous_reroute is not retryable - it means two histories produced the same row and the manifest needs a human.
… reason out Sixth review round found the reroute guard collapsed to a single flag and the integrity code stopping at the worker boundary. The condition now names all five cells. A 1 to 0 drift is always foreign - nothing in this system clears the flag, so a baseline that moved down is a decision the manifest does not own. A 0 to 1 drift is the user's when the entry is exec-origin (routeExec moves source to cli and legacy recovery does not move it back, so it was never routed away and back), when a "none" marker proves the previous relabel was undone, or when the previous route would have written 0. It refuses when the previous route would have written 1, and when a legacy entry records nothing about it - undefined is not false. integrityCode existed on the sync result and went nowhere: HistoryWorkerResult and CodexHistoryJobOutcome had no field for it and classifyWorkerResult dropped it, so an operator still read "failed integrity checks; run doctor". It is typed and forwarded through both, and an ambiguous reroute now says what it is - the manifest cannot prove whether an earlier relabel was undone, nothing changed, resolve it manually rather than retrying. Retrying reaches the same refusal.
|
✅ Deterministic PR hygiene checks passed. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
Included review availability: Your plan provides up to 10 included reviews per hour; 5 remain after this review. 📝 WalkthroughWalkthroughThe change adds manifest provenance fields, classifies restore-time event drift, scopes rollout metadata reads by thread ID, preserves post-routing user activity, and propagates specific integrity codes to job failure messages. Tests cover relabel states, legacy manifests, forked rollouts, and ambiguous reroutes. ChangesHistory integrity flow
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: 🔵 Low · up to The change correctly restores history by thread and preserves snapshot-time user activity, but an integrity failure during restore finalization can leave provenance metadata unresolved after restoration has already occurred, potentially blocking later history operations. The PR is mergeable with explicit owner awareness and follow-up for this bounded recovery-state risk. Sequence Diagram(s)sequenceDiagram
participant HistoryJob
participant HistoryWorker
participant HistoryProvider
participant BackupManifest
participant StateDatabase
participant Rollout
HistoryJob->>HistoryWorker: run history transition
HistoryWorker->>HistoryProvider: apply or restore history
HistoryProvider->>BackupManifest: read provenance and relabel state
HistoryProvider->>StateDatabase: classify and update thread rows
HistoryProvider->>Rollout: read session_meta for thread ID
HistoryProvider-->>HistoryWorker: success or integrityCode
HistoryWorker-->>HistoryJob: failed outcome with historyIntegrityCode
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Title checkExplanation The title accurately identifies the primary history-restore fix: resolving rollout session_meta by thread ID. It is concise, specific, and directly related to the changes in src/codex/history-provider.ts and issue Full details: Linked Issues checkExplanation The pull request addresses both coding defects in issue Full details: Out of Scope Changes checkExplanation The changes remain within the linked issue scope. The manifest schema updates, integrity-code propagation, restore classification, marker handling, operator messaging, documentation, and tests directly support forked-rollout restore reliability and has_user_event preservation. Full details: Docstring CoverageExplanation Docstring coverage is 36.84% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 19 functions across 6 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
리뷰 · 우선순위 75 / 80이 PR은 지금 첫 번째 구멍은 지금 HEAD에 그대로 있습니다. 작성 길도 같은 구멍입니다. HEAD 두 번째 구멍도 HEAD에 있습니다. 이번 PR은 두 구멍을 같이 닫습니다. 커밋은 아홉 개입니다. 첫 커밋 아이디 반쪽은 경로 래퍼 이벤트 반쪽은 매니페스트에 불리언만 남깁니다. 본문은 절대 안 씁니다. 소유권은 라우팅 전에 테스트는 구멍 두 개를 빨강으로 먼저 잠급니다. types.ts/config.ts 분할과는 무관합니다. 손대는 파일은 히스토리 복구 경로뿐입니다. close-don't-rebase 대상이 아닙니다. #3056은 아이디 반쪽만 있는 원본이고 아직 OPEN입니다. 이번 것이 rebase+두 번째 반쪽 착륙입니다. 머지 전에 #3056을 닫지 마세요. 머지 뒤에 라인 552 / 631 / 878 (HEAD 메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
2 participants
Summary
Closes #3026. Carries @ntdat812's
0af9c0fd5from #3056 for the id-aware half, then adds the second half the issue reports.The id-aware half.
readLatestSessionMetareturned the lastsession_metaline regardless of whose thread id it carried. A forked rollout has more than one, so restore read a sibling thread's metadata, rejected the entry, and one bad entry fails the whole manifest — wedging everyocx stopandocx update. The reporter measured 6 of 389 entries affected.The
has_user_eventhalf. That field has two writers. Routing derives the post-image value fromfirst_user_messageat snapshot time; restore recomputed it from the message as it stands now. A row routed while its message was null, then given a first message by the user, looked exactly like OpenCodex's own write — and restore put it back to 0.The manifest entry now records whether the message was non-empty at snapshot time. A boolean only, never the text: the manifest is a file on disk and the message is user content. Ownership is then decided by which tuple the row wears and a per-entry relabel marker written before the routing write and resolved after it, so a crash between the two leaves an honest unknown that the observed row resolves.
Some states genuinely cannot be decided — an OpenAI-origin entry wearing its original tuple with
0 → 1drift, whose route would itself have written a 1, is reachable both by "routing never landed and the user typed" and by "routing landed and legacy recovery pulled it back". Those refuse, which is whatdevalready does there. A guess would either erase real activity or fabricate it, and the refusal now says so rather than reporting a generic integrity failure.Manifests written before these fields exist keep the current-row reading and restore exactly as they do today. Refusing them would brick the population this fix exists to repair.
Seven adversarial review rounds, findings 3 → 2 → 1 → 1 → 2 → 2 → 0. They caught, among others: a stale
committedmarker surviving into a new attempt, a regression that never reached the code it claimed to test, and an over-broad refusal that rejected a decidable case.Plan: devlog/_plan/260831_prio70_train_round2/030_wp3_forked_rollout_restore.md.
Verification
A sixteen-row classifier matrix covers every cell, plus end-to-end regressions for the null-then-typed history, the route-then-legacy-recovery history, a surviving manifest re-snapshotted by the next attempt, the ambiguous reroute refusing, and a legacy v1 manifest restoring unchanged. Each behavioral regression was driven red first.
Checklist
devdocs-site/update is requiredSummary by CodeRabbit
Bug Fixes
Improvements