From 647a9545bdab023ee7ec900883518625851d813b Mon Sep 17 00:00:00 2001 From: Javier Rodriguez Date: Wed, 8 Apr 2026 11:19:21 +0200 Subject: [PATCH] fix(cli): always reload state after collectors to prevent digest mismatch with old servers Signed-off-by: Javier Rodriguez --- pkg/attestation/crafter/crafter.go | 16 ++++----- pkg/attestation/crafter/runcollectors_test.go | 33 ++++++++++++++++--- 2 files changed, 34 insertions(+), 15 deletions(-) diff --git a/pkg/attestation/crafter/crafter.go b/pkg/attestation/crafter/crafter.go index f8f6bb8b3..f7c215aa0 100644 --- a/pkg/attestation/crafter/crafter.go +++ b/pkg/attestation/crafter/crafter.go @@ -147,22 +147,18 @@ func (c *Crafter) RunCollectors(ctx context.Context, attestationID string, casBa return } - digestBeforeCollectors := c.CraftingState.UpdateCheckSum - for _, collector := range c.collectors { if err := collector.Collect(ctx, c, attestationID, casBackend); err != nil { c.Logger.Warn().Err(err).Str("collector", collector.ID()).Msg("collector failed") } } - // NOTE: workaround for old servers that don't return digests in Save responses. - // not returning digest makes digests stale, and state save failing with conflict errors - // https://github.com/chainloop-dev/chainloop/issues/2908 - // this condition will not apply to new servers that return digests in Save responses. - if c.CraftingState.UpdateCheckSum != digestBeforeCollectors { - if err := c.LoadCraftingState(ctx, attestationID); err != nil { - c.Logger.Warn().Err(err).Msg("failed to reload crafting state after running collectors") - } + // Always reload state after collectors to ensure the digest is in sync with the server. + // Collectors persist changes via Write(), but old servers don't return the new digest + // in Save responses, leaving the client-side UpdateCheckSum stale. + // Reloading via Read() fetches the authoritative digest regardless of server version. + if err := c.LoadCraftingState(ctx, attestationID); err != nil { + c.Logger.Warn().Err(err).Msg("failed to reload crafting state after running collectors") } } diff --git a/pkg/attestation/crafter/runcollectors_test.go b/pkg/attestation/crafter/runcollectors_test.go index 8066e4e04..630d84c2f 100644 --- a/pkg/attestation/crafter/runcollectors_test.go +++ b/pkg/attestation/crafter/runcollectors_test.go @@ -57,7 +57,7 @@ func newSuccessCollector(t *testing.T, id string) *craftermocks.Collector { } func TestRunCollectors(t *testing.T) { - t.Run("reloads state after collectors when digest changes", func(t *testing.T) { + t.Run("always reloads state after collectors", func(t *testing.T) { sm := craftermocks.NewStateManager(t) setupReadExpectation(sm, "digest-1") sm.On("Info", mock.Anything, mock.Anything).Return("mock://run-1") @@ -77,7 +77,7 @@ func TestRunCollectors(t *testing.T) { cr.RunCollectors(context.Background(), "run-1", nil) - // Read before + Read after (digest changed, so reload triggered) + // Read before collectors + unconditional Read after collectors sm.AssertNumberOfCalls(t, "Read", 2) c1.AssertCalled(t, "Collect", mock.Anything, mock.Anything, "run-1", mock.Anything) }) @@ -102,9 +102,10 @@ func TestRunCollectors(t *testing.T) { c1.AssertCalled(t, "Collect", mock.Anything, mock.Anything, mock.Anything, mock.Anything) c2.AssertCalled(t, "Collect", mock.Anything, mock.Anything, mock.Anything, mock.Anything) + sm.AssertNumberOfCalls(t, "Read", 2) }) - t.Run("no collectors skips reload when digest unchanged", func(t *testing.T) { + t.Run("reloads state even with no collectors", func(t *testing.T) { sm := craftermocks.NewStateManager(t) setupReadExpectation(sm, "d") sm.On("Info", mock.Anything, mock.Anything).Return("mock://run-1") @@ -114,8 +115,30 @@ func TestRunCollectors(t *testing.T) { cr.RunCollectors(context.Background(), "run-1", nil) - // Only the initial Read, no reload since digest didn't change - sm.AssertNumberOfCalls(t, "Read", 1) + // Read before + unconditional Read after, even with no collectors + sm.AssertNumberOfCalls(t, "Read", 2) + }) + + t.Run("reloads state when digest is stale (old server)", func(t *testing.T) { + sm := craftermocks.NewStateManager(t) + setupReadExpectation(sm, "digest-1") + sm.On("Info", mock.Anything, mock.Anything).Return("mock://run-1") + + // Collector persists state via Write but the old server doesn't return + // a new digest, so UpdateCheckSum stays unchanged (stale). + c1 := craftermocks.NewCollector(t) + c1.On("ID").Maybe().Return("c1") + c1.On("Collect", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil) + + cr, err := crafter.NewCrafter(sm, nil) + require.NoError(t, err) + cr.RegisterCollectors(c1) + + cr.RunCollectors(context.Background(), "run-1", nil) + + // Unconditional reload ensures the stale digest is refreshed + sm.AssertNumberOfCalls(t, "Read", 2) + c1.AssertCalled(t, "Collect", mock.Anything, mock.Anything, "run-1", mock.Anything) }) t.Run("state load failure aborts before running collectors", func(t *testing.T) {