Skip to content

Commit d8e6ce7

Browse files
committed
fix(serverless): omit failed big-file copies from VolumeCache manifest (SLS-367)
_copy_parallel now returns (copied, current) source-path sets so callers can tell which files actually reached the volume. _do_sync records big_meta only for the copied|current union, so a big file whose copy failed is no longer written to the manifest as present -- preventing a later cold hydrate from serving stale volume bytes for it.
1 parent c876ec5 commit d8e6ce7

2 files changed

Lines changed: 85 additions & 12 deletions

File tree

‎runpod/serverless/utils/rp_volume_cache.py‎

Lines changed: 33 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -232,15 +232,30 @@ def _copy_file(self, src, dst):
232232
return False
233233

234234
def _copy_parallel(self, pairs):
235+
"""Copy every (src, dst) that needs it, in parallel.
236+
237+
Returns ``(copied, current)`` sets of source paths:
238+
- ``copied`` -- sources transferred successfully this call.
239+
- ``current`` -- sources skipped because the destination was already
240+
up to date.
241+
Their union is the set of sources the destination now holds a current
242+
copy of; a source in neither failed to copy. Callers that record a
243+
"mirror complete" manifest MUST include only sources in that union, or
244+
a failed copy would be marked present and later served stale. copy2 +
245+
atomic os.replace means a source in ``copied`` matches its destination
246+
at that instant, so no re-stat is needed to confirm it.
247+
"""
235248
todo = [(s, d) for s, d in pairs if self._needs_copy(s, d)]
249+
current = {s for s, _ in pairs} - {s for s, _ in todo}
236250
if not todo:
237-
return 0
251+
return set(), current
238252
with ThreadPoolExecutor(max_workers=self._max_workers) as pool:
239-
# Stream the success count rather than materializing a results list,
240-
# which would hold one entry per file for large (40k+) trees.
241-
return sum(
242-
1 for ok in pool.map(lambda sd: self._copy_file(sd[0], sd[1]), todo) if ok
243-
)
253+
# pool.map preserves input order, so zip pairs each result with its
254+
# source. This holds one path per copied file (unavoidable: per-file
255+
# identity is required to keep failed copies out of the manifest).
256+
results = pool.map(lambda sd: self._copy_file(sd[0], sd[1]), todo)
257+
copied = {s for (s, _), ok in zip(todo, results) if ok}
258+
return copied, current
244259

245260
@staticmethod
246261
def _within(base, path):
@@ -368,7 +383,8 @@ def _do_hydrate(self):
368383
# big/ so a malicious entry can't read outside the mirror.
369384
if self._is_safe_dest(dst) and self._within(self._big_root, src):
370385
big_pairs.append((src, dst))
371-
restored += self._copy_parallel(big_pairs)
386+
copied, _current = self._copy_parallel(big_pairs)
387+
restored += len(copied)
372388
if restored:
373389
log.info(f"VolumeCache: hydrated {restored} file(s) from {self._mirror_root}")
374390
return restored
@@ -425,8 +441,16 @@ def _do_sync(self):
425441
_silent_remove(self._small_archive_path)
426442

427443
big_pairs = [(f, os.path.join(self._big_root, os.path.relpath(f, "/"))) for f in big_files]
428-
big_copied = self._copy_parallel(big_pairs)
429-
big_meta = [m for m in (self._file_meta(f) for f in big_files) if m]
444+
copied, current = self._copy_parallel(big_pairs)
445+
# Record only sources the volume actually holds a current copy of.
446+
# A big file whose copy failed is in neither set, so it is omitted and
447+
# the manifest never claims it present -- hydrate then skips it rather
448+
# than restoring stale bytes left by the failed overwrite.
449+
mirrored = copied | current
450+
big_meta = [
451+
m for m in (self._file_meta(f) for f in big_files) if m and m["path"] in mirrored
452+
]
453+
big_copied = len(copied)
430454

