Skip to content

[SLOP(claude-opus-4-8-high)] feat(ups): table-backed postgres transport with coalesced doorbell#5335

Open
MasterPtato wants to merge 1 commit into
stack/slop-claude-opus-4-8-high-feat-universaldb-postgres-leader-resolver-driver-overhaul-xrrmomwzfrom
stack/slop-claude-opus-4-8-high-feat-ups-table-backed-postgres-transport-with-coalesced-doorbell-xutxrzrv
Open

[SLOP(claude-opus-4-8-high)] feat(ups): table-backed postgres transport with coalesced doorbell#5335
MasterPtato wants to merge 1 commit into
stack/slop-claude-opus-4-8-high-feat-universaldb-postgres-leader-resolver-driver-overhaul-xrrmomwzfrom
stack/slop-claude-opus-4-8-high-feat-ups-table-backed-postgres-transport-with-coalesced-doorbell-xutxrzrv

Conversation

@MasterPtato

Copy link
Copy Markdown
Contributor

No description provided.

@MasterPtato

MasterPtato commented Jun 25, 2026

Copy link
Copy Markdown
Contributor Author

Stack for rivet-dev/rivet

Current stack:

Dependencies:

Get stack: forklift get 5335
Push local edits: forklift submit
Merge when ready: forklift merge 5335

@claude

claude Bot commented Jun 25, 2026

Copy link
Copy Markdown
Contributor

Code Review: Table-backed Postgres Transport with Coalesced Doorbell

The architectural direction here is solid. Moving broadcast payloads from NOTIFY (8KB cap, transient) to a table (unbounded, poll-backstopped) eliminates the base64 encoding workaround, removes the per-subject LISTEN proliferation, and makes delivery correctness independent of whether a NOTIFY arrives. Good change overall. A few issues worth addressing:

Bug: existing.subscribe() will not compile (line 432)

scc::hash_map::OccupiedEntry does not implement Deref<Target = V>, so existing.subscribe() on an OccupiedEntry won't resolve to broadcast::Sender::subscribe(). Should be existing.get().subscribe().

Resource leak: Doorbell task runs forever with no cancellation path

Doorbell::new spawns a task holding Arc<Doorbell>. The task runs an unbounded loop with no cancellation token. When PostgresDriver is dropped, the task continues holding Arc<Doorbell> (and transitively Arc<Pool>), keeping the connection pool alive. Per repo guidelines: spawned futures capturing heavy resources must have a guaranteed completion path via a CancellationToken.

Semantic change: ups_queue_messages is now UNLOGGED

The diff changes ups_queue_messages from a logged to an UNLOGGED table. On a Postgres crash, all in-flight queue messages are silently lost. The broadcast table being UNLOGGED is intentional (matching NATS at-most-once), but queue subscribers typically expect at-least-once delivery. This is a behavioral regression worth calling out explicitly.

Missing index on created_at for broadcast GC

The GC query DELETE FROM ups_messages WHERE created_at < NOW() - (...) has no index on created_at — only (subject_hash, id) is indexed. This forces a sequential scan every 5 seconds. An index on created_at is needed. Also consider a LIMIT clause to bound each GC pass under high load.

Minor: cleanup task UNLISTEN race

In spawn_shard_cleanup_task, after token.cancelled() fires and receiver_count() == 0 is confirmed, there is a window before remove_async completes where a concurrent ensure_shard_listen can subscribe to the Occupied entry and receive a receiver for a sender that then disappears from shard_subscriptions. The new subscriber won't receive doorbell wakeups until the 1-second poll backstop. Low severity since the poll backstop covers it.

Nit: double-hashing in shard_for

shard_for takes a subject_hash string (already hex-encoded hash) and hashes it again via DefaultHasher. Simpler to parse the hex digits directly and reduce mod 32.

Nit: quoted numeric literal in SET

format!("SET idle_in_transaction_session_timeout = '{}'", LISTEN_IDLE_IN_TRANSACTION_TIMEOUT_MS)idle_in_transaction_session_timeout accepts an unquoted integer. Removing the quotes is more idiomatic. No injection risk since it's a compile-time constant either way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant