fix: avoid lost-wakeup hang in DeleteFileIndex::get_deletes_for_data_file - #2696

…#2859)

## Which issue does this PR close?

<!-- No issue; concurrency bug found while reviewing the delete-file
loading paths. -->

N/A — a latent concurrency bug in the positional-delete loading path.

## What changes are included in this PR?

Fixes a lost-wakeup hang on the positional-delete wait path
(`PosDelLoadAction::WaitFor` in `arrow/delete_filter.rs`, awaited in
`arrow/caching_delete_file_loader.rs`).

`tokio::sync::Notify::notify_waiters()` stores no permit and only wakes
`Notified` futures that already exist. Previously `WaitFor` carried a
raw `Arc<Notify>`, and the waiter turned it into a `Notified` only
later, at `.notified().await` — after releasing the state lock. The
exact losing interleaving:

1. Loader calls `try_start_pos_del_load(path)` → `Load` (claims the
load).
2. Waiter calls `try_start_pos_del_load(path)` → `WaitFor(Arc<Notify>)`
— a raw notifier, **not yet a `Notified`**. The lock is released as the
call returns.
3. Loader calls `finish_pos_del_load(path)` → sets `Loaded`, fires
`notify_waiters()`. No `Notified` exists yet, so the wakeup is dropped.
4. Waiter calls `notify.notified().await`, creating its `Notified`
**after** step 3.
5. That `Notified` waits for the next `notify_waiters()`, which never
comes → hangs forever. Since positional-delete retrieval
(`get_delete_vector`) is synchronous, this waiter must not proceed
early, so the hang is on the critical path.

**Fix:** create the `Notified` under the state lock in
`try_start_pos_del_load` and carry it in `WaitFor(OwnedNotified)`. The
loader cannot fire `notify_waiters()` until it takes the write lock, so
the waiter's `Notified` is guaranteed to exist before the signal.

This is latent today (the loader does real parquet I/O before signaling,
so the window is rarely hit), but is one scheduling hiccup from a
permanent hang. It is the same bug class as the `DeleteFileIndex` site
in #2696.

## Are these changes tested?

Yes — adds a unit test
(`wait_for_completes_when_load_finishes_before_await`) that drives the
exact interleaving above through the real API and asserts the waiter
completes after `finish_pos_del_load`. It passes with this change and
hangs (times out) on the old `WaitFor(Arc<Notify>)` contract. No test
seam is needed because `WaitFor` hands the notifier to the caller.

Signed-off-by: Dhruv Arya <[email protected]>
Co-authored-by: Dhruv Arya <[email protected]>

…-live-schema', 'fix/safety-joinhandle-channel' and 'fix/rest-secret-debug-ssrf' into fix/post-d1-block-2026-07-24 (#171)

* docs(task): G3 unit brief — interop-pin the adjacent-split merge (row R148)

The authoritative spec for this unit: the M-1..M-5 proposition ledger (both
directions, non-vacuity, adjacency, fail-closed sabotage), the fixture
determinism requirement, the in-scope file list, and the named
path-vs-reference-identity residue carried forward on row R148.

Co-Authored-By: Claude Fable 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_01MRhAQnro5233xoVDLfib1X

* test(interop): pin the adjacent-split merge cross-engine (row R148)

