From bdfc93b32922d7363d62cee91ce8f0fade1b0fe4 Mon Sep 17 00:00:00 2001 From: pythonlearner1025 Date: Fri, 28 Aug 2026 04:19:44 +0000 Subject: [PATCH] fix(workspaces): stop a destroy that succeeded from answering 500 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A destroy returned 500 while the server was deleted, the row reached destroyed, and no error was recorded anywhere. Two contract violations between them account for exactly that. WorkspaceTunnels.cleanup is documented as "callers log the returned errors", and it wraps a client whose own cleanup "never throws". Its D1 write was outside that contract: a transient D1 failure threw out of cleanup. Destroy calls cleanup AFTER vmProvider.destroy has already deleted the server, so that throw reached the router's onError as an opaque 500 for work that had irreversibly half-succeeded. The row stayed in destroying, the orphan sweep found the server already gone, skipped its own destroy, and transitioned the row to destroyed with error = NULL. Server deleted, row destroyed, nothing recorded, 500 to the caller. cleanup now pushes that failure into result.errors. Not clearing the columns is the safe half: both Cloudflare deletes tolerate an already-deleted resource, so the janitor's retry is a no-op and clears them then. Destroy's existing "honest destroy" branch handles it — 200, phase destroying, janitor finishes. The caller also dropped cleanup.errors on the floor, which is the "no error recorded" half. It now reports them, because the janitor's transition sets error back to NULL and these errors are the only account of why a destroy needed two attempts. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01J6fUBY1B27EzvDwbhfBf52 --- .../control-plane/core/workspace-tunnels.ts | 27 ++++++++++---- packages/control-plane/core/workspaces.ts | 9 +++++ .../test/workspace-tunnels.test.ts | 37 +++++++++++++++++++ 3 files changed, 66 insertions(+), 7 deletions(-) diff --git a/packages/control-plane/core/workspace-tunnels.ts b/packages/control-plane/core/workspace-tunnels.ts index 9d74de97..d74f4bb6 100644 --- a/packages/control-plane/core/workspace-tunnels.ts +++ b/packages/control-plane/core/workspace-tunnels.ts @@ -85,8 +85,9 @@ export class WorkspaceTunnels { } /** Deletes tunnel resources, clearing each column only after its - * confirmed deletion. Callers log the returned errors; anything left - * behind stays on the row for the janitor to retry. */ + * confirmed deletion. Never throws, like the client it wraps: callers log + * the returned errors, and anything left behind stays on the row for the + * janitor to retry. */ async cleanup(db: Db, row: WorkspaceTunnelRow): Promise { if (row.tunnel_id === null && row.dns_record_id === null) { return { dnsDeleted: true, tunnelDeleted: true, errors: [] }; @@ -101,11 +102,23 @@ export class WorkspaceTunnels { assignments.push("tunnel_id = NULL", "tunnel_hostname = NULL"); } if (assignments.length > 0) { - await rows(db, { - q: `UPDATE workspaces SET ${assignments.join(", ")}, updated_at = ?1 - WHERE id = ?2`, - v: [Date.now(), row.id], - }); + try { + await rows(db, { + q: `UPDATE workspaces SET ${assignments.join(", ")}, updated_at = ?1 + WHERE id = ?2`, + v: [Date.now(), row.id], + }); + } catch (error) { + // The client never throws and neither may this, because destroy calls + // cleanup AFTER the VM is gone: a throw here answered 500 for work + // that had already half-succeeded irreversibly, and left the caller + // with no way to tell that from a destroy which did nothing. + // + // Not clearing the columns is the safe half of this failure. Both + // deletes tolerate an already-deleted resource, so the janitor's retry + // is a no-op against Cloudflare and clears the columns then. + result.errors.push(error instanceof Error ? error.message : String(error)); + } } return result; } diff --git a/packages/control-plane/core/workspaces.ts b/packages/control-plane/core/workspaces.ts index f5d79216..c612cd84 100644 --- a/packages/control-plane/core/workspaces.ts +++ b/packages/control-plane/core/workspaces.ts @@ -1179,6 +1179,15 @@ export function addWorkspaceRoutes( if (cleanup.errors.length > 0) { // Honest destroy: the row stays in destroying with its remaining // identifiers; the janitor retries until Cloudflare cleanup lands. + // + // Report them. The caller gets a 200 and the janitor's own transition + // sets `error` back to NULL, so these errors are the only account of + // why a destroy needed two attempts, and dropping them is what made + // the first such destroy unexplainable. + runtime.reportError( + "workspace_destroy_cleanup_incomplete", + new Error(`workspace ${id}: ${cleanup.errors.join("; ")}`), + ); const pending = await workspaceById(runtime.db, id); if (pending === null) throw new Error("workspace disappeared during destroy"); return context.json({ diff --git a/packages/control-plane/test/workspace-tunnels.test.ts b/packages/control-plane/test/workspace-tunnels.test.ts index 0b0fe233..7f808541 100644 --- a/packages/control-plane/test/workspace-tunnels.test.ts +++ b/packages/control-plane/test/workspace-tunnels.test.ts @@ -1,7 +1,9 @@ import { describe, expect, it } from "vitest"; import { rows } from "../core/db.js"; +import type { Db } from "../core/db.js"; import { CloudflareTunnels } from "../core/compute/cloudflare-tunnels.js"; import { WorkspaceTunnels } from "../core/workspace-tunnels.js"; +import type { WorkspaceTunnelRow } from "../core/workspace-tunnels.js"; import { WorkspaceWebAppAuth } from "../core/webapp-tickets.js"; import type { WorkspaceRow } from "../core/workspaces.js"; import type { WorkspaceView } from "../core/wire.js"; @@ -90,6 +92,41 @@ describe("workspace tunnels", () => { expect(row?.dns_record_id).toBeNull(); }); + it("reports a failed column clear rather than throwing it at the caller", async () => { + // Destroy calls cleanup AFTER the VM is already deleted. A throw here + // answered an opaque 500 for a destroy that had irreversibly succeeded: + // the server was gone, the janitor finished the row and cleared `error` + // to NULL, and nothing was left to explain the 500 to whoever saw it. + const workspaceTunnels = new WorkspaceTunnels( + new CloudflareTunnels({ + accountId: "test-account", + zoneId: "test-zone-id", + apiToken: "test-api-token", + fetcher: async () => Response.json({ success: true, result: {} }), + }), + "webapp.test", + "test-webapp-root-secret", + async () => Response.json({ ok: true }), + ); + const unreachable = new Error("D1_ERROR: Network connection lost"); + const failingDb: Db = { + rawSQL: () => ({ run: async () => { throw unreachable; } }), + rawSQLTransaction: () => ({ run: async () => { throw unreachable; } }), + }; + + const result = await workspaceTunnels.cleanup(failingDb, { + id: "ws-1", + tunnel_id: "tun-1", + tunnel_hostname: "ws-1.webapp.test", + dns_record_id: "dns-1", + } satisfies WorkspaceTunnelRow); + + // Cloudflare did its half; only the column clear failed. + expect(result.tunnelDeleted).toBe(true); + expect(result.dnsDeleted).toBe(true); + expect(result.errors).toEqual(["D1_ERROR: Network connection lost"]); + }); + it("stays inert without configuration", async () => { const providers = new FakeProviders(); const app = appWithVmProviders([providers], providers);