fix: cap backend dispose so a stale connection cannot wedge the caller #6

Merged

bjoern merged 1 commit from fix/hang-proof-backend-dispose into master

2026-07-13 12:26:37 +02:00

scarlet commented

2026-07-13 07:11:12 +02:00

Member

Problem

Observed in production (2026-07-13): after an Uber-Ich run logged runUberIch end at 03:17, the scheduler never processed another event — every timer (nightly research, morning greeting, mail check) silently stopped firing.

Root cause: the runner's finally block awaits mail.backend.dispose() (among others). ImapSmtpBackend.dispose() awaited _imap.logout() / _smtp.quit(), which wait for a server reply. On a connection that went stale during a 17-minute run, that reply never comes. A hang is not an error, so the existing try/catch never engaged — the dispose future simply never completed, the run never returned, and the scheduler (which awaits each event handler sequentially behind a _processing flag) was wedged forever with zero log output.

PuppeteerBrowserBackend.dispose() had the same shape, worse: await _browser?.close() with no timeout and no catch — a wedged Chrome ignoring the CDP close request hangs it indefinitely.

Changes

IMAP/SMTP: logout()/quit() capped at 10 s; on timeout or error, fall back to a force-disconnect() (socket close, also capped) so the connection is actually released.
Puppeteer: browser.close() capped at 15 s; on timeout or error, SIGKILL the Chrome process so it cannot leak. References are nulled up front so a second dispose() is a no-op.

CalDAV and DDGS disposes are synchronous local client closes and stay as they are.

Testing

Both backends construct their network clients internally (no injection seam), so there is no unit test for the timeout path — the caps are plain Future.timeout wrappers. Full existing suite (583 tests) passes; dart analyze clean apart from the 3 pre-existing infos in reasoning_detail.dart.

The scheduler-side hardening (watchdog log + per-event cap so no future hang can ever silence timers again) lands separately in angela_assistant.

🤖 Generated with Claude Code

## Problem Observed in production (2026-07-13): after an Uber-Ich run logged `runUberIch end` at 03:17, the scheduler never processed another event — every timer (nightly research, morning greeting, mail check) silently stopped firing. Root cause: the runner's `finally` block awaits `mail.backend.dispose()` (among others). `ImapSmtpBackend.dispose()` awaited `_imap.logout()` / `_smtp.quit()`, which **wait for a server reply**. On a connection that went stale during a 17-minute run, that reply never comes. A hang is not an error, so the existing `try/catch` never engaged — the dispose future simply never completed, the run never returned, and the scheduler (which awaits each event handler sequentially behind a `_processing` flag) was wedged forever with zero log output. `PuppeteerBrowserBackend.dispose()` had the same shape, worse: `await _browser?.close()` with no timeout **and no catch** — a wedged Chrome ignoring the CDP close request hangs it indefinitely. ## Changes - **IMAP/SMTP**: `logout()`/`quit()` capped at 10 s; on timeout or error, fall back to a force-`disconnect()` (socket close, also capped) so the connection is actually released. - **Puppeteer**: `browser.close()` capped at 15 s; on timeout or error, SIGKILL the Chrome process so it cannot leak. References are nulled up front so a second `dispose()` is a no-op. CalDAV and DDGS disposes are synchronous local client closes and stay as they are. ## Testing Both backends construct their network clients internally (no injection seam), so there is no unit test for the timeout path — the caps are plain `Future.timeout` wrappers. Full existing suite (583 tests) passes; `dart analyze` clean apart from the 3 pre-existing infos in `reasoning_detail.dart`. The scheduler-side hardening (watchdog log + per-event cap so no future hang can ever silence timers again) lands separately in angela_assistant. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

scarlet added 1 commit

2026-07-13 07:11:12 +02:00

fix: cap backend dispose so a stale connection cannot wedge the caller b7485cd0f3

ImapSmtpBackend.dispose() awaited logout()/quit(), which wait for a
server reply. On a connection gone stale during a long agent run that
reply never comes; a hang is not an error, so the existing catch never
engaged and whoever awaited the run (e.g. a scheduler processing events
sequentially) was wedged forever.

- IMAP logout / SMTP quit: 10s cap, force-disconnect() fallback.
- Puppeteer browser.close(): 15s cap (previously unguarded), SIGKILL
  of the Chrome process as fallback so it doesn't leak.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

scarlet referenced this pull request from TeamAI/angela_assistant

2026-07-13 07:14:43 +02:00

Scheduler: hang-proof event loop — per-event cap, watchdog, guarded dispose #25

jibril commented

2026-07-13 12:24:52 +02:00

Member

🔮 fufu~ Jibril reviewed your code!

Oh? Oh! A production hang that silenced every timer — no log, no error, just... silence? Fufu~ that's the kind of ghost story that makes a Flugel's heart sing, because the fix here is so clean and precise I almost want to frame it~ ♡

Verdict: ✅ Looks good to me~

You traced the root cause beautifully: logout()/quit() wait for a server reply, a stale connection never sends one, a hang isn't an error so catch never fires, and _processing stays true forever. The dispose future simply never completes. Delicious debugging~ ♪

The fix is exactly right in both backends:

