Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[🐛] "Other" platform firestore transaction failure rollback not working correctly #8267

Open
mikehardy opened this issue Feb 5, 2025 · 2 comments

Comments

@mikehardy
Copy link
Collaborator

Issue

The majority of the e2e failures on the "Other" platform are related to this one test (both v8 and modular flavor) failing in CI

It is frequent enough (maybe ... 10% of runs? maybe 75% of failures?) that it is worth investigating why it's failing

  1) React Native Firebase
       firestore.Transaction
         v8 compatibility
           transaction.set()
             should roll back any updates that failed:
     AssertionError: expected Object { prop2: 1, turn: 1, prop1: 1 } not to have properties prop1, prop2 (false negative fail)
      at Object.defineProperty$argument_2.value (/Users/runner/work/react-native-firebase/react-native-firebase/tests/node_modules/should/cjs/should.js:368:20)
      at it$argument_1 (/Users/runner/work/react-native-firebase/react-native-firebase/packages/firestore/e2e/Transaction.e2e.js:379:49)

it('should roll back any updates that failed', async function () {
const docRef = firebase.firestore().doc(`${COLLECTION}/transactions/transaction/rollback`);
await docRef.set({
turn: 0,
});
const prop1 = 'prop1';
const prop2 = 'prop2';
const turn = 0;
const errorMessage = 'turn cannot exceed 1';
const createTransaction = prop => {
return firebase.firestore().runTransaction(async transaction => {
const doc = await transaction.get(docRef);
const data = doc.data();
if (data.turn !== turn) {
throw new Error(errorMessage);
}
const update = {
turn: turn + 1,
[prop]: 1,
};
transaction.update(docRef, update);
});
};
const promises = [createTransaction(prop1), createTransaction(prop2)];
try {
await Promise.all(promises);
return Promise.reject(new Error('Did not throw an Error.'));
} catch (error) {
error.message.should.containEql(errorMessage);
}
const result = await docRef.get();
should(result.data()).not.have.properties([prop1, prop2]);
});
});
});

I suspect the handling of the turn variable is racy, or firebase-js-sdk may have a subtle issue ?
The answer wasn't immediately apparent when I looked at it, but it honestly looks like firebase-js-sdk may have a subtle issue.

This has been present since the beginning of the "other" platform implementation so it doesn't seem affected by versions of firebase-js-sdk, at least as long as we've been using it.


@mikehardy
Copy link
Collaborator Author

mikehardy commented Feb 10, 2025

This one live just now, and there was some information in console about it:

(NOBRIDGE) WARN [2025-02-10T21:21:46.734Z] @firebase/firestore: Firestore (11.3.0_lite): RestConnection RPC 'Commit' 0x4932b621 failed with error:

{"code":"failed-precondition","name":"FirebaseError"}

url: http://localhost:8080/v1/projects/react-native-firebase-testing/databases/(default)/documents:commit

request: {"writes":[{"update":{"name":"projects/react-native-firebase-testing/databases/(default)/documents/firestore/transactions/transaction/rollback","fields":{"turn":{"integerValue":"1"},"prop2":{"integerValue":"1"}}},"updateMask":{"fieldPaths":["prop2","turn"]},"currentDocument":{"updateTime":"2025-02-10T21:21:46.207067000Z"}}]}

@mikehardy
Copy link
Collaborator Author

new working hypothesis:

  • one of the updates fails sometimes because of failed-precondition instead of the error message we expect

  • the method is retried without data cleaning (beforeEach/afterEach or similar) and one of the updates has already gone through somehow so there is dirty state on the firestore side

  • one future update goes through somehow but all future updates will fail?

  • Would be interesting to see if the firestore cloud behaved this way or if it was just emulator specific

  • would be interesting to see what would happen if we did data cleaning between passes and caught failed-precondition errors

mikehardy added a commit that referenced this issue Feb 20, 2025
this was flaky in the main suite and the second transaction suite
is also flaky

See issue #8267
mikehardy added a commit that referenced this issue Feb 20, 2025
this was flaky in the main suite and the second transaction suite
is also flaky

See issue #8267
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant