fix(store): read_messages last_seq returns head seq when all ephemeral messages expired (fixes #287)

[RobGenins]

Replaces #873 with regression test added.

Changes:

• src/store.py: read_messages last_seq returns head seq not 0 when all ephemeral messages expired
• tests/unit/test_store.py: regression test — creates expired ephemeral room, reads with since=None, asserts last_seq matches room head seq

All tests pass.

[github-actions]

Open pull requests citing the same issues:

• fix(app): a reaped d- room can be claimed again (regression from #343) #579 (also cites fix: read_messages() resets last_seq to 0 when messages in ephemeral room expire #287)

If one already covers this change, review or build on it instead of racing it (CONTRIBUTING.md "Overlapping work").

[yukkie3276]

There is a cross-fix regression with the reaped-room semantics being restored in #579. This change uses last_seq(root, room) whenever an unfiltered read returns no visible records. But last_seq() deliberately retains the persisted sequence floor after a room file has been reaped, so read_messages(..., since=None) on a reaped room will now return that old nonzero floor instead of 0. #579's regression explicitly requires the opposite: once the room file is gone, the read lane reports last_seq == 0 so the name behaves as a fresh empty room even though the internal sequence floor survives.

The fix therefore needs to distinguish "room file exists but every ephemeral record is expired" from "room file no longer exists/reaped" rather than using last_seq() for every empty result. Please add both cases to the regression coverage; the expired-file case should return the head, while the reaped/absent-file case should remain 0.

[bdunn77]

Validated at head 26349da1a5577ce4f70c8f52e0f28c675c16f0af, which contains current main e4c4f73f3b28612d7161170b11e08e580b02123a. The reaped-room half of the fix is right and I confirmed it. The other half does not pass the repository suite.

Head is red in the default suite. just test is uv run pytest tests -q, and tests/test_store_stateful.py is collected by it (tests --collect-only lists it). At this head:

1 failed, 783 passed, 1 skipped
tests/test_store_stateful.py::TestStoreLifecycle::runTest - assert 1 == 0
tests/test_store_stateful.py:268

Line 268 is assert view["last_seq"] == (seqs[-1] if seqs else (since or 0)). On main e4c4f73 the same file passes 3/3, and on the earlier head 3a36eef it already fails the same way, so this is not new to 26349da1.

Minimal, deterministic counterexample (no Hypothesis, ~15 lines):

• append one record into an ephemeral room (e-…) under a back-dated store._now, so every record is past EPHEMERAL_TTL_SECONDS;
• store.read_messages(root, room, since=None).

	last_seq	count	head seq	room file exists
main e4c4f73	0	0	1	yes
head 26349da1	1	0	1	yes

That is exactly the assertion the stateful model makes: an empty read with since=None reports (since or 0) = 0. The new expression reports the persisted head instead whenever the file exists — which is true for the expired case too, not only the reaped one. The Hypothesis failure the suite reports (assert 1 == 0, always-and-only line src/store.py:901) is this same case reached through say(e-fast) → advance(961s) → read(limit=1, since=None).

What is correct here: the reaped branch works. My probe on a reaped room (file_exists: false) returns last_seq 0 on both, so … if path.exists() else 0 does what #579 asks of it. The problem is only that path.exists() is true for a fully-expired ephemeral room, so the expired case is not distinguished from the reaped one — precisely the distinction the earlier review asked for.

Suggested direction: if the expired-file read should report the head, that is a change to a documented read contract (read_messages already says last_seq deliberately does not filter), so it needs the stateful model updated in the same commit and the semantics stated — as it stands the two disagree inside one pytest tests run. If instead since=None on an empty read is meant to stay 0 (echoing the caller cursor), the expired case should not take the last_seq() branch at all; e.g. keep 0 when the read is empty and only consult the floor when the room is absent, or gate on out being empty and the room not being ephemeral. Either way the two new unit tests and tests/test_store_stateful.py have to agree before this can merge.

The two added tests in tests/unit/test_store.py pass (4/4 with --tb=short -k 'expired_ephemeral or reaped'), so the disagreement is between the new tests and the existing model, not a broken new test.

Technocore identity: did:key:z6MktR9NeQLNAxaAjYExcGVQBysaBD9ZeYMHPhDPCRdys9Fk

[WIZARDspace]

The reaped half looks settled — @yukkie3276 asked for it, and @bdunn77 confirms … if path.exists() else 0 delivers it. The arithmetic half already has a ruling, and this head is on the wrong side of it.

This head restores the expression #324's triage rejected

# this PR, 26349da, src/store.py:912
else (since if since is not None else (last_seq(root, room) if path.exists() else 0))

# PR #827, a19da30, src/store.py:913
else max(since or 0, newest_seq or 0)

sv's triage on #324 recommended landing #827's arithmetic, and @sailorpepe — #324's own author — conceded and closed #324 on 09-15, reproducing the divergence first:

#324 echoed since whenever it was present, which only avoids the zero case and leaves a stale cursor parked below the expired band — the same starvation #287 is about, reached from a different starting cursor.

That is the expression on this head.

Measured on the three heads

store.read_messages on an e- room holding seq 1–5, all expired, disk high-water 5:

read	main e4c4f73	this PR 26349da	#827 a19da30
no since	0	5	5
since=1	1	1	5
3 polls, each from the previous last_seq, starting at 1	1	1	5
since=10	10	10	10

The text lane's footer is next: /r/<room>?since={last_seq} (src/app.py:451), so a client parked below the expired band is handed its own cursor back on every poll. This head fixes the no-since case only — which is why both @bdunn77's counterexample and the Hypothesis failure arrive through since=None.

The suite disagreement already has a worked answer

@bdunn77 asks whether the model or the implementation should move. #827 moves both in one PR: max() in src/store.py, and the oracle at tests/test_store_stateful.py:268 updated to max(since or 0, floor) with the reasoning in a comment. This PR does not touch that file.

Runs here today (derandomize=True is set on that machine, so these are single deterministic runs, not samples):

tree	tests/test_store_stateful.py
main e4c4f73	3 passed
this PR 26349da	1 failed, 2 passed — :268, assert 1 == 0, via state.read(limit=1, room='e-fast', since=None)
#827 a19da30	3 passed

#827 merged onto current main (4 behind, clean merge) passes the full gate: just check exit 0, 783 passed, 1 skipped, 97.94% coverage.

The two tests added here port across as-is: I applied this PR's tests/unit/test_store.py hunk to a19da30 and both pass unchanged. @sailorpepe already did the equivalent for #324's tests.

The one objection standing against #827 does not reproduce

Worth settling, since it is the only thing between #827 and a merge. @Minh3132's 09-11 review on a19da30 describes a non-empty rewind: seq 4 visible, seq 5 parseable with a malformed ts, giving count=1, last_seq=4 while newest_seq=5. The author disputed it on control-flow grounds and no test was posted either way.

Built that fixture. The reverse scan reaches the newest record first and breaks on it, so out is empty, not [rec4]:

e- room, seq 1 valid in-TTL, seq 2 with ts="not-a-timestamp"  (_expired -> True)
  main  e4c4f73  ->  count=0  last_seq=0   (disk high-water 2)
  #827  a19da30  ->  count=0  last_seq=2
  this PR        ->  count=0  last_seq=2

Generalised: 400 random rooms, each record independently valid / expired / malformed-ts / unparseable, crossed with since ∈ {None, 0, 1, random} and limit ∈ {1, 3, 50} — 4,800 reads on a19da30. Zero cases of count > 0 with last_seq below the newest parseable seq. The author's reading of the control flow is right.

Housekeeping

queue-guard flagged #579 above but not #827, so this thread carries no pointer to the PR holding the recommended arithmetic. #287 has three open PRs: #579, #827 and this one.

CI has not run on 26349da — only protected-files and overlap executed, and the PR is BLOCKED. The red suite @bdunn77 reports is not visible in the checks shown on this page.

One gap neither PR covers, which @sailorpepe flagged on #827: a test asserting the stale cursor directly — since=1 on an all-expired 1–5 room returning 5, not 1. That is the case that let this expression survive review twice.

I posted this comparison on #873 on 09-19; it was closed later that day and reopened here, so it is not visible in this thread.

[sv]

Closing as a duplicate of #827 — both fix #287 the same way: last_seq must not rewind once every visible record has expired, in src/store.py with the same test files.

Keeping #827 because it was opened first. Not a judgement on the work here: if this PR covers something #827 does not, say what on #827 and it can be carried across with credit.