docs(folders): add spec for keeping files when a folder delete rolls back (#37782)

[zJaaal]

Spec-Kit specification (PR 1 of 2) for fixing #37782. This PR carries specs/37782-folder-delete-binaries/spec.md only; the implementation lands in a second PR stacked on this one once the spec is approved.

The problem

When a folder delete fails partway through the folder's tree, the database rolls back but the files it already reached are gone from disk. The folder and every file still show in Content Drive, but the files the delete had reached return 404, and the author is only told the delete failed. Nothing can recover them.

It happens with the context-menu delete and the bulk delete alike. A permission refusal on one subfolder is enough to trigger it, and so is content locked by another user deeper in the tree, or any database error after the first file is destroyed.

The fix the spec proposes

Remove files from disk only after the database commits.

• Today the content destroy deletes a file's binary directory, resized-image cache and metadata immediately (ESContentletAPIImpl.deleteBinaryFiles), outside the transaction.
• The spec registers that removal with the existing after-commit hook (HibernateUtil.addCommitListener). A delete that commits removes the files as today; a delete that rolls back leaves them in place.
• With no transaction open, the hook runs immediately, so non-transactional callers see no change.
• The change is in the shared removal, so every way of destroying content gets the fix, not only folder delete.
• What to remove is worked out during the destroy; only the removal waits. (The metadata removal reads the content type's fields, which may be gone by commit time when a content type is deleted.)
• The removal runs synchronously right after the commit, not on the background listener thread, so the order of later writes is the same as today. This matters on push publishing receivers, which keep the sender's inodes.
• A failed removal is caught and logged, so it cannot stop the commit's other after-commit work (such as index updates) or turn a committed delete into an error.
• Deleting a single version removes its metadata outside the shared removal today; that call is deferred too.

What does not change: folder delete still runs one transaction per top-level folder and stops a folder on the first refusal, and bulk delete still continues with the other selected folders. Paging the walk and the transaction decision stay in #37565.

Three smaller bulk delete fixes carried with it

The bulk folder duplicate already fixed each of these; bulk delete still has them.

1. A descendant is reported "covered by parent" even when the parent was never deleted. It will be decided once the parent's outcome is known, with descendants run after the selected folders that contain them.
2. A request with no body answers 500. It will answer 400 EMPTY_SELECTION, as {} already does.
3. Resolving each submitted path also lists the folder's whole contents. Paths will be resolved without that listing.

Worth a reviewer's attention

• Blast radius. The file-removal change applies to every content destroy (editor, workflows, site deletion, push publishing, plugins). Only the timing moves, from during the transaction to after its commit. The plan step will look for any caller that expects the files to be gone, or writes to the same path, before its own transaction commits.
• It amends an approved rule. The bulk delete spec decided descendants are skipped up front, from the submitted paths alone (FR-013 in specs/37063-bulk-folder-delete-backend/spec.md). Fix 1 replaces that with a run-time decision, the same amendment the bulk duplicate spec made in review on 2026-09-29. The amendment is recorded in this spec; the approved one is not edited.
• Not proposed: checking the whole tree before deleting. It would only prevent the permission case, not locks, database errors or changes made after the check.

How it will be verified

Integration tests that fail on today's code first: the file is still on disk (and its metadata readable) after a refused or locked folder delete, and a rolled-back destroy keeps its files for each of the four destroy paths, tested directly on the content API. Guard tests that pass before and after: a successful delete and a content type delete still clear the disk, a non-transactional destroy still removes immediately, and a failing removal does not break the commit. One test per parent outcome for fix 1. Plus a unit test and a Postman request for the no-body case.

🤖 Generated with Claude Code

[claude]

Claude finished @zJaaal's task in 1m 12s —— View job

---

Spec review — #37782 folder-delete binaries

This PR adds only specs/37782-folder-delete-binaries/spec.md (no code), so the review below checks whether the spec's technical premises hold against the current codebase rather than hunting for code bugs.

Code claims — all verified against the tree

• Immediate, in-transaction removal is real. deleteBinaryFiles calls destroyMetadata(contentlets) and then FileUtil.deltree(...) for the asset dir and the resized-image cache, synchronously (ESContentletAPIImpl.java:9407-9420). It is reached from the destroy paths at lines 3653, 3708, 3746. Matches the Root-Cause section.
• The after-commit hook exists and is already used for binaries. addCommitListener(() -> moveBinaryFilesToTrash(finalList, oldField)) is at ESContentletAPIImpl.java:1832, exactly the in-class precedent the spec cites.
• Async default and the sync escape hatch exist. ASYNC_COMMIT_LISTENERS defaults to true (HibernateUtil.java:1417) and DotSyncRunnable is a real listener type with an order (HibernateUtil.java:997-1006, used in ordering at 1186-1187, 1210-1211) — so "run synchronously, before the committing call returns" is achievable as described.
• The single-version metadata removal is outside the shared path. deleteVersion calls fileMetadataAPI.removeVersionMetadata(contentlet) directly (ESContentletAPIImpl.java:3748), not through deleteBinaryFiles — confirming the "needs the same deferral" detail.
• The three bulk-delete defects live in modern code (FolderBulkDeleteHelper.java, FolderBulkDeleteProcessor), as scoped.

The spec is accurate, well-scoped, and its blast-radius call-out (every content destroy, timing-only change, push-publishing receiver as the first caller to vet) is the right thing to flag for the plan.

Worth the plan's attention (not spec defects — carry into /speckit-plan)

• 🟡 Ordering vs. other after-commit listeners is asserted, not yet proven. In-scope says the removal "needs no fixed position … because none of that work reads the content's files or metadata," with the proof deferred to the plan. That is the correct place for it, but it is the load-bearing assumption of the whole fix: ESContentletAPIImpl registers many commit listeners around these destroy paths (lines 3546, 3968-3970, 4343, 6541, 7078, 7183, 9940) and destroy/delete also flush caches and update the index on commit. The plan's AC-003/AC-003b work should enumerate what each co-registered listener touches, since a DotSyncRunnable ordering regression would be silent.
• 🟡 Synchronous removal re-raising to the caller. The spec correctly notes that in synchronous mode an exception in one listener "reaches the caller as an error after the delete has already committed," and requires the removal to catch-and-log. Worth making explicit in the plan that the catch must wrap the entire deferred unit (files + cache + metadata + single-version metadata) so a partial failure in one still lets the rest and later listeners run — AC-003b covers the "later listener still runs" half but not the "earlier parts of this same removal still attempted" half.

Verification coverage

The AC set is strong: each data-loss AC fails on today's code first, the guard ACs (AC-003/AC-003a/AC-004) pin behaviour that is correct today, AC-003 is asserted on the calling thread with no polling (so a regression to background removal fails rather than hides), and the suite-registration / -Dmaven.build.cache.enabled=false / Tests run: N discipline from CLAUDE.md is spelled out. No gaps found.

No blocking issues. The spec is sound and ready for sign-off; the two 🟡 items are plan-phase follow-ups, not changes to this PR.

[zJaaal]

Thanks for the review. All three observations checked against the code:

• In-class precedent: confirmed, addCommitListener(() -> moveBinaryFilesToTrash(...)) at ESContentletAPIImpl.java:1832. Now cited in the Root-Cause section next to the index removal (0fd731f).
• Outermost commit: added to the Root-Cause section: the hook fires on the outermost commit, so a destroy inside a folder delete waits for the folder as a whole, and the plan confirms it against the nested-transaction handling (0fd731f).
• Collect, then remove: already stated in the spec: paths and metadata keys are collected at destroy time and only the removal waits for the commit (Root-Cause, first of the four details; In scope, second bullet). No change.

Written by Claude (Claude Code) on behalf of @zJaaal.

[zJaaal]

Second review addressed in 7342ddc:

• Metadata in database storage: checked, and the premise does not hold. DataBaseStoragePersistenceAPIImpl removes metadata on its own pooled connection (getConnection uses DbConnectionFactory.getDataSource().getConnection()), not the delete's transaction, so a rollback does not undo it today either. Every backend behaves the same, so deferring treats them uniformly. Recorded in Assumptions with the evidence.
• AC-003b framing: agreed. It now asserts the removal's own catch-and-log directly: the delete still reports success, and a listener registered later in the same transaction still runs. (Listeners are kept in insertion order, LinkedHashMap, so "later" is well defined.)
• Outermost commit per destroy path: the spec now says the plan proves it for each of the four destroy paths, since the fix depends on it.
• Nit, synchronous assertion: added to the verification method. AC-003 is asserted on the calling thread right after the call returns, with no waiting or polling.

Written by Claude (Claude Code) on behalf of @zJaaal.

[zJaaal]

Third review addressed in 3b49463:

• Outermost commit as the first plan check: already required by the spec (Root-Cause: the plan proves it for each of the four destroy paths, since the fix depends on it). No change.
• Order among after-commit listeners: the spec now states the removal needs no fixed position, because no other after-commit work reads the content's files or metadata. The plan confirms that, and pins the order explicitly if any listener turns out to depend on them.
• Catch-and-log covers metadata too: in scope and AC-003b now say the removal's error handling covers every part of it: files, resized-image cache, metadata, and the single version's metadata.

Written by Claude (Claude Code) on behalf of @zJaaal.

[github-actions]

dotbot code review:

• Reviewer: meta/muse-spark-1.3 (medium)
• Overall: patch is correct
• New findings this run: 0
• Prior unresolved dotbot findings still relevant: 0
• Active findings total: 0

Docs-only addition of specs/37782-folder-delete-binaries/spec.md; no code, behavior, or API changes to break existing tests.

Tip: comment with "/dotbot address comments" to attempt automated fixes for unresolved review threads.

_{reviewed by dotbot · meta/muse-spark-1.3 · medium}

[github-actions]

dotbot code review:

• Reviewer: ~z-ai/glm-latest (medium)
• Overall: patch is correct
• New findings this run: 0
• Prior unresolved dotbot findings still relevant: 0
• Active findings total: 0

The change adds only a specification document; no code is modified. I verified the spec's key factual claims against the repository: HibernateUtil.addCommitListener runs the listener immediately when no transaction is open and queues DotSyncRunnable synchronously on commit; ESContentletAPIImpl:1832 already defers moveBinaryFilesToTrash through the same hook; FolderBulkDeleteHelper.submit dereferences form.assetPaths() with no null check (missing body → NPE → 500, as the spec states); FolderBulkDeleteProcessor.resolve goes through WebAssetHelper.getAssetInfo; and the bulk-duplicate processor already carries the amended COVERED_BY_PARENT behavior the spec proposes to port. The described root cause, in-scope fix, and acceptance criteria are consistent with the actual code paths.

Tip: comment with "/dotbot address comments" to attempt automated fixes for unresolved review threads.

_{reviewed by dotbot · ~z-ai/glm-latest · medium}

[dotCMS-Machine-User]

✅ dotbot review: all reviewer models (meta/muse-spark-1.3, ~z-ai/glm-latest) agree — patch is correct.

_{approved automatically by dotbot}

[zJaaal]

Fourth review: no spec changes; both items are carried into /speckit-plan as the review suggests.

• Co-registered after-commit listeners: the plan enumerates what each listener registered around the destroy paths touches, to prove that none reads the content's files or metadata before relying on the removal having no fixed position.
• Isolation inside the removal: the plan makes each part of the deferred removal (files, resized-image cache, metadata, single-version metadata) its own try/catch, so a failure in one part still lets the other parts run, as well as later listeners. A failure there can only leave orphaned files behind, never lose data, which is why it is a plan-level detail rather than a new acceptance criterion.

Written by Claude (Claude Code) on behalf of @zJaaal.

[dario-daza]

Approving the direction. One concern: the blast radius. The file removal moves into the shared content destroy (ESContentletAPIImpl.deleteBinaryFiles), so this changes timing for every content destroy, not only folder delete: editor, workflows, site and content type deletion, push publishing on the receiver, plugins.I'd suggest that @fabrizzio-dotCMS take a look to see if this has any negative impact.