Skip to content

fix(claude): align stop-hook timeout with Claude-side bound and add PID liveness check - #717

Merged
lavaman131 merged 2 commits into
mainfrom
prerelease/v0.5.28-1
Apr 22, 2026
Merged

fix(claude): align stop-hook timeout with Claude-side bound and add PID liveness check#717
lavaman131 merged 2 commits into
mainfrom
prerelease/v0.5.28-1

Conversation

@lavaman131

@lavaman131 lavaman131 commented Apr 22, 2026

Copy link
Copy Markdown
Collaborator

Summary

Fixes a session-stranding bug where the _claude-stop-hook had a 15-minute wait budget that expired long before the Claude-side hook timeout (~24 days), causing enqueuePrompt to write to a file no one was reading. Also adds an atomic-PID liveness check so the hook can self-exit if atomic is SIGKILL'd without running teardown.

Key Changes

  • Unified wait timeout: Raise DEFAULT_WAIT_TIMEOUT_MS from 15 minutes to 2_147_483_000 ms (~24 days) to match STOP_HOOK_TIMEOUT_SECONDS, eliminating the mismatch between hook-side and Claude-side wait bounds.
  • fs.watch-based delivery: Replace the 100 ms existsSync poll loop with fs.watch watchers on the queue and release directories for near-zero latency prompt delivery, with a slower polling fallback for dropped inotify/FSEvent notifications.
  • Atomic PID liveness check: createClaudeSession now writes process.pid to ~/.atomic/claude-pid/<session_id>; the stop hook polls process.kill(pid, 0) every 5s. If atomic is SIGKILL'd, the hook detects the dead PID and self-exits instead of parking Claude for the full timeout. clearClaudeSession unlinks the pid file on graceful shutdown. Missing pid files are tolerated (liveness is skipped).
  • abortableSleep: New helper that resolves immediately on AbortSignal abort, allowing any one winning wait task to cancel its siblings instantly.
  • Tests: Updated claude-stop-hook.test.ts to cover the new pid directory, and added a test verifying that a dead atomic PID triggers liveness exit before the full wait timeout.

Files Changed

  • src/commands/cli/claude-stop-hook.ts — core wait loop rewrite with watcher, liveness check, and timeout alignment
  • src/commands/cli/claude-stop-hook.test.ts — updated cleanup and new liveness test
  • src/sdk/providers/claude.tswriteAtomicPidFile / unlinkAtomicPidFile integrated into session lifecycle

…add liveness check

The workflow's `_claude-stop-hook` had its own 15-minute wait budget for the
queue/release poll loop — even after #713 extended the Claude-side hook
timeout to ~24 days, a turn with no follow-up prompt inside 15 min would
strand the session: the hook returned 0, Claude stopped, and the next
`enqueuePrompt` wrote to a file nobody was reading.

- Raise `DEFAULT_WAIT_TIMEOUT_MS` to match `STOP_HOOK_TIMEOUT_SECONDS` so
  both bounds are unified.
- Replace the 100ms `existsSync` poll with `fs.watch` on the queue/release
  dirs (plus a slower existsSync fallback for dropped inotify events), so
  the next-turn prompt is delivered with ~0 latency.
- Add an atomic-PID liveness check: `createClaudeSession` now writes
  `~/.atomic/claude-pid/<session_id>` containing `process.pid`, and the
  hook polls `process.kill(pid, 0)` every 5s. If atomic is SIGKILL'd
  without running teardown, the hook self-exits instead of parking Claude
  for the full 24-day budget. `clearClaudeSession` unlinks the pid file
  on graceful shutdown. Missing pid files are tolerated (liveness is
  skipped), so non-runtime hook invocations still work.
- Add `abortableSleep` so the cooperating wait tasks cancel immediately
  when any one of them detects a hit.
@lavaman131
lavaman131 merged commit e4dac31 into main Apr 22, 2026
4 checks passed
@lavaman131
lavaman131 deleted the prerelease/v0.5.28-1 branch April 22, 2026 02:29
@claude claude Bot changed the title Prerelease/v0.5.28 1 fix(claude): align stop-hook timeout with Claude-side bound and add PID liveness check Apr 22, 2026
@claude

