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{})