diff --git a/internal/dockeragent/agent.go b/internal/dockeragent/agent.go index 45c23ca61..0a0beb72e 100644 --- a/internal/dockeragent/agent.go +++ b/internal/dockeragent/agent.go @@ -939,9 +939,16 @@ func (a *Agent) handleCheckUpdatesCommand(ctx context.Context, target TargetConf a.registryChecker.ForceCheck() } - // Send intermediate completion ack + // Send intermediate completion ack. Don't propagate the error — the + // report was already delivered successfully. Propagating causes the + // report to be buffered and retried, which re-fetches the same command + // and creates a loop (issue #1504). if err := a.sendCommandAck(ctx, target, command.ID, agentsdocker.CommandStatusCompleted, "Registry cache cleared; checking for updates on next report cycle"); err != nil { - return fmt.Errorf("send check updates acknowledgement: %w", err) + a.logger.Warn(). + Err(err). + Str("commandID", command.ID). + Str("target", target.URL). + Msg("Failed to send check updates acknowledgement") } // Trigger an immediate collection cycle to report updates. diff --git a/internal/dockeragent/agent_http_test.go b/internal/dockeragent/agent_http_test.go index 6e10cd59d..aef7b3655 100644 --- a/internal/dockeragent/agent_http_test.go +++ b/internal/dockeragent/agent_http_test.go @@ -600,7 +600,7 @@ func TestHandleCommand(t *testing.T) { } }) - t.Run("check updates command ack error", func(t *testing.T) { + t.Run("check updates command ack error does not propagate", func(t *testing.T) { registryChecker := NewRegistryChecker(zerolog.Nop()) registryChecker.MarkChecked() registryChecker.cacheDigest("cached-key", "sha256:cached") @@ -610,13 +610,14 @@ func TestHandleCommand(t *testing.T) { hostID: "host1", registryChecker: registryChecker, } + t.Cleanup(func() { _ = agent.Close() }) err := agent.handleCheckUpdatesCommand(context.Background(), TargetConfig{URL: "http://example.com/\x7f", Token: "token"}, agentsdocker.Command{ ID: "cmd4", Type: agentsdocker.CommandTypeCheckUpdates, }) - if err == nil || !strings.Contains(err.Error(), "send check updates acknowledgement") { - t.Fatalf("expected check-updates ack error, got %v", err) + if err != nil { + t.Fatalf("expected nil error on ack failure, got: %v", err) } registryChecker.mu.RLock() diff --git a/internal/monitoring/docker_commands.go b/internal/monitoring/docker_commands.go index 87f10fb8a..856a02fb2 100644 --- a/internal/monitoring/docker_commands.go +++ b/internal/monitoring/docker_commands.go @@ -400,12 +400,17 @@ func (m *Monitor) getDockerCommandPayload(hostID string) (map[string]any, *model return nil, nil } - // Update dispatch metadata - if cmd.status.Status == DockerCommandStatusQueued { - cmd.markDispatched() - m.state.SetDockerHostCommand(hostID, &cmd.status) + // Only return the command on the first dispatch (queued → dispatched). + // Re-sending an already-dispatched command causes the agent to re-execute + // it on every report cycle until the ack succeeds, creating an infinite + // loop when the ack HTTP call fails (issue #1504). + if cmd.status.Status != DockerCommandStatusQueued { + return nil, nil } + cmd.markDispatched() + m.state.SetDockerHostCommand(hostID, &cmd.status) + statusCopy := cmd.status return cmd.payload, &statusCopy } diff --git a/internal/monitoring/docker_commands_test.go b/internal/monitoring/docker_commands_test.go index 320f74cb0..355823118 100644 --- a/internal/monitoring/docker_commands_test.go +++ b/internal/monitoring/docker_commands_test.go @@ -891,7 +891,7 @@ func TestGetDockerCommandPayload(t *testing.T) { } }) - t.Run("already dispatched command is not re-marked", func(t *testing.T) { + t.Run("already dispatched command is not re-sent", func(t *testing.T) { t.Parallel() monitor := newTestMonitorForCommands(t) @@ -910,39 +910,24 @@ func TestGetDockerCommandPayload(t *testing.T) { t.Fatalf("queue stop command: %v", err) } - // First fetch - marks as dispatched + // First fetch - marks as dispatched and returns status _, status1 := monitor.getDockerCommandPayload(host.ID) if status1 == nil { t.Fatal("expected status from first fetch") } - firstDispatchedAt := status1.DispatchedAt - firstUpdatedAt := status1.UpdatedAt - - // Small delay to ensure time would change if re-marked - time.Sleep(time.Millisecond) - - // Second fetch - should NOT re-mark - _, status2 := monitor.getDockerCommandPayload(host.ID) - if status2 == nil { - t.Fatal("expected status from second fetch") + if status1.Status != DockerCommandStatusDispatched { + t.Fatalf("expected dispatched status, got %q", status1.Status) } - // DispatchedAt should be the same - if status2.DispatchedAt == nil { - t.Fatal("expected DispatchedAt to still be set") + // Second fetch - should return nil. The command was already delivered + // in the first dispatch response; re-sending it causes the agent to + // re-execute the command on every report cycle (issue #1504). + payload2, status2 := monitor.getDockerCommandPayload(host.ID) + if payload2 != nil { + t.Fatalf("expected nil payload from second fetch, got %v", payload2) } - if !status2.DispatchedAt.Equal(*firstDispatchedAt) { - t.Fatalf("DispatchedAt changed: first=%v, second=%v", *firstDispatchedAt, *status2.DispatchedAt) - } - - // UpdatedAt should be the same (not re-updated) - if !status2.UpdatedAt.Equal(firstUpdatedAt) { - t.Fatalf("UpdatedAt changed: first=%v, second=%v", firstUpdatedAt, status2.UpdatedAt) - } - - // Status should still be dispatched - if status2.Status != DockerCommandStatusDispatched { - t.Fatalf("expected dispatched status, got %q", status2.Status) + if status2 != nil { + t.Fatalf("expected nil status from second fetch, got %+v", status2) } })