Skip to content

Commit c771e2e

Browse files
code-ashercode-asher
committed
Allow partial app status updates
Since we will be sending status updates without messages on an interval, we need to prevent duplicates and preserve the previous message. Also, I think this makes the endpoint more in alignment with PATCH semantics.
1 parent f1c742e commit c771e2e

File tree

10 files changed

+182
-1
lines changed

10 files changed

+182
-1
lines changed

‎coderd/database/dbauthz/dbauthz.go

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1991,6 +1991,13 @@ func (q *querier) GetLatestCryptoKeyByFeature(ctx context.Context, feature datab
19911991
return q.db.GetLatestCryptoKeyByFeature(ctx, feature)
19921992
}
19931993

1994+
func (q *querier) GetLatestWorkspaceAppStatusByAppID(ctx context.Context, appID uuid.UUID) (database.WorkspaceAppStatus, error) {
1995+
if err := q.authorizeContext(ctx, policy.ActionRead, rbac.ResourceSystem); err != nil {
1996+
return database.WorkspaceAppStatus{}, err
1997+
}
1998+
return q.db.GetLatestWorkspaceAppStatusByAppID(ctx, appID)
1999+
}
2000+
19942001
func (q *querier) GetLatestWorkspaceAppStatusesByWorkspaceIDs(ctx context.Context, ids []uuid.UUID) ([]database.WorkspaceAppStatus, error) {
19952002
if err := q.authorizeContext(ctx, policy.ActionRead, rbac.ResourceSystem); err != nil {
19962003
return nil, err

‎coderd/database/dbauthz/dbauthz_test.go

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3801,6 +3801,12 @@ func (s *MethodTestSuite) TestSystemFunctions() {
38013801
LoginType: database.LoginTypeGithub,
38023802
}).Asserts(rbac.ResourceSystem, policy.ActionUpdate).Returns(l)
38033803
}))
3804+
s.Run("GetLatestWorkspaceAppStatusByAppID", s.Subtest(func(db database.Store, check *expects) {
3805+
check.Args(uuid.New()).
3806+
Asserts(rbac.ResourceSystem, policy.ActionRead).
3807+
ErrorsWithInMemDB(sql.ErrNoRows).
3808+
Returns(database.WorkspaceAppStatus{})
3809+
}))
38043810
s.Run("GetLatestWorkspaceAppStatusesByWorkspaceIDs", s.Subtest(func(db database.Store, check *expects) {
38053811
check.Args([]uuid.UUID{}).Asserts(rbac.ResourceSystem, policy.ActionRead)
38063812
}))

‎coderd/database/dbmem/dbmem.go

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3777,6 +3777,22 @@ func (q *FakeQuerier) GetLatestCryptoKeyByFeature(_ context.Context, feature dat
37773777
return latestKey, nil
37783778
}
37793779

