[Bug]: expireSnapshots fails with `IllegalArgumentException: null` on object-store-backed Iceberg tables (S3FileIO / OSSFileIO / GCSFileIO / ResolvingFileIO)

[felixhzhu]

What happened?

When an Iceberg table's underlying FileIO is an object-store-native one (S3FileIO / OSSFileIO / GCSFileIO / ResolvingFileIO, commonly seen with Glue / REST / Polaris catalogs), the AMS-side expireSnapshots task always fails with IllegalArgumentException: null thrown from AuthenticatedFileIOAdapter.asFileSystemIO(). SnapshotsExpiringProcess is marked FAILED for every such table, even though the underlying data/manifest files have already been deleted successfully.

Root cause:
RollingFileCleaner.doCleanFiles() unconditionally calls TableFileUtil.deleteEmptyDirectory(), which invokes io.asFileSystemIO(). For object-store FileIOs, AuthenticatedFileIOAdapter.supportFileSystemOperations() returns false, so the single-arg Preconditions.checkArgument(boolean) throws IllegalArgumentException with a null message — matching the stack trace exactly.

The "delete empty parent directory" step only has meaning under HDFS / Hadoop FS semantics. On object stores there is no directory entity, the prefix disappears automatically when the last object is deleted, so this step is a no-op by definition.

Expected: expireSnapshots should succeed on object-store-backed tables; the directory-cleanup step should be skipped silently when the FileIO does not support filesystem operations.

(There is also a downstream issue: once the process is marked FAILED, DefaultTableProcessStore.tryTransitState rejects the subsequent RETRY_REQUESTED event because currentStatus is FAILED, producing a WARN storm. I will file that as a separate issue since the fix lives in amoro-ams, not amoro-format-iceberg.)

Affects Versions

0.9.0-incubating (also reproducible on the 0.9.x branch HEAD)

What table formats are you seeing the problem on?

Iceberg

What engines are you seeing the problem on?

AMS

How to reproduce

Environment:

• Amoro 0.9.0-incubating, single AMS node.
• Iceberg catalog: Glue / REST / Polaris (anything that produces a table whose io-impl is an object-store-native FileIO).
• Table property io-impl is one of: S3FileIO / OSSFileIO / GCSFileIO / ResolvingFileIO.
• Table has at least one expirable snapshot.

Steps:

1. Register the Iceberg catalog into AMS.
2. Pick a table whose oldest snapshot is older than history.expire.max-snapshot-age-ms.
3. Trigger expireSnapshots — either via the AMS dashboard, or wait for SnapshotsExpiringExecutor.
4. AMS log immediately reports IllegalArgumentException: null and the process is marked FAILED.

Observations:

• table.expireSnapshots()...commit() actually succeeds (old snapshots removed from metadata.json).
• RollingFileCleaner's bulk delete actually succeeds (S3FileIO.deleteFiles is invoked correctly).
• Only the trailing "delete empty parent directories" step throws and flips the whole process to FAILED.

Reproduces 100% of the time on every object-store-backed table. HDFS / Hadoop FS tables (HadoopFileIO, including s3a/cosn/oss) are NOT affected.

Relevant log output

ERROR [local-expire-snapshots-7] [org.apache.amoro.server.process.iceberg.SnapshotsExpiringProcess]
- unexpected expire error of table xxx(tableId=92)
java.lang.IllegalArgumentException: null
    at org.apache.amoro.shade.guava32.com.google.common.base.Preconditions.checkArgument(Preconditions.java:129)
    at org.apache.amoro.io.AuthenticatedFileIOAdapter.asFileSystemIO(AuthenticatedFileIOAdapter.java:98)
    at org.apache.amoro.utils.TableFileUtil.deleteEmptyDirectory(TableFileUtil.java:95)
    at org.apache.amoro.formats.iceberg.utils.RollingFileCleaner.doCleanFiles(RollingFileCleaner.java:114)
    at org.apache.amoro.formats.iceberg.utils.RollingFileCleaner.clear(RollingFileCleaner.java:133)
    at org.apache.amoro.formats.iceberg.maintainer.IcebergTableMaintainer.expireSnapshots(IcebergTableMaintainer.java:...)

Anything else

