Skip to content

fix(bundler): re-read the step registry when rolling back a failed step refresh - #4139

Open
jawwad-ali wants to merge 1 commit into
github:mainfrom
jawwad-ali:fix/bundler-step-refresh-rollback
Open

fix(bundler): re-read the step registry when rolling back a failed step refresh#4139
jawwad-ali wants to merge 1 commit into
github:mainfrom
jawwad-ali:fix/bundler-step-refresh-rollback

Conversation

@jawwad-ali

Copy link
Copy Markdown
Contributor

Problem

_StepKindManager.refresh documents its own contract:

Preserve an existing step until we've validated we can perform refresh. For already-installed steps, keep a backup and restore it if the remove+reinstall path fails.

The package half of that rollback works. The registry half is dead code:

except BundlerError:
    if backup_dir.exists():
        shutil.copytree(backup_dir, step_dir, dirs_exist_ok=True)
    if metadata is not None and not self._registry.is_installed(component.id):
        self._registry.add(component.id, metadata)
    raise

StepRegistry.__init__ snapshots the file once (self.data = self._load()) and is_installed reads only that snapshot.

Reproduction on current main (bf88c9f)

snapshot at construction:            is_installed('my-step') = True
after the entry is deleted on disk:  same object             = True   <-- stale
a fresh StepRegistry reads truth:                            = False

By the time the rollback runs, self.remove(component) has already deleted the entry from disk via workflow_step_remove — but self._registry's snapshot still contains it. So not self._registry.is_installed(...) is always False, and the restore never executes, in precisely the failure case it exists for.

Why it matters

After a failed specify bundle update, the user is left in a broken half-state: the step package is back on disk but unregistered.

  • specify workflow step list no longer shows it
  • the workflow engine cannot resolve the step type
  • a later specify workflow step add <id> refuses with "Step directory already exists"

…leaving them to clean up by hand.

Fix

Read the registry fresh at rollback time, so is_installed reflects what remove() actually did. The lazy import matches this class's own style — __init__ imports StepRegistry the same way.

No breaking change. Nothing on any success path is touched; only the already-failing rollback branch changes, and it changes from silently doing nothing to doing what it says.

Verification

  • Fail-before / pass-after: the new test fails on unpatched src (assert StepRegistry(tmp_path).is_installed("my-step") with the on-disk registry showing "steps": {}) and passes with the fix — 1 failed → 21 passed.
  • The test exercises the real removal path and fails only the re-install, which is what a catalog 404 / size-limit / type_key mismatch produces.
  • Scoped regression over tests/unit: no new failures vs a clean-main baseline captured on bf88c9f9.
  • uvx ruff@0.15.0 check src tests → clean

Written with assistance from Claude Code. Bug found, reproduced, and verified by me on current main.

…fresh

`_StepKindManager.refresh` documents that it keeps a backup and restores it
"if the remove+reinstall path fails". The package half of that rollback
works; the registry half was unreachable.

`StepRegistry.__init__` snapshots the file once (`self.data = self._load()`)
and `is_installed` consults only that snapshot. Measured:

  snapshot at construction:          is_installed('my-step') = True
  after the entry is deleted on disk: same object            = True   <-- stale
  a fresh StepRegistry:                                      = False

By rollback time `self.remove()` has already deleted the entry from disk,
but `self._registry`'s snapshot still contains it — so
`not self._registry.is_installed(...)` was always False and the restore
never ran, in exactly the failure case it was written for.

The user was left with the step package back on disk but unregistered:
`workflow step list` no longer shows it, the engine cannot resolve it, and
a later `workflow step add <id>` refuses with "Step directory already
exists".

Read the registry fresh at rollback time.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@jawwad-ali
jawwad-ali requested a review from mnriem as a code owner August 15, 2026 14:17
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