The diff-scoped bug-hunt panel, four arms, artifact-only. Its strongest finding is one I created two hours earlier while hardening the reader. `stat` reports size 0 for a FIFO and 0 for a symlink to /dev/zero, so both sail under the byte cap added for the RecursionError round — and then `read_text` either blocks in read() with no EOF, so the except never runs, or allocates until the kernel intervenes. `list_booths` reads every booth on every GET / and /healthz, so ONE such file stalls the front page for the whole service, with no error and no recovery short of a restart. Reproduced before believing it (timeout returned 124). S_ISREG is checked BEFORE the size in both modules now; verified against the live service with two FIFOs planted, which answered 200 in 36ms. The shape worth carrying: st_size answers a different question than "can this be read", and a bound that trusts it inherits everything it does not mean. A hardening fix opened a worse hole than the one it closed. THE UPLOAD PATH WROTE ABOVE ITS OWN CLEANUP GUARD (4/4) A failed manifest write orphaned a .uploaded half-booth with no files in it — and because the temp name now carries a random suffix, nothing ever overwrote the leak, and .booth.json.<hex>.tmp is not a .lock, so _newest_mtime counted it and kept that empty booth past every sweep. The uniqueness fix from the previous round is what made the leak permanent. Both writes moved inside the guard; the temp is removed on every exit path. DAMAGED BYTES ARE KEPT, NOT REPLACED (4/4, INV-6) Marks made this explicit in v0.2.1 and this write path contradicted it: a manifest that failed on ONE field lost the others with it, including a why the re-announcer may never have kept anywhere. It diverges from marks in HOW it honours the rule — marks refuse and answer 409 because the operator's judgment is not restatable; a manifest quarantines and proceeds, because refusing would fail `booth add` and lose the files it was copying. ONE OPENNESS PREDICATE, AS U2 SAID (2/4) `booth answer` spelled out `if m.answer is None` while `booth marks` asked `open_marks`, so a partially-answered pick read as done to one verb and open to the other — at the same instant, on the same booth. U2's INV-2 put openness in one function precisely so they could not drift. The mirror case is fixed too: a pick that hydrates broken is refused by the web route, so `answer --wait` polled an hour on a form nothing could ever land. ALSO - now_stamp was whole-second while the importer had moved to microseconds, and '-' sorts before '.', so a later mark came out ahead of an earlier import inside the same second. One format; the previous round's ordering fix had opened this one. - `_broken` was the third of three directory-name fallbacks and the one still handing a raw name into a card's sub-line. - An identical re-announce rewrote the file and reset the TTL. `booth link` does this on every post to the standing board. - The importer's return went through the bare _hydrate, not _hydrate_safe. - A marks document could be written larger than it can be read back, and then read as no marks at all. Refused at the write instead. - `choice` reached the answer builder raw while `notes` beside it did not. AND ONE FINDING DELIBERATELY NOT FULLY CLOSED The mtime-restore race is real. The clean fix — ignore a booth directory's own mtime whenever the booth holds anything — also silently retires the documented rule that releasing a kept board resets its clock, which the CLI header, the README and a deliberately-written test all pin. That is a TTL doctrine change, not a bug fix, and an existing test caught the attempt. The concrete half is fixed (a failing os.utime escaped and 500'd the route); the race is stated in the code where the next reader will meet it. 341 tests. Live service restarted, 24/24 booth pages verified.
275 lines
12 KiB
Python
275 lines
12 KiB
Python
"""A booth's own announcement — who posted it, and why.
|
|
|
|
U5. The index card used to show a name, an item count and a countdown, and
|
|
nothing the poster chose. An agent with something to show therefore had no way
|
|
to make the booth say "look at this" and posted a URL to the link board
|
|
instead — which is why 145 of that board's 210 rows (69%) ended up pointing at
|
|
booths that had already been swept. The board was absorbing a job it was never
|
|
shaped for. This is the shape.
|
|
|
|
.booth.json -> {"handle": ..., "title": ..., "why": ..., "created": ...}
|
|
|
|
⚠ STDLIB ONLY, and it imports nothing from `booth.*` either.
|
|
|
|
`scripts/booth` — the CLI every fleet session uses — imports this module
|
|
directly under the system `python3` with no venv, through a `python3 -c`
|
|
heredoc no AST extractor can see. A single third-party import here breaks
|
|
`booth new` and `booth add` on every host, and the failure surfaces in an
|
|
agent's session rather than in ours. The ban extends to sibling `booth` modules:
|
|
importing `marks` to reuse its atomic write would drag marks' own import list
|
|
into this one's, so the four-line pattern is copied instead. `test_stdlib_only`
|
|
in tests/test_manifest.py is the only thing standing here.
|
|
|
|
Contract: docs/contracts/u5_booth_manifest.contract.md.
|
|
"""
|
|
from __future__ import annotations
|
|
|
|
import json
|
|
import os
|
|
import secrets
|
|
import stat as statmod
|
|
from dataclasses import dataclass
|
|
from datetime import datetime
|
|
from pathlib import Path
|
|
|
|
MANIFEST_FILE = ".booth.json"
|
|
|
|
# A `why` renders inside a card's sub-line, so it is one line by construction
|
|
# rather than by convention — enforced at the WRITE so nothing downstream has to
|
|
# remember. The caps are display budgets, not storage limits.
|
|
HANDLE_MAX = 64
|
|
TITLE_MAX = 120
|
|
WHY_MAX = 200
|
|
CREATED_MAX = 64
|
|
|
|
# A manifest is four short fields. Anything near this is not one, and reading it
|
|
# into memory to find that out is the wrong order of operations: `list_booths`
|
|
# calls the reader once per booth on every index load, so an unbounded read is
|
|
# the service-wide outage the lenient reader exists to prevent, arriving in a
|
|
# different costume. Checked by `stat`, before the bytes are touched.
|
|
MANIFEST_MAX_BYTES = 64 * 1024
|
|
|
|
# Where bytes that could not be read go when a re-announcement replaces them.
|
|
# ONE fixed name, deliberately: a timestamped quarantine accumulates forever in
|
|
# a folder nothing prunes, and the most recent damage is the only copy anybody
|
|
# would look at. A dotfile, so it is invisible to every listing and zip.
|
|
QUARANTINE_FILE = ".booth.json.broken"
|
|
|
|
# The handle a booth created by the service itself carries. A pickup booth and
|
|
# the standing link board are made by the Booth, not by an agent, and saying so
|
|
# is true rather than manufactured — which is the whole reason there is no
|
|
# exemption list. One rule: a booth with no manifest is unannounced.
|
|
SERVICE_HANDLE = "booth"
|
|
|
|
|
|
@dataclass(frozen=True)
|
|
class Manifest:
|
|
"""One booth's announcement.
|
|
|
|
`handle` is an althing agent handle, or `SERVICE_HANDLE` for a booth the
|
|
Booth made. `error` is a read-time verdict and is never stored.
|
|
"""
|
|
|
|
handle: str
|
|
title: str
|
|
why: str
|
|
created: str
|
|
error: str | None = None
|
|
|
|
|
|
def _one_line(value, limit: int) -> str:
|
|
"""One line, bounded. Collapses ALL runs of whitespace, not only newlines —
|
|
a tab or a forty-space indent in a `why` renders as badly inside a card's
|
|
sub-line as a newline does, and the field is one line by construction."""
|
|
if not isinstance(value, str):
|
|
return ""
|
|
return " ".join(value.split())[:limit]
|
|
|
|
|
|
def _temp_path(booth: Path) -> Path:
|
|
"""A scratch name no other writer will pick.
|
|
|
|
Every writer used to derive the same `.booth.json.tmp`, so two `booth add`
|
|
calls on one booth could interleave through a stale descriptor into the
|
|
published path. Marks are protected from that by their flock; the manifest
|
|
deliberately has none — it is written once at creation, not read-modify-
|
|
written per click — so uniqueness is what stands in for the lock. Still a
|
|
dotfile, so no listing, gallery or zip can see it mid-write.
|
|
"""
|
|
return booth / f"{MANIFEST_FILE}.{secrets.token_hex(4)}.tmp"
|
|
|
|
|
|
def _as_doc(m: "Manifest") -> dict:
|
|
"""The stored shape of a record, for the no-op comparison."""
|
|
return {"handle": m.handle, "title": m.title, "why": m.why, "created": m.created}
|
|
|
|
|
|
def _now() -> str:
|
|
return datetime.now().astimezone().isoformat(timespec="seconds")
|
|
|
|
|
|
def read_manifest(booth: Path) -> Manifest | None:
|
|
"""This booth's announcement, or None if it never made one.
|
|
|
|
LENIENT, AND IT NEVER RAISES (INV-2). `list_booths` calls this once per
|
|
booth on every index page load, so a read that can raise is a service-wide
|
|
outage wearing a single-booth bug's clothes. That is not hypothetical: a
|
|
poisoned `.marks.json` did exactly that to `/` and `/healthz` across all 25
|
|
live booths, and the fix shipped in v0.2.2. Same posture, applied before the
|
|
same mistake rather than after it.
|
|
|
|
Absent -> None. Present but unreadable -> a Manifest carrying `error`, so a
|
|
card can say `unreadable` instead of quietly showing the same thing as a
|
|
booth that never announced (INV-5). Folding the two together would hide the
|
|
one case somebody has to go and fix.
|
|
|
|
Only `handle` is required. A hand-written manifest is a supported input —
|
|
the file is plain JSON in a folder the operator owns, and half the point of
|
|
the Booth is that a booth is just a directory.
|
|
"""
|
|
booth = Path(booth)
|
|
path = booth / MANIFEST_FILE
|
|
# BOUNDED BEFORE THE READ. "Never raises" was not true of an unbounded one:
|
|
# a 4 GB file raises MemoryError and a deeply nested document raises
|
|
# RecursionError out of `json.loads`, and neither is an OSError or a
|
|
# ValueError. Both escape into `list_booths`, which calls this per booth on
|
|
# every index load — so one file returns 500 for the whole front page. Size
|
|
# first, by `stat`; then catch the two classes anyway, because a bound that
|
|
# is one day raised should not quietly re-open the hole.
|
|
try:
|
|
st = path.stat()
|
|
except FileNotFoundError:
|
|
return None
|
|
except OSError as exc:
|
|
return _broken(booth, f"cannot be read: {exc}")
|
|
# ⚠ REGULAR-FILE FIRST, then size. `st_size` answers a different question
|
|
# than "can this be read": it is 0 for a FIFO and 0 for /dev/zero, so both
|
|
# sail under the cap, and then `read_text` either blocks forever with no EOF
|
|
# or allocates until the kernel intervenes. The bound ABOVE is what made
|
|
# this reachable — a cap that trusts st_size inherits everything st_size
|
|
# does not mean. One such file stalls every `GET /` and `/healthz`.
|
|
if not statmod.S_ISREG(st.st_mode):
|
|
return _broken(booth, "is not a regular file")
|
|
if st.st_size > MANIFEST_MAX_BYTES:
|
|
return _broken(booth, f"is too large to be a manifest ({st.st_size} bytes)")
|
|
try:
|
|
text = path.read_text(encoding="utf-8")
|
|
except FileNotFoundError:
|
|
return None
|
|
except (OSError, UnicodeDecodeError, MemoryError) as exc:
|
|
return _broken(booth, f"cannot be read: {exc}")
|
|
if not text.strip():
|
|
return _broken(booth, "is empty")
|
|
try:
|
|
raw = json.loads(text)
|
|
except (ValueError, RecursionError, MemoryError) as exc:
|
|
return _broken(booth, f"is not valid JSON: {type(exc).__name__}")
|
|
if not isinstance(raw, dict):
|
|
return _broken(booth, "is not a JSON object")
|
|
|
|
handle = _one_line(raw.get("handle"), HANDLE_MAX)
|
|
if not handle:
|
|
return _broken(booth, "names no handle")
|
|
return Manifest(
|
|
handle=handle,
|
|
# `or booth.name` goes THROUGH the normalizer too. A directory name may
|
|
# legally carry a newline on POSIX and may run to 255 bytes, and the
|
|
# fallback used to hand either straight into a card's sub-line.
|
|
title=_one_line(raw.get("title"), TITLE_MAX) or _one_line(booth.name, TITLE_MAX),
|
|
why=_one_line(raw.get("why"), WHY_MAX),
|
|
created=_one_line(raw.get("created"), CREATED_MAX),
|
|
)
|
|
|
|
|
|
def _broken(booth: Path, reason: str) -> Manifest:
|
|
# The directory name goes through the normalizer here too. This was the
|
|
# THIRD fallback of three; the write path's and the read path's were fixed a
|
|
# round earlier and this one was missed, with the same consequence — a
|
|
# newline or 255 bytes of directory name straight into a card's sub-line.
|
|
return Manifest(handle="", title=_one_line(booth.name, TITLE_MAX), why="",
|
|
created="", error=f"{MANIFEST_FILE} {reason}")
|
|
|
|
|
|
def write_manifest(booth: Path, handle: str, *, title: str | None = None,
|
|
why: str | None = None) -> Manifest:
|
|
"""Announce a booth, atomically (CLAUDE.md invariant 5).
|
|
|
|
Temp file + `os.replace`, because the CLI writes this in one process while
|
|
the browser reads it in another — a reader must never see a half-written
|
|
document. The temp file is itself a dotfile, so no listing, gallery or zip
|
|
can see it mid-write either.
|
|
|
|
OMITTED MEANS UNCHANGED; `""` MEANS CLEAR. `title` and `why` default to
|
|
None, not to the empty string, because the ordinary sequence is
|
|
`booth new x --why "..."` and then `booth add x out/*.png` — and while
|
|
omission meant empty, that second command silently erased the sentence the
|
|
first one existed to record. Two arms of the contract panel predicted it
|
|
from the wording alone; every test written for this module passed `--why`
|
|
on both calls and so could not see it.
|
|
|
|
RE-ANNOUNCING PRESERVES `created` (INV-3). It is when the booth APPEARED,
|
|
and saying something more about it later is not a second appearance. A
|
|
`created` that cannot be read back is replaced rather than guessed at: a
|
|
stamp that is silently wrong is worse than one that is silently new.
|
|
|
|
An empty `handle` becomes `SERVICE_HANDLE` rather than being refused — a
|
|
manifest with no handle does not read back at all, and an unreadable file is
|
|
the worse outcome. Unreachable from the CLI, whose fallback chain always
|
|
yields something; callers of this function directly should pass a real one.
|
|
"""
|
|
booth = Path(booth)
|
|
booth.mkdir(parents=True, exist_ok=True)
|
|
prior = read_manifest(booth)
|
|
usable = prior if prior and not prior.error else None
|
|
created = usable.created if usable and usable.created else _now()
|
|
|
|
record = Manifest(
|
|
handle=_one_line(handle, HANDLE_MAX) or SERVICE_HANDLE,
|
|
title=(_one_line(title, TITLE_MAX) if title is not None
|
|
else (usable.title if usable else "")) or _one_line(booth.name, TITLE_MAX),
|
|
why=(_one_line(why, WHY_MAX) if why is not None
|
|
else (usable.why if usable else "")),
|
|
created=created,
|
|
)
|
|
path = booth / MANIFEST_FILE
|
|
doc = {"handle": record.handle, "title": record.title,
|
|
"why": record.why, "created": record.created}
|
|
|
|
# A write that changes nothing is not activity and must not reset the
|
|
# booth's TTL — the rule marks learned in v0.2.0, applied here because
|
|
# `booth link` re-announces the standing board on EVERY post to it.
|
|
if prior is not None and not prior.error and _as_doc(prior) == doc:
|
|
return record
|
|
|
|
# NOTHING THAT COULD NOT BE READ IS DESTROYED. Reads stay lenient, writes
|
|
# go strict, damaged bytes stay on disk — the doctrine marks made explicit
|
|
# in v0.2.1, which this write path contradicted by replacing them outright.
|
|
# A file that fails on ONE field still holds the others, and a `why` the
|
|
# re-announcer never kept anywhere is exactly what went missing.
|
|
#
|
|
# QUARANTINED rather than REFUSED, which is where this diverges from marks:
|
|
# refusing would fail `booth add` and lose the files it was mid-way through
|
|
# copying, and a booth's own description is restatable in a way the
|
|
# operator's judgment is not.
|
|
if prior is not None and prior.error:
|
|
try:
|
|
os.replace(path, booth / QUARANTINE_FILE)
|
|
except OSError:
|
|
pass # nothing to preserve beats failing the write
|
|
|
|
tmp = _temp_path(booth)
|
|
try:
|
|
tmp.write_text(
|
|
json.dumps(doc, ensure_ascii=False, indent=2) + "\n",
|
|
encoding="utf-8",
|
|
)
|
|
os.replace(tmp, path)
|
|
except BaseException:
|
|
# A leaked temp is worse here than it would be with a fixed name: the
|
|
# unique suffix means nothing ever overwrites it, and it is not a
|
|
# `.lock`, so `_newest_mtime` counts it and it keeps a dead booth alive
|
|
# forever. Cleaning up is the price of the uniqueness.
|
|
tmp.unlink(missing_ok=True)
|
|
raise
|
|
return record
|