Skip to content

Fix Codex rollout writer handoff races - #2578

Closed
ymichael wants to merge 3 commits into
mainfrom
bb/fix-github-issue-2327-thr_w8uqhhkiw7
Closed

Fix Codex rollout writer handoff races#2578
ymichael wants to merge 3 commits into
mainfrom
bb/fix-github-issue-2327-thr_w8uqhhkiw7

Conversation

@ymichael

@ymichael ymichael commented Aug 27, 2026

Copy link
Copy Markdown
Collaborator

Human comments

What was wrong

Codex 0.146+ holds a rollout writer lock for the lifetime of each app-server process, but the Codex bridge acknowledged release and spawned replacement children immediately after sending SIGTERM. A settings rebuild, stop/resume, bridge restart, or environment move could therefore resume while bb's previous child still owned the rollout.

The first implementation awaited child exit, but review found two more bridge-local windows. Reconstruction initially removed the old map entry before awaiting exit, allowing a concurrent stop or release-after-maintenance request to miss the in-flight replacement. Publishing the replacement closed that gap, but the pending old-child exit still lived only on the constructor stack: concurrent release found the connection-less replacement and acknowledged before the old child released its writer lock.

What changed

  • Make Codex child termination awaitable through finalized process exit, including the existing SIGKILL and close-grace bounds.
  • Await release before rebuilding, answering thread/stop, and completing maintenance operations that release a session.
  • Publish a connection-less replacement before waiting for the old child, and park the old child's exit promise on it. Concurrent stop/discard now marks the replacement closing and waits for both the parked old exit and any current child exit before acknowledging; reconstruction re-checks closing before spawning.
  • Retry transient active-writer construction refusals on a bounded 100/400/1000 ms ladder and log every retry with its method, delay, and attempt number.
  • Improve persistent-writer guidance to cover both another visible Codex session and a previous/leftover codex app-server process.
  • Add deterministic regressions for settings rebuild, response-time stop serialization, concurrent stop and discard during reconstruction, transient ownership, and persistent ownership recovery. Shared process-test cleanup is missing-log-safe and attempts every cleanup stage even if an earlier one fails.
  • Keep recovery provider-local. Independent real-Codex reproduction showed that the same losing app-server can retry resume after the owner exits, so no new wire recovery kind is needed. Native late-item ordering is orthogonal to the process-lifetime writer lock.
  • No host-daemon wire contract changed, so this PR does not increment HOST_DAEMON_PROTOCOL_VERSION beyond the version already on main.

This PR fixes bb-owned writer handoff races. A persistent writer owned by an independent Codex process is improved, not eliminated: after bounded retries it still returns an untyped bridge error, now with retry diagnostics and actionable guidance.

How you verified

The regressions were proven red against the exact behaviors they guard:

  • Mutating the stop path back to fire-and-forget makes the response-time lock assertion fail with expected true to be false.
  • Restoring the old map-before-await gap makes both concurrent release tests fail because reconstruction succeeds after release was already acknowledged.
  • Before parking the old-child exit on the published replacement, the concurrent stop and concurrent discard tests both acknowledged while writer.lock still existed; both immediate lock-absence assertions failed deterministically with expected true to be false on two runs.

Green checks on the final tree:

  • pnpm exec turbo run test --filter=bb-plugin-provider-codex --force -- src/bridge/bridge.writer-lock.test.ts src/bridge/bridge.archived-rebuild.test.ts (2 files, 8/8 passed)
  • pnpm exec turbo run test --filter=bb-plugin-provider-codex --force (25 files, 255/255 passed)
  • pnpm exec turbo run test --filter=@bb/agent-runtime --force (22 files, 318/318 passed)
  • pnpm exec turbo run typecheck --filter=bb-plugin-provider-codex --force
  • pnpm exec turbo run build --filter=@bb/host-daemon --force
  • pnpm exec oxlint and pnpm exec oxfmt --check on every changed TypeScript file
  • Real Codex 0.148.0 scratch smoke: app-server B received the native active-writer refusal while A held the rollout, then the same B process resumed successfully after A exited.

Fixes #2327

AGENT GENERATED

@ymichael
ymichael force-pushed the bb/fix-github-issue-2327-thr_w8uqhhkiw7 branch from 8cf0f0b to 0e5d134 Compare August 28, 2026 04:50
@Willhong

Copy link
Copy Markdown
Contributor

Rebased these three commits onto current main (author preserved) in #3460 and added the missing piece on the daemon side.

While reproducing #2327 on a live machine, the active-writer refusal turned out to start in the host daemon, not in the bridge: emitTranslatedEvents bound the delta-assembled thread/identity (empty thread id) to a stale pending-identity slot, so after a message edit the next thread/start on the same bridge recorded its Codex thread id onto the rewound thread, and both bb threads then resumed one rollout. #3460 fixes that binding, refuses to resume a provider thread another hosted thread owns, and carries this PR's retry ladder, guidance, writer.lock fixture, and writer-lock tests. The handoff serialization in constructThreadSession / releaseSession already landed via #3025, so main's implementation is kept there.

Leaving this PR open for you or a maintainer to close once #3460 is reviewed.

AGENT GENERATED

@SawyerHood

Copy link
Copy Markdown
Collaborator

This is an automated message. We have closed this pull request because it has been open for more than two weeks.

@SawyerHood SawyerHood closed this Sep 11, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

turn/completed can precede command completion and wedge later turns between active-writer and no-session errors

3 participants