spawn_process never throws for a binary it cannot start, benchctl's tests go green on Windows, and CHANGELIST.md covers the 298 merged PRs it had lost since the RC2 gate - #4050
Merged
Conversation
…ests go green on Windows, and CHANGELIST.md covers the 298 merged PRs it had lost since the RC2 gate A binary that cannot be started is an exited child, on every platform. POSIX always reported it that way - fork succeeds, the exec fails in the child, the child _exit(127)s, the parent holds a live handle to a process that has already ended - while Windows learned the failure at CreateProcess and threw, so a supervisor could not survive an unrunnable binary on one platform without try / recover, which the runtime does not offer for soft failures. spawn_process now builds, at CreateProcess, the handle the POSIX path ends up with: reaped, exit 127, stdout closed, pid 0, no process or job handle, and every lifecycle call reads it the way it reads any reaped child. process_pid answers 0 on Windows for such a child and the real pid on POSIX, the one visible difference, and the reason a supervisor that needs "did it start at all" reads the exit code. src/builtin/ARCHITECTURE.md sec.8 carries the mechanism, cited from the site; the with_process docstring and the builtin's handmade page state the contract; tests/fio/test_process pins it (poll 127, wait 127, drain nothing, the pid per platform) - on the old runtime its first assertion is a panic, not a failure. utils/benchctl's tests were red on Windows since they landed: test_bench_runner's unrunnable-binary cell died at that throw, and test_bench_cli spawned "bin/daslang" where the same file passes get_host_binary() three lines below. Both green now, 3/3 and 13/13. CHANGELIST.md: an audit of every PR merged into master since v0.6.3 against the 0.6.4 block found 899 in scope, 427 cited, 472 not - 174 of them credited under the 0.6.3 heading (post-RC work through 2026-07-02, left there pending a release-boundary ruling) and 298 simply absent, almost all of them the 236 PRs merged after the RC2 gate (#3786, 2026-08-19), of which 224 were uncited. Every substantive one is placed now, grouped by arc in the block's own style; data-only re-records, deploy one-liners and process housekeeping stay out, as the RC1 changelist's own omission policy set. Seven arcs had no heading at all and get one: libDaScriptNano, dasLLAMA text-to-speech, the DAP debugger with daslang -lib, and the document system - [arch] citations and REVIEW.das gates - plus the iquant tier, the idot lattice and the nightly benchmark stand under their existing sections. Four phantom citations were checked: three are issues cited as "fixes #N", correctly, and MoltenVK#2793 is an upstream issue whose number collides with a daScript PR. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
borisbat
force-pushed
the
bbatkin/benchctl-windows
branch
from
September 16, 2026 20:20
0538dde to
60d6979
Compare
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Newly added architecture documentation contains an objectively incorrect claim about try/recover availability and should be corrected before approval.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This PR aligns spawn_process failure semantics across platforms (Windows now reports an unstartable binary as an already-exited child with exit code 127, matching POSIX), fixes utils/benchctl tests to run correctly on Windows by invoking the current host binary, and updates CHANGELIST.md to restore missing merged PR coverage in the 0.6.4 section.
Changes:
- Windows
spawn_processno longer throws onCreateProcessfailure; it returns a “dead-at-birth” process handle (reaped, exit 127, no output, pid 0). - Adds a regression test for “cannot start” behavior and updates benchctl CLI tests to use
get_host_binary(). - Expands
CHANGELIST.md0.6.4 entries to include previously missing merged PRs and new subsections.
File summaries
| File | Description |
|---|---|
| utils/benchctl/tests/test_bench_cli.das | Uses get_host_binary() instead of bin/daslang to make tests work on Windows layouts. |
| tests/fio/test_process.das | Adds test_process_cannot_start to assert “no throw, exit 127, no output” contract and platform-specific pid expectations. |
| src/builtin/module_builtin_fio.cpp | Implements Windows “dead handle” return path on failed CreateProcess to match POSIX exec-failure semantics. |
| src/builtin/ARCHITECTURE.md | Documents the unified cross-platform contract and Windows mechanism for spawn_process cannot-start cases. |
| doc/source/stdlib/handmade/function-fio-spawn_process-0x5eca268e4ba0b640.rst | Updates stdlib docs to state the new non-throwing cannot-start behavior and pid differences. |
| daslib/fio.das | Updates with_process docstring to reflect the new “cannot start => exit 127, no throw” contract. |
| CHANGELIST.md | Restores/extends 0.6.4 changelist coverage with missing merged PRs and new topical subsections. |
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…e anchor, and the architecture note stops claiming try / recover is absent Copilot round on #4050. get_running_platform_name is the non-folded runtime query, the right one for a platform branch a test takes at run time; the test's comment cites the section by its {#child-cannot-start} anchor the way the C++ site does, so section renumbering cannot orphan it; and the mechanism sentence now says what it meant - an expected failure is never routed through the panic path - rather than implying try / recover does not exist. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The new POSIX test assumes process_poll returns 127 immediately, but it can legally return process_running briefly after fork(), making the test potentially flaky under load.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 1
- Review effort level: Lite
…acing its own exec failure cannot fail the assertion Copilot round two on #4050. process_poll is waitpid(WNOHANG): right after fork the child may not have reached _exit(127) yet, and the poll answers process_running for that window. process_wait under the platform guard goes first and reads 127; poll then re-reads the code from the reaped child, which is the contract the test pins. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The behavioral change is narrowly implemented, consistent with existing POSIX behavior, and is covered by a targeted new test plus corresponding documentation updates.
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 0 new
- Review effort level: Lite
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.
Behavior changes:
spawn_processnever throws for a binary it cannot start - on Windows the handle now reads as an exited child (exit 127, no output) the way POSIX always reported it;utils/benchctl's test suite is green on Windows;CHANGELIST.md's 0.6.4 block gains the 298 merged PRs it had lost since the RC2 gate.Why. Windows learned an unstartable binary at
CreateProcessand threw; POSIX reports it through the child's own_exit(127). One contract split at the one place a supervisor cannot catch it withouttry/recover, and benchctl's tests found it - red on Windows since they landed. And the changelist's coverage stopped at #3786 (Aug 19): 224 of the 236 PRs merged since were uncited, seven arcs had no heading.What changes.
spawn_process(Windows) builds a dead-at-birth handle on a failedCreateProcess- reaped, exit 127, stdout closed, pid 0 - soprocess_poll/process_wait/process_drain/close_processread it like any reaped child.src/builtin/ARCHITECTURE.mdsec.8 carries the mechanism (cited from the site with the C++ pointer form);with_process's docstring and the handmade page state the contract.tests/fio/test_process.dasgainstest_process_cannot_start: poll 127, wait 127, drain nothing, pid 0 on Windows / real on POSIX.utils/benchctl/tests/test_bench_cli.dasspawnsget_host_binary()instead ofbin/daslang.CHANGELIST.md: every substantive merged PR since v0.6.3 placed by arc; new subsections for libDaScriptNano, dasLLAMA text-to-speech, the DAP debugger +daslang -lib, and the document system ([arch]citations,REVIEW.dasgates); the iquant tier, the idot lattice and the benchmark stand under their sections.Observable behavior.
with_process(["/nonexistent"]) $(p) { process_poll(p) }answers 127 on Windows where it panicked before.utils/benchctl/tests:test_bench_cli3/3,test_bench_runner13/13 on Windows (were 1 failed + 2 errors).CHANGELIST.md's 0.6.4 block cites 725 PRs where it cited 427.Where to look.
src/builtin/module_builtin_fio.cpp(the!okbranch of the Windows spawn);src/builtin/ARCHITECTURE.mdsec.8;tests/fio/test_process.das;CHANGELIST.md.Validation, claims, ledger
Validation
tests/fio/test_process.das --test-names test_process_cannot_start: 2 tests, 2 passed on the rebuilt runtime. Negative control: on the previous runtime the cell's first statement is a thrown error (spawn_process: CreateProcess failed), i.e. an error, not a failed assertion - the test cannot pass without the change.utils/benchctl/tests/test_bench_cli.das: 3/3;test_bench_runner.das: 13/13 - both on Windows, both red before (the CIutils-testsgate runs them on Linux, where they were always green, which is how the Windows red went unseen).--only format,lint,review-md,docsover the changed set: 10 passed, 0 failed - format, lint (6 files clean on host and linux-mirror rails), review-md, das2rst, imgui2rst, vulkan2rst, stubs, uncategorized, untracked, sphinx-html.masterthrough the GitHub API (gh pr list --limit 1000caps at exactly 1000 and silently dropped one), giving 899 merged in scope (2026-06-10 .. 2026-09-16); 427 cited in the 0.6.4 block; 472 uncited, of which 174 are cited under the 0.6.3 heading; 4 phantom#Ntokens, all legitimate (threefixes #issue, oneMoltenVK#2793). The 298 placed here were grouped by the block's own section headings; the agent's per-PR one-liners were checked against titles and bodies for the entries that name behavior.Claims - stated, not tested
(noise)-class PRs - data-only record re-mints, deploy path one-liners, changelist and version housekeeping, dependabot - are deliberately not listed, following the omission policy CHANGELIST: 0.6.4 coverage extended to master (the RC1 gate) #3768 stated at RC1. They are enumerated in the audit report if that policy should change.Not done
## 0.6.3(its RC2/RC3 entries) although thev0.6.3-RC1-take2tag predates them. They stay where they are; moving them is a call, not a fix.bench_runner.daskeeps itsprocess_pid(p) <= 0guard: on Windows it now fires and names the binary; on POSIX the 127 path reports throughclassify_run_output. Two honest paths, one observable - the test is indifferent.