Skip to content

fix(kanban): snap notify-sub cursor to current MAX(id) at creation - #29915

Closed
briandevans wants to merge 2 commits into
NousResearch:mainfrom
briandevans:fix/kanban-notify-snap-cursor-29905
Closed

fix(kanban): snap notify-sub cursor to current MAX(id) at creation#29915
briandevans wants to merge 2 commits into
NousResearch:mainfrom
briandevans:fix/kanban-notify-snap-cursor-29905

Conversation

@briandevans

Copy link
Copy Markdown
Contributor

What does this PR do?

add_notify_sub previously inserted kanban_notify_subs rows with last_event_id = 0 (the schema default). A subscription created on an already-active task therefore replayed every prior terminal event on the next _kanban_notifier_watcher tick — issue #29905 reports a 100+ message burst at gateway boot caused by 27 stale subs at last_event_id=0.

After this PR, add_notify_sub snaps the cursor to COALESCE(MAX(id) FROM task_events WHERE task_id = ?, 0) inside the same write_txn as the INSERT, so a new sub starts caught up. Only events generated after subscription fire notifications. The snap is gated on cur.rowcount > 0, so the existing INSERT OR IGNORE semantics are preserved: a re-subscription against an existing (task, platform, chat, thread) tuple is still a no-op and never rewinds the advancing cursor.

Scope is limited to the root cause (the missing snap on creation). Existing subs already sitting at last_event_id=0 are out of scope here — the reporter's workaround SQL handles those, and a separate on-connect migration would be a larger, riskier change to bundle.

Audited siblings: add_notify_sub is the only production path that inserts into kanban_notify_subs (verified via rg \"INSERT.*INTO kanban_notify_subs\"). claim_unseen_events_for_sub / advance_notify_cursor / rewind_notify_cursor all read/write the existing cursor and need no change. No widening needed.

Related Issue

Fixes #29905

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)
  • ✨ New feature
  • 🔒 Security fix
  • 📝 Documentation update
  • ✅ Tests (adding or improving test coverage)
  • ♻️ Refactor
  • 🎯 New skill

Changes Made

  • hermes_cli/kanban_db.pyadd_notify_sub now snaps last_event_id to the current MAX(id) over task_events for the task after a newly-inserted row.
  • tests/hermes_cli/test_kanban_notify.py — two new regression tests: backlogged terminal events must NOT replay for a newly-created sub; fresh post-subscription events MUST still deliver; idempotent re-subscription MUST NOT rewind the cursor.
  • tests/hermes_cli/test_kanban_notify.py::test_notifier_skips_subscription_owned_by_other_profile — invariant updated from "cursor must equal 0" to "cursor must equal the snapped baseline" (same intent: wrong-profile watcher must not advance the cursor).
  • tests/hermes_cli/test_kanban_core_functionality.py::test_notify_claim_is_single_owner_and_rewindableold_cursor baseline is now the snapped value rather than the hard-coded 0; the rest of the single-owner / rewind invariants are unchanged.

How to Test

uv run --with pytest --with pytest-xdist --with pytest-asyncio --with pytest-timeout \
  python3 -m pytest tests/hermes_cli/test_kanban_notify.py \
                    tests/gateway/test_kanban_notifier.py \
                    tests/hermes_cli/test_kanban_db.py \
                    tests/hermes_cli/test_kanban_core_functionality.py -v

Manual repro per the issue: create a task, append a couple of terminal events directly to task_events, then call add_notify_sub — on main the next watcher tick fires once per backlogged event; with this PR no notifications fire until a new event arrives.

Checklist

Code

Documentation & Housekeeping

  • I've updated relevant documentation — N/A (internal DB helper; no public-facing config or docs key changed)
  • I've updated cli-config.yaml.example if I added/changed config keys — N/A
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — N/A
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide — N/A (pure SQLite/Python logic)
  • I've updated tool descriptions/schemas if I changed tool behavior — N/A

Copilot AI review requested due to automatic review settings May 21, 2026 16:18

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

This PR prevents notification subscriptions from replaying backlogged terminal task events by snapping new subscription cursors to the current task_events max id, and updates/extends tests to assert the new cursor semantics (including idempotent re-subscribe behavior).

Changes:

  • Update add_notify_sub to initialize last_event_id for newly-inserted subscriptions to MAX(task_events.id) for the task.
  • Adjust existing tests that previously assumed last_event_id == 0 to instead assert against the snapped cursor baseline.
  • Add regression tests covering “late subscribe” catch-up behavior and idempotent subscriptions not rewinding the cursor.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File Description
hermes_cli/kanban_db.py Snap new notify subscription cursor to MAX(task_events.id) to avoid replay storms.
tests/hermes_cli/test_kanban_notify.py Add/adjust tests validating cursor snapping and idempotent behavior.
tests/hermes_cli/test_kanban_core_functionality.py Update expectation to use snapped cursor as the baseline instead of 0.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +671 to +675
sub = kb.list_notify_subs(conn, tid)[0]
assert sub["last_event_id"] == max_id_before, (
"Expected new sub to start at the current MAX(id); got "
f"{sub['last_event_id']} vs {max_id_before}"
)
Comment thread tests/hermes_cli/test_kanban_notify.py Outdated
kinds=("completed", "blocked", "gave_up", "crashed", "timed_out"),
)
sub_before = kb.list_notify_subs(conn, tid)[0]
cursor_before = sub_before["last_event_id"]
Comment thread tests/hermes_cli/test_kanban_notify.py Outdated
Comment on lines +660 to +663
# Pre-populate task_events so a fresh sub created now would otherwise
# see them all on the next tick.
kb._append_event(conn, tid, kind="completed")
kb._append_event(conn, tid, kind="blocked")
@alt-glitch alt-glitch added type/bug Something isn't working P3 Low — cosmetic, nice to have comp/gateway Gateway runner, session dispatch, delivery labels May 21, 2026
@briandevans