claude Bot commented Apr 22, 2026

Copy link
Copy Markdown

Code Review — PR #717

Nice hardening of the Stop hook. The liveness signal closes a real hole (workflow SIGKILL leaving the hook parked for 24 days), and the fs.watch upgrade should materially cut follow-up turn latency. A few notes below.

✅ What's working well

  • Design doc in codeDEFAULT_WAIT_TIMEOUT_MS and the 5. Wait for either a queued follow-up prompt or a release signal block explain the why at every surprising point (24-day bound, EPERM handling, tmp+rename avoidance). Future maintainers will be able to reason about edge cases without archaeology.
  • Abortable cooperating tasksabortableSleep + AbortController means the first task to see a hit cancels the rest instantly, rather than letting polls/liveness checks race to their next tick. Clean pattern.
  • Backward compatibilityatomicPid === null → return in runLivenessCheck means older runtimes (no pid file) still work, so the hook can be shipped ahead of the writer side without a flag day.

🐛 Potential issues

1. createClaudeSession isn't best-effort despite the comment claiming so (src/sdk/providers/claude.ts:268-271)

// Best-effort; failures just mean the hook falls back to waiting out Claude's own hook timeout.
await writeAtomicPidFile(claudeSessionId);

The comment says failures are tolerated, but the call is unwrapped — an EACCES/ENOSPC on ~/.atomic/claude-pid/ would throw out of createClaudeSession and fail session setup entirely. Either wrap in try/catch (matching the intent) or drop the "best-effort" framing. Wrapping is safer since the hook degrades gracefully when the pid file is missing.

2. PID reuse false-positives (claude-stop-hook.ts:173-186)
If atomic is SIGKILL'd and the OS recycles its PID to an unrelated process before the hook's liveness interval fires, isProcessAlive will return true and the hook will stay parked until the 24-day timeout. Not dangerous (the big timeout is still there), but worth noting — and trivially mitigated by also stat'ing the pid file and checking process.pid's start time, or by writing pid:starttime to the file. Acceptable for now given the Linux PID space.

3. Tiny watcher-attach race on runWatcher (claude-stop-hook.ts:337-350)
The initial synchronous check() on line 321 is what prevents this from being a real bug, but strictly speaking the for await doesn't guarantee the watcher is attached before the next line runs — watchDir returns an async iterable, and the underlying inotify subscription goes live when iteration starts. If the runtime writes the queue file in the tiny window between check() returning null and watchDir's first next(), the poll fallback (100ms default) catches it. So it's fine, just a reason to keep pollIntervalMs finite.

4. Dead-PID scan in the test can be slow under load (claude-stop-hook.test.ts:287-296)
The loop scans from 4_194_303 downward hitting process.kill(pid, 0) on each iteration. On a busy machine with many live PIDs between the starting number and a free one, this iterates synchronously and could stall. Consider starting from 4_194_303 and jumping by a larger step, or using a fixed well-known-unused PID (e.g., pick a random one in the upper range and try a handful). Low-priority since most CI machines have sparse high PIDs.

🔐 Security

  • PID file contents: parsed via Number.parseInt with an Number.isInteger && > 0 guard — injection-safe.
  • Session-id-based paths: UUIDs from crypto.randomUUID(), no traversal risk.
  • World-readable ~/.atomic/claude-pid/ is fine; the PID is already visible via ps.

🧪 Test coverage

Good addition of test #9 for the dead-PID path. Gaps worth considering:

  • Live atomic PID → hook waits the full timeout. The symmetric negative case: write String(process.pid) to the pid file and assert the hook times out normally. Right now, a bug flipping the ESRCH/EPERM branches would still pass test Lavaman131/feature/24 7 agent #9.
  • No pid file → liveness is skipped. Covered implicitly by tests 1-8, but an explicit assertion would lock it in.
  • Malformed pid file content ("garbage", empty string) → readAtomicPid returns null and the hook behaves as if no file exists. Easy test, good regression guard.
  • Watcher-driven wake vs poll-driven wake. Test updated readme #7 uses pollIntervalMs: 25 so you can't tell whether the watcher or the poll won. Setting pollIntervalMs: 10_000 would force the assertion to fail unless fs.watch fired — that's the whole point of this PR for turn latency.

