fix(diagnostics): deep-review round 4 — a claim only its owner may write, and a player that keeps the Tab

Round 4 was targeted at the surfaces rounds 2 and 3 changed. 29 findings raised,
26 survived deduplication, 12 survived three adversarial readings each.

`set_delivery` renewed the claim but never checked it was still ours, while its
sibling `release_send` always did. `expire_on_commit` is off, so the token it
re-stamped came from the caller's own memory, not the row: a send that outlived
its 600 s lease came back, overwrote the take-over send's committed links with a
full-column write, and put its own dead token back. It now takes the token, reads
the row's claim with a targeted scalar (not a refresh, which would discard the
`issue_url` assigned one line earlier) and raises `SendClaimLost` rather than
writing. An issue created by a send that had already lost its claim is logged with
its number — that record is the only one that will exist.

The tracker's settings are read above the pre-call checkpoint instead of inside
`_client`. Reading them there was a SELECT one line after the commit that was
meant to end the transaction, and held it — idle, snapshot pinned — across the
repository check and the create. The comment and the module docstring said
otherwise; measuring it in pg_stat_activity is what settled it. Two more comments
were saying things the code had stopped doing: the `**state` spread does not carry
the claim (`set_delivery` strips and re-stamps it), and react-query does not
debounce the preview — the editor being unmounted is what paces it.

The composer stopped stranding the focus on `<body>`: a send that creates the
issue and then fails on an attachment unmounts the button that was just pressed,
and only the success path handed the focus on. Export no longer saves into a
refusal it does not need — the zip is the way out of a stuck send, and its
pre-save was the one step that could not survive one. The delivery banner
announces through a role instead of an `aria-live` created with its own message.

`historyState` returns a union, so `chipTone` matching on the same keys is checked
by tsc rather than by hope.

PlexPlayer was the only full-screen overlay in the tree with no focus containment
at all — Tab walked out of an opaque player into a page nobody can see. It now
carries the shared trap. Closing that hole exposed the one underneath it:
`useDismiss` registered as a layer that traps the Tab while installing no trap,
so the player stood its own down for its own menus. Measured in a real browser:
30 Tabs with the settings panel open, 9 escapes into the nav rail, and 0 after.
Escape ordering is unchanged — the panel first, then the player.

