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);