Repository navigation
Conversation
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. Fixes github#4815 Assisted-by: AI
mnriem
added
the
triage-can-wait
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Rollback bypasses registry symlink protections, and new lock-failure branches lack coverage.
Review effort: Balanced
Findings: 1
Open (2)
What changed in this PR
Makes bundle step-refresh rollback concurrency-safe while preserving original install errors.
Changes:
- Locks snapshot, removal, and rollback operations.
- Preserves newer commits and adds rollback regression tests.
| File | Description |
|---|---|
src/specify_cli/bundles/primitives.py |
Implements transactional refresh rollback. |
tests/specify_cli/bundles/test_primitives.py |
Adds concurrency and failure-path coverage. |
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| except step_installer.StepInstallError as exc: | ||
| # Lock acquisition failed before any package or registry snapshot. | ||
| raise BundlerError( | ||
| f"Failed to refresh step '{component.id}': {exc}" | ||
| ) from exc |
|
Please address Copilot feedback and fix test & lint errors |
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
…p-refresh-rollback # Conflicts: # tests/specify_cli/bundles/test_primitives.py
|
@mnriem addressed both Copilot comments in 772a8be, and merged main to clear the test_primitives.py conflict (both sides' new tests kept):
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). |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The locked path still relies on a stale pre-lock installation snapshot, causing refresh to fail after a concurrent removal.
Review effort: Balanced
Findings: 1
Open (2)
Resolved since last review (1)
|
Please address Copilot feedback |
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.
There was a problem hiding this comment.
🟡 Changes recommended
Rollback still mishandles null registry entries and symlinked registry files.
2 open findings
1 resolved since last review
🧠 Review effort: Balanced
Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.
| entry = registry.get(component.id) | ||
| if entry is None: |
|
Please address Copilot feedback |
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). |
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.