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] }; }