Skip to content

Commit a2e67eb

Browse files
jorgeepditommasoclaude
authored
Fix S3 lookup unbounded pagination with double call (#6851)
* Fix S3ObjectSummaryLookup causing unbounded pagination on large prefixes The lookup method paginated through all objects under an S3 prefix (maxKeys=250) to check path existence. On prefixes with millions of objects this caused the main thread to hang for minutes parsing massive XML responses. Observed in production: nf-schema parameter validation calls Files.exists() on an S3 outdir path, which triggers S3ObjectSummaryLookup.lookup. With a large prefix like s3://bucket/results containing many objects from previous runs, the pagination loop iterated indefinitely. Fix: use maxKeys=2 and remove pagination. The matchName check only needs to find the exact key or its first child (key + "/"), which are guaranteed to appear in the first results due to S3 lexicographic ordering. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Paolo Di Tommaso <paolo.ditommaso@gmail.com> * add a second call to ensure directory are found when keys with same prefix and smaller lexico order characters than / Signed-off-by: jorgee <jorge.ejarque@seqera.io> * Add inline comments explaining S3 lookup rationale [ci skip] Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Paolo Di Tommaso <paolo.ditommaso@gmail.com> --------- Signed-off-by: Paolo Di Tommaso <paolo.ditommaso@gmail.com> Signed-off-by: jorgee <jorge.ejarque@seqera.io> Co-authored-by: Paolo Di Tommaso <paolo.ditommaso@gmail.com> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
1 parent 4a06f70 commit a2e67eb

File tree

2 files changed

+92
-29
lines changed

2 files changed

+92
-29
lines changed

‎plugins/nf-amazon/src/main/nextflow/cloud/aws/nio/util/S3ObjectSummaryLookup.java‎

Lines changed: 70 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -65,39 +65,80 @@ public S3Object lookup(S3Path s3Path) throws NoSuchFileException {
6565
return summary;
6666
}
6767

68-
/*
69-
* Lookup for the object summary for the specified object key
70-
* by using a `listObjects` request
71-
*/
72-
String marker = null;
73-
while( true ) {
74-
ListObjectsRequest.Builder request = ListObjectsRequest.builder();
75-
request.bucket(s3Path.getBucket());
76-
request.prefix(s3Path.getKey());
77-
request.maxKeys(250);
78-
if( marker != null )
79-
request.marker(marker);
80-
81-
ListObjectsResponse listing = client.listObjects(request.build());
82-
List<S3Object> results = listing.contents();
83-
84-
if (results.isEmpty()){
85-
break;
86-
}
68+
S3Object item = getS3Object(s3Path, client);
69+
if( item != null )
70+
return item;
8771

88-
for( S3Object item : results ) {
89-
if( matchName(s3Path.getKey(), item)) {
90-
return item;
91-
}
92-
}
72+
throw new NoSuchFileException("s3://" + s3Path.getBucket() + "/" + s3Path.getKey());
73+
}
9374

94-
if( listing.isTruncated() )
95-
marker = listing.nextMarker();
96-
else
97-
break;
75+
/**
76+
* Lookup for the S3 object matching the specified path using at most two bounded
77+
* {@code listObjects} calls (replaces the previous unbounded pagination loop).
78+
*
79+
* @param s3Path the S3 path to look up
80+
* @param client the S3 client
81+
* @return the matching {@link S3Object}, or {@code null} if not found
82+
*/
83+
private S3Object getS3Object(S3Path s3Path, S3Client client) {
84+
85+
// Call 1: list up to 2 objects whose key starts with the target key.
86+
//
87+
// Why maxKeys(2) instead of paginating all results?
88+
// The previous implementation used an unbounded while(true) loop fetching 250 keys
89+
// per page. On prefixes with millions of objects this caused excessive S3 LIST API
90+
// calls, high latency, and potential timeouts. Two results are enough to cover
91+
// the common cases:
92+
// - Exact file match: the key itself exists as an object (e.g. "data.txt")
93+
// - Directory match: a child object (e.g. "data/file1") appears within the
94+
// first 2 lexicographic results
95+
ListObjectsRequest request = ListObjectsRequest.builder()
96+
.bucket(s3Path.getBucket())
97+
.prefix(s3Path.getKey())
98+
.maxKeys(2)
99+
.build();
100+
101+
ListObjectsResponse listing = client.listObjects(request);
102+
List<S3Object> results = listing.contents();
103+
104+
for( S3Object item : results ) {
105+
if( matchName(s3Path.getKey(), item)) {
106+
return item;
107+
}
98108
}
99109

100-
throw new NoSuchFileException("s3://" + s3Path.getBucket() + "/" + s3Path.getKey());
110+
// Call 2 (fallback): list 1 object with prefix "key/" to detect directories
111+
// that Call 1 missed.
112+
//
113+
// Why can Call 1 miss a directory?
114+
// S3 lists keys in lexicographic (UTF-8 byte) order, and several common characters
115+
// sort *before* '/' (0x2F) — notably '-' (0x2D) and '.' (0x2E).
116+
//
117+
// Example: given keys "a-a/file-3", "a.txt", and "a/file-1", S3 returns them as:
118+
// a-a/file-3 ← '-' (0x2D) < '/' (0x2F)
119+
// a.txt ← '.' (0x2E) < '/' (0x2F)
120+
// a/file-1 ← '/' (0x2F) — the actual directory child
121+
//
122+
// With maxKeys(2), Call 1 only sees "a-a/file-3" and "a.txt" — neither matches
123+
// key "a" via matchName(). The directory child "a/file-1" is pushed beyond the
124+
// result window by sibling keys that sort earlier.
125+
//
126+
// By searching with prefix "a/" directly, we skip all those siblings and find
127+
// "a/file-1", confirming that "a" is a directory.
128+
request = ListObjectsRequest.builder()
129+
.bucket(s3Path.getBucket())
130+
.prefix(s3Path.getKey()+'/')
131+
.maxKeys(1)
132+
.build();
133+
134+
listing = client.listObjects(request);
135+
results = listing.contents();
136+
for( S3Object item : results ) {
137+
if( matchName(s3Path.getKey(), item)) {
138+
return item;
139+
}
140+
}
141+
return null;
101142
}
102143

103144
private boolean matchName(String fileName, S3Object summary) {

‎plugins/nf-amazon/src/test/nextflow/cloud/aws/nio/AwsS3NioTest.groovy‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1432,4 +1432,26 @@ class AwsS3NioTest extends Specification implements AwsS3BaseSpec {
14321432
deleteBucket(bucket1)
14331433
}
14341434

1435+
// In S3, keys are listed in lexicographic order and characters such as '-' and '.'
1436+
// sort before '/'. For example, given keys 'a/', 'a-a/' and 'a.txt', the listing order
1437+
// is: 'a-a/', 'a.txt', 'a/' — the directory 'a/' appears last.
1438+
// This means a lookup for the directory 'a' may not find the 'a/' marker in the first
1439+
// page of results, so the implementation needs a fallback second call to reliably
1440+
// detect directories when sibling keys with smaller-than-'/' characters exist.
1441+
def 'should exists file with similar files' () {
1442+
given:
1443+
def bucketName = createBucket()
1444+
createObject("$bucketName/similar-lexic-order/a/file-1",'File one')
1445+
createObject("$bucketName/similar-lexic-order/a.txt",'File two')
1446+
createObject("$bucketName/similar-lexic-order/a-a/file-3",'File three')
1447+
1448+
def path = s3path("s3://$bucketName/similar-lexic-order/a")
1449+
expect:
1450+
path.exists()
1451+
path.isDirectory()
1452+
1453+
cleanup:
1454+
deleteBucket(bucketName)
1455+
}
1456+
14351457
}

0 commit comments

Comments
 (0)