fix(downloads): give the three download-finalization settings a reader again - #919
Open
m4bard wants to merge 5 commits into
Open
fix(downloads): give the three download-finalization settings a reader again#919m4bard wants to merge 5 commits into
m4bard wants to merge 5 commits into
Conversation
…r again Settings > Download has six numeric inputs. The last three, Download Completion Stability, Missing-source Retry Initial Delay and Missing-source Max Retries, validate, persist, round-trip through the API and have frontend defaults, and nothing in the backend read any of them. Tuning them changed nothing. They are not controls that were never wired. All three had consumers, added alongside the properties in e7989da, and the reads were lost in 1d6a8e9 when Listenarrs#535/Listenarrs#492 restructured download importation and deleted the two files holding them. Stability window. Finalization began in the same pass that first saw the client report completion, so the setting's own default of 10 seconds was ignored along with any value an operator chose. The monitor now holds the transition into Completed until the client has kept reporting it for the configured window. It holds the transition rather than the update, so progress and size still persist and only finalization waits, and it reverts the status rather than skipping the write so the next cycle sees the same transition again and can let it through without anything needing to remember that one is pending. A window of zero finalizes in the same pass, as before. Retry budget. MaxRetries is a per-job column that EnqueueAsync never set, so every job got the property default of 3 whatever the setting said. It is now applied by the processor on each attempt. That is deliberate rather than set at enqueue: the budget then follows the current setting for jobs already in flight, and DownloadProcessingJobService stays clear of IConfigurationService, which DownloadProcessingJobService_ResolvesWithoutAdaptersOrHostedWorkers pins. Retry delay. ScheduleRetry now takes the configured initial delay. The old expression also had an off-by-one: it read RetryCount after incrementing it, so the first retry waited a minute while the summary comment above it said 30 seconds and the inline comment beside it said 0.5 minutes. Neither matched the code. The first retry now waits exactly the configured delay and each later one doubles it. The settings are named for the missing-source case and the old code read them only in that branch. In the current architecture there is a single per-job retry mechanism shared by every failure path in the processor, so applying them to some branches and not others would give an operator a control whose effect depended on which internal branch failed. I have applied them to the job. If you would rather they stayed scoped to the missing-source branches that is a smaller change and I will make it. Four existing tests drove a download to completion and asserted on the result in the same cycle. They passed because the stability setting had no reader. They now say which window they depend on rather than inheriting it. Verified by reverting each of the three reads in turn and confirming the matching test fails: the held transition, the backoff expression, and the configured retry budget. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YEVQ7qDJLk5196MFeggWuA
Of the three settings this branch gives a reader, two have a test that fails when the read is taken out again. The initial retry delay does not. The three domain tests hand ScheduleRetry a delay directly, so they pin the arithmetic and say nothing about where the number comes from, and a processor that never called GetApplicationSettingsAsync and let the parameter fall to its default of thirty seconds would keep every test in the suite green. This drives a missing source through the processor with the setting at ten minutes and asserts NextRetryAt lands there. Ten minutes is far enough from the thirty second default that the two cannot be confused. NOT YET RUN. The shared build lock on this machine did not come free before the review ended, so this has not been compiled or executed. Run it before relying on it. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
ScheduleRetry doubles the configured delay on every attempt with no ceiling. That was unreachable before this branch, because MaxRetries was pinned at the property default of three, so the worst case was two minutes. Now that the job takes MissingSourceMaxRetries from settings, both inputs are operator settable, and the download settings screen offers up to 600 seconds and up to 20 retries with no server-side validation of either. At both maxima the twentieth retry is scheduled a little over ten years out. Nothing throws, so the only symptom is a job that is still Pending and never runs, which reads as lost to whoever is looking at the queue. A day rather than an hour. Waiting hours for a source that needs an operator to intervene is a legitimate thing to configure, and an hour would cut the ladder short for anyone who deliberately raised the budget; a day is long enough to be a real backoff and short enough that the job is visibly still scheduled. At the shipped defaults nothing reaches it: 30 seconds over three retries is two minutes. The theory covers both sides. Rows at attempts 1, 3 and 12 are below the ceiling and pin that the ladder is untouched, so a clamp that pinned every delay to a day would fail them. Control: removing the Math.Min fails the two rows above the ceiling and leaves the three below it green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GDr4ZwWYwhX8fAy26Vq5KD
Two shapes in the stability window, neither of them observable today, both of them the kind of thing the next reader has to work out from scratch. The guard read "hold a completion" and meant "hold anything that is not already Completed". EfDownloadRepository.GetActiveAsync returns ImportPending and Moved rows on every cycle, so a row that had already finalized and was still being reported complete by the client entered the window and had its status written back for as long as the window lasted. It ends in the same place either way, because the window expires and the row is written to Completed regardless, so nothing observable changes. What changes is the blast radius: a download already in import is now left exactly as the monitor handled it before this window existed, which is what a change about holding a completion should do. IsPreCompletion names the set, and a theory pins every status in the enum against it. Control: adding the three post-completion statuses back to the set fails exactly those three rows. The dictionary comment answers the question the code invites. Entries are removed when the transition is let through and when the client stops reporting completion, but not when a download vanishes from the client mid-window, and the processor is a singleton, so those entries live for the process. The bound is the number of downloads that completed and then disappeared before their window elapsed, each one a string and a DateTime, which is a handful over an uptime rather than something that grows with the library. Sweeping it would cost a second timer or a walk of the dictionary every cycle, both more than the leak. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GDr4ZwWYwhX8fAy26Vq5KD
…that needs it InitializeAsync saved a settings row with the completion stability window zeroed for every test in the class, but the application settings repository only accepts one save per test: it stamps Version = 1 on the first write and rejects any later save whose version is not ahead of it. Every other test in this file does its own settings save, which the class-wide save in InitializeAsync was silently pre-empting; nothing failed here only because no other declared branch shares this test file yet. Only MonitorDownloadsAsync_DownloadingBecomesCompleted actually needs the window gone, since it polls in a tight loop on the real clock. Move the save there instead of the shared setup. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GDr4ZwWYwhX8fAy26Vq5KD
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 #894.
Settings > Download has six numeric inputs. The last three, Download Completion Stability, Missing-source Retry Initial Delay and Missing-source Max Retries, validate, persist, round-trip through the API and have frontend defaults, and nothing in the backend read any of them.
They are not controls that were never wired. All three had consumers, added alongside the properties in
e7989da3, and the reads were lost in1d6a8e97when #535/#492 restructured download importation and deleted the two files holding them.Stability window
Finalization began in the same pass that first saw the client report completion, so the setting's own default of 10 seconds was ignored along with any value an operator chose.
The monitor now holds the transition into
Completeduntil the client has kept reporting it for the configured window.It holds the transition rather than the update, so progress and size still persist and the row stays current while finalization waits. And it reverts the status rather than skipping the write, which means the next cycle sees the same transition again and can let it through once the window has passed, without anything else needing to remember that one is pending. A window of zero finalizes in the same pass, exactly as before.
Retry budget
MaxRetriesis a per-job column thatEnqueueAsyncnever set, so every job got the property default of 3 whatever the setting said.It is applied by the processor on each attempt rather than baked in at enqueue, for two reasons. The budget then follows the current setting for jobs already in flight. And
DownloadProcessingJobServicestays clear ofIConfigurationService, whichDownloadProcessingJobService_ResolvesWithoutAdaptersOrHostedWorkerspins on purpose. My first attempt injected it there and broke that test, which was the test doing its job.Retry delay
ScheduleRetrynow takes the configured initial delay.The old expression also had an off-by-one. It read
RetryCountafter incrementing it, so the first retry waited a minute while the summary comment above it said 30 seconds and the inline comment beside it said 0.5 minutes. Neither matched the code. The first retry now waits exactly the configured delay and each later one doubles it.One thing I would rather you decided
The settings are named for the missing-source case, and the old code read them only in that branch. In the current architecture there is a single per-job retry mechanism shared by every failure path in the processor, so applying them to some branches and not others would give an operator a control whose effect depended on which internal branch failed.
I have applied them to the job. If you would rather they stayed scoped to the missing-source branches, that is a smaller change and I will make it.
Tests
Four existing tests drove a download to completion and asserted on the result in the same cycle. They passed because the stability setting had no reader. They now say which window they depend on rather than inheriting it.
Seven added: three on the backoff arithmetic, three on the stability window, one theory on the retry budget. The suite has no fake clock, so the stability tests use a small adjustable
TimeProviderrather than sleeping.Checked by reverting each of the three reads in turn and confirming the matching test fails: the held transition, the backoff expression, and the configured retry budget.
Worked through with Claude Code at my direction. The claims above were checked by running them rather than by reading, and I reviewed this before posting.