Repository navigation
fix(codex): adopt pre-substrate homes into the write coordinator (#1049) - #2612
Conversation
|
✅ Deterministic PR hygiene checks passed. |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (10)
📝 WalkthroughWalkthroughChangesPre-substrate Codex adoption
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CodexInjection
participant CodexWriteLock
participant openCodexCoordinatorTransaction
participant CoordinatorDatabase
CodexInjection->>CodexWriteLock: request adoption with apply direction
CodexWriteLock->>openCodexCoordinatorTransaction: open adoption transaction
openCodexCoordinatorTransaction->>CoordinatorDatabase: publish adoption-pending database
CoordinatorDatabase-->>openCodexCoordinatorTransaction: return published database
openCodexCoordinatorTransaction-->>CodexWriteLock: provide coordinator transaction
CodexWriteLock-->>CodexInjection: apply coordinated transition
Suggested reviewers: ✨ Finishing Touches📝 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 |
![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: 4e1de09d21
ℹ️ 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".
| let transitionReceipt: { nativeGeneration: number; currentTxId: string } | undefined; | ||
|
|
||
| if (eligibility.kind === "coordinated") { | ||
| if (eligibility.kind === "coordinated" || eligibility.kind === "adopt") { |
There was a problem hiding this comment.
When a foreground ocx start adopts a pre-substrate home and then exits, syncCleanup in src/cli/index.ts:305-329 calls the synchronous restoreNativeCodex() at line 321 rather than this newly coordinated async path. That function still mutates config, profile, catalog, and history without the write lock or a remove transition, so it can overwrite another process's coordinated apply and, even without concurrency, leaves the database claiming the last operation was an apply after the native files were restored. Route synchronous shutdown through the coordinator or defer cleanup to this async path, with a focused shutdown regression.
AGENTS.md reference: src/AGENTS.md:L22-L25
Useful? React with 👍 / 👎.
|
|
||
| linkSync(tempPath, finalDatabasePath); | ||
| options.onCheckpoint?.("published"); | ||
| if (process.platform !== "win32") fsyncPath(parent); |
There was a problem hiding this comment.
On POSIX runtime namespaces backed by a filesystem that rejects directory fsync, such as a virtual/shared mount returning EINVAL or ENOTSUP, this throws after linkSync has already published the complete coordinator. The opener then reports a non-retryable lock_unavailable, causing the first injection or restore to fail even though a retry would open the newly created authority; src/lab/subject/installation-salt.ts:55-68 already handles this platform case by ignoring only known unsupported-directory-fsync codes. Apply the same handling here while retaining genuine I/O failures.
Useful? React with 👍 / 👎.
1 participant
Summary
Closes #1049.
inject-coordination.tsclassified routed and indeterminate pre-substrate homes aslegacy-uncoordinated, andinject.tsskipped transition publication on that path, so a home that predates the write substrate never entered the coordinator.git grep adoption-pendingreturned nothing — the planned adoption state was designed and never built.A routed pre-substrate home now publishes a complete
adoption-pendingcoordinator before any native write, and both apply and restore adopt through the write lock. Indeterminate homes keep the legacy-operable path: they are the case where we genuinely cannot tell what we are looking at, and guessing there is what would strand a home.Crash-safety is the primary property here, not a footnote. Publication is atomic by construction: the coordinator is built in a temp file, committed, fsynced, and then
linkSynced into place. Every crash window leaves exactly one of two states — no authoritative database at all, or a complete resumable one. There is no window that leaves a half-built coordinator, because a partially written temp file is never the published name.Verification
The crash tests are real: a child process is killed at each of the three checkpoints (
temp-created,temp-committed,published) and the parent then asserts the home is still adoptable, resuming a full transition through it.Falsified independently on the merge with
dev, not only on the branch: replacinglinkSyncwith in-place creation — the obvious "simpler" implementation — turns all three crash cases red withCodexCoordinatorLegacyAmbiguousError: An existing unversioned coordinator database cannot be adopted automatically. That is exactly the stranding this issue is about, and it is what the atomic publication buys. Restored, all three pass.The subagent's own run additionally showed 99 pass across 8 files and 56 pass across 5 files, with five new regressions driven red by reverting their production hunks.
Checklist
devdevlog/_plan/260826_pre_substrate_adoption/010_design.mdSummary by CodeRabbit
New Features
Bug Fixes
Tests