From d1f90f1fb6aef9e62d55984554f056fb081216da Mon Sep 17 00:00:00 2001 From: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Date: Fri, 9 Jan 2026 14:39:26 -0800 Subject: [PATCH 01/51] fix: Retry after partial file reads. --- .../Internal/DataSources/FileDataSource.cs | 38 +++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/pkgs/sdk/server/src/Internal/DataSources/FileDataSource.cs b/pkgs/sdk/server/src/Internal/DataSources/FileDataSource.cs index 91cb989b..98ea34c8 100644 --- a/pkgs/sdk/server/src/Internal/DataSources/FileDataSource.cs +++ b/pkgs/sdk/server/src/Internal/DataSources/FileDataSource.cs @@ -28,6 +28,10 @@ internal sealed class FileDataSource : IDataSource private volatile int _lastVersion; private object _updateLock = new object(); + private const int MaxRetries = 5; + private readonly TimeSpan RetryDelay = TimeSpan.FromSeconds(0.6); + private readonly Dictionary _retryCounts = new Dictionary(); + public FileDataSource(IDataSourceUpdates dataSourceUpdates, FileDataTypes.IFileReader fileReader, List paths, bool autoUpdate, Func alternateParser, bool skipMissingPaths, FileDataTypes.DuplicateKeysHandling duplicateKeysHandling, @@ -102,17 +106,51 @@ private void LoadAll() _logger.Debug("file data: {0}", content); var data = _parser.Parse(content, version); _dataMerger.AddToData(data, flags, segments); + // Remove any retry count associated with this path. + _retryCounts.Remove(path); } catch (FileNotFoundException) when (_skipMissingPaths) { _logger.Debug("{0}: {1}", path, "File not found"); } + catch (System.Text.Json.JsonException) + { + // We may have received the notification of a file change while the file was being written. + // So we may read an empty or partially written file. So, when we encounter a JSON parsing issue + // we will retry after a short delay. + // We will retry up to MaxRetries times before giving up. + if (!_retryCounts.ContainsKey(path)) + { + _retryCounts[path] = 0; + } + _retryCounts[path]++; + + if (_retryCounts[path] < MaxRetries) + { + _logger.Warn("{0}: {1}", path, "Failed to parse file, retrying in " + RetryDelay.TotalMilliseconds + " milliseconds"); + Task.Run(async () => + { + await Task.Delay(RetryDelay); + LoadAll(); + }); + } + else + { + _logger.Error("{0}: {1}", path, "Failed to parse file after " + MaxRetries + " retries"); + } + + return; + } catch (Exception e) { LogHelpers.LogException(_logger, "Failed to load " + path, e); return; } } + + // If any files failed to load, from anything other than not existing, then that + // update would fail. This behavior is retained with the addition of the retry. But it should be + // examined. var allData = new FullDataSet( ImmutableDictionary.Create>() From 28590643131bb7d43a7ddad8509ccb67bf5146d7 Mon Sep 17 00:00:00 2001 From: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Date: Tue, 26 May 2026 15:52:16 -0700 Subject: [PATCH 02/51] test: stop pinning segment version in flaky FileDataSource tests 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. --- .../Internal/DataSources/FileDataSource.cs | 31 +++--- .../DataSources/FileDataSourceTest.cs | 102 +++++++----------- 2 files changed, 56 insertions(+), 77 deletions(-) diff --git a/pkgs/sdk/server/src/Internal/DataSources/FileDataSource.cs b/pkgs/sdk/server/src/Internal/DataSources/FileDataSource.cs index 98ea34c8..1990622b 100644 --- a/pkgs/sdk/server/src/Internal/DataSources/FileDataSource.cs +++ b/pkgs/sdk/server/src/Internal/DataSources/FileDataSource.cs @@ -25,11 +25,13 @@ internal sealed class FileDataSource : IDataSource private readonly Logger _logger; private volatile bool _started; private volatile bool _loadedValidData; + private volatile bool _disposed; private volatile int _lastVersion; private object _updateLock = new object(); private const int MaxRetries = 5; - private readonly TimeSpan RetryDelay = TimeSpan.FromSeconds(0.6); + private static readonly TimeSpan RetryDelay = TimeSpan.FromMilliseconds(600); + // Per-path JSON-parse retry counters. Only touched inside _updateLock. private readonly Dictionary _retryCounts = new Dictionary(); public FileDataSource(IDataSourceUpdates dataSourceUpdates, FileDataTypes.IFileReader fileReader, @@ -87,6 +89,7 @@ private void Dispose(bool disposing) { if (disposing) { + _disposed = true; _reloader?.Dispose(); } } @@ -95,6 +98,10 @@ private void LoadAll() { lock (_updateLock) { + if (_disposed) + { + return; + } var version = Interlocked.Increment(ref _lastVersion); var flags = new Dictionary(); var segments = new Dictionary(); @@ -106,7 +113,6 @@ private void LoadAll() _logger.Debug("file data: {0}", content); var data = _parser.Parse(content, version); _dataMerger.AddToData(data, flags, segments); - // Remove any retry count associated with this path. _retryCounts.Remove(path); } catch (FileNotFoundException) when (_skipMissingPaths) @@ -115,10 +121,9 @@ private void LoadAll() } catch (System.Text.Json.JsonException) { - // We may have received the notification of a file change while the file was being written. - // So we may read an empty or partially written file. So, when we encounter a JSON parsing issue - // we will retry after a short delay. - // We will retry up to MaxRetries times before giving up. + // A file-change notification can fire while the file is mid-write, so we may read an + // empty or partially written file. Retry up to MaxRetries times before giving up; the + // counter is cleared on the next successful load. if (!_retryCounts.ContainsKey(path)) { _retryCounts[path] = 0; @@ -127,16 +132,20 @@ private void LoadAll() if (_retryCounts[path] < MaxRetries) { - _logger.Warn("{0}: {1}", path, "Failed to parse file, retrying in " + RetryDelay.TotalMilliseconds + " milliseconds"); + _logger.Warn("{0}: Failed to parse file, retrying in {1} ms", path, RetryDelay.TotalMilliseconds); Task.Run(async () => { - await Task.Delay(RetryDelay); + await Task.Delay(RetryDelay).ConfigureAwait(false); + if (_disposed) + { + return; + } LoadAll(); }); } else { - _logger.Error("{0}: {1}", path, "Failed to parse file after " + MaxRetries + " retries"); + _logger.Error("{0}: Failed to parse file after {1} retries", path, MaxRetries); } return; @@ -147,10 +156,6 @@ private void LoadAll() return; } } - - // If any files failed to load, from anything other than not existing, then that - // update would fail. This behavior is retained with the addition of the retry. But it should be - // examined. var allData = new FullDataSet( ImmutableDictionary.Create>() diff --git a/pkgs/sdk/server/test/Internal/DataSources/FileDataSourceTest.cs b/pkgs/sdk/server/test/Internal/DataSources/FileDataSourceTest.cs index 5322a38c..d3f8a701 100644 --- a/pkgs/sdk/server/test/Internal/DataSources/FileDataSourceTest.cs +++ b/pkgs/sdk/server/test/Internal/DataSources/FileDataSourceTest.cs @@ -151,50 +151,7 @@ public void ModifiedFileIsReloadedIfAutoUpdateIsOn() file.SetContentFromPath(TestUtils.TestFilePath("segment-only.json")); - AssertHelpers.ExpectPredicate(_updateSink.Inits, actual => - { - var segments = actual.Data.First(item => item.Key == DataModel.Segments); - var features = actual.Data.First(item => item.Key == DataModel.Features); - if (!features.Value.Items.IsNullOrEmpty()) - { - return false; - } - - var segmentItems = segments.Value.Items.ToList(); - - if (segmentItems.Count != 1) - { - return false; - } - - var segmentDescriptor = segmentItems[0]; - if (segmentDescriptor.Key != "seg1") - { - return false; - } - - if (segmentDescriptor.Value.Version == 1) - { - return false; - } - - if (!(segmentDescriptor.Value.Item is Segment segment)) - { - return false; - } - - if (segment.Deleted) - { - return false; - } - - if (segment.Included.Count != 1) - { - return false; - } - - return segment.Included[0] == "user1"; - }, + AssertHelpers.ExpectPredicate(_updateSink.Inits, IsSegmentOnlyDataAfterReload, "Did not receive expected update from the file data source.", TimeSpan.FromSeconds(30)); } @@ -270,16 +227,9 @@ public void ModifiedFileIsReloadedEvenIfOneFileIsMissingIfSkipMissingPathsIsSet( file1.SetContentFromPath(TestUtils.TestFilePath("segment-only.json")); - // Use ExpectJsonValue to handle potential race conditions where the file watcher - // may trigger multiple reload events. This keeps checking events until one matches - // the expected JSON or the timeout expires. - var newData = AssertHelpers.ExpectJsonValue( - _updateSink.Inits, - DataSetAsJson(ExpectedDataSetForSegmentOnlyFile(2)), - DataSetAsJson, + AssertHelpers.ExpectPredicate(_updateSink.Inits, IsSegmentOnlyDataAfterReload, + "Did not receive expected update from the file data source.", TimeSpan.FromSeconds(30)); - - AssertJsonEqual(DataSetAsJson(ExpectedDataSetForSegmentOnlyFile(2)), DataSetAsJson(newData)); } } } @@ -298,18 +248,9 @@ public void IfFlagsAreBadAtStartTimeAutoUpdateCanStillLoadGoodDataLater() file.SetContentFromPath(TestUtils.TestFilePath("segment-only.json")); - // Use ExpectJsonValue to handle potential race conditions where the file watcher - // may trigger multiple reload events. This keeps checking events until one matches - // the expected JSON or the timeout expires. - // Note that the expected version is 2 because we increment the version on each - // *attempt* to load the files, not on each successful load. - var newData = AssertHelpers.ExpectJsonValue( - _updateSink.Inits, - DataSetAsJson(ExpectedDataSetForSegmentOnlyFile(2)), - DataSetAsJson, + AssertHelpers.ExpectPredicate(_updateSink.Inits, IsSegmentOnlyDataAfterReload, + "Did not receive expected update from the file data source.", TimeSpan.FromSeconds(30)); - - AssertJsonEqual(DataSetAsJson(ExpectedDataSetForSegmentOnlyFile(2)), DataSetAsJson(newData)); } } } @@ -365,5 +306,38 @@ private static FullDataSet ExpectedDataSetForSegmentOnlyFile(int new SegmentBuilder("seg1").Version(version).Included("user1").Build() ) .Build(); + + // Predicate that matches the structure of segment-only.json reloaded after the initial load. + // We deliberately don't pin the exact version: with the file watcher firing on truncate-then-write + // and the JsonException retry, the version of the successful load is non-deterministic, so we + // only require that it isn't the initial version 1. + private static bool IsSegmentOnlyDataAfterReload(FullDataSet actual) + { + var features = actual.Data.First(item => item.Key == DataModel.Features); + if (!features.Value.Items.IsNullOrEmpty()) + { + return false; + } + + var segments = actual.Data.First(item => item.Key == DataModel.Segments); + var segmentItems = segments.Value.Items.ToList(); + if (segmentItems.Count != 1) + { + return false; + } + + var segmentDescriptor = segmentItems[0]; + if (segmentDescriptor.Key != "seg1" || segmentDescriptor.Value.Version == 1) + { + return false; + } + + if (!(segmentDescriptor.Value.Item is Segment segment) || segment.Deleted) + { + return false; + } + + return segment.Included.Count == 1 && segment.Included[0] == "user1"; + } } } From f9eb45554d3bcd30ef9f54136031f4c087b99bca Mon Sep 17 00:00:00 2001 From: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Date: Tue, 26 May 2026 16:00:04 -0700 Subject: [PATCH 03/51] ci: trigger flake check run 1/19 From d1add2aa20cfe5fc9cb371de34343517ce7c7982 Mon Sep 17 00:00:00 2001 From: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Date: Tue, 26 May 2026 16:00:36 -0700 Subject: [PATCH 04/51] ci: trigger flake check run 2/19 From 9924c10b72934d56ea486646514f04a826b4930b Mon Sep 17 00:00:00 2001 From: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Date: Tue, 26 May 2026 16:00:38 -0700 Subject: [PATCH 05/51] ci: trigger flake check run 3/19 From a13e6da2acc631896c6c108e4f18c60afe7b4ab2 Mon Sep 17 00:00:00 2001 From: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Date: Tue, 26 May 2026 16:00:39 -0700 Subject: [PATCH 06/51] ci: trigger flake check run 4/19 From e35db5dee01ddec40f6087446a5a6efb3f03f82d Mon Sep 17 00:00:00 2001 From: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Date: Tue, 26 May 2026 16:00:40 -0700 Subject: [PATCH 07/51] ci: trigger flake check run 5/19 From ce9dd1880a645831068087202ce669f8d7d01274 Mon Sep 17 00:00:00 2001 From: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Date: Tue, 26 May 2026 16:00:42 -0700 Subject: [PATCH 08/51] ci: trigger flake check run 6/19 From 8151abffc1fa6d96db5048c6b72a7b1c9477a66e Mon Sep 17 00:00:00 2001 From: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Date: Tue, 26 May 2026 16:00:43 -0700 Subject: [PATCH 09/51] ci: trigger flake check run 7/19 From c04a01d4712910d5bcc25c6daf3f6146ea58e59b Mon Sep 17 00:00:00 2001 From: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Date: Tue, 26 May 2026 16:00:45 -0700 Subject: [PATCH 10/51] ci: trigger flake check run 8/19 From 1baf7ecbdd824769b83f854434824f896d98df81 Mon Sep 17 00:00:00 2001 From: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Date: Tue, 26 May 2026 16:00:46 -0700 Subject: [PATCH 11/51] ci: trigger flake check run 9/19 From a458d0080f719291f0faf1c6a8e8aa15073b97e3 Mon Sep 17 00:00:00 2001 From: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Date: Tue, 26 May 2026 16:00:48 -0700 Subject: [PATCH 12/51] ci: trigger flake check run 10/19 From 81cf9ace93df3046ace7acf6023e3da8ac4e8797 Mon Sep 17 00:00:00 2001 From: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Date: Tue, 26 May 2026 16:00:49 -0700 Subject: [PATCH 13/51] ci: trigger flake check run 11/19 From d330eee2999d110a02910b93abcb5045aa2ca443 Mon Sep 17 00:00:00 2001 From: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Date: Tue, 26 May 2026 16:00:51 -0700 Subject: [PATCH 14/51] ci: trigger flake check run 12/19 From 98c80ececab2f7d0f0d7b9573970434d1cfc1ed8 Mon Sep 17 00:00:00 2001 From: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Date: Tue, 26 May 2026 16:00:52 -0700 Subject: [PATCH 15/51] ci: trigger flake check run 13/19 From 907e9bc3f4bfc79e0c06b976df19d8d231af1126 Mon Sep 17 00:00:00 2001 From: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Date: Tue, 26 May 2026 16:00:53 -0700 Subject: [PATCH 16/51] ci: trigger flake check run 14/19 From 50215b573c24cbcd1e2c39889d4f58a11ac33c6e Mon Sep 17 00:00:00 2001 From: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Date: Tue, 26 May 2026 16:00:55 -0700 Subject: [PATCH 17/51] ci: trigger flake check run 15/19 From 75ab3bf34c1b23b6257c4047744293e477565ee6 Mon Sep 17 00:00:00 2001 From: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Date: Tue, 26 May 2026 16:00:56 -0700 Subject: [PATCH 18/51] ci: trigger flake check run 16/19 From 9ca9b0ebe697bca43b1438d46cb66ec459f2d054 Mon Sep 17 00:00:00 2001 From: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Date: Tue, 26 May 2026 16:00:58 -0700 Subject: [PATCH 19/51] ci: trigger flake check run 17/19 From 9d4913b965773506fbf9f6fc2eb082716b886179 Mon Sep 17 00:00:00 2001 From: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Date: Tue, 26 May 2026 16:00:59 -0700 Subject: [PATCH 20/51] ci: trigger flake check run 18/19 From 0066cd9d98abb6af3980fb216cfd8ed56e304f8f Mon Sep 17 00:00:00 2001 From: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Date: Tue, 26 May 2026 16:01:01 -0700 Subject: [PATCH 21/51] ci: trigger flake check run 19/19 From 99a9cd0eba6b125b18c588535d4fd59c5d58a13c Mon Sep 17 00:00:00 2001 From: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Date: Tue, 26 May 2026 16:02:15 -0700 Subject: [PATCH 22/51] ci: trigger flake check run 20 From b0b31e17ce7cd730c1db8527add0777b90bcb9e1 Mon Sep 17 00:00:00 2001 From: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Date: Tue, 26 May 2026 16:02:31 -0700 Subject: [PATCH 23/51] ci: trigger flake check run 21 From 764e7d3db360c4ef43858ec4567f74e076a5904c Mon Sep 17 00:00:00 2001 From: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Date: Tue, 26 May 2026 16:02:47 -0700 Subject: [PATCH 24/51] ci: trigger flake check run 22 From dc58101f51755fbc51187c146d2c641b0af40e8a Mon Sep 17 00:00:00 2001 From: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Date: Tue, 26 May 2026 16:03:04 -0700 Subject: [PATCH 25/51] ci: trigger flake check run 23 From 22c836c2b853393c42d37b759d74ca3bb7d7ccbd Mon Sep 17 00:00:00 2001 From: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Date: Tue, 26 May 2026 16:03:20 -0700 Subject: [PATCH 26/51] ci: trigger flake check run 24 From 72cc40ab7c0c6c8644299a69600f8750a723d062 Mon Sep 17 00:00:00 2001 From: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Date: Tue, 26 May 2026 16:03:37 -0700 Subject: [PATCH 27/51] ci: trigger flake check run 25 From f90b545fb81b2fd1dba148f36ba6c1181aa356e9 Mon Sep 17 00:00:00 2001 From: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Date: Tue, 26 May 2026 16:03:53 -0700 Subject: [PATCH 28/51] ci: trigger flake check run 26 From ee1bf7ef5ed3eb559d044e2eea1b343b43965ad1 Mon Sep 17 00:00:00 2001 From: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Date: Tue, 26 May 2026 16:04:10 -0700 Subject: [PATCH 29/51] ci: trigger flake check run 27 From 824e929aaea5e22ca345be596d622f1ae2da458d Mon Sep 17 00:00:00 2001 From: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Date: Tue, 26 May 2026 16:04:57 -0700 Subject: [PATCH 30/51] ci: trigger flake check run 28 From ba42d22021ff84100249eea02361f4299ede8950 Mon Sep 17 00:00:00 2001 From: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Date: Tue, 26 May 2026 16:05:14 -0700 Subject: [PATCH 31/51] ci: trigger flake check run 29 From fc329559c85adcf1f3ad3e8874919e0890c0e9d7 Mon Sep 17 00:00:00 2001 From: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Date: Tue, 26 May 2026 16:05:30 -0700 Subject: [PATCH 32/51] ci: trigger flake check run 30 From 5bc5f9b63325957ce568a4f3a10d7deab343c55d Mon Sep 17 00:00:00 2001 From: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Date: Tue, 26 May 2026 16:05:47 -0700 Subject: [PATCH 33/51] ci: trigger flake check run 31 From 956bd47794c1e099cf24fb634b52367f80295293 Mon Sep 17 00:00:00 2001 From: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Date: Tue, 26 May 2026 16:06:04 -0700 Subject: [PATCH 34/51] ci: trigger flake check run 32 From 97bacd247444edcd3d165dbdb23255acd33247b7 Mon Sep 17 00:00:00 2001 From: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Date: Tue, 26 May 2026 16:06:20 -0700 Subject: [PATCH 35/51] ci: trigger flake check run 33 From 2de17efcc6c2bfb819922b9fcf6fc4c158c551cf Mon Sep 17 00:00:00 2001 From: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Date: Tue, 26 May 2026 16:06:37 -0700 Subject: [PATCH 36/51] ci: trigger flake check run 34 From 0a142262e5d5dc8eb971bf5e9476707fa64886cb Mon Sep 17 00:00:00 2001 From: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Date: Tue, 26 May 2026 16:06:53 -0700 Subject: [PATCH 37/51] ci: trigger flake check run 35 From c709ee4eb86ee57f5a00002a4a66c509ca17a2bf Mon Sep 17 00:00:00 2001 From: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Date: Tue, 26 May 2026 16:07:10 -0700 Subject: [PATCH 38/51] ci: trigger flake check run 36 From 5a3af9e96b6fa3d23539d84fa689355ecd75c44a Mon Sep 17 00:00:00 2001 From: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Date: Mon, 17 Aug 2026 15:48:15 -0700 Subject: [PATCH 39/51] fix: Make parse retry episode-aware, cover alternate parsers, gate on 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. --- .../Internal/DataSources/FileDataSource.cs | 138 ++++++++++++----- .../DataSources/FileDataSourceTest.cs | 144 ++++++++++++++++++ 2 files changed, 243 insertions(+), 39 deletions(-) diff --git a/pkgs/sdk/server/src/Internal/DataSources/FileDataSource.cs b/pkgs/sdk/server/src/Internal/DataSources/FileDataSource.cs index 1990622b..a30726a5 100644 --- a/pkgs/sdk/server/src/Internal/DataSources/FileDataSource.cs +++ b/pkgs/sdk/server/src/Internal/DataSources/FileDataSource.cs @@ -22,6 +22,7 @@ internal sealed class FileDataSource : IDataSource private readonly FlagFileDataMerger _dataMerger; private readonly FileDataTypes.IFileReader _fileReader; private readonly bool _skipMissingPaths; + private readonly bool _autoUpdate; private readonly Logger _logger; private volatile bool _started; private volatile bool _loadedValidData; @@ -29,10 +30,15 @@ internal sealed class FileDataSource : IDataSource private volatile int _lastVersion; private object _updateLock = new object(); - private const int MaxRetries = 5; + private const int MaxParseAttempts = 5; private static readonly TimeSpan RetryDelay = TimeSpan.FromMilliseconds(600); - // Per-path JSON-parse retry counters. Only touched inside _updateLock. - private readonly Dictionary _retryCounts = new Dictionary(); + // Consecutive parse failures per path within the current failure episode. A failure seen on + // an externally triggered load (Start or a file-change notification) starts a new episode, + // so the retry budget is per-episode, not per-lifetime. Only touched inside _updateLock. + private readonly Dictionary _parseFailureCounts = new Dictionary(); + // Whether a delayed retry is already scheduled; at most one retry chain exists at a time, + // since each retry re-reads every path anyway. Only touched inside _updateLock. + private bool _retryPending; public FileDataSource(IDataSourceUpdates dataSourceUpdates, FileDataTypes.IFileReader fileReader, List paths, bool autoUpdate, Func alternateParser, bool skipMissingPaths, @@ -46,6 +52,7 @@ public FileDataSource(IDataSourceUpdates dataSourceUpdates, FileDataTypes.IFileR _dataMerger = new FlagFileDataMerger(duplicateKeysHandling); _fileReader = fileReader; _skipMissingPaths = skipMissingPaths; + _autoUpdate = autoUpdate; _lastVersion = 0; if (autoUpdate) { @@ -68,7 +75,7 @@ public FileDataSource(IDataSourceUpdates dataSourceUpdates, FileDataTypes.IFileR public Task Start() { _started = true; - LoadAll(); + LoadAll(isRetry: false); // We always complete the start task regardless of whether we successfully loaded data or not; // if the data files were bad, they're unlikely to become good within the short interval that @@ -94,7 +101,7 @@ private void Dispose(bool disposing) } } - private void LoadAll() + private void LoadAll(bool isRetry) { lock (_updateLock) { @@ -111,45 +118,27 @@ private void LoadAll() { var content = _fileReader.ReadAllText(path); _logger.Debug("file data: {0}", content); - var data = _parser.Parse(content, version); + FullDataSet data; + try + { + data = _parser.Parse(content, version); + } + catch (Exception e) + { + // A file-change notification can fire while the file is mid-write, so a parse + // failure may just mean we read an empty or partially written file. This applies + // to any configured parser (JSON or alternate), so we treat every failure of + // Parse — as opposed to reading the file — as potentially transient. + HandleParseFailure(path, e, isRetry); + return; + } _dataMerger.AddToData(data, flags, segments); - _retryCounts.Remove(path); + _parseFailureCounts.Remove(path); } catch (FileNotFoundException) when (_skipMissingPaths) { _logger.Debug("{0}: {1}", path, "File not found"); } - catch (System.Text.Json.JsonException) - { - // A file-change notification can fire while the file is mid-write, so we may read an - // empty or partially written file. Retry up to MaxRetries times before giving up; the - // counter is cleared on the next successful load. - if (!_retryCounts.ContainsKey(path)) - { - _retryCounts[path] = 0; - } - _retryCounts[path]++; - - if (_retryCounts[path] < MaxRetries) - { - _logger.Warn("{0}: Failed to parse file, retrying in {1} ms", path, RetryDelay.TotalMilliseconds); - Task.Run(async () => - { - await Task.Delay(RetryDelay).ConfigureAwait(false); - if (_disposed) - { - return; - } - LoadAll(); - }); - } - else - { - _logger.Error("{0}: Failed to parse file after {1} retries", path, MaxRetries); - } - - return; - } catch (Exception e) { LogHelpers.LogException(_logger, "Failed to load " + path, e); @@ -167,12 +156,83 @@ private void LoadAll() } } + // Called under _updateLock when parsing a path's content fails. + private void HandleParseFailure(string path, Exception e, bool isRetry) + { + if (!_autoUpdate) + { + // With auto-update off, files are documented to be loaded only once, so we don't + // retry in the background — Start()'s result stays final. + LogHelpers.LogException(_logger, "Failed to parse " + path, e); + return; + } + + var attempts = 1; + if (isRetry && _parseFailureCounts.TryGetValue(path, out var previousAttempts)) + { + attempts = previousAttempts + 1; + } + + if (attempts < MaxParseAttempts) + { + _parseFailureCounts[path] = attempts; + _logger.Warn("{0}: failed to parse file ({1}); will retry in {2} ms in case it was incompletely written", + path, LogValues.ExceptionSummary(e), RetryDelay.TotalMilliseconds); + ScheduleRetry(); + } + else + { + _parseFailureCounts.Remove(path); + LogHelpers.LogException(_logger, + string.Format("{0}: failed to parse file after {1} attempts", path, MaxParseAttempts), e); + } + } + + // Called under _updateLock. + private void ScheduleRetry() + { + if (_retryPending) + { + return; // the already-scheduled retry will re-read every path + } + _retryPending = true; + Task.Run(async () => + { + await Task.Delay(RetryDelay).ConfigureAwait(false); + try + { + RetryLoadAll(); + } + catch (Exception e) + { + // Nothing observes this task, so any escaping exception would otherwise vanish. + LogHelpers.LogException(_logger, "Unexpected error while retrying file data load", e); + } + }); + } + + private void RetryLoadAll() + { + lock (_updateLock) + { + _retryPending = false; + if (_disposed || _parseFailureCounts.Count == 0) + { + // Disposed, or an externally triggered load already succeeded in the meantime + // (success clears the failure counts) — a reload would be redundant and would + // re-Init identical data at bumped versions, firing spurious change events. + return; + } + LoadAll(isRetry: true); + } + } + private void TriggerReload() { if (_started) { _logger.Info("detected file modification, reloading"); - LoadAll(); + LoadAll(isRetry: false); } } } diff --git a/pkgs/sdk/server/test/Internal/DataSources/FileDataSourceTest.cs b/pkgs/sdk/server/test/Internal/DataSources/FileDataSourceTest.cs index d3f8a701..72adc3cb 100644 --- a/pkgs/sdk/server/test/Internal/DataSources/FileDataSourceTest.cs +++ b/pkgs/sdk/server/test/Internal/DataSources/FileDataSourceTest.cs @@ -255,6 +255,150 @@ public void IfFlagsAreBadAtStartTimeAutoUpdateCanStillLoadGoodDataLater() } } + private const string ValidFlagJson = @"{""flagValues"":{""flag1"":""a""}}"; + private const string TruncatedFlagJson = @"{""flagValues"": {"; // invalid as JSON and as YAML + + // Simulates reading a file that is mid-write: returns truncated content until Bad is + // cleared, and counts reads so tests can observe retry attempts deterministically + // without depending on real file-watcher timing. + private class ScriptedFileReader : FileDataTypes.IFileReader + { + private int _reads; + public volatile bool Bad = true; + public int Reads => Volatile.Read(ref _reads); + + public string ReadAllText(string path) + { + Interlocked.Increment(ref _reads); + return Bad ? TruncatedFlagJson : ValidFlagJson; + } + } + + private static void WaitForReads(ScriptedFileReader reader, int count) + { + var deadline = DateTime.UtcNow.AddSeconds(10); + while (reader.Reads < count && DateTime.UtcNow < deadline) + { + Thread.Sleep(50); + } + } + + [Fact] + public void ParseFailureFromPartialReadIsRetriedUntilContentIsComplete() + { + var reader = new ScriptedFileReader(); + using (var file = TempFile.Create()) + { + factory.FilePaths(file.Path).AutoUpdate(true).FileReader(reader); + using (var fp = MakeDataSource()) + { + fp.Start(); + WaitForReads(reader, 2); + reader.Bad = false; // as if the write completed, with no further notification + _updateSink.Inits.ExpectValue(TimeSpan.FromSeconds(5)); + Assert.True(fp.Initialized); + } + } + } + + [Fact] + public void ParseRetryStopsAfterMaxAttemptsAndDoesNotInit() + { + var reader = new ScriptedFileReader(); + using (var file = TempFile.Create()) + { + factory.FilePaths(file.Path).AutoUpdate(true).FileReader(reader); + using (var fp = MakeDataSource()) + { + fp.Start(); + WaitForReads(reader, 5); // initial attempt + 4 retries + Thread.Sleep(1500); // longer than two retry delays + Assert.Equal(5, reader.Reads); // budget exhausted, no further attempts + _updateSink.Inits.ExpectNoValue(); + Assert.False(fp.Initialized); + } + } + } + + [Fact] + public void ParseRetryBudgetResetsForANewFailureEpisode() + { + var reader = new ScriptedFileReader(); + using (var file = TempFile.Create()) + { + factory.FilePaths(file.Path).AutoUpdate(true).FileReader(reader); + using (var fp = MakeDataSource()) + { + fp.Start(); + WaitForReads(reader, 5); // episode 1: all attempts fail + Thread.Sleep(1500); + Assert.Equal(5, reader.Reads); // episode 1 exhausted, nothing pending + + // A new file-change notification starts a new episode with a fresh retry + // budget, even though its first read still sees partial content. + file.SetContent("trigger-new-episode"); + WaitForReads(reader, 6); + + reader.Bad = false; // write completed; no further notification arrives + _updateSink.Inits.ExpectValue(TimeSpan.FromSeconds(5)); + } + } + } + + [Fact] + public void ParseRetryAppliesWhenAlternateParserIsConfigured() + { + var yaml = new DeserializerBuilder().Build(); + var reader = new ScriptedFileReader(); + using (var file = TempFile.Create()) + { + factory.FilePaths(file.Path).AutoUpdate(true).FileReader(reader) + .Parser(s => yaml.Deserialize(s)); + using (var fp = MakeDataSource()) + { + fp.Start(); + reader.Bad = false; + _updateSink.Inits.ExpectValue(TimeSpan.FromSeconds(5)); + } + } + } + + [Fact] + public void ParseFailureIsNotRetriedIfAutoUpdateIsOff() + { + var reader = new ScriptedFileReader(); + using (var file = TempFile.Create()) + { + factory.FilePaths(file.Path).AutoUpdate(false).FileReader(reader); + using (var fp = MakeDataSource()) + { + var task = fp.Start(); + Assert.True(task.IsCompleted); + Assert.False(fp.Initialized); + reader.Bad = false; + _updateSink.Inits.ExpectNoValue(TimeSpan.FromSeconds(2)); + Assert.False(fp.Initialized); + Assert.Equal(1, reader.Reads); + } + } + } + + [Fact] + public void PendingParseRetryIsCanceledByDispose() + { + var reader = new ScriptedFileReader(); + using (var file = TempFile.Create()) + { + factory.FilePaths(file.Path).AutoUpdate(true).FileReader(reader); + var fp = MakeDataSource(); + fp.Start(); // schedules a retry + Assert.Equal(1, reader.Reads); + fp.Dispose(); + Thread.Sleep(1500); + Assert.Equal(1, reader.Reads); // the pending retry observed the disposal and did nothing + } + } + [Fact] public void FullFlagDefinitionEvaluatesAsExpected() { From 12ec3a2c029e83d07d9ab0dc7bcb861d06305609 Mon Sep 17 00:00:00 2001 From: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Date: Mon, 17 Aug 2026 16:22:23 -0700 Subject: [PATCH 40/51] fix: End the whole failure episode on give-up; clear episode state on 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. --- .../src/Integrations/FileDataSourceBuilder.cs | 3 +- .../Internal/DataSources/FileDataSource.cs | 50 +++++-- .../DataSources/FileDataSourceTest.cs | 140 ++++++++++++++++-- 3 files changed, 166 insertions(+), 27 deletions(-) diff --git a/pkgs/sdk/server/src/Integrations/FileDataSourceBuilder.cs b/pkgs/sdk/server/src/Integrations/FileDataSourceBuilder.cs index 893ecd2f..2b12d83d 100644 --- a/pkgs/sdk/server/src/Integrations/FileDataSourceBuilder.cs +++ b/pkgs/sdk/server/src/Integrations/FileDataSourceBuilder.cs @@ -111,7 +111,8 @@ public FileDataSourceBuilder FileReader(FileDataTypes.IFileReader fileReader) /// Whenever possible, you should update a file's entire contents in one atomic operation; in Unix-like OSes, /// that can be done by creating a temporary file, writing to it, and then renaming it to replace the original /// file. In Windows, that is not always possible, so FileDataSource might detect an update before the file has - /// been fully written; in that case it will retry until it succeeds. + /// been fully written; in that case it will retry several times until the file can be parsed, giving up after + /// repeated failures until another change to the file is detected. /// /// /// Note that auto-updating may not work if any of the files you specified has an invalid directory path. diff --git a/pkgs/sdk/server/src/Internal/DataSources/FileDataSource.cs b/pkgs/sdk/server/src/Internal/DataSources/FileDataSource.cs index a30726a5..0589b7aa 100644 --- a/pkgs/sdk/server/src/Internal/DataSources/FileDataSource.cs +++ b/pkgs/sdk/server/src/Internal/DataSources/FileDataSource.cs @@ -32,8 +32,8 @@ internal sealed class FileDataSource : IDataSource private const int MaxParseAttempts = 5; private static readonly TimeSpan RetryDelay = TimeSpan.FromMilliseconds(600); - // Consecutive parse failures per path within the current failure episode. A failure seen on - // an externally triggered load (Start or a file-change notification) starts a new episode, + // Consecutive parse failures per path within the current failure episode. An externally + // triggered load (Start or a file-change notification) starts a new episode and clears this, // so the retry budget is per-episode, not per-lifetime. Only touched inside _updateLock. private readonly Dictionary _parseFailureCounts = new Dictionary(); // Whether a delayed retry is already scheduled; at most one retry chain exists at a time, @@ -109,6 +109,13 @@ private void LoadAll(bool isRetry) { return; } + if (!isRetry) + { + // An externally triggered load starts a new failure episode: parse failures + // observed from here on get a fresh retry budget, and any state left over + // from a previous episode is discarded. + _parseFailureCounts.Clear(); + } var version = Interlocked.Increment(ref _lastVersion); var flags = new Dictionary(); var segments = new Dictionary(); @@ -129,11 +136,11 @@ private void LoadAll(bool isRetry) // failure may just mean we read an empty or partially written file. This applies // to any configured parser (JSON or alternate), so we treat every failure of // Parse — as opposed to reading the file — as potentially transient. - HandleParseFailure(path, e, isRetry); + HandleParseFailure(path, e); return; } - _dataMerger.AddToData(data, flags, segments); _parseFailureCounts.Remove(path); + _dataMerger.AddToData(data, flags, segments); } catch (FileNotFoundException) when (_skipMissingPaths) { @@ -156,8 +163,10 @@ private void LoadAll(bool isRetry) } } - // Called under _updateLock when parsing a path's content fails. - private void HandleParseFailure(string path, Exception e, bool isRetry) + // Called under _updateLock when parsing a path's content fails. Since an externally + // triggered load clears _parseFailureCounts before reading, any existing count for the + // path belongs to the current episode. + private void HandleParseFailure(string path, Exception e) { if (!_autoUpdate) { @@ -167,24 +176,32 @@ private void HandleParseFailure(string path, Exception e, bool isRetry) return; } - var attempts = 1; - if (isRetry && _parseFailureCounts.TryGetValue(path, out var previousAttempts)) - { - attempts = previousAttempts + 1; - } + var attempts = _parseFailureCounts.TryGetValue(path, out var previousAttempts) + ? previousAttempts + 1 + : 1; if (attempts < MaxParseAttempts) { _parseFailureCounts[path] = attempts; - _logger.Warn("{0}: failed to parse file ({1}); will retry in {2} ms in case it was incompletely written", + _logger.Warn("{0}: Failed to parse file ({1}); will retry in {2} ms in case it was incompletely written", path, LogValues.ExceptionSummary(e), RetryDelay.TotalMilliseconds); + _logger.Debug("{0}", LogValues.ExceptionTrace(e)); ScheduleRetry(); } else { + // This path kept failing, so the retry chain ends until the next external trigger. + // Every attempt stopped at this path, so any other paths with recorded failures + // were never re-attempted; their episodes end here too. _parseFailureCounts.Remove(path); + foreach (var abandoned in _parseFailureCounts.Keys) + { + _logger.Error("{0}: Will not be retried because {1} repeatedly failed to parse; both will be re-read on the next detected file change", + abandoned, path); + } + _parseFailureCounts.Clear(); LogHelpers.LogException(_logger, - string.Format("{0}: failed to parse file after {1} attempts", path, MaxParseAttempts), e); + string.Format("{0}: Failed to parse file after {1} attempts", path, MaxParseAttempts), e); } } @@ -218,9 +235,10 @@ private void RetryLoadAll() _retryPending = false; if (_disposed || _parseFailureCounts.Count == 0) { - // Disposed, or an externally triggered load already succeeded in the meantime - // (success clears the failure counts) — a reload would be redundant and would - // re-Init identical data at bumped versions, firing spurious change events. + // Disposed, or the failure state was cleared in the meantime (an externally + // triggered load succeeded, or the chain gave up) — a reload would be redundant + // and would re-Init identical data at bumped versions, firing spurious change + // events. return; } LoadAll(isRetry: true); diff --git a/pkgs/sdk/server/test/Internal/DataSources/FileDataSourceTest.cs b/pkgs/sdk/server/test/Internal/DataSources/FileDataSourceTest.cs index 72adc3cb..f2cb1813 100644 --- a/pkgs/sdk/server/test/Internal/DataSources/FileDataSourceTest.cs +++ b/pkgs/sdk/server/test/Internal/DataSources/FileDataSourceTest.cs @@ -1,7 +1,9 @@ using System; +using System.Collections.Concurrent; using System.Linq; using System.Threading; using Castle.Core.Internal; +using LaunchDarkly.Logging; using LaunchDarkly.Sdk.Server.Integrations; using LaunchDarkly.Sdk.Server.Interfaces; using LaunchDarkly.Sdk.Server.Internal.Model; @@ -274,15 +276,19 @@ public string ReadAllText(string path) } } - private static void WaitForReads(ScriptedFileReader reader, int count) + private static void WaitUntil(Func condition, string description) { - var deadline = DateTime.UtcNow.AddSeconds(10); - while (reader.Reads < count && DateTime.UtcNow < deadline) + var deadline = DateTime.UtcNow.AddSeconds(15); + while (!condition() && DateTime.UtcNow < deadline) { - Thread.Sleep(50); + Thread.Sleep(20); } + Assert.True(condition(), "timed out waiting for " + description); } + private static void WaitForReads(ScriptedFileReader reader, int count) => + WaitUntil(() => reader.Reads >= count, count + " file reads"); + [Fact] public void ParseFailureFromPartialReadIsRetriedUntilContentIsComplete() { @@ -390,12 +396,126 @@ public void PendingParseRetryIsCanceledByDispose() using (var file = TempFile.Create()) { factory.FilePaths(file.Path).AutoUpdate(true).FileReader(reader); - var fp = MakeDataSource(); - fp.Start(); // schedules a retry - Assert.Equal(1, reader.Reads); - fp.Dispose(); - Thread.Sleep(1500); - Assert.Equal(1, reader.Reads); // the pending retry observed the disposal and did nothing + using (var fp = MakeDataSource()) + { + fp.Start(); // schedules a retry + fp.Dispose(); + // No new load may start after Dispose; any retry scheduled before it must + // observe the disposal and do nothing. (Reads are captured after Dispose so + // the test stays valid even if a retry fired before Dispose ran.) + var readsAtDispose = reader.Reads; + Thread.Sleep(1500); + Assert.Equal(readsAtDispose, reader.Reads); + } + } + } + + // The multi-path tests below use paths in a directory that does not exist, so the file + // watcher fails to construct (logged and swallowed, _reloader == null). That makes the + // load sequence fully deterministic: the only externally triggered loads are the Start() + // calls, which exercise the same code path as a file-change notification. + private const string MultiPathA = "/nonexistent-ld-filedatasource-test-dir/a.json"; + private const string MultiPathB = "/nonexistent-ld-filedatasource-test-dir/b.json"; + + private class PerPathScriptedReader : FileDataTypes.IFileReader + { + private readonly ConcurrentDictionary _reads = new ConcurrentDictionary(); + private readonly ConcurrentDictionary _bad = new ConcurrentDictionary(); + + public void SetBad(string path, bool bad) { _bad[path] = bad; } + public int Reads(string path) => _reads.TryGetValue(path, out var n) ? n : 0; + + public string ReadAllText(string path) + { + _reads.AddOrUpdate(path, 1, (_, n) => n + 1); + if (_bad.TryGetValue(path, out var bad) && bad) + { + return TruncatedFlagJson; + } + // distinct flag keys per path, so a successful merge of both files can't throw + // on duplicate keys (the builder default is DuplicateKeysHandling.Throw) + return path == MultiPathA + ? @"{""flagValues"":{""flagA"":""a""}}" + : @"{""flagValues"":{""flagB"":""b""}}"; + } + } + + [Fact] + public void PendingParseRetryIsSkippedIfAnExternalReloadAlreadySucceeded() + { + var reader = new ScriptedFileReader(); + using (var file = TempFile.Create()) + { + factory.FilePaths(file.Path).AutoUpdate(true).FileReader(reader); + using (var fp = MakeDataSource()) + { + fp.Start(); // fails, schedules a retry + + // An externally triggered load (same code path as a file-change notification) + // succeeds before the pending retry fires. + reader.Bad = false; + fp.Start(); + _updateSink.Inits.ExpectValue(TimeSpan.FromSeconds(1)); + var readsAfterSuccess = reader.Reads; + + Thread.Sleep(1500); // past the retry delay + // The pending retry saw that the load already succeeded and did not reload, + // so no redundant Init (which would fire spurious change events) occurred. + Assert.Equal(readsAfterSuccess, reader.Reads); + _updateSink.Inits.ExpectNoValue(); + } + } + } + + [Fact] + public void ParseFailureInANewEpisodeGetsAFullRetryBudgetAfterAnotherPathGaveUp() + { + var reader = new PerPathScriptedReader(); + reader.SetBad(MultiPathB, true); + factory.FilePaths(MultiPathA, MultiPathB).AutoUpdate(true).FileReader(reader); + using (var fp = MakeDataSource()) + { + fp.Start(); // A parses, B fails; B accumulates failures across retries + WaitUntil(() => reader.Reads(MultiPathB) >= 4, "4 reads of path B"); + reader.SetBad(MultiPathA, true); // now every attempt stops at A + WaitUntil(() => reader.Reads(MultiPathA) >= 9, "A to exhaust its retry budget"); + Thread.Sleep(1500); // the retry chain is dead + Assert.Equal(9, reader.Reads(MultiPathA)); + + // A new externally triggered load fails at A; A recovers before the retry fires. + fp.Start(); + reader.SetBad(MultiPathA, false); + WaitUntil(() => reader.Reads(MultiPathB) >= 5, "the retry to reach path B"); + reader.SetBad(MultiPathB, false); + + // B's failure was its first in the new episode, so it gets a fresh retry budget + // instead of inheriting the dead episode's count and giving up immediately. + _updateSink.Inits.ExpectValue(TimeSpan.FromSeconds(5)); + } + } + + [Fact] + public void PathAbandonedWhenAnotherPathGivesUpGetsATerminalLogMessage() + { + var reader = new PerPathScriptedReader(); + reader.SetBad(MultiPathB, true); + factory.FilePaths(MultiPathA, MultiPathB).AutoUpdate(true).FileReader(reader); + using (var fp = MakeDataSource()) + { + fp.Start(); + WaitUntil(() => reader.Reads(MultiPathB) >= 4, "4 reads of path B"); + reader.SetBad(MultiPathA, true); + WaitUntil(() => reader.Reads(MultiPathA) >= 9, "A to exhaust its retry budget"); + Thread.Sleep(1500); // the retry chain is dead + + // B was last warned "will retry in 600 ms", but A's give-up ended the chain. + // That promise must be either fulfilled (B re-read) or terminated with an + // error log naming B. + var bRetried = reader.Reads(MultiPathB) >= 5; + var bTerminallyLogged = LogCapture.GetMessages().Any(m => + m.Level == LogLevel.Error && m.Text.Contains(MultiPathB)); + Assert.True(bRetried || bTerminallyLogged, + "path B was promised a retry but was never re-read and got no terminal error log"); } } From 32918b33c7da594b0581cb6296ae97daf7025763 Mon Sep 17 00:00:00 2001 From: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Date: Mon, 17 Aug 2026 16:28:27 -0700 Subject: [PATCH 41/51] ci: trigger flake check run 37 From 5f8ed89bd253ed5db584412d252d0f13c156b089 Mon Sep 17 00:00:00 2001 From: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Date: Mon, 17 Aug 2026 16:28:45 -0700 Subject: [PATCH 42/51] ci: trigger flake check run 38 From 9f9a8024d0404d5d6b243458753dbda895bd52e6 Mon Sep 17 00:00:00 2001 From: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Date: Mon, 17 Aug 2026 16:29:02 -0700 Subject: [PATCH 43/51] ci: trigger flake check run 39 From c2b6c41761a4a12028081b2fafd80f0d77a9f4e1 Mon Sep 17 00:00:00 2001 From: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Date: Mon, 17 Aug 2026 16:29:20 -0700 Subject: [PATCH 44/51] ci: trigger flake check run 40 From 811987207ae79eb4be8d7cdd791a5c8a480d0fa7 Mon Sep 17 00:00:00 2001 From: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Date: Mon, 17 Aug 2026 16:29:37 -0700 Subject: [PATCH 45/51] ci: trigger flake check run 41 From 60252222e665a5487878c2c426513d2a5baf5cf3 Mon Sep 17 00:00:00 2001 From: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Date: Mon, 17 Aug 2026 16:29:54 -0700 Subject: [PATCH 46/51] ci: trigger flake check run 42 From 761004842f8eae5fb654ef1f24fd0110c691b588 Mon Sep 17 00:00:00 2001 From: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Date: Mon, 17 Aug 2026 16:30:12 -0700 Subject: [PATCH 47/51] ci: trigger flake check run 43 From 068fd1a43dfe521ee9f27bfdda555867cac3aafd Mon Sep 17 00:00:00 2001 From: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Date: Mon, 17 Aug 2026 16:30:29 -0700 Subject: [PATCH 48/51] ci: trigger flake check run 44 From e187c918de494d422fe498d5bed04246773900c2 Mon Sep 17 00:00:00 2001 From: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Date: Mon, 17 Aug 2026 16:30:46 -0700 Subject: [PATCH 49/51] ci: trigger flake check run 45 From 64cb51422f07f199d720c04735e2869d5e00776f Mon Sep 17 00:00:00 2001 From: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Date: Tue, 18 Aug 2026 11:04:30 -0700 Subject: [PATCH 50/51] fix: Continue the retry episode when a retry attempt fails before parsing. 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. --- .../Internal/DataSources/FileDataSource.cs | 58 +++++++++++++++---- .../DataSources/FileDataSourceTest.cs | 30 ++++++++++ 2 files changed, 78 insertions(+), 10 deletions(-) diff --git a/pkgs/sdk/server/src/Internal/DataSources/FileDataSource.cs b/pkgs/sdk/server/src/Internal/DataSources/FileDataSource.cs index 0589b7aa..3d14176a 100644 --- a/pkgs/sdk/server/src/Internal/DataSources/FileDataSource.cs +++ b/pkgs/sdk/server/src/Internal/DataSources/FileDataSource.cs @@ -149,6 +149,15 @@ private void LoadAll(bool isRetry) catch (Exception e) { LogHelpers.LogException(_logger, "Failed to load " + path, e); + if (isRetry) + { + // A transient read error (for example, a writer replacing the file) + // must not end a retry episode early: paths that were promised + // retries would keep stale data with budget remaining, and another + // file-change notification is not guaranteed. Charge the failure to + // the same per-path budget and continue the chain. + HandleRetryLoadFailure(path); + } return; } } @@ -190,21 +199,50 @@ private void HandleParseFailure(string path, Exception e) } else { - // This path kept failing, so the retry chain ends until the next external trigger. - // Every attempt stopped at this path, so any other paths with recorded failures - // were never re-attempted; their episodes end here too. - _parseFailureCounts.Remove(path); - foreach (var abandoned in _parseFailureCounts.Keys) - { - _logger.Error("{0}: Will not be retried because {1} repeatedly failed to parse; both will be re-read on the next detected file change", - abandoned, path); - } - _parseFailureCounts.Clear(); + EndEpisode(path); LogHelpers.LogException(_logger, string.Format("{0}: Failed to parse file after {1} attempts", path, MaxParseAttempts), e); } } + // Called under _updateLock when a retry attempt fails before parsing (for example, a + // transient read error). The caller has already logged the exception; this charges the + // failure to the path's per-episode budget and continues or ends the retry chain. + private void HandleRetryLoadFailure(string path) + { + var attempts = _parseFailureCounts.TryGetValue(path, out var previousAttempts) + ? previousAttempts + 1 + : 1; + + if (attempts < MaxParseAttempts) + { + _parseFailureCounts[path] = attempts; + _logger.Warn("{0}: Failed to read file on a retry; will retry again in {1} ms", + path, RetryDelay.TotalMilliseconds); + ScheduleRetry(); + } + else + { + EndEpisode(path); + _logger.Error("{0}: Failed to load file after {1} attempts; will not retry until the next detected file change", + path, MaxParseAttempts); + } + } + + // Called under _updateLock. Ends the current failure episode for every path: the chain + // stopped at failedPath on every attempt, so any other paths with recorded failures were + // never re-attempted and their promised retries cannot happen. + private void EndEpisode(string failedPath) + { + _parseFailureCounts.Remove(failedPath); + foreach (var abandoned in _parseFailureCounts.Keys) + { + _logger.Error("{0}: Will not be retried because {1} repeatedly failed to load; both will be re-read on the next detected file change", + abandoned, failedPath); + } + _parseFailureCounts.Clear(); + } + // Called under _updateLock. private void ScheduleRetry() { diff --git a/pkgs/sdk/server/test/Internal/DataSources/FileDataSourceTest.cs b/pkgs/sdk/server/test/Internal/DataSources/FileDataSourceTest.cs index f2cb1813..1b5dc29b 100644 --- a/pkgs/sdk/server/test/Internal/DataSources/FileDataSourceTest.cs +++ b/pkgs/sdk/server/test/Internal/DataSources/FileDataSourceTest.cs @@ -1,5 +1,6 @@ using System; using System.Collections.Concurrent; +using System.IO; using System.Linq; using System.Threading; using Castle.Core.Internal; @@ -267,11 +268,16 @@ private class ScriptedFileReader : FileDataTypes.IFileReader { private int _reads; public volatile bool Bad = true; + public volatile bool Throw = false; public int Reads => Volatile.Read(ref _reads); public string ReadAllText(string path) { Interlocked.Increment(ref _reads); + if (Throw) + { + throw new IOException("simulated transient read error"); + } return Bad ? TruncatedFlagJson : ValidFlagJson; } } @@ -389,6 +395,30 @@ public void ParseFailureIsNotRetriedIfAutoUpdateIsOff() } } + [Fact] + public void TransientReadErrorDuringRetryDoesNotEndTheEpisode() + { + var reader = new ScriptedFileReader(); + using (var file = TempFile.Create()) + { + factory.FilePaths(file.Path).AutoUpdate(true).FileReader(reader); + using (var fp = MakeDataSource()) + { + fp.Start(); // parse fails, schedules a retry + // The retry's read fails transiently (e.g. a writer is replacing the file); + // the content itself is complete from here on. + reader.Bad = false; + reader.Throw = true; + WaitForReads(reader, 2); + reader.Throw = false; + + // The episode still has budget, so the chain must continue and load the data + // instead of dying on the non-parse failure. + _updateSink.Inits.ExpectValue(TimeSpan.FromSeconds(5)); + } + } + } + [Fact] public void PendingParseRetryIsCanceledByDispose() { From b9fcb6368e0e520895be494b9aa324bbf8d6a514 Mon Sep 17 00:00:00 2001 From: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Date: Wed, 19 Aug 2026 09:55:25 -0700 Subject: [PATCH 51/51] refactor: Apply review feedback to the retry implementation. - 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. --- .../Internal/DataSources/FileDataSource.cs | 115 ++++++++++-------- 1 file changed, 64 insertions(+), 51 deletions(-) diff --git a/pkgs/sdk/server/src/Internal/DataSources/FileDataSource.cs b/pkgs/sdk/server/src/Internal/DataSources/FileDataSource.cs index 3d14176a..0a4a58d9 100644 --- a/pkgs/sdk/server/src/Internal/DataSources/FileDataSource.cs +++ b/pkgs/sdk/server/src/Internal/DataSources/FileDataSource.cs @@ -30,16 +30,30 @@ internal sealed class FileDataSource : IDataSource private volatile int _lastVersion; private object _updateLock = new object(); - private const int MaxParseAttempts = 5; + private const int MaxLoadAttempts = 5; private static readonly TimeSpan RetryDelay = TimeSpan.FromMilliseconds(600); - // Consecutive parse failures per path within the current failure episode. An externally - // triggered load (Start or a file-change notification) starts a new episode and clears this, - // so the retry budget is per-episode, not per-lifetime. Only touched inside _updateLock. - private readonly Dictionary _parseFailureCounts = new Dictionary(); + // Consecutive load failures (parse or read) per path within the current failure episode. + // An externally triggered load (Start or a file-change notification) starts a new episode + // and clears this, so the retry budget is per-episode, not per-lifetime. Only touched + // inside _updateLock. + private readonly Dictionary _loadFailureCounts = new Dictionary(); // Whether a delayed retry is already scheduled; at most one retry chain exists at a time, // since each retry re-reads every path anyway. Only touched inside _updateLock. private bool _retryPending; + /// + /// Constructs a file data source that loads flag and segment data from local files. + /// + /// receives the data set produced by each successful load + /// reads file contents; injectable for testing + /// the file paths to load, in order + /// true to watch the files and reload on changes; also enables the + /// bounded retry of loads that fail while a file is being written + /// optional parser for non-JSON content (for example YAML); + /// null to parse JSON only + /// true to skip missing files instead of failing the load + /// how to handle a key that appears in more than one file + /// the destination for log output public FileDataSource(IDataSourceUpdates dataSourceUpdates, FileDataTypes.IFileReader fileReader, List paths, bool autoUpdate, Func alternateParser, bool skipMissingPaths, FileDataTypes.DuplicateKeysHandling duplicateKeysHandling, @@ -111,10 +125,22 @@ private void LoadAll(bool isRetry) } if (!isRetry) { - // An externally triggered load starts a new failure episode: parse failures + // An externally triggered load starts a new failure episode: failures // observed from here on get a fresh retry budget, and any state left over // from a previous episode is discarded. - _parseFailureCounts.Clear(); + _loadFailureCounts.Clear(); + } + else + { + _retryPending = false; + if (_loadFailureCounts.Count == 0) + { + // The failure state was cleared in the meantime (an externally triggered + // load succeeded, or the chain gave up) — a reload would be redundant and + // would re-Init identical data at bumped versions, firing spurious change + // events. + return; + } } var version = Interlocked.Increment(ref _lastVersion); var flags = new Dictionary(); @@ -139,7 +165,7 @@ private void LoadAll(bool isRetry) HandleParseFailure(path, e); return; } - _parseFailureCounts.Remove(path); + _loadFailureCounts.Remove(path); _dataMerger.AddToData(data, flags, segments); } catch (FileNotFoundException) when (_skipMissingPaths) @@ -148,15 +174,19 @@ private void LoadAll(bool isRetry) } catch (Exception e) { - LogHelpers.LogException(_logger, "Failed to load " + path, e); if (isRetry) { // A transient read error (for example, a writer replacing the file) // must not end a retry episode early: paths that were promised // retries would keep stale data with budget remaining, and another // file-change notification is not guaranteed. Charge the failure to - // the same per-path budget and continue the chain. - HandleRetryLoadFailure(path); + // the same per-path budget and continue the chain; it logs a Warn + // while retrying and an Error only on give-up. + HandleRetryLoadFailure(path, e); + } + else + { + LogHelpers.LogException(_logger, "Failed to load " + path, e); } return; } @@ -173,7 +203,7 @@ private void LoadAll(bool isRetry) } // Called under _updateLock when parsing a path's content fails. Since an externally - // triggered load clears _parseFailureCounts before reading, any existing count for the + // triggered load clears _loadFailureCounts before reading, any existing count for the // path belongs to the current episode. private void HandleParseFailure(string path, Exception e) { @@ -185,13 +215,12 @@ private void HandleParseFailure(string path, Exception e) return; } - var attempts = _parseFailureCounts.TryGetValue(path, out var previousAttempts) - ? previousAttempts + 1 - : 1; + _loadFailureCounts.TryGetValue(path, out var previousAttempts); + var attempts = previousAttempts + 1; + _loadFailureCounts[path] = attempts; - if (attempts < MaxParseAttempts) + if (attempts < MaxLoadAttempts) { - _parseFailureCounts[path] = attempts; _logger.Warn("{0}: Failed to parse file ({1}); will retry in {2} ms in case it was incompletely written", path, LogValues.ExceptionSummary(e), RetryDelay.TotalMilliseconds); _logger.Debug("{0}", LogValues.ExceptionTrace(e)); @@ -201,31 +230,32 @@ private void HandleParseFailure(string path, Exception e) { EndEpisode(path); LogHelpers.LogException(_logger, - string.Format("{0}: Failed to parse file after {1} attempts", path, MaxParseAttempts), e); + string.Format("{0}: Failed to parse file after {1} attempts", path, MaxLoadAttempts), e); } } // Called under _updateLock when a retry attempt fails before parsing (for example, a - // transient read error). The caller has already logged the exception; this charges the - // failure to the path's per-episode budget and continues or ends the retry chain. - private void HandleRetryLoadFailure(string path) + // transient read error). Charges the failure to the path's per-episode budget and + // continues or ends the retry chain. + private void HandleRetryLoadFailure(string path, Exception e) { - var attempts = _parseFailureCounts.TryGetValue(path, out var previousAttempts) - ? previousAttempts + 1 - : 1; + _loadFailureCounts.TryGetValue(path, out var previousAttempts); + var attempts = previousAttempts + 1; + _loadFailureCounts[path] = attempts; - if (attempts < MaxParseAttempts) + if (attempts < MaxLoadAttempts) { - _parseFailureCounts[path] = attempts; - _logger.Warn("{0}: Failed to read file on a retry; will retry again in {1} ms", - path, RetryDelay.TotalMilliseconds); + _logger.Warn("{0}: Failed to read file on a retry ({1}); will retry again in {2} ms", + path, LogValues.ExceptionSummary(e), RetryDelay.TotalMilliseconds); + _logger.Debug("{0}", LogValues.ExceptionTrace(e)); ScheduleRetry(); } else { EndEpisode(path); - _logger.Error("{0}: Failed to load file after {1} attempts; will not retry until the next detected file change", - path, MaxParseAttempts); + LogHelpers.LogException(_logger, + string.Format("{0}: Failed to load file after {1} attempts; will not retry until the next detected file change", + path, MaxLoadAttempts), e); } } @@ -234,13 +264,13 @@ private void HandleRetryLoadFailure(string path) // never re-attempted and their promised retries cannot happen. private void EndEpisode(string failedPath) { - _parseFailureCounts.Remove(failedPath); - foreach (var abandoned in _parseFailureCounts.Keys) + _loadFailureCounts.Remove(failedPath); + foreach (var abandoned in _loadFailureCounts.Keys) { _logger.Error("{0}: Will not be retried because {1} repeatedly failed to load; both will be re-read on the next detected file change", abandoned, failedPath); } - _parseFailureCounts.Clear(); + _loadFailureCounts.Clear(); } // Called under _updateLock. @@ -256,7 +286,7 @@ private void ScheduleRetry() await Task.Delay(RetryDelay).ConfigureAwait(false); try { - RetryLoadAll(); + LoadAll(isRetry: true); } catch (Exception e) { @@ -266,23 +296,6 @@ private void ScheduleRetry() }); } - private void RetryLoadAll() - { - lock (_updateLock) - { - _retryPending = false; - if (_disposed || _parseFailureCounts.Count == 0) - { - // Disposed, or the failure state was cleared in the meantime (an externally - // triggered load succeeded, or the chain gave up) — a reload would be redundant - // and would re-Init identical data at bumped versions, firing spurious change - // events. - return; - } - LoadAll(isRetry: true); - } - } - private void TriggerReload() { if (_started)