Skip to content

Commit b5c4af4

Browse files
committed
fix(logstream): checkpoint the starting position once, whatever produced it
Fifth Codex round, one new P2 on dd9b123, and the sixth instance of one bug: a starting cursor that never reaches disk, so the next launch calls itself a first run and jumps to the tip. This time --since-event-id with no state file skipped the immediate checkpoint entirely, because that write lived inside the "resolve the tip" branch and an explicit cursor never enters it. Nothing was persisted until watch_events yielded, which by default is up to five minutes later; an interrupt inside that window leaves no file at all. Rather than add a third call site I hoisted it: one place resolves the starting position, one place records it. The position can arrive three ways — an explicit --since-event-id, the tip, or None from an empty log or --from-start — and all three now take the same required write whenever no state file exists yet. That removes the shape the last four rounds kept finding new instances of, instead of removing one more instance. Two test-quality notes, both the same lesson: The first version of this test passed against the unfixed code. The helper uses a 60ms poll timeout, so watch_events yielded immediately and the loop's own checkpoint wrote the file — the startup window the bug lives in never opened. It now stubs watch_events to interrupt before yielding, which is what actually pins the startup write, and covers all three entry paths. The hoist also broke test_match_does_not_checkpoint_if_output_fails, which asserted that no state file existed after a failed match write. That was pinning the symptom: a startup checkpoint now legitimately exists. The invariant is that its cursor must still be the pre-match position so the undelivered event replays, so it asserts the cursor value instead. 4407 passed, 31 skipped.
1 parent dd9b123 commit b5c4af4

2 files changed

Lines changed: 81 additions & 20 deletions

File tree

mempalace/cli.py

Lines changed: 20 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1612,21 +1612,26 @@ def _logstream_watch(ls, args, as_json):
16121612
# what arrives from now on. Never silent: say what was skipped.
16131613
cursor = ls.latest_event_id()
16141614
skipped_from = cursor
1615-
# Record the starting position immediately, even when the log is
1616-
# empty and that position is None. Without the file, the next launch
1617-
# is indistinguishable from a first run and would jump past anything
1618-
# that arrived while this watcher was stopped — so unlike later
1619-
# checkpoints this one must not fail quietly.
1620-
if args.state_file:
1621-
try:
1622-
write_watch_cursor(args.state_file, cursor, agent=args.agent, required=True)
1623-
except OSError as exc:
1624-
_logstream_fail(
1625-
f"could not write the initial checkpoint to {args.state_file}: {exc}. "
1626-
"Refusing to start: without it a restart would skip every event "
1627-
"that arrives before then.",
1628-
as_json,
1629-
)
1615+
1616+
# One place decides the starting position; one place records it. That
1617+
# position can arrive three ways — an explicit --since-event-id, the tip
1618+
# above, or None (an empty log, or --from-start) — and every one of them
1619+
# needs the same immediate checkpoint when no state file exists yet.
1620+
# Deferring to the first watch_events yield leaves a window of up to a
1621+
# full poll timeout in which an interrupt leaves no file behind, and the
1622+
# next launch calls itself a first run and jumps to the tip, skipping
1623+
# whatever arrived in between. Unlike later checkpoints this one cannot
1624+
# fail quietly: losing it costs a skipped event, not a replay.
1625+
if args.state_file and state_condition == WATCH_STATE_ABSENT:
1626+
try:
1627+
write_watch_cursor(args.state_file, cursor, agent=args.agent, required=True)
1628+
except OSError as exc:
1629+
_logstream_fail(
1630+
f"could not write the initial checkpoint to {args.state_file}: {exc}. "
1631+
"Refusing to start: without it a restart would skip every event "
1632+
"that arrives before then.",
1633+
as_json,
1634+
)
16301635
if not as_json:
16311636
where = args.agent or ", ".join(sorted(spec["to_agents"] or [])) or "everything"
16321637
print(f"Watching {where} from {cursor or 'now'}; Ctrl-C to stop.", file=sys.stderr)

tests/test_cli_logstream.py

