[cloud functions] Fix race condition when counting received chunks
Change-Id: I7784792801d71e2806303958d7b54a388c4db73c
Reviewed-on: https://dart-review.googlesource.com/c/dart_ci/+/130801
Reviewed-by: Alexander Thomas <athom@google.com>
diff --git a/functions/node/firestore_impl.dart b/functions/node/firestore_impl.dart
index 4ce1032..8e9bf75 100644
--- a/functions/node/firestore_impl.dart
+++ b/functions/node/firestore_impl.dart
@@ -88,8 +88,13 @@
final documentRef = firestore.document('builds/$builder:$index');
final record = await documentRef.get();
if (!record.exists) {
- await documentRef.setData(DocumentData.fromMap(
- {'builder': builder, 'build_number': buildNumber, 'index': index}));
+ await documentRef.setData(
+ DocumentData.fromMap({
+ 'builder': builder,
+ 'build_number': buildNumber,
+ 'index': index
+ }),
+ SetOptions(merge: true));
info('Created build record: '
'builder: $builder, build_number: $buildNumber, index: $index');
} else if (record.data.getInt('index') != index) {
@@ -331,7 +336,6 @@
break;
}
}
-
Future<void> updateStatus(Transaction transaction) async {
final snapshot = await transaction.get(document);
final data = snapshot.data.toMap();
@@ -359,13 +363,10 @@
Future<void> storeTryChunkStatus(String builder, int buildNumber, int review,
int patchset, bool success) async {
+ await _ensureTryBuildRecord(builder, buildNumber, review, patchset);
final reference =
firestore.document('try_builds/$builder:$review:$patchset');
- final snapshot = await reference.get();
- if (!snapshot.exists || snapshot.data.getBool('completed') != null) {
- await _createTryBuildRecord(builder, buildNumber, review, patchset);
- }
Future<void> updateStatus(Transaction transaction) async {
final snapshot = await transaction.get(reference);
final data = snapshot.data.toMap();
@@ -386,23 +387,46 @@
Future<void> storeTryBuildChunkCount(String builder, int buildNumber,
int review, int patchset, int numChunks) async {
+ await _ensureTryBuildRecord(builder, buildNumber, review, patchset);
final reference =
firestore.document('try_builds/$builder:$review:$patchset');
- final snapshot = await reference.get();
- if (!snapshot.exists || snapshot.data.getInt('num_chunks') != null) {
- await _createTryBuildRecord(builder, buildNumber, review, patchset);
- }
await reference.updateData(UpdateData.fromMap({'num_chunks': numChunks}));
}
- Future<void> _createTryBuildRecord(
- String builder, int buildNumber, int review, int patchset) =>
- firestore
- .document('try_builds/$builder:$review:$patchset')
- .setData(DocumentData.fromMap({
+ Future<void> _ensureTryBuildRecord(
+ String builder, int buildNumber, int review, int patchset) async {
+ final reference =
+ firestore.document('try_builds/$builder:$review:$patchset');
+ var snapshot = await reference.get();
+ if (snapshot.exists && snapshot.data.getInt('build_number') > buildNumber) {
+ throw ArgumentError("Received chunk from previous build $buildNumber"
+ " after chunk from a later build");
+ }
+ if (snapshot.exists && snapshot.data.getInt('build_number') < buildNumber) {
+ Future<void> deleteEarlierBuild(Transaction transaction) async {
+ final snapshot = await transaction.get(reference);
+ if (snapshot.exists &&
+ snapshot.data.getInt('build_number') < buildNumber) {
+ transaction.delete(reference);
+ }
+ }
+
+ try {
+ await firestore.runTransaction(deleteEarlierBuild);
+ } finally {
+ snapshot = await reference.get();
+ }
+ }
+
+ if (!snapshot.exists) {
+ await reference.setData(
+ DocumentData.fromMap({
'builder': builder,
'build_number': buildNumber,
'review': review,
'patchset': patchset,
- }));
+ }),
+ SetOptions(merge: true));
+ }
+ }
}
diff --git a/functions/node/test/firestore_test_nodejs.dart b/functions/node/test/firestore_test_nodejs.dart
index 1ff458b..c475dc9 100644
--- a/functions/node/test/firestore_test_nodejs.dart
+++ b/functions/node/test/firestore_test_nodejs.dart
@@ -138,7 +138,7 @@
// Send first chunk of second run on the same patchset, with an approved
// failure and an unapproved failure.
await Tryjob(testReviewPath, null, firestore, null)
- .process([tryjobOtherFailingChange, tryjobFailingChange]);
+ .process([tryjob2OtherFailingChange, tryjob2FailingChange]);
final reference = fs.firestore
.document('try_builds/$testBuilder:$testReview:$testPatchset');
document = await reference.get();
@@ -149,7 +149,7 @@
expect(document.data.getInt('processed_chunks'), 1);
// Send second chunk.
await Tryjob(testReviewPath, 3, firestore, null)
- .process([tryjobExistingFailure]);
+ .process([tryjob2ExistingFailure]);
document = await reference.get();
expect(document.data.getBool('success'), isFalse);
expect(document.data.getBool('completed'), isNull);
@@ -157,7 +157,7 @@
expect(document.data.getInt('processed_chunks'), 2);
// Send third and final chunk.
await Tryjob(testReviewPath, null, firestore, null)
- .process([tryjobPassingChange]);
+ .process([tryjob2PassingChange]);
document = await reference.get();
expect(document.data.getBool('success'), isFalse);
expect(document.data.getBool('completed'), isTrue);
@@ -166,7 +166,7 @@
// Send first chunk of a third run, with only one chunk.
await Tryjob(testReviewPath, 1, firestore, null)
- .process([tryjobPassingChange]);
+ .process([tryjob3PassingChange]);
document = await reference.get();
expect(document.data.getBool('success'), isTrue);
expect(document.data.getBool('completed'), isTrue);
diff --git a/functions/node/test/test_data.dart b/functions/node/test/test_data.dart
index 9ab4e31..1f88a2a 100644
--- a/functions/node/test/test_data.dart
+++ b/functions/node/test/test_data.dart
@@ -128,6 +128,8 @@
const testBuilder = 'test_builder';
const testBuildNumber = "308";
+const tryjob2BuildNumber = "309";
+const tryjob3BuildNumber = "310";
const testConfiguration = 'test_configuration';
const testReview = 123;
const testPatchset = 3;
@@ -158,7 +160,7 @@
"changed": true
};
-final Map<String, dynamic> tryjobOtherFailingChange =
+final Map<String, dynamic> tryjob2OtherFailingChange =
Map<String, dynamic>.from(tryjobFailingChange)
..addAll({
"name": "test_suite/other_failing_test",
@@ -167,7 +169,8 @@
"expected": "Pass",
"matches": false,
"previous_result": "Pass",
- "changed": true
+ "changed": true,
+ "build_number": tryjob2BuildNumber,
});
final Map<String, dynamic> tryjobExistingFailure =
@@ -182,6 +185,18 @@
"changed": false
});
+final Map<String, dynamic> tryjob2ExistingFailure =
+ Map<String, dynamic>.from(tryjobExistingFailure)
+ ..addAll({
+ "build_number": tryjob2BuildNumber,
+ });
+
+final Map<String, dynamic> tryjob2FailingChange =
+ Map<String, dynamic>.from(tryjobFailingChange)
+ ..addAll({
+ "build_number": tryjob2BuildNumber,
+ });
+
final Map<String, dynamic> tryjobPassingChange =
Map<String, dynamic>.from(tryjobFailingChange)
..addAll({
@@ -194,6 +209,18 @@
"changed": true
});
+final Map<String, dynamic> tryjob2PassingChange =
+ Map<String, dynamic>.from(tryjobPassingChange)
+ ..addAll({
+ "build_number": tryjob2BuildNumber,
+ });
+
+final Map<String, dynamic> tryjob3PassingChange =
+ Map<String, dynamic>.from(tryjobPassingChange)
+ ..addAll({
+ "build_number": tryjob3BuildNumber,
+ });
+
String gitilesLog = '''
)]}'
{
