Skip to content

feat: OCISDEV-1059 rollback upload - #669

Open
LarsJurgensen wants to merge 3 commits into
mainfrom
feat-ocisdev-1059-rollback-upload
Open

feat: OCISDEV-1059 rollback upload#669
LarsJurgensen wants to merge 3 commits into
mainfrom
feat-ocisdev-1059-rollback-upload

Conversation

@LarsJurgensen

@LarsJurgensen LarsJurgensen commented Jul 27, 2026

Copy link
Copy Markdown

Mirrors the behavior in decomposedfs postprocessing loop when postprocessing failed. This MR is a prerequisite to move the postprocessingloop into the cooridinator.

Additional changes:

  • Add sizeDiff to the return value of PrepareUpload. This value is necessary so the size propagation can be reverted.
  • Make the UnmarkProcessing call in RevertCurrentRevision method optional. RollbackUpload should not call UnmarkProcessing, because 1) coordinator calls MarkProcessing(false) itself and 2) UnmarkProcessing is changing metric values, which also should only happen in coordinator.

Notes:

  • There is a bug that when versioning is disabled and a node got overwritten, this method is deleting the file instead of reverting the changes. This bug is preexisting and is not fixed in this MR. It's not straightforward to fix, because at the time of reverting, we don't have the information about the overwritten file anymore. Could be solved in the future by e.g. storing the old overwritten metadata in the session. Out of scope for this MR

@LarsJurgensen
LarsJurgensen requested a review from a team as a code owner July 27, 2026 08:29
@kw-security

kw-security commented Jul 27, 2026

Copy link
Copy Markdown

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.

func (fs *Decomposedfs) RollbackUpload(ctx context.Context, ref *provider.Reference, sessionID string, nodeExisted bool, sizeDiff int64) error {
n, err := fs.lu.NodeFromResource(ctx, ref)
if err != nil {
return errtypes.InternalError("RollbackUpload: node lookup failed")

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Original error lost

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, updated

curProcessingID, err := n.ProcessingID(ctx)
if err != nil {
appctx.GetLogger(ctx).Warn().Err(err).Str("sessionID", sessionID).Msg("RollbackUpload: could not read processing ID, skipping")
return nil

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we return an error here?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, updated

@2403905

2403905 commented Jul 28, 2026

Copy link
Copy Markdown
  1. Potential double size-revert for new nodes (correctness risk) — upload.go:658

RollbackUpload propagates -sizeDiff whenever sizeDiff != 0, regardless of nodeExisted. For a new upload (nodeExisted=false), sizeDiff is the full info.Size, and the doc comment says the coordinator "calls Delete separately." If the coordinator calls both RollbackUpload (which propagates -size) and Delete/Purge
(which also propagates the size removal), the size is subtracted twice.

The early if !n.Exists { return nil } guard only protects the case where the node is already gone. If RollbackUpload is invoked while a freshly-created new node still exists, the propagate runs. The safe intent seems to be "only revert size for the overwrite case" — consider gating the propagate on nodeExisted, or
documenting explicitly that the coordinator must never call RollbackUpload with nodeExisted=false on an existing node. The coordinator isn't in this PR, so this is worth confirming before it lands.

@LarsJurgensen

Copy link
Copy Markdown
Author
  1. Potential double size-revert for new nodes (correctness risk) — upload.go:658

RollbackUpload propagates -sizeDiff whenever sizeDiff != 0, regardless of nodeExisted. For a new upload (nodeExisted=false), sizeDiff is the full info.Size, and the doc comment says the coordinator "calls Delete separately." If the coordinator calls both RollbackUpload (which propagates -size) and Delete/Purge (which also propagates the size removal), the size is subtracted twice.

The early if !n.Exists { return nil } guard only protects the case where the node is already gone. If RollbackUpload is invoked while a freshly-created new node still exists, the propagate runs. The safe intent seems to be "only revert size for the overwrite case" — consider gating the propagate on nodeExisted, or documenting explicitly that the coordinator must never call RollbackUpload with nodeExisted=false on an existing node. The coordinator isn't in this PR, so this is worth confirming before it lands.

Good point. The concern is valid, if the node is new, we should not propagate -sizeDiff, because we already delete the file.
So two options:
a) Have noop, in case nodeExisted=false & still call this method in coordinator.
b) Don't call this method in coordinator, if nodeExisted=false & make this explicit in contract. Then nodeExisted parameter is obsolete, because it's always true.

I implemented option a), in case some future driver wants to do different behavior and because this makes a bug in coordinator less likely

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.

3 participants