Skip to content

fix(bin): keep remote secondmate replies flowing and add guarded Codex lock handoff - #6396

Open
maruthiprithivi wants to merge 12 commits into
kunchenguid:mainfrom
maruthiprithivi:fm/fm-coord-resilience-r2
Open

maruthiprithivi wants to merge 12 commits into
kunchenguid:mainfrom
maruthiprithivi:fm/fm-coord-resilience-r2

Conversation

@maruthiprithivi

@maruthiprithivi maruthiprithivi commented Oct 2, 2026 •

Copy link
Copy Markdown

Intent

There has always been a secondmate connection issue. Analyze, debug and find the root cause, then make whatever changes firstmate's code or helper functions need, so that firstmate and secondmate coordination works seamlessly and is very resilient. Restarts are acceptable.

Observed incident context (2026-10-02): a primary firstmate session in Codex Desktop held the session lock, then went idle around 18:16 local. A Claude primary later took over the lock. From 18:17 until about 19:12, nothing the remote optimus secondmate appended reached the primary: its state/parent-replies.status on the remote host grew to 955639 bytes, but the primary's remote-reply cursor stayed at offset 947006. The secondmate looked silent even though it had received and handled every primary steer and was actively working. Running fm-procevent.sh ensure-listening remote-reply-optimus immediately ingested the backlog.

What Changed

  • Remote reply listener recovery: fm-procevent-remote-reply.sh now keeps its process-event owner when a remote read fails and retries up to three times before queueing one failure wake per episode. A new lag-check compares the remote log size with the committed local cursor. If the remote log stays ahead past the threshold, it runs fm-procevent.sh ensure-listening once for that episode and queues one "remote reply channel stalled" wake. Cursor progress clears the episode and its old lag receipts. fm-watch.sh runs these probes in the background on a bounded schedule, so they never block the watcher's main loop.
  • Safe recovery when the remote log gets shorter: fm-remote-delta-read.sh adds two modes, size and verify-rebase. The new rebase command in the reply adapter needs an expected cursor and checks that the bytes left on the remote match bytes already ingested. Only then does it move the cursor back to a complete-line boundary and re-arm the listener. If the bytes changed or there is no ingestion evidence, it stays blocked.
  • Codex session-lock ownership: fm-session-lock-lib.sh and fm-lock.sh now record a trusted Codex thread id in state/.lock-session. They refuse to take the lock through the shared managed Codex daemon unless that thread id is present. A new fm-lock.sh take-over --expect-pid --expect-session --attest-owner-ended replaces a lock held by an idle Codex daemon, but only if the expected pid and session match, the watcher is stopped, and both the lock and the watcher beacon have been quiet for FM_LOCK_TAKEOVER_QUIET_SECONDS. It saves a copy of the old lock and sidecar to state/.lock-takeover.* first. The related docs, the skill doc and the tests are updated to match.

🤖 Generated with Claude Code

Risk Assessment

🚨 High: The change reworks primary session-lock ownership for shared Codex daemons, and an unverified CODEX_THREAD_ID assumption can leave a home locked out with no owner. It also adds a large manual rebase subsystem the intent does not require. Both need explicit human approval.

Testing

  • ⏭️ Test - skipped

Pipeline

Updates from git push no-mistakes

✅ **intent** - passed

✅ No issues found.

⏭️ **Rebase** - skipped

Step was skipped.

⚠️ **Review** - 2 warnings
  • ⚠️ bin/fm-lock.sh:135 - Codex Desktop thread without a usable CODEX_THREAD_ID locks itself out. If CODEX_THREAD_ID is unset, or fails the [A-Za-z0-9_-] / 128-character check in fm_session_lock_trusted_session_id (bin/fm-session-lock-lib.sh:239-240), fm_session_lock_anchor_pid (bin/fm-session-lock-lib.sh:290) returns the outermost ancestor. That ancestor is the shared codex app-server --managed-daemon. On a free lock, fm-lock.sh records that pid and removes the sidecar (publish_lock_session, no trusted id). From then on fm_session_lock_owned_by_self (bin/fm-session-lock-lib.sh:312-316) needs a matching sidecar for a shared daemon pid, so it is false for every caller, including the acquiring thread. The next fm-lock.sh run hits refuse_live_owner (bin/fm-lock.sh:362). fm_session_lock_foreign_owner_live reports the lock as foreign, so the turn-end guard and auto-arm treat the home as owned by someone else. take-over cannot recover from that thread either, because it requires a trusted id (bin/fm-lock.sh:136-141). Before this change the ancestry rule made that thread the owner. Nothing in source shows that Codex always exports CODEX_THREAD_ID, and no test covers acquisition without it. Smallest fix: in fm-lock.sh, refuse to acquire (fail closed with a clear message) when the anchor is fm_session_lock_shared_codex_pid and there is no trusted id. That is a product decision on whether such a Codex Desktop thread may be primary at all.
  • ⚠️ bin/fm-procevent-remote-reply.sh:449 - Simplification: the new rebase subcommand (cmd_rebase plus rebase_find_evidence, bin/fm-procevent-remote-reply.sh:406-604) and the matching verify-rebase mode in bin/fm-remote-delta-read.sh:150-200 add about 200 lines of manual recovery for a truncated (shortened) remote log. No intent requirement needs it. In the incident the log grew (remote 955639 bytes, cursor 947006), so there was no truncation or continuity break, and the incident was fixed by re-ensuring the listener. A shortened log already stops the relay and surfaces a continuity failure. The same applies to the extra cursor/remote_bytes/generation fields added to the continuity blocked line (line 831), which mainly serve rebase. Recommend removing the rebase/verify-rebase component (and its docs in docs/remote-secondmates.md) from this change, or splitting it into its own scoped change.
  • ⚠️ bin/fm-lock.sh:126 - Simplification: the required --attest-owner-ended PID/SESSION argument must equal exactly $EXPECT_PID/$EXPECT_SESSION, which --expect-pid and --expect-session already supply, so it adds no information or check. The approved recovery (review-2 decision) named take-over --expect-session none. Narrower form: drop --attest-owner-ended and keep the documented operator verification step. Its copies are in the usage strings (bin/fm-lock.sh:115,131), the status recovery hints (bin/fm-lock.sh:78-79,86-87), the header (lines 28-45), docs/watcher-continuity.md:213-215 and docs/sessionstart-nudge.md:105.

