fix(heartbeat): evict wedged SDK subprocesses, never deadlock the lock by claudiusthebot · Pull Request #144 · dylanneve1/talon

claudiusthebot · 2026-05-11T22:32:11Z

Why

Production heartbeat #208 finished its work at 04:52Z on 2026-05-10 (the Marrow PR #26 hour) but the SDK subprocess never exited. PID 2221549 sat there with the heartbeat lock held for 17h 31m until Dylan flagged it at 22:22Z and I SIGTERMed it manually. No subsequent heartbeats fired in that entire window.

Root cause was line 337 of the old runHeartbeatAgent:

// On timeout, wait for the agent to actually finish before releasing the lock
// to prevent overlapping heartbeat runs
await agentPromise.catch(() => {});
throw err;

Combined with the stale comment two lines up — "the Agent SDK does not expose an abort mechanism" — the code chose to wait forever rather than risk an overlap. But the SDK does expose an abort mechanism (abortController in query options), and "wait forever" was the worse outcome.

What this PR does

Pass an AbortController to query() options. When the heartbeat timeout fires, controller.abort() signals the SDK to tear down cleanly.
Bounded grace window after abort. The agent promise gets HEARTBEAT_ABORT_GRACE_MS (default 30 s) to settle. If it doesn't, we release the lock anyway — the next interval can fire.
Fallback orphan sweep. evictOrphanHeartbeatSubprocesses() reads /proc/*/environ for any process carrying TALON_CHAT_ID=heartbeat (set by Talon's MCP launcher when spawning heartbeat-tier subprocesses), SIGTERMs them, waits SUBPROCESS_KILL_GRACE_MS (default 5 s), then SIGKILLs survivors. Runs fire-and-forget so it doesn't block the next heartbeat. Linux-only — macOS/Windows rely on SDK abort + grace alone.
All three timing constants are env-overridable (TALON_HEARTBEAT_TIMEOUT_MS, TALON_HEARTBEAT_ABORT_GRACE_MS, TALON_HEARTBEAT_SUBPROCESS_KILL_GRACE_MS) so tests can use 50 ms windows instead of 10-minute ones.
Stale comment deleted.

Tests

Five new vitest cases in src/__tests__/heartbeat.test.ts:

passes an AbortController to the SDK query() options — happy-path verification
calls AbortController.abort() when the agent hangs past the timeout — captures the signal, asserts signal.aborted === true after timeout
releases the running lock even when the SDK ignores abort — the canonical regression test. Mock SDK returns an async generator that never settles even after abort. Asserts forceHeartbeat() rejects with "Heartbeat agent timed out", then a second forceHeartbeat() immediately after resolves cleanly. Before this PR, the second call would deadlock.
evictOrphanHeartbeatSubprocesses no-op on non-Linux
evictOrphanHeartbeatSubprocesses end-to-end on Linux — mocks /proc, mocks process.kill, verifies found/termed/killed counts

Test plan

npx vitest run src/__tests__/heartbeat.test.ts — 25/25 pass
Full suite — 1796/1809 pass (1 pre-existing package.functional.test.ts failure unrelated, 12 skipped live-tier)
tsc --noEmit clean
prettier --check clean
npm run lint — 0 errors in changed files
CI green
After merge + restart: wedge an SDK manually (e.g. kill -STOP on a heartbeat subprocess) and watch the next timeout trigger the eviction path in logs

Won't fix in this PR

Doesn't change the 10-minute soft timeout. That's a separate question (PR feat(claude-sdk): Notification + StopFailure hooks for SDK telemetry #128 would add observability via the SDK Notification/StopFailure hooks).
Doesn't add a lastFailureKind field to heartbeat state. The journal-level signal is enough for now; a structured state field is overkill.

🤖 Generated with Claude Code

Copilot

Pull request overview

This PR hardens the heartbeat runner to avoid deadlocking the “heartbeat already running” guard when the Claude Agent SDK subprocess wedges, by adding SDK cancellation, bounded post-timeout grace, and a Linux /proc-based orphan sweep fallback.

Changes:

Add env-overridable timing constants and pass an AbortController into query() options; abort on timeout.
After timeout, wait a bounded grace period for the agent promise to settle; if it doesn’t, release the lock and trigger an orphan subprocess sweep in the background.
Add vitest coverage for abort-on-timeout, lock release even when abort is ignored, and Linux/non-Linux orphan sweep behavior.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File	Description
src/core/heartbeat.ts	Adds abort + grace logic to prevent lock deadlocks and introduces Linux orphan-subprocess eviction helper.
src/tests/heartbeat.test.ts	Adds tests covering abort wiring, timeout behavior, lock release regression, and orphan sweep.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

+  // Timeout that requests eviction (graceful first, force-kill on grace exit).
+  let timeoutFired = false;
  let timeoutHandle: ReturnType<typeof setTimeout> | null = null;
  const timeoutPromise = new Promise<never>((_, reject) => {
-    const t = setTimeout(
-      () => reject(new Error("Heartbeat agent timed out")),
-      HEARTBEAT_TIMEOUT_MS,
-    );
+    const t = setTimeout(() => {
+      timeoutFired = true;
+      try {
+        abortController.abort();
+      } catch {
+        /* ignore */
+      }
+      reject(new Error("Heartbeat agent timed out"));
+    }, HEARTBEAT_TIMEOUT_MS);
    t.unref(); // Don't prevent Node.js from exiting cleanly during shutdown
    timeoutHandle = t;
  });


