fix(checkpoints): don't prune a project whose volume is merely unmounted#71117
Open
teknium1 wants to merge 3 commits into
Open
fix(checkpoints): don't prune a project whose volume is merely unmounted#71117teknium1 wants to merge 3 commits into
teknium1 wants to merge 3 commits into
Conversation
Orphan pruning decides a project is gone from a single probe:
if delete_orphans and (not workdir or not Path(workdir).exists()):
reason = "orphan"
then deletes its ref, index, and metadata — the project's entire checkpoint
history. `Path.exists()` is False for a deleted directory, but it is equally
False for one whose storage is not attached right now: an unplugged external
drive, a share behind a downed VPN, a bind-mount absent from this container,
an offline Windows mapped drive. The project is fine; only our view of it is.
This is not an opt-in maintenance command. `maybe_auto_prune_checkpoints`
runs unattended at startup from both `cli.py` and `gateway/run.py`, with
`delete_orphans=True` by default. So starting Hermes once while the drive is
unplugged silently destroys the restore points for every project on it — the
one thing checkpoints exist to provide, and there is nothing to restore from
afterwards.
Reproduced against the real store: a project registered under an unmounted
path and one on local disk, then a startup prune —
prune: {'scanned': 2, 'deleted_orphan': 1}
unreachable project index still on disk: False
The legacy pre-v2 branch has the same flaw plus a second one: a
`HERMES_WORKDIR` marker that exists but cannot be read leaves `workdir = None`,
which the same condition treats as an orphan. Failing to read a file is not
evidence that a project was deleted.
Require corroboration before deleting: the workdir's parent must be present,
so its absence is something we actually observed. A missing parent means the
volume is not there and we know nothing, so the entry is left alone — and an
unreadable marker never deletes at all. Genuinely abandoned projects are still
reclaimed, both by the unchanged orphan path (parent present, project gone)
and by the retention/stale rule, which runs off `last_touch` rather than a
filesystem probe.
tests/tools/test_checkpoint_manager.py: a project whose whole mount disappears
keeps its history; controls prove a genuinely deleted project is still pruned
and a live project is untouched. The data-loss test fails on main; both
controls pass there. 81 passed across the checkpoint suites (2 failures in
test_checkpoint_manager.py are pre-existing and fail identically on clean
main).
…eletion Addresses @egilewski's review: the parent-directory check still deleted checkpoint history for the most common unmount layout. Detaching storage removes the parent outright in some layouts (`/Volumes/Ext/proj` on macOS, `/media/<user>/<label>/proj`), which the first commit handles. But in the classic static layout — `/mnt/volume/proj`, an fstab entry, a container bind-mount — unmounting removes the contents and leaves the mount point behind as an empty directory. `parent.is_dir()` is then true, the project is absent, and the startup sweep deletes its ref, index and metadata: exactly the case this PR set out to protect. Reproduced against the real predicate before this commit: mount root vanished (macOS) -> False ok empty surviving mount point -> True <-- history deleted really deleted (siblings) -> True ok An empty parent carries no information: it looks identical whether the volume was detached or the project was deleted. So require the parent to actually say something — it holds some other entry (we observed a populated directory that does not contain the project), or it is itself a live mount point (the volume is attached right now and demonstrably does not hold the project). The cost is that a project deleted out of an otherwise-empty parent is no longer reclaimed by the orphan rule. It is not leaked: the retention rule reads `last_touch` rather than probing the filesystem and still collects it, so reclamation is deferred, not lost. That is the right direction for a predicate whose false positive destroys a user's restore points unattended. `_dir_has_any_entry` stops at the first entry via `os.scandir` instead of materializing a listing, since a project root can hold a large tree. tests/tools/test_checkpoint_manager.py: `test_surviving_empty_mountpoint_ keeps_its_checkpoints` pins the reviewed case, and `test_empty_parent_project_ is_still_reclaimed_by_retention` pins the deferral above so the safety valve cannot silently regress into a leak. Both fail on the previous commit. The real-orphan control now seeds a sibling so it exercises a populated parent rather than the ambiguous empty one. 80 passed in the checkpoint suite; the 2 remaining failures (`TestGitEnvIsolation`, `TestClearFunctions`) fail identically on clean main.
…orphan classification Follow-up to the cherry-picked #69063: egilewski's review found that the _dir_has_any_entry(parent) guard treats ANY entry in the mount point's parent as proof the volume is attached — but unmounting exposes the UNDERLAY directory's own files (e.g. a .keep placeholder), so a populated underlying mount-point dir still classified the project as an orphan and deleted its ref/index/metadata. Reproduced on both main and the PR head. Attachment evidence is now positive instead of circumstantial: * _volume_evidence() records the parent directory's (st_dev, st_ino) identity in the project's metadata while the workdir is observably live (at _register_project/_touch_project time). A mount point resolves to the mounted filesystem's root while attached and to the underlay directory after detach — same path, different directory, different identity. * _workdir_is_observably_gone() now requires the parent visible at prune time to match that recorded identity before the populated-parent check can classify an orphan. A mismatch means a different directory (the underlay) is showing through — a detached volume, not an observed deletion. * Metadata without a recorded identity (written by older versions) is never orphan-classified — unsure never deletes; the retention/stale rule still reclaims genuinely abandoned projects off last_touch. * The frozen pre-v2 layout has no metadata channel for the identity, so it keeps the structural checks only (require_parent_identity=False). * A failed evidence probe on re-registration preserves the previously recorded identity — stale evidence can only make pruning MORE conservative. Windows: st_dev/st_ino of 0 (filesystems without file IDs, some network shares) is treated as "no evidence recorded", which falls into the conservative never-orphan path. os.path.ismount and Path.stat are cross-platform; no POSIX-only calls added. tests/tools/test_checkpoint_manager.py: adds egilewski's exact regression (checkpoint history for mnt/volume/project, detach exposes mnt/volume/.keep, prune with orphan deletion enabled → NOT deleted; fails on the bare cherry-pick, passes with this fix), plus no-recorded-identity conservatism and probe-failure identity preservation. His absent-parent/empty-parent/retention/genuine-deletion/ live-project controls all still pass. Reported-by: egilewski (review on #69063)
Contributor
૮ >ﻌ< ა ci reviewran on 7a74eb7 all good! |
alt-glitch
added
type/bug
Collaborator
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
An unmounted or detached volume no longer classifies its projects as orphans — checkpoint metadata survives, and attachment evidence must be positive rather than inferred from sibling directory entries.
Changes
_pre_v2_shadow_reposcarries the unreadable-marker guard so preview and prune stay in lockstep)_volume_evidence()records the workdir parent's (st_dev, st_ino) while the workdir is observably live; after unmount the same path resolves to a different directory identity, so an exposed.keepin the underlay no longer counts as attachment evidence. In doubt → unreachable, never orphan.Validation
.keepin underlayCredit: @Frowtek (base fix), @egilewski (underlay data-loss finding + repro).
Infographic