Join effect worker threads before teardown frees the channel#124
Merged
Conversation
- Store spawn/fetch/file worker handles on their slots instead of detaching, join them in reclaim and unconditionally in deinit: the old ~5s give-up abandoned workers still holding slot/queue/io pointers into memory the owner frees right after, which is how a torn-down harness segfaulted the next test inside a stale slot's child_mutex. - Spawn each child into its own POSIX process group and kill the group on cancel/teardown, so shell-wrapped commands' grandchildren cannot hold the stdout pipe open and stall the worker (and now the join) past the cancel. - Regression tests: teardown mid-stream joins and returns promptly with no running slots, and an 8-round teardown storm (immediate and cancel-racing) over recycled harness memory.
- Scale the host battery's real-child wait budget 10x (20s -> 200s): the waits prove correctness and poll, so a healthy run still returns in milliseconds while a loaded runner gets the slack it needs to schedule and reap /bin/sh children. - A blown wait budget now tears the effects channel down (kill children, join workers) before surfacing TestTimedOut, so one slow test can never cascade a straggling child into the next test's harness. - The soundboard dispatch-latency test asserts the best of three attempts with unchanged budgets: scheduler contention only adds time, so real regressions still fail while parallel-suite noise no longer does.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
- Effects.deinit joined every worker unconditionally, but a file worker blocked in I/O that nothing converges (a write to a FIFO with no reader, a stalled network filesystem) made teardown hang forever behind it; file workers now get an injectable budget (file_join_deadline_ms, 15s default) with a best-effort cancel of the blocked task at the halfway mark, and past it teardown detaches the thread, warns once naming the stuck op and path, and deliberately leaks everything the worker can still reach (its context, its data buffer, the executor io) so the owner can free the channel safely. Spawn and fetch joins stay unconditional. - The blocking phase moved out of the channel: each file worker supervises its op as a cancelable Io task (mirroring fetchWorkerMain, which also supplies the platform interruption: SIG.IO signaling on POSIX, NtCancelSynchronousIoFile on Windows, via the threaded io) against a heap FileWorkerContext holding the path copy, buffer, and a commit/abandon handshake - the worker only touches the slot and queue after committing under the context mutex, so an abandoned worker that wakes later walks only leaked memory. - Regression tests: a write against a reader-less FIFO is abandoned within a tiny injected deadline (loud counter, healthy process after, the woken worker exercised under the leak invariant), the default interruption path joins the same posture with no leak, and the happy-path teardown pins the abandon counter at zero.
…cancelable fetches - Allocate FileWorkerContext, its private data buffer, and the shared IoThreaded executor from a process-lifetime allocator (page_allocator) so an abandoned worker never walks memory that dies with the owner's arena or GPA; the happy path frees all three through the same seam (joinWorker, deinit), and a committed read publishes its bytes into the slot's channel-owned delivery buffer. - Refuse a fetch whose exchange cannot start as a cancelable task instead of running it inline: an inline exchange observes neither cancel nor the timeout and would hang deinit's unconditional fetch join, so the honest terminal is one journaled .rejected (replay reproduces it like any transport failure), with an injectable fetch_concurrent_start seam and a warn-once counter. - Pin both with tests: an arena-backed channel abandons a FIFO-stuck worker, dies, and the woken worker walks only process-lived memory (leak-checked happy path alongside); the fetch rejection seam delivers exactly one .rejected and teardown returns promptly.
…hang - Group-kill cannot guarantee spawn convergence: a descendant that leaves the child's process group (setsid, a shell's set -m background job) keeps the inherited stdout write end open, so the worker's read never sees EOF and deinit's unconditional join hung forever; spawn workers now get the file workers' full discipline — an injectable budget (spawn_join_deadline_ms, same 15s default) with a best-effort cancel of the blocked task at the halfway mark, then a detach-warn-and-leak abandon with its own abandoned_spawn_workers counter, so spawn, fetch, and file teardown all share one terminal guarantee: bounded return, and every byte a live thread can still touch stays valid forever (Windows, where the direct-handle terminate never reached descendants, is bounded by the same net; job objects remain the future strengthening). - The blocking phase's world moved out of the channel into a process-lived SpawnWorkerContext (argv/stdin copies, the published-child kill handshake, private framing/collect buffers, the stderr tail ring, drop accounting), supervised as a cancelable Io task exactly like file ops; unlike a file op a spawn DELIVERS while it blocks, so streaming lines enqueue under the context's abandon fence (produceSpawnLine re-checks the abandon with every channel touch) and the committed epilogue publishes collect payloads into the slot's channel-owned delivery buffer, preserving cancel semantics, the stale-event window, and record/replay byte-identity. - Regression tests pin the escaped-descendant shape (/bin/bash -c 'set -m; sleep 300 & echo $!' — portable setsid): the default interruption path joins it with no leak, the disabled-interruption path abandons it within a tiny injected deadline (loud counter, healthy process after, the woken worker proven to reap its zombie child through only leaked memory), the arena-lifetime test wakes an abandoned worker after the owner's allocator died, and the existing mid-stream teardown pins abandon-count zero.
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
1 participant
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.
Fixes a use-after-free window in the effects executor that ships in 0.5.0, found via a CI segfault cascade on an unrelated PR.
The bug: spawn/fetch/file workers were spawned detached, each holding pointers into the
Effectsstruct for their whole thread lifetime — butEffects.deinitwaited at most ~5s for running slots before destroying the channel, and the owner frees that memory immediately after. A worker that outlived the wait (slow child, congested machine) later resumed against freed memory: observed in CI as a SIGSEGV in the next test'sslot.child_mutex.lock()after a timed-out cancel test tore down its harness. Theui_appstop-hook contract even documented "join effect workers" — the implementation didn't.Slot.worker_thread, comptime-void on freestanding — the wasm docs build stays clean) and joined:reclaimSlotsjoins as slots leave.running, anddeinitjoins unconditionally after kill/shutdown signaling — a bounded give-up cannot be memory-safe. No worker survivesdeinit..pgid = 0at spawn,kill(-pid)with direct fallback): previously ash -ccompound command's grandchildren inherited the stdout pipe and stalled teardown until they exited on their own. Windows keeps its direct-handle terminate.Test-battery hardening (second commit): the ts-core e2e
waitIdlebudgets get 10x headroom (they poll — healthy runs still return in ms), a blown budget fully tears down children before failing so a timeout can never cascade, and the one perf assertion in the battery takes best-of-3 with unchanged budgets (contention only adds time; real regressions fail all attempts).Verification:
test-ts-core-e2egreen 12/12 invocations + 20x sequential + 10x under 10-way CPU-hog load; full battery 281/281 steps; validate, test-tooling, and the wasm docs preview all green.