fix(storage emulator): don't report request failures as "document not found" in firestore.get()

[jon-comley]

Description

fetchFirestoreDocument() in src/emulator/storage/rules/runtime.ts the path
backing firestore.get() / firestore.exists() inside Storage rules collapses
every possible failure into "the document does not exist":

try {
  const doc = await client.get(pathname);
  const { name, fields } = doc.body as { name: string; fields: string };
  const result = { name, fields };
  return { result, status: DataLoadStatus.OK, warnings: [], errors: [] };
} catch (e) {
  // Don't care what the error is, just return not_found
  return { status: DataLoadStatus.NOT_FOUND, warnings: [], errors: [] };
}

A confirmed HTTP 404 and a request that never reached a healthy server are
reported identically, with no retry and nothing logged. So a connection refusal,
a timeout, or a malformed response all cause a rule like:

allow write: if firestore.get(/databases/(default)/documents/jobs/$(jobId)).data.ownerId == request.auth.uid;

to evaluate as though the document is absent, and deny the write indistinguishable,
from the caller's side, from a genuine permissions failure.

Because the try also wraps the response destructuring, a malformed payload
(e.g. a null body) throws a TypeError that lands in the same catch and is
likewise reported as "not found", hiding a schema or programming error.

Fix

• Only a confirmed HTTP 404 is treated as a real "not found", returned
  immediately so the common "does this exist yet" path is unchanged and stays fast.
• Any other failure is retried a small, bounded number of times before giving up,
  on the grounds that it reflects the request not completing rather than an answer
  about the document.
• Response parsing moves outside the try, so a malformed payload surfaces as
  the error it is instead of being retried as a network problem.
• Errors are narrowed with a hasHttpStatus type guard rather than any.

Scope and honesty about evidence

I hit a run of failures where Storage rules using firestore.get() behaved as
though the referenced document was missing under emulators:exec, which is what
led me here. I have not been able to reproduce that failure on demand since
I tried a dropped page cache, cold JARs, and heavy CPU contention, and the
unpatched code passed every time. I also never captured the swallowed error
itself, so I can't tell you which error it was.

I've therefore deliberately avoided asserting a specific root cause in this PR.
What I'm proposing stands on the code as written: conflating "the request failed"
with "the document doesn't exist" produces incorrect rule evaluations, and the
current code cannot distinguish them. If maintainers consider the existing
behaviour intentional, I'm happy to close this.

Testing

Unit tests added in src/emulator/storage/rules/runtime.spec.ts:

• successful first attempt (no retries)
• confirmed 404 returns immediately without retrying
• transient connection failure retried, then succeeds
• malformed response body returns NOT_FOUND after exactly one call (no retries)
• persistent non-404 failure gives up and returns NOT_FOUND

npm run test:compile and npm run lint:quiet pass; the runtime.spec.ts suite
(11 tests) passes locally. I have not run the integration suites, which need
emulators and credentials I don't have configured.

[google-cla]

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

[gemini-code-assist]

Code Review

This pull request introduces a retry mechanism for firestore.get() and firestore.exists() calls in Storage rules to resolve intermittent connection failures during Firestore emulator startup. Feedback on the changes highlights a style guide violation regarding the use of any in the catch block. Additionally, the reviewer recommended separating the network request from response parsing to prevent standard JavaScript errors (such as TypeError) from incorrectly triggering retry attempts.

[gemini-code-assist]

The current implementation uses e: any in the catch block, which violates the repository style guide rule: Never use any or unknown as an escape hatch. Define proper interfaces/types or use type guards.

Additionally, performing response destructuring inside the try block means standard JS errors (such as a TypeError if doc.body is null or undefined) will be caught and trigger retry attempts and sleeping. This is inefficient and masks programming/schema errors as transient network issues.

To address both issues, we can:

1. Type e as unknown and use a safe type assertion to check the status code.
2. Separate the network request (client.get) from the response parsing.
3. Add defensive checks to ensure doc.body and body.name are valid before using them.

  for (let attempt = 1; attempt <= FETCH_FIRESTORE_DOCUMENT_MAX_ATTEMPTS; attempt++) {
    let doc;
    try {
      doc = await client.get(pathname);
    } catch (e: unknown) {
      const isConfirmedNotFound =
        typeof e === "object" &&
        e !== null &&
        "status" in e &&
        (e as { status?: number }).status === 404;
      const isLastAttempt = attempt === FETCH_FIRESTORE_DOCUMENT_MAX_ATTEMPTS;
      if (isConfirmedNotFound || isLastAttempt) {
        return { status: DataLoadStatus.NOT_FOUND, warnings: [], errors: [] };
      }
      await utils.sleep(FETCH_FIRESTORE_DOCUMENT_RETRY_DELAY_MS);
      continue;
    }

    const body = doc.body as { name?: string; fields?: unknown } | undefined;
    if (!body || typeof body.name !== "string") {
      return { status: DataLoadStatus.NOT_FOUND, warnings: [], errors: [] };
    }
    const result = { name: body.name, fields: body.fields };
    return { result, status: DataLoadStatus.OK, warnings: [], errors: [] };
  }

References

1. Never use any or unknown as an escape hatch. Define proper interfaces/types or use type guards. ^(link)

[jon-comley]

@googlebot I signed it!

[jon-comley]

@googlebot I signed it!