From 909999a84ecef0843f1b5c4efa854cc3f4b7cff0 Mon Sep 17 00:00:00 2001 From: Bryan Maass Date: Mon, 6 Jul 2026 15:04:29 -0600 Subject: [PATCH] =?UTF-8?q?feat(workspace):=20destroy=20safety=20=E2=80=94?= =?UTF-8?q?=20is-dirty=20check,=20auto-export,=20--discard?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Destroy is the only irreversible moment, so it closes the work-loss window there: when a ws- profile exists locally, the child is checked for unsynced work and a dirty workspace is auto-exported to its target branch before teardown. --discard skips both. No local profile warns and proceeds (destroy is the billing-stop lever; ops cleanup must work from any machine); an unreachable child refuses without --discard rather than silently reopening the window. The matching profile is dropped after a successful destroy. Refs GHY-4051 --- README.md | 6 +- src/commands/git-sync/export.ts | 2 +- src/commands/workspace/destroy.ts | 77 ++++++++++++++++++++- src/commands/workspace/profile-name.ts | 3 + tests/e2e/workspace.e2e.test.ts | 92 +++++++++++++++++++++++++- 5 files changed, 172 insertions(+), 8 deletions(-) create mode 100644 src/commands/workspace/profile-name.ts diff --git a/README.md b/README.md index d66dee4..4162e6d 100644 --- a/README.md +++ b/README.md @@ -1528,16 +1528,20 @@ Before creating, the profile store is swept for broader credentials against the ### `mb workspace destroy ` -Tears down each provisioned database's warehouse isolation, then removes the workspace. Prompts unless `--yes`. Refuses (409) while any database is still provisioning/deprovisioning unless `--ignore-pending`, which removes the records and leaves those warehouse objects behind; unreachable-warehouse leftovers are reported as `orphaned_resources`. +Destroy is the only irreversible moment, so it closes the work-loss window first: when a local profile named `ws-` exists, the child is checked for unsynced work (`remote-sync is-dirty`) and a dirty workspace is auto-exported to its target branch before teardown. `--discard` skips the check and the export; a machine without the workspace's profile warns and proceeds (destroy is the billing-stop lever — ops cleanup must work from anywhere), while a profile whose child can't be reached refuses without `--discard`. + +Then each provisioned database's warehouse isolation is torn down and the workspace removed; the matching local profile is dropped on success. Prompts unless `--yes`. Refuses (409) while any database is still provisioning/deprovisioning unless `--ignore-pending`, which removes the records and leaves those warehouse objects behind; unreachable-warehouse leftovers are reported as `orphaned_resources`. ```sh mb workspace destroy 1 --yes +mb workspace destroy 1 --yes --discard mb workspace destroy 1 --yes --ignore-pending ``` | Flag | Description | | ------------------ | ----------------------------------------------------------------------- | | `--yes` | Skip confirmation. | +| `--discard` | Destroy without exporting unsynced work (skips the is-dirty check). | | `--ignore-pending` | Remove workspace records even while databases are mid-(de)provisioning. | ## Instance setup diff --git a/src/commands/git-sync/export.ts b/src/commands/git-sync/export.ts index 612a054..a8b9778 100644 --- a/src/commands/git-sync/export.ts +++ b/src/commands/git-sync/export.ts @@ -11,7 +11,7 @@ import { gitSyncWaitFlags, parseWaitFlags } from "../wait-flags"; import { formatSyncTask, pollSyncTask, REMOTE_SYNC_PATHS, throwIfFailedTask } from "./poll-task"; -const SyncExportKickoff = z.object({ +export const SyncExportKickoff = z.object({ message: z.string(), task_id: z.number().int().positive(), }); diff --git a/src/commands/workspace/destroy.ts b/src/commands/workspace/destroy.ts index 14e2218..d4c47fc 100644 --- a/src/commands/workspace/destroy.ts +++ b/src/commands/workspace/destroy.ts @@ -1,13 +1,22 @@ import { z } from "zod"; -import { ConfigError } from "../../core/errors"; +import { clearProfile, readProfileCredential } from "../../core/auth/storage"; +import { ConfigError, errorMessage } from "../../core/errors"; +import { createClient, type Client } from "../../core/http/client"; import type { ResourceView } from "../../domain/view"; +import { warn } from "../../output/notice"; import { promptConfirm } from "../../output/prompt"; import { renderSummary } from "../../output/render"; +import { DEFAULT_INTERVAL_MS, DEFAULT_TIMEOUT_MS } from "../../runtime/poll"; import { connectionFlags, outputFlags, profileFlag } from "../flags"; +import { SyncExportKickoff } from "../git-sync/export"; +import { IsDirtyResult } from "../git-sync/is-dirty"; +import { pollSyncTask, REMOTE_SYNC_PATHS, throwIfFailedTask } from "../git-sync/poll-task"; import { parseId } from "../parse-id"; import { defineMetabaseCommand } from "../runtime"; +import { workspaceProfileName } from "./profile-name"; + const WorkspaceOrphanedResource = z .object({ workspace_database_id: z.number().int(), @@ -46,13 +55,18 @@ const workspaceDestroyView: ResourceView = { export default defineMetabaseCommand({ meta: { name: "destroy", description: "Destroy a workspace and its warehouse isolation" }, details: - "Tears down each provisioned database's warehouse isolation (temporary schema + user) before removing the workspace. Refuses with a 409 while any database is still provisioning/deprovisioning unless --ignore-pending is passed, which removes the workspace records and leaves those warehouse objects behind. If the warehouse was unreachable, the result lists the leftover schema/user objects under orphaned_resources.", + "Destroy is the only irreversible moment, so it closes the work-loss window first: when a local profile named ws- exists, the child is checked for unsynced work and a dirty workspace is auto-exported to its target branch before anything is torn down (--discard skips the check and the export). Then each provisioned database's warehouse isolation (temporary schema + user) is dropped and the workspace removed; the matching local profile is dropped on success. Refuses with a 409 while any database is still provisioning/deprovisioning unless --ignore-pending is passed, which removes the workspace records and leaves those warehouse objects behind. If the warehouse was unreachable, the result lists the leftover schema/user objects under orphaned_resources.", capabilities: { minVersion: 62, tokenFeature: "workspaces" }, args: { ...outputFlags, ...profileFlag, ...connectionFlags, yes: { type: "boolean", description: "Skip confirmation", default: false }, + discard: { + type: "boolean", + description: "Destroy without exporting unsynced work (skips the is-dirty check)", + default: false, + }, "ignore-pending": { type: "boolean", description: "Remove workspace records even while databases are provisioning/deprovisioning", @@ -61,7 +75,11 @@ export default defineMetabaseCommand({ id: { type: "positional", description: "Workspace id", required: true }, }, outputSchema: WorkspaceDestroyResult, - examples: ["mb workspace destroy 1 --yes", "mb workspace destroy 1 --yes --ignore-pending"], + examples: [ + "mb workspace destroy 1 --yes", + "mb workspace destroy 1 --yes --discard", + "mb workspace destroy 1 --yes --ignore-pending", + ], async run({ args, ctx, getClient }) { const id = parseId(args.id); if (!args.yes) { @@ -85,6 +103,8 @@ export default defineMetabaseCommand({ } } const client = await getClient(); + const profileName = workspaceProfileName(id); + const hasProfile = await guardUnsyncedWork(profileName, args.discard); const response = await client.requestParsed( WorkspaceDeleteResponse, `/api/ee/workspace-manager/${id}`, @@ -93,6 +113,10 @@ export default defineMetabaseCommand({ query: { "ignore-pending": args["ignore-pending"] ? true : undefined }, }, ); + if (response.deleted && hasProfile) { + await clearProfile(profileName); + warn(`dropped profile "${profileName}"`); + } renderSummary( { ...response, aborted: false }, workspaceDestroyView, @@ -101,3 +125,50 @@ export default defineMetabaseCommand({ ); }, }); + +// The dirty check needs the child's credential, which only exists on machines that created or +// connected to this workspace. Without one the check is impossible — destroy must still work +// (it is the billing-stop lever), so warn and proceed rather than block ops cleanup. +async function guardUnsyncedWork(profileName: string, discard: boolean): Promise { + const resolved = await readProfileCredential(profileName); + if (resolved === null) { + warn(`no local profile "${profileName}" for this workspace — skipping the unsynced-work check`); + return false; + } + if (discard) { + warn("--discard given — skipping the unsynced-work check"); + return true; + } + const child = createClient({ url: resolved.url, credential: resolved.credential }); + const dirty = await checkIsDirty(child, profileName); + if (!dirty) { + return true; + } + warn("workspace has unsynced work — exporting to the target branch before destroy"); + await exportUnsyncedWork(child); + return true; +} + +async function checkIsDirty(child: Client, profileName: string): Promise { + try { + const result = await child.requestParsed(IsDirtyResult, REMOTE_SYNC_PATHS.isDirty); + return result.is_dirty; + } catch (error) { + throw new ConfigError( + `could not check workspace profile "${profileName}" for unsynced work: ${errorMessage(error)} — pass --discard to destroy anyway`, + ); + } +} + +async function exportUnsyncedWork(child: Client): Promise { + const kickoff = await child.requestParsed(SyncExportKickoff, REMOTE_SYNC_PATHS.export, { + method: "POST", + body: {}, + }); + const final = await pollSyncTask(child, { + intervalMs: DEFAULT_INTERVAL_MS, + timeoutMs: DEFAULT_TIMEOUT_MS, + }); + throwIfFailedTask(final, "export"); + warn(`export task #${kickoff.task_id} completed; unsynced work is saved`); +} diff --git a/src/commands/workspace/profile-name.ts b/src/commands/workspace/profile-name.ts new file mode 100644 index 0000000..8fe9b39 --- /dev/null +++ b/src/commands/workspace/profile-name.ts @@ -0,0 +1,3 @@ +export function workspaceProfileName(workspaceId: number): string { + return `ws-${workspaceId}`; +} diff --git a/tests/e2e/workspace.e2e.test.ts b/tests/e2e/workspace.e2e.test.ts index 26e3ddf..f46be29 100644 --- a/tests/e2e/workspace.e2e.test.ts +++ b/tests/e2e/workspace.e2e.test.ts @@ -1,5 +1,6 @@ import { afterEach, assert, beforeAll, describe, expect, it } from "vitest"; +import { AuthStatus } from "../../src/commands/auth/status"; import { WorkspaceDestroyResult } from "../../src/commands/workspace/destroy"; import { WorkspaceListEnvelope } from "../../src/commands/workspace/list"; import { createClient, type Client } from "../../src/core/http/client"; @@ -80,7 +81,7 @@ describe.skipIf(skipReason !== null)("workspace e2e", () => { }); } - async function createSeedWorkspace(): Promise { + async function createSeedWorkspace(configHome?: string): Promise { await enableWorkspacesOnWarehouse(); const result = await runCli({ args: [ @@ -92,7 +93,7 @@ describe.skipIf(skipReason !== null)("workspace e2e", () => { String(SEEDED.warehouseDbId), "--json", ], - configHome: await makeIsolatedConfigHome(), + configHome: configHome ?? (await makeIsolatedConfigHome()), env: authEnv(), timeoutMs: PROVISION_TIMEOUT_MS, }); @@ -130,7 +131,7 @@ describe.skipIf(skipReason !== null)("workspace e2e", () => { expect(parseJson(result.stdout, WorkspaceCompact)).toEqual(WORKSPACE_COMPACT); }); - it("destroy --yes tears down the workspace; subsequent get returns 404", async () => { + it("destroy --yes without a matching local profile warns, skips the dirty check, and tears down", async () => { await createSeedWorkspace(); const destroyResult = await runCli({ @@ -140,6 +141,9 @@ describe.skipIf(skipReason !== null)("workspace e2e", () => { timeoutMs: PROVISION_TIMEOUT_MS, }); expect(destroyResult.exitCode, destroyResult.stderr).toBe(0); + expect(destroyResult.stderr).toContain( + `no local profile "ws-${FIRST_WORKSPACE_ID}" for this workspace — skipping the unsynced-work check`, + ); expect(parseJson(destroyResult.stdout, WorkspaceDestroyResult)).toEqual({ id: FIRST_WORKSPACE_ID, deleted: true, @@ -171,6 +175,88 @@ describe.skipIf(skipReason !== null)("workspace e2e", () => { ); expect(result.stdout).toBe(""); }); + + // The ws- profile is seeded AFTER create: it targets the same host in this rig, so the + // stale-credential sweep would (correctly) refuse the create if it existed beforehand. + async function seedWorkspaceProfile(configHome: string, url: string): Promise { + const skipVerify = url === bootstrap.baseUrl ? [] : ["--skip-verify"]; + const login = await runCli({ + args: [ + "auth", + "login", + "--profile", + `ws-${FIRST_WORKSPACE_ID}`, + "--url", + url, + ...skipVerify, + "--json", + ], + configHome, + stdin: bootstrap.adminApiKey, + }); + expect(login.exitCode, login.stderr).toBe(0); + } + + it("destroy runs the dirty check through the ws- profile and drops it after teardown", async () => { + const configHome = await makeIsolatedConfigHome(); + await createSeedWorkspace(configHome); + await seedWorkspaceProfile(configHome, bootstrap.baseUrl); + + const destroyResult = await runCli({ + args: ["workspace", "destroy", String(FIRST_WORKSPACE_ID), "--yes", "--json"], + configHome, + env: authEnv(), + timeoutMs: PROVISION_TIMEOUT_MS, + }); + expect(destroyResult.exitCode, destroyResult.stderr).toBe(0); + expect(destroyResult.stderr).not.toContain("no local profile"); + expect(destroyResult.stderr).toContain(`dropped profile "ws-${FIRST_WORKSPACE_ID}"`); + expect(parseJson(destroyResult.stdout, WorkspaceDestroyResult)).toEqual({ + id: FIRST_WORKSPACE_ID, + deleted: true, + aborted: false, + }); + + const status = await runCli({ + args: ["auth", "status", "--profile", `ws-${FIRST_WORKSPACE_ID}`, "--json"], + configHome, + }); + expect(status.exitCode, status.stderr).toBe(0); + expect(parseJson(status.stdout, AuthStatus).present).toBe(false); + }); + + it("destroy refuses when the dirty check is impossible, and --discard overrides", async () => { + const configHome = await makeIsolatedConfigHome(); + await createSeedWorkspace(configHome); + await seedWorkspaceProfile(configHome, "https://unreachable.invalid"); + + const refused = await runCli({ + args: ["workspace", "destroy", String(FIRST_WORKSPACE_ID), "--yes", "--json"], + configHome, + env: authEnv(), + timeoutMs: PROVISION_TIMEOUT_MS, + }); + expect(refused.exitCode).toBe(2); + expect(cliErrorMessage(refused.stderr)).toContain( + `could not check workspace profile "ws-${FIRST_WORKSPACE_ID}" for unsynced work:`, + ); + expect(cliErrorMessage(refused.stderr)).toContain("— pass --discard to destroy anyway"); + expect(refused.stdout).toBe(""); + + const discarded = await runCli({ + args: ["workspace", "destroy", String(FIRST_WORKSPACE_ID), "--yes", "--discard", "--json"], + configHome, + env: authEnv(), + timeoutMs: PROVISION_TIMEOUT_MS, + }); + expect(discarded.exitCode, discarded.stderr).toBe(0); + expect(discarded.stderr).toContain("--discard given — skipping the unsynced-work check"); + expect(parseJson(discarded.stdout, WorkspaceDestroyResult)).toEqual({ + id: FIRST_WORKSPACE_ID, + deleted: true, + aborted: false, + }); + }); }); describe("workspace arg validation e2e (no Metabase contact required)", () => {