fix(u6): the booth check fails closed with a reason, and a dead write leaves no scratch

Two more from the in-session adversarial pass.

`booth link`'s new booth-URL check shells out to booth/links.py. When that
import cannot run, the command substitution under `set -e` aborted the script
with a bare ModuleNotFoundError traceback: the right DIRECTION (no row was
appended — a guard that fails open is not a guard) reached by accident, and
unactionable when it fires. Handled explicitly now: exit 3, and a message
naming what the check needs. The fail-closed direction is stated rather than
inherited from shell semantics, and a test pins it — the defeating change in
either direction goes red.

_write_all's scratch file was stranded beside the registry if the write died
between create and replace. Cleaned up on every exit path. The prior registry
was never at risk either way: os.replace is the only thing that publishes.

Also pins normalization idempotence, which `bench state <id|url>` and
`bench rm <id|url>` both rely on: they normalize whatever they are handed, so
an id that did not normalize to itself would miss the row it names.
This commit is contained in:
vh
2026-09-22 13:33:59 -07:00
parent 8c7f2127eb
commit 0a2bb1d26c
4 changed files with 84 additions and 3 deletions
+12 -2
View File
@@ -258,9 +258,19 @@ def _write_all(root: Path, benches: dict[str, Bench]) -> None:
# of one fact is two things that can disagree.
for b in benches.values()
}
# Per-pid scratch name so two writers cannot share it: the atomic-replace
# promise is that a READER never sees a partial file, not that two writers
# never collide on the way there.
tmp = path.with_suffix(path.suffix + f".tmp.{os.getpid()}")
tmp.write_text(json.dumps(payload, indent=2, sort_keys=True) + "\n")
os.replace(tmp, path)
try:
tmp.write_text(json.dumps(payload, indent=2, sort_keys=True) + "\n")
os.replace(tmp, path)
except BaseException:
# A write that dies between create and replace would otherwise strand
# the scratch file beside the registry forever. The prior registry is
# untouched either way — os.replace is the only thing that publishes.
tmp.unlink(missing_ok=True)
raise
class _Locked:
+17 -1
View File
@@ -327,7 +327,23 @@ case "$cmd" in
# moment the booth is swept — 156 of the board's 221 rows are exactly
# that. Refusing AFTER the mkdir/announce below would leave a new booth
# behind as the side effect of a call that failed.
refused_name="$(booth_target_of "$link_url")"
# `|| pred_rc=$?` so a BROKEN PREDICATE is handled here rather than aborting
# the script under `set -e` with a raw Python traceback and nothing else.
# The direction is FAIL-CLOSED and stays that way: if we cannot tell whether
# this is a booth, we do not append. A guard that fails open is not a guard,
# and the cost of being wrong in the other direction is one message telling
# the poster exactly what broke.
pred_rc=0
refused_name="$(booth_target_of "$link_url" 2>/dev/null)" || pred_rc=$?
if [ "$pred_rc" -ne 0 ]; then
{
echo "booth link: could not check whether that URL is a booth, so nothing was posted."
echo " the check runs booth/links.py under the system python3 with no venv."
echo " re-run from a checkout where \`python3 -c 'import booth.links'\` works,"
echo " or post it from a host that has one."
} >&2
exit 3
fi
if [ -n "$refused_name" ]; then
{
echo "booth link: that is a booth, and a booth announces itself now."
+33
View File
@@ -579,3 +579,36 @@ def test_a_control_character_is_not_an_addressable_booth(encoded):
control-character clause — the `%2e%2e` and `%2f` rows above stay green
under it, so this needs its own."""
assert booth_target(f"http://h:8090/b/{encoded}/") is None
def test_normalization_is_idempotent(tmp_path):
"""LOAD-BEARING for `bench state <id|url>` and `bench rm <id|url>`: both
normalize whatever they are handed, so an id must normalize to itself or
addressing a bench by the id the registry stores would miss it. Defeating
change: any rule that rewrites an already-normalized form."""
for u in (GITEA_EIGHT + TALK_FIVE + [
"http://x.test/", "http://x.test:8080/p/", "https://x.test/?a=1",
"HTTP://X.Test:80/Some%20Path/?b=2&a=1#frag",
]):
once = normalize_bench_url(u)
assert normalize_bench_url(once) == once, u
def test_a_failed_write_leaves_no_scratch_file(tmp_path, monkeypatch):
"""The temp file is named per-pid so two writers cannot share it, but a
write that dies between create and replace would strand it beside the
registry forever. Defeating change: dropping the cleanup."""
import booth.benches as B
upsert_bench(tmp_path, "http://a.test/", "one", "o")
real = B.os.replace
def boom(src, dst):
raise OSError("disk full")
monkeypatch.setattr(B.os, "replace", boom)
with pytest.raises(OSError):
upsert_bench(tmp_path, "http://b.test/", "two", "o")
monkeypatch.setattr(B.os, "replace", real)
strays = [p.name for p in tmp_path.iterdir() if ".tmp" in p.name]
assert not strays, strays
# and the prior registry is intact — a failed write destroys nothing
assert [b.name for b in read_benches(tmp_path)[0]] == ["one"]
+22
View File
@@ -506,3 +506,25 @@ def test_nothing_in_the_unit_touches_links_md(booth):
run(data, "bench", "rm", "http://new.test/")
after = hashlib.sha256((board / "links.md").read_bytes()).hexdigest()
assert before == after
def test_link_fails_CLOSED_when_the_booth_check_cannot_run(booth, tmp_path):
"""A guard that fails open is not a guard. If `booth.links` cannot be
imported, `booth link` must post NOTHING and say why — not append the row
it could not classify, and not abort with a bare traceback.
Defeating change: dropping the `|| pred_rc=$?` handling, which under
`set -e` aborts with a Python traceback (safe, but unactionable), or
treating a failed check as "not a booth" (unsafe — fails open)."""
data, _ = booth
lone = tmp_path / "lone" / "scripts"
lone.mkdir(parents=True)
(lone / "booth").write_text(SCRIPT.read_text())
(lone / "booth").chmod(0o755)
r = subprocess.run([str(lone / "booth"), "link", "https://ok.test/x", "a bookmark"],
capture_output=True, text=True, cwd="/tmp", timeout=30,
env={**os.environ, "BOOTH_DATA_DIR": str(data),
"BOOTH_URL": "http://booth.invalid"})
assert r.returncode != OK
assert "could not check" in r.stderr, r.stderr
assert not (data / "links" / "links.md").exists(), "a row landed despite an unusable check"