Skip to content

fix: bind session locks to session identities - #21

Merged
adibirzu merged 8 commits into
mainfrom
fm/fm-lock-ownership-2883
Aug 30, 2026
Merged

adibirzu merged 8 commits into
mainfrom
fm/fm-lock-ownership-2883

Conversation

@adibirzu

@adibirzu adibirzu commented Aug 24, 2026 •

Copy link
Copy Markdown
Owner

Intent

Fix issue 2883: make the session lock publish an identity that distinguishes the acquiring session instead of a shared Claude Code worker pool, fail closed when that session identity cannot be established, preserve legitimate same-session re-entry across tool calls, and handle existing old-format lock files without wedging homes. Keep the temporary legacy compatibility path explicitly linked to follow-up task fm-remove-legacy-lock-compat. Explicitly test both sibling-session rejection and same-session acceptance through executable interfaces. Do not claim, imply, or hint that this change fixes or relates to the tests/fm-pi-watch-extension.test.sh CI flake.

What Changed

  • Publish a validated session-identity binding alongside the compatible lock PID, requiring Claude session ID and served PID while preserving verified ancestry identities for other harnesses.
  • Use the binding for lock ownership, session-start completion, nudge, auto-arm, and startup-network mutation authorization; temporarily log accepted PID-only legacy locks under fm-remove-legacy-lock-compat.
  • Add executable coverage for same-session re-entry, sibling-session rejection, identity failures, legacy compatibility, and binding changes during startup work.

Risk Assessment

✅ Low: The identity binding, fail-closed Claude path, legacy compatibility containment, worker snapshotting, and completion-record validation are coherently applied across the changed call sites, with executable regression coverage for owner re-entry and sibling rejection.

Testing

The focused executable lock-ownership suite passed and produced reviewer-visible evidence for all required ownership, re-entry, legacy-compatibility, and fail-closed behaviors. Ancillary changed-consumer suites were attempted but not relied on after their harnesses stalled.

Evidence: Session-lock end-to-end behavior transcript

Source: Session-lock end-to-end behavior transcript

ok - fm-lock: a new session binding readmits its owner and rejects a sibling in the same pool ok - fm-lock: legacy shared-pool ownership is accepted only visibly during migration ok - fm-lock: an unidentifiable new Claude acquisition refuses without legacy fallback ok - fm-lock: a Claude worker without session identity fails closed

ok - fm-lock: a new session binding readmits its owner and rejects a sibling in the same pool
ok - fm-lock: legacy shared-pool ownership is accepted only visibly during migration
ok - fm-lock: an unidentifiable new Claude acquisition refuses without legacy fallback
ok - fm-lock: a Claude worker without session identity fails closed

Pipeline

Updates from git push no-mistakes

✅ **intent** - passed

✅ No issues found.

✅ **Rebase** - passed

✅ No issues found.

🔧 **Review** - 2 issues found → auto-fixed (2) ✅
  • 🚨 bin/fm-startup-network.sh:397 - The detached startup worker revalidates only the numeric lock PID after the initial identity-aware check. If session A launches it, exits, and session B reuses A's PID but acquires a new lock with a different session ID, lock_unchanged still authorizes A's worker to run mutating sweeps under B's lock. Capture and compare the complete lock binding at worker launch and immediately before sweeping.
  • 🚨 bin/fm-sessionstart-run.sh:78 - Clear/compact chooses re-emit by matching .session-start-complete to only .lock's PID. After PID reuse, a new session can own its new sidecar binding yet match the prior session's completion PID, skipping the full startup it never completed. Bind the completion record to the full lock identity and validate it through the shared lock-record boundary.

🔧 Fix: Bind workers and completion to lock identities
3 errors still open:

  • 🚨 bin/fm-session-lock-lib.sh:235 - Required criterion: “fail closed when that session identity cannot be established.” The changed if fm_harness_ancestry_is_claude && [ "${CLAUDECODE:-}" = 1 ] hunk falls through to an ancestry binding whenever a verified Claude process lacks that marker, even though no Claude session ID/PID identity was established. A shared-pool sibling can then regain the old ancestry-based ownership path; require the Claude identity whenever the verified ancestry is Claude.
  • 🚨 bin/fm-startup-network.sh:202 - The worker launch still does not capture one atomic lock binding. It reads A’s raw PID, verifies A, then separately reads the record; if A exits and B reuses A’s PID before the second read, the worker is launched with B’s kind/session and passes its later unchanged check, so A’s detached worker mutates under B’s lock. Hold the lock acquisition lease while validating and snapshotting the complete record, then pass that snapshot to the worker.
  • 🚨 bin/fm-session-start.sh:940 - Completion is bound to whichever lock record exists at the end of the digest, not the session that ran it. After A acquires the lock and starts a full digest, A can lose it and B can acquire it; lines 940–946 then serialize B’s binding as A’s completion. B’s later clear/compact matches that forged completion and re-emits instead of running its required full startup. Capture A’s binding immediately after successful acquisition and publish completion only if the current record still matches that snapshot.

