An unread exit code is unknown, not success - #148
Merged
Merged
Conversation
defineExitCode() falls back to scanning Nomad task events for the first
integer exitCode when the .exitcode file cannot be read. Nomad leaves
ExitCode at its zero value on every event that is not a task exit, so an
allocation that fails before the task body runs yields 0 from `Received`.
Captured from a live cluster (Nomad 1.11.2), image-pull failure:
Received ExitCode 0 Task received by client
Task Setup ExitCode 0 Building Task Directory
Driver ExitCode 0 Downloading image
Driver Failure ExitCode 0 Failed to pull image
Not Restarting ExitCode 0 Policy allows no restarts
ClientStatus=failed, Failed=true, and no .exitcode written.
The caller then reads that 0 as the worker reporting success and uses it to
override Nomad's own alloc-state failure, so Nextflow looks for outputs that
were never produced and reports MissingFileException against a process script
that was correct.
The existing coverage passes only because its fixture carries a single
Terminated event with the real code. This test uses the real sequence and
fails on current behaviour.
Not fixed here: this commit is the reproducer.
defineExitCode() fell back to scanning Nomad task events for the first
integer exitCode whenever the .exitcode file could not be read. Nomad
leaves ExitCode at its zero value on every event that is not a task exit,
so an allocation that failed before the task body ran returned 0 from
`Received`:
Received ExitCode 0 Task received by client
Task Setup ExitCode 0 Building Task Directory
Driver ExitCode 0 Downloading image
Driver Failure ExitCode 0 Failed to pull image
Not Restarting ExitCode 0 Policy allows no restarts
The caller reads a 0 as the worker reporting success and uses it to
override Nomad's own alloc-state failure. Nextflow was therefore told the
task succeeded, looked for outputs that were never produced, and raised
MissingFileException against a process script that was correct. The real
failure — an allocation that never started — had been reported by Nomad
and discarded.
The comment above the caller claimed a missing exit file could not
spuriously suppress, because defineExitCode returns Integer.MAX_VALUE when
it reads nothing. That held only if the events fallback did not exist.
Now only an event representing the task terminating is accepted, which is
the sole case where Nomad populates ExitCode with a real value. Everything
else falls through to MAX_VALUE, so the failure propagates and
failureMessage surfaces the allocation's own events. The genuine case the
fallback served — a worker killed before it could write .exitcode, where
Nomad recorded the real code on Terminated — still works.
Also tracks where the value came from, so the warning stops attributing an
events-derived code to `local .exitcode`. That is why the original report
read as self-contradictory: the log said the file was unreadable and then
quoted a value from it.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
When a task's
.exitcodefile cannot be read,defineExitCode()falls back toscanning the Nomad task events and returning the first integer
exitCodeitfinds. Nomad leaves
ExitCodeat its zero value on every event that is not atask exit, so an allocation that fails before the task body runs looks like
this — captured from an image-pull failure on Nomad 1.11.2:
ReceivedTask SetupDriverDriver FailureNot RestartingClientStatus=failed,Failed=true, and no.exitcodewritten anywhere. Thescan returns 0 from
Received.The caller reads a 0 as the worker reporting success and uses it to override
Nomad's own alloc-state failure. Nextflow is therefore told the task succeeded,
looks for outputs that were never produced, and raises:
That message points at the process script, which was correct. The real failure —
an allocation that never started — had already been reported by Nomad and was
discarded.
This is not a race. Any allocation failing before the task body runs is read as
success, deterministically.
The comment above the caller claimed a missing exit file could not spuriously
suppress, "because defineExitCode returns Integer.MAX_VALUE when it couldn't
read any signal". That holds only if the events fallback does not exist.
Fix
Only accept a code from an event that represents the task terminating, which is
the sole case where Nomad populates
ExitCodewith a real value. Everythingelse falls through to
MAX_VALUE, so the failure propagates andfailureMessagesurfaces the allocation's own events.The genuine case the fallback served still works: a worker killed before it
could write
.exitcode, where Nomad recorded the real code onTerminated(the existing
exitCode: 143OOM coverage still passes unchanged).Also tracks where the value came from, so the warning stops attributing an
events-derived code to
local .exitcode. That mislabelling is why the originalreport read as self-contradictory: the log said the file was unreadable and then
quoted a value from it.
Effect
Same pipeline, same cluster, one process whose container image does not exist:
Before
After
Tests
Two cases, both using the real five-event sequence above:
defineExitCodereturnsMAX_VALUErather than 0 when the allocation never ranerror names the pull failure
The first commit adds them as reproducers failing against current behaviour; the
second fixes it.
Full suite: 455 tests, 0 failures.