Skip to content

pkg/storage: report sessions without a node as orphaned - #699

Open
kobergj wants to merge 1 commit into
owncloud:mainfrom
kobergj:fix/ocisdev-1177-orphaned-missing-node
Open

pkg/storage: report sessions without a node as orphaned#699
kobergj wants to merge 1 commit into
owncloud:mainfrom
kobergj:fix/ocisdev-1177-orphaned-missing-node

Conversation

@kobergj

@kobergj kobergj commented Aug 7, 2026

Copy link
Copy Markdown
Collaborator

Problem

Follow-up to #692, which added the orphaned upload session filter. That filter only matches sessions whose node cannot be read:

func (s *OcisSession) IsOrphaned(ctx context.Context) bool {
	_, err := s.Node(ctx)
	return err != nil
}

It does not match a session whose node does not exist at all, because ReadNode deliberately swallows a missing node and reports no error:

attrs, err := n.Xattrs(ctx)
switch {
case metadata.IsNotExist(err):
	return n, nil // swallow not found, the node defaults to exists = false

So err == nil and the session is reported as healthy, even though its upload can never finish.

That leaves a state which cannot be recovered. Cleanup removes the orphaned node before deleting the bin and info files (deliberately — the data must not be destroyed if the revert fails). If it does not get as far as the deletions, the session and its upload payload remain on disk while the node is already gone. The filter then no longer matches them, uploads sessions --clean has nothing to act on, and the leftover bytes can never be collected.

Found while testing the ownCloud Infinite Scale CLI against a deliberately orphaned upload: after cleaning it once, a second --orphaned run reported nothing while the session's .info and payload were still present in uploads/.

Changes

Report a session as orphaned when its node cannot be read or does not exist:

n, err := s.Node(ctx)
return err != nil || n == nil || !n.Exists

The doc comment now spells out both routes into the state and why the second has to be detected through Exists rather than through an error.

No change was needed in revertNode: for a missing node ReadNode returns no error, so ProcessingID fails, the revert is skipped and it returns nil without propagating a size change a second time.

Tests

Adds a test that drives the filter through ListUploadSessions, the way the CLI uses it: a healthy session is not reported, a session whose node has been removed entirely is reported exactly once, and the negated filter reports nothing. It also asserts the underlying premise — that ReadNode on a missing node returns no error with Exists == false — so the test documents why the check is written this way.

Verified that the test fails without the change and passes with it.

🤖 Generated with Claude Code

The orphaned upload session filter only matched sessions whose node could not be
read. A session whose node does not exist at all was not matched, because
ReadNode deliberately swallows a missing node and reports no error.

That left an unrecoverable state: when a cleanup removed the orphaned node but
did not get as far as removing the session, the remaining session and its upload
data became invisible to the filter and could never be cleaned up again.

Report a session as orphaned when its node cannot be read or does not exist.

Signed-off-by: Julian Koberg <julian.koberg@kiteworks.com>
@kobergj
kobergj requested a review from a team as a code owner August 7, 2026 12:57
@kw-security

kw-security commented Aug 7, 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.

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.

2 participants