Skip to content

Two of the three mark-read paths discard the result: a message can be shown and stay unread, silently #1011

Description

@fujibee

Two of the three places that mark a message read discard the result, so a message can be displayed
and then remain unread
with nobody told. Found while measuring #1001; recording it separately
because it is not that issue's defect and it interacts with #677.

The sites

scripts/inbox.sh:89         storage_mark_read_batch … >/dev/null 2>&1 || true
scripts/check-inbox.sh:300  storage_mark_read_batch … >/dev/null 2>&1 || true
scripts/watch.sh:757,760    storage_read_cursor_consume …

The first two swallow both the output and the status. The third is the monitor path and is shaped
differently.

Why it matters now

Measured under an artificial writer holding BEGIN IMMEDIATE: a read-only command's wait was 100%
storage_init's schema batch failing with SQLITE_BUSY and being swallowed
(#1001). The fix in
flight removes that wait — but it does not change what happens to the actual mark-read write when a
genuine writer contends. That write can still lose, and || true means:

  • the operator sees the message printed;
  • the row stays unread;
  • the exit status is 0;
  • nothing is written anywhere.

The next inbox shows it again. That is benign-looking — a duplicate is not a loss — but it is the
mirror of the failure #677 reports, where a message was consumed without being shown. Here it is
shown without being consumed. Both come from the same place: mark-read and delivery are not tied
to each other's success.

The retry pattern already exists here

scripts/drivers/storage/sqlite.sh:274-282, the send path:

if ! printf '%s\n' "$insert" | agmsg_sqlite -bail "$db" >/dev/null 2>&1; then
  storage_init "$team" >/dev/null
  printf '%s\n' "$insert" | agmsg_sqlite -bail "$db" >/dev/null 2>&1 || return 1
fi

Write, and on failure initialise and write again — and return non-zero if it still fails. The
pattern was built for #114 and applied to the send path only. The read paths kept || true.

What is not established

  • Whether this fires in practice. The measurement above forced contention artificially; nobody
    has caught a real mark-read losing to a real writer. The failure is silent, so an absence of
    reports is not evidence of absence — which is itself part of the problem.
  • Whether watch.sh's consume has the same hole. It is a different call with different failure
    handling and was not examined.

What would settle it

Instrument the two call sites to report a failed mark-read (to stderr, or as an event), then look at
whether it ever appears. That is small, and it converts "we would not know" into "we would". Applying
the #114 retry is the fix if it does fire; adding a retry to something never observed to fail would
be the wrong order.

Related: #677 (the same seam, opposite direction), #1001 (how this was found), #114 (where the
pattern comes from).

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions