chore: feedback

coder · DanielleMaywood · Feb 17, 2025 · Jan 24, 2025 · Jan 30, 2025 · Jan 30, 2025
commit babc48f8cfd6faf3a73e3af3c3d7d40c4a43b7f5
diff --git a/coderd/agentapi/resources_monitoring.go b/coderd/agentapi/resources_monitoring.go
@@ -125,32 +125,39 @@ func (a *ResourcesMonitoringAPI) monitorMemory(ctx context.Context, datapoints [
 		return xerrors.Errorf("update workspace monitor: %w", err)
 	}
 
-	if shouldNotify {
-		workspace, err := a.Database.GetWorkspaceByID(ctx, a.WorkspaceID)
-		if err != nil {
-			return xerrors.Errorf("get workspace by id: %w", err)
-		}
+	if !shouldNotify {
+		return nil
+	}
 
-		_, err = a.NotificationsEnqueuer.EnqueueWithData(
-			// nolint:gocritic // We need to be able to send the notification.
-			dbauthz.AsNotifier(ctx),
-			workspace.OwnerID,
-			notifications.TemplateWorkspaceOutOfMemory,
-			map[string]string{
-				"workspace": workspace.Name,
-				"threshold": fmt.Sprintf("%d%%", monitor.Threshold),
-			},
-			map[string]any{
-				// NOTE(DanielleMaywood):
-				// We are injecting a timestamp to circumvent the notification
-				// deduplication logic.
-				"timestamp": a.Clock.Now(),
-			},
-			"workspace-monitor-memory",
-		)
-		if err != nil {
-			return xerrors.Errorf("notify workspace OOM: %w", err)
-		}
+	workspace, err := a.Database.GetWorkspaceByID(ctx, a.WorkspaceID)
+	if err != nil {
+		return xerrors.Errorf("get workspace by id: %w", err)
+	}
+
+	_, err = a.NotificationsEnqueuer.EnqueueWithData(
+		// nolint:gocritic // We need to be able to send the notification.
+		dbauthz.AsNotifier(ctx),
+		workspace.OwnerID,
+		notifications.TemplateWorkspaceOutOfMemory,
+		map[string]string{
+			"workspace": workspace.Name,
+			"threshold": fmt.Sprintf("%d%%", monitor.Threshold),
+		},
+		map[string]any{
+			// NOTE(DanielleMaywood):
+			// When notifications are enqueued, they are checked to be
+			// unique within a single day. This means that if we attempt
+			// to send two OOM notifications for the same workspace on
+			// the same day, the enqueuer will prevent us from sending
+			// a second one. We are inject a timestamp to make the
+			// notifications appear different enough to circumvent this
+			// deduplication logic.
+			"timestamp": a.Clock.Now(),
+		},
+		"workspace-monitor-memory",
+	)
+	if err != nil {
+		return xerrors.Errorf("notify workspace OOM: %w", err)
 	}
 
 	return nil
@@ -209,31 +216,38 @@ func (a *ResourcesMonitoringAPI) monitorVolumes(ctx context.Context, datapoints
 		}
 	}
 
-	if len(outOfDiskVolumes) != 0 {
-		workspace, err := a.Database.GetWorkspaceByID(ctx, a.WorkspaceID)
-		if err != nil {
-			return xerrors.Errorf("get workspace by id: %w", err)
-		}
+	if len(outOfDiskVolumes) == 0 {
+		return nil
+	}
 
-		if _, err := a.NotificationsEnqueuer.EnqueueWithData(
-			// nolint:gocritic // We need to be able to send the notification.
-			dbauthz.AsNotifier(ctx),
-			workspace.OwnerID,
-			notifications.TemplateWorkspaceOutOfDisk,
-			map[string]string{
-				"workspace": workspace.Name,
-			},
-			map[string]any{
-				"volumes": outOfDiskVolumes,
-				// NOTE(DanielleMaywood):
-				// We are injecting a timestamp to circumvent the notification
-				// deduplication logic.
-				"timestamp": a.Clock.Now(),
-			},
-			"workspace-monitor-volumes",
-		); err != nil {
-			return xerrors.Errorf("notify workspace OOD: %w", err)
-		}
+	workspace, err := a.Database.GetWorkspaceByID(ctx, a.WorkspaceID)
+	if err != nil {
+		return xerrors.Errorf("get workspace by id: %w", err)
+	}
+
+	if _, err := a.NotificationsEnqueuer.EnqueueWithData(
+		// nolint:gocritic // We need to be able to send the notification.
+		dbauthz.AsNotifier(ctx),
+		workspace.OwnerID,
+		notifications.TemplateWorkspaceOutOfDisk,
+		map[string]string{
+			"workspace": workspace.Name,
+		},
+		map[string]any{
+			"volumes": outOfDiskVolumes,
+			// NOTE(DanielleMaywood):
+			// When notifications are enqueued, they are checked to be
+			// unique within a single day. This means that if we attempt
+			// to send two OOM notifications for the same workspace on
+			// the same day, the enqueuer will prevent us from sending
+			// a second one. We are inject a timestamp to make the
+			// notifications appear different enough to circumvent this
+			// deduplication logic.
+			"timestamp": a.Clock.Now(),
+		},
+		"workspace-monitor-volumes",
+	); err != nil {
+    		return xerrors.Errorf("notify workspace OOD: %w", err)
 	}
 
 	return nil

diff --git a/coderd/agentapi/resourcesmonitor/resources_monitor.go b/coderd/agentapi/resourcesmonitor/resources_monitor.go
@@ -82,6 +82,9 @@ func NextState(c Config, oldState database.WorkspaceAgentMonitorState, states []
 		return database.WorkspaceAgentMonitorStateNOK
 	}
 
+	// We do not explicitly handle StateUnknown because it could have
+	// been either StateOK or StateNOK if collection didn't fail. As
+	// it could be either, our best bet is to ignore it.
 	nokCount, okCount := 0, 0
 	for _, state := range states {
 		switch state {

diff --git a/coderd/database/dbmem/dbmem.go b/coderd/database/dbmem/dbmem.go
@@ -9649,6 +9649,9 @@ func (q *FakeQuerier) UpdateMemoryResourceMonitor(_ context.Context, arg databas
 		return err
 	}
 
+	q.mutex.Lock()
+	defer q.mutex.Unlock()
+
 	for i, monitor := range q.workspaceAgentMemoryResourceMonitors {
 		if monitor.AgentID != arg.AgentID {
 			continue
@@ -10448,6 +10451,9 @@ func (q *FakeQuerier) UpdateVolumeResourceMonitor(_ context.Context, arg databas
 		return err
 	}
 
+	q.mutex.Lock()
+	defer q.mutex.Unlock()
+
 	for i, monitor := range q.workspaceAgentVolumeResourceMonitors {
 		if monitor.AgentID != arg.AgentID || monitor.Path != arg.Path {
 			continue