pkg/storage: report sessions without a node as orphaned

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

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

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: &notOrphaned})
+			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{})