fix(r3): judge each rel once per request — a side or review item that vanishes mid-request never 500s
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.
This commit is contained in:
+25
-19
@@ -1971,7 +1971,14 @@ def create_app(
|
|||||||
members = [r for r in ring if by_rel[r].group == item.group]
|
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)}
|
group = {"key": item.group, "k": members.index(f) + 1, "n": len(members)}
|
||||||
open_now = open_marks(marks)
|
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(
|
return templates.TemplateResponse(
|
||||||
request, "view.html", {
|
request, "view.html", {
|
||||||
**common,
|
**common,
|
||||||
@@ -1993,12 +2000,12 @@ def create_app(
|
|||||||
"is_last": pos == len(ring) - 1,
|
"is_last": pos == len(ring) - 1,
|
||||||
"tray": [x for x in film if x["flagged"]],
|
"tray": [x for x in film if x["flagged"]],
|
||||||
"back_url": f"/b/{quote(name, safe='')}/#item-{item.url}",
|
"back_url": f"/b/{quote(name, safe='')}/#item-{item.url}",
|
||||||
# R3 C2: this item against the NEXT in the compare ring
|
# R3 C2: this item against the NEXT comparable item in the
|
||||||
# (itself in a ring of one), keyed by rel like every compare
|
# ring (itself in a ring of one), keyed by rel like every
|
||||||
# URL. This item passed the containment check above, so it
|
# compare URL; None, and no control, when it is not itself
|
||||||
# is in the compare ring.
|
# comparable any more (it vanished after the check above).
|
||||||
"compare_url": (f"/b/{quote(name, safe='')}/compare?a={quote(f, safe='/')}"
|
"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
|
# .md renders, .txt/.log show as text — viewable in-booth, no download
|
||||||
@@ -2039,16 +2046,15 @@ def create_app(
|
|||||||
bug hunt, 3 of 4)."""
|
bug hunt, 3 of 4)."""
|
||||||
return [r for r in review_chain(items) if _in_booth(booth, r)]
|
return [r for r in review_chain(items) if _in_booth(booth, r)]
|
||||||
|
|
||||||
def _compare_side(booth: Path, ring: list[str], rel) -> str:
|
def _compare_side(ring: list[str], rel) -> str:
|
||||||
"""One side of a compare, or a 404 (R3 C1). A CONJUNCTION: the view
|
"""One side of a compare, or a 404 (R3 C1): membership of the COMPARE
|
||||||
route's resolve / containment / is_file check, AND membership of the
|
ring, which is already the conjunction — the review ring (a doc is not
|
||||||
review ring. The ring alone is not enough — `booth_items` follows
|
in it) filtered by the view route's containment (an outside symlink is
|
||||||
symlinks, so a link pointing outside the booth is IN the ring and only
|
in the review ring and not in this one). Judged against the ONE ring
|
||||||
containment refuses it. The view's check alone is not enough — it
|
the route builds per request: resolving a rel twice lets a file that
|
||||||
renders a doc, and compare takes media only."""
|
vanishes between the two reach a `.index()` that raises, a 500
|
||||||
if not _in_booth(booth, rel):
|
(booth-dev, after the merge)."""
|
||||||
raise HTTPException(status_code=404, detail="no such item")
|
if not isinstance(rel, str) or rel not in ring:
|
||||||
if rel not in ring:
|
|
||||||
raise HTTPException(status_code=404, detail="no such item")
|
raise HTTPException(status_code=404, detail="no such item")
|
||||||
return rel
|
return rel
|
||||||
|
|
||||||
@@ -2073,9 +2079,9 @@ def create_app(
|
|||||||
"""
|
"""
|
||||||
booth = resolve_booth(name)
|
booth = resolve_booth(name)
|
||||||
items = booth_items(booth)
|
items = booth_items(booth)
|
||||||
a = _compare_side(booth, review_chain(items), a)
|
ring = _compare_ring(booth, items) # built ONCE; every rel judged once
|
||||||
b = _compare_side(booth, review_chain(items), b)
|
a = _compare_side(ring, a)
|
||||||
ring = _compare_ring(booth, items)
|
b = _compare_side(ring, b)
|
||||||
# A look records both — below the 404s, so only a real pair counts.
|
# A look records both — below the 404s, so only a real pair counts.
|
||||||
record_view(booth)
|
record_view(booth)
|
||||||
record_seen(booth, a, items)
|
record_seen(booth, a, items)
|
||||||
|
|||||||
@@ -14,7 +14,7 @@
|
|||||||
<span class="vname"><span class="ord">{{ num(ordinal) }}</span> {{ file }}</span>
|
<span class="vname"><span class="ord">{{ num(ordinal) }}</span> {{ file }}</span>
|
||||||
<span class="vspacer"></span>
|
<span class="vspacer"></span>
|
||||||
{# R3: this item against the next one in the ring, side by side (C). #}
|
{# R3: this item against the next one in the ring, side by side (C). #}
|
||||||
<a class="vbtn vcompare" href="{{ compare_url }}" title="compare with the next item (C)"><span aria-hidden="true">⇆</span><span class="vcompare-l"> compare</span></a>
|
{% if compare_url %}<a class="vbtn vcompare" href="{{ compare_url }}" title="compare with the next item (C)"><span aria-hidden="true">⇆</span><span class="vcompare-l"> compare</span></a>{% endif %}
|
||||||
{% if kind == 'image' %}
|
{% if kind == 'image' %}
|
||||||
{# A JS-only VIEWING convenience (INV-3): hidden until the script shows it,
|
{# 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
|
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 */
|
var b = document.getElementById('vflag-btn'); /* re-read: the rail may have been swapped */
|
||||||
if (b) { e.preventDefault(); b.click(); }
|
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') {
|
else if (e.key === 'n' || e.key === 'N') {
|
||||||
var t = document.getElementById('vnote-text');
|
var t = document.getElementById('vnote-text');
|
||||||
if (t) { e.preventDefault(); t.focus(); }
|
if (t) { e.preventDefault(); t.focus(); }
|
||||||
|
|||||||
@@ -62,6 +62,13 @@ assumptions:
|
|||||||
stays in the review ring). EVERY compare link is built from it: the strip,
|
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
|
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.
|
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,
|
- 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
|
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),
|
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
|
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.
|
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
|
- **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
|
WRAPS** instead of scrolling the page sideways or crushing a control. The
|
||||||
applies to the review's bar too, which gains the Compare control (only its
|
rule is on `.vbar`, so it applies to every bar of that class: compare's, the
|
||||||
glyph below 600px). The review's bar was already full: a fogged booth
|
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.
|
overflowed it by 3px at 390px before r3.
|
||||||
- **Each stage is the r2c stage:** Fit fills (up or down, contain, never
|
- **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
|
cropped), or 1:1 at natural pixels with every pixel reachable. Drag pans a
|
||||||
|
|||||||
+32
-8
@@ -19,11 +19,9 @@ label = "C1 the conjunction loses the ring (a doc or a sidecar opens as a side)"
|
|||||||
file = "booth/app.py"
|
file = "booth/app.py"
|
||||||
test = "tests/test_compare.py::test_a_bad_side_is_a_404"
|
test = "tests/test_compare.py::test_a_bad_side_is_a_404"
|
||||||
old = '''
|
old = '''
|
||||||
if rel not in ring:
|
return [r for r in review_chain(items) if _in_booth(booth, r)]'''
|
||||||
raise HTTPException(status_code=404, detail="no such item")
|
|
||||||
return rel'''
|
|
||||||
new = '''
|
new = '''
|
||||||
return rel'''
|
return [it.rel for it in items if _in_booth(booth, it.rel)]'''
|
||||||
|
|
||||||
[[mutation]]
|
[[mutation]]
|
||||||
label = "C1 a missing side is FastAPI's 422 (no default)"
|
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 = '''
|
old = '''
|
||||||
booth = resolve_booth(name)
|
booth = resolve_booth(name)
|
||||||
items = booth_items(booth)
|
items = booth_items(booth)
|
||||||
a = _compare_side(booth, review_chain(items), a)'''
|
ring = _compare_ring(booth, items)'''
|
||||||
new = '''
|
new = '''
|
||||||
booth = resolve_booth(name)
|
booth = resolve_booth(name)
|
||||||
record_view(booth)
|
record_view(booth)
|
||||||
items = booth_items(booth)
|
items = booth_items(booth)
|
||||||
a = _compare_side(booth, review_chain(items), a)'''
|
ring = _compare_ring(booth, items)'''
|
||||||
|
|
||||||
[[mutation]]
|
[[mutation]]
|
||||||
label = "C6 compare does not carry data-booth (Reveal all and its restore bail)"
|
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)"
|
label = "C2 the review's Compare does not wrap (the last item compares with itself)"
|
||||||
file = "booth/app.py"
|
file = "booth/app.py"
|
||||||
test = "tests/test_compare.py::test_the_review_offers_compare_with_the_next_item"
|
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='/')}"),'''
|
old = '''(ring[(pos + k) % len(ring)] for k in range(1, len(ring) + 1))'''
|
||||||
new = ''' f"&b={quote(cring[min(cring.index(f) + 1, len(cring) - 1)], safe='/')}"),'''
|
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
|
# ---- C5: the regions and the JS-off flag landing
|
||||||
|
|
||||||
@@ -543,6 +541,32 @@ old = '''
|
|||||||
new = '''
|
new = '''
|
||||||
return p && !/^(side|link)(=|$)/.test(p);'''
|
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
|
# 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
|
# second button pressed and released during a drag arrives as chorded
|
||||||
# `pointermove` events, never a `pointerup` (measured 3/3 in the test browser,
|
# `pointermove` events, never a `pointerup` (measured 3/3 in the test browser,
|
||||||
|
|||||||
@@ -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"
|
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):
|
def test_a_look_records_both_seen(tmp_path):
|
||||||
"""A compare GET is a look at both sides; a 404 records nothing."""
|
"""A compare GET is a look at both sides; a 404 records nothing."""
|
||||||
import json
|
import json
|
||||||
|
|||||||
Reference in New Issue
Block a user