fix(gpu): record WHERE a wedged job was stuck when its budget expires (#1338) - #1355
Conversation
Three reporters on v0.4.2 hit "TTS generate ran for more than 300s of actual compute time and was abandoned" (#1338, #1329, #1348) — two of them on an RTX 3050 and an RTX 3060, rendering a single sentence. That is not a machine too slow for the job. The message says "too heavy for the available compute" because it is the only story the timeout path can tell. And nothing in the log could contradict it. The timeout branch logged THAT the budget was exceeded, reset the pool, and returned. The worker cannot be cancelled, so it was still running on a real stack — and we threw that away, which is why every report of this class arrives undiagnosable and the only advice available is "reproduce it under a debugger". sys._current_frames() reads the frame of every live thread including one wedged inside a C call, which is exactly this case. Filtered to gpu-pool workers so the log names the stuck job rather than the web server, capped at 25 frames, and it can never raise — a diagnostic that throws would replace a real GpuJobTimeoutError with an unrelated crash. Ordering is load-bearing and asserted: the capture runs BEFORE reset(), because reset() swaps in a fresh executor and the wedged thread then stops being identifiable as a pool worker — the diagnostic would still run, still log, and be empty, which looks like it worked. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
| Filename | Overview |
|---|---|
| backend/services/model_manager.py | Adds fail-safe timeout stack capture and resolves the prior stale-worker attribution issue by identifying workers belonging to the current executor. |
| tests/test_gpu_wedge_stack_capture.py | Covers stack visibility, filtering, ordering before reset, sanitization, stale-worker labels, and fallback behavior. |
| CHANGELOG.md | Documents the new timeout diagnostic in the unreleased fixes. |
Reviews (4): Last reviewed commit: "Merge remote-tracking branch 'origin/mai..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughGPU timeout handling captures and sanitizes GPU worker stacks before pool reset. It labels current and stale workers and suppresses diagnostic failures. Tests cover filtering, ordering, redaction, and fallback behavior. The changelog documents TTS timeout-stage reporting. ChangesGPU timeout diagnostics
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 9✅ Passed checks (9 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/services/model_manager.py`:
- Around line 647-658: Redact home-directory paths from the captured stack text
before the logger.warning call in the stack-reporting flow, preserving the
existing stack content otherwise. Reuse the project’s existing home-path
redaction helper if available, and add coverage for Unix and Windows-style home
prefixes confirming account names are absent from the emitted warning.
In `@tests/test_gpu_wedge_stack_capture.py`:
- Around line 98-101: Update test_capture_is_empty_when_no_pool_worker_exists to
control the process-global worker state before calling
log_gpu_pool_worker_stacks. Monkeypatch threading.enumerate() to return no
gpu-pool workers, or establish an equivalent isolated precondition, while
preserving the assertion that the method returns an empty string.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: c15d8d23-7f89-4039-8a2f-460bb197e10f
📒 Files selected for processing (3)
CHANGELOG.mdbackend/services/model_manager.pytests/test_gpu_wedge_stack_capture.py
…r-stacks # Conflicts: # CHANGELOG.md
|
CI caught a real defect in my own test rather than in the code: Fixed by manufacturing the absence instead of assuming it: the filter prefix is monkeypatched to something nothing uses, which tests the branch rather than the environment. Verified both ways — |
Both review findings on this PR, both valid. CodeRabbit (CWE-532): traceback frames carry absolute source paths, which on a user machine start with their home directory — their account name. This log lands in backend.log, which goes into diagnostic bundles and prefilled bug reports, so it has to be sanitized like every other surfaced text. Reuses core.failure.sanitize rather than inventing a second answer; if sanitizing itself fails the stacks are dropped, not logged raw. greptile P1: a wedged worker survives reset() — it cannot be cancelled and keeps running under the same gpu-pool name the replacement pool uses. The second timeout in a session would log both with nothing to tell them apart, and the stale one is the more misleading, since it names an operation that is not the job that just failed. The live pool is now identified through its own thread set and the others are marked STALE. That set comes from ThreadPoolExecutor._threads, which is private, so unknown internals degrade to labelling nothing rather than to failing — a diagnostic that vanishes because an attribute moved is worse than an unlabelled one, and that degrade path has its own test. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Both findings valid and fixed. CodeRabbit (CWE-532) — the important one. greptile P1 — also real. A wedged worker survives That set comes from Three new tests; the redaction and stale-label ones both fail against the previous commit. Related suites green (43 passed). |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/test_gpu_wedge_stack_capture.py`:
- Around line 197-198: Ensure readiness-check failures still execute cleanup by
moving the assertion at tests/test_gpu_wedge_stack_capture.py:197-198 inside its
surrounding try/finally; likewise move both readiness checks at
tests/test_gpu_wedge_stack_capture.py:248-249 and the assertion at
tests/test_gpu_wedge_stack_capture.py:285-286 inside their respective try
blocks, preserving the existing cleanup behavior.
- Around line 200-206: Update the redaction test in the worker stack capture
test to construct a worker frame whose filename is explicitly nested under the
resolved home directory, rather than relying on the checkout path. Assert that
this home-path prefix is redacted from the captured text while the worker
function name remains present, ensuring the test fails before and passes after
the redaction fix.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: c9a0afc9-bad4-4137-b21d-a8825032a411
📒 Files selected for processing (2)
backend/services/model_manager.pytests/test_gpu_wedge_stack_capture.py
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/services/model_manager.py
| assert entered.wait(5) | ||
| try: |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Guarantee cleanup after a failed readiness check.
At Line 197, Line 248, and Line 285, a failed Event.wait() assertion occurs before finally, leaving a pool worker blocked and able to contaminate later process-wide captures.
tests/test_gpu_wedge_stack_capture.py#L197-L198: move the readiness assertion insidetrytests/test_gpu_wedge_stack_capture.py#L248-L249: move both readiness checks insidetrytests/test_gpu_wedge_stack_capture.py#L285-L286: move the readiness assertion insidetry
As per coding guidelines, “Fix the root cause and entire bug class.”
📍 Affects 1 file
tests/test_gpu_wedge_stack_capture.py#L197-L198(this comment)tests/test_gpu_wedge_stack_capture.py#L248-L249tests/test_gpu_wedge_stack_capture.py#L285-L286
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_gpu_wedge_stack_capture.py` around lines 197 - 198, Ensure
readiness-check failures still execute cleanup by moving the assertion at
tests/test_gpu_wedge_stack_capture.py:197-198 inside its surrounding
try/finally; likewise move both readiness checks at
tests/test_gpu_wedge_stack_capture.py:248-249 and the assertion at
tests/test_gpu_wedge_stack_capture.py:285-286 inside their respective try
blocks, preserving the existing cleanup behavior.
Source: Coding guidelines
| home = os.path.expanduser("~") | ||
| # This test file lives under the developer's home on any dev machine | ||
| # and under the runner's home in CI, so the raw frame necessarily | ||
| # contains it — which is what makes this a real check rather than a | ||
| # tautology. | ||
| assert home and home != "~", "cannot verify redaction without a home dir" | ||
| assert home not in text, ( |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Make the redaction input deterministic.
At Line 200, home comes from the runner, but the worker stack uses the checkout path; if the checkout is outside home, the assertion passes without testing redaction. Create a worker frame with a filename explicitly below home, then assert that this prefix is removed while the function name remains. As per coding guidelines, “add a fail-before/pass-after regression test,” and path instructions, “the test would fail before the fix and pass after.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_gpu_wedge_stack_capture.py` around lines 200 - 206, Update the
redaction test in the worker stack capture test to construct a worker frame
whose filename is explicitly nested under the resolved home directory, rather
than relying on the checkout path. Assert that this home-path prefix is redacted
from the captured text while the worker function name remains present, ensuring
the test fails before and passes after the redaction fix.
Sources: Coding guidelines, Path instructions
…r-stacks # Conflicts: # CHANGELOG.md
1 participant
Makes #1338 / #1329 / #1348 diagnosable. Does not fix the underlying hang — see "What this is not" below.
The problem with the reports
Three reporters on v0.4.2 hit "TTS generate ran for more than 300s of actual compute time and was abandoned". Two of them were on an RTX 3050 and an RTX 3060, rendering a single sentence.
GpuJobTimeoutErroris only raised once a worker has picked the job up — queue wait is bounded separately and surfaces as a retryable 503 — so that is 300+ seconds of execution, not of waiting in line.On a 12 GB card, one sentence is not a workload. But the message says "too heavy for the available compute", because that is the only story the timeout path is able to tell.
And nothing in the log could contradict it. The timeout branch logged that the budget was exceeded, reset the pool, and returned. The worker thread cannot be cancelled, so it was still running, still on a real stack — and we discarded it. Every report of this class therefore arrives undiagnosable, and the only advice available is "reproduce it under a debugger", which is not a reasonable thing to ask of someone who just wanted to dub a video.
The change
log_gpu_pool_worker_stacks()— at timeout, before abandoning the job, dump the current frame of every GPU-pool worker.sys._current_frames()reads the frame of every live thread including one wedged inside a C call, which is precisely the case that matters: the worker is stuck somewhere in the model, and it is the one thing we cannot ask it about._GPU_POOL_THREAD_PREFIX. The process also holds a web server, a watchdog, and a watermark pool; a dump of everything is a dump nobody reads.GpuJobTimeoutErrorwith an unrelated crash.reset(), and that ordering is asserted rather than assumed.reset()swaps in a fresh executor, and once the wedged thread is no longer named as a pool worker the capture finds nothing — it would still run, still log, and be empty, which is worse than not having it because it looks like it worked.It logs at WARNING, so it lands in
backend.logand therefore in diagnostic bundles and auto-filed bug reports without the user doing anything.What this is not
This does not fix the hang. It makes the next report of it carry the one fact needed to fix it: which frame the job died in. If the deepest frame is inside the model's forward pass, it is a hang and the "too heavy for the available compute" wording is actively misleading; if it is in a host-to-device copy, the VRAM story was right after all. Right now we cannot tell those apart, which is why #1338 has been stuck on "please attach a log" for a week.
Tests
tests/test_gpu_wedge_stack_capture.py— 5 tests, all failing before this change:run_on_gpu_pool_guarded: the capture happens beforereset(), and the captured text names the wedged function.Related suites green:
test_generate_timeout_730,test_gpu_pool_queue_accounting_1190,test_recurrence_hardening.One thing I noticed while running these and did not touch here:
tests/test_load_budget_split_1033.py::test_speech_survives_a_load_slower_than_the_generate_budgetfails when the file is run on its own and passes in the full suite, on cleanmainas well as on this branch. That is order-dependence in the opposite direction from #1269 and belongs in its own change.When a GPU job exceeds its execution-time budget, the timeout path logs up to 25 sanitized stack frames for each GPU-pool worker before resetting the pool. The capture filters unrelated and stale workers, supports C calls, and suppresses diagnostic failures so timeout handling continues. Reviewers should assess WARNING-level log volume and confirm sanitization remains fail-closed.