Conversation
|
🚅 Deployed to the rivet-pr-4486 environment in rivet-frontend
|
ec32bdf to
ae0f886
Compare
6570bf2 to
b46226c
Compare
PR Review:
|
| Severity | Issue |
|---|---|
| Critical | DefaultHasher used for persistent DB keys - not stable across processes/versions |
| Medium | No metrics update in queue subscription cleanup task |
| Medium | retain_sync inside async retain - TODO unresolved |
| Medium | No queue depth limit; unbounded growth if consumers are down |
| Low | return Ok(...) vs idiomatic expression form |
| Low | SQL NOTIFY injection pattern fragile (safe today, not future-proof) |
| Low | Unused anyhow! import if all usages replaced |
| Info | Memory metric counts topics not subscribers (expanded surface) |
| Info | Load-balance test 500ms window may be flaky on slow CI |
The DefaultHasher stability issue is the most important fix - it will silently corrupt queue routing in any multi-process deployment.
|
Code Review summary for feat(ups): implement queue subs -- The implementation is well-structured and the test coverage is solid. Two medium issues to address before merge: (1) retain_sync on scc::HashMap inside the memory driver GC task runs synchronously inside an async closure and can block the executor; (2) The SELECT and INSERT+NOTIFY in publish_to_queues are not wrapped in a transaction, creating a race where a message can be orphaned for up to 1 hour if the subscriber dies between the two operations. Minor items: one remaining anyhow! macro call in publish() should use .context() instead; the queue parameter is missing from the tracing span in queue_subscribe (fields(%subject) should also include queue); and the Cargo.lock version downgrade from 2.1.7 to 2.1.6-rc.1 appears to be a stacked-PR artifact. On test coverage: test_queue_subscribe_load_balance only checks total message count, not per-subscriber distribution -- this is the right choice to avoid flakiness but worth noting in a comment. Also missing a test for Postgres reconnection behavior (does not drain pending ups_queue_messages on reconnect). |
PR Review:
|
b46226c to
66ec30f
Compare
ae0f886 to
4e8c22e
Compare
e5b3f53 to
6af4511
Compare
97b9cfd to
3fc4f7f
Compare
6af4511 to
f975d35
Compare
3fc4f7f to
662fee6
Compare
5113d59 to
b0cc12a
Compare
715bec8 to
31da82c
Compare
31da82c to
85f7553
Compare
b0cc12a to
3d99705
Compare
3d99705 to
3fbc6dc
Compare
85f7553 to
626728f
Compare
626728f to
83ebe90
Compare
3fbc6dc to
0126cf8
Compare
Merge activity
|
0126cf8 to
c732915
Compare

Description
Please include a summary of the changes and the related issue. Please also include relevant motivation and context.
Type of change
How Has This Been Tested?
Please describe the tests that you ran to verify your changes.
Checklist: