Repository navigation
Conversation
C1, C2, L6: always-end-and-report H1, L1, L2: safe-download-setup H2, H3, M10, M11, L7, L9: safe-download-files M1, M3, L8, L11: writer-queue-accounting M2, M14: faster-read-loop M4, M8, M9, M13, L3, L5: errors-and-retries M5, M6, M7, M12, L4: safer-links M15, L10, L12, L13, L14: release-and-housekeeping
…d (C1, C2) C1: a write error of the storage is now a StorageException. It is not retried, so the download fails at once. When the writer dies, waiting chunks and flushes wake up. C2: the end of a download does the work first (flush, close, rename) and sets the status last. If the work fails the status is Failed and DownloadFileCompleted comes once with the error. The file is always closed.
When the storage refused a block, the rented buffer went back to the ArrayPool twice (by the packet and by the read loop). Two later reads then shared one array, and a download could write the bytes of another download into its file. The storage now owns the buffer, also when the write throws.
A bad request header or a client factory that throws left the status Created and the completion task open, so Clear(), Dispose() and the next download waited for ever. The caller still gets the exception, but the status is Failed and the task ends.
…(H2, H3) H2: a resume from a DownloadPackage starts from zero when the temp file is missing or cut, or when the server file changed (size, ETag or Last-Modified). Before, it gave a broken file that said Completed. H3: only one download in the process may write a name.download file. With FileExistPolicy.Rename the second download gets another name; otherwise it fails at once with an IOException. Before, both said Completed and no file was left.
CI runs the net10.0 and net11.0 tests at the same time in one folder. With the fixed name f.bin the two runs moved and deleted each other's file. The old code hid it (a lost rename still said Completed); now the lost rename is reported as Failed, so the test failed.
|
There are still some problems at present. I will create a issue later. |
Two downloads that start at the same moment with FileExistPolicy.Rename could both pick the same free name; the second then failed its claim. Now a lost claim asks for the next free name again.
The test checks the file bytes. The 500 ms BlockTimeout made its one-connection resume fail on the slow AppVeyor box.
The temp file claim was freed after DownloadFileCompleted, so a retry started from the event failed with 'another download is writing'. A new download started right after a stop failed the same way. Now the claim is freed before the event, and a new download waits while a cancelled holder still closes the file.
…s the file A resume from a DownloadPackage did not claim its temp file, so a second download to the same path could still write it. With ClearPackageOnCompletionWithFailure, a failed rename at the end deleted the fully downloaded temp file. It is kept now.
On the Windows CI box the second download started before the resume had claimed its file. Now the resume waits in its first progress event until the second download has ended.
DownloadProgressChanged does not stop the download, so on the slow Windows box the resume ended before the second download started. A chunk event runs on the download thread and really holds it.
On the slow Windows box the first download was cancelled before it started, so its package had no file name and the resume went to memory. The test now waits for the first chunk event before the stop.
This branch was successfully deployed
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 the 2 critical and 3 high issues from the 7.0.1 review, and adds OpenSpec changes for all 34 issues.
OpenSpec
8 new changes in
openspec/changes/cover all 34 issues (the 16 older changes are not touched):always-end-and-reportsafe-download-setupsafe-download-fileswriter-queue-accountingfaster-read-looperrors-and-retriessafer-linksrelease-and-housekeepingOnly the C and H tasks are ticked. Medium and low issues are not fixed here.
Fixes (each test failed on the old code first)
StorageException(anIOException) and is never retried. When the writer dies, waiting chunks and flushes wake up. Test:StorageErrorTest(Running for ever before; even cancel hung with a memory limit).DownloadPackage.TryFinishAsyncdoes the flush, close and rename first and sets the status last. If that work fails the status is Failed andDownloadFileCompletedcomes once with the error. The file is always closed. Test:StorageErrorTest(memory and file downloads, failed rename).Dispose()and the next download hang. The caller still gets the exception, but the status is Failed and the completion task ends. Test:SetupErrorTest(badContent-Typeheader, throwing client factory).ETag,Last-Modified). Test:PackageResumeTest.TempFileClaimslets one download per process write aname.download. WithRenamethe second one gets another name; otherwise it fails at once.FileShare.Readwas not used: on Linux it takes a shared lock and does not stop a second writer. Test:SameFileTwiceTest.ArrayPooltwice, so two later reads could share one array and mix the bytes of two downloads (seen in a full test run). Test:ChunkBufferOwnershipTest.Small behavior change
ConcurrentStream.FlushAsync()throwsStorageExceptionafter a write error (it threwAggregateException).Tests
Full suite on .NET 10, Linux: 721 passed, 0 failed, build with 0 warnings. Other target frameworks and Windows/macOS are checked by CI.
Generated by Claude Code