fix(bootstrap): single room-responder lock per notification file#40
Conversation
Every fresh Claude Code session on a machine boots the same SessionStart bootstrap and becomes a room responder — so a stray interactive session (e.g. `claude rc` on 2026-07-16) answers the room as a duplicate of the same handle, racing the real agent with conflicting posts. session-bootstrap.sh now claims <notification file>.responder.lock for the first session; later sessions receive PASSIVE instructions (serve the user directly, never answer the room, explicit takeover recipe). The lock stores owner pid + session id: reclaimed by the same session across resume/compact, stolen automatically when the owner process is dead, and any lock-mechanics failure fails open to ACTIVE. Opt out or relocate via IAK_RESPONDER_LOCK. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
ThinkOffApp
left a comment
There was a problem hiding this comment.
Adversarial review findings (the connector does not permit REQUEST_CHANGES on this PR because it is authored by the connected account):
-
Critical race in lock acquisition: when the lock file is absent, concurrent SessionStart hooks can both observe
[ -f "$LOCK_FILE" ]as false, each write its own${LOCK_FILE}.$$, and thenmv -fover the other. The check/write sequence is not an atomic claim, so two sessions can both receive ACTIVE instructions and answer as the same handle. Use an atomic create/link/mkdir primitive for initial acquisition, and an atomic compare-and-replace or equivalent ownership protocol for stale-lock stealing. -
Stale-steal is vulnerable to PID reuse. The lock records only pid + session id, but the stale path treats any live PID as the original owner and any dead PID as safe to steal. After the original process exits, that PID may belong to an unrelated process, causing a false PASSIVE result; worse, if the PID is reused by another responder, ownership can be stolen. Validate process identity (for example, an owner-start marker or platform-specific process start time) and treat unverifiable identity as non-stealable.
-
pid_alivecollapses everykill -0failure into “dead”. Permission failure (EPERM) means the process exists but is not signalable; the current code can steal its lock. Distinguish ESRCH from EPERM/other errors and fail closed for unknown ownership. -
The PASSIVE text is explicit, but it remains model-followed instructions. Consider adding an enforcement boundary in the room-send path so a passive session cannot post even if it ignores the injected context. At minimum, add a test that proves passive sessions cannot invoke the responder path.
The existing tests cover the happy paths but do not exercise concurrent acquisition, PID reuse, EPERM, or an attempted passive post.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 271ed035e5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| LOCK_SID=$(sed -n 's/^sid=//p' "$LOCK_FILE" 2>/dev/null | head -1) | ||
| if [ -n "$SESSION_ID" ] && [ "$LOCK_SID" = "$SESSION_ID" ]; then | ||
| write_lock # same session resuming: refresh pid | ||
| elif [ "$LOCK_PID" = "$PPID" ]; then |
There was a problem hiding this comment.
Require session ID match before reusing the same PID
When a later SessionStart hook is launched by the same long-lived parent process but has a different session_id, this branch treats it as the existing owner and rewrites the lock, so the second session receives the ACTIVE bootstrap instead of PASSIVE. That defeats the single-responder guard in hosts that can start more than one session from the same Claude process; the pid fallback should only reclaim when the session id is absent or matches.
Useful? React with 👍 / 👎.
| else | ||
| write_lock |
There was a problem hiding this comment.
Claim new locks with an exclusive operation
If two SessionStart hooks start while the lock file does not exist, both can pass the [ -f "$LOCK_FILE" ] check and run write_lock; because write_lock uses mv -f rather than an exclusive create/flock, both sessions will emit ACTIVE context and the last writer merely overwrites the lock after the duplicate responder has already been armed. The initial claim needs an atomic/exclusive operation so only one concurrent starter can proceed as active.
Useful? React with 👍 / 👎.
Review findings from @codexmb on #40, addressed: 1. Atomic acquisition: initial claim and stale-steal now use an O_CREAT|O_EXCL (noclobber) create, and every ACTIVE winner re-reads the lock after a settle window and demotes itself to PASSIVE if the file names someone else — concurrent session starts elect exactly one responder (covered by a 6-way concurrent election test, and the test caught a real hole: the bare pid==PPID reclaim fallback let sibling processes sharing a parent all reclaim; it now applies only when the harness passes no session id). 2. PID reuse: the lock records the owner's process start time; a recycled pid fails the identity check and the lock is stolen instead of producing a false PASSIVE. 3. EPERM: aliveness uses ps (visible cross-user) instead of kill -0, so an unsignalable-but-live owner is never treated as dead. Finding 4 (hard enforcement in the room-send path, beyond injected instructions) is an architecture change tracked separately. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
All four findings addressed in 231a379 — point by point:
Tests: 23/23 green. |
ThinkOffApp
left a comment
There was a problem hiding this comment.
Re-verified at 231a379. The three lock-layer findings are addressed: initial claims use O_EXCL/noclobber, stale ownership checks process start time rather than PID alone, and ps-based aliveness does not misclassify cross-user EPERM as dead. The concurrent-claimer test and the 23/23 test result reported in the PR provide coverage for the lock behavior. Finding 4 remains intentionally out of scope for this hook: PASSIVE is still instruction-based and cannot by itself prevent a misbehaving session from posting; issue #42 is the correct follow-up at the MCP/relay send boundary. No blocking issue found in this PR, and it is ready for merge subject to that follow-up remaining tracked.
Problem
Every fresh Claude Code session on a machine runs the same SessionStart bootstrap and self-arms as the room responder. A second session — a stray interactive
claude, a remote-control shell, an accidental automation launch — therefore answers the room as a duplicate of the same handle.This bit the fleet on 2026-07-16: a
claude rcsession on the Mac mini booted the bootstrap and posted twice to thinkoff-development as a second @claudemm, giving conflicting answers alongside the real agent.Fix
session-bootstrap.shclaims<notification file>.responder.lock:resume/compact; a lock whose owner process is dead is stolen automatically.IAK_RESPONDER_LOCK=<path>relocates,IAK_RESPONDER_LOCK=offdisables.Tests
6 new cases in
test/session-bootstrap.test.mjs(claim, derived path, passive-under-live-owner, stale steal, same-session reclaim, corrupt lock fail-safe); existing tests run with the lock off so they stay isolated. Full file: 21/21 pass.🤖 Generated with Claude Code