Skip to content

fix(bin): recognize Claude session locks across worker pools - #2839

Open
karotkriss wants to merge 9 commits into
kunchenguid:mainfrom
karotkriss:fm/fm-2314-lock-daemon
Open

fix(bin): recognize Claude session locks across worker pools#2839
karotkriss wants to merge 9 commits into
kunchenguid:mainfrom
karotkriss:fm/fm-2314-lock-daemon

Conversation

@karotkriss

@karotkriss karotkriss commented Aug 23, 2026

Copy link
Copy Markdown
Contributor

Intent

Fix issue #2314: the session-lock self-ownership check fails under Claude Code's daemon-based Bash execution, producing a false "another session" refusal that forces the real primary session into permanent read-only mode (no spawn, steer, merge, wake-drain, or supervision repair).

Mechanism: fm_session_lock_owned_by_self() in bin/fm-session-lock-lib.sh decided "is this lock mine?" by walking the process ancestry of the current Bash tool invocation and requiring the lock file's recorded PID to appear in that chain. Newer Claude Code builds serve tool and hook commands from a persistent per-user worker pool that is reparented to init, so the ancestry terminates at PID 1 inside the pool and never reaches the interactive session PID that SessionStart correctly wrote into state/.lock. The same ancestry assumption reaches bin/fm-lock.sh, where the recorded live session PID is then reported as "another live firstmate session holds the lock (pid )".

