diff --git a/workspace-server/internal/handlers/workspace_abilities.go b/workspace-server/internal/handlers/workspace_abilities.go index ff8efab70..e6a9e0bdc 100644 --- a/workspace-server/internal/handlers/workspace_abilities.go +++ b/workspace-server/internal/handlers/workspace_abilities.go @@ -56,7 +56,20 @@ func PatchAbilities(c *gin.Context) { return } - if body.BroadcastEnabled != nil { + // Atomic update: when both fields are supplied, apply them in one SQL + // statement so the request is all-or-nothing (#2131). A partial mutation + // (e.g. broadcast_enabled updated but talk_to_user_enabled failing) would + // leave the workspace in an ambiguous capability state. + if body.BroadcastEnabled != nil && body.TalkToUserEnabled != nil { + if _, err := db.DB.ExecContext(ctx, + `UPDATE workspaces SET broadcast_enabled = $2, talk_to_user_enabled = $3, updated_at = now() WHERE id = $1`, + id, *body.BroadcastEnabled, *body.TalkToUserEnabled, + ); err != nil { + log.Printf("PatchAbilities both-fields for %s: %v", id, err) + c.JSON(http.StatusInternalServerError, gin.H{"error": "update failed"}) + return + } + } else if body.BroadcastEnabled != nil { if _, err := db.DB.ExecContext(ctx, `UPDATE workspaces SET broadcast_enabled = $2, updated_at = now() WHERE id = $1`, id, *body.BroadcastEnabled, @@ -65,9 +78,7 @@ func PatchAbilities(c *gin.Context) { c.JSON(http.StatusInternalServerError, gin.H{"error": "update failed"}) return } - } - - if body.TalkToUserEnabled != nil { + } else if body.TalkToUserEnabled != nil { if _, err := db.DB.ExecContext(ctx, `UPDATE workspaces SET talk_to_user_enabled = $2, updated_at = now() WHERE id = $1`, id, *body.TalkToUserEnabled, diff --git a/workspace-server/internal/handlers/workspace_abilities_test.go b/workspace-server/internal/handlers/workspace_abilities_test.go index d2fc5a08c..1e63f1f6b 100644 --- a/workspace-server/internal/handlers/workspace_abilities_test.go +++ b/workspace-server/internal/handlers/workspace_abilities_test.go @@ -130,11 +130,8 @@ func TestPatchAbilities_BothFields(t *testing.T) { mock.ExpectQuery(`SELECT EXISTS\(SELECT 1 FROM workspaces WHERE id = \$1 AND status != 'removed'\)`). WithArgs(wsUUID1). WillReturnRows(sqlmock.NewRows([]string{"exists"}).AddRow(true)) - mock.ExpectExec(`UPDATE workspaces SET broadcast_enabled = \$2, updated_at = now\(\) WHERE id = \$1`). - WithArgs(wsUUID1, true). - WillReturnResult(sqlmock.NewResult(0, 1)) - mock.ExpectExec(`UPDATE workspaces SET talk_to_user_enabled = \$2, updated_at = now\(\) WHERE id = \$1`). - WithArgs(wsUUID1, true). + mock.ExpectExec(`UPDATE workspaces SET broadcast_enabled = \$2, talk_to_user_enabled = \$3, updated_at = now\(\) WHERE id = \$1`). + WithArgs(wsUUID1, true, true). WillReturnResult(sqlmock.NewResult(0, 1)) w := patchAbilitiesReq(t, wsUUID1, `{"broadcast_enabled":true,"talk_to_user_enabled":true}`) @@ -182,19 +179,25 @@ func TestPatchAbilities_TalkToUserUpdateError(t *testing.T) { } } -func TestPatchAbilities_BothFields_BroadcastFails(t *testing.T) { +// TestPatchAbilities_BothFields_UpdateError — regression for #2131. When +// both fields are supplied the handler uses a single combined UPDATE. A +// failure of that UPDATE must leave the workspace unchanged (atomic). +func TestPatchAbilities_BothFields_UpdateError(t *testing.T) { mock, cleanup := withMockDB(t) defer cleanup() mock.ExpectQuery(`SELECT EXISTS\(SELECT 1 FROM workspaces WHERE id = \$1 AND status != 'removed'\)`). WithArgs(wsUUID1). WillReturnRows(sqlmock.NewRows([]string{"exists"}).AddRow(true)) - mock.ExpectExec(`UPDATE workspaces SET broadcast_enabled = \$2, updated_at = now\(\) WHERE id = \$1`). - WithArgs(wsUUID1, true). + mock.ExpectExec(`UPDATE workspaces SET broadcast_enabled = \$2, talk_to_user_enabled = \$3, updated_at = now\(\) WHERE id = \$1`). + WithArgs(wsUUID1, true, true). WillReturnError(errors.New("disk full")) w := patchAbilitiesReq(t, wsUUID1, `{"broadcast_enabled":true,"talk_to_user_enabled":true}`) if w.Code != http.StatusInternalServerError { t.Fatalf("expected 500, got %d: %s", w.Code, w.Body.String()) } + // Because only one UPDATE is issued, there is no partial-mutation + // path to assert against; sqlmock implicitly verifies no second + // exec occurred. }