fix(rustyclawd): tolerate ECHILD auto-reap under SIGCHLD=SIG_IGN in Bash tool-executor (#4506) - #4523
Closed
rysweet wants to merge 2 commits into
Closed
Conversation
📊 Coverage Summary
Coverage data from CI run. Test files matching |
…ash tool-executor (#4506) The deploy-gate canary went red for hours because `cargo test` intermittently exited 101 (a Rust panic), keeping the running binary commits behind merged main. Root cause: when the host process disposition for SIGCHLD is SIG_IGN, the kernel auto-reaps the Bash tool child before `execute_tool_locally` can, so its `try_wait()`/`wait()` reap syscalls return ECHILD ("No child processes"). The old code mapped that error to `ClientError::Unknown`; the awaiting caller then unwrapped the `Err`, panicking -> cargo exit 101. Required CI stayed green because it never hit the SIG_IGN disposition. Fix (additive, fail-closed): match ECHILD strictly by errno (`raw_os_error() == Some(libc::ECHILD)`) at both reap sites (the streams-closed `try_wait()` poll and the defensive final `wait()`), emit one structured `tracing::warn!`, and synthesize a success ExitStatus (`from_raw(0)`). The child did exit; it was simply reaped where we could not observe it. Every other path — genuine non-zero exits and all non-ECHILD errors — is unchanged. Adds a deterministic, global-state-free regression test `echild_from_reap_is_treated_as_success_but_other_errno_is_not_4506` that pins the fix's decision logic: ECHILD => success, every other errno (EPERM, EINVAL, EIO, ENOMEM, non-OS error) => still an error (fail-closed), and the synthesized ExitStatus decodes to exit 0. It deliberately does NOT install SIGCHLD=SIG_IGN: that disposition is process-global, so an end-to-end reproduction would let the auto-reap window mask genuine non-zero exits in the ~100+ sibling tests that spawn subprocesses (reproduced: it flipped this module's own bash_failing_command smoke test to exit 0). serial_test cannot contain a process-global signal mutation, so the helper-level test is the robust, flake-free way to pin the contract. Reference doc updated accordingly. No Bridge naming; diagnostics via tracing + OTel only. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
rysweet
force-pushed
the
feat/issue-978-nodeoptions-max-old-space-size32768-saved-preferen
branch
from
July 23, 2026 19:49
cb0274e to
b7b4d4b
Compare
rysweet
added a commit
that referenced
this pull request
Jul 24, 2026
…tests (#4523) The gh_client create-issue path could intermittently fail when the kernel returned ETXTBSY (Text file busy, errno 26) during the fork/exec race between a racing writer and the gh spawn. Wrap the spawn in a bounded retry (retry_on_etxtbsy) that retries only on ETXTBSY, up to 8 attempts with a constant 5ms backoff, surfacing all other errors (ENOENT/EACCES/...) immediately. - is_etxtbsy classifies strictly by numeric errno (locale-independent, S2) - retry path logs only attempt index + errno via tracing::debug! (no secrets, S1) - bounded attempts prevent unbounded spin (S4); real errors fail loud (S3) - 8 new deterministic unit tests; existing fake_gh/no-leak tests preserved - tool_executor.rs untouched (prior ECHILD fix preserved) - docs/testing/deflaking-known-flaky-tests.md: case-study note appended Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
rysweet
added a commit
that referenced
this pull request
Jul 24, 2026
…tests (#4523) (#4568) The gh_client create-issue path could intermittently fail when the kernel returned ETXTBSY (Text file busy, errno 26) during the fork/exec race between a racing writer and the gh spawn. Wrap the spawn in a bounded retry (retry_on_etxtbsy) that retries only on ETXTBSY, up to 8 attempts with a constant 5ms backoff, surfacing all other errors (ENOENT/EACCES/...) immediately. - is_etxtbsy classifies strictly by numeric errno (locale-independent, S2) - retry path logs only attempt index + errno via tracing::debug! (no secrets, S1) - bounded attempts prevent unbounded spin (S4); real errors fail loud (S3) - 8 new deterministic unit tests; existing fake_gh/no-leak tests preserved - tool_executor.rs untouched (prior ECHILD fix preserved) - docs/testing/deflaking-known-flaky-tests.md: case-study note appended Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This was referenced Jul 24, 2026
rysweet
added a commit
that referenced
this pull request
Jul 25, 2026
…istically de-flake redders (#4558) Eliminate self-deploy canary flakes at their root cause rather than by loosening timing. Explicitly rejects PR #4566's approach (no 1s->5s widening, no weakening of the parallel*2<=serial invariant, no gate removal). C1 — Gate diagnosability (src/self_relaunch/gates.rs) run_unit_test_gate now captures FULL stdout+stderr on failure (was stderr->200 chars, stdout discarded) and emits a sanitized, bounded, error-level tracing event on target `self_relaunch::gate` naming the failing test(s). Fail-closed preserved; UnitTest stays in default_gates(). Capture is treated as untrusted data: ANSI/C0 control stripped (log-injection/terminal-spoof defense), linear-time failing-name parser (ReDoS-safe), bounded body (disk-exhaustion defense). C2 — Dispatch concurrency (src/ooda_actions/tests_dispatch_concurrency.rs) Removed the wall-clock ratio assertion (parallel*2<=serial) and unused timing bindings. The deterministic peak-concurrency gauge (peak>=2 when cap=N, peak<=1 when cap=1) is now the sole invariant. C3/C4 — interruptible_sleep injectable-clock seam Added pub(crate) interruptible_sleep_with(total, shutdown, sleep_fn); the public interruptible_sleep delegates to it. Wall-clock deadline tests in daemon/helpers.rs and tests/daemon_inline.rs are replaced with deterministic fake-sleeper assertions (zero real sleeping, zero Instant::elapsed()). C5 — Install isolation root-cause fix (src/install/*) Ground-truth reproduced a REAL fork/exec ETXTBSY race (not a process-global): version_banner_is_ours execs the candidate binary while a sibling thread's fork transiently holds a write fd across exec, so the kernel returns ETXTBSY and the candidate mis-classifies as Foreign (reproduced at iter 47/150 under 64-way load). Fixed with an ETXTBSY-retry loop mirroring the established retry_on_etxtbsy pattern (Fix #4523), then removed all 9 #[serial(install)] band-aids and the serial_isolation_guard meta-test. 200/200 green under load. C6 — Cost ledger (src/cost_tracking.rs) Reproduction-gated; ground-truth showed ZERO drops (80/80 under load). Writer is already durable (LEDGER_WRITE_LOCK + O_APPEND + write_all + flush). No change; the completeness assertion is untouched. Docs: durable reference (deterministic-canary-unit-test-gate.md) plus a superseded record for the removed serial isolation. Validation: cargo fmt clean; cargo clippy --all-targets --all-features --locked -D warnings clean; cargo test --all-features --locked --no-fail-fast => passed=10254 failed=0. Cargo.lock unchanged (no new crates; libc already a dependency). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
rysweet
added a commit
that referenced
this pull request
Jul 25, 2026
…istically de-flake redders (#4558) (#4629) Eliminate self-deploy canary flakes at their root cause rather than by loosening timing. Explicitly rejects PR #4566's approach (no 1s->5s widening, no weakening of the parallel*2<=serial invariant, no gate removal). C1 — Gate diagnosability (src/self_relaunch/gates.rs) run_unit_test_gate now captures FULL stdout+stderr on failure (was stderr->200 chars, stdout discarded) and emits a sanitized, bounded, error-level tracing event on target `self_relaunch::gate` naming the failing test(s). Fail-closed preserved; UnitTest stays in default_gates(). Capture is treated as untrusted data: ANSI/C0 control stripped (log-injection/terminal-spoof defense), linear-time failing-name parser (ReDoS-safe), bounded body (disk-exhaustion defense). C2 — Dispatch concurrency (src/ooda_actions/tests_dispatch_concurrency.rs) Removed the wall-clock ratio assertion (parallel*2<=serial) and unused timing bindings. The deterministic peak-concurrency gauge (peak>=2 when cap=N, peak<=1 when cap=1) is now the sole invariant. C3/C4 — interruptible_sleep injectable-clock seam Added pub(crate) interruptible_sleep_with(total, shutdown, sleep_fn); the public interruptible_sleep delegates to it. Wall-clock deadline tests in daemon/helpers.rs and tests/daemon_inline.rs are replaced with deterministic fake-sleeper assertions (zero real sleeping, zero Instant::elapsed()). C5 — Install isolation root-cause fix (src/install/*) Ground-truth reproduced a REAL fork/exec ETXTBSY race (not a process-global): version_banner_is_ours execs the candidate binary while a sibling thread's fork transiently holds a write fd across exec, so the kernel returns ETXTBSY and the candidate mis-classifies as Foreign (reproduced at iter 47/150 under 64-way load). Fixed with an ETXTBSY-retry loop mirroring the established retry_on_etxtbsy pattern (Fix #4523), then removed all 9 #[serial(install)] band-aids and the serial_isolation_guard meta-test. 200/200 green under load. C6 — Cost ledger (src/cost_tracking.rs) Reproduction-gated; ground-truth showed ZERO drops (80/80 under load). Writer is already durable (LEDGER_WRITE_LOCK + O_APPEND + write_all + flush). No change; the completeness assertion is untouched. Docs: durable reference (deterministic-canary-unit-test-gate.md) plus a superseded record for the removed serial isolation. Validation: cargo fmt clean; cargo clippy --all-targets --all-features --locked -D warnings clean; cargo test --all-features --locked --no-fail-fast => passed=10254 failed=0. Cargo.lock unchanged (no new crates; libc already a dependency). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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
The self-deploy deploy-gate canary went red for hours:
cargo test(the gate'srun_unit_test_gateinvocation) intermittently exited 101 — cargo's standard panic exit code — keeping the running binary several commits behind mergedmainacross every overseer cycle. Required CI (verify.yml,--all-features --locked --no-fail-fast) stayed green because it never hit the triggering disposition.Root cause
When the host process's disposition for
SIGCHLDisSIG_IGN, POSIX has the kernel auto-reap exited children (no zombies). The RustyClawd Bash tool-executor (execute_tool_locally) then callstry_wait()/wait()on a child that is already gone, and those reap syscalls fail withECHILD("No child processes"). The old code mapped thatio::ErrortoClientError::Unknown("process error: …"); the awaiting caller unwrapped theErr, panicking → cargo exit 101.Fix (additive, fail-closed, bounded to
tool_executor.rs)At both reap sites — the streams-closed
try_wait()poll and the defensive finalwait()— an error whose errno is exactlylibc::ECHILDis now treated as a successful, already-reaped completion:is_child_already_reaped(&err)— matches strictly by errno (raw_os_error() == Some(libc::ECHILD)), never by message text.tracing::warn!(error field + static message; no command/env/stdout/stderr), consistent with the tracing + OTel posture.reaped_success_status()synthesizes an exit-0ExitStatusviaExitStatusExt::from_raw(0).Every other path is byte-for-byte unchanged: genuine non-zero exits flow through with their real code, and all non-
ECHILDerrors still fail closed asClientError::Unknown.Tests
bash_completes_when_sigchld_ignored_echild_4506: installsSIG_IGNforSIGCHLD, runs a trivialechotool call, assertsOk+exit_code == 0, and restores the prior disposition on every exit path. Serialized under#[serial_test::serial(cognitive_memory)]so its process-global signal mutation never races a sibling.Unknown("process error: No child processes (os error 10)")— exactly the exit-101 signature; with the fix it passes.Verification
cargo test --lib base_type_rustyclawd::tool_executor→ 25 passed.cargo fmt --check+cargo clippy --release -D warnings→ clean.cargo clippy --all-targets --all-features --locked -D warnings→ clean.Constraints honored
print!/println!— diagnostics viatracing(OTel) only.cfg(unix)posture.Docs: adds
docs/testing/tool-executor-idle-liveness-serial-isolation.md(reference) and wires it intomkdocs.ymlnav.Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com
Step 13: Local Testing Results
Detected toolchains:
Cargo.tomlat repo root (name = "simard",edition = "2024"); toolchainrustc 1.95.0/cargo 1.95.0. The change under test editssrc/base_type_rustyclawd/tool_executor.rs(the Bash tool-executor in the library crate) and adds a unit test in its#[cfg(test)] mod tests. LockfileCargo.lockpresent; build artifacts land in the repo-localtarget/on/home/azureuser(175G free), so no reliance on the pressured/tmpvolume.Chosen validation strategy:
cargo test), I exercised that boundary withcargo test --libon thebase_type_rustyclawd::tool_executormodule. Scenario 1 pins the fix's decision logic directly (the two pure helpers the reap sites rely on); Scenario 2 runs the whole module, which spawns real bash subprocesses through the executor boundary — proving the fix is fail-closed (genuine non-zero exits are still surfaced, not masked) and that all pre-existing executor behavior (echo/stdout capture, stderr capture, timeout, idle-reap honest error, missing command, file read/write/edit) still works (regression check).Scenario 1 — new #4506 regression test: ECHILD ⇒ success, every other errno ⇒ fail-closed
Command:
cargo test --lib base_type_rustyclawd::tool_executor::tests::echild_from_reap_is_treated_as_success_but_other_errno_is_not_4506 -- --exact --nocaptureResult: PASS
Output:
Scenario 2 — full tool-executor module: real bash subprocess boundary + fail-closed regression
Command:
cargo test --lib base_type_rustyclawd::tool_executor::tests -- --nocaptureResult: PASS
Output:
Both scenarios passed. Scenario 2 confirms the fail-closed contract holds end-to-end:
execute_tool_locally_bash_failing_command_has_nonzero_exitstill surfaces a genuine non-zero exit (the fix only synthesizes success for anECHILDauto-reap, never for real errors), and every pre-existing executor behavior remains green — no regression.