Repository navigation
fix: Retry a file data load whose read fails, and make the retry tests and the FDv2 fallback test robust to timing - #395
Open
kinyoklion wants to merge 2 commits into
Open
kinyoklion wants to merge 2 commits into
kinyoklion wants to merge 2 commits into
Conversation
…t robust to timing The file data source retry tests count the reads of a scripted reader, but they configured a real temporary file with auto-update, so a live file watcher ran. On macOS the watcher has delivered a change notification for the test's own new file shortly after Start, and that extra load added a read or, while the reader threw, started a new failure episode that never retried. The tests that count reads, or that check that a retry applies the data, now use a path in a directory that does not exist, as the multi-path tests do, so no watcher runs. The budget test still needs a real notification: it keeps a watched file changed with an in-place write, waits for the reads and then the notifications to stop instead of asserting a count, so only the new episode's retry can apply the data. FallbackAndRecoveryTasksWellBehaved asserted after a fixed delay while the applier's actions could still be running on its task, through a mock whose fields another thread wrote. The mock records calls under a lock, and the test waits for the last action before it checks the whole call sequence.
Member
Author
|
bugbot review |
With auto-update on, a load that Start or a file-change notification started cleared the failure counts and, if reading a file failed, logged an error without scheduling a retry. A retry that was already pending then found no failures and did nothing, so the data stayed stale, or the source stayed uninitialized, until the next detected change. Only a read error on a retry was retried. A read error on any load is now charged to the path's retry budget, like a parse failure, and schedules a retry. With auto-update off, files are still loaded once. Tests pin both cases, the retry bound for read errors, the budget reset for a load from outside the chain, and the content that a retry applies after a read error.
Member
Author
|
bugbot review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 7e4e268. Configure here.
kinyoklion
marked this pull request as ready for review
November 3, 2026 23:50
This branch has not been 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.
Summary
Four server SDK unit tests fail intermittently on the macOS runner: three
FileDataSourceTestretry tests, andFDv2DataSourceTest.FallbackAndRecoveryTasksWellBehaved, which has also failed onmainitself (run 37331939401). One of those failures was not a test problem: it was the file data source dropping its retries after a read error. This change fixes that in the SDK, and fixes the tests.A read error ends the file data source's retries
Cause. With auto-update on, the file data source retries a load that fails, so that a file read while it is being written is loaded once the write is complete (
FileDataSource.cs). A load that is not a retry (Start, or a file-change notification) starts a new failure episode and clears the failure counts. When such a load failed to read a file, it logged an error and returned without scheduling a retry. A retry that was already pending then found no failures and did nothing. So the data stayed stale, or the source stayed uninitialized, until the next detected change, although the code's own comment on read errors says "another file-change notification is not guaranteed". Only a read error on a retry was retried.The default reader already retries sharing violations inside the read for about 30 seconds, so an ordinary in-place write on Windows does not get here. A missing file (with missing files not skipped), an access error while a file is replaced, a sharing violation that outlasts the reader's own retries, and any error from a custom reader (
FileDataSourceBuilder.FileReader, which is public) do.Fix. With auto-update on, a read error on any load is charged to the path's retry budget, like a parse failure, and schedules a retry: up to five attempts, 600 ms apart, with a warning for each retry and an error when it gives up. With auto-update off, files are loaded once, and a read error is logged as before. The
AutoUpdatedocumentation now says the source retries until the file "can be read and parsed".This changes what an application sees when a file stays unreadable with auto-update on: a warning per retry and an error after five attempts, instead of one error, before the source waits for the next detected change.
File data source retry tests
Cause. The retry tests read through a
ScriptedFileReaderand count its reads, but they configured a realTempFilewithAutoUpdate(true), so a liveFileSystemWatcherwatched the file's directory. On macOS the watcher has delivered a change notification for the test's own newly created file, 1 to 155 ms afterStart(), although the test never changes the file. Every failing run logsdetected file modification, reloadingat that point, and the extra load explains each failure:ParseRetryStopsAfterMaxAttemptsAndDoesNotInit("Expected: 5, Actual: 6") andPendingParseRetryIsSkippedIfAnExternalReloadAlreadySucceeded("Expected: 2, Actual: 3"): the notification's load is one more read.TransientReadErrorDuringRetryDoesNotEndTheEpisode("expected an event but did not get one"): the notification's load ran while the scripted reader threw, and hit the SDK defect above.On Linux, a scratch copy of each test that makes one in-place write to its temp file at that point fails the same way.
Fix.
ParseRetryBudgetResetsForANewFailureEpisodeneeds a real change notification to start a new episode, so it keeps a watched file and changes it with an in-place write. It no longer asserts the read count after the first episode: it waits until the reads stop. A rewrite in place can produce more than one notification, and a later one could load the data that the test makes valid. So the test waits until no notification has arrived for 300 ms before it makes the content valid, and only the new episode's retry can then apply it.Startis retried; a read error on a load from outside the chain does not end a pending retry; read errors stop after five attempts; with auto-update off they are not retried; and a load from outside the chain gives a failing episode a fresh budget, which no test covered before (the existing test starts its second episode only after the first gave up, which clears the counts anyway).TransientReadErrorDuringRetryDoesNotEndTheEpisodenow checks that the retry applies the whole file.FDv2 fallback and recovery test
Cause. The applier runs its fallback and recovery actions (
DisposeCurrent, thenGoToNextorGoToFirst, thenStartCurrent) on its own task after a 20 ms timeout. The test asserted after a fixed delay of 70 ms, and its mock recorded calls in plain fields that another thread wrote. The failing runs logged the recovery message and sawDisposeCurrentbut notGoToFirst("GoToFirst should be called on recovery"), so the assertion ran while the actions were still running.Fix. The mock records its calls under a lock and reports them from the recorded list. The test waits until
StartCurrent, the last action, has been recorded, then checks the whole call sequence. The negative check (no recovery when already at the first source) keeps its fixed wait.Testing
mainReadErrorOnALoadThatIsNotARetryIsRetried,ReadErrorOnALoadThatIsNotARetryDoesNotEndThePendingRetries,ReadErrorRetryStopsAfterMaxAttemptsAndDoesNotInitLoadThatIsNotARetryGivesAFailingEpisodeAFreshBudgetReadErrorRetryStopsAfterMaxAttemptsAndDoesNotInitTransientReadErrorDuringRetryDoesNotEndTheEpisodeReadErrorIsNotRetriedIfAutoUpdateIsOffParseFailureFromPartialReadIsRetriedUntilContentIsComplete,ParseRetryAppliesWhenAlternateParserIsConfigured,ParseRetryStopsAfterMaxAttemptsAndDoesNotInit,TransientReadErrorDuringRetryDoesNotEndTheEpisode,ParseRetryBudgetResetsForANewFailureEpisode, and two multi-path testsParseRetryStopsAfterMaxAttemptsAndDoesNotInitand a multi-path testTransientReadErrorDuringRetryDoesNotEndTheEpisodeDisposePendingParseRetryIsCanceledByDisposePendingParseRetryIsSkippedIfAnExternalReloadAlreadySucceededParseRetryBudgetResetsForANewFailureEpisodeand a multi-path testFallbackAndRecoveryTasksWellBehavedDisposeCurrentandGoToFirstin the applier, the old FDv2 test fails every time with the CI message, and the new one passes. CPU load alone does not tell them apart: both passed hundreds of runs with the machine's cores oversubscribed.FileDataSourceTestandFDv2DataSourceTest(53 tests) passed 10 runs in a row on a loaded machine. The full server SDK test suite passes on net8.0 (1658 tests), and the SDK builds for netstandard2.0, net462 and net8.0.Opened from termag
SDK-3248
Note
Overview
File data source: With
AutoUpdate(true), read failures on any load (Start, file-change reload, or retry) now use the same bounded retry path as parse failures (HandleReadFailure), instead of logging once and bailing on non-retry loads—which could leave data stale when no further watcher event arrives. With auto-update off, behavior stays single-load + error log. Public docs now say retries continue until the file can be read and parsed.Tests: Retry tests that count scripted reads use an unwatched path so macOS
FileSystemWatchernoise does not add extra loads; episode-reset tests wait for reads/notifications to settle. New coverage pins read-error retry, budget reset on external reload, and max-attempt give-up. FDv2 fallback/recovery test waits for the applier’s async actions via a locked call-sequence mock instead of a fixed delay.Reviewed by Cursor Bugbot for commit 7e4e268. Bugbot is set up for automated code reviews on this repo. Configure here.