diff --git a/backend/src/__tests__/blueprints-edge-cases.test.ts b/backend/src/__tests__/blueprints-edge-cases.test.ts index c5f3257b..f7beba38 100644 --- a/backend/src/__tests__/blueprints-edge-cases.test.ts +++ b/backend/src/__tests__/blueprints-edge-cases.test.ts @@ -2,6 +2,7 @@ * Edge-case coverage for the Blueprints feature that the existing suites leave open: * - PUT /:id refusing to disable a blueprint that still has active deployments (409). * - POST / rejecting a selector that exceeds the 200-entry cap (400). + * - DELETE /:id blocking only on live stateful deployments, not never-deployed reviews. * - checkForDrift flagging revision drift when the on-node marker is stale. * - withdrawFromNode refusing to act when the marker belongs to a different blueprint. */ @@ -28,7 +29,7 @@ function seedNode(): { id: number; name: string } { return { id: result.lastInsertRowid as number, name }; } -function seedBlueprint(nodeIds: number[]) { +function seedBlueprint(nodeIds: number[], classification: 'stateless' | 'stateful' = 'stateless') { counter += 1; return DatabaseService.getInstance().createBlueprint({ name: `bp-edge-${counter}`, @@ -36,7 +37,7 @@ function seedBlueprint(nodeIds: number[]) { compose_content: 'services:\n app:\n image: nginx\n', selector: { type: 'nodes', ids: nodeIds }, drift_mode: 'suggest', - classification: 'stateless', + classification, classification_reasons: [], enabled: true, created_by: 'admin', @@ -108,6 +109,131 @@ describe('Blueprint route edge cases', () => { }); }); +describe('Blueprint delete guard', () => { + // Rows with no stack of ours on the node must not block delete, AND the route must not run the + // withdraw primitive for them: withdrawFromNode proceeds on a missing marker and would + // down/delete a same-name stack Sencho never owned. A name_conflict is exactly that unmanaged + // stack, so it is excluded even though it carries a last_deployed_at timestamp. + it.each([ + { label: 'never-deployed pending review', status: 'pending_state_review' as const, last_deployed_at: null }, + { label: 'first-deploy failure', status: 'failed' as const, last_deployed_at: null }, + { label: 'unmanaged same-name stack', status: 'name_conflict' as const, last_deployed_at: Date.now() }, + ])('deletes a stateful blueprint with a $label without touching the node', async ({ status, last_deployed_at }) => { + const node = seedNode(); + const bp = seedBlueprint([node.id], 'stateful'); + DatabaseService.getInstance().upsertDeployment({ blueprint_id: bp.id, node_id: node.id, status, last_deployed_at }); + const withdrawSpy = vi.spyOn(BlueprintService.getInstance(), 'withdrawFromNode').mockResolvedValue({ status: 'withdrawn' }); + + const res = await request(app) + .delete(`/api/blueprints/${bp.id}`) + .set('Cookie', adminCookie); + + expect(res.status).toBe(204); + // Critical: never withdraw a row we did not deploy; it could destroy an unmanaged stack. + expect(withdrawSpy).not.toHaveBeenCalled(); + expect(DatabaseService.getInstance().getBlueprint(bp.id)).toBeUndefined(); + // The blueprint-delete cascade removes the leftover row. + expect(DatabaseService.getInstance().listDeployments(bp.id)).toHaveLength(0); + }); + + // A stack Sencho deployed and still owns (last_deployed_at set) blocks delete regardless of its + // current status, so the operator makes the snapshot-vs-destroy choice. failed and deploying + // carry over last_deployed_at from the prior deploy, so they block too. + it.each(['active', 'drifted', 'correcting', 'evict_blocked', 'failed', 'deploying'] as const)( + 'refuses to delete a stateful blueprint with a deployed %s deployment', + async (status) => { + const node = seedNode(); + const bp = seedBlueprint([node.id], 'stateful'); + DatabaseService.getInstance().upsertDeployment({ + blueprint_id: bp.id, + node_id: node.id, + status, + applied_revision: bp.revision, + last_deployed_at: Date.now(), + }); + + const res = await request(app) + .delete(`/api/blueprints/${bp.id}`) + .set('Cookie', adminCookie); + + expect(res.status).toBe(409); + expect(res.body.code).toBe('stateful_deployments_blocking'); + expect(DatabaseService.getInstance().getBlueprint(bp.id)).toBeDefined(); + }, + ); + + it('withdraws an owned deployment before deleting a stateless blueprint', async () => { + const node = seedNode(); + const bp = seedBlueprint([node.id]); // stateless: the guard is skipped, so the loop runs + DatabaseService.getInstance().upsertDeployment({ + blueprint_id: bp.id, + node_id: node.id, + status: 'active', + applied_revision: bp.revision, + last_deployed_at: Date.now(), + }); + const withdrawSpy = vi.spyOn(BlueprintService.getInstance(), 'withdrawFromNode').mockResolvedValue({ status: 'withdrawn' }); + + const res = await request(app) + .delete(`/api/blueprints/${bp.id}`) + .set('Cookie', adminCookie); + + expect(res.status).toBe(204); + // A deployed, owned stack must still be withdrawn from the node before the blueprint goes. + expect(withdrawSpy).toHaveBeenCalledTimes(1); + expect(DatabaseService.getInstance().getBlueprint(bp.id)).toBeUndefined(); + }); + + it('refuses to delete when a pending review still has a deployed stack (revision drift)', async () => { + const node = seedNode(); + const bp = seedBlueprint([node.id], 'stateful'); + // pending_state_review carried over from a prior deploy keeps last_deployed_at set, + // so the old stack is still on the node and delete must refuse. + DatabaseService.getInstance().upsertDeployment({ + blueprint_id: bp.id, + node_id: node.id, + status: 'pending_state_review', + applied_revision: bp.revision, + last_deployed_at: Date.now(), + }); + + const res = await request(app) + .delete(`/api/blueprints/${bp.id}`) + .set('Cookie', adminCookie); + + expect(res.status).toBe(409); + expect(res.body.code).toBe('stateful_deployments_blocking'); + expect(DatabaseService.getInstance().getBlueprint(bp.id)).toBeDefined(); + }); + + it('counts only the live deployment, not the never-deployed review, in a mixed set', async () => { + const liveNode = seedNode(); + const reviewNode = seedNode(); + const bp = seedBlueprint([liveNode.id, reviewNode.id], 'stateful'); + DatabaseService.getInstance().upsertDeployment({ + blueprint_id: bp.id, + node_id: liveNode.id, + status: 'active', + applied_revision: bp.revision, + last_deployed_at: Date.now(), + }); + DatabaseService.getInstance().upsertDeployment({ + blueprint_id: bp.id, + node_id: reviewNode.id, + status: 'pending_state_review', + }); + + const res = await request(app) + .delete(`/api/blueprints/${bp.id}`) + .set('Cookie', adminCookie); + + expect(res.status).toBe(409); + expect(res.body.code).toBe('stateful_deployments_blocking'); + // Only the live deployment blocks; the never-deployed review is excluded from the count. + expect(res.body.error).toContain('1 live deployment'); + }); +}); + describe('BlueprintService marker edge cases', () => { it('flags revision drift when the on-node marker is stale', async () => { const localNode = DatabaseService.getInstance().getNodes()[0]; diff --git a/backend/src/routes/blueprints.ts b/backend/src/routes/blueprints.ts index 5287d346..78da3cc0 100644 --- a/backend/src/routes/blueprints.ts +++ b/backend/src/routes/blueprints.ts @@ -264,22 +264,36 @@ blueprintsRouter.delete('/:id', async (req: Request, res: Response): Promise d.status === 'active' || d.status === 'evict_blocked' || d.status === 'pending_state_review'); + const blocking = deployments.filter(d => + d.last_deployed_at != null && d.status !== 'name_conflict' && d.status !== 'withdrawn', + ); if (blocking.length > 0) { res.status(409).json({ - error: `Cannot delete a stateful blueprint with ${blocking.length} active or pending deployment(s). Withdraw each deployment explicitly first.`, + error: `Cannot delete a stateful blueprint with ${blocking.length} live deployment(s). Withdraw each from the deployment table first.`, code: 'stateful_deployments_blocking', }); return; } } - // For stateless: best-effort withdraw before delete + // Best-effort cleanup before delete: withdraw exactly the rows a stateful delete would + // block, i.e. stacks Sencho deployed and still owns (last_deployed_at set, and neither a + // name_conflict nor an already-withdrawn row). Never run the withdraw primitive for a + // never-deployed or unmanaged row: withdrawFromNode proceeds on a missing marker and would + // down/delete a same-name stack Sencho does not own. The blueprint-delete cascade removes + // the rows the loop skips. const nodes = DatabaseService.getInstance().getNodes(); const deployments = DatabaseService.getInstance().listDeployments(id); for (const dep of deployments) { + if (dep.last_deployed_at == null || dep.status === 'name_conflict' || dep.status === 'withdrawn') continue; const node = nodes.find(n => n.id === dep.node_id); if (!node) continue; try { diff --git a/frontend/src/components/blueprints/BlueprintDetail.tsx b/frontend/src/components/blueprints/BlueprintDetail.tsx index a31f0c89..4b41c167 100644 --- a/frontend/src/components/blueprints/BlueprintDetail.tsx +++ b/frontend/src/components/blueprints/BlueprintDetail.tsx @@ -341,11 +341,11 @@ export function BlueprintDetail({ blueprintId, open, onOpenChange, onChanged, ca

- Stateless deployments will be withdrawn first. Stateful deployments must be withdrawn explicitly through the deployment table before delete. + Stateless and not-yet-deployed deployments are withdrawn for you. A stateful deployment that is live on a node must be withdrawn from the deployment table first, so you choose whether to snapshot or destroy its data.