fix(streams): make StreamQueue.record! independent of Rails schema-cache index resolution - #402
Merged
Merged
Conversation
…che index resolution
## Summary
record! used upsert(unique_by: :queue_name), which resolves the unique
index through the pool's schema cache. That cache latches a negative
data_source_exists? probe permanently while the table_exists? guard is a
live query — so one wrong first probe poisoned every subsequent record!
in the process ("No unique index found for queue_name", swallowed at
DEBUG) until restart. The registry write is now a raw
INSERT ... ON CONFLICT (queue_name) DO NOTHING on the model connection:
the gem's own migration owns the index, so there is nothing to resolve
and no schema-cache traffic leaves the first-broadcast hot path.
Failure logging is class-aware: ActiveRecordError stays DEBUG per
attempt; any other StandardError (the bug-signal class) logs at WARN
once per process, DEBUG thereafter.
## Test Coverage
- unit: raw INSERT SQL, no upsert machinery, WARN-once vs DEBUG split,
unchanged return/cache semantics
- integration: poisoned pool schema cache still registers (fails on the
old implementation), zero SCHEMA sql.active_record events with a warm
table_exists? memo, ON CONFLICT idempotency; integration harness now
bootstraps pgbus_stream_queues
## Verification
- [x] bundle exec rake rubocop passes (553 files)
- [x] bundle exec rspec spec/pgbus/ spec/generators/ — 3866 examples, 0 failures
- [x] integration suite — 196 examples, 0 failures
Refs #401
There was a problem hiding this comment.
All reported issues were addressed across 5 files
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
… warm the guard memo explicitly Review follow-ups (PR #402): - log_record_failure's check-and-set now runs under a class-level mutex so concurrent failures (coalescer flush thread + callers) cannot emit two WARNs; logging happens outside the critical section. - The poisoned-cache regression spec warms and asserts the table_exists? memo before poisoning, making the index-resolution target explicit rather than a side effect of the live-query guard.
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
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
Pgbus::StreamQueue.record!(app/models/pgbus/stream_queue.rb) no longer usesupsert(unique_by: :queue_name)— it writes via rawINSERT INTO … (queue_name) VALUES (…) ON CONFLICT (queue_name) DO NOTHINGon the model's connection. The unique index is owned by the gem's own migration, so there is nothing for Rails to resolve, and the pool schema cache's permanently-latched negativedata_source_exists?probe can no longer poison registration for the process lifetime.ActiveRecord::ActiveRecordError) log at DEBUG per attempt as before; any otherStandardError— the bug-signal class the old path'sArgumentErrorbelonged to — logs at WARN once per process, DEBUG thereafter.pgbus_stream_queues(mirroring the generator template) so the registry is exercised for real, and a newspec/integration/stream_queue_registry_spec.rbpins the fix.Closes #401
Acceptance criteria → proof
record!succeeds with a deliberately poisoned pool schema cachespec/integration/stream_queue_registry_spec.rbpoisons@data_sources["pgbus_stream_queues"] = falseexactly as the issue's repro; verified RED against the old implementation (fails) and GREEN with the fixindexes/data_source_exists?query fromrecord!sql.active_recordsubscription asserts zeroSCHEMA-named events duringrecord!once thetable_exists?memo is warmArgumentError→ one WARN then DEBUG, across different queue names;StatementInvalid→ DEBUG only, never WARNbackfill!/all_names/ cache-consistency semantics unchangedTest plan
bundle exec rspec spec/pgbus/ spec/generators/— 3866 examples, 0 failuresbundle exec rake rubocop— 553 files, no offensesspec/integration/stream_queue_registry_spec.rb— poisoned-cache regression fails on old code, passes on newDeviations & judgment calls
InsertAlltraffic from the flush thread, and the write is already once-per-stream-per-process via the@stream_indexes_createdmemo. Deferral would changeensure_stream_queuesemantics and belongs in its own PR if still wanted.ActiveRecord::ActiveRecordError→ DEBUG (coversStatementInvalidand connection errors likeConnectionNotEstablished, which are equally transient), any otherStandardError→ WARN once per process then DEBUG. The issue said "non-StatementInvalid", but that literal reading would WARN on transient connection drops.connection.quote/quote_table_nameinterpolation instead of the issue'ssanitize_sql([… ?])— in Rails 8.1sanitize_sql_arraychecks out a pool connection viawith_connection, which can't be stubbed in the DB-free unit suite; connection-level quoting is equivalent and testable through the sameconnectionseam the spec file already stubs.Summary by cubic
Makes
Pgbus::StreamQueue.record!write via raw INSERT…ON CONFLICT instead ofupsert(unique_by: :queue_name), removing dependence on Rails’ pool schema cache and preventing process-lifetime failures after a false negative probe.table_exists?memo,record!issues a single INSERT…ON CONFLICT DO NOTHING.ActiveRecord::ActiveRecordErrorlogs at DEBUG per attempt; any otherStandardErrorlogs at WARN once per process (latch serialized via a class-level mutex), then DEBUG.backfill!/all_namescache semantics are unchanged.pgbus_stream_queues.Written for commit cc3a2c3. Summary will update on new commits.