Repository navigation
Retry Codex resume lock failures and preserve cwd - #5611
Closed
austinywang wants to merge 18 commits into
Closed
austinywang wants to merge 18 commits into
austinywang wants to merge 18 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThreads AgentResumeRetryPolicy through resume startup paths, adds zsh launcher-script generation that preserves the session working directory and optionally retries transient failures (Codex DB locks), introduces shell-quoting and script-builder utilities, and adds unit plus end-to-end tests. ChangesAgent resume retry mechanism and working directory restoration
Sequence Diagram(s)sequenceDiagram
participant Snapshot as RestorableAgentSnapshot
participant ScriptStore as AgentResumeScriptStore
participant Builder as AgentResumeShellScriptBuilder
participant Policy as AgentResumeRetryPolicy
Snapshot->>Policy: policy(kind, launcher)
Snapshot->>ScriptStore: writeLauncherScript(..., retryPolicy)
ScriptStore->>Builder: commandThenReturnLines(command, workingDirectory, retryPolicy)
alt retryPolicy.isEnabled
Builder->>Builder: emit retry loop with /usr/bin/script capture
Builder->>Builder: grep log against policy needles
Builder->>Builder: break on success or mismatch
else
Builder->>Builder: emit plain child-shell invoke
end
Builder->>Builder: cd to workingDirectory
Builder->>Builder: exec -l into resume shell
Builder-->>ScriptStore: launcher script lines
ScriptStore-->>Snapshot: launcher script written
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (2 errors, 1 warning)
✅ Passed checks (18 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Sources/SessionPersistence.swift (1)
397-421:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFall back to the inline command if the retry wrapper can't be written.
retryPolicy.isEnablednow routes even short Codex bindings throughSurfaceResumeBindingScriptStore, but this path returnsnilwhen the temp script write fails. That regresses from "resume without retries/cwd restoration" to "can't resume at all" even thoughinlineInputalready fits undermaxInlineStartupInputBytes.Suggested fix
guard let scriptURL = SurfaceResumeBindingScriptStore.writeLauncherScript( inlineInput: inlineInput, binding: self, fileManager: fileManager, temporaryDirectory: temporaryDirectory, returnToLoginShell: retryPolicy.isEnabled, retryPolicy: retryPolicy ) else { - return nil + return inlineInput.utf8.count <= Self.maxInlineStartupInputBytes ? inlineInput : nil }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Sources/SessionPersistence.swift` around lines 397 - 421, In startupInputWithLauncherScript, when SurfaceResumeBindingScriptStore.writeLauncherScript(...) returns nil you must fall back to inlineInput if it fits under Self.maxInlineStartupInputBytes (so short inputs still resume), otherwise return nil; update the guard/else around scriptURL in startupInputWithLauncherScript to return inlineInput when inlineInput.utf8.count <= Self.maxInlineStartupInputBytes and return nil only when the inline input is too large.Sources/RestorableAgentSession.swift (1)
761-797:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve the inline fallback contract when the retry wrapper is unavailable.
With
retryPolicy.isEnabled, this helper now bypasses both fallback guards: disabling launcher scripts returns oversized inline input, and a temp-script write failure returnsnileven when the original command still fits inline. That makes the new retry path more fragile than the pre-retry behavior.Suggested fix
guard retryPolicy.isEnabled || !allowOversizedInlineInput else { return inlineInput } guard allowLauncherScript else { - return retryPolicy.isEnabled ? inlineInput : nil + if inlineInput.utf8.count <= Self.maxInlineStartupInputBytes || allowOversizedInlineInput { + return inlineInput + } + return nil } guard let scriptURL = AgentResumeScriptStore.writeLauncherScript( command: command, kind: kind, sessionId: sessionId, @@ workingDirectory: registration?.cwd == .ignore ? nil : (workingDirectory ?? launchCommand?.workingDirectory) ) else { - return nil + if inlineInput.utf8.count <= Self.maxInlineStartupInputBytes || allowOversizedInlineInput { + return inlineInput + } + return nil }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Sources/RestorableAgentSession.swift` around lines 761 - 797, The startupInput logic currently lets retryPolicy.isEnabled bypass the inline-size and launcher-script guards, causing oversized or missing-script fallbacks to behave incorrectly; update startupInput so that retryPolicy only enables use of a launcher wrapper but does not disable the original inline fallback contract: keep the inlineInput size check (inlineInput.utf8.count <= Self.maxInlineStartupInputBytes) as the default condition for returning inlineInput, only allow creating/using a launcher script when inlineInput is too large and allowLauncherScript is true, and if AgentResumeScriptStore.writeLauncherScript returns nil fallback to returning inlineInput only when inlineInput.utf8.count <= Self.maxInlineStartupInputBytes (otherwise return nil); reference startupInput, Self.maxInlineStartupInputBytes, retryPolicy.isEnabled, allowLauncherScript, inlineInput, and AgentResumeScriptStore.writeLauncherScript when making the changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@Packages/CMUXAgentLaunch/Sources/CMUXAgentLaunch/AgentResumeRetryPolicy.swift`:
- Around line 65-68: The doc comment for the public function matches(output:) is
missing a "- Returns:" DocC callout; update the comment block above public func
matches(output: String) -> Bool to include a "- Returns:" line that clearly
states what the Bool represents (e.g., true when the combined stdout/stderr
contains a retryable signature, false otherwise), keeping the existing "-
Parameter output:" and overall formatting consistent with package public API
guidelines.
- Around line 24-27: The initializer public init(maximumRetries: Int,
delaySeconds: Double, outputNeedles: [String]) currently assigns outputNeedles
directly which allows empty or whitespace-only strings (e.g. [""]) so
matches(output:) will always succeed; update the init to normalize outputNeedles
by trimming whitespace from each string and filtering out any resulting empty
strings (e.g. outputNeedles.map { $0.trimmingCharacters(...) }.filter {
!$0.isEmpty }) before assigning to self.outputNeedles so isEnabled and
matches(output:) behave correctly.
In
`@Packages/CMUXAgentLaunch/Sources/CMUXAgentLaunch/AgentResumeShellQuoting.swift`:
- Around line 4-16: Add targeted unit tests for singleQuoted(_:) and
asciiPrintfCommandSubstitution(for:) that assert exact output for both branches:
(1) ASCII-only input including embedded single quotes and newlines (verify the
returned string equals "'" + value.replacingOccurrences(of: "'", with: "'\\''")
+ "'"), and (2) non-ASCII input (e.g., emoji, accented characters, multi-byte
UTF‑8) which must exercise asciiPrintfCommandSubstitution(for:) and assert the
returned string equals the literal "$(printf '<octal-bytes>')" form produced by
mapping value.utf8 to \%03o octal sequences; include explicit expected strings
in assertions so any regression in quoting or octal encoding fails the tests.
---
Outside diff comments:
In `@Sources/RestorableAgentSession.swift`:
- Around line 761-797: The startupInput logic currently lets
retryPolicy.isEnabled bypass the inline-size and launcher-script guards, causing
oversized or missing-script fallbacks to behave incorrectly; update startupInput
so that retryPolicy only enables use of a launcher wrapper but does not disable
the original inline fallback contract: keep the inlineInput size check
(inlineInput.utf8.count <= Self.maxInlineStartupInputBytes) as the default
condition for returning inlineInput, only allow creating/using a launcher script
when inlineInput is too large and allowLauncherScript is true, and if
AgentResumeScriptStore.writeLauncherScript returns nil fallback to returning
inlineInput only when inlineInput.utf8.count <= Self.maxInlineStartupInputBytes
(otherwise return nil); reference startupInput, Self.maxInlineStartupInputBytes,
retryPolicy.isEnabled, allowLauncherScript, inlineInput, and
AgentResumeScriptStore.writeLauncherScript when making the changes.
In `@Sources/SessionPersistence.swift`:
- Around line 397-421: In startupInputWithLauncherScript, when
SurfaceResumeBindingScriptStore.writeLauncherScript(...) returns nil you must
fall back to inlineInput if it fits under Self.maxInlineStartupInputBytes (so
short inputs still resume), otherwise return nil; update the guard/else around
scriptURL in startupInputWithLauncherScript to return inlineInput when
inlineInput.utf8.count <= Self.maxInlineStartupInputBytes and return nil only
when the inline input is too large.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2ca085c7-50b3-4623-8c1b-aba2fcb2a519
📒 Files selected for processing (9)
CLI/cmux.swiftPackages/CMUXAgentLaunch/Sources/CMUXAgentLaunch/AgentResumeRetryPolicy.swiftPackages/CMUXAgentLaunch/Sources/CMUXAgentLaunch/AgentResumeShellQuoting.swiftPackages/CMUXAgentLaunch/Sources/CMUXAgentLaunch/AgentResumeShellScriptBuilder.swiftPackages/CMUXAgentLaunch/Tests/CMUXAgentLaunchTests/AgentResumeRetryPolicyTests.swiftPackages/CMUXAgentLaunch/Tests/CMUXAgentLaunchTests/AgentResumeShellScriptBuilderTests.swiftSources/RestorableAgentSession.swiftSources/SessionPersistence.swiftcmuxTests/SessionPersistenceTests.swift
Contributor
Greptile SummaryThis PR adds a shared
Confidence Score: 5/5Safe to merge — no defects found in the changed paths. The retry policy is narrowly scoped to Codex lock signatures, the startup-window and pattern-match guards prevent runaway retries on slow or non-matching failures, and the FIFO/dd/grep pipeline for output capture correctly handles partial data and FIFO unavailability. The fallback from script to inline on blocked temp dirs is an improvement over the previous nil return. EXIT/INT/TERM traps ensure log cleanup on shutdown signals (addressing the previous review thread). The Swift layer is nonisolated value types with no actor isolation concerns, and end-to-end tests in both the package and app target cover the key retry, bound-exhaustion, and cwd-preservation scenarios. No files require special attention. Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant App as cmux App / CLI
participant Builder as AgentResumeShellScriptBuilder
participant Script as Generated zsh launcher
participant FIFO as FIFO + dd capture
participant Codex as Codex process
participant Shell as Login shell
App->>Builder: commandThenReturnLines(command, workingDirectory, retryPolicy)
Builder-->>App: zsh script lines
App->>Script: Write and exec launcher
loop Retry loop up to 3 times
Script->>FIFO: mkfifo + dd capture 4096B in background
Script->>Codex: script -F FIFO then shell -lic command
Codex-->>Script: exit 1 with lock error on stderr
FIFO-->>Script: captured log written
Script->>Script: grep log for lock signature
alt lock pattern matched AND elapsed less than 5s AND retry less than limit
Script->>Script: cleanup log then sleep backoff then increment retry
else no match OR slow failure OR limit reached
Script->>Script: break
end
end
Script->>Script: cleanup log then clear traps
Script->>Shell: cd workingDirectory then exec login shell
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant App as cmux App / CLI
participant Builder as AgentResumeShellScriptBuilder
participant Script as Generated zsh launcher
participant FIFO as FIFO + dd capture
participant Codex as Codex process
participant Shell as Login shell
App->>Builder: commandThenReturnLines(command, workingDirectory, retryPolicy)
Builder-->>App: zsh script lines
App->>Script: Write and exec launcher
loop Retry loop up to 3 times
Script->>FIFO: mkfifo + dd capture 4096B in background
Script->>Codex: script -F FIFO then shell -lic command
Codex-->>Script: exit 1 with lock error on stderr
FIFO-->>Script: captured log written
Script->>Script: grep log for lock signature
alt lock pattern matched AND elapsed less than 5s AND retry less than limit
Script->>Script: cleanup log then sleep backoff then increment retry
else no match OR slow failure OR limit reached
Script->>Script: break
end
end
Script->>Script: cleanup log then clear traps
Script->>Shell: cd workingDirectory then exec login shell
Reviews (11): Last reviewed commit: "merge: resolve conflicts with main" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
Author
|
Final feedback note: Greptile's latest summary says safe to merge and flags only the retry sleep as a discussion point. That is intentional here: the sleep is shell-side, bounded by AgentResumeRetryPolicy, scoped only to the Codex lock signature, staggered, and test-overridable to zero because there is no OS-level notification for another process releasing Codex's SQLite lock. No code change needed beyond the already-pushed bounded retry/fallback coverage. |
…me-lock-retry-cwd # Conflicts: # Sources/SessionPersistence.swift
…me-lock-retry-cwd
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@Packages/CMUXAgentLaunch/Sources/CMUXAgentLaunch/AgentResumeShellScriptBuilder.swift`:
- Around line 113-119: When mkfifo fails in the conditional block starting at
line 115, the _cmux_resume_script_output variable remains set to /dev/null
instead of being updated to capture output for retry matching. This causes the
subsequent /usr/bin/script execution and the grep operation at line 160 to fail
silently since the log file remains empty. Add a fallback mechanism after the fi
statement on line 119 to set _cmux_resume_script_output to the actual
_cmux_resume_log file path when the FIFO setup fails, ensuring that output is
still captured and retry matching logic can function properly even when mkfifo
is unavailable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 95d44856-c516-4848-805e-59dae46a0eaf
📒 Files selected for processing (2)
Packages/CMUXAgentLaunch/Sources/CMUXAgentLaunch/AgentResumeShellScriptBuilder.swiftPackages/CMUXAgentLaunch/Tests/CMUXAgentLaunchTests/AgentResumeShellScriptBuilderTests.swift
Contributor
|
Greptile encountered an error while reviewing this PR. Please reach out to support@greptile.com for assistance. |
lawrencecchen
added
the
stale-revisit
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #5557
Summary
database is locked/another Codex process is using its local data) with a short staggered backoff before surfacing the failureRelated: #5391, #5271, #4256, #4963, #4150
Tests
swift test --package-path Packages/CMUXAgentLaunchNote: app-target unit tests are left to CI per repo instructions. The first commit adds the red regression coverage; the second commit makes it pass.
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Adds bounded, startup-window retries for Codex resumes when SQLite lock signatures appear, and keeps the terminal in the session’s working directory after the agent exits. Consolidates a zsh launcher in
CMUXAgentLaunchused for app restore, surface resume, and thecodex-teamsCLI with safer quoting and login-shell reentry.New Features
CMUX_AGENT_RESUME_RETRY_LIMIT,CMUX_AGENT_RESUME_RETRY_DELAY_SECONDS, andCMUX_AGENT_RESUME_RETRY_STARTUP_SECONDS. Auto-enabled forcodexandcodexTeams.AgentResumeShellScriptBuilder) now powers local restore, surface resume, and thecodex-teamsstartup script, preserving cwd and reentering a login shell. Adds robust shell quoting (including non-ASCII) and switches thecodex-teamsscript to.zsh.Bug Fixes
Written for commit 0c94af5. Summary will update on new commits.
Summary by CodeRabbit