From a659ee854db611fbd7b007652f67918f66e7e075 Mon Sep 17 00:00:00 2001 From: "Mars.P" Date: Mon, 15 Jun 2026 10:29:38 +0800 Subject: [PATCH 1/2] Align in_flight_scripts_json to the array shape it actually carries MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The deployment checkpoint's in_flight_scripts_json column was seeded/defaulted to the legacy object form "{}", but InFlightScriptMap serializes a List — a JSON array [{ "m", "s", "a", "t" }]. Parse("{}") throws (caught → treated as empty), so "{}" only ever worked via that catch fallback, and a fresh checkpoint's "{}" leaked past IsMachineBusyAsync's "!= []" pre-filter. Align the empty form to "[]": - Entity default (DeploymentExecutionCheckpoint) + doc-comment (now describes the array shape). - EF column default (DeploymentExecutionCheckpointConfiguration) → "[]". - EnsureExistsAsync seed → "[]". - DbUp migration: ALTER COLUMN ... SET DEFAULT '[]'::jsonb and rewrite existing empty '{}' rows to '[]' (empty object == empty array in meaning, so a pure shape normalization). Non-breaking: InFlightScriptMap.Parse still tolerates any leftover legacy shape (left untouched), and EF always supplies an explicit value on insert, so the DB default is belt-and-braces. No new MachineUpgradeStatus enum / no upgrade-defer logic change — purely a data-shape alignment. Tests: new integration case pins EnsureExistsAsync seeds "[]" (not "{}"); existing InFlightScriptStore (16) + InFlightScriptMap (23, incl. legacy-{} → empty via the catch) suites green. --- ...lign_inflight_scripts_default_to_array.sql | 28 +++++++++++++++++++ .../DeploymentExecutionCheckpoint.cs | 8 ++++-- ...loymentExecutionCheckpointConfiguration.cs | 3 +- .../DeploymentCheckpointService.cs | 2 +- .../Checkpoints/InFlightScriptStoreTests.cs | 26 +++++++++++++---- 5 files changed, 57 insertions(+), 10 deletions(-) create mode 100644 src/Squid.Core/Persistence/DbUpFiles/20260615_align_inflight_scripts_default_to_array.sql diff --git a/src/Squid.Core/Persistence/DbUpFiles/20260615_align_inflight_scripts_default_to_array.sql b/src/Squid.Core/Persistence/DbUpFiles/20260615_align_inflight_scripts_default_to_array.sql new file mode 100644 index 00000000..2efd92df --- /dev/null +++ b/src/Squid.Core/Persistence/DbUpFiles/20260615_align_inflight_scripts_default_to_array.sql @@ -0,0 +1,28 @@ +-- Align in_flight_scripts_json to the JSON ARRAY shape it actually carries. +-- +-- The column was created (20260417_add_per_target_checkpoint.sql) with DEFAULT +-- '{}'::jsonb and documented as the object form { "": "" }. +-- That object shape was abandoned: InFlightScriptMap serializes a List — +-- an ARRAY keyed by dispatch slot: +-- [{ "m": machineId, "s": stepId, "a": actionId, "t": scriptTicket }] +-- and its Parse() throws (caught → treated as empty) on an object, so '{}' only +-- ever worked via that catch fallback. +-- +-- This migration: +-- 1. Re-points the column DEFAULT to the empty array '[]'::jsonb so the default +-- matches the EF model (HasDefaultValue("[]")) and the EnsureExistsAsync seed. +-- 2. Rewrites EXISTING empty '{}' rows to '[]'. An empty object means "no +-- in-flight scripts" — identical in meaning to '[]' — so this is a pure +-- shape normalization with no behavioural change. Non-empty legacy object +-- rows (if any) are left untouched and remain handled by InFlightScriptMap's +-- Parse() catch (re-dispatch fresh), exactly as before. +-- +-- Non-breaking: InFlightScriptMap tolerates any leftover legacy shape, and EF +-- always supplies an explicit value on insert, so the DB default is belt-and-braces. + +ALTER TABLE deployment_execution_checkpoint + ALTER COLUMN in_flight_scripts_json SET DEFAULT '[]'::jsonb; + +UPDATE deployment_execution_checkpoint + SET in_flight_scripts_json = '[]'::jsonb + WHERE in_flight_scripts_json = '{}'::jsonb; diff --git a/src/Squid.Core/Persistence/Entities/Deployments/DeploymentExecutionCheckpoint.cs b/src/Squid.Core/Persistence/Entities/Deployments/DeploymentExecutionCheckpoint.cs index c361bc1e..635dc400 100644 --- a/src/Squid.Core/Persistence/Entities/Deployments/DeploymentExecutionCheckpoint.cs +++ b/src/Squid.Core/Persistence/Entities/Deployments/DeploymentExecutionCheckpoint.cs @@ -20,10 +20,12 @@ public class DeploymentExecutionCheckpoint : IEntity, IAuditable /// /// Tickets of scripts that were dispatched to agents but whose completion was /// not yet observed. On resume, the server can probe the agent with the same - /// ticket rather than launching a duplicate script. JSON shape: - /// { "<machineId>": "<scriptTicket>" } + /// ticket rather than launching a duplicate script. JSON shape is the array + /// InFlightScriptMap emits, keyed by dispatch slot (machine + step + action): + /// [{ "m": machineId, "s": stepId, "a": actionId, "t": scriptTicket }]. + /// Empty is the array form []. /// - public string InFlightScriptsJson { get; set; } = "{}"; + public string InFlightScriptsJson { get; set; } = "[]"; public DateTimeOffset CreatedDate { get; set; } public int CreatedBy { get; set; } diff --git a/src/Squid.Core/Persistence/EntityConfigurations/DeploymentExecutionCheckpointConfiguration.cs b/src/Squid.Core/Persistence/EntityConfigurations/DeploymentExecutionCheckpointConfiguration.cs index cb255849..752f141f 100644 --- a/src/Squid.Core/Persistence/EntityConfigurations/DeploymentExecutionCheckpointConfiguration.cs +++ b/src/Squid.Core/Persistence/EntityConfigurations/DeploymentExecutionCheckpointConfiguration.cs @@ -14,6 +14,7 @@ public void Configure(EntityTypeBuilder builder) builder.HasIndex(p => p.ServerTaskId).IsUnique(); builder.Property(p => p.BatchStatesJson).HasColumnType("jsonb").HasDefaultValue("{}"); - builder.Property(p => p.InFlightScriptsJson).HasColumnType("jsonb").HasDefaultValue("{}"); + // InFlightScriptsJson is a JSON ARRAY (InFlightScriptMap serializes List); empty = "[]". + builder.Property(p => p.InFlightScriptsJson).HasColumnType("jsonb").HasDefaultValue("[]"); } } diff --git a/src/Squid.Core/Services/Deployments/Checkpoints/DeploymentCheckpointService.cs b/src/Squid.Core/Services/Deployments/Checkpoints/DeploymentCheckpointService.cs index 4b26c206..7177b40e 100644 --- a/src/Squid.Core/Services/Deployments/Checkpoints/DeploymentCheckpointService.cs +++ b/src/Squid.Core/Services/Deployments/Checkpoints/DeploymentCheckpointService.cs @@ -65,7 +65,7 @@ await repository.InsertAsync(new DeploymentExecutionCheckpoint FailureEncountered = false, OutputVariablesJson = "[]", BatchStatesJson = "{}", - InFlightScriptsJson = "{}" + InFlightScriptsJson = "[]" // array shape — matches what InFlightScriptMap emits }, ct).ConfigureAwait(false); await unitOfWork.SaveChangesAsync(ct).ConfigureAwait(false); diff --git a/tests/Squid.IntegrationTests/Services/Deployments/Checkpoints/InFlightScriptStoreTests.cs b/tests/Squid.IntegrationTests/Services/Deployments/Checkpoints/InFlightScriptStoreTests.cs index 3376e8c8..95d10dd1 100644 --- a/tests/Squid.IntegrationTests/Services/Deployments/Checkpoints/InFlightScriptStoreTests.cs +++ b/tests/Squid.IntegrationTests/Services/Deployments/Checkpoints/InFlightScriptStoreTests.cs @@ -275,16 +275,32 @@ public async Task IsMachineBusy_NoCheckpointForMachine_ReturnsFalse() [Fact] public async Task IsMachineBusy_CheckpointSeededButNeverDispatched_ReturnsFalse() { - // A checkpoint that EXISTS but has not (yet) dispatched any script carries the idle - // seed value "{}" (EnsureExistsAsync). It must report NOT busy — and the scan predicate - // now excludes BOTH idle shapes ("[]" and "{}") at the DB layer, so such rows are never - // even materialized into the busy-check. + // A checkpoint that EXISTS but has not (yet) dispatched any script carries the empty-array + // seed "[]" (EnsureExistsAsync). It must report NOT busy — the scan predicate excludes the + // empty array at the DB layer (and still excludes the legacy "{}" defensively), so such rows + // are never even materialized into the busy-check. const int taskId = 700024; const int machineId = 9106; await EnsureRowAsync(taskId).ConfigureAwait(false); (await IsBusyAsync(machineId).ConfigureAwait(false)).ShouldBeFalse( - customMessage: "A seeded-but-never-dispatched checkpoint ('{}') must not report any machine busy."); + customMessage: "A seeded-but-never-dispatched checkpoint ('[]') must not report any machine busy."); + } + + [Fact] + public async Task EnsureExists_SeedsInFlightScriptsJson_AsEmptyArray() + { + // Discriminating pin for the data-shape alignment: EnsureExistsAsync must seed the in-flight + // column as the empty ARRAY "[]" that InFlightScriptMap emits — NOT the legacy object "{}". + // This makes the IsMachineBusyAsync pre-filter ("!= []") exact for fresh checkpoints rather + // than relying on the parse-fallback for "{}". + const int taskId = 700031; + await EnsureRowAsync(taskId).ConfigureAwait(false); + + var row = await LoadAsync(taskId).ConfigureAwait(false); + + row.InFlightScriptsJson.ShouldBe("[]", + customMessage: "the seed must be the empty array shape '[]' matching InFlightScriptMap, not the legacy object '{}'."); } [Fact] From 658006945e286be42a246e3ba01e3ff9bfbda16d Mon Sep 17 00:00:00 2001 From: "Mars.P" Date: Mon, 15 Jun 2026 13:00:29 +0800 Subject: [PATCH 2/2] Cover the migration UPDATE path + fix stale IsMachineBusyAsync comment (review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adversarial review of #447 surfaced two fix-optional items, both addressed: - The migration's UPDATE ('{}' -> '[]') was untested: the integration harness migrates a FRESH (empty) DB, so the UPDATE is a no-op there. Add an integration test that seeds BOTH legacy shapes (empty '{}' and a non-empty {machineId:ticket} object) then runs the same UPDATE, asserting the empty one normalizes to '[]' and the non-empty one is NOT clobbered (pins the WHERE = '{}' selectivity the migration comment promises). - Update the IsMachineBusyAsync comment, which still claimed '{}' was the seed / column default — it is now '[]'; '{}' is the legacy shape kept defensively until the 20260615 migration normalizes it. InFlightScriptStore (17) integration tests green. --- .../Checkpoints/InFlightScriptStore.cs | 12 ++--- .../Checkpoints/InFlightScriptStoreTests.cs | 45 +++++++++++++++++++ 2 files changed, 51 insertions(+), 6 deletions(-) diff --git a/src/Squid.Core/Services/Deployments/Checkpoints/InFlightScriptStore.cs b/src/Squid.Core/Services/Deployments/Checkpoints/InFlightScriptStore.cs index 69d869dc..58e5a18b 100644 --- a/src/Squid.Core/Services/Deployments/Checkpoints/InFlightScriptStore.cs +++ b/src/Squid.Core/Services/Deployments/Checkpoints/InFlightScriptStore.cs @@ -118,12 +118,12 @@ public async Task IsMachineBusyAsync(int machineId, CancellationToken canc // checkpoint is deleted, so the scanned set is just the active/paused deployments. No per-task // stripe: this is a stand-alone read on the caller's own (upgrade) scope, not the deploy worker's. // - // Both IDLE shapes are excluded at the DB layer: "[]" (an array emptied after add-then-remove) - // AND "{}" (the value EnsureExistsAsync seeds + the column default) — a checkpoint that exists - // but has never dispatched, or is between batches, carries "{}". Filtering both keeps the scan - // tight to checkpoints that genuinely hold an in-flight entry. (ContainsMachine would return - // false for either idle shape anyway via its parse-fallback, so this is a narrowing, not a - // correctness fix.) + // Both IDLE shapes are excluded at the DB layer: "[]" (the current seed + column default, and an + // array emptied after add-then-remove) AND "{}" (the LEGACY object default, kept defensively for + // rows written by older servers / not yet normalized by 20260615_align_inflight_scripts_default_ + // to_array.sql). Filtering both keeps the scan tight to checkpoints that genuinely hold an + // in-flight entry. (ContainsMachine would return false for either idle shape anyway via its + // parse-fallback, so this is a narrowing, not a correctness fix.) var jsons = await repository.QueryNoTracking( c => c.InFlightScriptsJson != null && c.InFlightScriptsJson != "[]" && c.InFlightScriptsJson != "{}") .Select(c => c.InFlightScriptsJson) diff --git a/tests/Squid.IntegrationTests/Services/Deployments/Checkpoints/InFlightScriptStoreTests.cs b/tests/Squid.IntegrationTests/Services/Deployments/Checkpoints/InFlightScriptStoreTests.cs index 95d10dd1..cb99cddb 100644 --- a/tests/Squid.IntegrationTests/Services/Deployments/Checkpoints/InFlightScriptStoreTests.cs +++ b/tests/Squid.IntegrationTests/Services/Deployments/Checkpoints/InFlightScriptStoreTests.cs @@ -1,4 +1,6 @@ using System.Linq; +using Microsoft.EntityFrameworkCore; +using Squid.Core.Persistence.Db; using Squid.Core.Persistence.Entities.Deployments; using Squid.Core.Services.Deployments.Checkpoints; using Squid.IntegrationTests.Base; @@ -303,6 +305,49 @@ public async Task EnsureExists_SeedsInFlightScriptsJson_AsEmptyArray() customMessage: "the seed must be the empty array shape '[]' matching InFlightScriptMap, not the legacy object '{}'."); } + [Fact] + public async Task Migration_NormalizesEmptyLegacyObject_ButLeavesNonEmptyLegacyObjectUntouched() + { + // Pins the 20260615 migration's UPDATE clause: rewrite the empty legacy object '{}' to the + // array '[]', but NEVER clobber a NON-EMPTY legacy object (a pre-#433 {machineId:ticket} row that + // still carries data). The integration harness migrates a FRESH (empty) DB so the UPDATE is a + // no-op there — exercise it explicitly by seeding both legacy shapes, then running the same UPDATE. + const int emptyTaskId = 700040; + const int nonEmptyTaskId = 700041; + + await Run(async (repo, uow) => + { + await repo.InsertAsync(NewCheckpoint(emptyTaskId, "{}")).ConfigureAwait(false); + await repo.InsertAsync(NewCheckpoint(nonEmptyTaskId, "{\"11\":\"legacy-ticket\"}")).ConfigureAwait(false); + await uow.SaveChangesAsync().ConfigureAwait(false); + }).ConfigureAwait(false); + + // Mirrors the UPDATE in 20260615_align_inflight_scripts_default_to_array.sql. Parameterized + // ({0}/{1} are bound params, the [] / {} values pass as text cast to jsonb) — semantically the + // same as the migration's literal '[]'::jsonb / '{}'::jsonb, but avoids ExecuteSqlRaw treating + // the literal braces as String.Format placeholders. + await Run(ctx => ctx.Database.ExecuteSqlRawAsync( + "UPDATE deployment_execution_checkpoint SET in_flight_scripts_json = {0}::jsonb WHERE in_flight_scripts_json = {1}::jsonb", + "[]", "{}")).ConfigureAwait(false); + + (await LoadAsync(emptyTaskId).ConfigureAwait(false)).InFlightScriptsJson.ShouldBe("[]", + customMessage: "the migration must normalize an empty legacy '{}' to the array '[]'."); + + (await LoadAsync(nonEmptyTaskId).ConfigureAwait(false)).InFlightScriptsJson.ShouldContain("legacy-ticket", + customMessage: "the migration's WHERE = '{}' must NOT clobber a non-empty legacy object row."); + } + + private static DeploymentExecutionCheckpoint NewCheckpoint(int serverTaskId, string inFlightScriptsJson) => new() + { + ServerTaskId = serverTaskId, + DeploymentId = 1, + LastCompletedBatchIndex = -1, + FailureEncountered = false, + OutputVariablesJson = "[]", + BatchStatesJson = "{}", + InFlightScriptsJson = inFlightScriptsJson + }; + [Fact] public async Task DependencyInjection_ResolvesRealInFlightScriptStore_SoTheUpgradeDeferGuardStaysWired() {