Frequency: every scheduled run, every object-store-backed table. Verified on AWS S3 (Glue) and on a Polaris-backed REST catalog.

Proposed fix (already implemented locally, happy to upstream as a PR):

1. TableFileUtil.deleteEmptyDirectory: early-return when !io.supportFileSystemOperations(). Object stores have no real directories — this step is a no-op by definition, and it's also where the existing exception originates.

2. RollingFileCleaner.doCleanFiles:

   • guard the "delete empty parent directories" loop with the same supportFileSystemOperations() check (defense in depth + explicit intent),
   • catch per-directory exceptions as WARN so one bad parent dir does not abort the batch,
   • move parentDirectories.clear() and collectedFiles.clear() into a finally block so a failure in directory cleanup never causes already-deleted paths to be re-submitted on the next 1000-file flush.

No schema / config / Thrift IDL changes. No public API / SPI signature changes. HadoopFileIO-based tables are completely unaffected — the new guards return true and code follows the original path. BasicTableTrashManager and other callers of deleteEmptyDirectory benefit from the same guard for free.

Are you willing to submit a PR?

• Yes I am willing to submit a PR!

Code of Conduct

• I agree to follow this project's Code of Conduct

[j1wonpark]

Thanks for the report! The root cause checks out — for object-store FileIOs the single-arg checkArgument(boolean) in asFileSystemIO() does throw IllegalArgumentException: null.

question: I can't quite square the "reproduces 100%" claim with the io.exists() guard right before the throwing line:

if (!io.exists(directoryPath)) return;   // TableFileUtil:90
... io.asFileSystemIO() ...              // :95 — throws

