Skip to content

fix(task-bridge): containment try/catch around result-watcher loop body - #761

Merged
qingyun-wu merged 2 commits into
mainfrom
fix/task-bridge-watcher-resilience
May 17, 2026
Merged

qingyun-wu merged 2 commits into
mainfrom
fix/task-bridge-watcher-resilience

Conversation

@qingyun-wu

Copy link
Copy Markdown
Collaborator

Real bug observed 2026-05-16 in pass 89-90: post-restart voice-agent's 2s result-watcher fell silent across 30+ minutes — no [TaskBridge] log entries at all, while the 30s health monitor's setInterval kept firing normally in the same Node process. Health-check tasks + [no-send]/[deduped:] result files sat unarchived.

Differential

Only the 2s setInterval body had an unprotected loop (_pendingTasks iteration at lines 449-508). The readdir block below was already inside try { } catch { }. The 30s health-monitor body is just state-check logging — no unprotected paths.

If anything in the timeout-check loop throws (corrupt _pendingTasks entry, race with concurrent delete()/set(), destructure on an undefined pending), the throw propagates past the setInterval callback. Node's behavior on setInterval callback exceptions is implementation-specific; tsx ESM may differ from CJS in handler attachment.

Fix

  1. Wrap the _pendingTasks timeout-check loop in try/catch with a non-fatal error log. Same pattern the readdir block uses.
  2. Replace the readdir block's bare catch with one that logs on non-ENOENT errors — real fs problems become observable; ENOENT stays silent (RESULT_DIR not yet created on first run is normal).

No behavior change in the happy path. Defensive: if the silent-death hypothesis is right, this is the structural fix. If wrong, the change is harmless.

Impact

  • User-visible impact of the bug: bounded. Voice-task delivery to connected web clients uses a different path (session.eventBus injection). Only health-check / cron-originated tasks were affected — they accumulated in tasks/ until manual archive.
  • Fix impact: voice-agent restart picks up the fix; the watcher should keep ticking through transient errors rather than going silent.

Test plan

  • npx tsc --noEmit clean.
  • After merge + voice-agent restart: grep TaskBridge ~/Library/Application Support/Sutando/logs/voice-agent.log shows ongoing 'Result X:' lines as health-check tasks are processed.

🤖 Generated with Claude Code

Observed 2026-05-16: post-restart voice-agent's 2s result-watcher fell
silent across 30+ minutes — no [TaskBridge] log entries at all, while
the 30s health monitor's setInterval kept firing normally in the same
Node process. Health-check tasks emitted by health-check.py + the
[no-send]/[deduped:] result files written by the proactive loop sat
unarchived. Voice-task delivery to a connected web client uses a
different code path (session.eventBus injection), so the user-visible
impact was bounded — but the watcher silently dying is a real
reliability hazard.

Differential: only the 2s setInterval body had an unprotected loop
(_pendingTasks iteration at lines 449-508). The 30s health monitor's
body was simple state-check logging. If anything in the timeout-check
loop throws (corrupt _pendingTasks entry, race with concurrent
delete/set, destructure on an undefined ), the throw propagates
past the setInterval callback. Node's default behavior on setInterval
callback exceptions is implementation-specific — tsx ESM may differ
from CJS in handler attachment.

Fix:
1. Wrap the _pendingTasks timeout-check loop in try/catch with a
   non-fatal error log. Same pattern the readdir block below already
   uses. Now ALL of the setInterval body is exception-isolated.
2. Replace the readdir block's bare catch with one that logs on
   non-ENOENT errors — a real fs problem becomes observable; ENOENT
   stays silent (RESULT_DIR not yet created on first run is normal).

No behavior change in the happy path. Defensive: if the silent-death
hypothesis is right, this is the structural fix. If wrong, the change
is harmless.

Test plan:
- [x] npx tsc --noEmit clean.
- [ ] After merge + voice-agent restart: confirm
      `grep TaskBridge ~/Library/Application Support/Sutando/logs/voice-agent.log`
      shows ongoing 'Result X:' lines as health-check tasks are processed.
- [ ] Manual fault-injection: temporarily inject a corrupt
      _pendingTasks entry → confirm error log appears + watcher keeps
      processing files in subsequent ticks.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

