fix: stop tearing down meeting recordings when the observed identity changes - #375
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughMeeting auto-stop matching now tracks calendar event identity, supports URL-less dedicated meeting applications by bundle ID, excludes browser bundle fallback, preserves identity through session stabilization, and routes meeting tests to the meetings CI shard. ChangesMeeting auto-stop matching
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant MeetingCandidateResolver
participant MeetingMediaSessionTracker
participant MeetingAutoStopPolicy
MeetingCandidateResolver->>MeetingMediaSessionTracker: provide candidate with calendarEventID
MeetingMediaSessionTracker->>MeetingAutoStopPolicy: provide stabilized candidate
MeetingAutoStopPolicy->>MeetingAutoStopPolicy: match calendar event or dedicated-app identity
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThe PR expands meeting auto-stop identity matching so recordings can recover when media-session identities rotate, and propagates calendar identity through candidate stabilization.
Confidence Score: 4/5The PR does not yet appear safe to merge because both fallback identities can keep an existing recording attached to unrelated meeting activity. Dedicated-app recovery still treats bundle equality as meeting identity across distinct calls, while snapshot-wide calendar attribution lets unrelated qualifying candidates refresh the armed source and prevent auto-stop. Files Needing Attention: native/MuesliNative/Sources/MuesliNativeApp/MeetingAutoStopPolicy.swift; native/MuesliNative/Sources/MuesliNativeApp/MeetingCandidateResolver.swift
|
| Filename | Overview |
|---|---|
| native/MuesliNative/Sources/MuesliNativeApp/MeetingAutoStopPolicy.swift | Adds calendar-event and dedicated-app fallback identities to auto-stop matching. |
| native/MuesliNative/Sources/MuesliNativeApp/MeetingCandidateResolver.swift | Propagates the active snapshot calendar event onto candidates from browser, app-audio, and fallback branches. |
| native/MuesliNative/Sources/MuesliNativeApp/MeetingMediaSessionTracker.swift | Preserves and refreshes calendar identity while stabilizing media-session candidates. |
| native/MuesliNative/Sources/MuesliNativeApp/MuesliController.swift | Supplies calendar event identity when arming auto-stop from scheduled meeting links. |
| native/MuesliNative/Tests/MuesliTests/MeetingAutoStopPolicyTests.swift | Adds positive and negative coverage for rotated app sessions and calendar-based matching. |
| native/MuesliNative/Tests/MuesliTests/MeetingMediaSessionTrackerTests.swift | Verifies calendar identity preservation and replacement across stabilized session refreshes. |
| scripts/run_ci_test_shard.sh | Adds the auto-stop policy and media-session tracker suites to the meetings shard. |
Reviews (7): Last reviewed commit: "fix: let auto-stop re-match a dedicated ..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
Flagging a trade-off I under-stated in the description, rather than leaving it to be found in review. Falling back to app identity makes the match broader: any candidate from the same dedicated app now matches the armed source. The failure mode that introduces is the mirror of the one it fixes — if a meeting app keeps its microphone and speaker streams open after a call ends, the candidate keeps matching, I still think that is the right direction, for three reasons:
If you would rather bound it, the narrow version is to require that the app also still be running (a On process: I am conscious this touches the same file as #368. If you would rather have one change here than two, I am glad to close this and fold the matching fix into that PR, or rebase on top of it once it lands — whichever keeps your review queue smaller. No attachment to it being a separate PR. |
|
I've pushed a second commit here, because a real-world failure turned out to be the same defect with a different identity — and I'd rather widen this PR than open a third one. What happenedA "Join & Record" recording was torn down 44 seconds in, with the "Meeting signal lost" prompt, while the call was still going. It isn't a one-off: the same user has 9 recordings under two minutes going back several weeks. Why
When any of that happens, "Join & Record" is hit hardest because it arms auto-stop from The changeCarry the active calendar event ID on This is tighter than the app-identity fallback in the first commit: it matches only when both sides agree on the same calendar event, so it can't bleed into an unrelated meeting, and a source with no calendar event behaves exactly as before. It's also robust to all three triggers above rather than any one of them, which matters because I could not prove from the user's data which one fired — only that every identity the matcher relies on is derived from the observation rather than the meeting. Verified by mutation: dropping the new match arm fails exactly the two behavioural tests and leaves the three negative-control tests passing. Known gapA browser meeting with no calendar event still has no stable identity, so backgrounding its tab can still tear it down. Fixing that means either treating a background tab as a live meeting or keying liveness on the browser still holding the mic — both change what counts as a live meeting for auto-start too, which is a much bigger blast radius than I'd want in this PR. Happy to follow up separately if you think that's worth doing. Still glad to reshape this however suits you — including splitting the two commits back apart if you'd rather review them independently. |
jamesagarside
changed the title
MeetingAutoStopPolicy.matches() compares a live candidate against the armed source by candidate ID, suppression ID, and normalized URL. It never consults the source's bundle ID. MeetingMediaSessionTracker mints a fresh session ID once its 30s quiet window lapses. A dedicated meeting app has no meeting URL, so when a call briefly stops reporting microphone input -- being muted, or a transient input device reconfiguration -- it reappears under an ID that can never match the armed source again. matches() returns false permanently, markSourceRecovered() can never fire, so the signal-loss warning cannot be dismissed by the meeting coming back, and 30s later a still-running meeting is finalized. The teardown is unrecoverable by construction for any non-browser meeting. Fall back to app identity when the source has no URL and is not a browser. Browsers stay excluded because one browser hosts many unrelated sessions, so its bundle ID is not a meeting identity. Also assign MeetingAutoStopPolicyTests to the meetings CI shard. It sat on the legacy-unsharded allowlist, so none of its coverage ran in CI. Signed-off-by: James Garside <james.garside@elastic.co>
Auto-stop matched a recording's source against a live candidate using only identities that describe how the meeting is currently being observed: the candidate ID, the suppression ID and the normalized room URL. All three can change while the same call is still running. The room URL is only reported while the call's browser tab is frontmost, and the candidate and suppression IDs are audio-session IDs that rotate once their idle timeout lapses. When that happens, MeetingCandidateResolver still resolves the meeting -- via its calendar-keyed fallback -- but matches() rejects it, so the disappearance grace period elapses, "Meeting signal lost" is shown and the recording is stopped mid-meeting. This hits "Join & Record" hardest, because that path arms auto-stop from the calendar event's join URL alone, so the URL is the only identity the source starts with. Carry the active calendar event ID on MeetingCandidate, learn it into MeetingAutoStopSource through the existing refinement step, and match on it. The match is scoped to a single calendar event, so it cannot bleed into an unrelated meeting, and a source with no calendar event behaves exactly as before. Signed-off-by: James Garside <james.garside@elastic.co>
0204e01 to
61746e7
Compare
|
Want your agent to iterate on Greptile's feedback? Try greploops. |
MeetingMediaSessionTracker.stabilize rebuilds the candidate to swap in the stable session identity, but it did not carry calendarEventID across. The field defaults to nil, so it was dropped silently. stabilize sits between resolve and the auto-stop matcher (MeetingMonitor.swift), so the stripped candidate is what reaches MeetingAutoStopPolicy.matches. matchesCalendarEventIdentity could therefore never fire for a media-backed candidate, and every candidate on the calendar path is media-backed: resolve only reaches the calendar fallbacks once hasMediaActivity holds. The match arm added in 0204e01 was unreachable in practice. The session now carries the calendar event the same way it already carries url and meetingTitle, preferring the newest observation and falling back to what the session already knew. MeetingMediaSessionTrackerTests sat on ci_unsharded_test_suites.txt and never ran in required CI, which is why this was not caught. Moved into the meetings shard. Signed-off-by: James Garside <james.garside@elastic.co>
|
Rebased onto current
The session now carries the calendar event the same way it already carries Why CI did not catch it. Added a regression test that asserts the session identity is rewritten while the Full suite: 1546 tests across 150 suites, passing. Separately, field testing on 2026-08-08 found the muting hypothesis in the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
native/MuesliNative/Tests/MuesliTests/MeetingMediaSessionTrackerTests.swift (1)
152-194: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the existing-session update path.
This test covers session creation and candidate rebuilding. It does not execute
session.calendarEventID = candidate.calendarEventID ?? session.calendarEventIDat Line 71. Add a second stabilization call for the same session key. Assert that a non-nilcalendar event ID replaces the stored ID. Then pass a candidate without an event ID and assert that the stored ID remains.As per coding guidelines, audio lifecycle changes should cover state-machine transitions and stale callbacks.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@native/MuesliNative/Tests/MuesliTests/MeetingMediaSessionTrackerTests.swift` around lines 152 - 194, Extend stabilizePreservesCalendarEventID to call stabilize again for the same session key, first with a candidate containing a different non-nil calendarEventID and assert the stored attribution is replaced, then with a candidate lacking calendarEventID and assert the previously stored ID remains unchanged. Ensure these calls exercise the existing-session update path rather than creating a new session.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@native/MuesliNative/Tests/MuesliTests/MeetingMediaSessionTrackerTests.swift`:
- Around line 152-194: Extend stabilizePreservesCalendarEventID to call
stabilize again for the same session key, first with a candidate containing a
different non-nil calendarEventID and assert the stored attribution is replaced,
then with a candidate lacking calendarEventID and assert the previously stored
ID remains unchanged. Ensure these calls exercise the existing-session update
path rather than creating a new session.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c30d48e8-0c8d-4183-a842-d78930249506
📒 Files selected for processing (4)
native/MuesliNative/Sources/MuesliNativeApp/MeetingMediaSessionTracker.swiftnative/MuesliNative/Tests/MuesliTests/MeetingMediaSessionTrackerTests.swiftscripts/ci_unsharded_test_suites.txtscripts/run_ci_test_shard.sh
💤 Files with no reviewable changes (1)
- scripts/ci_unsharded_test_suites.txt
MeetingCandidateResolver.resolve attributes any media activity to the current calendar event once one exists, and its last fallback returns a bare 'cal:<id>' candidate with no meeting app at all. Calendar selection does not check for a join link, so a reminder or a placeholder is an ordinary calendar event as far as detection is concerned. matchesCalendarEventIdentity therefore let a placeholder hold a recording open for the event's whole duration on the strength of unrelated microphone use — a dictation tool is enough. That was latent while the calendar arm was inert; the previous commit made it reachable. Require attributed meeting audio or a room URL on the candidate. Every calendar branch backed by real meeting media carries one of those; only the bare fallback carries neither. This is the auto-stop half. Filtering placeholder events out of detection itself is a broader behaviour change and is handled separately, behind a setting. Signed-off-by: James Garside <james.garside@elastic.co>
The existing test only exercises session creation. Every candidate after the first takes the refresh path instead, where the stored event id is updated rather than set, and that path had no coverage. Both halves matter. A live meeting emits candidates continuously and the cached calendar event drops off whenever it lapses mid-call, so treating absence as a new answer would clear the id and take matchesCalendarEventIdentity dark for the rest of the recording — the same failure as dropping it on rebuild, arriving later in the call. Verified by mutation: clearing on absence fails the preserve assertion, removing the update fails both. Neither mutation is caught by the existing creation-path test. Signed-off-by: James Garside <james.garside@elastic.co>
|
Good catch on the untested path. Added in The existing test only exercised session creation. Every candidate after the The new test drives three stabilize calls against one session: a newer event id Mutation-verified: clearing on absence fails the preserve assertion, removing |
"Join & Record" arms auto-stop from the calendar event's join link, which leaves the source with URL-derived identity only: no bundle id, no calendar event. A dedicated meeting app's candidates carry no URL, so nothing matched once the observed session id rotated, and the recording was torn down while the call was still running. The source could not recover on its own. It only refines after a successful match, and the only candidates that match a URL are browser ones — so it could never learn the meeting app's bundle id, and the dedicated-app arm added in 9a11dd1 was unreachable for this entire entry point. Two changes, each needed: - Carry the calendar event id into the source when arming from a join link. Both sides derive it from the same EventKit `eventIdentifier`, so the event is an identity they can agree on from the first candidate, without waiting for a refinement that never comes. - Key the dedicated-app arm off the *candidate* having no URL rather than the source. `refined` never clears `normalizedURL`, so requiring the source to have none disabled that arm permanently for every Join & Record recording. The browser exclusion still carries the original intent. Verified by mutation, separately for each: dropping the event id at arm time fails only the join-link calendar test; restoring the source-URL guard fails only the rotation test. Negative controls — a different calendar event, and an unrelated browser session on a source that learned a browser — keep passing. Full suite 1558/1558. MeetingAutoStopPolicyTests is assigned to a real CI shard, so these run in required CI. Signed-off-by: James Garside <james.garside@elastic.co>
|
Pushed Zoom held The recording was started from a "Join & Record" notification, which arms via
Dropping the guard alone would therefore have been inert: the arm also needs
Mutation-verified separately: dropping the event id at arm time fails only the One thing I cannot show from here: which of the two arming paths produced last |
1 participant
Summary
MeetingAutoStopPolicy.matches()compares a live candidate against the armed source by candidate ID, suppression ID, and normalized URL. It never consults the source's bundle ID.MeetingMediaSessionTrackermints a fresh session ID once its 30s quiet window lapses. A dedicated meeting app (Teams, Zoom, Webex, FaceTime) has no meeting URL, so the sequence is:matches()compares new-ID against old-ID with no URL to fall back on → false, permanently.markSourceRecovered()can therefore never fire, so the "Meeting signal lost" warning cannot be dismissed by the meeting coming back.For any non-browser meeting this teardown is unrecoverable by construction — once the session ID rotates there is no path back. That matches the report in #303: "never auto-resumed even though the device was back", during a stretch where the microphone was demonstrably still in use by Teams.
What changed
Fall back to app identity when — and only when — the source has no URL and is not a browser. Browsers stay excluded because a single browser hosts many unrelated sessions, so its bundle ID is not a meeting identity; there is a test pinning that.
This deliberately does not touch the 20s disappearance grace or the 30s warning window. It only restores the ability of a dedicated app source to re-match, which is what makes the existing recovery path reachable.
Relationship to #368
Complementary rather than overlapping. #368 changes the response to signal loss for
.detectedPromptorigins; this changes the matching that decides whether signal was lost at all. Both touchMeetingAutoStopPolicy.swiftbut different functions, and either can land first. Happy to rebase around whichever you prefer.Note on CI coverage
MeetingAutoStopPolicyTestswas on theci_unsharded_test_suites.txtlegacy allowlist, so none of its coverage has been running in CI. Assigned to themeetingsshard and removed from the allowlist;scripts/test_ci_test_shards.shpasses.Validation
Developed red-then-green rather than asserting after the fact:
mainwith exactly the expected mismatch (app:com.microsoft.teams2:session:1800000000vs...:1800000045,normalizedURL: nil), and passes after the change.swift test --filter MeetingAutoStopPolicyTests— 20 tests pass, including all 17 pre-existing.bash scripts/run_ci_test_shard.sh meetings— 373 tests / 23 suites, all passing (up from 351/22, since this suite now actually runs).bash scripts/test_ci_test_shards.sh— shard assignments verified.Still open
Whether muting is what produces these >30s gaps is app-specific and I have not verified it, so I have not claimed it as the trigger. This change is deliberately independent of the cause: it fixes the fact that any gap longer than the quiet window is unrecoverable for a dedicated app. If it turns out meeting apps do drop
kAudioProcessPropertyIsRunningInputwhile muted, then the deeper fix is for liveness to key on the call existing rather than on microphone capture — a larger change I did not want to bundle here.Contribution certification
Signed-off-bytrailer from its author under theDeveloper Certificate of Origin.
under Muesli's
MIT License.
institution, or other party that may have rights in this contribution.
other assets introduced by this pull request, including their sources
and licenses or terms.
resulting changes.
Third-party materials
None.
AI assistance
Claude Code was used to locate the root cause, implement the change, and write the tests. All changes were reviewed and tested locally, including verifying the new test fails against unpatched code.
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by CodeRabbit
Bug Fixes
Tests