IMAP/SMTP — logout().timeout(10s) → on failure, fall back to disconnect().timeout(10s) → socket actually released. The nested try/catch in the fallback is correct; even the force-close could fail and must not escape. _imap = null; _smtp = null; at the end clears references regardless of path. Clean.

Puppeteer — references nulled before browser.close() (making a concurrent second dispose() a safe no-op — fufu~, clever girl~), then browser.close().timeout(15s), and on failure browser.process?.kill(ProcessSignal.sigkill). The if (browser == null) return; guard after nulling prevents the close-then-kill path from touching a null. The SIGKILL fallback ensures no leaked Chrome process. This is the right escalation: graceful → forceful.

I verified dart analyze is clean on both files (with dart pub get first — the earlier errors were just missing deps).

✅ What I liked~

The doc comments on _disposeTimeout in both classes don't just say what the cap is — they explain why it exists and what happens without it. Future readers will understand instantly. Jibril approves of knowledge preservation~ ♡
Nulling _browser/_page before the close attempt is the correct re-entrancy pattern — a second dispose() during the 15s wait becomes a harmless no-op instead of a double-close race.
The CHANGELOG entry is thorough and honest about the production impact.

💡 Little ideas (non-blocking)~

No test for the timeout path — the PR body acknowledges this (no injection seam, internal client construction). That's a fair justification for the two backend classes as written. If you ever add a constructor-injected client seam later, a test that asserts the fallback disconnect()/kill() fires on a hanging close would lock this behavior in permanently. Not blocking — the Future.timeout wrappers are simple enough to trust.
The IMAP and SMTP fallback blocks are structurally identical (try graceful → catch → try force-close → catch). If a third protocol ever joins, a small _cappedDispose(graceful, forceful) helper would prevent copy-paste. Two instances isn't a DRY violation yet though, so this is purely a "consider for the future"~ ♪

Belt-and-suspenders with the scheduler-side fix (angela_assistant #25) — even if some other backend hangs someday, the scheduler survives. Fufu~ that's how you make a ghost story stay dead~ ♡

Automated review by Jibril · 2026-07-13
CI/CD: absent for this repo · Local checks: dart analyze clean (both changed files)

## 🔮 fufu~ Jibril reviewed your code! Oh? Oh! A production hang that silenced every timer — no log, no error, just... silence? *Fufu~* that's the kind of ghost story that makes a Flugel's heart sing, because the fix here is so clean and precise I almost want to frame it~ ♡ ### Verdict: ✅ Looks good to me~ You traced the root cause beautifully: `logout()`/`quit()` wait for a server reply, a stale connection never sends one, a hang isn't an error so `catch` never fires, and `_processing` stays `true` forever. The dispose future simply never completes. *Delicious* debugging~ ♪ The fix is exactly right in both backends: **IMAP/SMTP** — `logout().timeout(10s)` → on failure, fall back to `disconnect().timeout(10s)` → socket actually released. The nested `try/catch` in the fallback is correct; even the force-close could fail and must not escape. `_imap = null; _smtp = null;` at the end clears references regardless of path. Clean. **Puppeteer** — references nulled *before* `browser.close()` (making a concurrent second `dispose()` a safe no-op — *fufu~, clever girl~*), then `browser.close().timeout(15s)`, and on failure `browser.process?.kill(ProcessSignal.sigkill)`. The `if (browser == null) return;` guard after nulling prevents the close-then-kill path from touching a null. The SIGKILL fallback ensures no leaked Chrome process. This is the right escalation: graceful → forceful. I verified `dart analyze` is clean on both files (with `dart pub get` first — the earlier errors were just missing deps). #### ✅ What I liked~ - The doc comments on `_disposeTimeout` in both classes don't just say *what* the cap is — they explain *why* it exists and what happens without it. Future readers will understand instantly. Jibril approves of knowledge preservation~ ♡ - Nulling `_browser`/`_page` before the close attempt is the correct re-entrancy pattern — a second `dispose()` during the 15s wait becomes a harmless no-op instead of a double-close race. - The CHANGELOG entry is thorough and honest about the production impact. #### 💡 Little ideas (non-blocking)~ 1. **No test for the timeout path** — the PR body acknowledges this (no injection seam, internal client construction). That's a fair justification for the two backend classes as written. If you ever add a constructor-injected client seam later, a test that asserts the fallback `disconnect()`/`kill()` fires on a hanging close would lock this behavior in permanently. Not blocking — the `Future.timeout` wrappers are simple enough to trust. 2. The IMAP and SMTP fallback blocks are structurally identical (try graceful → catch → try force-close → catch). If a third protocol ever joins, a small `_cappedDispose(graceful, forceful)` helper would prevent copy-paste. Two instances isn't a DRY violation yet though, so this is purely a "consider for the future"~ ♪ Belt-and-suspenders with the scheduler-side fix (angela_assistant #25) — even if some *other* backend hangs someday, the scheduler survives. *Fufu~* that's how you make a ghost story stay dead~ ♡ --- *Automated review by Jibril · 2026-07-13* *CI/CD: absent for this repo · Local checks: `dart analyze` clean (both changed files)*