Copy link
Copy Markdown
Contributor Author

@copilot All three findings addressed in commit e7727de8c:

  • tests/hermes_cli/test_kanban_notify.py:663,689,715_append_event calls now wrapped in with kb.write_txn(conn): per the function's documented "called from within an already-open txn" contract.
  • tests/hermes_cli/test_kanban_notify.py:672sub["last_event_id"] cast to int before == max_id_before.
  • tests/hermes_cli/test_kanban_notify.py:724,730sub_before/sub_after["last_event_id"] cast to int before numeric comparison, matching the convention used elsewhere in this file.

The two failing CI tests (test_browser_secret_exfil::test_allows_normal_url and test_browser_supervisor::test_supervisor_start_and_snapshot) are unrelated network/Chrome environment failures — they live in tests/tools/ and have no dependency on the kanban DB path touched by this PR.

@briandevans
briandevans force-pushed the fix/kanban-notify-snap-cursor-29905 branch from e7727de to c5df9ff Compare May 22, 2026 18:17
@briandevans
briandevans force-pushed the fix/kanban-notify-snap-cursor-29905 branch from c5df9ff to 73c9ed5 Compare May 24, 2026 21:17
@briandevans
briandevans force-pushed the fix/kanban-notify-snap-cursor-29905 branch from 73c9ed5 to 5f3490f Compare May 27, 2026 04:10
@briandevans
briandevans force-pushed the fix/kanban-notify-snap-cursor-29905 branch from 5f3490f to bdc03b5 Compare May 29, 2026 07:14
@briandevans
briandevans force-pushed the fix/kanban-notify-snap-cursor-29905 branch from bdc03b5 to d73e6c1 Compare May 30, 2026 07:11
@briandevans
briandevans force-pushed the fix/kanban-notify-snap-cursor-29905 branch from d73e6c1 to 2aee372 Compare May 31, 2026 01:12
@briandevans
briandevans force-pushed the fix/kanban-notify-snap-cursor-29905 branch from 2aee372 to 7afcba5 Compare May 31, 2026 21:19
@briandevans
briandevans force-pushed the fix/kanban-notify-snap-cursor-29905 branch from 7afcba5 to 150dd33 Compare June 2, 2026 22:14
@briandevans
briandevans force-pushed the fix/kanban-notify-snap-cursor-29905 branch from 150dd33 to 95b4d86 Compare June 3, 2026 13:15
@briandevans
briandevans force-pushed the fix/kanban-notify-snap-cursor-29905 branch from 95b4d86 to 1b27515 Compare June 5, 2026 12:17
`add_notify_sub` inserted rows with `last_event_id = 0` (schema default).
A sub created on an already-active task therefore claimed every prior
terminal event on the next `_kanban_notifier_watcher` tick. Issue NousResearch#29905
reports 100+ "✔ Kanban done" messages firing in a single burst when 27
subs sat at `last_event_id=0` across a gateway restart.

Snap the cursor to `MAX(id) FROM task_events WHERE task_id = ?` after
the INSERT OR IGNORE so newly-inserted subs start caught up; only events
generated AFTER subscription fire notifications. Existing rows are
unaffected — the snap runs only when `cur.rowcount > 0`, so re-subscribing
never rewinds an advancing cursor.

Regression coverage in `tests/hermes_cli/test_kanban_notify.py`:
pre-existing terminal events must NOT replay, fresh events MUST still
deliver, and idempotent resubscription MUST NOT move the cursor.
Address Copilot review on NousResearch#29915:

- Wrap `_append_event` calls in `with kb.write_txn(conn):` per the
  function's documented "called from within an already-open txn" contract.
- Cast `sub["last_event_id"]` to `int` before numeric comparisons —
  `sqlite3.Row` dict materialization may surface the column as `str`
  under some sqlite typing/coercion paths, in which case `> 0` raises
  `TypeError` and `== max_id_before` is silently False. The rest of the
  file already follows this `int()` cast convention.
@briandevans
briandevans force-pushed the fix/kanban-notify-snap-cursor-29905 branch from 1b27515 to 996895b Compare June 5, 2026 21:17
@briandevans

Copy link
Copy Markdown
Contributor Author

Closing to keep my queue focused — this P3 kanban notify-cursor snap has been idle ~22 days with no maintainer engagement and the kanban SQLite area is actively owned by other contributors. Happy to reopen if maintainers want it. Thanks!

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

Labels

comp/gateway Gateway runner, session dispatch, delivery P3 Low — cosmetic, nice to have type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

kanban_notify_subs storm at gateway boot when last_event_id=0

3 participants