diff --git a/ROADMAP.md b/ROADMAP.md index f4a4589..5670516 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -25,15 +25,24 @@ surface) and therefore the safest thing to land first or in parallel. **U1, U2 and U5 are landed.** U3 and U4 are unblocked and unstarted; U6 remains independent and unstarted; U7 waits on the rest. -**U5's adoption is a measured prediction, not a finished result.** The operator -declined a fleetwide announcement so that adoption could be told apart from -design: the convention propagates through the README alone, and the count of -booths carrying a `.booth.json` gets re-measured on **2026-09-29** against a -baseline of **0 of 26** at landing. A near-zero count means nobody heard about -it — an adoption failure, fixed by announcing — which is a different thing from -nobody wanting it. Same instrument as U4's `.forever` prediction below. +**U5's adoption is a measured prediction, not a finished result**, and it is +TWO predictions rather than one. The operator declined a fleetwide announcement +so that adoption could be told apart from design; within fifty minutes of the +deploy a peer that had been told nothing (`comfy-dev`) created a booth and it +announced itself with a handle and an empty `why`. That is the split: - find ~/booth-data -maxdepth 2 -name .booth.json | wc -l +- **The handle rides for free.** It is written by `booth new` and `booth add`, + so every existing caller starts announcing without learning anything. +- **The `why` has to be learned.** It needs someone to know the flag exists. + +Both get re-measured on **2026-09-29**: + + find ~/booth-data -maxdepth 2 -name .booth.json | wc -l # free + grep -l '"why": "[^"]' ~/booth-data/*/.booth.json | wc -l # learned + +A high first count with a near-zero second is the predicted shape of "nobody was +told" — an adoption failure fixed by announcing, which is a different thing from +nobody wanting it. Same instrument as U4's `.forever` prediction below. ### Cross-cutting invariant — deterministic order, everywhere diff --git a/booth/app.py b/booth/app.py index ffe3806..1d44e2c 100644 --- a/booth/app.py +++ b/booth/app.py @@ -812,14 +812,19 @@ def create_app( notes = _form_text(form, "notes") try: if spec.multi: - choice = {q["key"]: form.get(f"choice.{q['key']}") for q in spec.questions} + choice = {q["key"]: _form_text(form, f"choice.{q['key']}") + for q in spec.questions} 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: + # `choice` through the same reader as `notes`. It was raw, so a + # multipart FILE part named `choice` reached the answer builder + # as an UploadFile — the asymmetry that had already been fixed + # once on the field beside it. await run_in_threadpool(answer_pick, booth, mark_id, - form.get("choice"), notes, who=who) + _form_text(form, "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='')}") @@ -1101,12 +1106,6 @@ def create_app( booth_id = generate_pickup_id(lambda n: (data_dir / n).exists()) dest = data_dir / booth_id dest.mkdir(parents=True) - (dest / UPLOAD_MARKER).write_text("") # stamp as an upload (dotfile, not listed) - # A booth the SERVICE made says so, rather than being exempted from the - # unannounced marker. One rule instead of an exemption list, and the - # handle is true: nobody's agent posted this, the browser did. - write_manifest(dest, SERVICE_HANDLE, title=booth_id, - why="browser upload, for pickup") total = 0 # Both markers are belt-and-braces: `safe_upload_name` strips leading @@ -1114,6 +1113,15 @@ def create_app( # anyway so the set says what the directory already contains. used: set = {UPLOAD_MARKER, MANIFEST_FILE} try: + (dest / UPLOAD_MARKER).write_text("") # dotfile, not listed + # A booth the SERVICE made says so, rather than being exempted from + # the unannounced marker. INSIDE the guard, with the marker: both + # sat above it, so a failure here left a half-booth on disk with no + # files in it — and the manifest's unique temp name meant a leaked + # `.booth.json..tmp` was never overwritten, was not a `.lock`, + # and so kept that empty booth alive past every sweep. Found 4/4. + write_manifest(dest, SERVICE_HANDLE, title=booth_id, + why="browser upload, for pickup") for i, f in enumerate(files): name = _dedupe_name(safe_upload_name(f.filename, f"file-{i + 1}"), used) used.add(name) diff --git a/booth/manifest.py b/booth/manifest.py index dd3349a..bf0554e 100644 --- a/booth/manifest.py +++ b/booth/manifest.py @@ -27,6 +27,7 @@ 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 @@ -48,6 +49,12 @@ CREATED_MAX = 64 # 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 @@ -92,6 +99,11 @@ def _temp_path(booth: Path) -> Path: 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") @@ -125,13 +137,21 @@ def read_manifest(booth: Path) -> Manifest | None: # 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: - size = path.stat().st_size + st = path.stat() except FileNotFoundError: return None except OSError as exc: return _broken(booth, f"cannot be read: {exc}") - if size > MANIFEST_MAX_BYTES: - return _broken(booth, f"is too large to be a manifest ({size} bytes)") + # ⚠ 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: @@ -162,8 +182,12 @@ def read_manifest(booth: Path) -> Manifest | None: def _broken(booth: Path, reason: str) -> Manifest: - return Manifest(handle="", title=booth.name, why="", created="", - error=f"{MANIFEST_FILE} {reason}") + # 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, @@ -208,14 +232,43 @@ def write_manifest(booth: Path, handle: str, *, title: str | None = None, 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) - tmp.write_text( - json.dumps( - {"handle": record.handle, "title": record.title, - "why": record.why, "created": record.created}, - ensure_ascii=False, indent=2, - ) + "\n", - encoding="utf-8", - ) - os.replace(tmp, path) + 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 diff --git a/booth/marks.py b/booth/marks.py index 4fd926d..282a025 100644 --- a/booth/marks.py +++ b/booth/marks.py @@ -42,6 +42,7 @@ from __future__ import annotations import fcntl import json import os +import stat as statmod from dataclasses import asdict, dataclass, field from datetime import datetime from pathlib import Path @@ -134,7 +135,17 @@ class Mark: def now_stamp() -> str: - return datetime.now().astimezone().isoformat(timespec="seconds") + """ONE stamp format across every writer in this module. + + MICROSECONDS, matching `import_legacy_asks`. They diverged when the + importer was moved to sub-second precision to stop same-second sidecars + re-sorting — and the divergence opened a fresh ordering bug in the other + direction, because `-` (0x2D) sorts before `.` (0x2E): a whole-second stamp + lands ahead of ANY fractional stamp in the same second, so a later mark came + out before an earlier import. Marks sort on `(created, id)`; one format is + what makes that rule statable. + """ + return datetime.now().astimezone().isoformat(timespec="microseconds") def _clean_text(text) -> str: @@ -183,7 +194,12 @@ def _read_raw(booth: Path) -> list[dict]: """ path = Path(booth) / MARKS_FILE try: - if path.stat().st_size > MARKS_MAX_BYTES: + st = path.stat() + # Regular-file first, then size. `st_size` is 0 for a FIFO and 0 for a + # symlink to /dev/zero, so both pass a byte cap and then `read_text` + # either blocks with no EOF or allocates until the kernel intervenes. + # This loop runs over EVERY booth on every index load. + if not statmod.S_ISREG(st.st_mode) or st.st_size > MARKS_MAX_BYTES: return [] raw = json.loads(path.read_text(encoding="utf-8")) except (OSError, ValueError, UnicodeDecodeError, RecursionError, MemoryError): @@ -210,15 +226,18 @@ def _read_raw_strict(booth: Path) -> list[dict]: """ path = Path(booth) / MARKS_FILE try: - size = path.stat().st_size + st = path.stat() except FileNotFoundError: return [] except OSError as exc: raise MarksCorrupt(f"{path} cannot be read: {exc}") from exc # The strict half has to refuse everything the lenient half tolerates, or a # file that reads as "no marks" gets replaced by a write that believed it. - if size > MARKS_MAX_BYTES: - raise MarksCorrupt(f"{path} is too large to be a marks document ({size} bytes)") + if not statmod.S_ISREG(st.st_mode): + raise MarksCorrupt(f"{path} is not a regular file") + if st.st_size > MARKS_MAX_BYTES: + raise MarksCorrupt( + f"{path} is too large to be a marks document ({st.st_size} bytes)") try: text = path.read_text(encoding="utf-8") except FileNotFoundError: @@ -264,8 +283,17 @@ def _write_raw(booth: Path, entries: list[dict]) -> None: quieter — set of marks.""" path = Path(booth) / MARKS_FILE doc = {"version": SCHEMA_VERSION, "marks": entries} + body = json.dumps(doc, ensure_ascii=False, indent=2) + "\n" + # The read bound is on the STORED bytes and `indent=2` grows them, so a + # document that fits in memory can land over the limit on disk and then read + # back as no marks at all. Refuse loudly instead: a write that fails is + # recoverable, a file that silently empties is not. + if len(body.encode("utf-8")) > MARKS_MAX_BYTES: + raise MarksCorrupt( + f"{path} would be larger than this version can read back " + f"({len(body.encode('utf-8'))} bytes)") tmp = path.with_suffix(path.suffix + ".tmp") - tmp.write_text(json.dumps(doc, ensure_ascii=False, indent=2) + "\n", encoding="utf-8") + tmp.write_text(body, encoding="utf-8") os.replace(tmp, path) @@ -296,13 +324,41 @@ class _Locked: # 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. + # what `_newest_mtime` reads. An earlier version put the clock back + # with `os.utime` — which closed the bug and opened a race: the + # restore ran before the flock, so anything landing in the window + # between the stat and the utime had its bump rolled backward. An + # `rsync -a` batch is the case that bites, because it PRESERVES + # source mtimes and so has only the directory's freshness to look + # alive by. It could also raise OSError on a read-only directory + # and take the route down with it. + # + # THE RESTORE STAYS, and the honest reason is that the alternative + # was worse. Ignoring a booth directory's own mtime whenever the + # booth holds anything would close the race outright — and would + # also silently retire the documented behaviour that RELEASING a + # kept board resets its clock, which the CLI header, the README and + # a deliberate test all pin. That is a TTL doctrine change, not a + # bug fix, and it does not belong in one. + # + # ⚠ RESIDUAL RACE, stated rather than papered over: between the stat + # and the utime, another writer's directory-entry change can be + # rolled backward. The case that bites is an `rsync -a` batch, which + # preserves source mtimes and so has only the directory's freshness + # to look alive by. The window is the two syscalls below and the + # booth must also be one being written to at that instant. + # + # The concrete half IS fixed: a failing utime (read-only directory, + # a booth whose owner we are not) used to escape and take the whole + # route down with a 500. Not putting the clock back is a cost this + # module can absorb; not answering the request is not. before = self.booth.stat() lock.touch() self._made_lock = True - os.utime(self.booth, (before.st_atime, before.st_mtime)) + try: + os.utime(self.booth, (before.st_atime, before.st_mtime)) + except OSError: + pass self._lf = lock.open("r+") fcntl.flock(self._lf, fcntl.LOCK_EX) try: @@ -761,4 +817,6 @@ def import_legacy_asks(booth: Path) -> list[Mark]: # Hydrated AFTER the lock so a broken declaration surfaces as `error` here # exactly as it does on a normal read, rather than through a second path. - return [_hydrate(e) for e in created] + # `_hydrate_safe`, not `_hydrate`: this is the one path that reads entries + # it did not write, and it was the one without the guard. + return [_hydrate_safe(e) for e in created] diff --git a/docs/contracts/u5_booth_manifest.contract.md b/docs/contracts/u5_booth_manifest.contract.md index b31c0ba..e1f2412 100644 --- a/docs/contracts/u5_booth_manifest.contract.md +++ b/docs/contracts/u5_booth_manifest.contract.md @@ -88,6 +88,7 @@ HANDLE_MAX, TITLE_MAX, WHY_MAX = 64, 120, 200 MANIFEST_MAX_BYTES = 64 * 1024 +QUARANTINE_FILE = ".booth.json.broken" def read_manifest(booth: Path) -> Manifest | None: @@ -108,6 +109,14 @@ def read_manifest(booth: Path) -> Manifest | None: huge document, `RecursionError` from a deeply nested one — are caught as well, so that raising the bound one day cannot quietly re-open the hole. + REGULAR-FILE FIRST, THEN SIZE — and the order is the whole point. `st_size` + is 0 for a FIFO and 0 for a symlink to `/dev/zero`, so both sail under any + byte cap and then the read either blocks forever with no EOF or allocates + until the kernel intervenes. The bound 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`, with no error and no recovery + short of a restart. + Absent -> None. Present but too large, unreadable, unparseable, not an object, or missing `handle` -> a Manifest carrying `error`, so the card can say `unreadable` rather than quietly showing the same thing as a booth that @@ -133,6 +142,19 @@ def write_manifest(booth: Path, handle: str, *, title: str | None = None, `""`, is treated as having no stamp to preserve and gets `now()`: a stamp that is silently wrong is worse than one that is silently new. + A WRITE THAT CHANGES NOTHING IS NOT ACTIVITY and does not touch the file, + so it cannot reset the booth's TTL — the rule marks learned in v0.2.0, + needed here because `booth link` re-announces the standing board on every + single post to it. + + BYTES THAT COULD NOT BE READ ARE KEPT, not replaced. See INV-6. + + A FAILED WRITE LEAVES NOTHING BEHIND. The temp name carries a random suffix + so two writers cannot share it — which also means nothing ever overwrites an + orphan, and `.booth.json..tmp` is not a `.lock`, so `_newest_mtime` + counts it and a leak would keep a dead booth alive forever. Cleaned up on + every exit path. + `title` falls back to the directory name, THROUGH the same normalizer the explicit value gets — a directory name may legally carry a newline on POSIX and may run to 255 bytes, and the fallback used to hand either straight @@ -360,6 +382,22 @@ standard library and nothing from `booth.*` — a cross-import between two stdlib-only modules is a second way for the repo rule to break. Relative imports count; the AST walk sees them. +**INV-6 — bytes that could not be read are never destroyed.** When +`write_manifest` replaces a manifest whose read returned `error`, the old bytes +move to `QUARANTINE_FILE` first. This is the doctrine marks made explicit in +v0.2.1 — reads lenient, writes strict, damaged bytes stay on disk — and this +unit contradicted it by replacing outright, so a file 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, and the divergence is the +interesting part. Marks REFUSE the write 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 mid-way through +copying — and a booth's own description is something its poster can say again. +One fixed quarantine name rather than a timestamped series: nothing prunes a +booth but the sweep, and the most recent damage is the only copy anyone opens. + **INV-5 — unannounced and unreadable render DIFFERENT TEXT.** Not merely different styling: the words differ (`unannounced` / `unreadable`), so the distinction survives a stylesheet change and a reader who cannot see colour. A diff --git a/persistent-memory.md b/persistent-memory.md index 9367c95..557e872 100644 --- a/persistent-memory.md +++ b/persistent-memory.md @@ -59,6 +59,34 @@ _As of 2026-09-22:_ ## Recent decisions +- `[2026-09-22]` **The U5 bug-hunt panel found a service-wide hang that the + SIZE CAP ITSELF opened — two hours after I added the cap.** `stat` reports + size 0 for a FIFO and 0 for a symlink to `/dev/zero`, so both sail under a + byte cap and then `read_text` blocks with no EOF or allocates until the kernel + intervenes. `list_booths` reads every booth on every `GET /`, so ONE such file + stalls the front page for the whole service with no error and no recovery + short of a restart. Reproduced (`timeout` returned 124), fixed with an + `S_ISREG` check BEFORE the size check in both modules, verified live: the + index answered 200 in 36 ms with two FIFOs planted. **The reusable shape: + `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.** Also adopted: the upload path wrote the + manifest ABOVE its own cleanup guard (4/4), so a failure orphaned a half-booth + whose uniquely-named leaked temp then kept it alive forever; replace-over- + damaged destroyed recoverable bytes (4/4, now QUARANTINED rather than refused + — marks refuse because judgment is not restatable, a booth's description is); + and `booth answer` spelled out its own openness test, disagreeing with + `booth marks` about a partially-answered pick, which is a direct violation of + U2's INV-2. Full triage in `persistent-memory.d/2026-09-22-u5-panels.md`. +- `[2026-09-22]` **An existing test stopped me retiring documented behaviour + while fixing a race.** The mtime-restore race is real, and the clean fix — + ignoring a booth directory's own mtime whenever the booth holds anything — + would also have silently retired the 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. Fixed the concrete half + (a failing `os.utime` used to escape and 500 the route), left the race stated + in the code. **A fix that changes a documented rule is a proposal, not a + patch.** - `[2026-09-22]` **Two cross-frontier panels on U5, and a paraphrase panel reached a production outage two modules away.** 3-of-4 flagged the contract's "4 GB" case as letter-compliant but purpose-defeating; the conformance round found that @@ -76,17 +104,25 @@ _As of 2026-09-22:_ `booth add` wiped the `why` on the one sequence the feature exists for, and `--title` was write-only. Full triage in `persistent-memory.d/2026-09-22-u5-panels.md`. -- `[2026-09-22]` **U5's adoption is a stated, falsifiable prediction — RE-MEASURE - 2026-09-29.** Operator declined the fleetwide announcement and chose to let the - convention sit, explicitly so that adoption can be told apart from design: - **count how many booths under `~/booth-data` carry a `.booth.json` a week - after landing.** A near-zero count means the convention was never heard of - (an adoption failure — fix by announcing), NOT that the shape was wrong. A - non-zero count from handles that only read the README means it propagates on - its own. Baseline at landing: **0 of 26**. Same instrument as the `.forever` - prediction below, and for the same reason — a diagnosis nobody re-measures is - a belief. - `find ~/booth-data -maxdepth 2 -name .booth.json | wc -l` +- `[2026-09-22]` **U5's adoption prediction, SPLIT IN TWO within an hour of + landing — and the split is the interesting part.** The baseline was recorded as + 0 of 26. Fifty minutes after the deploy, `comfy-dev` created `muse-clothed-repro` + and it announced itself: `{handle: comfy-dev, why: "", created: ...}`. That peer + was told nothing. **The HANDLE propagates for free** — it rides on `booth new` + and `booth add`, so every existing CLI caller starts announcing without learning + anything, which is the flags-on-existing-verbs decision paying off on day zero. + **The WHY does not** — it needs someone to know the flag exists, and this first + one is empty. + + So re-measure BOTH on **2026-09-29**, because they answer different questions: + + find ~/booth-data -maxdepth 2 -name .booth.json | wc -l # free + grep -l '"why": "[^"]' ~/booth-data/*/.booth.json 2>/dev/null | wc -l # learned + + A high first count and a near-zero second is the predicted shape of "nobody was + told", and it is the case the operator's no-announcement decision was designed + to be able to see. Do not read the n=1 above as a rate — it is a code-path + observation (every CLI caller writes a handle), not a sample. - `[2026-09-22]` **The U2 bug-hunt panel landed and it was not ceremony — `v0.2.2`.** Nine adopted findings across four arms; eight were real against live code and one was already fixed. The headline was **4/4 convergent from diff --git a/pyproject.toml b/pyproject.toml index 9c6f4f8..6f31bc9 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1,6 +1,6 @@ [project] name = "booth" -version = "0.2.2" +version = "0.3.0" description = "The Booth — a dead-simple standing web server that scans a data dir of drop-folders and renders each as an ephemeral media 'booth' (image/webm/audio auto-gallery, or a folder's own index.html verbatim). Also accepts browser/curl uploads for pickup under a human-readable id. 24h TTL, then the folder is wiped. Fleet tool for CC sessions to surface A/B and smoke results to the operator." requires-python = ">=3.11" dependencies = [ diff --git a/scripts/booth b/scripts/booth index 9c0db38..f3badea 100755 --- a/scripts/booth +++ b/scripts/booth @@ -30,7 +30,11 @@ # how a broken `.marks.json` used to look like an unanswered question and wait # out the full hour. # marks 0 read ok · 1 --wait timed out with picks open · 3 unreadable -# answer 0 answered · 1 unanswered · 2 no such pick · 3 unreadable +# answer 0 answered · 1 unanswered · 2 no such pick · 3 unreadable · +# 4 the pick hydrated broken and can never be answered +# +# `answer` and `marks` use the SAME openness predicate. A partially-answered +# pick is still open to both; a broken one is closed to both. # booth marks-import import legacy *.ask.json into .marks.json # booth asks alias for `marks` (deprecated) # @@ -433,7 +437,7 @@ sys.exit(2 if open_marks(marks) else 0) import json, os, pathlib, sys sys.path.insert(0, os.environ["BOOTH_SRC"]) try: - from booth.marks import marks_for, read_error + from booth.marks import marks_for, open_marks, read_error booth, mid = sys.argv[1:3] broken = read_error(pathlib.Path(booth)) if broken: @@ -442,14 +446,27 @@ try: # id AND shape, matching the web route. Matching on id alone reported a # note id as "unanswered" and then polled it for an hour — a question that # could never be answered because it was never a question. - m = next((x for x in marks_for(pathlib.Path(booth)) - if x.id == mid and x.shape == "pick"), None) + marks = marks_for(pathlib.Path(booth)) + m = next((x for x in marks if x.id == mid and x.shape == "pick"), None) + # THE openness predicate, not a second spelling of it. `answer is None` is + # what this read used to test, and it disagreed with `marks --wait` on a + # PARTIALLY answered pick: one verb returned the half-filled form while the + # other blocked on the same booth at the same instant. U2 put openness in + # one function precisely so the two could not drift. + still_open = m is not None and m in open_marks(marks) except Exception as exc: print(f"booth: cannot read marks: {exc}", file=sys.stderr) sys.exit(3) if m is None: sys.exit(2) -if m.answer is None: +if m.error: + # Not open, and never going to be: the web route refuses this form with a + # 400, so waiting on it is waiting on nothing. `marks --wait` already + # returns immediately here; this is the other half of that agreement. + print(f"booth: pick is broken and cannot be answered: {m.error}", + file=sys.stderr) + sys.exit(4) +if still_open: sys.exit(1) print(json.dumps(m.answer, ensure_ascii=False, indent=2)) ' "$DATA/$name" "$mid")" || rc=$? @@ -460,6 +477,9 @@ print(json.dumps(m.answer, ensure_ascii=False, indent=2)) # spinning for the full hour on a broken file and then blamed the # operator for not answering. 3) echo "cannot read marks in $name" >&2; exit 3 ;; + # A pick that hydrated broken is refused by the web route, so no answer + # can ever land. Waiting on it is waiting on nothing. + 4) exit 4 ;; esac if [ "$wait_s" -eq 0 ]; then echo "unanswered: $URL/b/$name/#mark-$mid" >&2; exit 1; fi if [ "$(date +%s)" -ge "$deadline" ]; then diff --git a/tests/test_cli.py b/tests/test_cli.py index c9e0047..af8aa66 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -285,3 +285,64 @@ def test_an_explicitly_empty_why_still_clears_it(tmp_path): capture_output=True, timeout=30, env=env) assert _manifest(tmp_path / "b").why == "" + + +def test_answer_and_marks_agree_about_what_open_means(tmp_path): + """U2 made `_is_open` THE openness predicate — "nothing else may spell this + out" — and `booth answer`'s reader spelled it out anyway, as + `if m.answer is None`. So a PARTIALLY answered pick read as done to + `answer` and still-open to `marks --wait`: one verb returns the half-filled + form and the other blocks on the same booth at the same instant. + + Found 2/4. The two verbs are the session's whole view of the loop, and a + session that asks both gets two answers. + """ + import sys + sys.path.insert(0, str(pathlib.Path(__file__).parent.parent)) + from booth.marks import answer_pick, declare_pick + + b = tmp_path / "b" + b.mkdir() + declare_pick(b, "batch", { + "title": "R18", + "questions": [ + {"key": "q1", "prompt": "One?", "options": ["keep", "drop"]}, + {"key": "q2", "prompt": "Two?", "options": ["keep", "drop"]}, + ], + }) + answer_pick(b, "batch", {"q1": "keep", "q2": None}) # partial + + env = {**os.environ, "BOOTH_DATA_DIR": str(tmp_path), + "BOOTH_URL": "http://booth.invalid"} + marks = subprocess.run([str(SCRIPT), "marks", "b"], capture_output=True, + text=True, timeout=30, env=env) + answer = subprocess.run([str(SCRIPT), "answer", "b", "batch"], + capture_output=True, text=True, timeout=30, env=env) + + still_open = "batch" in json.loads(marks.stdout)["open"] + assert still_open, "a partial answer stopped counting as open" + assert answer.returncode == UNANSWERED, ( + "`answer` called a partially-answered pick done while `marks` called it open" + ) + + +def test_answer_does_not_poll_forever_on_a_pick_that_cannot_be_answered(tmp_path): + """The mirror failure. A pick whose declaration went bad hydrates with + `error` set, which makes it NOT open — so `marks --wait` returns at once + while `answer --wait` polled the full hour against a form the web route + refuses with a 400. Nothing was ever going to land.""" + b = tmp_path / "b" + b.mkdir() + (b / ".marks.json").write_text(json.dumps({ + "version": 1, + "marks": [{"id": "broken", "shape": "pick", "declaration": {}, + "error": "pick has no declaration", + "created": "2026-09-21T00:00:00.000000+00:00"}], + })) + + r = subprocess.run([str(SCRIPT), "answer", "b", "broken", "--wait", "8"], + capture_output=True, text=True, timeout=40, + env={**os.environ, "BOOTH_DATA_DIR": str(tmp_path), + "BOOTH_URL": "http://booth.invalid"}) + assert r.returncode != 0 + assert "broken" in r.stderr.lower() or "cannot" in r.stderr.lower() diff --git a/tests/test_manifest.py b/tests/test_manifest.py index 5dad562..c9d8176 100644 --- a/tests/test_manifest.py +++ b/tests/test_manifest.py @@ -519,9 +519,25 @@ def test_only_the_manifest_module_opens_the_manifest(tmp_path): for src in sorted((root / "booth").glob("*.py")): if src.name == "manifest.py": continue - if ".booth.json" in src.read_text(): - offenders.append(src.name) - assert not offenders, f"{offenders} name the manifest file directly" + tree = ast.parse(src.read_text()) + # STRING CONSTANTS, not raw text. A comment naming the file is prose + # about the design and harms nothing — the first version of this test + # scanned the whole source and went red on a comment explaining why a + # leaked `.booth.json..tmp` keeps a booth alive. The invariant is + # about code that knows the filename, so ask the code. + docstrings = set() + for node in ast.walk(tree): + if isinstance(node, (ast.Module, ast.ClassDef, + ast.FunctionDef, ast.AsyncFunctionDef)): + body = getattr(node, "body", None) + if body and isinstance(body[0], ast.Expr) and \ + isinstance(body[0].value, ast.Constant): + docstrings.add(id(body[0].value)) + for node in ast.walk(tree): + if (isinstance(node, ast.Constant) and isinstance(node.value, str) + and id(node) not in docstrings and ".booth.json" in node.value): + offenders.append(f"{src.name}:{node.lineno}") + assert not offenders, f"{offenders} name the manifest file in code" def test_announcing_is_activity_via_the_manifest_file_itself(tmp_path): @@ -572,3 +588,132 @@ def test_the_title_reaches_a_surface(client): page = c.get("/b/r18-ab/").text assert "R18 A/B — denoiser bakeoff" in page assert "r18-ab" in page, "the directory name stopped being visible" + + +# ---- findings from the cross-frontier BUG-HUNT panel, 2026-09-22 ------------- +# +# Heid panel (thread 01M343SXX27Z47C3STXXRC7M42). Four arms, artifact-only, +# diff-scoped. The strongest finding is one the SIZE CAP ITSELF opened. + + +def test_a_reader_never_blocks_on_a_file_that_is_not_a_file(tmp_path): + """`stat` reports size 0 for a FIFO, so it sails under the byte cap — and + then `read_text` blocks in `read` with no EOF, so the `except` never runs + and the call never returns. `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. + + A symlink to `/dev/zero` is the same hole with unbounded allocation instead + of a hang: `st_size` is 0 there too. + + Two of four arms reached it independently. The bound added an hour earlier + is what made it reachable — `st_size` answers a different question than + "can this be read", and a cap that trusts it inherits the difference. + """ + import os + import signal + + b = tmp_path / "b" + b.mkdir() + os.mkfifo(b / MANIFEST_FILE) + + # ⚠ ALARMED. Without this the RED state of this test does not fail, it HANGS + # — which is the defect itself, and is also useless as a signal: a suite that + # stops is indistinguishable from a suite that is slow. Five seconds is a + # thousand times the budget a read of a four-field file should need. + def _timeout(signum, frame): + raise AssertionError("read_manifest blocked on a FIFO and never returned") + + old_handler = signal.signal(signal.SIGALRM, _timeout) + signal.alarm(5) + try: + got = read_manifest(b) + finally: + signal.alarm(0) + signal.signal(signal.SIGALRM, old_handler) + + assert isinstance(got, Manifest) and got.error + assert "regular file" in got.error + + +def test_a_damaged_manifest_is_kept_when_it_is_replaced(tmp_path): + """4/4, and it contradicted this repo's own doctrine. Marks made the rule + explicit in v0.2.1 — reads stay lenient, writes go strict, damaged bytes + STAY ON DISK — and the manifest's write replaced them outright. + + The sharpest leg: a file that fails on ONE field still holds the others. + `{"handle": 7, "why": "the thing I wanted you to look at"}` reads as broken + and used to be destroyed whole, taking a `why` the re-announcer may not have + kept anywhere. + + Quarantined rather than refused: refusing would fail `booth add` and lose + the files it was copying, which is the worse trade. One fixed-name + quarantine, so this cannot accumulate. + """ + from booth.manifest import QUARANTINE_FILE + + b = tmp_path / "b" + b.mkdir() + damaged = json.dumps({"handle": 7, "why": "the thing I wanted you to see"}) + (b / MANIFEST_FILE).write_text(damaged) + + write_manifest(b, "booth-dev", why="rescued") + + assert read_manifest(b).why == "rescued" + assert (b / QUARANTINE_FILE).read_text() == damaged, "the damaged bytes were destroyed" + + +def test_a_broken_record_normalizes_the_directory_name_too(tmp_path): + """The third fallback. `write_manifest`'s and `read_manifest`'s were fixed + in the previous round and `_broken`'s was missed — same raw `booth.name`, + same card sub-line, same newline.""" + b = tmp_path / ("wei" + "i" * 200 + "rd\nname") + b.mkdir() + (b / MANIFEST_FILE).write_text("{oops") + + got = read_manifest(b) + assert got.error and "\n" not in got.title and len(got.title) <= 120 + + +def test_an_identical_re_announce_does_not_touch_the_booth(tmp_path): + """Marks learned this in v0.2.0: a write that changes nothing is not + activity and must not reset a booth's TTL. The manifest wrote + unconditionally, so `booth add` on an unchanged booth kept a dead one alive + — and `booth link` does it on every single post to the standing board.""" + import os + + b = tmp_path / "b" + b.mkdir() + write_manifest(b, "booth-dev", why="x") + path = b / MANIFEST_FILE + os.utime(path, (1_000_000_000, 1_000_000_000)) + os.utime(b, (1_000_000_000, 1_000_000_000)) + before = path.stat().st_mtime + + write_manifest(b, "booth-dev", why="x") # identical + + assert path.stat().st_mtime == before, "an identical re-announce rewrote the file" + + +def test_a_failed_write_leaves_no_temp_file_behind(tmp_path): + """The unique temp name fixed a cross-writer hazard and created a litter + one: a fixed name is overwritten by the next writer, a random one is not. + And `.booth.json..tmp` is NOT a `.lock`, so `_newest_mtime` counts it — + an orphaned temp would keep a dead booth alive forever.""" + import os + + b = tmp_path / "b" + b.mkdir() + real_replace = os.replace + + def boom(src, dst, *a, **kw): + raise OSError("no space left on device") + + os.replace = boom + try: + with pytest.raises(OSError): + write_manifest(b, "booth-dev", why="x") + finally: + os.replace = real_replace + + assert not list(b.glob("*.tmp")), f"orphaned temp: {list(b.glob('*.tmp'))}" diff --git a/tests/test_marks.py b/tests/test_marks.py index 96d2a36..87db93b 100644 --- a/tests/test_marks.py +++ b/tests/test_marks.py @@ -1247,3 +1247,130 @@ def test_a_write_over_an_unparseable_marks_file_still_refuses(tmp_path): with pytest.raises(MarksCorrupt): set_flag(booth, "a.png", True) + + +# ---- findings from the U5 diff-scoped BUG-HUNT panel, 2026-09-22 ------------ + + +def test_the_marks_reader_never_blocks_on_a_file_that_is_not_a_file(tmp_path): + """Same hole the size cap opened in the manifest, in the sibling it was + copied from. `st_size` is 0 for a FIFO, so it passes the cap, and then + `read_text` blocks with no EOF. `list_booths` reads every booth's marks on + every `GET /` and `/healthz`.""" + import os + import signal + + booth = tmp_path / "b" + booth.mkdir() + os.mkfifo(booth / MARKS_FILE) + + def _timeout(signum, frame): + raise AssertionError("marks_for blocked on a FIFO and never returned") + + old = signal.signal(signal.SIGALRM, _timeout) + signal.alarm(5) + try: + assert marks_for(booth) == [] + finally: + signal.alarm(0) + signal.signal(signal.SIGALRM, old) + + +def test_new_marks_and_imported_marks_share_one_stamp_format(tmp_path): + """The v0.2.2 fix for the legacy-import ordering opened a NEW ordering bug, + which is the shape worth remembering. `import_legacy_asks` moved to + microsecond precision while `now_stamp` stayed at whole seconds, and `-` is + 0x2D against `.` at 0x2E — so `...T10:00:00-07:00` sorts BEFORE + `...T10:00:00.500000-07:00`, putting a LATER mark ahead of an EARLIER + import inside the same second. + + Deterministic order is a v1 invariant precisely because the operator refers + to things positionally. One format, or the rule cannot be stated. + """ + from booth.marks import now_stamp + + stamp = now_stamp() + assert "." in stamp.split("T")[1], f"now_stamp is not sub-second: {stamp}" + assert len(stamp.split(".")[1].split("+")[0].split("-")[0]) == 6 + + +def test_the_importer_cannot_raise_out_of_a_poisoned_entry(tmp_path): + """`marks_for` routes every entry through `_hydrate_safe`; the importer's + return still went through the bare `_hydrate`, so the one path that reads + entries it did not write was the one without the guard.""" + booth = tmp_path / "b" + booth.mkdir() + (booth / MARKS_FILE).write_text(json.dumps({ + "version": 1, + "marks": [{"id": "n1", "shape": "note", "text": {"bad": True}, + "created": "2026-09-21T00:00:00+00:00"}], + })) + (booth / f"q1{ASK_SUFFIX}").write_text(json.dumps(_single())) + + from booth.marks import import_legacy_asks + out = import_legacy_asks(booth) # must not raise + assert isinstance(out, list) + + +def test_a_document_that_would_not_read_back_is_refused_at_the_write(tmp_path): + """The read bound is on the STORED bytes and the write adds `indent=2`, so a + document that fits in memory can land over the limit on disk and then read + back as no marks at all — every mark in the booth gone, silently. Refuse + loudly instead: a write that fails is recoverable. + + Asserted against `_write_raw` directly, because no single mark can get + there: `_clean_text` caps a note at TEXT_MAX and a flag is a fixed shape. + The reachable path is accumulation — `_note_id` puts no ceiling on how many + notes one booth may carry — which is thousands of writes, not one. Testing + it through `write_note` would need a fixture nobody could justify, and + would be testing the cap rather than the guard. + """ + from booth.marks import MARKS_MAX_BYTES, MarksCorrupt, _write_raw + + booth = tmp_path / "b" + booth.mkdir() + bulk = [{"id": f"note-{i}", "shape": "note", "text": "x" * 500, + "created": "2026-09-21T00:00:00.000000+00:00"} + for i in range(MARKS_MAX_BYTES // 400)] + + with pytest.raises(MarksCorrupt): + _write_raw(booth, bulk) + assert not (booth / MARKS_FILE).exists(), "a refused write still landed" + + +def test_a_clock_restore_that_fails_does_not_take_the_route_down(tmp_path): + """The concrete half of the mtime-restore finding. + + `_Locked.__enter__` puts the booth directory's clock back after creating its + lock, and `os.utime` can fail — a read-only directory, a booth whose owner + we are not. It used to escape into the route and answer 500 for what is + otherwise a perfectly good request. Not putting the clock back is a cost + this module can absorb; not answering is not. + + The RACE half of that finding is documented in the code and deliberately not + closed: the alternative fix would silently retire the documented behaviour + that releasing a kept board resets its clock + (`test_releasing_a_board_RESETS_its_ttl_clock` pins that on purpose), which + is a TTL doctrine change rather than a bug fix. + """ + import os + + from booth.marks import MARKS_LOCK, set_flag + + booth = tmp_path / "b" + booth.mkdir() + real_utime = os.utime + + def boom(path, *a, **kw): + if str(path) == str(booth): + raise PermissionError("read-only directory") + return real_utime(path, *a, **kw) + + os.utime = boom + try: + assert set_flag(booth, "a.png", True) is not None + finally: + os.utime = real_utime + + assert (booth / MARKS_LOCK).exists() + assert [m.target for m in marks_for(booth)] == ["a.png"]