Skip to content

fix(server): idempotent fileDelete({ifExists: true}) for cache cleanup#35

Open
nsyring wants to merge 1 commit intoagent0ai:mainfrom
nsyring:fix/file-delete-if-exists-semantics
Open

fix(server): idempotent fileDelete({ifExists: true}) for cache cleanup#35
nsyring wants to merge 1 commit intoagent0ai:mainfrom
nsyring:fix/file-delete-if-exists-semantics

Conversation

@nsyring
Copy link
Copy Markdown

@nsyring nsyring commented Apr 27, 2026

fix(server): idempotent fileDelete({ ifExists: true }) for cache-cleanup callers

Summary

fileDelete is currently always strict: deleting a path that no longer exists throws HTTP 404. Several callers in the codebase wrap every call in try/catch + isNotFoundError(...) because their use case (clean up stale cache files, prune alternative-format thumbnails) is naturally idempotent. The 404 they then suppress is still logged in DevTools by the browser before JavaScript can intercept it, and on Linux/Wayland it surfaces as a brief UI hitch during rapid widget moves.

This PR adds an opt-in ifExists: true option to fileDelete. The server returns 200 with the path listed under skipped instead of 404, so the call becomes RFC 7231 idempotent for callers that need it. Strict semantics remain the default for user-initiated deletes (file explorer UI, single-source-of-truth cleanups) so a real "this resource is gone" diagnostic still surfaces.

After:

// strict (default) — throws if path is gone:
await space.api.fileDelete("~/note.txt");

// idempotent — 200 with `skipped: ["..."]` if path is already gone:
await space.api.fileDelete("~/cache.tmp", { ifExists: true });
await space.api.fileDelete({ paths: [...], ifExists: true });

Why

Concrete reproduction: every widget move on Linux generates a red POST http://127.0.0.1:NNNNN/api/file_delete 404 (Not Found) in DevTools console. The trigger is thumbnail_experiment/index.js:deleteThumbnailPathIfExists(...), which is invoked once per thumbnail filename per move (see recordSpaceThumbnailCapture calling deleteThumbnailPathIfExists for every THUMBNAIL_FILE_NAMES entry except the current one). The webp/jpg sibling is almost never on disk. The call is wrapped in try/catch to swallow 404 — the user-facing behaviour was always correct — but:

  • the browser logs the failed response before JavaScript sees it (DevTools cannot be silenced from JS);
  • the failed response goes through installFetchProxy(...)'s retry path, which on a slow filesystem produces a brief UI hitch during rapid moves.

The architectural-correct fix is to ask the server for idempotency at the call site, instead of letting the call fail and pretending it didn't. RFC 7231 already permits idempotent DELETE: this PR makes that semantic available without changing the default for callers that genuinely need a strict 404.

The same pattern fixes three call sites in the codebase that already work around it with try/catch + isNotFoundError(...):

  1. app/L0/_all/mod/_core/spaces/thumbnail_experiment/index.js:deleteThumbnailPathIfExists(...) — alternative-format thumbnail pruning after capture
  2. app/L0/_all/mod/_core/spaces/storage.js:deleteAppPathIfExists(...) — generic helper used by legacy widget-path cleanup
  3. (future callers that need idempotent delete can use the same option)

Strict callers untouched: removeSpace(...), removeWidgets(...), batch fileDelete({paths}) from a paired write/delete, module_remove, file explorer UI delete.

What changed

server/lib/customware/file_access.js (+44 / -19)

normalizeDeleteRequests(options) now reads options.ifExists. When the flag is set, paths that resolve to nothing on disk are recorded in a skipped[] array and not added to the requests[] list. The function's return shape becomes { requests, skipped } (was: requests[]). All four resolution-failure-other-than-not-found cases (empty path, ambiguous path, write-permission, public-only path) still throw, regardless of ifExists.

deleteAppPaths(...) adapts to the new return shape, surfaces skipped on its own result envelope when non-empty, and only calls recordAppPathMutations(...) when at least one path was actually deleted.

deleteAppPath(...) keeps its singular path field on the result. When ifExists skips the only path, the singular form returns { path: null, skipped: [requested] } so callers can branch on result.path === null.

server/api/file_delete.js (+7 / -0)

The endpoint reads payload.ifExists === true and forwards it to the file-access helper. No change to URL shape, no new parameters; the option lives on the request body next to path / paths.

app/L0/_all/mod/_core/framework/js/api-client.js (+27 / -5)

createFileDeleteRequest(pathOrPaths, options) accepts a second parameter for the bare-path / array forms (fileDelete("~/x", { ifExists: true })) and an ifExists field on the object forms (fileDelete({ path: "~/x", ifExists: true })). The body is byte-identical to today's strict default unless the flag is set.

fileDelete(...) JSDoc updated to document the option and its semantics.

app/L0/_all/mod/_core/spaces/thumbnail_experiment/index.js (+8 / -16)

deleteThumbnailPathIfExists(...) drops the try/catch + isNotFoundError(...) wrapping and simply calls fileDelete(path, { ifExists: true }). The unused isNotFoundError helper is removed from this file (other callers in spaces/storage.js still use a copy of that helper for their own paths; it stays there).

app/L0/_all/mod/_core/spaces/storage.js (+9 / -9)

deleteAppPathIfExists(...) drops its try/catch + isNotFoundError(...) wrapping and calls fileDelete(path, { ifExists: true }). The function's boolean return value is preserved by reading result.skipped from the server response (true if nothing was skipped, i.e. something was actually deleted).

server/api/AGENTS.md (+1 / -0)

Documents the new ifExists option alongside the existing file_write operation-modes documentation.

