fix(storage): resolve race conditions in archived generation tests - #3341
Conversation
Introduce a pre-configured versioned bucket in the StorageFixture to eliminate GCS metadata propagation delays and utilized shared versioned bucket across all generation tests
product-auto-label
Bot
added
api: storage
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors several integration tests—specifically CopyFileArchivedGenerationTest, DeleteFileArchivedGenerationTest, and ListFileArchivedGenerationTest—to use a shared, pre-configured versioned bucket (BucketNameVersioned) from the test fixture, eliminating the need to repeatedly enable and disable versioning within individual tests. It also cleans up unused sample variables and standardizes object name generation. The review feedback correctly identifies a performance issue in DeleteFileArchivedGenerationTest.cs where an IEnumerable is enumerated multiple times, causing redundant HTTP requests, and suggests materializing the sequence to avoid this.
| var objects = listFileArchivedGenerationSample.ListFileArchivedGeneration(_fixture.BucketNameVersioned); | ||
|
|
||
| Assert.Equal(2, objects.Count(a => a.Name == objectName)); | ||
| Assert.Equal(2, objects.Count(a => a.Name == objectName)); | ||
|
|
||
| // Get Generations | ||
| var testFiles = objects.Where(a => a.Name == objectName).ToList(); | ||
| long? fileArchivedGeneration = testFiles[0].Generation; | ||
| long? fileCurrentGeneration = testFiles[1].Generation; | ||
| // Get Generations | ||
| var testFiles = objects.Where(a => a.Name == objectName).ToList(); |
There was a problem hiding this comment.
The objects variable is an IEnumerable returned by ListFileArchivedGeneration, which is lazily evaluated. Calling objects.Count(...) and then objects.Where(...).ToList() causes the sequence to be enumerated twice, triggering multiple redundant HTTP requests to Google Cloud Storage. Materializing the sequence with .ToList() immediately avoids this double enumeration.
var objects = listFileArchivedGenerationSample.ListFileArchivedGeneration(_fixture.BucketNameVersioned).ToList();
// Get Generations
var testFiles = objects.Where(a => a.Name == objectName).ToList();
Assert.Equal(2, testFiles.Count);
| objects = listFileArchivedGenerationSample.ListFileArchivedGeneration(_fixture.BucketNameGeneric); | ||
| Assert.Equal(1, objects.Count(a => a.Name == objectName)); | ||
| objects = listFileArchivedGenerationSample.ListFileArchivedGeneration(_fixture.BucketNameVersioned); | ||
| Assert.Equal(1, objects.Count(a => a.Name == objectName)); |
There was a problem hiding this comment.
If the test fails during assertion, we are failing to cleanup the object versions.
| _fixture.CollectArchivedFiles(_fixture.BucketNameVersioned, objectName, testFiles[0].Generation); | ||
| _fixture.CollectArchivedFiles(_fixture.BucketNameVersioned, objectName, testFiles[1].Generation); |
There was a problem hiding this comment.
Move these lines above the assertion
|
|
||
| _fixture.CollectArchivedFiles(_fixture.BucketNameGeneric, objectName, fileArchivedGeneration); | ||
| _fixture.CollectArchivedFiles(_fixture.BucketNameGeneric, objectName, fileCurrentGeneration); | ||
| _fixture.CollectArchivedFiles(_fixture.BucketNameVersioned, objectName, fileArchivedGeneration); |
There was a problem hiding this comment.
Move the collection of these generations to their respective locations. If second upload fails, we will fail to collect the first generation will not be collected.
3 participants
This PR provides a fix to the issue and
resolves the test flakiness observed in the archived generation tests (such as
CopyFileArchivedGenerationTest,ListFileArchivedGenerationTestandDeleteFileArchivedGenerationTest).Previously, these tests were failing intermittently with
404 No such objector assertion errors (Expected: 2, Actual: 1).Because the tests were creating, deleting objects immediately after enabling versioning on the bucket, GCS had not always propagated the versioning state, resulting in objects being permanently deleted instead of archived.
This PR removes bucket versioning toggling from generation tests and introduces
BucketNameVersioned(shared versioned bucket) in theStorageFixtureand utilized shared versioned bucket across all generation tests eliminating race conditions.