🔧 Fix: Snapshot lock ownership across workers and completion
✅ Re-checked - no issues remain.

✅ **Test** - passed

✅ No issues found.

  • git diff --check 3c544d6a1958784845356e62bc09d3dd56ee8a67 bbf36dfa4911a7b9c18fa1983d20854bb04e1ce1
  • bash tests/fm-lock-ownership.test.sh
  • Attempted bash tests/fm-session-lock-ancestry.test.sh and bash tests/fm-sessionstart-nudge.test.sh; they were stopped after the test harness failed to make further progress, so they are not counted as validation evidence.
✅ **Document** - passed

✅ No issues found.

✅ **Lint** - passed

✅ No issues found.

✅ **Push** - passed

✅ No issues found.

An unidentifiable Claude acquisition must refuse without taking the
legacy compatibility path. The suite runner can inherit CLAUDECODE and
CLAUDE_CODE_SESSION_ID from a parent Claude session, which made those
cases identifiable and let them acquire the lock. Drop those markers so
the fail-closed assertions exercise a genuinely missing identity.
@adibirzu
adibirzu force-pushed the fm/fm-lock-ownership-2883 branch from 3245c96 to c28b993 Compare August 30, 2026 05:05
@adibirzu

Copy link
Copy Markdown
Owner Author

Rebased onto current main (one test-list overlap in bin/fm-test-run.sh: kept both fm-opencode-secondmate-arm.test.sh and fm-lock-ownership.test.sh) and fixed the pre-existing fail-closed case without weakening its assertion.

Cause: tests/fm-lock-ownership.test.sh omitted CLAUDE_CODE_SESSION_ID in the unidentifiable/fail-closed cases but did not unset an inherited value. A suite runner launched under Claude Code therefore still looked identifiable and took the legacy compatibility path (lock acquired) instead of refusing.

Fix: drop ambient CLAUDECODE / CLAUDE_CODE_SESSION_ID / CLAUDE_PID for those cases. The assertions still require a non-zero refusal, the exact cannot establish this session's lock identity diagnostic, and no lock/binding mutation.

Local verification of record (ambient Claude identity was present in the runner: CLAUDECODE=1 and a live CLAUDE_CODE_SESSION_ID):

$ bash tests/fm-lock-ownership.test.sh
ok - fm-lock: a new session binding readmits its owner and rejects a sibling in the same pool
ok - fm-lock: legacy shared-pool ownership is accepted only visibly during migration
ok - fm-lock: an unidentifiable new Claude acquisition refuses without legacy fallback
ok - fm-lock: a Claude worker without session identity fails closed

$ bin/fm-test-run.sh tests/fm-lock-ownership.test.sh
FM_TEST_BEGIN 2026-08-30T05:04:14Z tests/fm-lock-ownership.test.sh family=watcher-wake-lock expected_gate_skip=none
ok - fm-lock: a new session binding readmits its owner and rejects a sibling in the same pool
ok - fm-lock: legacy shared-pool ownership is accepted only visibly during migration
ok - fm-lock: an unidentifiable new Claude acquisition refuses without legacy fallback
ok - fm-lock: a Claude worker without session identity fails closed
FM_TEST_END 2026-08-30T05:04:17Z tests/fm-lock-ownership.test.sh exit=0 duration_ms=3088 gate_skip=false
FM_TEST_SUMMARY total=1 failed=0 skipped_gate=0 duration_ms=3144

bin/fm-lint.sh passed. tests/fm-session-lock-ancestry.test.sh also passed locally.

@adibirzu
adibirzu merged commit c48a90f into main Aug 30, 2026
26 of 28 checks passed
adibirzu pushed a commit that referenced this pull request Aug 30, 2026
Brings adibirzu/firstmate main (c48a90f, session locks bound to session
identities) into the upstream-sync branch.

docs/sessionstart-nudge.md was the only conflict. Kept upstream's
description of the shared fm_session_lock_owned_by_current_session()
verifier that replaced the nudge wrapper's hard-coded eight-parent
ancestry walk, and kept the fork-only exit contract that qualifies the
exit-0 rule to ordinary transport paths and documents the run wrapper's
internal --pi-prerequisite silent exit 3.
adibirzu pushed a commit that referenced this pull request Aug 30, 2026
…entity

Upstream #21 bound the session lock to a session identity and updated
every fixture it owned, including run_autoarm(), to export a controlled
CLAUDECODE / CLAUDE_CODE_SESSION_ID / CLAUDE_PID triple. The fork-only
run_autoarm_bg() background twin was invisible to that change, so after
the merge its hook could not prove ownership of the lock it had just
written, exited early, and never reached the reset boundary that
test_owner_mutex_contention_preserves_failure_episode_reset waits on.

Apply the same controlled identity there.
@adibirzu
adibirzu deleted the fm/fm-lock-ownership-2883 branch September 6, 2026 20:07
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