+/**
+ * Race a promise against a timeout. Returns the promise's value on success,
+ * or the sentinel `"timed_out"` if the timeout fires first. Never throws.
+ */
+async function raceWithTimeout<T>(
+  p: Promise<T>,
+  ms: number,
+): Promise<T | "timed_out"> {


+      // /proc/<pid>/environ is NUL-delimited. Looking for our heartbeat marker.
+      if (environRaw.includes("TALON_CHAT_ID=heartbeat")) {


Three surgical fixes responding to the Copilot review at 12:35Z: 1. **timeoutFired race in catch path** (src/core/heartbeat.ts:367) The flag could flip to true during the `await appendHeartbeatLog` inside the catch — a non-timeout agent rejection would then be misclassified as a timeout and trigger a spurious abort + orphan sweep. Fix: snapshot the flag (`wasTimeout`) and clear the timer IMMEDIATELY at catch entry, before any awaits. The `finally` clear stays as a safety net for the success path. 2. **raceWithTimeout JSDoc** (src/core/heartbeat.ts:419) Doc claimed "Never throws" but the helper does throw if `p` rejects before the timeout. Updated the doc to reflect actual behaviour and note that callers needing a never-throwing race should `.catch()` the input themselves (as the eviction path already does). 3. **/proc/<pid>/environ substring false-positive** (src/core/heartbeat.ts:467) Raw `.includes("TALON_CHAT_ID=heartbeat")` would false-positive on any process whose env carries that substring as part of another var's value (e.g. `OTHER_VAR=TALON_CHAT_ID=heartbeat`) and SIGKILL it. Fix: split the environ on \0 and match exact entries. Tests - New regression case in heartbeat.test.ts: PID 3 with `OTHER_VAR=TALON_CHAT_ID=heartbeat\0FOO=bar` must NOT be matched (asserts both result counts and that killSpy was never called with pid 3). The old substring check would have flagged it. - Existing 24 tests + the new strengthened assertion → 25/25 pass. - `tsc --noEmit` clean. `prettier --check` clean. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

…age (#151) * fix(heartbeat): frontend-agnostic outbound-messaging prompt PR #150 introduced an outbound-messaging block in the heartbeat system prompt that hard-codes `telegram-tools`, "Telegram ID", and Telegram-only examples. Talon supports multiple frontends — including running multiple at once — and the heartbeat agent gets one `${frontend}-tools` MCP server spawned per non-terminal frontend via `buildMcpServers`. The hard-coding in the prompt misleads heartbeats running on non-Telegram deployments. What changes - New `getActiveFrontends()` helper in `backend/claude-sdk/options.ts`, exported via the barrel. Returns the list of non-terminal frontends — same filter `buildMcpServers` was using inline, now de-duplicated. - New `buildHeartbeatSystemPrompt()` in `core/heartbeat.ts` (exported for tests). Builds the prompt dynamically based on `getActiveFrontends()`: * Zero non-terminal frontends → base prompt only, no outbound block, no `chat_id` mention. Terminal-only deployments don't have an outbound messaging surface so the section would be confusing. * One or more frontends → lists every `${frontend}-tools` server by name in the OUTBOUND MESSAGING block, uses the first frontend in the example `send(...)` call, refers to chat IDs as "per-frontend" (Telegram-specific guidance moved to memory.md territory). - Falls back to the base prompt if `getActiveFrontends()` throws (test paths where the agent config isn't initialised) — same try/catch pattern as the `buildMcpServers` call in #150. The hard-coded Telegram chat_id (`352042062`) is no longer in the system prompt — that lives in memory.md anyway. Other deployments' user IDs are unaffected. Tests 7 new vitest cases in `src/__tests__/heartbeat.test.ts`: - no frontends → base only, no `OUTBOUND MESSAGING`, no `telegram-tools` - config throws → base only - `telegram` only → references `telegram-tools`, not `teams-tools` - `teams` only → references `teams-tools`, not `telegram-tools` - multi-frontend (`telegram` + `teams`) → both listed - first frontend goes into the example send() call - no `chat_id` mention when no frontends are configured 27/27 heartbeat tests pass. Full suite 1821/1834 (1 pre-existing `package.functional` failure unrelated, 12 skipped live-tier). typecheck / prettier / lint all clean. Stacking note This branches off `main` (`77e7771`, post-#150). It does NOT include PR #144 (heartbeat-eviction). When both land, the second to merge will need a trivial rebase — different sections of `heartbeat.ts`, no real conflict. 🤖 Generated with [Claude Code](https://claude.com/claude-code) * test(heartbeat-outbound): comprehensive unit + integration coverage Expands the test surface around PR #150's heartbeat outbound flow and the frontend-agnostic prompt builder added earlier in this PR. Adds 22 new test cases across three files focused on the gaps the existing coverage left. claude-sdk-options.test.ts (+11) getActiveFrontends helper — 8 cases covering scalar/array config, terminal filtering, multi-frontend order preservation, throw propagation when config not initialised. buildMcpServers heartbeat-tier — 7 cases (3 new) covering the heartbeat sentinel (TALON_CHAT_ID="heartbeat"), per-frontend env isolation (no cross-pollination between telegram-tools and teams-tools env vars), multi-frontend + brave-search combos. bridge.test.ts (+6) Edge cases the original 6 didn't cover: - Negative numeric chat_id (Telegram supergroup IDs are negative — -100<id> — these are the most common heartbeat targets in practice; coercion bugs would have been silent). - chat_id=0 promoted correctly (gateway decides what to do). - chat_id=null forwarded as String(null)="null" (defensive; the schema validates upstream but the bridge mustn't crash). - Heartbeat sentinel default ("heartbeat") MUST be overridden when explicit chat_id is supplied — otherwise outbound from heartbeat would silently fail to route. - Heartbeat sentinel passed through verbatim when no explicit chat_id (gateway's rawChatId !== "heartbeat" guard catches it). - String chat_id with leading zeros preserved (no Number() coercion). gateway-http.test.ts (+5) Edge cases on the explicit-routing branch: - Negative chat_id (Telegram supergroup) routes without active context. - chat_id=0 rejected by the !chatId falsy guard. - Heartbeat sentinel as explicit chat_id rejected (rawChatId !== "heartbeat" guard). - Negative chat_id sign preserved through the handler — defensive against accidental Math.abs / int32 truncation. - Coexistence: explicit chat_id matching an active context still routes via the explicit-routing branch (no exception, no double-dispatch). Verification - npx vitest run — full suite 1848/1861 pass (1 pre-existing package.functional failure unrelated, 12 skipped live-tier) - The four directly-touched files (claude-sdk-options, bridge, gateway-http, heartbeat) account for 96 tests, all passing - tsc --noEmit clean - prettier --check clean - lint — 0 errors, 0 warnings on changed files Why these specifically: negative chat_id is the most subtle silent-bug surface (Telegram groups all have negative IDs, a sign-handling bug would route to the wrong chat or fail silently). The heartbeat sentinel guard is the routing-safety net (if it ever stopped working, heartbeats could spam unrelated chats). The getActiveFrontends helper is the multi-frontend correctness backbone for the new prompt builder. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

… on terminal, etc. Address 5 of the 6 Copilot review threads on PR #96. The 6th (mid-run TriggerStatus rename) is a larger semantic refactor and deferred to a later commit; this commit pairs the smallest reasonable fix at each site with regression tests where they're cleanly testable. 1. src/core/triggers.ts:128 — log WriteStream missing error handler. createWriteStream() with no .on("error") emits an unhandled EventEmitter `error` event if the log file becomes unwritable (disk full, permission flip), which crashes the Node process. Added a logError-routing handler so disk-tier failures degrade gracefully without killing the supervisor. 2. src/storage/trigger-store.ts:60 — fireCount doc/semantics mismatch. The doc-comment claimed "Number of mid-run TALON_FIRE: lines" but the supervisor increments fireCount for every wake (mid-run and terminal). Fixed the comment to match the actual semantics; same counter, accurate meaning. Avoids breaking any caller that already relies on fireCount as a total. 3. src/core/triggers.ts:fireWake — mid-run prompt says "Status: fired" even when terminal=false, which can mislead downstream handling into treating an in-flight watcher as a completed run. Decoupled the prompt-display status from the on-disk TriggerStatus enum: non-terminal fires now show "Status: signalled" in both the header and the "Status:" line. Terminal fires unchanged. 4. src/core/triggers.ts:bufferAsPayload + fireWake — FIRE_PAYLOAD_MAX_BYTES truncation used String.prototype.slice (UTF-16 code units), so a payload of N multi-byte characters could exceed the documented byte cap and split a character mid-codepoint. Introduced truncateUtf8Tail/ truncateUtf8Head helpers that encode to UTF-8 bytes, slice on a byte boundary, then walk over UTF-8 continuation bytes (10xxxxxx) so we never cut a multi-byte sequence in half. The byte cap is now actually enforced as bytes. 5. src/storage/trigger-store.ts:282 + supervisor — updateTrigger only marks the store dirty (10s autosave window). For terminal status transitions (errored/cancelled/timed_out/fired) a crash inside that window would leave on-disk status as "running", which loadTriggers() then misclassifies as "terminated by previous restart". Added persistNow() calls at the four supervisor transition sites: failTrigger, cancelTrigger, the hard-timeout handler, and the finalizeExit path. The on-disk store now always agrees with the in-memory state across terminal transitions. 6. src/core/gateway-actions.ts:trigger_create — always returned "Status: running" even when spawnTrigger() failed without throwing (unsupported language slipping past validation, child.pid undefined, failTrigger() routing). With #5 above, the store now reflects the real state synchronously, so we re-read getTrigger(id) after spawn and return ok:false + lastError when it landed in "errored", or surface the actual status string otherwise. Callers no longer get false success responses. Regression tests (triggers-extended.test.ts): - mid-run prompt asserts "Status: signalled" appears and "Status: fired" does NOT, on a still-running trigger. Covers fix #3. - multi-byte UTF-8 payload (2000× 💧 = 8000 bytes vs 4000 string length) asserts the prompt's byte length stays bounded AND contains no Unicode replacement character (U+FFFD), which would indicate a mid-codepoint split. Covers fix #4. The remaining Copilot thread (introducing a dedicated non-terminal status enum value, replacing the TriggerStatus|"signalled" display- status hack) is a wider type change with API/store implications; better tackled in a follow-up than bundled into a defensive sweep. Verification: - npx vitest run → 1747/1748 pass (the 1 failure is the pre-existing package.functional "Stopped" expectation that fails whenever a live Talon daemon is running on the host — same env-dependent failure noted in PR #144 and PR #90's verification blocks). - npx vitest run src/__tests__/triggers* src/__tests__/trigger-store* → 74/74 pass (35 in triggers-extended including 2 new, 22 in trigger-store, 17 in triggers). - npx tsc --noEmit → clean. - npx prettier --check on all 4 changed files → clean. - npm run lint → 0 errors in changed files (10 pre-existing warnings elsewhere unchanged). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

Four test failures in the PR #156 CI run all reflected old assertions that match the bugs the PR explicitly fixes: - cron-store: "does not write when not dirty" was asserting the very bug being fixed (flush should always persist defensively, matching flushSessions/flushHistory). Updated to assert writeFileSync IS called. - errors: "uses extracted status instead of 429 default" was relying on '200' being matched by the old [2-5]\d{2} regex — the whole point of the PR is to stop matching 2xx/3xx. Switched the example to 418 and added a regression test that 200ms in an error message no longer pollutes status. - dream (timeout fake-timers): the mocked query awaited a never-resolving Promise, so the new `await agentPromise.catch(() => {})` (matching the heartbeat.ts pattern that waits for the subprocess before releasing the lock) hung the test. Switched to a controllable promise and resolve it after the timeout fires to simulate the agent finishing post-timeout. - dream (executeDream catch): cascading failure from the same hang — vi.useRealTimers() at the end of the timeout test was unreachable, so fake timers leaked into the next test's `setTimeout(r, 0)`. Fixed by unhanging the timeout test. Plus prettier --write on formatting.ts and history.ts (the PR's edits nudged both files past the line-length limit). Verified locally: 1900/1901 pass (the one remaining failure is the pre-existing package.functional env-dependent flake, same as PR #144/#90/#96). typecheck clean, prettier --check clean on all touched files. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

… component, flush consistency, status regex) (#156) * fix: six correctness and safety bugs found by code review - dream.ts: capture and clear the 10-minute timeout handle in a finally block, and call .unref() on it. Without this the timer kept the Node.js event loop alive after a successful dream run and, when it eventually fired, rejected a Promise with no handler → UnhandledPromiseRejection warning. The fix mirrors the already-correct pattern in heartbeat.ts and additionally waits for the agent to finish before releasing the dreaming lock on timeout, preventing overlapping dream runs. - formatting.ts: HTML-escape the URL before embedding it in the <a href> attribute. A crafted markdown link such as [x](https://a.com" onX="y) could inject arbitrary HTML attributes into the generated anchor tag. - history.ts: fix two log() / logError() calls that passed "sessions" as the component instead of "history", causing history-load events to appear under the wrong tag in structured logs. - cron-store.ts / chat-settings.ts: add `dirty = true` before the final save() in flushCronJobs() and flushChatSettings() to guarantee the store is written on shutdown regardless of auto-save timer timing. Matches the defensive pattern already used by flushSessions() and flushHistory(). - errors.ts: narrow the HTTP status-code regex from /\b([2-5]\d{2})\b/ to /\b([45]\d{2})\b/. Only 4xx/5xx codes are acted on by the classifier; matching 2xx/3xx codes was harmless for control flow but could stamp a spurious status (e.g. 200 from "took 200ms") onto a TalonError, making diagnostic logs misleading. https://claude.ai/code/session_01Xq1CTjS2UG5ErEbqVaELRn * test+style: align tests with #156 corrected behaviour + prettier Four test failures in the PR #156 CI run all reflected old assertions that match the bugs the PR explicitly fixes: - cron-store: "does not write when not dirty" was asserting the very bug being fixed (flush should always persist defensively, matching flushSessions/flushHistory). Updated to assert writeFileSync IS called. - errors: "uses extracted status instead of 429 default" was relying on '200' being matched by the old [2-5]\d{2} regex — the whole point of the PR is to stop matching 2xx/3xx. Switched the example to 418 and added a regression test that 200ms in an error message no longer pollutes status. - dream (timeout fake-timers): the mocked query awaited a never-resolving Promise, so the new `await agentPromise.catch(() => {})` (matching the heartbeat.ts pattern that waits for the subprocess before releasing the lock) hung the test. Switched to a controllable promise and resolve it after the timeout fires to simulate the agent finishing post-timeout. - dream (executeDream catch): cascading failure from the same hang — vi.useRealTimers() at the end of the timeout test was unreachable, so fake timers leaked into the next test's `setTimeout(r, 0)`. Fixed by unhanging the timeout test. Plus prettier --write on formatting.ts and history.ts (the PR's edits nudged both files past the line-length limit). Verified locally: 1900/1901 pass (the one remaining failure is the pre-existing package.functional env-dependent flake, same as PR #144/#90/#96). typecheck clean, prettier --check clean on all touched files. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> --------- Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: Claudius <claudiusthebot@gmail.com>

Three surgical fixes responding to the Copilot review at 12:35Z: 1. **timeoutFired race in catch path** (src/core/heartbeat.ts:367) The flag could flip to true during the `await appendHeartbeatLog` inside the catch — a non-timeout agent rejection would then be misclassified as a timeout and trigger a spurious abort + orphan sweep. Fix: snapshot the flag (`wasTimeout`) and clear the timer IMMEDIATELY at catch entry, before any awaits. The `finally` clear stays as a safety net for the success path. 2. **raceWithTimeout JSDoc** (src/core/heartbeat.ts:419) Doc claimed "Never throws" but the helper does throw if `p` rejects before the timeout. Updated the doc to reflect actual behaviour and note that callers needing a never-throwing race should `.catch()` the input themselves (as the eviction path already does). 3. **/proc/<pid>/environ substring false-positive** (src/core/heartbeat.ts:467) Raw `.includes("TALON_CHAT_ID=heartbeat")` would false-positive on any process whose env carries that substring as part of another var's value (e.g. `OTHER_VAR=TALON_CHAT_ID=heartbeat`) and SIGKILL it. Fix: split the environ on \0 and match exact entries. Tests - New regression case in heartbeat.test.ts: PID 3 with `OTHER_VAR=TALON_CHAT_ID=heartbeat\0FOO=bar` must NOT be matched (asserts both result counts and that killSpy was never called with pid 3). The old substring check would have flagged it. - Existing 24 tests + the new strengthened assertion → 25/25 pass. - `tsc --noEmit` clean. `prettier --check` clean. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

… on terminal, etc. Address 5 of the 6 Copilot review threads on PR #96. The 6th (mid-run TriggerStatus rename) is a larger semantic refactor and deferred to a later commit; this commit pairs the smallest reasonable fix at each site with regression tests where they're cleanly testable. 1. src/core/triggers.ts:128 — log WriteStream missing error handler. createWriteStream() with no .on("error") emits an unhandled EventEmitter `error` event if the log file becomes unwritable (disk full, permission flip), which crashes the Node process. Added a logError-routing handler so disk-tier failures degrade gracefully without killing the supervisor. 2. src/storage/trigger-store.ts:60 — fireCount doc/semantics mismatch. The doc-comment claimed "Number of mid-run TALON_FIRE: lines" but the supervisor increments fireCount for every wake (mid-run and terminal). Fixed the comment to match the actual semantics; same counter, accurate meaning. Avoids breaking any caller that already relies on fireCount as a total. 3. src/core/triggers.ts:fireWake — mid-run prompt says "Status: fired" even when terminal=false, which can mislead downstream handling into treating an in-flight watcher as a completed run. Decoupled the prompt-display status from the on-disk TriggerStatus enum: non-terminal fires now show "Status: signalled" in both the header and the "Status:" line. Terminal fires unchanged. 4. src/core/triggers.ts:bufferAsPayload + fireWake — FIRE_PAYLOAD_MAX_BYTES truncation used String.prototype.slice (UTF-16 code units), so a payload of N multi-byte characters could exceed the documented byte cap and split a character mid-codepoint. Introduced truncateUtf8Tail/ truncateUtf8Head helpers that encode to UTF-8 bytes, slice on a byte boundary, then walk over UTF-8 continuation bytes (10xxxxxx) so we never cut a multi-byte sequence in half. The byte cap is now actually enforced as bytes. 5. src/storage/trigger-store.ts:282 + supervisor — updateTrigger only marks the store dirty (10s autosave window). For terminal status transitions (errored/cancelled/timed_out/fired) a crash inside that window would leave on-disk status as "running", which loadTriggers() then misclassifies as "terminated by previous restart". Added persistNow() calls at the four supervisor transition sites: failTrigger, cancelTrigger, the hard-timeout handler, and the finalizeExit path. The on-disk store now always agrees with the in-memory state across terminal transitions. 6. src/core/gateway-actions.ts:trigger_create — always returned "Status: running" even when spawnTrigger() failed without throwing (unsupported language slipping past validation, child.pid undefined, failTrigger() routing). With #5 above, the store now reflects the real state synchronously, so we re-read getTrigger(id) after spawn and return ok:false + lastError when it landed in "errored", or surface the actual status string otherwise. Callers no longer get false success responses. Regression tests (triggers-extended.test.ts): - mid-run prompt asserts "Status: signalled" appears and "Status: fired" does NOT, on a still-running trigger. Covers fix #3. - multi-byte UTF-8 payload (2000× 💧 = 8000 bytes vs 4000 string length) asserts the prompt's byte length stays bounded AND contains no Unicode replacement character (U+FFFD), which would indicate a mid-codepoint split. Covers fix #4. The remaining Copilot thread (introducing a dedicated non-terminal status enum value, replacing the TriggerStatus|"signalled" display- status hack) is a wider type change with API/store implications; better tackled in a follow-up than bundled into a defensive sweep. Verification: - npx vitest run → 1747/1748 pass (the 1 failure is the pre-existing package.functional "Stopped" expectation that fails whenever a live Talon daemon is running on the host — same env-dependent failure noted in PR #144 and PR #90's verification blocks). - npx vitest run src/__tests__/triggers* src/__tests__/trigger-store* → 74/74 pass (35 in triggers-extended including 2 new, 22 in trigger-store, 17 in triggers). - npx tsc --noEmit → clean. - npx prettier --check on all 4 changed files → clean. - npm run lint → 0 errors in changed files (10 pre-existing warnings elsewhere unchanged). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

* feat(triggers): self-authored watcher scripts that wake the bot Triggers are bot-authored long-running scripts (bash / python / node) that run as supervised subprocesses and signal back via stdout to fire wake-up messages into the originating chat. Built for the case where a fixed cron schedule doesn't fit — "wake me when this PR merges", "tell me if BTC moves >5%", "watch this URL until it returns 200". Contract: - Mid-run: a stdout line `TALON_FIRE: <text>` fires immediately and the script keeps running (multi-event watchers). - Exit 0: final fire with the tail of stdout/stderr as payload. - Exit non-zero: error fire with exit code + log tail. - Hard timeout (default 24h, max 7d): SIGTERM → SIGKILL → timed_out fire. Tools exposed: trigger_create, trigger_list, trigger_cancel, trigger_logs, trigger_delete. Per-chat cap of 5 active. Children are killed on Talon shutdown — they do not survive a restart, and any trigger left in running/pending on load is reaped to "terminated". New modules: - src/storage/trigger-store.ts: persistence + validation - src/core/triggers.ts: supervisor (spawn, line buffers, fire dispatch) - src/core/tools/triggers.ts: 5 tool definitions - gateway-actions.ts: trigger_* bridge handlers - paths.ts: ~/.talon/data/trigger-runs/<chatId>/ for scripts and logs - bootstrap.ts: loadTriggers + initTriggers + resumeAfterRestart - index.ts: shutdownTriggers + flushTriggers in shutdown paths Tests: 30 new (22 store + 8 supervisor including real-bash spawn integration). 1397/1397 passing. * style: auto-format with prettier * test(triggers): branch coverage for python/node/idempotency/resumeAfterRestart/store Adds targeted tests that bring branch coverage above the 60% global threshold imposed by PR #121. Exercises: commandForLanguage python & node paths, spawnTrigger idempotency guard, cancelTrigger false-return, resumeAfterRestart (no-deps / empty / matching / already-fired / old), trigger-store unknown-id paths, FIRE_PAYLOAD_MAX_BYTES truncation, and finalizeExit status-branch (cancelled exit + non-zero exit code). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * test(triggers): fix Windows path separator + add missing branch coverage Two CI failures in the coverage-fix commit (9096518): 1. Windows N22 — trigger-store.test.ts: triggerScriptPath regex used Unix forward-slashes but resolve() returns backslashes on Windows. Fixed by normalising path separators before matching. 2. Ubuntu N22 — branch coverage 59.67% < 60% (threshold differs from N24 due to V8 v11 vs v12 branch instrumentation). Added 4 more targeted tests in triggers-extended.test.ts: - trigger-store persistNow() path (lines 159-160) - readTriggerLogTail catch path via EISDIR (line 329) - readTriggerLogTail truncated:true path (line 327) - fireWakeUp dispatch-error catch path (line 394 of triggers.ts) All 18 extended tests pass. typecheck ✅ · format:check ✅. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * test(triggers): add N22 branch coverage for TALON_FIRE, empty-output, and store helpers Ubuntu Node 22 (V8 v11) instruments more branches than Node 24 (V8 v12), causing coverage to land at 59.67% on N22 while passing on N24. Added 7 new tests to triggers-extended.test.ts: - Empty stdout: covers fireWake's `trimmed ? ... : (no output)` false branch - Mid-run TALON_FIRE: covers handleStdoutLine true branch + fireWake terminal=false ("signalled" header path) - validateLanguage false paths: includes() false + typeof short-circuit - sanitizeChatId with special chars: confirms replace() regex path - languageExtension direct call: covers all 3 switch arms explicitly - getTriggerByName returning undefined: covers find() undefined path Also imports getTriggerByName, validateLanguage, sanitizeChatId, and languageExtension from trigger-store for direct testing. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * test(triggers): comprehensive branch coverage round 4 — target Node 22 CI pass Add 15 new tests across 4 test files to cover previously-unreachable branches, specifically the ones V8 v11 (Node 22) counts but were uncovered since hb #179: trigger-store.test.ts (+9 tests): - loadTriggers: file-not-present, non-object JSON, corrupt primary (3 variants) - Backup parses as null (line 94 ternary false arm) - save() catch: Error throw and non-Error string throw (both instanceof arms) - readTriggerLogTail: non-Error throw (false arm of err instanceof Error) triggers-extended.test.ts (+5 new describe blocks): - fireWake with undefined payload (payload ?? '' false arm) - fireCount undefined treated as 0 (fireCount ?? 0 false arm) - finalizeExit with null exit code — signal kill (code ?? undefined false arm) - handleStdoutLine with no lineBuffer entry (pushBufferLine if(!buf) true arm) - shutdownTriggers when no children running (if(children.size===0) true arm) - Child process error event handler (line 152 handler covered) - Timeout timer fires after child already exited (if(!c) return true arm) cleanup-registry.test.ts (+1 test): - Handler throws a non-Error string (err instanceof Error false arm) mcp-launcher.test.ts (+1 test): - ensureLauncher throws when file does not exist (if(!existsSync) true arm) Uses vi.doMock to intercept node:fs before fresh module import. Node 24 local result: 60.26% (2146/3561). Previous Node 22 CI: 59.73%. 1744 tests, all passing. tsc clean, prettier clean. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(triggers): 5 Copilot review fixes — byte-safe truncation, persist on terminal, etc. Address 5 of the 6 Copilot review threads on PR #96. The 6th (mid-run TriggerStatus rename) is a larger semantic refactor and deferred to a later commit; this commit pairs the smallest reasonable fix at each site with regression tests where they're cleanly testable. 1. src/core/triggers.ts:128 — log WriteStream missing error handler. createWriteStream() with no .on("error") emits an unhandled EventEmitter `error` event if the log file becomes unwritable (disk full, permission flip), which crashes the Node process. Added a logError-routing handler so disk-tier failures degrade gracefully without killing the supervisor. 2. src/storage/trigger-store.ts:60 — fireCount doc/semantics mismatch. The doc-comment claimed "Number of mid-run TALON_FIRE: lines" but the supervisor increments fireCount for every wake (mid-run and terminal). Fixed the comment to match the actual semantics; same counter, accurate meaning. Avoids breaking any caller that already relies on fireCount as a total. 3. src/core/triggers.ts:fireWake — mid-run prompt says "Status: fired" even when terminal=false, which can mislead downstream handling into treating an in-flight watcher as a completed run. Decoupled the prompt-display status from the on-disk TriggerStatus enum: non-terminal fires now show "Status: signalled" in both the header and the "Status:" line. Terminal fires unchanged. 4. src/core/triggers.ts:bufferAsPayload + fireWake — FIRE_PAYLOAD_MAX_BYTES truncation used String.prototype.slice (UTF-16 code units), so a payload of N multi-byte characters could exceed the documented byte cap and split a character mid-codepoint. Introduced truncateUtf8Tail/ truncateUtf8Head helpers that encode to UTF-8 bytes, slice on a byte boundary, then walk over UTF-8 continuation bytes (10xxxxxx) so we never cut a multi-byte sequence in half. The byte cap is now actually enforced as bytes. 5. src/storage/trigger-store.ts:282 + supervisor — updateTrigger only marks the store dirty (10s autosave window). For terminal status transitions (errored/cancelled/timed_out/fired) a crash inside that window would leave on-disk status as "running", which loadTriggers() then misclassifies as "terminated by previous restart". Added persistNow() calls at the four supervisor transition sites: failTrigger, cancelTrigger, the hard-timeout handler, and the finalizeExit path. The on-disk store now always agrees with the in-memory state across terminal transitions. 6. src/core/gateway-actions.ts:trigger_create — always returned "Status: running" even when spawnTrigger() failed without throwing (unsupported language slipping past validation, child.pid undefined, failTrigger() routing). With #5 above, the store now reflects the real state synchronously, so we re-read getTrigger(id) after spawn and return ok:false + lastError when it landed in "errored", or surface the actual status string otherwise. Callers no longer get false success responses. Regression tests (triggers-extended.test.ts): - mid-run prompt asserts "Status: signalled" appears and "Status: fired" does NOT, on a still-running trigger. Covers fix #3. - multi-byte UTF-8 payload (2000× 💧 = 8000 bytes vs 4000 string length) asserts the prompt's byte length stays bounded AND contains no Unicode replacement character (U+FFFD), which would indicate a mid-codepoint split. Covers fix #4. The remaining Copilot thread (introducing a dedicated non-terminal status enum value, replacing the TriggerStatus|"signalled" display- status hack) is a wider type change with API/store implications; better tackled in a follow-up than bundled into a defensive sweep. Verification: - npx vitest run → 1747/1748 pass (the 1 failure is the pre-existing package.functional "Stopped" expectation that fails whenever a live Talon daemon is running on the host — same env-dependent failure noted in PR #144 and PR #90's verification blocks). - npx vitest run src/__tests__/triggers* src/__tests__/trigger-store* → 74/74 pass (35 in triggers-extended including 2 new, 22 in trigger-store, 17 in triggers). - npx tsc --noEmit → clean. - npx prettier --check on all 4 changed files → clean. - npm run lint → 0 errors in changed files (10 pre-existing warnings elsewhere unchanged). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> Co-authored-by: claudiusthebot <claudius@anthropic.com>

When the Claude Agent SDK subprocess hangs and the heartbeat timeout fires, the previous code awaited `agentPromise.catch(() => {})` indefinitely so the running lock would stay held for as long as the SDK was wedged. Production hit this on 2026-05-10 — heartbeat #208 finished its work at 04:52Z but the SDK subprocess never exited, holding the lock for 17+ hours until I killed it manually at 22:23Z. The smoking gun was the stale comment "the Agent SDK does not expose an abort mechanism" — it does, via the `abortController` option, since at least 0.2.x. This PR uses it. Changes - Pass an `AbortController` to `query()` options. - On timeout: call `controller.abort()`, then race `agentPromise.catch` against a bounded grace window (`HEARTBEAT_ABORT_GRACE_MS`, default 30 s). If the SDK ignores the abort and the agent promise still hasn't settled, log a warning and release the lock anyway so the next heartbeat can fire. - Fire-and-forget `evictOrphanHeartbeatSubprocesses()` after grace expires: scans `/proc/<pid>/environ` for processes carrying `TALON_CHAT_ID=heartbeat` (set by Talon's MCP launcher when spawning heartbeat-tier subprocesses), SIGTERMs, waits `SUBPROCESS_KILL_GRACE_MS` (default 5 s), then SIGKILLs any that didn't exit. No-op on non-Linux platforms (relies on /proc). - All three timing constants are env-overridable (`TALON_HEARTBEAT_TIMEOUT_MS`, `TALON_HEARTBEAT_ABORT_GRACE_MS`, `TALON_HEARTBEAT_SUBPROCESS_KILL_GRACE_MS`) so the tests can use 50 ms windows. Test plan - Five new vitest cases in `src/__tests__/heartbeat.test.ts`: - AbortController is passed in query() options - `controller.abort()` fires when the SDK hangs past the timeout - Lock is released even when the SDK ignores abort entirely (the exact prod failure mode — a second `forceHeartbeat()` after the first wedged now resolves cleanly; previously it would deadlock) - `evictOrphanHeartbeatSubprocesses()` is a no-op on non-Linux - `evictOrphanHeartbeatSubprocesses()` scans `/proc`, filters by env var, SIGTERMs, escalates to SIGKILL after grace - 25/25 heartbeat tests pass, full suite 1796/1809 pass (1 pre-existing package.functional failure unrelated, 12 skipped live-tier). - `tsc --noEmit` clean. Prettier clean. Lint clean. Manual smoke - This is the fix for the bug observed in prod at 22:23Z 2026-05-10. The stuck PID 2221549 was SIGTERMed; with this PR merged + Talon restarted, a future SDK hang will trigger abort → grace → evict instead of indefinite lock retention. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

Three surgical fixes responding to the Copilot review at 12:35Z: 1. **timeoutFired race in catch path** (src/core/heartbeat.ts:367) The flag could flip to true during the `await appendHeartbeatLog` inside the catch — a non-timeout agent rejection would then be misclassified as a timeout and trigger a spurious abort + orphan sweep. Fix: snapshot the flag (`wasTimeout`) and clear the timer IMMEDIATELY at catch entry, before any awaits. The `finally` clear stays as a safety net for the success path. 2. **raceWithTimeout JSDoc** (src/core/heartbeat.ts:419) Doc claimed "Never throws" but the helper does throw if `p` rejects before the timeout. Updated the doc to reflect actual behaviour and note that callers needing a never-throwing race should `.catch()` the input themselves (as the eviction path already does). 3. **/proc/<pid>/environ substring false-positive** (src/core/heartbeat.ts:467) Raw `.includes("TALON_CHAT_ID=heartbeat")` would false-positive on any process whose env carries that substring as part of another var's value (e.g. `OTHER_VAR=TALON_CHAT_ID=heartbeat`) and SIGKILL it. Fix: split the environ on \0 and match exact entries. Tests - New regression case in heartbeat.test.ts: PID 3 with `OTHER_VAR=TALON_CHAT_ID=heartbeat\0FOO=bar` must NOT be matched (asserts both result counts and that killSpy was never called with pid 3). The old substring check would have flagged it. - Existing 24 tests + the new strengthened assertion → 25/25 pass. - `tsc --noEmit` clean. `prettier --check` clean. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

claudiusthebot requested a review from dylanneve1 as a code owner May 11, 2026 22:32

dylanneve1 requested a review from Copilot May 12, 2026 12:30

Copilot started reviewing on behalf of dylanneve1 May 12, 2026 12:31 View session

Copilot AI reviewed May 12, 2026

View reviewed changes

claudiusthebot mentioned this pull request May 12, 2026

fix(heartbeat): frontend-agnostic outbound + comprehensive test coverage #151

Merged

5 tasks

claudiusthebot mentioned this pull request May 13, 2026

fix: six correctness and safety bugs (timer leak, HTML injection, log component, flush consistency, status regex) #156

Merged

7 tasks

dylanneve1 force-pushed the fix/heartbeat-eviction branch from 8f08d90 to 1a87ee4 Compare May 13, 2026 13:07

claudiusthebot and others added 2 commits May 13, 2026 17:27

dylanneve1 force-pushed the fix/heartbeat-eviction branch from 1a87ee4 to 6a336fe Compare May 13, 2026 16:27

dylanneve1 enabled auto-merge (squash) May 13, 2026 16:27

dylanneve1 merged commit d17a4eb into main May 13, 2026
22 checks passed

dylanneve1 mentioned this pull request May 18, 2026

chore(main): release 1.12.0 #209

Merged

claudiusthebot mentioned this pull request May 19, 2026

fix(claude-sdk): post-result watchdog for stuck SDK iterator #218

Merged

Navigation Menu

fix(heartbeat): evict wedged SDK subprocesses, never deadlock the lock - #144

fix(heartbeat): evict wedged SDK subprocesses, never deadlock the lock#144
dylanneve1 merged 2 commits into
mainfrom
fix/heartbeat-eviction

claudiusthebot commented May 11, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		// /proc/<pid>/environ is NUL-delimited. Looking for our heartbeat marker.
		if (environRaw.includes("TALON_CHAT_ID=heartbeat")) {

Conversation

claudiusthebot commented May 11, 2026

Why

What this PR does

Tests

Test plan

Won't fix in this PR

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants