From 3a5c3ca848eb48b5a3bea30099cab3f933ab173c Mon Sep 17 00:00:00 2001 From: Sas Swart Date: Tue, 11 Nov 2025 10:12:09 +0000 Subject: [PATCH 01/14] feat(agent): add agent unit manager --- agent/unit/manager.go | 307 ++++++++++++++++ agent/unit/manager_test.go | 691 +++++++++++++++++++++++++++++++++++++ 2 files changed, 998 insertions(+) create mode 100644 agent/unit/manager.go create mode 100644 agent/unit/manager_test.go diff --git a/agent/unit/manager.go b/agent/unit/manager.go new file mode 100644 index 0000000000000..5b187eb805ce5 --- /dev/null +++ b/agent/unit/manager.go @@ -0,0 +1,307 @@ +package unit + +import ( + "sync" + + "golang.org/x/xerrors" +) + +// ErrConsumerNotFound is returned when a consumer ID is not registered. +var ErrConsumerNotFound = xerrors.New("consumer not found") + +// ErrConsumerAlreadyRegistered is returned when a consumer ID is already registered. +var ErrConsumerAlreadyRegistered = xerrors.New("consumer already registered") + +// ErrCannotUpdateOtherConsumer is returned when attempting to update another consumer's status. +var ErrCannotUpdateOtherConsumer = xerrors.New("cannot update other consumer's status") + +// ErrDependenciesNotSatisfied is returned when a consumer's dependencies are not satisfied. +var ErrDependenciesNotSatisfied = xerrors.New("unit dependencies not satisfied") + +// ErrSameStatusAlreadySet is returned when attempting to set the same status as the current status. +var ErrSameStatusAlreadySet = xerrors.New("same status already set") + +// Status constants for dependency tracking +const ( + StatusStarted = "started" + StatusComplete = "completed" +) + +// dependencyVertex represents a vertex in the dependency graph that is associated with a consumer. +type dependencyVertex[ConsumerID comparable] struct { + ID ConsumerID +} + +// Dependency represents a dependency relationship between consumers. +type Dependency[StatusType, ConsumerID comparable] struct { + Consumer ConsumerID + DependsOn ConsumerID + RequiredStatus StatusType + CurrentStatus StatusType + IsSatisfied bool +} + +// Manager provides reactive dependency tracking over a Graph. +// It manages consumer registration, dependency relationships, and status updates +// with automatic recalculation of readiness when dependencies are satisfied. +type Manager[StatusType, ConsumerID comparable] struct { + mu sync.RWMutex + + // The underlying graph that stores dependency relationships + graph *Graph[StatusType, *dependencyVertex[ConsumerID]] + + // Track current status of each consumer + consumerStatus map[ConsumerID]StatusType + + // Track readiness state (cached to avoid repeated graph traversal) + consumerReadiness map[ConsumerID]bool + + // Track which consumers are registered + registeredConsumers map[ConsumerID]bool + + // Store vertex instances for each consumer to ensure consistent references + consumerVertices map[ConsumerID]*dependencyVertex[ConsumerID] +} + +// NewManager creates a new Manager instance. +func NewManager[StatusType, ConsumerID comparable]() *Manager[StatusType, ConsumerID] { + return &Manager[StatusType, ConsumerID]{ + graph: &Graph[StatusType, *dependencyVertex[ConsumerID]]{}, + consumerStatus: make(map[ConsumerID]StatusType), + consumerReadiness: make(map[ConsumerID]bool), + registeredConsumers: make(map[ConsumerID]bool), + consumerVertices: make(map[ConsumerID]*dependencyVertex[ConsumerID]), + } +} + +// Register registers a new consumer as a vertex in the dependency graph. +func (dt *Manager[StatusType, ConsumerID]) Register(id ConsumerID) error { + dt.mu.Lock() + defer dt.mu.Unlock() + + if dt.registeredConsumers[id] { + return ErrConsumerAlreadyRegistered + } + + // Create and store the vertex for this consumer + vertex := &dependencyVertex[ConsumerID]{ID: id} + dt.consumerVertices[id] = vertex + dt.registeredConsumers[id] = true + dt.consumerReadiness[id] = true // New consumers start as ready (no dependencies) + + return nil +} + +// AddDependency adds a dependency relationship between consumers. +// The consumer depends on the dependsOn consumer reaching the requiredStatus. +func (dt *Manager[StatusType, ConsumerID]) AddDependency(consumer ConsumerID, dependsOn ConsumerID, requiredStatus StatusType) error { + dt.mu.Lock() + defer dt.mu.Unlock() + + if !dt.registeredConsumers[consumer] { + return xerrors.Errorf("consumer %v is not registered", consumer) + } + if !dt.registeredConsumers[dependsOn] { + return xerrors.Errorf("consumer %v is not registered", dependsOn) + } + + // Get the stored vertices for both consumers + consumerVertex := dt.consumerVertices[consumer] + dependsOnVertex := dt.consumerVertices[dependsOn] + + // Add the dependency edge to the graph + // The edge goes from consumer to dependsOn, representing the dependency + err := dt.graph.AddEdge(consumerVertex, dependsOnVertex, requiredStatus) + if err != nil { + return xerrors.Errorf("failed to add dependency: %w", err) + } + + // Recalculate readiness for the consumer since it now has a dependency + dt.recalculateReadinessUnsafe(consumer) + + return nil +} + +// UpdateStatus updates a consumer's status and recalculates readiness for affected dependents. +func (dt *Manager[StatusType, ConsumerID]) UpdateStatus(consumer ConsumerID, newStatus StatusType) error { + dt.mu.Lock() + defer dt.mu.Unlock() + + if !dt.registeredConsumers[consumer] { + return ErrConsumerNotFound + } + + // Update the consumer's status + if dt.consumerStatus[consumer] == newStatus { + return ErrSameStatusAlreadySet + } + dt.consumerStatus[consumer] = newStatus + + // Get all consumers that depend on this one (reverse adjacent vertices) + consumerVertex := dt.consumerVertices[consumer] + dependentEdges := dt.graph.GetReverseAdjacentVertices(consumerVertex) + + // Recalculate readiness for all dependents + for _, edge := range dependentEdges { + dt.recalculateReadinessUnsafe(edge.From.ID) + } + + return nil +} + +// IsReady checks if all dependencies for a consumer are satisfied. +func (dt *Manager[StatusType, ConsumerID]) IsReady(consumer ConsumerID) (bool, error) { + dt.mu.RLock() + defer dt.mu.RUnlock() + + if !dt.registeredConsumers[consumer] { + return false, ErrConsumerNotFound + } + + return dt.consumerReadiness[consumer], nil +} + +// GetUnmetDependencies returns a list of unsatisfied dependencies for a consumer. +func (dt *Manager[StatusType, ConsumerID]) GetUnmetDependencies(consumer ConsumerID) ([]Dependency[StatusType, ConsumerID], error) { + dt.mu.RLock() + defer dt.mu.RUnlock() + + if !dt.registeredConsumers[consumer] { + return nil, ErrConsumerNotFound + } + + consumerVertex := dt.consumerVertices[consumer] + forwardEdges := dt.graph.GetForwardAdjacentVertices(consumerVertex) + + var unmetDependencies []Dependency[StatusType, ConsumerID] + + for _, edge := range forwardEdges { + dependsOnConsumer := edge.To.ID + requiredStatus := edge.Edge + currentStatus, exists := dt.consumerStatus[dependsOnConsumer] + if !exists { + // If the dependency consumer has no status, it's not satisfied + var zeroStatus StatusType + unmetDependencies = append(unmetDependencies, Dependency[StatusType, ConsumerID]{ + Consumer: consumer, + DependsOn: dependsOnConsumer, + RequiredStatus: requiredStatus, + CurrentStatus: zeroStatus, // Zero value + IsSatisfied: false, + }) + } else { + isSatisfied := currentStatus == requiredStatus + if !isSatisfied { + unmetDependencies = append(unmetDependencies, Dependency[StatusType, ConsumerID]{ + Consumer: consumer, + DependsOn: dependsOnConsumer, + RequiredStatus: requiredStatus, + CurrentStatus: currentStatus, + IsSatisfied: false, + }) + } + } + } + + return unmetDependencies, nil +} + +// recalculateReadinessUnsafe recalculates the readiness state for a consumer. +// This method assumes the caller holds the write lock. +func (dt *Manager[StatusType, ConsumerID]) recalculateReadinessUnsafe(consumer ConsumerID) { + consumerVertex := dt.consumerVertices[consumer] + forwardEdges := dt.graph.GetForwardAdjacentVertices(consumerVertex) + + // If there are no dependencies, the consumer is ready + if len(forwardEdges) == 0 { + dt.consumerReadiness[consumer] = true + return + } + + // Check if all dependencies are satisfied + allSatisfied := true + for _, edge := range forwardEdges { + dependsOnConsumer := edge.To.ID + requiredStatus := edge.Edge + currentStatus, exists := dt.consumerStatus[dependsOnConsumer] + if !exists || currentStatus != requiredStatus { + allSatisfied = false + break + } + } + + dt.consumerReadiness[consumer] = allSatisfied +} + +// GetGraph returns the underlying graph for visualization and debugging. +// This should be used carefully as it exposes the internal graph structure. +func (dt *Manager[StatusType, ConsumerID]) GetGraph() *Graph[StatusType, *dependencyVertex[ConsumerID]] { + return dt.graph +} + +// GetStatus returns the current status of a consumer. +func (dt *Manager[StatusType, ConsumerID]) GetStatus(consumer ConsumerID) (StatusType, error) { + dt.mu.RLock() + defer dt.mu.RUnlock() + + if !dt.registeredConsumers[consumer] { + var zeroStatus StatusType + return zeroStatus, ErrConsumerNotFound + } + + status, exists := dt.consumerStatus[consumer] + if !exists { + var zeroStatus StatusType + return zeroStatus, nil + } + + return status, nil +} + +// GetAllDependencies returns all dependencies for a consumer, both satisfied and unsatisfied. +func (dt *Manager[StatusType, ConsumerID]) GetAllDependencies(consumer ConsumerID) ([]Dependency[StatusType, ConsumerID], error) { + dt.mu.RLock() + defer dt.mu.RUnlock() + + if !dt.registeredConsumers[consumer] { + return nil, ErrConsumerNotFound + } + + consumerVertex := dt.consumerVertices[consumer] + forwardEdges := dt.graph.GetForwardAdjacentVertices(consumerVertex) + + var allDependencies []Dependency[StatusType, ConsumerID] + + for _, edge := range forwardEdges { + dependsOnConsumer := edge.To.ID + requiredStatus := edge.Edge + currentStatus, exists := dt.consumerStatus[dependsOnConsumer] + if !exists { + // If the dependency consumer has no status, it's not satisfied + var zeroStatus StatusType + allDependencies = append(allDependencies, Dependency[StatusType, ConsumerID]{ + Consumer: consumer, + DependsOn: dependsOnConsumer, + RequiredStatus: requiredStatus, + CurrentStatus: zeroStatus, // Zero value + IsSatisfied: false, + }) + } else { + isSatisfied := currentStatus == requiredStatus + allDependencies = append(allDependencies, Dependency[StatusType, ConsumerID]{ + Consumer: consumer, + DependsOn: dependsOnConsumer, + RequiredStatus: requiredStatus, + CurrentStatus: currentStatus, + IsSatisfied: isSatisfied, + }) + } + } + + return allDependencies, nil +} + +// ExportDOT exports the dependency graph to DOT format for visualization. +func (dt *Manager[StatusType, ConsumerID]) ExportDOT(name string) (string, error) { + return dt.graph.ToDOT(name) +} diff --git a/agent/unit/manager_test.go b/agent/unit/manager_test.go new file mode 100644 index 0000000000000..d11de47c88ea5 --- /dev/null +++ b/agent/unit/manager_test.go @@ -0,0 +1,691 @@ +package unit_test + +import ( + "sync" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/coder/coder/v2/agent/unit" +) + +type testStatus string + +const ( + statusStarted testStatus = "started" + statusRunning testStatus = "running" + statusCompleted testStatus = "completed" +) + +type testConsumerID string + +const ( + consumerA testConsumerID = "serviceA" + consumerB testConsumerID = "serviceB" + consumerC testConsumerID = "serviceC" + consumerD testConsumerID = "serviceD" +) + +func TestDependencyTracker_Register(t *testing.T) { + t.Parallel() + + tracker := unit.NewManager[testStatus, testConsumerID]() + + t.Run("RegisterNewConsumer", func(t *testing.T) { + t.Parallel() + + err := tracker.Register(consumerA) + require.NoError(t, err) + + // Consumer should be ready initially (no dependencies) + ready, err := tracker.IsReady(consumerA) + require.NoError(t, err) + assert.True(t, ready) + }) + + t.Run("RegisterDuplicateConsumer", func(t *testing.T) { + t.Parallel() + + tracker := unit.NewManager[testStatus, testConsumerID]() + err := tracker.Register(consumerA) + require.NoError(t, err) + + err = tracker.Register(consumerA) + require.Error(t, err) + assert.Contains(t, err.Error(), "already registered") + }) + + t.Run("RegisterMultipleConsumers", func(t *testing.T) { + t.Parallel() + + tracker := unit.NewManager[testStatus, testConsumerID]() + + consumers := []testConsumerID{consumerA, consumerB, consumerC} + for _, consumer := range consumers { + err := tracker.Register(consumer) + require.NoError(t, err) + } + + // All should be ready initially + for _, consumer := range consumers { + ready, err := tracker.IsReady(consumer) + require.NoError(t, err) + assert.True(t, ready) + } + }) +} + +func TestDependencyTracker_AddDependency(t *testing.T) { + t.Parallel() + + t.Run("AddDependencyBetweenRegisteredConsumers", func(t *testing.T) { + t.Parallel() + + tracker := unit.NewManager[testStatus, testConsumerID]() + err := tracker.Register(consumerA) + require.NoError(t, err) + err = tracker.Register(consumerB) + require.NoError(t, err) + + // A depends on B being "running" + err = tracker.AddDependency(consumerA, consumerB, statusRunning) + require.NoError(t, err) + + // A should no longer be ready (depends on B) + ready, err := tracker.IsReady(consumerA) + require.NoError(t, err) + assert.False(t, ready) + + // B should still be ready (no dependencies) + ready, err = tracker.IsReady(consumerB) + require.NoError(t, err) + assert.True(t, ready) + }) + + t.Run("AddDependencyWithUnregisteredConsumer", func(t *testing.T) { + t.Parallel() + + tracker := unit.NewManager[testStatus, testConsumerID]() + err := tracker.Register(consumerA) + require.NoError(t, err) + + // Try to add dependency to unregistered consumer + err = tracker.AddDependency(consumerA, consumerB, statusRunning) + require.Error(t, err) + assert.Contains(t, err.Error(), "not registered") + }) + + t.Run("AddDependencyFromUnregisteredConsumer", func(t *testing.T) { + t.Parallel() + + tracker := unit.NewManager[testStatus, testConsumerID]() + err := tracker.Register(consumerB) + require.NoError(t, err) + + // Try to add dependency from unregistered consumer + err = tracker.AddDependency(consumerA, consumerB, statusRunning) + require.Error(t, err) + assert.Contains(t, err.Error(), "not registered") + }) +} + +func TestDependencyTracker_UpdateStatus(t *testing.T) { + t.Parallel() + + t.Run("UpdateStatusTriggersReadinessRecalculation", func(t *testing.T) { + t.Parallel() + + tracker := unit.NewManager[testStatus, testConsumerID]() + err := tracker.Register(consumerA) + require.NoError(t, err) + err = tracker.Register(consumerB) + require.NoError(t, err) + + // A depends on B being "running" + err = tracker.AddDependency(consumerA, consumerB, statusRunning) + require.NoError(t, err) + + // Initially A is not ready + ready, err := tracker.IsReady(consumerA) + require.NoError(t, err) + assert.False(t, ready) + + // Update B to "running" - A should become ready + err = tracker.UpdateStatus(consumerB, statusRunning) + require.NoError(t, err) + + ready, err = tracker.IsReady(consumerA) + require.NoError(t, err) + assert.True(t, ready) + }) + + t.Run("UpdateStatusWithUnregisteredConsumer", func(t *testing.T) { + t.Parallel() + + tracker := unit.NewManager[testStatus, testConsumerID]() + + err := tracker.UpdateStatus(consumerA, statusRunning) + require.Error(t, err) + assert.Equal(t, unit.ErrConsumerNotFound, err) + }) + + t.Run("LinearChainDependencies", func(t *testing.T) { + t.Parallel() + + tracker := unit.NewManager[testStatus, testConsumerID]() + + // Register all consumers + consumers := []testConsumerID{consumerA, consumerB, consumerC} + for _, consumer := range consumers { + err := tracker.Register(consumer) + require.NoError(t, err) + } + + // Create chain: A depends on B being "started", B depends on C being "completed" + err := tracker.AddDependency(consumerA, consumerB, statusStarted) + require.NoError(t, err) + err = tracker.AddDependency(consumerB, consumerC, statusCompleted) + require.NoError(t, err) + + // Initially only C is ready + ready, err := tracker.IsReady(consumerC) + require.NoError(t, err) + assert.True(t, ready) + + ready, err = tracker.IsReady(consumerB) + require.NoError(t, err) + assert.False(t, ready) + + ready, err = tracker.IsReady(consumerA) + require.NoError(t, err) + assert.False(t, ready) + + // Update C to "completed" - B should become ready + err = tracker.UpdateStatus(consumerC, statusCompleted) + require.NoError(t, err) + + ready, err = tracker.IsReady(consumerB) + require.NoError(t, err) + assert.True(t, ready) + + ready, err = tracker.IsReady(consumerA) + require.NoError(t, err) + assert.False(t, ready) + + // Update B to "started" - A should become ready + err = tracker.UpdateStatus(consumerB, statusStarted) + require.NoError(t, err) + + ready, err = tracker.IsReady(consumerA) + require.NoError(t, err) + assert.True(t, ready) + }) +} + +func TestDependencyTracker_GetUnmetDependencies(t *testing.T) { + t.Parallel() + + t.Run("GetUnmetDependenciesForConsumerWithNoDependencies", func(t *testing.T) { + t.Parallel() + + tracker := unit.NewManager[testStatus, testConsumerID]() + err := tracker.Register(consumerA) + require.NoError(t, err) + + unmet, err := tracker.GetUnmetDependencies(consumerA) + require.NoError(t, err) + assert.Empty(t, unmet) + }) + + t.Run("GetUnmetDependenciesForConsumerWithUnsatisfiedDependencies", func(t *testing.T) { + t.Parallel() + + tracker := unit.NewManager[testStatus, testConsumerID]() + err := tracker.Register(consumerA) + require.NoError(t, err) + err = tracker.Register(consumerB) + require.NoError(t, err) + + // A depends on B being "running" + err = tracker.AddDependency(consumerA, consumerB, statusRunning) + require.NoError(t, err) + + unmet, err := tracker.GetUnmetDependencies(consumerA) + require.NoError(t, err) + require.Len(t, unmet, 1) + + assert.Equal(t, consumerA, unmet[0].Consumer) + assert.Equal(t, consumerB, unmet[0].DependsOn) + assert.Equal(t, statusRunning, unmet[0].RequiredStatus) + assert.False(t, unmet[0].IsSatisfied) + }) + + t.Run("GetUnmetDependenciesForConsumerWithSatisfiedDependencies", func(t *testing.T) { + t.Parallel() + + tracker := unit.NewManager[testStatus, testConsumerID]() + err := tracker.Register(consumerA) + require.NoError(t, err) + err = tracker.Register(consumerB) + require.NoError(t, err) + + // A depends on B being "running" + err = tracker.AddDependency(consumerA, consumerB, statusRunning) + require.NoError(t, err) + + // Update B to "running" + err = tracker.UpdateStatus(consumerB, statusRunning) + require.NoError(t, err) + + unmet, err := tracker.GetUnmetDependencies(consumerA) + require.NoError(t, err) + assert.Empty(t, unmet) + }) + + t.Run("GetUnmetDependenciesForUnregisteredConsumer", func(t *testing.T) { + t.Parallel() + + tracker := unit.NewManager[testStatus, testConsumerID]() + + unmet, err := tracker.GetUnmetDependencies(consumerA) + require.Error(t, err) + assert.Equal(t, unit.ErrConsumerNotFound, err) + assert.Nil(t, unmet) + }) +} + +func TestDependencyTracker_ConcurrentOperations(t *testing.T) { + t.Parallel() + + t.Run("ConcurrentStatusUpdates", func(t *testing.T) { + t.Parallel() + + tracker := unit.NewManager[testStatus, testConsumerID]() + + // Register consumers + consumers := []testConsumerID{consumerA, consumerB, consumerC, consumerD} + for _, consumer := range consumers { + err := tracker.Register(consumer) + require.NoError(t, err) + } + + // Create dependencies: A depends on B, B depends on C, C depends on D + err := tracker.AddDependency(consumerA, consumerB, statusRunning) + require.NoError(t, err) + err = tracker.AddDependency(consumerB, consumerC, statusStarted) + require.NoError(t, err) + err = tracker.AddDependency(consumerC, consumerD, statusCompleted) + require.NoError(t, err) + + var wg sync.WaitGroup + const numGoroutines = 10 + + // Launch goroutines that update statuses + errors := make([]error, numGoroutines) + for i := 0; i < numGoroutines; i++ { + wg.Add(1) + go func(goroutineID int) { + defer wg.Done() + + // Update D to completed (should make C ready) + err := tracker.UpdateStatus(consumerD, statusCompleted) + if err != nil { + errors[goroutineID] = err + return + } + + // Update C to started (should make B ready) + err = tracker.UpdateStatus(consumerC, statusStarted) + if err != nil { + errors[goroutineID] = err + return + } + + // Update B to running (should make A ready) + err = tracker.UpdateStatus(consumerB, statusRunning) + if err != nil { + errors[goroutineID] = err + return + } + }(i) + } + + wg.Wait() + + // Check for any errors in goroutines + for i, err := range errors { + require.NoError(t, err, "goroutine %d had error", i) + } + + // All consumers should be ready after the updates + for _, consumer := range consumers { + ready, err := tracker.IsReady(consumer) + require.NoError(t, err) + assert.True(t, ready) + } + }) + + t.Run("ConcurrentReadinessChecks", func(t *testing.T) { + t.Parallel() + + tracker := unit.NewManager[testStatus, testConsumerID]() + + // Register consumers + err := tracker.Register(consumerA) + require.NoError(t, err) + err = tracker.Register(consumerB) + require.NoError(t, err) + + // A depends on B being "running" + err = tracker.AddDependency(consumerA, consumerB, statusRunning) + require.NoError(t, err) + + var wg sync.WaitGroup + const numGoroutines = 20 + + // Launch goroutines that check readiness + for i := 0; i < numGoroutines; i++ { + wg.Add(1) + go func(goroutineID int) { + defer wg.Done() + + // Check readiness multiple times + for j := 0; j < 10; j++ { + ready, err := tracker.IsReady(consumerA) + require.NoError(t, err) + // Initially should be false, then true after B is updated + _ = ready + + ready, err = tracker.IsReady(consumerB) + require.NoError(t, err) + // B should always be ready (no dependencies) + assert.True(t, ready) + } + }(i) + } + + // Update B to "running" in the middle of readiness checks + err = tracker.UpdateStatus(consumerB, statusRunning) + require.NoError(t, err) + + wg.Wait() + }) +} + +func TestDependencyTracker_MultipleDependencies(t *testing.T) { + t.Parallel() + + t.Run("ConsumerWithMultipleDependencies", func(t *testing.T) { + t.Parallel() + + tracker := unit.NewManager[testStatus, testConsumerID]() + + // Register all consumers + consumers := []testConsumerID{consumerA, consumerB, consumerC, consumerD} + for _, consumer := range consumers { + err := tracker.Register(consumer) + require.NoError(t, err) + } + + // A depends on B being "running" AND C being "started" + err := tracker.AddDependency(consumerA, consumerB, statusRunning) + require.NoError(t, err) + err = tracker.AddDependency(consumerA, consumerC, statusStarted) + require.NoError(t, err) + + // A should not be ready (depends on both B and C) + ready, err := tracker.IsReady(consumerA) + require.NoError(t, err) + assert.False(t, ready) + + // Update B to "running" - A should still not be ready (needs C too) + err = tracker.UpdateStatus(consumerB, statusRunning) + require.NoError(t, err) + + ready, err = tracker.IsReady(consumerA) + require.NoError(t, err) + assert.False(t, ready) + + // Update C to "started" - A should now be ready + err = tracker.UpdateStatus(consumerC, statusStarted) + require.NoError(t, err) + + ready, err = tracker.IsReady(consumerA) + require.NoError(t, err) + assert.True(t, ready) + }) + + t.Run("ComplexDependencyChain", func(t *testing.T) { + t.Parallel() + + tracker := unit.NewManager[testStatus, testConsumerID]() + + // Register all consumers + consumers := []testConsumerID{consumerA, consumerB, consumerC, consumerD} + for _, consumer := range consumers { + err := tracker.Register(consumer) + require.NoError(t, err) + } + + // Create complex dependency graph: + // A depends on B being "running" AND C being "started" + // B depends on D being "completed" + // C depends on D being "completed" + err := tracker.AddDependency(consumerA, consumerB, statusRunning) + require.NoError(t, err) + err = tracker.AddDependency(consumerA, consumerC, statusStarted) + require.NoError(t, err) + err = tracker.AddDependency(consumerB, consumerD, statusCompleted) + require.NoError(t, err) + err = tracker.AddDependency(consumerC, consumerD, statusCompleted) + require.NoError(t, err) + + // Initially only D is ready + ready, err := tracker.IsReady(consumerD) + require.NoError(t, err) + assert.True(t, ready) + + ready, err = tracker.IsReady(consumerB) + require.NoError(t, err) + assert.False(t, ready) + + ready, err = tracker.IsReady(consumerC) + require.NoError(t, err) + assert.False(t, ready) + + ready, err = tracker.IsReady(consumerA) + require.NoError(t, err) + assert.False(t, ready) + + // Update D to "completed" - B and C should become ready + err = tracker.UpdateStatus(consumerD, statusCompleted) + require.NoError(t, err) + + ready, err = tracker.IsReady(consumerB) + require.NoError(t, err) + assert.True(t, ready) + + ready, err = tracker.IsReady(consumerC) + require.NoError(t, err) + assert.True(t, ready) + + ready, err = tracker.IsReady(consumerA) + require.NoError(t, err) + assert.False(t, ready) + + // Update B to "running" - A should still not be ready (needs C) + err = tracker.UpdateStatus(consumerB, statusRunning) + require.NoError(t, err) + + ready, err = tracker.IsReady(consumerA) + require.NoError(t, err) + assert.False(t, ready) + + // Update C to "started" - A should now be ready + err = tracker.UpdateStatus(consumerC, statusStarted) + require.NoError(t, err) + + ready, err = tracker.IsReady(consumerA) + require.NoError(t, err) + assert.True(t, ready) + }) + + t.Run("DifferentStatusTypes", func(t *testing.T) { + t.Parallel() + + tracker := unit.NewManager[testStatus, testConsumerID]() + + // Register consumers + err := tracker.Register(consumerA) + require.NoError(t, err) + err = tracker.Register(consumerB) + require.NoError(t, err) + err = tracker.Register(consumerC) + require.NoError(t, err) + + // A depends on B being "running" AND C being "completed" + err = tracker.AddDependency(consumerA, consumerB, statusRunning) + require.NoError(t, err) + err = tracker.AddDependency(consumerA, consumerC, statusCompleted) + require.NoError(t, err) + + // Update B to "running" but not C - A should not be ready + err = tracker.UpdateStatus(consumerB, statusRunning) + require.NoError(t, err) + + ready, err := tracker.IsReady(consumerA) + require.NoError(t, err) + assert.False(t, ready) + + // Update C to "completed" - A should now be ready + err = tracker.UpdateStatus(consumerC, statusCompleted) + require.NoError(t, err) + + ready, err = tracker.IsReady(consumerA) + require.NoError(t, err) + assert.True(t, ready) + }) +} + +func TestDependencyTracker_ErrorCases(t *testing.T) { + t.Parallel() + + t.Run("UpdateStatusWithUnregisteredConsumer", func(t *testing.T) { + t.Parallel() + + tracker := unit.NewManager[testStatus, testConsumerID]() + + err := tracker.UpdateStatus(consumerA, statusRunning) + require.Error(t, err) + assert.Equal(t, unit.ErrConsumerNotFound, err) + }) + + t.Run("IsReadyWithUnregisteredConsumer", func(t *testing.T) { + t.Parallel() + + tracker := unit.NewManager[testStatus, testConsumerID]() + + ready, err := tracker.IsReady(consumerA) + require.Error(t, err) + assert.Equal(t, unit.ErrConsumerNotFound, err) + assert.False(t, ready) + }) + + t.Run("GetUnmetDependenciesWithUnregisteredConsumer", func(t *testing.T) { + t.Parallel() + + tracker := unit.NewManager[testStatus, testConsumerID]() + + unmet, err := tracker.GetUnmetDependencies(consumerA) + require.Error(t, err) + assert.Equal(t, unit.ErrConsumerNotFound, err) + assert.Nil(t, unmet) + }) + + t.Run("AddDependencyWithUnregisteredConsumers", func(t *testing.T) { + t.Parallel() + + tracker := unit.NewManager[testStatus, testConsumerID]() + + // Try to add dependency with unregistered consumers + err := tracker.AddDependency(consumerA, consumerB, statusRunning) + require.Error(t, err) + assert.Contains(t, err.Error(), "not registered") + }) + + t.Run("CyclicDependencyDetection", func(t *testing.T) { + t.Parallel() + + tracker := unit.NewManager[testStatus, testConsumerID]() + + // Register consumers + err := tracker.Register(consumerA) + require.NoError(t, err) + err = tracker.Register(consumerB) + require.NoError(t, err) + + // A depends on B + err = tracker.AddDependency(consumerA, consumerB, statusRunning) + require.NoError(t, err) + + // Try to make B depend on A (creates cycle) + err = tracker.AddDependency(consumerB, consumerA, statusStarted) + require.Error(t, err) + assert.Contains(t, err.Error(), "would create a cycle") + }) +} + +func TestDependencyTracker_ToDOT(t *testing.T) { + t.Parallel() + + t.Run("ExportSimpleGraph", func(t *testing.T) { + t.Parallel() + + tracker := unit.NewManager[testStatus, testConsumerID]() + + // Register consumers + err := tracker.Register(consumerA) + require.NoError(t, err) + err = tracker.Register(consumerB) + require.NoError(t, err) + + // Add dependency + err = tracker.AddDependency(consumerA, consumerB, statusRunning) + require.NoError(t, err) + + dot, err := tracker.ExportDOT("test") + require.NoError(t, err) + assert.NotEmpty(t, dot) + assert.Contains(t, dot, "digraph") + }) + + t.Run("ExportComplexGraph", func(t *testing.T) { + t.Parallel() + + tracker := unit.NewManager[testStatus, testConsumerID]() + + // Register all consumers + consumers := []testConsumerID{consumerA, consumerB, consumerC, consumerD} + for _, consumer := range consumers { + err := tracker.Register(consumer) + require.NoError(t, err) + } + + // Create complex dependency graph + // A depends on B and C, B depends on D, C depends on D + err := tracker.AddDependency(consumerA, consumerB, statusRunning) + require.NoError(t, err) + err = tracker.AddDependency(consumerA, consumerC, statusStarted) + require.NoError(t, err) + err = tracker.AddDependency(consumerB, consumerD, statusCompleted) + require.NoError(t, err) + err = tracker.AddDependency(consumerC, consumerD, statusCompleted) + require.NoError(t, err) + + dot, err := tracker.ExportDOT("complex") + require.NoError(t, err) + assert.NotEmpty(t, dot) + assert.Contains(t, dot, "digraph") + }) +} From c4bd0d308db991cc51a46ad879e661dd1a99c012 Mon Sep 17 00:00:00 2001 From: Sas Swart Date: Tue, 11 Nov 2025 14:34:17 +0000 Subject: [PATCH 02/14] linter fixes --- agent/unit/manager_test.go | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/agent/unit/manager_test.go b/agent/unit/manager_test.go index d11de47c88ea5..80b27b9d7542d 100644 --- a/agent/unit/manager_test.go +++ b/agent/unit/manager_test.go @@ -1,6 +1,7 @@ package unit_test import ( + "errors" "sync" "testing" @@ -322,7 +323,7 @@ func TestDependencyTracker_ConcurrentOperations(t *testing.T) { const numGoroutines = 10 // Launch goroutines that update statuses - errors := make([]error, numGoroutines) + goroutineErrors := make([]error, numGoroutines) for i := 0; i < numGoroutines; i++ { wg.Add(1) go func(goroutineID int) { @@ -331,21 +332,21 @@ func TestDependencyTracker_ConcurrentOperations(t *testing.T) { // Update D to completed (should make C ready) err := tracker.UpdateStatus(consumerD, statusCompleted) if err != nil { - errors[goroutineID] = err + goroutineErrors[goroutineID] = err return } // Update C to started (should make B ready) err = tracker.UpdateStatus(consumerC, statusStarted) if err != nil { - errors[goroutineID] = err + goroutineErrors[goroutineID] = err return } // Update B to running (should make A ready) err = tracker.UpdateStatus(consumerB, statusRunning) if err != nil { - errors[goroutineID] = err + goroutineErrors[goroutineID] = err return } }(i) @@ -354,8 +355,11 @@ func TestDependencyTracker_ConcurrentOperations(t *testing.T) { wg.Wait() // Check for any errors in goroutines - for i, err := range errors { - require.NoError(t, err, "goroutine %d had error", i) + // ErrSameStatusAlreadySet is expected when multiple goroutines try to set the same status + for i, err := range goroutineErrors { + if err != nil && !errors.Is(err, unit.ErrSameStatusAlreadySet) { + require.NoError(t, err, "goroutine %d had unexpected error", i) + } } // All consumers should be ready after the updates From 9c59bb6af91282ab9dba2e3786ed4c3c10f34960 Mon Sep 17 00:00:00 2001 From: Sas Swart Date: Wed, 12 Nov 2025 10:36:03 +0000 Subject: [PATCH 03/14] rename consumer to unit --- agent/unit/manager.go | 234 +++++++++++------------ agent/unit/manager_test.go | 380 ++++++++++++++++++------------------- 2 files changed, 307 insertions(+), 307 deletions(-) diff --git a/agent/unit/manager.go b/agent/unit/manager.go index 5b187eb805ce5..4d14ad7aa1031 100644 --- a/agent/unit/manager.go +++ b/agent/unit/manager.go @@ -6,16 +6,16 @@ import ( "golang.org/x/xerrors" ) -// ErrConsumerNotFound is returned when a consumer ID is not registered. -var ErrConsumerNotFound = xerrors.New("consumer not found") +// ErrUnitNotFound is returned when a unit ID is not registered. +var ErrUnitNotFound = xerrors.New("unit not found") -// ErrConsumerAlreadyRegistered is returned when a consumer ID is already registered. -var ErrConsumerAlreadyRegistered = xerrors.New("consumer already registered") +// ErrUnitAlreadyRegistered is returned when a unit ID is already registered. +var ErrUnitAlreadyRegistered = xerrors.New("unit already registered") -// ErrCannotUpdateOtherConsumer is returned when attempting to update another consumer's status. -var ErrCannotUpdateOtherConsumer = xerrors.New("cannot update other consumer's status") +// ErrCannotUpdateOtherUnit is returned when attempting to update another unit's status. +var ErrCannotUpdateOtherUnit = xerrors.New("cannot update other unit's status") -// ErrDependenciesNotSatisfied is returned when a consumer's dependencies are not satisfied. +// ErrDependenciesNotSatisfied is returned when a unit's dependencies are not satisfied. var ErrDependenciesNotSatisfied = xerrors.New("unit dependencies not satisfied") // ErrSameStatusAlreadySet is returned when attempting to set the same status as the current status. @@ -27,119 +27,119 @@ const ( StatusComplete = "completed" ) -// dependencyVertex represents a vertex in the dependency graph that is associated with a consumer. -type dependencyVertex[ConsumerID comparable] struct { - ID ConsumerID +// dependencyVertex represents a vertex in the dependency graph that is associated with a unit. +type dependencyVertex[UnitID comparable] struct { + ID UnitID } -// Dependency represents a dependency relationship between consumers. -type Dependency[StatusType, ConsumerID comparable] struct { - Consumer ConsumerID - DependsOn ConsumerID +// Dependency represents a dependency relationship between units. +type Dependency[StatusType, UnitID comparable] struct { + Unit UnitID + DependsOn UnitID RequiredStatus StatusType CurrentStatus StatusType IsSatisfied bool } // Manager provides reactive dependency tracking over a Graph. -// It manages consumer registration, dependency relationships, and status updates +// It manages unit registration, dependency relationships, and status updates // with automatic recalculation of readiness when dependencies are satisfied. -type Manager[StatusType, ConsumerID comparable] struct { +type Manager[StatusType, UnitID comparable] struct { mu sync.RWMutex // The underlying graph that stores dependency relationships - graph *Graph[StatusType, *dependencyVertex[ConsumerID]] + graph *Graph[StatusType, *dependencyVertex[UnitID]] - // Track current status of each consumer - consumerStatus map[ConsumerID]StatusType + // Track current status of each unit + unitStatus map[UnitID]StatusType // Track readiness state (cached to avoid repeated graph traversal) - consumerReadiness map[ConsumerID]bool + unitReadiness map[UnitID]bool - // Track which consumers are registered - registeredConsumers map[ConsumerID]bool + // Track which units are registered + registeredUnits map[UnitID]bool - // Store vertex instances for each consumer to ensure consistent references - consumerVertices map[ConsumerID]*dependencyVertex[ConsumerID] + // Store vertex instances for each unit to ensure consistent references + unitVertices map[UnitID]*dependencyVertex[UnitID] } // NewManager creates a new Manager instance. -func NewManager[StatusType, ConsumerID comparable]() *Manager[StatusType, ConsumerID] { - return &Manager[StatusType, ConsumerID]{ - graph: &Graph[StatusType, *dependencyVertex[ConsumerID]]{}, - consumerStatus: make(map[ConsumerID]StatusType), - consumerReadiness: make(map[ConsumerID]bool), - registeredConsumers: make(map[ConsumerID]bool), - consumerVertices: make(map[ConsumerID]*dependencyVertex[ConsumerID]), +func NewManager[StatusType, UnitID comparable]() *Manager[StatusType, UnitID] { + return &Manager[StatusType, UnitID]{ + graph: &Graph[StatusType, *dependencyVertex[UnitID]]{}, + unitStatus: make(map[UnitID]StatusType), + unitReadiness: make(map[UnitID]bool), + registeredUnits: make(map[UnitID]bool), + unitVertices: make(map[UnitID]*dependencyVertex[UnitID]), } } -// Register registers a new consumer as a vertex in the dependency graph. -func (dt *Manager[StatusType, ConsumerID]) Register(id ConsumerID) error { +// Register registers a new unit as a vertex in the dependency graph. +func (dt *Manager[StatusType, UnitID]) Register(id UnitID) error { dt.mu.Lock() defer dt.mu.Unlock() - if dt.registeredConsumers[id] { - return ErrConsumerAlreadyRegistered + if dt.registeredUnits[id] { + return ErrUnitAlreadyRegistered } - // Create and store the vertex for this consumer - vertex := &dependencyVertex[ConsumerID]{ID: id} - dt.consumerVertices[id] = vertex - dt.registeredConsumers[id] = true - dt.consumerReadiness[id] = true // New consumers start as ready (no dependencies) + // Create and store the vertex for this unit + vertex := &dependencyVertex[UnitID]{ID: id} + dt.unitVertices[id] = vertex + dt.registeredUnits[id] = true + dt.unitReadiness[id] = true // New units start as ready (no dependencies) return nil } -// AddDependency adds a dependency relationship between consumers. -// The consumer depends on the dependsOn consumer reaching the requiredStatus. -func (dt *Manager[StatusType, ConsumerID]) AddDependency(consumer ConsumerID, dependsOn ConsumerID, requiredStatus StatusType) error { +// AddDependency adds a dependency relationship between units. +// The unit depends on the dependsOn unit reaching the requiredStatus. +func (dt *Manager[StatusType, UnitID]) AddDependency(unit UnitID, dependsOn UnitID, requiredStatus StatusType) error { dt.mu.Lock() defer dt.mu.Unlock() - if !dt.registeredConsumers[consumer] { - return xerrors.Errorf("consumer %v is not registered", consumer) + if !dt.registeredUnits[unit] { + return xerrors.Errorf("unit %v is not registered", unit) } - if !dt.registeredConsumers[dependsOn] { - return xerrors.Errorf("consumer %v is not registered", dependsOn) + if !dt.registeredUnits[dependsOn] { + return xerrors.Errorf("unit %v is not registered", dependsOn) } - // Get the stored vertices for both consumers - consumerVertex := dt.consumerVertices[consumer] - dependsOnVertex := dt.consumerVertices[dependsOn] + // Get the stored vertices for both units + unitVertex := dt.unitVertices[unit] + dependsOnVertex := dt.unitVertices[dependsOn] // Add the dependency edge to the graph - // The edge goes from consumer to dependsOn, representing the dependency - err := dt.graph.AddEdge(consumerVertex, dependsOnVertex, requiredStatus) + // The edge goes from unit to dependsOn, representing the dependency + err := dt.graph.AddEdge(unitVertex, dependsOnVertex, requiredStatus) if err != nil { return xerrors.Errorf("failed to add dependency: %w", err) } - // Recalculate readiness for the consumer since it now has a dependency - dt.recalculateReadinessUnsafe(consumer) + // Recalculate readiness for the unit since it now has a dependency + dt.recalculateReadinessUnsafe(unit) return nil } -// UpdateStatus updates a consumer's status and recalculates readiness for affected dependents. -func (dt *Manager[StatusType, ConsumerID]) UpdateStatus(consumer ConsumerID, newStatus StatusType) error { +// UpdateStatus updates a unit's status and recalculates readiness for affected dependents. +func (dt *Manager[StatusType, UnitID]) UpdateStatus(unit UnitID, newStatus StatusType) error { dt.mu.Lock() defer dt.mu.Unlock() - if !dt.registeredConsumers[consumer] { - return ErrConsumerNotFound + if !dt.registeredUnits[unit] { + return ErrUnitNotFound } - // Update the consumer's status - if dt.consumerStatus[consumer] == newStatus { + // Update the unit's status + if dt.unitStatus[unit] == newStatus { return ErrSameStatusAlreadySet } - dt.consumerStatus[consumer] = newStatus + dt.unitStatus[unit] = newStatus - // Get all consumers that depend on this one (reverse adjacent vertices) - consumerVertex := dt.consumerVertices[consumer] - dependentEdges := dt.graph.GetReverseAdjacentVertices(consumerVertex) + // Get all units that depend on this one (reverse adjacent vertices) + unitVertex := dt.unitVertices[unit] + dependentEdges := dt.graph.GetReverseAdjacentVertices(unitVertex) // Recalculate readiness for all dependents for _, edge := range dependentEdges { @@ -149,42 +149,42 @@ func (dt *Manager[StatusType, ConsumerID]) UpdateStatus(consumer ConsumerID, new return nil } -// IsReady checks if all dependencies for a consumer are satisfied. -func (dt *Manager[StatusType, ConsumerID]) IsReady(consumer ConsumerID) (bool, error) { +// IsReady checks if all dependencies for a unit are satisfied. +func (dt *Manager[StatusType, UnitID]) IsReady(unit UnitID) (bool, error) { dt.mu.RLock() defer dt.mu.RUnlock() - if !dt.registeredConsumers[consumer] { - return false, ErrConsumerNotFound + if !dt.registeredUnits[unit] { + return false, ErrUnitNotFound } - return dt.consumerReadiness[consumer], nil + return dt.unitReadiness[unit], nil } -// GetUnmetDependencies returns a list of unsatisfied dependencies for a consumer. -func (dt *Manager[StatusType, ConsumerID]) GetUnmetDependencies(consumer ConsumerID) ([]Dependency[StatusType, ConsumerID], error) { +// GetUnmetDependencies returns a list of unsatisfied dependencies for a unit. +func (dt *Manager[StatusType, UnitID]) GetUnmetDependencies(unit UnitID) ([]Dependency[StatusType, UnitID], error) { dt.mu.RLock() defer dt.mu.RUnlock() - if !dt.registeredConsumers[consumer] { - return nil, ErrConsumerNotFound + if !dt.registeredUnits[unit] { + return nil, ErrUnitNotFound } - consumerVertex := dt.consumerVertices[consumer] - forwardEdges := dt.graph.GetForwardAdjacentVertices(consumerVertex) + unitVertex := dt.unitVertices[unit] + forwardEdges := dt.graph.GetForwardAdjacentVertices(unitVertex) - var unmetDependencies []Dependency[StatusType, ConsumerID] + var unmetDependencies []Dependency[StatusType, UnitID] for _, edge := range forwardEdges { - dependsOnConsumer := edge.To.ID + dependsOnUnit := edge.To.ID requiredStatus := edge.Edge - currentStatus, exists := dt.consumerStatus[dependsOnConsumer] + currentStatus, exists := dt.unitStatus[dependsOnUnit] if !exists { - // If the dependency consumer has no status, it's not satisfied + // If the dependency unit has no status, it's not satisfied var zeroStatus StatusType - unmetDependencies = append(unmetDependencies, Dependency[StatusType, ConsumerID]{ - Consumer: consumer, - DependsOn: dependsOnConsumer, + unmetDependencies = append(unmetDependencies, Dependency[StatusType, UnitID]{ + Unit: unit, + DependsOn: dependsOnUnit, RequiredStatus: requiredStatus, CurrentStatus: zeroStatus, // Zero value IsSatisfied: false, @@ -192,9 +192,9 @@ func (dt *Manager[StatusType, ConsumerID]) GetUnmetDependencies(consumer Consume } else { isSatisfied := currentStatus == requiredStatus if !isSatisfied { - unmetDependencies = append(unmetDependencies, Dependency[StatusType, ConsumerID]{ - Consumer: consumer, - DependsOn: dependsOnConsumer, + unmetDependencies = append(unmetDependencies, Dependency[StatusType, UnitID]{ + Unit: unit, + DependsOn: dependsOnUnit, RequiredStatus: requiredStatus, CurrentStatus: currentStatus, IsSatisfied: false, @@ -206,50 +206,50 @@ func (dt *Manager[StatusType, ConsumerID]) GetUnmetDependencies(consumer Consume return unmetDependencies, nil } -// recalculateReadinessUnsafe recalculates the readiness state for a consumer. +// recalculateReadinessUnsafe recalculates the readiness state for a unit. // This method assumes the caller holds the write lock. -func (dt *Manager[StatusType, ConsumerID]) recalculateReadinessUnsafe(consumer ConsumerID) { - consumerVertex := dt.consumerVertices[consumer] - forwardEdges := dt.graph.GetForwardAdjacentVertices(consumerVertex) +func (dt *Manager[StatusType, UnitID]) recalculateReadinessUnsafe(unit UnitID) { + unitVertex := dt.unitVertices[unit] + forwardEdges := dt.graph.GetForwardAdjacentVertices(unitVertex) - // If there are no dependencies, the consumer is ready + // If there are no dependencies, the unit is ready if len(forwardEdges) == 0 { - dt.consumerReadiness[consumer] = true + dt.unitReadiness[unit] = true return } // Check if all dependencies are satisfied allSatisfied := true for _, edge := range forwardEdges { - dependsOnConsumer := edge.To.ID + dependsOnUnit := edge.To.ID requiredStatus := edge.Edge - currentStatus, exists := dt.consumerStatus[dependsOnConsumer] + currentStatus, exists := dt.unitStatus[dependsOnUnit] if !exists || currentStatus != requiredStatus { allSatisfied = false break } } - dt.consumerReadiness[consumer] = allSatisfied + dt.unitReadiness[unit] = allSatisfied } // GetGraph returns the underlying graph for visualization and debugging. // This should be used carefully as it exposes the internal graph structure. -func (dt *Manager[StatusType, ConsumerID]) GetGraph() *Graph[StatusType, *dependencyVertex[ConsumerID]] { +func (dt *Manager[StatusType, UnitID]) GetGraph() *Graph[StatusType, *dependencyVertex[UnitID]] { return dt.graph } -// GetStatus returns the current status of a consumer. -func (dt *Manager[StatusType, ConsumerID]) GetStatus(consumer ConsumerID) (StatusType, error) { +// GetStatus returns the current status of a unit. +func (dt *Manager[StatusType, UnitID]) GetStatus(unit UnitID) (StatusType, error) { dt.mu.RLock() defer dt.mu.RUnlock() - if !dt.registeredConsumers[consumer] { + if !dt.registeredUnits[unit] { var zeroStatus StatusType - return zeroStatus, ErrConsumerNotFound + return zeroStatus, ErrUnitNotFound } - status, exists := dt.consumerStatus[consumer] + status, exists := dt.unitStatus[unit] if !exists { var zeroStatus StatusType return zeroStatus, nil @@ -258,39 +258,39 @@ func (dt *Manager[StatusType, ConsumerID]) GetStatus(consumer ConsumerID) (Statu return status, nil } -// GetAllDependencies returns all dependencies for a consumer, both satisfied and unsatisfied. -func (dt *Manager[StatusType, ConsumerID]) GetAllDependencies(consumer ConsumerID) ([]Dependency[StatusType, ConsumerID], error) { +// GetAllDependencies returns all dependencies for a unit, both satisfied and unsatisfied. +func (dt *Manager[StatusType, UnitID]) GetAllDependencies(unit UnitID) ([]Dependency[StatusType, UnitID], error) { dt.mu.RLock() defer dt.mu.RUnlock() - if !dt.registeredConsumers[consumer] { - return nil, ErrConsumerNotFound + if !dt.registeredUnits[unit] { + return nil, ErrUnitNotFound } - consumerVertex := dt.consumerVertices[consumer] - forwardEdges := dt.graph.GetForwardAdjacentVertices(consumerVertex) + unitVertex := dt.unitVertices[unit] + forwardEdges := dt.graph.GetForwardAdjacentVertices(unitVertex) - var allDependencies []Dependency[StatusType, ConsumerID] + var allDependencies []Dependency[StatusType, UnitID] for _, edge := range forwardEdges { - dependsOnConsumer := edge.To.ID + dependsOnUnit := edge.To.ID requiredStatus := edge.Edge - currentStatus, exists := dt.consumerStatus[dependsOnConsumer] + currentStatus, exists := dt.unitStatus[dependsOnUnit] if !exists { - // If the dependency consumer has no status, it's not satisfied + // If the dependency unit has no status, it's not satisfied var zeroStatus StatusType - allDependencies = append(allDependencies, Dependency[StatusType, ConsumerID]{ - Consumer: consumer, - DependsOn: dependsOnConsumer, + allDependencies = append(allDependencies, Dependency[StatusType, UnitID]{ + Unit: unit, + DependsOn: dependsOnUnit, RequiredStatus: requiredStatus, CurrentStatus: zeroStatus, // Zero value IsSatisfied: false, }) } else { isSatisfied := currentStatus == requiredStatus - allDependencies = append(allDependencies, Dependency[StatusType, ConsumerID]{ - Consumer: consumer, - DependsOn: dependsOnConsumer, + allDependencies = append(allDependencies, Dependency[StatusType, UnitID]{ + Unit: unit, + DependsOn: dependsOnUnit, RequiredStatus: requiredStatus, CurrentStatus: currentStatus, IsSatisfied: isSatisfied, @@ -302,6 +302,6 @@ func (dt *Manager[StatusType, ConsumerID]) GetAllDependencies(consumer ConsumerI } // ExportDOT exports the dependency graph to DOT format for visualization. -func (dt *Manager[StatusType, ConsumerID]) ExportDOT(name string) (string, error) { +func (dt *Manager[StatusType, UnitID]) ExportDOT(name string) (string, error) { return dt.graph.ToDOT(name) } diff --git a/agent/unit/manager_test.go b/agent/unit/manager_test.go index 80b27b9d7542d..aca94484fcdf8 100644 --- a/agent/unit/manager_test.go +++ b/agent/unit/manager_test.go @@ -19,58 +19,58 @@ const ( statusCompleted testStatus = "completed" ) -type testConsumerID string +type testUnitID string const ( - consumerA testConsumerID = "serviceA" - consumerB testConsumerID = "serviceB" - consumerC testConsumerID = "serviceC" - consumerD testConsumerID = "serviceD" + unitA testUnitID = "serviceA" + unitB testUnitID = "serviceB" + unitC testUnitID = "serviceC" + unitD testUnitID = "serviceD" ) func TestDependencyTracker_Register(t *testing.T) { t.Parallel() - tracker := unit.NewManager[testStatus, testConsumerID]() + tracker := unit.NewManager[testStatus, testUnitID]() - t.Run("RegisterNewConsumer", func(t *testing.T) { + t.Run("RegisterNewUnit", func(t *testing.T) { t.Parallel() - err := tracker.Register(consumerA) + err := tracker.Register(unitA) require.NoError(t, err) - // Consumer should be ready initially (no dependencies) - ready, err := tracker.IsReady(consumerA) + // Unit should be ready initially (no dependencies) + ready, err := tracker.IsReady(unitA) require.NoError(t, err) assert.True(t, ready) }) - t.Run("RegisterDuplicateConsumer", func(t *testing.T) { + t.Run("RegisterDuplicateUnit", func(t *testing.T) { t.Parallel() - tracker := unit.NewManager[testStatus, testConsumerID]() - err := tracker.Register(consumerA) + tracker := unit.NewManager[testStatus, testUnitID]() + err := tracker.Register(unitA) require.NoError(t, err) - err = tracker.Register(consumerA) + err = tracker.Register(unitA) require.Error(t, err) assert.Contains(t, err.Error(), "already registered") }) - t.Run("RegisterMultipleConsumers", func(t *testing.T) { + t.Run("RegisterMultipleUnits", func(t *testing.T) { t.Parallel() - tracker := unit.NewManager[testStatus, testConsumerID]() + tracker := unit.NewManager[testStatus, testUnitID]() - consumers := []testConsumerID{consumerA, consumerB, consumerC} - for _, consumer := range consumers { - err := tracker.Register(consumer) + units := []testUnitID{unitA, unitB, unitC} + for _, unit := range units { + err := tracker.Register(unit) require.NoError(t, err) } // All should be ready initially - for _, consumer := range consumers { - ready, err := tracker.IsReady(consumer) + for _, unit := range units { + ready, err := tracker.IsReady(unit) require.NoError(t, err) assert.True(t, ready) } @@ -80,52 +80,52 @@ func TestDependencyTracker_Register(t *testing.T) { func TestDependencyTracker_AddDependency(t *testing.T) { t.Parallel() - t.Run("AddDependencyBetweenRegisteredConsumers", func(t *testing.T) { + t.Run("AddDependencyBetweenRegisteredUnits", func(t *testing.T) { t.Parallel() - tracker := unit.NewManager[testStatus, testConsumerID]() - err := tracker.Register(consumerA) + tracker := unit.NewManager[testStatus, testUnitID]() + err := tracker.Register(unitA) require.NoError(t, err) - err = tracker.Register(consumerB) + err = tracker.Register(unitB) require.NoError(t, err) // A depends on B being "running" - err = tracker.AddDependency(consumerA, consumerB, statusRunning) + err = tracker.AddDependency(unitA, unitB, statusRunning) require.NoError(t, err) // A should no longer be ready (depends on B) - ready, err := tracker.IsReady(consumerA) + ready, err := tracker.IsReady(unitA) require.NoError(t, err) assert.False(t, ready) // B should still be ready (no dependencies) - ready, err = tracker.IsReady(consumerB) + ready, err = tracker.IsReady(unitB) require.NoError(t, err) assert.True(t, ready) }) - t.Run("AddDependencyWithUnregisteredConsumer", func(t *testing.T) { + t.Run("AddDependencyWithUnregisteredUnit", func(t *testing.T) { t.Parallel() - tracker := unit.NewManager[testStatus, testConsumerID]() - err := tracker.Register(consumerA) + tracker := unit.NewManager[testStatus, testUnitID]() + err := tracker.Register(unitA) require.NoError(t, err) - // Try to add dependency to unregistered consumer - err = tracker.AddDependency(consumerA, consumerB, statusRunning) + // Try to add dependency to unregistered unit + err = tracker.AddDependency(unitA, unitB, statusRunning) require.Error(t, err) assert.Contains(t, err.Error(), "not registered") }) - t.Run("AddDependencyFromUnregisteredConsumer", func(t *testing.T) { + t.Run("AddDependencyFromUnregisteredUnit", func(t *testing.T) { t.Parallel() - tracker := unit.NewManager[testStatus, testConsumerID]() - err := tracker.Register(consumerB) + tracker := unit.NewManager[testStatus, testUnitID]() + err := tracker.Register(unitB) require.NoError(t, err) - // Try to add dependency from unregistered consumer - err = tracker.AddDependency(consumerA, consumerB, statusRunning) + // Try to add dependency from unregistered unit + err = tracker.AddDependency(unitA, unitB, statusRunning) require.Error(t, err) assert.Contains(t, err.Error(), "not registered") }) @@ -137,88 +137,88 @@ func TestDependencyTracker_UpdateStatus(t *testing.T) { t.Run("UpdateStatusTriggersReadinessRecalculation", func(t *testing.T) { t.Parallel() - tracker := unit.NewManager[testStatus, testConsumerID]() - err := tracker.Register(consumerA) + tracker := unit.NewManager[testStatus, testUnitID]() + err := tracker.Register(unitA) require.NoError(t, err) - err = tracker.Register(consumerB) + err = tracker.Register(unitB) require.NoError(t, err) // A depends on B being "running" - err = tracker.AddDependency(consumerA, consumerB, statusRunning) + err = tracker.AddDependency(unitA, unitB, statusRunning) require.NoError(t, err) // Initially A is not ready - ready, err := tracker.IsReady(consumerA) + ready, err := tracker.IsReady(unitA) require.NoError(t, err) assert.False(t, ready) // Update B to "running" - A should become ready - err = tracker.UpdateStatus(consumerB, statusRunning) + err = tracker.UpdateStatus(unitB, statusRunning) require.NoError(t, err) - ready, err = tracker.IsReady(consumerA) + ready, err = tracker.IsReady(unitA) require.NoError(t, err) assert.True(t, ready) }) - t.Run("UpdateStatusWithUnregisteredConsumer", func(t *testing.T) { + t.Run("UpdateStatusWithUnregisteredUnit", func(t *testing.T) { t.Parallel() - tracker := unit.NewManager[testStatus, testConsumerID]() + tracker := unit.NewManager[testStatus, testUnitID]() - err := tracker.UpdateStatus(consumerA, statusRunning) + err := tracker.UpdateStatus(unitA, statusRunning) require.Error(t, err) - assert.Equal(t, unit.ErrConsumerNotFound, err) + assert.Equal(t, unit.ErrUnitNotFound, err) }) t.Run("LinearChainDependencies", func(t *testing.T) { t.Parallel() - tracker := unit.NewManager[testStatus, testConsumerID]() + tracker := unit.NewManager[testStatus, testUnitID]() - // Register all consumers - consumers := []testConsumerID{consumerA, consumerB, consumerC} - for _, consumer := range consumers { - err := tracker.Register(consumer) + // Register all units + units := []testUnitID{unitA, unitB, unitC} + for _, unit := range units { + err := tracker.Register(unit) require.NoError(t, err) } // Create chain: A depends on B being "started", B depends on C being "completed" - err := tracker.AddDependency(consumerA, consumerB, statusStarted) + err := tracker.AddDependency(unitA, unitB, statusStarted) require.NoError(t, err) - err = tracker.AddDependency(consumerB, consumerC, statusCompleted) + err = tracker.AddDependency(unitB, unitC, statusCompleted) require.NoError(t, err) // Initially only C is ready - ready, err := tracker.IsReady(consumerC) + ready, err := tracker.IsReady(unitC) require.NoError(t, err) assert.True(t, ready) - ready, err = tracker.IsReady(consumerB) + ready, err = tracker.IsReady(unitB) require.NoError(t, err) assert.False(t, ready) - ready, err = tracker.IsReady(consumerA) + ready, err = tracker.IsReady(unitA) require.NoError(t, err) assert.False(t, ready) // Update C to "completed" - B should become ready - err = tracker.UpdateStatus(consumerC, statusCompleted) + err = tracker.UpdateStatus(unitC, statusCompleted) require.NoError(t, err) - ready, err = tracker.IsReady(consumerB) + ready, err = tracker.IsReady(unitB) require.NoError(t, err) assert.True(t, ready) - ready, err = tracker.IsReady(consumerA) + ready, err = tracker.IsReady(unitA) require.NoError(t, err) assert.False(t, ready) // Update B to "started" - A should become ready - err = tracker.UpdateStatus(consumerB, statusStarted) + err = tracker.UpdateStatus(unitB, statusStarted) require.NoError(t, err) - ready, err = tracker.IsReady(consumerA) + ready, err = tracker.IsReady(unitA) require.NoError(t, err) assert.True(t, ready) }) @@ -227,71 +227,71 @@ func TestDependencyTracker_UpdateStatus(t *testing.T) { func TestDependencyTracker_GetUnmetDependencies(t *testing.T) { t.Parallel() - t.Run("GetUnmetDependenciesForConsumerWithNoDependencies", func(t *testing.T) { + t.Run("GetUnmetDependenciesForUnitWithNoDependencies", func(t *testing.T) { t.Parallel() - tracker := unit.NewManager[testStatus, testConsumerID]() - err := tracker.Register(consumerA) + tracker := unit.NewManager[testStatus, testUnitID]() + err := tracker.Register(unitA) require.NoError(t, err) - unmet, err := tracker.GetUnmetDependencies(consumerA) + unmet, err := tracker.GetUnmetDependencies(unitA) require.NoError(t, err) assert.Empty(t, unmet) }) - t.Run("GetUnmetDependenciesForConsumerWithUnsatisfiedDependencies", func(t *testing.T) { + t.Run("GetUnmetDependenciesForUnitWithUnsatisfiedDependencies", func(t *testing.T) { t.Parallel() - tracker := unit.NewManager[testStatus, testConsumerID]() - err := tracker.Register(consumerA) + tracker := unit.NewManager[testStatus, testUnitID]() + err := tracker.Register(unitA) require.NoError(t, err) - err = tracker.Register(consumerB) + err = tracker.Register(unitB) require.NoError(t, err) // A depends on B being "running" - err = tracker.AddDependency(consumerA, consumerB, statusRunning) + err = tracker.AddDependency(unitA, unitB, statusRunning) require.NoError(t, err) - unmet, err := tracker.GetUnmetDependencies(consumerA) + unmet, err := tracker.GetUnmetDependencies(unitA) require.NoError(t, err) require.Len(t, unmet, 1) - assert.Equal(t, consumerA, unmet[0].Consumer) - assert.Equal(t, consumerB, unmet[0].DependsOn) + assert.Equal(t, unitA, unmet[0].Unit) + assert.Equal(t, unitB, unmet[0].DependsOn) assert.Equal(t, statusRunning, unmet[0].RequiredStatus) assert.False(t, unmet[0].IsSatisfied) }) - t.Run("GetUnmetDependenciesForConsumerWithSatisfiedDependencies", func(t *testing.T) { + t.Run("GetUnmetDependenciesForUnitWithSatisfiedDependencies", func(t *testing.T) { t.Parallel() - tracker := unit.NewManager[testStatus, testConsumerID]() - err := tracker.Register(consumerA) + tracker := unit.NewManager[testStatus, testUnitID]() + err := tracker.Register(unitA) require.NoError(t, err) - err = tracker.Register(consumerB) + err = tracker.Register(unitB) require.NoError(t, err) // A depends on B being "running" - err = tracker.AddDependency(consumerA, consumerB, statusRunning) + err = tracker.AddDependency(unitA, unitB, statusRunning) require.NoError(t, err) // Update B to "running" - err = tracker.UpdateStatus(consumerB, statusRunning) + err = tracker.UpdateStatus(unitB, statusRunning) require.NoError(t, err) - unmet, err := tracker.GetUnmetDependencies(consumerA) + unmet, err := tracker.GetUnmetDependencies(unitA) require.NoError(t, err) assert.Empty(t, unmet) }) - t.Run("GetUnmetDependenciesForUnregisteredConsumer", func(t *testing.T) { + t.Run("GetUnmetDependenciesForUnregisteredUnit", func(t *testing.T) { t.Parallel() - tracker := unit.NewManager[testStatus, testConsumerID]() + tracker := unit.NewManager[testStatus, testUnitID]() - unmet, err := tracker.GetUnmetDependencies(consumerA) + unmet, err := tracker.GetUnmetDependencies(unitA) require.Error(t, err) - assert.Equal(t, unit.ErrConsumerNotFound, err) + assert.Equal(t, unit.ErrUnitNotFound, err) assert.Nil(t, unmet) }) } @@ -302,21 +302,21 @@ func TestDependencyTracker_ConcurrentOperations(t *testing.T) { t.Run("ConcurrentStatusUpdates", func(t *testing.T) { t.Parallel() - tracker := unit.NewManager[testStatus, testConsumerID]() + tracker := unit.NewManager[testStatus, testUnitID]() - // Register consumers - consumers := []testConsumerID{consumerA, consumerB, consumerC, consumerD} - for _, consumer := range consumers { - err := tracker.Register(consumer) + // Register units + units := []testUnitID{unitA, unitB, unitC, unitD} + for _, unit := range units { + err := tracker.Register(unit) require.NoError(t, err) } // Create dependencies: A depends on B, B depends on C, C depends on D - err := tracker.AddDependency(consumerA, consumerB, statusRunning) + err := tracker.AddDependency(unitA, unitB, statusRunning) require.NoError(t, err) - err = tracker.AddDependency(consumerB, consumerC, statusStarted) + err = tracker.AddDependency(unitB, unitC, statusStarted) require.NoError(t, err) - err = tracker.AddDependency(consumerC, consumerD, statusCompleted) + err = tracker.AddDependency(unitC, unitD, statusCompleted) require.NoError(t, err) var wg sync.WaitGroup @@ -330,21 +330,21 @@ func TestDependencyTracker_ConcurrentOperations(t *testing.T) { defer wg.Done() // Update D to completed (should make C ready) - err := tracker.UpdateStatus(consumerD, statusCompleted) + err := tracker.UpdateStatus(unitD, statusCompleted) if err != nil { goroutineErrors[goroutineID] = err return } // Update C to started (should make B ready) - err = tracker.UpdateStatus(consumerC, statusStarted) + err = tracker.UpdateStatus(unitC, statusStarted) if err != nil { goroutineErrors[goroutineID] = err return } // Update B to running (should make A ready) - err = tracker.UpdateStatus(consumerB, statusRunning) + err = tracker.UpdateStatus(unitB, statusRunning) if err != nil { goroutineErrors[goroutineID] = err return @@ -362,9 +362,9 @@ func TestDependencyTracker_ConcurrentOperations(t *testing.T) { } } - // All consumers should be ready after the updates - for _, consumer := range consumers { - ready, err := tracker.IsReady(consumer) + // All units should be ready after the updates + for _, unit := range units { + ready, err := tracker.IsReady(unit) require.NoError(t, err) assert.True(t, ready) } @@ -373,16 +373,16 @@ func TestDependencyTracker_ConcurrentOperations(t *testing.T) { t.Run("ConcurrentReadinessChecks", func(t *testing.T) { t.Parallel() - tracker := unit.NewManager[testStatus, testConsumerID]() + tracker := unit.NewManager[testStatus, testUnitID]() - // Register consumers - err := tracker.Register(consumerA) + // Register units + err := tracker.Register(unitA) require.NoError(t, err) - err = tracker.Register(consumerB) + err = tracker.Register(unitB) require.NoError(t, err) // A depends on B being "running" - err = tracker.AddDependency(consumerA, consumerB, statusRunning) + err = tracker.AddDependency(unitA, unitB, statusRunning) require.NoError(t, err) var wg sync.WaitGroup @@ -396,12 +396,12 @@ func TestDependencyTracker_ConcurrentOperations(t *testing.T) { // Check readiness multiple times for j := 0; j < 10; j++ { - ready, err := tracker.IsReady(consumerA) + ready, err := tracker.IsReady(unitA) require.NoError(t, err) // Initially should be false, then true after B is updated _ = ready - ready, err = tracker.IsReady(consumerB) + ready, err = tracker.IsReady(unitB) require.NoError(t, err) // B should always be ready (no dependencies) assert.True(t, ready) @@ -410,7 +410,7 @@ func TestDependencyTracker_ConcurrentOperations(t *testing.T) { } // Update B to "running" in the middle of readiness checks - err = tracker.UpdateStatus(consumerB, statusRunning) + err = tracker.UpdateStatus(unitB, statusRunning) require.NoError(t, err) wg.Wait() @@ -420,42 +420,42 @@ func TestDependencyTracker_ConcurrentOperations(t *testing.T) { func TestDependencyTracker_MultipleDependencies(t *testing.T) { t.Parallel() - t.Run("ConsumerWithMultipleDependencies", func(t *testing.T) { + t.Run("UnitWithMultipleDependencies", func(t *testing.T) { t.Parallel() - tracker := unit.NewManager[testStatus, testConsumerID]() + tracker := unit.NewManager[testStatus, testUnitID]() - // Register all consumers - consumers := []testConsumerID{consumerA, consumerB, consumerC, consumerD} - for _, consumer := range consumers { - err := tracker.Register(consumer) + // Register all units + units := []testUnitID{unitA, unitB, unitC, unitD} + for _, unit := range units { + err := tracker.Register(unit) require.NoError(t, err) } // A depends on B being "running" AND C being "started" - err := tracker.AddDependency(consumerA, consumerB, statusRunning) + err := tracker.AddDependency(unitA, unitB, statusRunning) require.NoError(t, err) - err = tracker.AddDependency(consumerA, consumerC, statusStarted) + err = tracker.AddDependency(unitA, unitC, statusStarted) require.NoError(t, err) // A should not be ready (depends on both B and C) - ready, err := tracker.IsReady(consumerA) + ready, err := tracker.IsReady(unitA) require.NoError(t, err) assert.False(t, ready) // Update B to "running" - A should still not be ready (needs C too) - err = tracker.UpdateStatus(consumerB, statusRunning) + err = tracker.UpdateStatus(unitB, statusRunning) require.NoError(t, err) - ready, err = tracker.IsReady(consumerA) + ready, err = tracker.IsReady(unitA) require.NoError(t, err) assert.False(t, ready) // Update C to "started" - A should now be ready - err = tracker.UpdateStatus(consumerC, statusStarted) + err = tracker.UpdateStatus(unitC, statusStarted) require.NoError(t, err) - ready, err = tracker.IsReady(consumerA) + ready, err = tracker.IsReady(unitA) require.NoError(t, err) assert.True(t, ready) }) @@ -463,12 +463,12 @@ func TestDependencyTracker_MultipleDependencies(t *testing.T) { t.Run("ComplexDependencyChain", func(t *testing.T) { t.Parallel() - tracker := unit.NewManager[testStatus, testConsumerID]() + tracker := unit.NewManager[testStatus, testUnitID]() - // Register all consumers - consumers := []testConsumerID{consumerA, consumerB, consumerC, consumerD} - for _, consumer := range consumers { - err := tracker.Register(consumer) + // Register all units + units := []testUnitID{unitA, unitB, unitC, unitD} + for _, unit := range units { + err := tracker.Register(unit) require.NoError(t, err) } @@ -476,61 +476,61 @@ func TestDependencyTracker_MultipleDependencies(t *testing.T) { // A depends on B being "running" AND C being "started" // B depends on D being "completed" // C depends on D being "completed" - err := tracker.AddDependency(consumerA, consumerB, statusRunning) + err := tracker.AddDependency(unitA, unitB, statusRunning) require.NoError(t, err) - err = tracker.AddDependency(consumerA, consumerC, statusStarted) + err = tracker.AddDependency(unitA, unitC, statusStarted) require.NoError(t, err) - err = tracker.AddDependency(consumerB, consumerD, statusCompleted) + err = tracker.AddDependency(unitB, unitD, statusCompleted) require.NoError(t, err) - err = tracker.AddDependency(consumerC, consumerD, statusCompleted) + err = tracker.AddDependency(unitC, unitD, statusCompleted) require.NoError(t, err) // Initially only D is ready - ready, err := tracker.IsReady(consumerD) + ready, err := tracker.IsReady(unitD) require.NoError(t, err) assert.True(t, ready) - ready, err = tracker.IsReady(consumerB) + ready, err = tracker.IsReady(unitB) require.NoError(t, err) assert.False(t, ready) - ready, err = tracker.IsReady(consumerC) + ready, err = tracker.IsReady(unitC) require.NoError(t, err) assert.False(t, ready) - ready, err = tracker.IsReady(consumerA) + ready, err = tracker.IsReady(unitA) require.NoError(t, err) assert.False(t, ready) // Update D to "completed" - B and C should become ready - err = tracker.UpdateStatus(consumerD, statusCompleted) + err = tracker.UpdateStatus(unitD, statusCompleted) require.NoError(t, err) - ready, err = tracker.IsReady(consumerB) + ready, err = tracker.IsReady(unitB) require.NoError(t, err) assert.True(t, ready) - ready, err = tracker.IsReady(consumerC) + ready, err = tracker.IsReady(unitC) require.NoError(t, err) assert.True(t, ready) - ready, err = tracker.IsReady(consumerA) + ready, err = tracker.IsReady(unitA) require.NoError(t, err) assert.False(t, ready) // Update B to "running" - A should still not be ready (needs C) - err = tracker.UpdateStatus(consumerB, statusRunning) + err = tracker.UpdateStatus(unitB, statusRunning) require.NoError(t, err) - ready, err = tracker.IsReady(consumerA) + ready, err = tracker.IsReady(unitA) require.NoError(t, err) assert.False(t, ready) // Update C to "started" - A should now be ready - err = tracker.UpdateStatus(consumerC, statusStarted) + err = tracker.UpdateStatus(unitC, statusStarted) require.NoError(t, err) - ready, err = tracker.IsReady(consumerA) + ready, err = tracker.IsReady(unitA) require.NoError(t, err) assert.True(t, ready) }) @@ -538,35 +538,35 @@ func TestDependencyTracker_MultipleDependencies(t *testing.T) { t.Run("DifferentStatusTypes", func(t *testing.T) { t.Parallel() - tracker := unit.NewManager[testStatus, testConsumerID]() + tracker := unit.NewManager[testStatus, testUnitID]() - // Register consumers - err := tracker.Register(consumerA) + // Register units + err := tracker.Register(unitA) require.NoError(t, err) - err = tracker.Register(consumerB) + err = tracker.Register(unitB) require.NoError(t, err) - err = tracker.Register(consumerC) + err = tracker.Register(unitC) require.NoError(t, err) // A depends on B being "running" AND C being "completed" - err = tracker.AddDependency(consumerA, consumerB, statusRunning) + err = tracker.AddDependency(unitA, unitB, statusRunning) require.NoError(t, err) - err = tracker.AddDependency(consumerA, consumerC, statusCompleted) + err = tracker.AddDependency(unitA, unitC, statusCompleted) require.NoError(t, err) // Update B to "running" but not C - A should not be ready - err = tracker.UpdateStatus(consumerB, statusRunning) + err = tracker.UpdateStatus(unitB, statusRunning) require.NoError(t, err) - ready, err := tracker.IsReady(consumerA) + ready, err := tracker.IsReady(unitA) require.NoError(t, err) assert.False(t, ready) // Update C to "completed" - A should now be ready - err = tracker.UpdateStatus(consumerC, statusCompleted) + err = tracker.UpdateStatus(unitC, statusCompleted) require.NoError(t, err) - ready, err = tracker.IsReady(consumerA) + ready, err = tracker.IsReady(unitA) require.NoError(t, err) assert.True(t, ready) }) @@ -575,45 +575,45 @@ func TestDependencyTracker_MultipleDependencies(t *testing.T) { func TestDependencyTracker_ErrorCases(t *testing.T) { t.Parallel() - t.Run("UpdateStatusWithUnregisteredConsumer", func(t *testing.T) { + t.Run("UpdateStatusWithUnregisteredUnit", func(t *testing.T) { t.Parallel() - tracker := unit.NewManager[testStatus, testConsumerID]() + tracker := unit.NewManager[testStatus, testUnitID]() - err := tracker.UpdateStatus(consumerA, statusRunning) + err := tracker.UpdateStatus(unitA, statusRunning) require.Error(t, err) - assert.Equal(t, unit.ErrConsumerNotFound, err) + assert.Equal(t, unit.ErrUnitNotFound, err) }) - t.Run("IsReadyWithUnregisteredConsumer", func(t *testing.T) { + t.Run("IsReadyWithUnregisteredUnit", func(t *testing.T) { t.Parallel() - tracker := unit.NewManager[testStatus, testConsumerID]() + tracker := unit.NewManager[testStatus, testUnitID]() - ready, err := tracker.IsReady(consumerA) + ready, err := tracker.IsReady(unitA) require.Error(t, err) - assert.Equal(t, unit.ErrConsumerNotFound, err) + assert.Equal(t, unit.ErrUnitNotFound, err) assert.False(t, ready) }) - t.Run("GetUnmetDependenciesWithUnregisteredConsumer", func(t *testing.T) { + t.Run("GetUnmetDependenciesWithUnregisteredUnit", func(t *testing.T) { t.Parallel() - tracker := unit.NewManager[testStatus, testConsumerID]() + tracker := unit.NewManager[testStatus, testUnitID]() - unmet, err := tracker.GetUnmetDependencies(consumerA) + unmet, err := tracker.GetUnmetDependencies(unitA) require.Error(t, err) - assert.Equal(t, unit.ErrConsumerNotFound, err) + assert.Equal(t, unit.ErrUnitNotFound, err) assert.Nil(t, unmet) }) - t.Run("AddDependencyWithUnregisteredConsumers", func(t *testing.T) { + t.Run("AddDependencyWithUnregisteredUnits", func(t *testing.T) { t.Parallel() - tracker := unit.NewManager[testStatus, testConsumerID]() + tracker := unit.NewManager[testStatus, testUnitID]() - // Try to add dependency with unregistered consumers - err := tracker.AddDependency(consumerA, consumerB, statusRunning) + // Try to add dependency with unregistered units + err := tracker.AddDependency(unitA, unitB, statusRunning) require.Error(t, err) assert.Contains(t, err.Error(), "not registered") }) @@ -621,20 +621,20 @@ func TestDependencyTracker_ErrorCases(t *testing.T) { t.Run("CyclicDependencyDetection", func(t *testing.T) { t.Parallel() - tracker := unit.NewManager[testStatus, testConsumerID]() + tracker := unit.NewManager[testStatus, testUnitID]() - // Register consumers - err := tracker.Register(consumerA) + // Register units + err := tracker.Register(unitA) require.NoError(t, err) - err = tracker.Register(consumerB) + err = tracker.Register(unitB) require.NoError(t, err) // A depends on B - err = tracker.AddDependency(consumerA, consumerB, statusRunning) + err = tracker.AddDependency(unitA, unitB, statusRunning) require.NoError(t, err) // Try to make B depend on A (creates cycle) - err = tracker.AddDependency(consumerB, consumerA, statusStarted) + err = tracker.AddDependency(unitB, unitA, statusStarted) require.Error(t, err) assert.Contains(t, err.Error(), "would create a cycle") }) @@ -646,16 +646,16 @@ func TestDependencyTracker_ToDOT(t *testing.T) { t.Run("ExportSimpleGraph", func(t *testing.T) { t.Parallel() - tracker := unit.NewManager[testStatus, testConsumerID]() + tracker := unit.NewManager[testStatus, testUnitID]() - // Register consumers - err := tracker.Register(consumerA) + // Register units + err := tracker.Register(unitA) require.NoError(t, err) - err = tracker.Register(consumerB) + err = tracker.Register(unitB) require.NoError(t, err) // Add dependency - err = tracker.AddDependency(consumerA, consumerB, statusRunning) + err = tracker.AddDependency(unitA, unitB, statusRunning) require.NoError(t, err) dot, err := tracker.ExportDOT("test") @@ -667,24 +667,24 @@ func TestDependencyTracker_ToDOT(t *testing.T) { t.Run("ExportComplexGraph", func(t *testing.T) { t.Parallel() - tracker := unit.NewManager[testStatus, testConsumerID]() + tracker := unit.NewManager[testStatus, testUnitID]() - // Register all consumers - consumers := []testConsumerID{consumerA, consumerB, consumerC, consumerD} - for _, consumer := range consumers { - err := tracker.Register(consumer) + // Register all units + units := []testUnitID{unitA, unitB, unitC, unitD} + for _, unit := range units { + err := tracker.Register(unit) require.NoError(t, err) } // Create complex dependency graph // A depends on B and C, B depends on D, C depends on D - err := tracker.AddDependency(consumerA, consumerB, statusRunning) + err := tracker.AddDependency(unitA, unitB, statusRunning) require.NoError(t, err) - err = tracker.AddDependency(consumerA, consumerC, statusStarted) + err = tracker.AddDependency(unitA, unitC, statusStarted) require.NoError(t, err) - err = tracker.AddDependency(consumerB, consumerD, statusCompleted) + err = tracker.AddDependency(unitB, unitD, statusCompleted) require.NoError(t, err) - err = tracker.AddDependency(consumerC, consumerD, statusCompleted) + err = tracker.AddDependency(unitC, unitD, statusCompleted) require.NoError(t, err) dot, err := tracker.ExportDOT("complex") From 95e66a398947f7060e892b14333b4ac1224f8d57 Mon Sep 17 00:00:00 2001 From: Sas Swart Date: Wed, 12 Nov 2025 10:50:06 +0000 Subject: [PATCH 04/14] remove defunct test status types in the agent unit manager --- agent/unit/manager.go | 20 +++-- agent/unit/manager_test.go | 178 ++++++++++++++++++------------------- 2 files changed, 97 insertions(+), 101 deletions(-) diff --git a/agent/unit/manager.go b/agent/unit/manager.go index 4d14ad7aa1031..581623f6a9cba 100644 --- a/agent/unit/manager.go +++ b/agent/unit/manager.go @@ -181,12 +181,13 @@ func (dt *Manager[StatusType, UnitID]) GetUnmetDependencies(unit UnitID) ([]Depe currentStatus, exists := dt.unitStatus[dependsOnUnit] if !exists { // If the dependency unit has no status, it's not satisfied - var zeroStatus StatusType + // Zero value represents StatusPending + var pendingStatus StatusType unmetDependencies = append(unmetDependencies, Dependency[StatusType, UnitID]{ Unit: unit, DependsOn: dependsOnUnit, RequiredStatus: requiredStatus, - CurrentStatus: zeroStatus, // Zero value + CurrentStatus: pendingStatus, // StatusPending (zero value) IsSatisfied: false, }) } else { @@ -245,14 +246,16 @@ func (dt *Manager[StatusType, UnitID]) GetStatus(unit UnitID) (StatusType, error defer dt.mu.RUnlock() if !dt.registeredUnits[unit] { - var zeroStatus StatusType - return zeroStatus, ErrUnitNotFound + // Zero value represents StatusPending + var pendingStatus StatusType + return pendingStatus, ErrUnitNotFound } status, exists := dt.unitStatus[unit] if !exists { - var zeroStatus StatusType - return zeroStatus, nil + // Zero value represents StatusPending + var pendingStatus StatusType + return pendingStatus, nil } return status, nil @@ -278,12 +281,13 @@ func (dt *Manager[StatusType, UnitID]) GetAllDependencies(unit UnitID) ([]Depend currentStatus, exists := dt.unitStatus[dependsOnUnit] if !exists { // If the dependency unit has no status, it's not satisfied - var zeroStatus StatusType + // Zero value represents StatusPending + var pendingStatus StatusType allDependencies = append(allDependencies, Dependency[StatusType, UnitID]{ Unit: unit, DependsOn: dependsOnUnit, RequiredStatus: requiredStatus, - CurrentStatus: zeroStatus, // Zero value + CurrentStatus: pendingStatus, // StatusPending (zero value) IsSatisfied: false, }) } else { diff --git a/agent/unit/manager_test.go b/agent/unit/manager_test.go index aca94484fcdf8..c936586fdf297 100644 --- a/agent/unit/manager_test.go +++ b/agent/unit/manager_test.go @@ -11,14 +11,6 @@ import ( "github.com/coder/coder/v2/agent/unit" ) -type testStatus string - -const ( - statusStarted testStatus = "started" - statusRunning testStatus = "running" - statusCompleted testStatus = "completed" -) - type testUnitID string const ( @@ -31,7 +23,7 @@ const ( func TestDependencyTracker_Register(t *testing.T) { t.Parallel() - tracker := unit.NewManager[testStatus, testUnitID]() + tracker := unit.NewManager[string, testUnitID]() t.Run("RegisterNewUnit", func(t *testing.T) { t.Parallel() @@ -48,7 +40,7 @@ func TestDependencyTracker_Register(t *testing.T) { t.Run("RegisterDuplicateUnit", func(t *testing.T) { t.Parallel() - tracker := unit.NewManager[testStatus, testUnitID]() + tracker := unit.NewManager[string, testUnitID]() err := tracker.Register(unitA) require.NoError(t, err) @@ -60,7 +52,7 @@ func TestDependencyTracker_Register(t *testing.T) { t.Run("RegisterMultipleUnits", func(t *testing.T) { t.Parallel() - tracker := unit.NewManager[testStatus, testUnitID]() + tracker := unit.NewManager[string, testUnitID]() units := []testUnitID{unitA, unitB, unitC} for _, unit := range units { @@ -83,14 +75,14 @@ func TestDependencyTracker_AddDependency(t *testing.T) { t.Run("AddDependencyBetweenRegisteredUnits", func(t *testing.T) { t.Parallel() - tracker := unit.NewManager[testStatus, testUnitID]() + tracker := unit.NewManager[string, testUnitID]() err := tracker.Register(unitA) require.NoError(t, err) err = tracker.Register(unitB) require.NoError(t, err) - // A depends on B being "running" - err = tracker.AddDependency(unitA, unitB, statusRunning) + // A depends on B being unit.StatusStarted + err = tracker.AddDependency(unitA, unitB, unit.StatusStarted) require.NoError(t, err) // A should no longer be ready (depends on B) @@ -107,12 +99,12 @@ func TestDependencyTracker_AddDependency(t *testing.T) { t.Run("AddDependencyWithUnregisteredUnit", func(t *testing.T) { t.Parallel() - tracker := unit.NewManager[testStatus, testUnitID]() + tracker := unit.NewManager[string, testUnitID]() err := tracker.Register(unitA) require.NoError(t, err) // Try to add dependency to unregistered unit - err = tracker.AddDependency(unitA, unitB, statusRunning) + err = tracker.AddDependency(unitA, unitB, unit.StatusStarted) require.Error(t, err) assert.Contains(t, err.Error(), "not registered") }) @@ -120,12 +112,12 @@ func TestDependencyTracker_AddDependency(t *testing.T) { t.Run("AddDependencyFromUnregisteredUnit", func(t *testing.T) { t.Parallel() - tracker := unit.NewManager[testStatus, testUnitID]() + tracker := unit.NewManager[string, testUnitID]() err := tracker.Register(unitB) require.NoError(t, err) // Try to add dependency from unregistered unit - err = tracker.AddDependency(unitA, unitB, statusRunning) + err = tracker.AddDependency(unitA, unitB, unit.StatusStarted) require.Error(t, err) assert.Contains(t, err.Error(), "not registered") }) @@ -137,14 +129,14 @@ func TestDependencyTracker_UpdateStatus(t *testing.T) { t.Run("UpdateStatusTriggersReadinessRecalculation", func(t *testing.T) { t.Parallel() - tracker := unit.NewManager[testStatus, testUnitID]() + tracker := unit.NewManager[string, testUnitID]() err := tracker.Register(unitA) require.NoError(t, err) err = tracker.Register(unitB) require.NoError(t, err) - // A depends on B being "running" - err = tracker.AddDependency(unitA, unitB, statusRunning) + // A depends on B being unit.StatusStarted + err = tracker.AddDependency(unitA, unitB, unit.StatusStarted) require.NoError(t, err) // Initially A is not ready @@ -152,8 +144,8 @@ func TestDependencyTracker_UpdateStatus(t *testing.T) { require.NoError(t, err) assert.False(t, ready) - // Update B to "running" - A should become ready - err = tracker.UpdateStatus(unitB, statusRunning) + // Update B to unit.StatusStarted - A should become ready + err = tracker.UpdateStatus(unitB, unit.StatusStarted) require.NoError(t, err) ready, err = tracker.IsReady(unitA) @@ -164,9 +156,9 @@ func TestDependencyTracker_UpdateStatus(t *testing.T) { t.Run("UpdateStatusWithUnregisteredUnit", func(t *testing.T) { t.Parallel() - tracker := unit.NewManager[testStatus, testUnitID]() + tracker := unit.NewManager[string, testUnitID]() - err := tracker.UpdateStatus(unitA, statusRunning) + err := tracker.UpdateStatus(unitA, unit.StatusStarted) require.Error(t, err) assert.Equal(t, unit.ErrUnitNotFound, err) }) @@ -174,7 +166,7 @@ func TestDependencyTracker_UpdateStatus(t *testing.T) { t.Run("LinearChainDependencies", func(t *testing.T) { t.Parallel() - tracker := unit.NewManager[testStatus, testUnitID]() + tracker := unit.NewManager[string, testUnitID]() // Register all units units := []testUnitID{unitA, unitB, unitC} @@ -184,9 +176,9 @@ func TestDependencyTracker_UpdateStatus(t *testing.T) { } // Create chain: A depends on B being "started", B depends on C being "completed" - err := tracker.AddDependency(unitA, unitB, statusStarted) + err := tracker.AddDependency(unitA, unitB, unit.StatusStarted) require.NoError(t, err) - err = tracker.AddDependency(unitB, unitC, statusCompleted) + err = tracker.AddDependency(unitB, unitC, unit.StatusComplete) require.NoError(t, err) // Initially only C is ready @@ -203,7 +195,7 @@ func TestDependencyTracker_UpdateStatus(t *testing.T) { assert.False(t, ready) // Update C to "completed" - B should become ready - err = tracker.UpdateStatus(unitC, statusCompleted) + err = tracker.UpdateStatus(unitC, unit.StatusComplete) require.NoError(t, err) ready, err = tracker.IsReady(unitB) @@ -215,7 +207,7 @@ func TestDependencyTracker_UpdateStatus(t *testing.T) { assert.False(t, ready) // Update B to "started" - A should become ready - err = tracker.UpdateStatus(unitB, statusStarted) + err = tracker.UpdateStatus(unitB, unit.StatusStarted) require.NoError(t, err) ready, err = tracker.IsReady(unitA) @@ -230,7 +222,7 @@ func TestDependencyTracker_GetUnmetDependencies(t *testing.T) { t.Run("GetUnmetDependenciesForUnitWithNoDependencies", func(t *testing.T) { t.Parallel() - tracker := unit.NewManager[testStatus, testUnitID]() + tracker := unit.NewManager[string, testUnitID]() err := tracker.Register(unitA) require.NoError(t, err) @@ -242,14 +234,14 @@ func TestDependencyTracker_GetUnmetDependencies(t *testing.T) { t.Run("GetUnmetDependenciesForUnitWithUnsatisfiedDependencies", func(t *testing.T) { t.Parallel() - tracker := unit.NewManager[testStatus, testUnitID]() + tracker := unit.NewManager[string, testUnitID]() err := tracker.Register(unitA) require.NoError(t, err) err = tracker.Register(unitB) require.NoError(t, err) - // A depends on B being "running" - err = tracker.AddDependency(unitA, unitB, statusRunning) + // A depends on B being unit.StatusStarted + err = tracker.AddDependency(unitA, unitB, unit.StatusStarted) require.NoError(t, err) unmet, err := tracker.GetUnmetDependencies(unitA) @@ -258,25 +250,25 @@ func TestDependencyTracker_GetUnmetDependencies(t *testing.T) { assert.Equal(t, unitA, unmet[0].Unit) assert.Equal(t, unitB, unmet[0].DependsOn) - assert.Equal(t, statusRunning, unmet[0].RequiredStatus) + assert.Equal(t, unit.StatusStarted, unmet[0].RequiredStatus) assert.False(t, unmet[0].IsSatisfied) }) t.Run("GetUnmetDependenciesForUnitWithSatisfiedDependencies", func(t *testing.T) { t.Parallel() - tracker := unit.NewManager[testStatus, testUnitID]() + tracker := unit.NewManager[string, testUnitID]() err := tracker.Register(unitA) require.NoError(t, err) err = tracker.Register(unitB) require.NoError(t, err) - // A depends on B being "running" - err = tracker.AddDependency(unitA, unitB, statusRunning) + // A depends on B being unit.StatusStarted + err = tracker.AddDependency(unitA, unitB, unit.StatusStarted) require.NoError(t, err) - // Update B to "running" - err = tracker.UpdateStatus(unitB, statusRunning) + // Update B to unit.StatusStarted + err = tracker.UpdateStatus(unitB, unit.StatusStarted) require.NoError(t, err) unmet, err := tracker.GetUnmetDependencies(unitA) @@ -287,7 +279,7 @@ func TestDependencyTracker_GetUnmetDependencies(t *testing.T) { t.Run("GetUnmetDependenciesForUnregisteredUnit", func(t *testing.T) { t.Parallel() - tracker := unit.NewManager[testStatus, testUnitID]() + tracker := unit.NewManager[string, testUnitID]() unmet, err := tracker.GetUnmetDependencies(unitA) require.Error(t, err) @@ -302,7 +294,7 @@ func TestDependencyTracker_ConcurrentOperations(t *testing.T) { t.Run("ConcurrentStatusUpdates", func(t *testing.T) { t.Parallel() - tracker := unit.NewManager[testStatus, testUnitID]() + tracker := unit.NewManager[string, testUnitID]() // Register units units := []testUnitID{unitA, unitB, unitC, unitD} @@ -312,11 +304,11 @@ func TestDependencyTracker_ConcurrentOperations(t *testing.T) { } // Create dependencies: A depends on B, B depends on C, C depends on D - err := tracker.AddDependency(unitA, unitB, statusRunning) + err := tracker.AddDependency(unitA, unitB, unit.StatusStarted) require.NoError(t, err) - err = tracker.AddDependency(unitB, unitC, statusStarted) + err = tracker.AddDependency(unitB, unitC, unit.StatusStarted) require.NoError(t, err) - err = tracker.AddDependency(unitC, unitD, statusCompleted) + err = tracker.AddDependency(unitC, unitD, unit.StatusComplete) require.NoError(t, err) var wg sync.WaitGroup @@ -330,21 +322,21 @@ func TestDependencyTracker_ConcurrentOperations(t *testing.T) { defer wg.Done() // Update D to completed (should make C ready) - err := tracker.UpdateStatus(unitD, statusCompleted) + err := tracker.UpdateStatus(unitD, unit.StatusComplete) if err != nil { goroutineErrors[goroutineID] = err return } // Update C to started (should make B ready) - err = tracker.UpdateStatus(unitC, statusStarted) + err = tracker.UpdateStatus(unitC, unit.StatusStarted) if err != nil { goroutineErrors[goroutineID] = err return } // Update B to running (should make A ready) - err = tracker.UpdateStatus(unitB, statusRunning) + err = tracker.UpdateStatus(unitB, unit.StatusStarted) if err != nil { goroutineErrors[goroutineID] = err return @@ -373,7 +365,7 @@ func TestDependencyTracker_ConcurrentOperations(t *testing.T) { t.Run("ConcurrentReadinessChecks", func(t *testing.T) { t.Parallel() - tracker := unit.NewManager[testStatus, testUnitID]() + tracker := unit.NewManager[string, testUnitID]() // Register units err := tracker.Register(unitA) @@ -381,8 +373,8 @@ func TestDependencyTracker_ConcurrentOperations(t *testing.T) { err = tracker.Register(unitB) require.NoError(t, err) - // A depends on B being "running" - err = tracker.AddDependency(unitA, unitB, statusRunning) + // A depends on B being unit.StatusStarted + err = tracker.AddDependency(unitA, unitB, unit.StatusStarted) require.NoError(t, err) var wg sync.WaitGroup @@ -409,8 +401,8 @@ func TestDependencyTracker_ConcurrentOperations(t *testing.T) { }(i) } - // Update B to "running" in the middle of readiness checks - err = tracker.UpdateStatus(unitB, statusRunning) + // Update B to unit.StatusStarted in the middle of readiness checks + err = tracker.UpdateStatus(unitB, unit.StatusStarted) require.NoError(t, err) wg.Wait() @@ -423,7 +415,7 @@ func TestDependencyTracker_MultipleDependencies(t *testing.T) { t.Run("UnitWithMultipleDependencies", func(t *testing.T) { t.Parallel() - tracker := unit.NewManager[testStatus, testUnitID]() + tracker := unit.NewManager[string, testUnitID]() // Register all units units := []testUnitID{unitA, unitB, unitC, unitD} @@ -432,10 +424,10 @@ func TestDependencyTracker_MultipleDependencies(t *testing.T) { require.NoError(t, err) } - // A depends on B being "running" AND C being "started" - err := tracker.AddDependency(unitA, unitB, statusRunning) + // A depends on B being unit.StatusStarted AND C being "started" + err := tracker.AddDependency(unitA, unitB, unit.StatusStarted) require.NoError(t, err) - err = tracker.AddDependency(unitA, unitC, statusStarted) + err = tracker.AddDependency(unitA, unitC, unit.StatusStarted) require.NoError(t, err) // A should not be ready (depends on both B and C) @@ -443,8 +435,8 @@ func TestDependencyTracker_MultipleDependencies(t *testing.T) { require.NoError(t, err) assert.False(t, ready) - // Update B to "running" - A should still not be ready (needs C too) - err = tracker.UpdateStatus(unitB, statusRunning) + // Update B to unit.StatusStarted - A should still not be ready (needs C too) + err = tracker.UpdateStatus(unitB, unit.StatusStarted) require.NoError(t, err) ready, err = tracker.IsReady(unitA) @@ -452,7 +444,7 @@ func TestDependencyTracker_MultipleDependencies(t *testing.T) { assert.False(t, ready) // Update C to "started" - A should now be ready - err = tracker.UpdateStatus(unitC, statusStarted) + err = tracker.UpdateStatus(unitC, unit.StatusStarted) require.NoError(t, err) ready, err = tracker.IsReady(unitA) @@ -463,7 +455,7 @@ func TestDependencyTracker_MultipleDependencies(t *testing.T) { t.Run("ComplexDependencyChain", func(t *testing.T) { t.Parallel() - tracker := unit.NewManager[testStatus, testUnitID]() + tracker := unit.NewManager[string, testUnitID]() // Register all units units := []testUnitID{unitA, unitB, unitC, unitD} @@ -473,16 +465,16 @@ func TestDependencyTracker_MultipleDependencies(t *testing.T) { } // Create complex dependency graph: - // A depends on B being "running" AND C being "started" + // A depends on B being unit.StatusStarted AND C being "started" // B depends on D being "completed" // C depends on D being "completed" - err := tracker.AddDependency(unitA, unitB, statusRunning) + err := tracker.AddDependency(unitA, unitB, unit.StatusStarted) require.NoError(t, err) - err = tracker.AddDependency(unitA, unitC, statusStarted) + err = tracker.AddDependency(unitA, unitC, unit.StatusStarted) require.NoError(t, err) - err = tracker.AddDependency(unitB, unitD, statusCompleted) + err = tracker.AddDependency(unitB, unitD, unit.StatusComplete) require.NoError(t, err) - err = tracker.AddDependency(unitC, unitD, statusCompleted) + err = tracker.AddDependency(unitC, unitD, unit.StatusComplete) require.NoError(t, err) // Initially only D is ready @@ -503,7 +495,7 @@ func TestDependencyTracker_MultipleDependencies(t *testing.T) { assert.False(t, ready) // Update D to "completed" - B and C should become ready - err = tracker.UpdateStatus(unitD, statusCompleted) + err = tracker.UpdateStatus(unitD, unit.StatusComplete) require.NoError(t, err) ready, err = tracker.IsReady(unitB) @@ -518,8 +510,8 @@ func TestDependencyTracker_MultipleDependencies(t *testing.T) { require.NoError(t, err) assert.False(t, ready) - // Update B to "running" - A should still not be ready (needs C) - err = tracker.UpdateStatus(unitB, statusRunning) + // Update B to unit.StatusStarted - A should still not be ready (needs C) + err = tracker.UpdateStatus(unitB, unit.StatusStarted) require.NoError(t, err) ready, err = tracker.IsReady(unitA) @@ -527,7 +519,7 @@ func TestDependencyTracker_MultipleDependencies(t *testing.T) { assert.False(t, ready) // Update C to "started" - A should now be ready - err = tracker.UpdateStatus(unitC, statusStarted) + err = tracker.UpdateStatus(unitC, unit.StatusStarted) require.NoError(t, err) ready, err = tracker.IsReady(unitA) @@ -538,7 +530,7 @@ func TestDependencyTracker_MultipleDependencies(t *testing.T) { t.Run("DifferentStatusTypes", func(t *testing.T) { t.Parallel() - tracker := unit.NewManager[testStatus, testUnitID]() + tracker := unit.NewManager[string, testUnitID]() // Register units err := tracker.Register(unitA) @@ -548,14 +540,14 @@ func TestDependencyTracker_MultipleDependencies(t *testing.T) { err = tracker.Register(unitC) require.NoError(t, err) - // A depends on B being "running" AND C being "completed" - err = tracker.AddDependency(unitA, unitB, statusRunning) + // A depends on B being unit.StatusStarted AND C being "completed" + err = tracker.AddDependency(unitA, unitB, unit.StatusStarted) require.NoError(t, err) - err = tracker.AddDependency(unitA, unitC, statusCompleted) + err = tracker.AddDependency(unitA, unitC, unit.StatusComplete) require.NoError(t, err) - // Update B to "running" but not C - A should not be ready - err = tracker.UpdateStatus(unitB, statusRunning) + // Update B to unit.StatusStarted but not C - A should not be ready + err = tracker.UpdateStatus(unitB, unit.StatusStarted) require.NoError(t, err) ready, err := tracker.IsReady(unitA) @@ -563,7 +555,7 @@ func TestDependencyTracker_MultipleDependencies(t *testing.T) { assert.False(t, ready) // Update C to "completed" - A should now be ready - err = tracker.UpdateStatus(unitC, statusCompleted) + err = tracker.UpdateStatus(unitC, unit.StatusComplete) require.NoError(t, err) ready, err = tracker.IsReady(unitA) @@ -578,9 +570,9 @@ func TestDependencyTracker_ErrorCases(t *testing.T) { t.Run("UpdateStatusWithUnregisteredUnit", func(t *testing.T) { t.Parallel() - tracker := unit.NewManager[testStatus, testUnitID]() + tracker := unit.NewManager[string, testUnitID]() - err := tracker.UpdateStatus(unitA, statusRunning) + err := tracker.UpdateStatus(unitA, unit.StatusStarted) require.Error(t, err) assert.Equal(t, unit.ErrUnitNotFound, err) }) @@ -588,7 +580,7 @@ func TestDependencyTracker_ErrorCases(t *testing.T) { t.Run("IsReadyWithUnregisteredUnit", func(t *testing.T) { t.Parallel() - tracker := unit.NewManager[testStatus, testUnitID]() + tracker := unit.NewManager[string, testUnitID]() ready, err := tracker.IsReady(unitA) require.Error(t, err) @@ -599,7 +591,7 @@ func TestDependencyTracker_ErrorCases(t *testing.T) { t.Run("GetUnmetDependenciesWithUnregisteredUnit", func(t *testing.T) { t.Parallel() - tracker := unit.NewManager[testStatus, testUnitID]() + tracker := unit.NewManager[string, testUnitID]() unmet, err := tracker.GetUnmetDependencies(unitA) require.Error(t, err) @@ -610,10 +602,10 @@ func TestDependencyTracker_ErrorCases(t *testing.T) { t.Run("AddDependencyWithUnregisteredUnits", func(t *testing.T) { t.Parallel() - tracker := unit.NewManager[testStatus, testUnitID]() + tracker := unit.NewManager[string, testUnitID]() // Try to add dependency with unregistered units - err := tracker.AddDependency(unitA, unitB, statusRunning) + err := tracker.AddDependency(unitA, unitB, unit.StatusStarted) require.Error(t, err) assert.Contains(t, err.Error(), "not registered") }) @@ -621,7 +613,7 @@ func TestDependencyTracker_ErrorCases(t *testing.T) { t.Run("CyclicDependencyDetection", func(t *testing.T) { t.Parallel() - tracker := unit.NewManager[testStatus, testUnitID]() + tracker := unit.NewManager[string, testUnitID]() // Register units err := tracker.Register(unitA) @@ -630,11 +622,11 @@ func TestDependencyTracker_ErrorCases(t *testing.T) { require.NoError(t, err) // A depends on B - err = tracker.AddDependency(unitA, unitB, statusRunning) + err = tracker.AddDependency(unitA, unitB, unit.StatusStarted) require.NoError(t, err) // Try to make B depend on A (creates cycle) - err = tracker.AddDependency(unitB, unitA, statusStarted) + err = tracker.AddDependency(unitB, unitA, unit.StatusStarted) require.Error(t, err) assert.Contains(t, err.Error(), "would create a cycle") }) @@ -646,7 +638,7 @@ func TestDependencyTracker_ToDOT(t *testing.T) { t.Run("ExportSimpleGraph", func(t *testing.T) { t.Parallel() - tracker := unit.NewManager[testStatus, testUnitID]() + tracker := unit.NewManager[string, testUnitID]() // Register units err := tracker.Register(unitA) @@ -655,7 +647,7 @@ func TestDependencyTracker_ToDOT(t *testing.T) { require.NoError(t, err) // Add dependency - err = tracker.AddDependency(unitA, unitB, statusRunning) + err = tracker.AddDependency(unitA, unitB, unit.StatusStarted) require.NoError(t, err) dot, err := tracker.ExportDOT("test") @@ -667,7 +659,7 @@ func TestDependencyTracker_ToDOT(t *testing.T) { t.Run("ExportComplexGraph", func(t *testing.T) { t.Parallel() - tracker := unit.NewManager[testStatus, testUnitID]() + tracker := unit.NewManager[string, testUnitID]() // Register all units units := []testUnitID{unitA, unitB, unitC, unitD} @@ -678,13 +670,13 @@ func TestDependencyTracker_ToDOT(t *testing.T) { // Create complex dependency graph // A depends on B and C, B depends on D, C depends on D - err := tracker.AddDependency(unitA, unitB, statusRunning) + err := tracker.AddDependency(unitA, unitB, unit.StatusStarted) require.NoError(t, err) - err = tracker.AddDependency(unitA, unitC, statusStarted) + err = tracker.AddDependency(unitA, unitC, unit.StatusStarted) require.NoError(t, err) - err = tracker.AddDependency(unitB, unitD, statusCompleted) + err = tracker.AddDependency(unitB, unitD, unit.StatusComplete) require.NoError(t, err) - err = tracker.AddDependency(unitC, unitD, statusCompleted) + err = tracker.AddDependency(unitC, unitD, unit.StatusComplete) require.NoError(t, err) dot, err := tracker.ExportDOT("complex") From e55e3c877e972a80bf2085f8e53042ee49370bd0 Mon Sep 17 00:00:00 2001 From: Sas Swart Date: Wed, 12 Nov 2025 11:09:34 +0000 Subject: [PATCH 05/14] remove low value tests --- agent/unit/manager_test.go | 123 ------------------------------------- 1 file changed, 123 deletions(-) diff --git a/agent/unit/manager_test.go b/agent/unit/manager_test.go index c936586fdf297..3915f9e755a16 100644 --- a/agent/unit/manager_test.go +++ b/agent/unit/manager_test.go @@ -1,8 +1,6 @@ package unit_test import ( - "errors" - "sync" "testing" "github.com/stretchr/testify/assert" @@ -288,127 +286,6 @@ func TestDependencyTracker_GetUnmetDependencies(t *testing.T) { }) } -func TestDependencyTracker_ConcurrentOperations(t *testing.T) { - t.Parallel() - - t.Run("ConcurrentStatusUpdates", func(t *testing.T) { - t.Parallel() - - tracker := unit.NewManager[string, testUnitID]() - - // Register units - units := []testUnitID{unitA, unitB, unitC, unitD} - for _, unit := range units { - err := tracker.Register(unit) - require.NoError(t, err) - } - - // Create dependencies: A depends on B, B depends on C, C depends on D - err := tracker.AddDependency(unitA, unitB, unit.StatusStarted) - require.NoError(t, err) - err = tracker.AddDependency(unitB, unitC, unit.StatusStarted) - require.NoError(t, err) - err = tracker.AddDependency(unitC, unitD, unit.StatusComplete) - require.NoError(t, err) - - var wg sync.WaitGroup - const numGoroutines = 10 - - // Launch goroutines that update statuses - goroutineErrors := make([]error, numGoroutines) - for i := 0; i < numGoroutines; i++ { - wg.Add(1) - go func(goroutineID int) { - defer wg.Done() - - // Update D to completed (should make C ready) - err := tracker.UpdateStatus(unitD, unit.StatusComplete) - if err != nil { - goroutineErrors[goroutineID] = err - return - } - - // Update C to started (should make B ready) - err = tracker.UpdateStatus(unitC, unit.StatusStarted) - if err != nil { - goroutineErrors[goroutineID] = err - return - } - - // Update B to running (should make A ready) - err = tracker.UpdateStatus(unitB, unit.StatusStarted) - if err != nil { - goroutineErrors[goroutineID] = err - return - } - }(i) - } - - wg.Wait() - - // Check for any errors in goroutines - // ErrSameStatusAlreadySet is expected when multiple goroutines try to set the same status - for i, err := range goroutineErrors { - if err != nil && !errors.Is(err, unit.ErrSameStatusAlreadySet) { - require.NoError(t, err, "goroutine %d had unexpected error", i) - } - } - - // All units should be ready after the updates - for _, unit := range units { - ready, err := tracker.IsReady(unit) - require.NoError(t, err) - assert.True(t, ready) - } - }) - - t.Run("ConcurrentReadinessChecks", func(t *testing.T) { - t.Parallel() - - tracker := unit.NewManager[string, testUnitID]() - - // Register units - err := tracker.Register(unitA) - require.NoError(t, err) - err = tracker.Register(unitB) - require.NoError(t, err) - - // A depends on B being unit.StatusStarted - err = tracker.AddDependency(unitA, unitB, unit.StatusStarted) - require.NoError(t, err) - - var wg sync.WaitGroup - const numGoroutines = 20 - - // Launch goroutines that check readiness - for i := 0; i < numGoroutines; i++ { - wg.Add(1) - go func(goroutineID int) { - defer wg.Done() - - // Check readiness multiple times - for j := 0; j < 10; j++ { - ready, err := tracker.IsReady(unitA) - require.NoError(t, err) - // Initially should be false, then true after B is updated - _ = ready - - ready, err = tracker.IsReady(unitB) - require.NoError(t, err) - // B should always be ready (no dependencies) - assert.True(t, ready) - } - }(i) - } - - // Update B to unit.StatusStarted in the middle of readiness checks - err = tracker.UpdateStatus(unitB, unit.StatusStarted) - require.NoError(t, err) - - wg.Wait() - }) -} - func TestDependencyTracker_MultipleDependencies(t *testing.T) { t.Parallel() From e64eff5db3b44af2ba0c93c8940c4f597193ec61 Mon Sep 17 00:00:00 2001 From: Sas Swart Date: Fri, 14 Nov 2025 09:43:31 +0000 Subject: [PATCH 06/14] tidy up variable declaration --- agent/unit/manager.go | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/agent/unit/manager.go b/agent/unit/manager.go index 581623f6a9cba..699952a322fdf 100644 --- a/agent/unit/manager.go +++ b/agent/unit/manager.go @@ -6,20 +6,22 @@ import ( "golang.org/x/xerrors" ) -// ErrUnitNotFound is returned when a unit ID is not registered. -var ErrUnitNotFound = xerrors.New("unit not found") +var ( + // ErrUnitNotFound is returned when a unit ID is not registered. + ErrUnitNotFound = xerrors.New("unit not found") -// ErrUnitAlreadyRegistered is returned when a unit ID is already registered. -var ErrUnitAlreadyRegistered = xerrors.New("unit already registered") + // ErrUnitAlreadyRegistered is returned when a unit ID is already registered. + ErrUnitAlreadyRegistered = xerrors.New("unit already registered") -// ErrCannotUpdateOtherUnit is returned when attempting to update another unit's status. -var ErrCannotUpdateOtherUnit = xerrors.New("cannot update other unit's status") + // ErrCannotUpdateOtherUnit is returned when attempting to update another unit's status. + ErrCannotUpdateOtherUnit = xerrors.New("cannot update other unit's status") -// ErrDependenciesNotSatisfied is returned when a unit's dependencies are not satisfied. -var ErrDependenciesNotSatisfied = xerrors.New("unit dependencies not satisfied") + // ErrDependenciesNotSatisfied is returned when a unit's dependencies are not satisfied. + ErrDependenciesNotSatisfied = xerrors.New("unit dependencies not satisfied") -// ErrSameStatusAlreadySet is returned when attempting to set the same status as the current status. -var ErrSameStatusAlreadySet = xerrors.New("same status already set") + // ErrSameStatusAlreadySet is returned when attempting to set the same status as the current status. + ErrSameStatusAlreadySet = xerrors.New("same status already set") +) // Status constants for dependency tracking const ( From 97e52258553ebca639602901eeb30dd7a2bb6aff Mon Sep 17 00:00:00 2001 From: Sas Swart Date: Fri, 14 Nov 2025 09:45:34 +0000 Subject: [PATCH 07/14] ensure we catch indirect cycles --- agent/unit/manager_test.go | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/agent/unit/manager_test.go b/agent/unit/manager_test.go index 3915f9e755a16..24cf491b7c431 100644 --- a/agent/unit/manager_test.go +++ b/agent/unit/manager_test.go @@ -497,15 +497,26 @@ func TestDependencyTracker_ErrorCases(t *testing.T) { require.NoError(t, err) err = tracker.Register(unitB) require.NoError(t, err) + err = tracker.Register(unitC) + require.NoError(t, err) + err = tracker.Register(unitD) + require.NoError(t, err) // A depends on B err = tracker.AddDependency(unitA, unitB, unit.StatusStarted) require.NoError(t, err) + // B depends on C + err = tracker.AddDependency(unitB, unitC, unit.StatusStarted) + require.NoError(t, err) + + // C depends on D + err = tracker.AddDependency(unitC, unitD, unit.StatusStarted) + require.NoError(t, err) - // Try to make B depend on A (creates cycle) - err = tracker.AddDependency(unitB, unitA, unit.StatusStarted) + // Try to make D depend on A (creates indirect cycle) + err = tracker.AddDependency(unitD, unitA, unit.StatusStarted) require.Error(t, err) - assert.Contains(t, err.Error(), "would create a cycle") + assert.ErrorContains(t, err, "would create a cycle") }) } From 0b1b90382d09237ace2edca1efd4b32353a4820e Mon Sep 17 00:00:00 2001 From: Sas Swart Date: Fri, 14 Nov 2025 09:51:37 +0000 Subject: [PATCH 08/14] deduplicate agent fecthing logic --- agent/unit/manager.go | 40 ++++++---------------------------------- 1 file changed, 6 insertions(+), 34 deletions(-) diff --git a/agent/unit/manager.go b/agent/unit/manager.go index 699952a322fdf..f36ce959e475a 100644 --- a/agent/unit/manager.go +++ b/agent/unit/manager.go @@ -165,44 +165,16 @@ func (dt *Manager[StatusType, UnitID]) IsReady(unit UnitID) (bool, error) { // GetUnmetDependencies returns a list of unsatisfied dependencies for a unit. func (dt *Manager[StatusType, UnitID]) GetUnmetDependencies(unit UnitID) ([]Dependency[StatusType, UnitID], error) { - dt.mu.RLock() - defer dt.mu.RUnlock() - - if !dt.registeredUnits[unit] { - return nil, ErrUnitNotFound + allDependencies, err := dt.GetAllDependencies(unit) + if err != nil { + return nil, err } - unitVertex := dt.unitVertices[unit] - forwardEdges := dt.graph.GetForwardAdjacentVertices(unitVertex) - var unmetDependencies []Dependency[StatusType, UnitID] - for _, edge := range forwardEdges { - dependsOnUnit := edge.To.ID - requiredStatus := edge.Edge - currentStatus, exists := dt.unitStatus[dependsOnUnit] - if !exists { - // If the dependency unit has no status, it's not satisfied - // Zero value represents StatusPending - var pendingStatus StatusType - unmetDependencies = append(unmetDependencies, Dependency[StatusType, UnitID]{ - Unit: unit, - DependsOn: dependsOnUnit, - RequiredStatus: requiredStatus, - CurrentStatus: pendingStatus, // StatusPending (zero value) - IsSatisfied: false, - }) - } else { - isSatisfied := currentStatus == requiredStatus - if !isSatisfied { - unmetDependencies = append(unmetDependencies, Dependency[StatusType, UnitID]{ - Unit: unit, - DependsOn: dependsOnUnit, - RequiredStatus: requiredStatus, - CurrentStatus: currentStatus, - IsSatisfied: false, - }) - } + for _, dependency := range allDependencies { + if !dependency.IsSatisfied { + unmetDependencies = append(unmetDependencies, dependency) } } From c9166bed0371df11565f2eb32677aeb754c073ed Mon Sep 17 00:00:00 2001 From: Sas Swart Date: Fri, 14 Nov 2025 10:01:58 +0000 Subject: [PATCH 09/14] properly utilize unit sentinel errors --- agent/unit/graph.go | 2 +- agent/unit/manager.go | 7 +++++-- agent/unit/manager_test.go | 24 ++++++++---------------- 3 files changed, 14 insertions(+), 19 deletions(-) diff --git a/agent/unit/graph.go b/agent/unit/graph.go index 3d8a6703addf2..e9388680c10d1 100644 --- a/agent/unit/graph.go +++ b/agent/unit/graph.go @@ -58,7 +58,7 @@ func (g *Graph[EdgeType, VertexType]) AddEdge(from, to VertexType, edge EdgeType toID := g.getOrCreateVertexID(to) if g.canReach(to, from) { - return xerrors.Errorf("adding edge (%v -> %v) would create a cycle", from, to) + return xerrors.Errorf("adding edge (%v -> %v): %w", from, to, ErrCycleDetected) } g.gonumGraph.SetEdge(simple.Edge{F: simple.Node(fromID), T: simple.Node(toID)}) diff --git a/agent/unit/manager.go b/agent/unit/manager.go index f36ce959e475a..71e11331cb8d6 100644 --- a/agent/unit/manager.go +++ b/agent/unit/manager.go @@ -21,6 +21,9 @@ var ( // ErrSameStatusAlreadySet is returned when attempting to set the same status as the current status. ErrSameStatusAlreadySet = xerrors.New("same status already set") + + // ErrCycleDetected is returned when a cycle is detected in the dependency graph. + ErrCycleDetected = xerrors.New("cycle detected") ) // Status constants for dependency tracking @@ -101,10 +104,10 @@ func (dt *Manager[StatusType, UnitID]) AddDependency(unit UnitID, dependsOn Unit defer dt.mu.Unlock() if !dt.registeredUnits[unit] { - return xerrors.Errorf("unit %v is not registered", unit) + return xerrors.Errorf("unit %v is not registered: %w", unit, ErrUnitNotFound) } if !dt.registeredUnits[dependsOn] { - return xerrors.Errorf("unit %v is not registered", dependsOn) + return xerrors.Errorf("unit %v is not registered: %w", dependsOn, ErrUnitNotFound) } // Get the stored vertices for both units diff --git a/agent/unit/manager_test.go b/agent/unit/manager_test.go index 24cf491b7c431..55f6560c2728a 100644 --- a/agent/unit/manager_test.go +++ b/agent/unit/manager_test.go @@ -43,8 +43,7 @@ func TestDependencyTracker_Register(t *testing.T) { require.NoError(t, err) err = tracker.Register(unitA) - require.Error(t, err) - assert.Contains(t, err.Error(), "already registered") + require.ErrorIs(t, err, unit.ErrUnitAlreadyRegistered) }) t.Run("RegisterMultipleUnits", func(t *testing.T) { @@ -103,8 +102,7 @@ func TestDependencyTracker_AddDependency(t *testing.T) { // Try to add dependency to unregistered unit err = tracker.AddDependency(unitA, unitB, unit.StatusStarted) - require.Error(t, err) - assert.Contains(t, err.Error(), "not registered") + require.ErrorIs(t, err, unit.ErrUnitNotFound) }) t.Run("AddDependencyFromUnregisteredUnit", func(t *testing.T) { @@ -116,8 +114,7 @@ func TestDependencyTracker_AddDependency(t *testing.T) { // Try to add dependency from unregistered unit err = tracker.AddDependency(unitA, unitB, unit.StatusStarted) - require.Error(t, err) - assert.Contains(t, err.Error(), "not registered") + require.ErrorIs(t, err, unit.ErrUnitNotFound) }) } @@ -157,8 +154,7 @@ func TestDependencyTracker_UpdateStatus(t *testing.T) { tracker := unit.NewManager[string, testUnitID]() err := tracker.UpdateStatus(unitA, unit.StatusStarted) - require.Error(t, err) - assert.Equal(t, unit.ErrUnitNotFound, err) + require.ErrorIs(t, err, unit.ErrUnitNotFound) }) t.Run("LinearChainDependencies", func(t *testing.T) { @@ -280,8 +276,7 @@ func TestDependencyTracker_GetUnmetDependencies(t *testing.T) { tracker := unit.NewManager[string, testUnitID]() unmet, err := tracker.GetUnmetDependencies(unitA) - require.Error(t, err) - assert.Equal(t, unit.ErrUnitNotFound, err) + require.ErrorIs(t, err, unit.ErrUnitNotFound) assert.Nil(t, unmet) }) } @@ -450,8 +445,7 @@ func TestDependencyTracker_ErrorCases(t *testing.T) { tracker := unit.NewManager[string, testUnitID]() err := tracker.UpdateStatus(unitA, unit.StatusStarted) - require.Error(t, err) - assert.Equal(t, unit.ErrUnitNotFound, err) + require.ErrorIs(t, err, unit.ErrUnitNotFound) }) t.Run("IsReadyWithUnregisteredUnit", func(t *testing.T) { @@ -460,8 +454,7 @@ func TestDependencyTracker_ErrorCases(t *testing.T) { tracker := unit.NewManager[string, testUnitID]() ready, err := tracker.IsReady(unitA) - require.Error(t, err) - assert.Equal(t, unit.ErrUnitNotFound, err) + require.ErrorIs(t, err, unit.ErrUnitNotFound) assert.False(t, ready) }) @@ -515,8 +508,7 @@ func TestDependencyTracker_ErrorCases(t *testing.T) { // Try to make D depend on A (creates indirect cycle) err = tracker.AddDependency(unitD, unitA, unit.StatusStarted) - require.Error(t, err) - assert.ErrorContains(t, err, "would create a cycle") + require.ErrorIs(t, err, unit.ErrCycleDetected) }) } From 62cb94890e6d1a1039dee182b5a441b271fa29c5 Mon Sep 17 00:00:00 2001 From: Sas Swart Date: Fri, 14 Nov 2025 10:14:29 +0000 Subject: [PATCH 10/14] Provide proper type information to unit status constants --- agent/unit/graph_test.go | 8 ++--- agent/unit/manager.go | 62 ++++++++++++++++++-------------------- agent/unit/manager_test.go | 46 ++++++++++++++-------------- 3 files changed, 56 insertions(+), 60 deletions(-) diff --git a/agent/unit/graph_test.go b/agent/unit/graph_test.go index 3c76756aee88c..f7d1117be74b3 100644 --- a/agent/unit/graph_test.go +++ b/agent/unit/graph_test.go @@ -148,8 +148,7 @@ func TestGraph(t *testing.T) { graph := &testGraph{} unit1 := &testGraphVertex{Name: "unit1"} err := graph.AddEdge(unit1, unit1, testEdgeCompleted) - require.Error(t, err) - require.ErrorContains(t, err, fmt.Sprintf("adding edge (%v -> %v) would create a cycle", unit1, unit1)) + require.ErrorIs(t, err, unit.ErrCycleDetected) return graph }, @@ -160,8 +159,7 @@ func TestGraph(t *testing.T) { err := graph.AddEdge(unit1, unit2, testEdgeCompleted) require.NoError(t, err) err = graph.AddEdge(unit2, unit1, testEdgeStarted) - require.Error(t, err) - require.ErrorContains(t, err, fmt.Sprintf("adding edge (%v -> %v) would create a cycle", unit2, unit1)) + require.ErrorIs(t, err, unit.ErrCycleDetected) return graph }, @@ -341,7 +339,7 @@ func TestGraphThreadSafety(t *testing.T) { // Verify all attempts correctly returned cycle error for i, err := range cycleErrors { require.Error(t, err, "goroutine %d should have detected cycle", i) - require.Contains(t, err.Error(), "would create a cycle") + require.ErrorIs(t, err, unit.ErrCycleDetected) } // Verify graph remains valid (original chain intact) diff --git a/agent/unit/manager.go b/agent/unit/manager.go index 71e11331cb8d6..bc275356ad140 100644 --- a/agent/unit/manager.go +++ b/agent/unit/manager.go @@ -26,10 +26,14 @@ var ( ErrCycleDetected = xerrors.New("cycle detected") ) +// Status represents the status of a unit. +type Status string + // Status constants for dependency tracking const ( - StatusStarted = "started" - StatusComplete = "completed" + StatusPending Status = "" + StatusStarted Status = "started" + StatusComplete Status = "completed" ) // dependencyVertex represents a vertex in the dependency graph that is associated with a unit. @@ -49,14 +53,14 @@ type Dependency[StatusType, UnitID comparable] struct { // Manager provides reactive dependency tracking over a Graph. // It manages unit registration, dependency relationships, and status updates // with automatic recalculation of readiness when dependencies are satisfied. -type Manager[StatusType, UnitID comparable] struct { +type Manager[UnitID comparable] struct { mu sync.RWMutex // The underlying graph that stores dependency relationships - graph *Graph[StatusType, *dependencyVertex[UnitID]] + graph *Graph[Status, *dependencyVertex[UnitID]] // Track current status of each unit - unitStatus map[UnitID]StatusType + unitStatus map[UnitID]Status // Track readiness state (cached to avoid repeated graph traversal) unitReadiness map[UnitID]bool @@ -69,10 +73,10 @@ type Manager[StatusType, UnitID comparable] struct { } // NewManager creates a new Manager instance. -func NewManager[StatusType, UnitID comparable]() *Manager[StatusType, UnitID] { - return &Manager[StatusType, UnitID]{ - graph: &Graph[StatusType, *dependencyVertex[UnitID]]{}, - unitStatus: make(map[UnitID]StatusType), +func NewManager[UnitID comparable]() *Manager[UnitID] { + return &Manager[UnitID]{ + graph: &Graph[Status, *dependencyVertex[UnitID]]{}, + unitStatus: make(map[UnitID]Status), unitReadiness: make(map[UnitID]bool), registeredUnits: make(map[UnitID]bool), unitVertices: make(map[UnitID]*dependencyVertex[UnitID]), @@ -80,7 +84,7 @@ func NewManager[StatusType, UnitID comparable]() *Manager[StatusType, UnitID] { } // Register registers a new unit as a vertex in the dependency graph. -func (dt *Manager[StatusType, UnitID]) Register(id UnitID) error { +func (dt *Manager[UnitID]) Register(id UnitID) error { dt.mu.Lock() defer dt.mu.Unlock() @@ -99,7 +103,7 @@ func (dt *Manager[StatusType, UnitID]) Register(id UnitID) error { // AddDependency adds a dependency relationship between units. // The unit depends on the dependsOn unit reaching the requiredStatus. -func (dt *Manager[StatusType, UnitID]) AddDependency(unit UnitID, dependsOn UnitID, requiredStatus StatusType) error { +func (dt *Manager[UnitID]) AddDependency(unit UnitID, dependsOn UnitID, requiredStatus Status) error { dt.mu.Lock() defer dt.mu.Unlock() @@ -128,7 +132,7 @@ func (dt *Manager[StatusType, UnitID]) AddDependency(unit UnitID, dependsOn Unit } // UpdateStatus updates a unit's status and recalculates readiness for affected dependents. -func (dt *Manager[StatusType, UnitID]) UpdateStatus(unit UnitID, newStatus StatusType) error { +func (dt *Manager[UnitID]) UpdateStatus(unit UnitID, newStatus Status) error { dt.mu.Lock() defer dt.mu.Unlock() @@ -155,7 +159,7 @@ func (dt *Manager[StatusType, UnitID]) UpdateStatus(unit UnitID, newStatus Statu } // IsReady checks if all dependencies for a unit are satisfied. -func (dt *Manager[StatusType, UnitID]) IsReady(unit UnitID) (bool, error) { +func (dt *Manager[UnitID]) IsReady(unit UnitID) (bool, error) { dt.mu.RLock() defer dt.mu.RUnlock() @@ -167,13 +171,13 @@ func (dt *Manager[StatusType, UnitID]) IsReady(unit UnitID) (bool, error) { } // GetUnmetDependencies returns a list of unsatisfied dependencies for a unit. -func (dt *Manager[StatusType, UnitID]) GetUnmetDependencies(unit UnitID) ([]Dependency[StatusType, UnitID], error) { +func (dt *Manager[UnitID]) GetUnmetDependencies(unit UnitID) ([]Dependency[Status, UnitID], error) { allDependencies, err := dt.GetAllDependencies(unit) if err != nil { return nil, err } - var unmetDependencies []Dependency[StatusType, UnitID] + var unmetDependencies []Dependency[Status, UnitID] for _, dependency := range allDependencies { if !dependency.IsSatisfied { @@ -186,7 +190,7 @@ func (dt *Manager[StatusType, UnitID]) GetUnmetDependencies(unit UnitID) ([]Depe // recalculateReadinessUnsafe recalculates the readiness state for a unit. // This method assumes the caller holds the write lock. -func (dt *Manager[StatusType, UnitID]) recalculateReadinessUnsafe(unit UnitID) { +func (dt *Manager[UnitID]) recalculateReadinessUnsafe(unit UnitID) { unitVertex := dt.unitVertices[unit] forwardEdges := dt.graph.GetForwardAdjacentVertices(unitVertex) @@ -213,33 +217,29 @@ func (dt *Manager[StatusType, UnitID]) recalculateReadinessUnsafe(unit UnitID) { // GetGraph returns the underlying graph for visualization and debugging. // This should be used carefully as it exposes the internal graph structure. -func (dt *Manager[StatusType, UnitID]) GetGraph() *Graph[StatusType, *dependencyVertex[UnitID]] { +func (dt *Manager[UnitID]) GetGraph() *Graph[Status, *dependencyVertex[UnitID]] { return dt.graph } // GetStatus returns the current status of a unit. -func (dt *Manager[StatusType, UnitID]) GetStatus(unit UnitID) (StatusType, error) { +func (dt *Manager[UnitID]) GetStatus(unit UnitID) (Status, error) { dt.mu.RLock() defer dt.mu.RUnlock() if !dt.registeredUnits[unit] { - // Zero value represents StatusPending - var pendingStatus StatusType - return pendingStatus, ErrUnitNotFound + return StatusPending, ErrUnitNotFound } status, exists := dt.unitStatus[unit] if !exists { - // Zero value represents StatusPending - var pendingStatus StatusType - return pendingStatus, nil + return StatusPending, nil } return status, nil } // GetAllDependencies returns all dependencies for a unit, both satisfied and unsatisfied. -func (dt *Manager[StatusType, UnitID]) GetAllDependencies(unit UnitID) ([]Dependency[StatusType, UnitID], error) { +func (dt *Manager[UnitID]) GetAllDependencies(unit UnitID) ([]Dependency[Status, UnitID], error) { dt.mu.RLock() defer dt.mu.RUnlock() @@ -250,7 +250,7 @@ func (dt *Manager[StatusType, UnitID]) GetAllDependencies(unit UnitID) ([]Depend unitVertex := dt.unitVertices[unit] forwardEdges := dt.graph.GetForwardAdjacentVertices(unitVertex) - var allDependencies []Dependency[StatusType, UnitID] + var allDependencies []Dependency[Status, UnitID] for _, edge := range forwardEdges { dependsOnUnit := edge.To.ID @@ -258,18 +258,16 @@ func (dt *Manager[StatusType, UnitID]) GetAllDependencies(unit UnitID) ([]Depend currentStatus, exists := dt.unitStatus[dependsOnUnit] if !exists { // If the dependency unit has no status, it's not satisfied - // Zero value represents StatusPending - var pendingStatus StatusType - allDependencies = append(allDependencies, Dependency[StatusType, UnitID]{ + allDependencies = append(allDependencies, Dependency[Status, UnitID]{ Unit: unit, DependsOn: dependsOnUnit, RequiredStatus: requiredStatus, - CurrentStatus: pendingStatus, // StatusPending (zero value) + CurrentStatus: StatusPending, IsSatisfied: false, }) } else { isSatisfied := currentStatus == requiredStatus - allDependencies = append(allDependencies, Dependency[StatusType, UnitID]{ + allDependencies = append(allDependencies, Dependency[Status, UnitID]{ Unit: unit, DependsOn: dependsOnUnit, RequiredStatus: requiredStatus, @@ -283,6 +281,6 @@ func (dt *Manager[StatusType, UnitID]) GetAllDependencies(unit UnitID) ([]Depend } // ExportDOT exports the dependency graph to DOT format for visualization. -func (dt *Manager[StatusType, UnitID]) ExportDOT(name string) (string, error) { +func (dt *Manager[UnitID]) ExportDOT(name string) (string, error) { return dt.graph.ToDOT(name) } diff --git a/agent/unit/manager_test.go b/agent/unit/manager_test.go index 55f6560c2728a..bc9eca25ae9e3 100644 --- a/agent/unit/manager_test.go +++ b/agent/unit/manager_test.go @@ -21,7 +21,7 @@ const ( func TestDependencyTracker_Register(t *testing.T) { t.Parallel() - tracker := unit.NewManager[string, testUnitID]() + tracker := unit.NewManager[testUnitID]() t.Run("RegisterNewUnit", func(t *testing.T) { t.Parallel() @@ -38,7 +38,7 @@ func TestDependencyTracker_Register(t *testing.T) { t.Run("RegisterDuplicateUnit", func(t *testing.T) { t.Parallel() - tracker := unit.NewManager[string, testUnitID]() + tracker := unit.NewManager[testUnitID]() err := tracker.Register(unitA) require.NoError(t, err) @@ -49,7 +49,7 @@ func TestDependencyTracker_Register(t *testing.T) { t.Run("RegisterMultipleUnits", func(t *testing.T) { t.Parallel() - tracker := unit.NewManager[string, testUnitID]() + tracker := unit.NewManager[testUnitID]() units := []testUnitID{unitA, unitB, unitC} for _, unit := range units { @@ -72,7 +72,7 @@ func TestDependencyTracker_AddDependency(t *testing.T) { t.Run("AddDependencyBetweenRegisteredUnits", func(t *testing.T) { t.Parallel() - tracker := unit.NewManager[string, testUnitID]() + tracker := unit.NewManager[testUnitID]() err := tracker.Register(unitA) require.NoError(t, err) err = tracker.Register(unitB) @@ -96,7 +96,7 @@ func TestDependencyTracker_AddDependency(t *testing.T) { t.Run("AddDependencyWithUnregisteredUnit", func(t *testing.T) { t.Parallel() - tracker := unit.NewManager[string, testUnitID]() + tracker := unit.NewManager[testUnitID]() err := tracker.Register(unitA) require.NoError(t, err) @@ -108,7 +108,7 @@ func TestDependencyTracker_AddDependency(t *testing.T) { t.Run("AddDependencyFromUnregisteredUnit", func(t *testing.T) { t.Parallel() - tracker := unit.NewManager[string, testUnitID]() + tracker := unit.NewManager[testUnitID]() err := tracker.Register(unitB) require.NoError(t, err) @@ -124,7 +124,7 @@ func TestDependencyTracker_UpdateStatus(t *testing.T) { t.Run("UpdateStatusTriggersReadinessRecalculation", func(t *testing.T) { t.Parallel() - tracker := unit.NewManager[string, testUnitID]() + tracker := unit.NewManager[testUnitID]() err := tracker.Register(unitA) require.NoError(t, err) err = tracker.Register(unitB) @@ -151,7 +151,7 @@ func TestDependencyTracker_UpdateStatus(t *testing.T) { t.Run("UpdateStatusWithUnregisteredUnit", func(t *testing.T) { t.Parallel() - tracker := unit.NewManager[string, testUnitID]() + tracker := unit.NewManager[testUnitID]() err := tracker.UpdateStatus(unitA, unit.StatusStarted) require.ErrorIs(t, err, unit.ErrUnitNotFound) @@ -160,7 +160,7 @@ func TestDependencyTracker_UpdateStatus(t *testing.T) { t.Run("LinearChainDependencies", func(t *testing.T) { t.Parallel() - tracker := unit.NewManager[string, testUnitID]() + tracker := unit.NewManager[testUnitID]() // Register all units units := []testUnitID{unitA, unitB, unitC} @@ -216,7 +216,7 @@ func TestDependencyTracker_GetUnmetDependencies(t *testing.T) { t.Run("GetUnmetDependenciesForUnitWithNoDependencies", func(t *testing.T) { t.Parallel() - tracker := unit.NewManager[string, testUnitID]() + tracker := unit.NewManager[testUnitID]() err := tracker.Register(unitA) require.NoError(t, err) @@ -228,7 +228,7 @@ func TestDependencyTracker_GetUnmetDependencies(t *testing.T) { t.Run("GetUnmetDependenciesForUnitWithUnsatisfiedDependencies", func(t *testing.T) { t.Parallel() - tracker := unit.NewManager[string, testUnitID]() + tracker := unit.NewManager[testUnitID]() err := tracker.Register(unitA) require.NoError(t, err) err = tracker.Register(unitB) @@ -251,7 +251,7 @@ func TestDependencyTracker_GetUnmetDependencies(t *testing.T) { t.Run("GetUnmetDependenciesForUnitWithSatisfiedDependencies", func(t *testing.T) { t.Parallel() - tracker := unit.NewManager[string, testUnitID]() + tracker := unit.NewManager[testUnitID]() err := tracker.Register(unitA) require.NoError(t, err) err = tracker.Register(unitB) @@ -273,7 +273,7 @@ func TestDependencyTracker_GetUnmetDependencies(t *testing.T) { t.Run("GetUnmetDependenciesForUnregisteredUnit", func(t *testing.T) { t.Parallel() - tracker := unit.NewManager[string, testUnitID]() + tracker := unit.NewManager[testUnitID]() unmet, err := tracker.GetUnmetDependencies(unitA) require.ErrorIs(t, err, unit.ErrUnitNotFound) @@ -287,7 +287,7 @@ func TestDependencyTracker_MultipleDependencies(t *testing.T) { t.Run("UnitWithMultipleDependencies", func(t *testing.T) { t.Parallel() - tracker := unit.NewManager[string, testUnitID]() + tracker := unit.NewManager[testUnitID]() // Register all units units := []testUnitID{unitA, unitB, unitC, unitD} @@ -327,7 +327,7 @@ func TestDependencyTracker_MultipleDependencies(t *testing.T) { t.Run("ComplexDependencyChain", func(t *testing.T) { t.Parallel() - tracker := unit.NewManager[string, testUnitID]() + tracker := unit.NewManager[testUnitID]() // Register all units units := []testUnitID{unitA, unitB, unitC, unitD} @@ -402,7 +402,7 @@ func TestDependencyTracker_MultipleDependencies(t *testing.T) { t.Run("DifferentStatusTypes", func(t *testing.T) { t.Parallel() - tracker := unit.NewManager[string, testUnitID]() + tracker := unit.NewManager[testUnitID]() // Register units err := tracker.Register(unitA) @@ -442,7 +442,7 @@ func TestDependencyTracker_ErrorCases(t *testing.T) { t.Run("UpdateStatusWithUnregisteredUnit", func(t *testing.T) { t.Parallel() - tracker := unit.NewManager[string, testUnitID]() + tracker := unit.NewManager[testUnitID]() err := tracker.UpdateStatus(unitA, unit.StatusStarted) require.ErrorIs(t, err, unit.ErrUnitNotFound) @@ -451,7 +451,7 @@ func TestDependencyTracker_ErrorCases(t *testing.T) { t.Run("IsReadyWithUnregisteredUnit", func(t *testing.T) { t.Parallel() - tracker := unit.NewManager[string, testUnitID]() + tracker := unit.NewManager[testUnitID]() ready, err := tracker.IsReady(unitA) require.ErrorIs(t, err, unit.ErrUnitNotFound) @@ -461,7 +461,7 @@ func TestDependencyTracker_ErrorCases(t *testing.T) { t.Run("GetUnmetDependenciesWithUnregisteredUnit", func(t *testing.T) { t.Parallel() - tracker := unit.NewManager[string, testUnitID]() + tracker := unit.NewManager[testUnitID]() unmet, err := tracker.GetUnmetDependencies(unitA) require.Error(t, err) @@ -472,7 +472,7 @@ func TestDependencyTracker_ErrorCases(t *testing.T) { t.Run("AddDependencyWithUnregisteredUnits", func(t *testing.T) { t.Parallel() - tracker := unit.NewManager[string, testUnitID]() + tracker := unit.NewManager[testUnitID]() // Try to add dependency with unregistered units err := tracker.AddDependency(unitA, unitB, unit.StatusStarted) @@ -483,7 +483,7 @@ func TestDependencyTracker_ErrorCases(t *testing.T) { t.Run("CyclicDependencyDetection", func(t *testing.T) { t.Parallel() - tracker := unit.NewManager[string, testUnitID]() + tracker := unit.NewManager[testUnitID]() // Register units err := tracker.Register(unitA) @@ -518,7 +518,7 @@ func TestDependencyTracker_ToDOT(t *testing.T) { t.Run("ExportSimpleGraph", func(t *testing.T) { t.Parallel() - tracker := unit.NewManager[string, testUnitID]() + tracker := unit.NewManager[testUnitID]() // Register units err := tracker.Register(unitA) @@ -539,7 +539,7 @@ func TestDependencyTracker_ToDOT(t *testing.T) { t.Run("ExportComplexGraph", func(t *testing.T) { t.Parallel() - tracker := unit.NewManager[string, testUnitID]() + tracker := unit.NewManager[testUnitID]() // Register all units units := []testUnitID{unitA, unitB, unitC, unitD} From 838999d5e78968eebffaa5b6b73a718df820e108 Mon Sep 17 00:00:00 2001 From: Sas Swart Date: Fri, 14 Nov 2025 10:31:11 +0000 Subject: [PATCH 11/14] review nits --- agent/unit/manager.go | 12 +++++------- agent/unit/manager_test.go | 3 +-- 2 files changed, 6 insertions(+), 9 deletions(-) diff --git a/agent/unit/manager.go b/agent/unit/manager.go index bc275356ad140..9d940b9917437 100644 --- a/agent/unit/manager.go +++ b/agent/unit/manager.go @@ -4,6 +4,8 @@ import ( "sync" "golang.org/x/xerrors" + + "github.com/coder/coder/v2/coderd/util/slice" ) var ( @@ -177,13 +179,9 @@ func (dt *Manager[UnitID]) GetUnmetDependencies(unit UnitID) ([]Dependency[Statu return nil, err } - var unmetDependencies []Dependency[Status, UnitID] - - for _, dependency := range allDependencies { - if !dependency.IsSatisfied { - unmetDependencies = append(unmetDependencies, dependency) - } - } + var unmetDependencies []Dependency[Status, UnitID] = slice.Filter(allDependencies, func(dependency Dependency[Status, UnitID]) bool { + return !dependency.IsSatisfied + }) return unmetDependencies, nil } diff --git a/agent/unit/manager_test.go b/agent/unit/manager_test.go index bc9eca25ae9e3..3fb99a6367a1e 100644 --- a/agent/unit/manager_test.go +++ b/agent/unit/manager_test.go @@ -21,11 +21,10 @@ const ( func TestDependencyTracker_Register(t *testing.T) { t.Parallel() - tracker := unit.NewManager[testUnitID]() - t.Run("RegisterNewUnit", func(t *testing.T) { t.Parallel() + tracker := unit.NewManager[testUnitID]() err := tracker.Register(unitA) require.NoError(t, err) From 04bc903c734d326647543e63e247d0441de1fa7c Mon Sep 17 00:00:00 2001 From: Sas Swart Date: Wed, 19 Nov 2025 13:08:34 +0000 Subject: [PATCH 12/14] Simplify the unit manager Remove generics. Provide a more sensible zero value status type. --- agent/unit/graph_test.go | 6 +- agent/unit/manager.go | 303 +++++++++---------- agent/unit/manager_test.go | 588 +++++++++++++++++++------------------ 3 files changed, 454 insertions(+), 443 deletions(-) diff --git a/agent/unit/graph_test.go b/agent/unit/graph_test.go index f7d1117be74b3..8c619aff4c3d9 100644 --- a/agent/unit/graph_test.go +++ b/agent/unit/graph_test.go @@ -148,7 +148,7 @@ func TestGraph(t *testing.T) { graph := &testGraph{} unit1 := &testGraphVertex{Name: "unit1"} err := graph.AddEdge(unit1, unit1, testEdgeCompleted) - require.ErrorIs(t, err, unit.ErrCycleDetected) + require.ErrorContains(t, err, unit.ErrCycleDetected) return graph }, @@ -159,7 +159,7 @@ func TestGraph(t *testing.T) { err := graph.AddEdge(unit1, unit2, testEdgeCompleted) require.NoError(t, err) err = graph.AddEdge(unit2, unit1, testEdgeStarted) - require.ErrorIs(t, err, unit.ErrCycleDetected) + require.ErrorContains(t, err, unit.ErrCycleDetected) return graph }, @@ -339,7 +339,7 @@ func TestGraphThreadSafety(t *testing.T) { // Verify all attempts correctly returned cycle error for i, err := range cycleErrors { require.Error(t, err, "goroutine %d should have detected cycle", i) - require.ErrorIs(t, err, unit.ErrCycleDetected) + require.ErrorContains(t, err, unit.ErrCycleDetected) } // Verify graph remains valid (original chain intact) diff --git a/agent/unit/manager.go b/agent/unit/manager.go index 9d940b9917437..a082f0f7943db 100644 --- a/agent/unit/manager.go +++ b/agent/unit/manager.go @@ -9,23 +9,12 @@ import ( ) var ( - // ErrUnitNotFound is returned when a unit ID is not registered. - ErrUnitNotFound = xerrors.New("unit not found") - - // ErrUnitAlreadyRegistered is returned when a unit ID is already registered. - ErrUnitAlreadyRegistered = xerrors.New("unit already registered") - - // ErrCannotUpdateOtherUnit is returned when attempting to update another unit's status. - ErrCannotUpdateOtherUnit = xerrors.New("cannot update other unit's status") - - // ErrDependenciesNotSatisfied is returned when a unit's dependencies are not satisfied. - ErrDependenciesNotSatisfied = xerrors.New("unit dependencies not satisfied") - - // ErrSameStatusAlreadySet is returned when attempting to set the same status as the current status. - ErrSameStatusAlreadySet = xerrors.New("same status already set") - - // ErrCycleDetected is returned when a cycle is detected in the dependency graph. - ErrCycleDetected = xerrors.New("cycle detected") + ErrUnitNotFound = "unit not found" + ErrUnitAlreadyRegistered = "unit already registered" + ErrCannotUpdateOtherUnit = "cannot update other unit's status" + ErrDependenciesNotSatisfied = "unit dependencies not satisfied" + ErrSameStatusAlreadySet = "same status already set" + ErrCycleDetected = "cycle detected" ) // Status represents the status of a unit. @@ -33,153 +22,170 @@ type Status string // Status constants for dependency tracking const ( - StatusPending Status = "" - StatusStarted Status = "started" - StatusComplete Status = "completed" + StatusNotRegistered Status = "" + StatusPending Status = "pending" + StatusStarted Status = "started" + StatusComplete Status = "completed" ) -// dependencyVertex represents a vertex in the dependency graph that is associated with a unit. -type dependencyVertex[UnitID comparable] struct { - ID UnitID +// ID provides a type narrowed representation of the unique identifier of a unit. +type ID string + +// Unit represents a point-in-time snapshot of a vertex in the dependency graph. +// Units may depend on other units, or be depended on by other units. The unit struct +// is not aware of updates made to the dependency graph after it is initialized and should +// not be cached. +type Unit struct { + id ID + status Status + // ready is true if all dependencies are satisfied. + // It does not have an accessor method on Unit, because a unit cannot know whether it is ready. + // Only the Manager can calculate whether a unit is ready based on knowledge of the dependency graph. + // To discourage use of an outdated readiness value, only the Manager should set and return this field. + ready bool +} + +func (u *Unit) ID() ID { + return u.id +} + +func (u *Unit) Status() Status { + return u.status } // Dependency represents a dependency relationship between units. -type Dependency[StatusType, UnitID comparable] struct { - Unit UnitID - DependsOn UnitID - RequiredStatus StatusType - CurrentStatus StatusType +type Dependency struct { + Unit ID + DependsOn ID + RequiredStatus Status + CurrentStatus Status IsSatisfied bool } // Manager provides reactive dependency tracking over a Graph. -// It manages unit registration, dependency relationships, and status updates +// It manages Unit registration, dependency relationships, and status updates // with automatic recalculation of readiness when dependencies are satisfied. -type Manager[UnitID comparable] struct { +type Manager struct { mu sync.RWMutex // The underlying graph that stores dependency relationships - graph *Graph[Status, *dependencyVertex[UnitID]] - - // Track current status of each unit - unitStatus map[UnitID]Status - - // Track readiness state (cached to avoid repeated graph traversal) - unitReadiness map[UnitID]bool - - // Track which units are registered - registeredUnits map[UnitID]bool + graph *Graph[Status, ID] // Store vertex instances for each unit to ensure consistent references - unitVertices map[UnitID]*dependencyVertex[UnitID] + units map[ID]Unit } // NewManager creates a new Manager instance. -func NewManager[UnitID comparable]() *Manager[UnitID] { - return &Manager[UnitID]{ - graph: &Graph[Status, *dependencyVertex[UnitID]]{}, - unitStatus: make(map[UnitID]Status), - unitReadiness: make(map[UnitID]bool), - registeredUnits: make(map[UnitID]bool), - unitVertices: make(map[UnitID]*dependencyVertex[UnitID]), +func NewManager() *Manager { + return &Manager{ + graph: &Graph[Status, ID]{}, + units: make(map[ID]Unit), } } -// Register registers a new unit as a vertex in the dependency graph. -func (dt *Manager[UnitID]) Register(id UnitID) error { - dt.mu.Lock() - defer dt.mu.Unlock() +// Register adds a unit to the manager if it is not already registered. +// If a Unit is already registered (per the ID field), it is not updated. +func (m *Manager) Register(id ID) error { + m.mu.Lock() + defer m.mu.Unlock() - if dt.registeredUnits[id] { - return ErrUnitAlreadyRegistered + if m.registered(id) { + return xerrors.Errorf(ErrUnitAlreadyRegistered) } - // Create and store the vertex for this unit - vertex := &dependencyVertex[UnitID]{ID: id} - dt.unitVertices[id] = vertex - dt.registeredUnits[id] = true - dt.unitReadiness[id] = true // New units start as ready (no dependencies) + m.units[id] = Unit{ + id: id, + status: StatusPending, + ready: true, + } return nil } +// registered checks if a unit is registered in the manager. +func (m *Manager) registered(id ID) bool { + return m.units[id].status != StatusNotRegistered +} + +// Unit fetches a unit from the manager. If the unit does not exist, +// it returns the Unit zero-value as a placeholder unit, because +// units may depend on other units that have not yet been created. +func (m *Manager) Unit(id ID) Unit { + m.mu.RLock() + defer m.mu.RUnlock() + + return m.units[id] +} + +func (m *Manager) IsReady(id ID) bool { + m.mu.RLock() + defer m.mu.RUnlock() + + if !m.registered(id) { + return false + } + + return m.units[id].ready +} + // AddDependency adds a dependency relationship between units. // The unit depends on the dependsOn unit reaching the requiredStatus. -func (dt *Manager[UnitID]) AddDependency(unit UnitID, dependsOn UnitID, requiredStatus Status) error { - dt.mu.Lock() - defer dt.mu.Unlock() +func (m *Manager) AddDependency(unit ID, dependsOn ID, requiredStatus Status) error { + m.mu.Lock() + defer m.mu.Unlock() - if !dt.registeredUnits[unit] { + if !m.registered(unit) { return xerrors.Errorf("unit %v is not registered: %w", unit, ErrUnitNotFound) } - if !dt.registeredUnits[dependsOn] { - return xerrors.Errorf("unit %v is not registered: %w", dependsOn, ErrUnitNotFound) - } - - // Get the stored vertices for both units - unitVertex := dt.unitVertices[unit] - dependsOnVertex := dt.unitVertices[dependsOn] // Add the dependency edge to the graph // The edge goes from unit to dependsOn, representing the dependency - err := dt.graph.AddEdge(unitVertex, dependsOnVertex, requiredStatus) + err := m.graph.AddEdge(unit, dependsOn, requiredStatus) if err != nil { return xerrors.Errorf("failed to add dependency: %w", err) } - // Recalculate readiness for the unit since it now has a dependency - dt.recalculateReadinessUnsafe(unit) + // Recalculate readiness for the unit since it now has a new dependency + m.recalculateReadinessUnsafe(unit) return nil } // UpdateStatus updates a unit's status and recalculates readiness for affected dependents. -func (dt *Manager[UnitID]) UpdateStatus(unit UnitID, newStatus Status) error { - dt.mu.Lock() - defer dt.mu.Unlock() +func (m *Manager) UpdateStatus(unit ID, newStatus Status) error { + m.mu.Lock() + defer m.mu.Unlock() - if !dt.registeredUnits[unit] { - return ErrUnitNotFound + u := m.units[unit] + if !m.registered(unit) { + return xerrors.Errorf("unit %v is not registered: %w", unit, ErrUnitNotFound) } - - // Update the unit's status - if dt.unitStatus[unit] == newStatus { - return ErrSameStatusAlreadySet + if u.status == newStatus { + return xerrors.Errorf("%s", ErrSameStatusAlreadySet) } - dt.unitStatus[unit] = newStatus + + u.status = newStatus + m.units[unit] = u // Get all units that depend on this one (reverse adjacent vertices) - unitVertex := dt.unitVertices[unit] - dependentEdges := dt.graph.GetReverseAdjacentVertices(unitVertex) + dependentEdges := m.graph.GetReverseAdjacentVertices(unit) // Recalculate readiness for all dependents for _, edge := range dependentEdges { - dt.recalculateReadinessUnsafe(edge.From.ID) + m.recalculateReadinessUnsafe(edge.From) } return nil } -// IsReady checks if all dependencies for a unit are satisfied. -func (dt *Manager[UnitID]) IsReady(unit UnitID) (bool, error) { - dt.mu.RLock() - defer dt.mu.RUnlock() - - if !dt.registeredUnits[unit] { - return false, ErrUnitNotFound - } - - return dt.unitReadiness[unit], nil -} - // GetUnmetDependencies returns a list of unsatisfied dependencies for a unit. -func (dt *Manager[UnitID]) GetUnmetDependencies(unit UnitID) ([]Dependency[Status, UnitID], error) { - allDependencies, err := dt.GetAllDependencies(unit) +func (m *Manager) GetUnmetDependencies(unit ID) ([]Dependency, error) { + allDependencies, err := m.GetAllDependencies(unit) if err != nil { return nil, err } - var unmetDependencies []Dependency[Status, UnitID] = slice.Filter(allDependencies, func(dependency Dependency[Status, UnitID]) bool { + var unmetDependencies []Dependency = slice.Filter(allDependencies, func(dependency Dependency) bool { return !dependency.IsSatisfied }) @@ -188,97 +194,74 @@ func (dt *Manager[UnitID]) GetUnmetDependencies(unit UnitID) ([]Dependency[Statu // recalculateReadinessUnsafe recalculates the readiness state for a unit. // This method assumes the caller holds the write lock. -func (dt *Manager[UnitID]) recalculateReadinessUnsafe(unit UnitID) { - unitVertex := dt.unitVertices[unit] - forwardEdges := dt.graph.GetForwardAdjacentVertices(unitVertex) +func (m *Manager) recalculateReadinessUnsafe(unit ID) { + u := m.units[unit] + dependencies := m.graph.GetForwardAdjacentVertices(unit) - // If there are no dependencies, the unit is ready - if len(forwardEdges) == 0 { - dt.unitReadiness[unit] = true + if len(dependencies) == 0 { + u.ready = true + m.units[unit] = u return } - // Check if all dependencies are satisfied allSatisfied := true - for _, edge := range forwardEdges { - dependsOnUnit := edge.To.ID - requiredStatus := edge.Edge - currentStatus, exists := dt.unitStatus[dependsOnUnit] - if !exists || currentStatus != requiredStatus { + for _, dependency := range dependencies { + requiredStatus := dependency.Edge + dependsOnUnit := m.units[dependency.To] + if dependsOnUnit.status != requiredStatus { allSatisfied = false break } } - dt.unitReadiness[unit] = allSatisfied + u.ready = allSatisfied + m.units[unit] = u } // GetGraph returns the underlying graph for visualization and debugging. // This should be used carefully as it exposes the internal graph structure. -func (dt *Manager[UnitID]) GetGraph() *Graph[Status, *dependencyVertex[UnitID]] { - return dt.graph +func (m *Manager) GetGraph() *Graph[Status, ID] { + return m.graph } // GetStatus returns the current status of a unit. -func (dt *Manager[UnitID]) GetStatus(unit UnitID) (Status, error) { - dt.mu.RLock() - defer dt.mu.RUnlock() +func (m *Manager) GetStatus(unit ID) Status { + m.mu.RLock() + defer m.mu.RUnlock() - if !dt.registeredUnits[unit] { - return StatusPending, ErrUnitNotFound - } - - status, exists := dt.unitStatus[unit] - if !exists { - return StatusPending, nil - } - - return status, nil + u := m.units[unit] + return u.status } // GetAllDependencies returns all dependencies for a unit, both satisfied and unsatisfied. -func (dt *Manager[UnitID]) GetAllDependencies(unit UnitID) ([]Dependency[Status, UnitID], error) { - dt.mu.RLock() - defer dt.mu.RUnlock() +func (m *Manager) GetAllDependencies(unit ID) ([]Dependency, error) { + m.mu.RLock() + defer m.mu.RUnlock() - if !dt.registeredUnits[unit] { - return nil, ErrUnitNotFound + if !m.registered(unit) { + return nil, xerrors.Errorf("%s", ErrUnitNotFound) } - unitVertex := dt.unitVertices[unit] - forwardEdges := dt.graph.GetForwardAdjacentVertices(unitVertex) + forwardEdges := m.graph.GetForwardAdjacentVertices(unit) - var allDependencies []Dependency[Status, UnitID] + var allDependencies []Dependency for _, edge := range forwardEdges { - dependsOnUnit := edge.To.ID + dependsOnUnit := m.units[edge.To] requiredStatus := edge.Edge - currentStatus, exists := dt.unitStatus[dependsOnUnit] - if !exists { - // If the dependency unit has no status, it's not satisfied - allDependencies = append(allDependencies, Dependency[Status, UnitID]{ - Unit: unit, - DependsOn: dependsOnUnit, - RequiredStatus: requiredStatus, - CurrentStatus: StatusPending, - IsSatisfied: false, - }) - } else { - isSatisfied := currentStatus == requiredStatus - allDependencies = append(allDependencies, Dependency[Status, UnitID]{ - Unit: unit, - DependsOn: dependsOnUnit, - RequiredStatus: requiredStatus, - CurrentStatus: currentStatus, - IsSatisfied: isSatisfied, - }) - } + allDependencies = append(allDependencies, Dependency{ + Unit: unit, + DependsOn: dependsOnUnit.id, + RequiredStatus: requiredStatus, + CurrentStatus: dependsOnUnit.status, + IsSatisfied: dependsOnUnit.status == requiredStatus, + }) } return allDependencies, nil } // ExportDOT exports the dependency graph to DOT format for visualization. -func (dt *Manager[UnitID]) ExportDOT(name string) (string, error) { - return dt.graph.ToDOT(name) +func (m *Manager) ExportDOT(name string) (string, error) { + return m.graph.ToDOT(name) } diff --git a/agent/unit/manager_test.go b/agent/unit/manager_test.go index 3fb99a6367a1e..1e5847ad763bf 100644 --- a/agent/unit/manager_test.go +++ b/agent/unit/manager_test.go @@ -9,217 +9,328 @@ import ( "github.com/coder/coder/v2/agent/unit" ) -type testUnitID string - const ( - unitA testUnitID = "serviceA" - unitB testUnitID = "serviceB" - unitC testUnitID = "serviceC" - unitD testUnitID = "serviceD" + unitA unit.ID = "serviceA" + unitB unit.ID = "serviceB" + unitC unit.ID = "serviceC" + unitD unit.ID = "serviceD" ) -func TestDependencyTracker_Register(t *testing.T) { +func TestManager_Register(t *testing.T) { t.Parallel() t.Run("RegisterNewUnit", func(t *testing.T) { t.Parallel() - tracker := unit.NewManager[testUnitID]() - err := tracker.Register(unitA) - require.NoError(t, err) + manager := unit.NewManager() - // Unit should be ready initially (no dependencies) - ready, err := tracker.IsReady(unitA) + // Given: a unit is registered + err := manager.Register(unitA) require.NoError(t, err) - assert.True(t, ready) + + // Then: the unit should be ready (no dependencies) + u := manager.Unit(unitA) + assert.Equal(t, unitA, u.ID()) + assert.Equal(t, unit.StatusPending, u.Status()) + assert.True(t, manager.IsReady(unitA)) }) t.Run("RegisterDuplicateUnit", func(t *testing.T) { t.Parallel() - tracker := unit.NewManager[testUnitID]() - err := tracker.Register(unitA) + manager := unit.NewManager() + + // Given: a unit is registered + err := manager.Register(unitA) require.NoError(t, err) - err = tracker.Register(unitA) - require.ErrorIs(t, err, unit.ErrUnitAlreadyRegistered) + // Newly registered units have StatusPending. We update the unit status to StatusStarted, + // so we can later assert that it is not overwritten back to StatusPending by the second + // register call + manager.UpdateStatus(unitA, unit.StatusStarted) + + // When: the unit is registered again + err = manager.Register(unitA) + + // Then: a descriptive error should be returned + require.ErrorContains(t, err, unit.ErrUnitAlreadyRegistered) + + // Then: the unit status should not be overwritten + u := manager.Unit(unitA) + assert.Equal(t, unit.StatusStarted, u.Status()) + assert.True(t, manager.IsReady(unitA)) }) t.Run("RegisterMultipleUnits", func(t *testing.T) { t.Parallel() - tracker := unit.NewManager[testUnitID]() + manager := unit.NewManager() - units := []testUnitID{unitA, unitB, unitC} - for _, unit := range units { - err := tracker.Register(unit) + // Given: multiple units are registered + unitIDs := []unit.ID{unitA, unitB, unitC} + for _, unit := range unitIDs { + err := manager.Register(unit) require.NoError(t, err) } - // All should be ready initially - for _, unit := range units { - ready, err := tracker.IsReady(unit) - require.NoError(t, err) - assert.True(t, ready) + // Then: all units should be ready initially + for _, unitID := range unitIDs { + u := manager.Unit(unitID) + assert.Equal(t, unit.StatusPending, u.Status()) + assert.True(t, manager.IsReady(unitID)) } }) } -func TestDependencyTracker_AddDependency(t *testing.T) { +func TestManager_AddDependency(t *testing.T) { t.Parallel() t.Run("AddDependencyBetweenRegisteredUnits", func(t *testing.T) { t.Parallel() - tracker := unit.NewManager[testUnitID]() - err := tracker.Register(unitA) + manager := unit.NewManager() + + // Given: units A and B are registered + err := manager.Register(unitA) + require.NoError(t, err) + err = manager.Register(unitB) require.NoError(t, err) - err = tracker.Register(unitB) + + // Given: Unit A depends on Unit B being unit.StatusStarted + err = manager.AddDependency(unitA, unitB, unit.StatusStarted) require.NoError(t, err) - // A depends on B being unit.StatusStarted - err = tracker.AddDependency(unitA, unitB, unit.StatusStarted) + // Then: Unit A should not be ready (depends on B) + u := manager.Unit(unitA) + assert.Equal(t, unit.StatusPending, u.Status()) + assert.False(t, manager.IsReady(unitA)) + + // Then: Unit B should still be ready (no dependencies) + u = manager.Unit(unitB) + assert.Equal(t, unit.StatusPending, u.Status()) + assert.True(t, manager.IsReady(unitB)) + + // When: Unit B is started + err = manager.UpdateStatus(unitB, unit.StatusStarted) require.NoError(t, err) - // A should no longer be ready (depends on B) - ready, err := tracker.IsReady(unitA) + // Then: Unit A should be ready, because its dependency is now in the desired state. + assert.True(t, manager.IsReady(unitA)) + + // When: Unit B is stopped + err = manager.UpdateStatus(unitB, unit.StatusPending) require.NoError(t, err) - assert.False(t, ready) - // B should still be ready (no dependencies) - ready, err = tracker.IsReady(unitB) + // Then: Unit A should no longer be ready, because its dependency is not in the desired state. + assert.False(t, manager.IsReady(unitA)) + }) + + t.Run("AddDependencyByAnUnregisteredDependentUnit", func(t *testing.T) { + t.Parallel() + + manager := unit.NewManager() + + // Given Unit B is registered + err := manager.Register(unitB) require.NoError(t, err) - assert.True(t, ready) + + // Given Unit A depends on Unit B being started + err = manager.AddDependency(unitA, unitB, unit.StatusStarted) + + // Then: a descriptive error communicates that the dependency cannot be added + // because the dependent unit must be registered first. + require.ErrorContains(t, err, unit.ErrUnitNotFound) }) - t.Run("AddDependencyWithUnregisteredUnit", func(t *testing.T) { + t.Run("AddDependencyOnAnUnregisteredUnit", func(t *testing.T) { t.Parallel() - tracker := unit.NewManager[testUnitID]() - err := tracker.Register(unitA) + manager := unit.NewManager() + + // Given unit A is registered + err := manager.Register(unitA) + require.NoError(t, err) + + // Given Unit B is not yet registered + // And Unit A depends on Unit B being started + err = manager.AddDependency(unitA, unitB, unit.StatusStarted) + require.NoError(t, err) + + u := manager.Unit(unitB) + assert.Equal(t, unit.StatusNotRegistered, u.Status()) + + // Then: Unit A should not be ready, because it depends on Unit B + assert.False(t, manager.IsReady(unitA)) + + // When: Unit B is registered + err = manager.Register(unitB) + require.NoError(t, err) + + // Then: Unit A should still not be ready. + // Unit B is not registered, but it has not been started as required by the dependency. + assert.False(t, manager.IsReady(unitA)) + + // When: Unit B is started + err = manager.UpdateStatus(unitB, unit.StatusStarted) require.NoError(t, err) - // Try to add dependency to unregistered unit - err = tracker.AddDependency(unitA, unitB, unit.StatusStarted) - require.ErrorIs(t, err, unit.ErrUnitNotFound) + // Then: Unit A should be ready, because its dependency is now in the desired state. + assert.True(t, manager.IsReady(unitA)) }) - t.Run("AddDependencyFromUnregisteredUnit", func(t *testing.T) { + t.Run("AddDependencyCreatesACyclicDependency", func(t *testing.T) { t.Parallel() - tracker := unit.NewManager[testUnitID]() - err := tracker.Register(unitB) + manager := unit.NewManager() + + // Register units + err := manager.Register(unitA) + require.NoError(t, err) + err = manager.Register(unitB) + require.NoError(t, err) + err = manager.Register(unitC) + require.NoError(t, err) + err = manager.Register(unitD) + require.NoError(t, err) + + // A depends on B + err = manager.AddDependency(unitA, unitB, unit.StatusStarted) + require.NoError(t, err) + // B depends on C + err = manager.AddDependency(unitB, unitC, unit.StatusStarted) + require.NoError(t, err) + + // C depends on D + err = manager.AddDependency(unitC, unitD, unit.StatusStarted) require.NoError(t, err) - // Try to add dependency from unregistered unit - err = tracker.AddDependency(unitA, unitB, unit.StatusStarted) - require.ErrorIs(t, err, unit.ErrUnitNotFound) + // Try to make D depend on A (creates indirect cycle) + err = manager.AddDependency(unitD, unitA, unit.StatusStarted) + require.ErrorContains(t, err, unit.ErrCycleDetected) }) } -func TestDependencyTracker_UpdateStatus(t *testing.T) { +func TestManager_UpdateStatus(t *testing.T) { t.Parallel() t.Run("UpdateStatusTriggersReadinessRecalculation", func(t *testing.T) { t.Parallel() - tracker := unit.NewManager[testUnitID]() - err := tracker.Register(unitA) + manager := unit.NewManager() + + // Given units A and B are registered + err := manager.Register(unitA) require.NoError(t, err) - err = tracker.Register(unitB) + err = manager.Register(unitB) require.NoError(t, err) - // A depends on B being unit.StatusStarted - err = tracker.AddDependency(unitA, unitB, unit.StatusStarted) + // Given Unit A depends on Unit B being unit.StatusStarted + err = manager.AddDependency(unitA, unitB, unit.StatusStarted) require.NoError(t, err) - // Initially A is not ready - ready, err := tracker.IsReady(unitA) - require.NoError(t, err) - assert.False(t, ready) + // Then: Unit A should not be ready (depends on B) + u := manager.Unit(unitA) + assert.Equal(t, unit.StatusPending, u.Status()) + assert.False(t, manager.IsReady(unitA)) - // Update B to unit.StatusStarted - A should become ready - err = tracker.UpdateStatus(unitB, unit.StatusStarted) + // When: Unit B is started + err = manager.UpdateStatus(unitB, unit.StatusStarted) require.NoError(t, err) - ready, err = tracker.IsReady(unitA) - require.NoError(t, err) - assert.True(t, ready) + // Then: Unit A should be ready, because its dependency is now in the desired state. + u = manager.Unit(unitA) + assert.Equal(t, unit.StatusPending, u.Status()) + assert.True(t, manager.IsReady(unitA)) }) t.Run("UpdateStatusWithUnregisteredUnit", func(t *testing.T) { t.Parallel() - tracker := unit.NewManager[testUnitID]() + manager := unit.NewManager() - err := tracker.UpdateStatus(unitA, unit.StatusStarted) - require.ErrorIs(t, err, unit.ErrUnitNotFound) + // Given Unit A is not registered + // When: Unit A is updated to unit.StatusStarted + err := manager.UpdateStatus(unitA, unit.StatusStarted) + + // Then: a descriptive error communicates that the unit must be registered first. + require.ErrorContains(t, err, unit.ErrUnitNotFound) }) t.Run("LinearChainDependencies", func(t *testing.T) { t.Parallel() - tracker := unit.NewManager[testUnitID]() + manager := unit.NewManager() - // Register all units - units := []testUnitID{unitA, unitB, unitC} - for _, unit := range units { - err := tracker.Register(unit) - require.NoError(t, err) - } + // Given units A, B, and C are registered + err := manager.Register(unitA) + require.NoError(t, err) + err = manager.Register(unitB) + require.NoError(t, err) + err = manager.Register(unitC) + require.NoError(t, err) // Create chain: A depends on B being "started", B depends on C being "completed" - err := tracker.AddDependency(unitA, unitB, unit.StatusStarted) + err = manager.AddDependency(unitA, unitB, unit.StatusStarted) require.NoError(t, err) - err = tracker.AddDependency(unitB, unitC, unit.StatusComplete) + err = manager.AddDependency(unitB, unitC, unit.StatusComplete) require.NoError(t, err) - // Initially only C is ready - ready, err := tracker.IsReady(unitC) - require.NoError(t, err) - assert.True(t, ready) + // Then: only Unit C should be ready (no dependencies) + u := manager.Unit(unitC) + assert.Equal(t, unit.StatusPending, u.Status()) + assert.True(t, manager.IsReady(unitC)) - ready, err = tracker.IsReady(unitB) - require.NoError(t, err) - assert.False(t, ready) + u = manager.Unit(unitB) + assert.Equal(t, unit.StatusPending, u.Status()) + assert.False(t, manager.IsReady(unitB)) - ready, err = tracker.IsReady(unitA) - require.NoError(t, err) - assert.False(t, ready) + u = manager.Unit(unitA) + assert.Equal(t, unit.StatusPending, u.Status()) + assert.False(t, manager.IsReady(unitA)) - // Update C to "completed" - B should become ready - err = tracker.UpdateStatus(unitC, unit.StatusComplete) + // When: Unit C is completed + err = manager.UpdateStatus(unitC, unit.StatusComplete) require.NoError(t, err) - ready, err = tracker.IsReady(unitB) - require.NoError(t, err) - assert.True(t, ready) + // Then: Unit B should be ready, because its dependency is now in the desired state. + u = manager.Unit(unitB) + assert.Equal(t, unit.StatusPending, u.Status()) + assert.True(t, manager.IsReady(unitB)) - ready, err = tracker.IsReady(unitA) - require.NoError(t, err) - assert.False(t, ready) + u = manager.Unit(unitA) + assert.Equal(t, unit.StatusPending, u.Status()) + assert.False(t, manager.IsReady(unitA)) - // Update B to "started" - A should become ready - err = tracker.UpdateStatus(unitB, unit.StatusStarted) - require.NoError(t, err) + u = manager.Unit(unitB) + assert.Equal(t, unit.StatusPending, u.Status()) + assert.True(t, manager.IsReady(unitB)) - ready, err = tracker.IsReady(unitA) + // When: Unit B is started + err = manager.UpdateStatus(unitB, unit.StatusStarted) require.NoError(t, err) - assert.True(t, ready) + + // Then: Unit A should be ready, because its dependency is now in the desired state. + u = manager.Unit(unitA) + assert.Equal(t, unit.StatusPending, u.Status()) + assert.True(t, manager.IsReady(unitA)) }) } -func TestDependencyTracker_GetUnmetDependencies(t *testing.T) { +func TestManager_GetUnmetDependencies(t *testing.T) { t.Parallel() t.Run("GetUnmetDependenciesForUnitWithNoDependencies", func(t *testing.T) { t.Parallel() - tracker := unit.NewManager[testUnitID]() - err := tracker.Register(unitA) + manager := unit.NewManager() + + // Given: Unit A is registered + err := manager.Register(unitA) require.NoError(t, err) - unmet, err := tracker.GetUnmetDependencies(unitA) + // Given: Unit A has no dependencies + // Then: Unit A should have no unmet dependencies + unmet, err := manager.GetUnmetDependencies(unitA) require.NoError(t, err) assert.Empty(t, unmet) }) @@ -227,17 +338,17 @@ func TestDependencyTracker_GetUnmetDependencies(t *testing.T) { t.Run("GetUnmetDependenciesForUnitWithUnsatisfiedDependencies", func(t *testing.T) { t.Parallel() - tracker := unit.NewManager[testUnitID]() - err := tracker.Register(unitA) + manager := unit.NewManager() + err := manager.Register(unitA) require.NoError(t, err) - err = tracker.Register(unitB) + err = manager.Register(unitB) require.NoError(t, err) - // A depends on B being unit.StatusStarted - err = tracker.AddDependency(unitA, unitB, unit.StatusStarted) + // Given: Unit A depends on Unit B being unit.StatusStarted + err = manager.AddDependency(unitA, unitB, unit.StatusStarted) require.NoError(t, err) - unmet, err := tracker.GetUnmetDependencies(unitA) + unmet, err := manager.GetUnmetDependencies(unitA) require.NoError(t, err) require.Len(t, unmet, 1) @@ -250,21 +361,24 @@ func TestDependencyTracker_GetUnmetDependencies(t *testing.T) { t.Run("GetUnmetDependenciesForUnitWithSatisfiedDependencies", func(t *testing.T) { t.Parallel() - tracker := unit.NewManager[testUnitID]() - err := tracker.Register(unitA) + manager := unit.NewManager() + + // Given: Unit A and Unit B are registered + err := manager.Register(unitA) require.NoError(t, err) - err = tracker.Register(unitB) + err = manager.Register(unitB) require.NoError(t, err) - // A depends on B being unit.StatusStarted - err = tracker.AddDependency(unitA, unitB, unit.StatusStarted) + // Given: Unit A depends on Unit B being unit.StatusStarted + err = manager.AddDependency(unitA, unitB, unit.StatusStarted) require.NoError(t, err) - // Update B to unit.StatusStarted - err = tracker.UpdateStatus(unitB, unit.StatusStarted) + // When: Unit B is started + err = manager.UpdateStatus(unitB, unit.StatusStarted) require.NoError(t, err) - unmet, err := tracker.GetUnmetDependencies(unitA) + // Then: Unit A should have no unmet dependencies + unmet, err := manager.GetUnmetDependencies(unitA) require.NoError(t, err) assert.Empty(t, unmet) }) @@ -272,264 +386,178 @@ func TestDependencyTracker_GetUnmetDependencies(t *testing.T) { t.Run("GetUnmetDependenciesForUnregisteredUnit", func(t *testing.T) { t.Parallel() - tracker := unit.NewManager[testUnitID]() + manager := unit.NewManager() + + // When: Unit A is requested + unmet, err := manager.GetUnmetDependencies(unitA) - unmet, err := tracker.GetUnmetDependencies(unitA) - require.ErrorIs(t, err, unit.ErrUnitNotFound) + // Then: a descriptive error communicates that the unit must be registered first. + require.ErrorContains(t, err, unit.ErrUnitNotFound) assert.Nil(t, unmet) }) } -func TestDependencyTracker_MultipleDependencies(t *testing.T) { +func TestManager_MultipleDependencies(t *testing.T) { t.Parallel() t.Run("UnitWithMultipleDependencies", func(t *testing.T) { t.Parallel() - tracker := unit.NewManager[testUnitID]() + manager := unit.NewManager() // Register all units - units := []testUnitID{unitA, unitB, unitC, unitD} + units := []unit.ID{unitA, unitB, unitC, unitD} for _, unit := range units { - err := tracker.Register(unit) + err := manager.Register(unit) require.NoError(t, err) } // A depends on B being unit.StatusStarted AND C being "started" - err := tracker.AddDependency(unitA, unitB, unit.StatusStarted) + err := manager.AddDependency(unitA, unitB, unit.StatusStarted) require.NoError(t, err) - err = tracker.AddDependency(unitA, unitC, unit.StatusStarted) + err = manager.AddDependency(unitA, unitC, unit.StatusStarted) require.NoError(t, err) // A should not be ready (depends on both B and C) - ready, err := tracker.IsReady(unitA) - require.NoError(t, err) - assert.False(t, ready) + assert.False(t, manager.IsReady(unitA)) // Update B to unit.StatusStarted - A should still not be ready (needs C too) - err = tracker.UpdateStatus(unitB, unit.StatusStarted) + err = manager.UpdateStatus(unitB, unit.StatusStarted) require.NoError(t, err) - ready, err = tracker.IsReady(unitA) - require.NoError(t, err) - assert.False(t, ready) + assert.False(t, manager.IsReady(unitA)) // Update C to "started" - A should now be ready - err = tracker.UpdateStatus(unitC, unit.StatusStarted) + err = manager.UpdateStatus(unitC, unit.StatusStarted) require.NoError(t, err) - ready, err = tracker.IsReady(unitA) - require.NoError(t, err) - assert.True(t, ready) + assert.True(t, manager.IsReady(unitA)) }) t.Run("ComplexDependencyChain", func(t *testing.T) { t.Parallel() - tracker := unit.NewManager[testUnitID]() + manager := unit.NewManager() // Register all units - units := []testUnitID{unitA, unitB, unitC, unitD} + units := []unit.ID{unitA, unitB, unitC, unitD} for _, unit := range units { - err := tracker.Register(unit) + err := manager.Register(unit) require.NoError(t, err) } // Create complex dependency graph: // A depends on B being unit.StatusStarted AND C being "started" - // B depends on D being "completed" - // C depends on D being "completed" - err := tracker.AddDependency(unitA, unitB, unit.StatusStarted) + err := manager.AddDependency(unitA, unitB, unit.StatusStarted) require.NoError(t, err) - err = tracker.AddDependency(unitA, unitC, unit.StatusStarted) + err = manager.AddDependency(unitA, unitC, unit.StatusStarted) require.NoError(t, err) - err = tracker.AddDependency(unitB, unitD, unit.StatusComplete) + // B depends on D being "completed" + err = manager.AddDependency(unitB, unitD, unit.StatusComplete) require.NoError(t, err) - err = tracker.AddDependency(unitC, unitD, unit.StatusComplete) + // C depends on D being "completed" + err = manager.AddDependency(unitC, unitD, unit.StatusComplete) require.NoError(t, err) // Initially only D is ready - ready, err := tracker.IsReady(unitD) - require.NoError(t, err) - assert.True(t, ready) - - ready, err = tracker.IsReady(unitB) - require.NoError(t, err) - assert.False(t, ready) - - ready, err = tracker.IsReady(unitC) - require.NoError(t, err) - assert.False(t, ready) - - ready, err = tracker.IsReady(unitA) - require.NoError(t, err) - assert.False(t, ready) + assert.True(t, manager.IsReady(unitD)) + assert.False(t, manager.IsReady(unitB)) + assert.False(t, manager.IsReady(unitC)) + assert.False(t, manager.IsReady(unitA)) // Update D to "completed" - B and C should become ready - err = tracker.UpdateStatus(unitD, unit.StatusComplete) - require.NoError(t, err) - - ready, err = tracker.IsReady(unitB) + err = manager.UpdateStatus(unitD, unit.StatusComplete) require.NoError(t, err) - assert.True(t, ready) - ready, err = tracker.IsReady(unitC) - require.NoError(t, err) - assert.True(t, ready) - - ready, err = tracker.IsReady(unitA) - require.NoError(t, err) - assert.False(t, ready) + assert.True(t, manager.IsReady(unitB)) + assert.True(t, manager.IsReady(unitC)) + assert.False(t, manager.IsReady(unitA)) // Update B to unit.StatusStarted - A should still not be ready (needs C) - err = tracker.UpdateStatus(unitB, unit.StatusStarted) + err = manager.UpdateStatus(unitB, unit.StatusStarted) require.NoError(t, err) - ready, err = tracker.IsReady(unitA) - require.NoError(t, err) - assert.False(t, ready) + assert.False(t, manager.IsReady(unitA)) // Update C to "started" - A should now be ready - err = tracker.UpdateStatus(unitC, unit.StatusStarted) + err = manager.UpdateStatus(unitC, unit.StatusStarted) require.NoError(t, err) - ready, err = tracker.IsReady(unitA) - require.NoError(t, err) - assert.True(t, ready) + assert.True(t, manager.IsReady(unitA)) }) t.Run("DifferentStatusTypes", func(t *testing.T) { t.Parallel() - tracker := unit.NewManager[testUnitID]() + manager := unit.NewManager() // Register units - err := tracker.Register(unitA) + err := manager.Register(unitA) require.NoError(t, err) - err = tracker.Register(unitB) + err = manager.Register(unitB) require.NoError(t, err) - err = tracker.Register(unitC) + err = manager.Register(unitC) require.NoError(t, err) - // A depends on B being unit.StatusStarted AND C being "completed" - err = tracker.AddDependency(unitA, unitB, unit.StatusStarted) + // Given: Unit A depends on Unit B being unit.StatusStarted + err = manager.AddDependency(unitA, unitB, unit.StatusStarted) require.NoError(t, err) - err = tracker.AddDependency(unitA, unitC, unit.StatusComplete) + // Given: Unit A depends on Unit C being "completed" + err = manager.AddDependency(unitA, unitC, unit.StatusComplete) require.NoError(t, err) - // Update B to unit.StatusStarted but not C - A should not be ready - err = tracker.UpdateStatus(unitB, unit.StatusStarted) + // When: Unit B is started + err = manager.UpdateStatus(unitB, unit.StatusStarted) require.NoError(t, err) - ready, err := tracker.IsReady(unitA) - require.NoError(t, err) - assert.False(t, ready) + // Then: Unit A should not be ready, because only one of its dependencies is in the desired state. + // It still requires Unit C to be completed. + assert.False(t, manager.IsReady(unitA)) - // Update C to "completed" - A should now be ready - err = tracker.UpdateStatus(unitC, unit.StatusComplete) + // When: Unit C is completed + err = manager.UpdateStatus(unitC, unit.StatusComplete) require.NoError(t, err) - ready, err = tracker.IsReady(unitA) - require.NoError(t, err) - assert.True(t, ready) + // Then: Unit A should be ready, because both of its dependencies are in the desired state. + assert.True(t, manager.IsReady(unitA)) }) } -func TestDependencyTracker_ErrorCases(t *testing.T) { +func TestManager_IsReady(t *testing.T) { t.Parallel() - t.Run("UpdateStatusWithUnregisteredUnit", func(t *testing.T) { - t.Parallel() - - tracker := unit.NewManager[testUnitID]() - - err := tracker.UpdateStatus(unitA, unit.StatusStarted) - require.ErrorIs(t, err, unit.ErrUnitNotFound) - }) - t.Run("IsReadyWithUnregisteredUnit", func(t *testing.T) { t.Parallel() - tracker := unit.NewManager[testUnitID]() - - ready, err := tracker.IsReady(unitA) - require.ErrorIs(t, err, unit.ErrUnitNotFound) - assert.False(t, ready) - }) - - t.Run("GetUnmetDependenciesWithUnregisteredUnit", func(t *testing.T) { - t.Parallel() - - tracker := unit.NewManager[testUnitID]() - - unmet, err := tracker.GetUnmetDependencies(unitA) - require.Error(t, err) - assert.Equal(t, unit.ErrUnitNotFound, err) - assert.Nil(t, unmet) - }) - - t.Run("AddDependencyWithUnregisteredUnits", func(t *testing.T) { - t.Parallel() - - tracker := unit.NewManager[testUnitID]() - - // Try to add dependency with unregistered units - err := tracker.AddDependency(unitA, unitB, unit.StatusStarted) - require.Error(t, err) - assert.Contains(t, err.Error(), "not registered") - }) - - t.Run("CyclicDependencyDetection", func(t *testing.T) { - t.Parallel() - - tracker := unit.NewManager[testUnitID]() - - // Register units - err := tracker.Register(unitA) - require.NoError(t, err) - err = tracker.Register(unitB) - require.NoError(t, err) - err = tracker.Register(unitC) - require.NoError(t, err) - err = tracker.Register(unitD) - require.NoError(t, err) - - // A depends on B - err = tracker.AddDependency(unitA, unitB, unit.StatusStarted) - require.NoError(t, err) - // B depends on C - err = tracker.AddDependency(unitB, unitC, unit.StatusStarted) - require.NoError(t, err) + manager := unit.NewManager() - // C depends on D - err = tracker.AddDependency(unitC, unitD, unit.StatusStarted) - require.NoError(t, err) - - // Try to make D depend on A (creates indirect cycle) - err = tracker.AddDependency(unitD, unitA, unit.StatusStarted) - require.ErrorIs(t, err, unit.ErrCycleDetected) + // Given: a unit is not registered + u := manager.Unit(unitA) + assert.Equal(t, unit.StatusNotRegistered, u.Status()) + // Then: the unit is not ready + assert.False(t, manager.IsReady(unitA)) }) } -func TestDependencyTracker_ToDOT(t *testing.T) { +func TestManager_ToDOT(t *testing.T) { t.Parallel() t.Run("ExportSimpleGraph", func(t *testing.T) { t.Parallel() - tracker := unit.NewManager[testUnitID]() + manager := unit.NewManager() // Register units - err := tracker.Register(unitA) + err := manager.Register(unitA) require.NoError(t, err) - err = tracker.Register(unitB) + err = manager.Register(unitB) require.NoError(t, err) // Add dependency - err = tracker.AddDependency(unitA, unitB, unit.StatusStarted) + err = manager.AddDependency(unitA, unitB, unit.StatusStarted) require.NoError(t, err) - dot, err := tracker.ExportDOT("test") + dot, err := manager.ExportDOT("test") require.NoError(t, err) assert.NotEmpty(t, dot) assert.Contains(t, dot, "digraph") @@ -538,27 +566,27 @@ func TestDependencyTracker_ToDOT(t *testing.T) { t.Run("ExportComplexGraph", func(t *testing.T) { t.Parallel() - tracker := unit.NewManager[testUnitID]() + manager := unit.NewManager() // Register all units - units := []testUnitID{unitA, unitB, unitC, unitD} + units := []unit.ID{unitA, unitB, unitC, unitD} for _, unit := range units { - err := tracker.Register(unit) + err := manager.Register(unit) require.NoError(t, err) } // Create complex dependency graph // A depends on B and C, B depends on D, C depends on D - err := tracker.AddDependency(unitA, unitB, unit.StatusStarted) + err := manager.AddDependency(unitA, unitB, unit.StatusStarted) require.NoError(t, err) - err = tracker.AddDependency(unitA, unitC, unit.StatusStarted) + err = manager.AddDependency(unitA, unitC, unit.StatusStarted) require.NoError(t, err) - err = tracker.AddDependency(unitB, unitD, unit.StatusComplete) + err = manager.AddDependency(unitB, unitD, unit.StatusComplete) require.NoError(t, err) - err = tracker.AddDependency(unitC, unitD, unit.StatusComplete) + err = manager.AddDependency(unitC, unitD, unit.StatusComplete) require.NoError(t, err) - dot, err := tracker.ExportDOT("complex") + dot, err := manager.ExportDOT("complex") require.NoError(t, err) assert.NotEmpty(t, dot) assert.Contains(t, dot, "digraph") From 117e1389c29636fc66a8dae220d8be8862b3bd11 Mon Sep 17 00:00:00 2001 From: Sas Swart Date: Wed, 19 Nov 2025 13:18:13 +0000 Subject: [PATCH 13/14] make unit manager errors more consistent --- agent/unit/manager.go | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/agent/unit/manager.go b/agent/unit/manager.go index a082f0f7943db..3c0f713cabb03 100644 --- a/agent/unit/manager.go +++ b/agent/unit/manager.go @@ -15,6 +15,7 @@ var ( ErrDependenciesNotSatisfied = "unit dependencies not satisfied" ErrSameStatusAlreadySet = "same status already set" ErrCycleDetected = "cycle detected" + ErrFailedToAddDependency = "failed to add dependency" ) // Status represents the status of a unit. @@ -90,7 +91,7 @@ func (m *Manager) Register(id ID) error { defer m.mu.Unlock() if m.registered(id) { - return xerrors.Errorf(ErrUnitAlreadyRegistered) + return xerrors.Errorf("%s: unit %q", ErrUnitAlreadyRegistered, id) } m.units[id] = Unit{ @@ -135,14 +136,14 @@ func (m *Manager) AddDependency(unit ID, dependsOn ID, requiredStatus Status) er defer m.mu.Unlock() if !m.registered(unit) { - return xerrors.Errorf("unit %v is not registered: %w", unit, ErrUnitNotFound) + return xerrors.Errorf("%s: unit %q", ErrUnitNotFound, unit) } // Add the dependency edge to the graph // The edge goes from unit to dependsOn, representing the dependency err := m.graph.AddEdge(unit, dependsOn, requiredStatus) if err != nil { - return xerrors.Errorf("failed to add dependency: %w", err) + return xerrors.Errorf("%s: %w", ErrFailedToAddDependency, err) } // Recalculate readiness for the unit since it now has a new dependency @@ -158,10 +159,10 @@ func (m *Manager) UpdateStatus(unit ID, newStatus Status) error { u := m.units[unit] if !m.registered(unit) { - return xerrors.Errorf("unit %v is not registered: %w", unit, ErrUnitNotFound) + return xerrors.Errorf("%s: unit %q", ErrUnitNotFound, unit) } if u.status == newStatus { - return xerrors.Errorf("%s", ErrSameStatusAlreadySet) + return xerrors.Errorf("%s: unit %q", ErrSameStatusAlreadySet, unit) } u.status = newStatus @@ -239,7 +240,7 @@ func (m *Manager) GetAllDependencies(unit ID) ([]Dependency, error) { defer m.mu.RUnlock() if !m.registered(unit) { - return nil, xerrors.Errorf("%s", ErrUnitNotFound) + return nil, xerrors.Errorf("%s: unit %q", ErrUnitNotFound, unit) } forwardEdges := m.graph.GetForwardAdjacentVertices(unit) From 029fd18fbb47797fc7c73f741477ee9a44bb13e3 Mon Sep 17 00:00:00 2001 From: Sas Swart Date: Wed, 19 Nov 2025 16:42:57 +0000 Subject: [PATCH 14/14] Review notes Remove defunct code paths Improve error handling --- agent/unit/graph_test.go | 6 +-- agent/unit/manager.go | 93 ++++++++++++++++---------------------- agent/unit/manager_test.go | 10 ++-- 3 files changed, 48 insertions(+), 61 deletions(-) diff --git a/agent/unit/graph_test.go b/agent/unit/graph_test.go index 8c619aff4c3d9..f7d1117be74b3 100644 --- a/agent/unit/graph_test.go +++ b/agent/unit/graph_test.go @@ -148,7 +148,7 @@ func TestGraph(t *testing.T) { graph := &testGraph{} unit1 := &testGraphVertex{Name: "unit1"} err := graph.AddEdge(unit1, unit1, testEdgeCompleted) - require.ErrorContains(t, err, unit.ErrCycleDetected) + require.ErrorIs(t, err, unit.ErrCycleDetected) return graph }, @@ -159,7 +159,7 @@ func TestGraph(t *testing.T) { err := graph.AddEdge(unit1, unit2, testEdgeCompleted) require.NoError(t, err) err = graph.AddEdge(unit2, unit1, testEdgeStarted) - require.ErrorContains(t, err, unit.ErrCycleDetected) + require.ErrorIs(t, err, unit.ErrCycleDetected) return graph }, @@ -339,7 +339,7 @@ func TestGraphThreadSafety(t *testing.T) { // Verify all attempts correctly returned cycle error for i, err := range cycleErrors { require.Error(t, err, "goroutine %d should have detected cycle", i) - require.ErrorContains(t, err, unit.ErrCycleDetected) + require.ErrorIs(t, err, unit.ErrCycleDetected) } // Verify graph remains valid (original chain intact) diff --git a/agent/unit/manager.go b/agent/unit/manager.go index 3c0f713cabb03..56ec0213c131f 100644 --- a/agent/unit/manager.go +++ b/agent/unit/manager.go @@ -1,6 +1,7 @@ package unit import ( + "errors" "sync" "golang.org/x/xerrors" @@ -9,19 +10,19 @@ import ( ) var ( - ErrUnitNotFound = "unit not found" - ErrUnitAlreadyRegistered = "unit already registered" - ErrCannotUpdateOtherUnit = "cannot update other unit's status" - ErrDependenciesNotSatisfied = "unit dependencies not satisfied" - ErrSameStatusAlreadySet = "same status already set" - ErrCycleDetected = "cycle detected" - ErrFailedToAddDependency = "failed to add dependency" + ErrUnitNotFound = xerrors.New("unit not found") + ErrUnitAlreadyRegistered = xerrors.New("unit already registered") + ErrCannotUpdateOtherUnit = xerrors.New("cannot update other unit's status") + ErrDependenciesNotSatisfied = xerrors.New("unit dependencies not satisfied") + ErrSameStatusAlreadySet = xerrors.New("same status already set") + ErrCycleDetected = xerrors.New("cycle detected") + ErrFailedToAddDependency = xerrors.New("failed to add dependency") ) // Status represents the status of a unit. type Status string -// Status constants for dependency tracking +// Status constants for dependency tracking. const ( StatusNotRegistered Status = "" StatusPending Status = "pending" @@ -46,11 +47,11 @@ type Unit struct { ready bool } -func (u *Unit) ID() ID { +func (u Unit) ID() ID { return u.id } -func (u *Unit) Status() Status { +func (u Unit) Status() Status { return u.status } @@ -91,7 +92,7 @@ func (m *Manager) Register(id ID) error { defer m.mu.Unlock() if m.registered(id) { - return xerrors.Errorf("%s: unit %q", ErrUnitAlreadyRegistered, id) + return xerrors.Errorf("registering unit %q: %w", id, ErrUnitAlreadyRegistered) } m.units[id] = Unit{ @@ -136,14 +137,14 @@ func (m *Manager) AddDependency(unit ID, dependsOn ID, requiredStatus Status) er defer m.mu.Unlock() if !m.registered(unit) { - return xerrors.Errorf("%s: unit %q", ErrUnitNotFound, unit) + return xerrors.Errorf("checking registration for unit %q: %w", unit, ErrUnitNotFound) } // Add the dependency edge to the graph // The edge goes from unit to dependsOn, representing the dependency err := m.graph.AddEdge(unit, dependsOn, requiredStatus) if err != nil { - return xerrors.Errorf("%s: %w", ErrFailedToAddDependency, err) + return xerrors.Errorf("adding edge for unit %q: %w", unit, errors.Join(ErrFailedToAddDependency, err)) } // Recalculate readiness for the unit since it now has a new dependency @@ -157,54 +158,35 @@ func (m *Manager) UpdateStatus(unit ID, newStatus Status) error { m.mu.Lock() defer m.mu.Unlock() - u := m.units[unit] if !m.registered(unit) { - return xerrors.Errorf("%s: unit %q", ErrUnitNotFound, unit) + return xerrors.Errorf("checking registration for unit %q: %w", unit, ErrUnitNotFound) } + + u := m.units[unit] if u.status == newStatus { - return xerrors.Errorf("%s: unit %q", ErrSameStatusAlreadySet, unit) + return xerrors.Errorf("checking status for unit %q: %w", unit, ErrSameStatusAlreadySet) } u.status = newStatus m.units[unit] = u // Get all units that depend on this one (reverse adjacent vertices) - dependentEdges := m.graph.GetReverseAdjacentVertices(unit) + dependents := m.graph.GetReverseAdjacentVertices(unit) // Recalculate readiness for all dependents - for _, edge := range dependentEdges { - m.recalculateReadinessUnsafe(edge.From) + for _, dependent := range dependents { + m.recalculateReadinessUnsafe(dependent.From) } return nil } -// GetUnmetDependencies returns a list of unsatisfied dependencies for a unit. -func (m *Manager) GetUnmetDependencies(unit ID) ([]Dependency, error) { - allDependencies, err := m.GetAllDependencies(unit) - if err != nil { - return nil, err - } - - var unmetDependencies []Dependency = slice.Filter(allDependencies, func(dependency Dependency) bool { - return !dependency.IsSatisfied - }) - - return unmetDependencies, nil -} - // recalculateReadinessUnsafe recalculates the readiness state for a unit. // This method assumes the caller holds the write lock. func (m *Manager) recalculateReadinessUnsafe(unit ID) { u := m.units[unit] dependencies := m.graph.GetForwardAdjacentVertices(unit) - if len(dependencies) == 0 { - u.ready = true - m.units[unit] = u - return - } - allSatisfied := true for _, dependency := range dependencies { requiredStatus := dependency.Edge @@ -225,31 +207,22 @@ func (m *Manager) GetGraph() *Graph[Status, ID] { return m.graph } -// GetStatus returns the current status of a unit. -func (m *Manager) GetStatus(unit ID) Status { - m.mu.RLock() - defer m.mu.RUnlock() - - u := m.units[unit] - return u.status -} - // GetAllDependencies returns all dependencies for a unit, both satisfied and unsatisfied. func (m *Manager) GetAllDependencies(unit ID) ([]Dependency, error) { m.mu.RLock() defer m.mu.RUnlock() if !m.registered(unit) { - return nil, xerrors.Errorf("%s: unit %q", ErrUnitNotFound, unit) + return nil, xerrors.Errorf("checking registration for unit %q: %w", unit, ErrUnitNotFound) } - forwardEdges := m.graph.GetForwardAdjacentVertices(unit) + dependencies := m.graph.GetForwardAdjacentVertices(unit) var allDependencies []Dependency - for _, edge := range forwardEdges { - dependsOnUnit := m.units[edge.To] - requiredStatus := edge.Edge + for _, dependency := range dependencies { + dependsOnUnit := m.units[dependency.To] + requiredStatus := dependency.Edge allDependencies = append(allDependencies, Dependency{ Unit: unit, DependsOn: dependsOnUnit.id, @@ -262,6 +235,20 @@ func (m *Manager) GetAllDependencies(unit ID) ([]Dependency, error) { return allDependencies, nil } +// GetUnmetDependencies returns a list of unsatisfied dependencies for a unit. +func (m *Manager) GetUnmetDependencies(unit ID) ([]Dependency, error) { + allDependencies, err := m.GetAllDependencies(unit) + if err != nil { + return nil, err + } + + var unmetDependencies []Dependency = slice.Filter(allDependencies, func(dependency Dependency) bool { + return !dependency.IsSatisfied + }) + + return unmetDependencies, nil +} + // ExportDOT exports the dependency graph to DOT format for visualization. func (m *Manager) ExportDOT(name string) (string, error) { return m.graph.ToDOT(name) diff --git a/agent/unit/manager_test.go b/agent/unit/manager_test.go index 1e5847ad763bf..d85b1752a17b7 100644 --- a/agent/unit/manager_test.go +++ b/agent/unit/manager_test.go @@ -53,7 +53,7 @@ func TestManager_Register(t *testing.T) { err = manager.Register(unitA) // Then: a descriptive error should be returned - require.ErrorContains(t, err, unit.ErrUnitAlreadyRegistered) + require.ErrorIs(t, err, unit.ErrUnitAlreadyRegistered) // Then: the unit status should not be overwritten u := manager.Unit(unitA) @@ -139,7 +139,7 @@ func TestManager_AddDependency(t *testing.T) { // Then: a descriptive error communicates that the dependency cannot be added // because the dependent unit must be registered first. - require.ErrorContains(t, err, unit.ErrUnitNotFound) + require.ErrorIs(t, err, unit.ErrUnitNotFound) }) t.Run("AddDependencyOnAnUnregisteredUnit", func(t *testing.T) { @@ -206,7 +206,7 @@ func TestManager_AddDependency(t *testing.T) { // Try to make D depend on A (creates indirect cycle) err = manager.AddDependency(unitD, unitA, unit.StatusStarted) - require.ErrorContains(t, err, unit.ErrCycleDetected) + require.ErrorIs(t, err, unit.ErrCycleDetected) }) } @@ -253,7 +253,7 @@ func TestManager_UpdateStatus(t *testing.T) { err := manager.UpdateStatus(unitA, unit.StatusStarted) // Then: a descriptive error communicates that the unit must be registered first. - require.ErrorContains(t, err, unit.ErrUnitNotFound) + require.ErrorIs(t, err, unit.ErrUnitNotFound) }) t.Run("LinearChainDependencies", func(t *testing.T) { @@ -392,7 +392,7 @@ func TestManager_GetUnmetDependencies(t *testing.T) { unmet, err := manager.GetUnmetDependencies(unitA) // Then: a descriptive error communicates that the unit must be registered first. - require.ErrorContains(t, err, unit.ErrUnitNotFound) + require.ErrorIs(t, err, unit.ErrUnitNotFound) assert.Nil(t, unmet) }) }