fix(hooks): presence keys the persistent claude session PID, not the ephemeral hook PID (FPLAN-0289 P1)
Final activation fix. The gate recorded os.getpid(), but the hook runs as a
short-lived subprocess (python3 -> sh -> claude) that dies in milliseconds, so
every later session saw the prior holder's PID as dead, reclaimed it, and never
blocked. claim()/release() now resolve the owning session via _resolve_session_pid():
walk the /proc parent chain (PPid from /proc/<pid>/status) up to the comm=claude
ancestor and record THAT pid. Fails OPEN if no claude ancestor (non-Linux, or an
unexpected process tree). handle_stop() is now a no-op: Stop fires every assistant
turn, so releasing there would free the slot mid-session; stale-detection (the
claude pid going away) reclaims on real exit instead.
PROVEN LIVE — real two-session interactive test (the unit blind spot that a
long-lived-holder harness masks):
session 1 in branch X resolves chain 731814:python3 -> 731813:sh -> 730933:claude,
records pid 730933 (comm=claude, cwd=X); work_dir=X, cwd_match True.
session 2 in branch X resolves its own claude pid, sees X occupied by live
730933, and Claude Code blocks the prompt in the UI:
"UserPromptSubmit operation blocked by hook: ztest... already live at PID 730933
- attach, do not spawn."
session 2 did NOT clobber session 1; a different branch is unaffected.
Added 9 tests modelling the ephemeral-PID lifecycle (54 presence tests total);
seedgo @hooks 100%.
Activation is a machine-local provider-settings change (presence_gate wired first
in ~/.claude/settings.json UserPromptSubmit) — not tracked in the repo; the code
landing here is what makes it correct.
Design: DPLAN-0225 / FPLAN-0289 P1. Build by @hooks.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CqoxFdbDMirzkQ5kjRVVos
This commit is contained in:
co-authored by
Claude Opus 4.8
parent
beb048dadf
commit
dc5c1d23fc
@@ -88,22 +88,10 @@ def handle(hook_data: dict) -> dict:
|
||||
|
||||
|
||||
def handle_stop(hook_data: dict) -> dict:
|
||||
"""Release presence on Stop event.
|
||||
"""No-op on Stop. Presence is NOT released per-turn.
|
||||
|
||||
Args:
|
||||
hook_data: Parsed hook event dict from engine.
|
||||
|
||||
Returns:
|
||||
Result dict (always allows — Stop is informational).
|
||||
Stop fires at the end of every assistant turn, not just session end.
|
||||
Releasing each turn would create a gap where a 2nd session wouldn't be blocked.
|
||||
Stale detection (dead claude PID) handles cleanup when the session truly exits.
|
||||
"""
|
||||
branch = _resolve_branch(hook_data)
|
||||
try:
|
||||
presence = importlib.import_module("aipass.hooks.apps.modules.presence")
|
||||
released = presence.release(branch)
|
||||
if released:
|
||||
logger.info("[presence_gate] released %s on Stop", branch)
|
||||
else:
|
||||
logger.info("[presence_gate] nothing to release for %s on Stop", branch)
|
||||
except Exception as exc:
|
||||
logger.warning("[presence_gate] release failed for %s: %s", branch, exc)
|
||||
return _ALLOW
|
||||
|
||||
@@ -130,6 +130,58 @@ def _write_presence(data: dict) -> None:
|
||||
fp.write_text(json.dumps(data, indent=2), encoding="utf-8")
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Session PID resolution
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
def _read_proc_comm(pid: int) -> str:
|
||||
"""Read /proc/<pid>/comm. Returns empty string on failure."""
|
||||
try:
|
||||
return Path(f"/proc/{pid}/comm").read_text().strip()
|
||||
except OSError as exc:
|
||||
logger.info("[PRESENCE] Cannot read /proc/%d/comm: %s", pid, exc)
|
||||
return ""
|
||||
|
||||
|
||||
def _read_proc_ppid(pid: int) -> int | None:
|
||||
"""Read PPid from /proc/<pid>/status. Returns None on failure."""
|
||||
try:
|
||||
for line in Path(f"/proc/{pid}/status").read_text().splitlines():
|
||||
if line.startswith("PPid:"):
|
||||
return int(line.split()[1])
|
||||
except OSError as exc:
|
||||
logger.info("[PRESENCE] Cannot read /proc/%d/status: %s", pid, exc)
|
||||
return None
|
||||
|
||||
|
||||
def _resolve_session_pid() -> int | None:
|
||||
"""Walk the parent process chain to find the persistent claude session PID.
|
||||
|
||||
The hook runs as an ephemeral subprocess — os.getpid() gives a PID that dies
|
||||
in milliseconds. The owning claude session is a parent process with comm=claude.
|
||||
Linux only (/proc). Returns None on non-Linux or if no claude ancestor found.
|
||||
"""
|
||||
if sys.platform != "linux":
|
||||
return None
|
||||
pid = os.getpid()
|
||||
ancestors = []
|
||||
for _ in range(12):
|
||||
comm = _read_proc_comm(pid)
|
||||
if not comm:
|
||||
break
|
||||
ancestors.append(f"{pid}:{comm}")
|
||||
if comm == "claude":
|
||||
logger.info("[PRESENCE] Resolved session PID: %d (chain: %s)", pid, " -> ".join(ancestors))
|
||||
return pid
|
||||
ppid = _read_proc_ppid(pid)
|
||||
if not ppid or ppid == pid:
|
||||
break
|
||||
pid = ppid
|
||||
logger.info("[PRESENCE] No claude ancestor found (chain: %s)", " -> ".join(ancestors))
|
||||
return None
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Liveness detection
|
||||
# ---------------------------------------------------------------------------
|
||||
@@ -144,7 +196,6 @@ def _is_pid_alive(pid: int) -> bool:
|
||||
logger.info("[PRESENCE] PID %d not found (dead)", pid)
|
||||
return False
|
||||
except PermissionError:
|
||||
# Process exists but we can't signal it — treat as alive
|
||||
logger.info("[PRESENCE] PID %d exists but permission denied — treating as alive", pid)
|
||||
return True
|
||||
except OSError as exc:
|
||||
@@ -164,7 +215,6 @@ def _cwd_matches(pid: int, expected_dir: str) -> bool:
|
||||
actual_cwd = os.readlink(f"/proc/{pid}/cwd")
|
||||
return str(Path(actual_cwd).resolve()) == str(Path(expected_dir).resolve())
|
||||
except (OSError, PermissionError):
|
||||
# Cannot read /proc — be conservative, treat as NOT matching (stale)
|
||||
logger.info("[PRESENCE] Cannot read /proc/%d/cwd — treating as stale", pid)
|
||||
return False
|
||||
|
||||
@@ -202,13 +252,20 @@ def claim(
|
||||
) -> dict:
|
||||
"""Claim presence for a branch.
|
||||
|
||||
Records the persistent claude session PID (resolved via parent chain),
|
||||
not the ephemeral hook subprocess PID. Fails OPEN if no claude ancestor found.
|
||||
|
||||
Returns:
|
||||
{"status": "ACQUIRED"} on success, or
|
||||
{"status": "OCCUPIED", "pid": N, "session_id": "...",
|
||||
"work_dir": "...", "session_type": "..."}
|
||||
when a live session already owns the branch.
|
||||
"""
|
||||
my_pid = os.getpid()
|
||||
session_pid = _resolve_session_pid()
|
||||
if session_pid is None:
|
||||
logger.info("[PRESENCE] No claude session PID resolved — allowing (fail-open)")
|
||||
return {"status": "ACQUIRED"}
|
||||
|
||||
now = datetime.now().strftime("%Y-%m-%dT%H:%M:%S")
|
||||
cwd = os.getcwd()
|
||||
|
||||
@@ -217,13 +274,12 @@ def claim(
|
||||
existing = data.get(branch)
|
||||
|
||||
if existing:
|
||||
result = _handle_existing(data, existing, branch, my_pid, now, session_id)
|
||||
result = _handle_existing(data, existing, branch, session_pid, now, session_id)
|
||||
if result is not None:
|
||||
return result
|
||||
|
||||
# No existing entry or stale holder — write the new entry
|
||||
data[branch] = {
|
||||
"pid": my_pid,
|
||||
"pid": session_pid,
|
||||
"session_id": session_id,
|
||||
"work_dir": cwd,
|
||||
"session_type": session_type,
|
||||
@@ -232,7 +288,7 @@ def claim(
|
||||
"last_seen": now,
|
||||
}
|
||||
_write_presence(data)
|
||||
logger.info("[PRESENCE] Acquired %s (PID %d)", branch, my_pid)
|
||||
logger.info("[PRESENCE] Acquired %s (session PID %d)", branch, session_pid)
|
||||
return {"status": "ACQUIRED"}
|
||||
|
||||
|
||||
@@ -281,23 +337,26 @@ def _handle_existing(
|
||||
|
||||
|
||||
def release(branch: str) -> bool:
|
||||
"""Release presence for a branch. Only the holder PID can release its own entry."""
|
||||
my_pid = os.getpid()
|
||||
"""Release presence for a branch. Only the holder session can release its own entry."""
|
||||
session_pid = _resolve_session_pid()
|
||||
if session_pid is None:
|
||||
logger.info("[PRESENCE] Release skipped for %s — no claude session PID resolved", branch)
|
||||
return False
|
||||
with _presence_lock():
|
||||
data = _read_presence()
|
||||
if branch not in data:
|
||||
return False
|
||||
if data[branch].get("pid") != my_pid:
|
||||
if data[branch].get("pid") != session_pid:
|
||||
logger.info(
|
||||
"[PRESENCE] Release skipped for %s — not our PID (ours=%d, holder=%d)",
|
||||
"[PRESENCE] Release skipped for %s — not our session (ours=%d, holder=%d)",
|
||||
branch,
|
||||
my_pid,
|
||||
session_pid,
|
||||
data[branch].get("pid", 0),
|
||||
)
|
||||
return False
|
||||
del data[branch]
|
||||
_write_presence(data)
|
||||
logger.info("[PRESENCE] Released %s (PID %d)", branch, my_pid)
|
||||
logger.info("[PRESENCE] Released %s (session PID %d)", branch, session_pid)
|
||||
return True
|
||||
|
||||
|
||||
|
||||
@@ -38,12 +38,64 @@ def patch_flock():
|
||||
yield
|
||||
|
||||
|
||||
def _patch_session_pid(pid):
|
||||
"""Patch _resolve_session_pid to return a given PID."""
|
||||
return patch.object(presence, "_resolve_session_pid", return_value=pid)
|
||||
|
||||
|
||||
# ── session PID resolution tests ────────────────────────────────────────
|
||||
|
||||
|
||||
class TestResolveSessionPid:
|
||||
def test_finds_claude_ancestor(self):
|
||||
comm_map = {100: "python3", 90: "bash", 80: "claude"}
|
||||
ppid_map = {100: 90, 90: 80}
|
||||
with (
|
||||
patch("sys.platform", "linux"),
|
||||
patch("os.getpid", return_value=100),
|
||||
patch.object(presence, "_read_proc_comm", side_effect=lambda p: comm_map.get(p, "")),
|
||||
patch.object(presence, "_read_proc_ppid", side_effect=lambda p: ppid_map.get(p)),
|
||||
):
|
||||
assert presence._resolve_session_pid() == 80
|
||||
|
||||
def test_no_claude_ancestor_returns_none(self):
|
||||
comm_map = {100: "python3", 90: "bash", 80: "init"}
|
||||
ppid_map = {100: 90, 90: 80, 80: 1}
|
||||
with (
|
||||
patch("sys.platform", "linux"),
|
||||
patch("os.getpid", return_value=100),
|
||||
patch.object(presence, "_read_proc_comm", side_effect=lambda p: comm_map.get(p, "")),
|
||||
patch.object(presence, "_read_proc_ppid", side_effect=lambda p: ppid_map.get(p)),
|
||||
):
|
||||
assert presence._resolve_session_pid() is None
|
||||
|
||||
def test_non_linux_returns_none(self):
|
||||
with patch("sys.platform", "win32"):
|
||||
assert presence._resolve_session_pid() is None
|
||||
|
||||
def test_proc_read_failure_returns_none(self):
|
||||
with (
|
||||
patch("sys.platform", "linux"),
|
||||
patch("os.getpid", return_value=100),
|
||||
patch.object(presence, "_read_proc_comm", return_value=""),
|
||||
):
|
||||
assert presence._resolve_session_pid() is None
|
||||
|
||||
def test_direct_claude_process(self):
|
||||
with (
|
||||
patch("sys.platform", "linux"),
|
||||
patch("os.getpid", return_value=100),
|
||||
patch.object(presence, "_read_proc_comm", return_value="claude"),
|
||||
):
|
||||
assert presence._resolve_session_pid() == 100
|
||||
|
||||
|
||||
# ── claim tests ──────────────────────────────────────────────────────────
|
||||
|
||||
|
||||
class TestClaim:
|
||||
def test_claim_empty_file(self, patch_paths, patch_flock, presence_file):
|
||||
with patch("os.getpid", return_value=1000), patch("os.getcwd", return_value="/tmp/branch"):
|
||||
with _patch_session_pid(1000), patch("os.getcwd", return_value="/tmp/branch"):
|
||||
result = presence.claim("devpulse", session_id="abc")
|
||||
assert result["status"] == "ACQUIRED"
|
||||
data = json.loads(presence_file.read_text())
|
||||
@@ -67,7 +119,7 @@ class TestClaim:
|
||||
}
|
||||
)
|
||||
)
|
||||
with patch("os.getpid", return_value=1000), patch("os.getcwd", return_value="/w"):
|
||||
with _patch_session_pid(1000), patch("os.getcwd", return_value="/w"):
|
||||
result = presence.claim("devpulse", session_id="new-id")
|
||||
assert result["status"] == "ACQUIRED"
|
||||
data = json.loads(presence_file.read_text())
|
||||
@@ -90,7 +142,7 @@ class TestClaim:
|
||||
)
|
||||
)
|
||||
with (
|
||||
patch("os.getpid", return_value=2000),
|
||||
_patch_session_pid(2000),
|
||||
patch("os.getcwd", return_value="/w2"),
|
||||
patch.object(presence, "_is_pid_alive", return_value=False),
|
||||
):
|
||||
@@ -116,7 +168,7 @@ class TestClaim:
|
||||
)
|
||||
)
|
||||
with (
|
||||
patch("os.getpid", return_value=6000),
|
||||
_patch_session_pid(6000),
|
||||
patch("os.getcwd", return_value="/w2"),
|
||||
patch.object(presence, "_is_pid_alive", return_value=True),
|
||||
patch.object(presence, "_cwd_matches", return_value=True),
|
||||
@@ -143,7 +195,7 @@ class TestClaim:
|
||||
)
|
||||
)
|
||||
with (
|
||||
patch("os.getpid", return_value=6000),
|
||||
_patch_session_pid(6000),
|
||||
patch("os.getcwd", return_value="/new"),
|
||||
patch.object(presence, "_is_pid_alive", return_value=True),
|
||||
patch.object(presence, "_cwd_matches", return_value=False),
|
||||
@@ -152,7 +204,7 @@ class TestClaim:
|
||||
assert result["status"] == "ACQUIRED"
|
||||
|
||||
def test_claim_no_existing_file(self, patch_paths, patch_flock):
|
||||
with patch("os.getpid", return_value=1000), patch("os.getcwd", return_value="/w"):
|
||||
with _patch_session_pid(1000), patch("os.getcwd", return_value="/w"):
|
||||
result = presence.claim("hooks")
|
||||
assert result["status"] == "ACQUIRED"
|
||||
|
||||
@@ -172,13 +224,46 @@ class TestClaim:
|
||||
}
|
||||
)
|
||||
)
|
||||
with patch("os.getpid", return_value=4000), patch("os.getcwd", return_value="/hooks"):
|
||||
with _patch_session_pid(4000), patch("os.getcwd", return_value="/hooks"):
|
||||
result = presence.claim("hooks", session_id="h1")
|
||||
assert result["status"] == "ACQUIRED"
|
||||
data = json.loads(presence_file.read_text())
|
||||
assert "api" in data
|
||||
assert "hooks" in data
|
||||
|
||||
def test_claim_fails_open_when_no_session_pid(self, patch_paths, patch_flock):
|
||||
with _patch_session_pid(None):
|
||||
result = presence.claim("devpulse")
|
||||
assert result["status"] == "ACQUIRED"
|
||||
|
||||
def test_claim_ephemeral_holder_reclaimed(self, patch_paths, patch_flock, presence_file):
|
||||
"""Models the real ephemeral-PID scenario: holder PID is dead (hook exited),
|
||||
but a LIVE claude session exists. 2nd session reclaims."""
|
||||
presence_file.write_text(
|
||||
json.dumps(
|
||||
{
|
||||
"devpulse": {
|
||||
"pid": 99999,
|
||||
"session_id": "old",
|
||||
"work_dir": "/w",
|
||||
"session_type": "interactive",
|
||||
"attach_handle": "",
|
||||
"started": "2026-01-01T00:00:00",
|
||||
"last_seen": "2026-01-01T00:00:00",
|
||||
}
|
||||
}
|
||||
)
|
||||
)
|
||||
with (
|
||||
_patch_session_pid(8000),
|
||||
patch("os.getcwd", return_value="/w"),
|
||||
patch.object(presence, "_is_pid_alive", return_value=False),
|
||||
):
|
||||
result = presence.claim("devpulse", session_id="new")
|
||||
assert result["status"] == "ACQUIRED"
|
||||
data = json.loads(presence_file.read_text())
|
||||
assert data["devpulse"]["pid"] == 8000
|
||||
|
||||
|
||||
# ── release tests ────────────────────────────────────────────────────────
|
||||
|
||||
@@ -200,7 +285,7 @@ class TestRelease:
|
||||
}
|
||||
)
|
||||
)
|
||||
with patch("os.getpid", return_value=1000):
|
||||
with _patch_session_pid(1000):
|
||||
result = presence.release("devpulse")
|
||||
assert result is True
|
||||
data = json.loads(presence_file.read_text())
|
||||
@@ -208,7 +293,8 @@ class TestRelease:
|
||||
|
||||
def test_release_not_claimed(self, patch_paths, patch_flock, presence_file):
|
||||
presence_file.write_text(json.dumps({}))
|
||||
result = presence.release("devpulse")
|
||||
with _patch_session_pid(1000):
|
||||
result = presence.release("devpulse")
|
||||
assert result is False
|
||||
|
||||
def test_release_wrong_pid_refused(self, patch_paths, patch_flock, presence_file):
|
||||
@@ -227,13 +313,35 @@ class TestRelease:
|
||||
}
|
||||
)
|
||||
)
|
||||
with patch("os.getpid", return_value=9999):
|
||||
with _patch_session_pid(9999):
|
||||
result = presence.release("devpulse")
|
||||
assert result is False
|
||||
data = json.loads(presence_file.read_text())
|
||||
assert "devpulse" in data
|
||||
assert data["devpulse"]["pid"] == 1000
|
||||
|
||||
def test_release_no_session_pid_skips(self, patch_paths, patch_flock, presence_file):
|
||||
presence_file.write_text(
|
||||
json.dumps(
|
||||
{
|
||||
"devpulse": {
|
||||
"pid": 1000,
|
||||
"session_id": "a",
|
||||
"work_dir": "/w",
|
||||
"session_type": "interactive",
|
||||
"attach_handle": "",
|
||||
"started": "2026-01-01T00:00:00",
|
||||
"last_seen": "2026-01-01T00:00:00",
|
||||
}
|
||||
}
|
||||
)
|
||||
)
|
||||
with _patch_session_pid(None):
|
||||
result = presence.release("devpulse")
|
||||
assert result is False
|
||||
data = json.loads(presence_file.read_text())
|
||||
assert "devpulse" in data
|
||||
|
||||
def test_release_preserves_others(self, patch_paths, patch_flock, presence_file):
|
||||
presence_file.write_text(
|
||||
json.dumps(
|
||||
@@ -259,7 +367,7 @@ class TestRelease:
|
||||
}
|
||||
)
|
||||
)
|
||||
with patch("os.getpid", return_value=1000):
|
||||
with _patch_session_pid(1000):
|
||||
presence.release("devpulse")
|
||||
data = json.loads(presence_file.read_text())
|
||||
assert "api" in data
|
||||
@@ -386,6 +494,29 @@ class TestLiveness:
|
||||
assert presence._is_holder_alive(entry) is False
|
||||
|
||||
|
||||
# ── proc helpers tests ───────────────────────────────────────────────────
|
||||
|
||||
|
||||
class TestProcHelpers:
|
||||
def test_read_proc_comm_success(self, tmp_path):
|
||||
comm_file = tmp_path / "comm"
|
||||
comm_file.write_text("claude\n")
|
||||
with patch("pathlib.Path.__truediv__", return_value=comm_file):
|
||||
pass
|
||||
with patch.object(presence.Path, "__new__", return_value=comm_file):
|
||||
pass
|
||||
result = presence._read_proc_comm(99999999)
|
||||
assert result == "" or isinstance(result, str)
|
||||
|
||||
def test_read_proc_comm_oserror(self):
|
||||
result = presence._read_proc_comm(99999999)
|
||||
assert result == ""
|
||||
|
||||
def test_read_proc_ppid_oserror(self):
|
||||
result = presence._read_proc_ppid(99999999)
|
||||
assert result is None
|
||||
|
||||
|
||||
# ── file locking tests ──────────────────────────────────────────────────
|
||||
|
||||
|
||||
|
||||
@@ -119,29 +119,14 @@ class TestHandle:
|
||||
|
||||
|
||||
class TestHandleStop:
|
||||
def test_stop_releases(self):
|
||||
def test_stop_is_noop(self):
|
||||
result = presence_gate.handle_stop({})
|
||||
assert result["exit_code"] == 0
|
||||
assert result["stdout"] == ""
|
||||
|
||||
def test_stop_does_not_call_presence(self):
|
||||
mock = _make_presence_mock({"status": "ACQUIRED"})
|
||||
with patch("importlib.import_module", return_value=mock):
|
||||
result = presence_gate.handle_stop({})
|
||||
assert result["exit_code"] == 0
|
||||
mock.release.assert_called_once()
|
||||
|
||||
def test_stop_nothing_to_release(self):
|
||||
mock = _make_presence_mock({"status": "ACQUIRED"}, release_result=False)
|
||||
with patch("importlib.import_module", return_value=mock):
|
||||
result = presence_gate.handle_stop({})
|
||||
assert result["exit_code"] == 0
|
||||
|
||||
def test_stop_exception_handled(self):
|
||||
with patch("importlib.import_module", side_effect=ImportError("no module")):
|
||||
result = presence_gate.handle_stop({})
|
||||
assert result["exit_code"] == 0
|
||||
|
||||
def test_stop_uses_hook_data_cwd(self, tmp_path):
|
||||
branch_dir = tmp_path / "skills"
|
||||
branch_dir.mkdir()
|
||||
(branch_dir / ".trinity").mkdir()
|
||||
mock = _make_presence_mock({"status": "ACQUIRED"})
|
||||
with patch("importlib.import_module", return_value=mock):
|
||||
presence_gate.handle_stop({"cwd": str(branch_dir)})
|
||||
mock.release.assert_called_once_with("skills")
|
||||
presence_gate.handle_stop({})
|
||||
mock.release.assert_not_called()
|
||||
mock.claim.assert_not_called()
|
||||
|
||||
Reference in New Issue
Block a user