fix(remote-config): prevent stream error propagation when app moves to background - #10384
ritiktaneja wants to merge 1 commit into
Conversation
🦋 Changeset detectedLatest commit: 15e1910 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
There was a problem hiding this comment.
Code Review
This pull request suppresses false-positive CONFIG_UPDATE_STREAM_ERROR events by checking if the application is in the background before handling retries or propagating errors, and resets the retry count when returning to the foreground. The review feedback highlights a critical race condition during rapid background/foreground transitions, where a false-positive backoff could still be triggered or the connection could remain dead. To resolve this, the reviewer suggests tracking whether the connection was closed due to backgrounding using a closedDueToBackground flag to safely manage transitions and immediately attempt reconnection when returning to the foreground.
| // Only handle retries and errors if the failure occurred in the foreground. | ||
| // Closing the connection when moving to the background is expected lifecycle behavior. | ||
| if (!this.isInBackground) { | ||
| // Update backoff metadata if the connection failed in the foreground. | ||
| const connectionFailed = | ||
| responseCode === undefined || | ||
| this.isStatusCodeRetryable(responseCode); | ||
|
|
||
| if (connectionFailed) { | ||
| await this.updateBackoffMetadataWithLastFailedStreamConnectionTime( | ||
| new Date() | ||
| ); | ||
| } | ||
| // If responseCode is null then no connection was made to server and the SDK should still retry. | ||
| if (connectionFailed || response?.ok) { | ||
| await this.retryHttpConnectionWhenBackoffEnds(); | ||
| } else { | ||
| const errorMessage = `Unable to connect to the server. HTTP status code: ${responseCode}`; | ||
| const firebaseError = ERROR_FACTORY.create( | ||
| ErrorCode.CONFIG_UPDATE_STREAM_ERROR, | ||
| { | ||
| originalErrorMessage: errorMessage | ||
| } | ||
| ); | ||
| this.propagateError(firebaseError); | ||
| } | ||
| } |
There was a problem hiding this comment.
Race Condition & False-Positive Backoff on Rapid Background/Foreground Transitions
There is a race condition when the application is quickly backgrounded and then immediately foregrounded:
- False-Positive Backoff/Error: When backgrounded,
closeRealtimeHttpConnection()is called, aborting the active stream. If the app returns to the foreground before thefinallyblock finishes executing,this.isInBackgroundwill befalse. Thefinallyblock will then treat the abort as a foreground network failure, updating the backoff metadata with a penalty and potentially propagating a false-positive stream error. - Dead Connection: When returning to the foreground,
onVisibilityChange(true)callsbeginRealtime(), which checkscanEstablishStreamConnection(). Since the previous connection is still in the process of closing/cleaning up in itsfinallyblock,this.isConnectionActiveis stilltrue. Thus,canEstablishStreamConnection()returnsfalse, and the reconnection attempt is silently skipped. Once the cleanup finally completes and setsthis.isConnectionActive = false, no new connection is ever attempted, leaving the real-time stream dead.
To resolve this, we can track whether the connection was explicitly closed due to backgrounding using a temporary flag closedDueToBackground. If the app returns to the foreground while or after closing, we can trigger a fresh connection attempt immediately once the cleanup is complete.
const wasClosedDueToBackground = (this as any).closedDueToBackground;
(this as any).closedDueToBackground = false;
// Only handle retries and errors if the failure occurred in the foreground.
// Closing the connection when moving to the background is expected lifecycle behavior.
if (!wasClosedDueToBackground && !this.isInBackground) {
// Update backoff metadata if the connection failed in the foreground.
const connectionFailed =
responseCode === undefined ||
this.isStatusCodeRetryable(responseCode);
if (connectionFailed) {
await this.updateBackoffMetadataWithLastFailedStreamConnectionTime(
new Date()
);
}
// If responseCode is null then no connection was made to server and the SDK should still retry.
if (connectionFailed || response?.ok) {
await this.retryHttpConnectionWhenBackoffEnds();
} else {
const errorMessage = `Unable to connect to the server. HTTP status code: ${responseCode}`;
const firebaseError = ERROR_FACTORY.create(
ErrorCode.CONFIG_UPDATE_STREAM_ERROR,
{
originalErrorMessage: errorMessage
}
);
this.propagateError(firebaseError);
}
} else if (wasClosedDueToBackground && !this.isInBackground) {
// The connection was closed due to backgrounding, but we are already back in the foreground.
// Trigger a reconnection attempt immediately.
void this.beginRealtime();
}| private async onVisibilityChange(visible: unknown): Promise<void> { | ||
| this.isInBackground = !visible; | ||
| if (!visible) { | ||
| await this.closeRealtimeHttpConnection(); | ||
| } else if (visible) { | ||
| // Reset retry count so returning to foreground grants a fresh connection attempt budget. | ||
| this.resetRetryCount(); | ||
| await this.beginRealtime(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Set the closedDueToBackground flag when the active connection is closed due to the application moving to the background. This ensures we can safely distinguish between normal backgrounding and actual network failures, and trigger an immediate reconnection if the app returns to the foreground before the cleanup completes.
private async onVisibilityChange(visible: unknown): Promise<void> {
this.isInBackground = !visible;
if (!visible) {
if (this.isConnectionActive) {
(this as any).closedDueToBackground = true;
}
await this.closeRealtimeHttpConnection();
} else if (visible) {
// Reset retry count so returning to foreground grants a fresh connection attempt budget.
this.resetRetryCount();
await this.beginRealtime();
}
}03f23eb to
ab68aab
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request suppresses false-positive CONFIG_UPDATE_STREAM_ERROR events in @firebase/remote-config by ensuring that retries and stream errors are only handled when the application is in the foreground. However, the implementation is missing the logic to reset the retry counter when the app returns to the foreground, which was mentioned in the PR description and is necessary to prevent reconnection attempts from being skipped if the retry budget was previously exhausted.
| this.propagateError(firebaseError); | ||
| // Only handle retries and errors if the failure occurred in the foreground. | ||
| // Closing the connection when moving to the background is expected lifecycle behavior. | ||
| if (!this.isInBackground) { |
There was a problem hiding this comment.
The PR description states:
In addition, reset the retry counter when the app returns to the foreground so the reconnection attempt receives a fresh retry budget.
However, the implementation to reset the retry counter (this.resetRetryCount()) when the app returns to the foreground is missing from this PR.
If the retry budget (this.httpRetriesRemaining) was previously exhausted (reduced to 0) while in the foreground, and the app is then backgrounded and foregrounded again:
onVisibilityChange(true)is triggered.- It calls
beginRealtime(), which callsmakeRealtimeHttpConnection(0). - Since
this.httpRetriesRemainingis0, the connection attempt is skipped, and it immediately propagates aCONFIG_UPDATE_STREAM_ERRORin the foreground.
To fix this, please reset the retry count when the app returns to the foreground. This can be done in onVisibilityChange or beginRealtime. For example, in onVisibilityChange:
private async onVisibilityChange(visible: unknown): Promise<void> {
this.isInBackground = !visible;
if (!visible) {
await this.closeRealtimeHttpConnection();
} else if (visible) {
this.resetRetryCount(); // Reset retry budget on foregrounding
await this.beginRealtime();
}
}ab68aab to
ccc9d1c
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request suppresses false-positive CONFIG_UPDATE_STREAM_ERROR events and avoids recording connection backoff penalties when the application enters the background. It achieves this by capturing the background state before closing the connection and skipping retry/error handling if the app was or is in the background. However, a critical race condition was identified where the connection could be left dead if the application returns to the foreground during the asynchronous teardown of a background close. A code suggestion has been provided to resolve this by reconnecting immediately without a penalty if the app returns to the foreground during teardown.
| if (!wasInBackground && !this.isInBackground) { | ||
| // Update backoff metadata if the connection failed in the foreground. | ||
| const connectionFailed = | ||
| responseCode === undefined || | ||
| this.isStatusCodeRetryable(responseCode); | ||
|
|
||
| if (connectionFailed) { | ||
| await this.updateBackoffMetadataWithLastFailedStreamConnectionTime( | ||
| new Date() | ||
| ); | ||
| } | ||
| // If responseCode is null then no connection was made to server and the SDK should still retry. | ||
| if (connectionFailed || response?.ok) { | ||
| await this.retryHttpConnectionWhenBackoffEnds(); | ||
| } else { | ||
| const errorMessage = `Unable to connect to the server. HTTP status code: ${responseCode}`; | ||
| const firebaseError = ERROR_FACTORY.create( | ||
| ErrorCode.CONFIG_UPDATE_STREAM_ERROR, | ||
| { | ||
| originalErrorMessage: errorMessage | ||
| } | ||
| ); | ||
| this.propagateError(firebaseError); | ||
| } | ||
| } |
There was a problem hiding this comment.
There is a race condition when the application returns to the foreground during the asynchronous teardown of a background close.
- When the app goes to the background,
onVisibilityChange(false)setsthis.isInBackground = trueand callscloseRealtimeHttpConnection(), which asynchronously awaitsreader.cancel(). - While
reader.cancel()is pending, the app returns to the foreground, triggeringonVisibilityChange(true). This setsthis.isInBackground = falseand callsbeginRealtime(). - However, because
this.isConnectionActiveis stilltrue(it is only set tofalseat the end of thefinallyblock),canEstablishStreamConnection()returnsfalse, andbeginRealtime()exits early without establishing a connection. - Once
reader.cancel()completes, thefinallyblock resumes. SincewasInBackgroundwas captured astrue(before the teardown), the condition!wasInBackground && !this.isInBackgroundevaluates tofalse. - As a result, no retry/reconnect is scheduled, and the connection is left dead even though the app is in the foreground with active observers.
To fix this, if the app is in the foreground when the teardown completes (!this.isInBackground), we should always ensure a connection is established. If wasInBackground was true, we should reconnect immediately without recording a backoff penalty or propagating an error.
if (!this.isInBackground) {
if (wasInBackground) {
// The app returned to the foreground during teardown. We must reconnect,
// but since the close was expected due to backgrounding, we do not record
// a backoff penalty or propagate an error.
await this.retryHttpConnectionWhenBackoffEnds();
} else {
// Update backoff metadata if the connection failed in the foreground.
const connectionFailed =
responseCode === undefined ||
this.isStatusCodeRetryable(responseCode);
if (connectionFailed) {
await this.updateBackoffMetadataWithLastFailedStreamConnectionTime(
new Date()
);
}
// If responseCode is null then no connection was made to server and the SDK should still retry.
if (connectionFailed || response?.ok) {
await this.retryHttpConnectionWhenBackoffEnds();
} else {
const errorMessage = `Unable to connect to the server. HTTP status code: ${responseCode}`;
const firebaseError = ERROR_FACTORY.create(
ErrorCode.CONFIG_UPDATE_STREAM_ERROR,
{
originalErrorMessage: errorMessage
}
);
this.propagateError(firebaseError);
}
}
}ccc9d1c to
38fab85
Compare
Hiding the tab aborts a pending realtime fetch, leaving responseCode undefined. The background check was folded into connectionFailed, which short-circuited it to false and fell through to the error branch, surfacing 'HTTP status code: undefined' to onConfigUpdated listeners. Guard the post-mortem block instead, so connectionFailed depends only on the response. Reading isInBackground in that block also races with the visibility change, because the teardown before it awaits. Capture the state before the teardown so a late foreground return cannot turn a background close into a reported failure, and skip the block while the app is hidden so a foreground failure that overlaps with backgrounding does not persist a backoff penalty for a retry that cannot run. When a teardown begins in the background but ends in the foreground, the reconnect from onVisibilityChange was dropped because this connection was still marked active, so it is made here instead. The retry budget is restored first: backgrounding is not a failure, and reconnecting on an exhausted budget would itself surface a stream error. Fixes firebase#9426
38fab85 to
15e1910
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request modifies the RealtimeHandler in @firebase/remote-config to suppress false-positive CONFIG_UPDATE_STREAM_ERROR events when the application transitions to the background. It captures the background state before asynchronously closing the connection, avoids recording backoff penalties for expected background closes, and handles reconnection if the app returns to the foreground during teardown. Additionally, comprehensive unit tests have been added to verify these scenarios. There are no review comments, and I have no feedback to provide.
fix(remote-config): don't emit stream error when app moves to background
When the app moves to the background, the in-flight realtime fetch is aborted, leaving responseCode undefined, and the teardown is reported to listeners as a failed connection.
Reading this.isInBackground in the finally block also races with the visibility
change, because the teardown awaits: the app can return to the foreground
before the check runs, and the background close is then misread as a failure.
Capture the visibility state before the teardown, and require the state after
it to still be foreground as well, so a late foreground return does not turn a
background close into an error, and a genuine foreground failure that overlaps with backgrounding does not persist a backoff penalty for a retry that cannot run while hidden.
This prevents CONFIG_UPDATE_STREAM_ERROR ('Unable to connect to the server. HTTP status code: undefined') from reaching onConfigUpdated listeners on an ordinary tab switch.
Fixes #9426