Skip to content

Retry Codex lock contention instead of refusing restores on relaunch - #20499

Merged
austinywang merged 4 commits into
mainfrom
fix-codex-restore-coordination-retry
Oct 14, 2026
Merged

austinywang merged 4 commits into
mainfrom
fix-codex-restore-coordination-retry

Conversation

@austinywang

@austinywang austinywang commented Oct 14, 2026 •

Copy link
Copy Markdown
Contributor

Summary

On relaunch, cmux restore codex <id> could refuse an unowned conversation with "cmux could not verify whether this agent session is already running." When dozens of Codex tabs resume in the same second, each new Codex briefly holds thread-writer-locks/.coordination.lock. The CLI's final exec guard probed once, saw .changing, and failed it the same way as an unreadable lock.

The final guard now re-probes .changing for a bounded ~3.5 s (backoff 20 ms → 1 s). It wakes early when Codex creates, renames or unlinks a thread lock, and otherwise re-probes at each bound; a flock release raises no file event. The shared policy is CodexWriterLockSettle in CMUXAgentLaunch. .active still refuses at once with the existing held-writer notice, and .unavailable still refuses at once. Each case now has its own stage and message: session.writer-lock-held, session.writer-lock-changing ("Codex was still updating its session locks…"), and session.writer-check-unavailable ("cmux could not read this conversation's Codex session lock…").

App-side admission (agent.restore.admit) already treated .changing as recoverable, so the CLI kept retrying. Each waiting request spent up to 8 s on a file event, though, and a coordination release never raises one, so a burst could stall each restore for 8 s per round. A waiting request on .changing now re-probes inside the same request on the same settle bounds, still woken early by lock-directory events. The request stays bounded at about 3.5 s, and a lock that never settles adds no extra RPC rounds. Every other state keeps the 8 s event wait.

The re-probe bounds are timed waits, not synchronization: releasing a flock raises no event to wait on, so a bound is the only signal available. Each is capped and also ends early on a lock-namespace file event.

Testing

  • Red 7e1dd03 / green 59558c1: CLICodexWriterLockRestoreTests/coordinationContentionIsTransient runs the bundled CLI with another holder on the coordination lock. releases: true releases it 0.4 s after the CLI's socket request and expects the restore to exec. releases: false keeps it held and expects the "still updating" failure, not the generic one. Red receipts are pending: an app-host rerun of the test-only commit needs a main commit with CI-built products, and the nearest one is still building. On a slow host, releases: true could reach the guard after the release and pass without retrying; releases: false always exercises the bounded retry.
  • App-host: CodexWriterAdmissionRecoveryTests/coordinationContentionResolvesWithinOneWaitingRequest holds coordination, releases it after 0.3 s, and expects the waiting admission to return in under 4 s (previously 8 s) and the next one to admit. Added after the fix, so it has no separate red run.
  • CodexWriterLockTests covers the settle policy: it retries only .changing, stops when contention ends, spends exactly its budget when contention never ends, never retries .active or .unavailable, and wakes on a lock-file change. It runs in the package test lane. The HQ build host for nx-remote was unreachable from this machine, so these ran only in CI.
  • python3 scripts/verify-local.py: 7/7 selected checks passed (syntax, xcstrings, localization parity, defaultValue parity, test wiring, package groups, feature flags).
  • Localization: two new keys, translated for all nine macOS locales with scripts/localize-changes. The now-unused agentRestore.admission.unavailable key was removed.
  • Review: a correctness-first subagent review found no blocking defects. Its one load concern (a short app bound turning a stuck lock into ~4 RPC/s per pane) is fixed in 1c8fc56.
  • Tagged-build relaunch check: pending.

Changelog

Fixed: Relaunching cmux with many Codex tabs no longer leaves some of them unrestored with "could not verify whether this agent session is already running"

Proof

CLI/runtime behavior; evidence is the CLI regression above plus the tagged-build relaunch check.

🤖 Generated with Claude Code

austinywang and others added 2 commits October 14, 2026 00:58
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
During a relaunch dozens of Codex threads resume at once, each briefly
holding thread-writer-locks/.coordination.lock. The CLI's final exec guard
treated that `.changing` probe like an unreadable lock and refused an
unowned conversation. It now re-probes `.changing` for a bounded ~3.5s,
woken early by lock-namespace file events, while `.active` and
`.unavailable` still refuse at once with case-specific messages.

The app admission path already kept `.changing` recoverable, but waited up
to 8s per round for a file event a flock release never raises; it now
re-probes after 250ms.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@cursor

cursor Bot commented Oct 14, 2026

Copy link
Copy Markdown

Bugbot needs on-demand usage enabled

Bugbot uses usage-based billing for this team and requires on-demand usage to be enabled.

A team admin can enable on-demand usage in the Cursor dashboard.

@github-actions

Copy link
Copy Markdown
Contributor

All contributors have signed the CLA ✍️ ✅
Posted by the CLA Assistant Lite bot.

@coderabbitai

coderabbitai Bot commented Oct 14, 2026 •

Copy link
Copy Markdown

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 3 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used all 10 included reviews currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration
  • Configuration used: Repository: manaflow-ai/cmux/.coderabbit.yaml
  • Review profile: ASSERTIVE
  • Plan: Advanced
  • Run ID: 53425785-84b4-4997-980b-9bd11a7ca599
📥 Commits

Reviewing files that changed from the base of the PR and between 28e8640 and d84ab65.

