Skip to content

fix(claude): extend Stop hook timeout and apply headless opts in SDK query - #713

Merged
lavaman131 merged 1 commit into
mainfrom
fix/claude-stop-hook-timeout
Apr 22, 2026
Merged

fix(claude): extend Stop hook timeout and apply headless opts in SDK query#713
lavaman131 merged 1 commit into
mainfrom
fix/claude-stop-hook-timeout

Conversation

@lavaman131

@lavaman131 lavaman131 commented Apr 22, 2026

Copy link
Copy Markdown
Collaborator

Summary

Fixes two related issues in the Claude SDK provider: the Stop hook being killed during long TaskCompleted/TeammateIdle phases, and headless SDK queries incorrectly ignoring the auto-deny options for AskUserQuestion.

Key Changes

  • Stop hook timeout (WORKFLOW_HOOK_SETTINGS): Introduces STOP_HOOK_TIMEOUT_SECONDS = 2_147_483 (~24 days, the max-safe setTimeout value in seconds) and passes it as timeout on the Stop hook entry. Claude Code's Stop hook process runs three phases sequentially — Stop hooks, then TaskCompleted hooks per in-progress task, then TeammateIdle hooks — and the per-hook timeout applies to the whole lifecycle. The prior default (10 min) could kill the hook mid-run, severing the _claude-stop-hook queue/release poll and stranding the workflow. waitForIdle still resolves on the marker-file write, not on timer expiry, so real hook completion is unaffected.

  • Headless SDK query options (HeadlessClaudeSessionWrapper.query): The sdkQuery call was passing options ?? {} instead of the constructed headlessSdkOpts, so the AskUserQuestion auto-deny (disallowedTools merge) was silently dropped on every headless run. Now passes headlessSdkOpts so the tool is correctly blocked and headless queries cannot stall waiting for a human response.

…eammateIdle phases

The default 10-minute timeout covers all three sequential hook phases,
so turns with in-progress tasks can get killed and strand the workflow's
queue/release poll. Use the max-safe setTimeout value (~24 days) to
effectively remove the timeout; waitForIdle still fires on the marker
file write, not timer expiry.
@lavaman131
lavaman131 merged commit b9a233a into main Apr 22, 2026
4 checks passed
@lavaman131
lavaman131 deleted the fix/claude-stop-hook-timeout branch April 22, 2026 00:01
@claude claude Bot changed the title fix(claude): extend Stop hook timeout to survive long TaskCompleted/T… fix(claude): extend Stop hook timeout and apply headless opts in SDK query Apr 22, 2026
@claude

claude Bot commented Apr 22, 2026

Copy link
Copy Markdown

Code Review — PR #713

Nice, tightly scoped fix. The reasoning about the sequential Stop → TaskCompleted → TeammateIdle phases sharing a single timeout budget is well captured in the docstring, and pointing the reader at waitForIdle's marker-file watch as the real completion signal is exactly the context a future reader will need.

Correctness

  • Constant value checks out. setTimeout's max safe ms value is 2**31 - 1 = 2_147_483_647 ms ≈ 2_147_483.647 s, so 2_147_483 s is the right floor. Claude Code likely computes timeoutSeconds * 1000 internally, and 2_147_483 * 1000 = 2_147_483_000 ms stays under the limit — no overflow risk.
  • Unit is correct. docs/claude-code/cli/hooks.md:756 confirms the timeout field is in seconds, matching the constant's units.
  • Timeout removal is safe in practice. claudeStopHookCommand already caps itself with DEFAULT_WAIT_TIMEOUT_MS = 15 * 60 * 1000 (src/commands/cli/claude-stop-hook.ts:81), so an effectively-unbounded Claude-side timeout doesn't actually let the hook run for 24 days — it just defers the kill switch to our own internal deadline. Worth a sentence in the docstring IMO: readers seeing a 24-day timeout may reasonably worry about zombie processes, and the answer ("we have our own bounded poll") lives in another file.

Scope drift (flagging, not blocking)

The diff also changes line 1099:

-for await (const msg of sdkQuery({ prompt, options: options ?? {} })) {
+for await (const msg of sdkQuery({ prompt, options: headlessSdkOpts })) {

This is a legitimate and probably important bug fix — headlessSdkOpts was being constructed (with disallowedTools: [\"AskUserQuestion\"]) and then thrown away, so headless runs could still hang on AskUserQuestion tool calls despite the comment above saying they wouldn't. But it's unrelated to the Stop-hook timeout and isn't called out in the title, body, or commit message. Two options:

  1. Split into its own commit/PR with a dedicated message so git blame/git log tell the story accurately.
  2. Amend the commit message to mention both fixes.

Either works — the combined PR is fine, just discoverability suffers.

Other notes

  • Tests. No new tests, which is reasonable — the timeout value is consumed by Claude Code's hook runner, not by anything in this repo, so there's nothing to assert against without mocking the CLI. The headless disallowedTools fix is technically testable (spy on sdkQuery and assert the options it's called with), but the existing test surface here doesn't seem to go that deep.
  • Style. Matches the surrounding file conventions: underscored numeric literal, pointed docstring, @link reference from the settings-block doc back to the constant. Good.
  • Security/perf. No concerns. The timeout is a passive config value; no new I/O, no new escape surface in the settings JSON (still no single quotes).

Verdict

LGTM on the primary change. Consider the docstring nit (mentioning the internal 15-min poll cap) and clarify the scope of the headless-options fix in history before/after merge.

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
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
…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.
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