Add mobile Huddles voice MVP - #6056
Conversation
wesbillman
left a comment
wesbillman
left a comment
There was a problem hiding this comment.
Adversarial mobile audio review at e395e3320fe13609ecaba36aac009b15a415cdb6 found two blocking correctness risks shared by iOS and Android:
-
Unbounded Flutter ingress queue defeats the native jitter bounds (high). Every remote packet is appended to
_playbackTail(mobile/lib/shared/huddle/huddle_session.dart:392-404), and each link waits for a platform-channel call to complete before the next starts. There is no queue cap, coalescing, or late-frame drop before that chain. The relay permits 25 peers (crates/buzz-relay/src/audio/room.rs:46-49), so one mobile listener can receive up to 24 × 50 = 1,200 packets/s. If platform-channel service is slower than ingress—even briefly—the Dart future chain retains packets without bound, growing memory and latency while native's advertised 10-packet jitter bounds never get a chance to apply. This contradicts the foreground-session bounded-latency guarantee inmobile/HUDDLES.md:97-103. Smallest safe remedy: place a bounded, drop-oldest ingress queue in Dart (preferably per peer), run a single drain loop, and clear it on peer-left/dispose; add a burst/backpressure test proving the queue stays bounded and newest audio wins. -
Mobile fatally supports fewer talkers than the relay admits (high). Both native engines hard-fail the entire call after 15 remote playback objects (
HuddleAudioEngine.kt:355-381,493;HuddleAudioEngine.swift:396-426). The relay admits 25 total peers, and Desktop can add up to 20 agents, so a valid room can exceed this limit. When a 16th distinct remote peer sends audio,reportFailuretears down media rather than dropping/evicting that track. This is especially plausible for the advertised Desktop-agent TTS interoperability. Smallest safe remedy: align the supported peer cap end-to-end (admission/UI/protocol), or make native playback resource management tolerate the relay maximum; exceeding a local playback budget must degrade one track explicitly, not terminate an otherwise valid Huddle. Add a 16+ remote-talker test on both native paths or enforce a lower server-visible admission contract.
Related hardening: peer-left is only cleared from Flutter speaking UI (huddle_session.dart:435-439); neither native engine receives a remove-peer signal, so decoder/player state and queued audio survive peer departure and index reuse (HuddleAudioEngine.kt:393-478; HuddleAudioEngine.swift:779-835). Clearing native peer state on left should accompany the bounded ingress fix.
Verdict: BLOCK until ingress is bounded before the platform channel and the participant-cap mismatch cannot tear down valid rooms. I did not duplicate CI suites.
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.
Reviewed exact head e395e3320fe13609ecaba36aac009b15a415cdb6. Requesting changes for these mobile correctness defects:
-
Mobile does not implement the relay's authoritative
rosterresync control, and it retains native playout state across peer-index reuse. The relay emits{ "type": "roster", ... }after a mesh revision-gap recovery (crates/buzz-relay/src/audio/join.rs:1538-1557), but mobile's control switch only acceptschallenge,joined,left, anderror; it reportsrosteras unknown (mobile/lib/shared/huddle/huddle_transport.dart:328-350). Even for an ordinaryleft, Flutter only clears active-speaker UI (huddle_session.dart:435-439). It never tells native media to remove that peer. Android and iOS therefore retain the decoder, duplicate-filter sequence, queued packets, and player keyed solely by reusablepeerIndexuntil the entire call stops (HuddleAudioEngine.kt:355-417;HuddleAudioEngine.swift:396-423,779-835). Desktop explicitly drops this state on leave, roster removal, or index-to-new-pubkey remapping (desktop/src-tauri/src/huddle/playout.rs:438-486). A replacement participant can inherit stale queued audio and Opus decoder history, and a matching first sequence can be discarded. Handle roster snapshots as authoritative, detect index identity changes, and add a bridge operation that destroys/clears native per-peer state on leave/remap. Cover leave + index reuse and roster resync on both native paths. -
Every remote packet is appended to an unbounded serialized Dart future chain before native's bounded queues.
_playbackTailretains all incoming frames until each platform call completes (huddle_session.dart:392-404). With the relay-valid maximum of 24 remote senders at 50 packets/s, ingress can reach 1,200 calls/s. Any period where platform-channel service trails ingress grows memory and latency without a cap or late-frame policy, so native's 10-packet queues cannot enforce the documented bounded-ingress guarantee (mobile/HUDDLES.md:97-103). Put a bounded drop-oldest queue before the channel, preferably per peer, use one drain loop, clear it on leave/remap/dispose, and add a burst/backpressure regression test. -
A relay-valid room can terminally fail mobile audio at the 16th remote talker. The relay admits 25 total peers (
crates/buzz-relay/src/audio/room.rs:46-49), while both native engines allow only 15 remote playback objects and reportplayback_failedon the next distinct speaker (HuddleAudioEngine.kt:355-381,493;HuddleAudioEngine.swift:396-426). Dart treats a native media failure as terminal (huddle_session.dart:230-240). Align admission and client capacity end-to-end, or make local resource exhaustion degrade/evict a track rather than tear down a valid call. Test the boundary on iOS and Android. -
Background leave can lose durable channel cleanup.
paused/detachedindependently triggers relay shutdown (mobile/lib/shared/relay/app_lifecycle_provider.dart:35-47) and an unawaited Huddle leave (mobile_huddle_controller.dart:78-83), with no ordering contract. If relay shutdown wins, the member lookup falls back to2(mobile_huddle_controller.dart:168-178), thenleaveChannelruns while disconnected and its failure is swallowed with no resume retry (:205-232). Audio stops, but backing-channel membership can remain, producing ghost roster state and preventing last-human end/archive logic. Coordinate lifecycle publication before relay teardown or persist/retry the cleanup, and test both listener orderings.
CI is also currently red: Desktop Smoke E2E (2) failed in run 32006323730, job 95316546605. The Mobile job passed, but it does not exercise these relay/native lifecycle contracts. I did not duplicate CI-equivalent suites locally; git diff --check origin/main...HEAD passed on the reviewed head.
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.
Re-reviewed exact head b9ebeb349212c337e29460e1d16228e12c36153c. The only changes since the prior reviewed head are one-line desktop roster call-site updates in HuddleBar.tsx and HuddleRoomHeader.tsx; they do not address the four requested mobile correctness fixes:
- authoritative
rosterresync plus native per-peer cleanup on leave/index remap; - a bounded, late-frame-dropping Dart ingress queue before serialized platform-channel playback;
- alignment between the relay's 25-peer room capacity and native's fatal 15-remote-playback cap; and
- durable/ordered background channel cleanup rather than racing relay shutdown and swallowing failure.
The detailed reproduction paths and smallest safe remedies remain in my changes-requested review on commit e395e3320fe13609ecaba36aac009b15a415cdb6. Please resolve those blockers before landing this MVP.
|
Carl, an automated agent commenting via Kenny Lopez’s GitHub account. Fixed the four mobile correctness blockers and the two follow-up races at head
Validation on the exact pushed head:
The earlier one-off full-suite failure ( There are no inline review threads to resolve. Please re-review this head. |
![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: ddaaef418b
ℹ️ 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".
💡 Codex Reviewbuzz/mobile/ios/Runner/HuddleAudioEngine.swift Lines 884 to 890 in 2368ed6  Reorder mobile audio packets before decoding
When participants are attached to different relay pods, Huddle media crosses the lossy QUIC-datagram mesh ( buzz/mobile/lib/features/channels/channel_detail_page.dart Lines 362 to 364 in 2368ed6 Track active Huddles outside the timeline window
In a channel with more than 50 newer top-level rows after a Huddle starts, the initial channel-window query in buzz/desktop/src/features/huddle/hooks/useHuddleParticipantRoster.ts Lines 64 to 67 in 2368ed6  Preserve causal order for same-second reconnect events
When a participant disconnects and rejoins within the same Nostr timestamp second, both lifecycle events have equal ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
@codex review |
brow
left a comment
brow
left a comment
There was a problem hiding this comment.
🤖 Review of #6056 at head 1c9180bda8efb61b68fc1f567f77e833852f8d88. Four independent reviewers read this in parallel: the Dart control plane, the native audio bridge, the relay and desktop side, and test power. Each worked blind, with no findings shared between them. Every claim below is measured at that head, in clean worktrees.
You asked about the foundation of the wiring. The architecture is sound and it is worth building on. The layering is right. HuddleTransport owns the control plane and treats the relay roster as authoritative. HuddleSession owns the media bridge. MobileHuddleController owns the Nostr lifecycle. The revision contract in the Dart transport genuinely matches the relay contract in room.rs: monotonic per mutation, snapshot-authoritative, and a gap triggers a resync. We checked that against Room::add_peer, remove_peer_and_check_ended and roster_snapshot, not against the Dart comments. The generation and epoch guards are sufficient. We traced all four counters and found no interleaving where a stale callback mutates live state.
The findings below are concentrated in one seam. That seam is reconnect and native release, not the design.
We first measured this at 2368ed605ab28c992af8e1619ef4137b63029279. You pushed 1c9180bda8efb61b68fc1f567f77e833852f8d88 while we were writing, so we re-measured everything below at the new head. Your new commit fixes the packet-reordering finding, with a real sequence-ordered jitter queue on both platforms, and it adds a Huddle lifecycle query independent of timeline paging plus a community-transition hook that leaves the call. Both blockers below still reproduce at the new head. The two constructs behind the first one are byte-identical between the two heads.
We also checked the four blockers from the earlier automated review, because we did not want to repeat work you have already done. At this head all four are addressed. The transport now handles the authoritative roster control. The session now has a bounded per-peer playback queue that drops the oldest frame. The native active-talker limit now evicts one track instead of failing the call. The session now calls removeRemotePeer on left and on replaced. Nothing below repeats those points. Our first blocker is the residual case that those fixes do not cover.
Blocker 1: a media-socket reconnect can give one person's live audio state to another person
This is the one finding we ask you to fix before merge.
HuddleTransport.connect() emits a fresh transport state for the connecting phase. That constructor defaults its peer map to empty. The reconnect path therefore discards the previous roster. When the relay re-admits the client, the re-admission handler diffs the new roster against the now-empty previous map, so it finds nothing removed and nothing replaced. No left event and no replaced event is emitted.
HuddleSession releases native per-peer state only from peerEvents, on left or replaced. It is the only production caller of HuddleMedia.removeRemotePeer. That release works correctly for a steady-state departure or replacement. Reconnect is the path it does not cover, because reconnect emits no peer event at all. Both native engines key playout state by peer index: the Android engine keeps a peer-index-keyed playback map and an active-talker map, and the iOS engine keeps peerPlaybacks and activeTalkers the same way. Neither platform exposes a clear-all call. _audioIngress.clear() runs only in HuddleTransport.dispose(), never on reconnect.
The relay is free to reassign a peer index on re-admission. AdmissionGuard::release pushes freed indices onto a free list and alloc pops them, so index reuse across a reconnect window is expected relay behavior. When it happens, the decoder, the jitter buffer and the active-talker slot for the previous occupant of that index stay live and are reused for the new occupant.
Measured, two independent reviewers, separate rigs and separate probes:
- Reviewer 1 control, same socket, an authoritative roster reassigns index 1 from
alicetoeve: the transport emitsreplacedfor index 1. The teardown works. - Reviewer 1 probe, socket drops, then re-admission reassigns index 1 from
alicetoeve: the transport emits no peer events at all. No teardown. - Reviewer 2 probe, same shape at index 4: roster after reconnect shows the new pubkey, and the list of peers removed from native is empty.
- Reviewer 2 also measured two further consequences. A peer that departs during the outage leaves a stale active speaker, so
participantPubkeysandactiveSpeakerPubkeysdisagree for a few hundred milliseconds, and the UI reads the latter. Playback queues survive the reconnect and are replayed after the recycle, so frames authored by the old occupant reachplayRemoteFramewhile the roster says the slot belongs to the new occupant.
One relay detail decides where the fix belongs. We traced the admission paths in handler.rs and join.rs. Every fresh admission, including one after a socket drop, ends by sending a joined message whose peers field is the complete snapshot at the current revision. A client-facing roster is conditional repair traffic only: the owner emits it when its roster broadcast receiver lags or in response to an explicit resync, and the same-pod path never emits it at all. So a re-admitted client normally receives joined and nothing else. The flush must run on the re-admission joined path. The existing roster handling cannot cover this case.
The fix shape is already in this repo. The desktop playout loop handles this. On joined it compares the incoming pubkey against its index_to_pubkey entry for that index, and when they differ it drops that index from peers, frame_counts, active_indices and speaker_levels, under a comment that says "peer_index reuse with a new pubkey: flush the old peer's NetEq + Player so the next frame starts clean". Its roster path does the same with an identity-preserving retain. Mobile has no equivalent on its re-admission path. Mirroring the desktop behavior in the session, or having the transport emit replaced and left for a re-admission diff instead of going silent, would close it.
One warning if you fix this. A fix that only clears the Dart playback queues will pass a naive test and still leave the native decoder stale. Please make the fix drive removeRemotePeer as well, and test it against the index-reassignment shape specifically.
Why the current tests cannot see it: the session-level fake transport always republishes the same peers on connect, with the same local peer index. The identity behind an index never changes across a reconnect in any test, so the existing case that keeps media alive through a bounded transport reconnect passes while this hazard stays open.
Blocker 2: iOS native audio failures do not fail closed
HuddleAudioEngine.reportFailure on iOS sets a flag and calls onFailure. It does not clear running, remove the input tap, stop the engine, cancel the playout timer, or release the peer players. Capture and every peer player stay live until the Dart side completes a cross-bridge teardown. If that callback is not handled, the microphone can stay live.
The iOS engine does clear every peer player, but only in stopOnQueue, and reportFailure never reaches it. Android is the useful contrast. Its reportFailure sets running to false, and both realtime loops then release in a finally. We recommend the iOS path fail closed locally, in the same way, rather than depending on a round trip through Dart.
Further release and lifetime findings, native side
These are all in code this PR adds, so they are in scope for this branch. We rate them below Blocker 2 but above nits.
- Android partial-construction leaks.
HuddleAudioEngine.startcreates theAudioRecordbefore the encoder and outside its cleanup block, so an encoder failure leaks the record.createAudioRecordthrows on a non-initialized record without releasing it.PeerPlaybackbuilds a decoder and anAudioTrackbefore starting either, so a failure in track creation, decoder start, or track play leaves a partially built codec and track outside the playbacks map, where the playout loop cannot release them. These are exactly the resource-exhaustion conditions where release matters most. - Android keeps reading the microphone through an audio-focus interruption. The focus listener only sets a flag. The capture loop keeps calling a blocking read and discards whole frames. It does not stop or pause the record. A long focus loss leaves capture live until an explicit stop.
- iOS can leave the audio session active after a failed prepare.
prepareactivates the session before it overrides the output port. If the override throws, the catch clearsaudioSessionPreparedbut does not deactivate the session. A laterstopsees that flag false and returns early without deactivating. No capture starts, but the active session leaks.
Test power
The suite is in good shape for a POC: the full mobile package suite is 1,521 tests and passed at 2368ed605ab2, and the focused Huddle suite is 32 tests and passes at the current head, just mobile-check is clean, and the Android debug and iOS simulator builds pass. Mutation testing on the wire codec found the recognizer tests strong. Byte order, the relay header prefix offset, header length, and the flags field all produce correct failures when mutated.
Four coverage gaps survived mutation. None is exploitable through an honest relay, because the relay caps binary frames and drops undersized v2 frames, so we report them as coverage rather than defects: the oversize length guard accepts one byte over the maximum, the minimum-length guard accepts an empty Opus payload, the client length guard has no exact-maximum payload test, and the invalid-level lower bound has no test for a valid -127.
Non-blocking observations
- A
leftmessage naming the local peer index is accepted. The transport has no guard for that case, so the client stays connected and keeps encoding into a slot the relay says it does not own. We could not find a relay path that emits this, so we are not calling it a live defect. It is untested defense against a relay bug. - Bridge error codes are asymmetric. A missing engine in
removeRemotePeerreturnsplayback_failedon Android andinvalid_stateon iOS. A malformedsetSpeakerEnabledwhile stopped differs the same way, because validation order differs. Android alone returns a code for a second in-flight permission request. Dart discards the native code and maps everything to a generic platform failure, so this is invisible today, but it will matter to any later direct caller. - Queue bounds match the description, with one caveat. The three-packet startup and ten-packet per-peer jitter bounds are correct on both platforms, and both cap native peers at 15. The whole pipeline can still hold more than ten packets for one peer, because the Dart per-peer queue and the 50-packet native ingress queue sit in front of the jitter queue. Worth a comment so a later reader does not read ten as end to end.
- An unbounded bookkeeping set.
_finishedLifecycleAdmissionsonly grows for the life of the notifier. It is one small object per huddle joined, but it gates a decision. - Pre-existing, not yours, noted so it is not lost.
Room::add_peerhas no pubkey uniqueness check, andregister_remote_peeradmits before it records the peer in a pubkey-keyed map. A second registration for one pubkey therefore admits a duplicate roster member and orphans the first. We confirmed this shape already exists at the merge base and that this PR only threads revisions through that code, so it is a follow-up issue rather than something this branch owes.
Summary
The wiring foundation is good. Two blockers to fix before merge: reconcile native peer state on reconnect, and make iOS native failures fail closed. The native release paths listed above deserve a pass in the same sitting. Everything else is a follow-up or a comment.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1c9180bda8
ℹ️ 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".
|
Princess Donut, an automated agent commenting via Kenny Lopez’s GitHub account. Addressed the latest review findings in
Validation at exact pushed head
Replied to and resolved both current Codex threads. Please re-review this head. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8b950daef3
ℹ️ 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".
|
🤖 Re-reviewed #6056 at Both blockers are fixedReconnect peer-index reconciliation.
The previous head is the control, so the probe can show both results. Your new test crosses the reconnect boundary, which is the part that matters. Fail-closed native audio. iOS Also correct, and easy to miss: the new sequence window handles a continuous sender across its own 16-bit wrap without loss. I measured 69,998 of 70,000 frames heard, on both platforms. The reorder fix does what it says. Follow-up: a rejoin on the same slot can silence a participantOne route is still open. It does not need new work in this pull request. A remote participant leaves and rejoins while the local client is reconnecting its media socket. The relay free list is last-in-first-out, so the rejoin can take the same Measured at this head. Dart, after a leave and rejoin on slot 4 during the outage: Native, using the shipped Same results on iOS and Android. The cost law. The new participant stays silent for as long as the previous session talked on that slot, up to a ceiling near 655 seconds. Past that the cost falls to zero, because the window wraps back into the accepted range. It is a ramp and then a cliff, not a straight line, so a long call is not automatically the worst case. One caution about grading a fix. The shipped test
Why this is follow-up material and not a gate. Closing it is a design choice, not a small patch. Either the relay stamps each admission so a rejoin is distinguishable from an unbroken session, or the client resets per-slot playout on any roster revision that touches that slot. The desktop playout path keys its flush on a pubkey change as well, so the same route reaches desktop, and the sizing question is wider than this pull request. Since mobile Huddles are new here, there is no working behavior to protect, and landing this and fixing the rejoin route separately looks right to me. This is not a regression. For the record on scope: this is separate from the three findings the bot reviewer posted at this same head. Those stand on their own. |
jedwards27
left a comment
jedwards27
left a comment
There was a problem hiding this comment.
Changes requested on 8b950daef326a3222085ab97244779efee778f25 (base f956e6fe06a76e50cbd8fba1a162482e752e7f1a). The prior admission/reconnect/teardown fixes look substantially better, but three runtime correctness defects remain:
-
Android audio-focus interruption can become terminal session failure.
HuddleMediaPlugin.kt:38-48maps focus loss tosetInterrupted(true), which stopsAudioRecordwhilerunningremains true (HuddleAudioEngine.kt:213-220). The capture loop treats a negative return from the blocked/stoppedread()as fatal and callsreportFailurebecauserunningis still true (:322-331,387-394,520-524). Flutter then tears down the entire Huddle (huddle_session.dart:235-243). A transient call/focus duck should pause and resume, not eject the participant. Please synchronize interruption with the capture loop and add a native loss → read wakeup/error → gain regression proving no failure callback and resumed capture. -
More than 15 remote packet streams can churn every native jitter buffer before startup. The relay admits 25 peers (
crates/buzz-relay/src/audio/room.rs:47-49), while both mobile engines cap playback at 15. Android callsactiveTalkers.activatefor every packet and immediately releases the evicted playback (HuddleAudioEngine.kt:477-501); iOS mirrors this (HuddleAudioEngine.swift:500-537). Capture emits ordinary Opus packets every 20 ms whenever unmuted, including silence (HuddleAudioEngine.kt:346-377,423-456; Swift:766-796), yet each newly created playback requires three packets (Kotlin :531-534; Swift:914-920). A deterministic 16-stream round-robin probe produced 81 evictions over six cycles and no stream ever retained three packets. Select on actual speech/inactivity or otherwise preserve jitter state; add a >15-stream regression around selector + playback startup, not only selector cardinality. -
Desktop historical lifecycle reconstruction has no causal order for equal-second events. The subscription replays history in relay order
created_at DESC, id ASC, butreconstructHuddleParticipantRostersorts only bycreated_atand preserves the incoming arbitrary ID order for ties (useHuddleParticipantRoster.ts:61-65). With causalleave(z)then immediatejoin(a)in one second, history arrivesjoin(a), leave(z)and the reducer incorrectly removes the live participant. Start/join can invert similarly. The existing test supplies causal array order, so it does not exercise actual relay history order. This needs an authoritative sequence/state contract (not another arbitrary tie-breaker) plus a regression using the real historical ordering.
Validation at the exact head:
cd mobile && flutter test: PASS — 1,528 tests.cd desktop && pnpm test: PASS — 4,958 tests.cargo test -p buzz-relay: 868 passed / 13 failed / 43 ignored; the failures were not accepted as product regressions because the configured local DB lacked thecommunitiesrelation, with one telemetry global-state failure and one mesh-demo timeout. Exact-head GitHub relay/unit/desktop/mobile/security checks are green.- Source-model probes reproduced the 16-stream jitter starvation and same-second roster inversion.
Even after these fixes, this native audio MVP still needs the physical iOS/Android acceptance receipt documented in mobile/HUDDLES.md:111-114 (permission, routes/headsets, interruption/resume, background microphone teardown, and two-way Desktop audio). Static and CI evidence cannot prove OS microphone release or route recovery.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 88ed4212c7
ℹ️ 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".
|
🤖 One correction to my previous comment on #6056, re-derived at The cost is periodic, not a ramp and a cliffI wrote that the silence "falls to zero" once the previous session talked longer than about 655 seconds, so a long call is not the worst case. That is wrong. The cost repeats. It does not decay. The sequence window compares against the last drained sequence, and that value is kept modulo 65536. 65536 frames at 20 ms is 1310.72 seconds, so the whole effect restarts every 21.8 minutes of transmitted airtime on that slot. Within each cycle the silence ramps from zero up to 655.36 seconds, then is near zero for the rest of the cycle. Measured at the current head, with the shipped jitter queue and each session tagged so only new-session audio counts. The jitter queue is byte-identical to the head I first measured, and the mobile Dart transport is untouched since then, so the route and its cost carry forward:
The 25 minute row is the one that shows my error. Under the law I published it should have been free. A released slot hears 2998 of 3000 packets in every case, which is the control. Same numbers on iOS and Android. So a long call is not self-healing. Sweeping one full cycle at even intervals (512 samples), the mean silence is 164.5 seconds and the worst case is 655.36 seconds. About 46 percent of rejoins cost more than a minute of silence and about 27 percent cost more than five minutes. The test arm needs to be strongerMy earlier suggestion of one recycled-slot arm is too weak. A single arm can sit in the near-zero half of the cycle and pass while the defect is fully present. Please use two departed-airtime values that fall in opposite halves:
Everything else in my previous comment stands as written. |
|
🤖 Re-reviewed #6056 at Two credits I owed this commit do not survive measurement at this head. I state them accurately below. Fixed at this head, each verified against a controlTalker churn above 15 senders. Desktop same-second phase ordering. Correction 1: the relay
|
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5efcaada9c
ℹ️ 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".
|
🤖 Re-reviewed at Credit: the audio-focus chain is fixedMy earlier blocker was that a transient focus loss ended the call instead of pausing it. The new capture loop closes it. It snapshots the interruption epoch before it reads, waits while interrupted instead of reading, and treats a negative read whose epoch has moved as an expected stop. I ran the old loop shape and the new loop shape against identical inputs: The old arm failing and the new arm passing on the same inputs is what makes this a fix and not a coincidence. The device receipt in Above 15 simultaneous senders a quiet speaker can never be heardThis corroborates the missing-expiry finding in the automated review at this head (3798836339). The mechanism there is right. One precondition needs correcting, and the correction is not in the author's favour. The finding says calls with more than 16 participants. The real precondition is more than 15 simultaneous senders, which is a different and easier condition to reach: the relay admits 25 peers per room while mobile capacity is 15, and a peer only sends while unmuted, so 16 people who take turns talking can trip this in a room of 20. Measured against this head's selector on both platforms, controls first: The last row is the important one. A peer who spoke loudly and then muted keeps its slot forever, because a muted peer sends nothing and so its recorded level never decays. Talking longer does not heal it. Provenance, and my share of it: the strictly-louder admission rule came from my own churn blocker and from the automated review, it does fix the churn, and neither of us specified the inactivity expiry that would have stopped it from becoming starvation. The missing expiry is on the askers too. Fix shape: give a slot an inactivity deadline, or clamp room capacity to the mobile selector capacity so the condition cannot arise. Either one closes it. A regression test worth having: 16 senders where the loudest peer stops sending, and the quiet peer becomes audible within a bounded time. Android only: one rejected packet skips the playback drain for everybodyThis is independent of the selector and it degrades the call for every listener, not only for the starved talker. In the Android playback loop, a packet whose peer did not earn a slot takes the iOS does not share this. Its drain runs on a separate timer tick, so a rejected packet cannot skip it. Fix shape: drop the packet without leaving the iteration, so the drain block still runs. Correction to my own cost-law commentTwo errors in my earlier comment about recycled-slot silence were mine. The jitter queue is byte-identical between the previous head and this one, so both corrections still apply here. 1. My mutation rule was wrong, and following it produces a green test over the live defect. I said to pick two departed-airtime values in opposite halves of the 21.8 minute cycle. The arms I listed are fine. The rule beside them is not. The correct rule is: Measured, 3000-frame arm, arriving counter at 0: "One short and one long" is exactly the wrong heuristic: 15 and 33 minutes both read as long, and both pass on broken code. 2. The cost is a function of two counters, not of the departed peer's airtime alone. N is the departed peer's transmitted frame count, A is the arriving peer's own counter. Verified over 17 pairs with both controls passing. A scorer must credit the three-packet warmup: in the free case the arriving peer is first heard at frame 2, not frame 0. My first run of this law printed mismatches that were only the warmup, so a test that does not allow for it will report a false failure. A has two reachable magnitudes. A fresh joiner starts near 0, and then the cost is the departed peer's airtime, which is the severe case. A participant who reconnects carries its own counter, and then the cost is only the skew between the two peers. 3. The distribution I gave holds only under a uniform assumption I did not state. Enumerating the period instead of sampling it: For any speaker who held the mic under about 10.9 minutes the free fraction is exactly zero, and the silence equals their airtime. So "half the time it is free" is not a mitigation that survives a realistic call. My posted worst case and mean were sampling error against these enumerated values and are close enough to leave standing; the framing around them was not. This closes the cost-law thread from my side. Further refinement of these constants stays out of your way unless it changes the fix shape or the test arms. |
|
🤖 Re-reviewed at The re-admission fix only works in one direction, and the other direction misattributes live audioCredit first: Two arms, same probe, only the revision order changed: It does not heal. The client keeps the high retained revision, so the fresh room's own deltas are rejected as stale until its counter climbs back past that value: No resync is requested, no error is surfaced, and the phase stays This is a regression, and it comes from the fix. I ran the same probe bytes against the pre-fix version of the file. The descending arm is clean there: no ghosts, roster correct. The old The new test cannot see this. The re-admission test pins revision 1 and then revision 4, so it only exercises the ascending order. It is a genuine test of the arithmetic regression and it does kill that one. The ordered pair is a two-valued axis and only one value is covered. Provenance: I asked for the re-admission rebuild, it is correct, and the guard interaction it created is on me as much as on you. Fix candidate, measured rather than suggested. Scope the guard to the current connection: one flag, cleared in Bound on reachability, stated plainly. The client side is measured. The relay side is a source read only: I did not run the relay. The revision lives in the per-room admission guard, starts at zero, and a room is constructed fresh once an empty one is cleaned up, so a client that retained a high revision and rejoins a rebuilt room is admitted at a low one. Owner migration reaches the same state from the new owner's room. The route I would defend without qualification is room rebuild or owner migration under a live client. I have not executed that end to end, so treat the frequency as unmeasured and the state as reachable. Withdrawal: my mutation-arm selection rule was wrongIn my earlier comment on recycled-slot silence I told you to pick two departed-airtime values in "opposite halves" of the cycle. That sentence is wrong and I withdraw it. The two arms I actually listed, 5 and 25 minutes, are both valid, and your shipped test does kill the arithmetic regression. The correct selection rule is: 15 and 33 minutes both read as "long" under my withdrawn phrasing and both pass on broken code. This is the last cost-law text I will add to this PR. |
|
🤖 Lifecycle and teardown pass at One failing native teardown abandons a community switch
Six arms, controls first: The two consequences fail in opposite directions:
The error is an ordinary failure path, not a synthetic one. On iOS the plugin returns the error code The correct shape is already in this branch, one file away. Fix candidate, measured rather than proposed. Move the error handling inside the Test power, which is the more useful half of this. The suite reports 1529 passed both before and after the fix. The totals are identical, so no current test can tell the guarded coordinator from the unguarded one, in either direction. Three test files use the coordinator and none of them has an arm where a callback throws. This is the same shape as the re-admission test I wrote about earlier: the mechanism is covered on its success path only. Severity, stated plainly. This is a weaker class than the roster misattribution finding. No pubkey boundary is crossed and no audio is attributed to the wrong person. It is a blocker because the trigger is a normal failure, both results are visible to the user during an identity change, and the fix is small and already patterned in this branch. Non-blocking nit: a pending leave is dropped if the user joins another HuddleIf the member count lookup for a leave is still in flight and the user joins a different Huddle, The client publishes no end event and archives nothing for the Huddle it just left. The relay covers the visible part of this. I read the peer removal and auto-end path on the relay: the last peer to leave archives the channel and emits the end event under the admission guard, and it rolls the ended flag back if the archive fails. So the Huddle still ends, but a different actor does it. The remaining point is only that the epoch check asks whether this is still the current admission, while the work it guards belongs to the Huddle being left and would be safe to finish. Worth a look, not a blocker. I did not run the relay, so the backstop above is a source read. |
|
🤖 Two more results at
|
jedwards27
left a comment
jedwards27
left a comment
There was a problem hiding this comment.
Reviewed exact head 5efcaada9c3d29d76e8ab452d3a54642cd082f87 against base f956e6fe06a76e50cbd8fba1a162482e752e7f1a, the PR's foreground voice-MVP scope, VISION.md, TESTING.md, and the mobile Huddles acceptance contract. Requesting changes for one reproduced runtime defect; the current validation also has a deterministic-test gap and lacks inspectable native-device evidence for the MVP's core microphone/route/recovery behavior.
Major — same-second remote join then leave is reconstructed as still joined
desktop/src/features/huddle/hooks/useHuddleParticipantRoster.ts:63-94 orders an unrevisioned PARTICIPANT_LEFT before a revised PARTICIPANT_JOINED for the same participant. That handles the opposite sequence (leave then rejoin), but reverses an ordinary join then leave.
The relay produces exactly this mixed-revision shape: a non-owner ingress join emits the owner's revision (crates/buzz-relay/src/audio/handler.rs:611-677), while ingress disconnect deliberately omits revision (crates/buzz-relay/src/audio/handler.rs:836-878). Nostr timestamps have one-second resolution, so a quick join and disconnect can share created_at; reconstruction then applies leave first and join second. Desktop consequently shows a departed participant until later lifecycle/fallback reconciliation happens.
At the pinned clean head, a source-level repro using start at t=1, remote join at t=2 with roster_revision=1, and remote leave at t=2 without revision produced:
{"expected":["creator"],"actual":["creator","mobile"]}The repro exited 42. The checked-in suite passes but only covers unrevisioned leave then revised rejoin (useHuddleParticipantRoster.test.mjs:147-172), not the reversed causal sequence. A one-sided null-revision heuristic cannot infer both directions; please make the relay/client ordering contract authoritative for both and add a regression that proves join→leave and leave→rejoin.
Validation/test-quality gaps
mobile/test/shared/huddle/huddle_transport_test.dart:357-383uses a fixed 20 ms sleep and then requires all 50 retained frames. Production drains one frame per zero-duration timer turn (mobile/lib/shared/huddle/huddle_transport.dart:769-773,790-807), so this is scheduler-dependent. I independently reproduced the focused test failing at this exact head with only 30 frames delivered (expected 50); another review execution observed 9 and 1. The first retained sequence began at 50, so this does not establish a product queue defect, but it does make the claimed backpressure regression nondeterministic. Replace wall-clock sleep with deterministic drain synchronization/fake scheduling and mutation-check the cap/drop-oldest behavior.- The PR claims physical Pixel/iPhone review but supplies no exact-head receipt, device/runtime identity, or observable outcomes.
mobile/HUDDLES.md:111-114correctly says static tests cannot establish iOS permission, route/headset, interruption recovery, or two-way Desktop audio. Before merge, attach exact-head physical iOS and Android evidence for permission deny/grant, receiver↔speaker and connected headset, interruption→resume without ejection, background/detach microphone release, and two-way Desktop audio. Native interruption paths currently lack a checked-in focus-loss→blocked-read wake/error→gain regression as well.
Exact-head evidence
- Live PR head and local
HEAD:5efcaada9c3d29d76e8ab452d3a54642cd082f87; clean tree after probes. - Full Desktop JS package suite: 4,961 passed, 0 failed.
- Focused transport test: failed, expected 50 frames, received 30.
- Focused mobile Huddle UI rows: 14/14 passed in an independent clean-tree execution, including lifecycle/admission, dense roster, reactions, community transition, semantics, and reduced-motion paths.
- CI is green at this head, but does not exercise the missing causal ordering or provide physical-device proof.
Please fix the roster contract and deterministic regression, then provide the native-device receipts. Any new head needs delta review before this verdict can change.
|
🤖 Three teardown findings at First, the overlap, so you can skip what you already have. Codex's open P2 on this file ("Await failed-session cleanup before changing communities") is about the unawaited 1. Awaiting
|
| arm | result |
|---|---|
| transition with a parked background leave | run() returns, mediaDispose=0, transportDispose=0, phase=connected |
| control, no parked leave, same gate | parks first, then mediaDispose=1, phase=idle |
The control discriminates in both directions. Traced one step further, to the consequence: switching the relay config after run() returns, as switchCommunity does, leaves the old transport live and a subsequently captured frame still reaches it (transportStillOldCommunity=true, frames observed after the switch). A positive control on the frame path ran first, so the quiet arm is a measured silence and not a dead rig. Net effect: capture and the Huddle socket survive the identity switch and keep publishing audio authenticated to the previous community's key.
This is the same hazard class as the transition fix in 5efcaada9, one layer down: the fix made the coordinator await teardown, and memoization is what makes that await occasionally vacuous.
2. A leave that loses its epoch race abandons the backing channel's lifecycle permanently
_finishLeaveLifecycle awaits the humanCount lookup, then re-checks admissionToken.epoch != _admissionEpoch. A start() or join() in that window bumps the epoch (those are the only two sites that do), the guard fires, and the previous backing channel's end/archive/leave work is dropped with nothing left to finish it.
| arm | result | control |
|---|---|---|
| second start, departing user was last human | nothing archived, no ended published | control: archived, ended published |
| second start, others remain | old backing channel never left | control: old backing channel left |
| join a different huddle | same loss | joining the same one correctly skips |
| second start fails | old backing channel still not archived, new one is | — |
| every later user exit | only ever handles the current backing channel | — |
Why it is permanent rather than deferred, and this is the part worth fixing first: the dedupe ledger is written at the top of the method, before the await.
if (!_finishedLifecycleAdmissions.add(admissionToken)) return;The admission is recorded as finished before the work happens, so the early return leaves a ledger entry asserting completion of work that never ran. No retry can ever pick it up. Moving the add after the epoch re-check, or removing the entry on the guarded return, makes this recoverable.
Reachability is measured, not assumed. Mid-leave, while the lookup is parked, both UI gates are already clear (phase=idle, isInSession=false, controller state false), so a second start is reachable from the UI. The window is the humanCount relay round trip: fetchHistory carries an 8 s default timeout and first awaits the rate-limit gate. The failing-second-start arm needs no timing luck at all.
ref.onDispose reaches the same end state by a different route: local media and transport are released (by HuddleSessionNotifier's own dispose), but nothing is published or archived.
3. The relay backstop covers the archive but not the membership
This is the reason finding 2 is a real leak rather than cosmetic, and it is the one asymmetry I would not have guessed:
- Backstopped.
remove_peer_and_check_endedreleases the peer index, bumps the roster revision, and elects exactly one auto-end winner under the guard; the handler archives, emits 48103, and rollsendedback if the archive fails. So a missed end/archive is repaired server-side. - Not backstopped. There is no kind 9022 membership removal anywhere in the relay's audio module, and
archive_channelinbuzz-dbonly issuesUPDATE channels SET archived_at = NOW(); it does not touchchannel_members. Both are source reads, not executions, each run with a positive control in the same file so a clean result is not a broken grep.
So the user stays a member of an abandoned, archived backing channel, and nothing anywhere cleans that up.
One precedent worth pointing at: ChannelActions already has an isCommunityValid guard for exactly this "the community moved under me" hazard, but its three call sites are all inside addMembers. leaveChannel, archiveChannel and the huddle-ended publication have no equivalent.
Severity bound, stated plainly
Two publishers can drive the background leave from one real backgrounding, five seconds apart: the relay hook fires from _pauseAfterCallbacks behind a 5 s grace timer, while the app-lifecycle listener fires immediately. That makes the memoized-leave collision routine rather than exotic. But it self-heals on the next pause, so on its own it is one pause cycle of a live mic in the background, not a permanent condition, and I would treat it as non-blocking.
Holding severity down further: there is no UIBackgroundModes key in the iOS Info.plist and no foreground-service declaration in the Android manifest (both checked with positive controls; both files do carry mic permissions, so the greps can see content). The OS should suspend capture shortly after backgrounding. The finding that does not benefit from that bound is finding 1, where the app stays in the foreground across a community switch, and finding 2, which is permanent by construction.
Numbers are scoped to 5efcaada9; the controller is the only in-scope Dart construct that moved across recent heads, so a re-run is owed after the next change to it.
|
🤖 Correction to my comment above (issuecomment-5323770634), finding 2: I withdraw the closing sentence "Moving the Severity, the five arms, and the 9022 backstop asymmetry in that comment are unchanged; only the remedy changed, and it is larger than I stated. |
|
🤖 One requirement on the retention remedy I named in issuecomment-5323859662, because the obvious implementation of it was measured and it is worse than the bug: retaining the abandoned admission and draining it later does recover the orphaned channel, but the drain must skip any retained id equal to the session's current |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bf111d0721
ℹ️ 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
Verdict: REQUEST CHANGES
Reviewed: f88cda9eb886500ec7d205e1d265ac6f654aa433..bf111d0721603fc50dbd82e37afb3974f1286239 (exact head bf111d0721603fc50dbd82e37afb3974f1286239, clean worktrees)
Risk: high. This adds a relay-backed, identity-attributed, native full-duplex voice path whose correctness depends on relay allocation, ordered roster/media delivery, mobile lifecycle recovery, permissions, and OS audio routing.
Blocking findings
1. Delayed media can be attributed to a replacement identity after peer-index wrap
AdmissionGuard::alloc rotates through 0..=254, while removal immediately releases the departed peer's index for later reuse (crates/buzz-relay/src/audio/room.rs:118-170,346-365). The relay-to-client wire frame carries only peer index | header | Opus payload (mobile/lib/shared/huddle/huddle_wire.dart:38-59), and mobile accepts/drains a frame whenever that index exists in the current roster (mobile/lib/shared/huddle/huddle_transport.dart:766-805) before resolving the current pubkey for speaker attribution (mobile/lib/shared/huddle/huddle_session.dart:515-523). There is no per-index epoch in media.
A causal exact-head probe reproduced this sequence: peer A held index 0 and queued old-audio; A left; allocator churn wrapped; another pubkey acquired index 0; the keeper then dequeued A's old frame while the authoritative roster mapped 0 to the replacement pubkey. Mesh generation fencing does not distinguish index assignments within one room generation. This can misattribute speech and can also apply the replacement identity's human/agent STT inclusion policy to stale audio, so it is not merely a cosmetic active-speaker error.
Please make media identity unambiguous across reassignment: never reuse an index in one room generation, carry a peer epoch in roster and media, or roll generation while invalidating prior queues before reuse. Add a checked-in causal regression delivering a pre-reassignment frame after the slot maps to another pubkey.
2. The iOS permission-denial recovery action cannot recover
Once iOS microphone authorization is denied, the plugin returns denied immediately rather than presenting another system prompt (mobile/ios/Runner/HuddleMediaPlugin.swift:101-115). The session collapses every non-granted result to permissionDenied (mobile/lib/shared/huddle/huddle_session.dart:264-271), but the failed call UI offers generic Try again, which only calls join() again (mobile/lib/features/channels/channel_detail_page/huddle_sheet.dart:610-642). The only specialized recovery branch handles not a member (huddle_sheet.dart:785-786). A user who denies once is therefore sent through a truthful-looking recovery action that deterministically returns to the same failure, with no Settings guidance or action.
Please distinguish permanent denial/restriction from transient failures, provide an explicit OS-settings recovery path, and add widget coverage proving that denial does not render generic Try again. Physical iOS acceptance should exercise deny → Settings grant → successful join.
Verified fixes and validation
The prior reconnect-exhaustion and different-room teardown blockers are fixed on this head:
- Relay controls passed for a continuously seated peer surviving more than the index space of reconnects and for no simultaneous index reuse.
- Mobile controller regressions passed for different-Huddle supersession during human-count and end-publication awaits, same-Huddle rejoin during end publication, and background pause waiting for failed-session lifecycle cleanup.
- The new background-pause regression passed and causally failed when its added awaits were removed; the worktree was then restored clean.
just mobile-check: PASS.just mobile-test: PASS, 1,620 tests.git diff --check f88cda9eb886500ec7d205e1d265ac6f654aa433..bf111d0721603fc50dbd82e37afb3974f1286239: PASS.- Exact-head GitHub checks observed: 28 successful, 3 skipped.
- Controls retain button/toggled semantics, drawer semantics, and reduced-motion handling; no additional material accessibility/layout regression was found in the reviewed Huddle UI paths.
Native evidence / residual risk
No inspectable exact-head physical-device receipt, recording, logs, or observed matrix was available. This is an evidence gap, not a claimed native reproduction. mobile/HUDDLES.md:111-114 requires physical iOS verification of permission, receiver/speaker/headset routing, interruption recovery, and two-way Desktop audio. Before clearance, exact-head iOS/Android acceptance should cover permission denial/grant recovery, output routes/headset, interruption resume, pause/detach microphone release, and two-way Desktop audio.
|
Both blocking findings from the 08-20 review are addressed in 1. Delayed media attributed to a replacement identity after peer-index wrap — FIXEDAdded a per-index occupancy epoch and fence stale media on both clients rather than retiring indices (retire-forever would reintroduce the index-exhaustion regression the rotation allocator was built to avoid).
2. iOS permission-denial recovery cannot recover — FIXED
Validation (at
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 52f8ddb8d6
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
buzz/crates/buzz-relay/src/audio/handler.rs
Line 456 in b27f2de
Remove the production-path expect
This newly introduced expect() executes in the production mesh admission handler, contrary to the repository rule prohibiting new production-path unwrap()/expect() calls. Although the preceding match currently establishes the variant, retaining a panic point makes later control-flow changes capable of crashing the connection task; destructure the matched outcome directly or return/log a recoverable invariant error instead.
AGENTS.md reference: AGENTS.md:L134-L137
ℹ️ 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
Verdict: REQUEST CHANGES
Reviewed: da818eddc2f470c006a1073c8c5452f8a989f272..b27f2def0b082d111fe5883a1815bef974f76468 (prior-blocker delta bf111d0721603fc50dbd82e37afb3974f1286239..b27f2def0b082d111fe5883a1815bef974f76468; exact head b27f2def0b082d111fe5883a1815bef974f76468; clean review trees; live head rechecked before submission)
Risk: high. This changes the identity carried by every relay→client audio frame across relay, Desktop, and mobile, while also owning native microphone-denial recovery.
Blocking findings
1. The epoch fix changes the v2 binary frame layout without changing the negotiated protocol version
Desktop still advertises PROTOCOL_VERSION = 2 while now defining relay→client v2 as [peer_index][epoch][8-byte header][opus] (desktop/src-tauri/src/huddle/wire.rs:12-23,42-48). Mobile likewise requests v2 and decodes a two-byte relay prefix (mobile/lib/shared/huddle/huddle_wire.dart:5-16,38-61). The relay still advertises max v2 and admits versions 1..=2 (crates/buzz-relay/src/audio/handler.rs:121-141,419-444); room compatibility is only numeric equality (crates/buzz-relay/src/audio/room.rs:264-307), yet fan-out now unconditionally prepends index and epoch (room.rs:454-467).
A released old-v2 client and this new-v2 client are therefore admitted to the same v2-pinned room despite incompatible framing. Old v2 interprets the epoch as header byte 0; new v2 interprets an old relay frame’s header byte 0 as epoch and shifts the remaining header/payload. During staged rollout that corrupts or drops audio. JSON epoch defaults do not make this binary shift compatible.
Please negotiate/pin a new wire version (v3), or provide a genuinely backward-compatible encoding, and add mixed-generation tests proving incompatible clients cannot share a room.
2. Same-pubkey index reuse does not reset decoder/playout state when the occupancy epoch changes
The new ingress epoch comparison correctly rejects a stale frame against the current roster (mobile/lib/shared/huddle/huddle_transport.dart:822-829), but occupancy replacement is still detected by pubkey alone:
- Desktop retains NetEq/Player, frame counters, active state, and speaker level when an index keeps the same pubkey even if its epoch changes (
desktop/src-tauri/src/huddle/playout.rs:488-532). - Mobile joined/admission/roster paths purge ingress and emit
replacedonly when the pubkey changes (mobile/lib/shared/huddle/huddle_transport.dart:516-539,558-578,644-667). - Mobile clears queued/native playback only for
leftorreplaced(mobile/lib/shared/huddle/huddle_session.dart:466-480).
Consequently, a same-user reconnect that reuses an index with a new epoch can inherit queued native audio and sequence/decoder/speaker state from the prior occupancy. The checked-in causal cases use different pubkeys (mobile/test/shared/huddle/huddle_transport_test.dart:255-319,438-486; mobile/test/shared/huddle/huddle_session_test.dart:138-161), so this path is not guarded.
Treat (peer_index, epoch) as occupancy identity and add a same-pubkey epoch-change regression proving ingress purge plus decoder/native playout reset before new media.
Verified fixes
The prior blockers did move in the right direction:
- Roster and media now carry an allocation epoch, and current mobile ingress fences stale-epoch frames.
- Microphone denial is explicit session state; failed-call UI renders Open Settings rather than generic Try again (
mobile/lib/shared/huddle/huddle_session.dart:50-54,277-284,315-321,659-681;mobile/lib/features/channels/channel_detail_page/huddle_sheet.dart:541-550,574-603). Native bridges open app settings on iOS and Android (mobile/ios/Runner/HuddleMediaPlugin.swift:104-138;mobile/android/app/src/main/kotlin/xyz/block/buzz/mobile/HuddleMediaPlugin.kt:91-140). The denial widget regression was mutation-proved: disabling the specialized branch made it fail, then the clean tree passed again.
Validation
At exact head with clean trees:
- Relay room suite: 15 passed.
- Focused mobile wire/transport/session suites: 32 passed.
- Focused mobile media/session suites: 15 passed.
- Permission-denial widget regression: passed and mutation-failed when its branch was disabled.
git diff --check bf111d0721603fc50dbd82e37afb3974f1286239..b27f2def0b082d111fe5883a1815bef974f76468: passed.- GitHub exact-head checks: 28 successful / 3 skipped.
- Desktop Tauri tests did not execute locally because
desktop/src-tauri/binaries/buzz-acp-aarch64-apple-darwinwas absent; this is unvalidated coverage, not a reproduced product failure. mobile/HUDDLES.md:56-65still documents the obsolete one-byte relay prefix and should be updated with the protocol correction.
Native evidence / residual risk
No inspectable exact-head physical-device receipt, recording, logs, or acceptance matrix was available. This is an evidence gap, not a claimed native failure. mobile/HUDDLES.md:111-114 requires physical acceptance for this boundary. After the protocol/lifecycle blockers are fixed, exact-head iOS and Android evidence should cover deny → Settings grant → successful join, receiver/speaker and wired/Bluetooth routes, interruption recovery, pause/detach microphone release, and mobile↔Desktop two-way audio.
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.
Verdict: REQUEST CHANGES
Reviewed: exact head b27f2def0b082d111fe5883a1815bef974f76468 (merge-base da818eddc2f470c006a1073c8c5452f8a989f272).
Blocking findings
[P1] Owner-side removal of the final remote peer never ends the huddle
The owner admits cross-pod participants into its authoritative Room, but both remote teardown paths only call room.remove_peer() and broadcast the roster delta: explicit unregister at crates/buzz-relay/src/audio/join.rs:1298-1308, and stream-close cleanup at join.rs:1347-1359. Neither path performs the authoritative empty-room transition.
The equivalent local-owner cleanup deliberately uses remove_peer_and_check_ended, then archives the channel, emits the end event, removes the empty room, and generation-fenced releases the owner lease (crates/buzz-relay/src/audio/handler.rs:865-968). Therefore, when all participants are routed through non-owner pods and the last participant leaves, the owner can retain an empty room and keep renewing its lease indefinitely. The ephemeral huddle does not auto-end/archive, and later joins can encounter stale room lifetime state.
Please put remote owner-side removal through the same atomic last-peer/end/archive/cleanup/release lifecycle (or move that lifecycle behind one manager-owned operation), with coverage for explicit unregister and control-stream loss when the final participant is remote.
[P1] A non-resumable iOS interruption strands the call in a false-connected state
mobile/ios/Runner/HuddleMediaPlugin.swift:420-451 emits interruption start, but interruption end returns silently when .shouldResume is absent (:438). Capture/playout remain interrupted and Flutter receives no terminal or recovery event. Flutter only transitions out of HuddleSessionPhase.interrupted after an active media event with isInterrupted == false (mobile/lib/shared/huddle/huddle_session.dart:245-265), while the call sheet intentionally treats interrupted as connected (mobile/lib/features/channels/channel_detail_page/huddle_sheet.dart:541-543).
A phone/Siri interruption that iOS declines to resume can therefore leave audio stopped indefinitely while the controls and participant UI still represent a live connected call. Android does not have this terminal hole: every focus change emits interruption state and focus gain emits resumed (mobile/android/app/src/main/kotlin/xyz/block/buzz/mobile/HuddleMediaPlugin.kt:42-52). Existing Flutter coverage exercises only begin → successful resume (mobile/test/shared/huddle/huddle_session_test.dart:187-209).
Please emit a terminal/failure state or explicitly tear down and expose retry when .shouldResume is absent, and add native/Flutter coverage for that branch.
Validation
cargo test -p buzz-relay --lib audio::room::tests: 15 passed.- Focused Flutter Huddle transport/session/wire tests: 32 passed.
- Exact-head GitHub checks are green, including Mobile, relay/backend integration, Desktop, lint, security, and builds.
- A merge-tree against fetched
origin/mainaea0ef8df9fc24d9aa8bf5c761ab2910026a601bcurrently conflicts indesktop/src-tauri/src/huddle/stt.rsandmobile/ios/RunnerTests/RunnerTests.swift; that is integration work in addition to the lifecycle blockers above.
Dismissed at Wes’s direction to clear the merge block. The two P1 findings remain documented and are being accepted as merge risk; this dismissal does not indicate they were fixed.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ef3229d621
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3d8ad0b2c8
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3f7cadb17c
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9a6abf40c0
ℹ️ 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".
| return; | ||
| } | ||
| if (!_finishedLifecycleAdmissions.add(admissionToken)) return; | ||
| if (humansRemaining <= 1 && parentChannelId != null) { |
There was a problem hiding this comment.
When one identity has concurrent admissions from desktop and mobile, the kind:39002 snapshot still contains only one human member, so humansRemaining is 1 when either device leaves. This branch then publishes the end event and archives the backing channel even though the relay room still contains the other admission, disconnecting that active device. Base the last-participant decision on active admission IDs/socket count, or let the relay's room-empty path perform the automatic end.
Useful? React with 👍 / 👎.
Signed-off-by: kenny lopez <klopez4212@gmail.com>
9a6abf4 to
cff6e82
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cff6e82414
ℹ️ 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".
| if !signer_created_backing | ||
| || backing.channel_type != "stream" | ||
| || backing.visibility != "private" | ||
| || backing.ttl_seconds != Some(expected_ttl) | ||
| || backing.archived_at.is_some() |
There was a problem hiding this comment.
When a backing-channel creator publishes otherwise valid kind:48100 starts for the same private stream in two parent channels, both pass this validation because it checks only the backing properties and signer. ensure_membership then independently accepts either parent linkage and auto-adds members of both parents to the single audio room keyed by the backing UUID, causing participants who share no parent channel to hear each other and splitting lifecycle events between channels. Enforce one canonical parent linkage per backing channel, including for concurrent starts.
AGENTS.md reference: AGENTS.md:L180-L182
Useful? React with 👍 / 👎.
jedwards27
left a comment
jedwards27
left a comment
There was a problem hiding this comment.
Verdict: COMMENT — code blockers cleared; physical-device acceptance remains a release gate.
Reviewed: aeb741fd31044ec560d953b0986dec2e7e93e2c6..cff6e82414abda715319abd5ef541e6274ee3448 (exact head cff6e82414abda715319abd5ef541e6274ee3448)
Risk: high. This changes a versioned relay/mobile/Desktop audio protocol, peer-occupancy identity and decoder lifecycle, plus iOS/Android microphone, routing, interruption, and app-lifecycle behavior.
The two previously blocking code findings are resolved:
-
Version negotiation and mixed-generation framing: Desktop and mobile authenticate as protocol v3; the relay validates versions before admission, pins the room version, rejects mismatches without consuming a seat, preserves that pin across churn, and emits
upgrade_required. Fan-out retains the released one-byte v1/v2 prefix and uses[peer_index][epoch]only for v3 (desktop/src-tauri/src/huddle/relay_api.rs:110-122,mobile/lib/shared/huddle/huddle_auth.dart:58-85,crates/buzz-relay/src/audio/handler.rs:419-445,crates/buzz-relay/src/audio/room.rs:267-341,462-487). Focused tests cover mismatch/no-admission, pin persistence/reset, v2/v3 bytes, full-vs-mismatch precedence, mesh framing, mobile golden frames, and surfaced upgrade errors. A mutation deleting the mismatch gate causally failedadmit_rejects_mismatched_version. -
Same-pubkey reconnect cleanup: occupancy is now
(peer_index, pubkey, epoch). Relay increments and stamps epochs on index reuse; Desktop and mobile fence stale media before decoder allocation and purge decoder/player, queued ingress, speaker/floor, and native playout state when either pubkey or epoch changes (crates/buzz-relay/src/audio/room.rs:196-217,309-341,desktop/src-tauri/src/huddle/playout.rs:152-177,544-625,mobile/lib/shared/huddle/huddle_transport.dart:509-545,651-675,769-780,mobile/lib/shared/huddle/huddle_session.dart:466-480). Regression tests cover stale frames after reassignment, same-pubkey/new-epoch replacement, re-admission, queued-frame purge, and native playback reset. Removing epoch from mobile occupancy equality causally changed the expectedreplacedevent tojoinedand failed the regression.
Exact-head validation: focused relay room tests passed 15/15; focused mobile wire/transport/session tests passed 35/35; an independent full Flutter run passed 1,661 tests; an independent full Desktop Tauri workspace run passed. Full local cargo test -p buzz-relay completed 906 passed, 1 failed, 47 ignored; the sole failure, api::mesh_demo::tests::demo_join_forwarded_arm_round_trips_echo (504 vs 200), reproduced alone and its source is unchanged in this PR range, so it is not attributed to this change, but the local package gate is not literally green. All current GitHub checks are completed green, including Mobile, Desktop Core/build, relay/backend integration, Desktop E2E, Rust lint, security, and cross-compiles.
Manual/native evidence: no exact-head physical iOS or Android receipt was supplied or produced. Automated tests do not establish microphone denial → Settings grant → successful join, receiver/speaker/wired/Bluetooth routing, phone-call/audio-session interruption recovery, foreground/background transitions and microphone release, or live two-way Mobile ↔ Desktop relay audio. mobile/HUDDLES.md:108-117 itself identifies these as device acceptance checks and scopes the MVP foreground-only.
There is no remaining code defect from this review. I am not granting an unconditional approval because the evidence does not yet match the native boundary. Please attach exact-head physical iOS and Android acceptance receipts for the documented matrix before release/merge approval. Any new head invalidates this result.
— :bot: Jude’s code review agent
…nd-join-channels-in-mobile * origin/main: Add mobile Huddles voice MVP (#6056) feat(desktop-messages): keep agents addressed across messages (#6315) fix(desktop): remove Buzz entity link previews (#6512) Signed-off-by: Tom Brow <tomb@block.xyz> # Conflicts: # mobile/lib/features/channels/channels_provider.dart # mobile/test/features/channels/channels_provider_test.dart
…ake-fix * origin/main: (33 commits) perf(desktop): make the Projects surface render-cheap (#6460) refactor(acp): clarify agent prompt sections (#6501) Add mobile Huddles voice MVP (#6056) feat(desktop-messages): keep agents addressed across messages (#6315) fix(desktop): remove Buzz entity link previews (#6512) fix(composer): preserve caret when inserting mentions mid-message (#6531) chore(deps): update rust crate async-trait to v0.1.92 (#6094) chore(deps): update dependency sonner to v2.0.8 (#6093) chore(deps): update rust crate http-body-util to v0.1.4 (#5452) chore(deps): update rust crate http to v1.4.2 (#5451) chore(deps): update rust crate futures-util to v0.3.33 (#5448) chore(deps): update rust crate futures to v0.3.33 (#5445) chore(deps): update dependency @tauri-apps/api to v2.11.1 (#5444) chore(deps): update ubuntu:24.04 docker digest to 561618e (#5442) chore(deps): update swatinem/rust-cache digest to 6323deb (#5441) fix(desktop): restore true zoom by scaling the root rem (#6514) chore(desktop): drop unused ORIGINAL_CONTENT from empty-edit-delete spec (#6517) feat(workflows): clarify workflow setup and activation (#6470) perf(desktop): stop the Projects fan refetching on re-entry and running after leave (#6458) perf(desktop): keep the member roster off the channel-switch path (#6456) ... Signed-off-by: Duncan <dcfd242e557282d7a1e2cf2e6877522682f1e5c6156dc92ca7d90eaedd3b0f95@buzz.block.builderlab.xyz>
…ions-sync-fixes * origin/main: (22 commits) Downgrade mobile Huddles to audio protocol v2 (#6558) perf(desktop): make the Projects surface render-cheap (#6460) refactor(acp): clarify agent prompt sections (#6501) Add mobile Huddles voice MVP (#6056) feat(desktop-messages): keep agents addressed across messages (#6315) fix(desktop): remove Buzz entity link previews (#6512) fix(composer): preserve caret when inserting mentions mid-message (#6531) chore(deps): update rust crate async-trait to v0.1.92 (#6094) chore(deps): update dependency sonner to v2.0.8 (#6093) chore(deps): update rust crate http-body-util to v0.1.4 (#5452) chore(deps): update rust crate http to v1.4.2 (#5451) chore(deps): update rust crate futures-util to v0.3.33 (#5448) chore(deps): update rust crate futures to v0.3.33 (#5445) chore(deps): update dependency @tauri-apps/api to v2.11.1 (#5444) chore(deps): update ubuntu:24.04 docker digest to 561618e (#5442) chore(deps): update swatinem/rust-cache digest to 6323deb (#5441) fix(desktop): restore true zoom by scaling the root rem (#6514) chore(desktop): drop unused ORIGINAL_CONTENT from empty-edit-delete spec (#6517) feat(workflows): clarify workflow setup and activation (#6470) perf(desktop): stop the Projects fan refetching on re-entry and running after leave (#6458) ... Signed-off-by: Duncan <dcfd242e557282d7a1e2cf2e6877522682f1e5c6156dc92ca7d90eaedd3b0f95@buzz.block.builderlab.xyz>
4 participants
Summary
Scope
Foreground human-to-human voice MVP only. Agent setup/transcripts, background calling, recording, and advanced device controls remain out of scope.
Validation
just mobile-checkjust mobile-test— 1,500 passedjust desktop-checkandjust desktop-test— 4,957 passed