From 8e57b2534bbf5ece2518c3c24ce79c5bb07adf3a Mon Sep 17 00:00:00 2001
From: Tom Clegg <tom@curii.com>
Date: Tue, 11 Jul 2023 15:25:22 -0400
Subject: [PATCH] 20726: Fix ListObjects[V2] pages duplicating last item on
 next page.

Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tom@curii.com>
---
 services/keep-web/s3.go      | 24 +++++++++++++++++++-----
 services/keep-web/s3_test.go | 16 +++++++++++-----
 2 files changed, 30 insertions(+), 10 deletions(-)

diff --git a/services/keep-web/s3.go b/services/keep-web/s3.go
index 38428cdab1..fd8764da8f 100644
--- a/services/keep-web/s3.go
+++ b/services/keep-web/s3.go
@@ -772,6 +772,9 @@ func (h *handler) s3list(bucket string, w http.ResponseWriter, r *http.Request,
 			http.Error(w, "invalid continuation token", http.StatusBadRequest)
 			return
 		}
+		// marker and start-after perform the same function,
+		// but we keep them separate so we can repeat them
+		// back to the client in the response.
 		params.marker = string(marker)
 		params.startAfter = r.FormValue("start-after")
 		switch r.FormValue("encoding-type") {
@@ -783,6 +786,7 @@ func (h *handler) s3list(bucket string, w http.ResponseWriter, r *http.Request,
 			return
 		}
 	} else {
+		// marker is functionally equivalent to start-after.
 		params.marker = r.FormValue("marker")
 	}
 
@@ -809,6 +813,11 @@ func (h *handler) s3list(bucket string, w http.ResponseWriter, r *http.Request,
 		ContinuationToken: r.FormValue("continuation-token"),
 		StartAfter:        params.startAfter,
 	}
+
+	// nextMarker will be the last path we add to either
+	// resp.Contents or commonPrefixes.  It will be included in
+	// the response as NextMarker or NextContinuationToken if
+	// needed.
 	nextMarker := ""
 
 	commonPrefixes := map[string]bool{}
@@ -850,7 +859,7 @@ func (h *handler) s3list(bucket string, w http.ResponseWriter, r *http.Request,
 				return errDone
 			}
 		}
-		if path < params.marker || path < params.prefix || path <= params.startAfter {
+		if path <= params.marker || path < params.prefix || path <= params.startAfter {
 			return nil
 		}
 		if fi.IsDir() && !h.Cluster.Collections.S3FolderObjects {
@@ -863,9 +872,6 @@ func (h *handler) s3list(bucket string, w http.ResponseWriter, r *http.Request,
 		}
 		if len(resp.Contents)+len(commonPrefixes) >= params.maxKeys {
 			resp.IsTruncated = true
-			if params.delimiter != "" || params.v2 {
-				nextMarker = path
-			}
 			return errDone
 		}
 		if params.delimiter != "" {
@@ -875,7 +881,11 @@ func (h *handler) s3list(bucket string, w http.ResponseWriter, r *http.Request,
 				// "z", when we hit "foobar/baz", we
 				// add "/baz" to commonPrefixes and
 				// stop descending.
-				commonPrefixes[path[:len(params.prefix)+idx+1]] = true
+				prefix := path[:len(params.prefix)+idx+1]
+				if prefix != params.marker {
+					commonPrefixes[prefix] = true
+					nextMarker = prefix
+				}
 				return filepath.SkipDir
 			}
 		}
@@ -884,12 +894,16 @@ func (h *handler) s3list(bucket string, w http.ResponseWriter, r *http.Request,
 			LastModified: fi.ModTime().UTC().Format("2006-01-02T15:04:05.999") + "Z",
 			Size:         filesize,
 		})
+		nextMarker = path
 		return nil
 	})
 	if err != nil && err != errDone {
 		http.Error(w, err.Error(), http.StatusInternalServerError)
 		return
 	}
+	if params.delimiter == "" && !params.v2 || !resp.IsTruncated {
+		nextMarker = ""
+	}
 	if params.delimiter != "" {
 		resp.CommonPrefixes = make([]commonPrefix, 0, len(commonPrefixes))
 		for prefix := range commonPrefixes {
diff --git a/services/keep-web/s3_test.go b/services/keep-web/s3_test.go
index bfe8bb862c..556f8ba2c2 100644
--- a/services/keep-web/s3_test.go
+++ b/services/keep-web/s3_test.go
@@ -851,6 +851,9 @@ func (s *IntegrationSuite) testS3List(c *check.C, bucket *s3.Bucket, prefix stri
 			break
 		}
 		for _, key := range resp.Contents {
+			if _, dup := gotKeys[key.Key]; dup {
+				c.Errorf("got duplicate key %q on page %d", key.Key, pages)
+			}
 			gotKeys[key.Key] = key
 			if strings.Contains(key.Key, "sailboat.txt") {
 				c.Check(key.Size, check.Equals, int64(4))
@@ -971,28 +974,31 @@ func (s *IntegrationSuite) testS3CollectionListRollup(c *check.C) {
 		var expectTruncated bool
 		for _, key := range allfiles {
 			full := len(expectKeys)+len(expectPrefixes) >= maxKeys
-			if !strings.HasPrefix(key, trial.prefix) || key < trial.marker {
+			if !strings.HasPrefix(key, trial.prefix) || key <= trial.marker {
 				continue
 			} else if idx := strings.Index(key[len(trial.prefix):], trial.delimiter); trial.delimiter != "" && idx >= 0 {
 				prefix := key[:len(trial.prefix)+idx+1]
 				if len(expectPrefixes) > 0 && expectPrefixes[len(expectPrefixes)-1] == prefix {
 					// same prefix as previous key
 				} else if full {
-					expectNextMarker = key
 					expectTruncated = true
 				} else {
 					expectPrefixes = append(expectPrefixes, prefix)
+					expectNextMarker = prefix
 				}
 			} else if full {
-				if trial.delimiter != "" {
-					expectNextMarker = key
-				}
 				expectTruncated = true
 				break
 			} else {
 				expectKeys = append(expectKeys, key)
+				if trial.delimiter != "" {
+					expectNextMarker = key
+				}
 			}
 		}
+		if !expectTruncated {
+			expectNextMarker = ""
+		}
 
 		var gotKeys []string
 		for _, key := range resp.Contents {
-- 
2.30.2