fix(mesh): serve the artifact regardless of deployment status (Phase 3 live-gate deadlock)
The Phase 3 live gate deadlocked every FetchAndCache deploy: DeploymentArtifactGrpcService gated on Status==Sealed, but central seals a deployment only AFTER every node acks Applied, and a FetchAndCache node cannot ack until it has fetched the artifact — seal-needs-ack -> ack-needs-fetch -> fetch-needs-sealed. The node's fetch reached central and passed the shared-key interceptor, then got a clean NotFound; the #485 apply-failure path correctly kept last-known-good, but the deploy could never seal. Direct mode reads the same ArtifactBlob from SQL while the row is still AwaitingApplyAcks, so the serve path must too. Drop the Sealed gate: serve any deployment whose blob is non-empty (unknown id / empty blob still collapse to NotFound — existence-hiding + #485). Access is already gated by the interceptor, so there is no reason to hide a non-sealed deployment a node is legitimately applying. The Task 2 'non-sealed -> NotFound' test flips to 'non-sealed with a blob is served'. Claude-Session: https://claude.ai/code/session_01GASWkNEi68FSCtvr6rLoEW
This commit is contained in:
@@ -4,7 +4,6 @@ using Microsoft.EntityFrameworkCore;
|
||||
using Microsoft.Extensions.Logging;
|
||||
using ZB.MOM.WW.OtOpcUa.Commons.Protos.DeploymentArtifact.V1;
|
||||
using ZB.MOM.WW.OtOpcUa.Configuration;
|
||||
using ZB.MOM.WW.OtOpcUa.Configuration.Enums;
|
||||
// The generated service container is itself named DeploymentArtifactService; alias its nested server
|
||||
// base so this impl can keep the natural name without colliding with the generated type.
|
||||
using GeneratedServiceBase =
|
||||
@@ -13,17 +12,27 @@ using GeneratedServiceBase =
|
||||
namespace ZB.MOM.WW.OtOpcUa.AdminUI.Grpc;
|
||||
|
||||
/// <summary>
|
||||
/// Central-side artifact serve (per-cluster mesh Phase 3). Streams a sealed deployment's
|
||||
/// Central-side artifact serve (per-cluster mesh Phase 3). Streams a deployment's
|
||||
/// <c>ArtifactBlob</c> to a driver node fetching in <c>FetchAndCache</c> mode. Registered only on
|
||||
/// admin-role nodes (which hold the central SQL connection) and only when
|
||||
/// <c>ConfigServe:GrpcListenPort > 0</c>.
|
||||
/// </summary>
|
||||
/// <remarks>
|
||||
/// <para>
|
||||
/// <b>NotFound is deliberately indistinguishable across three cases</b> — unknown id, a
|
||||
/// deployment that is not sealed, and an empty blob. The first is existence-hiding; the last
|
||||
/// is the #485 rule (a zero-length artifact is "no answer," never "a configuration with no
|
||||
/// drivers"). Streaming empty bytes as if valid is exactly what that defect class forbids.
|
||||
/// <b>Serves any deployment that has a non-empty blob — status is NOT gated.</b> A node
|
||||
/// fetches the artifact <em>during apply</em>, before the deployment is sealed: central seals
|
||||
/// only once every node has acked Applied, and a <c>FetchAndCache</c> node cannot ack until it
|
||||
/// has fetched. Gating on <c>Sealed</c> would deadlock every such deploy (seal-needs-ack →
|
||||
/// ack-needs-fetch → fetch-needs-sealed), and it matches Direct mode, which reads the same
|
||||
/// <c>ArtifactBlob</c> from SQL while the row is still <c>AwaitingApplyAcks</c>. (Caught on the
|
||||
/// Phase 3 live gate.) Access is already gated by the shared-key interceptor, so there is no
|
||||
/// reason to hide a non-sealed deployment.
|
||||
/// </para>
|
||||
/// <para>
|
||||
/// <b>NotFound is indistinguishable across two cases</b> — unknown id and an empty blob. The
|
||||
/// first is existence-hiding; the second is the #485 rule (a zero-length artifact is "no
|
||||
/// answer," never "a configuration with no drivers"). Streaming empty bytes as if valid is
|
||||
/// exactly what that defect class forbids.
|
||||
/// </para>
|
||||
/// </remarks>
|
||||
public sealed class DeploymentArtifactGrpcService : GeneratedServiceBase
|
||||
@@ -62,17 +71,16 @@ public sealed class DeploymentArtifactGrpcService : GeneratedServiceBase
|
||||
var row = await db.Deployments
|
||||
.AsNoTracking()
|
||||
.Where(d => d.DeploymentId == id)
|
||||
.Select(d => new { d.Status, d.ArtifactBlob })
|
||||
.Select(d => new { d.ArtifactBlob })
|
||||
.FirstOrDefaultAsync(context.CancellationToken);
|
||||
|
||||
// Existence-hiding + #485: unknown / not-sealed / empty collapse to one NotFound.
|
||||
if (row is null || row.Status != DeploymentStatus.Sealed || row.ArtifactBlob.Length == 0)
|
||||
// Existence-hiding + #485: unknown id / empty blob collapse to one NotFound. Status is NOT
|
||||
// gated — a node fetches during apply, before the deployment is sealed (see the class remarks).
|
||||
if (row is null || row.ArtifactBlob.Length == 0)
|
||||
{
|
||||
_log.LogDebug(
|
||||
"Artifact fetch for {DeploymentId} -> NotFound (found={Found}, sealed={Sealed}, bytes={Bytes})",
|
||||
request.DeploymentId, row is not null,
|
||||
row is not null && row.Status == DeploymentStatus.Sealed,
|
||||
row?.ArtifactBlob.Length ?? 0);
|
||||
"Artifact fetch for {DeploymentId} -> NotFound (found={Found}, bytes={Bytes})",
|
||||
request.DeploymentId, row is not null, row?.ArtifactBlob.Length ?? 0);
|
||||
throw new RpcException(new Status(StatusCode.NotFound, "unknown deployment"));
|
||||
}
|
||||
|
||||
|
||||
+11
-6
@@ -73,16 +73,21 @@ public class DeploymentArtifactServiceTests
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task A_non_sealed_deployment_throws_NotFound()
|
||||
public async Task A_non_sealed_deployment_with_a_blob_is_served()
|
||||
{
|
||||
// A node fetches DURING apply, before the deployment is sealed (seal-needs-ack →
|
||||
// ack-needs-fetch). Gating on Sealed would deadlock every FetchAndCache deploy. So a
|
||||
// non-sealed deployment that has a blob must stream — matching Direct mode reading the same
|
||||
// AwaitingApplyAcks row from SQL. (Regression for the Phase 3 live-gate deadlock.)
|
||||
var dbName = $"artsvc-{Guid.NewGuid():N}";
|
||||
var id = Seed(dbName, DeploymentStatus.AwaitingApplyAcks, [1, 2, 3]);
|
||||
var blob = new byte[] { 1, 2, 3 };
|
||||
var id = Seed(dbName, DeploymentStatus.AwaitingApplyAcks, blob);
|
||||
|
||||
var ex = await Should.ThrowAsync<RpcException>(() => Service(dbName).Fetch(
|
||||
new FetchRequest { DeploymentId = id.ToString() },
|
||||
new CollectingStreamWriter(), new FakeServerCallContext()));
|
||||
var writer = new CollectingStreamWriter();
|
||||
await Service(dbName).Fetch(
|
||||
new FetchRequest { DeploymentId = id.ToString() }, writer, new FakeServerCallContext());
|
||||
|
||||
ex.StatusCode.ShouldBe(StatusCode.NotFound);
|
||||
writer.Reassembled().ShouldBe(blob);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
|
||||
Reference in New Issue
Block a user