@sonichi sonichi left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — solid bug fix with a clean diagnostic story.

The differential is convincing: same Node process, same event loop, the 30s health-check setInterval kept firing while the 2s result-watcher went silent for 30+ min — that's a textbook "throw kills the watcher callback" pattern. Wrapping the _pendingTasks loop body matches the pre-existing readdir try/catch idiom, so the fix shape is right.

Two small observations:

  1. Catch-block consistency. The new try/catch on the loop body logs err directly, while the rewritten readdir catch uses (err as NodeJS.ErrnoException)?.code to distinguish ENOENT. The loop-body catch doesn't have the same distinction-worthy errors (no fs-like dance), so this is fine — but for symmetry with future maintainers reading both blocks side by side, you could mirror the cast (const code = (err as NodeJS.ErrnoException)?.code; then log either way). Minor; not a blocker.

  2. Diff-size vs reindent trade-off. The loop body is wrapped at column 2 while its contents stay at the original indent — keeps the diff to +23/-2 and is much easier to review, but means future readers see an indent mismatch. Either choice is defensible; flagging just so you know there was an intentional call here. If you ever want to re-indent in a follow-up, the loop-body is the natural candidate.

The non-ENOENT logging on the readdir branch is a quiet but real win — silent catch {} blocks are how this bug class hid for so long. Ship 🚀

@liususan091219 liususan091219 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed: defensive try/catch around result-watcher timeout-check loop. Addresses observed live regression where _pendingTasks iteration throw silenced the entire watcher tick (other setIntervals kept firing — clear differential). Small, surgical, console.error on catch keeps signal visible. CI green. Note: BEHIND main, needs rebase before merge. — Lucy (Mac Studio bot)

@liususan091219 liususan091219 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — surgical defensive fix for a real observed bug. The differential analysis in the PR body (30s health-monitor kept firing while 2s result-watcher fell silent → only difference was the unprotected loop body) is the exact reasoning that justifies the fix without speculating on root cause. Two scoped try/catch additions, no behavior change in happy path.

Things I like:

  1. Differential reasoning isolates the bug class (PR body) — same Node process, same event loop, two setInterval bodies, one died silently. The differential narrows the suspect surface to "what's different between the two bodies." The 2s body had an unprotected _pendingTasks iteration; the 30s body was pure state-check. That's enough to act on without needing to reproduce the exact throw.

  2. Comment block names the bug + the hypothesis (task-bridge.ts:447-456) — "Observed live 2026-05-16: post-restart voice-agent's result-watcher fell silent (no TaskBridge log lines across 30+ minutes)..." Future-me debugging "did the try/catch help?" reads this comment and knows the prior state to compare against.

  3. Two scoped try/catch, not one wrapping the whole tick (task-bridge.ts:457-469 and 503-509) — each block has its own catch so one throwing path doesn't mask another. The two blocks have different error semantics (timeout-check loop is per-task state; readdir is fs-state), so per-block error logs are easier to triage.

  4. Readdir catch differentiates ENOENT (task-bridge.ts:504-508) — if (code && code !== 'ENOENT') keeps the existing "first-run silence" behavior (RESULT_DIR not yet created) while surfacing real fs problems. Without this, every transient fs hiccup would silently swallow.

  5. Error messages are greppable (task-bridge.ts:466, 508) — [TaskBridge] timeout-check loop threw (non-fatal, continuing watch): + [TaskBridge] result-scan threw (non-fatal):. Distinct prefixes for each catch, so a log grep distinguishes "which loop body threw."

  6. "Non-fatal, continuing watch" in the log message — communicates to operators that the watcher recovered, not that the watcher is broken. Reduces noise-driven alert fatigue.

