Skip to content

Commit 3f53a70

Browse files
committed
fix(manifest): verify cached ManifestFile content before reusing it
The process-wide 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. Compare the full record before reusing a cached instance, falling back to Record's structural equality, and replace the entry when the content differs. Genuine reuse across consecutive snapshots still hits the cache. Closes #3978
1 parent 1312c55 commit 3f53a70

2 files changed

Lines changed: 47 additions & 3 deletions

File tree

pyiceberg/manifest.py

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -941,7 +941,8 @@ class _ManifestCache:
941941
942942
Consecutive snapshots often reference the same manifests after append
943943
operations, so reusing ManifestFile instances avoids retaining duplicate
944-
objects.
944+
objects. A path that matches but carries different content is detected,
945+
so a collision does not result in stale data being served.
945946
"""
946947

947948
DEFAULT_SIZE = 128
@@ -975,8 +976,11 @@ def get_or_cache(self, manifest_file: ManifestFile) -> ManifestFile:
975976

976977
with self._lock:
977978
manifest_path = manifest_file.manifest_path
978-
if manifest_path in self._cache:
979-
return self._cache[manifest_path]
979+
cached = self._cache.get(manifest_path)
980+
# ManifestFile equality compares manifest_path only, so fall back to the
981+
# full record to guard against a path collision with different content.
982+
if cached is not None and cached._data == manifest_file._data:
983+
return cached
980984

981985
self._cache[manifest_path] = manifest_file
982986
return manifest_file

tests/utils/test_manifest.py

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1324,6 +1324,46 @@ def test_manifest_cache_respects_positive_env_size(monkeypatch: pytest.MonkeyPat
13241324
importlib.reload(manifest_module)
13251325

13261326

1327+
def test_manifest_cache_detects_path_collision_with_different_content() -> None:
1328+
"""Test that get_or_cache does not serve stale content for a manifest_path collision."""
1329+
manifest_path = "s3://bucket/metadata/manifest.avro"
1330+
first = ManifestFile.from_args(
1331+
manifest_path=manifest_path,
1332+
manifest_length=1000,
1333+
partition_spec_id=0,
1334+
added_snapshot_id=1,
1335+
sequence_number=1,
1336+
existing_files_count=0,
1337+
)
1338+
second = ManifestFile.from_args(
1339+
manifest_path=manifest_path,
1340+
manifest_length=1000,
1341+
partition_spec_id=0,
1342+
added_snapshot_id=2,
1343+
sequence_number=1,
1344+
existing_files_count=5,
1345+
)
1346+
1347+
cached_first = manifest_module._manifest_cache.get_or_cache(first)
1348+
assert cached_first is first
1349+
1350+
cached_second = manifest_module._manifest_cache.get_or_cache(second)
1351+
assert cached_second is second
1352+
assert cached_second.added_snapshot_id == 2
1353+
assert cached_second.existing_files_count == 5
1354+
1355+
identical = ManifestFile.from_args(
1356+
manifest_path=manifest_path,
1357+
manifest_length=1000,
1358+
partition_spec_id=0,
1359+
added_snapshot_id=2,
1360+
sequence_number=1,
1361+
existing_files_count=5,
1362+
)
1363+
cached_identical = manifest_module._manifest_cache.get_or_cache(identical)
1364+
assert cached_identical is second
1365+
1366+
13271367
def test_manifest_cache_reads_size_from_configuration_file(monkeypatch: pytest.MonkeyPatch, tmp_path: Path) -> None:
13281368
"""Test that manifest-cache-size can be loaded from .pyiceberg.yaml."""
13291369
config_dir = tmp_path / "config"

0 commit comments

Comments
 (0)