fix(watch): mark read_at on live delivery so a later inbox.sh does not replay it#439
Open
Masashi-Ono0611 wants to merge 2 commits into
Open
fix(watch): mark read_at on live delivery so a later inbox.sh does not replay it#439Masashi-Ono0611 wants to merge 2 commits into
Masashi-Ono0611 wants to merge 2 commits into
Conversation
…lay it watch.sh's monitor delivery loop only advances the per-session watermark on delivery; it never touches read_at. Messages streamed live therefore stay read_at IS NULL forever, and a subsequent inbox.sh call (e.g. a respawned session's actas re-registration) re-surfaces the entire history as "new". Add a shared mark_read() helper (id validated as a bare integer before interpolation into SQL, matching inbox.sh's own defensive stance) and call it from both the normal delivery path and the ctrl:despawn control-row path, so a broad watcher's later inbox scan does not resurface a stale despawn control message either. Tests: new coverage in test_watch.bats (delivery marks read_at, inbox.sh does not re-report it) and test_despawn.bats (ctrl:despawn row is marked read too). Existing fixed-sleep timing in the new watch.sh test replaced with bounded polling to avoid CI flakiness. Full bats suite green (728+ cases).
Two gaps found by /review:self-multi-model (Codex + Fugu, 2026-07-19): - A broad (non-actas) watcher subscribes to every registered role in the project, so without scoping it would mark read_at for a role that has its OWN exclusive watcher — e.g. a leader's default SessionStart watcher racing the read state an actas'd member's exclusive watcher is responsible for. mark_read now skips when an exclusive ready sentinel exists for (team, to) and this watcher isn't it. - A pipe write to the Monitor stream can succeed (into a kernel buffer) even if the downstream reader is about to exit — there is no ack protocol here. Documented as a known best-effort limitation rather than papering over it; a real fix needs the claim/ack redesign tracked in fujibee#373. Also logs (rather than silently swallowing) a genuine SQLite update failure.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
inbox.shandcheck-inbox.shboth mark delivered rows'read_atbeforedisplaying them, but
watch.sh— the live Monitor-mode delivery path — nevertouched
read_atat all. A message delivered live through a watcher stayedread_at IS NULLforever, so a laterinbox.sh/check-inbox.shcall (or thehistory.shunread marker●) would surface it again as if it were stillunread, even though the recipient already saw it via the watcher.
This adds a small
mark_read()helper inwatch.shand calls it from both thenormal delivery branch (replacing the ambient assumption that display alone
was enough) and the
ctrl:despawncontrol-row branch, so control messageshandled by the watcher also stop lingering as unread.
Why this is a distinct fix from #338 / #373
ever attaches (never drained at all).
(a broader redesign).
are delivered live via an already-attached watcher, which is the most
common path, simply never persisting that fact to storage. Confirmed via
direct DB inspection and reading
watch.sh(noread_atreference existedanywhere in the file before this change).
Test plan
New tests:
tests/test_watch.bats("watch: marks a delivered message'sread_at so a later inbox.sh does not re-surface it") and
tests/test_despawn.bats("despawn: graceful — ctrl:despawn control row ismarked read (does not linger as unread)").
bats tests/*.bats— 728/729 passing; the sole failure(
install: watch.sh self-cleans a prior watcher on re-invocation for the same sid) is a pre-existing order-dependent flake unrelated to this diff,confirmed by running
tests/test_install.batsalone (46/46 pass).Cross-reviewed by two independent peers before submission (both found real
issues, since fixed): a not-yet-integer-validated
$idinterpolated intoSQL, the despawn control-row gap, and flaky fixed-
sleeptest timing(replaced with bounded polling helpers).
CI green
Update (2026-07-19): additional review pass, two more fixes
A follow-up
/review:self-multi-modelpass (Codex + Fugu, run independentlyof the peer review above) found two more real issues before this had any
maintainer eyes on it, now fixed in the latest commit:
(non-actas) watcher subscribes to every registered role in the project.
Without scoping, it would also mark
read_atfor a role that has its OWNexclusive watcher — e.g. a leader's default SessionStart watcher racing the
read state an actas'd member's exclusive watcher is responsible for.
mark_readnow skips when an exclusive ready sentinel exists for(team, to)and this watcher isn't it. New test:tests/test_watch.bats("a broad watcher does not mark read_at for a rolewith its own exclusive ready sentinel").
can succeed (accepted into a kernel buffer) even if the downstream reader
is about to exit — there's no ack protocol here. Marking
read_atimmediately after a "successful"
printftherefore isn't a deliveryguarantee. This is now documented as a known best-effort limitation in the
code rather than silently assumed away; a real fix needs the claim/ack
redesign already tracked in Atomic message consumption: prevent duplicate delivery and inject-failure loss with claim/ack #373 — out of scope for this small bugfix.
Also: a genuine SQLite update failure is now logged to stderr instead of
silently swallowed.
Full suite re-verified: 730/730 (this worktree's copy of the pre-existing
install: watch.sh self-cleans...flake did not reproduce on this run).