fix(marks): v0.2.2 — nine findings from the cross-frontier bug-hunt panel
`/heid-bug-hunt` on U2's diff, four arms, artifact-only. Eight findings were real against live code; a ninth was already closed by v0.2.1 and is recorded as declined. Full triage in persistent-memory.d/2026-09-22-bug-hunt-panel.md. THE LOCK LIFECYCLE (4/4 convergent, and two defects in one place) `_Locked.__exit__` unlinked `.marks.lock` on the no-op path so a booth that had never been marked was left exactly as it was found. `flock` binds to an INODE: unlinking it under a blocked waiter leaves that waiter holding an exclusive lock on a deleted file while the next writer creates a fresh lock and takes it immediately. Two processes then run the read-modify-write concurrently, the later os.replace drops the earlier one's mark, and both obeyed the protocol. The cleanup existed to protect the booth's TTL, and was failing at that too: creating or removing a directory entry bumps the DIRECTORY's mtime, which is what `_newest_mtime` seeds from. The guard's comment reasons about the lock file's own mtime and misses that the directory moved underneath it. One fix: never unlink the lock, exempt `.<name>.lock` dotfiles from `_newest_mtime`, and restore the directory's mtime after creating one. THE READ PATH'S BLAST RADIUS `_clean_text` did `(text or "").replace(...)` and `marks_for` sorts on `(created, id)`, so a stored `text` that was a dict or a `created` that was a number raised out of the read path. `list_booths` reads every booth's marks on every index load, so one hand-edited file returned 500 for `/` and `/healthz` across all 25 booths. Guarded in two layers — a named type check and a `_hydrate_safe` backstop that cannot raise — and an unreadable mark now renders as ⚠ broken rather than as an empty note. ALSO - import_legacy_asks stamped `created` at whole-second resolution, so two sidecars from the same second lost the ordering the importer had just established and re-sorted alphabetically. Microseconds, per the stated `(mtime, name)` rule. - The five mark-write routes ran a blocking flock on the event loop; they now dispatch through run_in_threadpool, asserted structurally like INV-1. - `/answer` 500'd on a non-string `notes` form value where `/note` handled it. - The inline-doc tile had a flag control and no note field. - The marks panel was suppressed on any booth carrying a links.md. - The viewer's arrow keys and Escape threw away a note being typed. CLI `booth marks` printed a traceback and exited 0 on a failed read, and `--wait` emitted a whole JSON document per poll. `booth answer --wait` read a damaged file as "not yet" and spun the full hour. Both now use real exit codes — 0 ok, 1 unanswered/timed-out, 2 no such pick, 3 unreadable — and `--wait` prints once. `marks.read_error()` lets the CLI ask what the page must not: the browser stays lenient, the machine consumer gets the truth. `scripts/booth` had no tests; it has five now, run against the real script under the system python3, which also makes them a live check on INV-1. 275 tests (253 before). Live service restarted, 25/25 booth pages verified 200.
This commit is contained in:
Vuong Hoang
+49
-9
@@ -45,6 +45,7 @@ from fastapi.responses import (
|
||||
Response,
|
||||
)
|
||||
from fastapi.templating import Jinja2Templates
|
||||
from starlette.concurrency import run_in_threadpool
|
||||
from jinja2 import Environment, FileSystemLoader, select_autoescape
|
||||
|
||||
try:
|
||||
@@ -169,12 +170,27 @@ def human_dur(seconds: float) -> str:
|
||||
|
||||
|
||||
def _newest_mtime(path: Path) -> float:
|
||||
"""Newest mtime among a folder and everything under it."""
|
||||
"""Newest mtime among a folder and everything under it — OUR LOCKS EXCEPT.
|
||||
|
||||
A booth's age is how long since somebody touched it, and a lock sidecar is
|
||||
machinery: `marks.py` and `links.py` each create one on the way into a
|
||||
read-modify-write, including one that turns out to change nothing. Counting
|
||||
it made reading-through-a-write-path look like activity, and a no-op mark
|
||||
POST on a dead booth reset its clock.
|
||||
|
||||
The exclusion is `.<something>.lock` — a DOTfile, which is the Booth's own
|
||||
namespace. An agent that posts a real artifact called `build.lock` still
|
||||
gets its clock counted. Everything else counts too, dotfiles included,
|
||||
because `.marks.json`, `.blurred` and `.pins` are the operator doing
|
||||
something.
|
||||
"""
|
||||
try:
|
||||
newest = path.stat().st_mtime
|
||||
except OSError:
|
||||
return 0.0
|
||||
for p in path.rglob("*"):
|
||||
if p.name.startswith(".") and p.name.endswith(".lock"):
|
||||
continue
|
||||
try:
|
||||
m = p.stat().st_mtime
|
||||
except OSError:
|
||||
@@ -478,6 +494,18 @@ PICKUP_WORDS = (
|
||||
).split()
|
||||
|
||||
|
||||
def _form_text(form, key: str) -> str:
|
||||
"""One form field as text, or "" for anything that is not text.
|
||||
|
||||
A multipart FILE part named `notes` parses to an UploadFile, not a str, and
|
||||
every downstream cleaner calls `.replace` on what it is handed. Coercing
|
||||
here keeps that decision in one place instead of one `isinstance` per call
|
||||
site — which is how `/note` came to have the guard and `/answer` not to.
|
||||
"""
|
||||
value = form.get(key)
|
||||
return value if isinstance(value, str) else ""
|
||||
|
||||
|
||||
def safe_upload_name(name: str, fallback: str) -> str:
|
||||
"""Reduce a client-supplied filename to a safe basename (no path, no hidden)."""
|
||||
base = (name or "").replace("\\", "/").split("/")[-1].strip()
|
||||
@@ -760,13 +788,22 @@ def create_app(
|
||||
if spec.error is not None:
|
||||
raise HTTPException(status_code=400, detail=spec.error)
|
||||
who = request.client.host if request.client else ""
|
||||
# `notes` is whatever the form parser yielded. A multipart FILE part
|
||||
# named `notes` is an UploadFile, and `_clean_notes` calls `.replace` on
|
||||
# it — a 500 on hostile-but-legal input, where the sibling `/note` route
|
||||
# returns 400 for exactly the same class of value. Same parser, same
|
||||
# question, one answer.
|
||||
notes = _form_text(form, "notes")
|
||||
try:
|
||||
if spec.multi:
|
||||
choice = {q["key"]: form.get(f"choice.{q['key']}") for q in spec.questions}
|
||||
qnotes = {q["key"]: form.get(f"notes.{q['key']}") for q in spec.questions}
|
||||
answer_pick(booth, mark_id, choice, form.get("notes", ""), who=who, qnotes=qnotes)
|
||||
qnotes = {q["key"]: _form_text(form, f"notes.{q['key']}")
|
||||
for q in spec.questions}
|
||||
await run_in_threadpool(answer_pick, booth, mark_id, choice, notes,
|
||||
who=who, qnotes=qnotes)
|
||||
else:
|
||||
answer_pick(booth, mark_id, form.get("choice"), form.get("notes", ""), who=who)
|
||||
await run_in_threadpool(answer_pick, booth, mark_id,
|
||||
form.get("choice"), notes, who=who)
|
||||
except AskError as exc:
|
||||
raise HTTPException(status_code=400, detail=str(exc))
|
||||
return _mark_redirect(name, form, f"mark-{quote(mark_id, safe='')}")
|
||||
@@ -785,8 +822,9 @@ def create_app(
|
||||
target = raw_target if isinstance(raw_target, str) and raw_target else None
|
||||
text = form.get("text")
|
||||
try:
|
||||
mark = write_note(booth, target, text if isinstance(text, str) else "",
|
||||
who=request.client.host if request.client else "")
|
||||
mark = await run_in_threadpool(
|
||||
write_note, booth, target, text if isinstance(text, str) else "",
|
||||
who=request.client.host if request.client else "")
|
||||
except AskError as exc:
|
||||
raise HTTPException(status_code=400, detail=str(exc))
|
||||
return _mark_redirect(name, form, f"mark-{quote(mark.id, safe='')}")
|
||||
@@ -806,7 +844,9 @@ def create_app(
|
||||
raise HTTPException(status_code=400, detail="a flag needs a target")
|
||||
on = str(form.get("on", "1")) not in ("0", "", "false", "off")
|
||||
try:
|
||||
set_flag(booth, target, on, who=request.client.host if request.client else "")
|
||||
await run_in_threadpool(
|
||||
set_flag, booth, target, on,
|
||||
who=request.client.host if request.client else "")
|
||||
except AskError as exc:
|
||||
raise HTTPException(status_code=400, detail=str(exc))
|
||||
return _mark_redirect(name, form, f"item-{quote(target, safe='')}")
|
||||
@@ -820,7 +860,7 @@ def create_app(
|
||||
mark_id = form.get("mark")
|
||||
if not isinstance(mark_id, str) or not mark_id:
|
||||
raise HTTPException(status_code=400, detail="which mark?")
|
||||
delete_mark(booth, mark_id)
|
||||
await run_in_threadpool(delete_mark, booth, mark_id)
|
||||
return _mark_redirect(name, form, "marks")
|
||||
|
||||
@app.post("/b/{name}/import-asks")
|
||||
@@ -832,7 +872,7 @@ def create_app(
|
||||
migrated from the page you are already looking at.
|
||||
"""
|
||||
booth = resolve_booth(name)
|
||||
import_legacy_asks(booth)
|
||||
await run_in_threadpool(import_legacy_asks, booth)
|
||||
form = await request.form()
|
||||
return _mark_redirect(name, form, "marks")
|
||||
|
||||
|
||||
+95
-11
@@ -218,6 +218,24 @@ def _read_raw_strict(booth: Path) -> list[dict]:
|
||||
return entries
|
||||
|
||||
|
||||
def read_error(booth: Path) -> str | None:
|
||||
"""Why this booth's marks cannot be read, or None if they can.
|
||||
|
||||
`marks_for` is lenient on purpose — a review page that will not load is
|
||||
worse than one missing an annotation — and that leniency turns an
|
||||
unreadable file into "no marks". For a BROWSER that is the right trade. For
|
||||
the CLI it is not: a session that asked a question and is told "no such
|
||||
pick" will conclude the question was never posted, when in fact the file
|
||||
holding it is damaged. A machine consumer can act on the difference, so it
|
||||
gets to ask.
|
||||
"""
|
||||
try:
|
||||
_read_raw_strict(booth)
|
||||
except MarksCorrupt as exc:
|
||||
return str(exc)
|
||||
return None
|
||||
|
||||
|
||||
def _write_raw(booth: Path, entries: list[dict]) -> None:
|
||||
"""Atomic replace, so a reader never sees a half-written document and a
|
||||
crash mid-write cannot truncate the file into a shorter — and therefore
|
||||
@@ -249,12 +267,20 @@ class _Locked:
|
||||
self.booth.mkdir(parents=True, exist_ok=True)
|
||||
lock = self.booth / MARKS_LOCK
|
||||
# `touch(exist_ok=True)` on an EXISTING file bumps its mtime, and a
|
||||
# booth's TTL is measured from its newest mtime including dotfiles — so
|
||||
# an unconditional touch would keep a booth alive just for being read
|
||||
# through a write path. Create it only when it is not there.
|
||||
# booth's TTL is measured from its newest mtime — so an unconditional
|
||||
# touch would keep a booth alive just for being read through a write
|
||||
# path. Create it only when it is not there.
|
||||
#
|
||||
# ONCE CREATED, THE LOCK FILE IS NEVER REMOVED (see __exit__).
|
||||
if not lock.exists():
|
||||
# Creating a directory entry bumps the DIRECTORY's mtime, which is
|
||||
# what `_newest_mtime` seeds from — so making our own lock file
|
||||
# would itself read as activity. Put the clock back: the lock is
|
||||
# machinery, and machinery is not the operator touching the booth.
|
||||
before = self.booth.stat()
|
||||
lock.touch()
|
||||
self._made_lock = True
|
||||
os.utime(self.booth, (before.st_atime, before.st_mtime))
|
||||
self._lf = lock.open("r+")
|
||||
fcntl.flock(self._lf, fcntl.LOCK_EX)
|
||||
try:
|
||||
@@ -264,8 +290,6 @@ class _Locked:
|
||||
fcntl.flock(self._lf, fcntl.LOCK_UN)
|
||||
self._lf.close()
|
||||
self._lf = None
|
||||
if self._made_lock:
|
||||
lock.unlink(missing_ok=True)
|
||||
raise
|
||||
self._before = _fingerprint(self.entries)
|
||||
return self
|
||||
@@ -284,10 +308,17 @@ class _Locked:
|
||||
# would otherwise keep a dead booth alive forever.
|
||||
if exc_type is None and _fingerprint(self.entries) != self._before:
|
||||
_write_raw(self.booth, self.entries)
|
||||
elif self._made_lock and not (self.booth / MARKS_FILE).exists():
|
||||
# Nothing was written and this booth had no marks before: do not
|
||||
# leave a lock file behind as the only trace of a no-op.
|
||||
(self.booth / MARKS_LOCK).unlink(missing_ok=True)
|
||||
# THE LOCK FILE IS NEVER UNLINKED. It used to be, on the no-op path,
|
||||
# so a booth that had never been marked was left exactly as it was
|
||||
# found. That tidiness cost mutual exclusion outright: `flock` binds
|
||||
# to an INODE, so unlinking the lock while a second writer is blocked
|
||||
# on it leaves that writer holding an exclusive lock on a deleted
|
||||
# file, and the NEXT writer creates a fresh lock and takes it at
|
||||
# once. Two processes then run the read-modify-write concurrently,
|
||||
# the later `os.replace` drops the earlier one's mark, and both of
|
||||
# them obeyed the protocol. A zero-byte dotfile is the cheaper
|
||||
# thing to leave behind — `booth_items` skips it, the zip skips it,
|
||||
# and `_newest_mtime` exempts it so it cannot hold a booth open.
|
||||
finally:
|
||||
fcntl.flock(lf, fcntl.LOCK_UN)
|
||||
lf.close()
|
||||
@@ -301,6 +332,25 @@ class _Locked:
|
||||
# ---- read -------------------------------------------------------------------
|
||||
|
||||
|
||||
def _entry_type_error(entry: dict) -> str | None:
|
||||
"""The stored scalars this module refuses to guess at.
|
||||
|
||||
`_clean_text` did `(text or "").replace(...)` and `marks_for` sorts on
|
||||
`(created, id)` — so a stored `text` that is a dict, or a `created` that is a
|
||||
number, raised AttributeError or TypeError out of the READ path. That is not
|
||||
a marks bug, it is an INDEX bug: `list_booths` reads every booth's marks on
|
||||
every page load and `/healthz` does the same, so one hand-edited or
|
||||
foreign-written file took down the front page for every booth on the
|
||||
service. A wrong type is a broken mark, and this module already knows how to
|
||||
render one of those.
|
||||
"""
|
||||
for name in ("created", "by", "text", "error"):
|
||||
value = entry.get(name)
|
||||
if value is not None and not isinstance(value, str):
|
||||
return f"{name} is {type(value).__name__}, not a string"
|
||||
return None
|
||||
|
||||
|
||||
def _hydrate(entry: dict) -> Mark:
|
||||
"""One stored entry -> one Mark, declarations normalized.
|
||||
|
||||
@@ -312,6 +362,15 @@ def _hydrate(entry: dict) -> Mark:
|
||||
"""
|
||||
mid = entry["id"]
|
||||
shape = entry.get("shape") if entry.get("shape") in SHAPES else NOTE
|
||||
bad = _entry_type_error(entry)
|
||||
if bad is not None:
|
||||
# `created` is dropped rather than coerced, which sorts the entry to the
|
||||
# TOP of the booth's marks: a mark nobody can read is the one that wants
|
||||
# looking at, and burying it under 270 items' worth of notes is how it
|
||||
# stays unnoticed. Deterministic, and stated — `("", id)` against
|
||||
# `(created, id)`.
|
||||
return Mark(id=mid, shape=shape, target=None, created="",
|
||||
error=f"unreadable mark: {bad}")
|
||||
target = entry.get("target")
|
||||
if not _valid_target(target):
|
||||
target = None
|
||||
@@ -359,11 +418,26 @@ def _hydrate(entry: dict) -> Mark:
|
||||
return Mark(**base, text=_clean_text(entry.get("text")))
|
||||
|
||||
|
||||
def _hydrate_safe(entry: dict) -> Mark:
|
||||
"""`_hydrate`, with the promise that it cannot raise.
|
||||
|
||||
`_entry_type_error` covers the shapes we know how to name; this is the
|
||||
backstop for the ones we do not, and it exists because of WHERE this runs.
|
||||
One unreadable mark must cost that mark, never the page — and on the index
|
||||
it is not even that booth's page, it is all of them.
|
||||
"""
|
||||
try:
|
||||
return _hydrate(entry)
|
||||
except Exception as exc: # noqa: BLE001 - deliberate
|
||||
return Mark(id=str(entry.get("id", "")), shape=NOTE, target=None,
|
||||
created="", error=f"unreadable mark: {exc}")
|
||||
|
||||
|
||||
def marks_for(booth: Path) -> list[Mark]:
|
||||
"""Every mark in a booth, oldest first, declarations normalized and answers
|
||||
folded in. ONE file read — which is the whole point of the storage shape."""
|
||||
entries = _read_raw(booth)
|
||||
marks = [_hydrate(e) for e in entries]
|
||||
marks = [_hydrate_safe(e) for e in entries]
|
||||
# (created, id) rather than created alone: two marks written in the same
|
||||
# second would otherwise order by however json listed them.
|
||||
marks.sort(key=lambda m: (m.created, m.id))
|
||||
@@ -642,7 +716,17 @@ def import_legacy_asks(booth: Path) -> list[Mark]:
|
||||
"id": stem,
|
||||
"shape": PICK,
|
||||
"target": None,
|
||||
"created": datetime.fromtimestamp(mtime).astimezone().isoformat(timespec="seconds"),
|
||||
# MICROSECONDS, not seconds. `found` is ordered by fractional
|
||||
# mtime and `marks_for` re-sorts on this string, so truncating
|
||||
# to the whole second threw away the only thing distinguishing
|
||||
# two sidecars written in the same second — and the `(created,
|
||||
# id)` tie-break then silently re-sorted them alphabetically,
|
||||
# reversing the order the importer had just established. The
|
||||
# ROADMAP states this import's order is `(mtime, name)`; an
|
||||
# order that is stated and not kept is worse than one never
|
||||
# claimed.
|
||||
"created": datetime.fromtimestamp(mtime).astimezone().isoformat(
|
||||
timespec="microseconds"),
|
||||
"declaration": decl,
|
||||
"answer": answer,
|
||||
}
|
||||
|
||||
@@ -13,11 +13,35 @@
|
||||
Works with JS off — plain form POST, every shape. An answered pick shows the
|
||||
recorded judgment and a collapsed "change" form, because the mark is the
|
||||
CURRENT judgment and not a log. #}
|
||||
{# A mark carrying `error` is sorted out FIRST, whatever shape it claims. A
|
||||
pick keeps its own ⚠ broken rendering below (richer — it has a declaration to
|
||||
show); a broken note would otherwise render as an empty <pre> with a withdraw
|
||||
button, indistinguishable from a note the operator wrote and then cleared,
|
||||
and a broken flag would link to a target that is not there. Unreadable state
|
||||
is visible state — the rule `_hydrate` states for picks, applied to all
|
||||
three. #}
|
||||
{% set broken = marks | selectattr('error') | rejectattr('shape', 'equalto', 'pick') | list %}
|
||||
{% set picks = marks | selectattr('shape', 'equalto', 'pick') | list %}
|
||||
{% set notes = marks | selectattr('shape', 'equalto', 'note') | list %}
|
||||
{% set flags = marks | selectattr('shape', 'equalto', 'flag') | list %}
|
||||
{% set notes = marks | selectattr('shape', 'equalto', 'note') | rejectattr('error') | list %}
|
||||
{% set flags = marks | selectattr('shape', 'equalto', 'flag') | rejectattr('error') | list %}
|
||||
<section class="marks">
|
||||
|
||||
{% for a in broken %}
|
||||
<article class="mark mark-note is-broken" id="mark-{{ a.id }}">
|
||||
<header class="mark-head">
|
||||
<span class="mark-state">⚠ broken</span>
|
||||
<span class="mark-id"><code>{{ a.id }}</code></span>
|
||||
<span class="board-spacer"></span>
|
||||
<form class="mark-undo" method="post" action="/b/{{ name_url }}/unmark">
|
||||
<input type="hidden" name="mark" value="{{ a.id }}">
|
||||
{% if marks_page %}<input type="hidden" name="back" value="marks">{% endif %}
|
||||
<button type="submit" class="mark-x" title="withdraw this mark">×</button>
|
||||
</form>
|
||||
</header>
|
||||
<p class="mark-error">This mark could not be read: {{ a.error }}</p>
|
||||
</article>
|
||||
{% endfor %}
|
||||
|
||||
{% for a in picks %}
|
||||
<article class="mark mark-pick{% if a.answer and a.answer.complete %} is-answered{% elif a.answer %} is-partial{% elif a.error %} is-broken{% endif %}" id="mark-{{ a.id }}">
|
||||
<header class="mark-head">
|
||||
|
||||
@@ -94,7 +94,11 @@
|
||||
back to the flagged items. Always rendered on a gallery booth — the add-note
|
||||
field is a control, not a result, so it has to be there before the first
|
||||
mark exists. #}
|
||||
{% if not board %}
|
||||
{# `marks or not board`: the standing link board renders as a board rather than
|
||||
a gallery, and the add-note control would be noise on it — but the
|
||||
suppression was unconditional, so a pick declared on a booth that happens to
|
||||
carry a links.md had no form to answer it and nothing said so. #}
|
||||
{% if marks or not board %}
|
||||
{% include "_marks.html" %}
|
||||
{% endif %}
|
||||
|
||||
@@ -187,6 +191,12 @@
|
||||
{% else %}
|
||||
<pre class="textview doc-body">{{ it.rendered }}</pre>
|
||||
{% endif %}
|
||||
{# The doc branch had `markcontrols` and not `marknotes`, so the
|
||||
operator could point at a report and not write down why — on the
|
||||
one item kind whose whole content is prose. Exactly the
|
||||
"patched two of three" failure the blurtoggle macro above was
|
||||
written to prevent, recurring on the macro written to prevent it. #}
|
||||
{{ marknotes(name_url, it, item_marks.get(it.name, [])) }}
|
||||
</details>
|
||||
</figure>
|
||||
{% else %}
|
||||
|
||||
@@ -33,8 +33,18 @@
|
||||
white-space:pre-wrap}
|
||||
</style>
|
||||
<script>
|
||||
document.addEventListener('keydown', function (e) {
|
||||
if (e.key === 'Escape') window.location.href = {{ ('/b/' ~ name_url ~ '/')|tojson }};
|
||||
});
|
||||
(function () {
|
||||
/* Escape leaves the page, so it must not fire from inside a field someone
|
||||
is typing in — the same guard the image viewer carries, stated in both
|
||||
places because the handler is on `document` in both. */
|
||||
function isEditable(el) {
|
||||
return !!(el && (el.isContentEditable ||
|
||||
/^(input|textarea|select)$/i.test(el.tagName || '')));
|
||||
}
|
||||
document.addEventListener('keydown', function (e) {
|
||||
if (isEditable(e.target)) return;
|
||||
if (e.key === 'Escape') window.location.href = {{ ('/b/' ~ name_url ~ '/')|tojson }};
|
||||
});
|
||||
})();
|
||||
</script>
|
||||
{% endblock %}
|
||||
|
||||
@@ -99,7 +99,17 @@
|
||||
img.addEventListener('load', evaluate);
|
||||
window.addEventListener('resize', evaluate);
|
||||
if (img.complete) evaluate();
|
||||
|
||||
/* An arrow key inside the note field is a CARET move, not a navigation.
|
||||
The handler is on `document` and the note textarea shipped into this same
|
||||
page, so typing a note and reaching for ← threw the draft away; Escape
|
||||
did it in one keystroke. Anything editable keeps its own keys. */
|
||||
function isEditable(el) {
|
||||
return !!(el && (el.isContentEditable ||
|
||||
/^(input|textarea|select)$/i.test(el.tagName || '')));
|
||||
}
|
||||
document.addEventListener('keydown', function (e) {
|
||||
if (isEditable(e.target)) return;
|
||||
if (e.key === 'Escape') window.location.href = BACK;
|
||||
else if (e.key === 'ArrowLeft' && PREV) window.location.href = PREV;
|
||||
else if (e.key === 'ArrowRight' && NEXT) window.location.href = NEXT;
|
||||
|
||||
Reference in New Issue
Block a user