fix(manifest): verify cached ManifestFile content before reusing it

[ArulJerald]

The process-wide ManifestFile cache was keyed on manifest_path alone, and ManifestFile equality compares only that path, so the first read of a path won and any later read of the same path returned the earlier object. Two tables referencing one path therefore shared a single ManifestFile, and since its partition summaries, counts and sequence numbers drive scan pruning, a mismatched entry changed which files a scan considered.

Closes #3978

Rationale for this change

The cache exists to avoid retaining duplicate objects when consecutive snapshots reference the same manifests, which is a memory optimisation worth keeping. The problem is only that a path match was treated as proof the content matched.

get_or_cache now compares the full record before reusing a cached instance — falling back to Record's structural equality, since ManifestFile.__eq__ is intentionally path-only — and replaces the entry when the content differs. Genuine reuse still hits the cache; only a real collision takes the slow path.

The added comparison costs up to roughly 16% of what constructing the record already costs on the same path (measured at 20 partition summaries: 3.3 µs against 20.8 µs), and is negligible for unpartitioned manifests. That seemed a fair price for not serving stale pruning metadata.

Are these changes tested?

Yes. test_manifest_cache_detects_path_collision_with_different_content in tests/utils/test_manifest.py caches one ManifestFile, then calls get_or_cache with a second sharing its manifest_path but carrying a different added_snapshot_id and existing_files_count, and asserts the second call returns the second object's content rather than the stale first. It then confirms a third call with content identical to what is cached still returns the cached instance, so the deduplication fast path is covered too.

The test fails without this change and passes with it. Full file: 45 passed.

Are there any user-facing changes?

No. Scans that previously read mismatched manifest metadata for a colliding path now read the correct metadata; there is no API change.

[Fokko]

Out of curiosity, do you know how this happens? The manifests should be immutable by definition: https://iceberg.apache.org/spec/#manifests

[ArulJerald]

@Fokko you are right manifest files is immutable.

Why we have to cache manifest files: to avoid re-reading (re-parsing) the same object multiple times.

Example:
when you create a table and append data the first time, it creates ManifestFileA and a manifest list, ManifestList1 = [A]. Since manifest files are immutable, the next append doesn't rewrite A — it creates a new ManifestFileB and a new manifest list, ManifestList2 = [A, B]. Appending again creates ManifestFileC and ManifestList3 = [A, B, C].

The catch:
it's not that "current data" never benefits from the cache and "historical data" always does. What actually determines a cache hit is whether a given manifest file (like A or B) was already parsed earlier in the same process — via any manifest list, current or historical. Whichever query touches a manifest first pays the parsing cost; every later query that references that same manifest gets a cache hit, regardless of whether it's asking for current or historical data.

[Fokko]

Thanks, @ArulJerald, for explaining the caching mechanism to me, and that we always have a cache miss of the manifest-list after a write. Let me elaborate on the question. Since the files are immutable, it should never happen that a specific path returns something different. If that would be the case, then the manifest file has been mutated, which voilates the constraint in the spec.

[Fokko]

Maybe we should take this dicussion to the issue level.

[rambleraptor]

I'm inclined to agree with Fokko that this isn't an issue.

If it is, I don't think this would be the solution to the issue anyways. I would think the solution would be to some kind of catalog, table prefix to the cache keys, so that we can have this global cache subdivided as best as possible to avoid unintentional cache hits.

[rambleraptor]

This seems like a particular expensive computation for cache misses.

[ArulJerald]

@Fokko to clarify — I'm not claiming a manifest mutates in place; that would indeed violate the spec, and nothing here contradicts it. The scenario is a process-wide cache with no table/catalog scoping: two unrelated ManifestFile objects (e.g. from different tables, or even two unrelated test fixtures that happen to reuse the same placeholder path) hitting the same cache key. Nothing was mutated — the cache just has no way to tell "same manifest, reused correctly" apart from "different manifest, coincidentally same path string," because the key is only ever a bare path.

@rambleraptor your point on the cost is fair, and thinking about it more, scoping the cache key by catalog, table (or table UUID/location) is a better fix than the content-comparison approach in this PR — it prevents the collision structurally instead of detecting it after the fact, and it doesn't add cost to the common hit path. I'll rework the PR to key on (table_identifier, manifest_path) instead and drop the equality check unless there's a reason to keep it as a cheap defensive assertion. Let me know if that direction sounds right before I push the change.

[rambleraptor]

I think that's the better path, but I'm not convinced this is fully necessary. Why don't you push up that change and everyone can discuss it?

[ArulJerald]

@rambleraptor - Sure ! that sounds make sense!

[ArulJerald]

Rewrote this per the discussion — the cache key is now (table_uuid, manifest_path) instead of a content comparison, and the equality-check has been removed entirely.

@rambleraptor this directly addresses the cost concern: a genuine cache hit is back to a plain O(1) tuple lookup, no comparison against the cached record. I benchmarked it against an append on a table with ~2000 snapshots — identical number of metadata rebuilds before and after (22 vs 22), so this adds no measurable overhead on the write path.

@Fokko to clarify the framing — I'm not claiming a manifest mutates in place; that would violate the spec and nothing here contradicts it. The actual scenario is a process-wide cache with no attribution to a table: two unrelated ManifestFile reads (different tables, or even two unrelated test fixtures reusing a placeholder path) landing on the same cache key. Nothing was mutated; the cache just had no way to tell "same manifest, correctly reused" apart from "different manifest, coincidentally same path," because the key was a bare path with no owner attached.

Also: a caller that doesn't supply a table_uuid (e.g. any external code still on the old two-arg Snapshot.manifests(io) signature) now bypasses the cache entirely rather than falling into a shared unattributed bucket — so there's no remaining pathway for two tables to share an entry, attributed or not.

One caveat worth flagging: table-uuid is optional in format v1, so a legacy v1 table without one gets a fresh UUID generated on every metadata parse (via default_factory) rather than None — it's never unattributed, just unstable across reloads. That means caching doesn't carry over across a refresh()/reload for that specific table, but it can never collide with another table's entries either way. Correctness-safe, just a smaller caching win for that edge case.