fix: stop the e2e suite racing itself, and resolve diver overlap up front - #132
Conversation
…ront Closes #130, #126, #131. [#130 / #126] Three specs failed intermittently under full-suite load and never in isolation: reload-resume (30 s timeout), wreck-slice (depth off by ~1 m) and game.spec. The cause was not any of them. Playwright defaults to one worker per two cores, so on a 14-core machine seven headless Chromium instances ran WebGL and canvas work concurrently against a single-threaded static server. Measured, full suite: 7 workers ~40% of runs failed ~31 s 4 workers 6 of 6 clean ~21 s <- also fastest 2 workers 3 of 3 clean ~35 s 1 worker 2 of 2 clean ~36 s Parallelism past four bought nothing and cost reliability, so workers is now one per four cores capped at four. My earlier claim on #130 that the suite ran `workers: 1` was simply wrong; it was running seven. The static server also did four synchronous filesystem round-trips per request (existsSync + statSync, twice) on the event loop that is the whole server. Now one. Measured separately this is worth about 9% under concurrency — real, but not the fix; the queueing is dominated by streaming the 648 KB harness. [#126] The depth assertion compared two different moments: it read the HUD, a rendered snapshot, then reloaded — and `pagehide` saves controller.authoritativeState, which has kept moving. Buoyancy momentum explains 27 m read against 28.1 m restored. It now reads back what was actually persisted and asserts restoration reproduces that, which is the property worth testing rather than "the HUD did not change". [#131] A fully engulfed diver could slide along inside a slab. Escaping during movement requires equal-area steps to be legal, because a fully engulfed diver has no strictly-reducing step available, and that allowance is what permitted sliding. resolveDiverOverlap() now pushes the diver out along whichever axis needs least movement before physics runs, making the engulfed state transient instead of somewhere it can travel. It overshoots the face by 1e-6 m because solidAt is inclusive on its bounds, and is a no-op on every normal tick. Each fix fails its own test: removing the resolve call from the tick, landing the push flush instead of clearing, and reverting the HUD-snapshot comparison all break exactly one thing. Verified: 3 of 3 clean full runs at 4 workers, 35 tests, ~26 s. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
| var outDown = (worst.dBottom + DIVER_HALF_HEIGHT_M + CLEAR) - depth; // positive | ||
| var best = outLeft; | ||
| if (Math.abs(outRight) < Math.abs(best)) best = outRight; | ||
| var bestVertical = Math.abs(outUp) < Math.abs(outDown) ? outUp : outDown; |
There was a problem hiding this comment.
[P2] Reject exits that the site clamp immediately undoes
The minimum-translation choice considers only the AABB, so it can pick an exit outside the legal floor/ceiling and remain overlapped forever after physics clamps it back. Repro on the wreck keel (x=14..170, d=65..66, site floor=66): start the real dive loop at x=100, d=65.5. Up and down are tied, this ternary chooses down, resolveDiverOverlap moves to d=66.300001, and updateBuoyancyPhysics immediately clamps back to d=66 inside the keel. After 120 real updateDiving(1/60) ticks the diver is still solid with 0.27 m^2 overlap and zero vertical velocity. Buried shore AABBs expose the same boundary interaction, although many escape on a later tick. Please choose among candidates that are legal against floorAt/ceilingAt (and ideally the other structures), or re-resolve after clamping, and cover the keel through updateDiving; the current single slab at d=39 has open water on both sides and cannot catch this.
…geometry Overlap resolution chose the smallest translation out of the single deepest structure, judged on that box alone. Two ways that fails, both leaving the diver permanently stuck: The site clamp undoes the exit. On the wreck keel (x=14..170, d=65..66, site floor 66) at (100, 65.5), up and down tie on distance, "down" won, resolution moved to d=66.300001 and updateBuoyancyPhysics clamped straight back to d=66, still inside. After 120 real updateDiving ticks the diver was still there with 0.27 m^2 of overlap and zero vertical velocity. Stacked geometry ping-pongs. The mast (x=75..76, d=10..18) sits on the bridge deck (x=72..108, d=18..19) forming one continuous column; leaving the mast downward lands in the deck, whose own cheapest exit is back up into the mast. The diver oscillated 17.7 <-> 18.3 until the pass limit gave up. Exits are now scored by the buried area they would actually leave the diver in, then by distance, and rejected if the site clamp would undo them. Sweeping every authored structure in every site: 30 of 513 buried start positions stayed stuck before, 0 after. The single-probe test is replaced by that sweep. The slab it used (x=14..78, d=39..40) has open water above and below, so both exits are legal and it could not have caught either failure — the same shape of gap as testing one coordinate against an exact float comparison. The sweep also asserts it found buried positions to test, so it cannot pass vacuously, and drives the real updateDiving loop rather than resolveDiverOverlap() directly. Scoring by resulting overlap and checking clamp legality are both load-bearing: reverting either leaves 14 and 12 positions stuck respectively. Generating candidates from every overlapping structure rather than one is belt-and-braces — with overlap scoring in place I could not construct a case that needs it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Confirmed and fixed —
|
| Reverted | Still stuck |
|---|---|
| scoring by resulting overlap | 14 of 513 |
| the site-clamp legality check | 12 of 513 |
| candidates from every structure vs one | 0 — passes |
Being straight about the third: with overlap scoring in place I could not construct a case that needs multi-structure candidates. It is belt-and-braces, not demonstrated necessity. I left it in because the cost is a few lines and the failure it guards against is a permanent stuck diver, but it is not carrying its weight the way the other two are.
Verification
typecheck, lint, sites:check, build, 133 unit, 28 parity, 8 collision. Full e2e 2 of 2 clean, 35 tests at 4 workers.
| if (!legal) continue; | ||
| var after = diverOverlapArea(cand.x, cand.d); | ||
| var move = Math.abs(cand.vertical ? cand.d - depth : cand.x - diverX); | ||
| if (best === null || after < best.after - 1e-12 || |
There was a problem hiding this comment.
[P2] Do not prefer a zero-overlap teleport over a nearby reducing path
Ranking residual area before distance makes the resolver clear the current state in one jump even when a short multi-pass escape exists. On the wreck at x=56.1, d=51.9 (the bulkhead/deck corner), a real updateDiving(1/60) tick moves the diver to x=78.450001: a 22.35 m horizontal teleport to the main hatch. Moving left to x=55.549999 first reduces total overlap from 0.40 to 0.18 m^2, then moving up to d=51.699999 clears it, for about 0.75 m total. The new sweep only asserts that the endpoint is clear, so it reports the 22 m teleport as success. Please preserve strict overlap reduction for termination while selecting the nearest reducing candidate (or otherwise minimize the complete escape path), and add a displacement/nearest-exit assertion for this stacked corner.
Ranking residual overlap ahead of distance made the resolver clear itself in one jump however far that jump was. At the wreck bulkhead/deck corner (56.1, 51.9) a single real updateDiving tick moved the diver 22.35 m sideways to the main hatch at x=78.450001, because the hatch was the nearest place that left it completely free. Stepping 0.55 m left and then 0.2 m up clears it in two passes for about 0.75 m. Selection now takes the nearest candidate that strictly reduces buried area and lets the loop iterate. Strict reduction is what guarantees termination — each pass leaves the diver less buried than it found it, and zero is the floor — so the property that made the previous version terminate is preserved rather than traded away. The corner now resolves 0.585 m, to (55.549999, 51.699999). The sweep could not have caught this: it only asserts the diver ends up unstuck, and a 22 m teleport satisfies that. Distance needed asserting separately, so the corner has its own regression bounded at 2 m — the two-step escape is 0.75 m and the teleport was 22.35 m, and anything between them is still the wrong shape of answer. Restoring residual-first ranking fails it with "moved 22.35 m to (78.45, 51.90)". Checked the rest of the sweep for the same pattern rather than assuming the one case was alone. Six shore positions move about 12 m against a nearest face of 0.4 m, which is correct: those boulders are buried in the seabed, so at (109, 25.9) with floorAt 19.6 the down, left and right exits are all below the floor and rejected, leaving up as the only legal way out. High ratio, no alternative. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Confirmed and fixed —
|
|
Rereviewed f6be04b: no remaining findings. The 22.35 m corner teleport now resolves locally to (55.549999, 51.699999), the keel and mast/deck regressions also clear in one real tick, and an independent 6,897-position authored-geometry sweep found no stuck or site-illegal endpoints. Focused collision + wreck-slice suites pass 13/13; current CI is green. |
Closes #130, #126, #131 — batched because the first two share a root cause and the third is small and independent.
The suite was racing itself (#130, #126)
Three specs failed intermittently under full-suite load and never in isolation. The cause was none of them.
Playwright defaults to one worker per two cores. On a 14-core machine that is seven headless Chromium instances doing WebGL and canvas work against a single-threaded static server. Measured across full runs:
Parallelism past four was buying nothing and costing reliability — four is both stable and the fastest.
workersis now one per four cores capped at four, which stays conservative on a small CI runner.Correcting myself: I claimed on #130 that the suite ran
workers: 1. It was running seven. That wrong assumption is part of why the diagnosis took a second pass.The static server also did four synchronous filesystem round-trips per request —
existsSync+statSync, twice over — on the event loop that is the server. Now one. Measured separately that is worth about 9% under concurrency: real, but not the fix, since the queueing is dominated by streaming the 648 KB harness. Included as hygiene, not as the remedy.The depth assertion compared two different moments (#126)
Exactly your diagnosis. It read the HUD — a rendered snapshot — then reloaded, and
pagehidesavescontroller.authoritativeState, which has kept moving. Buoyancy momentum explains 27 m read against 28.1 m restored.It now reads back what was actually persisted and asserts restoration reproduces that. That is the property worth testing; "the HUD did not change across a reload" never was.
Engulfed divers are pushed out, not left to slide (#131)
The residual from #129. Escaping during movement requires equal-area steps to be legal — a fully engulfed diver has no strictly-reducing step — and that allowance also let it slide the length of a slab.
resolveDiverOverlap()now pushes the diver out along whichever axis needs least movement, before physics runs, making the engulfed state transient instead of somewhere it can travel. It overshoots the face by 1e-6 m becausesolidAtis inclusive on its bounds, and is a no-op on every normal tick.Verification
Each fix fails its own test and no other:
still inside at {"x":46,"d":39.501}One of those caught a hole in my own test. The first version of the #131 test called
resolveDiverOverlap()directly, so it passed just as happily with the call removed from the dive loop — testing the function while the wiring went unguarded. It now drives the realupdateDiving()tick.Full suite: 3 of 3 clean runs, 35 tests, ~26 s. Plus
typecheck,lint,sites:check,build, 133 unit, 28 parity.Not included