Skip to content

Fix pending-upload download race that purges local PVE files - #15

Open
Leonorus wants to merge 2 commits into
sol1:mainfrom
Leonorus:down_up_race_fix
Open

Fix pending-upload download race that purges local PVE files#15
Leonorus wants to merge 2 commits into
sol1:mainfrom
Leonorus:down_up_race_fix

Conversation

@Leonorus

@Leonorus Leonorus commented Jul 6, 2026

Copy link
Copy Markdown

Fixes Issue #14

Summary

Fix a data-loss race where /v1/download could 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 returns HeadObject 404. Previously, handleDownload treated that as “deleted on S3”, purged the cached file, and lost the only copy.

Changes

  • Add explicit durable PendingUpload metadata for locally authoritative files.
  • Serve pending or metadata-less cached files locally and queue them for upload instead of purging or overwriting them from S3.
  • Route download-triggered uploads through the watcher debounce path.
  • Scan cache directories on watcher startup so unconfirmed local files survive daemon restarts.
  • Skip age/size eviction for files still pending upload.
  • Deduplicate background uploads, rerun uploads when a file changes mid-transfer, and cancel in-flight uploads on delete.
  • Record S3 ETags after watcher uploads so cache staleness checks recognize freshly uploaded files.
  • Add regression coverage for pending download handling, pending rewrite handling, startup scan, upload rerun/cancel behavior, and pending-upload eviction protection.
  • Update debian/changelog.

Testing

  • go test ./internal/cache passed.
  • go test ./internal/api -run 'TestHandleDownload_LocalPendingUpload_ServedNotPurged|TestHandleDownload_PendingRewrite_ServesLocalNotS3|TestHandleDelete_CancelsInFlightUpload|TestUploadNewFile_RerunAfterTriggerDuringUpload|TestMarkPendingUpload|TestScanPendingUploads' passed.

Filipp Vysokov and others added 2 commits July 6, 2026 20:11
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant