firebase-admin-node icon indicating copy to clipboard operation
firebase-admin-node copied to clipboard

`DocumentReference.create()` keeps trying to create an already existing document, leading to a hang in functions

Open jellynoone opened this issue 1 year ago • 18 comments

[READ] Step 1: Are you in the right place? ✅

[REQUIRED] Step 2: Describe your environment

  • Operating System version: MacOS 14.5
  • Firebase Product: functions, firestore
  • Node.js version: v18.20.3
  • NPM version: 10.7.0

[REQUIRED] Step 3: Describe the problem

Steps to reproduce:

  1. Create a new demo project via firebase init
  2. Select functions and firestore
  3. Run emulators through: firebase emulators:start --inspect-functions --project demo-me
  4. Visit http://127.0.0.1:5001/demo-me/us-central1/runMeTwice in browser
  5. Refresh

Relevant Code:

{
  "name": "functions",
  "description": "Cloud Functions for Firebase",
  "engines": {
    "node": "18"
  },
  "main": "index.js",
  "dependencies": {
    "firebase-admin": "^12.1.0",
    "firebase-functions": "^5.0.0"
  },
  "private": true
}
//@ts-check
const { onRequest } = require("firebase-functions/v2/https");
const logger = require("firebase-functions/logger");
const { firestore, initializeApp } = require("firebase-admin");
const { initializeFirestore } = require("firebase-admin/firestore");

/** @type {firestore.Firestore} */
let _defaultFirestore;
function defaultFirestore() {
    return _defaultFirestore ??= initializeFirestore(initializeApp(), {
        preferRest: true,
    })
}

exports.runMeTwice = onRequest(async (_, res) => {
    try {
        const doc = defaultFirestore().collection("test").doc("smt");
        await doc.create({});
        logger.log("Created");
    } catch (error) {
        logger.log(error);
    } finally {
        res.end();
    }
});

Expected behavior

The second time the function is ran, it should fail.

Actual behavior

The first time the function is run the document is created. The second time, the function never completes.

Additionally, attaching to the node process, it seems the creation process keeps getting retried even though it should fail.

jellynoone avatar Jun 04 '24 17:06 jellynoone

I found a few problems with this issue:

  • I couldn't figure out how to label this issue, so I've labeled it for a human to triage. Hang tight.
  • This issue does not seem to follow the issue template. Make sure you provide all the required information.

google-oss-bot avatar Jun 04 '24 17:06 google-oss-bot

In addition, performing this in a transaction will eventually fail and terminate the function. However, attaching the debugger it shows, the transaction is still attempted multiple times. This seems incorrect.

exports.runMeTwice = onRequest(async (_, res) => {
    try {
        const firestore = defaultFirestore();
        await firestore.runTransaction(async (txn) => {
            txn.create(firestore.collection("test").doc("smt"), {});
        })
        logger.log("Created");
    } catch (error) {
        logger.log(error);
    } finally {
        res.end();
    }
});

jellynoone avatar Jun 04 '24 17:06 jellynoone

Hi @jellynoone ,

I will take a look at this and get back to you.

cherylEnkidu avatar Jun 04 '24 20:06 cherylEnkidu

@jellynoone

While investigating, there are some things can be tried as a workaround:

cherylEnkidu avatar Jun 04 '24 22:06 cherylEnkidu

This is the code I used to reproduce the issue against production, it throws Document already exists successfully.

const ref = randomCol.doc();
await ref.create({});
const snap1 = await ref.get();

await ref.create({});

Are you testing against production or emulator?

cherylEnkidu avatar Jun 05 '24 16:06 cherylEnkidu

I'm testing this against the emulator. Did you try with the provided example? I think the preferRest: true, configuration option might be significant.

Tested this again against the emulator, without preferRest the function completes with an error. With preferRest the function hangs.

jellynoone avatar Jun 06 '24 05:06 jellynoone

@cherylEnkidu Did you perhaps have a chance to reevaluate based on the additional information provided?

jellynoone avatar Jun 07 '24 13:06 jellynoone

Hi @jellynoone ,

I will continue the investigation this week, and I will reply the ticket as soon as I get any updates :)

cherylEnkidu avatar Jun 10 '24 17:06 cherylEnkidu

Hi @cherylEnkidu were you able to take a look?

jellynoone avatar Jun 25 '24 07:06 jellynoone

Hi @jellynoone ,

Thanks for the waiting. I tried the same code you provided, but the process never hangs. Could you please provided a small repo app?

cherylEnkidu avatar Jun 26 '24 18:06 cherylEnkidu

Attaching a small repro app.

export.zip

Additional notes:

  • The problem reproduces in:
    • emulated environments
    • against the live Firestore instance being called locally
  • The function has to be called two times
    • The first call will complete normally
    • The second call will hang

jellynoone avatar Jun 27 '24 07:06 jellynoone

@cherylEnkidu Attaching a video of the bug being reproduced.

https://github.com/firebase/firebase-admin-node/assets/77585130/d4bba6c7-4385-4165-bf79-3c677dce04ed

In the video I'm using firebase emulators:start --project demo-me to start the emulators without needing a real project. However, I tested this with a live Firestore instance and emulated functions and observed the same bug.

In addition, in the video I'm calling curl instead of the browser to make the process more streamlined.

jellynoone avatar Jun 27 '24 07:06 jellynoone

@cherylEnkidu Do you need more information or anything else? Were you able to reproduce the issue?

jellynoone avatar Jul 02 '24 21:07 jellynoone

Hi @jellynoone ,

Thank you for your inputs! I will take a look at the code provided and get back to you.

cherylEnkidu avatar Jul 03 '24 19:07 cherylEnkidu

Hi @jellynoone ,

I have successfully reproduce the issue, while investigating, you may consider removing preferRest: true to always use grpc. I will get back to you once I have more info.

cherylEnkidu avatar Jul 10 '24 03:07 cherylEnkidu

@cherylEnkidu Thank you for getting back to me. Unfortunately disabling preferRest: true is not an option as that would increase the function's cold start. The other alternative I found is to always manually check for the existence of the document before calling create.

While the issue can be circumvented, I can imagine that people who don't do this manually and rely on the firebase-admin package to fail when an existing document is attempted to be created again, and allow user supplied ids, might be exposing themselves to a resources exhaustion attack. So to me this seems like a high priority issue.

jellynoone avatar Jul 10 '24 08:07 jellynoone

When using preferRest, the Firestore endpoint is returning status code 409 when the document already exists. It's also returning 409 when the server aborted the request. The SDK cannot distinguish these two conditions and it is retrying the request, which is appropriate for aborted requests. Unfortunately, I don't have a workaround for this at the moment, other than what you've suggested. We are looking into it.

Googlers see b/352387470

MarkDuckworth avatar Jul 10 '24 21:07 MarkDuckworth

@MarkDuckworth Out of curiosity, couldn't the firebase_admin package use the status code returned in the response to further deduce if the request is retriable?

Attaching the debugger, the raw response received looks something like this: '{"error":{"code":409,"message":"entity already exists: EntityRef{partitionRef=dev~demo-me, path=test\\/smt}","status":"ALREADY_EXISTS"}}'

This is where the exception is first thrown by the google-gax package at: build/src/fallbackRest.json#decodeResponse.

When this raw response is processed however, the status is dropped from the error object. However, all the information seems to be sent already it is just that the GoogleError.parseHttpError doesn't expose it.

Additionally, it seems the GoogleError.parseHttpError exclusively looks at the http code could it be made to also take into account the status?

jellynoone avatar Jul 14 '24 16:07 jellynoone