From 07f83725ae95c0b3cc628915d6665e63ed1328e4 Mon Sep 17 00:00:00 2001 From: Julian Koberg Date: Fri, 7 Aug 2026 14:56:44 +0200 Subject: [PATCH] pkg/storage: report sessions without a node as orphaned 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 --- .../fix-orphaned-session-without-node.md | 15 ++++++++ .../utils/decomposedfs/upload/session.go | 21 +++++++---- .../utils/decomposedfs/upload_async_test.go | 36 +++++++++++++++++++ 3 files changed, 66 insertions(+), 6 deletions(-) create mode 100644 changelog/unreleased/fix-orphaned-session-without-node.md diff --git a/changelog/unreleased/fix-orphaned-session-without-node.md b/changelog/unreleased/fix-orphaned-session-without-node.md new file mode 100644 index 00000000000..b1afbd3592b --- /dev/null +++ b/changelog/unreleased/fix-orphaned-session-without-node.md @@ -0,0 +1,15 @@ +Bugfix: Report upload sessions whose node is gone as orphaned + +The orphaned upload session filter only matched sessions whose node could not be +read. It did not match sessions whose node does not exist at all, because +ReadNode deliberately swallows a missing node and reports no error. + +That left a state which could not be recovered: if a cleanup removed the +orphaned node but did not get as far as removing the session, the remaining +session and its upload data were invisible to the filter and could never be +cleaned up again. + +A session is now reported as orphaned when its node cannot be read *or* does not +exist. + +https://github.com/owncloud/reva/pull/699 diff --git a/pkg/storage/utils/decomposedfs/upload/session.go b/pkg/storage/utils/decomposedfs/upload/session.go index 73c0e280347..6d7f691eb9c 100644 --- a/pkg/storage/utils/decomposedfs/upload/session.go +++ b/pkg/storage/utils/decomposedfs/upload/session.go @@ -168,13 +168,22 @@ func (s *OcisSession) Node(ctx context.Context) (*node.Node, error) { } // IsOrphaned returns true if the session's target node can no longer be -// resolved. This happens when the node file still exists but its metadata is -// gone, e.g. because an ancestor was moved to the trash while the upload was in -// flight. Such a session can never finish postprocessing: reading the node -// fails before the destination can be determined. +// resolved. Such a session can never finish postprocessing, because the +// destination of the upload cannot be determined. There are two ways to end up +// in that state: +// +// - reading the node fails, e.g. because the node file still exists but its +// metadata is gone after an ancestor was moved to the trash while the upload +// was in flight +// - the node does not exist at all, e.g. because a previous cleanup removed it +// but did not get as far as removing the session +// +// The second case matters for recovery: ReadNode deliberately swallows a missing +// node and reports no error, so it has to be detected through Exists. Otherwise a +// session left behind by an interrupted cleanup could never be found again. func (s *OcisSession) IsOrphaned(ctx context.Context) bool { - _, err := s.Node(ctx) - return err != nil + n, err := s.Node(ctx) + return err != nil || n == nil || !n.Exists } // syntheticNode builds a node from the session metadata alone, without reading diff --git a/pkg/storage/utils/decomposedfs/upload_async_test.go b/pkg/storage/utils/decomposedfs/upload_async_test.go index 65a25e78898..35ff8ad88aa 100644 --- a/pkg/storage/utils/decomposedfs/upload_async_test.go +++ b/pkg/storage/utils/decomposedfs/upload_async_test.go @@ -336,6 +336,42 @@ var _ = Describe("Async file uploads", Ordered, func() { Eventually(parentSize).Should(Equal(0)) }) + It("reports a session whose node no longer exists as orphaned", func() { + resources, err := fs.ListFolder(ctx, rootRef, []string{}, []string{}) + Expect(err).ToNot(HaveOccurred()) + Expect(len(resources)).To(Equal(1)) + + lister, ok := fs.(storage.UploadSessionLister) + Expect(ok).To(BeTrue()) + + orphaned := true + sessions, err := lister.ListUploadSessions(ctx, storage.UploadSessionFilter{Orphaned: &orphaned}) + Expect(err).ToNot(HaveOccurred()) + Expect(sessions).To(BeEmpty(), "a healthy session is not orphaned") + + // Remove the node entirely, as an interrupted cleanup would leave it: + // the node is gone but the session is still there. ReadNode swallows a + // missing node and reports no error, so this has to be detected through + // the node's Exists flag. + nodePath := lu.InternalPath(ref.GetResourceId().GetSpaceId(), resources[0].GetId().GetOpaqueId()) + Expect(lu.MetadataBackend().Purge(ctx, nodePath)).To(Succeed()) + Expect(os.Remove(nodePath)).To(Succeed()) + + n, err := node.ReadNode(ctx, lu, ref.GetResourceId().GetSpaceId(), resources[0].GetId().GetOpaqueId(), false, nil, true) + Expect(err).ToNot(HaveOccurred(), "reading a missing node does not fail") + Expect(n.Exists).To(BeFalse()) + + sessions, err = lister.ListUploadSessions(ctx, storage.UploadSessionFilter{Orphaned: &orphaned}) + Expect(err).ToNot(HaveOccurred()) + Expect(sessions).To(HaveLen(1), "a session without a node should be reported as orphaned") + Expect(sessions[0].ID()).To(Equal(uploadID)) + + notOrphaned := false + sessions, err = lister.ListUploadSessions(ctx, storage.UploadSessionFilter{Orphaned: ¬Orphaned}) + Expect(err).ToNot(HaveOccurred()) + Expect(sessions).To(BeEmpty()) + }) + It("deletes node and keeps the bytes when instructed", func() { // node is created resources, err := fs.ListFolder(ctx, rootRef, []string{}, []string{})