sdk_v2/cpp: resumable downloads (cross-process lock + per-chunk state + linked cancellation)#793
Draft
bmehta001 wants to merge 4 commits into
Draft
sdk_v2/cpp: resumable downloads (cross-process lock + per-chunk state + linked cancellation)#793bmehta001 wants to merge 4 commits into
bmehta001 wants to merge 4 commits into
Conversation
Increment 1 of the resumable-downloads port (see docs/ResumableDownloadsPlan.md). No public C ABI changes. CrossProcessFileLock - New RAII helper backed by an OS-level exclusive lock on <dir>/.download.lock: Windows uses CreateFileW with FILE_SHARE_NONE + FILE_FLAG_DELETE_ON_CLOSE; POSIX uses open(O_CREAT|O_RDWR|O_CLOEXEC) + flock(LOCK_EX|LOCK_NB). - Writes a PID:<pid>,Time:<iso8601> diagnostic line for crash forensics. - WaitForLockForDirectory polls at 1.25 s with a 3 h timeout. The cancellation hook is a std::function<bool()> predicate (not a bare atomic) so callers can route it through their own cancellation channel — DownloadManager forwards it through the existing progress callback's non-zero return. DownloadManager::DownloadModel - Acquires the cross-process lock immediately after create_directories and before writing the in-progress signal file. - Re-checks the cache after acquiring the lock to short-circuit when another process just finished the same download. - Now stores ILogger& logger_ so the lock acquisition can log who is waiting. DownloadBlobsToDirectory (skip-existing) - New IsDownloadNeeded(blob, local_path) filter: blobs whose local file already exists at the expected content_length are skipped. - Skipped bytes are credited toward the total — the initial progress callback now emits skipped_bytes / total_size * 100 instead of always 0%, so resumed downloads start at an honest percentage rather than rewinding to zero. - If every blob is already on disk the function emits 100% and returns. Tests - 9 new CrossProcessFileLockTest cases (acquire, release, contention, recovery after release, directory creation, wait happy path, wait-then-acquire, cancellation, timeout). - 4 new BlobDownloadTest cases for skip-existing (same-size, wrong-size, progress accounting, everything-cached). - Full targeted suite passes 40/40 in 14 s. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Increment 2 of the resumable-downloads C++ port. Adds a <file>.dlstate sidecar that tracks per-chunk completion via a truncated bitmap (matching the C# BlobDownloadState design from neutron-server), and replaces AzureBlobDownloader::DownloadBlob's batch loop with a worker pool that shares a single Azure::Core::Context. The first chunk failure calls Cancel() on the shared context and flips an internal cancel flag, so every other in-flight chunk drains within tens of milliseconds instead of waiting on its own retry+timeout budget. Highlights: - BlobDownloadState (new): compact binary on-disk format. ~45-byte LE header (magic FLDS + version + sizes + counters) followed by the truncated bitmap suffix. The prefix of fully-completed chunks is implied — SaveState advances �itmap_byte_aligned_start past every fully-set word to keep the sidecar proportional to the unfinished tail. LoadState rejects on magic, version, or layout (blob_size / chunk_size / total_chunks) mismatch and starts the download fresh in that case. Atomic save via tmp file + rename, with remove-then-rename fallback for filesystems that don't replace atomically. - AzureBlobDownloader rework: protected virtual GetBlobSize and DownloadChunkToBuffer (against an opaque ChunkContext) form a test seam so subclasses can simulate per-chunk behavior without touching Azure. Worker pool uses an atomic queue index over pending chunks; workers claim, fetch, write, mark complete, and periodically save (max(10, num_chunks/50) chunks). Pre-allocation is skipped if the file is already at full size, so resume doesn't discard valid bytes. Sidecar is persisted on any failure and deleted on full success. - IsDownloadNeeded now treats the presence of a .dlstate sidecar as "download still needed" — the data file may be pre-allocated with holes. - AzureBlobDownloader picks up an optional ILogger*; DownloadManager passes its own logger through. Tests: - 15 BlobDownloadStateTest cases (create/mark/save/load/delete, gap enumeration, partial final chunk math, byte-aligned-start advancement, rejection of magic/size mismatches). - 5 AzureBlobDownloaderResumeTest cases via a FakeChunkAzureDownloader subclass: resume skips already-completed chunks, sidecar persists on chunk failure, stale sidecar is cleaned up for empty blobs, and a failing chunk drains 9 sleeping peers within ~300 ms (well under the 2 s threshold) — exercising the linked-cancellation cascade end to end. 20 new tests; full BlobDownloader/DownloadManager/CrossProcessFileLock suite is 59/59 in ~15 s. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
…ker memory at 64 KB) Eliminate the 2 MB-per-chunk std::vector<uint8_t> allocation in AzureBlobDownloader::DownloadBlob by streaming each chunk through a sink callback that forwards 64 KB pieces straight to a thread-safe file writer. Peak download memory drops from concurrency * chunk_size (128 MB at 64-way concurrency on 2 MB chunks) to roughly concurrency * 64 KB (~4 MB) regardless of chunk size, matching the .NET Stream.CopyTo semantics we ported from instead of doubling memory with a buffer copy. The file write strategy is now selected via a new `FileWriterKind` constructor argument on `AzureBlobDownloader` and backed by a small `IFileWriter` abstraction with two implementations: - `Positional` (default, recommended): lock-free positional writes using `pwrite` on POSIX and `WriteFile` + `OVERLAPPED.Offset` on Windows. No user-space mutex; the kernel orders disjoint-range writes. - `MutexFstream` (comparison / portable fallback): single shared `std::fstream` guarded by an internal mutex. The chunk-write fast path that used to open a fresh fstream and seek under a mutex per chunk is now subsumed by this writer; the file handle is opened once and reused for every WriteAt. Both writers handle `Open` correctly for the resume path: an existing file at exactly the expected size is preserved (so already-downloaded bytes survive across the writer swap), and any other state triggers pre-allocation to the expected size. The orchestrator's worker pool, atomic cancel cascade, sidecar save cadence, and progress reporting are unchanged. The renamed protected virtual `DownloadChunkStreaming` (replacing `DownloadChunkToBuffer`) is the new test seam; both production code and the existing `FakeChunkAzureDownloader` test double now use the sink callback to deliver chunk bytes. Tests: - New `WriterImpls/FileWriterTest` runs 5 correctness checks against both writer implementations (10 tests total): open semantics for fresh / existing-at-size / existing-at-different-size files, single thread WriteAt, and 8 threads writing 256 KB regions to disjoint offsets validated byte-for-byte after close. - New `FileWriterPerfComparison.PositionalVsMutexFstream` runs the realistic AzureBlobDownloader workload (32 workers, 8 chunks/worker, 2 MB chunks, 64 KB sink pieces, 512 MB total) against both writers and prints wall-clock + MB/s for comparison. Measured locally on NVMe NTFS: Positional averages ~590 MB/s, MutexFstream ~545 MB/s (Positional ~8 percent faster on average; both well above 500 MB/s). - All existing 60 BlobDownload* + AzureBlobDownloader* tests still pass without modification beyond the chunk_hook signature update. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Two unrelated -Werror diagnostics from Clang (and modern GCC) were tripping the Linux x64 and macOS ARM64 jobs on PR #793; Windows + MSVC silently accepted them. 1. blob_download_state.cc: 'kHeaderSize' was a namespace-scope constexpr that nothing referenced (the header layout is materialized by the WriteLE call sequence, not this constant). Triggers -Wunused-const-variable on Clang. Delete it; the layout comment above already documents the 45-byte size. 2. download_test.cc: ChunkFailureCancelsInFlightPeersFast captured 'kFailOffset' in a lambda, but it's a constexpr int64_t used only in a constant expression so the capture is redundant and -Wunused-lambda-capture flags it. Replace [kFailOffset] with [] to match the sister test's pattern. Also fix a latent issue surfaced during review: 3. MutexFstreamFileWriter::WriteAt now calls file_.clear() before seekp() so a prior failure doesn't permanently poison the stream and cause subsequent workers to surface a spurious 'write failed' instead of the original error. Positional writers are unaffected (pwrite/WriteFile are stateless). 71 tests still pass on Windows (35 in the affected suites verified explicitly). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.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
1 participant
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.
Resumable downloads (C++ port)
Ports the resumable-download flow from neutron-server's
AzureExtensions.cs/BlobDownloadState.cs/CrossProcessFileLock.csto the C++ SDK insdk_v2/cpp. Two commits, one per increment.Increment 1 — cross-process lock + skip-existing (commit
45c99d5d)CrossProcessFileLock: RAII,<dir>/.download.lockexclusive lock. Win:CreateFileW FILE_SHARE_NONE FILE_FLAG_DELETE_ON_CLOSE; POSIX:flock LOCK_EX|LOCK_NB. PIMPL (struct Statedefined only in the .cc) keeps platform headers out of the public include.WaitForLockForDirectory: polls every 1.25 s with a 100 ms cancellation slice and a 3 h ceiling, driven by astd::function<bool()>predicate so cancellation can also serve as a heartbeat (the progress callback returning non-zero).DownloadManager::DownloadModel: takes the lock right aftercreate_directories, re-checks the cache once acquired, then proceeds with the existing download flow. StoresILogger&so the lock and resume layers can log uniformly.DownloadBlobsToDirectory: blobs already present at the expected size are skipped; their bytes are credited to the initial progress callback so the percentage stays accurate on resume. Emits 100% immediately if everything is cached.CrossProcessFileLocktests + 4 new skip-existing tests indownload_test.cc.Increment 2 — per-chunk resume + linked cancellation (commit
b6d98db5)BlobDownloadState(new): compact binary<file>.dlstatesidecar. ~45-byte LE header (magicFLDS+ version + sizes + counters) followed by the truncated bitmap suffix. The prefix of fully-completed chunks is implicit —SaveStateadvancesbitmap_byte_aligned_startpast every all-1s word so the file stays proportional to the unfinished tail.LoadStaterejects on magic, version, or layout mismatch and restarts the download in that case. Atomic save via tmp + rename, with remove-then-rename fallback.AzureBlobDownloader::DownloadBlobrework: protected virtualsGetBlobSizeandDownloadChunkToBuffer(against an opaqueChunkContext) form a test seam. Worker pool uses an atomic queue index over the pending-chunks list; workers claim, fetch, write to file (under a singlefile_mutex), mark complete, and periodicallySaveState(everymax(10, num_chunks/50)chunks). Pre-allocation is skipped if the file already matches blob size, so resume doesn't discard valid bytes.Azure::Core::Context+ a singlestd::atomic<bool> internal_cancelper call. The first chunk failure (or external cancel signal) flips the flag and callsctx.Cancel(). Other in-flight chunks see either signal and exit fast — a chunk failing while 9 others are mid-flight drains in ~300 ms in the newChunkFailureCancelsInFlightPeersFasttest.IsDownloadNeedednow also returns true if a.dlstatesidecar exists (the data file may be pre-allocated with holes).Open decisions (resolved)
BlobDownloadStatedesign.dlstate(cross-language compat with C#.download.statewas explicitly out of scope).OnDownloadCompletetelemetryIBlobDownloadermock for orchestrator tests +AzureBlobDownloadersubclass via protected virtuals for chunk-level injection.Tests
CrossProcessFileLockTestcases (Inc 1)download_test.cc(Inc 1)BlobDownloadStateTestcases (Inc 2): create/mark/save/load/delete, gap enumeration, partial final chunk math, byte-aligned-start advancement, rejection of magic / size / total_chunks mismatches.AzureBlobDownloaderResumeTestcases (Inc 2): resume from sidecar, fresh download, sidecar persists on chunk failure, stale sidecar cleaned up for empty blobs, cancel-cascade timing.Targeted
BlobDownloader|DownloadManager|CrossProcessFileLock|AzureBlobDownloadersuite: 60/60 in 13 s. Full gtest run is 789 passed / 54 skipped; the 3 pre-existingModelLoadManagerUnloadTestfailures are environmental (missing local test model fileqwen2.5-0.5b-instruct-generic-cpu-4), not caused by this PR.Out of scope
Region-based download (Increment 3 in the spec) — tracked separately.
Notes for reviewers
sdk_v2/cpp.neutron-server/src/Downloader/(private repo); see Increment 2's bitmap-truncation logic inBlobDownloadState.cs:237-281for the closest analog.