Prevent duplicate Codex resume launch - #228
Conversation
|
Follow-up from the independent Codex review: this PR now also closes the confirmed P1/P2 findings. Codex daemon readiness requires the actual Codex command in the zmx task row; the app waits for that daemon-owned session before attaching, preventing a bare attach shell from blocking launch. Remote status now reads zmx ls task state rather than stale ZMX_TASK_COMPLETED history. Focused AttachedSessionBriefingTests, ZmxSessionLauncherTests, and RemoteLoopSurvivalTests pass. |
The status probe matched the session name with grep -F over a pattern holding a literal backslash-t, which matches no zmx ls row — every remote probe reported an existing session absent. The tab is a real one now (grep -F never interprets escapes). The app's waiting attach polled zmx ls at 10 Hz forever when the daemon never created the session; it now gives up after a minute with a message. Also formats the new daemon-check code to the restored swift-format gate.
scgopi
left a comment
scgopi
left a comment
There was a problem hiding this comment.
Review of the diff, three findings — all fixed and pushed as db669f3 (focused suites: 63 tests pass; swift format + swiftlint clean).
P1 — remote status probe can never match a session. remoteStatusInvocation built the grep -F pattern from a Swift \\t, i.e. the two characters backslash-t. grep -F never interprets escapes, and zmx ls rows are real-tab-separated (name=…\tpid=…\tcmd=…), so gc_row was always empty and every remote probe reported an existing session absent — remote loops would all read as dead, sends would stage instead of delivering. The pattern now carries a real tab; pinned by assertions in RemoteLoopSurvivalTests (present real tab, absent literal backslash-t).
P2 — the waiting attach polls forever. waitingAttachCommand's until loop had no cap: if the daemon never creates the session (failed launch, loop deleted mid-wait), the pane spawns two zmx ls pipelines ten times a second indefinitely. Now capped at 60s, then prints a message and exits 1; the success path is unchanged (exec attach once cmd=.*codex appears). Pinned in appWaitsForTheDaemonBeforeAttachingCodex.
P3 — PR fails the restored swift-format gate. make check failed on the branch: three [AddLines] errors in the new daemonReadyCheckCommand/waitingAttachCommand bodies (swift format format --in-place applied; diff confined to those lines).
Verified not findings: the sed \\t in the probe works on both BSD and GNU sed (checked on macOS); cmd=.*codex is correct because the daemon launches via zmx run <name> -d codex …, so daemon-owned rows carry the executable, while a bare attach husk correctly fails the check; reordering defersCodexLaunchToDaemon ahead of localResumeOrFreshCommand is the intended fix for the duplicate-resume race.
Every Codex pane in the product failed to attach. It polled `zmx ls` at 10 Hz for sixty seconds, printed "never became ready to attach", exited, and left the human at a bare login shell — while the codex agent underneath ran fine and answered its goal headless with clients=0. The gate #228 added waits for proof that the daemon's session really is the Codex one, by grepping the session's `cmd=` field for the agent's name. That proof does not exist for a loop: zmx records a command only for `attach` (main.zig:217) and hardcodes `.command = null` for `run` (:259), and `zmx ls` prints the field only when it is non-nil (util.zig:911). Graphcode launches every loop with `zmx run -d`. Measured on a live machine: of 56 sessions, 7 carried a `cmd=` at all — the seven created by `attach` — and 6 of those were truncated to `/bin/zsh -i -l -c...` at zmx's 256-byte cap, which is a second reason the same grep could never match. So the gate was unsatisfiable, and it is `aliveCheckCommand` too: for a Codex node it was permanently false at the pane's attach, at the local atomic ensure (which therefore always took the `zmx run` branch — the very race the ensure exists to prevent), at the remote ensure, and at the remote send gate, where a message edge to a Codex loop silently did nothing. The gate now waits on `agent=<backend>`, a label graphcode writes itself on the channel `presence=` and `usage=` already travel on. The daemon writes it in the run branch of the ensure, after the launch it describes and only if that launch was made. Not from inside the launch script: what `zmx run` carries is typed into the session's tty, a canonical-mode tty drops everything past MAX_CANON (issue #57), and a stamp in there tipped theLaunchStillFitsInATypedCommandLine red on the first attempt. Out here it costs the typed line nothing. The regression test runs a real `zmx run -d` session and greps its real `zmx ls` output, through a PTY because zmx wants a terminal to create a session against. That shape is deliberate: the tests that existed asserted only that the pattern was *built*, and a synthetic attach-shaped `ls` line would have passed while the product stayed broken. One of its assertions is the old pattern failing against a real session — the bug itself, pinned. Fixes #272. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KMDUe3kmSwvx4Bth3xM6qF
1 participant
Summary
This is a follow-up to merged PR #219. Claude launch behavior is unchanged.
Verification
make test