From d401a8710f4204f4b917ab79838a392711527dd6 Mon Sep 17 00:00:00 2001 From: Hongming Wang Date: Sun, 7 Jun 2026 19:46:04 -0700 Subject: [PATCH 1/3] feat(ws-server): POST /workspaces/:id/switch-provider (ordered, leak-safe) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Switch an EXISTING workspace to a different cloud (compute.provider). The VM is cloud-specific so this reprovisions the box on the new cloud; the agent's durable identity (agent_card, config, secrets, tokens, memory) lives in the tenant DB and is preserved (the row is never deleted). CRITICAL ordering (RFC #622 Hazard 1): stop the OLD box with the OLD provider BEFORE writing the new provider — cpStopWithRetry resolves provider+instance_id from the row at call time, so writing the new provider first would deprovision the old box against the new backend and leak it. Sequence: stop-old -> clear instance_id + jsonb_set provider -> provision-new (withStoredCompute re-reads the new provider). Clearing instance_id makes a retried switch safe. Guards: SaaS-only (cpProv), external/mock no-op, paused-parent, same- provider no-op, and a confirm_data_loss gate for persistent volumes (the filesystem cross-cloud migration is RFC #622 PR3/PR4). Tests: source-level ordering pin (stop-before-write), bad/missing-provider 400s, route wiring. PR2 of the switch-existing-workspace-provider series. Stacked on PR1 (#2420, the compute.provider validation it reuses). Co-Authored-By: Claude Opus 4.8 (1M context) --- .../handlers/workspace_switch_provider.go | 158 ++++++++++++++++++ .../workspace_switch_provider_test.go | 81 +++++++++ workspace-server/internal/router/router.go | 1 + 3 files changed, 240 insertions(+) create mode 100644 workspace-server/internal/handlers/workspace_switch_provider.go create mode 100644 workspace-server/internal/handlers/workspace_switch_provider_test.go diff --git a/workspace-server/internal/handlers/workspace_switch_provider.go b/workspace-server/internal/handlers/workspace_switch_provider.go new file mode 100644 index 000000000..be37a379c --- /dev/null +++ b/workspace-server/internal/handlers/workspace_switch_provider.go @@ -0,0 +1,158 @@ +package handlers + +import ( + "context" + "database/sql" + "io" + "log" + "net/http" + "strings" + + "git.moleculesai.app/molecule-ai/molecule-core/workspace-server/internal/db" + "git.moleculesai.app/molecule-ai/molecule-core/workspace-server/internal/events" + "git.moleculesai.app/molecule-ai/molecule-core/workspace-server/internal/models" + "github.com/gin-gonic/gin" +) + +// SwitchProvider handles POST /workspaces/:id/switch-provider — move an EXISTING +// workspace to a different cloud (compute.provider). The VM is cloud-specific, so +// this reprovisions the box on the new cloud; the agent's durable identity +// (agent_card, config, secrets, tokens, memory) lives in the tenant DB and is +// preserved because the row is never deleted. +// +// CRITICAL ORDERING (the wrong-backend leak, RFC #622 Hazard 1): the stop must +// run with the OLD provider BEFORE the DB provider is changed. cpStopWithRetry +// resolves provider + instance_id from the workspaces row at call time; if we +// wrote the new provider first, the stop would issue +// DELETE …?instance_id=&provider= → CP routes teardown to the NEW +// backend → the old box is never terminated and leaks (billed forever, and not +// covered by the status='removed' orphan sweeper). So the sequence is strictly: +// +// 1. stop OLD box (DB still has old provider + old instance_id) +// 2. clear instance_id + write new provider (jsonb_set, preserving the rest) +// 3. provision NEW box (withStoredCompute now reads the new provider) +// +// Clearing instance_id in step 2 also makes a retried switch safe: a second call +// finds no stale instance to (mis-)deprovision against the new backend. +func (h *WorkspaceHandler) SwitchProvider(c *gin.Context) { + id := c.Param("id") + ctx := c.Request.Context() + + var body struct { + Provider string `json:"provider"` + ConfirmDataLoss bool `json:"confirm_data_loss"` + // MigrateData is accepted for forward-compat (RFC #622 PR4 wires the + // filesystem migration). Until then it is a no-op and a persistent + // volume still requires confirm_data_loss. + MigrateData bool `json:"migrate_data"` + } + if err := c.ShouldBindJSON(&body); err != nil && err != io.EOF { + c.JSON(http.StatusBadRequest, gin.H{"error": "invalid JSON body"}) + return + } + newProvider := strings.ToLower(strings.TrimSpace(body.Provider)) + if newProvider == "" { + c.JSON(http.StatusBadRequest, gin.H{"error": "provider is required"}) + return + } + if _, ok := workspaceComputeProviderAllowlist[newProvider]; !ok { + c.JSON(http.StatusBadRequest, gin.H{"error": "unsupported provider (want aws|gcp|hetzner)"}) + return + } + + var status, wsName, dbRuntime, oldProvider, dataPersistence string + var tier int + err := db.DB.QueryRowContext(ctx, ` + SELECT status, name, tier, COALESCE(runtime, 'claude-code'), + COALESCE(compute->>'provider', ''), COALESCE(compute->>'data_persistence', '') + FROM workspaces WHERE id = $1`, id, + ).Scan(&status, &wsName, &tier, &dbRuntime, &oldProvider, &dataPersistence) + if err == sql.ErrNoRows || status == string(models.StatusRemoved) { + c.JSON(http.StatusNotFound, gin.H{"error": "workspace not found"}) + return + } + if err != nil { + c.JSON(http.StatusInternalServerError, gin.H{"error": "lookup failed"}) + return + } + + // Switching the cloud backend is a SaaS-only concept — a self-hosted Docker + // workspace has no cloud provider to switch. external/mock runtimes have no + // box at all. + if h.cpProv == nil { + c.JSON(http.StatusConflict, gin.H{"error": "provider switching is only available for cloud (SaaS) workspaces"}) + return + } + if isExternalLikeRuntime(dbRuntime) || dbRuntime == "mock" { + c.JSON(http.StatusConflict, gin.H{"error": dbRuntime + " workspaces have no cloud box to switch"}) + return + } + if paused, parentName := isParentPaused(ctx, id); paused { + c.JSON(http.StatusConflict, gin.H{"error": "parent workspace \"" + parentName + "\" is paused — resume it first"}) + return + } + + // "" provider means the default AWS path. + effectiveOld := oldProvider + if effectiveOld == "" { + effectiveOld = "aws" + } + if newProvider == effectiveOld { + c.JSON(http.StatusOK, gin.H{"status": "noop", "provider": newProvider, "message": "workspace is already on this provider"}) + return + } + + // Data gate: a persistent data volume is cloud-specific and cannot move as a + // block device. Until the filesystem migration lands (RFC #622 PR3/PR4), the + // switch is allowed only with an explicit confirm_data_loss. + if dataPersistence == "persist" && !body.ConfirmDataLoss { + c.JSON(http.StatusConflict, gin.H{ + "error": "DATA_LOSS_UNCONFIRMED", + "detail": "this workspace has a persistent data volume that cannot move across clouds; set confirm_data_loss=true to switch with a fresh volume (cross-cloud data migration is RFC #622, not yet wired). Identity/config/secrets/memory are preserved regardless.", + }) + return + } + + // --- ordered switch (see doc-comment) --- + + // 1. Stop the OLD box with the OLD provider. DB is unchanged here, so + // cpStopWithRetry resolves the old provider + old instance_id. Synchronous + // (bounded retry); proceeds even on exhaustion so a stuck old box never + // strands the switch — the audit log + reconciler are the backstop. + h.cpStopWithRetry(ctx, id, "SwitchProvider") + + // 2. Clear instance_id + write the new provider (jsonb_set preserves + // instance_type/volume/display/data_persistence) + go provisioning. + if _, err := db.DB.ExecContext(ctx, ` + UPDATE workspaces + SET instance_id = NULL, + compute = jsonb_set(COALESCE(compute, '{}'::jsonb), '{provider}', to_jsonb($2::text)), + status = $3, url = '', updated_at = now() + WHERE id = $1`, id, newProvider, models.StatusProvisioning); err != nil { + log.Printf("SwitchProvider: failed to write new provider for %s: %v", id, err) + c.JSON(http.StatusInternalServerError, gin.H{"error": "failed to switch provider"}) + return + } + log.Printf("SwitchProvider: %s %s → %s (old box stopped, reprovisioning)", id, effectiveOld, newProvider) + h.broadcaster.RecordAndBroadcast(ctx, string(events.EventWorkspaceProvisioning), id, map[string]interface{}{ + "name": wsName, + "tier": tier, + "runtime": dbRuntime, + "provider_from": effectiveOld, + "provider_to": newProvider, + }) + + // 3. Provision the NEW box. withStoredCompute re-reads compute (now carrying + // the new provider) → provisionWorkspaceCP routes to the new backend. + // Reuse the existing config volume (templatePath="") so identity/config + // are preserved. Detached context: the reprovision outlives the request. + payload := withStoredCompute(context.Background(), id, models.CreateWorkspacePayload{Name: wsName, Tier: tier, Runtime: dbRuntime}) + h.goAsync(func() { h.provisionWorkspaceCP(id, "", nil, payload) }) + + c.JSON(http.StatusAccepted, gin.H{ + "status": "switching", + "workspace_id": id, + "from": effectiveOld, + "to": newProvider, + }) +} diff --git a/workspace-server/internal/handlers/workspace_switch_provider_test.go b/workspace-server/internal/handlers/workspace_switch_provider_test.go new file mode 100644 index 000000000..4e939e49a --- /dev/null +++ b/workspace-server/internal/handlers/workspace_switch_provider_test.go @@ -0,0 +1,81 @@ +package handlers + +import ( + "bytes" + "net/http" + "net/http/httptest" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/gin-gonic/gin" +) + +// TestSwitchProvider_StopBeforeProviderWrite is the load-bearing ordering pin +// (RFC #622 Hazard 1). The stop (cpStopWithRetry) MUST appear before the UPDATE +// that writes the new provider — otherwise the stop resolves the new provider +// and deprovisions the old box against the wrong backend, leaking it. A +// source-level position check guards against a refactor reordering the two. +func TestSwitchProvider_StopBeforeProviderWrite(t *testing.T) { + wd, _ := os.Getwd() + src, err := os.ReadFile(filepath.Join(wd, "workspace_switch_provider.go")) + if err != nil { + t.Fatalf("read source: %v", err) + } + stripped := stripGoComments(src) + stopIdx := bytes.Index(stripped, []byte("cpStopWithRetry(ctx, id, \"SwitchProvider\")")) + if stopIdx < 0 { + t.Fatal("SwitchProvider must stop the old box via cpStopWithRetry before reprovisioning") + } + // the provider write is the jsonb_set on compute -> {provider} + writeIdx := bytes.Index(stripped, []byte("'{provider}'")) + if writeIdx < 0 { + t.Fatal("SwitchProvider must write the new provider via jsonb_set on compute->{provider}") + } + if stopIdx >= writeIdx { + t.Fatalf("ORDERING HAZARD: cpStopWithRetry (idx %d) must come BEFORE the provider write (idx %d) — else the old box is deprovisioned with the new backend and leaks", stopIdx, writeIdx) + } + // and the instance_id must be cleared in the same UPDATE (retry-safety) + if !bytes.Contains(stripped, []byte("instance_id = NULL")) { + t.Fatal("SwitchProvider must clear instance_id when writing the new provider (retry-safety)") + } +} + +// TestSwitchProvider_RejectsBadProvider: the allowlist check fires before any DB +// access, so a bad/missing provider is a clean 400 without touching the backend. +func TestSwitchProvider_RejectsBadProvider(t *testing.T) { + gin.SetMode(gin.TestMode) + h := &WorkspaceHandler{} + for _, tc := range []struct { + body string + want int + }{ + {`{"provider":"azure"}`, http.StatusBadRequest}, + {`{"provider":""}`, http.StatusBadRequest}, + {`{"provider":"AWS-typo"}`, http.StatusBadRequest}, + {`{}`, http.StatusBadRequest}, + } { + w := httptest.NewRecorder() + c, _ := gin.CreateTestContext(w) + c.Params = gin.Params{{Key: "id", Value: "ws-1"}} + c.Request = httptest.NewRequest("POST", "/workspaces/ws-1/switch-provider", strings.NewReader(tc.body)) + c.Request.Header.Set("Content-Type", "application/json") + h.SwitchProvider(c) + if w.Code != tc.want { + t.Errorf("body %s: got %d want %d (%s)", tc.body, w.Code, tc.want, w.Body.String()) + } + } +} + +// TestSwitchProvider_RouteRegistered pins the route wiring. +func TestSwitchProvider_RouteRegistered(t *testing.T) { + wd, _ := os.Getwd() + src, err := os.ReadFile(filepath.Join(wd, "..", "router", "router.go")) + if err != nil { + t.Fatalf("read router: %v", err) + } + if !bytes.Contains(src, []byte(`POST("/switch-provider", wh.SwitchProvider)`)) { + t.Fatal("router must register POST /switch-provider → wh.SwitchProvider") + } +} diff --git a/workspace-server/internal/router/router.go b/workspace-server/internal/router/router.go index fb1cb36f8..e9b6a80e0 100644 --- a/workspace-server/internal/router/router.go +++ b/workspace-server/internal/router/router.go @@ -229,6 +229,7 @@ func Setup(hub *ws.Hub, broadcaster *events.Broadcaster, prov *provisioner.Provi // Lifecycle wsAuth.GET("/state", wh.State) wsAuth.POST("/restart", wh.Restart) + wsAuth.POST("/switch-provider", wh.SwitchProvider) wsAuth.POST("/pause", wh.Pause) wsAuth.POST("/resume", wh.Resume) // Manual hibernate (opt-in, #711) — stops the container and sets status -- 2.52.0 From f38dc96f96e924e82b9bee0532ac6c5f4717aac8 Mon Sep 17 00:00:00 2001 From: Hongming Wang Date: Sun, 7 Jun 2026 22:57:53 -0700 Subject: [PATCH 2/3] harden(switch-provider): CAS guard + stop-exhaustion audit (#2422 review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the two NEEDS-CHANGES from the correctness review: - Concurrency (review #4): the provider-write UPDATE is now an atomic CAS (WHERE status <> 'provisioning' AND provider unchanged); RowsAffected==0 → 409 ALREADY_SWITCHING, so two concurrent switches can't both launch a provision and orphan a box. - Stop-exhaustion leak (review #3): switch to cpStopWithRetryErr, capture the old instance_id from the row before nulling it, and on stop failure emit a durable structure_events row (workspace.provider.switch_stop_exhausted) carrying {old_instance_id, old_provider} so the CP orphan reconciler can terminate the leaked box — the un-pointed orphan the status='removed' sweeper can't catch. Mirrors emitDeleteTerminateRetryExhausted. Tests pin both (CAS clauses + 409 + cpStopWithRetryErr + audit emit). Co-Authored-By: Claude Opus 4.8 (1M context) --- .../handlers/workspace_switch_provider.go | 81 ++++++++++++++++--- .../workspace_switch_provider_test.go | 30 ++++++- 2 files changed, 98 insertions(+), 13 deletions(-) diff --git a/workspace-server/internal/handlers/workspace_switch_provider.go b/workspace-server/internal/handlers/workspace_switch_provider.go index be37a379c..f10f86838 100644 --- a/workspace-server/internal/handlers/workspace_switch_provider.go +++ b/workspace-server/internal/handlers/workspace_switch_provider.go @@ -3,6 +3,7 @@ package handlers import ( "context" "database/sql" + "encoding/json" "io" "log" "net/http" @@ -61,12 +62,14 @@ func (h *WorkspaceHandler) SwitchProvider(c *gin.Context) { } var status, wsName, dbRuntime, oldProvider, dataPersistence string + var oldInstanceID sql.NullString var tier int err := db.DB.QueryRowContext(ctx, ` SELECT status, name, tier, COALESCE(runtime, 'claude-code'), - COALESCE(compute->>'provider', ''), COALESCE(compute->>'data_persistence', '') + COALESCE(compute->>'provider', ''), COALESCE(compute->>'data_persistence', ''), + instance_id FROM workspaces WHERE id = $1`, id, - ).Scan(&status, &wsName, &tier, &dbRuntime, &oldProvider, &dataPersistence) + ).Scan(&status, &wsName, &tier, &dbRuntime, &oldProvider, &dataPersistence, &oldInstanceID) if err == sql.ErrNoRows || status == string(models.StatusRemoved) { c.JSON(http.StatusNotFound, gin.H{"error": "workspace not found"}) return @@ -116,24 +119,50 @@ func (h *WorkspaceHandler) SwitchProvider(c *gin.Context) { // --- ordered switch (see doc-comment) --- // 1. Stop the OLD box with the OLD provider. DB is unchanged here, so - // cpStopWithRetry resolves the old provider + old instance_id. Synchronous - // (bounded retry); proceeds even on exhaustion so a stuck old box never - // strands the switch — the audit log + reconciler are the backstop. - h.cpStopWithRetry(ctx, id, "SwitchProvider") + // cpStopWithRetryErr resolves the old provider + old instance_id. Bounded + // retry; on exhaustion it returns an error but we STILL proceed (a stuck + // old box must not strand the switch) — except we capture the failure so + // step 2.5 can emit a durable audit row, because step 2 nulls instance_id + // and flips provider, which otherwise orphans the old box with no DB + // pointer for the sweeper to find (review finding #3). + stopErr := h.cpStopWithRetryErr(ctx, id, "SwitchProvider", false) - // 2. Clear instance_id + write the new provider (jsonb_set preserves - // instance_type/volume/display/data_persistence) + go provisioning. - if _, err := db.DB.ExecContext(ctx, ` + // 2. Atomically claim the switch AND clear instance_id + write the new + // provider. The CAS (status not already provisioning, provider still the + // one we read) makes concurrent/duplicate switch calls safe: only the + // first winner launches a provision; a racing call sees 0 rows → 409, + // never a second provision against a second backend (review finding #4). + // jsonb_set preserves instance_type/volume/display/data_persistence. + res, err := db.DB.ExecContext(ctx, ` UPDATE workspaces SET instance_id = NULL, compute = jsonb_set(COALESCE(compute, '{}'::jsonb), '{provider}', to_jsonb($2::text)), status = $3, url = '', updated_at = now() - WHERE id = $1`, id, newProvider, models.StatusProvisioning); err != nil { + WHERE id = $1 + AND status <> $3 + AND COALESCE(compute->>'provider', '') IS NOT DISTINCT FROM $4`, + id, newProvider, models.StatusProvisioning, oldProvider) + if err != nil { log.Printf("SwitchProvider: failed to write new provider for %s: %v", id, err) c.JSON(http.StatusInternalServerError, gin.H{"error": "failed to switch provider"}) return } - log.Printf("SwitchProvider: %s %s → %s (old box stopped, reprovisioning)", id, effectiveOld, newProvider) + if n, _ := res.RowsAffected(); n == 0 { + // Lost the CAS: another switch already flipped the provider / set + // provisioning, or the row changed under us. Do NOT launch a second + // provision (would orphan a box). + c.JSON(http.StatusConflict, gin.H{"error": "ALREADY_SWITCHING", "detail": "a provider switch or provision is already in progress for this workspace"}) + return + } + + // 2.5. If the old box never confirmed stopped, the old instance is now + // orphaned with no DB pointer (we just nulled instance_id). Emit a + // durable audit row carrying the old instance_id + provider so a CP-side + // reconciler can find and terminate it (review finding #3). + if stopErr != nil && oldInstanceID.Valid && oldInstanceID.String != "" { + h.emitSwitchProviderStopExhausted(ctx, id, oldInstanceID.String, effectiveOld, newProvider, stopErr) + } + log.Printf("SwitchProvider: %s %s → %s (old box stop err=%v, reprovisioning)", id, effectiveOld, newProvider, stopErr) h.broadcaster.RecordAndBroadcast(ctx, string(events.EventWorkspaceProvisioning), id, map[string]interface{}{ "name": wsName, "tier": tier, @@ -156,3 +185,33 @@ func (h *WorkspaceHandler) SwitchProvider(c *gin.Context) { "to": newProvider, }) } + +// emitSwitchProviderStopExhausted records a durable audit row when a provider +// switch could not confirm the OLD box stopped before its instance_id was +// cleared from the row. Without it the old box is an un-pointed orphan that the +// status='removed' sweeper won't catch; a CP-side reconciler reads these rows by +// old_instance_id + old_provider to terminate the leaked box. Mirrors +// emitDeleteTerminateRetryExhausted (workspace_dispatchers.go). Best-effort. +func (h *WorkspaceHandler) emitSwitchProviderStopExhausted(ctx context.Context, workspaceID, oldInstanceID, oldProvider, newProvider string, cause error) { + if db.DB == nil { + return + } + payload, err := json.Marshal(map[string]any{ + "workspace_id": workspaceID, + "old_instance_id": oldInstanceID, + "old_provider": oldProvider, + "new_provider": newProvider, + "last_error": cause.Error(), + "recovery_path": "cp_orphan_reconciler", + }) + if err != nil { + log.Printf("emitSwitchProviderStopExhausted: marshal failed for %s: %v", workspaceID, err) + return + } + if _, err := db.DB.ExecContext(ctx, ` + INSERT INTO structure_events (event_type, workspace_id, payload, created_at) + VALUES ($1, $2, $3, now()) + `, "workspace.provider.switch_stop_exhausted", workspaceID, payload); err != nil { + log.Printf("emitSwitchProviderStopExhausted: insert failed for %s: %v", workspaceID, err) + } +} diff --git a/workspace-server/internal/handlers/workspace_switch_provider_test.go b/workspace-server/internal/handlers/workspace_switch_provider_test.go index 4e939e49a..35ea7db0a 100644 --- a/workspace-server/internal/handlers/workspace_switch_provider_test.go +++ b/workspace-server/internal/handlers/workspace_switch_provider_test.go @@ -24,9 +24,9 @@ func TestSwitchProvider_StopBeforeProviderWrite(t *testing.T) { t.Fatalf("read source: %v", err) } stripped := stripGoComments(src) - stopIdx := bytes.Index(stripped, []byte("cpStopWithRetry(ctx, id, \"SwitchProvider\")")) + stopIdx := bytes.Index(stripped, []byte("cpStopWithRetryErr(ctx, id, \"SwitchProvider\"")) if stopIdx < 0 { - t.Fatal("SwitchProvider must stop the old box via cpStopWithRetry before reprovisioning") + t.Fatal("SwitchProvider must stop the old box via cpStopWithRetryErr before reprovisioning") } // the provider write is the jsonb_set on compute -> {provider} writeIdx := bytes.Index(stripped, []byte("'{provider}'")) @@ -42,6 +42,32 @@ func TestSwitchProvider_StopBeforeProviderWrite(t *testing.T) { } } +// TestSwitchProvider_ConcurrencyGuardAndAudit pins the two hardening items from +// the #2422 correctness review: (a) the provider-write is an atomic CAS so two +// concurrent switches can't both launch a provision (orphan), and (b) a +// stop-exhaustion emits a durable audit row carrying the old instance_id+provider +// (else the old box orphans with no DB pointer once instance_id is nulled). +func TestSwitchProvider_ConcurrencyGuardAndAudit(t *testing.T) { + wd, _ := os.Getwd() + src, err := os.ReadFile(filepath.Join(wd, "workspace_switch_provider.go")) + if err != nil { + t.Fatalf("read source: %v", err) + } + s := stripGoComments(src) + if !bytes.Contains(s, []byte("status <> $3")) || !bytes.Contains(s, []byte("IS NOT DISTINCT FROM $4")) { + t.Error("the provider-write UPDATE must be a CAS (status not already provisioning AND provider unchanged) to prevent a double-provision race") + } + if !bytes.Contains(s, []byte("RowsAffected")) || !bytes.Contains(s, []byte("ALREADY_SWITCHING")) { + t.Error("SwitchProvider must 409 ALREADY_SWITCHING when the CAS affects 0 rows (lost the race)") + } + if !bytes.Contains(s, []byte("cpStopWithRetryErr")) { + t.Error("SwitchProvider must use cpStopWithRetryErr to detect stop exhaustion") + } + if !bytes.Contains(s, []byte("emitSwitchProviderStopExhausted")) { + t.Error("SwitchProvider must emit an audit row with old instance_id+provider on stop exhaustion") + } +} + // TestSwitchProvider_RejectsBadProvider: the allowlist check fires before any DB // access, so a bad/missing provider is a clean 400 without touching the backend. func TestSwitchProvider_RejectsBadProvider(t *testing.T) { -- 2.52.0 From 4d7256c6e3d7a9e796aacea50b6e94454734e076 Mon Sep 17 00:00:00 2001 From: "Molecule AI Dev Engineer A (Kimi)" Date: Fri, 12 Jun 2026 04:57:59 +0000 Subject: [PATCH 3/3] chore(switch-provider): sanitize comments/test prose for content security (#2422) Addresses agent-reviewer content-security feedback: - Generalize comments describing teardown ordering without exposing query shapes, backend routing, or lifecycle sweeper details. - Replace 'orphan' / 'no DB pointer' / 'CP-side reconciler' language with neutral lifecycle-cleanup wording. - Rename emitted recovery_path marker from cp_orphan_reconciler to platform_cleanup. Functional identifiers (function names, table names, SQL) remain unchanged. Refs molecule-core#2422. Co-Authored-By: Claude --- .../handlers/workspace_switch_provider.go | 46 +++++++++---------- .../workspace_switch_provider_test.go | 21 ++++----- 2 files changed, 31 insertions(+), 36 deletions(-) diff --git a/workspace-server/internal/handlers/workspace_switch_provider.go b/workspace-server/internal/handlers/workspace_switch_provider.go index f10f86838..991d46219 100644 --- a/workspace-server/internal/handlers/workspace_switch_provider.go +++ b/workspace-server/internal/handlers/workspace_switch_provider.go @@ -21,20 +21,17 @@ import ( // (agent_card, config, secrets, tokens, memory) lives in the tenant DB and is // preserved because the row is never deleted. // -// CRITICAL ORDERING (the wrong-backend leak, RFC #622 Hazard 1): the stop must -// run with the OLD provider BEFORE the DB provider is changed. cpStopWithRetry -// resolves provider + instance_id from the workspaces row at call time; if we -// wrote the new provider first, the stop would issue -// DELETE …?instance_id=&provider= → CP routes teardown to the NEW -// backend → the old box is never terminated and leaks (billed forever, and not -// covered by the status='removed' orphan sweeper). So the sequence is strictly: +// CRITICAL ORDERING: the stop must run with the OLD provider BEFORE the DB +// provider is changed. The stop helper reads the current row at call time; if we +// wrote the new provider first, the teardown request would target the wrong +// backend and the old box would leak. So the sequence is strictly: // // 1. stop OLD box (DB still has old provider + old instance_id) // 2. clear instance_id + write new provider (jsonb_set, preserving the rest) // 3. provision NEW box (withStoredCompute now reads the new provider) // // Clearing instance_id in step 2 also makes a retried switch safe: a second call -// finds no stale instance to (mis-)deprovision against the new backend. +// finds no stale instance to act on with the new provider metadata. func (h *WorkspaceHandler) SwitchProvider(c *gin.Context) { id := c.Param("id") ctx := c.Request.Context() @@ -118,13 +115,13 @@ func (h *WorkspaceHandler) SwitchProvider(c *gin.Context) { // --- ordered switch (see doc-comment) --- - // 1. Stop the OLD box with the OLD provider. DB is unchanged here, so - // cpStopWithRetryErr resolves the old provider + old instance_id. Bounded - // retry; on exhaustion it returns an error but we STILL proceed (a stuck - // old box must not strand the switch) — except we capture the failure so - // step 2.5 can emit a durable audit row, because step 2 nulls instance_id - // and flips provider, which otherwise orphans the old box with no DB - // pointer for the sweeper to find (review finding #3). + // 1. Stop the OLD box with the OLD provider. DB is unchanged here, so the + // stop helper reads the old provider + old instance_id. Bounded retry; on + // exhaustion it returns an error but we STILL proceed (a stuck old box + // must not strand the switch) — except we capture the failure so step 2.5 + // can emit a durable audit row, because step 2 nulls instance_id and flips + // provider, which otherwise leaves the old box untracked by normal + // lifecycle cleanup (review finding #3). stopErr := h.cpStopWithRetryErr(ctx, id, "SwitchProvider", false) // 2. Atomically claim the switch AND clear instance_id + write the new @@ -150,15 +147,15 @@ func (h *WorkspaceHandler) SwitchProvider(c *gin.Context) { if n, _ := res.RowsAffected(); n == 0 { // Lost the CAS: another switch already flipped the provider / set // provisioning, or the row changed under us. Do NOT launch a second - // provision (would orphan a box). + // provision (would leave an untracked box). c.JSON(http.StatusConflict, gin.H{"error": "ALREADY_SWITCHING", "detail": "a provider switch or provision is already in progress for this workspace"}) return } - // 2.5. If the old box never confirmed stopped, the old instance is now - // orphaned with no DB pointer (we just nulled instance_id). Emit a - // durable audit row carrying the old instance_id + provider so a CP-side - // reconciler can find and terminate it (review finding #3). + // 2.5. If the old box never confirmed stopped, it may not be tracked by the + // normal lifecycle cleanup after instance_id was nulled. Emit a durable + // audit row carrying the old instance_id + provider so a platform cleanup + // process can locate and terminate it (review finding #3). if stopErr != nil && oldInstanceID.Valid && oldInstanceID.String != "" { h.emitSwitchProviderStopExhausted(ctx, id, oldInstanceID.String, effectiveOld, newProvider, stopErr) } @@ -188,10 +185,9 @@ func (h *WorkspaceHandler) SwitchProvider(c *gin.Context) { // emitSwitchProviderStopExhausted records a durable audit row when a provider // switch could not confirm the OLD box stopped before its instance_id was -// cleared from the row. Without it the old box is an un-pointed orphan that the -// status='removed' sweeper won't catch; a CP-side reconciler reads these rows by -// old_instance_id + old_provider to terminate the leaked box. Mirrors -// emitDeleteTerminateRetryExhausted (workspace_dispatchers.go). Best-effort. +// cleared from the row. Without it the old box may not be tracked by normal +// lifecycle cleanup; a platform cleanup process reads these rows by +// old_instance_id + old_provider to terminate the leaked box. Best-effort. func (h *WorkspaceHandler) emitSwitchProviderStopExhausted(ctx context.Context, workspaceID, oldInstanceID, oldProvider, newProvider string, cause error) { if db.DB == nil { return @@ -202,7 +198,7 @@ func (h *WorkspaceHandler) emitSwitchProviderStopExhausted(ctx context.Context, "old_provider": oldProvider, "new_provider": newProvider, "last_error": cause.Error(), - "recovery_path": "cp_orphan_reconciler", + "recovery_path": "platform_cleanup", }) if err != nil { log.Printf("emitSwitchProviderStopExhausted: marshal failed for %s: %v", workspaceID, err) diff --git a/workspace-server/internal/handlers/workspace_switch_provider_test.go b/workspace-server/internal/handlers/workspace_switch_provider_test.go index 35ea7db0a..73f0d6fce 100644 --- a/workspace-server/internal/handlers/workspace_switch_provider_test.go +++ b/workspace-server/internal/handlers/workspace_switch_provider_test.go @@ -12,11 +12,10 @@ import ( "github.com/gin-gonic/gin" ) -// TestSwitchProvider_StopBeforeProviderWrite is the load-bearing ordering pin -// (RFC #622 Hazard 1). The stop (cpStopWithRetry) MUST appear before the UPDATE -// that writes the new provider — otherwise the stop resolves the new provider -// and deprovisions the old box against the wrong backend, leaking it. A -// source-level position check guards against a refactor reordering the two. +// TestSwitchProvider_StopBeforeProviderWrite is the load-bearing ordering pin. +// The stop helper MUST appear before the UPDATE that writes the new provider — +// otherwise the teardown uses the wrong backend metadata and the old box leaks. +// A source-level position check guards against a refactor reordering the two. func TestSwitchProvider_StopBeforeProviderWrite(t *testing.T) { wd, _ := os.Getwd() src, err := os.ReadFile(filepath.Join(wd, "workspace_switch_provider.go")) @@ -34,7 +33,7 @@ func TestSwitchProvider_StopBeforeProviderWrite(t *testing.T) { t.Fatal("SwitchProvider must write the new provider via jsonb_set on compute->{provider}") } if stopIdx >= writeIdx { - t.Fatalf("ORDERING HAZARD: cpStopWithRetry (idx %d) must come BEFORE the provider write (idx %d) — else the old box is deprovisioned with the new backend and leaks", stopIdx, writeIdx) + t.Fatalf("ORDERING HAZARD: stop helper (idx %d) must come BEFORE the provider write (idx %d) — else the old box is torn down with wrong backend metadata and leaks", stopIdx, writeIdx) } // and the instance_id must be cleared in the same UPDATE (retry-safety) if !bytes.Contains(stripped, []byte("instance_id = NULL")) { @@ -43,10 +42,10 @@ func TestSwitchProvider_StopBeforeProviderWrite(t *testing.T) { } // TestSwitchProvider_ConcurrencyGuardAndAudit pins the two hardening items from -// the #2422 correctness review: (a) the provider-write is an atomic CAS so two -// concurrent switches can't both launch a provision (orphan), and (b) a -// stop-exhaustion emits a durable audit row carrying the old instance_id+provider -// (else the old box orphans with no DB pointer once instance_id is nulled). +// the correctness review: (a) the provider-write is an atomic CAS so two +// concurrent switches can't both launch a provision, and (b) stop-exhaustion +// emits a durable audit row carrying the old instance_id+provider so the old box +// remains discoverable after instance_id is nulled. func TestSwitchProvider_ConcurrencyGuardAndAudit(t *testing.T) { wd, _ := os.Getwd() src, err := os.ReadFile(filepath.Join(wd, "workspace_switch_provider.go")) @@ -64,7 +63,7 @@ func TestSwitchProvider_ConcurrencyGuardAndAudit(t *testing.T) { t.Error("SwitchProvider must use cpStopWithRetryErr to detect stop exhaustion") } if !bytes.Contains(s, []byte("emitSwitchProviderStopExhausted")) { - t.Error("SwitchProvider must emit an audit row with old instance_id+provider on stop exhaustion") + t.Error("SwitchProvider must emit an audit row with old instance/provider metadata on stop exhaustion") } } -- 2.52.0