refactor(checkpoints): call the size-cap predicate instead of restating it
Review feedback on #95207: `_exceeds_size_cap` and `_drop_oversize_from_index`
each computed the byte cap and compared against it. Both used `> cap`, so they
agreed, but only by coincidence of two independent expressions — nothing held
them together.
The coupling is the whole point of the fix. The checkpoint decides what to
store and safe restore decides what may be deleted; a threshold that drifted
between them would produce a file both absent from the checkpoint and not
recognised as capped at restore, which is exactly the deletion this branch
exists to prevent. `_drop_oversize_from_index` now calls the predicate.
Added a boundary case to TestSafeRestore that pins the round trip from both
ends: a file at exactly the cap is stored, so it must revert; one byte more is
excluded, so it must be kept. Mutation-checked — moving either side to `>=`
fails it, including the re-inlined-with-`>=` shape the reviewer described.
No behaviour change: the byte cap, the strict comparison and the
unstattable-path result are all as before.
Regression: the 10 test files covering checkpoint_manager / rollback, against
current main (1fe0f2f3a, 134 commits newer than the base measured on the first
commit) — 130 passed on main, 135 here (+5 new), zero failures either side.
This commit is contained in:
@@ -472,6 +472,45 @@ class TestSafeRestore:
|
||||
assert "grew.txt" in result["restored_files"]
|
||||
assert "grew.txt" not in result.get("skipped_oversize", [])
|
||||
|
||||
def test_the_cap_boundary_is_the_same_on_both_sides_of_the_round_trip(
|
||||
self, work_dir, checkpoint_base, monkeypatch,
|
||||
):
|
||||
"""One threshold, checked from both ends.
|
||||
|
||||
The checkpoint decides what to store and safe restore decides what may
|
||||
be deleted, and the safety of the pair rests on the two agreeing. A file
|
||||
at exactly the cap is stored (the test is strictly greater-than), so it
|
||||
must revert; one byte more is excluded, so it must be kept. Were the
|
||||
checkpoint side to drift to ``>=`` while restore kept ``>``, the at-cap
|
||||
file would be stored nowhere and recognised as capped nowhere — the
|
||||
exact gap that deletes it — and this fails.
|
||||
"""
|
||||
cap = 1024 * 1024
|
||||
m = self._capped_mgr(checkpoint_base, monkeypatch, cap_mb=1)
|
||||
|
||||
at_cap = work_dir / "at_cap.bin"
|
||||
over_cap = work_dir / "over_cap.bin"
|
||||
at_cap.write_bytes(b"o" * cap)
|
||||
over_cap.write_bytes(b"o" * (cap + 1))
|
||||
base = self._checkpoint(m, work_dir)
|
||||
|
||||
at_cap.write_bytes(b"a" * cap)
|
||||
over_cap.write_bytes(b"a" * (cap + 1))
|
||||
m.record_agent_write(str(at_cap))
|
||||
m.record_agent_write(str(over_cap))
|
||||
|
||||
result = m.restore(str(work_dir), base, safe=True)
|
||||
|
||||
assert result["success"] is True
|
||||
# Stored, therefore recoverable, therefore reverted.
|
||||
assert at_cap.read_bytes() == b"o" * cap
|
||||
assert "at_cap.bin" in result["restored_files"]
|
||||
assert "at_cap.bin" not in result.get("skipped_oversize", [])
|
||||
# Never stored, therefore unrecoverable, therefore left alone.
|
||||
assert over_cap.read_bytes() == b"a" * (cap + 1)
|
||||
assert "over_cap.bin" in result.get("skipped_oversize", [])
|
||||
assert "over_cap.bin" not in result["restored_files"]
|
||||
|
||||
def test_safe_restore_still_deletes_a_small_agent_created_file(
|
||||
self, work_dir, checkpoint_base, monkeypatch,
|
||||
):
|
||||
|
||||
@@ -1409,8 +1409,7 @@ class CheckpointManager:
|
||||
Lets the agent keep snapshotting source code while refusing to
|
||||
swallow generated assets (datasets, model weights, logs, videos).
|
||||
"""
|
||||
cap = self.max_file_size_mb * 1024 * 1024
|
||||
if cap <= 0:
|
||||
if self.max_file_size_mb <= 0:
|
||||
return
|
||||
ok, stdout, _ = _run_git(
|
||||
["ls-files", "--cached", "-z"],
|
||||
@@ -1422,14 +1421,13 @@ class CheckpointManager:
|
||||
# whitespace but that leaves NULs alone; rebuild list.
|
||||
paths = [p for p in stdout.split("\x00") if p]
|
||||
abs_workdir = _normalize_path(working_dir)
|
||||
oversize: List[str] = []
|
||||
for rel in paths:
|
||||
try:
|
||||
size = (abs_workdir / rel).stat().st_size
|
||||
except OSError:
|
||||
continue
|
||||
if size > cap:
|
||||
oversize.append(rel)
|
||||
# Same predicate safe restore consults, called rather than restated:
|
||||
# a threshold that drifted between the two would make a file both
|
||||
# absent from the checkpoint and not recognised as capped at restore,
|
||||
# which is precisely the deletion this change exists to prevent.
|
||||
oversize = [
|
||||
rel for rel in paths if self._exceeds_size_cap(abs_workdir / rel)
|
||||
]
|
||||
if not oversize:
|
||||
return
|
||||
logger.debug(
|
||||
|
||||
Reference in New Issue
Block a user