You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
fix: make bundle step refresh rollback transactional - #4827
Installed-step refresh with network access now snapshots the package and registry entry, copies the backup, and removes the step inside _step_install_transaction, using command_remove._remove_step_locked so the same non-reentrant flock is not taken twice, while the catalog reinstall stays outside the lock. On BundlerError, rollback acquires that lock again, rereads step-registry.json, and restores the backup and the original metadata only when the step id is missing, assigning the saved entry verbatim so installed_at and updated_at stay unchanged. When the id is already present, the later package and the rest of the reread registry are left in place. A failed copy or registry write is attached with add_note and the original install error is re-raised with the backup kept on disk; lock failure before a snapshot is wrapped as BundlerError, offline and not-yet-installed refresh still delegate to install without a backup, and the tests assert those lock, concurrency, and error-preservation paths.
Refreshing an installed custom step and then failing the reinstall could replace a newer copy of that step and its registry entry with the backup taken at the start of the refresh, and it could drop other step entries committed in between. If copying the backup back or writing the registry failed, that secondary error is what the caller saw, and the temporary backup was deleted. The package and metadata were snapshotted and the step directory was backed up before removal, outside the lock shared with step add and step remove, and the failure path then copied that backup onto the step directory with dirs_exist_ok. StepRegistry.save() replaces the entire registry file from the in-memory document loaded in the constructor, so a snapshot taken before a concurrent update overwrites entries committed later. An exception from the copy or the save propagated in place of the original BundlerError, and the cleanup that followed always removed the backup directory.
Tested locally with uv run specify --help
Not claimed: ran uv run python -m pytest tests/test_agent_config_consistency.py -q locally; it fails the same way on the base branch, so the failure predates this change (it fails the same way on main).
Ran existing tests with uv sync && uv run pytest
Not claimed: uv sync && uv run pytest was not run locally either.
Tested with a sample project (if applicable)
Not verified: this needs a person on the named hardware or environment.
Ran uv run python -m pytest tests/test_agent_config_consistency.py -q locally; it fails the same way on the base branch, so the failure predates this change (it fails the same way on main). Tests for this live in tests/specify_cli/bundles/test_primitives.py.
AI Disclosure
I did not use AI assistance for this contribution
I did use AI assistance (describe below)
AI disclosure
AI was used for assistance.
Extent: AI wrote the code changes and drafted this description.
Installed-step refresh with network access now snapshots the package and
registry entry, copies the backup, and removes the step inside
_step_install_transaction, using command_remove._remove_step_locked so
the same non-reentrant flock is not taken twice, while the catalog
reinstall stays outside the lock. On BundlerError, rollback acquires
that lock again, rereads step-registry.json, and restores the backup and
the original metadata only when the step id is missing, assigning the
saved entry verbatim so installed_at and updated_at stay unchanged. When
the id is already present, the later package and the rest of the reread
registry are left in place. A failed copy or registry write is attached
with add_note and the original install error is re-raised with the
backup kept on disk; lock failure before a snapshot is wrapped as
BundlerError, offline and not-yet-installed refresh still delegate to
install without a backup, and the tests assert those lock, concurrency,
and error-preservation paths.
Fixesgithub#4815
Assisted-by: AI
Rollback read step-registry.json directly, which bypassed StepRegistry's
symlink checks. It now reloads through StepRegistry._load and resolves
the steps base with resolve_steps_base_dir, _resolve_step_dir and
_reject_unsafe_destination before removing or copying the package, so a
steps directory or step directory swapped for a symlink during the
unlocked reinstall is refused and the backup is kept.
Adds tests for the initial lock failure (wrapped, no backup), a failed
rollback lock (original error kept with a note and the backup), and both
symlink swaps.
Assisted-by: Grok Build (model: unknown) and Claude Code (model: Claude Opus 5.5), autonomous
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019wSaUMZrtm6XSzwEcxavkv
@mnriem addressed both Copilot comments in 772a8be, and merged main to clear the test_primitives.py conflict (both sides' new tests kept):
Rollback no longer reads step-registry.json directly. It reloads through StepRegistry._load, which refuses a symlinked steps path or registry file, and resolves the step directory with resolve_steps_base_dir / _resolve_step_dir / _reject_unsafe_destination before any rmtree or copytree. If the steps tree or the step dir is swapped for a symlink during the unlocked reinstall, the restore is refused, the original install error is raised with a note, and the backup stays on disk.
Added tests for the lock-failure branches: initial lock failure is wrapped in BundlerError with no backup created, and a failed rollback lock keeps the original error with a note and the backup. Two more tests cover the steps-dir and step-dir symlink swaps and check nothing outside the project is read or deleted.
About test_catalog_versions.py::test_exact_add_uses_historical_url_digest_and_requirements: this PR doesn't touch it, and the same test fails on main too (the Oct 5 15:56 UTC Test & Lint Python run on main, windows-latest 3.13, and the fix/ps-probe-python3 PR run an hour later). I think _archive() is the cause: writestr stamps the zip entry with the current time, and the test builds the catalog digest and the served archive separately, so if the CLI call takes more than 2 seconds the digests differ. I left it alone since it is outside this PR. Happy to send a separate fix that pins the ZipInfo date_time if you want one.
Lint is clean with ruff check src tests. The full pytest suite ran green before the merge, and test_primitives.py again after it.
Disclosure: posted on behalf of @mvanhorn. The change was written by the Grok coding agent (model unknown, autonomous) and reviewed and verified by Claude Code (Claude Opus 5.5, autonomous).
refresh() decided the step was installed from the registry loaded at
manager construction. If a concurrent step remove committed before the
refresh transaction took its lock, the locked snapshot had no entry but
refresh still called _remove_step_locked, which reported "not installed"
and failed. The locked registry entry is now the deciding check: when it
is absent, refresh leaves the transaction without a backup and delegates
to install, which takes the same lock itself.
Adds a regression test that commits the removal just before the first
lock acquisition and checks that remove is not called, install runs once
outside the lock, and no refresh backup is created.
@mnriem the Copilot feedback is addressed in 3868a95. refresh() now re-checks the step under the lock and falls through to install when a concurrent remove got there first, with a regression test for that ordering. The bundles tests and ruff check are clean, and the full suite passed on my machine.
refresh() used StepRegistry.get() to decide whether the step was
installed, but get() returns None both for an absent id and for a key
whose value is JSON null. A null entry therefore skipped the snapshot
and removal and fell through to the non-force install path, which
rejects the id as already installed, and rollback skipped restoring a
None snapshot.
Installed-ness is now key membership via is_installed(), and the
snapshot is tracked by a separate flag so rollback writes a null value
back verbatim. Adds a regression test that seeds a null entry, fails
the reinstall, and checks the key, the null value and the package are
restored.
@mnriem addressed the new Copilot finding. refresh() now decides installed-ness by key membership (StepRegistry.is_installed) instead of get(), so a step whose registry value is JSON null gets snapshotted and removed like any other installed step instead of falling through to the non-force install path. The snapshot is tracked separately from its value, so rollback writes the null back verbatim.
Added test_step_refresh_restores_null_registry_entry_when_reinstall_fails, which seeds a null entry, fails the reinstall, and checks the key and null value come back along with the package. It fails without the fix. ruff check src tests is clean and the full test suite passes locally (9500 passed, 264 skipped).
The lock-failure coverage item Copilot still lists as open was handled in 772a8be (test_step_refresh_wraps_initial_lock_failure_without_backup and test_step_refresh_notes_rollback_lock_failure_and_keeps_backup).
Read registry under lock before deciding refresh mode
src/specify_cli/bundles/primitives.py:512
This online decision still uses self._registry, which was loaded in the manager constructor before any lock. If the step was absent then but a concurrent step add commits before this check, refresh delegates to the non-force install path; check_installable() rejects the now-installed step, so refresh fails instead of refreshing the state that already committed. Make online refresh determine presence from the registry read under _step_install_transaction; only the offline gate can safely use this early branch.
…r the lock
A BundlerError from _remove_step_locked was raised before the rollback
handler, so the finally block deleted the backup. Since the removal drops
the registry key before deleting the directory, a failed rmtree could lose
a JSON null entry and the step files. The removal error now goes through
the same rollback path as a failed reinstall: the snapshot is restored, or
kept on disk and named in the error if the restore fails.
Online refresh also no longer decides presence from the registry loaded at
manager construction. Only offline refresh skips straight to install;
online refresh reads the registry under _step_install_transaction, so a
step added after construction is snapshotted, removed and reinstalled
instead of failing check_installable().
Adds regression tests for a removal failure on a null registry entry and
for a step committed after the manager was built.
@mnriem both Copilot findings from the 10-09 round are fixed in 6173940. A BundlerError from _remove_step_locked now goes through the same rollback path as a failed reinstall, so the snapshot is restored instead of the backup being deleted. Online refresh now reads the registry under _step_install_transaction instead of trusting the one loaded at construction, so a step added later is backed up, removed and reinstalled.
Each has a regression test: a removal failure on a JSON null entry, and a step committed after the manager was built. The lock-failure tests Copilot asked about were already there. uv run pytest passes locally (9502 passed, 264 skipped), and ruff is clean.
This branch has not been deployed
No deployments
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
triage-can-waitVerdict: valid and in-scope but deprioritized; held behind the evidence gate
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.
mnriem
added
the
triage-can-wait
Description
Installed-step refresh with network access now snapshots the package and registry entry, copies the backup, and removes the step inside _step_install_transaction, using command_remove._remove_step_locked so the same non-reentrant flock is not taken twice, while the catalog reinstall stays outside the lock. On BundlerError, rollback acquires that lock again, rereads step-registry.json, and restores the backup and the original metadata only when the step id is missing, assigning the saved entry verbatim so installed_at and updated_at stay unchanged. When the id is already present, the later package and the rest of the reread registry are left in place. A failed copy or registry write is attached with add_note and the original install error is re-raised with the backup kept on disk; lock failure before a snapshot is wrapped as BundlerError, offline and not-yet-installed refresh still delegate to install without a backup, and the tests assert those lock, concurrency, and error-preservation paths.
Refreshing an installed custom step and then failing the reinstall could replace a newer copy of that step and its registry entry with the backup taken at the start of the refresh, and it could drop other step entries committed in between. If copying the backup back or writing the registry failed, that secondary error is what the caller saw, and the temporary backup was deleted. The package and metadata were snapshotted and the step directory was backed up before removal, outside the lock shared with step add and step remove, and the failure path then copied that backup onto the step directory with dirs_exist_ok. StepRegistry.save() replaces the entire registry file from the in-memory document loaded in the constructor, so a snapshot taken before a concurrent update overwrites entries committed later. An exception from the copy or the save propagated in place of the original BundlerError, and the cleanup that followed always removed the backup directory.
Fixes #4815
Testing
uv run specify --helpNot claimed: ran
uv run python -m pytest tests/test_agent_config_consistency.py -qlocally; it fails the same way on the base branch, so the failure predates this change (it fails the same way on main).uv sync && uv run pytestNot claimed:
uv sync && uv run pytestwas not run locally either.Not verified: this needs a person on the named hardware or environment.
Ran
uv run python -m pytest tests/test_agent_config_consistency.py -qlocally; it fails the same way on the base branch, so the failure predates this change (it fails the same way on main). Tests for this live intests/specify_cli/bundles/test_primitives.py.AI Disclosure
AI disclosure
AI was used for assistance.