Skip to content

Fix: ensure atomic file replacement when destination exists #2227

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
VanRitzOwen wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Fix: ensure atomic file replacement when destination exists #2227

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
109 changes: 98 additions & 11 deletions src/backend/src/filesystem/hl_operations/hl_copy.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@ const { HLFilesystemOperation } = require('./definitions');
const { MkTree } = require('./hl_mkdir');
const { HLRemove } = require('./hl_remove');
const { LLCopy } = require('../ll_operations/ll_copy');
const { LLMove } = require('../ll_operations/ll_move');
const { v4: uuidv4 } = require('uuid');

class HLCopy extends HLFilesystemOperation {
static DESCRIPTION = `
Expand Down Expand Up @@ -164,6 +166,9 @@ class HLCopy extends HLFilesystemOperation {
}

let overwritten;
let destinationBackupNode = null;
let temporaryBackupName = null;

if ( await dest.exists() ) {
// condition: no overwrite behaviour specified
if ( !values.overwrite && !values.dedupe_name ) {
Expand Down Expand Up @@ -191,26 +196,108 @@ class HLCopy extends HLFilesystemOperation {
throw APIError.create('forbidden');
}

// TODO: This will be LLRemove
// TODO: what to do with parent_operation?
overwritten = await dest.getSafeEntry();

// Atomic replacement strategy to prevent data loss:
// 1. Rename destination file to a temporary name (backup)
// 2. Copy source file to the destination
// 3. On success: delete the backup file
// 4. On failure: rename the backup file back to original name
//
// This ensures that at least one valid file always exists:
// - If step 1 fails: both files remain intact
// - If step 2 fails: source file is intact, backup is restored
// - If step 3 fails: operation succeeded, backup cleanup is best-effort

const destinationName = await dest.get('name');
temporaryBackupName = `.puter_backup_${uuidv4()}_${destinationName}`;

// Step 1: Rename destination to temporary backup name
const backupMoveOperation = new LLMove();
destinationBackupNode = await backupMoveOperation.run({
source: dest,
parent,
target_name: temporaryBackupName,
user: values.user,
metadata: await dest.get('metadata'),
});
}
}

let copySucceeded = false;
try {
// Step 2: Copy source file to destination
const ll_copy = new LLCopy();
this.copied = await ll_copy.run({
source,
parent,
user: values.user,
target_name,
});
copySucceeded = true;
} catch ( copyError ) {
// Step 4 (failure path): Restore the backup if we created one
if ( destinationBackupNode && temporaryBackupName ) {
let restoreSucceeded = false;
try {
const restoreMoveOperation = new LLMove();
await restoreMoveOperation.run({
source: destinationBackupNode,
parent,
target_name: target_name,
user: values.user,
metadata: await destinationBackupNode.get('metadata'),
});
restoreSucceeded = true;
} catch ( restoreError ) {
// Restore failed - the backup file still exists with the temporary name.
// This is a critical failure: the destination is gone and we couldn't restore it.
// The backup file remains with the temporary name for manual recovery.
this.log?.error?.('Failed to restore destination backup after copy failure', {
original_error: copyError.message,
restore_error: restoreError.message,
backup_name: temporaryBackupName,
parent_path: parent.path,
});

// Wrap the original error with additional context about the backup file
const errorMessage =
`Copy failed and restore failed. Backup file "${temporaryBackupName}" ` +
`may remain in the folder. Original error: ${copyError.message}`;
const enhancedError = new Error(errorMessage);
enhancedError.originalError = copyError;
enhancedError.restoreError = restoreError;
enhancedError.backupFileName = temporaryBackupName;
throw enhancedError;
}

// Restore succeeded - folder is back to original state
if ( restoreSucceeded ) {
throw copyError;
}
}
throw copyError;
}

// Step 3: Delete the backup file (best-effort cleanup)
if ( destinationBackupNode && copySucceeded ) {
try {
const hl_remove = new HLRemove();
await hl_remove.run({
target: dest,
target: destinationBackupNode,
user: values.user,
recursive: true,
});
} catch ( cleanupError ) {
// Log cleanup failure but don't fail the operation
// The copy succeeded, so the backup can be cleaned up later
this.log?.warn?.('Failed to clean up destination backup after successful copy', {
error: cleanupError.message,
backup_name: temporaryBackupName,
});
}
}

const ll_copy = new LLCopy();
this.copied = await ll_copy.run({
source,
parent,
user: values.user,
target_name,
});

await this.copied.awaitStableEntry();
const response = await this.copied.getSafeEntry({ thumbnail: true });
return {
Expand Down
114 changes: 103 additions & 11 deletions src/backend/src/filesystem/hl_operations/hl_move.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ const { HLFilesystemOperation } = require('./definitions');
const { MkTree } = require('./hl_mkdir');
const { HLRemove } = require('./hl_remove');
const { TYPE_DIRECTORY } = require('../FSNodeContext');
const { v4: uuidv4 } = require('uuid');

class HLMove extends HLFilesystemOperation {
static MODULES = {
Expand Down Expand Up @@ -148,6 +149,9 @@ class HLMove extends HLFilesystemOperation {
}

let overwritten;
let destinationBackupNode = null;
let temporaryBackupName = null;

if ( await dest.exists() ) {
if ( !values.overwrite && !values.dedupe_name ) {
throw APIError.create('item_with_same_name_exists', null, {
Expand All @@ -171,10 +175,34 @@ class HLMove extends HLFilesystemOperation {
}
else if ( values.overwrite ) {
overwritten = await dest.getSafeEntry();
const hl_remove = new HLRemove();
await hl_remove.run({
target: dest,

// Check if destination is immutable before attempting replacement
if ( dest.entry.immutable ) {
throw APIError.create('immutable');
}

// Atomic replacement strategy to prevent data loss:
// 1. Rename destination file to a temporary name (backup)
// 2. Move source file to the destination
// 3. On success: delete the backup file
// 4. On failure: rename the backup file back to original name
//
// This ensures that at least one valid file always exists:
// - If step 1 fails: both files remain intact
// - If step 2 fails: source file is intact, backup is restored
// - If step 3 fails: operation succeeded, backup cleanup is best-effort

const destinationName = await dest.get('name');
temporaryBackupName = `.puter_backup_${uuidv4()}_${destinationName}`;

// Step 1: Rename destination to temporary backup name
const backupMoveOperation = new LLMove();
destinationBackupNode = await backupMoveOperation.run({
source: dest,
parent,
target_name: temporaryBackupName,
user: values.user,
metadata: await dest.get('metadata'),
});
}
else {
Expand All @@ -184,14 +212,78 @@ class HLMove extends HLFilesystemOperation {

const old_path = await source.get('path');

const ll_move = new LLMove();
const source_new = await ll_move.run({
source,
parent,
target_name,
user: values.user,
metadata: metadata,
});
let source_new;
try {
// Step 2: Move source file to destination
const ll_move = new LLMove();
source_new = await ll_move.run({
source,
parent,
target_name,
user: values.user,
metadata: metadata,
});
} catch ( moveError ) {
// Step 4 (failure path): Restore the backup if we created one
if ( destinationBackupNode && temporaryBackupName ) {
let restoreSucceeded = false;
try {
const restoreMoveOperation = new LLMove();
await restoreMoveOperation.run({
source: destinationBackupNode,
parent,
target_name: target_name,
user: values.user,
metadata: await destinationBackupNode.get('metadata'),
});
restoreSucceeded = true;
} catch ( restoreError ) {
// Restore failed - the backup file still exists with the temporary name.
// This is a critical failure: the destination is gone and we couldn't restore it.
// The backup file remains with the temporary name for manual recovery.
this.log?.error?.('Failed to restore destination backup after move failure', {
original_error: moveError.message,
restore_error: restoreError.message,
backup_name: temporaryBackupName,
parent_path: parent.path,
});

// Wrap the original error with additional context about the backup file
const errorMessage =
`Move failed and restore failed. Backup file "${temporaryBackupName}" ` +
`may remain in the folder. Original error: ${moveError.message}`;
const enhancedError = new Error(errorMessage);
enhancedError.originalError = moveError;
enhancedError.restoreError = restoreError;
enhancedError.backupFileName = temporaryBackupName;
throw enhancedError;
}

// Restore succeeded - folder is back to original state
if ( restoreSucceeded ) {
throw moveError;
}
}
throw moveError;
}

// Step 3: Delete the backup file (best-effort cleanup)
if ( destinationBackupNode ) {
try {
const hl_remove = new HLRemove();
await hl_remove.run({
target: destinationBackupNode,
user: values.user,
});
} catch ( cleanupError ) {
// Log cleanup failure but don't fail the operation
// The move succeeded, so the backup can be cleaned up later
this.log?.warn?.('Failed to clean up destination backup after successful move', {
error: cleanupError.message,
backup_name: temporaryBackupName,
});
}
}

await source_new.awaitStableEntry();
await source_new.fetchSuggestedApps();
Expand Down