fix(cubestore): drop the staged file when an upload fails - #11621
Open
karthikb-appcard wants to merge 1 commit into
Open
fix(cubestore): drop the staged file when an upload fails#11621karthikb-appcard wants to merge 1 commit into
karthikb-appcard wants to merge 1 commit into
Conversation
Every upload is staged to uploads/<remote_path> via CommonRemoteFsUtils::temp_upload_path, and only a SUCCESSFUL upload removes it, by renaming it into place at the end of upload_file. Every early return above that rename leaves the file behind, including the ? on put_object_stream. Nothing else deletes it. cleanup_local_files_loop in remotefs/cleanup.rs uses a non-recursive read_dir(), skips anything that is not a plain file, and skips anything not ending in .parquet. uploads/ is a directory, so the loop never enters it, which is also what the comment on temp_upload_path intends: the subdirectory is used so cleanup will not touch files being prepared for upload. So any failed upload leaks its staged bytes permanently. On a router whose uploads were failing, this accumulated until the volume filled. The chunk upload paths already guard against exactly this with scopeguard::guard(path, ensure_temp_file_is_dropped) in store/mod.rs and store/compaction.rs. This applies the same guard at QueueRemoteFs::upload_loop, the single choke point every upload passes through, so it covers all backends. ensure_temp_file_is_dropped checks the file exists before removing it, so it is a no-op after a successful rename. Also covers the already-deleted skip branch and the lock error return.
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.
Problem
Every upload is staged to
uploads/<remote_path>viaCommonRemoteFsUtils::temp_upload_path, and only a successful upload removes it, by renaming it into place at the end ofupload_file. Every early return above that rename leaves the file behind — including the?onput_object_stream.Nothing else deletes it:
cleanup_local_files_loop(remotefs/cleanup.rs) uses a non-recursiveread_dir(), skips anything that is not a plain file, and skips anything not ending in.parquet.uploads/is a directory, so the loop never enters it.temp_upload_pathintends — the subdirectory exists precisely so cleanup will not remove files being prepared for upload.So any failed upload leaks its staged bytes permanently, for the lifetime of the node. With a persistent upload failure (expired credentials, a bucket policy change, a network partition) this accumulates without bound. We hit it on a router whose S3 uploads had been failing: the local volume filled and had to be reclaimed by hand.
Fix
The chunk upload paths already guard against exactly this:
store/mod.rs:let local_file = scopeguard::guard(local_file, ensure_temp_file_is_dropped);store/compaction.rs: same, over a list of staged files.The metastore/cachestore snapshot path does not. Rather than add a guard per call site, this applies it at
QueueRemoteFs::upload_loop— the single choke point every queued upload passes through — so it covers allRemoteFsimplementations.ensure_temp_file_is_droppedchecks the file exists before removing it, so it is a no-op after a successful rename. The guard also covers the two non-error paths that currently leak: thedeletedskip branch, and the?onacquire_lock.Three lines plus the import, no behaviour change on the success path, and it reuses the existing helper and the existing idiom.
Possibly related
#9114 reports router-side
temp-uploadsgrowing continuously while workers stay clean. That asymmetry is consistent with this bug: the worker chunk paths are scope-guarded and the router's snapshot uploads are not. I have not reproduced that reporter's setup, so I would not claim it as the same root cause without their logs — but if their uploads were failing, this would explain it.