For S3FileIO, exists() is a HEAD on the exact key, and directoryPath is a slash-less prefix (s3://.../data) → 404 → false → returns at line 90, never reaching 95. Verified on our own S3FileIO-backed catalog: a HEAD on the slash-less prefix key returns 404, and I couldn't reproduce the failure there.

Could you share:

1. the exact io-impl + Iceberg version,
2. which object storage you're on (AWS S3, Ceph RGW, MinIO, etc.), and
3. whether a HEAD on the slash-less prefix key returns 200 in your env?

Reaching line 95 seems to need exists()==true for that prefix, which looks storage/environment-specific rather than universal.

[felixhzhu]

Thanks for the report! The root cause checks out — for object-store FileIOs the single-arg checkArgument(boolean) in asFileSystemIO() does throw IllegalArgumentException: null.

question: I can't quite square the "reproduces 100%" claim with the io.exists() guard right before the throwing line:

if (!io.exists(directoryPath)) return; // TableFileUtil:90
... io.asFileSystemIO() ... // :95 — throws
For S3FileIO, exists() is a HEAD on the exact key, and directoryPath is a slash-less prefix (s3://.../data) → 404 → false → returns at line 90, never reaching 95. Verified on our own S3FileIO-backed catalog: a HEAD on the slash-less prefix key returns 404, and I couldn't reproduce the failure there.

Could you share:

1. the exact io-impl + Iceberg version,
2. which object storage you're on (AWS S3, Ceph RGW, MinIO, etc.), and
3. whether a HEAD on the slash-less prefix key returns 200 in your env?

Reaching line 95 seems to need exists()==true for that prefix, which looks storage/environment-specific rather than universal.

Thanks for the careful read! You're right about the JDK / Iceberg behavior in your environment, but I think the assumption "S3FileIO + non-trailing-slash prefix → HEAD 404 → false → 90 returns" doesn't hold across all the object stores / call shapes hit by this code path. Let me share details and walk through why the fix on the entry-point is still the right place.

The exists() path

AuthenticatedFileIOAdapter.exists() for non-AuthenticatedFileIO backends ends up calling AuthenticatedFileIO.super.exists(path), which is:

default boolean exists(String path) {
  InputFile inputFile = newInputFile(path);
  return inputFile.exists();
}

For S3FileIO this is indeed a HEAD on the key. So far so good — your read is correct.

Why HEAD is not always 404 in practice

Three things in the actual call chain mean exists() can return true and we do reach line 95:

1. directoryPath is not the top-level data/ prefix.
   RollingFileCleaner.doCleanFiles() feeds deleteEmptyDirectory() with the parent dir of every deleted data file, e.g.
   cos://my-bucket-1250000000/warehouse/db/t/data/op_time_day=2024-01-01/op_time_hour=12.
   Many object stores (Tencent Cloud COS / Aliyun OSS / Huawei OBS / Ceph RGW / some MinIO setups) write a 0-byte placeholder / directory marker when objects are uploaded under a prefix, and HEAD on those placeholder keys returns 200.

2. S3-compatible implementations don't all follow strict AWS S3 semantics.
   Several proxies / gateways translate HeadObject(prefix) internally to ListObjects(prefix=..., max=1), returning 200 if the prefix is non-empty. AWS S3 itself doesn't do this, but a lot of the S3-compatible ecosystem does.

3. deleteEmptyDirectory is recursive. It walks up to the parent partition. For partitions that still have sibling sub-partitions, the prefix is necessarily non-empty, which on placeholder-friendly stores means HEAD = 200, hitting line 95.

I've reproduced this 100% of the time on Tencent Cloud COS (S3-compatible API, accessed via Iceberg's native S3FileIO with endpoint=cos.<region>.myqcloud.com — not through cosn:// / Hadoop FS). On AWS S3 you can reproduce it deterministically by leaving a 0-byte placeholder under data/<partition>/ (which is exactly what the AWS S3 console "Create folder" action does).

Even if exists() were always false, this code shape is a latent trap

Putting asFileSystemIO() after exists() only "accidentally avoids" the precondition on AWS S3 strict semantics. Any future refactor (switching to LIST-based existence check, going through SupportsPrefixOperations, etc.) re-arms the trap. Routing on supportFileSystemOperations() at the entry point matches the contract AuthenticatedFileIO itself uses everywhere else (asPrefixFileIO(), asBulkFileIO(), asFileRecycleIO() all pair with supportXxx()), and additionally protects every other caller of deleteEmptyDirectory (e.g. BasicTableTrashManager).

Direct answers

• io-impl + Iceberg version: production runs org.apache.iceberg.aws.s3.S3FileIO against Tencent Cloud COS (S3-compatible API). Cross-checked envs: S3FileIO on AWS S3 (Glue) and org.apache.iceberg.io.ResolvingFileIO on a Polaris-backed REST catalog. Iceberg 1.6.1 in production. (Side note: the Amoro repo pom.xml currently declares 1.7.2, but the relevant code paths — S3InputFile.exists() / BaseS3File.exists() and SupportsPrefixOperations semantics — are unchanged across 1.6.x → 1.7.x, so the analysis applies to both.)
• Object store: production repro is Tencent Cloud COS via S3-compatible API (S3FileIO, not the cosn:// Hadoop FS connector). Cross-verified on AWS S3 (Glue) and a Polaris-backed REST + S3 catalog.
• Does HEAD on a non-trailing-slash prefix return 200? On AWS S3 strict — no. On Tencent Cloud COS / Aliyun OSS / Ceph RGW / some MinIO setups / S3-compatible proxies — yes (placeholder objects, or LIST-flavored HEAD semantics). And in our actual call shape directoryPath is most often a partition sub-directory rather than data/, which makes the placeholder case the common one. We see HEAD = 200 deterministically on COS for those partition sub-dir keys.

Repro on AWS S3 (in case you want one):

1. Glue + S3 catalog, partitioned Iceberg table.
2. Use the AWS console "Create folder" under s3://bucket/.../data/<some-partition>/ — this writes a 0-byte placeholder.
3. Commit enough rows / time so an expirable snapshot exists.
4. Trigger expireSnapshots → reproduces.

Happy to add a unit test that mocks an AuthenticatedFileIO whose supportFileSystemOperations() returns false to lock in the contract — currently the closest thing in tree only exercises HadoopFileIO.

[j1wonpark]

Thanks for the detail. Before treating this as established, I'd like to pin down the actual trigger, because line 95 is only reached when a HEAD on a slash-less key returns 200.

directoryPath comes from TableFileUtil.getParent() = new Path(path).getParent().toString(), which has no trailing slash (e.g. .../op_time_hour=12). For S3FileIO, exists() is BaseS3File.exists() → headObject(key = uri().key()) on that exact key, returning false on 404. If it's 404 we return at TableFileUtil:90 and never reach the throw.

That undercuts the three mechanisms on flat S3 (AWS/MinIO/Ceph):

• Directory markers are slash-suffixed (.../op_time_hour=12/), so a HEAD on the slash-less key is a different key → 404. These stores also don't auto-create markers on PUT.
• HeadObject is an exact-key op — a non-empty prefix or a LIST-style rewrite doesn't make it 200. Do you have a concrete gateway impl that does?

I also ran the AWS "Create folder" recipe and couldn't reproduce: the console writes .../<partition>/ (with slash) but the cleaner HEADs .../<partition> (no slash) → 404. So "100% on every object-store table" doesn't match what the code does on flat S3.

A HEAD returning 200 on a slash-less directory key would imply a hierarchical-namespace bucket rather than plain flat S3 — could you confirm the bucket type/mode? To make it reproducible, please share, for one failing partition dir, the raw aws s3api head-object --key <slash-less-key> response (200 vs 404) and the exact io-impl + storage config.

[felixhzhu]

Thanks for the detail. Before treating this as established, I'd like to pin down the actual trigger, because line 95 is only reached when a HEAD on a slash-less key returns 200.

directoryPath comes from TableFileUtil.getParent() = new Path(path).getParent().toString(), which has no trailing slash (e.g. .../op_time_hour=12). For S3FileIO, exists() is BaseS3File.exists() → headObject(key = uri().key()) on that exact key, returning false on 404. If it's 404 we return at TableFileUtil:90 and never reach the throw.

That undercuts the three mechanisms on flat S3 (AWS/MinIO/Ceph):

• Directory markers are slash-suffixed (.../op_time_hour=12/), so a HEAD on the slash-less key is a different key → 404. These stores also don't auto-create markers on PUT.
• HeadObject is an exact-key op — a non-empty prefix or a LIST-style rewrite doesn't make it 200. Do you have a concrete gateway impl that does?

I also ran the AWS "Create folder" recipe and couldn't reproduce: the console writes .../<partition>/ (with slash) but the cleaner HEADs .../<partition> (no slash) → 404. So "100% on every object-store table" doesn't match what the code does on flat S3.

A HEAD returning 200 on a slash-less directory key would imply a hierarchical-namespace bucket rather than plain flat S3 — could you confirm the bucket type/mode? To make it reproducible, please share, for one failing partition dir, the raw aws s3api head-object --key <slash-less-key> response (200 vs 404) and the exact io-impl + storage config.

You're right, and I want to retract the over-reach before anything else.

Where I was wrong

"100% on every object-store table" was sloppy. What I can actually claim is narrower:

100% reproducible on our production environment, with the IllegalArgumentException: null stack trace from AuthenticatedFileIOAdapter:98 via TableFileUtil.deleteEmptyDirectory:95 — i.e. a black-box symptom observation. Not "HEAD on a slash-less key returns 200 on every S3-compatible store."

Your code reading on strict S3 (BaseS3File.exists() → headObject(uri().key()) on the slash-less key → 404 → early return at line 90) is correct. On flat AWS S3 / MinIO / Ceph with no placeholder marker and no hierarchical-namespace mode, line 95 is unreachable. I should not have asserted otherwise without raw HEAD evidence.

What I don't yet have

I haven't captured the raw aws s3api head-object response on a failing partition dir on our side. Until I do, I'm not going to defend any specific mechanism. The plausible candidates I'd want to rule in/out — none of which I'm asserting as fact right now — are:

• (H1) A 0-byte placeholder at the slash-less key (left over by some historical write tool / SDK / aws s3api put-object --key path/to/dir/).
• (H2) Iceberg S3InputFile.exists() 1.6.1 falling through to a non-pure-HEAD path under some retry/transient condition.
• (H3) Bucket mode: this is the one I think is most likely given your line of reasoning. Our prod runs against Tencent Cloud COS via the S3-compatible API, and COS does offer a hierarchical-namespace ("directory-flavoured") bucket mode. I need to verify whether the affected bucket has it enabled — I don't want to guess.

What I'll attach on the next reproduction

Exactly your asks, plus a few I think are useful:

• one specific failing partition path (anonymised);
• raw response of aws s3api head-object --key <slash-less-key> (status code + body);
• raw response of aws s3api head-object --key <slash-suffixed-key> (control);
• aws s3api list-objects-v2 --prefix <slash-less-key> output (to see whether a placeholder marker actually exists);
• the table's io-impl and full s3.* / write.object-storage.* properties;
• AMS DEBUG-level log lines containing directoryPath from the failing run;
• explicit confirmation of the bucket mode (regular S3-compatible vs. COS directory-mode / hierarchical-namespace).

A repro that doesn't depend on the S3 mechanism

While I get those artefacts, I'd like to separate the question "does this PR fix a real contract violation" from "what exactly makes exists() return true on our COS bucket". The first one I can answer deterministically without any object store at all, because the bug is a contract violation between RollingFileCleaner / TableFileUtil.deleteEmptyDirectory and AuthenticatedFileIO, not an S3 quirk:

@Test
void deleteEmptyDirectory_throwsOnPureObjectStoreFileIO() {
  AuthenticatedFileIO io = mock(AuthenticatedFileIO.class);
  when(io.exists(anyString())).thenReturn(true);              // any path "exists"
  when(io.supportFileSystemOperations()).thenReturn(false);   // pure object-store contract
  when(io.asFileSystemIO()).thenCallRealMethod();             // hits Preconditions.checkArgument

  assertThatThrownBy(() ->
      TableFileUtil.deleteEmptyDirectory(io, "s3://bucket/db/t/data/p=1", Set.of()))
      .isInstanceOf(IllegalArgumentException.class)
      .hasMessage(null);
}

@Test
void deleteEmptyDirectory_isNoOpAfterFix() {
  AuthenticatedFileIO io = mock(AuthenticatedFileIO.class);
  when(io.exists(anyString())).thenReturn(true);
  when(io.supportFileSystemOperations()).thenReturn(false);

  TableFileUtil.deleteEmptyDirectory(io, "s3://bucket/db/t/data/p=1", Set.of());
  verify(io, never()).asFileSystemIO();
}

The two preconditions of the repro (exists() == true + supportFileSystemOperations() == false) are exactly the contract a pure object-store FileIO advertises. Anything that can satisfy the first half — by any mechanism, HEAD/LIST/marker/hierarchical bucket — trips line 95. Happy to push these as part of this PR; they run in milliseconds, no Testcontainers, no S3.

Why I still think the guard belongs at the entry point

Even granting your reading on strict flat S3, the same deleteEmptyDirectory body calls asFileSystemIO() three more times after the line-90 exists() guard:

if (!io.exists(directoryPath)) return;                              // line 90
String parent = new Path(directoryPath).getParent().toString();
if (!io.asFileSystemIO().isDirectory(directoryPath)                 // line 95
    || exclude.contains(directoryPath) || exclude.contains(parent)) return;
...
if (io.asFileSystemIO().isEmptyDirectory(directoryPath)) {           // line 100
  io.asFileSystemIO().deletePrefix(directoryPath);                   // line 101
  deleteEmptyDirectory(io, parent, exclude);
}

For the pre-fix code to be safe, every caller in every deployment in every bucket configuration would have to guarantee exists() returns false for every directoryPath the cleaner walks. The same author has already used the supportFileSystemOperations() guard at BasicTableTrashManager.java:104 and IcebergTableMaintainer.java:378; the new RollingFileCleaner path is the only place that broke the convention. Fixing the entry point re-aligns it.

Proposal

1. I'll push the two unit tests above to this PR shortly so you can reproduce the contract violation deterministically without any S3 dependency.
2. I'll follow up with the production HEAD artefacts (your full list above) once we re-reproduce on the COS bucket, and post them as a comment here so we can settle whether (H1)/(H2)/(H3) is the actual mechanism.
3. Until that lands, please treat the mechanism question as open on my side, and the fix's justification as resting on (a) the deterministic contract repro and (b) the three downstream asFileSystemIO() calls at lines 95/100/101 — not on my earlier "100% on every object-store table" wording, which I'm withdrawing.

Does that work as a path forward?