fix(event_bus): dispatch a queue's message to its owning subscriber only - #471
Conversation
Every subscriber has its own queue, so a topic with N matching subscribers puts N copies of each event on the bus. Consumer#handle_message resolved handlers by pattern alone, ignoring which queue the message came from, so each copy fanned out to all N handlers — every handler ran N times per event, and across hosts could run concurrently: the two-phase claim reads a pending row as "prior holder crashed, re-run", which is also what a handler still running elsewhere looks like. Registry#handlers_for now takes a required queue_name: and selects on ownership AND pattern. The keyword is required so a keyword-less call cannot silently restore the fan-out; the two queue-less callers (Testing.inline! publish, EventStore#drain!) move to Registry#subscribers_matching. A message on a queue nobody owns is still archived, but now warns once per queue per process and emits pgbus.event_unrouted every time. Closes #469 Claude-Session: https://claude.ai/code/session_015yNc4hDgowAEZVTmWKMANs
|
Running ultrareview automatically — This PR changes the core event-dispatch rule from pattern-only to owner-only, which directly affects whether each handler runs once per event or fans out to all matches; a subtle bug here could silently drop or duplicate events across every subscriber.... I'll post findings when complete. |
|
cubic can't run this ultrareview because your workspace has reached its monthly review limit. cubic has reviewed 121,696 of the 120,000 allowed lines of code this month. Reviews resume on 10 October 2026 (in 23 days). You've reached your flex budget. Increase your flex budget to resume reviews now, or learn how flex capacity spend limits work. To help optimise your usage, you can tune cubic to get the most out of your usage limits:
|
Summary
Pgbus::Process::Consumer#handle_messageread a message from one subscriber's queue and then resolved handlers withRegistry#handlers_for(routing_key)— a pattern match that ignored which queue the message came from — and ran every match.Each subscriber owns its own queue (
Subscriber#setup!→ensure_queue+bind_topic), so a topic with N matching subscribers puts N copies of every event on the bus. Each copy then fanned out to all N handlers: every handler ran N times per event. With four"#"subscribers, every handler was invoked four times for every event.idempotent!was the only thing hiding this, and it hides it imperfectly by design. The two-phase claim (#385) deliberately re-runs when the existing claim is still pending, on the theory that the prior holder crashed — but a pending claim is also exactly what a handler still running on another host looks like. Host A wins the claim and starts a 700ms handler; host B reads a different subscriber's copy of the same event, dispatches to that same handler, loses the claim insert, readscompleted_at IS NULLas "crashed, re-run", and executes it concurrently.Non-idempotent handlers simply ran N times with no guard at all.
Key changes
lib/pgbus/event_bus/registry.rb—handlers_for(routing_key, queue_name:)selects ons.queue_name == queue_name && matches?(s.pattern, routing_key).queue_name:is required, not defaulted, so a keyword-less call cannot silently restore the fan-out. Newsubscribers_matching(routing_key)carries the pattern-only view under an explicit name.lib/pgbus/process/consumer.rb— passesqueue_nameinto the lookup; newreport_unroutedbranch warns once per queue per process (Concurrent::Set#add?), emitspgbus.event_unroutedon every occurrence, and archives as before.lib/pgbus/event_bus/publisher.rb,lib/pgbus/testing.rb— the two queue-less callers (theTesting.inline!publish path andTesting::EventStore#drain!) move tosubscribers_matching.docs/app/views/docs/pages/event_bus.rb,lib/pgbus/instrumentation.rb, CHANGELOG — the delivery contract and the new instrumentation event.Two handlers registered against the same explicit
queue_name:still both run on that queue's delivery. The pattern check is kept alongside the ownership check because a routing key the owner's pattern doesn't match means a stalepgmq.topic_bindingsrow, and a stale binding must not run the handler.Closes #469
Test plan
New specs (written first, confirmed RED on arity / wrong handler invoked):
spec/pgbus/event_bus/registry_spec.rb—#handlers_forqueue_name:[]when the owner's pattern doesn't match (stale binding)[]for a queue no subscriber ownsArgumentErroron a keyword-less callspec/pgbus/process/consumer_spec.rb—handle_messageRegistry: two subscribers both matchingorders.#on separate queues, message read from the first's queue → only the first handler'sprocessrunshandlers_forreceives the queue name (both the headers and top-levelrouting_keyshapes)pgbus.event_unroutedwithqueue_name:/routing_key:Gates run:
bundle exec rspec spec/pgbus/event_bus/registry_spec.rb spec/pgbus/process/consumer_spec.rb spec/pgbus/process/consumer_priority_spec.rb— greenbundle exec rspec spec/pgbus/ spec/generators/(CI's unit job) — 4450 examples, 0 failures, 1 pendingbundle exec rspec spec/requests spec/pgbus_spec.rb spec/i18n_spec.rb— 115 examples, 0 failuresbundle exec rubocop— clean (615 files)bundle exec rubocopindocs/on the changed page — cleanDeviations & judgment calls
Discoveries
handlers_forhad two callers the issue did not name:EventBus::Publisher#publish(theTesting.inline!path) andTesting::EventStore#drain!. Neither has a queue — in test mode the event never reaches PGMQ. Both want the pattern view, and under owner-only dispatch the per-subscriber delivery count is identical (one invocation per matching subscriber), so both moved tosubscribers_matchingrather than being handed a synthetic queue name.Judgment calls
docs/app/views/docs/pages/event_bus.rbis the canonical event-bus guide and its "Topic routing" section would otherwise still imply pattern fan-out. Same paragraph in both.pgbus.event_unroutedadded to thelib/pgbus/instrumentation.rbcatalog comment. Not requested, but that comment is what the README points at for the fullpgbus.*list.Concurrent::Setfor the warned-queues set, notSet+ a mutex.handle_messageruns on the execution pool, so this is touched from several threads, andadd?is the atomic test-and-set the once-per-queue guarantee needs in a single call.concurrent-rubyis already required at the top ofconsumer.rb.archive_message,record_success,record_stat("success")— rather than introducing a distinct stat status. An unowned queue is an operator problem, not a queue failure, and a new status value would change the shape ofpgbus_job_statsrows.Verification deviation
bundle exec rspec(whole suite) segfaults locally inpuma/reactor.rbon Ruby 3.4.2 / arm64, at a spec boundary that moves between runs. Verified it reproduces identically on a cleanmain(git stash+ rerun, same crash), so it is environmental and pre-existing, not from this change. Gated instead on the exact sets CI runs, listed above.spec/systemandspec/integration/streamssegfault onmaintoo and run on Linux in CI.https://claude.ai/code/session_015yNc4hDgowAEZVTmWKMANs
Summary by cubic
Fixes event dispatch so a queue's message runs only that queue's owning subscriber, stopping N-times-per-event handler fan-out on wildcard topics. Previously a message read from one subscriber's queue was handed to every handler whose pattern matched, so with N matching subscribers every handler ran N times per event — and invoked concurrently across hosts when an
idempotent!claim was still pending.Registry#handlers_fornow requiresqueue_name:and selects on queue ownership plus pattern; two handlers sharing an explicitqueue_name:still both run.Testing.inline!,EventStore#drain!) use the newRegistry#subscribers_matching.pgbus.event_unroutedon every occurrence.Migration
handlers_forcallers must passqueue_name:; the pattern-only view issubscribers_matching.Written for commit c8c309a. Summary will update on new commits.