fix(reconcile): batch startup reconcile into fixed round trips, bound central work by QueryTimeout, retry failed site passes with back-off (review 2026-10-21 03/P-1)

2026-10-22 06:25:57 -04:00
parent d6a9a79c27
commit 7c83a4aa94
9 changed files with 638 additions and 130 deletions
@@ -40,13 +40,17 @@ public class CentralCommunicationActorReconcileTests : TestKit
            {
                new(2, "inst-B", "rev2", "dep-B", true),
            });
-        deploymentRepo.GetDeployedSnapshotByInstanceIdAsync(2, Arg.Any<CancellationToken>())
-            .Returns(new DeployedConfigSnapshot("dep-B", "rev2", "{\"cfg\":\"B\"}"));
-        deploymentRepo.StagePendingIfAbsentAsync(
-                Arg.Any<int>(), Arg.Any<string>(), Arg.Any<string>(), Arg.Any<string>(),
-                Arg.Any<string>(), Arg.Any<DateTimeOffset>(), Arg.Any<DateTimeOffset>(),
-                Arg.Any<CancellationToken>())
-            .Returns(true);
+        deploymentRepo.GetDeployedSnapshotsByInstanceIdsAsync(
+                Arg.Any<IReadOnlyCollection<int>>(), Arg.Any<CancellationToken>())
+            .Returns(new Dictionary<int, DeployedConfigSnapshot>
+            {
+                [2] = new("dep-B", "rev2", "{\"cfg\":\"B\"}") { InstanceId = 2 },
+            });
+        // Every candidate is staged fresh: the batch hands each one back as its live row.
+        deploymentRepo.StagePendingBatchIfAbsentAsync(
+                Arg.Any<IReadOnlyList<PendingDeployment>>(), Arg.Any<DateTimeOffset>(), Arg.Any<CancellationToken>())
+            .Returns(ci => (IReadOnlyDictionary<int, PendingDeployment>)ci.ArgAt<IReadOnlyList<PendingDeployment>>(0)
+                .ToDictionary(p => p.InstanceId));

        var options = Options.Create(new CommunicationOptions
        {
@@ -25,12 +25,43 @@ public class ReconcileServiceTests
        Substitute.For<IDeploymentManagerRepository>();
    private readonly ISiteRepository _siteRepo = Substitute.For<ISiteRepository>();

-    private ReconcileService CreateService(TimeSpan? ttl = null)
+    // Fake repository state behind the two batched reconcile calls.
+    private readonly Dictionary<int, DeployedConfigSnapshot> _snapshots = new();
+    private readonly Dictionary<int, PendingDeployment> _existingLive = new();
+    private readonly HashSet<int> _racedAway = new();
+
+    /// <summary>Wires the batched snapshot read and batched stage to the fake state above.</summary>
+    public ReconcileServiceTests()
+    {
+        _deploymentRepo.GetDeployedSnapshotsByInstanceIdsAsync(
+                Arg.Any<IReadOnlyCollection<int>>(), Arg.Any<CancellationToken>())
+            .Returns(ci => (IReadOnlyDictionary<int, DeployedConfigSnapshot>)ci.ArgAt<IReadOnlyCollection<int>>(0)
+                .Where(_snapshots.ContainsKey)
+                .ToDictionary(id => id, id => _snapshots[id]));
+
+        _deploymentRepo.StagePendingBatchIfAbsentAsync(
+                Arg.Any<IReadOnlyList<PendingDeployment>>(), Arg.Any<DateTimeOffset>(), Arg.Any<CancellationToken>())
+            .Returns(ci =>
+            {
+                var live = new Dictionary<int, PendingDeployment>();
+                foreach (var candidate in ci.ArgAt<IReadOnlyList<PendingDeployment>>(0))
+                {
+                    if (_existingLive.TryGetValue(candidate.InstanceId, out var existing))
+                        live[candidate.InstanceId] = existing;
+                    else if (!_racedAway.Contains(candidate.InstanceId))
+                        live[candidate.InstanceId] = candidate;
+                }
+                return (IReadOnlyDictionary<int, PendingDeployment>)live;
+            });
+    }
+
+    private ReconcileService CreateService(TimeSpan? ttl = null, TimeSpan? queryTimeout = null)
    {
        var options = Options.Create(new CommunicationOptions
        {
            CentralFetchBaseUrl = BaseUrl,
            PendingDeploymentTtl = ttl ?? TimeSpan.FromMinutes(5),
+            QueryTimeout = queryTimeout ?? TimeSpan.FromSeconds(30),
        });
        return new ReconcileService(
            _deploymentRepo, _siteRepo, options, NullLogger<ReconcileService>.Instance);
@@ -48,15 +79,11 @@ public class ReconcileServiceTests
            .Returns(expected.ToList());

    private void SnapshotFor(ExpectedDeployment exp) =>
-        _deploymentRepo.GetDeployedSnapshotByInstanceIdAsync(exp.InstanceId, Arg.Any<CancellationToken>())
-            .Returns(new DeployedConfigSnapshot(exp.DeploymentId, exp.RevisionHash, $"{{\"cfg\":\"{exp.InstanceUniqueName}\"}}"));
-
-    private void StageReturns(bool result) =>
-        _deploymentRepo.StagePendingIfAbsentAsync(
-                Arg.Any<int>(), Arg.Any<string>(), Arg.Any<string>(), Arg.Any<string>(),
-                Arg.Any<string>(), Arg.Any<DateTimeOffset>(), Arg.Any<DateTimeOffset>(),
-                Arg.Any<CancellationToken>())
-            .Returns(result);
+        _snapshots[exp.InstanceId] = new DeployedConfigSnapshot(
+            exp.DeploymentId, exp.RevisionHash, $"{{\"cfg\":\"{exp.InstanceUniqueName}\"}}")
+        {
+            InstanceId = exp.InstanceId
+        };

    private static ReconcileSiteRequest Request(params (string Name, string Rev)[] local) =>
        new(SiteIdentifier, "node-a",
@@ -74,7 +101,6 @@ public class ReconcileServiceTests
        ExpectedSet(a, b, c);
        SnapshotFor(b);
        SnapshotFor(c);
-        StageReturns(true);

        // Node has A current, B stale (revOLD), C missing entirely.
        var response = await CreateService().ReconcileAsync(
@@ -109,7 +135,6 @@ public class ReconcileServiceTests
        SiteResolves();
        var a = Expected(1, "inst-A", "rev1", "dep-A");
        ExpectedSet(a);
-        StageReturns(true);

        // inst-A is current; inst-Z is not deployed centrally → orphan.
        var response = await CreateService().ReconcileAsync(
@@ -120,10 +145,10 @@ public class ReconcileServiceTests
        Assert.Equal("inst-Z", orphan);
    }

-    /// <summary>Verifies that when staging a pending deployment returns false and a row already exists, the gap carries the existing token.</summary>
+    /// <summary>Verifies that when a live pending row already exists for a gap instance, the gap carries the existing token.</summary>
    /// <returns>A task that represents the asynchronous operation.</returns>
    [Fact]
-    public async Task Reconcile_StagePendingReturnsFalse_ExistingRowPresent_ReturnsExistingTokenInGap()
+    public async Task Reconcile_LiveRowAlreadyExists_ReturnsExistingTokenInGap()
    {
        // Live-found bug: two site nodes concurrently missing the SAME instance both reconcile at
        // startup. The first stages a pending row; the second's StagePendingIfAbsent returns false.
@@ -138,19 +163,11 @@ public class ReconcileServiceTests

        // B stages fresh (true). C already has a pending row (a concurrent reconcile from the
        // other node, or an in-flight deploy, staged it first) → stage returns false.
-        StageReturns(true);
-        _deploymentRepo.StagePendingIfAbsentAsync(
-                3, Arg.Any<string>(), Arg.Any<string>(), Arg.Any<string>(),
-                Arg.Any<string>(), Arg.Any<DateTimeOffset>(), Arg.Any<DateTimeOffset>(),
-                Arg.Any<CancellationToken>())
-            .Returns(false);
-
        // The existing pending row for inst-C carries the FIRST node's deploymentId + token.
        var nowUtc = DateTimeOffset.UtcNow;
-        _deploymentRepo.GetPendingDeploymentByInstanceIdAsync(3, Arg.Any<DateTimeOffset?>(), Arg.Any<CancellationToken>())
-            .Returns(new PendingDeployment(
-                "dep-C-existing", 3, "rev3-existing", "{\"cfg\":\"inst-C\"}",
-                "tok-C-existing", nowUtc, nowUtc.AddMinutes(5)));
+        _existingLive[3] = new PendingDeployment(
+            "dep-C-existing", 3, "rev3-existing", string.Empty,
+            "tok-C-existing", nowUtc, nowUtc.AddMinutes(5));

        var response = await CreateService().ReconcileAsync(Request(/* empty local inventory */));

@@ -168,10 +185,10 @@ public class ReconcileServiceTests
        Assert.True(gapC.IsEnabled);
    }

-    /// <summary>Verifies that when staging a pending deployment returns false and no row exists, that gap item is omitted.</summary>
+    /// <summary>Verifies that when no pending row can be staged or found, that gap item is omitted.</summary>
    /// <returns>A task that represents the asynchronous operation.</returns>
    [Fact]
-    public async Task Reconcile_StagePendingReturnsFalse_NoExistingRow_OmitsThatGapItem()
+    public async Task Reconcile_NoLiveRowStagedOrFound_OmitsThatGapItem()
    {
        // Fallback path: stage returns false, but the pending row raced away (was purged between
        // the stage attempt and the read). Omit the item — the node retries on the next reconcile.
@@ -182,16 +199,8 @@ public class ReconcileServiceTests
        SnapshotFor(b);
        SnapshotFor(c);

-        StageReturns(true);
-        _deploymentRepo.StagePendingIfAbsentAsync(
-                3, Arg.Any<string>(), Arg.Any<string>(), Arg.Any<string>(),
-                Arg.Any<string>(), Arg.Any<DateTimeOffset>(), Arg.Any<DateTimeOffset>(),
-                Arg.Any<CancellationToken>())
-            .Returns(false);
-
-        // The pending row was purged between the stage attempt and the read.
-        _deploymentRepo.GetPendingDeploymentByInstanceIdAsync(3, Arg.Any<DateTimeOffset?>(), Arg.Any<CancellationToken>())
-            .Returns((PendingDeployment?)null);
+        // No row could be staged or found for inst-C (raced away).
+        _racedAway.Add(3);

        var response = await CreateService().ReconcileAsync(Request(/* empty local inventory */));

@@ -209,7 +218,6 @@ public class ReconcileServiceTests
        var b = Expected(2, "inst-B", "rev2", "dep-B");
        ExpectedSet(b);
        // No snapshot configured for inst-B → repo returns null (deleted race).
-        StageReturns(true);

        var response = await CreateService().ReconcileAsync(Request());

@@ -234,4 +242,79 @@ public class ReconcileServiceTests
        await _deploymentRepo.DidNotReceive()
            .GetExpectedDeploymentsForSiteAsync(Arg.Any<int>(), Arg.Any<CancellationToken>());
    }
+
+    // ── review 2026-10-21 03/P-1: batched reconcile + bounded work ──
+
+    /// <summary>
+    /// The reconcile pass is a fixed number of round trips: snapshot JSON is read ONCE, only
+    /// for the gap, and the pending rows are staged in ONE batch — never per gap instance.
+    /// </summary>
+    /// <returns>A task that represents the asynchronous operation.</returns>
+    [Fact]
+    public async Task Reconcile_ReadsGapSnapshotsOnce_AndStagesInOneBatch()
+    {
+        SiteResolves();
+        var a = Expected(1, "inst-A", "rev1", "dep-A");
+        var b = Expected(2, "inst-B", "rev2", "dep-B");
+        var c = Expected(3, "inst-C", "rev3", "dep-C");
+        ExpectedSet(a, b, c);
+        SnapshotFor(a);
+        SnapshotFor(b);
+        SnapshotFor(c);
+
+        var response = await CreateService().ReconcileAsync(Request(("inst-A", "rev1")));
+
+        Assert.Equal(2, response.Gap.Count);
+        await _deploymentRepo.Received(1).GetDeployedSnapshotsByInstanceIdsAsync(
+            Arg.Is<IReadOnlyCollection<int>>(ids => ids.Order().SequenceEqual(new[] { 2, 3 })),
+            Arg.Any<CancellationToken>());
+        await _deploymentRepo.Received(1).StagePendingBatchIfAbsentAsync(
+            Arg.Is<IReadOnlyList<PendingDeployment>>(rows => rows.Count == 2),
+            Arg.Any<DateTimeOffset>(), Arg.Any<CancellationToken>());
+        await _deploymentRepo.DidNotReceive().GetDeployedSnapshotByInstanceIdAsync(
+            Arg.Any<int>(), Arg.Any<CancellationToken>());
+        await _deploymentRepo.DidNotReceive().StagePendingIfAbsentAsync(
+            Arg.Any<int>(), Arg.Any<string>(), Arg.Any<string>(), Arg.Any<string>(),
+            Arg.Any<string>(), Arg.Any<DateTimeOffset>(), Arg.Any<DateTimeOffset>(),
+            Arg.Any<CancellationToken>());
+    }
+
+    /// <summary>A node with nothing missing costs no snapshot read and no staging at all.</summary>
+    /// <returns>A task that represents the asynchronous operation.</returns>
+    [Fact]
+    public async Task Reconcile_NoGap_DoesNoSnapshotReadOrStaging()
+    {
+        SiteResolves();
+        ExpectedSet(Expected(1, "inst-A", "rev1", "dep-A"));
+
+        var response = await CreateService().ReconcileAsync(Request(("inst-A", "rev1")));
+
+        Assert.Empty(response.Gap);
+        await _deploymentRepo.DidNotReceive().GetDeployedSnapshotsByInstanceIdsAsync(
+            Arg.Any<IReadOnlyCollection<int>>(), Arg.Any<CancellationToken>());
+        await _deploymentRepo.DidNotReceive().StagePendingBatchIfAbsentAsync(
+            Arg.Any<IReadOnlyList<PendingDeployment>>(), Arg.Any<DateTimeOffset>(), Arg.Any<CancellationToken>());
+    }
+
+    /// <summary>
+    /// Central stops working once the site's own deadline (QueryTimeout) has passed instead of
+    /// staging tokens for a caller that has already given up.
+    /// </summary>
+    /// <returns>A task that represents the asynchronous operation.</returns>
+    [Fact]
+    public async Task Reconcile_WorkIsBoundedByQueryTimeout()
+    {
+        SiteResolves();
+        _deploymentRepo.GetExpectedDeploymentsForSiteAsync(SiteId, Arg.Any<CancellationToken>())
+            .Returns(async ci =>
+            {
+                await Task.Delay(TimeSpan.FromSeconds(30), ci.ArgAt<CancellationToken>(1));
+                return (IReadOnlyList<ExpectedDeployment>)new List<ExpectedDeployment>();
+            });
+
+        var service = CreateService(queryTimeout: TimeSpan.FromMilliseconds(100));
+
+        await Assert.ThrowsAnyAsync<OperationCanceledException>(
+            () => service.ReconcileAsync(Request()));
+    }
 }