DRAFT (do not merge): coherence F2 — invalidate facet counts at input, before the await - #347
Draft
rdhyee wants to merge 2 commits into
Draft
DRAFT (do not merge): coherence F2 — invalidate facet counts at input, before the await#347rdhyee wants to merge 2 commits into
rdhyee wants to merge 2 commits into
Conversation
The audit's highest-value open item, and the one that turns the honesty rule
from call-site discipline into an invariant.
THE BUG. Both filter handlers had this shape:
writeQueryState(); refreshHeatmap();
await <globe reload>; // long
refreshFacetCounts(); // only now are the counts invalidated
The await is not short. Measured on production: the globe reload takes 168s on
4G and 423s on 3G (see PERF_BANDWIDTH_FINDINGS_2026-08-06.md — it is currently
downloading ~74 MB). For that entire window the facet counts kept displaying the
PREVIOUS filter's numbers, unmarked and looking settled. isamplesorg#340 was this same
shape with a different trigger; isamplesorg#341/isamplesorg#342 fixed instances, not the class.
THE FIX. invalidateFacetCountsNow() — synchronous, cheap, called at the top of
the source and facet handlers BEFORE any await:
1. ++facetCountsReqId so an in-flight recompute cannot repaint over us
2. clearTimeout on the debounce armed for the old inputs
3. markFacetCountsRecomputing() — dims now, and via its 400ms timer (isamplesorg#342)
swaps text to "(Loading…)" when the wait is long enough to matter
It deliberately does NOT schedule a recompute: the caller still owns that, after
its await, once the new inputs have settled. Scheduling here would query
half-applied state and add contention to the very load being waited on.
Under an active search it returns early — those counts are already the honest
"(—)" dash (isamplesorg#340), and re-dimming would downgrade "we cannot know this" to
"we're about to know it", a worse claim rather than a better one.
PROVEN by A/B, not asserted. Same script, same 400 kbps throttle, toggle one
source filter, sample the DOM every 0.4s:
production (no fix) invalidated within 0.4s: False
stale UNMARKED windows : 27
user sees "(4,389,231)" for 10+s after deselecting SESAR
this branch invalidated within 0.4s: True
stale UNMARKED windows : 0
user sees "(Loading…)" throughout
Adds tests/playwright/verify_preawait_invariant.py, which is that experiment.
Verified: quarto render clean; test_smoke.py passes; test_frontend_derived 40
passed; isamplesorg#341/isamplesorg#342 behaviour unaffected (search -> 60/60 dashes 0 stuck;
facet-only -> real counts); no pageerrors.
SCOPE. Facet counts only. The samples table has the same shape
(applySearchFilterChange awaits reconcileGlobeForFilters before
refreshSamplesTable) — audit F8, deliberately left for a separate change.
Refs isamplesorg#340, isamplesorg#304, isamplesorg#305
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QCCDurpcLzMe7L72y2HDAa
…unable to false-pass Codex blocked round 1 on two findings, both valid: 1. A TERMINAL EXCEPTION COULD STRAND (Loading...). invalidateFacetCountsNow() bumps facetCountsReqId and cancels the pending debounce, but refreshFacetCounts() sat only on the success path. If anything between the invalidate and the end of the globe work threw — updateSourceLegendState, writeQueryState, refreshHeatmap, reconcileGlobeForFilters — nothing would ever schedule a recompute and the counts would sit at (Loading...) until some unrelated later event. A HUNG promise staying Loading is honest (isamplesorg#342); a REJECTED one is terminal, so Loading becomes a different kind of lie. Fixed with an inner try/finally in both handlers, so refreshFacetCounts() runs on success AND on throw. No watchdog, no query issued while work is pending — it only guarantees a successor once the work settles or rejects. The outer finally still owns busyRelease(), and selection revalidation still runs after. Deliberately NOT applied to applySearchFilterChange(): Codex analysed it and it is not a stale-number hole (activation paints the (—) dash synchronously, and clearing leaves that honest dash visible during reconciliation). 2. THE VERIFIER COULD FALSE-PASS. It printed ever_loading without asserting it, checked stale-unmarked only before t=12s while sampling past 18s, would accept a permanently dimmed numeric value, and never proved a successor repaint. All four clauses are now required, the stale check covers every observation, and it un-throttles at the end and requires real numbers to come back — which is precisely the anti-stranding assertion for finding 1. Also took Codex's idempotence hardening: under an active search invalidateFacetCountsNow() now repaints the (—) dash rather than merely returning, so it does not depend on an earlier isamplesorg#340 caller having painted it. Verified on the rebuilt page: verifier passes all four clauses (invalidated within 0.4s / swapped to Loading / 0 stale-unmarked windows / successor repainted); test_smoke.py passes; test_frontend_derived 40 passed; isamplesorg#341/isamplesorg#342 behaviour unaffected. STILL OPEN from the review, deliberately not done here: converting the verifier to a discovered .spec.js that deterministically holds a globe request, and hardening facet-tree.spec.js's fixed 3s sleeps (they parse numeric text and could now read (Loading...) if a globe reload exceeds 3s). Refs isamplesorg#340, isamplesorg#304, isamplesorg#305 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QCCDurpcLzMe7L72y2HDAa
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.
What this is
The coherence audit's highest-value open item (F2), and the approach Codex argued for over a watchdog: "the invariant belongs at input invalidation, not at a timer attached to selected query paths."
The bug it targets
Both filter handlers had this shape:
That await is not short. Measured overnight on production: 168 s on 4G, 423 s on 3G — because the Explorer currently downloads ~74 MB on cold load (separate finding, #345). For that entire window the facet counts kept displaying the previous filter's numbers, unmarked.
#340 was this same shape with a different trigger. #341 and #342 fixed instances; this targets the class.
The change
invalidateFacetCountsNow()— synchronous, called before any await: bumps the request id, clears the pending debounce, and marks counts recomputing (which swaps text to(Loading…)after 400 ms via #342). It deliberately does not schedule a recompute. An innertry/finallyguarantees a successor on success and on throw.Demonstrated effect
400 kbps, toggle one source filter, sample the DOM every 0.4 s:
(4,389,231)for 10+ s after deselecting SESAR(Loading…)Why it is NOT ready
1. Overlapping handlers can still fire a premature query. Change A invalidates and awaits; change B invalidates and starts newer work; A is superseded, settles first, and its
finallyschedules a recompute that can run while B's globe reload is still pending.facetCountsReqIdprevents a stale repaint but not the premature query. Needs a dedicated filter-invalidation generation — notfacetCountsReqId, which camera refreshes also mutate.2. The verifier cannot prove what I claimed. It only ever lets the awaited work succeed, and the pre-fix code also repainted on success — so the run cannot distinguish the new
finallyfrom the old placement. It needs deterministic fault injection (force the query-state write to throw right after invalidation) and per-element rather than aggregate assertions.3. Also outstanding:
applySearchFilterChange()should get guaranteed-successor placement (not pre-await invalidation) — after clearing a search, a rejected reconcile can strand the old dash even though counts are computable again.4. Untested risk:
facet-tree.spec.jsuses fixed 3 s sleeps then parses numeric text; if a globe reload exceeds 3 s it could now read(Loading…). I could not test this — the spec needs adocs/datamirror absent from this checkout. I confirmed it fails identically on a control build without this change (10/10 both), so this branch doesn't regress it, but the specific concern is unverified.Verification that did pass
quarto renderclean ·tests/test_smoke.pyPASS ·tests/test_frontend_derived.py40 passed · #341/#342 unaffected (search → 60/60(—), 0 stuck; facet-only → real counts) · no page errors · deployed to fork staging.Refs #340, #304, #305
🤖 Generated with Claude Code
https://claude.ai/code/session_01QCCDurpcLzMe7L72y2HDAa