📒 Files selected for processing: 9
  • CLI/CMUXCLI+CodexWriterFinalGuard.swift
  • Packages/macOS/CMUXAgentLaunch/Sources/CMUXAgentLaunch/CodexWriterLockSettle.swift
  • Packages/macOS/CMUXAgentLaunch/Tests/CMUXAgentLaunchTests/CodexWriterLockTests.swift
  • Resources/Localizable.xcstrings
  • Sources/AgentRestoreEvidenceObservation.swift
  • Sources/TerminalController+AgentRestoreAdmission.swift
  • cmuxTests/CLICodexWriterLockRestoreTests.swift
  • cmuxTests/CMUXCLIErrorOutputRegressionTests.swift
  • cmuxTests/CodexWriterAdmissionRecoveryTests.swift
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

austinywang and others added 2 commits October 14, 2026 01:16
Review found that a 250ms evidence bound turns a coordination lock that
never settles into ~4 admission RPCs per second per pane. Keep the 8s
request bound and instead re-probe `.changing` on the settle bounds within
the same request, with an app-host regression for the waiting request.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Merge-main commit by scripts/merge-main.sh.
Merged by scripts/merge-main.sh: origin/main at 28e8640, the newest commit with green CI fast guards (1 newer skipped).

Resolved conflicts:
- Resources/Localizable.xcstrings: xcstrings key-level union

Merge-main-previous-head: 1c8fc56
Merge-main-base: 28e8640
@austinywang

Copy link
Copy Markdown
Contributor Author

Correctness-first review (subagent), findings and resolution

  1. Low/medium. With a 250 ms app-side evidence bound, a coordination lock that never settles drove about 4 agent.restore.admit RPCs per second per restoring pane, each with a main-actor hop. Fixed in 1c8fc56: the request keeps its 8 s bound and re-probes .changing inside that same request on the settle bounds. A stuck lock adds no extra RPC rounds.
  2. Low. The open(O_EVTONLY) fallback in awaitNamespaceChange is a bounded timed wait. Resolved: commented in place. It is CLI-only and effectively unreachable, because .changing implies the lock directory exists.
  3. Test caveat. On a very slow host, coordinationContentionIsTransient(releases: true) could reach the guard after the 0.4 s release and pass without retrying. Accepted: it can't go falsely red, and releases: false always exercises the bounded path and the new message.
  4. Policy note. The re-probe bounds are timed waits because a flock release raises no event. Resolved: stated in the PR description.

Checked and found fine: Swift 6 compile concerns (default-argument function reference, method references, Sendable captures), no fd leaks, and .active / .unavailable still refusing at once. No path admits while the last probe is .active, .changing or .unavailable. Total settle is about 3.6 s, under the 60 s claim TTL. The removed localization key has no remaining users.

@austinywang
austinywang merged commit b6981ca into main Oct 14, 2026
27 checks passed
@austinywang
austinywang deleted the fix-codex-restore-coordination-retry branch October 14, 2026 08:18
@github-actions

Copy link
Copy Markdown
Contributor

Merge receipt for d84ab65d85, merged 2026-10-14 08:18:29 UTC

  • Not verified at merge: ci-status (not reported), detect-ios-changes (in progress)
  • Verified: backend migrations applied, catalog-structure, plan, runner, web-validation
  • Skipped by policy: apply-production, apply-staging, web-build, web-database-tests, web-tests
  • Full suite: runs on main after merge.

Labeled merged-unverified: if main breaks near this merge, look here first.

@github-actions github-actions Bot added the merged-unverified A judging check was not green at merge; see the merge receipt comment label Oct 14, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Probably yours: CLIVMLayoutEnvTests.swift:965 fails here but not on main's latest full suite. Seen on other PRs too (likely flaky): CLINotifyProcessIntegrationRegressionTests.swift. This PR merged before its CI finished: fix forward on main.

CI failed on d84ab65d85 (run 38534039260 attempt 1): 1 code.

Job Verdict Why
macos / app-host unit tests (changed suites) code a test failed

Failures:

  • new in this PR CLIVMLayoutEnvTests.swift:965 CLINotifyProcessIntegrationRegressionTests.testVMRestartSendsTheProviderRecoveryMethod: XCTAssertTrue failed - frozen-vm restarted (status=running) (job)
  • seen on other PRs CLINotifyProcessIntegrationRegressionTests.swift CLINotifyProcessIntegrationRegressionTests.testVMRestartSendsTheProviderRecoveryMethod(): failed (job)
Matched log lines
macos / app-host unit tests (changed suites): /tmp/cmux-ci/src/cmuxTests/CLIVMLayoutEnvTests.swift:965: error: -[cmuxTests.CLINotifyProcessIntegrationRegressionTests testVMRestartSendsTheProviderRecoveryMethod] : XCTAssertTrue failed - frozen-vm restarted (status=running)

Not re-run automatically: macos / app-host unit tests (changed suites) is not a machine failure.

Written by scripts/ci/classify_failures.py (ci-failure-attribution.yml); signatures are its SIGNATURES table. A machine verdict is the runner's fault, not this PR's; yours means the failing file is one this PR changes, also red on main that main's latest full suite fails the same way, seen on other PRs that it failed on another pull request's run lately.

@lawrencecchen

Copy link
Copy Markdown
Contributor

main -> feat-cmux-next sync (2026-10-14, bead cx-q1lnl): this PR added 1 file(s) under the legacy macOS app / legacy Swift packages, which feat-cmux-next (the cmux-next rewrite) has deleted. The sync removes them on feat-cmux-next only (main is unchanged):

  • Packages/macOS/CMUXAgentLaunch/Sources/CMUXAgentLaunch/CodexWriterLockSettle.swift

No action is needed on main.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merged-unverified A judging check was not green at merge; see the merge receipt comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants