Remove policy change action acks

Author: michel-laterman

internal/pkg/agent/application/actions/handlers/handler_action_policy_change.go

@@ -473,7 +473,6 @@ type policyChange struct {
 	cfg        *config.Config
 	action     fleetapi.Action
 	acker      acker.Acker
-	commit     bool
 	ackWatcher chan struct{}
 }
 
@@ -482,9 +481,9 @@ func newPolicyChange(
 	config *config.Config,
 	action fleetapi.Action,
 	acker acker.Acker,
-	commit bool) *policyChange {
+	makeCh bool) *policyChange {
 	var ackWatcher chan struct{}
-	if commit {
+	if makeCh {
 		// we don't need it otherwise
 		ackWatcher = make(chan struct{})
 	}
@@ -493,7 +492,6 @@ func newPolicyChange(
 		cfg:        config,
 		action:     action,
 		acker:      acker,
-		commit:     true,
 		ackWatcher: ackWatcher,
 	}
 }
@@ -502,30 +500,29 @@ func (l *policyChange) Config() *config.Config {
 	return l.cfg
 }
 
+// Ack sends an ack for the associated action if the results are expected.
+// An ack will not be sent for a POLICY_CHANGE action, but will be when this method is used by UNENROLL actions.
 func (l *policyChange) Ack() error {
-	if l.action == nil {
+	if l.action == nil || l.ackWatcher == nil {
 		return nil
 	}
 	err := l.acker.Ack(l.ctx, l.action)
 	if err != nil {
 		return err
 	}
-	if l.commit {
-		err := l.acker.Commit(l.ctx)
-		if l.ackWatcher != nil && err == nil {
-			close(l.ackWatcher)
-		}
-		return err
+	err = l.acker.Commit(l.ctx)
+	if err == nil {
+		close(l.ackWatcher)
 	}
-	return nil
+	return err
 }
 
 // WaitAck waits for policy change to be acked.
 // Policy change ack is awaitable only in case commit flag was set.
 // Caller is responsible to use any reasonable deadline otherwise
 // function call can be endlessly blocking.
 func (l *policyChange) WaitAck(ctx context.Context) {
-	if !l.commit || l.ackWatcher == nil {
+	if l.ackWatcher == nil {
 		return
 	}
 

internal/pkg/agent/application/actions/handlers/handler_action_policy_change_test.go

@@ -105,7 +105,7 @@ func TestPolicyAcked(t *testing.T) {
 	agentInfo := &info.AgentInfo{}
 	nullStore := &storage.NullStore{}
 
-	t.Run("Config change should ACK", func(t *testing.T) {
+	t.Run("Config change shouldn't ACK", func(t *testing.T) {
 		ch := make(chan coordinator.ConfigChange, 1)
 		tacker := &testAcker{}
 
@@ -129,8 +129,7 @@ func TestPolicyAcked(t *testing.T) {
 		require.NoError(t, change.Ack())
 
 		actions := tacker.Items()
-		assert.EqualValues(t, 1, len(actions))
-		assert.Equal(t, actionID, actions[0])
+		assert.Empty(t, actions)
 	})
 }
 

internal/pkg/agent/application/coordinator/coordinator.go

@@ -1463,23 +1463,7 @@ func (c *Coordinator) runLoopIteration(ctx context.Context) {
 			c.logger.Errorf("applying new policy: %s", err.Error())
 			change.Fail(err)
 		} else {
-			if err := change.Ack(); err != nil {
-				err = fmt.Errorf("failed to ack configuration change: %w", err)
-				// Workaround: setConfigManagerError is usually used by the config
-				// manager to report failed ACKs / etc when communicating with Fleet.
-				// We need to report a failed ACK here, but the policy change has
-				// already been successfully applied so we don't want to report it as
-				// a general Coordinator or policy failure.
-				// This arises uniquely here because this is the only case where an
-				// action is responsible for reporting the failure of its own ACK
-				// call. The "correct" fix is to make this Ack() call unfailable
-				// and handle ACK retries and reporting in the config manager like
-				// with other action types -- this error would then end up invoking
-				// setConfigManagerError "organically" via the config manager's
-				// reporting channel. In the meantime, we do it manually.
-				c.setConfigManagerError(err)
-				c.logger.Errorf("%s", err.Error())
-			}
+			_ = change.Ack() // will not return errors
 		}
 
 	case vars := <-c.managerChans.varsManagerUpdate: