Skip to content

fix(tools): make checkpoint restores consistent - #64966

Open
aahmed954 wants to merge 1 commit into
NousResearch:mainfrom
aahmed954:fix/checkpoint-restore-consistency
Open

fix(tools): make checkpoint restores consistent#64966
aahmed954 wants to merge 1 commit into
NousResearch:mainfrom
aahmed954:fix/checkpoint-restore-consistency

Conversation

@aahmed954

Copy link
Copy Markdown

Summary

  • remove files added after a checkpoint during full-directory restores
  • pin the target tree across pre-rollback retention pruning and immediate Git GC
  • preserve excluded files and keep path-specific restore behavior unchanged
  • add regression coverage for added-path removal, excluded-file survival, and oldest retained checkpoint restore

Why

A full restore previously used git checkout <commit> -- ., which restored paths present in the checkpoint but left files added afterward. Separately, creating the pre-rollback snapshot could exceed max_snapshots; pruning then rewrote retained commits and ran gc --prune=now, invalidating the requested target before restore.

Test plan

  • PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 uv run --isolated --python 3.12 --with pytest --with pyyaml pytest -q tests/tools/test_checkpoint_manager.py
  • Result: 80 passed in 7.78s
  • git diff origin/main...HEAD --check
  • Added-line security scan: no hardcoded-secret, shell-injection, eval/exec, or pickle patterns

Notes

  • No new dependencies or configuration surfaces
  • Temporary restore refs are deleted in finally
  • No duplicate open issue or PR found for this checkpoint restore bug class

Copilot AI review requested due to automatic review settings July 15, 2026 12:25
Remove files added after a full checkpoint restore and pin the target tree across retention pruning so rollback remains reliable after immediate garbage collection.
@aahmed954
aahmed954 force-pushed the fix/checkpoint-restore-consistency branch from ce6ea97 to b5014cb Compare July 15, 2026 12:28

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR improves the reliability and correctness of checkpoint restores in CheckpointManager, specifically for full-directory restores. It ensures that restores clean up paths introduced after the target checkpoint and makes restores resilient to retention pruning and aggressive git gc that could otherwise invalidate the restore target mid-operation.

Changes:

  • Pin the checkpoint’s target tree via a temporary ref before taking the pre-rollback snapshot, preventing retention pruning + GC from making the restore target unreachable.
  • For full-directory restores, switch from git checkout <commit> -- . to git read-tree --reset -u <tree> to also remove tracked paths added after the checkpoint.
  • Add regression tests for: removing added-after-checkpoint paths, preserving excluded files, and restoring successfully even when retention rewriting would otherwise invalidate the target.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
tools/checkpoint_manager.py Pins restore target tree across prune/GC and uses read-tree --reset -u for full restores to remove added tracked paths.
tests/tools/test_checkpoint_manager.py Adds regression coverage for added-path removal, excluded-file preservation, and retention-rewrite resilience.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread tools/checkpoint_manager.py Outdated
# complete tracked tree, including removing those added paths,
# without touching excluded files.
restore_args = ["read-tree", "--reset", "-u", restore_ref]
ok, stdout, err = _run_git(
@alt-glitch alt-glitch added type/bug Something isn't working comp/tools Tool registry, model_tools, toolsets P2 Medium — degraded but workaround exists labels Jul 15, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Competing with #64477 for the same checkpoint-restore data-safety fix. This PR also pins the target tree across pre-rollback retention pruning + gc --prune=now (which could invalidate the requested target before restore); #64477 focuses on the added-file removal via snapshot diff. Flagging the cluster so a reviewer can pick the canonical approach.

@tonydwb tonydwb left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review Summary

Verdict: Approved

Summary

PR #64966 makes checkpoint restores consistent: files added after a checkpoint are now removed on restore, and excluded files are properly preserved. Previously, added files would persist after restore, which could cause inconsistency.

Assessment

  • Correctness: Two new test cases cover the behavior.
  • Scope: Small focused change.
  • Risk: Low. Test-backed fix for a consistency issue.

No concerns


Reviewed by Hermes Agent

@teknium1

Copy link
Copy Markdown
Contributor

Thanks for the focused checkpoint-safety fix. The premise remains present on current main: tools/checkpoint_manager.py:819-830 snapshots before restore and then uses git checkout <commit> -- ., while _prune() rewrites history and runs immediate GC at tools/checkpoint_manager.py:1053-1118.

The patch pins the target tree before that snapshot, preserves the single-file checkout path, and changes only full restores to read-tree --reset -u. The added regressions cover later-added file removal, excluded-file survival, and both full/file restores across retention rewriting. Current main has no intervening changes to either touched file after the PR base (9df5f879), so this appears mechanically salvageable.

Automated hermes-sweeper review.

@teknium1 teknium1 added sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users labels Jul 16, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/tools Tool registry, model_tools, toolsets P2 Medium — degraded but workaround exists sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants