fix(watcher): reap finished issue-processing task handles [DISCORD-15363517] - #138
fix(watcher): reap finished issue-processing task handles [DISCORD-15363517]#138claudear wants to merge 1 commit into
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 SummaryThis PR reaps completed issue-processing JoinHandles during polling and dispatch, drains tracked tasks during graceful shutdown, and boxes the processor future to reduce the spawned task’s inline state-machine footprint.
Confidence Score: 4/5The shutdown synchronization gap should be fixed before merging because graceful shutdown can still return with an untracked issue-processing task running. Spawning a task and inserting its handle are not synchronized with the one-shot shutdown snapshot, so shutdown can observe no active work, drain an empty vector, and then miss a newly inserted handle. Files Needing Attention: crates/claudear-engine/src/watcher.rs Important Files Changed
Prompt To Fix All With AI### Issue 1
crates/claudear-engine/src/watcher.rs:1185-1188
**Shutdown misses concurrently recorded tasks**
When shutdown overlaps a dispatch that has passed its running check but has not yet recorded its spawned task, `drain_spawned_tasks` takes the current handle vector before the new handle is inserted. The one-shot drain then returns without joining that task, causing `stop_and_drain` to report a graceful stop while issue processing continues to mutate tracker, notifier, or agent state.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "fix(watcher): reap finished issue-proces..." | Re-trigger Greptile |
| // Join the spawned tasks themselves so shutdown waits for their teardown too, | ||
| // and so their handles are released rather than dropped with the watcher. | ||
| let remaining = max_wait.saturating_sub(start.elapsed()); | ||
| let _ = tokio::time::timeout(remaining, self.drain_spawned_tasks()).await; |
There was a problem hiding this comment.
Shutdown misses concurrently recorded tasks
When shutdown overlaps a dispatch that has passed its running check but has not yet recorded its spawned task, drain_spawned_tasks takes the current handle vector before the new handle is inserted. The one-shot drain then returns without joining that task, causing stop_and_drain to report a graceful stop while issue processing continues to mutate tracker, notifier, or agent state.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/claudear-engine/src/watcher.rs
Line: 1185-1188
Comment:
**Shutdown misses concurrently recorded tasks**
When shutdown overlaps a dispatch that has passed its running check but has not yet recorded its spawned task, `drain_spawned_tasks` takes the current handle vector before the new handle is inserted. The one-shot drain then returns without joining that task, causing `stop_and_drain` to report a graceful stop while issue processing continues to mutate tracker, notifier, or agent state.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
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. |
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