blobstore: implement async directory operations for Ali OSS

[kchoy-sfdc]

Summary

Add async directory related operations support for Ali blobstore.

Includes unit tests with mocked SDK client. Performed manual integration testing against live Ali environment to verify functionality.

Some conventions to follow

1. add the module name as a prefix
   • for example: add a prefix: docstore: for document store module, blobstore for Blob Store module
2. for a test only PR, add test:
3. for a perf improvement only PR, add perf:
4. for a refactoring only PR, add "refactor:"

[codecov-commenter]

Codecov Report

❌ Patch coverage is 83.90805% with 28 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.71%. Comparing base (f30b7a6) to head (d9c8876).

Files with missing lines	Patch %	Lines
.../multicloudj/blob/ali/async/AliAsyncBlobStore.java	83.03%	14 Missing and 14 partials ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main     #460      +/-   ##
============================================
+ Coverage     81.65%   81.71%   +0.05%     
  Complexity      652      652              
============================================
  Files           204      204              
  Lines         13613    13784     +171     
  Branches       1809     1830      +21     
============================================
+ Hits          11116    11263     +147     
- Misses         1701     1711      +10     
- Partials        796      810      +14     

Flag	Coverage Δ
unittests	81.71% <83.90%> (+0.05%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[iamabhilaksh]

Couple of blockers from reading the diff:

1. doDeleteDirectory data race on futures (AliAsyncBlobStore.java:736)

List<CompletableFuture<Void>> futures = new ArrayList<>();
Consumer<ListBlobsBatch> consumer = batch -> {
    ...
    futures.add(doDelete(...));   // mutated from completion thread
};
CompletableFuture<Void> listFuture = doList(..., consumer);
futures.add(listFuture);          // mutated from calling thread
return listFuture.thenCompose(v -> CompletableFuture.allOf(futures.toArray(new CompletableFuture[0])));

futures is a plain ArrayList mutated from the consumer callback (runs on doList's completion thread) and from the calling thread. That's a JMM data race. The thenCompose also assumes all consumer-added futures are visible by the time listFuture completes — true today depending on how doList schedules the consumer, but fragile.

You already used synchronized (blobInfos) in doDownloadDirectory for the same pattern — same care needed here. Simplest fix: Collections.synchronizedList(new ArrayList<>()) or CopyOnWriteArrayList.

2. Missing path-traversal check in doDownloadDirectory (AliAsyncBlobStore.java:575)

String relative = (prefix != null && key.startsWith(prefix)) ? key.substring(prefix.length()) : key;
Path destination = targetDir.resolve(relative).normalize();

PR #422 just added a safeTargetDir.startsWith() guard to GCP for exactly this — a malicious key like prefix/../../../../etc/passwd resolves outside targetDir. Same vector here, no guard. Should match the GCP pattern.

Nits:

• isExcluded (line 624) uses raw key.startsWith(excludePrefix). If excludes contain data/logs (no trailing slash), it'll false-positive on data/logs2/x. Either normalize trailing slash on entry or document the contract.
• partitionList and toBlobIdentifiers on AliTransformer (newly added) should be static + package-private — no instance state, not part of the public API surface.
• testUploadDirectoryWithSubFolders for includeSubFolders=false only checks getFailedTransfers().size() == 0 — doesn't assert putObjectAsync was called exactly once for root.txt. The behavior is implicit; an explicit verify(..., times(1)) would catch a regression where nested files leak in.

Rest is solid — upload/download/exclusion logic and pagination test coverage are good. Happy to re-review once the race + traversal are addressed.

[kchoy-sfdc]

Couple of blockers from reading the diff:

1. doDeleteDirectory data race on futures (AliAsyncBlobStore.java:736)

List<CompletableFuture<Void>> futures = new ArrayList<>();
Consumer<ListBlobsBatch> consumer = batch -> {
    ...
    futures.add(doDelete(...));   // mutated from completion thread
};
CompletableFuture<Void> listFuture = doList(..., consumer);
futures.add(listFuture);          // mutated from calling thread
return listFuture.thenCompose(v -> CompletableFuture.allOf(futures.toArray(new CompletableFuture[0])));

futures is a plain ArrayList mutated from the consumer callback (runs on doList's completion thread) and from the calling thread. That's a JMM data race. The thenCompose also assumes all consumer-added futures are visible by the time listFuture completes — true today depending on how doList schedules the consumer, but fragile.

You already used synchronized (blobInfos) in doDownloadDirectory for the same pattern — same care needed here. Simplest fix: Collections.synchronizedList(new ArrayList<>()) or CopyOnWriteArrayList.

2. Missing path-traversal check in doDownloadDirectory (AliAsyncBlobStore.java:575)

String relative = (prefix != null && key.startsWith(prefix)) ? key.substring(prefix.length()) : key;
Path destination = targetDir.resolve(relative).normalize();

PR #422 just added a safeTargetDir.startsWith() guard to GCP for exactly this — a malicious key like prefix/../../../../etc/passwd resolves outside targetDir. Same vector here, no guard. Should match the GCP pattern.

Nits:

• isExcluded (line 624) uses raw key.startsWith(excludePrefix). If excludes contain data/logs (no trailing slash), it'll false-positive on data/logs2/x. Either normalize trailing slash on entry or document the contract.
• partitionList and toBlobIdentifiers on AliTransformer (newly added) should be static + package-private — no instance state, not part of the public API surface.
• testUploadDirectoryWithSubFolders for includeSubFolders=false only checks getFailedTransfers().size() == 0 — doesn't assert putObjectAsync was called exactly once for root.txt. The behavior is implicit; an explicit verify(..., times(1)) would catch a regression where nested files leak in.

Rest is solid — upload/download/exclusion logic and pagination test coverage are good. Happy to re-review once the race + traversal are addressed.

Regarding pt 1, I will push a commit to add Collections.synchronizedList(new ArrayList<>()) to address this.
For pt 2, I think there was some discussion regarding whether we should add similar guardrails on GCP side in PR #422. @sandeepvinayak Can you chime in here?

[LihaoLiuXs]

Nested blocking .join() inside CompletableFuture.supplyAsync(): You call doList(...).join() (blocking) inside a supplyAsync lambda, and then later call CompletableFuture.allOf(...).join() again. This means one executor thread is blocked for the entire duration of listing + all downloads, which could limit throughput if the executor has few threads.

[kchoy-sfdc]

Ack. Will refactor this logic here in both upload/download to alleviate this

[iamabhilaksh]

Solid work overall — directory ops finally landing for Ali async is great. A few things I'd like to see resolved before we merge:

[Blocker] Path-traversal check missing in doDownloadDirectory — at the destination resolution site (~line 558), we still need the targetDir.resolve(relative).normalize().startsWith(targetDir) check. The sync side has it; without it, a crafted object key with ../ segments could escape the target dir. Same pattern Barry already enforced in #422.

[Thinking out loud] Blocking I/O on SDK completion threads (commit 05d937f) — the new Files.createDirectories(parent) and the double filePath.toFile().length() calls run on the SDK's async-completion thread, not on our executor. Under load that can starve the SDK's callback pool. One option is to wrap them in CompletableFuture.runAsync(..., executor); another is to do the directory creation up-front. Curious what you think — happy either way as long as we're aware of it.

Nits still open from earlier round (non-blocking, fine to chase in a follow-up):

• @Threads value still hardcoded
• TearDown ordering on the param state

Once the path-traversal one is in I'll re-approve.

[kchoy-sfdc]

Solid work overall — directory ops finally landing for Ali async is great. A few things I'd like to see resolved before we merge:

[Blocker] Path-traversal check missing in doDownloadDirectory — at the destination resolution site (~line 558), we still need the targetDir.resolve(relative).normalize().startsWith(targetDir) check. The sync side has it; without it, a crafted object key with ../ segments could escape the target dir. Same pattern Barry already enforced in #422.

[Thinking out loud] Blocking I/O on SDK completion threads (commit 05d937f) — the new Files.createDirectories(parent) and the double filePath.toFile().length() calls run on the SDK's async-completion thread, not on our executor. Under load that can starve the SDK's callback pool. One option is to wrap them in CompletableFuture.runAsync(..., executor); another is to do the directory creation up-front. Curious what you think — happy either way as long as we're aware of it.

Nits still open from earlier round (non-blocking, fine to chase in a follow-up):

• @Threads value still hardcoded
• TearDown ordering on the param state

Once the path-traversal one is in I'll re-approve.

I have added code to do the path traversal check, as well as tweaked the thread logic around the file I/O to make sure the I/Os are executed in our executor thread, as well as creating/collecting the file info up front to avoid these ops inside the OSS SDK callback thread pool.

PTAL

[iamabhilaksh]

LGTM 👍 — all three blockers from earlier rounds resolved cleanly:

• synchronizedList on futures in doDeleteDirectory
• path-traversal guard in doDownloadDirectory
• directory-creation + file-size stat moved off SDK completion threads via thenComposeAsync(executorService)

Earlier nits (excludePrefix trailing-slash semantics, static helpers on AliTransformer, mockito times(1) on the includeSubFolders=false test) are non-blocking — fine to chase in follow-up if you want.