Fix pending-upload download race that purges local PVE files - #15
Open
Leonorus wants to merge 2 commits into
Open
Fix pending-upload download race that purges local PVE files#15Leonorus wants to merge 2 commits into
Leonorus wants to merge 2 commits into
Conversation
handleDownload treated any HeadObject 404 on a cache-present file as "deleted on S3" and removed it. A file just written into the cache dir by PVE (e.g. a cloud-init user-data ISO attached right after creation) has no S3 object yet while it waits out the watcher's 3s debounce, so activate_volume's download call purged it before it was ever uploaded — losing the file permanently and booting the VM with no config. Cache metadata is only ever written after a confirmed S3 operation (download, upload, watcher upload, resync), so a cache file without a meta sidecar is now served locally and enqueued for upload instead of being purged. Files with confirmed S3 provenance keep the fail-closed purge-on-404 behavior introduced for deleted-on-S3 mutable ISOs. uploadNewFile now has two callers, so an in-flight map prevents the watcher and the download path from uploading the same file twice. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019aQcziV1pUTaKAbDkwy7Ro
Review of the HEAD-404 fix showed the pending-upload state was inferred from fragile signals (metadata absence, ETag-less metadata), leaving the same data-loss race open on other paths. Make it explicit and durable: - FileMeta gains PendingUpload; the watcher writes the marker as soon as it detects a local write (zero value = synced, so old sidecars keep their meaning). Download renames and upload links register themselves so the watcher doesn't mark the daemon's own files. - handleDownload decides from a metadata snapshot before looking at the HEAD result: pending (or metadata-less) files are served locally and queued for upload whatever S3 says — previously a HEAD 200 for a rewritten key re-downloaded the old S3 object over the new local file, and a HEAD-404/GetMeta interleaving could purge a just-uploaded file. - Uploads triggered outside fsnotify go through the watcher's debounce queue, so they get the same stability window and in-use check, and no /proc walk runs on the request path. - The watcher scans the cache on startup for unconfirmed files, so pending uploads survive daemon restarts; eviction (age and size) skips files whose only copy is local. - An upload trigger landing mid-transfer schedules a rerun instead of being silently dropped; delete cancels any in-flight upload of the same file so a completing PutObject can't resurrect the object. - Uploads record the S3-assigned ETag afterwards so the staleness check recognizes the cached copy instead of re-fetching it on first access. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019aQcziV1pUTaKAbDkwy7Ro
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.
Fixes Issue #14
Summary
Fix a data-loss race where
/v1/downloadcould delete a locally-created cache file before the watcher had uploaded it to S3.PVE can write files such as cloud-init user-data ISOs directly into the proxs3 cache and immediately call
activate_volume. During the watcher debounce window, S3 still returnsHeadObject404. Previously,handleDownloadtreated that as “deleted on S3”, purged the cached file, and lost the only copy.Changes
PendingUploadmetadata for locally authoritative files.debian/changelog.Testing
go test ./internal/cachepassed.go test ./internal/api -run 'TestHandleDownload_LocalPendingUpload_ServedNotPurged|TestHandleDownload_PendingRewrite_ServesLocalNotS3|TestHandleDelete_CancelsInFlightUpload|TestUploadNewFile_RerunAfterTriggerDuringUpload|TestMarkPendingUpload|TestScanPendingUploads'passed.