diff --git a/booth/benches.py b/booth/benches.py index 6b8160f..a06a34f 100644 --- a/booth/benches.py +++ b/booth/benches.py @@ -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: diff --git a/scripts/booth b/scripts/booth index 27c57f6..8cc8383 100755 --- a/scripts/booth +++ b/scripts/booth @@ -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." diff --git a/tests/test_benches.py b/tests/test_benches.py index 70f6af7..eb4cc39 100644 --- a/tests/test_benches.py +++ b/tests/test_benches.py @@ -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 ` and `bench rm `: 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"] diff --git a/tests/test_cli.py b/tests/test_cli.py index 1cdfc3f..20bcf1e 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -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"