fix(#2593): mark forge notifications read on broker-delivery

This commit is contained in:
damocles 2026-07-19 18:21:04 +02:00 committed by mara
commit da056d0043
3 changed files with 104 additions and 212 deletions

View file

@ -91,66 +91,48 @@ unread-only filter), formats each notification as a broker
`Wake { from: "forge" }` message, and delivers it to the agent's own
inbox so claude's normal turn loop picks it up.
### Mark-read on read, not on delivery
### Mark-read on delivery
Delivered conversation threads are deliberately left **unread** in
forge. The hive-forge read-before-comment guard keys off forge's own
notification read-state (`GET /notifications?all=false`) to refuse a
comment when a thread has unread activity by others — so the agent
reading the thread via the CLI (`hive-forge comments` / `view`,
which `PATCH`es `/notifications/threads/{id}`) is the single
mark-read point. If `forge_notify` marked threads read on delivery,
that unread signal would be consumed before the agent acts and the
guard could never fire.
On a **successful** broker delivery, `forge_notify` marks the thread
read on forge straight away (`PATCH /notifications/threads/{id}`). The
broker inbox is the durable work queue now — each delivered wake is a
sqlite row with its own ack lifecycle — so the forge unread flag no
longer needs to track whether the agent has *processed* a
notification. Clearing it on delivery keeps forge's unread set **tiny
by construction**: at rest it holds only threads that failed to
deliver plus whatever arrived since the last 30s poll.
Because a delivered thread stays unread, it reappears in every
`?all=false` poll. A **delivery-dedupe cursor** (thread id →
last-delivered `updated_at`) stops the same version from re-firing a
wake; a new comment bumps `updated_at` so genuinely new activity
re-delivers. The cursor is pure anti-spam, not a correctness oracle.
Each poll prunes it to the threads still in the unread set. A failed
wake delivery is left unread **and** out of the cursor, so it
resurfaces next tick.
That size property is the whole point. A container rebuild starts the
poller with no memory of what it delivered, re-scans `?all=false`, and
finds nothing stale — the delivered threads are already read on forge.
Forge's own read-state is thus the durable, cross-rebuild record of
what's been delivered; there is **no persisted cursor**. (This
replaced an earlier design that left threads unread and leaned on a
persisted dedup cursor: a rebuild that lost the cursor re-delivered the
entire still-unread backlog as fresh wakes — the notification flood of
#2593 / #2106.)
**Size bound:** the per-poll prune retains only ids present in the
single `limit=UNREAD_FETCH_LIMIT` (50) fetch page, so the cursor never
exceeds that many entries — it tracks the unread _window_, not the
all-time notification count. The fetch limit and the bound are the
same constant in `forge_notify.rs` (with a debug assertion), so a
future pagination change grows the ceiling visibly rather than
silently. This is why the cursor stays a small JSON field rather than
a db table — see the storage discussion on the tracker (issue 2117).
**Read-before-comment coupling, dropped on purpose.** The old design
left threads unread so the hive-forge read-before-comment guard (which
keys off forge unread-state) would force the agent to view a thread
before commenting. That coupling is gone: the broker wake already
carries the notification body, so *delivery is the read*. An agent that
wants the full thread still runs `hive-forge comments` / `view`; the
guard no longer blocks a first comment on a freshly-delivered thread.
The cursor is **persisted** as the `forge_cursor` field of the
harness's consolidated `hyperhive-harness.json` state file (atomic
tmp+rename, flushed only when it changed) and reloaded on boot, so a
container rebuild/restart doesn't re-deliver the whole currently-unread
backlog (#2106 — previously the in-memory-only cursor was lost on
restart and every old still-unread thread re-fired a wake). The poller
runs in the same harness process that owns that file, so it's one
daemon → one state file rather than a second json; both writers
(turn-loop fields + this cursor) go read-modify-write under a shared
lock so neither clobbers the other's fields. This is safe because a
thread is recorded **after** a successful broker delivery, and the
broker inbox is durable sqlite — so a persisted "delivered" entry can
never swallow a wake the agent never received. Crucially the cursor is
a private dedup mirror, **not** forge's read-state: it does not
reintroduce the read-before-comment coupling that ruled out the old
mark-read-on-delivery approach. A missing (first boot) or malformed
cursor degrades to empty — re-deliver the unread set once — never an
abort.
**In-process dedupe (tiny, ephemeral).** A single-process map (thread
id → last-delivered `updated_at`) guards the narrow window where a
mark-read call *transiently fails* and the thread reappears unread in
the next poll before its `updated_at` bumps — so a flaky PATCH doesn't
re-fire the wake. It is **not persisted** and resets on restart (forge
read-state covers the durable case). Each poll prunes it to the ids in
the single `limit=UNREAD_FETCH_LIMIT` (50) fetch page, so it can never
exceed that many entries (a debug assertion pins the invariant; the
fetch limit and the bound are the same constant). A failed *delivery*
is left unread and out of the map, so it resurfaces next tick.
Self-echo notifications (the agent's own writes, see below) are the
one path still marked-read directly (no read-before-comment value).
> Note: the unread list grows for threads the agent never reads via
> the CLI, since nothing else trims it. This does not affect guard
> correctness (the guard does a per-thread, repo-scoped query) nor
> wake delivery (Forgejo orders unread newest-first, so new activity
> always lands in the polled window). Bounding the unread list is a
> separate follow-up — explicit subscription management via a
> hive-forge CLI verb, rather than the poller second-guessing which
> repo watches to drop.
Self-echo notifications (the agent's own writes, see below) are marked
read directly without a delivery — same `mark_read` call, no wake.
### Activation gates (graceful no-ops)