Skip to content

fix: resolve result_future on inference error instead of tearing down the model - #157

Open
AmirF194 wants to merge 1 commit into
SearchSavior:mainfrom
AmirF194:fix/153-worker-error-hang-and-unload-crash
Open

fix: resolve result_future on inference error instead of tearing down the model#157
AmirF194 wants to merge 1 commit into
SearchSavior:mainfrom
AmirF194:fix/153-worker-error-hang-and-unload-crash

Conversation

@AmirF194

@AmirF194 AmirF194 commented Aug 12, 2026

Copy link
Copy Markdown

Root cause

_commit_completed_packet fires registry.register_unload(model_name) on every
inference error before returning True, which breaks every queue_worker_* loop
out of its while True. Unload runs the pipeline destructor, which can touch a
corrupted device context after a real GPU error and SIGABRT the whole process,
taking every other loaded model down with it.

This is the second half of #153. The first half (the HTTP caller hanging
forever because result_future was never resolved) is already fixed on main
by 204f543 (PR #158's _mark_inference_error/_commit_completed_packet
rework), which also unified all 8 workers through one function. That refactor
kept the unload-on-error call, so the SIGABRT risk this issue reports is still
live on current main.

Fix

_commit_completed_packet now resolves the caller's future with the error and
returns False unconditionally, so the worker stays in its loop and the model
stays loaded, instead of unloading and exiting.

Verification

  • test_commit_resolves_error_without_unloading_the_model (renamed from
    test_commit_treats_error_field_not_error_prefix, same file) now asserts
    _commit_completed_packet returns False and registry.unloaded == [] on
    an error packet. Confirmed fails on unmodified main (AssertionError: assert True is False, current behavior unloads) and passes on this
    branch
    , in a clean python:3.12-slim container.
  • Full uv run pytest -W ignore tests/unit: 125 passed, no regressions.
  • Not checked: the reported SIGABRT itself, same caveat as before, no real
    Intel GPU/OpenCL context available in this session. This removes the
    register_unload call on the error path entirely, but I can't reproduce
    the abort to confirm it's gone, only that the destructor is no longer
    called on that path.

Fixes #153

@SearchSavior

Copy link
Copy Markdown
Owner

@AmirF194 Thanks for the PR!

RE:

Not checked: the reported SIGABRT itself, since that needs a real Intel
GPU/OpenCL context corrupted by a genuine device error, which this session
doesn't have.

Does this mean the proposed fix has not been tested on hardware?

@AmirF194

Copy link
Copy Markdown
Author

Correct, not on real Intel GPU/OpenCL hardware, this session doesn't have one. What's verified: the hung future (the caller never gets a response) and that all 8 worker functions share the same break-before-resolve pattern, both confirmed by running the workers directly. The SIGABRT itself I can't reproduce without a genuine device error on real hardware, so the fix removes the crash-triggering call (register_unload tearing down the model on the error path) rather than catching the abort, which should hold regardless, but I can't claim to have watched it not crash on your hardware. Happy to have someone with an Intel GPU confirm before merge if that matters here.

…odel

_commit_completed_packet (added in the upstream stream-error-surfacing
rework) still fires registry.register_unload() on every inference
error and exits the worker loop. That unload runs the pipeline
destructor, which can touch a corrupted device context after a real
GPU error and SIGABRT the whole process, taking every other loaded
model down with it (the second half of SearchSavior#153, the first half was fixed
by the stream-error rework already on main).

Resolve the caller's future and keep the worker/model alive instead.

Fixes SearchSavior#153
@AmirF194
AmirF194 force-pushed the fix/153-worker-error-hang-and-unload-crash branch from a4b5416 to 54c861c Compare August 18, 2026 06:22
@AmirF194

Copy link
Copy Markdown
Author

Rebased onto main. #158 landed a stream-error-surfacing rework (204f543) that already fixes the hang half of this issue and unifies all 8 workers through _commit_completed_packet, but it kept the unload-on-error call. Rewrote this PR to remove just that call from the new shared function instead of reintroducing the old per-worker structure. Body updated with the new diff and verification.

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.

LLM inference failure hangs the caller, and error-triggered unload can SIGABRT the whole server

2 participants