Three tests that did not hold their line: the advisory lock's call site (deleting
it left the suite green), the trail assertion whose `all()` was vacuous over an
empty filter, and a `layerStack` assertion the stand-in dialog already forced.
All eleven executable fixes mutation-checked red for the right reason; the two
comment corrections have nothing to pin.
This commit is contained in:
2026-09-20 04:30:42 +02:00
parent 2fe58073e4
commit 90fc86d437
12 changed files with 486 additions and 61 deletions
+55 -18
View File
@@ -496,18 +496,24 @@ def test_a_half_sent_session_keeps_its_screenshots_a_finished_one_lets_them_go(d
shot = _upload(client, sid, _PNG).json()["screenshot"]["id"]
session = db.get(DebugSession, sid)
session.issue_url = "https://git.example/o/r/issues/1"
debugsession.set_delivery(session, {"number": 1, "pending": ["server.log"], "links": {}})
_progress(session, {"number": 1, "pending": ["server.log"], "links": {}})
db.commit()
# A send that still owes files names its screenshots by their order: removing one renumbers
# what it has left to send.
r = client.delete(f"/api/debug-sessions/{sid}/screenshots/{shot}")
assert r.status_code == 409 and r.json()["detail"]["reason"] == "screenshot_already_sent"
# Finished: the issue has its own copy, and ours is the user's to drop.
debugsession.set_delivery(session, {"number": 1, "pending": [], "links": {}})
_progress(session, {"number": 1, "pending": [], "links": {}})
db.commit()
assert client.delete(f"/api/debug-sessions/{sid}/screenshots/{shot}").status_code == 200
def _progress(session, state):
"""The delivery a finished-or-abandoned send left on the row, with no claim standing on it.
`set_delivery` is the live send's writer and refuses without one, which is the point of it."""
session.draft = {**(session.draft or {}), "delivery": state}
def _age_the_claim(session, hours=1):
"""The claim a send holds, as a request that died an hour ago left it."""
state = debugsession.delivery(session)
@@ -556,31 +562,50 @@ def test_a_slow_send_keeps_its_own_claim_alive_and_gives_back_only_that_one(db,
session = db.get(DebugSession, sid)
token = debugsession.claim_send(session)
_age_the_claim(session)
db.commit()
assert debugsession.sending(session) is False
# One more step reported — proof of life, so the claim stands again.
debugsession.set_delivery(session, {"number": 7, "pending": ["server.log"], "links": {}})
debugsession.set_delivery(
db, session, {"number": 7, "pending": ["server.log"], "links": {}}, token
)
db.commit()
assert debugsession.sending(session) is True
assert debugsession.not_uploaded(session) == ["server.log"]
# And once a send that aged out HAS been taken over, the old one writes nothing: its progress
# report is a whole-column overwrite, so without this it put its own dead token back and took
# the new send's committed links with it. `expire_on_commit` is off, so the old send still has
# its own token in memory — only the row can say whose claim it is.
taken = debugsession.claim_send(session)
db.commit()
with pytest.raises(debugsession.SendClaimLost):
debugsession.set_delivery(db, session, {"number": 9, "pending": [], "links": {}}, token)
assert debugsession.delivery(session)["number"] == 7
assert debugsession.not_uploaded(session) == ["server.log"]
# The claim of a send that took over after ours aged out is not ours to hand back.
debugsession.release_send(session, "a-different-send")
assert debugsession.sending(session) is True
debugsession.release_send(session, token)
assert debugsession.sending(session) is True
debugsession.release_send(session, taken)
assert debugsession.sending(session) is False
def test_a_users_uploads_wait_for_each_other_across_their_sessions(db):
def test_a_users_uploads_wait_for_each_other_across_their_sessions(db, recording):
"""The cap below is the user's across ALL their sessions, but a session's row lock only bounds
that session — two uploads to two sessions both read the total before either commits, and both
pass. The user's own lock is what makes the cap a cap instead of a guess."""
pass. The user's own lock is what makes the cap a cap instead of a guess.
Driven through `add_screenshot`, not through `lock_user`: the primitive works whether or not
the upload calls it, so a test that called it here stayed green with the call site deleted."""
from app.db import SessionLocal
debugsession.lock_user(db, 4242)
client, sid = recording
session = db.get(DebugSession, sid)
debugsession.add_screenshot(db, session, _PNG) # not committed: the lock is ours until it is
other = SessionLocal()
try:
# A second request for the same user cannot get in to read the total while we hold it...
assert other.scalar(select(func.pg_try_advisory_xact_lock(4242))) is False
# A second upload by this user cannot get in to read the total while this one holds it...
assert other.scalar(select(func.pg_try_advisory_xact_lock(session.user_id))) is False
# ...and another user is not queued behind them for it.
assert other.scalar(select(func.pg_try_advisory_xact_lock(4243))) is True
assert other.scalar(select(func.pg_try_advisory_xact_lock(session.user_id + 1))) is True
finally:
other.rollback()
other.close()
@@ -883,15 +908,23 @@ def test_the_claim_is_held_through_every_step_of_a_send(gitea_fake, db, make_use
_upload(client, sid, _PNG)
_ready(client, sid)
session = db.get(DebugSession, sid)
debugsession.claim_send(session)
token = debugsession.claim_send(session)
db.commit()
held: list[bool] = []
held: list[tuple[bool, int]] = []
gitea.deliver(
db,
session,
checkpoint=lambda: (db.commit(), held.append(debugsession.sending(session)))[0],
token=token,
checkpoint=lambda: (
db.commit(),
held.append((debugsession.sending(session), len(gitea_fake.calls))),
)[0],
)
assert held and all(held) # after the issue was created, and after every upload
# The first checkpoint comes before the tracker is touched at all — that is what keeps the
# bundle's read snapshot, and its connection, out of the calls that follow.
assert held[0] == (True, 0)
# ...and the claim outlasts the issue's creation and every upload after it.
assert len(held) > 1 and all(alive for alive, _ in held)
def test_the_issues_trail_is_written_by_the_step_that_writes_the_link(
@@ -908,7 +941,7 @@ def test_the_issues_trail_is_written_by_the_step_that_writes_the_link(
_upload(client, sid, _PNG)
_ready(client, sid)
session = db.get(DebugSession, sid)
debugsession.claim_send(session)
token = debugsession.claim_send(session)
db.commit()
seen: list[tuple[bool, int]] = []
@@ -922,11 +955,15 @@ def test_the_issues_trail_is_written_by_the_step_that_writes_the_link(
gitea.deliver(
db,
session,
token=token,
checkpoint=checkpoint,
on_created=lambda: routes._record_created(db, user, session),
)
# Every commit that can be seen with the link can be seen with the trail beside it.
assert seen and all(rows == 1 for linked, rows in seen if linked)
# Every commit that can be seen with the link can be seen with the trail beside it — and there
# has to BE one, or the filter selects nothing and `all()` is vacuously true. This assertion
# used to go green over a `deliver` that never wrote the link at all.
linked_rows = [rows for linked, rows in seen if linked]
assert linked_rows and all(rows == 1 for rows in linked_rows)
def test_a_send_that_cannot_give_its_claim_back_still_reports_its_own_failure(