🔧 Fix applied.
2 warnings still open:

  • ⚠️ bin/fm-procevent-remote-reply.sh:449 - Simplification: the new rebase subcommand (cmd_rebase plus rebase_find_evidence, bin/fm-procevent-remote-reply.sh:406-604) and the matching verify-rebase mode in bin/fm-remote-delta-read.sh:150-200 add about 200 lines of manual recovery for a truncated (shortened) remote log. No intent requirement needs it. In the incident the log grew (remote 955639 bytes, cursor 947006), so there was no truncation or continuity break, and the incident was fixed by re-ensuring the listener. A shortened log already stops the relay and surfaces a continuity failure. The same applies to the extra cursor/remote_bytes/generation fields added to the continuity blocked line (line 831), which mainly serve rebase. Recommend removing the rebase/verify-rebase component (and its docs in docs/remote-secondmates.md) from this change, or splitting it into its own scoped change.
  • ⚠️ bin/fm-lock.sh:126 - Simplification: the required --attest-owner-ended PID/SESSION argument must equal exactly $EXPECT_PID/$EXPECT_SESSION, which --expect-pid and --expect-session already supply, so it adds no information or check. The approved recovery (review-2 decision) named take-over --expect-session none. Narrower form: drop --attest-owner-ended and keep the documented operator verification step. Its copies are in the usage strings (bin/fm-lock.sh:115,131), the status recovery hints (bin/fm-lock.sh:78-79,86-87), the header (lines 28-45), docs/watcher-continuity.md:213-215 and docs/sessionstart-nudge.md:105.
⏭️ **Test** - skipped

Step was skipped.

✅ **Document** - passed

✅ No issues found.

⏭️ **Lint** - skipped

Step was skipped.

✅ **Push** - passed

✅ No issues found.

Related issues and review choices

@greptile-apps

greptile-apps Bot commented Oct 2, 2026 •

Copy link
Copy Markdown

RetriggerConfidence Score: 5/5

[High risk] Modifies session lock ownership logic and remote reply handling.

The PR appears safe to merge based on this review; no outstanding finding or new actionable issue was identified.

Reviews (7) · Last reviewed commit: "no-mistakes(document): Document Codex da..."

Comment thread bin/fm-lock.sh
Comment thread bin/fm-watch.sh Outdated
Comment thread bin/fm-procevent-remote-reply.sh Outdated
Comment thread bin/fm-procevent-remote-reply.sh Outdated
Comment thread bin/fm-procevent-remote-reply.sh Outdated
@maruthiprithivi

Copy link
Copy Markdown
Author

Hi @kunchenguid - a small request: the CI and Require no-mistakes workflows on our open fork PRs are held in action_required (first-time contributor approval), so none of them has run yet. When you have a moment, could you approve the workflow runs for these PRs?

We are addressing the open Greptile findings on #6396 and #6237 in parallel. Thanks.

@maruthiprithivi maruthiprithivi changed the title fix: recover remote secondmate replies and guard primary handoff fix(bin): keep remote secondmate replies flowing and add guarded Codex lock handoff Oct 3, 2026
@maruthiprithivi

Copy link
Copy Markdown
Author

Regarding the no-mistakes review suggestion to remove rebase and verify-rebase: retaining them is deliberate. A separate production incident on 2026-10-02 shortened the remote reply log by five bytes after ingestion, leaving the committed cursor past EOF. Issue 6415 records that failure mode. The guarded recovery verifies retained bytes against already ingested content before moving the cursor and re-arming the listener; changed bytes or missing evidence still block recovery. This replaces a manual cursor edit for a confirmed incident.

@maruthiprithivi

Copy link
Copy Markdown
Author

Regarding the no-mistakes review suggestion to remove --attest-owner-ended: retaining it is deliberate. Commit ad3686c added explicit operator attestation after review identified that a quiet lock and stopped watcher do not prove the original Codex thread ended. The expected PID and session guard against replacing a different lock; the separate attestation forces the operator to affirm that the named owner has ended before takeover. The documentation states that this is an operator assertion, not machine proof.

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.

1 participant