Skip to content

Commit 9dbfb77

Browse files
committed
feat(sandbox): readable container names + per-instance ownership
- containers are named superset-<workspace/branch-slug>-<short-id> so Docker Desktop shows which workspace each sandbox belongs to; the workspace-id label stays authoritative and destroy/cleanup resolve containers by label, healing renames and legacy names - new com.superset.home ownership label; the startup reconcile only sweeps containers created by THIS host instance. Fixes integration test runs (own temp DB, same docker daemon) deleting the dev app's live workspace containers as orphans
1 parent f353789 commit 9dbfb77

11 files changed

Lines changed: 172 additions & 15 deletions

File tree

packages/host-service/src/runtime/sandbox/container-manager.ts

Lines changed: 20 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ import {
1515
createContainer,
1616
imageExists,
1717
inspectContainer,
18+
listWorkspaceContainerNames,
1819
pullImage,
1920
removeContainer,
2021
startContainer,
@@ -26,6 +27,7 @@ import {
2627
CONTAINER_HOST_DIR,
2728
CONTAINER_SUPERSET_DIR,
2829
getSandboxContainerName,
30+
getSupersetHomeDir,
2931
getWorkspaceSandboxPaths,
3032
} from "./paths.ts";
3133
import { selectPublishablePorts } from "./port-probe.ts";
@@ -46,6 +48,8 @@ export interface EnsureContainerParams {
4648
worktreePath: string;
4749
repoPath: string;
4850
branch: string;
51+
/** Human-readable container-name slug (workspace name/branch). */
52+
nameSlug: string;
4953
settings: ResolvedSandboxSettings;
5054
}
5155

@@ -176,7 +180,7 @@ async function doEnsureContainer(params: EnsureContainerParams): Promise<void> {
176180
// Also re-registers the token in-memory after host-service restarts.
177181
const cliToken = await ensureCliTokenFile(params.workspaceId);
178182

179-
const name = getSandboxContainerName(params.workspaceId);
183+
const name = getSandboxContainerName(params.workspaceId, params.nameSlug);
180184
const configHash = computeConfigHash(params.settings);
181185
const inspection = await inspectContainer(name);
182186

@@ -219,11 +223,21 @@ async function doEnsureContainer(params: EnsureContainerParams): Promise<void> {
219223
}
220224
}
221225

226+
// A rename (workspace or branch) changes the display name; the old-named
227+
// container for this workspace would otherwise linger as a duplicate.
228+
const staleNames = (
229+
await listWorkspaceContainerNames(params.workspaceId)
230+
).filter((existing) => existing !== name);
231+
for (const stale of staleNames) {
232+
await removeContainer(stale);
233+
}
234+
222235
await createContainer(
223236
buildContainerCreateArgs({
224237
name,
225238
workspaceId: params.workspaceId,
226239
configHash,
240+
ownerHome: getSupersetHomeDir(),
227241
image: params.settings.image,
228242
runtime: params.settings.runtime,
229243
network: params.settings.network,
@@ -255,7 +269,11 @@ export async function destroyWorkspaceSandbox(
255269
provisioningStates.delete(workspaceId);
256270
const availability = await checkDockerAvailable();
257271
if (availability.ok) {
258-
await removeContainer(getSandboxContainerName(workspaceId));
272+
// By label, not by name: display names drift (slug/rename) and legacy
273+
// containers used the bare superset-ws-<id> form.
274+
for (const name of await listWorkspaceContainerNames(workspaceId)) {
275+
await removeContainer(name);
276+
}
259277
} else {
260278
console.warn(
261279
`[sandbox] docker unavailable during destroy of ${workspaceId}; ` +

packages/host-service/src/runtime/sandbox/docker-args.test.ts

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ describe("buildContainerCreateArgs", () => {
1414
name: "superset-ws-abc",
1515
workspaceId: "abc",
1616
configHash: "deadbeef",
17+
ownerHome: "/home/me/.superset",
1718
image: "img:1",
1819
runtime: "runsc",
1920
network: "none",
@@ -36,6 +37,8 @@ describe("buildContainerCreateArgs", () => {
3637
"com.superset.workspace-id=abc",
3738
"--label",
3839
"com.superset.config-hash=deadbeef",
40+
"--label",
41+
"com.superset.home=/home/me/.superset",
3942
"--restart",
4043
"unless-stopped",
4144
"--init",
@@ -68,6 +71,7 @@ describe("buildContainerCreateArgs", () => {
6871
name: "n",
6972
workspaceId: "w",
7073
configHash: "h",
74+
ownerHome: "/home/me/.superset",
7175
image: "img",
7276
network: "bridge",
7377
resources: { pidsLimit: 2048 },

packages/host-service/src/runtime/sandbox/docker-args.ts

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,11 +19,20 @@ export interface PublishedPort {
1919
export const MANAGED_LABEL = "com.superset.managed=true";
2020
export const WORKSPACE_ID_LABEL = "com.superset.workspace-id";
2121
export const CONFIG_HASH_LABEL = "com.superset.config-hash";
22+
/**
23+
* Which host-service instance owns the container, keyed by its superset home
24+
* dir. Reconcile only touches containers with ITS OWN home label — a test
25+
* host or a second dev instance must never sweep another instance's
26+
* containers as orphans.
27+
*/
28+
export const OWNER_HOME_LABEL = "com.superset.home";
2229

2330
export interface ContainerCreateSpec {
2431
name: string;
2532
workspaceId: string;
2633
configHash: string;
34+
/** Superset home dir of the owning host-service (OWNER_HOME_LABEL). */
35+
ownerHome: string;
2736
image: string;
2837
runtime?: string;
2938
network: "bridge" | "none";
@@ -59,6 +68,8 @@ export function buildContainerCreateArgs(spec: ContainerCreateSpec): string[] {
5968
`${WORKSPACE_ID_LABEL}=${spec.workspaceId}`,
6069
"--label",
6170
`${CONFIG_HASH_LABEL}=${spec.configHash}`,
71+
"--label",
72+
`${OWNER_HOME_LABEL}=${spec.ownerHome}`,
6273
"--restart",
6374
"unless-stopped",
6475
"--init",

packages/host-service/src/runtime/sandbox/docker-cli.ts

Lines changed: 26 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -178,10 +178,30 @@ export async function removeContainer(name: string): Promise<void> {
178178
export interface ManagedContainer {
179179
name: string;
180180
workspaceId: string | null;
181+
/** OWNER_HOME_LABEL value; null on containers from before the label. */
182+
ownerHome: string | null;
181183
running: boolean;
182184
}
183185

184186
/** All containers carrying the Superset managed label, running or not. */
187+
/**
188+
* All container names labeled with this workspace id — authoritative lookup
189+
* for destroy/cleanup, since display names can drift (rename, slug change).
190+
*/
191+
export async function listWorkspaceContainerNames(
192+
workspaceId: string,
193+
): Promise<string[]> {
194+
const out = await docker([
195+
"ps",
196+
"-a",
197+
"--filter",
198+
`label=com.superset.workspace-id=${workspaceId}`,
199+
"--format",
200+
"{{.Names}}",
201+
]);
202+
return out.split("\n").filter((name) => name.trim().length > 0);
203+
}
204+
185205
export async function listManagedContainers(): Promise<ManagedContainer[]> {
186206
const out = await docker([
187207
"ps",
@@ -200,13 +220,15 @@ export async function listManagedContainers(): Promise<ManagedContainer[]> {
200220
state: string;
201221
labels: string;
202222
};
203-
const workspaceLabel = parsed.labels
223+
const labelPairs = parsed.labels
204224
.split(",")
205-
.map((pair) => pair.split("="))
206-
.find(([key]) => key === "com.superset.workspace-id");
225+
.map((pair) => pair.split("="));
226+
const labelValue = (key: string) =>
227+
labelPairs.find(([k]) => k === key)?.[1] ?? null;
207228
containers.push({
208229
name: parsed.name,
209-
workspaceId: workspaceLabel?.[1] ?? null,
230+
workspaceId: labelValue("com.superset.workspace-id"),
231+
ownerHome: labelValue("com.superset.home"),
210232
running: parsed.state === "running",
211233
});
212234
} catch {

packages/host-service/src/runtime/sandbox/docker-runtime.ts

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,8 @@ export interface DockerRuntimeParams {
2929
worktreePath: string;
3030
repoPath: string;
3131
branch: string;
32+
/** Human-readable container-name slug (workspace name/branch). */
33+
nameSlug: string;
3234
settings: ResolvedSandboxSettings;
3335
}
3436

@@ -71,7 +73,10 @@ export class DockerRuntime implements WorkspaceRuntime {
7173
return {
7274
shell: "docker",
7375
argv: buildExecArgs({
74-
containerName: getSandboxContainerName(this.params.workspaceId),
76+
containerName: getSandboxContainerName(
77+
this.params.workspaceId,
78+
this.params.nameSlug,
79+
),
7580
cwd: ctx.cwd,
7681
env,
7782
command: ["/bin/bash", "--rcfile", CONTAINER_BASH_RCFILE],
Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,33 @@
1+
import { describe, expect, test } from "bun:test";
2+
import { getSandboxContainerName, sandboxNameSlug } from "./paths.ts";
3+
4+
describe("sandboxNameSlug", () => {
5+
test("sanitizes to docker-safe lowercase and dedupes parts", () => {
6+
expect(sandboxNameSlug("Add tests", "feat/add-tests")).toBe(
7+
"add-tests-feat-add-tests",
8+
);
9+
expect(sandboxNameSlug("main", "main")).toBe("main");
10+
expect(sandboxNameSlug(null, "Fix (WS) #12!")).toBe("fix-ws-12");
11+
});
12+
13+
test("truncates long inputs without a trailing dash", () => {
14+
const slug = sandboxNameSlug("a".repeat(28), "branch-name-that-is-long");
15+
expect(slug.length).toBeLessThanOrEqual(30);
16+
expect(slug.endsWith("-")).toBe(false);
17+
});
18+
});
19+
20+
describe("getSandboxContainerName", () => {
21+
test("slugged name keeps a short unique id suffix", () => {
22+
expect(
23+
getSandboxContainerName(
24+
"9431dce6-39c7-4fd9-b31c-4abc80b73170",
25+
"add-tests",
26+
),
27+
).toBe("superset-add-tests-9431dce6");
28+
});
29+
30+
test("falls back to the id-only form without a slug", () => {
31+
expect(getSandboxContainerName("abc")).toBe("superset-ws-abc");
32+
});
33+
});

packages/host-service/src/runtime/sandbox/paths.ts

Lines changed: 30 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -56,6 +56,34 @@ export function getWorkspaceSandboxPaths(
5656
};
5757
}
5858

59-
export function getSandboxContainerName(workspaceId: string): string {
60-
return `superset-ws-${workspaceId}`;
59+
const CONTAINER_SLUG_MAX = 30;
60+
61+
/**
62+
* Docker-safe, human-readable slug from workspace naming parts (name,
63+
* branch). Duplicate/empty parts collapse; "" when nothing usable survives.
64+
*/
65+
export function sandboxNameSlug(
66+
...parts: Array<string | null | undefined>
67+
): string {
68+
const unique = [...new Set(parts.filter((p): p is string => !!p))];
69+
return unique
70+
.join("-")
71+
.toLowerCase()
72+
.replace(/[^a-z0-9]+/g, "-")
73+
.replace(/^-+|-+$/g, "")
74+
.slice(0, CONTAINER_SLUG_MAX)
75+
.replace(/-+$/, "");
76+
}
77+
78+
/**
79+
* Container name: readable slug (so Docker Desktop shows which workspace it
80+
* is) + short workspace-id suffix for uniqueness. The workspace-id LABEL is
81+
* authoritative for cleanup — names are display + exec targeting only.
82+
*/
83+
export function getSandboxContainerName(
84+
workspaceId: string,
85+
nameSlug?: string,
86+
): string {
87+
if (!nameSlug) return `superset-ws-${workspaceId}`;
88+
return `superset-${nameSlug}-${workspaceId.slice(0, 8)}`;
6189
}

packages/host-service/src/runtime/sandbox/registry.ts

Lines changed: 20 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -6,14 +6,24 @@ import { computeConfigHash } from "./container-manager.ts";
66
import { resolveSandboxSettings } from "./docker-args.ts";
77
import { DockerRuntime } from "./docker-runtime.ts";
88
import { HostRuntime } from "./host-runtime.ts";
9+
import { sandboxNameSlug } from "./paths.ts";
910
import type { WorkspaceRuntime } from "./workspace-runtime.ts";
1011

1112
const hostRuntime = new HostRuntime();
1213
const dockerRuntimes = new Map<
1314
string,
14-
{ configHash: string; runtime: DockerRuntime }
15+
{ configHash: string; nameSlug: string; runtime: DockerRuntime }
1516
>();
1617

18+
/** Container-name slug from a workspace row — shared with tests so name
19+
* expectations can't drift from what the runtime actually creates. */
20+
export function computeWorkspaceNameSlug(row: {
21+
name: string | null;
22+
branch: string;
23+
}): string {
24+
return sandboxNameSlug(row.name, row.branch);
25+
}
26+
1727
/**
1828
* Resolve the execution runtime for a workspace.
1929
*
@@ -44,18 +54,25 @@ export function getWorkspaceRuntime(
4454
})?.sandbox ?? {};
4555
const settings = resolveSandboxSettings(sandboxConfig);
4656
const configHash = computeConfigHash(settings);
57+
const nameSlug = computeWorkspaceNameSlug(workspace);
4758

4859
const cached = dockerRuntimes.get(workspaceId);
49-
if (cached && cached.configHash === configHash) return cached.runtime;
60+
if (
61+
cached &&
62+
cached.configHash === configHash &&
63+
cached.nameSlug === nameSlug
64+
)
65+
return cached.runtime;
5066

5167
const runtime = new DockerRuntime({
5268
workspaceId,
5369
worktreePath: workspace.worktreePath,
5470
repoPath: project.repoPath,
5571
branch: workspace.branch,
72+
nameSlug,
5673
settings,
5774
});
58-
dockerRuntimes.set(workspaceId, { configHash, runtime });
75+
dockerRuntimes.set(workspaceId, { configHash, nameSlug, runtime });
5976
return runtime;
6077
}
6178

packages/host-service/src/runtime/sandbox/sandbox-docker.integration.test.ts

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,10 @@ describe.skipIf(!DOCKER_TESTS)("sandbox docker integration", () => {
4848
let repoPath: string;
4949
let worktreePath: string;
5050
let savedHomeDir: string | undefined;
51-
const containerName = getSandboxContainerName(WORKSPACE_ID);
51+
const containerName = getSandboxContainerName(
52+
WORKSPACE_ID,
53+
"inttest-feature",
54+
);
5255

5356
beforeAll(async () => {
5457
fixtureRoot = mkdtempSync(join(tmpdir(), "superset-sandbox-int-"));
@@ -133,6 +136,7 @@ describe.skipIf(!DOCKER_TESTS)("sandbox docker integration", () => {
133136
worktreePath,
134137
repoPath,
135138
branch: "feature",
139+
nameSlug: "inttest-feature",
136140
settings,
137141
});
138142

@@ -150,6 +154,7 @@ describe.skipIf(!DOCKER_TESTS)("sandbox docker integration", () => {
150154
worktreePath,
151155
repoPath,
152156
branch: "feature",
157+
nameSlug: "inttest-feature",
153158
settings,
154159
});
155160

packages/host-service/src/runtime/sandbox/sandbox-reconcile.ts

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import {
66
listManagedContainers,
77
removeContainer,
88
} from "./docker-cli.ts";
9+
import { getSupersetHomeDir } from "./paths.ts";
910

1011
/**
1112
* Startup sweep: remove Superset-managed containers whose workspace row is
@@ -20,7 +21,14 @@ export async function runSandboxReconcile(db: HostDb): Promise<void> {
2021
const containers = await listManagedContainers();
2122
if (containers.length === 0) return;
2223

24+
const ownerHome = getSupersetHomeDir();
2325
for (const container of containers) {
26+
// Only sweep containers THIS host instance created. Several instances
27+
// share one docker daemon (dev app, integration tests, multiple orgs);
28+
// another instance's live workspace looks like an orphan in our DB.
29+
// Unlabeled containers (pre-ownership builds) are left alone too —
30+
// never delete what we can't prove we own.
31+
if (container.ownerHome !== ownerHome) continue;
2432
const workspace = container.workspaceId
2533
? db.query.workspaces
2634
.findFirst({

0 commit comments

Comments
 (0)