From ded133a71c7a8c337740c64db9252407c7be9a79 Mon Sep 17 00:00:00 2001 From: deanxbox Date: Tue, 20 Oct 2026 23:51:07 +0100 Subject: [PATCH 1/2] fix: batch trash operations to prevent re-upload races Add a batch trash endpoint with per-path results and bounded concurrency. Trash files in batches, keeping rows dimmed until each batch is confirmed. Wait for in-flight deletes before uploads, drops, and paste operations. Skip selected children when their parent folder is also selected. Trashed 1,000 files in 10.13s with no failures on one remote host. TypeScript checks pass; all 256 Vitest tests pass. Real UI not tested. Copy, move, and rename can still target names being deleted; cancel cannot stop in-flight requests, and permanent delete remains per-item. --- locales/en.json | 1 + src/backend/operation-routes.ts | 48 ++++++ src/backend/trash-service.ts | 27 ++++ src/frontend/FileManager.tsx | 154 ++++++++++++++++---- src/frontend/FileManagerGrid.tsx | 7 + src/frontend/api/ssh-file-operations-api.ts | 34 +++++ src/frontend/file-manager-utils.ts | 69 +++++++++ tests/backend/trash-service.test.ts | 50 ++++++- tests/frontend/trash-in-batches.test.ts | 117 +++++++++++++++ tests/frontend/wait-for-deletes.test.ts | 48 ++++++ 10 files changed, 528 insertions(+), 27 deletions(-) create mode 100644 tests/frontend/trash-in-batches.test.ts create mode 100644 tests/frontend/wait-for-deletes.test.ts diff --git a/locales/en.json b/locales/en.json index 6965541..2114d4e 100644 --- a/locales/en.json +++ b/locales/en.json @@ -266,6 +266,7 @@ "moveToTrashWarning": "The item(s) will be moved to the Termix Trash and can be restored before the retention period expires.", "itemsMovedToTrash_one": "{{count}} item moved to Trash", "itemsMovedToTrash_other": "{{count}} items moved to Trash", + "movingToTrash": "Moving to Trash: {{done}} / {{total}}. Wait until it finishes before pasting or uploading.", "trash": "Trash", "trashDescription": "Deleted items are retained for {{days}} day(s) on this host.", "trashLoadFailed": "Failed to load Trash", diff --git a/src/backend/operation-routes.ts b/src/backend/operation-routes.ts index 6bc2c5c..4316e82 100644 --- a/src/backend/operation-routes.ts +++ b/src/backend/operation-routes.ts @@ -13,6 +13,7 @@ import { emptyTrash, listTrash, moveToTrash, + moveManyToTrash, permanentlyDeleteTrashItem, restoreTrashItem, } from "./trash-service.js"; @@ -26,6 +27,10 @@ type FileOperationRoutesDeps = { const TRASH_RETENTION_DAYS_KEY = "trashRetentionDays"; const TRASH_PURGE_INTERVAL_MS = 60 * 60 * 1000; +// 100 moves in flight finished with no failures on the test host; 16 leaves +// headroom for servers that cap in-flight SFTP requests. +const TRASH_BATCH_CONCURRENCY = 16; +const MAX_TRASH_BATCH = 500; export function registerFileOperationRoutes( app: Express, @@ -644,6 +649,49 @@ export function registerFileOperationRoutes( await executeDelete(false); }); + /** + * @openapi + * /plugin-api/file-manager/deleteItems: + * post: + * summary: Move many files or directories to the trash + * description: Moves up to 500 paths per call. Each path succeeds or fails on its own and is reported in results. + * tags: + * - File Manager + * responses: + * 200: { description: Per-path results, in request order. } + * 400: { description: Bad session or paths. } + * 403: { description: The session belongs to someone else. } + */ + app.post("/deleteItems", async (req, res) => { + const session = await ownedSession(req, res); + if (!session) return; + const paths: unknown = req.body?.paths; + if ( + !Array.isArray(paths) || + paths.length === 0 || + paths.length > MAX_TRASH_BATCH || + !paths.every((p) => typeof p === "string" && p) + ) { + return res.status(400).json({ + error: `paths must be 1 to ${MAX_TRASH_BATCH} non-empty strings`, + }); + } + try { + const sftp = await getSessionSftp(session); + const results = await moveManyToTrash( + sftp, + [...new Set(paths as string[])], + TRASH_BATCH_CONCURRENCY, + ); + purgeExpiredTrash(session, sftp); + res.json({ results }); + } catch (error) { + res + .status(409) + .json({ error: (error as Error).message, trashUnavailable: true }); + } + }); + /** * @openapi * /plugin-api/file-manager/renameItem: diff --git a/src/backend/trash-service.ts b/src/backend/trash-service.ts index f426a41..0efc73f 100644 --- a/src/backend/trash-service.ts +++ b/src/backend/trash-service.ts @@ -204,6 +204,33 @@ export async function moveToTrash( return publicItem(item); } +// Results keep input order; a failing item never affects the others. +export async function moveManyToTrash( + sftp: SFTPWrapper, + paths: string[], + concurrency: number, +): Promise> { + const results: Array<{ path: string; item?: TrashItem; error?: string }> = + paths.map((p) => ({ path: p })); + let next = 0; + await Promise.all( + Array.from( + { length: Math.min(Math.max(1, concurrency), paths.length) }, + async () => { + while (next < paths.length) { + const i = next++; + try { + results[i].item = await moveToTrash(sftp, paths[i]); + } catch (error) { + results[i].error = (error as Error).message; + } + } + }, + ), + ); + return results; +} + export async function listTrash( sftp: SFTPWrapper, retentionDays: number, diff --git a/src/frontend/FileManager.tsx b/src/frontend/FileManager.tsx index 3d27e8d..a640fd2 100644 --- a/src/frontend/FileManager.tsx +++ b/src/frontend/FileManager.tsx @@ -68,6 +68,7 @@ import { createSSHFolder, deleteSSHItem, copySSHItem, + trashSSHItems, renameSSHItem, moveSSHItem, connectSSH, @@ -116,6 +117,8 @@ import type { import { base64ToBytes, formatFileSize, + trashInBatches, + waitForDeletes, runWithConcurrency, } from "./file-manager-utils.ts"; import { @@ -131,6 +134,9 @@ import { restoreItems, } from "./optimistic-file-list"; +/** Paths per trash request, and requests in flight (backend adds its own pool). */ +const TRASH_BATCH_SIZE = 200; +const TRASH_BATCHES_IN_FLIGHT = 3; const LOCAL_PANE_OPEN_STORAGE_KEY = "termix:file-manager:local-pane:open"; const SIDEBAR_OPEN_STORAGE_KEY = "termix:file-manager:sidebar:open"; const LOCAL_PANE_WIDTH_STORAGE_KEY = "termix:file-manager:local-pane:width"; @@ -171,6 +177,7 @@ function FileManagerContent({ ".", ); const currentPathRef = useRef(currentPath); + const pendingDeleteRef = useRef>(Promise.resolve()); currentPathRef.current = currentPath; const lastSuccessfulPathRef = useRef( initialPath || @@ -352,6 +359,14 @@ function FileManagerContent({ const [transferMove, setTransferMove] = useState(false); const [sudoDialogOpen, setSudoDialogOpen] = useState(false); + const [deletingPaths, setDeletingPaths] = useState>( + new Set(), + ); + const deletingPathsRef = useRef>(new Set()); + const updateDeletingPaths = (next: ReadonlySet) => { + deletingPathsRef.current = next; + setDeletingPaths(next); + }; const [pendingSudoOperation, setPendingSudoOperation] = useState(null); @@ -1097,7 +1112,9 @@ function FileManagerContent({ const handleLocalFilesDrop = useCallback( (localPaths: string[], targetDir?: FileItem) => { const remoteDir = targetDir?.path ?? currentPathRef.current; - void localTransfers.uploadLocalPaths(localPaths, remoteDir); + void waitForDeletes(pendingDeleteRef).then(() => + localTransfers.uploadLocalPaths(localPaths, remoteDir), + ); }, [localTransfers], ); @@ -1164,7 +1181,6 @@ function FileManagerContent({ toast.error(t("fileManager.noSSHConnection")); return; } - const files: { file: File; relativePath: string }[] = []; const emptyDirs: string[] = []; @@ -1214,6 +1230,7 @@ function FileManagerContent({ } if (files.length === 0 && emptyDirs.length === 0) return; + await waitForDeletes(pendingDeleteRef); await uploadBatch(files, emptyDirs); } @@ -1370,9 +1387,10 @@ function FileManagerContent({ toast.error(t("fileManager.noSSHConnection")); return; } - + const dropped = Array.from(fileList); + await waitForDeletes(pendingDeleteRef); await uploadBatch( - Array.from(fileList).map((file) => ({ file, relativePath: file.name })), + dropped.map((file) => ({ file, relativePath: file.name })), [], ); } @@ -1470,6 +1488,8 @@ function FileManagerContent({ } async function handleDeleteFiles(files: FileItem[]) { + // Rows already being trashed can't be deleted again. + files = files.filter((file) => !deletingPathsRef.current.has(file.path)); if (!sshSessionId || files.length === 0) return; let confirmMessage: string; @@ -1500,33 +1520,105 @@ function FileManagerContent({ const moveToTrash = async () => { const operationPath = currentPath; const operationSession = sshSessionId; - const deletedPaths = new Set(files.map((file) => file.path)); - commitFilesForPath( - operationSession, - operationPath, - removePaths( - filesForPath(operationSession, operationPath), - deletedPaths, - ), + // A selected folder already takes its selected children with it. + const targets = files.filter( + (file) => + !files.some( + (other) => + other.type === "directory" && + file.path.startsWith(other.path.replace(/\/+$/, "") + "/"), + ), + ); + // Rows stay on screen, dimmed, until the server confirms each batch, so + // the list shows what is still being deleted. + const targetPaths = new Set(targets.map((file) => file.path)); + updateDeletingPaths( + new Set([...deletingPathsRef.current, ...targetPaths]), ); clearSelection(); - let completed = 0; + // Uploads wait for this, so a file re-uploaded under a name still being + // trashed can't be trashed too. + let release!: () => void; + const mine = Promise.all([ + pendingDeleteRef.current, + new Promise((resolve) => (release = resolve)), + ]); + pendingDeleteRef.current = mine; + const controller = new AbortController(); + const progressToast = toast.loading( + t("fileManager.movingToTrash", { done: 0, total: targets.length }), + { + duration: Infinity, + cancel: { + label: t("fileManager.cancel"), + onClick: () => controller.abort(), + }, + }, + ); + let remaining = targets; try { await ensureSSHConnection(); - for (const file of files) { - await deleteSSHItem( - sshSessionId, - file.path, - file.type === "directory", - currentHost?.id, - currentHost?.userId?.toString(), + const { done, itemError, thrown } = await trashInBatches( + targets, + TRASH_BATCH_SIZE, + TRASH_BATCHES_IN_FLIGHT, + async (batch) => + await trashSSHItems( + sshSessionId, + batch.map((file) => file.path), + ), + { + signal: controller.signal, + onBatchDone: (trashed) => + commitFilesForPath( + operationSession, + operationPath, + removePaths( + filesForPath(operationSession, operationPath), + new Set(trashed.map((file) => file.path)), + ), + ), + onProgress: (finished) => + !controller.signal.aborted && + toast.loading( + t("fileManager.movingToTrash", { + done: finished, + total: targets.length, + }), + { + id: progressToast, + duration: Infinity, + cancel: { + label: t("fileManager.cancel"), + onClick: () => controller.abort(), + }, + }, + ), + }, + ); + remaining = targets.filter((file) => !done.has(file)); + if (thrown !== undefined && !controller.signal.aborted) throw thrown; + // Cancel wins over a failed item: never offer permanent delete for + // files the user chose to keep. + if (controller.signal.aborted && remaining.length > 0) { + toast.info(t("fileManager.localTransferCancelled")); + commitFilesForPath( + operationSession, + operationPath, + restoreItems( + filesForPath(operationSession, operationPath), + remaining, + ), ); - completed += 1; + handleRefreshDirectory(); + return; } + if (itemError !== undefined) + throw { response: { data: { trashUnavailable: true } } }; toast.success( - t("fileManager.itemsMovedToTrash", { count: files.length }), + t("fileManager.itemsMovedToTrash", { count: targets.length }), ); if ( sshSessionIdRef.current === operationSession && @@ -1542,7 +1634,7 @@ function FileManagerContent({ operationPath, restoreItems( filesForPath(operationSession, operationPath), - files.slice(completed), + remaining, ), ); const axiosError = error as { @@ -1555,7 +1647,7 @@ function FileManagerContent({ if (axiosError.response?.data?.needsSudo) { setPendingSudoOperation({ type: "delete", - files: files.slice(completed), + files: remaining, }); setSudoDialogOpen(true); return; @@ -1567,7 +1659,7 @@ function FileManagerContent({ title: t("fileManager.trashUnavailableConfirm"), })) ) { - for (const file of files.slice(completed)) { + for (const file of remaining) { await deleteSSHItem( sshSessionId, file.path, @@ -1579,7 +1671,7 @@ function FileManagerContent({ } toast.success( t("fileManager.itemsDeletedSuccessfully", { - count: files.length - completed, + count: remaining.length, }), ); handleRefreshDirectory(); @@ -1606,6 +1698,14 @@ function FileManagerContent({ } else { toast.error(t("fileManager.failedToDeleteItems")); } + } finally { + toast.dismiss(progressToast); + updateDeletingPaths( + new Set( + [...deletingPathsRef.current].filter((p) => !targetPaths.has(p)), + ), + ); + release(); } }; if (confirmBeforeTrash) { @@ -2155,6 +2255,7 @@ function FileManagerContent({ async function handlePasteFiles() { if (!clipboard || !sshSessionId) return; + await waitForDeletes(pendingDeleteRef); if ( clipboard.sourceHostId !== null && @@ -3661,6 +3762,7 @@ function FileManagerContent({ ; onFileOpen: (file: FileItem) => void; onFileIntent?: (file: FileItem) => void; onSelectionChange: (files: FileItem[]) => void; @@ -225,6 +227,7 @@ const getFileIcon = ( export function FileManagerGrid({ files, selectedFiles, + deletingPaths, onFileOpen, onFileIntent, onSelectionChange, @@ -1233,6 +1236,8 @@ export function FileManagerGrid({ dragState.files.some( (f) => f.path === file.path, ) && "opacity-50", + deletingPaths?.has(file.path) && + "opacity-40 pointer-events-none", )} title={file.name} onClick={(e) => handleFileClick(file, e)} @@ -1439,6 +1444,8 @@ export function FileManagerGrid({ "bg-accent-brand/20 border-accent-brand border-dashed", dragState.files.some((f) => f.path === file.path) && "opacity-50", + deletingPaths?.has(file.path) && + "opacity-40 pointer-events-none", )} onClick={(e) => handleFileClick(file, e)} onContextMenu={(e) => { diff --git a/src/frontend/api/ssh-file-operations-api.ts b/src/frontend/api/ssh-file-operations-api.ts index 9e244a4..521cc9d 100644 --- a/src/frontend/api/ssh-file-operations-api.ts +++ b/src/frontend/api/ssh-file-operations-api.ts @@ -612,6 +612,40 @@ export async function deleteSSHItem( } } +export interface TrashBatchResult { + path: string; + error?: string; +} + +/** Moves paths to the trash in one request; each path reports on its own. */ +export async function trashSSHItems( + sessionId: string, + paths: string[], +): Promise { + try { + const response = await getFileManagerApiForSession(sessionId).post( + "/deleteItems", + { sessionId, paths }, + ); + for (const p of paths) invalidateCachedFileContent(sessionId, p); + // The server drops duplicate paths, so match by path, not position. + const byPath = new Map( + (response.data.results as TrashBatchResult[]).map((r) => [r.path, r]), + ); + return paths.map( + (p) => byPath.get(p) ?? { path: p, error: "No result from server" }, + ); + } catch (error) { + if ( + axios.isAxiosError(error) && + error.response?.data?.trashUnavailable === true + ) { + throw error; + } + handleApiError(error, "delete SSH items"); + } +} + export interface TrashItem { id: string; name: string; diff --git a/src/frontend/file-manager-utils.ts b/src/frontend/file-manager-utils.ts index d2992d0..5d759fa 100644 --- a/src/frontend/file-manager-utils.ts +++ b/src/frontend/file-manager-utils.ts @@ -46,3 +46,72 @@ export function base64ToBytes(b64: string): Uint8Array { } return out; } + +/** + * Sends `items` to `trash` in batches, `limit` batches at a time. `trash` + * returns one entry per item in order; an entry with `error` failed on its own + * and the rest carry on. A thrown batch (or an abort) stops new batches, waits + * for those running, and leaves the untouched items out of `done`, so `done` + * is exact when this returns. + */ +export async function trashInBatches( + items: T[], + size: number, + limit: number, + trash: (batch: T[]) => Promise>, + opts: { + signal?: AbortSignal; + onProgress?: (finished: number) => void; + /** Items of a batch the server confirmed trashed, as each batch returns. */ + onBatchDone?: (trashed: T[]) => void; + } = {}, +): Promise<{ + done: Set; + failed: Set; + itemError?: string; + thrown?: unknown; +}> { + const batches: T[][] = []; + for (let i = 0; i < items.length; i += size) + batches.push(items.slice(i, i + size)); + const done = new Set(); + const failed = new Set(); + let finished = 0; + let itemError: string | undefined; + let thrown: unknown; + await runWithConcurrency(batches, limit, async (batch) => { + if (thrown !== undefined || opts.signal?.aborted) return; + try { + const results = await trash(batch); + const trashed: T[] = []; + batch.forEach((item, i) => { + const error = results[i]?.error; + if (error === undefined && results[i]) { + done.add(item); + trashed.push(item); + } else { + failed.add(item); + itemError ??= error ?? "No result"; + } + }); + opts.onBatchDone?.(trashed); + } catch (error) { + thrown = error ?? new Error("Batch failed"); + return; + } + finished += batch.length; + opts.onProgress?.(finished); + }); + return { done, failed, itemError, thrown }; +} + +/** Resolves once no delete is running, including ones started while waiting. */ +export async function waitForDeletes(ref: { + current: Promise; +}): Promise { + let seen: Promise; + do { + seen = ref.current; + await seen; + } while (seen !== ref.current); +} diff --git a/tests/backend/trash-service.test.ts b/tests/backend/trash-service.test.ts index 3c1e082..7528d9d 100644 --- a/tests/backend/trash-service.test.ts +++ b/tests/backend/trash-service.test.ts @@ -1,6 +1,10 @@ import { describe, expect, it, vi } from "vitest"; import type { SFTPWrapper } from "ssh2"; -import { listTrash, moveToTrash } from "../../src/backend/trash-service.js"; +import { + listTrash, + moveManyToTrash, + moveToTrash, +} from "../../src/backend/trash-service.js"; /** An in-memory SFTP channel that counts every request. */ function fakeSftp() { @@ -71,6 +75,50 @@ function fakeSftp() { } describe("trash", () => { + it("never runs more moves at once than the limit", async () => { + const { sftp, files } = fakeSftp(); + const paths = Array.from({ length: 20 }, (_, i) => `/home/me/f${i}.txt`); + for (const p of paths) files.set(p, Buffer.from("x")); + let running = 0; + let peak = 0; + const rename = sftp.rename as unknown as ( + from: string, + to: string, + done: (e?: Error) => void, + ) => void; + const original = rename.bind(sftp); + (sftp as unknown as { rename: typeof rename }).rename = (f, t, d) => { + peak = Math.max(peak, ++running); + setTimeout(() => { + running--; + original(f, t, d); + }, 2); + }; + const results = await moveManyToTrash(sftp, paths, 4); + expect(results.every((r) => r.item)).toBe(true); + expect(peak).toBeLessThanOrEqual(4); + expect(peak).toBeGreaterThan(1); + }); + + it("moves many paths, keeps input order and isolates failures", async () => { + const { sftp, files } = fakeSftp(); + files.set("/home/me/b.txt", Buffer.from("b")); + const results = await moveManyToTrash( + sftp, + ["/home/me/a.txt", "/home/me/missing.txt", "/home/me/b.txt"], + 2, + ); + expect(results.map((r) => r.path)).toEqual([ + "/home/me/a.txt", + "/home/me/missing.txt", + "/home/me/b.txt", + ]); + expect(results[0].item).toBeDefined(); + expect(results[1].error).toBeDefined(); + expect(results[2].item).toBeDefined(); + expect(files.has("/home/me/a.txt")).toBe(false); + }); + it("finds the trash folders once per channel", async () => { const { sftp, calls, files } = fakeSftp(); files.set("/home/me/b.txt", Buffer.from("b")); diff --git a/tests/frontend/trash-in-batches.test.ts b/tests/frontend/trash-in-batches.test.ts new file mode 100644 index 0000000..b14e680 --- /dev/null +++ b/tests/frontend/trash-in-batches.test.ts @@ -0,0 +1,117 @@ +import { describe, expect, it } from "vitest"; +import { trashInBatches } from "../../src/frontend/file-manager-utils"; + +const tick = () => new Promise((resolve) => setTimeout(resolve, 1)); +const ok = (batch: unknown[]) => batch.map(() => ({})); + +describe("trashInBatches", () => { + it("batches, caps batches in flight and reports progress", async () => { + let running = 0; + let peak = 0; + const sizes: number[] = []; + const progress: number[] = []; + const items = Array.from({ length: 1050 }, (_, i) => i); + const { done, thrown } = await trashInBatches( + items, + 200, + 3, + async (batch) => { + peak = Math.max(peak, ++running); + sizes.push(batch.length); + await tick(); + running--; + return ok(batch); + }, + { onProgress: (n) => progress.push(n) }, + ); + expect(thrown).toBeUndefined(); + expect(done.size).toBe(1050); + expect(sizes.sort((a, b) => a - b)).toEqual([50, 200, 200, 200, 200, 200]); + expect(peak).toBe(3); + expect(Math.max(...progress)).toBe(1050); + }); + + it("keeps going past a failed item and reports exactly the failed ones", async () => { + const { done, itemError, thrown } = await trashInBatches( + [1, 2, 3, 4], + 2, + 1, + async (batch) => batch.map((i) => (i === 2 ? { error: "nope" } : {})), + ); + expect(thrown).toBeUndefined(); + expect(itemError).toBe("nope"); + expect([...done].sort()).toEqual([1, 3, 4]); + }); + + it("separates failed items from items never started when cancelled", async () => { + const controller = new AbortController(); + const { done, failed } = await trashInBatches( + [1, 2, 3, 4, 5, 6], + 2, + 1, + async (batch) => { + controller.abort(); + return batch.map((i) => (i === 2 ? { error: "nope" } : {})); + }, + { signal: controller.signal }, + ); + expect([...done]).toEqual([1]); + expect([...failed]).toEqual([2]); + // 3..6 were never started: neither done nor failed, so they go back to the list. + }); + + it("reports only confirmed items per batch as each one returns", async () => { + const seen: number[][] = []; + await trashInBatches( + [1, 2, 3, 4], + 2, + 1, + async (batch) => batch.map((i) => (i === 2 ? { error: "nope" } : {})), + { onBatchDone: (trashed) => seen.push(trashed) }, + ); + expect(seen).toEqual([[1], [3, 4]]); + }); + + it("starts no new batch after one throws", async () => { + const started: number[][] = []; + const items = Array.from({ length: 20 }, (_, i) => i); + const { done, thrown } = await trashInBatches( + items, + 5, + 1, + async (batch) => { + started.push(batch); + if (batch[0] === 5) throw new Error("boom"); + return ok(batch); + }, + ); + expect(thrown).toBeInstanceOf(Error); + expect(started).toHaveLength(2); + expect([...done]).toEqual([0, 1, 2, 3, 4]); + }); + + it("starts no new batch once aborted", async () => { + const controller = new AbortController(); + const started: number[][] = []; + const { done } = await trashInBatches( + Array.from({ length: 20 }, (_, i) => i), + 5, + 1, + async (batch) => { + started.push(batch); + controller.abort(); + return ok(batch); + }, + { signal: controller.signal }, + ); + expect(started).toHaveLength(1); + expect(done.size).toBe(5); + }); + + it("handles an empty list", async () => { + const { done } = await trashInBatches([], 5, 3, async () => { + throw new Error("never"); + }); + expect(done.size).toBe(0); + }); +}); diff --git a/tests/frontend/wait-for-deletes.test.ts b/tests/frontend/wait-for-deletes.test.ts new file mode 100644 index 0000000..06742aa --- /dev/null +++ b/tests/frontend/wait-for-deletes.test.ts @@ -0,0 +1,48 @@ +import { describe, expect, it } from "vitest"; +import { waitForDeletes } from "../../src/frontend/file-manager-utils"; + +/** Mirrors how handleDeleteFiles chains a delete onto the shared barrier. */ +function startDelete(ref: { current: Promise }) { + let release!: () => void; + ref.current = Promise.all([ + ref.current, + new Promise((resolve) => (release = resolve)), + ]); + return release; +} + +describe("waitForDeletes", () => { + it("resolves immediately when nothing is deleting", async () => { + await expect( + waitForDeletes({ current: Promise.resolve() }), + ).resolves.toBeUndefined(); + }); + + it("stays closed until the longer of two overlapping deletes finishes", async () => { + const ref = { current: Promise.resolve() as Promise }; + const releaseLong = startDelete(ref); + const releaseShort = startDelete(ref); + let open = false; + void waitForDeletes(ref).then(() => (open = true)); + releaseShort(); + await new Promise((r) => setTimeout(r, 5)); + expect(open).toBe(false); + releaseLong(); + await new Promise((r) => setTimeout(r, 5)); + expect(open).toBe(true); + }); + + it("also waits for a delete that starts while it is already waiting", async () => { + const ref = { current: Promise.resolve() as Promise }; + const releaseA = startDelete(ref); + let open = false; + void waitForDeletes(ref).then(() => (open = true)); + const releaseB = startDelete(ref); + releaseA(); + await new Promise((r) => setTimeout(r, 5)); + expect(open).toBe(false); + releaseB(); + await new Promise((r) => setTimeout(r, 5)); + expect(open).toBe(true); + }); +}); From e2072085ebecd63c626d05f36341cc5a6a0685f0 Mon Sep 17 00:00:00 2001 From: deanxbox Date: Wed, 21 Oct 2026 00:52:39 +0100 Subject: [PATCH 2/2] fix: batch permanent deletes and cancel queued operations Block rename, create, drag-move, and undo while deletes are in flight, and exclude dimmed rows from Ctrl+A. Add server cancellation so queued delete items stop, and offer permanent deletion only for items the server reports as failed. Batch permanent deletes through /deleteItems with per-call nonce status markers. Use rm -f for files and rm -rf only for listed directories; retry with sudo only paths reported as permission denied, and refuse roots and paths containing a . or .. segment. Hold the delete barrier through permanent-delete and sudo prompts. Add route and permanent-delete unit tests. --- locales/en.json | 1 + src/backend/operation-commands.ts | 2 +- src/backend/operation-routes.ts | 55 ++- src/backend/permanent-delete.ts | 146 +++++++ src/backend/trash-service.ts | 8 +- src/frontend/FileManager.tsx | 445 +++++++++++--------- src/frontend/FileManagerGrid.tsx | 2 +- src/frontend/api/ssh-file-operations-api.ts | 21 +- src/frontend/file-manager-utils.ts | 20 +- tests/backend/delete-items-routes.test.ts | 145 +++++++ tests/backend/permanent-delete.test.ts | 118 ++++++ 11 files changed, 747 insertions(+), 216 deletions(-) create mode 100644 src/backend/permanent-delete.ts create mode 100644 tests/backend/delete-items-routes.test.ts create mode 100644 tests/backend/permanent-delete.test.ts diff --git a/locales/en.json b/locales/en.json index 2114d4e..c0263d2 100644 --- a/locales/en.json +++ b/locales/en.json @@ -266,6 +266,7 @@ "moveToTrashWarning": "The item(s) will be moved to the Termix Trash and can be restored before the retention period expires.", "itemsMovedToTrash_one": "{{count}} item moved to Trash", "itemsMovedToTrash_other": "{{count}} items moved to Trash", + "deletingItems": "Deleting: {{done}} / {{total}}. Wait until it finishes before pasting or uploading.", "movingToTrash": "Moving to Trash: {{done}} / {{total}}. Wait until it finishes before pasting or uploading.", "trash": "Trash", "trashDescription": "Deleted items are retained for {{days}} day(s) on this host.", diff --git a/src/backend/operation-commands.ts b/src/backend/operation-commands.ts index 0eea91c..c21f9b2 100644 --- a/src/backend/operation-commands.ts +++ b/src/backend/operation-commands.ts @@ -5,7 +5,7 @@ export interface DeleteCommand { commandWithSuccess: string; } -function quotePosixPath(path: string): string { +export function quotePosixPath(path: string): string { return `'${path.replace(/'/g, "'\"'\"'")}'`; } diff --git a/src/backend/operation-routes.ts b/src/backend/operation-routes.ts index 4316e82..f24e78b 100644 --- a/src/backend/operation-routes.ts +++ b/src/backend/operation-routes.ts @@ -9,6 +9,7 @@ import { } from "./session.js"; import { ensureDirectoryTreeSftp } from "./transfer-sftp-dir.js"; import { buildDeleteCommand } from "./operation-commands.js"; +import { removePermanently } from "./permanent-delete.js"; import { emptyTrash, listTrash, @@ -31,6 +32,7 @@ const TRASH_PURGE_INTERVAL_MS = 60 * 60 * 1000; // headroom for servers that cap in-flight SFTP requests. const TRASH_BATCH_CONCURRENCY = 16; const MAX_TRASH_BATCH = 500; +const CANCELLED_OP_TTL_MS = 10 * 60 * 1000; export function registerFileOperationRoutes( app: Express, @@ -50,6 +52,15 @@ export function registerFileOperationRoutes( // Deleting used to purge expired trash first, reading every trash entry // before answering. Do it after the response, at most hourly per session. const lastPurge = new WeakMap(); + // Delete operations the user cancelled: batches still queued stop starting + // items, so Cancel does not leave hundreds of requests running. + const cancelledOps = new Map(); + const opKey = (session: SSHSession, req: Request) => { + const opId = req.body?.opId; + return typeof opId === "string" && opId + ? `${session.userId}:${req.body?.sessionId}:${opId}` + : null; + }; const purgeExpiredTrash = (session: SSHSession, sftp: SFTPWrapper) => { const now = Date.now(); if (now - (lastPurge.get(session) ?? 0) < TRASH_PURGE_INTERVAL_MS) return; @@ -654,7 +665,7 @@ export function registerFileOperationRoutes( * /plugin-api/file-manager/deleteItems: * post: * summary: Move many files or directories to the trash - * description: Moves up to 500 paths per call. Each path succeeds or fails on its own and is reported in results. + * description: Moves up to 500 paths per call to the trash, or deletes them permanently when permanent is true. Each path succeeds or fails on its own and is reported in results. * tags: * - File Manager * responses: @@ -676,12 +687,28 @@ export function registerFileOperationRoutes( error: `paths must be 1 to ${MAX_TRASH_BATCH} non-empty strings`, }); } + const unique = [...new Set(paths as string[])]; + const key = opKey(session, req); + const shouldStop = () => (key ? cancelledOps.has(key) : false); + const dirs = new Set( + Array.isArray(req.body?.directories) ? req.body.directories : [], + ); + if (req.body?.permanent === true) { + return res.json({ + results: await removePermanently( + session, + unique.map((p) => ({ path: p, isDirectory: dirs.has(p) })), + shouldStop, + ), + }); + } try { const sftp = await getSessionSftp(session); const results = await moveManyToTrash( sftp, - [...new Set(paths as string[])], + unique, TRASH_BATCH_CONCURRENCY, + shouldStop, ); purgeExpiredTrash(session, sftp); res.json({ results }); @@ -692,6 +719,30 @@ export function registerFileOperationRoutes( } }); + /** + * @openapi + * /plugin-api/file-manager/deleteItems/cancel: + * post: + * summary: Stop a bulk delete from starting more items + * tags: + * - File Manager + * responses: + * 200: { description: The operation will start no further items. } + * 400: { description: Bad session or operation id. } + * 403: { description: The session belongs to someone else. } + */ + app.post("/deleteItems/cancel", async (req, res) => { + const session = await ownedSession(req, res); + if (!session) return; + const key = opKey(session, req); + if (!key) return res.status(400).json({ error: "opId is required" }); + const now = Date.now(); + for (const [k, at] of cancelledOps) + if (now - at > CANCELLED_OP_TTL_MS) cancelledOps.delete(k); + cancelledOps.set(key, now); + res.json({ ok: true }); + }); + /** * @openapi * /plugin-api/file-manager/renameItem: diff --git a/src/backend/permanent-delete.ts b/src/backend/permanent-delete.ts new file mode 100644 index 0000000..e05922a --- /dev/null +++ b/src/backend/permanent-delete.ts @@ -0,0 +1,146 @@ +import { randomUUID } from "node:crypto"; +import path from "node:path"; +import { execBuffer, execWithSudo, type SSHSession } from "./session.js"; +import { buildDeleteCommand, quotePosixPath } from "./operation-commands.js"; +import { isWindowsSftpPath } from "./transfer-paths.js"; + +export type RemoveItem = { path: string; isDirectory: boolean }; +export type RemoveResult = { + path: string; + error?: string; + needsSudo?: boolean; +}; + +// One shell command removes many paths; keep each under typical exec limits. +const MAX_SCRIPT_CHARS = 16_000; +const WINDOWS_CONCURRENCY = 4; + +/** Refuses roots, empty paths and any . or .. segment: a batch rm -rf must never reach them. */ +export function isRemovablePath(itemPath: string): boolean { + const slashed = itemPath.replace(/\\/g, "/"); + if (slashed.split("/").some((s) => s === "." || s === "..")) return false; + const normalized = path.posix.normalize(slashed); + return ( + normalized !== "." && + normalized !== "/" && + !/^\/?[A-Za-z]:\/?$/.test(normalized) + ); +} + +async function runPosixGroup( + session: SSHSession, + group: RemoveItem[], + useSudo: boolean, +): Promise> { + // The nonce keeps file names or error text from forging a status line, and + // rm's own stderr is captured so only these markers reach stdout. + const nonce = randomUUID(); + const script = group + .map((item, i) => { + const rm = item.isDirectory ? "rm -rf --" : "rm -f --"; + return `e=$(LC_ALL=C ${rm} ${quotePosixPath(item.path)} 2>&1) && echo "${nonce} OK ${i}" || { if printf %s "$e" | grep -q ": Permission denied$"; then echo "${nonce} DENIED ${i}"; else echo "${nonce} FAIL ${i}"; fi; }`; + }) + .join("; "); + const command = `sh -c ${quotePosixPath(script)}`; + const result = useSudo + ? await execWithSudo(session, command, session.sudoPassword!) + : await execBuffer(session, command); + const status = new Map(); + const re = new RegExp(`^${nonce} (OK|FAIL|DENIED) (\\d+)$`, "gm"); + for (const m of result.stdout.toString().matchAll(re)) + if (!status.has(Number(m[2]))) + status.set(Number(m[2]), m[1] as "OK" | "FAIL" | "DENIED"); + return status; +} + +/** Permanently removes paths, one shell command per chunk, with per-path results. */ +export async function removePermanently( + session: SSHSession, + items: RemoveItem[], + shouldStop: () => boolean = () => false, +): Promise { + const results: RemoveResult[] = items.map((x) => ({ path: x.path })); + const posix: number[] = []; + const windows: number[] = []; + items.forEach((x, i) => { + if (!isRemovablePath(x.path)) + results[i].error = "This path cannot be deleted"; + else (isWindowsSftpPath(x.path) ? windows : posix).push(i); + }); + + const groups: number[][] = []; + let current: number[] = []; + let size = 0; + for (const i of posix) { + const cost = items[i].path.length * 2 + 120; + if (current.length > 0 && size + cost > MAX_SCRIPT_CHARS) { + groups.push(current); + current = []; + size = 0; + } + current.push(i); + size += cost; + } + if (current.length > 0) groups.push(current); + + for (const group of groups) { + if (shouldStop()) { + for (const i of group) results[i].error = "Cancelled"; + continue; + } + const status = await runPosixGroup( + session, + group.map((i) => items[i]), + false, + ); + // Only paths rm said were denied are retried with sudo, never other failures. + const denied: number[] = []; + group.forEach((i, k) => { + const s = status.get(k); + if (s === "OK") return; + if (s === "DENIED") denied.push(i); + else results[i].error = "Delete failed"; + }); + if (denied.length > 0 && session.sudoPassword && shouldStop()) { + for (const i of denied) results[i].error = "Cancelled"; + } else if (denied.length > 0 && session.sudoPassword) { + const retry = await runPosixGroup( + session, + denied.map((i) => items[i]), + true, + ); + denied.forEach((i, k) => { + if (retry.get(k) !== "OK") results[i].error = "Delete failed"; + }); + } else { + for (const i of denied) { + results[i].error = "Permission denied"; + results[i].needsSudo = true; + } + } + } + + let next = 0; + await Promise.all( + Array.from( + { length: Math.min(WINDOWS_CONCURRENCY, windows.length) }, + async () => { + while (next < windows.length) { + const i = windows[next++]; + if (shouldStop()) { + results[i].error = "Cancelled"; + continue; + } + const { commandWithSuccess } = buildDeleteCommand( + items[i].path, + items[i].isDirectory, + ); + const result = await execBuffer(session, commandWithSuccess); + if (!result.stdout.toString().includes("SUCCESS")) + results[i].error = result.stderr.trim() || "Delete failed"; + } + }, + ), + ); + return results; +} diff --git a/src/backend/trash-service.ts b/src/backend/trash-service.ts index 0efc73f..0e8b53f 100644 --- a/src/backend/trash-service.ts +++ b/src/backend/trash-service.ts @@ -204,11 +204,13 @@ export async function moveToTrash( return publicItem(item); } -// Results keep input order; a failing item never affects the others. +// Results keep input order; a failing item never affects the others. Once +// shouldStop() is true no further item is started; those report "Cancelled". export async function moveManyToTrash( sftp: SFTPWrapper, paths: string[], concurrency: number, + shouldStop: () => boolean = () => false, ): Promise> { const results: Array<{ path: string; item?: TrashItem; error?: string }> = paths.map((p) => ({ path: p })); @@ -219,6 +221,10 @@ export async function moveManyToTrash( async () => { while (next < paths.length) { const i = next++; + if (shouldStop()) { + results[i].error = "Cancelled"; + continue; + } try { results[i].item = await moveToTrash(sftp, paths[i]); } catch (error) { diff --git a/src/frontend/FileManager.tsx b/src/frontend/FileManager.tsx index a640fd2..3ed87df 100644 --- a/src/frontend/FileManager.tsx +++ b/src/frontend/FileManager.tsx @@ -69,6 +69,7 @@ import { deleteSSHItem, copySSHItem, trashSSHItems, + cancelDeleteItems, renameSSHItem, moveSSHItem, connectSSH, @@ -1487,6 +1488,225 @@ function FileManagerContent({ } } + type BulkDeleteOutcome = { + targets: FileItem[]; + done: Set; + failed: Set; + needsSudo: Set; + itemError?: string; + thrown?: unknown; + cancelled: boolean; + }; + + /** + * Deletes in batches while the rows stay on screen, dimmed, until the server + * confirms each batch. Cancel also tells the server to stop starting items. + */ + async function bulkDelete( + items: FileItem[], + permanent: boolean, + ): Promise { + const operationPath = currentPath; + const operationSession = sshSessionId; + // A selected folder already takes its selected children with it. + const targets = items.filter( + (file) => + !items.some( + (other) => + other.type === "directory" && + file.path.startsWith(other.path.replace(/\/+$/, "") + "/"), + ), + ); + const targetPaths = new Set(targets.map((file) => file.path)); + updateDeletingPaths(new Set([...deletingPathsRef.current, ...targetPaths])); + clearSelection(); + const controller = new AbortController(); + const opId = crypto.randomUUID(); + const labelKey = permanent + ? "fileManager.deletingItems" + : "fileManager.movingToTrash"; + const cancel = { + label: t("fileManager.cancel"), + onClick: () => { + controller.abort(); + void cancelDeleteItems(operationSession, opId); + }, + }; + const progressToast = toast.loading( + t(labelKey, { done: 0, total: targets.length }), + { duration: Infinity, cancel }, + ); + try { + await ensureSSHConnection(); + const result = await trashInBatches( + targets, + TRASH_BATCH_SIZE, + TRASH_BATCHES_IN_FLIGHT, + (batch) => + trashSSHItems( + operationSession, + batch.map((file) => file.path), + { + permanent, + opId, + directories: batch + .filter((file) => file.type === "directory") + .map((file) => file.path), + }, + ), + { + signal: controller.signal, + onBatchDone: (confirmed) => + commitFilesForPath( + operationSession, + operationPath, + removePaths( + filesForPath(operationSession, operationPath), + new Set(confirmed.map((file) => file.path)), + ), + ), + onProgress: (finished) => + !controller.signal.aborted && + toast.loading( + t(labelKey, { done: finished, total: targets.length }), + { + id: progressToast, + duration: Infinity, + cancel, + }, + ), + }, + ); + return { targets, ...result, cancelled: controller.signal.aborted }; + } catch (error) { + return { + targets, + done: new Set(), + failed: new Set(), + needsSudo: new Set(), + thrown: error, + cancelled: false, + }; + } finally { + toast.dismiss(progressToast); + updateDeletingPaths( + new Set( + [...deletingPathsRef.current].filter((p) => !targetPaths.has(p)), + ), + ); + } + } + + /** + * Uploads, pastes and moves wait for this, so a file re-uploaded under a name + * still being deleted can't be deleted too. Held through any fallback prompt. + */ + // Resolved when the sudo dialog is submitted or dismissed, so the delete + // barrier stays held while the user decides. + const sudoWaitRef = useRef<(() => void) | null>(null); + function dismissSudo() { + setPendingSudoOperation(null); + sudoWaitRef.current?.(); + sudoWaitRef.current = null; + } + + function holdDeletes(): () => void { + let release!: () => void; + pendingDeleteRef.current = Promise.all([ + pendingDeleteRef.current, + new Promise((resolve) => (release = resolve)), + ]); + return release; + } + + /** Reports a bulk delete and asks only about items the server actually failed. */ + async function afterBulkDelete( + outcome: BulkDeleteOutcome, + permanent: boolean, + ): Promise { + const refresh = () => handleRefreshDirectory(); + if (outcome.done.size > 0 && !outcome.cancelled) { + toast.success( + t( + permanent + ? "fileManager.itemsDeletedSuccessfully" + : "fileManager.itemsMovedToTrash", + { count: outcome.done.size }, + ), + ); + } + if (outcome.cancelled) { + toast.info(t("fileManager.localTransferCancelled")); + refresh(); + return; + } + const thrown = outcome.thrown as + | { + response?: { + data?: { trashUnavailable?: boolean; error?: string }; + status?: number; + }; + message?: string; + } + | undefined; + const trashUnavailable = thrown?.response?.data?.trashUnavailable === true; + if (thrown && !trashUnavailable) { + refresh(); + if ( + thrown.response?.status === 403 || + thrown.response?.data?.error + ?.toLowerCase() + .includes("permission denied") + ) { + toast.error(t("fileManager.permissionDenied")); + } else if ( + thrown.message?.includes("connection") || + thrown.message?.includes("established") + ) { + toast.error( + t("fileManager.sshConnectionFailed", { + name: currentHost?.name, + ip: currentHost?.ip, + port: currentHost?.port, + }), + ); + } else { + toast.error(t("fileManager.failedToDeleteItems")); + } + return; + } + // Only items the server tried and failed: batches never started stay put. + const failed = [...outcome.failed]; + if (failed.length === 0) { + refresh(); + return; + } + if (permanent) { + if (outcome.needsSudo.size > 0) { + setPendingSudoOperation({ + type: "delete", + files: [...outcome.needsSudo], + }); + const answered = new Promise((resolve) => { + sudoWaitRef.current = resolve; + }); + setSudoDialogOpen(true); + refresh(); + await answered; + return; + } + toast.error(t("fileManager.failedToDeleteItems")); + refresh(); + return; + } + if (await confirm({ title: t("fileManager.trashUnavailableConfirm") })) { + await afterBulkDelete(await bulkDelete(failed, true), true); + return; + } + refresh(); + toast.error(t("fileManager.failedToDeleteItems")); + } + async function handleDeleteFiles(files: FileItem[]) { // Rows already being trashed can't be deleted again. files = files.filter((file) => !deletingPathsRef.current.has(file.path)); @@ -1518,193 +1738,10 @@ function FileManagerContent({ const fullMessage = `${confirmMessage}\n\n${t("fileManager.moveToTrashWarning")}`; const moveToTrash = async () => { - const operationPath = currentPath; - const operationSession = sshSessionId; - // A selected folder already takes its selected children with it. - const targets = files.filter( - (file) => - !files.some( - (other) => - other.type === "directory" && - file.path.startsWith(other.path.replace(/\/+$/, "") + "/"), - ), - ); - // Rows stay on screen, dimmed, until the server confirms each batch, so - // the list shows what is still being deleted. - const targetPaths = new Set(targets.map((file) => file.path)); - updateDeletingPaths( - new Set([...deletingPathsRef.current, ...targetPaths]), - ); - clearSelection(); - // Uploads wait for this, so a file re-uploaded under a name still being - // trashed can't be trashed too. - let release!: () => void; - const mine = Promise.all([ - pendingDeleteRef.current, - new Promise((resolve) => (release = resolve)), - ]); - pendingDeleteRef.current = mine; - const controller = new AbortController(); - const progressToast = toast.loading( - t("fileManager.movingToTrash", { done: 0, total: targets.length }), - { - duration: Infinity, - cancel: { - label: t("fileManager.cancel"), - onClick: () => controller.abort(), - }, - }, - ); - let remaining = targets; + const release = holdDeletes(); try { - await ensureSSHConnection(); - - const { done, itemError, thrown } = await trashInBatches( - targets, - TRASH_BATCH_SIZE, - TRASH_BATCHES_IN_FLIGHT, - async (batch) => - await trashSSHItems( - sshSessionId, - batch.map((file) => file.path), - ), - { - signal: controller.signal, - onBatchDone: (trashed) => - commitFilesForPath( - operationSession, - operationPath, - removePaths( - filesForPath(operationSession, operationPath), - new Set(trashed.map((file) => file.path)), - ), - ), - onProgress: (finished) => - !controller.signal.aborted && - toast.loading( - t("fileManager.movingToTrash", { - done: finished, - total: targets.length, - }), - { - id: progressToast, - duration: Infinity, - cancel: { - label: t("fileManager.cancel"), - onClick: () => controller.abort(), - }, - }, - ), - }, - ); - remaining = targets.filter((file) => !done.has(file)); - if (thrown !== undefined && !controller.signal.aborted) throw thrown; - // Cancel wins over a failed item: never offer permanent delete for - // files the user chose to keep. - if (controller.signal.aborted && remaining.length > 0) { - toast.info(t("fileManager.localTransferCancelled")); - commitFilesForPath( - operationSession, - operationPath, - restoreItems( - filesForPath(operationSession, operationPath), - remaining, - ), - ); - handleRefreshDirectory(); - return; - } - if (itemError !== undefined) - throw { response: { data: { trashUnavailable: true } } }; - - toast.success( - t("fileManager.itemsMovedToTrash", { count: targets.length }), - ); - if ( - sshSessionIdRef.current === operationSession && - currentPathRef.current === operationPath - ) { - handleRefreshDirectory(); - } else { - invalidateCachedFileList(operationSession, operationPath); - } - } catch (error: unknown) { - commitFilesForPath( - operationSession, - operationPath, - restoreItems( - filesForPath(operationSession, operationPath), - remaining, - ), - ); - const axiosError = error as { - response?: { - data?: { needsSudo?: boolean; error?: string }; - status?: number; - }; - message?: string; - }; - if (axiosError.response?.data?.needsSudo) { - setPendingSudoOperation({ - type: "delete", - files: remaining, - }); - setSudoDialogOpen(true); - return; - } - if ( - (axiosError.response?.data as { trashUnavailable?: boolean }) - ?.trashUnavailable && - (await confirm({ - title: t("fileManager.trashUnavailableConfirm"), - })) - ) { - for (const file of remaining) { - await deleteSSHItem( - sshSessionId, - file.path, - file.type === "directory", - currentHost?.id, - currentHost?.userId?.toString(), - true, - ); - } - toast.success( - t("fileManager.itemsDeletedSuccessfully", { - count: remaining.length, - }), - ); - handleRefreshDirectory(); - return; - } - if ( - axiosError.response?.status === 403 || - axiosError.response?.data?.error - ?.toLowerCase() - .includes("permission denied") - ) { - toast.error(t("fileManager.permissionDenied")); - } else if ( - axiosError.message?.includes("connection") || - axiosError.message?.includes("established") - ) { - toast.error( - t("fileManager.sshConnectionFailed", { - name: currentHost?.name, - ip: currentHost?.ip, - port: currentHost?.port, - }), - ); - } else { - toast.error(t("fileManager.failedToDeleteItems")); - } + await afterBulkDelete(await bulkDelete(files, false), false); } finally { - toast.dismiss(progressToast); - updateDeletingPaths( - new Set( - [...deletingPathsRef.current].filter((p) => !targetPaths.has(p)), - ), - ); release(); } }; @@ -1726,32 +1763,26 @@ function FileManagerContent({ setSudoDialogOpen(false); if (pendingSudoOperation.type === "delete") { - for (const file of pendingSudoOperation.files) { - await deleteSSHItem( - sshSessionId, - file.path, - file.type === "directory", - currentHost?.id, - currentHost?.userId?.toString(), - ); + const sudoFiles = pendingSudoOperation.files; + // Take the new barrier before the old one is released. + const release = holdDeletes(); + dismissSudo(); + try { + await afterBulkDelete(await bulkDelete(sudoFiles, true), true); + } finally { + release(); } - toast.success( - t("fileManager.itemsDeletedSuccessfully", { - count: pendingSudoOperation.files.length, - }), - ); - handleRefreshDirectory(); - clearSelection(); + return; } else if (pendingSudoOperation.type === "navigate") { const success = await loadDirectory(pendingSudoOperation.path); if (success) { setCurrentPath(pendingSudoOperation.path); - setPendingSudoOperation(null); + dismissSudo(); } return; } - setPendingSudoOperation(null); + dismissSudo(); } catch (error: unknown) { const axiosError = error as { response?: { data?: { needsSudo?: boolean; sudoFailed?: boolean } }; @@ -1765,7 +1796,7 @@ function FileManagerContent({ } toast.error(axiosError.message || t("fileManager.sudoOperationFailed")); - setPendingSudoOperation(null); + dismissSudo(); } } @@ -2558,6 +2589,7 @@ function FileManagerContent({ } async function handleUndo() { + await waitForDeletes(pendingDeleteRef); if (undoHistory.length === 0) { toast.info(t("fileManager.noUndoableActions")); return; @@ -2728,6 +2760,7 @@ function FileManagerContent({ async function handleConfirmCreate(name: string) { if (!createIntent || !sshSessionId) return; + await waitForDeletes(pendingDeleteRef); const operationPath = currentPath; const operationSession = sshSessionId; @@ -2809,6 +2842,7 @@ function FileManagerContent({ async function handleRenameConfirm(file: FileItem, newName: string) { if (!sshSessionId) return; + await waitForDeletes(pendingDeleteRef); const operationPath = currentPath; const operationSession = sshSessionId; @@ -3180,6 +3214,7 @@ function FileManagerContent({ targetFolder: FileItem, ) { if (!sshSessionId || targetFolder.type !== "directory") return; + await waitForDeletes(pendingDeleteRef); try { await ensureSSHConnection(); @@ -3935,7 +3970,9 @@ function FileManagerContent({ handleSavePermissions={handleSavePermissions} sudoDialogOpen={sudoDialogOpen} setSudoDialogOpen={setSudoDialogOpen} - setPendingSudoOperation={setPendingSudoOperation} + setPendingSudoOperation={(op) => + op ? setPendingSudoOperation(op) : dismissSudo() + } handleSudoPasswordSubmit={handleSudoPasswordSubmit} /> !deletingPaths?.has(f.path))); } break; case "c": diff --git a/src/frontend/api/ssh-file-operations-api.ts b/src/frontend/api/ssh-file-operations-api.ts index 521cc9d..63035de 100644 --- a/src/frontend/api/ssh-file-operations-api.ts +++ b/src/frontend/api/ssh-file-operations-api.ts @@ -615,17 +615,24 @@ export async function deleteSSHItem( export interface TrashBatchResult { path: string; error?: string; + needsSudo?: boolean; } -/** Moves paths to the trash in one request; each path reports on its own. */ +/** Moves paths to the trash (or deletes them permanently) in one request. */ export async function trashSSHItems( sessionId: string, paths: string[], + opts: { + permanent?: boolean; + opId?: string; + /** Paths that are directories; other permanent deletes never recurse. */ + directories?: string[]; + } = {}, ): Promise { try { const response = await getFileManagerApiForSession(sessionId).post( "/deleteItems", - { sessionId, paths }, + { sessionId, paths, ...opts }, ); for (const p of paths) invalidateCachedFileContent(sessionId, p); // The server drops duplicate paths, so match by path, not position. @@ -646,6 +653,16 @@ export async function trashSSHItems( } } +/** Tells the server to stop starting items for a bulk delete. */ +export async function cancelDeleteItems( + sessionId: string, + opId: string, +): Promise { + await getFileManagerApiForSession(sessionId) + .post("/deleteItems/cancel", { sessionId, opId }) + .catch(() => undefined); +} + export interface TrashItem { id: string; name: string; diff --git a/src/frontend/file-manager-utils.ts b/src/frontend/file-manager-utils.ts index 5d759fa..644f31a 100644 --- a/src/frontend/file-manager-utils.ts +++ b/src/frontend/file-manager-utils.ts @@ -47,18 +47,24 @@ export function base64ToBytes(b64: string): Uint8Array { return out; } +/** What the server reports for items it skipped because the user cancelled. */ +const CANCELLED = "Cancelled"; + /** * Sends `items` to `trash` in batches, `limit` batches at a time. `trash` * returns one entry per item in order; an entry with `error` failed on its own * and the rest carry on. A thrown batch (or an abort) stops new batches, waits - * for those running, and leaves the untouched items out of `done`, so `done` - * is exact when this returns. + * for those running, and leaves the untouched items out of `done` and + * `failed`, so both are exact when this returns. Items of a batch that threw + * count as failed; items the server skipped on Cancel count as neither. */ export async function trashInBatches( items: T[], size: number, limit: number, - trash: (batch: T[]) => Promise>, + trash: ( + batch: T[], + ) => Promise>, opts: { signal?: AbortSignal; onProgress?: (finished: number) => void; @@ -68,6 +74,7 @@ export async function trashInBatches( ): Promise<{ done: Set; failed: Set; + needsSudo: Set; itemError?: string; thrown?: unknown; }> { @@ -76,6 +83,7 @@ export async function trashInBatches( batches.push(items.slice(i, i + size)); const done = new Set(); const failed = new Set(); + const needsSudo = new Set(); let finished = 0; let itemError: string | undefined; let thrown: unknown; @@ -89,20 +97,22 @@ export async function trashInBatches( if (error === undefined && results[i]) { done.add(item); trashed.push(item); - } else { + } else if (error !== CANCELLED) { failed.add(item); + if (results[i]?.needsSudo) needsSudo.add(item); itemError ??= error ?? "No result"; } }); opts.onBatchDone?.(trashed); } catch (error) { thrown = error ?? new Error("Batch failed"); + for (const item of batch) failed.add(item); return; } finished += batch.length; opts.onProgress?.(finished); }); - return { done, failed, itemError, thrown }; + return { done, failed, needsSudo, itemError, thrown }; } /** Resolves once no delete is running, including ones started while waiting. */ diff --git a/tests/backend/delete-items-routes.test.ts b/tests/backend/delete-items-routes.test.ts new file mode 100644 index 0000000..e507f86 --- /dev/null +++ b/tests/backend/delete-items-routes.test.ts @@ -0,0 +1,145 @@ +import type { Express, Request, Response } from "express"; +import { beforeEach, describe, expect, it, vi } from "vitest"; +import type { PluginContext } from "@termix-ssh/plugin-sdk/backend"; +import { registerFileOperationRoutes } from "../../src/backend/operation-routes.js"; +import type { SSHSession } from "../../src/backend/session.js"; + +const m = vi.hoisted(() => ({ + moveMany: vi.fn(), + remove: vi.fn(), +})); +vi.mock("../../src/backend/session.js", () => ({ + execChannel: vi.fn(), + execWithSudo: vi.fn(), + getSessionSftp: vi.fn(async () => ({})), +})); +vi.mock("../../src/backend/trash-service.js", () => ({ + emptyTrash: vi.fn(), + listTrash: vi.fn(async () => []), + moveToTrash: vi.fn(), + moveManyToTrash: m.moveMany, + permanentlyDeleteTrashItem: vi.fn(), + restoreTrashItem: vi.fn(), +})); +vi.mock("../../src/backend/permanent-delete.js", () => ({ + removePermanently: m.remove, +})); + +type Handler = (req: Request, res: Response) => Promise; + +function setup(owned = true) { + const routes = new Map(); + const app = { + get: vi.fn(), + put: vi.fn(), + delete: vi.fn(), + post: (p: string, h: Handler) => routes.set(p, h), + } as unknown as Express; + registerFileOperationRoutes(app, { + ctx: { + currentActor: () => "u", + log: { info: vi.fn(), error: vi.fn(), warn: vi.fn() }, + settings: { get: vi.fn() }, + rbac: { has: vi.fn() }, + } as unknown as PluginContext, + sshSessions: { s: { isConnected: true, userId: "u" } as SSHSession }, + verifySessionOwnership: () => owned, + }); + return (route: string, body: Record) => + new Promise<{ status: number; body: Record }>( + (resolve) => { + let status = 200; + const res = { + status(c: number) { + status = c; + return res; + }, + json(b: Record) { + resolve({ status, body: b }); + }, + }; + void routes.get(route)!( + { body, query: {} } as unknown as Request, + res as unknown as Response, + ); + }, + ); +} + +describe("POST /deleteItems", () => { + beforeEach(() => { + m.moveMany.mockReset(); + m.remove.mockReset(); + }); + + it.each([ + [{ sessionId: "s", paths: [] }], + [{ sessionId: "s", paths: [1] }], + [{ sessionId: "s", paths: [""] }], + [{ sessionId: "s", paths: Array(501).fill("/a") }], + [{ sessionId: "s" }], + ])("rejects bad paths %j", async (body) => { + expect((await setup()("/deleteItems", body)).status).toBe(400); + }); + + it("rejects unknown and foreign sessions", async () => { + expect( + (await setup()("/deleteItems", { sessionId: "x", paths: ["/a"] })).status, + ).toBe(400); + expect( + (await setup(false)("/deleteItems", { sessionId: "s", paths: ["/a"] })) + .status, + ).toBe(403); + }); + + it("dedupes paths and returns per-path results", async () => { + const results = [{ path: "/a" }, { path: "/b", error: "nope" }]; + m.moveMany.mockResolvedValue(results); + const out = await setup()("/deleteItems", { + sessionId: "s", + paths: ["/a", "/b", "/a"], + }); + expect(m.moveMany.mock.calls[0][1]).toEqual(["/a", "/b"]); + expect(out.body.results).toEqual(results); + }); + + it("uses permanent removal only when permanent is true", async () => { + m.remove.mockResolvedValue([{ path: "/a" }]); + const call = setup(); + await call("/deleteItems", { + sessionId: "s", + paths: ["/a"], + permanent: true, + }); + expect(m.remove).toHaveBeenCalledTimes(1); + expect(m.moveMany).not.toHaveBeenCalled(); + }); + + it("reports an unavailable trash as 409", async () => { + m.moveMany.mockRejectedValue(new Error("no trash")); + const out = await setup()("/deleteItems", { + sessionId: "s", + paths: ["/a"], + }); + expect(out.status).toBe(409); + expect(out.body.trashUnavailable).toBe(true); + }); + + it("stops queued items after /deleteItems/cancel for that opId only", async () => { + let stop: () => boolean = () => false; + m.moveMany.mockImplementation(async (_s, _p, _c, shouldStop) => { + stop = shouldStop; + return []; + }); + const call = setup(); + await call("/deleteItems", { sessionId: "s", paths: ["/a"], opId: "op1" }); + expect(stop()).toBe(false); + expect((await call("/deleteItems/cancel", { sessionId: "s" })).status).toBe( + 400, + ); + await call("/deleteItems/cancel", { sessionId: "s", opId: "op1" }); + expect(stop()).toBe(true); + await call("/deleteItems", { sessionId: "s", paths: ["/a"], opId: "op2" }); + expect(stop()).toBe(false); + }); +}); diff --git a/tests/backend/permanent-delete.test.ts b/tests/backend/permanent-delete.test.ts new file mode 100644 index 0000000..25f4fc8 --- /dev/null +++ b/tests/backend/permanent-delete.test.ts @@ -0,0 +1,118 @@ +import { beforeEach, describe, expect, it, vi } from "vitest"; +import type { SSHSession } from "../../src/backend/session.js"; + +const s = vi.hoisted(() => ({ execBuffer: vi.fn(), execWithSudo: vi.fn() })); +vi.mock("../../src/backend/session.js", () => s); +import { + isRemovablePath, + removePermanently, +} from "../../src/backend/permanent-delete.js"; + +const NL = String.fromCharCode(10); +// Echo the nonce the script embeds, mapping each index to a status. +const reply = (statuses: string[]) => (_s: unknown, command: string) => { + const nonce = /([0-9a-f-]{36}) OK 0/.exec(command)![1]; + return Promise.resolve({ + stdout: Buffer.from( + statuses.map((v, i) => nonce + " " + v + " " + i).join(NL), + ), + stderr: "", + code: 0, + }); +}; +const item = (path: string, isDirectory = false) => ({ path, isDirectory }); +const session = (sudoPassword?: string) => + ({ sudoPassword }) as unknown as SSHSession; + +describe("removePermanently", () => { + beforeEach(() => { + s.execBuffer.mockReset(); + s.execWithSudo.mockReset(); + }); + + it("refuses roots and dot segments, including Windows aliases", async () => { + for (const p of ["/", "/..", "C:/../", "C:/folder/../../", "/a/./b", ""]) + expect(isRemovablePath(p)).toBe(false); + expect(isRemovablePath("/a/b")).toBe(true); + const r = await removePermanently(session(), [item("/"), item("C:/../")]); + expect(r.every((x) => x.error)).toBe(true); + expect(s.execBuffer).not.toHaveBeenCalled(); + }); + + it("batches into one command, uses rm -f for files and rm -rf for directories", async () => { + s.execBuffer.mockImplementation(reply(["OK", "FAIL", "OK"])); + const r = await removePermanently(session(), [ + item("/a"), + item("/b"), + item("/c", true), + ]); + expect(s.execBuffer).toHaveBeenCalledTimes(1); + const cmd = s.execBuffer.mock.calls[0][1] as string; + expect(cmd).toContain("rm -f -- "); + expect(cmd).toContain("rm -rf -- "); + expect(r.map((x) => !!x.error)).toEqual([false, true, false]); + }); + + it("ignores status lines without the nonce (forged output)", async () => { + s.execBuffer.mockResolvedValue({ + stdout: Buffer.from("OK 0" + NL), + stderr: "OK 0", + code: 0, + }); + const r = await removePermanently(session(), [item("/a")]); + expect(r[0].error).toBe("Delete failed"); + }); + + it("retries only denied paths with sudo; flags needsSudo without a password", async () => { + s.execBuffer.mockImplementation(reply(["OK", "DENIED", "FAIL"])); + s.execWithSudo.mockImplementation(reply(["OK"])); + const r = await removePermanently(session("pw"), [ + item("/a"), + item("/b"), + item("/c"), + ]); + expect(s.execWithSudo).toHaveBeenCalledTimes(1); + expect(s.execWithSudo.mock.calls[0][1]).not.toContain("/c"); + expect(r.map((x) => x.error)).toEqual([ + undefined, + undefined, + "Delete failed", + ]); + s.execBuffer.mockImplementation(reply(["OK", "DENIED"])); + const r2 = await removePermanently(session(), [item("/a"), item("/b")]); + expect(r2[1]).toMatchObject({ + error: "Permission denied", + needsSudo: true, + }); + }); + + it("does not start sudo after cancel and reports Cancelled", async () => { + let stopped = false; + s.execBuffer.mockImplementation(async (...a: [unknown, string]) => { + stopped = true; + return reply(["DENIED"])(...a); + }); + const r = await removePermanently( + session("pw"), + [item("/a")], + () => stopped, + ); + expect(s.execWithSudo).not.toHaveBeenCalled(); + expect(r[0].error).toBe("Cancelled"); + const r2 = await removePermanently(session(), [item("/a")], () => true); + expect(r2[0].error).toBe("Cancelled"); + }); + + it("classifies denial by rm's error suffix, not by file names", async () => { + s.execBuffer.mockImplementation(async (_s: unknown, command: string) => { + expect(command).toContain('grep -q ": Permission denied$"'); + expect(command).toContain("LC_ALL=C"); + return reply(["FAIL"])(_s, command); + }); + const r = await removePermanently(session("pw"), [ + item("/tmp/Permission denied"), + ]); + expect(s.execWithSudo).not.toHaveBeenCalled(); + expect(r[0].error).toBe("Delete failed"); + }); +});