fix(codex): guard the first read in the history injection preflight - #5007
Conversation
#4957 narrowed the preflight so a cleanly-closed WAL store is inspected instead of refused, and it guarded the wrong operation. sqlite3_open_v2 never reads page 1, so a store whose header says WAL is not inspected until the first prepare. On macOS that is where the absent -shm is raised, one frame above the guarded attempt: the classification and the fallback had already returned, the error reached the catch-all, and a user with a cleanly-closed WAL store still got history_injection_preflight_unavailable with no way forward (#4943). The first read now happens inside the attempt, so the failure is classified where the policy lives. Every existing guarantee is unchanged: { readonly: true } stays the primary path and still joins a live writer's WAL, the immutable fallback is still admitted only when neither -wal nor -shm is on disk, and any other failure still keeps the original error and the refusal that follows from it. Linux cannot show the defect. Bun bundles its own SQLite there and that build materializes both sidecars on the same first read, so the primary path simply succeeds; macOS uses the system libsqlite3, which refuses. That is why #4957 passed the full Linux suite and nine Windows shards while its one platform- dependent case failed on macOS.
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. |
|
✅ Deterministic PR hygiene checks passed. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 5 remain after this review. 📝 WalkthroughWalkthroughThe preflight now performs an initial ChangesWAL preflight handling
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~15 minutes Change: Bug fix · Severity of issue fixed: Medium Sequence Diagram(s)sequenceDiagram
participant Preflight
participant SQLite
participant sqlite_master
Preflight->>SQLite: Open state database read-only
Preflight->>SQLite: Read sqlite_master
SQLite->>sqlite_master: Prepare first read
sqlite_master-->>Preflight: Success or SQLITE_CANTOPEN
Preflight->>Preflight: Classify the failure phase
Merge Risk: ⚪ Minimal · up to No actionable merge-blocking risk remains identified for this change. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 2 files. (1 skipped: 1 unsupported.)
✨ 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 |
리뷰 · 우선순위 74 / 80이 PR은 #4943 을 macOS에서 다시 고칩니다. 지금 이번 변경은 그 첫 읽기를 지금 라인 96 - 라인 1723 -
CI / #4956 - 지금 macOS·테스트 샤드가 pending 입니다. 콤보 failover 훅 타임아웃(#4956)은 30초짜리라, 8ms assertion 실패와는 구분해 보면 됩니다. 메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 923c156975
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const forcedRead = openFailureForTests?.(resolvedPath, "first-read"); | ||
| if (forcedRead) throw forcedRead; | ||
| // Page 1, read while the failure is still this function's to classify. | ||
| db.query<{ tables: number }, []>("SELECT count(*) AS tables FROM sqlite_master").get(); |
There was a problem hiding this comment.
This changes behavior in src/codex/, but the commit updates only structure/codex-home.md; structure/INDEX.md also maps this area to runtime.md, config.md, catalog.md, subagents.md, providers/openai-tiers.md, gui-and-management-api.md, and ops/docs-and-release.md. Review and update each mapped document in this change so their duplicated history-writer contract summaries remain synchronized, as required for changes to an owned source area.
AGENTS.md reference: src/AGENTS.md:L11-L11
Useful? React with 👍 / 👎.
1 participant
Summary
sqlite3_open_v2never reads page 1, so a store whose header says WAL is not inspected until the first prepare. On macOS that is where the absent-shmis raised — one frame aboveopenCodexStateForPreflight, after the classification and the immutable fallback have already returned. The error reached the catch-all, so a user with a cleanly-closed WAL store still gothistory_injection_preflight_unavailableandocx syncstill had no way forward.SQLiteError,code: "SQLITE_CANTOPEN",errno: 14,unable to open database file, with neither sidecar on disk.isStateDbCantOpenErrormatches it. It simply never saw it, because the read happened in the caller.openCodexStateForPreflightopens{ readonly: true }and readssqlite_masterbefore returning the connection.{ readonly: true }stays the primary path, so a live writer's WAL is still joined and a thread another process just migrated to paginated history still refuses. The immutable fallback is still admitted only when neither-walnor-shmis on disk. Any other failure still keeps the original error and the refusal that follows from it."open"or"first-read") instead of only the constructor; existing zero-argument hooks are unaffected.libsqlite3(3.51.0 onmacos-26-arm64) and refuses. Coverage looked complete because the other five cases inject the failure and therefore assert the narrowing decision rather than the platform.Closes #4943 on macOS; Linux and Windows behavior is unchanged.
Verification
ocxinvocation was executed at any point. Everything below is hosted CI at heada052e02f797b4ee4038bde71557bc48a485a4b9a.codex/diag-macos-wal-preflight, since deleted along with its temporary workflow and probe script; nothing from it is in this PR). Both runners built the fixture the failing test builds:journal_mode=walin the header, no-waland no-shmon disk.macos-26-arm64, Bun 1.4.0, SQLite 3.51.0: the read-only open succeeds; the firstPRAGMA table_infothen throwsSQLiteErrorSQLITE_CANTOPEN/ errno 14 /unable to open database file, with both sidecars still absent. Run through the policy exactly as fix(codex): inspect a cleanly-closed WAL state store in the history preflight #4957 shipped it, that error escapes to the caller; run through this PR's policy, the open, the pragma and a row read all succeed.-waland-shm. That is why the fallback is never exercised on Linux and why the defect is invisible there.file:…?immutable=1withSQLITE_OPEN_READONLY | SQLITE_OPEN_URIand reading through it succeeds, and leaves no sidecar.reaches a verdict on a cleanly-closed WAL store rather than the catch-all refusal— the case that failed deterministically ondev(runs 35286915280 and 35291460951, an 8ms assertion, not a timeout). The [Bug]: spawned Bun child processes stop producing output and never exit, on both macOS and Windows CI legs #4956 combo-failover hook timeout did not recur on this leg.pull_requestin this workflow, so — given that a platform gap is exactly what this PR is repairing — the full matrix was dispatched at the same head: run 35307801714, where all nine Windows shards, both macOS shards and all four Linux shards reported success. Read that run per job rather than by its aggregate: its trailingmacos controllane (the dispatch-only whole-pool serial rerun of the same macOS tests) was cancelled afterwards, which marks the run cancelled even though every test leg had already passed.tests/codex-integration/codex-history-provider.test.tsgains a case that injects the failure at the first read and asserts both that the preflight still reaches a verdict and that the attempt actually offers that phase. On the pre-fix code that phase does not exist, so the case fails on every platform rather than only on macOS.Checklist
structure/codex-home.mdowns this open policy and now records that the guard covers the first read, and why the Linux and Windows evidence could not see the gap.)