📝 Nits

  • import { watch as watchDir } from \"node:fs/promises\" on line 32 of claude-stop-hook.ts: there's no name collision in this file, so the alias is decorative. Could just be import { watch } from \"node:fs/promises\", or fold into the existing import fs from \"node:fs/promises\" as fs.watch(...).
  • pidDir() / pidFilePath() in claude.ts:624-632 are thin wrappers used only by two callers in the same file — could inline, but also fine to keep for symmetry with queueDir/releasePath.

Summary

Overall a solid fix with thoughtful fallback layering (watcher → poll → liveness → 24-day ceiling). Primary ask: wrap writeAtomicPidFile in createClaudeSession so the "best-effort" comment is actually true. Secondary ask: add a live-PID test so the liveness direction is pinned down.

lavaman131 added a commit that referenced this pull request Apr 22, 2026
* fix(claude): align stop-hook wait bound with Claude-side timeout and add liveness check

The workflow's `_claude-stop-hook` had its own 15-minute wait budget for the
queue/release poll loop — even after #713 extended the Claude-side hook
timeout to ~24 days, a turn with no follow-up prompt inside 15 min would
strand the session: the hook returned 0, Claude stopped, and the next
`enqueuePrompt` wrote to a file nobody was reading.

- Raise `DEFAULT_WAIT_TIMEOUT_MS` to match `STOP_HOOK_TIMEOUT_SECONDS` so
  both bounds are unified.
- Replace the 100ms `existsSync` poll with `fs.watch` on the queue/release
  dirs (plus a slower existsSync fallback for dropped inotify events), so
  the next-turn prompt is delivered with ~0 latency.
- Add an atomic-PID liveness check: `createClaudeSession` now writes
  `~/.atomic/claude-pid/<session_id>` containing `process.pid`, and the
  hook polls `process.kill(pid, 0)` every 5s. If atomic is SIGKILL'd
  without running teardown, the hook self-exits instead of parking Claude
  for the full 24-day budget. `clearClaudeSession` unlinks the pid file
  on graceful shutdown. Missing pid files are tolerated (liveness is
  skipped), so non-runtime hook invocations still work.
- Add `abortableSleep` so the cooperating wait tasks cancel immediately
  when any one of them detects a hit.

* chore(release): bump version to v0.5.28-1
lavaman131 added a commit that referenced this pull request Apr 22, 2026
* fix(claude): align stop-hook wait bound with Claude-side timeout and add liveness check

The workflow's `_claude-stop-hook` had its own 15-minute wait budget for the
queue/release poll loop — even after #713 extended the Claude-side hook
timeout to ~24 days, a turn with no follow-up prompt inside 15 min would
strand the session: the hook returned 0, Claude stopped, and the next
`enqueuePrompt` wrote to a file nobody was reading.

- Raise `DEFAULT_WAIT_TIMEOUT_MS` to match `STOP_HOOK_TIMEOUT_SECONDS` so
  both bounds are unified.
- Replace the 100ms `existsSync` poll with `fs.watch` on the queue/release
  dirs (plus a slower existsSync fallback for dropped inotify events), so
  the next-turn prompt is delivered with ~0 latency.
- Add an atomic-PID liveness check: `createClaudeSession` now writes
  `~/.atomic/claude-pid/<session_id>` containing `process.pid`, and the
  hook polls `process.kill(pid, 0)` every 5s. If atomic is SIGKILL'd
  without running teardown, the hook self-exits instead of parking Claude
  for the full 24-day budget. `clearClaudeSession` unlinks the pid file
  on graceful shutdown. Missing pid files are tolerated (liveness is
  skipped), so non-runtime hook invocations still work.
- Add `abortableSleep` so the cooperating wait tasks cancel immediately
  when any one of them detects a hit.

* chore(release): bump version to v0.5.28-1
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