3780+
func (q *FakeQuerier) GetLatestWorkspaceAppStatusByAppID(ctx context.Context, appID uuid.UUID) (database.WorkspaceAppStatus, error) {
3781+
q.mutex.RLock()
3782+
defer q.mutex.RUnlock()
3783+
3784+
var current *database.WorkspaceAppStatus = nil
3785+
for _, appStatus := range q.workspaceAppStatuses {
3786+
if appStatus.AppID == appID && (current == nil || appStatus.CreatedAt.After(current.CreatedAt)) {
3787+
current = &appStatus
3788+
}
3789+
}
3790+
if current == nil {
3791+
return database.WorkspaceAppStatus{}, sql.ErrNoRows
3792+
}
3793+
return *current, nil
3794+
}
3795+
37803796
func (q *FakeQuerier) GetLatestWorkspaceAppStatusesByWorkspaceIDs(_ context.Context, ids []uuid.UUID) ([]database.WorkspaceAppStatus, error) {
37813797
q.mutex.RLock()
37823798
defer q.mutex.RUnlock()

‎coderd/database/dbmetrics/querymetrics.go

Lines changed: 7 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎coderd/database/dbmock/dbmock.go

Lines changed: 15 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎coderd/database/querier.go

Lines changed: 1 addition & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎coderd/database/queries.sql.go

Lines changed: 21 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎coderd/database/queries/workspaceapps.sql

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -58,3 +58,7 @@ SELECT DISTINCT ON (workspace_id)
5858
FROM workspace_app_statuses
5959
WHERE workspace_id = ANY(@ids :: uuid[])
6060
ORDER BY workspace_id, created_at DESC;
61+
62+
-- name: GetLatestWorkspaceAppStatusByAppID :one
63+
SELECT * FROM workspace_app_statuses WHERE app_id = $1
64+
ORDER BY created_at DESC LIMIT 1;

‎coderd/workspaceagents.go

Lines changed: 30 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -380,14 +380,43 @@ func (api *API) patchWorkspaceAgentAppStatus(rw http.ResponseWriter, r *http.Req
380380
return
381381
}
382382

383+
// nolint:gocritic // This is a system restricted operation.
384+
status, err := api.Database.GetLatestWorkspaceAppStatusByAppID(dbauthz.AsSystemRestricted(ctx), app.ID)
385+
if err != nil && !errors.Is(err, sql.ErrNoRows) {
386+
httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
387+
Message: "Internal error fetching workspace app statuses.",
388+
Detail: err.Error(),
389+
})
390+
return
391+
}
392+
// Preserve the existing message, which allows partial updates where we change
393+
// the status but keep the message.
394+
// TODO: What if another status is added while we are doing these checks?
395+
// This would essentially revert the message to the previous one. We could
396+
// maybe do this in the query instead?
397+
if req.Message == "" {
398+
req.Message = status.Message
399+
// Only preserve the URI if there was no message. If there is a message and
400+
// a blank URI, we assume the intent is to remove the URI, not preserve it.
401+
if req.URI == "" {
402+
req.URI = status.Uri.String
403+
}
404+
}
405+
// Skip duplicate status updates.
406+
newState := database.WorkspaceAppStatusState(req.State)
407+
if status.State == newState && status.Message == req.Message && status.Uri.String == req.URI {
408+
httpapi.Write(ctx, rw, http.StatusOK, nil)
409+
return
410+
}
411+
383412
// nolint:gocritic // This is a system restricted operation.
384413
_, err = api.Database.InsertWorkspaceAppStatus(dbauthz.AsSystemRestricted(ctx), database.InsertWorkspaceAppStatusParams{
385414
ID: uuid.New(),
386415
CreatedAt: dbtime.Now(),
387416
WorkspaceID: workspace.ID,
388417
AgentID: workspaceAgent.ID,
389418
AppID: app.ID,
390-
State: database.WorkspaceAppStatusState(req.State),
419+
State: newState,
391420
Message: req.Message,
392421
Uri: sql.NullString{
393422
String: req.URI,

‎coderd/workspaceagents_test.go

Lines changed: 75 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -387,6 +387,81 @@ func TestWorkspaceAgentAppStatus(t *testing.T) {
387387
require.False(t, agent.Apps[0].Statuses[0].NeedsUserAttention)
388388
})
389389

390+
t.Run("Patch", func(t *testing.T) {
391+
t.Parallel()
392+
ctx := testutil.Context(t, testutil.WaitShort)
393+
394+
ws := dbfake.WorkspaceBuild(t, db, database.WorkspaceTable{
395+
OrganizationID: user.OrganizationID,
396+
OwnerID: user2.ID,
397+
}).WithAgent(func(a []*proto.Agent) []*proto.Agent {
398+
a[0].Apps = []*proto.App{
399+
{
400+
Slug: "vscode",
401+
},
402+
}
403+
return a
404+
}).Do()
405+
406+
agentClient := agentsdk.New(client.URL)
407+
agentClient.SetSessionToken(ws.AgentToken)
408+
409+
err := agentClient.PatchAppStatus(ctx, agentsdk.PatchAppStatus{
410+
AppSlug: "vscode",
411+
Message: "testing",
412+
URI: "https://example.com",
413+
State: codersdk.WorkspaceAppStatusStateComplete,
414+
})
415+
require.NoError(t, err)
416+
417+
// Duplicate should be a no-op.
418+
err = agentClient.PatchAppStatus(ctx, agentsdk.PatchAppStatus{
419+
AppSlug: "vscode",
420+
Message: "testing",
421+
URI: "https://example.com",
422+
State: codersdk.WorkspaceAppStatusStateComplete,
423+
})
424+
require.NoError(t, err)
425+
426+
// If no message, both the old message and URI are preserved, and this ends
427+
// up being a duplicate as well.
428+
err = agentClient.PatchAppStatus(ctx, agentsdk.PatchAppStatus{
429+
AppSlug: "vscode",
430+
State: codersdk.WorkspaceAppStatusStateComplete,
431+
})
432+
require.NoError(t, err)
433+
434+
// Same, but the status updated so it will not be a duplicate.
435+
err = agentClient.PatchAppStatus(ctx, agentsdk.PatchAppStatus{
436+
AppSlug: "vscode",
437+
State: codersdk.WorkspaceAppStatusStateWorking,
438+
})
439+
require.NoError(t, err)
440+
441+
// Message update, so nothing is preserved.
442+
err = agentClient.PatchAppStatus(ctx, agentsdk.PatchAppStatus{
443+
AppSlug: "vscode",
444+
Message: "updated",
445+
State: codersdk.WorkspaceAppStatusStateWorking,
446+
})
447+
require.NoError(t, err)
448+
449+
workspace, err := client.Workspace(ctx, ws.Workspace.ID)
450+
require.NoError(t, err)
451+
agent, err := client.WorkspaceAgent(ctx, workspace.LatestBuild.Resources[0].Agents[0].ID)
452+
require.NoError(t, err)
453+
require.Len(t, agent.Apps[0].Statuses, 3)
454+
require.Equal(t, agent.Apps[0].Statuses[0].State, codersdk.WorkspaceAppStatusStateComplete)
455+
require.Equal(t, agent.Apps[0].Statuses[0].Message, "testing")
456+
require.Equal(t, agent.Apps[0].Statuses[0].URI, "https://example.com")
457+
require.Equal(t, agent.Apps[0].Statuses[1].State, codersdk.WorkspaceAppStatusStateWorking)
458+
require.Equal(t, agent.Apps[0].Statuses[1].Message, "testing")
459+
require.Equal(t, agent.Apps[0].Statuses[1].URI, "https://example.com")
460+
require.Equal(t, agent.Apps[0].Statuses[2].State, codersdk.WorkspaceAppStatusStateWorking)
461+
require.Equal(t, agent.Apps[0].Statuses[2].Message, "updated")
462+
require.Equal(t, agent.Apps[0].Statuses[2].URI, "")
463+
})
464+
390465
t.Run("FailUnknownApp", func(t *testing.T) {
391466
t.Parallel()
392467
ctx := testutil.Context(t, testutil.WaitShort)

0 commit comments

Comments
 (0)