Repository navigation
Fix remote Huddle ending cleanup - #9376
klopez4212 wants to merge 2 commits into
Conversation
Co-authored-by: Terp <5856f6900e8e0a4e0ea1f4f8d2ee33e4753d2119bc122f94ecafc660b46c0511@buzz.block.builderlab.xyz> Signed-off-by: kenny lopez <[email protected]>
Codex Review Summary: ✅ CompletedThis 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. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 04321aa75e
ℹ️ 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".
| Err(error) => { | ||
| warn!(channel_id = %room.channel_id, %error, "auto-archive failed, huddle stays alive"); | ||
| room.clear_ended(); | ||
| return false; |
There was a problem hiding this comment.
Retry finalization after transient ownership-validation failures
If the new Redis ownership validation errors or times out when the final participant leaves, this branch clears the ended flag and abandons finalization without scheduling a retry. Both local and remote teardown then return without removing the empty room or releasing its owner lease. Even if Redis immediately recovers and lease renewal succeeds, nothing retries archival or emits 48103, so the empty Huddle continues offering Join until another participant joins and leaves. This newly affects otherwise healthy PostgreSQL archival whenever ownership validation temporarily fails. Preserve a durable retry obligation and retry under the same generation fence rather than relying on another user joining.
AGENTS.md reference: AGENTS.md:L222-L229
Useful? React with 👍 / 👎.
brow
left a comment
There was a problem hiding this comment.
🤖 Tom's mobile review team reviewed this at 04321aa and recommends landing it with one small fix: #9384 explicit-leave finalize fix. Without it, when everyone left in a Huddle is connected through a relay that restarts, crashes or loses its link, the Huddle ends for good and their automatic rejoin is refused.
Please approve and merge #9384 explicit-leave finalize fix into this branch. We then approve and merge this PR.
Report and discussion: buzz://message?channel=e71f9a2a-ac46-473d-a24e-5e16fd8cb7b1&id=b7fc77dbb90b6f3ad32d0bc77c74cf82e5fb31f659f1901f758e27f8fd36ba77
| let community = CommunityId::from_uuid(community_id); | ||
| for (_pubkey, peer_id) in registered { | ||
| self.remove_remote_peer(community, session_id, fenced.generation, peer_id); | ||
| self.remove_remote_peer(community, session_id, fenced.generation, peer_id) |
There was a problem hiding this comment.
[P1] Keep transport-loss cleanup from ending the Huddle
This loop also runs when the ingress relay restarts or its mesh stream drops. Calling remove_remote_peer now archives the channel when that removes the last connection, so clients attempting recovery cannot rejoin. Please include the explicit-leave distinction from #9384 here, or land these changes together, rather than merging this intermediate behavior independently.
Posted by Codex (gpt-6.1-sol) on behalf of kylez
When the creator leaves a Huddle with only agents connected to audio, desktop now publishes the ended state and archives the room. This replaces the leave path's unconditional reliance on relay teardown, which could leave the card offering Join. The existing View action and transcript membership are preserved. The service counts attendance from raw audio admissions, using fresh relay-signed membership to classify bot roles. Departed humans keep transcript membership without counting as present. Connected legacy agents with a member role are recognized from verified kind-30177 records signed by the viewer or room owners, matching the old desktop trust policy. Known additional devices, other connected humans, unknown peers, failed classification reads, failed joins, connection errors, and retired sessions cannot trigger ending. Native signing now validates and permits the existing 48103 event. Concurrent relay archival is confirmed as successful cleanup. This is the desktop client counterpart to [mobile #9375](block/buzz#9375); [relay #9376](block/buzz#9376) remains independent. Only the creator or relay may announce an ending. Membership and audio snapshots cannot atomically exclude a device joining during hangup; non-creators and ambiguous rosters retain relay cleanup. Validation: typecheck and all 63 Huddle service tests pass. Restoring the old ending helper makes the new positive regressions fail. The full Vitest run at this follow-up completed with 21,049 passes and 10 failures across six files outside Huddles; all six files pass isolated reruns with BUZZ_TEST_WORKERS=2 (229 tests). All required pre-push checks pass at 6d201b6 with BUZZ_TEST_WORKERS=4: TypeScript/related tests, design guards, and workspace Clippy. Independent review found no blockers. The native event-shape regression passed during the initial implementation; native code is unchanged in this follow-up. The broader native package run remains limited by terminal fixtures encountering this machine's interactive shell security prompt and a window-state mock SIGSEGV, including serial execution. No browser cases added or removed: service tests exercise join → connected → hangup using the real outbox, including departed humans, legacy agent trust, additional devices, membership, connection and archive-failure boundaries. Existing Huddle card/View tests pass. Packaged desktop tryout remains pending: agent DM hangup, another device staying connected, transcript View, and mobile/desktop interoperability. No deployment or live-device validation claimed. --------- Signed-off-by: klopez4212 <[email protected]> Co-authored-by: Terp <5856f6900e8e0a4e0ea1f4f8d2ee33e4753d2119bc122f94ecafc660b46c0511@buzz.block.builderlab.xyz>
…9375) ## Summary Leaving an agent DM Huddle previously skipped client lifecycle cleanup, allowing Join to remain visible while the relay waited for the agent. Apply the existing last-human rule to DMs: release audio first, then publish the ending and archive when the human count is at most one. Preserve the existing additional-device and same-Huddle rejoin guards. Retain DM membership when other humans remain so hanging up does not revoke transcript access. Offer **View** only when cached backing-room membership confirms transcript access, opening the original Huddle transcript and reply threads without joining audio. Non-members retain disabled Ended. Preserve Open for an active local session even when an ended event arrives. For admitted or just-left Huddles, switching/removing a community retires pending remote finalization and waits for local audio teardown. Generation guards prevent retired count/end results from launching follow-up writes or interfering with a later call. Existing in-flight Huddle-start rollback behavior is unchanged. Client only; remote relay finalization is separate in #9376. Human counts use backing-channel membership; known duplicate owner admissions prevent client cleanup. Concurrent device arrivals are not made atomic by this client-only change, and human-to-human DMs with retained memberships still rely on relay finalization. ### Related issue Split from the combined Huddle repair at the author's request. Searched open Huddle issues/PRs; no exact duplicate found for this mobile behavior. ### Testing Follow-up fixes are in `7f28dc486`; merged current main in `7a592f19d`. Required push checks pass on the merged head. - Mobile formatting, analysis, and full suite passed (4,705 tests, 71 skipped; supplementary suites: 4 and 27). - DM hangup journey covers sole-human ending, preservation of another owner device, and preservation of membership when another human remains. Existing rejoin/lifecycle regressions remain in the full suite. - View journey reads the original transcript and opens a server-only reply-thread summary without joining audio. - Mutation checks: restoring the DM cleanup exemption fails the last-human journey; removing thread summaries fails View navigation. Restored production code passes. - Independent review completed; follow-up review of the three blocking fixes found no remaining blockers. - Follow-up regressions cover live-call Open after an ended event, reactive history membership revocation, stalled count/end requests during community transition, and background teardown of a later call while retired cleanup is unresolved. Phone/human validation remains pending. Start an agent DM Huddle and leave from the last human device: its card should stop offering Join. Tap View to read the transcript and replies. Repeat with another device still in the Huddle and verify it stays open. Not deployed. --------- Signed-off-by: kenny lopez <[email protected]> Co-authored-by: Terp <5856f6900e8e0a4e0ea1f4f8d2ee33e4753d2119bc122f94ecafc660b46c0511@buzz.block.builderlab.xyz>
…minal (#9384) Stacked on [#9376 remote Huddle cleanup](#9376). It fixes two blocking findings from review of 04321aa. ## Summary **Only an explicit leave ends a remote Huddle.** On 04321aa, when a control stream ended with peers still registered, the owner archived the channel and published the ended event (48103). That happens when the ingress relay restarts (Huddle audio sockets aren't drained, so they never send `UnregisterPeer`), crashes, or loses its relay-to-relay link. The ingress tells its clients to rejoin, but the rejoin then finds the channel archived. Now only an `UnregisterPeer` frame finalizes the room. Loop-exit teardown goes back to main's behavior: it removes the room and releases the lease, without ending the Huddle. On the ingress, a `SessionEnded` teardown (the local client's control sink closed while a roster update was queued) now sends the clean close first, so a real leave that loses the reader's select race still unregisters. **An already-archived channel is a terminal outcome.** The mobile end flow archives the channel itself, often before the last remote connection closes. `archive_channel_if` returned "channel is already archived" as an error, so the finalizer kept the empty room and its renewing lease with no retry. It now returns `Ok(false)` when the locked row is already archived. The finalizer then releases local resources and publishes no second ended event. This also fixes main's equivalent leak on the local path. Out of scope, per review: retrying after a failed archive, the Redis validation inside the archive transaction, and the draining-owner behavior change. ### Related issue N/A. Review findings on [#9376 remote Huddle cleanup](#9376). ### Testing - `remote_last_leave_ends_huddle_and_releases_owner_resources` (new Postgres journey, runs in the postgres-ci lane with Redis): the owner's local participant leaves, then the last remote participant leaves cleanly through the production `HuddleControlAcceptor` and finalizer. The unarchived row ends the Huddle once. The creator-archived row publishes no relay ending. Both rows release the room and lease. - `remote_only_stream_close_releases_owner_room_and_lease` now covers both a bare stream drop (room and lease released, no ending) and a clean leave (exactly one ending). - `conditional_archive_rechecks_authority_before_committing` asserts that archiving an already-archived channel returns `Ok(false)`. - Ran each guard both ways: removing the already-archived early return fails the creator-archived row and the DB test, and letting stream loss finalize fails the bare-drop row. - Full non-ignored buzz-relay and buzz-db suites pass, and clippy is clean. Not validated on a live multi-relay call. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Signed-off-by: Tom Brow <[email protected]> Co-authored-by: Tom Brow <[email protected]> Co-authored-by: studio <dc2fc481343672df31a689840c09299972e704dac592e0da6f18486646a7ee3e@buzz.block.builderlab.xyz> Co-authored-by: Claude Opus 5.5 <[email protected]>
kazemek21
left a comment
There was a problem hiding this comment.
Kyle reviewed this PR and the Nexus review-tracker notes and approved with no comments.
Posted by Codex (gpt-6.1-sol) on behalf of kylez
jedwards27
left a comment
There was a problem hiding this comment.
🤖 :bot: Jude’s code review agent — one blocking correctness finding; changes recommended at 75d38b6c2d5f357348288ded8c15aba15f765ea5..5221467931a1bb456b7749ea7d2328b580b41498.
The shared finalizer reaches owner-local, remote committed, and pending-last-slot teardown, and the generation/DB-lock checks protect the archive write. The ingress 48102 can follow owner 48103; client reconstruction treats end as terminal, so that ordering is not a separate blocker. The archived-first branch still fails the stated goal of removing a stale Join card:
- P2 — An archived-first room may never acquire an ended event.
crates/buzz-db/src/store/channel.rs:2228-2230returnsfalseon an archived channel, andcrates/buzz-relay/src/audio/handler.rs:2520-2521treats that exactly like lost ownership, skipping the48103at:2528-2541. Mobile can fail to publish its creator-signed48103yet still archive (mobile/lib/features/channels/mobile_huddle_controller.dart:451-468,522-537). On the remote final clean leave the new regression explicitly expects zero relay end events in the archived-first case (handler.rs:11567-11606). Late clients then reconstruct the start/join as active, while Join targets an archived room (desktop/src/features/huddle/components/HuddleIndicator.tsx:66-155). Author action: distinguish authorized, already-archived cleanup from lost ownership; ensure a matching durable end event exists (or publish a safe fallback when absent). Test failed creator announcement → successful archive → remote last leave, and preserve no-duplicate behavior when the creator event did persist. Verification owner: PR author (DB regression).
Non-blocking, pre-existing durability risk: emit_participant_event logs an insert_event failure and only broadcasts in memory (handler.rs:4015-4043); remote finalization then removes the room/lease (crates/buzz-relay/src/audio/join.rs:1274-1280), leaving a stale historical card after a DB fault. The same best-effort helper already served local auto-end before this PR; this is not an independent PR-caused blocker or a mandate to redesign the event pipeline in this patch. Author action for this PR: none beyond fixing item 1. Verification owner: follow-up lifecycle durability work, if accepted.
Validation and limits: source trace and git diff --check 75d38b6...HEAD passed in a clean worktree at 5221467; no package suite or live/native Huddle test was run in this review. The PR description reports prior Postgres/package passes on the split patch, not independently reproduced here. The synthetic remote control-stream test does not prove two live relay processes, network interruption/rejoin, or final-human-device behavior. Author action for that separate confidence gap: none. Verification owner: reviewer/human live-call pass before deployment, including clients on different relays. Risk: high (persisted lifecycle and cross-relay teardown).
Posted as a comment review per this repository's AGENTS.md review rule; the recommendation above is to fix item 1 before merging.
Summary
When the final Huddle audio connection was routed through another relay, owner cleanup removed the room without archiving the channel or publishing the ended event, leaving Join visible. Share committed-room finalization across local, remote, and pending-last-slot teardown. Retain the admission gate until finalization and recheck fenced owner authority under the database lock.
Relay/database only; mobile history access is separate in #9375. Existing orphaned rooms are not retroactively archived. Existing clients remain responsible for disconnecting agents when the last human device leaves.
Related issue
Related: #4879 / #4918 concern discovery metadata after auto-archive. Searched open Huddle issues/PRs; no exact duplicate found.
Testing
This is the unchanged relay patch previously validated in combined commit
4ec349435151ca11ce92bd0ebe59b845b3465639, split onto current main (whose intervening change is the relay version/changelog release).artifact_headsoutside changed paths.Draft pending human/live-call validation. Start an agent Huddle, leave from the final human device, and verify its card stops offering Join, including connections routed through different relay servers. Not deployed.