EXPERIMENT (do not merge): BASELINE control for #15017 (pre-fix, pre-skip) - #15001
EXPERIMENT (do not merge): BASELINE control for #15017 (pre-fix, pre-skip)#15001petebacondarwin wants to merge 2 commits into
Conversation
|
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
I'm Bonk, and I've done a quick review of your PR.
Experimental (do-not-merge) branch that bisects the Windows dev-registry workerd crash; alongside the diagnostic fixture test it also makes real, changeset-bearing changes to Miniflare so that a fatal kj crash banner and its stack trace are surfaced at error level, and warns (with a count) when workerd is restarted after a crash.
The production changes are well-tested. One real (low-severity) logic issue in the crash-report state machine, noted inline. Everything else looks good.
Since this is marked "do not merge", the note below is mainly relevant if the Miniflare structured-logs.ts change is later split out into a mergeable PR.
| } | ||
|
|
||
| // An ordinary log line means the crash report (if any) is complete. | ||
| reportingFatalCrash = false; |
There was a problem hiding this comment.
reportingFatalCrash is only cleared here, by an ordinary (non-internal) log line. The isAddressInUse (line 149) and isAccessViolation (line 168) branches early-return without clearing it, and both represent a distinct workerd event rather than a continuation of the crash report.
As a result, after a fatal banner, if the next internal line is an address-in-use / access-violation — or if the stream later emits any unrelated hex-stack internal line before a normal log line arrives — the flag stays set and those subsequent internal stacks get re-emitted as error even though they aren't part of the original crash report.
Consider clearing reportingFatalCrash = false; inside those two early-return branches (or scoping the "keep" behaviour to only the symbolizer-notice / stack: lines) so only the diagnostics that immediately follow the banner are surfaced. Your new test "address-in-use errors are still swallowed, even during a crash report" ends right after the address-in-use line, so it doesn't currently exercise the leak.
|
Review posted successfully to PR #15001. Summary of what I did:
Everything else (crash-count warning logic, moving the handler creation out of the per-chunk loop to preserve cross-line state, and the test coverage) looked correct. |
|
✅ All changesets look good |
@cloudflare/autoconfig
@cloudflare/build-output-utils
@cloudflare/config
create-cloudflare
@cloudflare/deploy-helpers
@cloudflare/kv-asset-handler
miniflare
@cloudflare/pages-functions
@cloudflare/pages-shared
@cloudflare/unenv-preset
@cloudflare/vite-plugin
@cloudflare/vitest-pool-workers
@cloudflare/workers-auth
@cloudflare/workers-editor-shared
@cloudflare/workers-utils
wrangler
commit: |
Root cause narrowed to a minimal reproducerTwelve Windows CI rounds. The trigger is now pinned down exactly:
The isolating roundVictim is
Dropping the tail consumer stops it. Pointing the tail consumer at a peer without assets stops it. Removing the service binding changes nothing. Reproduced twice more since, always 4/4 vs 0/4. Related earlier evidence: killing an assets peer the victim has no relationship with is clean (0/6, 28 cycles), and killing a bound non-assets peer is clean (0/6, 23 cycles). Ruled out
The last two are worth emphasising: neither the unhandled rejection in the dev-registry proxy's Why CI is affected
The stack is byte-identical across every crash once ASLR bases are normalised, and unsymbolized ( |
8562bbd to
350ee29
Compare
petebacondarwin
changed the title
350ee29 to
7857df6
Compare
petebacondarwin
changed the title
…per-iteration tally Not for merge. Turbo runs the fixtures suite with --concurrency=2 and no --continue, so an unrelated fixture flake aborts the job before dev-registry executes (which is exactly what happened on the first run of #15017). This harness isolates dev-registry and repeats it so a single round of CI yields a usable sample instead of one coin flip.
Measures the baseline Windows flake rate under the same 6x harness. Reverts both the teardown fix and #15018's Windows skip, so the original cyclic tail_consumers are back and the tests actually execute on Windows.
7857df6 to
5dbc2f2
Compare
petebacondarwin
changed the title
2 participants
Scratch harness. Not for merge. Control arm for #15017.
Pre-fix and pre-skip dev-registry fixture (both cyclic
tail_consumersrestored, #15018's Windows skip reverted) plus a temporary harness that runs only@fixture/dev-registry, six times, uncached, with a per-iteration tally.pass=6 fail=0, 30/30 tests each, 0std::terminate, 0 timeouts, durations 237-250sAn earlier attempt at this control was void: #15018 landed mid-experiment and the PR merge ref picked it up, so all 27 tests were skipped on Windows (
↓ 27 tests | 27 skipped, 4.7s per iteration).Search the job log for
TALLYandITERATION n RESULT.