code cleanup in agent/

ammario · ammario · commit b7ddb4a319d0 · 2023-03-27T19:13:32.000Z
diff --git a/agent/agent.go b/agent/agent.go
@@ -223,6 +223,12 @@ func (a *agent) collectMetadata(ctx context.Context, md codersdk.WorkspaceAgentM
 	var out bytes.Buffer
 
 	result := &codersdk.WorkspaceAgentMetadataResult{
+		// CollectedAt is set here for testing purposes and overrided by
+		// the server to the time the server received the result to protect
+		// against clock skew.
+		//
+		// In the future, the server may accept the timestamp from the agent
+		// if it is certain the clocks are in sync.
 		CollectedAt: time.Now(),
 	}
 	cmd, err := a.createCommand(ctx, md.Script, nil)
@@ -237,7 +243,7 @@ func (a *agent) collectMetadata(ctx context.Context, md codersdk.WorkspaceAgentM
 	// The error isn't mutually exclusive with useful output.
 	err = cmd.Run()
 
-	const bufLimit = 10 << 14
+	const bufLimit = 10 << 10
 	if out.Len() > bufLimit {
 		err = errors.Join(
 			err,
@@ -293,7 +299,7 @@ func (a *agent) reportMetadataLoop(ctx context.Context) {
 				// If we're backpressured on sending back results, we risk
 				// runaway goroutine growth and/or overloading coderd. So,
 				// we just skip the collection. Since we never update
-				// "lastCollectedAt" for this key, we'll retry the collection
+				// the collections map, we'll retry the collection
 				// on the next tick.
 				a.logger.Debug(
 					ctx, "metadata collection backpressured",
@@ -317,8 +323,9 @@ func (a *agent) reportMetadataLoop(ctx context.Context) {
 				}
 			}
 
-			// Spawn a goroutine for each metadata collection, and use channels
-			// to synchronize the results and avoid messy mutex logic.
+			// Spawn a goroutine for each metadata collection, and use a
+			// channel to synchronize the results and avoid both messy
+			// mutex logic and overloading the API.
 			for _, md := range manifest.Metadata {
 				collectedAt, ok := lastCollectedAts[md.Key]
 				if ok {
diff --git a/agent/agent_test.go b/agent/agent_test.go
@@ -905,16 +905,21 @@ func TestAgent_Metadata(t *testing.T) {
 
 	t.Run("Many", func(t *testing.T) {
 		if runtime.GOOS == "windows" {
-			// Shell scripting in Windows is a pain, but we test that it works in the simpler "CollectOnce"
-			// test.
+			// Shell scripting in Windows is a pain, and we have already tested
+			// that the OS logic works in the simpler "Once" test above.
 			t.Skip()
 		}
 		t.Parallel()
 
 		dir := t.TempDir()
+
+		// In tests, the interval unit is 100 milliseconds while in production
+		// it is 1 second.
 		const reportInterval = 2
-		greetingPath := filepath.Join(dir, "greeting")
-		script := "echo hello | tee -a " + greetingPath
+		var (
+			greetingPath = filepath.Join(dir, "greeting")
+			script       = "echo hello | tee -a " + greetingPath
+		)
 		_, client, _, _, _ := setupAgent(t, agentsdk.Manifest{
 			Metadata: []codersdk.WorkspaceAgentMetadataDescription{
 				{
@@ -949,12 +954,16 @@ func TestAgent_Metadata(t *testing.T) {
 			var (
 				numGreetings      = bytes.Count(greetingByt, []byte("hello"))
 				idealNumGreetings = time.Since(start) / (reportInterval * 100 * time.Millisecond)
-				upperBound        = int(idealNumGreetings) + 1
-				lowerBound        = (int(idealNumGreetings) / 2)
+				// We allow a 50% error margin because the report loop may backlog
+				// in CI and other toasters. In production, there is no hard
+				// guarantee on timing either, and the frontend gives similar
+				// wiggle room to the staleness of the value.
+				upperBound = int(idealNumGreetings) + 1
+				lowerBound = (int(idealNumGreetings) / 2)
 			)
 
 			if idealNumGreetings < 5 {
-				// Not enough time has passed to get a good sample size.
+				// There is an insufficient sample size.
 				continue
 			}
 
