fix: Retry after partial file reads. - #218
Conversation
f3aaa08 to
d1f90f1
Compare
The two reload tests asserted an exact ExpectedDataSetForSegmentOnlyFile(2), which encoded an assumption of exactly two LoadAll attempts. With the file watcher firing on truncate-then-write plus the new JSON-parse retry, the successful attempt's version can be 3+ and the strict JSON comparison times out waiting for a version-2 event that never comes. Switch both tests to ExpectPredicate with a shared structural matcher and refactor the existing predicate in ModifiedFileIsReloadedIfAutoUpdateIsOn to use the same helper. Also tighten the retry path in FileDataSource: drop the trailing TODO, short-circuit LoadAll once Dispose() has been called, and skip the post- delay LoadAll if disposal raced the retry.
… autoUpdate. Review round-1 fixes: - Reset the retry budget when a failure is observed on an externally triggered load (Start or file-change notification), so a later partial-write episode still gets retries after an earlier episode exhausted the counter. - Treat any parser failure as retryable, not only JsonException, so alternate parsers (e.g. YAML) get the same partial-read handling. - Keep at most one pending retry, and skip it if an intervening load already succeeded, to avoid redundant Inits and spurious change events. - Do not retry when autoUpdate is off, preserving the documented load-once semantics. - Log the parse exception summary on retry and full detail on give-up; catch exceptions escaping the retry task so they are not unobserved. - Add deterministic IFileReader-driven tests for retry recovery, the attempt cap, episode reset, YAML parsing, autoUpdate(false), and dispose cancellation; episode reset, YAML, and autoUpdate(false) tests fail on the previous implementation.
… external loads. Review round-2 fixes: - Externally triggered loads clear the per-path failure counts up front, and give-up clears them as well, so a stale count from a dead retry chain can never be charged to a later episode (which previously caused an immediate give-up with zero retries on multi-path configs). - When one path's give-up ends the chain, other paths that were promised retries get a terminal error log naming them. - Remove a path's failure count as soon as it parses, before the merge step, so a duplicate-key configuration error cannot strand it. - Log the exception stack trace at debug level on each retry. - Correct the AutoUpdate doc: retries are bounded per detected change, not unconditional. - Tests: deterministic multi-path episode tests (fresh budget after another path's give-up; terminal log for abandoned paths) that fail on the previous implementation; a redundant-reload guard test; waits now fail loudly on timeout; the dispose test no longer depends on running within the retry delay.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
…sing. Addresses Bugbot review: a transient read error (for example, a writer replacing the file) during a retry previously ended the episode with budget remaining and no terminal log, recreating the stuck-stale-data state this PR targets. Such failures are now charged to the same per-path budget: the chain schedules the next retry until the budget is exhausted, then ends the episode with terminal logs. Externally triggered loads keep the existing non-retrying behavior for non-parse errors. New test fails on the previous implementation.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 64cb514. Configure here.
tanderson-ld
left a comment
tanderson-ld
left a comment
There was a problem hiding this comment.
I'm still reviewing for lock safety and other scheduling races / order of operations type issues, just leaving this so others know it is getting review.
| private volatile int _lastVersion; | ||
| private object _updateLock = new object(); | ||
|
|
||
| private const int MaxParseAttempts = 5; |
There was a problem hiding this comment.
Does this take the probability of the parsing incomplete data failure to 0 or just reduce it a bunch and nearly to 0?
There was a problem hiding this comment.
There isn't really any way to make it 0. But for practical purposes it will effectively make it 0. It is an arbitrary number.
If you are writing a file, then probably the first retry would always fix it. But if you just continually stream JSON into a file, then there isn't any point at which it would be safely parse-able.
| // path belongs to the current episode. | ||
| private void HandleParseFailure(string path, Exception e) | ||
| { | ||
| if (!_autoUpdate) |
There was a problem hiding this comment.
I know it is probably more complex to handle, but should it be willing to retry even when auto update is off? I guess this is the distinction between "load once (w failure)" vs "load once (success)". Which is "loaded once" ?
In the customer's shoes, I'd rather it retry if there was a race condition with whatever was updating the file (maybe an external sync process) even if I had auto update off.
There was a problem hiding this comment.
I would rather not change that contract right now. Not that I know if anyone depends on it, but if someone had a problem, then I think we could add the option. Though there isn't really much of a reason to not just use autoUpdate in that case.
- Log retriable read failures during a retry at Warn (with exception summary and debug trace), reserving Error for give-up, matching the parse-failure path. - Rename _parseFailureCounts/MaxParseAttempts to _loadFailureCounts/MaxLoadAttempts since the budget now also counts read failures. - Simplify the failure counters: default to zero and increment unconditionally. - Fold RetryLoadAll into LoadAll behind the existing isRetry parameter. - Document the constructor parameters.
2 participants
When we detect a file has been written there isn't any promise that the file content itself will actually be complete, or that a subsequent notification for that file will be received when it is.
Currently sometimes the file update will trigger, and the JSON parsing will fail, and then it won't get re-triggered. Which causes some tests to fail.
This introduces a retry for any failed JSON parsing up to a maximum number of times.
Ideally we would extract the file watcher and abstract the file system API so we could simulate these conditions.
Note
Overview
When auto-update is enabled,
FileDataSourceno longer treats a failed parse (or transient read error during a retry) as final if the file may have been read mid-write. It retries up to 5 times with a 600 ms delay, then stops until the next file-change notification or another external load (Start/ watcher).Retry logic is per failure episode: each watcher-triggered or
Start-driven load clears per-path failure counts. A single scheduled retry chain re-reads all paths; redundant retries are skipped if a successful load already cleared failure state. Dispose blocks further loads from pending retries. With auto-update off, behavior stays one-shot (no background retries).Public docs on
FileDataSourceBuilder.AutoUpdateare updated to describe give-up-after-repeated-failures instead of indefinite retry.Tests add scripted file readers and cover retry success, max attempts, budget reset, alternate parsers, read errors during retry, dispose, multi-path abandonment, and looser reload assertions where watcher timing makes exact versions non-deterministic.
Reviewed by Cursor Bugbot for commit b9fcb63. Bugbot is set up for automated code reviews on this repo. Configure here.