fix(watcher): reap finished issue-processing task handles [DISCORD-15363517] - #138
Conversation
The watcher pushed a JoinHandle into `spawn_handles` for every issue it dispatched and never removed it outside of tests, so the list was push-only in production. Tokio frees a task's allocation only once both the scheduler and the last JoinHandle are dropped, so each completed `process_issue` task stayed resident for the daemon's lifetime — and that allocation is large, since `process_issue` inlined the whole `IssueProcessor::run` pipeline. RSS therefore grew monotonically with issues processed until the container was OOM-killed and restarted with a fresh (empty) list, matching the reported intermittent OOM kills. - Reap finished handles once per poll cycle and on every dispatch, so the list is bounded by concurrency instead of issues-processed-ever. - Drain the spawned tasks in `stop_and_drain` so shutdown waits for their teardown too. - Box the `processor.run(...)` future so each spawned task allocation carries a pointer instead of the fully inlined pipeline state machine. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Greptile SummaryThe PR bounds issue-processing handle retention and makes shutdown explicitly wait for or cancel tracked tasks.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains. Important Files Changed
Reviews (7): Last reviewed commit: "fix(watcher): bound shutdown drain, abor..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Fix Confidence: 88/100High confidence the leak is real and fixed: the push-only Vec was verified directly in code (grep over crates/ and src/ returns only the field decl, init, the test-only drain, and the push), the new test fails on main and passes with the change, and all 1186 engine unit tests plus all 16 PR checks are green. Deducted for two things I could not verify end-to-end: (1) I did not measure actual RSS of a running daemon before/after, so the OOM kills could have a second contributing cause beyond this leak; (2) the |
|
@claudear Comment: When shutdown overlaps a dispatch that has passed its running check but has not yet recorded its spawned task, For each issue above, determine whether it is valid and should be fixed. If so, fix it directly. |
…ary' into fix/DISCORD-15363517-watcher-join-handle-leak
Re-check is_running and spawn+record the handle under the spawn_handles lock that drain_spawned_tasks takes. Since stop() sets is_running=false before draining, a dispatch either records before the drain's take (so shutdown joins it) or sees the stop and never spawns. The top-of-loop check alone left a window between check and push.
Wrapping drain_spawned_tasks in a timeout dropped the handles it had already taken from spawn_handles, detaching unfinished tasks so the runtime aborted them mid-operation. drain_spawned_tasks_until pops one handle at a time and joins via &mut handle, putting any handle that exceeds the budget back into spawn_handles instead of dropping it.
The 30s cap let stop_and_drain return while a task was still running, after which runtime teardown aborted it mid-operation. Drain the spawn_handles to completion instead (active_processing is only bumped inside handle-tracked process_issue, so the handles subsume it). Unbounded on purpose: production races this against an operator force-quit, the intended hard limit. Supersedes the cancel-safe timeout drain, which the fixed budget still undermined.
Unbounded drain could stall a non-interactive shutdown (redeploy SIGTERM) when a task was wedged in an external git/LLM op with no internal timeout. drain_or_abort waits up to GRACEFUL_DRAIN_BUDGET (30s), then aborts remaining tasks explicitly and logs it, rather than stalling or leaving them for runtime teardown. Cancel-safe: handles are joined via &mut so the timeout drops only the borrow, keeping them for the abort pass.
2ff2418 to
daa8c23
Compare
2 participants
Problem
appwrite/claudeargets OOM-killed intermittently. The daemon leaks memory in proportion to the number of issues it processes.Watcher.spawn_handlesis aVec<JoinHandle<()>>that is push-only in production.dispatch_lanepushes a handle for every spawnedprocess_issuetask, and the only code that removes entries isdrain_spawned_tasks(), whose four call sites are all inside#[cfg(test)] mod tests. Nothing in the run loop, the housekeeping loop, or the stop path ever reaps them.Tokio frees a task's allocation only once both the scheduler and the last
JoinHandleare dropped, so every completed issue-processing task stayed resident for the process lifetime. That allocation is not a 24-byte handle:process_issueawaitedIssueProcessor::runinline with no boxing anywhere, so each retained handle pinned the entire inlined pipeline state machine.RSS therefore climbed monotonically across days of polling until the kernel OOM-killed the container, which then restarted with a fresh (empty)
Vec— matching the reported "sometimes gets OOM killed" pattern.Agent spawning itself is not out of bounds: the per-source/per-lane gating in
dispatch_laneis correct,ProcessingState::removedecrements properly, and the intent-classification path is explicitly sequential.Fix
poll_source, before the rate-limit early return so an idle or paused watcher still frees them) and on every dispatch, so the list is bounded by concurrency rather than by issues-processed-ever.stop_and_drain(bounded by the existing 30s budget) so shutdown waits for their teardown too.Box::pintheprocessor.run(...)await so each spawned task allocation carries a pointer instead of the fully inlined pipeline state machine. This shrinks the per-task footprint while tasks are running, not just after they finish.Test
test_watcher_reaps_finished_spawn_handlesseedsspawn_handleswith the handles of 64 completed tasks, waits for them to finish, then runs a normal poll cycle and asserts nothing is retained. It fails onmainwithretained 64 handlesand passes with this change.Full suite: 1186 passed, 0 failed (
cargo test -p claudear-engine --lib --features sqlite).cargo fmt --all -- --checkandcargo clippy --workspace --all-targets -- -D warningsare clean.Reported in Discord.
🤖 Generated with Claude Code