The merge port (#169) landed with OFFLINE UNIT TESTS ONLY. The scan-plan oracle
could always REPRESENT a merge divergence — its member key is
`(basename,start,length)`, so `[(big,0,512),(big,512,512)]` and `[(big,0,1024)]`
compare unequal — but its FIXTURE never reached the branch: with big/mid/small +
one MoR delete at target=4096, no bin deterministically held two ADJACENT splits
of one file. Assertion granularity and fixture coverage are independent axes;
only the first was audited when R148 went green. MEASURED: with `merge_tasks`
deleted from `CombinedScanTask::new`, the pre-existing unfiltered comparison
still passes at 14 groups.

Two dedicated fixture files close it, each planned under an ISOLATING
metrics-prunable row filter (the fixture files own disjoint `id` ranges) at the
DELETE-FREE APPEND snapshot, so its splits meet an EMPTY bin-packer with weights
equal to their lengths — deterministic on any parquet build and any checkout
path, instead of tuned toward the 4096 knife edge that made the original failure
runner-only (the position delete attaches to every file of an unpartitioned
table and its size tracks the absolute path it embeds):

* merge.parquet — 120 rows / 2 row groups / whole file under TARGET, so both
  splits necessarily co-bin and merge into ONE spanning member
  (merge.parquet,4,fileLength-4); the key is independent of the row-group grid.
* gap.parquet — 210 rows / 3 row groups whose MIDDLE span alone exceeds TARGET
  while the outer two sum to under it, so first-fit co-bins splits 0 and 2
  NON-CONTIGUOUSLY: they must survive UNMERGED (adjacency, not group-by-file).
  Its wide rows use a mirrored 32-bit LCG over a 62-char alphabet — the engines'
  default parquet codecs differ (Rust uncompressed, Iceberg-Java zstd) and a
  low-entropy filler would compress the middle span back under TARGET on the
  Java side only.

Both plans are compared in BOTH directions against Java's REAL planTasks
(java_merge/gap_scan_plan.json <-> rust_merge/gap_scan_plan.json), and BOTH
engines assert NON-VACUITY — the merged member's length must STRICTLY exceed the
largest single field-132 row-group span — plus the sizing preconditions, so a
future fixture edit fails loudly instead of silently draining the pin.

Two new fail-closed legs: a Java sabotage leg (the UNMERGED split keys AND a
re-plan at a target too small to CO-BIN must both diverge from the canonical
merge-filtered plan; hard-fails if merge.parquet lacks multi-entry offsets), and
a PRODUCTION-SOURCE mutation leg that deletes the `merge_tasks(...)` call from
`CombinedScanTask::new`, requires the D1 leg to go RED, then restores the file
and md5-verifies the restore (hard-fails if the call site is absent or the mutant
passes). Chain [1/6] -> [1/7]. Also fixes the script's stale `row 146` citations
(R146 is conflict detection) and the same drift in tests/map.md.

ZERO production-code changes.

Co-Authored-By: Claude Fable 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_01MRhAQnro5233xoVDLfib1X

* test(interop): Critic S3 remediation — name the real proof, add the adjacency mutation, gate the signal

Independent Critic CONVERGED (zero S1/S2) with S3 advisories. All applied:

S3-1/S3-2 — the pin's non-vacuity claim was MISNAMED. `Σspans > max(span)` and
the M-4 `assert_ne!` are, given >=2 ascending offsets, arithmetically always
true: both sides derive from the same manifest facts, so neither discriminates.
The assertions are unchanged (they still catch a degenerate fixture) but every
CLAIM around them now names the real mechanism — the exact `assert_eq` on the
plan SHAPE (ONE group / ONE whole-file-spanning member for merge.parquet; TWO
groups with the outer pair INTACT for gap.parquet) read against the
offsets-aware-split invariant: the splitter emits exactly one sub-task PER SPLIT
OFFSET and ignores the target, so >=2 offsets means >=2 splits and a single
spanning member is only producible by the merge. Reworded in the Rust section
banner + asserts, the Java javadoc + throws, the R148 cell, both map.md files,
the script header and the lessons entry; the two numeric checks are relabelled
DEGENERATE-FIXTURE guards. The Rust and Java merge-length guards now read the
length out of the EMITTED plan (`member_length` / `memberLength`) instead of
recomputing it from the manifest.

S3-3 — one mutation could not cover both claims: deleting the merge outright
reds the merge leg but NOT the adjacency leg (with no merge at all the
non-contiguous pair survives for the WRONG reason). Stage [7] now runs TWO
mutations through a shared `mutation_leg` helper. Measured this run:
  merge-removal     (task_group.rs `tasks: merge_tasks(tasks),` -> `tasks,`)
                    RED at interop_scan_plan.rs:946 (merge-filtered assert)
  adjacency-removal (task.rs `&& self.start.checked_add(self.length) ==
                    Some(other.start)` -> `&& true`, i.e. group-by-file)
                    RED at interop_scan_plan.rs:953 (gap-filtered assert)
Each restores byte-identically (md5 7ced67df… / 738a21b2…), `touch`es (cargo's
mtime staleness check would otherwise reuse the mutant lib) and re-runs D1 GREEN.

S3-4 — gating the leg on `rc != 0` alone was a compile-failure false-green:
`cargo test` exits 101 for a BUILD error too. The assertion-signal grep
(`panicked at|assertion .*failed|test result: FAILED`) is now GATING — no signal
means restore + hard-fail as INCONCLUSIVE. Verified by running the chain with
the adjacency replacement swapped for a non-compiling one: exit 101, no signal,
"MUTATION [adjacency-removal] INCONCLUSIVE … most likely did not COMPILE",
script exit 1, source restored.

S3-5 — `InteropOracle.java` still cited bare `row 146` (:1524, :4804) and `row
122` (x4). All now cite the permanent anchors R148 / R124, with a note that R146
is CONFLICT DETECTION, not this harness.

S3-7 — `sabotagePerturbedTarget`'s javadoc and the dispatcher comment still said
"TWO independent corruptions" / "BOTH legs"; there are three. Fixed, and the
third leg is now documented in the `<ul>`.

S3-6 left as-is (reverting the Arrays.asList churn adds churn). LOW-8/9/11 left
unfixed; LOW-10 recorded as a comment on the mutation window — it is not
signal-safe and assumes no concurrent cargo build, so the nightly driver must
stay sequential across this stage.

Still ZERO production-code changes.

Co-Authored-By: Claude Fable 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_01MRhAQnro5233xoVDLfib1X

* docs(g3): close the three LOW framing residues from the Critic re-attestation

Comma splice in the R148 cell; the brief's M-3/done-gate lines now point at the
superseded framing note (the shipped discriminator is the plan-shape assert);
the todo tracker names both mutation legs and the degenerate-guard status of
the span inequalities.

Co-Authored-By: Claude Fable 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_01MRhAQnro5233xoVDLfib1X

* fix(datafusion): live provider schema + scan emits what it advertises (BUG-005/BUG-011)

`IcebergTableProvider` froze its Arrow schema at construction while every operation
reloaded the table, so a long-lived provider (the catalog's `SchemaProvider` caches one
per table for the session) planned forever against a schema that no longer described the
table, and the scan reloaded the table but let the stream adapter keep advertising the
construction-time schema.

* BUG-005 — the cached schema is now republished by every operation that loads the table
  (`scan`/`insert_into`/`delete_from`/`update`) and by a new `IcebergTableProvider::refresh`.
  Java's analog is Spark's `SparkTable`, resolved per query by `SparkCatalog.loadTable`, so
  it never outlives a plan; a provider that does outlive one has to converge instead.
* BUG-011 — `IcebergTableScan` no longer asks for `select_all()` on a reloaded table. It
  selects the ADVERTISED columns by name (minus the ones the scanned snapshot's schema
  cannot resolve, using the same `field_by_name` test `TableScanBuilder::build` applies) and
  conforms every emitted batch to the advertised schema: absent-and-nullable becomes an
  all-NULL column (Java's reader behaviour), a changed type or an absent required column is
  a loud, named error. This also fixes `SELECT *` after `ADD COLUMN`, which failed outright
  ("Column extra not found in table") because `ADD COLUMN` creates no snapshot.
* DML — `delete_from`/`update` bind the row filter, the `SET` assignment indices and the
  exec's projection base to the freshly loaded table's schema, not the cached one.
* `IcebergWriteExec` now advertises its RESULT schema (the data-file column) instead of the
  table schema it never emits — the same skew, in the write path.

Tests: 7 new provider/scan regressions (out-of-band evolution via a second catalog handle)
plus 4 `conform_batch` unit tests; each fix mutation-proved RED individually.

Co-Authored-By: Claude Fable 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_01MRhAQnro5233xoVDLfib1X

* fix(datafusion): bind the scan by FIELD ID, freeze the advertised schema, allow legal promotions

Critic remediation on top of e2a213f (2xS1 + S2 + S3 + LOW).

* S1-1 (silent wrong data on RENAME) — `resolve_projection` matched advertised columns
  against the scanned snapshot's schema BY NAME. A rename keeps the field id, rewrites the
  name and creates no snapshot, so `SELECT *` null-filled the renamed column over live
  data. Binding is now by FIELD ID (`PARQUET:field_id` on the advertised Arrow field ->
  `Schema::name_by_field_id`): the snapshot's name for that id is what gets selected, and
  `conform_batch` re-labels it to the advertised name. An advertised field with no id
  metadata is a loud error, never a name-based guess.
* S1-2 (DataFusion panic when the advertised schema shrank under a live plan) — the
  provider's schema is now IMMUTABLE for the life of the instance, because DataFusion
  stores projection ORDINALS resolved against it: a view or deferred DataFrame re-planned
  after the schema moved panics inside `datafusion-expr`'s `TableScan::try_new`. Freshness
  moves to a new INSTANCE, exactly as Java does it (`SparkCatalog.loadTable` per query):
  `IcebergSchemaProvider::table` now builds a provider from freshly loaded metadata on
  every resolution (it used to cache one per session — BUG-005 on the shipping path), and
  `IcebergTableProvider::refresh` becomes `refreshed()`, returning a NEW provider and
  leaving plans built on the old one valid.
* S2-3 (legal promotion rejected) — `conform_batch` applies Iceberg's legal type
  promotions (int->long, float->double, decimal precision widening) via an Arrow mirror of
  `iceberg::spec::is_promotion_allowed`, pinned against that authority over a 14x14
  primitive matrix. Illegal changes still error, naming the column and both types.
* S3-4 — the conform errors said "re-plan the query", which is a no-op for a
  snapshot-vs-current divergence; they now name the actual cause.
* LOW-5 — measured: in the steady state the reader's batch schema is IDENTICAL to the
  advertised one (fields AND `PARQUET:field_id` metadata), so the equality fast path hits
  and nothing is rebuilt. Kept, and pinned by a test so a reader change cannot silently
  move every scan onto the rebuild path.

Co-Authored-By: Claude Fable 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_01MRhAQnro5233xoVDLfib1X

* fix(datafusion): rebind pushed filters by field id; conform nested types (S2-1/S2-2)

Critic cycle-2 remediation on top of 2364c5b.

* S2-1 — a pushed-down filter PRUNES, and `Inexact` pushdown only lets DataFusion drop
  false positives, never resurrect a row the scan discarded. The filter was still
  name-keyed, so once names and field ids disagreed it either failed to bind ("Field opt2
  not found in schema" after a rename) or bound to a DIFFERENT column and silently
  returned the wrong rows (0 instead of 2 after drop+re-add; 0 instead of 1 after a name
  swap). Filters are now rewritten onto the scanned snapshot's names through the same
  field-id bindings the projection uses (`resolve_bindings` now covers the FULL advertised
  schema, since a filter may reference an unprojected column), and a filter over a column
  with no counterpart there is not pushed at all — its column reads NULL and only
  DataFusion's own re-check can evaluate it.
* S2-2 — `conform_batch` was top-level only, so `ADD COLUMN s.b` (which creates no
  snapshot) hard-failed a whole `SELECT *` where Java reads `{a: 5, b: null}`. Conforming
  now recurses through struct, list and map types with the same field-id resolution at
  every level, preserving offsets and null buffers, and names the dotted PATH (`s.a`) when
  a nested change is genuinely illegal.
* S3-3 — the promotion-mirror test covered 12 of 17 `PrimitiveType` variants. It now covers
  all 17 and encodes the one place the Arrow mirror is NECESSARILY coarser than the Iceberg
  rule: `uuid` and `fixed[16]` share one Arrow type, so the identity arm accepts a pair
  `is_promotion_allowed` rejects. Inert — `ensure_promotion_allowed` blocks it at the DDL,
  and the representations are identical — and now asserted rather than hidden.
* S3-4 — the projection narrowing is pinned through `get_batch_stream` itself, so a revert
  to `select_all()` goes RED instead of being masked by `conform_batch` narrowing the batch
  back down afterwards.

Co-Authored-By: Claude Fable 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_01MRhAQnro5233xoVDLfib1X

* fix(safety): honest join-failure propagation + close the DeleteFileIndex lost-wakeup hang (SAF-006/SAF-007)

Two hang/panic classes from the 2026-07-23 external audit, both on the shipping
read path.

SAF-006 - runtime/mod.rs `JoinHandle`
-------------------------------------
`Future::poll` collapsed EVERY join failure into `.expect("tokio spawned task
failed")`, so a panicking task's payload was destroyed and a *cancelled* task
(nothing failed) was reported with the same misleading string. There is exactly
one runtime arm in this crate (tokio) - no wasm/async-std/blocking alternative -
so this is the whole join-failure policy.

* Panic -> `std::panic::resume_unwind(join_error.into_panic())`. The waiter now
  observes the ORIGINAL payload, which is also the Java contract: `Future.get()`
  rethrows the task's own exception at the caller (`ExecutionException.getCause`
  is the original throwable; Iceberg's `Tasks`/`ExceptionUtil.castAndThrow`
  rethrows it unwrapped). The old `expect` was neither Java parity nor
  debuggable.
* Cancellation -> `Output = T` has no error channel, so a panic is unavoidable;
  it now says what actually happened instead of blaming the task.
* New `JoinHandle::try_join() -> Result<T>` maps BOTH cases to a typed
  `ErrorKind::Unexpected` (panic payload rendered into the message and logged via
  `tracing`; the `JoinError` kept as `source()` for a cancellation). Converted
  the two await sites whose signature already carries an error - the Avro and ORC
  data-file readers, where the panic source is third-party decode over untrusted
  file bytes. A malformed file now fails that read instead of unwinding into the
  awaiting scan.

This deliberately does NOT convert `Future::Output` from `T` to `Result<T, _>`;
that rewrite across every awaited AND detached spawn site stays deferred (as
#166 recorded), and `try_join` reaches the call sites that matter without it.

SAF-007 - delete_file_index.rs
------------------------------
The audit's stated mechanism (a full `channel(10)` hanging waiters) is STALE:
the populate task's `collect()` drains continuously, so a full buffer only
backpressures the senders, and the scan awaits the delete-manifest block, which
closes the channel. Two OTHER paths to a permanent hang were real, and are fixed:

* Lost wakeup. `get_deletes_for_data_file` observed `Populating`, released the
  read lock, and only then created its `Notified`. `notify_waiters()` stores no
  permit and `notified_owned()` snapshots the counter at CALL time, so a populate
  task that published in that window left the scan awaiting forever. The notifier
  is now armed while the read lock is still held (`lookup_or_arm`): the populate
  task cannot notify until it takes the write lock, so any notification follows
  the arming and is delivered. Same fix as upstream apache#2696 for
  this site (apache#2859 is the same class on the positional-delete path).
* Populate task death. The task is detached; if it unwinds or the runtime is torn
  down while it is parked on `collect()`, the state stayed `Populating` forever
  and no notification could ever be sent. A `PopulateGuard` armed before the
  task's first await now publishes a terminal `DeleteFileIndexState::Failed` on
  drop, so every waiter gets a typed error. Mirrors `EqDelState::Failed` in
  `arrow/delete_filter.rs` (#160).

Tests (+8, `-p iceberg --lib` 2855 -> 2863), each mutation-proved RED:
`test_await_re_raises_the_original_panic_payload`,
`test_try_join_maps_a_task_panic_to_a_typed_error`,
`test_try_join_maps_a_cancelled_task_to_a_typed_error`,
`test_try_join_describes_a_non_string_panic_payload`,
`test_try_join_returns_the_task_value`,
`test_waiter_is_armed_before_the_publisher_can_notify`,
`test_dead_populate_task_yields_a_typed_error_not_a_hang`,
`test_failed_index_state_is_terminal_for_later_callers`.
The two hang probes carry their own 5s timeout so a regression fails instead of
wedging CI. The panic-classification assertions read `Error::message()`, not
`to_string()`: `Display for Error` appends the source chain and a `JoinError`
renders the payload itself, which made a `to_string().contains(payload)` check
pass even with the conversion removed.

Known follow-up, NOT in this change: `arrow/delete_filter.rs` has the same
lost-wakeup pattern at three sites (`PosDelLoadAction::WaitFor(Arc<Notify>)` :112
awaited in `caching_delete_file_loader.rs` :276/:362, and the equality-delete
waits :232/:270). Upstream apache#2859 fixes the positional one and is a clean
cherry-pick; the equality-delete pair is not covered by it.

Co-Authored-By: Claude Fable 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_01MRhAQnro5233xoVDLfib1X

* fix(safety): Critic remediation — arm the populate guard at spawn, correct the Java-parity claim

Independent-Critic NOT-CONVERGED remediation of 099d4f3 (SAF-006/SAF-007).

S2-1 — the populate guard was armed on FIRST POLL, not at spawn
---------------------------------------------------------------
`PopulateGuard::new` was called inside the `async move` block, so it was a LOCAL
of the future, created on the first poll. A future dropped before it is ever
polled — a runtime torn down between `spawn` and the first poll — runs no local
destructors, so the guard never existed and the state stranded at `Populating`:
the exact hang the guard was added to close, on a path the parked-at-`collect()`
test did not reach. Construction is hoisted into the `spawn` prelude so the
future CAPTURES an already-armed guard; `Drop` now covers all three deaths
(never polled, unwound, teardown of a parked future). The two doc sentences that
over-claimed ("on every exit path", "Armed BEFORE the first await") are rewritten
to the guarantee that is now actually true.

S2-2 — the Java-parity justification was wrong
----------------------------------------------
Verified against the Java 1.10.0 reference: `Tasks.waitFor` unwraps ONLY `Error`
causes; for everything else it collects the `ExecutionException` WRAPPER, and
`ExceptionUtil.castAndThrow` then re-wraps it as
`RuntimeException(ExecutionException(cause))` — doubly wrapped, the opposite of
the previous "goes further and rethrows the unchecked exception unwrapped".
`Tasks.waitFor` also explicitly IGNORES `CancellationException`, which our
`Future` impl cannot do. The justification block now keeps the one true anchor
(`Future.get` throws `ExecutionException` whose `getCause()` IS the original
throwable — `resume_unwind` is parity on that alone), states the
`Tasks`/`castAndThrow` behaviour correctly, and NAMES the cancellation panic as a
deliberate divergence forced by `Output = T`.

S3s taken in the same pass
--------------------------
* S3-1: the Avro and ORC reader `try_join` errors now carry the file path via
  `Error::with_context("path", ...)` — a join error cannot name the file that
  killed the decoder.
* S3-2: documented why the `JoinError` is attached as `source()` only on the
  cancellation path (`into_panic` consumes it by value on the panic path).
* S3-3: added the populate-task UNWIND pin.
* S3-4: the two cancellation wordings are now one `CANCELLED_MESSAGE` const
  shared by the `Future` panic and the `try_join` error, so they cannot drift.

Tests: +2 (`-p iceberg --lib` 2863 -> 2865).
`test_never_polled_populate_task_yields_a_typed_error_not_a_hang` (new, pins
S2-1; mutation: move `PopulateGuard::new` back inside the `async move` block ->
RED at the 5s timeout while the parked and unwind pins stay green),
`test_unwinding_populate_task_yields_a_typed_error_not_a_hang` (new, S3-3),
plus the earlier eight, all still mutation-proved.

Not in this change: `arrow/delete_filter.rs` carries the same lost-wakeup class
at three sites and is ruled the next unit — see the PR body.

Co-Authored-By: Claude Fable 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_01MRhAQnro5233xoVDLfib1X

* fix(rest): redact secrets from REST wire-type Debug; SEC-011 is Java-parity, not a defect

SEC-010 — REST `Debug` impls printed live credential material.

`crates/catalog/rest/src/types.rs` derived `Debug` on every wire type, so any `{:?}`
or `tracing` site — including one in downstream user code this crate does not
control — printed the OAuth `access_token`, the server-vended `storage-credentials`,
and the `String -> String` config/property maps the REST server returns (which
`load_file_io` copies straight into the table's `FileIO` props). Ten types now carry
hand-written `Debug` impls following the house pattern established by #159/#162
(`StorageConfig`, `HttpClient::extra_headers`, the four catalog configs):

  TokenResponse (access_token -> "***"), CatalogConfig, LoadTableResult,
  StorageCredential, LoadViewResult, NamespaceResponse, CreateNamespaceRequest,
  UpdateNamespacePropertiesRequest, CreateTableRequest, CreateViewRequest.

Property maps are redacted PER KEY through the canonical needle test
`iceberg::io::is_secret_prop_key` (promoted `pub` in #162), so keys stay visible and
only secret VALUES are masked.

Same-family fix found during the sweep: `catalog.rs`'s REST-local `is_secret_prop_key`
was an EXACT match on `credential`/`token`/`client_secret` only, so `RestCatalogConfig`'s
own `Debug` printed `s3.secret-access-key`, `s3.session-token`, `gcs.oauth2.token` and
`adls.connection-string` in clear — the exact props operators hand this catalog for
FileIO. It now delegates to the canonical superset, removing the drift copy.

ASSESSED and deliberately left on `derive(Debug)` (anti-over-redaction), with the
rationale recorded in the module banner: `ErrorModel`/`ErrorResponse`, `OAuthError`
(RFC 6749 §5.2 diagnostic fields, already surfaced verbatim by their `From<_> for Error`),
and the identifier/pagination-only shapes. NAMED RESIDUE: core's derived
`TableMetadata`/`ViewMetadata` `.properties` and `TableUpdate::SetProperties` still print
in clear — a separate unit.

SEC-011 — REST `uri` / `oauth2-server-uri` SSRF: PARITY-SHARED, NOT A DEFECT.

Adjudicated against Java iceberg-core 1.10.0, which applies NO host, IP-range, or scheme
restriction: `HTTPClient.Builder.uri` only null-checks and `URI.create`s
(HTTPClient.java:483-491); `RESTSessionCatalog` passes the property straight through
(RESTSessionCatalog.java:175); `AuthConfig.fromProperties` takes `oauth2-server-uri`
verbatim (AuthConfig.java:85) and `OAuth2Manager` only WARNs when it is unset
(OAuth2Manager.java:274-291); the whole `core/.../rest/` tree contains no loopback /
`InetAddress` / site-local logic, and `rest.client.tls.configurer-impl` is an operator
EXTENSION point, not a restriction. A catalog URI is trusted operator configuration and
reaching a private endpoint is the normal deployment, so the audit's proposed private-IP
blocklist is rejected: it would break legitimate deployments and diverge from Java. The
trust model is documented on `REST_CATALOG_PROP_URI` / `get_token_endpoint`, and a
regression test pins loopback/RFC-1918/link-local acceptance so a future blocklist fails.

No behavioral change beyond `Debug` rendering; the wire format is untouched.

Tests: rest lib 82 -> 93 (+11). All 13 sabotage mutations (each Debug impl reverted to a
derive-equivalent raw render, both needle lists reverted to the old exact-match form, and
a private-IP blocklist added to `get_token_endpoint`) go RED against their guard test.
`cargo test -p iceberg --lib` 2855 green. The `--all-features` doctest failure
(`rt-multi-thread`) is pre-existing and reproduces identically on the base commit.

Co-Authored-By: Claude Fable 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_01MRhAQnro5233xoVDLfib1X

* fix(rest): Critic remediation — stop leaking raw response bodies via error context (F1); correct trust-model provenance and residue scoping (F2-F7)

Remediates the NOT-CONVERGED review of 38802b7 (1 S1 + 2 S2 + 4 S3/S4).

F1 (S1) — the raw-body leak defeated the whole unit. `deserialize_catalog_response`
attached the RAW 200-OK body via `.with_context("json", …)` on any parse failure, and
`iceberg::Error` renders context VERBATIM in both `Display` and `Debug`. Every type
redacted in the parent commit routes through that function — `CatalogConfig`
(catalog.rs:592), `LoadTableResult` incl. `storage_credentials` (:988/:1059/:1208),
`LoadViewResult` (:1439/:1475), `NamespaceResponse` (:810/:838) — so a single parse
failure printed exactly what the `Debug` impls had just masked. The trigger is realistic:
a server returning a payload this parser rejects (unknown V3 field, version skew) hands
back the same body carrying the vended `s3.session-token`.

Fixed with the pattern already used on the token endpoint (client.rs:295-307): attach
`status`, `url` and `response_body_len` — never the body. Two mockito guard tests drive
the Critic's probe shape end-to-end (`/v1/config` with a sentinel in `defaults` and
`overrides` missing; and a real `load_table` body carrying vended credentials, corrupted
so it cannot parse) and assert the sentinel is absent from BOTH renderings while the safe
diagnostics are present. Both go RED when the raw-body attach is restored.

`with_source` is retained (AGENTS.md requires the chain); `serde_json`'s message can echo
the offending token at the failure POSITION, never the surrounding payload. The
load_table test pins that bound. Documented as residue.

SCOPED, not changed: the NON-2xx path (`deserialize_unexpected_catalog_error`) still
attaches its body. A non-2xx response means no table was loaded, so no `config` overlay
and no vended credentials were issued; the body is an `ErrorResponse` whose `message` is
the diagnostic value, and Java surfaces exactly this (`ErrorHandlers`). The one non-2xx
body that could echo credentials — the token endpoint's — already withholds it. Rationale
recorded on the function.

F2 (S2) — the trust-model doc was factually wrong about provenance. `merge_with_config`
lets the SERVER's /v1/config `overrides.uri` replace `self.uri`, and server
defaults/overrides merge into `props`, so `oauth2-server-uri` can be server-supplied: a
compromised server can redirect the token POST — carrying the operator's `client_secret` —
to a third-party host. The doc now names BOTH provenances, adjudicates the server-override
variant as parity-shared (Java builds both the client and `AuthConfig.fromProperties` from
`mergedProps`, RESTSessionCatalog.java:211-230), and reconciles with the
`disable-header-redaction` strip: that key is a client-only logging control with no wire
meaning (pure downside if server-set, so stripped), whereas `uri`/`oauth2-server-uri`
server override is a documented functional part of the config protocol. No behavior change.

F3 (S2) — `CommitTableResponse` carries `metadata: TableMetadata` (with `.properties`); it
was wrongly in the "identifier/pagination-only, no value map" all-clear bucket. Moved into
the named core-crate residue paragraph.

F4 (S3) — corrected the `OAuth2Manager` citation to :275-293 and the guard wording: the
WARN fires only when the key is absent AND (`initToken != null` OR non-empty `credential`).

F5 (S3) — added `ViewVersion.summary` (derived `Debug`, `String -> String`, reachable via
`CreateViewRequest.view_version` and both view-metadata paths) to the named residue.

F6 (S3) — reframed `ErrorModel`/`ErrorResponse`/`OAuthError` from all-clear to RESIDUE:
their fields are server-controlled free text, and `message`/`error_description` are now the
one channel by which response-body content still reaches logs after F1.

F7 (S4) — added the transport-layer nuance: Iceberg configures no redirect policy on either
side, so Java inherits HC5 defaults (max 50, no cross-origin header strip) while this fork
inherits reqwest's (max 10, strips Authorization/Cookie/Proxy-Authorization cross-origin).
We are STRICTER, not looser — "no restriction of any kind" was imprecise.

Tests: rest lib 93 -> 95 (+2); 15/15 mutations RED (13 prior + F1a/F1b).
`cargo test -p iceberg --lib` 2855 green.

Co-Authored-By: Claude Fable 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_01MRhAQnro5233xoVDLfib1X

* fix(rest): Critic round-2 remediation — retract the refuted echo-bound, pin the double-encoded leak as known residue, correct F2 citations

Remediates the round-2 review of e985c5f (1 S2 + 2 S3, all doc/test level).

S2 — the echo-bound claim was REFUTED and is retracted. The previous residue note
asserted that serde_json's echo is "bounded to the single value at the parse-failure
position ... never the whole body", and concluded a secret was unreachable in practice.
That is wrong at a container boundary: a server or gateway that emits a nested object as
a JSON *string* (`writeValueAsString` over a sub-map, a proxy integration stringifying
the body — a real, well-known bug class) makes the value AT the failure position an entire
sub-document, which `Unexpected::Str` echoes in full. Reproduced through the public
`load_table` API with valid metadata and a double-encoded `config`: the whole vended
credential map prints verbatim in BOTH Display and Debug via `source:`.

Three fixes:
1. The RESIDUE block on `deserialize_catalog_response` is rewritten to state the true
   bound — the echo is the value at the failure position, and that value is an entire
   sub-document when the mismatch is at a container boundary, so a malformed
   credential-bearing body CAN still leak through `source`. The real fix is named as
   belonging to the core-crate unit that reworks `iceberg::Error`'s Display/Debug source
   rendering: AGENTS.md requires `source()` to SURVIVE, and it does — the chain obligation
   is satisfied; the leak is core's verbatim `source: {source}` interpolation.
2. New test `test_known_residue_double_encoded_body_leaks_through_error_source` pins the
   leaky shape AS KNOWN RESIDUE — it asserts the leak HAPPENS, with the flip condition
   spelled out (when the core rendering unit lands, invert the assertions and delete the
   doc block). A documented-leak pin, not a false-green.
3. The types.rs banner sentence claiming the REST credential channels are "covered here"
   is softened: covered at the `Debug` LAYER; the serde-source echo path remains.

Also corrected the load_table guard test's comment, which pinned the SAFE shape while
generalizing to "credentials elsewhere stay unreachable" — it now states that it pins the
safe scalar-mismatch shape only and points at the residue pin for the leaky one.

S3 — F2 citations were imprecise (same class as the F4 fixed last cycle).
`AuthConfig.fromProperties` is never called in RESTSessionCatalog.java and `.uri(` does not
appear at :211-230. Corrected to: RESTSessionCatalog.java:171-179 (default builder, `.uri`
at :175), :202-211 (init client + config fetch, then merge), :225-230 (rebuild + catalog
session); OAuth2Manager.java:104-109 (`catalogSession` → `AuthConfig.fromProperties`);
AuthConfig.java:84-85. Folded in the Critic's strengtheners: (a) server `overrides` OUTRANK
user properties — `merge` is defaults → clientProperties → overrides, last-write-wins, and
the only filter is `Maps.filterValues(merged, Objects::nonNull)` with no key allow/denylist
and an empty `validate()` (ConfigResponse.java:108-119, :68); (b) bootstrap parity — Java's
init client and config fetch use PRE-merge props and only then rebuild from mergedProps, and
this fork matches (`HttpClient::new(&self.user_config)` + `load_config(&client,
&self.user_config)` precede `merge_with_config`/`update_with`, catalog.rs:602-605), so the
config fetch can never be redirected by the response it is fetching; (c) Java has NO
`disable-header-redaction` analogue and filters nothing by key — the strip is fork-local
hardening of a fork-local knob, not a parity claim.

S3 — the non-2xx rationale was one-directional. `deserialize_unexpected_catalog_error` is
the fallthrough for create_table / create_namespace / update_namespace_properties /
create_view, whose request types were redacted precisely BECAUSE they carry credentials, so
a 400/409 echoing the offending request (a common validation pattern) puts those submitted
values into the error context. The note now states both directions explicitly — response
direction safe (a failed request vends nothing), request-echo direction NOT credential-free
— and keeps the body with the exposure named as parity-shared rather than implying the path
is credential-free.

No behavior change in this cycle.

Tests: rest lib 95 -> 96 (+1, the residue pin); 15/15 mutations RED.
`cargo test -p iceberg --lib` 2855 green.

Co-Authored-By: Claude Fable 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_01MRhAQnro5233xoVDLfib1X

---------

Co-authored-by: Claude Fable 5 <[email protected]>

…file (apache#2696)

N/A — concurrency bug found while reviewing apache#2590.

Fixes a lost-wakeup hang in
`DeleteFileIndex::get_deletes_for_data_file`.

`DeleteFileIndex` populates asynchronously on a background task that
signals completion with `tokio::sync::Notify::notify_waiters()`. Per the
tokio docs, `notify_waiters()` stores no permit and only wakes
`Notified` futures that have already been **created**.

`get_deletes_for_data_file` observes the `Populating` state, **releases
the read lock**, and only *then* creates the `Notified` future. If the
populating task flips the state to `Populated` and calls
`notify_waiters()` in that window, the wakeup is lost and the query
awaits forever.

The window is normally only a few instructions wide and is effectively
never hit, because the querier usually registers before the populator is
even scheduled. This is reachable from the `SnapshotValidator` commit
path (apache#2590), which drops the channel sender
immediately before querying — making the populator finish right as the
query starts.

Create the `Notified` future (via `notified_owned`) **while still
holding the read lock**. The populating task cannot call
`notify_waiters()` until it acquires the write lock, which is blocked
while the read lock is held — so the notification is guaranteed to
happen *after* the future has been created, and tokio guarantees such
notifications are delivered. No re-check needed.

The same latent pattern exists in `arrow/delete_filter.rs` and
`arrow/caching_delete_file_loader.rs`; this PR fixes only the index
site, and I'm happy to follow up on the others.

This production change is intentionally minimal (no test), because a
deterministic regression test requires a small test-only seam to make
the otherwise nanosecond-wide race window reliable. To keep that out of
this PR, the seam + test live in companion PRs on my fork:

- Reproduction only — the test **fails** (hangs → timeout),
demonstrating the bug:
dhruvarya-db#9
- The same fix **with** the regression test — the test **passes**:
dhruvarya-db#10

I'm happy to fold the regression test into this PR if maintainers
prefer.

`get_deletes_for_data_file` releases the read lock at **(B)** and only
creates its `Notified` future at **(C)**. The populating task can flip
the state to `Populated` and call `notify_waiters()` in the window
between (B) and (C). Because `notify_waiters()` stores no permit (unlike
`notify_one()`) and only wakes `Notified` futures created *before* the
call, a notification fired before the future exists is lost — and the
consumer then awaits forever:

```
 CONSUMER  get_deletes_for_data_file       PRODUCER  populating task
 ─────────────────────────────────         ──────────────────────────
 read lock: state == Populating   (A)
 clone notifier (Arc<Notify>)
 drop read lock                   (B)
        │
        │  ◄═══════ WINDOW ═══════►         write lock: state = Populated
        │                                   drop write lock
        │                                   notify_waiters()  ✗ no waiter registered yet
        │                                           → wakeup lost (no permit stored)
        ▼
 notifier.notified().await        (C)        (task has exited)
        │
        ▼
   ░░░ awaits forever ░░░  ← never woken → commit/query hangs
```

This is reachable in practice because observing `Populating` at (A) is
the common case: the consumer does only synchronous work between
dropping the sender and the read at (A), while the populator must first
be woken from its parked `collect()`, scheduled, and acquire the write
lock — so the consumer typically reaches (A) before the state flips.

**The fix** creates the `Notified` future at **(A)**, while the read
lock is still held. The populator cannot call `notify_waiters()` until
it takes the write lock (blocked while the read lock is held), so the
future always precedes any `notify_waiters()` and the notification is
guaranteed to be delivered.

Signed-off-by: Dhruv Arya <[email protected]>
Co-authored-by: Dhruv Arya <[email protected]>