Skip to content

Fix remote Huddle ending cleanup - #9376

Open
klopez4212 wants to merge 2 commits into
mainfrom
terp/huddle-relay-ending-20261111
Open

klopez4212 wants to merge 2 commits into
mainfrom
terp/huddle-relay-ending-20261111

Conversation

@klopez4212

Copy link
Copy Markdown
Contributor

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).

  • Full PostgreSQL package lane: 1,376 initially passed; both initial failures passed after fixing local psql PATH and using actual joined/teardown completion in the test. Cleanup asserts archival and exactly one ended event.
  • Remote-control, pending-last-slot, and conditional archival regressions passed.
  • Full relay/database unit packages: 1,960 passed; five identity/archive and audit fixture failures report missing artifact_heads outside changed paths.
  • Relay/database all-target clippy and legacy desktop clippy/tests passed. Broader pre-push Rust groups passed after rerunning the load-generator group with a higher file-descriptor limit and four test threads (110 passed).
  • Independent relay review completed; formatting/diff/file-size guards passed. The split reused completed test evidence; no full rebuild was repeated solely for the split.

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.

Co-authored-by: Terp <5856f6900e8e0a4e0ea1f4f8d2ee33e4753d2119bc122f94ecafc660b46c0511@buzz.block.builderlab.xyz>
Signed-off-by: kenny lopez <[email protected]>
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Nov 11, 2026 •

Copy link
Copy Markdown
Codex Review Summary: ✅ Completed

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-11-11T23:56:02.378453Z 04321aa Draft marked ready
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment on lines +2514 to +2517
Err(error) => {
warn!(channel_id = %room.channel_id, %error, "auto-archive failed, huddle stays alive");
room.clear_ended();
return false;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge 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 brow left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 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

Comment thread crates/buzz-relay/src/audio/join.rs Outdated
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)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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

klopez4212 added a commit to block/buzz-app that referenced this pull request Nov 12, 2026
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>
brow pushed a commit that referenced this pull request Nov 12, 2026
…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 kazemek21 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 jedwards27 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 :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:

  1. P2 — An archived-first room may never acquire an ended event. crates/buzz-db/src/store/channel.rs:2228-2230 returns false on an archived channel, and crates/buzz-relay/src/audio/handler.rs:2520-2521 treats that exactly like lost ownership, skipping the 48103 at :2528-2541. Mobile can fail to publish its creator-signed 48103 yet 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.

This branch was successfully deployed

No deployments
codex-review — 52214679 Deployed Nov 13, 2026 by brow via Run Codex Security Review #14759
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.

4 participants