fix(gateway): isolate per-subscription failures in kanban notifier - #59278
AlexFucuson9 wants to merge 1 commit into
Conversation
The kanban notifier _collect() loop iterates subscriptions without per-subscription error handling. When claim_unseen_events_for_sub raises for one subscription (e.g. DB corruption, lock contention), the entire tick aborts — silently blocking delivery for ALL other subscriptions. Wrap the per-subscription logic in try/except so one bad subscription logs a warning and continues to the next, instead of jamming the entire notifier. Closes NousResearch#59269
teknium1
left a comment
There was a problem hiding this comment.
Thank you for the focused notifier-isolation fix. The current collection loop has no per-subscription handler at gateway/kanban_watchers.py:254-293, so the proposed try/except directly addresses a real failure boundary.
Problems
tests/gateway/test_kanban_notifier.py:326-332creates the successful subscription before the failing one.hermes_cli/kanban_db.py:8692selects subscriptions withoutORDER BY; when that scan returns insertion order, current main sends the good notification before the bad claim raises, so this test passes without the fix.
Suggested changes
- Make the failing subscription deterministically appear first (for example by controlling
list_notify_subsin the test), then assert the good subscription is delivered. That makes the regression fail on current main and verifies continuation after a failure.
Automated hermes-sweeper review.
| # Create two tasks with subscriptions and complete both. | ||
| conn = kb.connect() | ||
| try: | ||
| tid_good = kb.create_task(conn, title="good task", assignee="worker") |
There was a problem hiding this comment.
list_notify_subs() uses an unordered SELECT *, and this inserts the good row before the failing row. On an insertion-order scan, current main sends the good event before the bad claim aborts the tick, so this assertion passes without the fix. Please force the bad subscription to be returned first so the test proves continuation after failure.
…cted e2e coverage Follow-ups from review of salvaged PRs #59278 and #62712: * test_kanban_notifier_isolates_per_subscription_failure previously created the good subscription first; list_notify_subs() has no ORDER BY, so the good delivery happened before the bad claim raised and the test passed even without the isolation fix. The bad task is now created first AND a deterministic-order shim forces the failing subscription to be iterated first, so the test fails on the old whole-tick-abort behavior. * New test_notifier_delivers_block_loop_detected_triage_ping: drives a block_loop_detected event through one notifier tick end-to-end, asserting the triage ping reaches the adapter and the cursor advances (the sweeper review of #62712 flagged that only DB-level emission was tested).
…cted e2e coverage Follow-ups from review of salvaged PRs #59278 and #62712: * test_kanban_notifier_isolates_per_subscription_failure previously created the good subscription first; list_notify_subs() has no ORDER BY, so the good delivery happened before the bad claim raised and the test passed even without the isolation fix. The bad task is now created first AND a deterministic-order shim forces the failing subscription to be iterated first, so the test fails on the old whole-tick-abort behavior. * New test_notifier_delivers_block_loop_detected_triage_ping: drives a block_loop_detected event through one notifier tick end-to-end, asserting the triage ping reaches the adapter and the cursor advances (the sweeper review of #62712 flagged that only DB-level emission was tested).
|
Merged via PR #72236 — your commit was cherry-picked onto current main with your authorship preserved. Your per-subscription isolation fix was the core of the jam repair (#59269); we hardened the regression test on top (the original passed without the fix due to subscription ordering) and added a cursor-snap fix for the related boot-storm. Thanks! |
…cted e2e coverage Follow-ups from review of salvaged PRs NousResearch#59278 and NousResearch#62712: * test_kanban_notifier_isolates_per_subscription_failure previously created the good subscription first; list_notify_subs() has no ORDER BY, so the good delivery happened before the bad claim raised and the test passed even without the isolation fix. The bad task is now created first AND a deterministic-order shim forces the failing subscription to be iterated first, so the test fails on the old whole-tick-abort behavior. * New test_notifier_delivers_block_loop_detected_triage_ping: drives a block_loop_detected event through one notifier tick end-to-end, asserting the triage ping reaches the adapter and the cursor advances (the sweeper review of NousResearch#62712 flagged that only DB-level emission was tested).
…cted e2e coverage Follow-ups from review of salvaged PRs NousResearch#59278 and NousResearch#62712: * test_kanban_notifier_isolates_per_subscription_failure previously created the good subscription first; list_notify_subs() has no ORDER BY, so the good delivery happened before the bad claim raised and the test passed even without the isolation fix. The bad task is now created first AND a deterministic-order shim forces the failing subscription to be iterated first, so the test fails on the old whole-tick-abort behavior. * New test_notifier_delivers_block_loop_detected_triage_ping: drives a block_loop_detected event through one notifier tick end-to-end, asserting the triage ping reaches the adapter and the cursor advances (the sweeper review of NousResearch#62712 flagged that only DB-level emission was tested).
…cted e2e coverage Follow-ups from review of salvaged PRs NousResearch#59278 and NousResearch#62712: * test_kanban_notifier_isolates_per_subscription_failure previously created the good subscription first; list_notify_subs() has no ORDER BY, so the good delivery happened before the bad claim raised and the test passed even without the isolation fix. The bad task is now created first AND a deterministic-order shim forces the failing subscription to be iterated first, so the test fails on the old whole-tick-abort behavior. * New test_notifier_delivers_block_loop_detected_triage_ping: drives a block_loop_detected event through one notifier tick end-to-end, asserting the triage ping reaches the adapter and the cursor advances (the sweeper review of NousResearch#62712 flagged that only DB-level emission was tested).
Summary
Fixes the kanban notifier getting permanently stuck when one subscription fails — blocking delivery for ALL other subscriptions.
Root Cause
The
_collect()loop in_kanban_notifier_watcheriterates subscriptions without per-subscription error handling. Whenclaim_unseen_events_for_subraises for one subscription (e.g. DB corruption, lock contention, transaction rollback failure), the entire_collect()call aborts, which means:exceptlogs "kanban notifier tick failed: cannot rollback - no transaction is active" — masking the real errorFix
Wrap the per-subscription logic inside the
for sub in subs:loop in atry/exceptblock. One bad subscription now logs a warning and continues to the next, instead of aborting the entire tick.Testing
test_kanban_notifier_isolates_per_subscription_failure— verifies that when one subscription raises duringclaim_unseen_events_for_sub, the other subscription still gets deliveredCloses #59269