Fix: seed subscriber bookmarks at startup so Reactions actually fire#551
Merged
Conversation
Reactions (side-effecting subscribers: run_debriefer, caution_drafter, caution_promoter, authority_revocation_holder, and any future ones) own no proj_* table, so unlike projections they have no migration to seed their projection_bookmarks row. Nothing else seeded it either. The worker's first advance therefore called read_bookmark, which raises MissingBookmarkError; the advance loop caught it, wrote a no-op failure UPDATE (no row to update), backed off, and retried forever. Every Reaction was silently inert in a real deployment, including the just-shipped kill-switch authority_revocation_holder. It went unnoticed because every subscriber test either calls apply() directly or hand-seeds the bookmark, so the production worker path was never exercised. Fix: add ensure_bookmarks(pool, names) and call it once from projection_worker_lifespan before the worker starts, for every registered subscriber. For a projection the row already exists so the INSERT is a no-op via ON CONFLICT DO NOTHING; for a reaction it creates the missing row so its first advance reads a cursor instead of raising. The projection "fail loud if the migration never landed" guard is preserved by test_projection_table_match, which still enforces every projection has its CREATE TABLE + bookmark insert. Tests (real Postgres): a bookmark-less reaction's advance raises MissingBookmarkError (reproduces the bug); after ensure_bookmarks the same worker-path advance delivers the event; the ensure is idempotent across a restart (does not rewind an advanced cursor); and entering the real projection_worker_lifespan leaves every registered reaction with a readable bookmark (pins the fix at the production seam so a refactor that drops the step regresses here). Also drops the now-stale bookmark.py em-dash allowlist entry (the edit scrubbed its last em-dash). Verified: pyright + ruff + tach clean, 5400 projection/worker/subscriber integration + architecture tests green. Co-Authored-By: Claude <noreply@anthropic.com>
Coverage report
Click to see where and how coverage changed
This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||||||||||||||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes a latent production bug: every event-driven Reaction (side-effecting subscriber) was silently inert in a real deployment, including the just-shipped kill-switch
authority_revocation_holder(#547).Root cause: Reactions own no
proj_*table, so unlike projections they have no migration to seed theirprojection_bookmarksrow, and nothing else seeded it. The worker's first advance callsread_bookmark→ raisesMissingBookmarkError→ the advance loop catches it, writes a no-op failure UPDATE (no row exists), backs off, and retries forever. The Reaction never fires.Why it went unnoticed: every subscriber test either calls
apply()directly or hand-seeds the bookmark, so the production worker path (drain/advance_subscriber_once→read_bookmark) was never exercised end to end.Fix
ensure_bookmarks(pool, names)(idempotentINSERT ... ON CONFLICT DO NOTHING).projection_worker_lifespanbefore the worker starts, for every registered subscriber. Projections no-op (row already seeded by their migration); Reactions get their otherwise-missing row.test_projection_table_match.Test plan
advance_subscriber_onceraisesMissingBookmarkError(real Postgres)ensure_bookmarks, the same worker-path advance delivers the eventprojection_worker_lifespanleaves every registered reaction with a readable bookmarkFollow-up
The shipped kill-switch (#547) and the in-flight consequence gate both depend on this. The consequence-gate branch will rebase onto this and drop its test-only bookmark seeding.
🤖 Generated with Claude Code