Ocisdev 900 pr6d orphan rollback

[LarsJurgensen]

No description provided.

[kw-security]

✅ Snyk checks have passed. No issues have been found so far.

Status	Scan Engine	Critical	High	Medium	Low	Total (0)
✅	Open Source Security	0	0	0	0	0 issues
✅	Licenses	0	0	0	0	0 issues
✅	Code Security	0	0	0	0	0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

[2403905]

Code review

Automated review of the orphan-rollback changes (pkg/storage/utils/decomposedfs/upload.go new rollbackOrphaned, pkg/upload/coordinator.go, pkg/upload/postprocessing.go, pkg/upload/tus_adapter.go, pkg/storage/storage.go new RollbackInfo).

1. pkg/storage/utils/decomposedfs/upload.go:653 — transient errors misrouted into destructive orphan cleanup

RollbackUpload now routes every NodeFromResource error into rollbackOrphaned, not just genuine metadata corruption. node.ReadNode (pkg/storage/utils/decomposedfs/node/node.go:382-388) can return a bare error for transient issues (EIO, lock contention, backend hiccup) on a perfectly live node. Previously this just returned "node lookup failed" and left the session/node untouched. Now, since info.NodeID/ParentID are populated from the session on essentially every call path (rollbackInfo() in coordinator.go), a transient error gets treated as "orphaned" — rollbackOrphaned reverts size propagation and force-removes the node file and its metadata, causing real data loss instead of a safe retry.

2. pkg/storage/utils/decomposedfs/upload.go:706 — missing ownership check before destructive rollback

The normal RollbackUpload path checks curProcessingID != sessionID (upload.go:667-673) before mutating a node, bailing out if a newer session now owns it. rollbackOrphaned has no equivalent check — it builds n purely from session-recorded NodeID/ParentID/Filename/Size and unconditionally reverts propagation + deletes the node. Combined with finding #1, a transient lookup failure could destroy a newer session's data on the same node ID with nothing to stop it.

3. pkg/storage/utils/decomposedfs/upload.go:719 — dangling symlink left behind

rollbackOrphaned removes the blob (utils.RemoveItem) and metadata (MetadataBackend.Purge) but never removes the parent directory's child-name symlink. node.Purge (pkg/storage/utils/decomposedfs/node/node.go:1162-1176) explicitly removes filepath.Join(n.ParentPath(), n.Name) in addition to InternalPath and metadata — rollbackOrphaned skips that step. After an orphan rollback, the parent directory keeps a dangling symlink for the rolled-back file name (e.g. surfaces as a broken entry in PROPFIND/listing).

4. pkg/upload/postprocessing.go:193 — quota-leak bug reintroduced via a different call site

This PR hardens rollbackPrepared in coordinator.go (line 416-428) to keep the session and skip unmarkProcessing/metric-decrement when fs.RollbackUpload fails — specifically to avoid "leaving the quota consumed with nothing to reclaim it from." However rollbackNode (postprocessing.go:193-199, called from PPOutcomeAbort, the unknown-outcome default, and CleanUpload's KeepUpload branch) and tusAdapter.Terminate (tus_adapter.go:66-77) still log-and-continue on the same RollbackUpload error — they call unmarkProcessing/MarkProcessing(false) and (for CleanUpload) still Dec() the metric regardless. A failed rollback in these paths marks the upload done and loses retry-ability, which is exactly the quota-leak scenario this PR fixes elsewhere.

[LarsJurgensen]

Code review

Automated review of the orphan-rollback changes (pkg/storage/utils/decomposedfs/upload.go new rollbackOrphaned, pkg/upload/coordinator.go, pkg/upload/postprocessing.go, pkg/upload/tus_adapter.go, pkg/storage/storage.go new RollbackInfo).

1. pkg/storage/utils/decomposedfs/upload.go:653 — transient errors misrouted into destructive orphan cleanup

RollbackUpload now routes every NodeFromResource error into rollbackOrphaned, not just genuine metadata corruption. node.ReadNode (pkg/storage/utils/decomposedfs/node/node.go:382-388) can return a bare error for transient issues (EIO, lock contention, backend hiccup) on a perfectly live node. Previously this just returned "node lookup failed" and left the session/node untouched. Now, since info.NodeID/ParentID are populated from the session on essentially every call path (rollbackInfo() in coordinator.go), a transient error gets treated as "orphaned" — rollbackOrphaned reverts size propagation and force-removes the node file and its metadata, causing real data loss instead of a safe retry.

2. pkg/storage/utils/decomposedfs/upload.go:706 — missing ownership check before destructive rollback

The normal RollbackUpload path checks curProcessingID != sessionID (upload.go:667-673) before mutating a node, bailing out if a newer session now owns it. rollbackOrphaned has no equivalent check — it builds n purely from session-recorded NodeID/ParentID/Filename/Size and unconditionally reverts propagation + deletes the node. Combined with finding #1, a transient lookup failure could destroy a newer session's data on the same node ID with nothing to stop it.

3. pkg/storage/utils/decomposedfs/upload.go:719 — dangling symlink left behind

rollbackOrphaned removes the blob (utils.RemoveItem) and metadata (MetadataBackend.Purge) but never removes the parent directory's child-name symlink. node.Purge (pkg/storage/utils/decomposedfs/node/node.go:1162-1176) explicitly removes filepath.Join(n.ParentPath(), n.Name) in addition to InternalPath and metadata — rollbackOrphaned skips that step. After an orphan rollback, the parent directory keeps a dangling symlink for the rolled-back file name (e.g. surfaces as a broken entry in PROPFIND/listing).

4. pkg/upload/postprocessing.go:193 — quota-leak bug reintroduced via a different call site

This PR hardens rollbackPrepared in coordinator.go (line 416-428) to keep the session and skip unmarkProcessing/metric-decrement when fs.RollbackUpload fails — specifically to avoid "leaving the quota consumed with nothing to reclaim it from." However rollbackNode (postprocessing.go:193-199, called from PPOutcomeAbort, the unknown-outcome default, and CleanUpload's KeepUpload branch) and tusAdapter.Terminate (tus_adapter.go:66-77) still log-and-continue on the same RollbackUpload error — they call unmarkProcessing/MarkProcessing(false) and (for CleanUpload) still Dec() the metric regardless. A failed rollback in these paths marks the upload done and loses retry-ability, which is exactly the quota-leak scenario this PR fixes elsewhere.

@2403905
1: This is preexisting behavior. The "causing real data loss" is intentional, here we are in the scenario that the upload failed and we want to rollback (both changes to node & changes to quota). In scenarios, where we want to retry, we don't call the RollbackUpload method at all. The only undesired scenario is: Versioning is enabled, a new version is uploaded, the upload fails & afterwards when rolling back, reading the node fails. In that case we would delete the full node, rather than just the version. The reason is that it's non trivial to revert only the version, since we can't even read the node & we don't know why. I suggest leaving this, since it's preexisting & from what I see very rare.

2: This is also preexisting. The reason that we don't do the ownership check is that the processing flag lives on the node, but we failed to read the node. So we are not able to do the ownership check. We still want to revert quota, because otherwise we leave it in a broken state.

3: This is preexisting, but the concern is valid. I updated the implementation to also remove the symlink

4: This is a regression. When the upload failed & then we fail to rollback, it's not an easy tradeoff what we should do. Either we still cleanup session & unmark processing flag, to make sure nothing is left behind (which however removes visibility of issue for the admin). Or we leave the session & processing flag behind, so admins can manually investigate this (but node stays in processing until someone manually fixes it). I now updated the implementation to leave the session behind as suggested, which also matches the previous implementation.