[fixtures] De-flake the dev-registry tests on Windows by making tail teardown safe - #15017
[fixtures] De-flake the dev-registry tests on Windows by making tail teardown safe#15017petebacondarwin wants to merge 2 commits into
Conversation
|
44e454c to
33ebaee
Compare
|
Those are all service-binding tests unrelated to tail_consumers — the removal of Let me verify the removed The old tests asserted tail via I've done a comprehensive review:
No logic bugs, security issues, backward-compat violations, or incorrect API behavior. The one minor race in LGTM |
@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: |
…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.
…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.
33ebaee to
4616d98
Compare
…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.
…teardown safe The `Tests (Windows, fixtures)` job failed roughly half the time on `fixtures/dev-registry`, almost always as a 50s timeout in one of the `vite dev <-> vite dev` tests. The cause is a workerd abort during test teardown: when a dev session is killed while another running session is forwarding tail events to it, the surviving session's workerd calls `std::terminate`. Miniflare then starts a replacement runtime, the Vite plugin restarts the dev server to rebuild its module-runner sockets, and the ~30s of churn lands on whichever test is still running. The fixture made that shape easy to hit. `tail_consumers` formed two cycles (`exported-handler` <-> `worker-entrypoint-with-assets` and `worker-entrypoint` <-> `exported-handler-with-assets`), so most tests carried a live tail edge whether or not they tested tail handlers, and no shutdown order could keep every producer shorter-lived than its consumer. Make the tail relationships a one-directional chain (`worker-entrypoint` -> `exported-handler-with-assets` -> `exported-handler`) and drop the incidental edges, which leaves tail edges only in the tests that assert on them. Split the three bidirectional tail tests into one test per direction so each can start its consumer first and its producer second; Vitest tears sessions down in LIFO order, so the producer is always killed while its consumer is still alive. All existing tail assertions are preserved, including the worker-name log prefixes from multi-worker Wrangler sessions. Two supporting fixes, both of which let a session observe a peer disappearing mid-teardown: - The `devRegistryPath` fixture deleted the registry directory in its teardown, which Vitest runs *before* `onTestFinished` callbacks — i.e. while every dev session was still running. Register the removal as an `onTestFinished` callback during fixture setup so LIFO ordering runs it last instead. - `runWranglerDev`'s `stop()` resolved when the kill signal had been delivered (on Windows, when `taskkill` exited), not when the process was gone. Wait for the actual exit, bounded, so sequential teardown really is sequential.
Reverts the temporary skip from #15018, which disabled the whole suite on Windows to unblock other work "until the underlying issue is fixed". The preceding commit fixes it: the timeouts were a workerd abort during teardown (cloudflare/workerd#6913), triggered by killing a dev session that a still running session was forwarding tail events to, and the fixture no longer sets that up. Windows is the only platform the dev registry flake ever appeared on, so leaving these skipped there would mean the suite never guards the thing it exists to guard.
4616d98 to
21edcb9
Compare
The
Tests (Windows, fixtures)required check has been failing roughly half the time onfixtures/dev-registry, almost always asError: Test timed out in 50000msin one of thevite dev <-> vite devtests.The failure is a workerd abort during test teardown. When a dev session is killed while another running session is forwarding tail events to it, the surviving session's workerd calls
std::terminate— filed upstream as cloudflare/workerd#6913. Miniflare then starts a replacement runtime, the Vite plugin restarts the dev server to rebuild its module-runner sockets, and the ~30s of churn lands on whichever test happens to still be running. That's why the timeout kept moving between tests and never pointed at the code it was blaming.This PR does not fix the abort — that belongs in workerd. It removes the shape that triggers it from the fixture.
The fixture made the crash easy to hit
tail_consumersformed two cycles:exported-handler<->worker-entrypoint-with-assetsworker-entrypoint<->exported-handler-with-assetsSo most tests carried a live tail edge whether or not they were testing tail handlers, and because the edges were cyclic there was no shutdown order that could keep every producer shorter-lived than its consumer. Whichever session died first left the other one forwarding to a dead peer.
Changes
Tail relationships are now a one-directional chain —
worker-entrypoint->exported-handler-with-assets->exported-handler— and the incidental edges onexported-handler,worker-entrypoint-with-assetsandexternal-durable-objectare gone. Reversing the middle edge was enough to break both cycles, so no new config files were needed. Tail edges now exist only in the tests that assert on them.The three bidirectional tail tests are split into one test per direction, so each can start its tail consumer first and its producer second. Vitest tears sessions down in LIFO order, so the producer is always killed while its consumer is still alive. The ordering is expressed through a documented
startTailPair()helper rather than a comment, so a future test can't quietly get it backwards. All existing tail assertions are preserved, including the[worker-entrypoint]log prefixes that only appear for multi-worker Wrangler sessions.Two supporting fixes, both of which let a session watch a peer disappear mid-teardown:
devRegistryPathfixture deleted the registry directory in its teardown. Vitest runs fixture teardown beforeonTestFinishedcallbacks (verified against@vitest/[email protected]:afterEach-> fixture teardown ->onTestFinishedLIFO), so the registry was being pulled out from under every session while they were all still running. Registering the removal as anonTestFinishedcallback during fixture setup makes it the first registered and therefore the last to run.runWranglerDev'sstop()resolved once the kill signal had been delivered — on Windows, oncetaskkillexited — not once the process was gone. It now waits for the actual exit, bounded at 10s, so sequential teardown really is sequential.Re-enables the tests on Windows
#15018 landed while this was in review and skipped the entire suite on Windows as a temporary measure "until the underlying issue is fixed". This PR fixes it, so the second commit reverts that skip. Windows is the only platform the flake ever appeared on, so leaving it skipped there would mean the suite no longer guards the thing it exists to guard.
Verification
main(169 pre-existinglib.dom/@cloudflare/workers-typesconflicts). Worth noting separately: this fixture's script ischeck:typeswhile the turbo task ischeck:type, so CI never runs it.Unrelated flake found along the way, not fixed here
vite dev <-> wrangler dev > supports queues across dev sessionsfails withexpected [] to include 'hello from vite to wrangler'on 2 of 6 runs onmainand 1 of 7 on this branch — equally present before and after, so it is independent of this change. Unlike the Windows crash it reproduces on macOS, so it should be tractable to chase separately.@fixture/dev-registryand@fixture/sharedare both private).A picture of a cute animal (not mandatory, but encouraged)
Note
This is a contribution from an AI agent: OpenCode, claude-opus-5.