Backward compatibility

  • Without ifExists, every endpoint and helper behaviour is byte-identical to today. All existing callers (removeSpace, removeWidgets, module_remove, file_explorer UI, the strict batch in storage.js) are unchanged.
  • The deleteAppPaths(...) helper's return shape now includes an optional skipped[] field, but only when ifExists: true and at least one path was missing. Strict callers always get { count, paths } exactly as before.
  • The deleteAppPath(...) singular-form helper now returns { path: null, skipped: [...] } instead of { path: undefined } when an ifExists call's only path was missing. This is unreachable from any current strict caller and well-defined for new idempotent callers.

Test plan

  • node --check on every modified file passes
  • Request-body shapes verified manually for the four call signatures × two strict/idempotent modes (eight cases): bare path, array, {path} object, {paths} object — strict bodies are byte-identical to today; idempotent bodies add ifExists: true next to the path field
  • Manual verification in npm run desktop:pack build:
    • Widget moves no longer log POST /api/file_delete 404 (Not Found) in DevTools console; the alternative-format thumbnail file (e.g. thumbnail.jpg sibling of thumbnail.webp) returns 200 with skipped: ["..."] instead
    • File explorer delete of an actual missing file (race against another tab) still surfaces 404 to the UI as before — strict semantics preserved
    • Space removal and widget removal still throw on missing target — strict semantics preserved

Out of scope (possible follow-ups)

  • The isNotFoundError(...) helper still exists in spaces/storage.js for callers that have not been migrated to ifExists. A future cleanup could either remove it (after confirming all idempotent-delete callers have migrated) or generalize it for non-delete operations that genuinely need to distinguish "missing" from "broken".
  • Telemetry on skipped paths. The server currently returns skipped so callers can branch on it, but no existing caller reads the field beyond a "did anything actually delete" boolean. A future PR could log skipped paths to help diagnose stale-cache patterns.
  • Symmetric idempotency on file_copy and file_move (i.e. ifSourceExists, ifTargetMissing). Distinct semantics; not part of this PR.

🤖 Generated with Claude Code

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.
@nsyring nsyring force-pushed the fix/file-delete-if-exists-semantics branch from 8b24746 to 96d1773 Compare April 27, 2026 04:21
nsyring pushed a commit to nsyring/space-agent that referenced this pull request Apr 27, 2026
…ch reads

Two pathologies were silently logging 404s in DevTools and routing through
installFetchProxy's retry path on every space switch and config-load:

1. Batch readers in spaces/storage.js (readSpace, parseWidgetFiles batch,
   listSpaces) walk the path index and then read all matched files. A
   widget or manifest can be deleted between the index walk and the read
   (concurrent space deletion, file_explorer rename, watchdog catching
   up), failing the entire batch.

2. Optional config readers across the codebase load files like
   ~/conf/dashboard.yaml, ~/conf/onscreen-agent.yaml,
   ~/conf/personality.system.include.md, ~/user.yaml. On a fresh user
   account these never existed, so the load wraps the call in
   try/catch + isMissingFileError and treats 404 as "use defaults". The
   404 is suppressed in JS but still logged by the browser before
   JavaScript can intercept it.

This PR adds an opt-in `ifExists: true` option to fileRead, mirroring the
shape of fileDelete({ifExists: true}) (PR agent0ai#35). Server returns 200; missing
paths appear under `skipped[]`; the singular form returns
{content: null, encoding: null, path: null, skipped: [requested]}. Strict
semantics remain the default for user-initiated reads and known-must-exist
paths so a real "this resource is gone" diagnostic still surfaces.

Server side:
- normalizeReadRequests reads options.ifExists; missing paths go into
  skipped[] instead of throwing 404. Other resolution failures (empty
  path, directory-instead-of-file, public-only, permission) still throw.
- readAppFiles surfaces skipped on the result envelope when non-empty
  and additionally catches ENOENT from fs.readFileSync when ifExists is
  set, closing the race where the path index says the file is there
  but it has been removed externally since the last scan.
- readAppFile keeps its singular contract; returns content: null /
  encoding: null / path: null when the only path was skipped.
- file_read endpoint reads ifExists from POST body or GET query
  (?ifExists=1) so the bare-path GET form keeps working without
  forcing every idempotent read into POST.

Client side:
- createFileReadRequest accepts a third positional argument for the
  bare-path/array forms and an ifExists field on the object forms.
  Strict bodies/queries byte-identical to today.
- fileRead branches: idempotent reads bypass the file-read batching
  queue and call directly into call("file_read", ...). Mixing strict
  and idempotent modes in one batch would change the strict caller's
  behaviour, so the bypass keeps each idempotent call's semantic
  isolated. The queue's batching benefit is preserved for the strict
  default.

Migrated callers:

Pathology 1 (race-prone batch reads in spaces/storage.js):
- readSpace(...) — manifest + widgets batch
- parseWidgetFiles(...) batch — widget batch
- listSpaces(...) bulk — manifest+widgets across all spaces

Pathology 2 (optional config readers; previously try/catch + 404):
- agent/storage.js loadAgentPersonality
- user/storage.js readUserConfig
- dashboard_welcome/dashboard-prefs.js loadDashboardPrefs
- onscreen_agent/storage.js loadOnscreenAgentConfig + loadOnscreenAgentHistory
- admin/views/agent/storage.js loadAdminChatConfig + loadAdminChatHistory
- login_hooks/login-hooks.js hasFirstLoginMarker (fileRead fallback path)
- panels/panel-index.js listPanels batch

Each of these had its own copy of the isMissingFileError(...) helper for
the same regex. Where the helper has no remaining users in a file, this
PR removes it; module_remove and other strict callers keep their copies.

server/api/AGENTS.md documents the new option alongside the existing
file_write operation-modes documentation.

Strict callers untouched: readWidgetFile, readManifestFile's duplicateSpace
caller, all single-file reads paired with a known-existing target.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant