From 40d0493556375ab89dcaaf2c28946d9db05bfc9f Mon Sep 17 00:00:00 2001 From: "Molecule AI Dev Engineer A (Kimi)" Date: Tue, 2 Jun 2026 20:54:52 +0000 Subject: [PATCH] fix(handlers): make PatchAbilities atomic when both fields supplied (#2131) Previously PatchAbilities applied broadcast_enabled and talk_to_user_enabled with two separate UPDATE statements. If the first succeeded and the second failed, the workspace was left in a partial/ ambiguous capability state. When both fields are present in the PATCH body, apply them in a single combined UPDATE so the mutation is all-or-nothing. Single-field updates continue to use the original per-column statements. Updates the existing BothFields test to expect one combined UPDATE, and replaces the old BothFields_BroadcastFails test with BothFields_UpdateError which validates the atomic path. Fixes #2131 Co-Authored-By: Claude Opus 4.7 --- .../internal/handlers/workspace_abilities.go | 19 +++++++++++++++---- .../handlers/workspace_abilities_test.go | 19 +++++++++++-------- 2 files changed, 26 insertions(+), 12 deletions(-) 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. } -- 2.52.0