Fix authoritative status and huddle removal ordering - #7574
Conversation
Signed-off-by: Princess Donut <b238ea756dee4d98afa5883fc7f1de61eeabe65bf700e3a5a5a80db5e42e2c2b@buzz.block.builderlab.xyz>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
🔐 Codex Security Review
|
![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: 2049789959
ℹ️ 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".
| lookup[pubkey] = !sameUserStatus(current, cached) | ||
| ? (current ?? null) | ||
| : null; |
There was a problem hiding this comment.
When two different useUserStatusQuery keys containing the same pubkey overlap, an older request can return a status after a newer request has returned an authoritative empty result. This value comparison notices the intervening null, but null carries no version, so the subsequent newerUserStatus call accepts the older page and resurrects the deleted status. Use a request generation or versioned absence tombstone so an older fetch cannot overwrite a later authoritative clear.
AGENTS.md reference: AGENTS.md:L176-L180
Useful? React with 👍 / 👎.
| participantEvent({ | ||
| id: "a", | ||
| kind: 48102, | ||
| admissionId: "remote", | ||
| rosterRevision: 2, |
There was a problem hiding this comment.
This regression test supplies rosterRevision: 2 directly, while the added Rust tests stop at producing/receiving PeerUnregistered; none exercises the production handoff through handle_active_audio_connection into the emitted kind-48102 event. Removing the new remote_removal_revision propagation from the handler leaves every added test green, so add a handler-level test that asserts the persisted or broadcast LEFT event contains the owner's revision.
AGENTS.md reference: AGENTS.md:L188-L192
Useful? React with 👍 / 👎.
wesbillman
left a comment
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Changes requested: two P2 correctness findings
Reviewed exact head 2049789959cdf161f51184a1d3472461bf98bb2e against base 813bbd14121edacc6cb4733301a3af12131aa10e. Scope: authoritative status absence without losing live ordering, and owner-revision propagation for remote clean departure while retaining rolling-relay compatibility.
1. P2: Clear the status display without discarding its version fence
hooks.ts:260–267 converts an unchanged cached entry to bare null, including an already-hidden clear/expired version. A supported NIP-09 coordinate deletion of 30315:<pubkey>:general legitimately makes history empty (deletion producer). A subsequently delivered older kind-30315 live event now passes the version check against null and resurrects the status. Previously the retained hidden version rejected it; the existing regression explicitly establishes that ordering contract, but does not cross a refetch.
Source-derived reproduction: cache hidden version (101, idB); delete the coordinate and settle an unchanged empty authoritative refetch; deliver a delayed visible status (100, idA). The older text becomes visible instead of staying cleared.
Exit criterion: preserve (updatedAt,eventId) as a versioned hidden entry when clearing authoritative absence, while retaining the concurrent-live merge. Add production-cache coverage for empty refetch → older delayed live event → genuinely newer event, asserting the status stays hidden until the newer event.
2. P2: Preserve receive framing across the close-time reader handoff
handler.rs:780–792 drops read_owner_control on cancellation, then the new send_clean_close receive loop reads the same stream again. Production IrohRecvHalf::recv_frame stores its length/body buffers inside the cancellable future. If cancellation lands after consuming the four-byte length but before completing a roster frame body, the fresh receive interprets body bytes as the next length and can return a framing/decode error immediately. The valid owner acknowledgement behind it is lost and cleanup emits LEFT without a revision.
This is new read-after-cancel behavior: base teardown did not receive again. On a healthy new/new relay connection, a same-second JOIN/leave with this fragmented-frame interleaving and LEFT ID sorting below JOIN still reproduces the ghost participant this PR targets. Revisionless fallback for old owners or broken links is an intentional compatibility limitation; losing an available acknowledgement because our own handoff corrupts framing is not merely teardown latency.
Exit criterion: keep the receiver/framing state alive across cancellation, or make the frame reader cancellation-safe before reusing it. Add a byte-fragmented receive regression that cancels after header/partial-body consumption, then supplies the remaining frame and removal acknowledgement and verifies the owner revision reaches LEFT. Current tests use whole-frame ChanRecv and cannot expose this handoff failure.
Scope and validation
Source/metadata-only review on Wes’s Mac Studio (BLKD2G9MWNRWY.local), with independent desktop, relay, and consumer lanes. No checkout, build, test, or PR-code execution; the reproductions above are source-derived, not runtime test claims. Successful acknowledgement propagation, unchanged wire discriminants, per-stream unregister ownership, bounded status fetches and failure propagation were traced.
The unchanged HuddleIndicator/HuddleAttachment same-second LEFT→reconnect-JOIN reducers are a separate pre-existing inconsistency, not another blocker for this JOIN→LEFT repair. No new requirement to consolidate them here.
jedwards27
left a comment
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent
Verdict: REQUEST CHANGES
Reviewed: 813bbd14121edacc6cb4733301a3af12131aa10e..2049789959cdf161f51184a1d3472461bf98bb2e (exact live head)
Risk: high — status ordering and relay huddle departure ordering cross live delivery, authoritative snapshots, community/session ownership, and user-visible truthfulness.
Blocking finding
[P2] Preserve the status version fence across authoritative absence
desktop/src/features/user-status/hooks.ts:255-263 maps an unchanged cached status to bare null when authoritative history returns no event. applyUserStatusEventToQueries at :167-205 then has no (updatedAt,eventId) fence, so a delayed older live event is accepted and can resurrect a status that another client cleared, expired, or deleted.
Both independent lanes reproduced this through the production cache seam at the clean exact head: seed hidden/versioned (updatedAt=101,eventId=b), settle an empty authoritative refetch, then deliver visible (100,a). The cache moved from null to visible "STALE RESURRECTED". The added helper test at desktop/src/features/user-status/hooks.test.mjs:114-132 asserts only bare null, thereby preserving the provenance loss rather than protecting the live-event merge.
Author action: retain a hidden/versioned tombstone across authoritative absence while preserving genuinely concurrent newer cache state. Add a causal production-cache regression proving: versioned visible/hidden state → empty authoritative refetch → delayed older event remains hidden → genuinely newer event becomes visible.
Verification owner: author for the fix and regression; reviewer/A Team for exact-new-head mutation and full-gate rerun.
Huddle ordering trace
No second concrete defect was established in the changed huddle path. Owner removal is community/session/generation scoped; removal captures the owner-monotonic revision before LEFT broadcast and PeerUnregistered, ingress waits boundedly for the matching pubkey acknowledgment, and the handler joins that reader before kind 48102 emission (crates/buzz-relay/src/handlers/join.rs:1131-1187,1346-1368,1883-1924; handler.rs:859-889,914-927). QUIC control frames use write_all/read_exact, so fragmented length/payload delivery is reconstructed before decode (crates/buzz-relay-mesh/src/peer.rs:138-190). The appended enum variant degrades boundedly with older peers without renumbering existing discriminants.
A single production-seam fragmented/reconnect journey from remote disconnect through emitted revisioned kind 48102 was not run. That remains a reviewer/relay/native confidence gap, not additional author rework.
Exact-head validation
- Full
cargo test -p buzz-relay: lib 1,041 passed / 93 ignored, main 13 passed / 1 ignored, lifecycle 9 passed; exit 0. - Full Desktop
pnpm test: 6,494 passed; exit 0. - Desktop typecheck and focused status/huddle suites (40/40) passed.
- Production-cache stale-resurrection probe reproduced the defect independently in both lanes.
git diff --check: passed; final lane trees clean at the exact live head.- Required CI is broadly green across Rust/PostgreSQL/relay E2E/Desktop core/integration/macOS/Windows/security/DCO. Desktop Smoke shard 4 has an unrelated unchanged video-playback failure; no changed file touches that surface, so CI/tooling owns its disposition rather than this author-actionable verdict.
- No native Desktop/iOS huddle journey was run.
Any new head invalidates this verdict until its delta is reviewed.
3 participants
Summary
Follow up on the two valid inherited findings from #7112:
Compatibility
PeerUnregisteredis appended to the postcard control enum, preserving every existing discriminant. During a rolling relay deployment, either mixed-version direction falls back to the existing revisionless LEFT behavior without breaking the control stream.Validation
audio::join: 37/37 passedaudio::handler: 5/5 passedA separate broad
cargo test -p buzz-relayinvestigation compiled and passed 1,034 tests; six unrelated media fixture tests failed withSqlx(PoolTimedOut)against local Postgres. The focused huddle suites and the subsequent pre-push Rust lane passed.