Description
When a folder delete fails part-way through the tree, the database changes are rolled back but the
files already removed from disk are not restored. The folder, its subfolders and every file
asset stay listed in the product, but the binaries of the files the delete had already reached are
gone: opening them returns 404. The user is told the delete failed, so nothing suggests that
anything was lost.
This is data loss with no warning, and there is no way to recover the file from dotCMS afterwards.
It reproduces with both folder delete entry points, so it predates the bulk delete:
Steps to reproduce
Use a back-end user without the CMS Administrator role (an admin passes every permission check,
so the delete never fails).
- Create a folder
parent/ with two subfolders, parent/a/ and parent/b/. Subfolders are
walked in name order, so a/ must sort before b/.
- Put a file asset in
parent/a/ (any small file, e.g. test.txt) and confirm it opens at
/parent/a/test.txt.
- Give the user's role View, Edit, Edit Permissions and Publish on
parent/, inherited by
its children.
- On
parent/b/ only, remove Edit Permissions for that role (keep View, Edit, Publish).
- As that user, delete
parent/ from Content Drive using either entry point above.
Expected
The delete fails because of parent/b/, and nothing under parent/ changes: every folder,
content and file is still there, and /parent/a/test.txt still opens.
Actual
- The delete fails as expected:
- bulk delete: the outcome reports
parent/ as FAILED, PERMISSION_DENIED;
- context menu: "User … does not have edit permissions on Folder /parent/b/".
- The database is unchanged: all folders, contentlets and the file asset's record are still
there, and the file still shows in Content Drive.
- The file's binary is gone from disk (
/data/shared/assets/<x>/<y>/<inode>/fileAsset/test.txt),
for every version of the file. /parent/a/test.txt and /dA/<inode>/fileAsset/test.txt return
404.
Cause
FolderAPIImpl.delete is @WrapInTransaction and recurses depth-first. When it reaches a/, it
calls ContentletAPI.destroy on its contents, and ESContentletAPIImpl.destroyContentlets calls
deleteBinaryFiles, which removes the asset directory from disk immediately
(FileUtil.deltree, ESContentletAPIImpl.java:9408-9420). That removal is outside the
transaction. When the recursion then fails on b/
(FolderAPIImpl.delete re-checks PERMISSION_EDIT_PERMISSIONS on every subfolder), the
transaction rolls back the rows, but the files are already gone.
Any failure after at least one file has been destroyed triggers the same thing, not only
permissions. For example: content locked by another user deeper in the tree, a database trigger
refusing a delete, or a transient database error.
Possible direction
Defer the disk removal until the transaction commits (HibernateUtil.addCommitListener), so a
rollback leaves the files in place. The spec for
#37063 already requires a failed folder to be left "fully deleted or untouched" (FR-023); on disk
that guarantee does not hold today.
Environment
Reproduced on dotcms/dotcms:trunk_0666682 (includes #37685 and #37688), Postgres, Elasticsearch,
single node.
Related
Also in scope: three smaller bulk delete defects
Found while reviewing the bulk folder duplicate (#37062, PR #37760). The duplicate side copied these from bulk delete and has already been fixed. Bulk delete still has all three. Each is small, so they are carried here, not in a ticket of their own.
1. A child is reported covered by its parent even when the parent is never deleted
FolderBulkDeleteProcessor decides COVERED_BY_PARENT up front, from the submitted paths alone (FolderBulkDeleteProcessor.java:196), before it knows whether the parent will actually be deleted.
- Steps: as a user who may delete
/a/b/ but not /a/, select both and run a bulk delete.
- Actual:
/a/ is FAILED; /a/b/ is SKIPPED / COVERED_BY_PARENT and is never deleted. The outcome says the parent took care of the child when nothing did.
- The same happens when the parent fails for any other reason (
IN_USE, PATH_NOT_FOUND, UNCLASSIFIED), or when a cancellation stops the run before reaching the parent. In the cancellation case the child should be SKIPPED with no reason, like the rest of the remainder.
- Expected: a child is
COVERED_BY_PARENT only when its parent was actually deleted. Otherwise it goes through its own checks and is deleted on its own. This has to be decided at run time, with descendants run after the selected folders that contain them. That is how the duplicate side does it now (4d52a52, FolderBulkDuplicateProcessor#process and #ancestorsFirst).
2. A request with no body gets a 500, not 400 EMPTY_SELECTION
FolderBulkDeleteHelper.submit reads form.assetPaths() with no null check (FolderBulkDeleteHelper.java:85), so POST /api/v1/assets/folders/_bulkdelete with no body throws a NullPointerException. A body of {} already gets 400 EMPTY_SELECTION. A missing form should be treated as an empty selection, as the duplicate side does since 4a49608.
3. Resolving each submitted path also lists the folder's whole contents
The processor turns each path into a folder with WebAssetHelper.getAssetInfo, which also runs a browser query over the folder's entire contents, only for the folder itself to be read afterwards. On large folders this is repeated work for up to the maximum number of paths, before the run's heartbeat starts. Resolving with AssetPathResolver alone finds the same site and folder, and raises the same exceptions the PATH_NOT_FOUND mapping relies on. That is how the duplicate side does it since 4da0ebe.
Acceptance criteria for these three
Description
When a folder delete fails part-way through the tree, the database changes are rolled back but the
files already removed from disk are not restored. The folder, its subfolders and every file
asset stay listed in the product, but the binaries of the files the delete had already reached are
gone: opening them returns
404. The user is told the delete failed, so nothing suggests thatanything was lost.
This is data loss with no warning, and there is no way to recover the file from dotCMS afterwards.
It reproduces with both folder delete entry points, so it predates the bulk delete:
Steps to reproduce
Use a back-end user without the CMS Administrator role (an admin passes every permission check,
so the delete never fails).
parent/with two subfolders,parent/a/andparent/b/. Subfolders arewalked in name order, so
a/must sort beforeb/.parent/a/(any small file, e.g.test.txt) and confirm it opens at/parent/a/test.txt.parent/, inherited byits children.
parent/b/only, remove Edit Permissions for that role (keep View, Edit, Publish).parent/from Content Drive using either entry point above.Expected
The delete fails because of
parent/b/, and nothing underparent/changes: every folder,content and file is still there, and
/parent/a/test.txtstill opens.Actual
parent/asFAILED,PERMISSION_DENIED;there, and the file still shows in Content Drive.
/data/shared/assets/<x>/<y>/<inode>/fileAsset/test.txt),for every version of the file.
/parent/a/test.txtand/dA/<inode>/fileAsset/test.txtreturn404.Cause
FolderAPIImpl.deleteis@WrapInTransactionand recurses depth-first. When it reachesa/, itcalls
ContentletAPI.destroyon its contents, andESContentletAPIImpl.destroyContentletscallsdeleteBinaryFiles, which removes the asset directory from disk immediately(
FileUtil.deltree,ESContentletAPIImpl.java:9408-9420). That removal is outside thetransaction. When the recursion then fails on
b/(
FolderAPIImpl.deletere-checksPERMISSION_EDIT_PERMISSIONSon every subfolder), thetransaction rolls back the rows, but the files are already gone.
Any failure after at least one file has been destroyed triggers the same thing, not only
permissions. For example: content locked by another user deeper in the tree, a database trigger
refusing a delete, or a transient database error.
Possible direction
Defer the disk removal until the transaction commits (
HibernateUtil.addCommitListener), so arollback leaves the files in place. The spec for
#37063 already requires a failed folder to be left "fully deleted or untouched" (FR-023); on disk
that guarantee does not hold today.
Environment
Reproduced on
dotcms/dotcms:trunk_0666682(includes #37685 and #37688), Postgres, Elasticsearch,single node.
Related
delete makes it more likely because it deletes more folders per action.
transaction, but on the index instead of the disk.
Also in scope: three smaller bulk delete defects
Found while reviewing the bulk folder duplicate (#37062, PR #37760). The duplicate side copied these from bulk delete and has already been fixed. Bulk delete still has all three. Each is small, so they are carried here, not in a ticket of their own.
1. A child is reported covered by its parent even when the parent is never deleted
FolderBulkDeleteProcessordecidesCOVERED_BY_PARENTup front, from the submitted paths alone (FolderBulkDeleteProcessor.java:196), before it knows whether the parent will actually be deleted./a/b/but not/a/, select both and run a bulk delete./a/isFAILED;/a/b/isSKIPPED / COVERED_BY_PARENTand is never deleted. The outcome says the parent took care of the child when nothing did.IN_USE,PATH_NOT_FOUND,UNCLASSIFIED), or when a cancellation stops the run before reaching the parent. In the cancellation case the child should beSKIPPEDwith no reason, like the rest of the remainder.COVERED_BY_PARENTonly when its parent was actually deleted. Otherwise it goes through its own checks and is deleted on its own. This has to be decided at run time, with descendants run after the selected folders that contain them. That is how the duplicate side does it now (4d52a52,FolderBulkDuplicateProcessor#processand#ancestorsFirst).2. A request with no body gets a
500, not400 EMPTY_SELECTIONFolderBulkDeleteHelper.submitreadsform.assetPaths()with no null check (FolderBulkDeleteHelper.java:85), soPOST /api/v1/assets/folders/_bulkdeletewith no body throws aNullPointerException. A body of{}already gets400 EMPTY_SELECTION. A missing form should be treated as an empty selection, as the duplicate side does since 4a49608.3. Resolving each submitted path also lists the folder's whole contents
The processor turns each path into a folder with
WebAssetHelper.getAssetInfo, which also runs a browser query over the folder's entire contents, only for the folder itself to be read afterwards. On large folders this is repeated work for up to the maximum number of paths, before the run's heartbeat starts. Resolving withAssetPathResolveralone finds the same site and folder, and raises the same exceptions thePATH_NOT_FOUNDmapping relies on. That is how the duplicate side does it since 4da0ebe.Acceptance criteria for these three
COVERED_BY_PARENT, with an integration test for each case.400 EMPTY_SELECTION, with a unit test and a Postman request.PATH_NOT_FOUND.