server: don't panic when a finished compile has neither code nor signal - #2776
server: don't panic when a finished compile has neither code nor signal#2776jetm wants to merge 1 commit into
Conversation
get_signal did `status.signal().expect("must have signal")`, assuming the
Unix invariant that an ExitStatus with no exit code was terminated by a
signal. That does not always hold: an ExitStatus reconstructed for a
distributed compile (or an abnormal wait status such as WIFSTOPPED) can
report neither a code nor a signal. When that happened the expect() panicked
the compile task, which the server surfaced as a misleading "Failed to bind
socket" and, under load, repeatedly fell back to local compilation.
Return Option<i32> from get_signal and assign it straight into res.signal, so
a compile that reports neither code nor signal leaves res.signal unset
instead of crashing the in-flight task. The Windows arm returns None rather
than panicking; ExitStatus::code() is always Some there, so the signal branch
is never reached anyway.
Add a unit test covering a real terminating signal (SIGKILL) and the
neither-code-nor-signal case (WIFSTOPPED via from_raw), which previously
panicked.
Signed-off-by: Javier Tia <javier@peridio.com>
(cherry picked from commit b2c46f6a2f48dbae8ac24ec5eefacac7dc124db2)
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2776 +/- ##
==========================================
- Coverage 70.90% 62.44% -8.47%
==========================================
Files 71 71
Lines 40393 35911 -4482
==========================================
- Hits 28642 22425 -6217
- Misses 11751 13486 +1735 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Merging this PR will degrade performance by 45.99%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ❌ | Memory | lru_sequential_scan_pattern |
128 B | 260 B | -50.77% |
| ❌ | Memory | lru_hotcold_access_pattern |
128 B | 260 B | -50.77% |
| ❌ | Memory | hash_multiple_files |
128 B | 260 B | -50.77% |
| ❌ | Memory | time_macro_finder_chunked |
128 B | 260 B | -50.77% |
| ❌ | Memory | lru_cache_get_hit |
128 B | 260 B | -50.77% |
| ❌ | Memory | hash_large_data |
192 B | 324 B | -40.74% |
| ❌ | Memory | hash_header_file |
192 B | 324 B | -40.74% |
| ❌ | Memory | cache_key_generation |
192 B | 324 B | -40.74% |
| ❌ | Memory | time_macro_finder_with_macros |
192 B | 324 B | -40.74% |
| ❌ | Memory | time_macro_finder_no_macros |
192 B | 324 B | -40.74% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing jetm:dist-server-signalless-panic (32fc12f) with main (7cd4f2c)
Footnotes
-
6 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
Part of #2775 (splitting the OpenEmbedded/Yocto sccache-dist work into focused
PRs; replaces the closed #2750).
Problem
get_signaldidstatus.signal().expect("must have signal"), assuming the Unixinvariant that an
ExitStatuswith no exit code was terminated by a signal. Thatdoes not always hold: an
ExitStatusreconstructed for a distributed compile (oran abnormal wait status such as
WIFSTOPPED) can report neither a code nor asignal. When that happened the
expect()panicked the compile task, which theserver surfaced as a misleading "Failed to bind socket" and, under load,
repeatedly fell back to local compilation. Found under a Yocto/BitBake host-mode
distributed build.
Approach
Return
Option<i32>fromget_signaland assign it straight intores.signal,so a compile that reports neither code nor signal leaves
res.signalunsetinstead of crashing the in-flight task. The Windows arm returns
Noneratherthan panicking;
ExitStatus::code()is alwaysSomethere, so the signal branchis never reached anyway.
Test
Adds a unit test covering a real terminating signal (SIGKILL) and the
neither-code-nor-signal case (
WIFSTOPPEDviafrom_raw), which previouslypanicked.