Bound the recursive folder delete: page a folder's contents, then decide on the transaction
Nobody has claimed this yet.
- Dominant language
- Java
- Stars
- 970
- Forks
- 486
- Avg merge
- 3d 33m
- Merged PRs (30d)
- 170
Description
Description
Deleting a folder loads every contentlet in that folder into memory at once, and deletes the whole subtree inside one database transaction. Both are fine at the sizes most folders actually are, and neither is fine at the sizes some customers actually have.
This is deferred work from #37063, recorded here rather than left as a sentence in a spec. That ticket asks for a bulk folder delete and describes bounding this walk as "the real work"; the decision taken there was to ship the multi-select against today's delete and carry the bounding separately, on the grounds that it changes code every caller of folder deletion shares. The reasoning is in Additional Context so it is not re-derived.
This is not a Content Drive issue. The shipped single-folder delete (POST /api/v1/assets/folders/_delete, #35161) goes through exactly the same path and has exactly the same ceilings. Fixing it here fixes it for the context menu, for site deletion, and for anything in a customer plugin that calls FolderAPI.delete.
Two problems, and they are separable
That separability is the point of this ticket. They look like one thing — "the delete doesn't scale" — and they have different costs, different risks, and different blast radii.
| Problem | Consequence | Fixing it touches | |
|---|---|---|---|
| 1 | A folder's contents are loaded whole | Node runs out of memory | How the contents are read |
| 2 | The whole subtree is one transaction | Locks held, vacuum blocked, rollback can exceed the delete | How the work is committed |
Problem 1 can be fixed without touching problem 2. Paging the read does not move the transaction boundary, so the all-or-nothing guarantee is untouched and nothing downstream is renegotiated. Problem 2 cannot be fixed without giving that guarantee up.
Problem 1 — the contents are loaded whole, and nothing batches them
// FolderAPIImpl._deleteChildrenAssetsFromFolder, FolderAPIImpl.java:564
List<Contentlet> conList = capi.findContentletsByFolder(folder, user, false);
capi.destroy(conList, user, false);
findContentletsByFolder returns the folder's entire contents as one list, and destroy has no internal batching — verified. Before a single row is deleted, internalDestroy then walks that list twice more:
// ESContentletAPIImpl.internalDestroy
for (final Contentlet contentlet : contentlets) {
final List<Contentlet> versions = this.findAllVersions(...); // a query per contentlet
for (final Contentlet version : versions) {
this.canLock(version, user); // a check per version
}
}
So a folder holding 100k files means 100k Contentlet objects plus all of their versions resident at once, and 100k+ queries before the first deletion happens. That is an out-of-memory failure on the node — it takes the JVM down for every other user, not just the one who pressed delete — and wrapping the call in a background job does nothing to help, because the memory is held by the call and not by the request.
Peak memory is bounded by the widest single folder, not by the depth of the tree. One flat media folder is enough; the subtree can be shallow.
Problem 2 — the subtree is one transaction
// FolderAPIImpl.java:427
@WrapInTransaction
public void delete(final Folder folder, final User user, final boolean respectFrontEndPermissions) {
...
for (Folder childFolder : folderChildren) {
delete(childFolder, user, respectFrontEndPermissions); // :478 — re-enters the same transaction
}
The recursion re-enters the same transaction, so deleting a folder with tens of thousands of descendants holds one transaction open for the whole run: locks retained, WAL accumulating, autovacuum blocked behind it. A failure late in a large delete rolls the whole thing back, and that rollback can take longer than the work it undoes.
The upside, which is why this half is the risky one to change: one transaction per folder is exactly what makes folder delete all-or-nothing today. A folder is never left half-deleted, under cancellation or under a crash — and deletion has no undo, so that guarantee is load-bearing rather than incidental. Splitting the transaction gives it up. That trade needs to be made deliberately, with the alternative (resume-on-restart, which the job framework does not provide) priced alongside it.
What is already decided, so it is not reopened here
- #37063 ships against today's delete. Bulk folder delete wraps
FolderAPI.deleteunchanged. That removes the proxy timeout, which is the reported failure; it does not touch either ceiling above. - The all-or-nothing guarantee stands for that feature, precisely because problem 2 is untouched.
- A follow-up is owed — this one.
What is not decided, and is the substance of this ticket, is whether either fix lives on the shared FolderAPI.delete that every caller uses, or on a second path. Changing the shared one is an API-behaviour change in the rollback-unsafe categories this repo tracks; adding a second one means folder delete behaves differently depending on how it was invoked, permanently. Neither is obviously right, and the answer may differ between the two halves.
Measure before building
Nobody has a number for where this actually breaks. "Large folder" is not a threshold anyone can act on, and both fixes need one — to size the work, to write a test that fails today, and to tell support what to warn about. That is the first acceptance criterion rather than an afterthought.
Acceptance Criteria
Measurement — do this first
- The folder size at which deleting one folder puts a node at risk is measured, not estimated, and recorded in the issue: contentlet count, resident memory, and wall-clock time, on a stated heap size
- The same is measured for transaction duration and lock retention on that folder
- An integration test reproduces the memory ceiling and fails on today's code
Problem 1 — page the contents
- A folder's contents are read and destroyed in bounded batches rather than loaded whole
- The batch size is configurable through
Config, with a documented default - Peak memory for deleting one folder is bounded by the batch size, not by the folder's width, demonstrated against the measurement above
- The transaction boundary is unchanged: one top-level folder is still one transaction, and a folder is still either fully deleted or untouched
- Nothing else changes — same contents destroyed, same permissions enforced, same events emitted, same behaviour for a folder small enough that batching never triggers
Problem 2 — decide, then either implement or record the decision
- A decision is recorded on whether the transaction is split, naming what replaces the all-or-nothing guarantee if it is, and what a partially deleted folder means for an operation with no undo
- If it is split: cancellation, process death and the abandoned-job sweep are each stated separately — they are three different interruptions and only the first is under the caller's control
- If it is not split: the reason is recorded here so the next reader does not re-derive it
Scope of the change
- For each half, it is stated and justified whether it lands on the shared
FolderAPI.deleteor on a separate path, and what that means for the shipped single-folder delete, site deletion, and customer plugins - If the shared path changes, the rollback-safety labels required by
docs/core/ROLLBACK_UNSAFE_CATEGORIES.mdare applied
Tests
- Integration coverage for: a folder wide enough to exercise more than one batch, a folder small enough to exercise none, an interrupted delete leaving the folder untouched, and a permission refusal partway through
- New test classes are registered in a
MainSuite*/Junit5Suite*@SuiteClasseslist — an unregistered class compiles, passes locally, and is never run in CI - Integration tests are run with
-Dmaven.build.cache.enabled=falseandTests run: Nis confirmed intarget/failsafe-reports/*.txtrather than trusting the exit code
Priority
Medium
Additional Context
Why this was split out of #37063 rather than done there. Recording it so the decision is not relitigated:
- The defect #37063 reports is that multi-select folder delete does not exist. Wrapping today's delete in a background job delivers that, and removes the proxy timeout along with it. Neither ceiling above is the reported failure.
- Bounding the walk means changing
FolderAPIImpl/FolderFactoryImpl— code the context menu, site deletion and customer plugins all share. Doing that as a side effect of a Content Drive feature is how a rollback-unsafe regression reaches production wearing someone else's ticket number. - As its own ticket it can be measured and tested against scale properly, and the shipped single-folder delete benefits too.
The two halves were deliberately deferred together so the cheap one is priced rather than assumed. If paging turns out to be small and safe, it is worth asking whether it belongs in #37063 after all — that is a judgement to make with an estimate in hand, and it is not a reason to reopen that ticket's scope decision.
Related findings in the same code, filed separately — do not fold them in here:
- The delete's contents lookup is permission-filtered and index-backed, while a later pass sweeps whatever it missed straight out of the database by path, with no permission check and no lock check. The narrower an author's rights, the less scrutiny the content under that folder receives.
- That same sweep removes content without recording anything that would remove it from the search index, and without the durable journal added by #37276 — whose fix is scoped to
destroyContentletsand does not reach this path. Every folder delete that reaches the sweep leaves orphaned search documents permanently.
Both are pre-existing, reachable through the shipped single-folder delete, and independent of this ticket.
Related: #37063 (bulk folder delete — the ticket this was deferred from), #35161 (single-folder delete, shipped, same ceilings), #37276 (silent index delete loss), #33999 (parent epic).
Analysis and the two-path comparison behind the deferral: specs/37063-bulk-folder-delete-backend/spec.md (D-013, FR-020 … FR-023) and specs/37063-bulk-folder-delete-backend/drafts/b5-decision-paths.md.
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start by measuring folder-delete memory, wall-clock time, transaction duration, and lock retention at a stated heap size, then read FolderAPIImpl around _deleteChildrenAssetsFromFolder and delete, plus FolderFactoryImpl. Review Config and docs/core/ROLLBACK_UNSAFE_CATEGORIES.md, and inspect the relevant MainSuite*/Junit5Suite* registration. Done means recording the transaction decision and scope, implementing or documenting the bounded-delete outcome, and adding the required integration coverage with verified Failsafe reports.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- java
- Domain
- backend, databases, performance, testing
- Issue type
- Feature
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Active
- Clarity
- Mostly clear
- Newbie friendliness
- 35/100