fix(gateway): single-owner gate + zero-sub early exit for kanban notifier - #63001
fix(gateway): single-owner gate + zero-sub early exit for kanban notifier#63001benegessarit wants to merge 1 commit into
Conversation
…fier The kanban notifier watcher had two gaps its own comment claimed were already handled: - Every gateway process on the machine polled every board DB each tick. dispatch_in_gateway defaults to true, so N gateways meant N concurrent pollers per board — exactly the -shm/-wal contention the dispatcher's singleton lock exists to prevent, but the notifier had no equivalent. - Boards with zero subscriptions were still opened writable every tick (connect() runs schema init/migration on first open per process), churning WAL state on DBs with nothing to deliver. Three changes in gateway/kanban_watchers.py: - Notifier singleton lock: acquire .notifier.lock (beside the dispatcher's .dispatcher.lock at the machine-global kanban root) via the existing _acquire_singleton_lock helper, after the env + dispatch_in_gateway short-circuits so those still win. Contended → this gateway polls nothing; unavailable → config-only fallback, mirroring the dispatcher's branches. Released on cancellation and on loop exit (the mid-sleep return became break so the single post-loop release covers it). - Zero-sub early exit: probe each board with the new read-only count_notify_subs before connect(); zero subscriptions skips the writable open entirely. A failed probe falls back to the writable open — delivery is never lost to the cheap check. - The stale gate comment (which described a dispatch-owner gate that never existed) now documents the real gate order. hermes_cli/kanban_db.py gains count_notify_subs: mode=ro URI open, missing DB or legacy table-less DB counts 0 without creating/migrating anything, WAL-uncheckpointed rows visible, sqlite3.Error propagated so callers pick the fallback. The singleton-lock helper docstrings now cover both loops. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
teknium1
left a comment
There was a problem hiding this comment.
Thanks for targeting the real writable-open and concurrent-poller cost: current main does reach connect() for every discovered board at gateway/kanban_watchers.py:234.
Problems
- The global lock can drop notifications from standalone profile gateways. The contender returns before collection at PR
gateway/kanban_watchers.py:180-186, but a lock-winning process only has other profiles' adapters in multiplex mode (gateway/run.py:8455-8471). Profile routing deliberately fails closed without that local registry (gateway/authz_mixin.py:50-57). A beta-owned subscription therefore remains undelivered if the default process wins the lock. - Cleanup does not cover all cancellation points. The lock is acquired at PR line 179, while the initial sleep (226) and between-tick sleep (623) are outside the
CancelledErrorcleanup block at 612-616.
Suggested changes
- Scope ownership so a lock holder can actually route every profile it suppresses, and add a two-standalone-profile delivery regression.
- Use an outer
try/finallyfrom lock acquisition through every await, with cancellation tests for both sleep sites.
Automated hermes-sweeper review.
| self._kanban_notifier_lock_handle = None | ||
| _lock_path = _kb.kanban_home() / "kanban" / ".notifier.lock" | ||
| _lock_handle, _lock_state = _acquire_singleton_lock(_lock_path) | ||
| if _lock_state == "contended": |
There was a problem hiding this comment.
Returning here suppresses a standalone profile gateway that may be the only process holding its profile's adapter. The winning process only has secondary adapters in multiplex mode, and _authorization_adapter fails closed when that registry entry is absent. Please preserve delivery for a beta-owned subscription when a default-profile process acquires this lock first.
| # `.dispatcher.lock` lets a gateway notify without dispatching. | ||
| self._kanban_notifier_lock_handle = None | ||
| _lock_path = _kb.kanban_home() / "kanban" / ".notifier.lock" | ||
| _lock_handle, _lock_state = _acquire_singleton_lock(_lock_path) |
There was a problem hiding this comment.
Please wrap the acquired handle in an outer try/finally covering the initial delay and all between-tick sleeps. The current CancelledError handler starts only inside the tick body, so cancellation at the initial await asyncio.sleep(5) or a later sleep leaks this lock until process exit.
Salvaged from PR #63001 (reduced scope): probe each board with the new read-only kanban_db.count_notify_subs() before the writable connect(), so boards with zero subscriptions are never opened writable on the 5s notifier tick (no schema migration, no WAL/-shm sidecar churn, no checkpoints). The PR's machine-global .notifier.lock singleton gate was deliberately NOT salvaged: a lock-winning default-profile gateway cannot deliver a secondary profile's subscriptions in standalone-profile deployments (profile routing fails closed in _authorization_adapter), so the lock could suppress delivery entirely. The probe captures the per-tick cost win without that regression.
Salvaged from PR #63001 (reduced scope): probe each board with the new read-only kanban_db.count_notify_subs() before the writable connect(), so boards with zero subscriptions are never opened writable on the 5s notifier tick (no schema migration, no WAL/-shm sidecar churn, no checkpoints). The PR's machine-global .notifier.lock singleton gate was deliberately NOT salvaged: a lock-winning default-profile gateway cannot deliver a secondary profile's subscriptions in standalone-profile deployments (profile routing fails closed in _authorization_adapter), so the lock could suppress delivery entirely. The probe captures the per-tick cost win without that regression.
|
Merged via PR #72236 — your zero-sub early-exit commit was cherry-picked onto current main with your authorship preserved. The single-owner gate half was NOT taken: the sweeper review's concern held (a global lock would drop notifications from standalone profile gateways), and profile-scoped routing landed separately via #72241. Thanks! |
Salvaged from PR NousResearch#63001 (reduced scope): probe each board with the new read-only kanban_db.count_notify_subs() before the writable connect(), so boards with zero subscriptions are never opened writable on the 5s notifier tick (no schema migration, no WAL/-shm sidecar churn, no checkpoints). The PR's machine-global .notifier.lock singleton gate was deliberately NOT salvaged: a lock-winning default-profile gateway cannot deliver a secondary profile's subscriptions in standalone-profile deployments (profile routing fails closed in _authorization_adapter), so the lock could suppress delivery entirely. The probe captures the per-tick cost win without that regression.
Problem
The kanban notifier watcher (
gateway/kanban_watchers.py) had two gaps that its own gate comment claimed were already handled:dispatch_in_gatewaydefaults to true, so every gateway process on the machine polled every board DB each tick. N gateways = N concurrent pollers per board — exactly the-shm/-walcontention the dispatcher's singleton.dispatcher.lockexists to prevent, but the notifier had no equivalent. (The comment at the top of the notifier described a "dispatch-owning gateway" gate that never existed.)connect()runs schema init + idempotent migration on first open per process, so boards with nothing to deliver still got writable opens and WAL churn on every poll.Fix
Three changes in
gateway/kanban_watchers.py:.notifier.lock— beside the dispatcher's.dispatcher.lockat the machine-global kanban root — via the existing_acquire_singleton_lockhelper, placed after theHERMES_KANBAN_DISPATCH_IN_GATEWAYenv override andkanban.dispatch_in_gatewayconfig short-circuits so those still win. The three lock states mirror the dispatcher's handling verbatim:contended→ this gateway logs and polls nothing;held→ handle kept for process lifetime;unavailable(filesystem can't flock) → warn and proceed on config control alone, i.e. today's behavior. Released onasyncio.CancelledErrorand on loop exit (the mid-sleepreturnbecamebreakso the single post-loop release covers both paths). A separate lock file — rather than reusing.dispatcher.lock— lets a gateway notify without dispatching._kb.connect(board=slug), probe the board with the new read-onlycount_notify_subs; zero subscriptions skips the writable open entirely. If the probe raises, the code logs and falls back to the writable open — delivery is never lost to a failed cheap check.hermes_cli/kanban_db.pygainscount_notify_subs(db_path=None, *, board=None):mode=roURI open; a missing DB or a legacy DB predating the subscriptions table counts as 0 without creating or migrating anything; rows in a not-yet-checkpointed WAL are visible (a freshly added subscription is never missed);sqlite3.Errorpropagates so callers choose their fallback. One honest-docs note: a read-only open of a WAL database can still create-shm/-walsidecars (verified empirically on SQLite 3.47.1) — the docstring states the real guarantees (no DB creation, no migration, no writable open) rather than the folk claim thatmode=roavoids sidecars.Known residual, mirrored deliberately from the dispatcher: a cancellation landing exactly in a between-tick sleep slice releases the flock only at process exit. Restructuring both loops is a separate change.
Tests
Two new files, 11 tests, fixture styles copied from the existing notifier/dispatch-lock suites:
tests/gateway/test_kanban_notifier_owner_gate.py(6): a zero-sub board is neverconnect()-ed and nothing is delivered; a subscribed board still delivers through the gate; a lock-losing instance calls neitherlist_boards,count_notify_subs, norconnectand delivers nothing; two concurrent instances → exactly one delivery; the lock is released on watcher return.tests/hermes_cli/test_kanban_count_notify_subs.py(5): missing DB counts 0 and creates nothing; board-path resolution; WAL-uncheckpointed rows visible to the read-only probe; legacy table-less DB counts 0 and stays unmigrated; explicitdb_pathoverridesboard.Existing regression suites (
test_kanban_notifier,test_kanban_notifier_watcher_dispatch_gate,test_kanban_watchers_mixin,test_kanban_notify,test_kanban_dispatch_lock): 41 passed combined in one process (also exercises sequential watcher runs against the lock). Broad sweeptests/hermes_cli -k "kanban or notify": 18 failures, every one reproduced identically at the base commit with this change stashed (sorted failure lists diff empty) — pre-existing, zero new failures.🤖 Generated with Claude Code