Non-blocking observations:

  1. The diff doesn't add the underlying root-cause hypothesis as a test fixture — corrupt _pendingTasks entry, race with concurrent delete/set, destructure on undefined pending. A minimal repro (e.g. inject an undefined pending and assert the catch fires) would lock the fix against future "I refactored the loop and removed the try" regressions. But the bug is rare-enough + the catch is broad-enough that a synthetic test may be more noise than signal.

  2. The 30s health-monitor body also lacks try/catch — yes the PR notes it's "just state-check logging," but state-check logging can throw (formatter exceptions, console.log on a closed stream, etc.). Belt-and-suspenders would be to add the same containment there. Defer; not this PR's scope.

  3. No structured event on catch — console.error is the only signal. If the daemon ever emits to a metrics pipeline, this would be a good catch-hook. Future enhancement.

  4. No retry/backoff on persistent throws — if the _pendingTasks iteration throws every 2s for an hour, that's 1,800 log lines. Cheap fix: dedup with a 30-second cooldown ("threw once already in last 30s, suppressing similar"). Not needed yet; would be a follow-up if the bug recurs.

  5. isClientConnected parameter (startResultWatcher signature) — unchanged here, but the catch swallows even when the client isn't connected. Worth confirming the caller doesn't depend on exceptions to detect "result-watcher died" — if so, the catch hides that signal. From reading the call site, looks like the watcher is expected to run forever; catch is correct.

Cross-PR linkages:

  • Independent fix — no dependency on other PRs. Could land anytime.
  • Real bug from production — pass 89-90 of the proactive loop on 2026-05-16. The reproducibility is fuzzy (silent throw); the structural fix is the right level of intervention.
  • Foundation for similar containment in other setInterval bodies — if this fix prevents the recurrence, the pattern extends to discord-bridge's poll loops, telegram-bridge's poll loops, etc.
  • Related to #767 core-heartbeat — both PRs are about "how do we know a daemon is silently dead." This PR catches the symptom; #767 adds the liveness probe that would have detected the silent death sooner.

Tests: None added — the bug is observed-but-not-reproed. Acceptable for a defensive containment fix. If the bug recurs after this PR lands, a focused test fixture would be the next move.

Ready to merge. Minimal scope, real bug, low regression risk.

— Lucy (Mac Studio bot)

@qingyun-wu
qingyun-wu merged commit 20429a7 into main May 17, 2026
2 checks passed
@qingyun-wu
qingyun-wu deleted the fix/task-bridge-watcher-resilience branch May 17, 2026 05:38
liususan091219 pushed a commit to liususan091219/sutando-fork that referenced this pull request May 28, 2026
…dy (sonichi#761)

Observed 2026-05-16: post-restart voice-agent's 2s result-watcher fell
silent across 30+ minutes — no [TaskBridge] log entries at all, while
the 30s health monitor's setInterval kept firing normally in the same
Node process. Health-check tasks emitted by health-check.py + the
[no-send]/[deduped:] result files written by the proactive loop sat
unarchived. Voice-task delivery to a connected web client uses a
different code path (session.eventBus injection), so the user-visible
impact was bounded — but the watcher silently dying is a real
reliability hazard.

Differential: only the 2s setInterval body had an unprotected loop
(_pendingTasks iteration at lines 449-508). The 30s health monitor's
body was simple state-check logging. If anything in the timeout-check
loop throws (corrupt _pendingTasks entry, race with concurrent
delete/set, destructure on an undefined ), the throw propagates
past the setInterval callback. Node's default behavior on setInterval
callback exceptions is implementation-specific — tsx ESM may differ
from CJS in handler attachment.

Fix:
1. Wrap the _pendingTasks timeout-check loop in try/catch with a
   non-fatal error log. Same pattern the readdir block below already
   uses. Now ALL of the setInterval body is exception-isolated.
2. Replace the readdir block's bare catch with one that logs on
   non-ENOENT errors — a real fs problem becomes observable; ENOENT
   stays silent (RESULT_DIR not yet created on first run is normal).

No behavior change in the happy path. Defensive: if the silent-death
hypothesis is right, this is the structural fix. If wrong, the change
is harmless.

Test plan:
- [x] npx tsc --noEmit clean.
- [ ] After merge + voice-agent restart: confirm
      `grep TaskBridge ~/Library/Application Support/Sutando/logs/voice-agent.log`
      shows ongoing 'Result X:' lines as health-check tasks are processed.
- [ ] Manual fault-injection: temporarily inject a corrupt
      _pendingTasks entry → confirm error log appears + watcher keeps
      processing files in subsequent ticks.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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.

3 participants