mirror of
https://github.com/Studio-Saelix/sencho.git
synced 2026-08-10 10:49:35 +00:00
fix(atomic-deploy): harden rollback locking, restore fidelity, and tier gating (#1247)
* fix(atomic-deploy): harden rollback locking, restore fidelity, and tier gating Hardens the Atomic Deployments feature found during a full audit: - Rollback now holds the per-stack lifecycle lock (deploy/update already do), so a rollback can no longer race a concurrent deploy on the same compose files. Adds a 'rollback' lifecycle action and releases the lock in finally. - restoreStackFiles is now a faithful revert: it removes managed compose/.env files added after the backup before copying, so a rollback no longer leaves a hybrid of old and new configuration. Scope is the protected file set only; user data is untouched. Aborts (rather than reporting success) if a stale managed file cannot be removed. - The scheduled image-update path derives the atomic flag from the licence tier instead of hardcoding it on, keeping the paid capability explicit at the call site (the scheduler is already paid-gated; this prevents silent drift). - The backup-metadata read (GET /stacks/:name/backup) now requires a paid licence, matching the rollback flow that is the only caller. - Manual rollback dispatches a success/failure notification, alongside the existing audit-log entry. Adds route integration tests (lock acquisition/release, tier 403, notifications, no-backup 404), filesystem tests for the faithful restore (orphan removal, variant switch, abort path, non-managed files preserved), a community-tier scheduler test, and a developer-mode logging matrix. Documents the restore semantics and reconciles the scheduled-update wording in the feature guide. * fix(atomic-deploy): assert restore target stays within the compose dir before unlink The orphan-removal step in restoreStackFiles joins the stack directory with a managed filename and unlinks it. The stack directory is already validated and contained by resolveStackDir (allowlist stack name + within-base assertion), but the containment guard was not reapplied to the joined target at the delete sink, so static analysis flagged the path as derived from user input. Reassert containment on the final path before unlinking, matching the barrier the other write/read helpers in this service already apply. No behavior change for valid stacks; defense-in-depth at the sink. * fix(atomic-deploy): inline the path-containment barrier at the restore unlink sink The wrapped within-base assertion was not recognized as a sanitizer by the static path-injection analysis, which still traced the stack name to the unlink sink. Replace it with the inline path.resolve + startsWith containment check the other write helpers in this service already use (the recognized barrier), kept in the same scope as the sink. Behavior is unchanged for valid stack names. * fix(atomic-deploy): clear stale managed files from the backup slot before writing The backup directory is reused across runs and was only ever added to, never cleared. A managed file removed from the stack since the last backup (e.g. a deleted .env or a switched compose variant) lingered in the slot, so a later rollback restored a file that did not exist immediately before the failed run, contradicting the faithful-revert guarantee. Clear the protected file set from the slot before copying the current files, with the same inline containment barrier the restore path uses. A clear failure is logged, not fatal, since it only risks a stale future rollback and should not block a valid deploy.
This commit is contained in:
@@ -488,6 +488,28 @@ export class FileSystemService {
|
||||
const backupDir = this.getBackupDir(stackName);
|
||||
await fsPromises.mkdir(backupDir, { recursive: true });
|
||||
|
||||
// Clear stale managed files from the backup slot before writing the current
|
||||
// ones. The slot is reused across runs, so a managed file removed from the
|
||||
// stack since the last backup (e.g. a deleted .env or a switched compose
|
||||
// variant) would otherwise linger here and a later restore would resurrect
|
||||
// it, breaking the faithful-revert guarantee. Scope is the protected set
|
||||
// Sencho writes; .timestamp is rewritten below. Containment is re-checked at
|
||||
// the sink, rooted at the backup base, for the same reason restoreStackFiles
|
||||
// does it. A clear failure is logged but not fatal: it only risks a stale
|
||||
// future rollback, so it should not block an otherwise valid deploy.
|
||||
const backupRoot = path.resolve(getBackupBaseDir());
|
||||
for (const file of PROTECTED_STACK_FILES) {
|
||||
const stale = path.resolve(backupRoot, path.join(backupDir, file));
|
||||
if (!stale.startsWith(backupRoot + path.sep)) continue;
|
||||
try {
|
||||
await fsPromises.unlink(stale);
|
||||
} catch (e: unknown) {
|
||||
if ((e as NodeJS.ErrnoException)?.code !== 'ENOENT') {
|
||||
console.warn(`[FileSystemService] Could not clear stale backup ${file}:`, (e as Error).message);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// Copy compose file
|
||||
const composeFiles = ['compose.yaml', 'compose.yml', 'docker-compose.yaml', 'docker-compose.yml'];
|
||||
for (const file of composeFiles) {
|
||||
@@ -527,11 +549,49 @@ export class FileSystemService {
|
||||
const backupDir = this.getBackupDir(stackName);
|
||||
|
||||
const items = await fsPromises.readdir(backupDir);
|
||||
const backedUp = new Set(items);
|
||||
|
||||
// Remove managed files the backup does not contain before copying, so a
|
||||
// rollback is a faithful revert rather than an additive overlay. If the
|
||||
// failed deploy switched compose variants (e.g. compose.yaml ->
|
||||
// docker-compose.yml) or added a .env the backup predates, leaving the new
|
||||
// file in place would re-deploy a hybrid of old and new configuration.
|
||||
// Scope is strictly PROTECTED_STACK_FILES (the same set Sencho backs up);
|
||||
// user data and bind-mounted content in the stack directory are untouched.
|
||||
// Canonical js/path-injection barrier: path.resolve(SAFE_ROOT, untrusted)
|
||||
// followed by a single startsWith check, both inline with the sink. stackDir
|
||||
// is already validated by resolveStackDir; this re-establishes containment at
|
||||
// the delete sink itself so static analysis sees the barrier.
|
||||
const baseResolved = path.resolve(this.baseDir);
|
||||
let removedOrphans = 0;
|
||||
for (const file of PROTECTED_STACK_FILES) {
|
||||
if (backedUp.has(file)) continue;
|
||||
const target = path.resolve(baseResolved, path.join(stackDir, file));
|
||||
if (!target.startsWith(baseResolved + path.sep)) {
|
||||
throw Object.assign(new Error('Path escapes compose directory'), { code: 'INVALID_PATH' });
|
||||
}
|
||||
try {
|
||||
await fsPromises.unlink(target);
|
||||
removedOrphans++;
|
||||
} catch (e: unknown) {
|
||||
const code = (e as NodeJS.ErrnoException)?.code;
|
||||
// ENOENT means the file is already absent, which is the desired end
|
||||
// state. Any other code (EACCES on a chowned bind mount, EBUSY on a
|
||||
// held file) means a managed file Sencho meant to remove is still on
|
||||
// disk: completing the copy below would leave a hybrid config while
|
||||
// reporting success. Abort so the caller surfaces a real failure and
|
||||
// preserves the backup for manual recovery.
|
||||
if (code !== 'ENOENT') {
|
||||
throw new Error(`Rollback aborted: could not remove stale ${file} (${code ?? 'unknown error'}); the restore would leave a mix of old and new configuration.`);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
for (const item of items) {
|
||||
if (item === '.timestamp') continue;
|
||||
await fsPromises.copyFile(path.join(backupDir, item), path.join(stackDir, item));
|
||||
}
|
||||
if (debug) console.debug(`[FileSystemService:debug] Restore completed in ${Date.now() - t0}ms`, { stackName, files: items.filter(i => i !== '.timestamp') });
|
||||
if (debug) console.debug(`[FileSystemService:debug] Restore completed in ${Date.now() - t0}ms`, { stackName, restored: items.filter(i => i !== '.timestamp').length, removedOrphans });
|
||||
}
|
||||
|
||||
async getBackupInfo(stackName: string): Promise<{ exists: boolean; timestamp: number | null }> {
|
||||
|
||||
@@ -719,7 +719,13 @@ export class SchedulerService {
|
||||
auditPath: `/api/scheduled-tasks/auto-update/${stackName}`,
|
||||
}),
|
||||
);
|
||||
await compose.updateStack(stackName, undefined, true);
|
||||
// Atomic backup/rollback is a paid capability. Every path that reaches
|
||||
// this method is already paid-gated (the scheduler tick and the manual
|
||||
// run route both require a paid licence), but the flag is resolved from
|
||||
// the licence here so the tier intent is explicit at the call site and
|
||||
// survives any future refactor that introduces another caller.
|
||||
const atomic = LicenseService.getInstance().getTier() === 'paid';
|
||||
await compose.updateStack(stackName, undefined, atomic);
|
||||
db.clearStackUpdateStatus(nodeId, stackName);
|
||||
|
||||
this.safeDispatch(
|
||||
|
||||
@@ -1,13 +1,14 @@
|
||||
/**
|
||||
* Tracks in-flight stack lifecycle operations (deploy, down, restart, stop,
|
||||
* start, update) per (nodeId, stackName). A second request to the same stack
|
||||
* while the first is still running returns 409 instead of racing the first.
|
||||
* start, update, rollback) per (nodeId, stackName). A second request to the
|
||||
* same stack while the first is still running returns 409 instead of racing
|
||||
* the first.
|
||||
*
|
||||
* State is intentionally process-local: a Sencho restart clears all locks,
|
||||
* which matches the lifecycle of any in-flight `docker compose` child process.
|
||||
*/
|
||||
|
||||
export type StackOpAction = 'deploy' | 'down' | 'restart' | 'stop' | 'start' | 'update';
|
||||
export type StackOpAction = 'deploy' | 'down' | 'restart' | 'stop' | 'start' | 'update' | 'rollback';
|
||||
|
||||
export interface StackOpLock {
|
||||
action: StackOpAction;
|
||||
|
||||
Reference in New Issue
Block a user