mirror of
https://github.com/Studio-Saelix/sencho.git
synced 2026-07-26 11:49:16 +00:00
fix(blueprints): allow deleting blueprints stuck on awaiting-confirmation deployments (#1313)
* fix(blueprints): allow deleting blueprints stuck on awaiting-confirmation deployments Withdrawing a stateful deployment removed its row, but the reconciler re-created it as "Awaiting confirmation" for any node still matching the selector. The delete guard counted that recreated row as blocking, so a stateful blueprint could never be deleted: every withdraw came back as a pending review and delete kept refusing. The delete guard now blocks only on deployments that have a live stack on a node (active, drifted, correcting, evict_blocked, or a pending review that was previously deployed). A never-deployed "Awaiting confirmation" deployment no longer blocks; the delete path's best-effort withdraw-all loop and the foreign-key cascade clear those rows. Live stateful deployments still require explicit withdrawal first, so the snapshot-vs-destroy choice is always made by the operator. Updates the delete dialog copy to match and adds delete-guard coverage. * fix(blueprints): harden delete against destroying unmanaged or live stacks Address two issues found while auditing the delete guard: - A never-deployed deployment (for example a reconciler-created pending review) no longer blocked delete, but the route's withdraw-all loop still ran the withdraw primitive for it. withdrawFromNode proceeds on a missing marker, so deleting the blueprint could down and delete an unmanaged same-name stack on the node. The loop now withdraws only the stacks Sencho deployed and still owns, and skips never-deployed, name_conflict, and withdrawn rows. - A stack that was deployed then failed keeps its last_deployed_at, but the guard's status list did not include failed, so it could be deleted without the snapshot-vs-destroy choice. The guard now keys on last_deployed_at (a stack we deployed and own) rather than a status list, so any owned live stack blocks regardless of its current status, while never-deployed, name_conflict, and withdrawn rows do not. Adds delete-guard coverage for these paths, including that withdraw is never invoked for an unmanaged row and is invoked for an owned one.
This commit is contained in:
@@ -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];
|
||||
|
||||
@@ -264,22 +264,36 @@ blueprintsRouter.delete('/:id', async (req: Request, res: Response): Promise<voi
|
||||
try {
|
||||
const blueprint = DatabaseService.getInstance().getBlueprint(id);
|
||||
if (!blueprint) { res.status(404).json({ error: 'Blueprint not found' }); return; }
|
||||
// Refuse delete on stateful blueprints with active deployments. Operator must withdraw explicitly first
|
||||
// Refuse delete on stateful blueprints that still have a stack Sencho deployed and
|
||||
// owns on a node; the operator must withdraw those explicitly so the snapshot-vs-destroy
|
||||
// choice is made. "Deployed by us" is last_deployed_at != null, which holds regardless of
|
||||
// the current status (a previously deployed stack that later failed still carries it). A
|
||||
// never-deployed row (e.g. a reconciler-created pending_state_review awaiting first deploy)
|
||||
// has nothing of ours on the node, so it must not block delete. name_conflict is a
|
||||
// same-name stack we do not own, and withdrawn is already gone, so neither blocks.
|
||||
if (blueprint.classification === 'stateful' || blueprint.classification === 'unknown') {
|
||||
const deployments = DatabaseService.getInstance().listDeployments(id);
|
||||
const blocking = deployments.filter(d => 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 {
|
||||
|
||||
@@ -341,11 +341,11 @@ export function BlueprintDetail({ blueprintId, open, onOpenChange, onChanged, ca
|
||||
<ModalDestructiveHeader
|
||||
kicker="BLUEPRINT · DELETE · IRREVERSIBLE"
|
||||
title={`Delete ${blueprint.name}`}
|
||||
description="Stateless deployments will be withdrawn first. Stateful deployments must be withdrawn explicitly through the deployment table before delete; the API will refuse otherwise."
|
||||
description="Stateless and not-yet-deployed deployments are withdrawn automatically. Live stateful deployments must be withdrawn from the deployment table first, or delete is refused."
|
||||
/>
|
||||
<ModalBody>
|
||||
<p className="text-sm text-stat-subtitle">
|
||||
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.
|
||||
</p>
|
||||
<div className="space-y-2">
|
||||
<p className="text-xs text-stat-subtitle leading-relaxed">
|
||||
|
||||
Reference in New Issue
Block a user