diff --git a/api/run_journal.py b/api/run_journal.py index 087a08ba..f4185268 100644 --- a/api/run_journal.py +++ b/api/run_journal.py @@ -464,18 +464,15 @@ class RunJournalWriter: # the offline-gap coverage and replay-cursor contiguity checks rely on. if str(event_name or "").strip() in REPLAY_SKIPPED_SSE_EVENTS: return None - # Draw from the shared module-level seq cache under the per-path lock so - # this writer and any direct append_run_event() call on the same path - # agree on one monotonic, gapless sequence. - with self._lock: - seq = _reserve_next_seq(self._path) + # Allocate the sequence inside the same per-path transaction that writes + # the row. Reserving here, then releasing the lock before append, lets a + # concurrent writer put a higher sequence on disk first. return append_run_event( self.session_id, self.run_id, event_name, payload or {}, session_dir=self.session_dir, - seq=seq, ) diff --git a/docs/rfcs/webui-run-state-consistency-contract.md b/docs/rfcs/webui-run-state-consistency-contract.md index d1362548..9128e910 100644 --- a/docs/rfcs/webui-run-state-consistency-contract.md +++ b/docs/rfcs/webui-run-state-consistency-contract.md @@ -30,6 +30,16 @@ This RFC defines a consistency contract for those layers. It complements the larger run adapter direction in #1925 by documenting what must remain coherent while WebUI still has multiple overlapping state stores. +## Run-journal sequence publication + +Within one WebUI process, auto-numbered appends to the same journal allocate +sequence numbers and write their rows under the same per-path lock. +`RunJournalWriter` delegates both operations to `append_run_event`; it must not +reserve a sequence and release the lock before the physical append. Otherwise +individually valid rows can reach disk out of order and the session replay +reader must reject them as noncontiguous. This does not change caller-supplied +sequence semantics, cross-process ownership, or failed-write recovery. + ## Inactive compression continuation recovery The Agent profile's SQLite compression lineage owns the canonical continuation, diff --git a/tests/test_run_journal_writer_ordering.py b/tests/test_run_journal_writer_ordering.py new file mode 100644 index 00000000..432ea067 --- /dev/null +++ b/tests/test_run_journal_writer_ordering.py @@ -0,0 +1,74 @@ +"""Sequence allocation and physical append must share one journal lock. + +Only scheduling at the real append boundary is controlled. Writers, sequence +allocation, on-disk JSONL and the session replay reader are production code. +""" +from concurrent.futures import ThreadPoolExecutor +import threading + +import pytest + +from api import run_journal + + +@pytest.mark.parametrize("competitor", ["same_writer", "other_writer", "free_function"]) +def test_writer_sequence_follows_physical_append_order(tmp_path, monkeypatch, competitor): + sid, rid = "writer_order_session", "writer_order_run" + writer = run_journal.RunJournalWriter(sid, rid, session_dir=tmp_path) + other = run_journal.RunJournalWriter(sid, rid, session_dir=tmp_path) + reached = threading.Event() + release = threading.Event() + real_append = run_journal.append_run_event + + def pause_first_append(session_id, run_id, event_name, payload=None, **kwargs): + if payload == {"text": "paused"}: + reached.set() + assert release.wait(5), "test did not release the paused append" + return real_append(session_id, run_id, event_name, payload, **kwargs) + + monkeypatch.setattr(run_journal, "append_run_event", pause_first_append) + + def append_competitor(): + if competitor == "free_function": + return run_journal.append_run_event( + sid, rid, "token", {"text": "overtaking"}, session_dir=tmp_path + ) + target = writer if competitor == "same_writer" else other + return target.append_sse_event("token", {"text": "overtaking"}) + + with ThreadPoolExecutor(max_workers=2) as pool: + first = pool.submit(writer.append_sse_event, "token", {"text": "paused"}) + try: + assert reached.wait(5), "writer did not reach the append boundary" + second = pool.submit(append_competitor).result(timeout=5) + finally: + release.set() + first_result = first.result(timeout=5) + + journal = run_journal.read_run_events(sid, rid, session_dir=tmp_path) + # Deliberately do not sort: the reader requires physical contiguous order. + assert [row["seq"] for row in journal["events"]] == [1, 2] + assert [row["payload"]["text"] for row in journal["events"]] == ["overtaking", "paused"] + assert second["event_id"] == f"{rid}:1" + assert first_result["event_id"] == f"{rid}:2" + replay = run_journal.read_session_run_events( + sid, after_event_id=second["event_id"], session_dir=tmp_path + ) + assert replay["status"] == "ok" + assert [row["event_id"] for row in replay["events"]] == [first_result["event_id"]] + + +def test_writer_rejected_empty_name_does_not_reserve_a_sequence(tmp_path): + writer = run_journal.RunJournalWriter("name_session", "name_run", session_dir=tmp_path) + with pytest.raises(ValueError, match="event_name is required"): + writer.append_sse_event(" ", {"text": "rejected"}) + event = writer.append_sse_event("token", {"text": "accepted"}) + assert event["seq"] == 1 + assert event["event_id"] == "name_run:1" + + +def test_metering_skip_still_does_not_reserve_or_write(tmp_path): + writer = run_journal.RunJournalWriter("meter_session", "meter_run", session_dir=tmp_path) + assert writer.append_sse_event("metering", {"tps": 10}) is None + assert not (tmp_path / "_run_journal" / "meter_session" / "meter_run.jsonl").exists() + assert writer.append_sse_event("token", {"text": "kept"})["seq"] == 1