From 8633b1dded62c7dfc12c5132e1c1ca3f0c123201 Mon Sep 17 00:00:00 2001 From: Vuong Hoang Date: Thu, 24 Sep 2026 15:59:43 -0700 Subject: [PATCH] =?UTF-8?q?fix(r3):=20judge=20each=20rel=20once=20per=20re?= =?UTF-8?q?quest=20=E2=80=94=20a=20side=20or=20review=20item=20that=20vani?= =?UTF-8?q?shes=20mid-request=20never=20500s?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit booth-dev's race note after the merge: the compare route resolved each side in _compare_side and again in _compare_ring, then ring.index(a) raised if the file vanished (or was relinked outside the booth) between the two; the review did the same through cring.index(f). The compare ring is now built once and the sides are judged by membership of it. The review re-judges its item and scans forward for the next comparable one (usually one step, no longer a resolve of the whole ring per render); an item no longer comparable renders the review without a Compare control, and C does nothing. The contract records the once-per-request rule and that the phone-width wrap covers doc.html's bar too. r3.toml: 59 rows, four re-anchored. --- booth/app.py | 44 +++++++++++++----------- booth/templates/view.html | 4 +-- docs/contracts/r3_compare.contract.md | 14 ++++++-- tests/mutations/r3.toml | 40 +++++++++++++++++----- tests/test_compare.py | 48 +++++++++++++++++++++++++++ 5 files changed, 118 insertions(+), 32 deletions(-) diff --git a/booth/app.py b/booth/app.py index 4184932..30b0e11 100644 --- a/booth/app.py +++ b/booth/app.py @@ -1971,7 +1971,14 @@ def create_app( members = [r for r in ring if by_rel[r].group == item.group] group = {"key": item.group, "k": members.index(f) + 1, "n": len(members)} open_now = open_marks(marks) - cring = _compare_ring(booth, items) + # The compare partner: the next ring item a compare can open, + # scanning forward (usually one step) rather than resolving the + # whole ring per render. Each rel is judged once; this item is + # re-judged first, so a race costs the control, never the page. + partner = None + if _in_booth(booth, f): + partner = next((r for r in (ring[(pos + k) % len(ring)] for k in range(1, len(ring) + 1)) + if r == f or _in_booth(booth, r)), None) return templates.TemplateResponse( request, "view.html", { **common, @@ -1993,12 +2000,12 @@ def create_app( "is_last": pos == len(ring) - 1, "tray": [x for x in film if x["flagged"]], "back_url": f"/b/{quote(name, safe='')}/#item-{item.url}", - # R3 C2: this item against the NEXT in the compare ring - # (itself in a ring of one), keyed by rel like every compare - # URL. This item passed the containment check above, so it - # is in the compare ring. + # R3 C2: this item against the NEXT comparable item in the + # ring (itself in a ring of one), keyed by rel like every + # compare URL; None, and no control, when it is not itself + # comparable any more (it vanished after the check above). "compare_url": (f"/b/{quote(name, safe='')}/compare?a={quote(f, safe='/')}" - f"&b={quote(cring[(cring.index(f) + 1) % len(cring)], safe='/')}"), + f"&b={quote(partner, safe='/')}") if partner else None, }) # .md renders, .txt/.log show as text — viewable in-booth, no download @@ -2039,16 +2046,15 @@ def create_app( bug hunt, 3 of 4).""" return [r for r in review_chain(items) if _in_booth(booth, r)] - def _compare_side(booth: Path, ring: list[str], rel) -> str: - """One side of a compare, or a 404 (R3 C1). A CONJUNCTION: the view - route's resolve / containment / is_file check, AND membership of the - review ring. The ring alone is not enough — `booth_items` follows - symlinks, so a link pointing outside the booth is IN the ring and only - containment refuses it. The view's check alone is not enough — it - renders a doc, and compare takes media only.""" - if not _in_booth(booth, rel): - raise HTTPException(status_code=404, detail="no such item") - if rel not in ring: + def _compare_side(ring: list[str], rel) -> str: + """One side of a compare, or a 404 (R3 C1): membership of the COMPARE + ring, which is already the conjunction — the review ring (a doc is not + in it) filtered by the view route's containment (an outside symlink is + in the review ring and not in this one). Judged against the ONE ring + the route builds per request: resolving a rel twice lets a file that + vanishes between the two reach a `.index()` that raises, a 500 + (booth-dev, after the merge).""" + if not isinstance(rel, str) or rel not in ring: raise HTTPException(status_code=404, detail="no such item") return rel @@ -2073,9 +2079,9 @@ def create_app( """ booth = resolve_booth(name) items = booth_items(booth) - a = _compare_side(booth, review_chain(items), a) - b = _compare_side(booth, review_chain(items), b) - ring = _compare_ring(booth, items) + ring = _compare_ring(booth, items) # built ONCE; every rel judged once + a = _compare_side(ring, a) + b = _compare_side(ring, b) # A look records both — below the 404s, so only a real pair counts. record_view(booth) record_seen(booth, a, items) diff --git a/booth/templates/view.html b/booth/templates/view.html index 3a12647..b4d94d5 100644 --- a/booth/templates/view.html +++ b/booth/templates/view.html @@ -14,7 +14,7 @@ {{ num(ordinal) }} {{ file }} {# R3: this item against the next one in the ring, side by side (C). #} - compare + {% if compare_url %} compare{% endif %} {% if kind == 'image' %} {# A JS-only VIEWING convenience (INV-3): hidden until the script shows it, and only ever rendered for a picture. With scripts off the image shows at @@ -289,7 +289,7 @@ var b = document.getElementById('vflag-btn'); /* re-read: the rail may have been swapped */ if (b) { e.preventDefault(); b.click(); } } - else if (e.key === 'c' || e.key === 'C') { e.preventDefault(); window.location.href = COMPARE; } + else if ((e.key === 'c' || e.key === 'C') && COMPARE) { e.preventDefault(); window.location.href = COMPARE; } else if (e.key === 'n' || e.key === 'N') { var t = document.getElementById('vnote-text'); if (t) { e.preventDefault(); t.focus(); } diff --git a/docs/contracts/r3_compare.contract.md b/docs/contracts/r3_compare.contract.md index 6d3f60f..3a47f37 100644 --- a/docs/contracts/r3_compare.contract.md +++ b/docs/contracts/r3_compare.contract.md @@ -62,6 +62,13 @@ assumptions: stays in the review ring). EVERY compare link is built from it: the strip, the steps, the review's Compare control and the `back=compare` landing. So no navigation offers a pair that 404s, and a step walks over such an item. +- **Each rel is judged ONCE per request** (booth-dev, after the merge). The + route builds the compare ring once and judges both sides by membership of + it. Resolving a rel twice lets a file that vanishes between the two reach a + lookup that raises, a 500. The review re-judges its own item first and scans + forward for the next comparable one; when its item is no longer comparable, + the review renders WITHOUT a Compare control (and `C` does nothing), never a + 500. - The response carries, per side: the rel, its quoted url, ordinal, kind, caption, blurred, flagged and thumb. It also carries the compare ring as a filmstrip in RING ORDER (the view route's `film`, one line in the route's docstring), @@ -142,9 +149,10 @@ assumptions: gap, never a border that comes out of one side's width. Two stages of different sizes would draw the same picture at two scales in Fit. - **At phone width (600px and below) a top bar that cannot hold its controls - WRAPS** instead of scrolling the page sideways or crushing a control. This - applies to the review's bar too, which gains the Compare control (only its - glyph below 600px). The review's bar was already full: a fogged booth + WRAPS** instead of scrolling the page sideways or crushing a control. The + rule is on `.vbar`, so it applies to every bar of that class: compare's, the + review's (which gains the Compare control, only its glyph below 600px), and + doc.html's. The review's bar was already full: a fogged booth overflowed it by 3px at 390px before r3. - **Each stage is the r2c stage:** Fit fills (up or down, contain, never cropped), or 1:1 at natural pixels with every pixel reachable. Drag pans a diff --git a/tests/mutations/r3.toml b/tests/mutations/r3.toml index b30c0c2..e38f1bf 100644 --- a/tests/mutations/r3.toml +++ b/tests/mutations/r3.toml @@ -19,11 +19,9 @@ label = "C1 the conjunction loses the ring (a doc or a sidecar opens as a side)" file = "booth/app.py" test = "tests/test_compare.py::test_a_bad_side_is_a_404" old = ''' - if rel not in ring: - raise HTTPException(status_code=404, detail="no such item") - return rel''' + return [r for r in review_chain(items) if _in_booth(booth, r)]''' new = ''' - return rel''' + return [it.rel for it in items if _in_booth(booth, it.rel)]''' [[mutation]] label = "C1 a missing side is FastAPI's 422 (no default)" @@ -48,12 +46,12 @@ test = "tests/test_compare.py::test_a_look_records_both_seen" old = ''' booth = resolve_booth(name) items = booth_items(booth) - a = _compare_side(booth, review_chain(items), a)''' + ring = _compare_ring(booth, items)''' new = ''' booth = resolve_booth(name) record_view(booth) items = booth_items(booth) - a = _compare_side(booth, review_chain(items), a)''' + ring = _compare_ring(booth, items)''' [[mutation]] label = "C6 compare does not carry data-booth (Reveal all and its restore bail)" @@ -114,8 +112,8 @@ new = ''' side_a = side not in ("", "b")''' label = "C2 the review's Compare does not wrap (the last item compares with itself)" file = "booth/app.py" test = "tests/test_compare.py::test_the_review_offers_compare_with_the_next_item" -old = ''' f"&b={quote(cring[(cring.index(f) + 1) % len(cring)], safe='/')}"),''' -new = ''' f"&b={quote(cring[min(cring.index(f) + 1, len(cring) - 1)], safe='/')}"),''' +old = '''(ring[(pos + k) % len(ring)] for k in range(1, len(ring) + 1))''' +new = '''(ring[min(pos + k, len(ring) - 1)] for k in range(1, len(ring) + 1))''' # ---- C5: the regions and the JS-off flag landing @@ -543,6 +541,32 @@ old = ''' new = ''' return p && !/^(side|link)(=|$)/.test(p);''' +# ---- after the merge: booth-dev's race note (01M3AT7GKCPATJD5YW0PR3SRPT) + +[[mutation]] +label = "C1 a side is judged twice (the ring rebuilt per side): a side that vanishes between is a 500" +file = "booth/app.py" +test = "tests/test_compare.py::test_a_side_that_vanishes_mid_request_never_500s" +old = ''' + ring = _compare_ring(booth, items) # built ONCE; every rel judged once + a = _compare_side(ring, a) + b = _compare_side(ring, b)''' +new = ''' + a = _compare_side(_compare_ring(booth, items), a) + b = _compare_side(_compare_ring(booth, items), b) + ring = _compare_ring(booth, items)''' + +[[mutation]] +label = "C2 the review offers Compare for an item that vanished after its own check" +file = "booth/app.py" +test = "tests/test_compare.py::test_the_review_hides_compare_when_its_item_vanishes_mid_request" +old = ''' + if _in_booth(booth, f): + partner''' +new = ''' + if True: + partner''' + # Refuted, not rowed (bug hunt): "a right-click mid-drag ends the pan" — a # second button pressed and released during a drag arrives as chorded # `pointermove` events, never a `pointerup` (measured 3/3 in the test browser, diff --git a/tests/test_compare.py b/tests/test_compare.py index d117ed1..fa6b883 100644 --- a/tests/test_compare.py +++ b/tests/test_compare.py @@ -165,6 +165,54 @@ def test_a_planted_fifo_marker_cannot_hang_a_look(tmp_path): assert got == [200], "a planted FIFO held the look open" +def _vanish_after_scan(monkeypatch, name: str, grace: int) -> None: + """Make `name` stop being a file partway through a request: once + booth_items has scanned the booth, the first `grace` is_file checks of it + still pass and every later one fails — a file deleted or relinked outside + the booth mid-request, between two resolves of the same rel.""" + import pathlib as _pl + import booth.app as app_mod + state = {"armed": False, "calls": 0} + real_items, real_is_file = app_mod.booth_items, _pl.Path.is_file + + def items(booth): + out = real_items(booth) + state["armed"] = True + return out + + def is_file(self): + if state["armed"] and self.name == name: + state["calls"] += 1 + if state["calls"] > grace: + return False + return real_is_file(self) + + monkeypatch.setattr(app_mod, "booth_items", items) + monkeypatch.setattr(_pl.Path, "is_file", is_file) + + +def test_a_side_that_vanishes_mid_request_never_500s(tmp_path, monkeypatch): + """booth-dev's race: each rel must be judged ONCE per request. A side that + passes its check and then vanishes before a second resolve must not reach + a `.index()` that raises — a damaged file costs its own tile, never the + page.""" + _booth(tmp_path, "g", {"p.png": PNG, "q.png": PNG, "r.png": PNG}) + _vanish_after_scan(monkeypatch, "p.png", grace=1) + r = _client(tmp_path).get("/b/g/compare?a=p.png&b=q.png") + assert r.status_code in (200, 404), r.status_code + + +def test_the_review_hides_compare_when_its_item_vanishes_mid_request(tmp_path, monkeypatch): + """The review checked its item, then the item vanished before the compare + ring was built: the page still renders, without a Compare control (a + compare of it would 404) — never a 500.""" + _booth(tmp_path, "g", {"p.png": PNG, "q.png": PNG}) + _vanish_after_scan(monkeypatch, "p.png", grace=0) + r = _client(tmp_path).get("/b/g/view?f=p.png") + assert r.status_code == 200, r.status_code + assert 'class="vbtn vcompare"' not in r.text + + def test_a_look_records_both_seen(tmp_path): """A compare GET is a look at both sides; a 404 records nothing.""" import json