revert(watcher): back out #138 OOM/shutdown-drain fixes; always fix Sentry - #145
Conversation
This reverts commit a877a5d.
This reverts commit 0a0597d.
This reverts commit 4fc70e1.
This reverts commit ea378e0.
This reverts commit 24d1512.
This reverts commit 936eab6.
This reverts commit 5497d3f.
This reverts commit 9adabb5.
This reverts commit 4262991.
This reverts commit d6c4a00.
This reverts commit 948349f.
Sentry ships a stacktrace that pinpoints the defect, so an inability to reproduce at runtime must not deflect to a repro-steps reply.
ArnabChatterjee20k
changed the title
Greptile SummaryThis PR reverts watcher task-lifecycle and bounded-shutdown changes from #138 while retaining its other features, and routes Sentry reports into the fix pipeline even when runtime verification cannot reproduce them.
Confidence Score: 0/5The PR does not appear safe to merge because all five previously reported blocking lifecycle and diagnosis defects remain outstanding. Completed polling-task handles still accumulate in Files Needing Attention: crates/claudear-engine/src/watcher.rs, crates/claudear-engine/src/processing.rs, src/main.rs Important Files Changed
Reviews (2): Last reviewed commit: "linting" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| .process_issue(source_clone, issue, match_result, None, None, intent) | ||
| .await; | ||
| }); | ||
| self.spawn_handles.lock().await.push(handle); |
There was a problem hiding this comment.
Every polling dispatch adds its JoinHandle to spawn_handles, but completed handles are no longer reaped during normal daemon operation. Because the watcher remains alive and repeatedly polls for work, sustained issue processing retains one completed task allocation per issue. Memory usage can therefore grow without bound until the service is OOM-killed.
Knowledge Base Used:
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/claudear-engine/src/watcher.rs
Line: 3554
Comment:
**Completed Handles Accumulate Indefinitely**
Every polling dispatch adds its `JoinHandle` to `spawn_handles`, but completed handles are no longer reaped during normal daemon operation. Because the watcher remains alive and repeatedly polls for work, sustained issue processing retains one completed task allocation per issue. Memory usage can therefore grow without bound until the service is OOM-killed.
**Knowledge Base Used:**
- [AI review execution](https://app.greptile.com/appwrite/-/custom-context/knowledge-base/appwrite/claudear/-/docs/ai-review-execution.md)
- [Configuration and runtime operations](https://app.greptile.com/appwrite/-/custom-context/knowledge-base/appwrite/claudear/-/docs/configuration-and-operations.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
| let handle = tokio::spawn(async move { | ||
| watcher | ||
| .process_issue(source_clone, issue, match_result, None, None, intent) | ||
| .await; | ||
| }); | ||
| self.spawn_handles.lock().await.push(handle); |
There was a problem hiding this comment.
Dispatch checks is_running before waiting for capacity, then spawns without rechecking it while registering the task. If shutdown starts after the check but before process_issue increments active_processing, stop_and_drain can see zero and return while the task is still starting. Runtime teardown may then cancel repository or attempt-state work even though shutdown was reported as graceful.
Knowledge Base Used:
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/claudear-engine/src/watcher.rs
Line: 3549-3554
Comment:
**Shutdown Can Miss New Tasks**
Dispatch checks `is_running` before waiting for capacity, then spawns without rechecking it while registering the task. If shutdown starts after the check but before `process_issue` increments `active_processing`, `stop_and_drain` can see zero and return while the task is still starting. Runtime teardown may then cancel repository or attempt-state work even though shutdown was reported as graceful.
**Knowledge Base Used:**
- [AI review execution](https://app.greptile.com/appwrite/-/custom-context/knowledge-base/appwrite/claudear/-/docs/ai-review-execution.md)
- [Configuration and runtime operations](https://app.greptile.com/appwrite/-/custom-context/knowledge-base/appwrite/claudear/-/docs/configuration-and-operations.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| while self.active_processing.load(Ordering::SeqCst) > 0 { | ||
| if start.elapsed() > max_wait { | ||
| tracing::warn!( | ||
| remaining = self.active_processing.load(Ordering::SeqCst), | ||
| "Graceful shutdown timeout reached, some tasks may not have completed" | ||
| ); | ||
| break; | ||
| } | ||
| }) | ||
| .await; | ||
|
|
||
| if joined.is_ok() { | ||
| tracing::info!("Claude Watcher stopped gracefully"); | ||
| return; | ||
| tracing::info!( | ||
| active_count = self.active_processing.load(Ordering::SeqCst), | ||
| "Waiting for active tasks to complete..." | ||
| ); | ||
| // Wait for a task to finish (notifies via slot_available) or fall back | ||
| // to a periodic check in case the notification was missed. | ||
| let remaining = max_wait.saturating_sub(start.elapsed()); | ||
| let _ = tokio::time::timeout(remaining, self.slot_available.notified()).await; |
There was a problem hiding this comment.
The drain checks active_processing before registering the Notify waiter. If the final task decrements the counter and notifies between those operations, the wakeup is lost; approval-related early exits also decrement without notifying. Shutdown then waits for the full 30-second budget despite having no active work, which can exceed deployment termination windows.
Knowledge Base Used:
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/claudear-engine/src/watcher.rs
Line: 1159-1174
Comment:
**Drain Can Miss Completion Wakeups**
The drain checks `active_processing` before registering the `Notify` waiter. If the final task decrements the counter and notifies between those operations, the wakeup is lost; approval-related early exits also decrement without notifying. Shutdown then waits for the full 30-second budget despite having no active work, which can exceed deployment termination windows.
**Knowledge Base Used:**
- [AI review execution](https://app.greptile.com/appwrite/-/custom-context/knowledge-base/appwrite/claudear/-/docs/ai-review-execution.md)
- [Configuration and runtime operations](https://app.greptile.com/appwrite/-/custom-context/knowledge-base/appwrite/claudear/-/docs/configuration-and-operations.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| let always_fix = input.issue.source == "sentry"; | ||
| if verdict.reproduced || always_fix { | ||
| // Carry the diagnosis into the fix run. | ||
| input.diagnosis = Some(verdict); |
There was a problem hiding this comment.
A Sentry verdict with reproduced == false is now forwarded as the fix diagnosis, but build_diagnosis_context unconditionally tells the agent that the issue was independently reproduced and must not be questioned. When such a verdict contains useful details, the fix run receives factually incorrect instructions, increasing the risk of an unsupported change. The diagnosis text needs to distinguish telemetry evidence from successful reproduction.
Knowledge Base Used:
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/claudear-engine/src/processing.rs
Line: 2435
Comment:
**Unverified Diagnosis Claims Reproduction**
A Sentry verdict with `reproduced == false` is now forwarded as the fix diagnosis, but `build_diagnosis_context` unconditionally tells the agent that the issue was independently reproduced and must not be questioned. When such a verdict contains useful details, the fix run receives factually incorrect instructions, increasing the risk of an unsupported change. The diagnosis text needs to distinguish telemetry evidence from successful reproduction.
**Knowledge Base Used:**
- [AI review execution](https://app.greptile.com/appwrite/-/custom-context/knowledge-base/appwrite/claudear/-/docs/ai-review-execution.md)
- [Automated review workflows](https://app.greptile.com/appwrite/-/custom-context/knowledge-base/appwrite/claudear/-/docs/review-automation.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| scope.set_tag("app.component", "claudear-backend"); | ||
| }); | ||
|
|
||
| let runtime = tokio::runtime::Builder::new_multi_thread() | ||
| tokio::runtime::Builder::new_multi_thread() |
There was a problem hiding this comment.
Removing the explicit Runtime::shutdown_timeout restores unbounded default runtime teardown. If the watcher's 30-second drain gives up while work remains in a non-preemptible blocking operation, dropping the Tokio runtime can keep waiting indefinitely. A normal SIGTERM or redeploy may therefore never terminate the process.
Knowledge Base Used:
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/main.rs
Line: 1695-1698
Comment:
**Runtime Teardown Can Hang**
Removing the explicit `Runtime::shutdown_timeout` restores unbounded default runtime teardown. If the watcher's 30-second drain gives up while work remains in a non-preemptible blocking operation, dropping the Tokio runtime can keep waiting indefinitely. A normal SIGTERM or redeploy may therefore never terminate the process.
**Knowledge Base Used:**
- [AI review execution](https://app.greptile.com/appwrite/-/custom-context/knowledge-base/appwrite/claudear/-/docs/ai-review-execution.md)
- [Configuration and runtime operations](https://app.greptile.com/appwrite/-/custom-context/knowledge-base/appwrite/claudear/-/docs/configuration-and-operations.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
|
@greptile I am reverting it cause it was making the processing to much slow due to the synchronous dependency and anyways it wasn't protecting it from killing |
|
That makes sense. If the fix serialized processing behind a synchronous dependency, the throughput regression is a valid reason to revert it. Also, the shutdown drain could only bound graceful shutdown; it could not prevent an OOM/SIGKILL, and blocking work would remain non-preemptible anyway. The tradeoff is that reverting reintroduces join-handle retention and potentially slower runtime teardown, so monitoring handle/RSS growth after deployment is the right mitigation. The Sentry always-fix change is independent and should remain. |
1 participant
What does this PR do?
Backs out the watcher join-handle-leak / shutdown-drain work that shipped inside #138 (branch
fix/DISCORD-15363517-watcher-join-handle-leak), because that change is degrading the running daemon in production. Only those OOM-fix commits are reverted. Every other feature #138 carried is kept untouched: agent instructions (V10) and the dashboard instruction editors,routing_intent(V11), PR-review comment handling, intent classification, red-green enforcement, the resource monitor script, and Sentry-as-error classification.The 11 reverted commits:
It also adds one change: Sentry issues now always route to the fix pipeline instead of deflecting to a "share repro steps" reply. A Sentry event already ships a stacktrace that pinpoints the defect, so an inability to reproduce it at runtime should not stop us from fixing it. In production this deflection path stranded ~55% of Sentry traffic (77 of 141 issues got a repro-steps comment and no PR).
Test Plan
cargo buildpasses on the full workspace.MAX(version), so a prod DB already at V11 is unaffected.JoinHandlegrowth over time.Related PRs and Issues