Skip to content

Fail signal-killed pipelines instead of ingesting their partial output - #1752

Merged
BryonLewis merged 1 commit into
mainfrom
fix/subprocess-signal-exit-code
Jul 16, 2026
Merged

Fail signal-killed pipelines instead of ingesting their partial output#1752
BryonLewis merged 1 commit into
mainfrom
fix/subprocess-signal-exit-code

Conversation

@mattdawkins

Copy link
Copy Markdown
Member

Problem

stream_subprocess only treated a positive return code as failure:

code = process.wait(30)
...
if code > 0:
    raise RuntimeError(...)
else:
    manager.write(f"\nProcess completed in ...")

Popen reports death by signal as a negative return code. A pipeline killed by SIGKILL — most commonly the OOM killer, at -9 — fails the code > 0 test and falls into the success branch.

Why it corrupts data

Callers act on that "success". run_pipeline proceeds straight to PUSHING_OUTPUT, uploads the output file the dead pipeline left behind, and posts it to postprocess, which imports it over the dataset's existing annotations.

So a pipeline OOM-killed partway through replaced the user's annotations with a truncated CSV — or an empty one — and the job still reported success.

Reproduced directly:

$ bash -c 'echo partial-output; kill -9 $$'
child stdout     : b'partial-output\n'
Popen returncode : -9
  'code > 0'  -> False   => success branch, partial output ingested
  'code != 0' -> True    => raises, upload/import skipped

Fix

Treat any nonzero code as failure, and name the signal in the error so an OOM-killed run is identifiable from the job log (was terminated by SIGKILL (9) rather than a bare number).

Cancellation is unaffected — it is detected and raised before this check.

Tests

server/tests/test_stream_subprocess.py runs real subprocesses, including one that SIGKILLs itself, and asserts it raises rather than returning normally.

🤖 Generated with Claude Code

stream_subprocess only treated a positive return code as failure.  Popen reports
death by signal as a negative return code, so a pipeline killed by SIGKILL -- most
commonly the OOM killer, at -9 -- fell through to the success branch.

Callers act on that: run_pipeline moves straight to PUSHING_OUTPUT, uploads the
output file the dead pipeline left behind and posts it to postprocess, which
imports it over the dataset's existing annotations.  A pipeline OOM-killed
partway through therefore replaced the user's annotations with a truncated CSV,
or an empty one, and the job was still reported as successful.

Treat any nonzero code as failure, and name the signal in the error so an
OOM-killed run is identifiable from the job log.  Cancellation is unaffected: it
is detected and raised before this check.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

@BryonLewis BryonLewis left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good

@BryonLewis
BryonLewis merged commit ef0499b into main Jul 16, 2026
3 checks passed
@BryonLewis
BryonLewis deleted the fix/subprocess-signal-exit-code branch July 16, 2026 12:42
mattdawkins added a commit that referenced this pull request Jul 16, 2026
Brings in 8 commits, including suppression regions (#1760), the
multicam camera order fix (#1759), signal-killed pipeline handling
(#1752, #1761), the annotation rollback tombstone fix (#1751), CSV note
columns (#1753), and the vitest/ws dependabot bumps.

Viewer.vue merged cleanly: f557b21 restored the SealTK wiring that
merge 8b0ab93 had dropped, so the file no longer conflicts with main.

Two conflicts, both from suppression regions landing on main while
viame/master already carried its own copy:

* LayerManager.vue -- keep viame/master's ../utils import. main's copy
  of this file diverged and no longer needs those symbols, but the
  merged body still calls geojsonToBound, isRotationValue,
  ROTATION_ATTRIBUTE_NAME and featureHasSegmentationPolygon.
* TypeSettingsPanel.vue -- take main's suppression row (adds mt-5).

package.json resolves to main's vitest 4.1.10 bump (viame/master never
changed it off the 3.2.4 base) while keeping onnxruntime-web.

Verified: client tests 611/611 passing on Node 22, eslint clean, desktop
app builds and runs with no console errors, and on a multicam stereo
dataset ImportAnnotations still completes setup with no Annotation Set
combobox and a live openUpload.

Note: vitest 4 requires Node ^20 || ^22 || >=24, so `npm test` now fails
to start on Node 18. client/README.md still says "Requires Node 18+".

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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.

2 participants