fix(calling): handle 409 error for keepalive - #5172
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 65f0ca8bb0
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| await this.performHardStopCleanup(METHODS.HANDLE_409_KEEPALIVE_FAILURE, { | ||
| reason: HARD_STOP_REASON.SESSION_SUPERSEDED, | ||
| error: lineError, | ||
| }); |
There was a problem hiding this comment.
End active calls when the session is superseded
When a second tab or device supersedes a registration during an active call, this path marks the line inactive and disconnects its transport without ending the call retained by CallManager. Unlike handleRegistrationDownEvent(), which explicitly calls activeCall?.end(), the 409 path leaves the call, media connection, and call timers alive even though no further Mobius events can arrive, so consumers may continue displaying a stale established call. End the active call(s) before performing this hard-stop cleanup.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
It's already handled. SDK receives "normal_disconnect" event from mercury and mobius-socket as soon as the same user registers in a different session. Vidcast is attached in the PR description.
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
Kesari3008
left a comment
There was a problem hiding this comment.
Let's discuss the comments if needed
| logContext | ||
| ); | ||
|
|
||
| if (Number(error.statusCode) === ERROR_CODE.CONFLICT) { |
There was a problem hiding this comment.
Why are we not adding a separate switchcase in handleRegistrationError itself where we are addressing all the error codes for this error as well. We should move the handling there. Anyway it seems all we need to do is invoke the handler
| this.emit(event); | ||
| break; | ||
| case LINE_EVENTS.ERROR: | ||
| case LINE_EVENTS.SESSION_SUPERSEDED: |
There was a problem hiding this comment.
Why do we need to define new event for just handling this error ? If 409 is received for keepalive, we are to assume that in this tab registration doesn't exist so sending Unregistered event back to the application should be enough. We can add reason for deregistration in the payload for Unregistered event but a new event just for keepalive failure is not required
| RECONNECTING = 'reconnecting', | ||
| REGISTERED = 'registered', | ||
| UNREGISTERED = 'unregistered', | ||
| SESSION_SUPERSEDED = 'session_superseded', |
| * @param hardStop - Why the registration is being torn down as {@link HardStop}. | ||
| */ | ||
| private async performRegistrationDownCleanup(caller: string): Promise<void> { | ||
| private async performHardStopCleanup(caller: string, hardStop: HardStop): Promise<void> { |
There was a problem hiding this comment.
If we really wanna rename this method, registrationCleanup is enough. Also the changes with respect to new event are not needed. Noticed that we are already emitting Unregistered event here after cleaning everything up and after sedning unregistered, we send Session_Superseded again which is not ok by design. After receiving Unregistered event, no events should be emitted and application wont be handling it coz ideally they are supposed to destroy the line object post receiving Unregistered event
There was a problem hiding this comment.
Renamed to registrationCleanup
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6423b2432b
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| } | ||
|
|
||
| this.lineEmitter(LINE_EVENTS.UNREGISTERED); | ||
| this.lineEmitter(LINE_EVENTS.UNREGISTERED, undefined, hardStop.error); |
There was a problem hiding this comment.
Deduplicate concurrent supersession cleanup
When another session supersedes this one while a keepalive is in flight, Mobius can deliver both the normal registration.down event and the keepalive 409; each path enters registrationCleanup, and the mutex only serializes them, so this unconditional emission fires twice. If the registration-down path runs last, its undefined error also replaces the useful supersession reason in consumers such as the updated sample, reverting the displayed status to generic “Not Registered.” Make the hard-stop cleanup idempotent or preserve the terminal reason across the second signal.
AGENTS.md reference: packages/calling/AGENTS.md:L80-L85
Useful? React with 👍 / 👎.
COMPLETES #< SPARK-832708 >
This pull request addresses
When Mobius answers a keepalive (
device_status) with409 Conflict, the device registrationis gone but the same user still holds an active registration elsewhere — typically calling
opened in a second browser tab, which superseded this one. Previously a 409 fell through to the
defaultbranch ofhandleRegistrationErrors, so the SDK re-registered. That unregistered theother tab, which then re-registered and unregistered this one, producing a registration
ping-pong between tabs.
by making the following changes
A 409 on keepalive is now a hard stop: the keepalive worker is terminated, no re-registration is
attempted, the Mobius WebSocket is closed, and the app is notified that its session was
superseded.
register.ts— theKEEPALIVE_FAILUREbranch short-circuits to a newhandle409KeepaliveFailurebeforehandleRegistrationErrorsruns, so no retry, failover, orrestore path can execute. The worker is stopped up front (before waiting on the shared mutex)
so no further keepalive can be sent.
performRegistrationDownCleanup→performHardStopCleanup(caller, hardStop), now shared withthe 409 path. A
HardStopdiscriminated union (registration/types.ts) makes it impossible tosignal a superseded session without the
LineErrorthe consumer receives, and itsHARD_STOP_REASONvalues double as the log label. Teardown is unchanged: timers cleared,transient flags and failover cache reset, status
INACTIVE, WebSocket closed with the existingpermanent code
{code: 3050, reason: 'done (permanent)'}.KEEPALIVE_ERROR/KEEPALIVE_FAILUREregistration metricand calls
uploadLogs().Public API (additive)
LINE_EVENTS.SESSION_SUPERSEDED = 'session_superseded', typed inLineEventTypesas(error: LineError) => void.ERROR_TYPE.SESSION_SUPERSEDEDandERROR_CODE.CONFLICT = 409.unregisteredstill fires first, followed bysession_supersededwith the error.
Line.lineEmitterre-emits it through the same guarded branch aserror.No existing event, payload, or status transition changed. Keepalive
404/429/5xxand theregistration, restoration, failover, and failback paths are untouched — the short-circuit is
scoped to the keepalive worker's failure branch only.
Notes
The
LineErrormessage comes from a newSESSION_SUPERSEDED_MESSAGEconstant rather thanMobius's
data.message: the keepalive worker's failure envelope forwards only headers/status, andadding the response body would have silently changed the 403 device-limit branch in
handleRegistrationErrors. Detection relies solely on the status code.Change Type
The following scenarios were tested
Vidcast - https://app.vidcast.io/share/d6bda2af-24a9-4cee-ab69-d9a2da283aa3
Further Testing
User registers in different session with old session having active call - with http/mercury and without 409 handling
https://app.vidcast.io/share/7fbc2f34-ec31-459c-bfc2-5a92c3a11383
User registers in different session with old session having active call - with mobius socket and 409 handling
https://app.vidcast.io/share/a8a4ea5c-792d-45f1-b343-59128d55e903
The GAI Coding Policy And Copyright Annotation Best Practices
I certified that
Make sure to have followed the contributing guidelines before submitting.