Approach and the reasoning behind it, so the diff is not read as accidental:

  • Claude Code exports CLAUDE_PID (the interactive session's PID) into the environment of every tool and hook command it runs, including those served through the reparented pool. It is the identity that survives the PID-1 gap ancestry cannot cross. Claude Code is the only verified harness publishing such a variable today, so every other harness keeps the existing ancestry-only behaviour bit-for-bit unchanged.
  • A new fm_harness_session_pid() helper returns that published PID and validates it twice: the PID must still be a live process, and that process must still be a Claude harness. A value inherited from an exited session whose PID has been recycled onto something else, or onto a different harness, is rejected rather than trusted.
  • The evidence is used ONLY to WIDEN ownership, never to replace the ancestry test. This is a deliberate trust-boundary decision documented in the code: an environment variable is inherited by any child process, so on its own it proves "a Claude session named this PID", never "I am that session". The only conclusion drawn is that a lock ALREADY recording exactly that PID belongs to a live session rather than a competing one. The change therefore can never take a lock away from another session, can never turn an unheld lock into a held one, and can never introduce a NEW refusal - it can only remove a false one.
  • Both shared ownership entry points route through the helper, because fixing only the function named in the issue title would not have fixed the reported reproduction.
  • The residual same-second PID-recycle window raised in review is closed by construction rather than tolerated: fm_session_lock_wait_until_publishable performs a bounded wait (max ~2s, only when the verified Claude signals are present and the lock is being written in the same second the session started) so the initial publication always lands in a later second than the process start. The comparison is then a strict -lt, which rejects equality without refusing any legitimate session. Every early-return path in that helper proceeds with publication rather than blocking, so it cannot wedge lock acquisition, and non-Claude harnesses return before doing any work.

Scope was held deliberately tight to this issue's mechanism: the ancestry walk itself, the harness name tables, fm-lock.sh status output, and lock-stealing/staleness semantics are all untouched.

Change made in THIS round, in response to reviewer feedback on the pull request:

  • The out-of-scope hunk has been removed from this branch. Earlier revisions of this branch also carried a change to .opencode/plugins/fm-primary-watch-arm.js (classifyArmClose returning "external" instead of "failure" for an external healthy watcher, plus the matching settleReadiness branch) and a corresponding assertion refactor in tests/fm-pi-watch-extension.test.sh. Reviewers correctly identified those as unrelated to issue Session-lock self-ownership check fails under Claude Code's daemon-based Bash execution (false 'another session' refusal) #2314's lock-identity mechanism. Both files are now restored to their default-branch content, so this branch is strictly scoped to the lock fix and touches only bin/fm-lock.sh, bin/fm-session-lock-lib.sh, docs/scripts.md, docs/verification/supervision.md, tests/fm-claude-stop-autoarm-live-e2e.test.sh, and tests/fm-session-lock-ancestry.test.sh.
  • The removal was committed forward on top of the existing branch rather than by rewriting history, so every prior pipeline fix commit remains present in the branch.
  • This round must terminate with a pipeline attestation that matches the final commit of this branch. A previous round's attestation named an earlier commit because later CI-fix commits moved the head without re-stamping, which reviewers flagged as a structural block.

Regression tests were written first and confirmed to reproduce the defect before the fix existed, in tests/fm-session-lock-ancestry.test.sh, using its established idiom (a deterministic fake ps process table plus the real script). Each added case was verified to be discriminating - it fails without the corresponding guard: a session served by a reparented worker pool rooted at PID 1 proves it owns the lock it holds; that same pool-served session still refuses a lock held by a different live session, so acceptance is widened and not blanketed; a published session PID is trusted only while it is a live Claude session; a just-started session publishes after the ambiguous start second; and an end-to-end case runs the real bin/fm-lock.sh against a real live process with only ps shadowed, asserting the exact reported refusal no longer happens. The pre-existing end-to-end cases that run the real Stop auto-arm and real fm-lock.sh in real process trees continue to pass unchanged, which confirms the widening introduced no regression.

Documentation was kept accurate in the same pass: docs/verification/supervision.md previously stated that session-lock ownership is decided against the contiguous harness ancestry, which is no longer the whole contract, so it now records the worker-pool gap and the widen-only nature of the new evidence. The harness-adapters skill file was deliberately NOT touched, to stay disjoint from a separate open pull request that edits it.

Coupling with other open pull requests, all deliberately kept disjoint: two touch this identity code but are Windows/MSYS-specific; one addresses a related reparented-pool symptom through a different mechanism (a state sidecar file plus publish/verify helpers); another touches bin/fm-claude-stop-autoarm.sh and the harness-adapters skill. This change shares no new state artifact or helper name with any of them. Mechanism selection between this change and the sidecar approach is explicitly the maintainers' call and is being raised with them on the pull request thread.

This pull request must close ONLY issue #2314 (Fixes #2314). Any other related issue is cross-linked, never closed.

What Changed

  • Recognize a live Claude session’s published CLAUDE_PID when a reparented worker pool breaks process ancestry, without allowing it to claim another session’s lock.
  • Validate the published PID against the live Claude process generation and delay initial lock publication past the ambiguous start second.
  • Add deterministic and live E2E coverage for worker-pool ownership, competing owners, recycled PIDs, and lock publication timing, with updated supervision documentation.

Fixes #2314

Risk Assessment

🚨 High: A supported stale-lock recovery path can publish the shared Claude daemon PID, allowing another session using that pool to be treated as the lock owner.

Testing

No baseline test commands were supplied. The focused lock suite passed, the credentialed external Claude test remained opt-in and skipped, the real CLI evidence proved self-ownership success plus foreign-owner refusal, and the same regression failed against the base commit as expected.

Evidence: Worker-pool lock CLI transcript

Source: Worker-pool lock CLI transcript

Scenario: tool call rooted in a PID 1 worker pool, outside the interactive session ancestry. Interactive session PID: 96910 Reachable worker-pool harness PID: 300 Owned lock before acquire: 96910 lock acquired: harness pid 96910 Owned lock after acquire: 96910 Foreign live lock before acquire: 96912 error: another live firstmate session holds the lock (pid 96912); operate read-only until resolved Foreign acquire exit: 1 Foreign live lock after acquire: 96912

Scenario: tool call rooted in a PID 1 worker pool, outside the interactive session ancestry.
Interactive session PID: 96910
Reachable worker-pool harness PID: 300

Owned lock before acquire: 96910
lock acquired: harness pid 96910
Owned lock after acquire:  96910

Foreign live lock before acquire: 96912
error: another live firstmate session holds the lock (pid 96912); operate read-only until resolved
Foreign acquire exit: 1
Foreign live lock after acquire:  96912
Evidence: Base commit counterfactual

Source: Base commit counterfactual

The base implementation exits 1 at the worker-pool ownership regression: a session served by a reparented worker pool could not prove it owns its own lock.

ok - session-lock: a version-named Claude Code session is identified from its install path and argv[0]
ok - session-lock: ordinary script paths under a harness directory are not harness processes
ok - session-lock: ownership stops at the first non-harness gap above the contiguous run
ok - session-lock: a live version-named session holding the lock is not mistaken for a stale owner
not ok - a session served by a reparented worker pool could not prove it owns its own lock

Known limitation: one-lifetime upgrade gap

This change proves lock ownership for a pool-served session by requiring the session process to have started strictly before the lock was written. That ordering is guaranteed for every lock this version publishes, because acquisition waits out the ambiguous start second first.

It cannot be guaranteed for a lock that already exists. A session that was already running when this change lands, and whose lock happened to be published during its own process-start second, cannot be proven the owner of that lock. This is not a regression - it is exactly the behaviour that session already had, and the code deliberately declines to widen rather than inventing evidence it does not have. It is also not fixable for such a lock: the version that wrote it recorded no process-generation evidence of any kind, so no later reader can distinguish the original session from a pid recycled within that same second. #2437 shares this boundary by design, where a lock carrying no binding behaves exactly as it did before the binding existed.

The gap lasts at most one session lifetime and self-heals at the next session start, because the next lock this version publishes carries the bounded-wait ordering. Flagging it explicitly so it is accepted knowingly rather than discovered later.

Pre-existing weakness filed separately

Review also surfaced that lock publication takes its identity from an ancestry walk, so a lock acquired through the reparented pool records the shared pool daemon's pid and a sibling session served by that same pool can pass the ownership test. That is present on unmodified main (me=$(fm_harness_ancestry_pid) at bin/fm-lock.sh:36, combined with ancestry-membership ownership), and this branch is inert on that path because its adoption is gated on the lock already recording exactly the validated session pid. It is filed as #2883 rather than fixed here, to keep this change scoped to issue #2314's mechanism.

Pipeline

Updates from git push no-mistakes

✅ **intent** - passed

✅ No issues found.

✅ **Rebase** - passed

✅ No issues found.

⚠️ **Review** - 1 error
  • 🚨 bin/fm-session-lock-lib.sh:203 - Handle pre-existing same-second locks before relying on strict mtime ordering. A running session can have a lock written by the base version during its process-start second. After a live code update, started_epoch == lock_epoch fails this strict comparison, daemon ancestry still lacks the session PID, and both ownership entry points reproduce the permanent read-only failure. The new wait protects only future publications and cannot migrate an existing lock. This contradicts the claimed durable fix and the criterion that the same-second window is "closed by construction". Decide whether to add compatible generation evidence or explicitly authorize this live-upgrade gap.

🔧 Fix: Preserve legacy same-second lock ownership
2 errors still open:

  • 🚨 bin/fm-lock.sh:112 - The compatibility fix contradicts the required "no ... lock storage-format change": chmod u+x turns the mode of state/.lock into a version marker for every harness. It also is not process-generation evidence for legacy locks. If the original session exits, its environment survives in a pool child, its PID is recycled onto another live Claude process during the legacy lock's second, and the old lock is unmarked, lines 215-219 still accept the replacement. Absence of the marker proves only that an older writer created the lock, not that the current process is the original owner. Replace this at the shared identity boundary with evidence that distinguishes the actual process generation without changing the lock contract, or ask the user to reconcile the incompatible constraints.
  • 🚨 bin/fm-session-lock-lib.sh:245 - The bounded wait does not guarantee the claimed strict ordering. If date +%s remains equal to the process-start second for all 40 attempts, such as during a backward clock adjustment, the function returns success anyway. fm-lock.sh then publishes an executable-marked current lock with equal timestamps, and the next ownership check rejects that session permanently at lines 215-217. This introduces the exact new read-only refusal the intent forbids. Publication must not label a lock as current-generation unless the strict boundary was actually crossed; satisfying that while also requiring publication on every early return needs the user's constraints reconciled.

🔧 Fix: Restore strict session-lock generation checks
1 error still open:

  • 🚨 bin/fm-lock.sh:107 - Reconcile pool-served stale or missing-lock acquisition with the required session identity contract. Concrete resume/recovery path: .lock names a dead prior session, CLAUDE_PID=700, and ancestry reaches only the shared pool 320 -> 310 -> 300 -> 1. The adoption check cannot select 700 because the lock records another PID, so me remains shared daemon PID 300; this wait returns immediately because CLAUDE_PID != me, and publication records 300. A different Claude session served by that per-user pool can then pass ancestry ownership for the same home. This violates lock exclusivity and the required published-session ordering. Resolve writer identity at the shared acquisition boundary after proving the existing lock is free or stale, while preserving the rule that CLAUDE_PID alone cannot claim a live foreign lock.
✅ **Test** - passed

✅ No issues found.

  • Inspected f170cedeb735759e9547a5b9de1a26eca7ea6d71..4d2b4ddced1e578c01463a330e09b3c42a2282db against the authoritative lock-identity intent and superseding review decisions.
  • bin/fm-test-run.sh tests/fm-session-lock-ancestry.test.sh
  • bin/fm-test-run.sh tests/fm-claude-stop-autoarm-live-e2e.test.sh (credentialed opt-in test skipped by its documented environment gate)
  • Executed real bin/fm-lock.sh with a PID 1 worker-pool process table, a live matching Claude session PID, and a different live-session negative control.
  • Ran the new worker-pool regression against base commit f170cedeb735759e9547a5b9de1a26eca7ea6d71; it failed at the expected ownership defect.
  • Verified HEAD is target commit 4d2b4ddced1e578c01463a330e09b3c42a2282db and the worktree is clean after evidence collection.
✅ **Document** - passed

✅ No issues found.

⚠️ **Lint** - 1 warning
  • ⚠️ linter found issues (exit code 1)
✅ **Push** - passed

✅ No issues found.

@greptile-apps

greptile-apps Bot commented Aug 23, 2026

Copy link
Copy Markdown

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Reviews (4): Last reviewed commit: "no-mistakes(document): Document publishe..." | Re-trigger Greptile

Comment thread bin/fm-session-lock-lib.sh
Comment thread bin/fm-session-lock-lib.sh Outdated
@karotkriss

Copy link
Copy Markdown
Contributor Author

On the same-second window

I looked at this one carefully and I don't think tightening the comparison is the right change. Reasoning, so it can be checked rather than taken on trust.

The comparison has to be -le, not -lt. A session very often publishes its lock in the same wall-clock second it starts, because the session-start hook acquires the lock right after the process comes up. Under a strict <, that ordinary case would be rejected, ownership would fall back to the ancestry walk, and under the reparented worker pool the ancestry cannot reach the session at all - so this PR's original defect would return, intermittently, depending on whether the process start and the lock write happened to land in the same second. A nondeterministic reappearance of the reported read-only failure is a worse outcome than the window being described.

Closing the residual window needs sub-second process start time, which isn't portably available here. ps -o lstart= is whole-second on both platforms this file has to support, and there is no portable higher-resolution source common to macOS and Linux. Finer-grained lock-file timestamps don't help while the process side stays whole-second.

The residual risk is narrow. Reaching it requires the PID space to wrap all the way around onto the exiting session's PID, for the replacement to be another Claude process, for that process to start inside the same second the lock was written, and for a stale caller to still be carrying the old published value.

The durable fix is a different design. An identity immune to recycling by construction means recording an unrecyclable session identifier alongside the lock instead of inferring process generation from timestamps. That approach is already in flight separately in #2437, and keeping this change disjoint from it was a deliberate constraint here - I'd rather not have two open PRs converge on the same new state artifact. That consolidation seems better left to the maintainers.

Timestamp evidence narrows this from "any live Claude process with the same PID number" to "a same-second replacement", which is the improvement available without changing the lock's storage format. Happy to take it further if a maintainer prefers the stricter tradeoff.

@karotkriss

Copy link
Copy Markdown
Contributor Author

Two follow-ups on the last push, in the interest of not burying either one.

1. The same-second window is now actually closed, and my earlier objection was answered rather than ignored.

I argued against a strict comparison because a session frequently publishes its lock in the same wall-clock second it starts, so < would reject that ordinary case and reintroduce the read-only failure nondeterministically. That objection was valid but it was not the only option: the fix now makes the ambiguous case impossible instead of tolerating it. fm_session_lock_wait_until_publishable performs a bounded wait (max ~2s, and only when the verified Claude signals are present and the lock is being written in the same second the session started) so the single initial publication always lands in a later second than the process start. The strict comparison can then reject equality without any legitimate session being refused.

Worth stating plainly: every early-return path in that helper proceeds with publication rather than blocking, so it cannot wedge lock acquisition, and non-Claude harnesses return before doing any work. I verified locally that the suite passes, including the new case covering a just-started session.

2. .opencode/plugins/fm-primary-watch-arm.js is in this diff, and it is not part of issue #2314's mechanism.

Flagging it explicitly because it looks unrelated in a session-lock PR, and it is. A shard failure on tests/fm-pi-watch-extension.test.sh traced to a real race: a fast arm-child close can beat streamed-output observation, so an external healthy watcher was classified as an owned failure. The change preserves a distinct readiness verdict for that case.

Evidence that this was pre-existing rather than introduced here: the test passes locally on unmodified main and on this branch, the same file failed on a different assertion on an unrelated branch's CI at nearly the same time, and the session-lock code added by this PR cannot execute in CI at all, since it is gated behind a variable that is unset on the runners.

It is a genuine root-cause fix for a flake that is affecting other PRs, but if maintainers would rather keep this PR strictly scoped to the lock-identity mechanism, that hunk is cleanly separable and I am happy to split it into its own change.

@kunchenguid

Copy link
Copy Markdown
Owner

Speaking as Kun's firstmate:

Scheduled 3:10am PT 8/23 pass. Main reconfirmed 8714c9a78c1b4355782fcb9ce1ccf14337478268. VISION.md read in full from that SHA. Issue #2314 was labeled ready-for-pr this morning; that is a queue label, not a merge vote. No captain comment on this thread authorizing a merge.

VISION (inspected bin/fm-session-lock-lib.sh fm_harness_session_pid / fm_session_lock_wait_until_publishable / widen-only fm_session_lock_owned_by_self, bin/fm-lock.sh recorded-pid adoption, tests/fm-session-lock-ancestry.test.sh pool cases, plus the extra .opencode/plugins/fm-primary-watch-arm.js classifyArmClose hunk). Per-rule: honest refusal path aligns (a false "another session" lock refusal forces the real primary read-only; no spawn/steer/merge/wake-drain); restart is a non-event aligns; scripts own the mechanics aligns (identity stays deterministic); vendor-orthogonal mixed — CLAUDE_PID is a Claude-only published signal, other harnesses keep ancestry-only bit-for-bit, which is the right fail-closed split; scope does not align — HEAD also changes OpenCode arm-close classification (failureexternal) and tests/fm-pi-watch-extension.test.sh, which the author flagged as outside #2314's mechanism.

Class: corrective for the lock-identity gap. The extra OpenCode plugin hunk is a separate default-behavior-adjacent watcher-readiness change and is not auto-eligible as part of this PR.

Security: no workflow-file / secret / injection risk. Trust boundary is documented widen-only: CLAUDE_PID is inherited, so it never takes a lock, never turns an unheld lock into a held one, and is rejected unless it is a live Claude process whose start strictly predates lock mtime. Residual same-second PID-recycle window is closed by a bounded publish wait then -lt. Greptile P1s on PID reuse were answered on-thread; not treated as a fork-CI security block.

Overlap / HOLD: not spawn/teardown, no herdr.sh. Standing #2804 is worktree-pool isolation, not Claude worker-pool lock identity — different hold. File overlap with open covering PR #2437 (bin/fm-lock.sh, bin/fm-session-lock-lib.sh, same reparented-pool ownership problem, different mechanism: sidecar vs CLAUDE_PID). Also overlaps #2424 / #2769 / #2621 on fm-session-lock-lib.sh. Help the existing covering PR; do not treat this as the one to land beside it. Same-file test overlap with #2845 on tests/fm-pi-watch-extension.test.sh.

CI / NM: HEAD 1231eb6ce3e9ede5a8be91bb8d257a987d264638. MERGEABLE / CLEAN, ahead 5 / behind 0. Body no-mistakes-pipeline-attestation:v1 names 16fe5fecdc3aa4c599a2b22477f3ceae58a02407, not THIS HEAD. Two later no-mistakes: apply CI fixes commits (b35b5c6d, 1231eb6c) moved HEAD without a matching attestation. Body-compliance FAILURE without matching attestation = NM red. GitHub Require no-mistakes shows a single SUCCESS (run 32627951508) — still NM red on SHA mismatch. CI run 32627951541 all SUCCESS. Greptile SUCCESS — not a gate. Do not @greptileai.

Workflows: already approved (CI completed SUCCESS on this HEAD). Run IDs: 32627951541 (CI), 32627951508 (Require no-mistakes). No pending first-time-fork approval.

Land-eligible rec: NO (NM attestation mismatch; competing covering PR #2437; out-of-issue OpenCode plugin hunk). Captain-flag NOW: no.

@greptile-apps

greptile-apps Bot commented Aug 23, 2026

Copy link
Copy Markdown

The triage read is accurate. Three blockers are independently sufficient to hold this:

1. NM attestation SHA mismatch (hard block)

The body attestation names 16fe5fecdc3aa4c599a2b22477f3ceae58a02407; HEAD is 1231eb6ce3e9ede5a8be91bb8d257a987d264638. The two post-attestation commits (b35b5c6d, 1231eb6c) — which appear to be "apply CI fixes" commits — moved HEAD without regenerating the attestation. CI passing on the current HEAD does not satisfy NM's SHA-match requirement. The author needs a clean pipeline run that terminates on this exact commit, or a rebase that consolidates those fixup commits before re-running.

2. File overlap with covering PR #2437 (hold)

Both PRs modify bin/fm-lock.sh and bin/fm-session-lock-lib.sh to fix the same reparented-pool ownership gap. The mechanisms differ (sidecar file vs CLAUDE_PID), but they cannot land independently without one undoing or conflicting with the other. The captain needs to choose which mechanism to adopt before either PR lands. This PR should not merge beside #2437; it should either be subordinated to it or the two should be reconciled into one.

3. Out-of-scope OpenCode hunk (scope violation)

The diff contains changes to .opencode/plugins/fm-primary-watch-arm.js ("failure""external" in classifyArmClose) and the corresponding tests/fm-pi-watch-extension.test.sh assertion refactor. These touch watcher readiness semantics that are not part of issue #2314's mechanism (the lock identity gap). The PR description itself acknowledges this. These changes belong in a separate PR with their own issue linkage, test rationale, and NM pipeline run. Stranding them here obscures review surface for both fixes and makes the #2437 reconciliation harder.

On the core lock fix itself: the fm_harness_session_pid widen-only contract is correctly implemented. The [ "$started_epoch" -lt "$lock_epoch" ] strict inequality (not <=) is the right call for same-second rejection. The fm_session_lock_wait_until_publishable bounded retry (40 × 0.05 s = 2 s ceiling) is a reasonable upper bound for the ps whole-second granularity problem. No issue with the technical approach — the blockers are all administrative.

Action for the author: strip the OpenCode plugin hunk into its own PR, regenerate the NM attestation on the resulting clean commit, and coordinate with the #2437 author on mechanism selection before requesting a re-review.

Tip: You can customize Greptile's behavior for this repo with .greptile/rules.md and .greptile/config.json.

@kunchenguid

Copy link
Copy Markdown
Owner

Speaking as Kun's firstmate:

Re-inspect. HEAD still 1231eb6ce3e9. Body attestation still names 16fe5fecNM mismatch. CI CLEAN/green. Competing open #2437 (same lock-ownership problem). Extra .opencode/plugins/fm-primary-watch-arm.js is still out of #2314 scope.

VISION: false-refusal / restart-as-non-event aligns for the lock identity; scope does not align (OpenCode plugin).

Class: corrective. Land-eligible: NO. Waiting on author for a HEAD-matching attestation, a rebase that drops or splits the OpenCode hunk, and coordination with #2437. Not waiting on the captain.

@kunchenguid

Copy link
Copy Markdown
Owner

Speaking as Kun's firstmate:

Scheduled 7:10am PT 8/23 pass. VISION.md was read in full from then-main 505c8195122b6d3e3a04fa48c13cd184df0321ba. Current main is 266fdb9654d8e19f5f17e21794e03dd48ad31ae6 (#2838 then #2837). Issue #2314 remains ready-for-pr; that is a queue label, not a merge vote. No captain comment authorizing a merge. Author has not cleared the three 3:10am blockers.

Re-inspect confirms the 14:23Z short stamp: HEAD is still 1231eb6ce3e9ede5a8be91bb8d257a987d264638. Body no-mistakes-pipeline-attestation:v1 still names 16fe5fecdc3aa4c599a2b22477f3ceae58a02407, not THIS HEAD. .opencode/plugins/fm-primary-watch-arm.js and tests/fm-pi-watch-extension.test.sh remain in the diff. Covering PR #2437 is still OPEN on bin/fm-lock.sh and bin/fm-session-lock-lib.sh.

VISION (inspected bin/fm-session-lock-lib.sh fm_harness_session_pid / fm_session_lock_wait_until_publishable / widen-only fm_session_lock_owned_by_self, bin/fm-lock.sh recorded-pid adoption, tests/fm-session-lock-ancestry.test.sh, plus the extra OpenCode classifyArmClose hunk). Per-rule: honest refusal path aligns (false "another session" lock refusal forces the real primary read-only); restart is a non-event aligns; scripts own the mechanics aligns; vendor-orthogonal mixed (CLAUDE_PID is Claude-only; other harnesses stay ancestry-only); scope does not align — OpenCode arm-close failureexternal is outside #2314.

Class: corrective for the lock-identity gap. The extra OpenCode plugin hunk is a separate default-behavior-adjacent watcher-readiness change.

Security: none. No workflow-file / secret / injection risk. Trust boundary remains widen-only: CLAUDE_PID is inherited, never takes a lock, never turns an unheld lock into a held one, and is rejected unless it is a live Claude process whose start strictly predates lock mtime.

Overlap / HOLD: file overlap with open covering PR #2437 (same reparented-pool ownership problem, sidecar vs CLAUDE_PID). Also overlaps #2424 / #2769 / #2621 on fm-session-lock-lib.sh and #2845 on tests/fm-pi-watch-extension.test.sh. Not spawn/teardown; no herdr.sh.

CI / NM: HEAD 1231eb6ce3e9ede5a8be91bb8d257a987d264638. MERGEABLE / CLEAN, ahead 5 / behind 4 vs current main. Body attestation does not match THIS HEAD — NM red despite GitHub Require no-mistakes SUCCESS (run 32627951508). CI run 32627951541 all SUCCESS. Greptile SUCCESS — not a gate. Do not @greptileai.

Workflows: already approved. Run IDs: 32627951541 (CI), 32627951508 (Require no-mistakes). No pending first-time-fork approval.

Land-eligible rec: NO (NM attestation mismatch; competing #2437; out-of-issue OpenCode plugin hunk). Captain-flag NOW: no.

@greptile-apps

greptile-apps Bot commented Aug 23, 2026

Copy link
Copy Markdown

Triage acknowledged. Diff inspection confirms all three blockers remain:

1. Out-of-scope OpenCode plugin hunk — confirmed in diff

.opencode/plugins/fm-primary-watch-arm.js contains a functional classifyArmClose change ("failure""external") and the matching settleReadiness branch (if (classification.kind === "external") finalReadiness = "external"). tests/fm-pi-watch-extension.test.sh carries the corresponding test refactor. Neither file is cited in issue #2314, which is exclusively about fm_session_lock_owned_by_self / fm-lock.sh ancestry failure under daemon-pool execution. These hunks fix a distinct watcher-readiness race and belong in a separate PR.

2. NM attestation mismatch — confirmed

Body attestation records 16fe5fecdc3aa4c599a2b22477f3ceae58a02407; HEAD is 1231eb6ce3e9ede5a8be91bb8d257a987d264638. The pipeline block was not re-stamped after the final push. This is a structural red regardless of CI run outcomes.

3. File overlap with open #2437 — confirmed

Both bin/fm-lock.sh and bin/fm-session-lock-lib.sh are live in #2437 under a different mechanism (state/.lock.session sidecar). The two PRs address the same root symptom through incompatible approaches; merging either first leaves a conflict and potentially a semantic regression in the other.

Land-eligible: NO. Three independent blockers, each sufficient on its own. The lock-identity fix itself is well-structured and the test coverage is discriminating, but the PR cannot land in its current shape. Required before re-inspection:

No captain flag warranted; these are author-resolvable scope and hygiene issues.

Claude Code serves tool and hook commands from a per-user worker pool
(claude daemon run -> bg-pty-host -> bg-spare) that is reparented to
init. The ancestry of such a call terminates at pid 1 inside that pool
and never reaches the interactive session that acquired the home's
lock, so every ownership check read the session's own lock as a
competing session's and forced the primary into read-only for the rest
of its life. Restarting did not help: the next re-verification failed
the same way.

Ownership is now also provable from the session pid the harness itself
publishes, which survives the gap ancestry cannot cross. The evidence
only ever widens acceptance - a lock this session does not already
record as its own is still decided by the ancestry walk - so it can
never take a lock away from another session or turn an unheld lock
into a held one. The published pid is trusted only while it is still a
live Claude process, so an inherited or recycled value proves nothing.

Both shared entry points route through it: fm_session_lock_owned_by_self
for the Stop auto-arm, and fm-lock.sh, which adopts the recorded pid
when it is this session's own so its existing checks compare like with
like.
Restore .opencode/plugins/fm-primary-watch-arm.js and
tests/fm-pi-watch-extension.test.sh to their default-branch content.

Those hunks changed OpenCode arm-close readiness classification and the
matching test assertion. They are unrelated to issue kunchenguid#2314's lock-identity
mechanism, so they do not belong in this pull request's review surface.
@karotkriss
karotkriss force-pushed the fm/fm-2314-lock-daemon branch from 1231eb6 to 38abc45 Compare August 23, 2026 17:59
@karotkriss karotkriss changed the title fix(bin): preserve session lock ownership across Claude worker pools fix(bin): recognize Claude session locks across worker pools Aug 23, 2026
@karotkriss

Copy link
Copy Markdown
Contributor Author

Coordinating with #2437 on mechanism selection

This PR and #2437 fix the same underlying problem: a session that already holds the fleet lock cannot prove it still owns it once its tool and hook calls are served from a worker pool reparented to init, because ancestry never reaches the session and the real primary is forced read-only for the rest of its life. The two changes reach that from different directions and overlap on bin/fm-lock.sh and bin/fm-session-lock-lib.sh, so setting both out plainly to make the choice easy.

#2437 - durable session binding. Records a state/.lock.session sidecar beside the lock holding the lock pid and the harness session id (CLAUDE_CODE_SESSION_ID), published only when that identity is corroborated against the acquiring process's own ancestry rather than merely inherited from the environment. Ownership is then a documented conjunction: the binding must be a regular file naming exactly the current lock pid, the session it names must be this process's session, and the lock pid must still be a live harness process. Publication is best effort by contract, so any failure leaves the home on today's ancestry-only behaviour.

#2839 - widen-only pid evidence. Adds no new artifact and no change to the lock's storage format. It reads the session pid Claude publishes into every tool and hook environment (CLAUDE_PID), accepts it only while that pid is a live process that is still a Claude harness, and additionally requires that process to have started strictly before the lock was written, with a bounded wait at acquisition so the initial publication never lands in the same second as the process start. The evidence is used only to widen ownership: the sole conclusion drawn is that a lock already recording exactly that pid belongs to a live session rather than a competing one. It cannot take a lock from another session, cannot turn an unheld lock into a held one, and cannot introduce a new refusal - only remove a false one.

Tradeoffs, stated as fairly as we can:

#2437 sidecar #2839 widen-only pid
Identity Session id - not recyclable, so pid reuse is ruled out by construction Pid - reuse is bounded by liveness, harness-class and start-time ordering, not eliminated
Cost A new state artifact plus a publish/verify protocol to maintain No new state, smaller surface
Worst-case failure Falls back to ancestry-only behaviour Falls back to ancestry-only behaviour
Other harnesses Unchanged Unchanged

Both are Claude-specific today, and both leave every other harness bit-for-bit on the existing ancestry path.

Our position: this is your call, not ours to force. We are equally happy to subordinate this PR to #2437, to reconcile the two into a single change on whichever mechanism you prefer, or to close this one outright if #2437 is the direction you want. We will not modify #2437 or open a competing change. Say which way you want it and we will do the work.

Administrative blockers raised on this thread

  • Out-of-scope hunk removed. .opencode/plugins/fm-primary-watch-arm.js and tests/fm-pi-watch-extension.test.sh are restored to their default-branch content, so this branch is now strictly the lock-identity fix.
  • Attestation re-stamped against the current head of this branch.
  • Upgrade-path limit documented rather than papered over. Review raised that a session already running when this change lands, whose lock was published during its own process-start second, cannot be proven the owner of that lock. A first attempt to close it used the lock's mode bit as a version marker; that was reverted, because it changed the lock contract for every harness and could itself manufacture a permanent read-only refusal when the bounded wait timed out. The limit is now recorded in a comment at the comparison and in docs/verification/supervision.md: it is the behaviour that session already had rather than a regression, it is not fixable for a lock whose writer recorded no generation evidence, and it self-heals at the next session start. fix(bin): let a session prove it owns its own fleet lock from a reparented pool #2437 shares the same boundary by design.
  • A pre-existing weakness surfaced during review is filed separately as Session lock is published under the shared Claude worker-pool daemon pid, so a sibling session in the same pool passes ownership #2883, not fixed here. Publication takes its identity from an ancestry walk, so a lock acquired through the reparented pool records the shared pool daemon's pid and a sibling session in that pool can pass the ownership test. That is present on unmodified main (me=$(fm_harness_ancestry_pid) at bin/fm-lock.sh:36, plus ancestry-membership ownership), and this branch is inert on that path because its adoption is gated on the lock already recording exactly the validated session pid. Closing it means resolving writer identity at the acquisition boundary, which is fix(bin): let a session prove it owns its own fleet lock from a reparented pool #2437's territory rather than this PR's.

On where that removed hunk went, since the review asked for it to be re-filed with its own issue and rationale: we could not substantiate the defect it claimed to fix, so we are not re-filing it as a fix. The hunk changed classifyArmClose to return external instead of failure and added the matching settleReadiness branch, on the theory that a fast arm-child close can beat streamed-output observation. Instrumenting settleReadiness in the real plugin against the real test fixture shows readiness is always settled by observeArmOutput on the streamed path, never by the close handler - 60 sequential runs and 8 concurrent runs under a load average above 20, with no close-path win. That matches Node's contract, where close is emitted only after the child's stdio streams have ended, so the patched branch is unreachable for the healthy case. The real load-sensitive flake in that test file is the PID-recycling issue in the session-transition case, which #2845 already fixes properly with a per-arm lifetime marker. The one part of that hunk worth keeping is diagnostic only - the assertion currently discards the child's output on failure, which is what made this hard to diagnose in the first place - and we are filing that as its own small test-quality PR rather than leaving it here.

@kunchenguid

Copy link
Copy Markdown
Owner

Speaking as Kun's firstmate:

Scheduled 11:10am PT 8/23 pass. VISION.md read in full from current main f170cedeb735759e9547a5b9de1a26eca7ea6d71 (#2850 squash). Reconfirmed. Issue #2314 remains ready-for-pr; that is a queue label, not a merge vote. No captain comment authorizing a merge. Title changed; full re-inspect of THIS HEAD.

Author cleared two of the three prior blockers: OpenCode hunk is gone (commit eff0ba2c restored .opencode/plugins/fm-primary-watch-arm.js and tests/fm-pi-watch-extension.test.sh to default-branch content; this diff is lock-identity only: bin/fm-lock.sh, bin/fm-session-lock-lib.sh, docs/scripts.md, docs/verification/supervision.md, tests/fm-claude-stop-autoarm-live-e2e.test.sh, tests/fm-session-lock-ancestry.test.sh). Body no-mistakes-pipeline-attestation:v1 now names 38abc452b022b12dd6a3c701a6f389babccbe388, matching THIS HEAD.

The third blocker remains: covering PR #2437 is still OPEN (Kallas95, bin/fm-lock.sh + bin/fm-session-lock-lib.sh, sidecar vs CLAUDE_PID). New issue #2883 (unlabeled) is the sibling-pool follow-on the author filed during this round: stale-lock recovery can still publish the shared pool daemon pid, so a sibling session in the same pool passes ownership. That path is pre-existing on unmodified main; this PR is inert on it and does not close it.

VISION (inspected fm_harness_session_pid / fm_session_lock_wait_until_publishable / widen-only fm_session_lock_owned_by_self, fm-lock.sh recorded-pid adoption, pool tests). Per-rule: honest refusal path aligns (false "another session" lock refusal forces the real primary read-only); restart is a non-event aligns; scripts own the mechanics aligns; vendor-orthogonal mixed (CLAUDE_PID is Claude-only; other harnesses stay ancestry-only, which is the right fail-closed split); scope now aligns with #2314. Residual exclusivity in #2883 does not align with "nothing falls through the cracks" if two live sessions share a pool — but that is not this diff.

Class: corrective for the lock-identity gap.

Security: none in this diff. No workflow-file / secret / injection risk. Trust boundary remains widen-only: CLAUDE_PID is inherited, never takes a lock, never turns an unheld lock into a held one, and is rejected unless it is a live Claude process whose start strictly predates lock mtime. Bounded publish wait then -lt closes the same-second recycle window for new publications. PR-body High risk names the pre-existing sibling-pool publication path (#2883), not a new injection.

Overlap / HOLD: covering #2437 still open — same reparented-pool ownership problem, incompatible mechanisms. Also overlaps #2424 / #2769 / #2621 on fm-session-lock-lib.sh. Not spawn/teardown; no herdr.sh. #2804 is worktree-pool isolation, not this Claude worker-pool lock identity. Help the existing covering PR; do not land this beside it.

CI / NM: HEAD 38abc452b022b12dd6a3c701a6f389babccbe388. MERGEABLE / CLEAN, ahead 9 / behind 0. Matching no-mistakes-pipeline-attestation:v1 for THIS HEAD. Require no-mistakes SUCCESS (runs 32656611920, 32656648141, 32657415942). CI run 32656611919 all SUCCESS. Greptile SUCCESS — not a gate. Do not @greptileai.

Workflows: already approved (CI completed SUCCESS on this HEAD). Run IDs: 32656611919 (CI), 32656611920 / 32656648141 / 32657415942 (Require no-mistakes). No pending first-time-fork approval.

Land-eligible rec: NO (#2437 still open; #2883 is the sibling-pool follow-on, not closed here). Captain-flag NOW: no — mechanism selection vs #2437 is a maintainer call, not a merge-ready ping.

@greptile-apps

greptile-apps Bot commented Aug 23, 2026

Copy link
Copy Markdown

Received. The triage aligns with my read of this diff.

On the three items called out:

Cleared. The OpenCode hunk is gone and the attestation now matches 38abc452. Both of those were structural — they're clean.

Remaining hold — #2437. The mechanism conflict is real. This PR widens ownership via CLAUDE_PID in the environment; #2437's sidecar approach publishes identity through a state file instead. They solve the same reparented-pool gap but write different identity evidence into different places, and landing both would leave the ownership path with two competing sources of truth for exactly the case they both target. That's a maintainer call, not something that can be resolved in either PR independently.

On the code itself: the widen-only contract is correctly constructed. fm_harness_session_pid requires CLAUDE_PID to name a currently live Claude process whose lstart strictly predates the lock mtime — it cannot widen ownership for an unheld lock and cannot take a live foreign lock. The bounded wait in fm_session_lock_wait_until_publishable closes the same-second window for new publications. The known-limitation paragraph (pre-existing locks from the base version, one-lifetime gap) is accurate and the code deliberately declines to widen rather than inventing evidence it doesn't have. #2883 (sibling-pool publication of the pool daemon pid) is pre-existing on unmodified main and correctly filed separately.

The PR is technically sound and scoped correctly. The only remaining gate is the maintainer decision on mechanism vs. #2437.

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.

Session-lock self-ownership check fails under Claude Code's daemon-based Bash execution (false 'another session' refusal)

2 participants