From 4f309f8d94ded92607e27d80f05cbabbbafa69c5 Mon Sep 17 00:00:00 2001 From: Nikolay Semernya Date: Fri, 17 Jul 2026 14:19:34 +0300 Subject: [PATCH] =?UTF-8?q?sinks:=20=5Fwrite=5Flock=20stops=20at=20the=20p?= =?UTF-8?q?rocess=20boundary=20=E2=80=94=20say=20so=20where=20it=20is=20pr?= =?UTF-8?q?omised?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit _DailyFileState warns that interleaved appends corrupt the rolling hash, then reassures the caller that "LocalFileSink guarantees that with _write_lock, which is why append_line does no locking of its own". It is a threading.Lock. It guarantees nothing across processes, and a caller reading that docstring to decide whether they need their own locking gets the wrong answer. emit.py already states the truth — "What is NOT guaranteed: ... two processes appending to the same log" — so the library knows. It documented the limit accurately in one file and promised the opposite in another. Two bosun workers appending to one chain produced 7 forked chains across 5825 records. Worth being precise about the harm: nothing was corrupted. 0 dangling prev_hash, 4927/4927 resolvable signatures verified. The chain branched, and verify reported CHAIN_BREAK over intact evidence — which is worse than a clean failure, because it teaches the operator to disregard the verifier. The promise now names its boundary and points at the one piece of cross-process serialisation that does exist (the flock in hook-record, Claude Code path only). Not this commit: making appends actually cross-process safe. That is a real change to the write path — an OS lock around [read head, sign, append, publish manifest] — and it lands on top of the manifest's per-record rewrite, which is its own open question. Doing both at once would re-entangle two concerns in one file. This closes the misleading half. The first draft of the guard passed against the lying docstring, because the text says "previous process wrote to it today" elsewhere and a whole-docstring search found that. It now anchors to the paragraph that makes the claim. --- src/chiplog/sinks/local_file.py | 12 ++++- tests/test_sink_lock_scope_guard.py | 74 +++++++++++++++++++++++++++++ 2 files changed, 84 insertions(+), 2 deletions(-) create mode 100644 tests/test_sink_lock_scope_guard.py diff --git a/src/chiplog/sinks/local_file.py b/src/chiplog/sinks/local_file.py index cf3c541..82bf300 100644 --- a/src/chiplog/sinks/local_file.py +++ b/src/chiplog/sinks/local_file.py @@ -63,8 +63,16 @@ class _DailyFileState: The rolling hash is only equal to the file's real SHA-256 if the context is fed the lines in the SAME ORDER the file received them. Callers must not let - two appends interleave — `LocalFileSink` guarantees that with `_write_lock`, - which is why `append_line` does no locking of its own. + two appends interleave — `LocalFileSink` serialises them with `_write_lock`, + which is why `append_line` does no locking of its own. That guarantee stops + at the process boundary: `_write_lock` is a `threading.Lock`, so it orders + appends from threads and recorders sharing ONE process and nothing else. A + second process appending to the same log is not serialised by this sink, and + the result is a forked chain that `verify` reports as CHAIN_BREAK over + evidence that is in fact intact. `emit.py` states the same limit for the + recorder; the only cross-process serialisation in the library is the `flock` + in the `chiplog hook-record` subprocess, which covers the Claude Code hook + path and nothing else. """ def __init__(self, path: Path) -> None: diff --git a/tests/test_sink_lock_scope_guard.py b/tests/test_sink_lock_scope_guard.py new file mode 100644 index 0000000..a9d66f3 --- /dev/null +++ b/tests/test_sink_lock_scope_guard.py @@ -0,0 +1,74 @@ +"""Structural: where the sink claims to serialise appends, it must name the boundary. + +`_DailyFileState` tells its caller that interleaved appends would corrupt the +rolling hash, and then reassures them: "`LocalFileSink` guarantees that with +`_write_lock`, which is why `append_line` does no locking of its own." Read +that and you conclude serialisation is handled. It is a `threading.Lock`. It +handles nothing across processes. + +`emit.py` states the truth in the same codebase — "What is NOT guaranteed: ... +two processes appending to the same log" — so the library knows. It documents +the limit accurately in one file and promises the opposite in another, and a +caller reading the sink's own docstring to decide whether they need their own +locking gets the wrong answer. + +That is not hypothetical. Two bosun workers appending to one chain produced 7 +forked chains across 5825 records (forensics, 2026-07-17). Nothing was +corrupted — 0 dangling `prev_hash`, every resolvable signature verified — but +`verify` reported CHAIN_BREAK over intact evidence, which is worse than a clean +failure: it teaches the operator to disregard the verifier. + +This guard is prose-shaped because the defect is prose-shaped, the same way +`test_report_claims_guard.py` is. A docstring that makes a concurrency promise +has to say where the promise stops; the word this checks for is the cheapest +proxy for that, and if a rewrite drops it the test should fail and make someone +re-read the claim. + +It does NOT pin that appends are cross-process safe — they are not. Making them +so is a real change to the sink's write path, and it is deliberately not this. +""" + +from __future__ import annotations + +import threading +from pathlib import Path + +from chiplog.sinks import local_file + + +def _the_promise() -> str: + """The paragraph that makes the serialisation claim, not the whole docstring. + + Anchoring to the paragraph matters: the docstring elsewhere says "previous + process wrote to it today", so a naive search for the word across the whole + text passes while the claim itself stays unqualified — which is how the + first draft of this guard went green against a docstring that still lied. + """ + doc = local_file._DailyFileState.__doc__ + assert doc is not None, "_DailyFileState lost its docstring" + paras = [p for p in doc.split("\n\n") if "_write_lock" in p] + assert len(paras) == 1, ( + "this guard is anchored to the one paragraph that promises " + f"serialisation; found {len(paras)}. If the claim moved or split, " + "move the guard with it rather than loosening it" + ) + return paras[0] + + +def test_the_lock_backing_the_promise_is_process_local(tmp_path: Path) -> None: + # The premise of this guard. If this ever becomes a cross-process lock, + # the docstring rule below should be revisited, not silenced. + sink = local_file.LocalFileSink(dir=tmp_path / "audit") + assert isinstance(sink._write_lock, type(threading.Lock())) + + +def test_serialisation_promise_names_its_boundary() -> None: + promise = _the_promise() + assert "process" in promise.lower(), ( + "_DailyFileState tells callers LocalFileSink serialises appends for them, " + "and _write_lock is a threading.Lock — so the promise holds only within " + "one process. Say so in the paragraph that makes it. A caller who " + "believes the unqualified version writes from a second process and gets " + "a forked chain that verify reports as CHAIN_BREAK over intact evidence. " + "See emit.py's accurate statement of the same limit.\n\n" + promise + )