431455
self._write_manifest(small_meta, big_meta)
432456
total = big_copied + small_transferred

‎tests/test_serverless/test_utils/test_rp_volume_cache.py‎

Lines changed: 52 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -637,6 +637,52 @@ def test_hydrate_does_not_resurrect_deleted_small_file(tmp_path):
637637
assert not b.exists()
638638

639639

640+
def test_sync_omits_big_file_from_manifest_when_copy_fails(tmp_path, monkeypatch):
641+
# The manifest is the "mirror is complete" marker. A big file whose copy to
642+
# the volume fails (disk full / transient error) must NOT be recorded in the
643+
# manifest; otherwise a later cold hydrate trusts the manifest and restores
644+
# the stale volume bytes as if current (or silently skips a missing file).
645+
vc, cache, vol = _mk_cache_with_volume(tmp_path)
646+
good = cache / "good.bin"
647+
bad = cache / "bad.bin"
648+
big = b"x" * (vcmod._SMALL_FILE_THRESHOLD + 10)
649+
good.write_bytes(big)
650+
bad.write_bytes(b"stale-v1" + big)
651+
652+
# First sync mirrors both files to the volume successfully.
653+
vc.sync(background=False)
654+
assert sorted(e["path"] for e in vc._read_manifest()["big"]) == sorted(
655+
[str(good), str(bad)]
656+
)
657+
658+
# Grow bad so _needs_copy detects the change (size differs); on the next
659+
# sync force only its copy to fail. good is unchanged and stays mirrored.
660+
bad.write_bytes(b"new-v2" + big + b"-and-then-some-extra-bytes")
661+
real_copy_file = vc._copy_file
662+
663+
def flaky_copy(src, dst):
664+
if src == str(bad):
665+
return False # simulate a failed overwrite (leaves stale v1 on volume)
666+
return real_copy_file(src, dst)
667+
668+
monkeypatch.setattr(vc, "_copy_file", flaky_copy)
669+
vc._do_sync()
670+
671+
# bad's failed copy must be absent from the manifest; good stays.
672+
big_paths = [e["path"] for e in vc._read_manifest()["big"]]
673+
assert str(good) in big_paths
674+
assert str(bad) not in big_paths
675+
676+
# Cold worker: both gone locally. Hydrate must restore good but never
677+
# resurrect the stale v1 bytes for bad.
678+
good.unlink()
679+
bad.unlink()
680+
fresh = VolumeCache([str(cache)], namespace="ep1", volume_path=str(vol))
681+
fresh.hydrate()
682+
assert good.read_bytes() == big
683+
assert not bad.exists()
684+
685+
640686
def test_join_pending_syncs_bounded_by_timeout(monkeypatch):
641687
vcmod._reset_pending_for_test()
642688
monkeypatch.setattr(vcmod, "_JOIN_TIMEOUT_SECONDS", 0.05)
@@ -817,11 +863,14 @@ def test_copy_parallel_copies_needed_only(tmp_path):
817863
dst2 = tmp_path / "d2"
818864
# pre-satisfy dst1 so it is skipped
819865
shutil.copy2(str(src1), str(dst1))
820-
n = vc._copy_parallel([(str(src1), str(dst1)), (str(src2), str(dst2))])
821-
assert n == 1
866+
copied, current = vc._copy_parallel(
867+
[(str(src1), str(dst1)), (str(src2), str(dst2))]
868+
)
869+
assert copied == {str(src2)} # only the missing dst was transferred
870+
assert current == {str(src1)} # already-current source reported as mirrored
822871
assert dst2.read_text() == "two"
823872

824873

825874
def test_copy_parallel_empty_is_zero(tmp_path):
826875
vc, _cache, _vol = _mk_cache_with_volume(tmp_path)
827-
assert vc._copy_parallel([]) == 0
876+
assert vc._copy_parallel([]) == (set(), set())

0 commit comments

Comments
 (0)