Lines changed: 61 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -520,18 +520,29 @@ def test_idle_deadline_caps_the_poll(self, palace_path, capsys):
520520
def test_match_does_not_checkpoint_if_output_fails(
521521
self, palace_path, tmp_path, capsys, monkeypatch
522522
):
523-
"""A broken pipe after a match must not persist the cursor."""
524-
state = str(tmp_path / "watch.json")
523+
"""A broken pipe after a match must not advance the cursor past it.
524+
525+
Asserting "no state file" would be pinning the symptom: since the
526+
starting position is now checkpointed before the first poll, a file
527+
legitimately exists. What must hold is that its cursor is still the
528+
pre-match position, so the undelivered event replays on restart —
529+
a duplicate, never a skip.
530+
"""
531+
state = tmp_path / "watch.json"
525532
cmd_logstream(_append_args(palace_path, to_agent="mac-claude", from_agent="windows-grok"))
526-
capsys.readouterr()
533+
matched_id = json.loads(capsys.readouterr().out)["id"]
527534

528535
def boom(*_a, **_k):
529536
raise OSError("broken pipe")
530537

531538
monkeypatch.setattr("json.dumps", boom)
532539
with pytest.raises(OSError, match="broken pipe"):
533-
cmd_logstream(_watch_args(palace_path, agent="mac-claude", state_file=state))
534-
assert not (tmp_path / "watch.json").exists()
540+
cmd_logstream(_watch_args(palace_path, agent="mac-claude", state_file=str(state)))
541+
542+
monkeypatch.undo()
543+
stored = json.loads(state.read_text(encoding="utf-8"))["cursor"]
544+
assert stored != matched_id, "cursor advanced past an event that was never delivered"
545+
assert stored is None, "cursor should still be the pre-match starting position"
535546

536547
def test_fresh_watch_starts_at_the_tip_not_the_beginning(self, palace_path, capsys):
537548
"""A first watch must not replay the whole log.
@@ -705,3 +716,48 @@ def denied(*_a, **kw):
705716
)
706717
assert exc.value.code == 1
707718
assert "initial checkpoint" in json.loads(capsys.readouterr().out)["error"]
719+
720+
def test_starting_cursor_is_checkpointed_before_the_first_poll(
721+
self, palace_path, tmp_path, monkeypatch, capsys
722+
):
723+
"""Every entry path must checkpoint before polling, not after.
724+
725+
The starting position arrives three ways — an explicit
726+
--since-event-id, the tip, or None (empty log / --from-start) — and
727+
all three need the file on disk *before* the first poll. Deferring to
728+
the first watch_events yield leaves a window of up to a full poll
729+
timeout; an interrupt inside it leaves no file, so the next launch
730+
calls itself a first run and jumps to the tip.
731+
732+
watch_events is stubbed to interrupt immediately, which is what makes
733+
this pin the startup write. Letting the real loop run would pass on
734+
the loop's own checkpoint instead, since the test poll timeout is
735+
milliseconds rather than the five-minute default.
736+
"""
737+
import mempalace.logstream as logstream_module
738+
739+
cmd_logstream(_append_args(palace_path, to_agent="mac-claude", from_agent="windows-grok"))
740+
first_id = json.loads(capsys.readouterr().out)["id"]
741+
742+
def interrupt_before_yielding(*_a, **_k):
743+
raise KeyboardInterrupt
744+
745+
monkeypatch.setattr(logstream_module.Logstream, "watch_events", interrupt_before_yielding)
746+
747+
for label, overrides, expected in (
748+
("explicit cursor", {"since_event_id": first_id}, first_id),
749+
("tip", {}, first_id),
750+
("from-start", {"from_start": True}, None),
751+
):
752+
state = tmp_path / f"{label.replace(' ', '_')}.json"
753+
kwargs = {
754+
"agent": "mac-claude",
755+
"state_file": str(state),
756+
"from_start": False,
757+
**overrides,
758+
}
759+
with pytest.raises(SystemExit) as exc:
760+
cmd_logstream(_watch_args(palace_path, **kwargs))
761+
assert exc.value.code == 130
762+
assert state.exists(), f"{label}: interrupted before any checkpoint landed"
763+
assert json.loads(state.read_text(encoding="utf-8"))["cursor"] == expected, label

0 commit comments

Comments
 (0)