Skip to content

Fix watchdog timeouts: memory throttling and tracemalloc snapshot stalls - #425

Merged
mnot merged 6 commits into
mainfrom
claude/redbot-watchdog-timeout-bb8a0e
Jul 23, 2026
Merged

Fix watchdog timeouts: memory throttling and tracemalloc snapshot stalls#425
mnot merged 6 commits into
mainfrom
claude/redbot-watchdog-timeout-bb8a0e

Conversation

@mnot

@mnot mnot commented Jul 23, 2026

Copy link
Copy Markdown
Owner

Diagnoses and addresses repeated redbot.service: Watchdog timeout kills on redbot.org.

Root causes

Two independent sources of multi-second event-loop stalls, both of which trip the systemd watchdog.

1. cgroup memory throttling. The shipped unit capped the daemon well below its actual working set — MemoryHigh=50M against ~57M steady state (interpreter, Jinja templates, compiled regexes, babel's CLDR tables). Crossing memory.high doesn't fail an allocation; the kernel throttles the task in reclaim. With MemorySwapMax=60M on top, an overshoot became swap thrashing. The signature in the journal is distinctive: SIGABRT delivered at 04:11:25, but Python didn't run its handler until 04:12:00 — 35 seconds during which the main thread executed no bytecode at all, with scheduled events overdue by 37s. A live deployment showed memory.events: high 44 with MemoryPeak pinned exactly at the limit.

2. tracemalloc snapshots under --debug. tracemalloc.start(25) captured 25-frame tracebacks on every allocation, but dump_memory_stats() aggregates with statistics("lineno") and only ever reads traceback[0] — the other 24 frames were captured and discarded. take_snapshot() cost scales with that depth: completed dumps grew from 2s to 25s as the heap grew, and three watchdog kills have tracebacks landing directly in _get_traces(), each firing ~27s after the dump began.

Changes

  • tracemalloc.start(25)start(1) — identical output, a fraction of the cost.
  • watchdog_ping() reschedules in a finally — it previously rescheduled only after SYSTEMD_NOTIFIER returned, so one exception from the notify would drop the process out of the ping chain permanently and systemd would kill it WatchdogSec later with nothing in the journal to explain why. Unrelated to these incidents, but a trap worth closing.
  • Unit file: MemoryHigh=96M, MemoryMax=128M, MemorySwapMax=0, WatchdogSec=30, with comments recording the measured working set and the reasoning. Swap is now off deliberately: an overshoot becomes a prompt OOM kill and restart rather than tens of seconds of thrashing.

For reviewers

  • The 96M figure is chosen for headroom, not measured. The ~57M working set comes from a MemoryPeak that was itself pinned against the old 50M MemoryHigh, so it understates what the process actually wants. Once --debug is off in production, MemoryPeak plus a flat high counter will give the true number, and the limits can be tightened.
  • The try/finally change is verified by inspection only. It needs SYSTEMD_WATCHDOG and cysystemd, so it doesn't exercise on macOS.
  • The unit file is not syntax-checkedsystemd-analyze verify isn't available on the dev machine.
  • Not everything is explained. Three of five observed watchdog kills are attributable to the tracemalloc snapshots. Two others (20:04, 04:10) produced no usable traceback and needed SIGKILL — consistent with the memory-throttling mechanism, but not individually proven. Turning --debug off removes both the snapshot stalls and ~45M of overhead; anything that still trips after that is a genuine remaining bug, and with high staying flat it will be cleanly diagnosable for the first time.
  • Separately worth a look: chardet accounts for 22.2 MiB in every dump — ten times everything else combined, and constant across processes. It isn't a redbot dependency; it arrives via httplint 2026.05.2. Probably its lazily-loaded language-model tables rather than retained body copies, but unconfirmed.

Verified with make typecheck, make lint (10.00/10), and make test (37 passed), each checked by exit code. Also ran the daemon end-to-end with -d: starts, serves 200 on /, and SIGTERM produces the expected memory dump.


🤖 Written by Claude Opus 4.8 in Claude Code. The diagnosis was developed interactively with @mnot over the course of the investigation — he supplied the journal excerpts and memory.events output, adjusted the deployed unit himself, and reviewed the reasoning at each step before approving these three changes. The code and this description are AI-generated; the conclusions were checked against live production data by a human.

mnot added 3 commits July 23, 2026 07:53
tracemalloc was started with a 25-frame capture depth, but
dump_memory_stats() aggregates with statistics("lineno") and only ever
reads traceback[0] -- the other 24 frames were captured on every
allocation and discarded.

take_snapshot() cost scales with that depth. On a ~100M heap, snapshots
grew past 25s of synchronous work on the loop thread, which is long
enough to miss the systemd watchdog deadline: three separate watchdog
kills had tracebacks landing in _get_traces(), with the loop's scheduled
events overdue by 57-100s.

Depth 1 produces identical output at a fraction of the cost.
watchdog_ping() rescheduled itself only after SYSTEMD_NOTIFIER returned,
so a single exception from the notify would drop the process out of the
ping chain permanently. systemd then kills it WatchdogSec later, with
nothing in the journal explaining why the pings stopped.

Reschedule in a finally, so a failed notify costs one missed ping rather
than all of them.
The unit capped the daemon well below its actual working set: ~57M
steady state (interpreter, Jinja templates, compiled regexes, babel's
CLDR tables) against MemoryHigh=50M. Crossing memory.high doesn't fail
an allocation, it throttles the task in kernel reclaim -- and with
MemorySwapMax=60M the overshoot became swap thrashing. The result was
the loop parked in the kernel for 35-45s at a stretch, running no Python
bytecode, which tripped the 10s watchdog. A deployment showed 44
memory.high throttle events with the peak pinned exactly at the limit.

MemoryHigh=96M / MemoryMax=128M gives the working set room. Swap is now
disabled outright: without it an overshoot is a prompt OOM kill and
restart, which is far better than tens of seconds of thrashing. 30s of
watchdog leaves margin for an ordinary reclaim stall while still
catching a genuinely wedged loop within ten ping intervals.
@mnot

mnot commented Jul 23, 2026

Copy link
Copy Markdown
Owner Author

Review

Diagnosis in the description is well-evidenced, and it's unusually clear about separating what's proven from what's inferred. Three findings, one of which I think blocks merge as written.

The tracemalloc change is correct

dump_memory_stats() reads only stat.traceback[0] and aggregates with statistics("lineno"), which keys on the first frame regardless of capture depth. Depth 1 gives byte-identical output. The "<frozen" in filename filter is the one place deeper frames could have been useful — attributing importlib allocations to a real caller — but the code drops those entries rather than walking up, so nothing is lost.

The try/finally doesn't do what its comment claims

The comment says the old code would "silently stop all future pings, and systemd would kill us WatchdogSec later with nothing in the log to say why." That isn't the old behaviour, and the new code doesn't change the outcome.

thor's scheduler calls what() with no exception handling (_run_scheduled_events in thor/loop.py), and RedBotServer.run() catches only KeyboardInterrupt. So if SYSTEMD_NOTIFIER raises:

  • Before: the exception propagates out of thor.run() and the process exits with a traceback on stderr. Loud, not silent.
  • After: finally schedules the next ping, then the exception propagates identically — the loop has already exited, so the newly-scheduled event never fires.

Net behavioural difference: none. To actually get the described resilience it has to swallow and log:

try:
    SYSTEMD_NOTIFIER(SYSTEMD_NOTIFICATION.WATCHDOG)
except Exception:  # pylint: disable=broad-except
    self.console(f"Watchdog notify failed:\n{traceback.format_exc()}")
finally:
    thor.schedule(self.watchdog_freq, self.watchdog_ping)

dump_memory_stats() is the precedent for that broad-except-and-log style. Whether it's desirable is a separate call — a persistently failing notify gets the service killed anyway, so crashing loudly is defensible. But then drop the change and the comment with it, rather than shipping a comment that misdescribes both the old and new behaviour.

This is also the one piece here that's cheaply unit-testable (patch SYSTEMD_NOTIFIER to raise, assert a ping got rescheduled) — and since the finding is precisely that inspection reached the wrong conclusion, a small test would have caught it.

The ~45M figure in the unit-file comment is invalidated by this same PR

The new comment records "Running with --debug adds ~45M of tracemalloc overhead". That was measured at capture depth 25. tracemalloc's per-allocation traceback storage scales with depth, so cutting to 1 should remove most of it — the diff makes its own comment stale. Either re-measure after deploy or hedge the wording.

Relatedly: 96M against a ~57M working set is ~39M of headroom, less than the 45M the comment says --debug costs. The comment is self-consistent in saying MemoryHigh "needs raising to match" — but it means the unit change alone doesn't fix the deployed configuration while production is still running --debug. That dependency lives only in the PR description; nothing in the repo encodes it. Worth making the comment say outright that 96M assumes --debug is off.

Smaller notes

  • watchdog_freq = 3 is hardcoded while systemd exports WATCHDOG_USEC. Now that the two have diverged 10x, deriving the interval (conventionally WATCHDOG_USEC / 2) would stop them drifting further. Out of scope for this PR, but the new WatchdogSec comment is a good place to note the coupling.
  • finally catches BaseException, so a KeyboardInterrupt inside the notify now schedules one more ping during shutdown. Harmless — pings already continue through graceful_shutdown() — just a wider scope than intended.
  • MemorySwapMax=0 is valid syntax and does what's intended; on hosts without the cgroup v2 swap controller systemd warns and continues, so no deployment risk. MemoryLow/High/Max are correctly ordered.
  • Nothing security-relevant. Loosening the caps slightly weakens a resource-exhaustion bound, but 128M under a 60% CPU quota is still a tight sandbox.

Recommendation

Land the tracemalloc and unit-file changes. Before merging, either rework watchdog_ping into an actual except-and-log or drop it, and fix the two comments (the ~45M staleness, the --debug dependency).


🤖 Review generated by Claude Opus 4.8 in Claude Code, at @mnot's request. Claude read the diff, the surrounding daemon.py code, and thor's scheduler source to check the exception-propagation claim; the memory-limit reasoning was checked by inspection only, not against a live system. Not yet independently verified by a human.

mnot added 2 commits July 23, 2026 09:09
The finally added earlier was a no-op: thor's scheduler calls events bare
and run() catches only KeyboardInterrupt, so an exception from the notify
propagated out of thor.run() and killed the process either way -- the
newly-scheduled ping never fired because the loop had already exited. The
comment claiming the old behaviour was silent was wrong too; it crashed
loudly with a traceback.

Catch and log instead, so a transient notify failure costs one ping
rather than dropping in-flight requests. Persistent failures still get
the service killed, since the watchdog simply stops being fed.

No finally, so a KeyboardInterrupt during the notify propagates without
scheduling a stray ping on the way out.

Adds test/test_daemon.py covering both paths; it fails against the
previous version.
The ~45M tracemalloc figure was measured at a 25-frame capture depth,
which this same branch drops to 1 -- the comment was stale on arrival.
Say what it was measured at and point at memory.events for re-measuring
rather than quoting a number that no longer holds.

Also state outright that the limits assume --debug is off (96M against a
~57M working set is less headroom than debug mode was costing), and note
that watchdog_freq is hardcoded rather than derived from WATCHDOG_USEC,
so the two have to be kept in step by hand.
@mnot

mnot commented Jul 23, 2026

Copy link
Copy Markdown
Owner Author

All three points addressed. Two new commits.

The try/finally was a no-op — confirmed and reworked

The review is right, and I verified it rather than taking it on faith: _run_scheduled_events calls what() bare, and run() catches only KeyboardInterrupt. So the exception propagated out of thor.run() and killed the process both before and after my change; the newly-scheduled ping never fired because the loop had already exited. The comment was wrong in the other direction too — the old behaviour was a loud crash with a traceback, not a silent stall.

Reworked to catch-and-log (9c6656c), following the dump_memory_stats() precedent. I went with resilience over crashing loudly: a transient notify failure isn't worth dropping in-flight requests for, and a persistently failing notify still gets the service killed, since the watchdog simply stops being fed. The backstop is intact either way.

Dropped the finally while I was there — the reschedule now sits after the except, so a KeyboardInterrupt inside the notify propagates without scheduling a stray ping on the way out. That was the BaseException scope note.

Added a test

test/test_daemon.py covers the failing-notify path, the success path, and the no-systemd path, driving watchdog_ping against a stub rather than building a whole server. Confirmed it fails against the previous version (git stash the daemon change, run, restore) — so it does catch the regression it's meant to, rather than just passing alongside it.

Point taken on the broader lesson: the finding was that inspection reached the wrong conclusion, and I'd shipped that method flagged as "verified by inspection only". The test is what should have accompanied it in the first place.

Comment fixes

6b8005a rewrites both. The ~45M figure now says what depth it was measured at, notes that dropping to 1 should cut most of it, and points at memory.events for re-measuring instead of quoting a number the same branch invalidates. The limits now state outright that they assume --debug is off. Also noted the watchdog_freq / WATCHDOG_USEC coupling next to WatchdogSec, as suggested — deriving it is left out of scope, but the hardcoding is at least written down where someone changing one will see it.

make typecheck, make lint (10.00/10), and make test all clean by exit code.


🤖 Written by Claude Opus 4.8 in Claude Code, responding to @mnot's request to address the review. The exception-propagation claim was checked against thor's source before acting on it; the test was verified to fail against the prior commit. The memory-limit reasoning remains inspection-only — no live system was touched.

Comment thread redbot/daemon.py Outdated
@mnot
mnot merged commit 5ae197b into main Jul 23, 2026
@mnot
mnot deleted the claude/redbot-watchdog-timeout-bb8a0e branch July 23, 2026 07:18
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