vh/booth
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix(manifest)!: the size cap opened a service-wide hang; close it

Browse Source 
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.
This commit is contained in:
Vuong Hoang
2026-09-22 02:27:18 -07:00
parent f3193fb054
commit 95beede3c3
11 changed files with 616 additions and 61 deletions
Download Patch File Download Diff File Expand all files Collapse all files
booth/marks.py
+69 -11
View File
@@ -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]