Severity: Medium · Category: reliability
Affected code
crates/arco-iceberg/src/commit.rs:323-354, 426-442, 456-459crates/arco-iceberg/src/commit.rs:280-286crates/arco-iceberg/src/commit.rs:691-736crates/arco-api/src/config.rs:443-449
Problem
When a duplicate request finds an InProgress marker older than IN_PROGRESS_TIMEOUT (10 min, commit.rs:139), it takes the marker over by CAS-ing it to a new metadata_location/event_id (lines 323-349). Nothing revokes the original writer: the first request still holds its own pointer_version from line 217 and proceeds to write its metadata file and run the pointer CAS at lines 428-431. If the original writer wins that CAS, its commit is durable, but its finalize_committed_marker (line 458 → 713-736) CAS-es with the now-stale marker_version, gets Conflict, reloads, finds status InProgress (the takeover's), and returns CommitError::RetryAfter → HTTP 503. The taking-over writer then loses its own pointer CAS, and finalize_failed_marker (line 438 → 691-711) writes the marker as Failed with a cached 409 payload. From then on every replay of that Idempotency-Key hits the IdempotencyStatus::Failed branch at lines 280-286 and is answered with the cached 409 (CommitError::CachedFailure), even though the snapshot is live in the pointer.
Why it matters
This inverts the guarantee the idempotency marker exists to provide: a commit that is durably applied is reported to the client as a terminal failure, and the cached failure is replayed for the lifetime of the marker, so the client cannot discover the truth by retrying. A client that treats 409 as "my commit did not happen" will re-plan and re-append the same data files; a client that treats it as "someone else won" will silently drop work. It is reachable because there is no server-side request timeout on the Iceberg surface — to_iceberg_config sets request_timeout: None (crates/arco-api/src/config.rs:446), so a handler stalled on slow object storage can exceed the 10-minute staleness window while still alive and still holding a valid pointer version.
Failure scenario
Writer W1 POSTs a commit with Idempotency-Key: K and stalls >10 min inside step 7 (slow/retrying GCS write) while holding pointer_version = v1. The client times out and re-POSTs the identical body with the same key K (W2). W2 loads pointer v1, sees marker K InProgress with matching request_hash, existing.metadata_location != effective.metadata_location, is_stale == true → takes the marker over (marker m1→m2). W1 resumes, writes its metadata file, CAS-es the pointer v1→v2 successfully (commit is now live), then fails finalize_committed_marker on stale m1 → 503 to W1's client. W2 then CAS-es the pointer expecting v1 → Conflict → finalize_failed_marker stores Failed{409} under key K → 409 to W2's client. Every subsequent retry with key K replays the cached 409 while the table's current snapshot is W1's commit.
Suggested fix
Make the takeover fence the original writer instead of racing it. Two workable options: (1) re-read and re-verify the marker version immediately before the pointer CAS and abort the commit if the marker was taken over (marker_version no longer current) — this makes the marker, not the pointer, the single serialization point; or (2) before caching a failure, re-resolve the pointer and refuse to write Failed when the pointer already reflects a commit produced under the same idempotency key — i.e. in finalize_failed_marker, load the current pointer and, if pointer.current_metadata_location equals any metadata location previously recorded for this marker, finalize Committed with that location instead of Failed. Add a regression test that drives the interleaving with two CommitService calls against a MemoryBackend and asserts the replay of key K returns the committed response, not a cached 409.
Verification notes
Existing mitigations found (do not fully close it): Partial only, none closing the hole: (1) commit.rs:298-321 reconciles a duplicate whose marker location already equals the pointer's effective location — but it runs before the takeover, so it misses a W1 that commits afterwards; (2) build_takeover_metadata_location (commit.rs:755-770) prevents the takeover from colliding with the original's metadata object, which is what makes W1 and W2 both able to reach their pointer CAS; (3) gc.rs:438-500 reconciles InProgress markers against the pointer but never Failed ones, so it expires (PT1H + grace, config.rs:442) rather than repairs the wrong verdict; (4) the whole Iceberg REST surface defaults to enabled:false/allow_write:false and is not enabled anywhere in infra/terraform.
Caveat / scope: I did not execute the interleaving; the argument is static. The reachability estimate rests on object_store 0.11.2's documented defaults (30s request timeout, 180s retry budget) applied to three sequential puts between started_at and the pointer CAS, giving ~10.5 min worst case against a 10-min threshold — a ~30-second margin, so if a deployment sets a shorter storage timeout or a server request timeout, the window closes. I also could not confirm any production environment actually sets ARCO_ICEBERG_ENABLED=true; nothing in infra/terraform does.
Found during a staff-level audit of origin/main @ c3c0867. Mechanism confirmed by an independent adversarial verifier.
Severity: Medium · Category: reliability
Affected code
crates/arco-iceberg/src/commit.rs:323-354, 426-442, 456-459crates/arco-iceberg/src/commit.rs:280-286crates/arco-iceberg/src/commit.rs:691-736crates/arco-api/src/config.rs:443-449Problem
When a duplicate request finds an
InProgressmarker older thanIN_PROGRESS_TIMEOUT(10 min, commit.rs:139), it takes the marker over by CAS-ing it to a newmetadata_location/event_id(lines 323-349). Nothing revokes the original writer: the first request still holds its ownpointer_versionfrom line 217 and proceeds to write its metadata file and run the pointer CAS at lines 428-431. If the original writer wins that CAS, its commit is durable, but itsfinalize_committed_marker(line 458 → 713-736) CAS-es with the now-stalemarker_version, getsConflict, reloads, finds statusInProgress(the takeover's), and returnsCommitError::RetryAfter→ HTTP 503. The taking-over writer then loses its own pointer CAS, andfinalize_failed_marker(line 438 → 691-711) writes the marker asFailedwith a cached 409 payload. From then on every replay of that Idempotency-Key hits theIdempotencyStatus::Failedbranch at lines 280-286 and is answered with the cached 409 (CommitError::CachedFailure), even though the snapshot is live in the pointer.Why it matters
This inverts the guarantee the idempotency marker exists to provide: a commit that is durably applied is reported to the client as a terminal failure, and the cached failure is replayed for the lifetime of the marker, so the client cannot discover the truth by retrying. A client that treats 409 as "my commit did not happen" will re-plan and re-append the same data files; a client that treats it as "someone else won" will silently drop work. It is reachable because there is no server-side request timeout on the Iceberg surface —
to_iceberg_configsetsrequest_timeout: None(crates/arco-api/src/config.rs:446), so a handler stalled on slow object storage can exceed the 10-minute staleness window while still alive and still holding a valid pointer version.Failure scenario
Writer W1 POSTs a commit with
Idempotency-Key: Kand stalls >10 min inside step 7 (slow/retrying GCS write) while holdingpointer_version = v1. The client times out and re-POSTs the identical body with the same key K (W2). W2 loads pointer v1, sees marker KInProgresswith matchingrequest_hash,existing.metadata_location != effective.metadata_location,is_stale == true→ takes the marker over (marker m1→m2). W1 resumes, writes its metadata file, CAS-es the pointer v1→v2 successfully (commit is now live), then failsfinalize_committed_markeron stale m1 → 503 to W1's client. W2 then CAS-es the pointer expecting v1 →Conflict→finalize_failed_markerstoresFailed{409}under key K → 409 to W2's client. Every subsequent retry with key K replays the cached 409 while the table's current snapshot is W1's commit.Suggested fix
Make the takeover fence the original writer instead of racing it. Two workable options: (1) re-read and re-verify the marker version immediately before the pointer CAS and abort the commit if the marker was taken over (
marker_versionno longer current) — this makes the marker, not the pointer, the single serialization point; or (2) before caching a failure, re-resolve the pointer and refuse to writeFailedwhen the pointer already reflects a commit produced under the same idempotency key — i.e. infinalize_failed_marker, load the current pointer and, ifpointer.current_metadata_locationequals any metadata location previously recorded for this marker, finalizeCommittedwith that location instead ofFailed. Add a regression test that drives the interleaving with twoCommitServicecalls against aMemoryBackendand asserts the replay of key K returns the committed response, not a cached 409.Verification notes
Existing mitigations found (do not fully close it): Partial only, none closing the hole: (1) commit.rs:298-321 reconciles a duplicate whose marker location already equals the pointer's effective location — but it runs before the takeover, so it misses a W1 that commits afterwards; (2) build_takeover_metadata_location (commit.rs:755-770) prevents the takeover from colliding with the original's metadata object, which is what makes W1 and W2 both able to reach their pointer CAS; (3) gc.rs:438-500 reconciles InProgress markers against the pointer but never Failed ones, so it expires (PT1H + grace, config.rs:442) rather than repairs the wrong verdict; (4) the whole Iceberg REST surface defaults to enabled:false/allow_write:false and is not enabled anywhere in infra/terraform.
Caveat / scope: I did not execute the interleaving; the argument is static. The reachability estimate rests on object_store 0.11.2's documented defaults (30s request timeout, 180s retry budget) applied to three sequential puts between
started_atand the pointer CAS, giving ~10.5 min worst case against a 10-min threshold — a ~30-second margin, so if a deployment sets a shorter storage timeout or a server request timeout, the window closes. I also could not confirm any production environment actually sets ARCO_ICEBERG_ENABLED=true; nothing in infra/terraform does.Found during a staff-level audit of
origin/main@c3c0867. Mechanism confirmed by an independent adversarial verifier.