feat: send log limit exceeded in response, not error (#12078)

spikecurtis · web-flow · commit 92b2e26a48c8 · 2024-02-09T16:17:20.000+04:00
When we exceed the db-imposed limit of logs, we need to communicate that back to the agent.  In v1 we did it with a 4xx-level HTTP status, but with dRPC, the errors are delivered as strings, which feels fragile to me for something we want to gracefully handle.

So, this PR adds the log limit exceeded as a field on the response message, and fixes the API handler to set it as appropriate instead of an error.
diff --git a/agent/proto/agent.pb.go b/agent/proto/agent.pb.go
diff --git a/agent/proto/agent.proto b/agent/proto/agent.proto
@@ -247,7 +247,9 @@ message BatchCreateLogsRequest {
 	repeated Log logs = 2;
 }
 
-message BatchCreateLogsResponse {}
+message BatchCreateLogsResponse {
+	bool log_limit_exceeded = 1;
+}
 
 service Agent {
 	rpc GetManifest(GetManifestRequest) returns (Manifest);
diff --git a/coderd/agentapi/logs.go b/coderd/agentapi/logs.go
@@ -37,7 +37,7 @@ func (a *LogsAPI) BatchCreateLogs(ctx context.Context, req *agentproto.BatchCrea
 		return nil, err
 	}
 	if workspaceAgent.LogsOverflowed {
-		return nil, xerrors.New("workspace agent logs overflowed")
+		return &agentproto.BatchCreateLogsResponse{LogLimitExceeded: true}, nil
 	}
 
 	if len(req.Logs) == 0 {
@@ -128,7 +128,7 @@ func (a *LogsAPI) BatchCreateLogs(ctx context.Context, req *agentproto.BatchCrea
 				return nil, xerrors.Errorf("publish workspace update: %w", err)
 			}
 		}
-		return nil, xerrors.New("workspace agent log limit exceeded")
+		return &agentproto.BatchCreateLogsResponse{LogLimitExceeded: true}, nil
 	}
 
 	// Publish by the lowest log ID inserted so the log stream will fetch
diff --git a/coderd/agentapi/logs_test.go b/coderd/agentapi/logs_test.go
@@ -215,9 +215,9 @@ func TestBatchCreateLogs(t *testing.T) {
 			LogSourceId: logSource.ID[:],
 			Logs:        []*agentproto.Log{},
 		})
-		require.Error(t, err)
-		require.ErrorContains(t, err, "workspace agent logs overflowed")
-		require.Nil(t, resp)
+		require.NoError(t, err)
+		require.NotNil(t, resp)
+		require.True(t, resp.LogLimitExceeded)
 		require.False(t, publishWorkspaceUpdateCalled)
 		require.False(t, publishWorkspaceAgentLogsUpdateCalled)
 	})
@@ -419,8 +419,9 @@ func TestBatchCreateLogs(t *testing.T) {
 				},
 			},
 		})
-		require.Error(t, err)
-		require.Nil(t, resp)
+		require.NoError(t, err)
+		require.NotNil(t, resp)
+		require.True(t, resp.LogLimitExceeded)
 		require.True(t, publishWorkspaceUpdateCalled)
 		require.False(t, publishWorkspaceAgentLogsUpdateCalled)
 	})

Original file line number	Diff line number	Diff line change
`@@ -247,7 +247,9 @@ message BatchCreateLogsRequest {`
`247`	`247`	`repeated Log logs = 2;`
`248`	`248`	`}`
`249`	`249`
`250`		`-message BatchCreateLogsResponse {}`
	`250`	`+message BatchCreateLogsResponse {`
	`251`	`+ bool log_limit_exceeded = 1;`
	`252`	`+}`
`251`	`253`
`252`	`254`	`service Agent {`
`253`	`255`	`rpc GetManifest(GetManifestRequest) returns (Manifest);`
Original file line number	Diff line number	Diff line change
`@@ -37,7 +37,7 @@ func (a LogsAPI) BatchCreateLogs(ctx context.Context, req agentproto.BatchCrea`
`37`	`37`	`return nil, err`
`38`	`38`	`}`
`39`	`39`	`if workspaceAgent.LogsOverflowed {`
`40`		`- return nil, xerrors.New("workspace agent logs overflowed")`
	`40`	`+ return &agentproto.BatchCreateLogsResponse{LogLimitExceeded: true}, nil`
`41`	`41`	`}`
`42`	`42`
`43`	`43`	`if len(req.Logs) == 0 {`
`@@ -128,7 +128,7 @@ func (a LogsAPI) BatchCreateLogs(ctx context.Context, req agentproto.BatchCrea`
`128`	`128`	`return nil, xerrors.Errorf("publish workspace update: %w", err)`
`129`	`129`	`}`
`130`	`130`	`}`
`131`		`- return nil, xerrors.New("workspace agent log limit exceeded")`
	`131`	`+ return &agentproto.BatchCreateLogsResponse{LogLimitExceeded: true}, nil`
`132`	`132`	`}`
`133`	`133`
`134`	`134`	`// Publish by the lowest log ID inserted so the log stream will fetch`