From 96d1773249df9e5bc865dcc927402c0c9ccd90a4 Mon Sep 17 00:00:00 2001 From: "Syring, Nikolas" Date: Mon, 27 Apr 2026 05:46:52 +0200 Subject: [PATCH] fix(server): idempotent fileDelete({ifExists: true}) for cache cleanup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Some fileDelete callers are naturally idempotent — pruning alternative thumbnail formats, cleaning up stale cache files, removing legacy widget paths. Today they wrap each call in try/catch + isNotFoundError because the server always returns 404 when the path is gone. The 404 they then suppress is still logged in DevTools by the browser before JavaScript can intercept it (DevTools cannot be silenced from JS), and the failed response goes through installFetchProxy's retry path which on a slow filesystem produces a brief UI hitch during rapid widget moves. Add an opt-in `ifExists: true` option that puts DELETE on the side of RFC 7231 idempotency: paths that resolve to nothing on disk return 200 with the path listed under `skipped` instead of throwing 404. Strict semantics remain the default so user-initiated deletes (file explorer UI, single-source-of-truth cleanups) still surface a real "this resource is gone" diagnostic. Server side: - normalizeDeleteRequests now reads options.ifExists; missing paths go into skipped[] instead of throwing 404. Other resolution failures (empty path, ambiguous path, write-permission, public-only path) still throw regardless. - deleteAppPaths surfaces skipped on the result envelope when non-empty and only calls recordAppPathMutations when at least one path was actually deleted. - deleteAppPath returns {path: null, skipped: [...]} when an ifExists call's only path was missing, preserving the singular contract. - file_delete API endpoint reads payload.ifExists and forwards it. Client side: - createFileDeleteRequest accepts a second positional argument for the bare-path/array forms and an ifExists field on the object forms. Strict bodies are byte-identical to today. - fileDelete JSDoc documents the option. Migrated callers: - thumbnail_experiment/deleteThumbnailPathIfExists: drops try/catch, calls fileDelete(path, {ifExists: true}). isNotFoundError helper removed from this file (no other usages there). - spaces/storage.js/deleteAppPathIfExists: drops try/catch, returns boolean by reading result.skipped from the server. Strict callers unchanged: removeSpace, removeWidgets, the batch fileDelete from paired write/delete in storage.js, module_remove, and the file_explorer UI delete. Concrete reproduction this fixes: every widget move on Linux generated a red `POST /api/file_delete 404` in DevTools console. The trigger was the alternative-format thumbnail pruning calling fileDelete on a sibling file that almost never exists. With ifExists, the call is a clean 200 with skipped, no console entry, no fetch-proxy retry path. server/api/AGENTS.md documents the new option alongside the existing file_write operation-modes documentation. --- .../_all/mod/_core/framework/js/api-client.js | 32 +++++++- app/L0/_all/mod/_core/spaces/storage.js | 18 ++--- .../spaces/thumbnail_experiment/index.js | 24 +++--- server/api/AGENTS.md | 1 + server/api/file_delete.js | 7 ++ server/lib/customware/file_access.js | 76 +++++++++++++++---- 6 files changed, 112 insertions(+), 46 deletions(-) diff --git a/app/L0/_all/mod/_core/framework/js/api-client.js b/app/L0/_all/mod/_core/framework/js/api-client.js index f3d4a066..5ffcc2f7 100644 --- a/app/L0/_all/mod/_core/framework/js/api-client.js +++ b/app/L0/_all/mod/_core/framework/js/api-client.js @@ -440,11 +440,23 @@ function createFileWriteRequest(pathOrFiles, content, encoding) { }; } -function createFileDeleteRequest(pathOrPaths) { +function createFileDeleteRequest(pathOrPaths, options) { + const optionsObject = isPlainObject(options) ? options : {}; + // `ifExists: true` requests idempotent delete semantics: paths that no + // longer exist on disk return `200` with the path listed under `skipped` + // instead of `404`. Useful for cache-cleanup callers (thumbnail pruning, + // stale layout files) that previously had to wrap each call in + // try/catch + isNotFoundError. Per RFC 7231 DELETE is idempotent, but we + // only opt into idempotency at the call site so strict callers + // (user-initiated file-explorer delete) keep their authoritative 404. + const ifExistsFlag = optionsObject.ifExists === true; + const ifExistsBody = ifExistsFlag ? { ifExists: true } : {}; + if (Array.isArray(pathOrPaths)) { return { method: "POST", body: { + ...ifExistsBody, paths: pathOrPaths } }; @@ -454,6 +466,8 @@ function createFileDeleteRequest(pathOrPaths) { return { method: "POST", body: { + ...ifExistsBody, + ...(typeof pathOrPaths.ifExists === "boolean" ? { ifExists: pathOrPaths.ifExists } : {}), paths: pathOrPaths.paths } }; @@ -463,6 +477,8 @@ function createFileDeleteRequest(pathOrPaths) { return { method: "POST", body: { + ...ifExistsBody, + ...(typeof pathOrPaths.ifExists === "boolean" ? { ifExists: pathOrPaths.ifExists } : {}), path: pathOrPaths.path } }; @@ -471,6 +487,7 @@ function createFileDeleteRequest(pathOrPaths) { return { method: "POST", body: { + ...ifExistsBody, path: pathOrPaths } }; @@ -1033,11 +1050,20 @@ export function createApiClient(options = {}) { * `L2/alice/old-folder/`, and the `~` or `~/...` shorthand for the current * user's `L2//...` path. Directory deletes are recursive. * + * Pass `{ ifExists: true }` (either on the input object for the + * `{path}` / `{paths}` forms or as a second positional argument for + * the bare-path / array forms) to opt into idempotent semantics: paths + * that do not exist return `200` and appear under `skipped` on the + * result instead of throwing `404`. Without `ifExists` the call stays + * strict so callers that need authoritative 404 (file-explorer UI, + * single-source-of-truth cleanups) keep their existing behaviour. + * * @param {string | FileDeleteInput[] | FileDeleteBatchOptions | FileDeleteInput} pathOrPaths + * @param {{ ifExists?: boolean }} [options] * @returns {Promise} */ - async function fileDelete(pathOrPaths) { - return call("file_delete", createFileDeleteRequest(pathOrPaths)); + async function fileDelete(pathOrPaths, options) { + return call("file_delete", createFileDeleteRequest(pathOrPaths, options)); } /** diff --git a/app/L0/_all/mod/_core/spaces/storage.js b/app/L0/_all/mod/_core/spaces/storage.js index 853f8b45..33a7b40e 100644 --- a/app/L0/_all/mod/_core/spaces/storage.js +++ b/app/L0/_all/mod/_core/spaces/storage.js @@ -1010,18 +1010,12 @@ function createHtmlRendererSource(htmlSource) { async function deleteAppPathIfExists(path) { const runtime = ensureSpaceRuntime(); - - try { - await runtime.api.fileDelete(path); - } catch (error) { - if (isNotFoundError(error)) { - return false; - } - - throw error; - } - - return true; + // Idempotent delete: server returns 200 with the path under `skipped` + // when the file is already gone. The `skipped` field is what tells us + // whether anything was actually removed, so callers that branch on + // "did we delete something" still get a meaningful boolean. + const result = await runtime.api.fileDelete(path, { ifExists: true }); + return Array.isArray(result?.skipped) ? result.skipped.length === 0 : true; } function normalizeLegacyFunctionSource(sourceText) { diff --git a/app/L0/_all/mod/_core/spaces/thumbnail_experiment/index.js b/app/L0/_all/mod/_core/spaces/thumbnail_experiment/index.js index 1591a414..ddf75918 100644 --- a/app/L0/_all/mod/_core/spaces/thumbnail_experiment/index.js +++ b/app/L0/_all/mod/_core/spaces/thumbnail_experiment/index.js @@ -23,11 +23,6 @@ function normalizeSpaceThumbnailId(value) { return String(value || "").trim().replace(/^\/+|\/+$/gu, ""); } -function isNotFoundError(error) { - const message = String(error?.message || "").toLowerCase(); - return message.includes("status 404") || message.includes("file not found") || message.includes("path not found"); -} - function ensureThumbnailRuntime() { if (!globalThis.space?.api?.fileDelete || !globalThis.space?.api?.fileWrite) { throw new Error("Space thumbnail capture requires the authenticated app-file runtime."); @@ -278,16 +273,15 @@ async function blobToBase64(blob) { async function deleteThumbnailPathIfExists(path) { const runtime = ensureThumbnailRuntime(); - - try { - await runtime.api.fileDelete(path); - } catch (error) { - if (isNotFoundError(error)) { - return; - } - - throw error; - } + // Idempotent delete: pass `ifExists: true` so the server returns 200 + // and lists the path under `skipped` instead of throwing 404 when the + // alternative-format thumbnail file (e.g. the `thumbnail.jpg` sibling + // of a `thumbnail.webp` that has always existed alone) was never on + // disk. Without this, every widget move generates a 404 entry in the + // DevTools console because the browser logs failed responses before + // JavaScript can suppress them, and triggers the fetch-proxy retry + // path which is what surfaces as a UI hitch during rapid moves. + await runtime.api.fileDelete(path, { ifExists: true }); } export function buildSpaceThumbnailPath(spaceId, fileName = SPACE_THUMBNAIL_WEBP_FILE) { diff --git a/server/api/AGENTS.md b/server/api/AGENTS.md index 0475d294..27c4248f 100644 --- a/server/api/AGENTS.md +++ b/server/api/AGENTS.md @@ -68,6 +68,7 @@ Current rules: - `file_paths` also accepts an optional explicit `maxLayer` body or query value when module-oriented discovery should ignore higher writable layers; this is used by the admin agent skill catalog so firmware-backed `ext/skills/` files are not shadowed by L1 or L2 customware - batch operations validate all targets before any mutation begins - `file_write` still defaults to full-file replacement, but object-form writes also support `operation: "append"`, `"prepend"`, or `"insert"`; insert writes accept exactly one of `line`, `before`, or `after`, use the first literal `before` or `after` match, and require `utf8` encoding +- `file_delete` defaults to strict semantics (404 when the path is gone) so user-initiated deletes can surface a real "this resource is gone" diagnostic; cache-cleanup callers can pass `ifExists: true` in the body to opt into RFC 7231 idempotent delete, in which case missing paths are returned in `skipped` instead of throwing 404 - when `USER_FOLDER_SIZE_LIMIT_BYTES` is positive, `file_write`, `file_copy`, `file_move`, `file_delete`, and module removal through `file_access.js` enforce the per-`L2//` folder quota before mutation; the shared quota helper should derive current totals and subtree deltas from indexed `sizeBytes` metadata instead of rescanning the whole user tree, and quota errors return `413` - single-file or single-folder copy and move requests must keep working when request plumbing omits `entries`; only real batch calls should forward an `entries` array to the shared helper - endpoint-specific validation should stay thin and reuse the shared helper contract diff --git a/server/api/file_delete.js b/server/api/file_delete.js index 07f7c457..f9b6bf9c 100644 --- a/server/api/file_delete.js +++ b/server/api/file_delete.js @@ -24,10 +24,17 @@ async function handleDelete(context) { headers: context.headers, requestUrl: context.requestUrl }); + // `ifExists: true` opts into RFC 7231 idempotent DELETE semantics: + // missing paths resolve to a 200 response with the path listed under + // `skipped` instead of throwing 404. Strict callers (default) keep + // their authoritative 404 so user-initiated deletes can still surface + // a "this resource is gone" diagnostic to the UI. + const ifExists = payload.ifExists === true; try { return await runTrackedMutation(context, async () => { const options = { + ifExists, maxLayer, path: readPath(context), paths: payload.paths, diff --git a/server/lib/customware/file_access.js b/server/lib/customware/file_access.js index 42ebbe13..26ed96b0 100644 --- a/server/lib/customware/file_access.js +++ b/server/lib/customware/file_access.js @@ -1309,8 +1309,15 @@ function normalizeDeleteRequests(options = {}) { runtimeParams: options.runtimeParams, username: options.username }); + // Per RFC 7231 DELETE is idempotent. Callers that want that idempotency + // pass `ifExists: true`, in which case paths that resolve to nothing on + // disk are recorded as `skipped` and not reported as 404. Strict callers + // (default) still get the authoritative 404 so user-initiated deletes + // surface a real "this resource is gone" diagnostic. + const ifExists = options.ifExists === true; const entries = normalizeDeleteEntries(options); - const requests = entries.map((entry) => { + const skipped = []; + const requests = entries.flatMap((entry) => { const request = isPlainObject(entry) ? entry : { path: entry }; const requestedPath = String(request.path || "").trim(); @@ -1324,13 +1331,18 @@ function normalizeDeleteRequests(options = {}) { ); if (!resolvedPath.projectPath || !resolvedPath.exists) { + if (ifExists) { + skipped.push(requestedPath); + return []; + } + throw createHttpError(`Path not found: ${requestedPath}`, 404); } ensurePublicAppProjectPath(resolvedPath.projectPath); ensureWritableProjectPath(resolvedPath.projectPath, accessController); - return { + return [{ absolutePath: createAbsolutePath( String(options.projectRoot || ""), resolvedPath.projectPath, @@ -1339,7 +1351,7 @@ function normalizeDeleteRequests(options = {}) { isDirectory: resolvedPath.isDirectory, path: toAppRelativePath(resolvedPath.projectPath), projectPath: resolvedPath.projectPath - }; + }]; }); requests.forEach((request, index) => { @@ -1360,19 +1372,28 @@ function normalizeDeleteRequests(options = {}) { }); }); - return requests; + return { requests, skipped }; } function deleteAppPaths(options = {}) { - const requests = normalizeDeleteRequests(options); + const { requests, skipped } = normalizeDeleteRequests(options); const quotaDeltas = getDeleteQuotaDeltas(options, requests); const quotaPlan = createQuotaPlan(options, quotaDeltas); + // With `ifExists: true` the caller has already accepted that the target + // may be gone, so close the second race window symmetrically: when the + // path index says the file is there but it has been removed externally + // since the last scan, `fs.rmSync` would otherwise throw `ENOENT` and + // surface as a 500. `force: true` makes that case the same no-op the + // pathIndex-says-missing branch already handles in normalizeDeleteRequests. + // Strict callers keep `force: false` so they still get the authoritative + // "this resource is gone" signal. + const rmSyncForce = options.ifExists === true; let paths; try { paths = requests.map((request) => { fs.rmSync(request.absolutePath, { - force: false, + force: rmSyncForce, recursive: request.isDirectory }); return request.path; @@ -1384,24 +1405,47 @@ function deleteAppPaths(options = {}) { applyUserFolderQuotaPlan(quotaPlan); - recordAppPathMutations( - { - projectRoot: options.projectRoot, - quotaCacheUpdated: true, - runtimeParams: options.runtimeParams - }, - requests.map((request) => request.projectPath) - ); + // Gate on `paths.length`, not `requests.length`: the invariant we want is + // "at least one path mutated on disk". `paths` is set after `fs.rmSync` + // succeeds, so it is the authoritative count even when the loop above + // throws partway through (the catch re-throws and we never reach here). + if (paths.length > 0) { + recordAppPathMutations( + { + projectRoot: options.projectRoot, + quotaCacheUpdated: true, + runtimeParams: options.runtimeParams + }, + requests.map((request) => request.projectPath) + ); + } - return { + const result = { count: paths.length, paths }; + + if (skipped.length > 0) { + result.skipped = skipped; + } + + return result; } function deleteAppPath(options = {}) { + const result = deleteAppPaths(options); + // Match the singular-form contract: the legacy result had a single + // `path` field. With ifExists, a missing path resolves to skipped so + // `paths[0]` is undefined; surface that case explicitly. + if (result.paths.length === 0 && Array.isArray(result.skipped) && result.skipped.length > 0) { + return { + path: null, + skipped: result.skipped + }; + } + return { - path: deleteAppPaths(options).paths[0] + path: result.paths[0] }; }