Repository navigation
test(runtime-core): bind stream relay publisher test ports in the publisher - #382
Merged
Merged
Conversation
The stream relay contract probed a free loopback port by binding 127.0.0.1:0 and dropping the listener before the publisher bound it. Under parallel tests another socket could take the port first; the publisher then failed bind() and exited 73 (EX_CANTCREAT) before writing its readiness line, which the helper reported as "readiness EOF" (CI run 39284084093). `stream-relay --publish 0` now binds a kernel-chosen loopback port and reports it as "ready <port>\n". Explicit ports and Unix frontends keep the bare "ready\n" line, and port 0 stays refused (64) in every other form, so existing callers are unchanged. Every test that used the released probe now publishes on port 0 or holds the port it needs, and the control-startup test drops its exit-73 retry. Refs HACK-1284 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ion contract The port-0 refusal cases waited on the child with output(), so a publisher that wrongly admitted "--publish 00" kept serving and hung the test run instead of failing it (found by mutating the exact-"0" check). Poll the child against a 3s deadline, then kill and reap it and fail with the admitted command. Refs HACK-1284 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
|
Found 1 test failure on Blacksmith runners: Failure
|
roodboi
marked this pull request as ready for review
October 22, 2026 08:11
roodboi
added a commit
that referenced
this pull request
Oct 22, 2026
provider::publisher::tests::Fixture picked a port by binding 127.0.0.1:0,
reading the port and dropping the probe, then started stream-relay-host on
that explicit port. In the shared parallel test process an unrelated test's
pre-exec fork (process::clean_command) can still hold the probe listener, or
another test can take the port, and the publisher exits 73 without
readiness ("publisher readiness missing", 2 of 5 full-suite runs under fork
pressure with a 20 ms pre-exec hold).
Start the publisher with --publish 0 (from #382): it binds a kernel-chosen
port and reports it as "ready <port>", which the fixture now requires.
Refs HACK-1347
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
roodboi
added a commit
that referenced
this pull request
Oct 24, 2026
…s unique (#399) ## Summary Fix unit G40: two runtime-core test reliability defects, following #387. Test-only; no product code changes. Rework after verification: the verifier found that part of the HACK-1347 audit was still open. Several tests outside the first commit's modules still checked a dropped listener in the shared process, and the full lib suite failed 0/5 under the brief's fork-pressure condition. The rework isolates or de-exposes every such test found (see the audit), and merges `origin/next` (b90b67e) for #382's `--publish 0`. | Issue | Root cause | Change | |---|---|---| | HACK-1347 | In the shared parallel test process, an unrelated test's pre-exec fork (`process::clean_command` sets a `pre_exec` umask, so std uses fork+exec, not `posix_spawn`) holds a copy of every open descriptor until it execs. `HostEndpoint::discover` (`host_endpoint.rs:202-247`) scans every same-user process running the selected executable; the test binary is both the scanner and the selected executable, so the fork is a second exclusive holder of the test's listener (`tests.rs:78`, ambiguity refusal, the CI signature) or still holds a listener the test already dropped (`tests.rs:84`). The `lsof` release checks (`lock_absent_disks_within`) see the fork's copy of a disk handle the fixture just wrote or read, and socket probes see a dropped listener still accepting, so they correctly refuse before the result under test. | Run each exposed fixture alone in a child test process with the shared `test_executable::run_isolated` helper from #387 (new shared `ISOLATED_HANG_GUARD`, 120 s). See the audit below. | | HACK-1348 | Fixtures named scratch directories `<prefix>-<pid>-<nanos>` and created them exclusively. macOS reports the wall clock in whole microseconds (nanos always end in `000`), so two parallel tests in one process can read the same value and the second create fails with `File exists (os error 17)` (`witnessed/tests.rs:22`). | New test-only `crate::next_test_scratch()` (process-unique sequence) appended to every pid+timestamp scratch name in the lib tests: the witnessed fixture and 18 sibling sites in 14 other files. The 4 integration-test files with the same pattern each have one test and one call, so they cannot collide in-process; they are unchanged. Regression test `test_scratch::tests::parallel_fixtures_reading_one_clock_tick_create_distinct_directories`. | ### HACK-1347 audit: what runs isolated now Rule: a test that runs a release or liveness check in the shared process (endpoint discovery, the `lsof` disk-handle check, a dropped listener that must refuse connections, an `flock` the fixture dropped and the check must acquire), and expects anything other than the fail-closed refusal, runs isolated, or its fixture stops depending on a dropped descriptor. First commit (1c14276): - `host_endpoint`: `discovery_requires_the_exact_executable_and_live_exclusive_listener` (the reported test), `executable_pinned_discovery_follows_a_new_listener_process`, and the two close/rebind fixtures that still shared the process (`replacement_between_precheck_and_connect_receives_no_payload`, `backend_fin_does_not_bypass_listener_replacement`). The module's own runner now delegates to `run_isolated` (its old 5 s watchdog for launching the whole binary becomes the 120 s hang guard; it now also requires the exact child's `... ok` line). - `admission_pool`: every release-check test that expects a pass or a non-active refusal (7 more, behind one `isolated()` helper that keeps holding `serial` so the module's whole-host scans stay one at a time). This closes the gap #387 left open. - `short_home` (4), `host_filesystem` (7, every test whose `inspect` must pass first), `interrupted` (1): recovery, migration and stop tests that need the `lsof` proof to pass. Rework (f6bc8ed, cb6661f), the rest of the brief's audit. The verifier's run found these still exposed; a full all-features lib run under fork pressure (below) found the rest: - `test_executable` is now compiled on every Unix target (`cfg(all(test, unix))`), because `quiescent_recovery` also runs on Linux. `sleeping_executable` stays macOS-only in use. - Isolated with `run_isolated` (each module has one `isolated()` helper with the reason; each child is `#[ignore]`d with its parent's name): - `graph::dependency_slots`: `recovery_refuses_live_and_replaced_sockets_without_touching_siblings`, `recovery_preflights_entire_assignment_before_unlinking` (a dropped listener must refuse). - `dependency_socket::quiescent_recovery`: `first_socket_refusal_records_closed_live_listener_facts_without_removal`, `live_listener_and_foreign_replacement_are_never_removed`. - `graph::foreground::transport`: the 7 `abandoned_publisher` tests that expect retirement or the dead-owner guard to pass (`recovered_publisher_retirement...`, `cleanup_fence...`, `retired_recovery_refuses_pending...`, `retired_recovery_rechecks...`, `interrupted_socket_move...`, `selected_legacy_device_socket_move...`, `dead_owner_guard...`). The fixture drops a live publication, so both its listener and its operation-lock `flock` can survive in a fork. - `graph::acknowledged_publisher::missing_lock`: all 5 tests (they all build on the dead- publisher fixture, which requires the dropped listener closed). - `graph::native::foreground`: `abandoned_publication_is_evidence_and_never_implicit_recovery_authority` (needs `native- config-plan`). - `https_recovery`: `full_command_preserves_large_public_mode_ca_and_exact_retry`, `full_legacy_command_archives_with_real_current_witness_and_exact_retry` (the fixture drops the provider `flock` that recovery must acquire). - Fixtures that only need a stale socket inode no longer make it from a dropped listener. New test-only `provider::test_socket::bound_unlistened` binds a socket that never listens, so every copy refuses connections (shared with the existing `quiescent_recovery` fixture, which already did this inline). Used by the `https_recovery` fixture (`owner.sock`) and `dependency_slots::fixture_prior_boot` (slot sockets). Regression test `test_socket::tests::an_inherited_copy_of_an_unlistened_socket_still_refuses_connections`. - `engine::relay_exec::framing_bounds_reject_truncation_invalid_stream_and_overflow`: shuts the peer down instead of only dropping it, and now holds a cloned copy of the peer for the whole check, so it proves the truncation refusal with an inherited copy present, every run. - `provider::publisher` stop fixture: started `stream-relay-host` on a probe port it had bound and dropped. A fork's copy of the probe (or another test) kept the port, and the publisher exited 73 without readiness. It now uses `--publish 0` from #382 and requires the `ready <port>` line. This needed `next`, so the branch merges `origin/next` (b90b67e). - Audited, unchanged: `rebind` (isolated in #387; `nested_tests` keeps its listener in a child that never forks), `network_update` and the remaining `short_home`/`host_filesystem`/`admission_pool`/`transport` tests (they assert only the fail- closed refusal, or refuse before any host-wide check), `identity::executable_running` users (a pre-exec fork runs the test binary, never the pool's provider path), `https_recovery::rejects_a_live_inherited_unix_listener_at_the_pinned_inode` (it already polls the dropped listener for 1 s), and other fixtures that drop an `flock` (`interrupted_start_cleanup`, `live_owner_cleanup::relay`, `retained_startup`, `persistent_data::engine`, `same_boot_absent_relay`): none failed in the pressure runs; see Gaps. ### Product question: can product forks cause false ambiguity refusals? `process::clean_command`, `node::supervisor` and `node::transport` all use `pre_exec`, so they fork+exec and have a pre-exec window in which the child runs hack-local's image and holds all of its descriptors. Discovery only counts processes running the user-selected dependency executable, and hack-local never selects itself, so a hack-local fork cannot be a second holder in the product. Only the test binary, which is both the scanner and the target, sees this. `FD_CLOEXEC` would not help: the descriptors are already close-on-exec, and the window ends at exec. No product change. A separate, real descriptor leak found during the audit is reported in Findings (non-atomic close-on-exec for sockets on macOS). ## Evidence "Fork pressure" adds a scratch co-tenant test (never committed) to the same test process: 4 threads loop `Command` + `pre_exec` (umask) on `/usr/bin/true`, the same fork+exec path `process::clean_command` takes, and it starts first. "Hold N ms" makes those children sleep N ms before exec, to model a stalled pre-exec child on a loaded host. "CPU load" is `nice` busy loops stopped by a shell trap. ### Rework (this Mac, Apple Silicon, 18 CPUs, already at load average 10-24 from other work; Rust 1.97.1) Full all-features lib test binary (CI's `--all-features` set, 1296-1297 tests), `--test- threads=16`, fork pressure for 90 s: | Condition | Before (PR head 1c14276) | After | |---|---|---| | hold 20 ms | **0/3**: 9-13 failures per run, all dropped-listener or dropped-`flock` refusals: `missing_lock` (4 of its 5 tests, `missing_lock.rs:832`), `dependency_slots` (`:719`, `:735`), `transport` (3), `native::foreground` (1), `https_recovery` (3), `quiescent_recovery` (1) | **5/5** at cb6661f | | no hold, 4 CPU loops (the verifier's condition) | verifier, on the M3: **0/5** | **4/5** at cb6661f; the one failure is a native runtime `graph_timeout` (Findings 7), not a descriptor refusal | Intermediate builds showed the remaining exposures one at a time. After the first isolation round, every run still failed on the two `https_recovery` full-command tests (`flock` drop, 0/3). After those, `provider::publisher` failed 2/5 with `publisher readiness missing` (exit 73: the dropped probe port). The publisher fixture change fixed that. Mutations (each new test must fail without the property it guards): | Mutation | Result | |---|---| | `relay_exec` framing test: remove the peer `shutdown` (keep the cloned copy) | killed: `relay_exec.rs:338` (no end of stream, so no truncation refusal) | | `test_socket::bound_unlistened`: `listen()` after `bind()` | killed: the regression test's connect succeeds (`test_socket.rs:73`) | | run_isolated wrapper with a misspelled child (1c14276, same helper) | wrapper fails: `assertions did not run` | Every isolated parent passes only if its child's exact `test ... ok` line and `1 passed` are in the child's output, so a missing or renamed child cannot pass silently. Linux: `cargo +1.97.1 clippy --all-targets -D warnings --target x86_64-unknown-linux-gnu` (cross-checked with zig cc, this Mac): exit 0, which covers `test_executable` and `test_socket` now compiling on Linux. Running the Linux tests is left to hosted CI. ### First commit (1c14276, M3, 16 CPUs) | Condition | Before (`next` 16f4547) | After | |---|---|---| | `host_endpoint` + fork pressure + CPU load | **3/10** (7 x `tests.rs:84` or `:78`) | 20/20 | | `host_endpoint`, `admission_pool`, `short_home`, `host_filesystem`, `interrupted` + fork pressure + CPU load | **4/10** (6 x discovery, CI signature) | 10/10 | | same modules, hold 20 ms | **0/3** | 3/3 | | same modules, hold 1000 ms | **0/3** | 2/3 (two `host_endpoint` drop tests whose own 1 s read timeout is shorter than the hold) | | `test_scratch` regression test, sequence mutated to 0 | **1/10** (9 x `File exists (os error 17)`, the CI signature) | 10/10 | The verifier independently confirmed these at 1c14276 (`host_endpoint` 20/20 vs next 3/10; the 5 modules 8/8 vs 3/5; the `test_scratch` mutation 0/10). ### Gates at cb6661f - `cargo +1.97.1 fmt --check`: exit 0. `clippy --all-targets -D warnings`, default and `--all- features` (with the pinned Zig): exit 0 (this Mac). - `cargo test` (the `test:local` command, private target dir): exit 0; lib 1049 passed, 101 ignored; 21 binaries, 1204 passed in total. - CI's `--all-features` `cargo test`: the first run, concurrent with this unit's 4-CPU-loop stress, failed 3 native runtime tests with `graph_timeout` (Findings 7). Two reruns without the stress: exit 0 both times, 1538 passed over 25 binaries. - Root `bun run check` (Bun 1.4.2; includes the Bun pin check and `privacy:check`: ok) and `bun run --cwd packages/cli typecheck`: exit 0. - gitleaks 8.30.1 (`gitleaks dir` on the `git log -p origin/next..HEAD` export): no leaks. - Full suite on the M3: queued behind the shared M3 CI lock (3 jobs ahead at push time; M3 free disk was 57 GiB, under the 58 GiB floor, so the job refuses to start below 59 GiB). This section will be updated with the result. No TypeScript, CLI surface, agent guidance or task pins changed. ## Gaps - Isolation removes in-process fork exposure only. A pre-exec stall longer than a test's own timeout can still fail it (two `host_endpoint` drop tests at 1 s; `https_recovery::rejects_a_live_inherited_unix_listener...` at 1 s). - The audit is from code reading plus the pressure runs (20 ms hold). Other fixtures drop an `flock` and later require it free (listed above) and did not fail in any of the 22 full-suite pressure runs on this Mac. A longer hold could still expose them. - Hosted CI does not run with fork pressure, so one green CI run does not prove these flakes are gone; the pressure runs above are the main evidence. The rework's pressure runs are on this Mac, not the M3. - The witnessed (HACK-1348) collision was never reproduced at module scale; the mechanism is proven by the `test_scratch` mutation. ## Findings (not fixed here) 1. ~~More fork-exposed release-check tests outside this unit~~: fixed in this rework (see the audit). 2. **Sockets can leak into concurrently spawned children on macOS (product, low- medium).** macOS has no `SOCK_CLOEXEC`, so std (`socket`, `accept`, `socketpair`) and `host_endpoint::pending::connect` (`pending.rs:16-25`) set close-on-exec in a second call. A child spawned by another thread in between (fork+exec or `posix_spawn`, which std uses without `POSIX_SPAWN_CLOEXEC_DEFAULT`) keeps the socket for its whole life. Repro (scratch probe, never committed): one thread creates sockets with the two-step pattern (gap widened to 500 us), another spawns the test binary to count its socket descriptors: 199/200 children inherited a socket with either spawn path; control without the creator: 0. Expected: a dropped `PendingConnection` or listener closes. Actual: it stays open until an unrelated child exits. Proposed fix: spawn product helpers with `POSIX_SPAWN_CLOEXEC_DEFAULT` (a small `posix_spawn` wrapper), or serialize socket creation against spawns with a process-wide lock. Needs a product- flow repro before changing behavior. 3. **The 5 s stop-path `lsof` bound still times out under load (HACK-1328 product part, known).** `short_home::vm_lock_releases...` failed once in 20 runs at `lock_absent_disks` with `provider_timeout_uncertain` under CPU load and concurrent scans. Unchanged here. 4. **`network_update::stopped` returns a plain `File` lock (minor).** Unlike `VmLock`, it does not `LOCK_UN` on drop, so a concurrent fork's copy keeps the VM lock held briefly after the network update ends. Not reproduced. 5. **One unexplained `EBADF` (low).** `host_endpoint::tests::accepted_stream_preserves_payload_and_half_close` failed once at `read_to_string` with `Bad file descriptor` on the unmodified base under fork pressure (1 of 10 runs); not seen in 60+ later runs. 6. **The task-catalog pin test depends on an attached checkout (test-only, low).** Repro: `git worktree add --detach <dir> <commit>`, then `bun test tests/project-task-catalog.test.ts -t "recipes are all reviewed"` in it. Expected: the pin review runs (it passes on an attached branch). Actual: it fails at `tests/project-task- catalog.test.ts:1415` with `E_USAGE` `detached_checkout`, because the test selects an instance implicitly from `REPO_ROOT`. Reproduced at both 331ec18 (base) and 1c14276. Any detached CI or verification worktree reports a false failure. Proposed fix: pass an explicit branch selector, or use `worktree.auto_branch=false`, in that test. 7. **Native runtime fake-backend tests time out under heavy CPU load (test-only, low-medium).** `graph::native::runtime::tests::{two_owned_b ridges_keep_per_network_members_and_aliases_through_cleanup, two_bridge_cleanup_retries_only_the_retained_second_bridge_after_first_delete}` (`tests.rs:143`, `:243`) and `authored_run::{cleanup_reaps_a_recorded_one_off..., finite_one_off_refuses_changed_member_mounts...}` (`run_tests.rs:155`) call `execution::run(..., Duration::from_secs(2))` against the in-memory `Fake` backend and fail with `graph_timeout` when the host is saturated. Repro: all-features lib binary, `--test-threads=16`, on a host at load average 17-24 plus 4 `nice` busy loops: 1 of 5 runs (and 3 tests in one CI-equivalent `cargo test --all-features` run during the stress). Expected: a fake backend finishes well within its deadline. Actual: the 2 s wall-clock deadline expires on an unoptimized, CPU-starved run. Not a descriptor refusal; reruns without the extra load passed (2/2). The isolated tests add ~20 short child test processes per suite run, which slightly raises load. Proposed fix: give these fake- backend runs a hang-guard deadline (for example 30 s), as #383 did for v14 routing. 8. **The hosted runner never exercises fork pressure (process, low).** Every flake in this unit needs an unrelated test's pre-exec fork to overlap a fixture's drop, which hosted CI hits rarely. Proposed: a scheduled job that runs the runtime-core lib binary with a fork-pressure co-tenant (as in Evidence) would catch new exposures before they flake on `next`. Refs HACK-1347 Refs HACK-1348 🤖 Generated with [Claude Code](https://claude.com/claude-code)
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.
![Fix with [code]smith](https://pr-comments-assets.blacksmith.sh/codesmith/fix-with-codesmith-light-3x.png)
Root cause
private_control_stops_only_matching_publisher_and_preserves_replaced_path(and three other tests inpackages/runtime-core/tests/stream_relay_contract.rs) picked a port withfree_loopback_port(). That helper bound127.0.0.1:0, read the port, and dropped the listener before the publisher bound it. Another test running in parallel could take the port in that window. The publisher'sbind()then failed, it exited 73 (EX_CANTCREAT, the stream relay's "frontend could not be bound" code inguest/stream-relay.c) without a readiness line, and the helper reportedstage: "readiness EOF". That matches CI run 39284084093, job 118164984179.I reproduced it locally on unmodified
next(ec223cd): 3 of 300 parallel runs of the contract binary failed in this test withPublisherStartupFailure { stage: "readiness EOF", status: unix_wait_status(18688) }, which is exit 73.Fix
The publisher now binds the port itself, so no released port is ever handed to it.
stream-relay --publish 0(exactly"0") binds a loopback port that the kernel chooses. It reports that port asready <port>\n, read back withgetsocknameafter the listener and the optional control socket are bound.--publish-unixstill write the bareready\nline.00,--publish-unix 0and relay-mode port0are still refused with 64.provider/publisher.rs) passes an explicit port and is unchanged.maininguest/stream-relay.c.ephemeral_publisher) or holds the port it needs for the whole test.free_loopback_port()is removed.control_startup_refuses_occupied_and_nonprivate_paths_without_retaining_publisher_resourcesis removed: its TCP leg now always reaches the control path.try_frontend_publisherreads one bounded readiness line of at most 12 bytes. It acceptsready\n, orready <canonical nonzero port>\nfor port 0.Tests
The new test is
ephemeral_publication_reports_its_bound_port_and_explicit_ports_keep_bare_ready. It checks that:0form is the only one admitted. Refusals are bounded by a 3 s deadline, so a wrongly admitted publisher fails the test instead of hanging it.AddrInUseon bind, connect succeeds).ready\nline. This leg still needs a released probe to choose a concrete port, so it retries at most 3 times, and only on the publisher's confirmed bind refusal (readiness EOFwith exit 73).Mutations, each run against the contract suite:
stream-relay.creadylinereadiness marker00) selects an ephemeral portcommand was admitted instead of refusedready 1readiness markerEvidence
All on an M-series macOS host under heavy shared load (load average 26-90).
Repeated parallel runs of the
stream_relay_contracttest binary (--test-threads 16, several processes at once):nextEvery other failure on both builds is the unrelated
EINVALinRelay::connectdescribed under remaining gaps.cargo +1.97.1 fmt --check: clean.clippy --all-targets --all-features -D warnings(Zig 0.15.2): clean.bun run check:local: clean.bun run test:local: pass.All-features
cargo test(CI's command): 1270 passed, 2 failed. The 2 failures weregraph_timeoutinprovider::graph::native::runtime::tests::two_*, which do not use the relay. All 3two_*tests pass when rerun serially.bun run --cwd packages/cli typecheck: 0 errors.check: 0 errors (existing warnings only).Full
bun run testwith Bun 1.4.2 (mise x bun@1.4.2): did not pass on this host. The parallel run hit load average 30-90 and had 1270 timeout and fixture-cascade failures across 118 files. Rerunning each of those files serially with--timeout 60000passed 98 of them. 20 files still fail: thenative-compose-*family plusnative-authored-project-start,-recoveryand-process-incarnation. Their errors areCompleted-job worktree acceptance refused,storage.enrollstalls of 31 s or more, and fixture-settlement cascades. The TypeScript suite does not read or run either changed file; the only TS mention is thenative-stream-relayfeature-name string innative-candidate-authored-artifact.test.ts. Two of these files (native-compose-runtime-metadata-command,native-compose-storage-fence-cost) fail the same way with the basenextrelay sources restored (1 pass, 6 fail). I am treating these as host and worktree failures, not regressions. Hosted CI is the authority here, andnexthas since landed fix(native): keep storage carrier commands alive through host stalls #356 ("keep storage carrier commands alive through host stalls").gitleaks 8.30.1 over
origin/next..HEAD: no leaks.Remaining gaps
Relay::connectcallsset_read_timeout(...).unwrap()after connecting. On macOS,setsockoptreturnsEINVALonce the relay has already closed the accepted socket, as it does for the excess connection or an unavailable target. This failedconnection_limit_refuses_excess_without_disrupting_existing_streamsandoccupied_socket_is_not_unlinked_and_unavailable_target_fails_connectionin about 1% of parallel runs, on both base and this branch.provider/publisher.rs's macOS test fixture uses the same released-probe pattern. That file is outside this unit and is reported separately.releases_portcheck (connectfails after stop) could still in theory see another process's listener on a reused ephemeral port. This was not observed.Refs HACK-1284
🤖 Generated with Claude Code
Need help on this PR? Tag
@codesmith-botwith what you need.