Skip to content

fix: prevent analytics timeouts from masking command exit codes - #11258

Merged
ajperel merged 1 commit into
ajp/functions-deploy-errorsfrom
ajp/command-exit-code
Oct 12, 2026
Merged

ajperel merged 1 commit into
ajp/functions-deploy-errorsfrom
ajp/command-exit-code

Conversation

@ajperel

@ajperel ajperel commented Oct 10, 2026 •

Copy link
Copy Markdown
Contributor

Description

Wraps post-command analytics tracking (withTimeout(5000, Promise.all(tracks))) in try / catch blocks (restoring the guard from #10253 that was inadvertently reverted in #10270) so telemetry timeouts cannot skip client.errorOut(err) on command failure or cause successful commands to fail.

Partially addresses #6989.

Scenarios Tested

  • Manually tested with firebase deploy --only functions.
  • Unit tests in src/command.spec.ts verifying client.errorOut is called when analytics tracking times out on command failure and is not called when analytics tracking times out on command success.

Sample Commands

firebase deploy --only functions

@ajperel
ajperel added this pull request to stack #11259 October 10, 2026 00:21
Comment thread src/command.ts

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request ensures that analytics tracking timeouts or failures do not mask command errors or cause successful commands to exit with an error. It wraps the tracking logic in try-catch blocks, logs tracking failures as debug messages, sets the process exit code appropriately on command failures, and adds comprehensive unit tests. The review feedback suggests standardizing on undefined over null for the exit code types in the new tests to align with the repository's style guide.

Comment thread src/command.spec.ts Outdated
Comment thread src/command.spec.ts Outdated
@ajperel
ajperel force-pushed the ajp/command-exit-code branch from a484f43 to 227468b Compare October 10, 2026 00:36
@ajperel
ajperel force-pushed the ajp/command-exit-code branch from 227468b to 792cc32 Compare October 10, 2026 00:38
@ajperel
ajperel requested a review from joehan October 10, 2026 00:43
@ajperel
ajperel force-pushed the ajp/command-exit-code branch from 792cc32 to c58c7a7 Compare October 12, 2026 16:10
@ajperel
ajperel force-pushed the ajp/command-exit-code branch from c58c7a7 to 463a272 Compare October 12, 2026 17:29
@ajperel
ajperel force-pushed the ajp/command-exit-code branch from 463a272 to 8a44d08 Compare October 12, 2026 17:33

@joehan joehan left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Good fix

### Description
Wraps post-command analytics tracking (`withTimeout(5000, Promise.all(tracks))`) in `try / catch` blocks (restoring the guard from #10253 that was inadvertently reverted in #10270) so telemetry timeouts cannot skip `client.errorOut(err)` on command failure or cause successful commands to fail.

Partially addresses #6989.

### Scenarios Tested
- Manually tested with `firebase deploy --only functions`.
- Unit tests in `src/command.spec.ts` verifying `client.errorOut` is called when analytics tracking times out on command failure and is not called when analytics tracking times out on command success.

### Sample Commands
```bash
firebase deploy --only functions
```
@ajperel
ajperel force-pushed the ajp/command-exit-code branch from 8a44d08 to b5c4bc7 Compare October 12, 2026 21:41
@ajperel
ajperel merged commit 4212532 into main Oct 12, 2026
56 of 66 checks passed
@ajperel
ajperel deleted the ajp/command-exit-code branch October 12, 2026 22:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants