Skip to content

ci: add a manual firestore flake probe for #776 - #780

Open
tyler-reitz wants to merge 1 commit into
FirebaseExtended:mainfrom
tyler-reitz:ci/flake-probe
Open

ci: add a manual firestore flake probe for #776#780
tyler-reitz wants to merge 1 commit into
FirebaseExtended:mainfrom
tyler-reitz:ci/flake-probe

Conversation

@tyler-reitz

Copy link
Copy Markdown
Contributor

Refs #776. This measures the flake; it does not fix it.

Why

Every measurement of the #776 flake so far has been on a laptop. Locally the failure is a plain waitFor timeout with no gRPC error at all. In CI it arrives alongside a gRPC framing desync:

RESOURCE_EXHAUSTED: Received message larger than max (795107700 vs 4194304)

A ~795 MB claimed frame against a 4 MB cap means the reader is interpreting payload bytes as a length header. That is a @grpc/grpc-js-layer failure, and it does not appear in any local run.

So the local repro and the CI failure may not be the same bug. That matters, because the @grpc/grpc-js override proposed as the fix was measured only against the local one, at p = 0.12. Merging that override would also mean CI never runs 1.9.16 again, so it is a before-and-after with no control.

This workflow makes the comparison in CI, where the failure actually happens, before anything is changed.

What it does

Matrixes Node 22 and 24 against @grpc/grpc-js baseline and override, runs the firestore suite N times per arm (default 20), and writes a table per arm to the job summary: resolved grpc-js version, pass / flake / infra counts, how many runs showed RESOURCE_EXHAUSTED, and the resulting rate.

Design decisions worth reviewing:

  • workflow_dispatch only. It never runs on a push, a PR, a release or a schedule, so it adds nothing to normal CI time.
  • A fresh emulator per iteration, matching how npm test runs in CI. Reusing one emulator across iterations would measure something else.
  • Failures are classified, not counted. Only the CI: test/firestore.test.tsx flakes intermittently on both Node 22 and Node 24 #776 assertion signature counts toward the rate. Emulator start failures are reported separately, because folding them in is exactly what inflated the earlier local estimate by roughly 50% and made 30 runs look adequately powered when they were not.
  • It reports rather than fails. A red job here means the probe broke, not that the flake reproduced. It exits non-zero only if every run failed for infrastructure reasons, meaning it measured nothing.
  • RESOURCE_EXHAUSTED is counted per arm. This is the one signal no existing measurement has, and it is what would show that CI is hitting a different bug.
  • Inputs reach the script through env rather than being interpolated into the shell, and iterations is validated before it reaches the loop.

Verification

⚠️ This workflow cannot be exercised from this PR. GitHub only offers workflow_dispatch for workflows present on the default branch, so it is unrunnable until this merges. Per the repo's own lesson about treating jobs authored on a branch as unrun, it was verified out of band instead:

  • The classifier was dry-run against synthetic logs covering pass, flake, flake-with-gRPC-error and infra-failure. It returned the expected counts and correctly excluded the infra failure from the rate. This is the part most worth getting right, since a probe that miscounts is worse than no probe.
  • zizmor 1.25.2 reports no findings beyond the cache-poisoning rule CI suppresses, which is the severity set the gate reads.
  • YAML parses; the package.json override edit produces the right key (npm pkg set mangles keys containing a slash, hence the node -e); npx firebase resolves through firebase-tools.

The first real run is therefore the first proof it works. If it misbehaves, the fix is another PR, not a rollback of anything consumers see.

Known confound

The override is repo-wide, so the override arm also moves @grpc/grpc-js for firebase-tools and google-gax, not only the Firestore client. The emulator itself is a Java process and so should not be in the data path, but the two arms differ by slightly more than the client library. Worth remembering when reading the numbers.

What this changes for anyone else

Nothing. One new file, no existing workflow touched, no runtime or packaging change, and no effect on normal CI.

Every measurement of the FirebaseExtended#776 flake so far has been local, where the
failure is a plain waitFor timeout with no gRPC error. In CI it arrives
alongside a gRPC framing desync (RESOURCE_EXHAUSTED: Received message
larger than max), which raises the possibility that the local repro and
the CI failure are not the same bug. That matters, because the
@grpc/grpc-js override proposed as the fix was measured only against
the local one.

This runs the firestore suite N times per arm, across both Node
versions and both grpc-js versions, under CI conditions, so the
comparison happens where the failure actually occurs.

Notes on the design:

- workflow_dispatch only. It never runs on a push, a PR or a schedule,
  so it costs nothing until someone asks for it.
- A fresh emulator per iteration, matching how npm test runs in CI.
  Reusing one emulator across iterations would measure something else.
- Failures are classified, not counted. Only the FirebaseExtended#776 assertion
  signature counts toward the rate; emulator start failures are
  reported separately, because folding them in previously inflated a
  local rate estimate by roughly 50%.
- The job reports rather than fails. A red run here means the probe
  broke, not that the flake reproduced.
- Inputs reach the script through env rather than interpolation, and
  iterations is validated before it reaches the loop.

Classifier dry-run against synthetic logs covering pass, flake,
flake-with-gRPC-error and infra-failure returns the expected counts and
excludes infra failures from the rate. zizmor 1.25.2 reports no
findings beyond the cache-poisoning rule CI suppresses.

Refs FirebaseExtended#776

@armando-navarro armando-navarro left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Nice, this is worth having. Approving it. A few things I would think about before anyone runs it and reads numbers off the table.

The two arms land on different runners

arm is a matrix dimension, so baseline and override come out as separate jobs on separate machines. You were careful about this on your laptop, same machine and back to back, and that is the one control that did not make the trip. Given the whole premise here is that the machine matters, I would pull arm out of the matrix and loop both arms inside one job instead. Costs you a longer job, but then the comparison means something.

It is not running the workload the failures came from

CI does npm run test, so five emulators, the whole suite, parallel workers. This does one emulator and one file.

Your local runs were firestore-only too and still hit 6/44, so I am not saying it will fail to reproduce anything. But RESOURCE_EXHAUSTED has never turned up in a firestore-only run, and that is the signal you actually care about. What worries me is that you reproduce the laptop bug on CI hardware and the CI bug never shows up. Might be worth a line in the job summary so nobody reads a clean table as a verdict.

The RESOURCE_EXHAUSTED number is a total, not an overlap

grpc_err goes up before the run gets classified, so Flake: 2 sitting next to RESOURCE_EXHAUSTED: 11 does not tell you whether those 2 are inside the 11. That overlap is the whole question. One more counter in the flake branch and it is in the table.

Hangs fall out of the rate entirely

If Firestore calls never finish you get Error: Test timed out in 5000ms and no assertion line, so the run goes to infra and drops out of both halves of the rate. #776 mentions a 120s hang. Should those get their own bucket?

Related: the flake match is a literal assertion string, so if vitest ever rewords it, every real flake quietly becomes an infra failure.

On run counts

#776 calls 30 per arm underpowered at p = 0.12, and the default here is 20.

Separately, the guard lets you pass 200. At your 25s estimate that is 83 minutes against a 60 minute timeout. I clocked 7s locally so your estimate is probably pessimistic, but if it ever does hit the wall the summary block never runs and you lose every count.

Your confound is not actually there

I diffed the two lockfiles and exactly one version moves: @grpc/grpc-js, 1.9.16 to 1.14.4. google-gax was already on 1.14.4, so its nested copy just dedupes into the hoisted one. Nothing else budges. The arms differ by the Firestore client's grpc-js and nothing else, which is cleaner than your caveat makes it sound.

One last thing. I checked the classifier by actually breaking the test rather than trusting the grep on sight. Shortened the waitFor on returns undefined if document does not exist and ran your command against it. Your pattern catches it.

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