From a1b37c682999b063f277486311129f2530877a8f Mon Sep 17 00:00:00 2001 From: LukeGus Date: Sat, 3 Oct 2026 15:45:01 -0500 Subject: [PATCH] fix: slow file deletes from the trash lookups on every delete (#1390) --- .../src/backend/operation-routes.ts | 22 ++++- .../file-manager/src/backend/trash-service.ts | 29 +++++- .../tests/backend/trash-service.test.ts | 95 +++++++++++++++++++ 3 files changed, 140 insertions(+), 6 deletions(-) create mode 100644 plugins/file-manager/tests/backend/trash-service.test.ts diff --git a/plugins/file-manager/src/backend/operation-routes.ts b/plugins/file-manager/src/backend/operation-routes.ts index 19fd1fa05..b2a3edc38 100644 --- a/plugins/file-manager/src/backend/operation-routes.ts +++ b/plugins/file-manager/src/backend/operation-routes.ts @@ -1,5 +1,6 @@ import type { Express, Request, Response } from "express"; import type { PluginContext } from "@termix/plugin-sdk/backend"; +import type { SFTPWrapper } from "ssh2"; import { execChannel, execWithSudo, @@ -24,6 +25,8 @@ type FileOperationRoutesDeps = { const TRASH_RETENTION_DAYS_KEY = "trashRetentionDays"; +const TRASH_PURGE_INTERVAL_MS = 60 * 60 * 1000; + export function registerFileOperationRoutes( app: Express, { ctx, sshSessions, verifySessionOwnership }: FileOperationRoutesDeps, @@ -39,6 +42,22 @@ 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(); + const purgeExpiredTrash = (session: SSHSession, sftp: SFTPWrapper) => { + const now = Date.now(); + if (now - (lastPurge.get(session) ?? 0) < TRASH_PURGE_INTERVAL_MS) return; + lastPurge.set(session, now); + void getTrashRetentionDays() + .then((days) => listTrash(sftp, days)) + .catch((error) => + ctx.log.warn( + `Could not purge expired trash: ${(error as Error).message}`, + ), + ); + }; + async function ownedSession( req: Request, res: Response, @@ -494,9 +513,8 @@ export function registerFileOperationRoutes( if (!permanent) { try { const sftp = await getSessionSftp(sshConn); - const retentionDays = await getTrashRetentionDays(); - await listTrash(sftp, retentionDays); const item = await moveToTrash(sftp, itemPath); + purgeExpiredTrash(sshConn, sftp); ctx.log.info( `Item moved to trash: ${sessionId} (${itemPath}, trashId=${item.id})`, ); diff --git a/plugins/file-manager/src/backend/trash-service.ts b/plugins/file-manager/src/backend/trash-service.ts index 6b0705df4..bf09e8e66 100644 --- a/plugins/file-manager/src/backend/trash-service.ts +++ b/plugins/file-manager/src/backend/trash-service.ts @@ -70,6 +70,8 @@ async function exists(sftp: SFTPWrapper, target: string) { } async function ensureDirectory(sftp: SFTPWrapper, target: string) { + // Usually there already: one round trip instead of one per segment. + if (await exists(sftp, target)) return; const normalized = target.replace(/\\/g, "/"); const root = normalized.startsWith("/") ? "/" : ""; const parts = normalized.split("/").filter(Boolean); @@ -95,13 +97,32 @@ async function removeTree(sftp: SFTPWrapper, target: string): Promise { await rmdir(sftp, target); } -async function trashPaths(sftp: SFTPWrapper) { +type TrashDirs = { root: string; files: string; info: string }; + +// The trash folders of a channel do not move while it is open, and finding +// them costs several round trips, which made every delete slow on a remote +// host (#1390). +const trashDirsByChannel = new WeakMap>(); + +function trashPaths(sftp: SFTPWrapper): Promise { + let dirs = trashDirsByChannel.get(sftp); + if (!dirs) { + dirs = findTrashPaths(sftp); + trashDirsByChannel.set(sftp, dirs); + dirs.catch(() => trashDirsByChannel.delete(sftp)); + } + return dirs; +} + +async function findTrashPaths(sftp: SFTPWrapper): Promise { const home = await call((done) => sftp.realpath(".", done)); const root = path.posix.join(home.replace(/\\/g, "/"), TRASH_DIR); const files = path.posix.join(root, "files"); const info = path.posix.join(root, "info"); - await ensureDirectory(sftp, files); - await ensureDirectory(sftp, info); + await Promise.all([ + ensureDirectory(sftp, files), + ensureDirectory(sftp, info), + ]); return { root, files, info }; } @@ -124,7 +145,7 @@ function publicItem(item: StoredTrashItem): TrashItem { async function readStoredItem( sftp: SFTPWrapper, - dirs: { root: string; files: string; info: string }, + dirs: TrashDirs, id: string, ): Promise { if (!ID_PATTERN.test(id)) throw new Error("Invalid trash item id"); diff --git a/plugins/file-manager/tests/backend/trash-service.test.ts b/plugins/file-manager/tests/backend/trash-service.test.ts new file mode 100644 index 000000000..fcfa37bc5 --- /dev/null +++ b/plugins/file-manager/tests/backend/trash-service.test.ts @@ -0,0 +1,95 @@ +import { describe, expect, it, vi } from "vitest"; +import type { SFTPWrapper } from "ssh2"; +import { listTrash, moveToTrash } from "../../src/backend/trash-service.js"; + +/** An in-memory SFTP channel that counts every request. */ +function fakeSftp() { + const dirs = new Set(["/", "/home", "/home/me"]); + const files = new Map([["/home/me/a.txt", Buffer.from("a")]]); + const calls: string[] = []; + const stats = (target: string) => + dirs.has(target) || files.has(target) + ? { + isDirectory: () => dirs.has(target), + size: files.get(target)?.length ?? 0, + } + : null; + const sftp = { + realpath: vi.fn((_p: string, done: (e?: Error, v?: string) => void) => { + calls.push("realpath"); + done(undefined, "/home/me"); + }), + stat: vi.fn((target: string, done: (e?: Error, v?: unknown) => void) => { + calls.push("stat"); + const found = stats(target); + done(found ? undefined : new Error("ENOENT"), found ?? undefined); + }), + lstat: vi.fn((target: string, done: (e?: Error, v?: unknown) => void) => { + calls.push("lstat"); + const found = stats(target); + done(found ? undefined : new Error("ENOENT"), found ?? undefined); + }), + mkdir: vi.fn((target: string, done: (e?: Error) => void) => { + calls.push("mkdir"); + dirs.add(target); + done(); + }), + rename: vi.fn((from: string, to: string, done: (e?: Error) => void) => { + calls.push("rename"); + files.set(to, files.get(from)!); + files.delete(from); + done(); + }), + writeFile: vi.fn( + (target: string, data: string, done: (e?: Error) => void) => { + calls.push("writeFile"); + files.set(target, Buffer.from(data)); + done(); + }, + ), + readdir: vi.fn((target: string, done: (e?: Error, v?: unknown) => void) => { + calls.push("readdir"); + const prefix = `${target}/`; + done( + undefined, + [...files.keys()] + .filter((name) => name.startsWith(prefix)) + .map((name) => ({ filename: name.slice(prefix.length) })), + ); + }), + readFile: vi.fn((target: string, done: (e?: Error, v?: Buffer) => void) => { + calls.push("readFile"); + done(undefined, files.get(target)); + }), + unlink: vi.fn((target: string, done: (e?: Error) => void) => { + calls.push("unlink"); + files.delete(target); + done(); + }), + }; + return { sftp: sftp as unknown as SFTPWrapper, calls, files }; +} + +describe("trash", () => { + it("finds the trash folders once per channel", async () => { + const { sftp, calls, files } = fakeSftp(); + files.set("/home/me/b.txt", Buffer.from("b")); + + await moveToTrash(sftp, "/home/me/a.txt"); + const first = calls.length; + calls.length = 0; + await moveToTrash(sftp, "/home/me/b.txt"); + + expect(calls.filter((call) => call === "realpath")).toHaveLength(0); + expect(calls.filter((call) => call === "mkdir")).toHaveLength(0); + // lstat, rename and the metadata write. + expect(calls.length).toBeLessThan(first); + expect(calls).toEqual(["lstat", "rename", "writeFile"]); + }); + + it("lists what was moved", async () => { + const { sftp } = fakeSftp(); + const item = await moveToTrash(sftp, "/home/me/a.txt"); + expect(await listTrash(sftp, 7)).toEqual([item]); + }); +});