Skip to content

Share rollback artifact retention across remove and rollback - #600

Open
Mikola Lysenko (mikolalysenko) wants to merge 2 commits into
mainfrom
refactor/shared-rollback-retention-20261002
Open

Mikola Lysenko (mikolalysenko) wants to merge 2 commits into
mainfrom
refactor/shared-rollback-retention-20261002

Conversation

@mikolalysenko

@mikolalysenko Mikola Lysenko (mikolalysenko) commented Oct 2, 2026 •

Copy link
Copy Markdown
Collaborator

Fixes #559.

Removing one patch could delete the only local rollback data for other patches left active. remove and rollback built different garbage-collection keep sets: only rollback retained every remaining patch's original blobs. A later offline rollback then failed with missing_blob, even though the earlier removal had reported success.

Both commands now use ArtifactReferences::after_removal in core. It retains patched bytes for remaining patches, original bytes for remaining and removed-but-not-installed patches, and the corresponding diff archives. Artifacts referenced only by successfully removed patches remain collectible. Repair and scan pruning use the same sweep with their existing apply-only retention policy.

This removes 29 production lines net, the synthetic #beforeHash-pin file records, and duplicate retention rules. Cleanup operates on explicit sets of blob hashes and patch UUIDs rather than cloned patch records with rewritten hash fields. A real filename ending in #beforeHash-pin can no longer collide with a synthetic keep record. The artifact sweep also moves from the rollback command into core; commands retain their existing error reporting and partial-cleanup behavior.

Validation on 2e403800:

  • The new offline lifecycle regression fails on the base (remove swept the remaining patch's rollback data) and passes after the change. It covers normal remove, remove --skip-rollback, and scoped rollback: remove one of two installed patches, verify the other's manifest and blobs survive, then restore it offline.
  • 25 core cleanup tests passed. New filesystem coverage checks apply/removal retention, crawler misses, missing records, created files, filename collisions, archive retention, and dry-run/wet agreement.
  • 314 command and lifecycle tests passed across remove, rollback, repair, rollback coverage, and in-process remove/repair; one existing test is ignored.
  • 28 scan GC unit tests, eight pruning integration tests, and four scan lifecycle tests passed.
  • Workspace/all-features Clippy passed with -D warnings -A unused-variables; the allowance covers the existing macOS warning at python_crawler.rs:1950. Strict CI Clippy also passed. Changed code is formatted and git diff --check passes.

Full CI, all compatibility workflows, and the benchmark comparison passed: all 486 checks and all 13 workflows completed without failure on the final commit. All 39 benchmark scenarios were classified unchanged. The PR has been converted from draft to a regular PR; the automatic Bugbot review passed with no findings.

Selected after refreshing all 146 open issues, six open PRs, and the 20-comment architecture discussion #560. No open PR covered #559. The discussion identifies duplicated removal/rollback orchestration; sharing its retention policy fixes data loss across package managers in one focused change. Development uses a separate worktree.


Note

Medium Risk
Changes post-command blob GC for remove, rollback, repair, and scan prune; incorrect retention could delete rollback data or leave orphans, though behavior is heavily tested and narrows a known data-loss bug.

Overview
Fixes #559 by unifying garbage-collection “keep” rules for remove and rollback so scoped removal no longer sweeps another active patch’s original (beforeHash) blobs, which broke later offline rollback (missing_blob).

ArtifactReferences in core replaces duplicated CLI logic (pin_before_hash_blobs, synthetic #beforeHash-pin manifest rows, and sweep_unused_artifacts). after_removal keeps patched bytes for remaining manifest entries, originals for those entries plus removed-but-not-installed (crawler-miss guard), and matching diff archives; only artifacts solely tied to successfully removed patches stay collectible. for_apply drives repair and scan --prune with the existing apply-only policy. Sweeps use explicit hash/UUID sets via ArtifactReferences::sweep.

Docs (CHANGELOG, CLI_CONTRACT) and integration/unit tests (including a remove/rollback lifecycle regression) document and lock in the shared retention behavior.

Reviewed by Cursor Bugbot for commit 2e40380. Configure here.


Generated by Claude Code

@mikolalysenko Mikola Lysenko (mikolalysenko) added the refactor Structural change: duplicated code or logic, missing abstraction, layering, dead code label Oct 2, 2026
@mikolalysenko
Mikola Lysenko (mikolalysenko) marked this pull request as ready for review October 2, 2026 20:23
@mikolalysenko Mikola Lysenko (mikolalysenko) added the Ready for review Agent-verified: mergeable, CI green, Bugbot clean — awaiting human review label Oct 2, 2026
@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

[burn-down agent] Labeled Ready for review at 2e403800c04e2ce566f2877ae233ceabe4c515d0.


Generated by Claude Code

@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

Reviewed 2e403800c04e2ce566f2877ae233ceabe4c515d0. Ready to merge as-is; no actionable findings.

The shared retention sets keep active patches’ original and patched blobs, preserve rollback data for crawler misses, and avoid synthetic filename collisions. I checked both command callers, partial failures, dry-run/preserve-state handling, and the unchanged repair/scan cleanup policy.

Validation: all 25 focused core cleanup tests passed locally; the head merges cleanly with current main. Exact-head CI shows 479 successful checks and 8 skips, with no failures or pending checks; Bugbot is clean and no review threads are unresolved. Broader CLI/platform coverage comes from CI.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Ready for review Agent-verified: mergeable, CI green, Bugbot clean — awaiting human review refactor Structural change: duplicated code or logic, missing abstraction, layering, dead code

Projects

None yet

1 participant