Repository navigation
fix: Exit 130 or 143 when a signal interrupts a running task - #2759
Conversation
| // Set when the pipeline was stopped by a signal (130 / 143). | ||
| // Return it before the task-failure fallback, which would collapse | ||
| // an interrupt to 1. | ||
| if let Some(code) = self.session.exit_code.get() { |
There was a problem hiding this comment.
We should just set line 307 below to this value.
There was a problem hiding this comment.
Thanks Miles, that's cleaner 👍
exit_code now starts from self.session.exit_code.get(), and the separate early return is gone.
The signal code still wins over a task's code, and the SIGINT and SIGTERM tests still pass with 130 and 143.
f27f8d3 to
0292559
Compare
| // Capture and handle any signals. A task that was already running | ||
| // cancels the token before this point, so the status may already be | ||
| // set. Read the signal that was stored, instead of assuming interrupt. | ||
| if let Some(signal) = signal_slot.lock().ok().and_then(|slot| *slot) { |
There was a problem hiding this comment.
@devtechedge This branch is a bit confusing. Couldn't this just be done in the else branch? Or just merge the branches.
I don't think a slot is necessary.
There was a problem hiding this comment.
Good call, the slot was more machinery than this needed 🙂
I dropped the shared slot and merged the signal handling into a single branch that awaits the monitor task whenever the token is cancelled.
The monitor is the only thing that cancels that token and it returns right after, so the await doesn't block and we always get the real signal, even when a running task already set the status.
Clippy is clean, and the SIGINT and SIGTERM tests still exit 130 and 143 with the task counted as failed.
There was a problem hiding this comment.
Yeah this is much cleaner, thanks!
439079e to
9e03ff6
Compare
Merging this PR will degrade performance by 49.85%
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
| ❌ | hash_files[1000] |
9.3 ms | 18.6 ms | -49.85% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing devtechedge:fix-interrupt-exit-code (62737b7) with master (7ff97ad)
|
@milesj I pushed a small test hardening, and the earlier failures were Node download timeouts unrelated to this change. Could you approve the workflows when you get a chance? |
|
Thanks! |
|
Thanks for the reviews and the merge, Miles! The suggestions made this a lot cleaner. |
Thanks for the clear report in #2758.
When
moon runreceives SIGINT or SIGTERM while a task is already running, moon stops the task and exits 0.The summary also counts that task as skipped, so a wrapper such as
timeout 600 moon run app:test && deploytreats the interrupt as success.The in-flight job was finished as skipped even though its command had already run.
That job is now a failed run, and moon exits with 128 plus the signal number, 130 for SIGINT and 143 for SIGTERM.
A task that had not started is still skipped.
A SIGTERM now prints (terminated) rather than (interrupted), which is the status the pipeline already used when no task had started.
I checked this on Linux by sending SIGINT and SIGTERM to a running sleep task, and both the process status and the summary matched.
Fixes #2758