Widen the callback-firing margin DistantMapClicksBothSurviveJourneyTests races on #195
Loading…
Reference in a new issue
No description provided.
Delete branch "fix/red-main-e2e"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
What broke
mainRun 791 (merge of #189,
1124f83c) failed thee2ejob with three failures. Two (CreateGroupJourneyTests,AddPlaceJourneyTests) timed out inRegisterAndSignInAsyncwaiting for#register-email/#login-email— a shared helper nearly every journey uses. Only 2 of 42 tests failed, with different failure sets on two other PRs'e2eruns queued at the same time on the same single-threaded runner, matching the CI-load signature task 233's own record already documents. No code change here for these; they read as load, not regression.The real finding
DistantMapClicksBothSurviveJourneyTestsfailed on its second assertion with both asserted latitudes identical (-11.178401873711772) — the shape you get when the read happens after both deferred callbacks have already fired, not a read that lands correctly and then never observes a later change.ADR-0160 (#173, merged hours before this run) widened
DOUBLE_TAP_WINDOW_MSto fix a delivery race — but the gap between the two callbacks firing isclick1_time + windowvsclick2_time + window; the window cancels out of that difference, leaving onlyStaggerMs. At the original 250ms stagger that left ~130ms between reading the first callback's value and the second overwriting it, and that read isn't at a fixed instant —ToBeVisibleAsyncauto-retries until the panel renders, so CI-load contention that delays asetTimeoutcan just as easily delay the Blazor render the visibility check is waiting on, eating into the same 130ms from the other side. ADR-0160's own fix never touched this margin.Raised
StaggerMsfrom 250ms to 1000ms (still a plausible gap between two deliberate clicks), and replaced the second wait's literal480with a derivation from named constants, so a future change toStaggerMscan't silently narrow this margin again the way this one did.What I couldn't do
Both the fixed ports (
5017/5169) this fixture needs are held by Rob's own dev session on the primary checkout, so I could not run this suite locally at all — not once, let alone repeatedly. The evidence here is the arithmetic above plus the exact shape of run 791's own failure; ADR-0160's own Consequences already treat that as the right evidentiary weight for this specific test, over a local run count under conditions that don't reproduce CI load anyway.Build of the modified project is clean under the pinned SDK (10.0.100).
Verdict: mergeable
CI green at
4cc78a02(build, container-images, e2e allsuccess).Checked the load-bearing claim against
map.jsitself, not just the prose:deferSingleClick's cancellation (matchIndex = pending.findIndex(entry => point.distanceTo(entry.point) < CLICK_CANCEL_SLOP_PIXELS)) is purely positional and never consults elapsed time. WithStaggerMs(1000ms) still well insideTestDoubleTapWindowMs(3000ms) and the two click coordinates hundreds of pixels apart, both clicks are still concurrently pending when the second lands, so the positional guard is genuinely exercised — a 1000ms stagger doesn't push this into being discriminated by time. RevertingdeferSingleClickto its pre-ADR-0145 single-slot shape would cancel the pending entry unconditionally, so neither callback fires and the firstExpect(latitudeField).ToBeVisibleAsync()times out red — consistent with ADR-0160's own Consequences, which record exactly that mutation being tried and watched fail.Cancellation arithmetic checks out: each click schedules its own callback at
click_time + DOUBLE_TAP_WINDOW_MS, so the gap between the two firings really isclick2_time − click1_time = StaggerMs, independent of the window. Re-derived bothTask.Delayexpressions by hand from the four named constants and they land at the intended absolute times (3120ms / 4500ms from click 1). PR #173's fix was correct but incomplete; this is the missing half.PR is candid about zero local runs (ports held by another session) and reasons instead from the arithmetic plus run 791's exact failure shape (both latitudes identical) — adequate given the constraint, and honestly stated rather than implied as verified.
No new ADR is the right call — ADR-0160's Consequences already scope locators/coordinates/assertions as this test's ordinary detail, not the override mechanism itself, and
StaggerMsis the same class of thing (an original task-229 parameter, only retuned here). Only the test file changed; no ADR body touched. British spelling and no AI attribution both fine.One non-blocking edge worth recording for a later reader: if
StaggerMsis ever raised pastTestDoubleTapWindowMs + FirstCallbackBufferMs, the firstTask.Delayargument goes negative and throws. Not a live risk at today's values (1000 vs 3120), and not worth a guard on its own, but worth knowing if either constant moves again.