Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 20 additions & 7 deletions packages/control-plane/core/workspace-tunnels.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<SurfaceCleanupResult> {
if (row.tunnel_id === null && row.dns_record_id === null) {
return { dnsDeleted: true, tunnelDeleted: true, errors: [] };
Expand All @@ -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;
}
Expand Down
9 changes: 9 additions & 0 deletions packages/control-plane/core/workspaces.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<CreateWorkspaceResponse>({
Expand Down
37 changes: 37 additions & 0 deletions packages/control-plane/test/workspace-tunnels.test.ts
Original file line number Diff line number Diff line change
@@ -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";
Expand Down Expand Up @@ -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);
Expand Down
Loading