fix(history): resolve a rollout's session_meta by thread id - #3056
fix(history): resolve a rollout's session_meta by thread id#3056ntdatt812 wants to merge 1 commit into
Conversation
|
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 (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughThe history provider now selects the latest ChangesHistory metadata handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to This localized fix updates thread-specific session metadata resolution and includes targeted tests; no actionable merge-blocking risk remains beyond normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Linked Issues checkExplanation The PR satisfies issue Full details: Out of Scope Changes checkExplanation The changes are within scope. The implementation updates the writer and both restore validation paths that issue
✨ 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 |
|
✅ Deterministic PR hygiene checks passed. |
✅ READY
Review readiness checklist
✅ 4/4 boxes ticked. This pull request is already Ready for Review. Hygiene✅ Deterministic PR hygiene checks passed. |
리뷰 · 우선순위 73 / 80이 PR은 이슈 #3026을 고칩니다. Codex 앱에서 포크/브랜치로 만든 세션의 롤아웃 파일에는, 자기 스레드의 쓰는 쪽이 id가 안 맞으면 파일을 건너뛰고, DB의 고치는 방법은 이미 있는 현재 라인 552 - 메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
9e54b60 to
0e892fc
Compare
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 lidge-jun#3026
0e892fc to
25ce1ed
Compare
Ingwannu
left a comment
Ingwannu
left a comment
There was a problem hiding this comment.
Approved on exact head 25ce1edc8941d7821ae2fe8e489a81a1407b4586 for the ID-aware forked-rollout repair.
The three changed boundaries now agree with Codex's own fold: updateSessionMeta, restore preflight, and restore readback all select the latest session_meta for the canonical thread ID, while foreign trailing records remain byte-preserved and same-ID concurrent writes still fail closed. The focused regression suite passed locally under isolated homes (48 pass, 0 fail). Repository typecheck currently reports the same three pre-existing fetch(..., { timeout }) errors on this head and on current origin/dev, so they are not introduced by this diff.
This approval covers the first, forked-session_meta defect in #3026 only. The issue's separate has_user_event drift case remains unresolved, and parseThreadFieldsFromRolloutText still performs an ID-agnostic last-record fold in a different reconstruction path. Please avoid closing those as fixed: change Fixes #3026 to scoped wording or keep/reopen #3026 with those remaining cases (a dedicated follow-up is also fine).
Before merge, rebase the currently-behind head onto latest dev and require exact-head cross-platform CI. No merge is authorized by this review alone.
|
Landed via #3103 at 원본 PR은 maintainer merge-train으로 이미 |
lidge-jun
added
the
landed-via-maintainer
3 participants
Fixes #3026.
The pair that can never validate
A forked rollout trails its parent thread's
session_meta. Three call sites folded the file with the id-agnosticreadLatestSessionMeta, so all three answered with the parent's provider:updateSessionMeta(line 878)snapshotRolloutForRestore(line 552)history_backup_rollout_postimage_mismatchassertRestoreReadback(line 631)history_backup_rollout_readback_mismatchThe reporter identified the two validators. The writer is the same defect on the write side, and it is where the unrecoverable state is created: routing flips the database row but skips the rollout, so the manifest records an entry whose file never received the OpenCodex post-image. Neither restore branch can match it, and
preflightRestoreTargetsis all-or-nothing — a handful of forked threads blocks every entry,ocx stopexits 1 on every invocation, andocx updateaborts at its gate. Nothing clears on retry.Fixing only the validators does not restore a forked thread: the writer still skips,
requireDurableProvideris unmet, and restore throwshistory_backup_rollout_unrestorable. I confirmed that by running it.The fix
Resolve the record by id at all three sites, via a
readLatestSessionMetaForIdwrapper around thereadLatestSessionMetaForIdFromTextalready used bycompensateConcurrentSessionMetaAppend.For an unforked rollout the last record is this thread's, so behaviour is byte-identical. The file's identity stays anchored by
readFirstLineProviderValue/inspectFirstLineProvider, which already require line 1 to carryentry.id.does not append when the latest session_meta belongs to a different thread idassertedfiles: 0— the skip this PR removes. Its comment gives the reason: "no misleading append for a foreign id". That concern was about cloning the foreign record, and resolving by id addresses it directly: we append this thread's own metadata, which the app honors, rather than a copy of a record it would discard.I rewrote the test rather than delete it, and it is now strictly stronger — it pins the append's id, that the original bytes remain a prefix, and that the foreign line survives unchanged exactly once:
Please look at this hunk first — it is the one judgement call in the PR. If you would rather keep forked rollouts unwritable, the alternative is to stop flipping the database row for them too, so no manifest entry is ever recorded; say the word and I will send that instead.
Tests
Three new tests, plus the rewritten one:
restores a forked rollout that trails its parent thread's session_meta— the reported end-to-end pathleaves a foreign trailing session_meta untouched while restoring its ownconsumes the manifest when a foreign-id session_meta lands after restore readback— the last-moment race, viasetBeforeHistoryBackupConsumeForTests. Its same-id sibling (keeps the manifest when a newer same-id rollout provider lands after readback) still fails closed, so the pair now pins both halves of that comment.Each site was reverted individually to confirm the tests actually hold it:
snapshotRolloutForRestoreassertRestoreReadbackupdateSessionMetaNot reproduced on macOS — I am on Windows and built the forked rollout from the fixture instead. The defect is filesystem-agnostic.
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.
Summary by CodeRabbit
Bug Fixes
Tests