Repository navigation
Tolerate missing storage objects in the file-size backfill - #2094
Merged
Merged
Conversation
The FileResourceSizeMigration job (#1777) crashes with a raw NoSuchKeyException/BlobStorageException/NoSuchFileException whenever a FileResource points at an object that's no longer in storage, exhausting JobRunr's retries and leaving the migration_item behind for investigation (per MigrationItemCleanupFilter) even though the outcome will never change on retry. Resources with genuinely missing backing files are a known, already-tolerated condition elsewhere in this codebase (see FixMissingFilesJobRequestHandler), so treat it the same way here. - New FileNotFoundInStorageException (IOException) signals "the object is confirmed absent", distinct from other storage errors that might be transient and worth retrying. - Each backend translates its own "definitely not found" signal into it: AWS (a HEAD response has no body, so S3 can't return a specific error code -- some implementations throw NoSuchKeyException regardless, but a plain S3Exception with a 404 status is what's actually guaranteed, and what LocalStack sends, so the status code is checked rather than the exception type), Azure (BlobErrorCode.BLOB_NOT_FOUND), GCS (already represented as a null Blob), Local (NoSuchFileException). Other storage errors continue to propagate unchanged. - MigrationService.getFileSize excludes FileNotFoundInStorageException from its @retryable, since retrying a confirmed absence just spends 3 more round trips (and delay) confirming the same thing again. - FileResourceSizeJobRequestHandler catches it, logs a warning, and leaves the resource's size unset instead of letting the job fail -- there's nothing more this job can do for a resource whose file is gone; FixMissingFilesMigration is what actually repairs that. Verified against real LocalStack S3 that a HEAD on a missing key surfaces as a plain S3Exception rather than NoSuchKeyException specifically, which is why the AWS side checks status code rather than catching the specific subtype. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Fixes the crash reported after merging #2089:
FileResourceSizeJobRequestHandler(the size backfill job from #1777/#2089) crashes whenever aFileResourcerow points at an object that's no longer actually in storage. This isn't transient — retrying will always fail the same way — but the job let it propagate as a generic failure, so it burned through@Retryable's 3 attempts + JobRunr's own 3 retries, then permanently failed and left themigration_itemrow behind for investigation (per #2092'sMigrationItemCleanupFilter), even though there's nothing to investigate: the file is just gone.This is a known, already-tolerated category of problem in this codebase — see
FixMissingFilesJobRequestHandler, which exists specifically to detect and repair extensions with missing files. This PR makes the size backfill tolerate it the same way instead of crash-looping on it.How
FileNotFoundInStorageException(anIOException) signals "the object is confirmed absent", as opposed to some other, possibly-transient, storage error.NoSuchKeyExceptionregardless, but a plainS3Exceptionwith a 404 status is what's actually guaranteed. I verified against a real LocalStack S3 instance that LocalStack'sHeadObjecton a missing key throws the plainS3Exception, notNoSuchKeyException— so the fix checks the status code rather than the exception subtype (matching the pattern this codebase already uses inAzureBlobStorageService.removeFile).BlobErrorCode.BLOB_NOT_FOUND.Blob— now translated explicitly instead of a genericIOException.NoSuchFileExceptionfromFiles.size(...).MigrationService.getFileSizeexcludesFileNotFoundInStorageExceptionfrom its@Retryable— retrying a confirmed absence just spends 3 more round trips (and delay) confirming the same thing again.FileResourceSizeJobRequestHandlercatches it, logs a warning, and leaves the resource'ssizeunset instead of letting the job fail. There's nothing more this job can do for a resource whose file is gone;FixMissingFilesMigrationis the actual repair mechanism.Testing
FileResourceSizeJobRequestHandlerTest: the job completes normally (no size set, no exception) when storage reports the object missing; still records the size on the normal path.AwsStorageServiceIntegrationTest.testGetFileSizeThrowsFileNotFoundInStorageExceptionForAMissingObject, run against real LocalStack — this is what caught that LocalStack throws a plainS3Exception, notNoSuchKeyException, for a missing HEAD target.unitTestssuite passes (948 tests) ands3IntegrationTestspasses (24 tests).🤖 Generated with Claude Code