Activity

1. mnriem

   added

   needs-triage

   bug

   triage-must-haveVerdict: high-value, important work for Spec Kit — do firstVerdict: high-value, important work for Spec Kit — do first

   on Oct 2, 2026
2. mvanhorn commented on Oct 3, 2026

   

   mvanhorn

   on Oct 3, 2026

   Contributor

   More actions

   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.

   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.
3. mnriem

   added

   bug-assess

   and removed

   needs-triage

   bug

   on Oct 15, 2026
4. github-actions commented on Oct 15, 2026

   

   github-actionsbot

   on Oct 15, 2026 – with GitHub Actions

   Contributor

   More actions

   Bug assessment — bundle-refresh-rollback: Valid · severity medium

   ---

   Bug Assessment: Bundle step refresh rollback is not transactional with step operations

   • Slug: bundle-refresh-rollback
   • Created: 2026-10-15T21:46:07Z
   • Source: issue [Bug]: Bundle step refresh rollback is not transactional with step operations #4815
   • Verdict: valid
   • Severity: medium

   Report (summarized)

   The report identifies two related defects in refreshing an installed custom workflow step. The refresh snapshots package and registry state, then removes and reinstalls the step, but the snapshot/removal and rollback operations are not coordinated with the shared step lock used by add/remove. A concurrent install can therefore be removed or overwritten by stale rollback state, and a stale registry snapshot can discard another concurrently committed entry. The report also says rollback failures can replace the original reinstall failure. The issue describes deterministic synchronization points and notes that existing tests cover only serial restoration.

   One comment confirms that the backup and metadata snapshot occur outside the shared lock, while the proposed change in the related discussion moves the package backup and removal into _step_install_transaction and leaves catalog reinstall outside the lock.

   Symptom

   Under concurrent custom-step mutations, a bundle refresh can restore an older package and registry entry over a newer committed step, or save a stale registry snapshot that loses an unrelated concurrent entry. If backup restoration or registry saving fails after reinstall failure, the restoration exception can mask the original BundlerError.

   Expected behavior is that snapshot/removal/rollback are atomic relative to step directory and registry mutations, later commits are not overwritten, and the reinstall error remains primary with rollback failure retained as diagnostic context.

   Reproduction

   1. Install a custom step and begin a bundle refresh.
   2. Pause after the refresh snapshots package and registry metadata but before removal.
   3. Concurrently force-install a newer version of the same step.
   4. Resume the refresh and force reinstall failure.
   5. Observe that stale rollback can remove the newer package and restore the older package/metadata.
   6. For registry loss, pause rollback after loading a fresh StepRegistry, commit another step concurrently, then resume save() and observe that the concurrent entry can be lost.
   7. For error preservation, make backup restoration or StepRegistry.save() fail after reinstall has raised BundlerError.

   The report does not specify the exact test hooks or synchronization mechanism; deterministic barriers should be added rather than timing sleeps.

   Suspected Code Paths

   • src/specify_cli/bundles/primitives.py:641-694 — _StepKindManager.refresh() snapshots metadata and the step directory before locking, calls self.remove(component) at line 660 (which acquires the lock independently), then restores the directory and creates/saves a new StepRegistry at lines 665 and 678-689 outside the shared lock.
   • src/specify_cli/workflows/step/installer.py:83-101 — _step_install_transaction() defines the shared .step-install.lock boundary for step directory and registry mutations.
   • src/specify_cli/workflows/step/command_remove.py:11-30 and :114-115 — _remove_step_locked() is the lock-held removal primitive, while the public remove command acquires _step_install_transaction() before invoking it. Calling the public remove path from refresh creates separate critical sections rather than one transaction.
   • src/specify_cli/workflows/step/catalog.py (imported by primitives.py) — StepRegistry loads a snapshot in its constructor and save() persists that snapshot; concurrent saves based on an older snapshot can overwrite entries committed after the snapshot.
   • tests/specify_cli/bundles/test_primitives.py:1002-1084 — existing regression coverage verifies serial restoration of the package and registry entry after reinstall failure, but does not synchronize concurrent mutations or exercise rollback-failure error preservation.

   Root Cause Hypothesis

   The root cause is a transaction-boundary mismatch: step add/remove use _step_install_transaction(), but bundle refresh performs its initial read, backup, rollback copy, and registry reload/save outside that same lock, and delegates removal through a separately acquired command boundary. Because StepRegistry is snapshot-based, a registry loaded before a concurrent mutation can later overwrite newer data. The rollback handler also uses a bare except BundlerError path without independently preserving/reporting exceptions from restoration. Confidence: high.

   Proposed Remediation

   Preferred: Add a refresh-specific transaction in the step domain that acquires _step_install_transaction() once for the package/registry snapshot, lock-held removal, and rollback restoration. Reuse _remove_step_locked() rather than the public remove command to avoid taking the non-reentrant lock twice. Keep catalog resolution/download/reinstall outside the lock if that is required for latency, but reacquire the same lock for any rollback and validate the current state before replacing it so a later committed package or registry entry is never overwritten. Preserve the original BundlerError as the raised exception; collect rollback failures as chained or secondary diagnostic context and retain the backup when restoration is incomplete.

   The transaction should explicitly define behavior when the step changed while the reinstall was in progress: abort stale rollback rather than deleting or overwriting a newer package, and merge/compare registry state rather than saving an old full snapshot.

   Alternatives:

   • Use a journal/rename-based commit protocol with a generation or version check for the package and registry. This can provide stronger stale-writer detection but is more invasive.
   • Hold the step lock across the entire catalog reinstall. This is simpler for serialization but unnecessarily blocks other step operations during network I/O and risks lock contention/timeouts.

   Files likely to change:

   • src/specify_cli/bundles/primitives.py
   • src/specify_cli/workflows/step/installer.py and/or src/specify_cli/workflows/step/command_remove.py for a reusable lock-held refresh/remove primitive
   • tests/specify_cli/bundles/test_primitives.py
   • Potentially step installer/command tests covering lock and rollback diagnostics

   Tests to add or update:

   • Deterministically pause after refresh snapshot and concurrently commit a newer version of the same step; verify failed refresh does not remove or restore over the newer package/metadata.
   • Deterministically pause after loading rollback registry state and concurrently add another step; verify rollback preserves the concurrent registry entry.
   • Force backup-copy and registry-save failures after reinstall failure; verify the original reinstall BundlerError remains primary and rollback failure is surfaced as secondary diagnostic context.
   • Retain the existing serial byte-for-byte package and registry restoration regression.
   • Add a negative test proving stale rollback is refused when the target has changed since the snapshot.

   Risks & Considerations

   • Lock scope must not include network/catalog I/O unless intentionally accepted; otherwise refresh can block all step mutations for the duration of downloads.
   • Registry conflict detection must preserve unrelated entries and avoid silently treating a newer same-step install as the old baseline.
   • Rollback diagnostics must not claim success or delete the only recovery backup after a failed restore.
   • Changes should preserve the existing public CLI behavior and Windows/POSIX lock semantics.

   Open Questions

   • [NEEDS CLARIFICATION: What exact generation/version marker should distinguish the refresh snapshot from a later same-step or registry commit?]
   • [NEEDS CLARIFICATION: Should an incomplete rollback retain its backup in a user-visible recovery location, or only include its path in diagnostics?]
   • [NEEDS CLARIFICATION: Is the intended contract to keep catalog reinstall outside the lock, as described in the issue comment, or to serialize the entire refresh?]

   Generated by 🐛 Assess Bug from Labeled Issue for #4815 · copilot · gpt52codex · 1.94 AIC · ⌖ 0.668 AIC · ⊞ 22.3K · ◷
5. github-actions

   added

   severity-medium

   on Oct 15, 2026