Skip to content

fix(cli): harden restore cleanup and bundle rollback (D-0286 D-1162 D-0760) - #945

Open
acamarata wants to merge 6 commits into
mainfrom
dw1-cli-4
Open

acamarata wants to merge 6 commits into
mainfrom
dw1-cli-4

Conversation

@acamarata

Copy link
Copy Markdown
Collaborator

Fix bundle reconciliation rollback and proxy restore cleanup on fresh origin/main (1a96c7bb).

D-0286: blocked outside this repository

The named Docker tests already use dockertest.Require, and TestHostSpecArgvSites walks source files without requiring .git (6dd4ebd4, #648). That commit deliberately removed the vendor secret-scan allowlist because vendor scans clean. gitleaks dir vendor --redact --no-banner --log-level error passes here. The remaining go:fmt gate invokes gofmt -l . in the separate plugins repository (free/ci/internal/gate_runners.go), which still reports vendored files. This task prohibits editing other repositories, so no D-0286 changes are included.

D-1162: fixed

Record dangling links with Lstat so checked apply accepts planned cleanup. Anchor recursive traversal and unlinks to the generated output directory, including deferred checked publication, so a swapped symlink cannot redirect cleanup into a neighboring directory. Regression tests cover dangling-link plan/apply and symlink swaps during direct deletion and before checked publication.

D-0760: fixed

A failed or refused reconcile now undoes newly added bundle members with plugin.UndoInstall, removes their ledger records, and restores prior ownership. Existing members and retained schemas survive. Cleanup failures remain visible alongside the reconcile error. Tests cover E403/E480/E481, preexisting members, and prior ownership during refused upgrades. Later app-ready failures retain their existing behavior.

Validation

  • Before the fixes, targeted regressions reproduced retained bundle directories/ledger records, omitted dangling-link removal, and deletion of an unrelated file after a symlink swap. The proxy tests also verify rollback after post-cleanup validation fails.
  • go test -mod=vendor ./internal/bundle/...: PASS.
  • go test -mod=vendor ./internal/build: PASS, including dangling-link checked apply, direct and deferred symlink swaps, and post-cleanup rollback.
  • CGO_ENABLED=0 go vet -mod=vendor ./internal/build ./internal/bundle/...: PASS.
  • CGO_ENABLED=0 go build -mod=vendor -o <scratch>/nself ./cmd/nself/: PASS.
  • make fmt-check and git diff --check: PASS.
  • gitleaks dir vendor --redact --no-banner --log-level error: PASS; gofmt -l vendor lists upstream vendored files, confirming the remaining external formatting-gate issue.
  • Checks used Go 1.27.2 on macOS/arm64. No Docker container tests were run.

No changes to HQ, other repositories, versions, releases, deployments, or repository settings.

acamarata and others added 3 commits October 25, 2026 10:59
…thout the fix (D-0760)

Persist the ledger before the members exist so Store.Update does not bootstrap a task record from disk, and assert the prior installed_by before the install.
@acamarata

Copy link
Copy Markdown
Collaborator Author

Review fix (5109aba): TestBundleInstallRefusedUpgradeRestoresOwnership now persists an empty ledger before the members are written. Before, Store.Update bootstrapped from the members on disk and inferred a task record, so the test passed without the restoration code. It also asserts the prior installed_by / bundle records before the install. With the finishInstall undo wiring reverted to origin/main, the false subtest fails: it ends up with an extra task bundle, installed_by [existing task] and version 1.0.0. With the fix, it and the rest of ./internal/bundle/... pass, and vet and gofmt are clean.

@acamarata

Copy link
Copy Markdown
Collaborator Author

Review fix (b11e99e): the reconcile undo now runs only for pre-commit failures. Once reconcile.Apply has written the generated files, failures in the container swap (orphan removal, service start) and the plan render are marked appready.ErrCommitted in cmd/commands/bundle_reconcile.go. finishInstall skips undoFreshMembers for those, so plugin directories and ledger records stay and match the committed config. Refusals (E403/E480/E481) and Apply errors are still undone.

Tests: TestBundleInstallPostCommitReconcileFailureKeepsMembers (bundle) and TestExtensionReconcileSwapFailureIsCommitted (cmd: remove failure, start failure, prod refusal not marked). Both fail with the fix reverted and pass with it. go test for cmd/commands, internal/bundle/... and internal/reconcile passes; vet, build and gofmt are clean.

@acamarata

Copy link
Copy Markdown
Collaborator Author

Fixed the P1 (30f207c). reconcile.Apply now marks a failure after the generated files were published and kept (the state record write) with reconcile.ErrPublished. reconcileAfterExtension wraps that as appready.Committed, so finishInstall keeps the fresh members instead of undoing them. A write-build failure is unchanged: the build rolls its own files back, so it carries no mark. Regression tests: TestApplyRecordWriteFailureIsPublished (reconcile) and TestExtensionReconcileRecordFailureIsCommitted (cmd/commands). Both fail on the previous head and pass now. Also green: internal/reconcile, internal/bundle, internal/build, cmd/commands, vet.

… so bundle install keeps its members (D-0760)
@acamarata

Copy link
Copy Markdown
Collaborator Author

Fixed the E125 finding. Build writes and commits every generated file before it returns the entitlement refresh error, but ApplyBuild returned that error without the ErrPublished mark, so the bundle install rollback removed members the published config references. ApplyBuild now marks a refresh failure that comes with a completed build result as published (unless the caller's Rollback restores the files). Test: TestApplyEntitlementRefreshFailureIsPublished fails on the previous head and passes now; reconcile, bundle, build, upgrade and cmd/commands tests pass.

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

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant