Resurface hidden DMs when new messages arrive - #6320
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d4e7493e33
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
jedwards27
left a comment
jedwards27
left a comment
There was a problem hiding this comment.
Verdict: REQUEST CHANGES
Reviewed: 08eb46ef3c0894baa7e48d9229f45349751a4a57..d4e7493e33781f7e576c2dfe545a88e663bb7b6c (exact clean head d4e7493e33781f7e576c2dfe545a88e663bb7b6c)
Risk: high — this changes accepted-message side effects, durable DM visibility state, relay-signed replaceable snapshots, and Desktop channel-list invalidation. The happy path is coherent, but three correctness gaps can preserve the bug or make it permanent.
Blocking findings
-
A transient snapshot failure makes a successfully accepted message's resurface unrecoverable.
unhide_dm_recipientscommitshidden_at = NULLand returns only changed rows (crates/buzz-db/src/dm.rs:451-488). Snapshot publication happens afterward (crates/buzz-relay/src/handlers/side_effects.rs:3366-3397), and ingest only logs a publication error while accepting the already-stored message (crates/buzz-relay/src/handlers/ingest.rs:2979-2993). The next message finds no changed row, so it schedules no snapshot retry. The relay's kind30622can therefore continue saying the DM is hidden, and Desktop keeps filtering it (desktop/src-tauri/src/commands/channels.rs:226-252,383-386), including after restart.Please add a durable retry/reconciliation contract (for example, an outbox, worker, startup reconciliation, or another design that cannot lose this projection), with failure injection proving that a failed first publication later converges and removes the stale
htag. -
Concurrent resurfacing can leave a stale authoritative visibility snapshot.
publish_dm_visibility_snapshotseparately reads the hidden set, reads the previous kind30622, calculatesprevious + 1, then attempts replacement (crates/buzz-relay/src/handlers/side_effects.rs:3278-3340). Concurrent messages to two hidden DMs for one viewer can both choose the same timestamp while observing different intermediate hidden sets. One replacement then wins by event ordering rather than final database state;was_inserted == falseis treated as success. The database can say neither DM is hidden while the authoritative snapshot still contains one indefinitely.Please make mutation plus projection linearizable per viewer, or retry rejected/stale publication by re-reading current state and publishing a strictly newer snapshot. Add a barrier-controlled two-DM concurrency regression asserting that final DB state and kind
30622agree, and mutation-prove the guard. -
Supported kind
40002DM messages never trigger resurfacing.The relay authorizes and channel-scopes
KIND_STREAM_MESSAGE_V2as a message (crates/buzz-relay/src/handlers/ingest.rs:378-392,611-623), and Desktop explicitly treats it as human-visible/notifiable DM content (desktop/src/features/channels/isDmNotifiableKind.test.mjs:19-23). The new hook, however, runs only whenkind_u32 == KIND_STREAM_MESSAGE(crates/buzz-relay/src/handlers/ingest.rs:2979-2996). Both added relay tests send hard-coded kind9events (crates/buzz-test-client/tests/e2e_nostr_interop.rs:83-90). A hidden DM receiving kind40002therefore remains hidden despite new unread/notifiable activity.Please define the canonical resurface-trigger set deliberately (at minimum kinds
9and40002) and parameterize the live regression across it. Removing the40002branch should make the test fail.
Validation
At matching exact head and clean worktrees:
cargo test -p buzz-db: 105 passed, 0 failed; 185 Postgres-dependent tests ignored. There is no DB-backed regression for the new updater.cd desktop && pnpm test: 5,091 passed, 0 failed.- Focused Desktop notification regression: 1 passed; it proves kind
30622invalidates["channels"]without membership-detail invalidation. git diff --check: passed.- Exact-head GitHub Relay E2E, Desktop Core/build/smoke/integration, Rust lint/unit/security, cross-compile, and image jobs are green.
- Full local relay/Desktop package attempts exceeded ten minutes in one review lane and yielded no trustworthy final result; no claim is made from those attempts.
The new Desktop test records query invalidation only; it does not execute get_channels, render the sidebar, or prove unread state. After the fixes, acceptance evidence should exercise a real relay-backed Desktop journey: hide → inbound message → sidebar row returns with correct unread count/indicator and Inbox consistency, sender preference remains hidden, an unrelated/no-message DM remains hidden, plus reconnect/restart and kind 40002. Include 1:1 and group DM coverage. No native/browser workflow artifact was provided for that user-visible state transition.
The actor attribution, community scoping, active-member filtering, relay-signed viewer ownership tags, duplicate-event early return, and happy-path Desktop invalidation otherwise looked coherent. No additional security bypass was found in the reviewed paths.
— :bot: Jude’s code review agent
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 61c40c006d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
jedwards27
left a comment
jedwards27
left a comment
There was a problem hiding this comment.
Reviewing exact head 61c40c006ddc2225917e3fbb0b752ed40a07c9fe against base bbd20fae75ecc3bd7a83cc12a65379fac22a2b79: request changes.
The revision repairs the earlier kind coverage, publication serialization, and failed-projection discovery in implementation shape, but two release-blocking defects remain.
1. Reconciliation performs an unbounded deployment-wide scan before serving, then repeats it every minute
dm_visibility_viewers_needing_reconciliation aggregates every active DM membership row in a community and expands tags for every live relay-authored kind 30622 snapshot before applying LIMIT $4 (crates/buzz-db/src/lib.rs:2904-2957). The limit bounds returned viewers, not database work. reconcile_dm_visibility_snapshots runs this serially for every active community (crates/buzz-relay/src/handlers/side_effects.rs:3314-3347), while main awaits the complete sweep before opening the listener and repeats it every 60 seconds by default (crates/buzz-relay/src/main.rs:537-575).
Relay recovery latency and steady-state load therefore scale with complete deployment DM membership/visibility history even when no projection is dirty. Please make reconciliation proportional to failed/dirty projections (for example, a durable outbox/dirty-viewer queue), or make startup non-blocking and demonstrate an index-supported bounded cursor with representative EXPLAIN/load evidence. A post-aggregation LIMIT is not bounded batching.
2. Retrying an already-delivered moderation notice unhides an old DM without inserting a message
send_moderation_notice opens/reuses the moderation DM, clears the recipient's hidden state, and publishes kind 30622 (crates/buzz-relay/src/handlers/moderation_notices.rs:97-130) before checking notice_already_sent and returning without inserting anything (:133-145). Thus: deliver notice → recipient hides DM → retry the same source/action. The retry removes the DM's h tag and Desktop renders the old conversation again, despite no new activity. This violates the documented idempotent crash-retry path and overrides an explicit user preference; the periodic reconciler also preserves the prematurely cleared DB state.
Please move the source-id idempotency check before visibility mutation/publication and add a regression covering deliver → hide → retry same source, asserting the DB remains hidden, kind 30622 still includes the DM, and no new notice exists.
Compatibility and validation still required
The reconciliation scan and monotonic timestamp lookup only consider snapshots signed by the current relay key (crates/buzz-db/src/lib.rs:2938-2944,3041-3051), while Desktop selects kind 30622 by viewer and greatest created_at without author filtering (desktop/src-tauri/src/commands/channels.rs:226-251). An old-key head can survive key rotation and be ahead of wall clock (crates/buzz-db/src/lib.rs:3067-3070), allowing a first current-key repair to lose to stale state. Please supply a key-rotation regression or document and enforce the supported key lifecycle.
Exact-head evidence: clean detached-head checks reported cargo check -p buzz-db -p buzz-relay --tests passing; cargo test -p buzz-db passed 105/105 non-ignored tests; the two new targeted ignored Postgres regressions passed (the contaminated whole ignored suite is not claimed green); and full Desktop pnpm test passed 5,091/5,091. Posted CI is green. The Desktop test only proves query invalidation, not the relay-backed visible journey, so hide → inbound kind 9/40002 → sidebar/unread/Inbox behavior remains without exact-artifact native proof.
|
🤖 Addressed the latest Jude review at c1eaa89:
Validation: |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c1eaa89ad3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
jedwards27
left a comment
jedwards27
left a comment
There was a problem hiding this comment.
Verdict: REQUEST CHANGES
Reviewed: bbd20fae75ecc3bd7a83cc12a65379fac22a2b79..c1eaa89ad388d39fba13f553a7e0fcd1a8d545d8 (exact head c1eaa89ad388d39fba13f553a7e0fcd1a8d545d8)
Risk: high — this changes canonical DM visibility state, relay-authored per-viewer snapshots, durable retry/recovery, a database migration, and Desktop invalidation/sidebar behavior.
Blocking findings
-
Migration 0032 abandons pre-upgrade visibility drift.
migrations/0032_dm_visibility_dirty_viewers.sql:6-32creates an empty queue without seeding viewers. Reconciliation now only claims queue rows (crates/buzz-relay/src/handlers/side_effects.rs:3312-3324), while the prior canonical-vs-snapshot startup scan was removed (crates/buzz-relay/src/main.rs:537-565). Consequently, a canonicalchannel_members.hidden_at/ kind-30622 mismatch created before this deployment has no queue row and can survive every restart indefinitely unless a later mutation happens to enqueue that viewer. This defeats the recovery contract precisely for brownfield state. Please add a bounded migration/backfill of active DM viewers (or an equivalent durable cursor) and a pre-0032 upgrade regression that starts with a stale/missing snapshot, migrates, reconciles, and proves the relay-authored snapshot matches canonical DB state. -
A full batch of poison rows can starve every newer repair indefinitely. The claimant always chooses the earliest 100 due rows (
crates/buzz-db/src/dm.rs:560-575). Failed claims become due again after a fixed five seconds (crates/buzz-db/src/dm.rs:606-625), but the worker ticks every 60 seconds by default (crates/buzz-relay/src/main.rs:541-565). Thus 100 persistent failures are due earlier than all subsequently enqueued mutations on every tick and monopolize every batch;attemptsis incremented but never affects scheduling. Please provide bounded fairness—such as capped exponential backoff plus separate fresh/retry quotas or a fair cursor—and a regression proving a later dirty viewer is claimed within a stated bound despite 100 persistent failures.
The claim query also does not substantiate its “indexed” claim: the pending/recovery OR and ORDER BY COALESCE(...) caused a sequential scan plus top-N sort in a representative 100k-row Postgres analog (100,000 rows scanned for 100 results, 40.7 ms before the production community join/write checks). Split the independently indexed pending/expired branches or otherwise make the claim shape index-supported, and provide representative EXPLAIN evidence.
Integrated trace and validation
The previously reported deployment-wide unbounded sweep, moderation retry race, and key-rotation issue are repaired in implementation and focused runtime coverage. The recipient-only one-to-one/group-DM resurface path, kinds 9/40002, per-viewer snapshots, unrelated-DM isolation, Desktop channels invalidation/sidebar filtering, restart/reconnect behavior, concurrent publication, and later-user-hide preservation were traced without another product, privacy, tenancy, or security blocker.
At the exact clean head:
cargo check -p buzz-db -p buzz-relay --tests: pass.cargo test -p buzz-db: 106 passed; 188 Postgres tests ignored by the default run.- Focused ignored Postgres tests for failed-publication recovery, concurrent unhide serialization, future-dated old-key rotation, and moderation duplicate-retry preservation: pass individually.
- Complete Desktop node suite: 5,091 passed, 0 failed.
git diff --check: pass.- Posted GitHub backend/relay/Desktop/Rust/build/security/mobile checks: green.
Manual/native evidence: no packaged-app visual/accessibility run. This adds no new visible control; the remaining blockers are migration and queue-liveness defects that packaged UI evidence would not resolve.
Residual risk: the native visible-state journey was not exercised, and the Postgres performance observation used a representative analog rather than the production dataset. Neither weakens the two deterministic correctness blockers above. Any newer head invalidates this verdict.
Signed-off-by: kenny lopez <klopez4212@gmail.com>
Signed-off-by: kenny lopez <klopez4212@gmail.com>
Signed-off-by: kenny lopez <klopez4212@gmail.com>
Signed-off-by: kenny lopez <klopez4212@gmail.com>
c1eaa89 to
1101c6f
Compare
|
🤖 Addressed the latest blocking review in 1101c6f:
Validation: cargo check -p buzz-relay --tests; cargo clippy -p buzz-db --tests with warnings denied; cargo test -p buzz-db (106 passed, 197 Postgres tests ignored); flutter analyze; flutter test (1,553 passed); Desktop Node tests (5,250 passed); formatting and diff checks. Docker/Postgres is unavailable locally, so the new migration/fairness Postgres tests and representative EXPLAIN are committed and compile but could not be executed here. @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1101c6f20e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
jedwards27
left a comment
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent reviewed exact head 1101c6f20e6bf82ed9cc10b173829ab4987dfaf4 against base 24ec6a468ec9d0d425ee58fbfc4d416412c446ad.
Request changes. The indexed queue/recovery design is substantially improved, but the accepted-message transaction boundary still permits permanent lost resurfacing, moderation can expose stale content without a durable notice, and reversible archived communities are omitted from brownfield repair.
Findings
P1 — Accepted DM persistence is not atomic with resurfacing
In ordinary ingest, the event is committed first, duplicate delivery returns immediately, and only a newly inserted event invokes recipient resurfacing (crates/buzz-relay/src/handlers/ingest.rs:3063-3123). The workflow sink has the same ordering (crates/buzz-relay/src/workflow_sink.rs:391-420). A crash after event commit but before resurface_dm_for_message_recipients leaves a durable kind 9/40002 message while canonical hidden_at and the dirty-viewer queue remain unchanged. Relay/client retry is classified as a duplicate and cannot repair it; the worker has no row to claim. The recipient can therefore remain hidden forever after restart/reconnect.
Make accepted DM insertion plus recipient unhide/dirty enqueue one DB transaction, or introduce an equivalently durable event-side recovery cursor. Add deterministic failure injection after message persistence and prove duplicate retry + restart converges while sender and unrelated DMs remain unchanged.
P1 — Moderation visibility can be published before the notice exists
send_moderation_notice correctly checks for a duplicate first, but then opens/unhides the recipient and publishes visibility at crates/buzz-relay/src/handlers/moderation_notices.rs:121-155, before discovery and notice insertion at lines 157-188. A crash, discovery error, or insertion failure in that interval resurfaces the moderation DM with only old content, despite no accepted new notice justifying the override of the user's hide. Even successful delivery has a stale-content visibility interval.
Insert the idempotent notice and apply recipient unhide/dirty enqueue atomically, then publish/fan out after commit. Add failure injection around discovery/insertion proving failure cannot resurface old content and retry converges.
P2 — Archived-at-upgrade communities never receive brownfield repair
Migration 0033 backfills only communities with archived_at IS NULL (migrations/0033_dm_visibility_dirty_viewers.sql:41-55). Archive is reversible, but unarchive only clears archived_at (crates/buzz-db/src/lib.rs:1612-1641) and does not enqueue viewers. The worker consumes queue rows, so pre-deployment snapshot drift in a community archived during migration survives a later unarchive indefinitely absent unrelated DM activity. An isolated exact-head repro produced queued_while_archived=0 and queued_after_unarchive=0.
Either backfill all non-deleted/non-deleting community viewers and defer claims while archived, or atomically enqueue active DM viewers on unarchive. Cover pre-0033 archived → migrate → unarchive → reconcile.
Verification and evidence limits
- Exact head was rechecked immediately before this review.
- Passed locally at this head: active-community brownfield upgrade repair; failed-publication recovery; concurrent publisher serialization; future-dated old-key rotation;
cargo check -p buzz-db -p buzz-relay --tests; full defaultcargo test -p buzz-db(106 passed, 197 ignored). - Representative 100k due-retry
EXPLAIN ANALYZEuseddm_visibility_dirty_viewers_retry, touched 25 index rows, and returned 25 in 0.288 ms. The prior OR/COALESCE full scan is gone. - The submitted ignored fairness/backoff regression is itself red:
cargo test -p buzz-db tests::dm_visibility_fresh_claims_are_not_starved_by_poison_retries -- --ignored --exactfails atcrates/buzz-db/src/lib.rs:7565-7574because PostgreSQLEXTRACT(EPOCH ...)isNUMERICbut the test decodesf64; it never proves the backoff assertion. - Relay E2E passed 27/27, including kind 9/40002 one-to-one/group resurfacing and sender/unrelated preservation. Focused Desktop invalidation passed 1/1; full Mobile channels-provider tests passed 20/20. Owner scoping of private kind 30622 remained intact in the traced filter/fan-out paths.
- No packaged native Desktop hide → inbound → sidebar/unread/Inbox workflow was demonstrated. The Desktop test proves cache invalidation, not rendered UX convergence.
- GitHub checks are currently red: Desktop Smoke E2E (4), and consequently its Desktop aggregator, failed. The observed tooltip/flaky virtualization failures were not attributed to this DM change, but green CI cannot be claimed.
A new head invalidates this verdict until its delta and exact artifacts are reviewed.
Signed-off-by: kenny lopez <klopez4212@gmail.com>
|
🤖 Addressed the exact-head review in 896378c:
Validation: cargo check -p buzz-db -p buzz-relay --tests; cargo clippy -p buzz-db -p buzz-relay --tests with warnings denied; cargo test -p buzz-db (106 passed, 198 Postgres tests ignored); focused moderation unit tests; Rust formatting and git diff checks. The new Postgres regressions compile, but this machine still has no reachable Postgres service, so their runtime execution and a new representative EXPLAIN are not claimed locally. @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 896378c902
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
jedwards27
left a comment
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent reviewed exact head 896378c902194fa54be062cf6e4471c2b11b0738 against base 24ec6a468ec9d0d425ee58fbfc4d416412c446ad.
Request changes. The prior crash-consistency, migration, queue-fairness, and Desktop concerns are repaired, but the moderation retry regression now fails repeatably and exposes a production ordering defect.
P1 — Moderation-notice retry can fail before delivery after publishing partial discovery state
The ignored regression handlers::moderation_notices::tests::delivered_notice_retry_preserves_a_later_user_hide failed consistently on two freshly migrated Postgres scratch databases. After its injected notice-insert failure is removed, the intended successful retry aborts at crates/buzz-relay/src/handlers/moderation_notices.rs:557-559 with:
kind 39002 roster does not match canonical channel membership
This is not merely a red test. send_moderation_notice awaits emit_group_discovery_events before atomically accepting the notice and resurfacing its recipient (crates/buzz-relay/src/handlers/moderation_notices.rs:149-180). That helper commits/replaces kind 39000 and 39001 before locking and validating the kind 39002 membership snapshot (crates/buzz-relay/src/handlers/side_effects.rs:1109-1214). The retained failed database had both recipient and relay in canonical channel_members, an active kind 39000 containing only the recipient p tag, and no active kind 39002 because the roster guard rejected it.
Consequence: a retryable moderation delivery can repeatedly fail before accepting the notice while already stranding a partial discovery generation (39000/39001 without 39002). The recipient remains hidden, which is safe, but the notice is not delivered and channel discovery state is internally inconsistent. Make discovery publication atomic/recoverable, or otherwise prove retries converge without partial state; the submitted regression must pass from a fresh migrated database.
Integrated verification
At the same clean exact head:
- Postgres-backed
dm_event_insert_and_recipient_resurface_roll_back_together: pass; message persistence, recipient unhide, and dirty enqueue roll back together. - All four targeted
dm_visibility_*tests: pass, covering failed-publication repair, concurrent rebuild serialization, old-key/future-dated snapshot replacement, and fresh-claim fairness under poison retries. pre_0033_dm_visibility_state_is_enqueued_and_reconciled: pass, including brownfield queue repair.- Default
cargo test -p buzz-db: 106 passed, 0 failed, 198 ignored. - Desktop
pnpm test: 5,250 passed, 0 failed; Mobileflutter test: 1,553 passed. - Exact-head GitHub CI is green across relay/backend integration, Desktop, Mobile, Rust, security, cross-compile, and image jobs.
Source tracing found recipient-only resurfacing for one-to-one/group DMs, sender and unrelated-DM preservation, kinds 9/40002 coverage, durable snapshot retry, and per-viewer Desktop/Mobile invalidation coherent. The Desktop subscription retries setup with bounded backoff and replay overlap; its focused test correctly treats kind 30622 as channel-list invalidation rather than a membership change.
Residual evidence limit: no packaged native multi-client reconnect/restart journey was performed. That does not weaken the deterministic moderation retry failure above. Any newer head invalidates this verdict.
Signed-off-by: kenny lopez <klopez4212@gmail.com>
|
🤖 Addressed Jude’s exact-head moderation retry finding in b0a3243:\n\n- Discovery now captures one locked membership snapshot, commits and dispatches kind 39002 first, then emits kinds 39000 and 39001 from the same generation. A roster failure can no longer strand metadata or admin discovery without a roster.\n- Moderation notice acceptance and resurface now commit before discovery. A post-commit discovery failure is propagated, and an idempotent duplicate retry repairs discovery without reinserting or resurfacing the notice.\n- The expanded regression injects a notice failure (no discovery), then a kind 39000 failure (durable notice plus roster only), then proves the duplicate retry converges to kinds 39000/39001/39002 and a later duplicate preserves a user hide.\n- Also closed the Mobile kind 30622 setup race with limit 1 replay, event-ID deduplication, and a subscription-gap regression.\n\nValidation: cargo check -p buzz-relay --tests; cargo clippy -p buzz-relay --tests -- -D warnings; Rust formatting and diff checks; Flutter analyze; focused channels provider suite (21 tests); full Flutter suite (1,554 tests). The ignored Postgres regression compiles, but Postgres is unavailable on this machine for a live run. The pre-push aggregate hook also exceeded local disk capacity during unrelated parallel workspace checks; authoritative CI is running on this head.\n\n@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b0a324357f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
klopez4212
left a comment
klopez4212
left a comment
There was a problem hiding this comment.
:bot: Carl is reviewing on Kenny Lopez's behalf.
Request changes at exact head b0a324357ffab6be41fdf7dbcfddd9641beb81e3.
P1 — Forum messages accepted in a DM still do not resurface it
crates/buzz-relay/src/handlers/ingest.rs:3039-3049 sends only kinds 9 and 40002 through the atomic DM insert/resurface path. The relay nevertheless accepts kinds 45001 and 45003 as channel-scoped message writes without restricting them to forum channels (ingest.rs:378-392, 611-627). Both clients explicitly classify all four as human-visible channel messages (desktop/src/shared/constants/kinds.ts:80-90; mobile/lib/shared/relay/nostr_models.dart:49-55), and Desktop's DM notification predicate derives from that same set.
A member using a custom or stale client can therefore publish a forum post/comment to a hidden DM. The event is accepted and rendered by clients when queried, but the recipient's hidden_at remains set and their clients remain unsubscribed to the conversation—the original bug, through another supported kind.
Please centralize the relay's resurface-trigger predicate around the accepted human-visible message set (currently 9, 40002, 45001, 45003) and parameterize the live regression over all four. Removing either forum kind from that predicate should fail the test. If forum events are invalid in DMs by product contract, enforce that channel-type restriction at ingest instead and add rejection coverage; the current accept-but-don't-resurface behavior is inconsistent.
Scope assessment
The PR is large for defensible reasons, but the growth history matters. The initial fix was 341 additions. Review then exposed that canonical hidden_at and relay-authored kind 30622 are a distributed projection: message acceptance, recipient-only unhide, snapshot publication, concurrent replacement, crash recovery, key rotation, brownfield migration, Desktop/Mobile subscription races, workflow messages, and moderation notices all need to converge. That drove the branch to 2,441 additions / 210 deletions across 16 files. Roughly half of the additions are regression/integration coverage; the remaining production surface is primarily the atomic DB path, durable dirty-viewer queue, reconciliation worker, and client subscriptions.
I would not replace the queue with best-effort publication: that recreates the permanent stale-snapshot failure this review history already demonstrated. The moderation discovery changes are collateral scope, but they close a deterministic retry failure introduced/exposed by routing moderation notices through the same atomic DM path. The branch should be cleaned/squashed before merge, but the architecture is not gratuitous abstraction around one boolean; it is repair machinery for an existing canonical-state/event-projection split.
git diff --check passes at the reviewed clean head. Rust lint, unit tests, security, DCO, and x86_64 cross-compile are green; several current-head CI jobs are still in progress, so full green is not claimed.
jedwards27
left a comment
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent reviewed exact head b0a324357ffab6be41fdf7dbcfddd9641beb81e3 against base 24ec6a468ec9d0d425ee58fbfc4d416412c446ad.
Request changes. Two independent P1 gaps remain: accepted human-visible forum content can bypass DM resurfacing, and the new moderation recovery path fails repeatably on fresh Postgres databases before it can repair discovery.
P1 — accepted forum messages in a DM do not resurface it
The atomic DM insert/resurface path is selected only for kinds 9 and 40002 (crates/buzz-relay/src/handlers/ingest.rs:3039-3050,3081-3103). The relay nevertheless accepts and channel-scopes forum kinds 45001 and 45003 without restricting them to forum channels (ingest.rs:378-392,611-627). Both clients classify all four kinds as human-visible channel messages (desktop/src/shared/constants/kinds.ts:80-90; mobile/lib/shared/relay/nostr_models.dart:49-55), and Desktop's DM notification predicate derives from that set (desktop/src/features/channels/isDmNotifiableKind.ts:6-18).
A custom or stale client can therefore publish a rendered, notifiable forum post/comment into a hidden DM while leaving the recipient hidden and unsubscribed. This preserves the original failure through two supported kinds. Centralize the relay predicate on the accepted human-visible set and parameterize the live regression across all four kinds. If forum events are invalid in DMs by contract, reject them by channel type and add negative coverage; accepting without resurfacing is inconsistent.
P1 — moderation roster-first recovery fails on fresh Postgres
The ignored Postgres regression handlers::moderation_notices::tests::delivered_notice_retry_preserves_a_later_user_hide failed twice on separate freshly migrated databases at crates/buzz-relay/src/handlers/moderation_notices.rs:603-606: expected live discovery kinds [39002], received []. A diagnostic run showed the earlier production error: kind 39002 roster does not match canonical channel membership.
The new flow durably accepts the notice and then invokes discovery (moderation_notices.rs:174-203); duplicate retry re-enters discovery (:101-126). Discovery locks one membership snapshot and attempts kind 39002 first (crates/buzz-relay/src/handlers/side_effects.rs:1118-1132), but the migration 0032 exact roster/role fence rejects it (migrations/0032_channel_roster_snapshot_fence.sql:25-67). The failed databases retain the kind-9 notice with no live 39000/39001/39002 events. Retry therefore returns the same error, and the regression never reaches its metadata-failure recovery or later-hide-preservation assertions.
Fix the emitted-39002/canonical comparison, then require this fresh-database test to pass all phases: failed notice leaves no discovery; injected 39000 failure leaves durable notice plus roster; duplicate retry converges to 39000/39001/39002; later duplicate preserves a user hide. Mutation-prove the roster-first ordering.
Integrated exact-head evidence
- Remote PR head and clean detached local head matched
b0a324357ffab6be41fdf7dbcfddd9641beb81e3immediately before review;git diff --checkpassed. cargo test -p buzz-db: 106 passed, 0 failed, 198 ignored; doc tests 0.- Fresh-Postgres moderation regression: failed twice, 0/1 passed each run,
rc=101, identical[]versus[39002]result. - Full Desktop package: 5,250 passed, 0 failed.
- Full Mobile package: 1,554 passed, 0 failed.
- Exact-head GitHub checks are green, including relay/backend E2E, Desktop core/build/smoke/integration, Mobile, Rust lint/unit/security, and cross-compiles. They do not execute the failing ignored Postgres regression.
The 896378c..b0a3243 delta's Mobile latest-snapshot replay plus event-ID dedup closes the setup race coherently in source. The earlier atomic DM insertion/resurfacing, durable dirty queue, migration repair, fairness/backoff, concurrent rebuild, key rotation, sender/unrelated-DM preservation, and Desktop/Mobile viewer-scoped invalidation remain coherent under re-trace. No packaged native multi-client reconnect/restart journey was run; that residual evidence gap does not weaken either deterministic blocker above.
Any newer head invalidates this verdict.
The relay accepted forum posts/comments (45001/45003) as channel-scoped writes but only routed kinds 9/40002 through the atomic DM insert/resurface path. Both clients classify all four as human-visible channel messages, so a custom or stale client could post a forum event into a hidden DM: the event rendered but the recipient's hidden_at stayed set and their clients stayed unsubscribed — the original bug through another supported kind. Centralize the resurface trigger on a shared is_human_visible_message_kind predicate (kinds 9/40002/45001/45003, matching the clients' CHANNEL_MESSAGE_EVENT_KINDS) and parameterize the live regression over all four kinds so dropping either forum kind fails the test. Signed-off-by: kenny lopez <klopez4212@gmail.com> Co-authored-by: Mongo <9cfd347903944d5b85aa6c93d2ab67381b978a92a31914bca69998968752a1d7@buzz.block.builderlab.xyz>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7265c10a29
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| kinds: [ | ||
| KIND_DM_VISIBILITY, | ||
| KIND_MEMBER_ADDED_NOTIFICATION, | ||
| KIND_MEMBER_REMOVED_NOTIFICATION, | ||
| ], |
There was a problem hiding this comment.
When this subscription starts after a visibility update, a burst of 50 newer membership notifications for the same viewer can consume the filter's entire shared limit, so the kind:30622 snapshot is not replayed and the hidden DM remains absent until the 60-second channel poll. The since cutoff also loses the update outright if subscription setup is delayed beyond 30 seconds. Subscribe to visibility separately with limit: 1 (as Mobile now does), or otherwise reserve replay capacity for its latest snapshot.
AGENTS.md reference: AGENTS.md:L22-L24
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
:bot: Carl reviewing on Kenny Lopez’s behalf. This is current at 7265c10a2, not stale: Desktop still combines membership kinds and kind 30622 in one limit: 50 replay filter, unlike Mobile’s dedicated latest-snapshot subscription. Leaving this thread open for an implementation fix.
jedwards27
left a comment
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent — REQUEST CHANGES at exact head 7265c10a2966ac9d0079282e5cd7d1f55138d9ba (base 24ec6a468ec9d0d425ee58fbfc4d416412c446ad).
P1 — fresh-database moderation recovery still cannot establish its roster-first state
The submitted ignored regression still fails on fresh, fully migrated Postgres databases:
TEST_DATABASE_URL=postgres://buzz:buzz_dev@localhost:5432/buzz \
cargo test -p buzz-relay \
handlers::moderation_notices::tests::delivered_notice_retry_preserves_a_later_user_hide \
-- --ignored --exact --nocaptureTwo independent exact-head runs reproduced FAIL, 0/1 at crates/buzz-relay/src/handlers/moderation_notices.rs:603-606:
metadata failure must not strand 39000/39001 without a roster
left: []
right: [39002]
The injected failure applies only to kind 39000 (moderation_notices.rs:576-588) after the moderation notice is durably accepted. The empty result therefore means the prerequisite kind-39002 roster did not survive, so the regression never reaches duplicate-retry convergence or later-hide preservation. The implementation still publishes roster first (crates/buzz-relay/src/side_effects.rs:1118-1132), while migration 0032 requires the emitted four-field p tags to exactly match canonical active membership and roles (crates/buzz-relay/migrations/0032_channel_roster_snapshot_fence.sql:25-67).
Please diagnose and fix why store_group_members_event yields no live 39002 for a freshly migrated moderation DM, then make this exact regression pass every phase: rejected notice → no discovery; metadata failure → durable notice plus live roster; duplicate retry → live 39000/39001/39002; later duplicate → the user's later hide remains preserved. Do not weaken the assertion to accept []—that would merely place a rug over the trapdoor.
Prior accepted-kind blocker is repaired
The new helper includes human-visible kinds 9/40002/45001/45003 (crates/buzz-core/src/kind.rs:814-829), and ingest routes all four accepted DM kinds through the atomic message-insert/recipient-resurface transaction (crates/buzz-relay/src/handlers/ingest.rs:3039-3108). The regression loops over all four while checking recipient resurfacing, sender-hidden preservation, and unrelated-DM isolation (crates/buzz-test-client/tests/e2e_nostr_interop.rs:1330-1388). That set also matches Desktop and Mobile message-kind definitions (desktop/src/shared/constants/kinds.ts:80-90; mobile/lib/shared/relay/nostr_models.dart:49-55).
Validation and residual risk
cargo test -p buzz-core: 263 unit + 2 doc passed.- Full Desktop
pnpm test: 5,250 passed, 0 failed. - Fresh-Postgres ignored moderation regression: failed repeatedly as above.
- Posted CI is green at this head, but does not execute the ignored Postgres regression.
- No exact-built multi-client native hide → inbound message → sidebar/unread/Inbox → reconnect journey was completed. That remains a product-evidence gap after the server-side blocker is repaired.
Any new head invalidates this verdict until its delta and risk-shaped evidence are reviewed.
|
:bot: Jude’s code review agent reproduced this with a local Postgres stack and isolated the cause. You do not need Docker to diagnose it.
The smallest fix is in let event = EventBuilder::new(Kind::Custom(KIND_NIP29_GROUP_MEMBERS as u16), "")
.tags(tags)
+ // The relay is itself a member of relay-authored moderation DMs.
+ // EventBuilder otherwise silently removes the author's own `p` tag,
+ // producing a roster that cannot match canonical membership.
+ .allow_self_tagging()
.custom_created_at(nostr::Timestamp::from(ts))I applied only those four lines on top of exact head I also mutation-checked it: removing |
2 participants
Summary
The bug had two halves: hiding a DM persisted
hidden_at, but normal message ingestion never cleared it for recipients; desktop also did not subscribe to the private DM-visibility snapshot that communicates those changes. Inbox remained correct because it is driven by message activity independently of the sidebar visibility state.Related issue
Supersedes #6316; the implementation is tracked directly in this PR.
Testing
cargo check -p buzz-relay -p buzz-db -p buzz-test-client --testscargo clippy -p buzz-db -p buzz-relay -p buzz-test-client --tests -- -D warningscargo test -p buzz-db --lib dm::testscargo test -p buzz-test-client --test e2e_nostr_interop --no-runcd desktop && pnpm check && pnpm typecheckuseMembershipNotificationsregression testThe new live-relay one-to-one and group DM regressions compile but were not executed locally because Docker/Postgres was unavailable. A broader relay library run otherwise reached 885 passing tests; eight existing database-backed media/admin tests failed on the unavailable local database.
Screenshots: N/A — this is state synchronization behavior with no visual design change.