Promote develop to main for v0.4.0 - #48
Merged
Merged
Conversation
A Windows shard failed intermittently with `expected exit 0, got -1 after
30200ms`, and the harness's own dump showed an empty stdout and an empty stderr.
That reads as a child which produced nothing at all -- a hang before first output
-- which is a very different diagnosis from one that did most of its work and
then stalled.
It was neither. Both streams were empty because the failure path DISCARDED them.
`AppProcess.run`'s timeout built its error as
`new AppProcessError({ command, cause: "Timed out" })`, dropping the buffers
`collect` had been filling, and the test harness then hardcoded
`stdout: Buffer.alloc(0)` on top of that. So every subprocess timeout reported
the same thing regardless of cause, and the one piece of evidence needed to tell
these cases apart was being thrown away at two separate layers.
Fixed at both. `collectStream` now accepts the accumulator so a CALLER can own
it -- the fold mutates one object and returns it on every chunk, so a caller
holding the reference sees whatever arrived even when the fold is interrupted,
which is what `Effect.timeoutOrElse` does to it. `AppProcessError` gained a
`stdout` field to carry it, and the harness passes the child's real output
through instead of an empty buffer.
The wait bounds are now derived from one number instead of written three times.
`cliIt.concurrent` carried a comment saying Bun's test timeout must stay ABOVE
the child timeout, while `60_000` and `30_000` sat in different functions as
unrelated literals with nothing keeping them in step; raising either alone breaks
the invariant the comment promises, and the symptom -- a test expiring while
holding no spawn permit -- looks like a slow command rather than a mis-tuned
harness.
Windows gets three times the base. Every `redrob.spawn` is `bun run` over the
TypeScript entry, so each one pays a cold transpile plus that platform's
process-creation cost. This is a MITIGATION, not a diagnosis, and it is labelled
as one at the definition: the flake it responds to timed out at exactly the bound
with no evidence preserved. The next occurrence will report what the child
printed, and this number should be revisited against that rather than raised
again by feel.
Verified by reverting the preservation: the new test fails without it.
…s-timeout Make a subprocess timeout say where the child got to
…ually written
The first version of this fix scaled only the DEFAULT child timeout, and the very
test that was flaking passes its own `timeoutMs: 30_000`. So it stayed pinned to
thirty seconds and failed again on the next Windows run, at 30872ms -- which is
how I know the allowance was not reaching it.
Two gaps, both now closed by construction rather than by remembering:
- `cliIt.concurrent` treats a caller's test timeout as a FLOOR, not a ceiling.
Thirty-seven call sites pass `60_000`, comfortably above the old fixed 30s
child bound and BELOW the Windows one, so honouring them literally
reintroduced exactly the drift the derived constants exist to prevent. One
clamp fixes all of them; editing thirty-seven call sites would not have kept
the thirty-eighth honest.
- `slowPlatform(ms)` is exported for the call sites that genuinely need their
own bound, so the intent stays readable at the call site while the correction
is applied for them.
The duration assertion beside one of those bounds is scaled WITH it. Its claim is
that the CLI exits promptly rather than being killed by the timeout, and a
duration pinned to a base figure while the bound moves tests something else.
…call-sites Apply the slow-platform allowance where the bounds are actually written
A third Windows shard failed, and this one was not a spawn timeout: `stdin EOF exits cleanly` gave a subprocess five seconds to exit and failed at 6.5s with a bare `TimeoutError`. Windows process teardown is slower, and five seconds was tight enough to flake. Every wait in the ACP test client is on a subprocess round trip, so all three bounds now go through the same scaler as the harness: the request wait, the notification wait's default, and the explicit values callers pass to it. This is the third place a fixed millisecond figure met a slower platform, after the spawn timeout and the ready timeout. The pattern is the point -- a bound written as a literal beside a subprocess is a bound that will eventually be too small on somebody's machine, and `slowPlatform` is where that correction belongs rather than in each number.
…waits Scale the ACP subprocess waits for the slow platform too
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.
Promotes the tree v0.4.0 was cut from.
release.ymlpublishes fromdevelopand deliberately does not push tomain, so this step is a reviewable one rather than a silent push.Contents:
Retry-Afterpaces it); the two 402s go through a newblocking()and a newblockedsession status, because the console answers 402 rather than 429 specifically so clients stop.cliIt.concurrentpromised its test timeout stayed above the child timeout while the two numbers sat in different functions as unrelated literals.Release v0.4.0 is already published: 21 assets, Windows installer signed with the org Authenticode certificate, macOS notarized, notes verified against the run's own steps.