Skip to content

Commit d5ba062

Browse files
jawwad-aliclaude
andauthored
fix(bundler): bundle update uninstalls components dropped by new version (#3353)
On refresh (bundle update), install_bundle iterated only the new plan's components, so a component the previous version owned but the new one no longer ships was left installed on disk while being dropped from the rewritten record (contributed only holds plan.components). With no record referencing it, remove_bundle could never clean it up — permanently orphaned, violating the provenance invariant (FR-022). After the component loop, when refresh and a prior record exists, uninstall each previously-owned component absent from the new plan — unless another bundle still needs it (components_still_needed refcount, mirroring remove_bundle), in which case it stays installed and is simply de-attributed. Runs inside the existing try so a failed removal takes the same no-record-written rollback path. Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
1 parent 12faf7b commit d5ba062

2 files changed

Lines changed: 87 additions & 0 deletions

File tree

src/specify_cli/bundler/services/installer.py

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -130,6 +130,28 @@ def install_bundle(
130130
done.append(component)
131131
result.installed.append(component)
132132
contributed.append(component)
133+
134+
# On update (refresh), uninstall components this bundle used to own
135+
# that the new version no longer ships. Otherwise they are dropped
136+
# from the record below (contributed only holds plan.components) yet
137+
# left on disk — permanently orphaned, since no bundle record can
138+
# ever remove them. A stale component still owned by another bundle
139+
# is kept installed and simply de-attributed here (it stays in that
140+
# bundle's record). Mirrors remove_bundle's refcount logic.
141+
if refresh and existing is not None:
142+
planned = {(c.kind, c.id) for c in plan.components}
143+
still_needed = components_still_needed(
144+
records, exclude_bundle_id=plan.bundle_id
145+
)
146+
for component in existing.contributed_components:
147+
key = (component.kind, component.id)
148+
if key in planned:
149+
continue
150+
if key in still_needed:
151+
continue
152+
if installer.is_installed(project_root, component):
153+
installer.remove(project_root, component)
154+
result.uninstalled.append(component)
133155
except BundlerError:
134156
_rollback(project_root, installer, done)
135157
raise

tests/integration/test_bundler_install_flow.py

Lines changed: 65 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -220,3 +220,68 @@ def test_pre_existing_component_is_not_attributed_or_removed(tmp_path: Path):
220220

221221
remove_bundle(tmp_path, "demo-bundle", installer)
222222
assert ("extensions", "ext-a") in installer.installed
223+
224+
225+
def _bundle(manifest_id, ext_ids, *, version="1.0.0"):
226+
data = valid_manifest_dict()
227+
data["bundle"]["id"] = manifest_id
228+
data["bundle"]["version"] = version
229+
data["provides"] = {
230+
"extensions": [{"id": e, "version": version} for e in ext_ids]
231+
}
232+
return BundleManifest.from_dict(data)
233+
234+
235+
def test_update_uninstalls_components_dropped_by_new_version(tmp_path: Path):
236+
"""`bundle update` must uninstall components the new version no longer
237+
ships, instead of orphaning them (installed on disk, tracked by nothing)."""
238+
make_project(tmp_path)
239+
installer = FakeInstaller()
240+
241+
man_v1 = _bundle("demo", ["ext-a", "ext-b"])
242+
install_bundle(tmp_path, _plan(man_v1), installer, manifest=man_v1)
243+
assert ("extensions", "ext-b") in installer.installed
244+
245+
man_v2 = _bundle("demo", ["ext-a"], version="2.0.0")
246+
result = install_bundle(
247+
tmp_path, _plan(man_v2), installer, manifest=man_v2, refresh=True
248+
)
249+
250+
# ext-b was dropped by v2 -> uninstalled and reported.
251+
assert ("extensions", "ext-b") in installer.remove_calls
252+
assert ("extensions", "ext-b") in {(c.kind, c.id) for c in result.uninstalled}
253+
assert ("extensions", "ext-b") not in installer.installed
254+
assert ("extensions", "ext-a") in installer.installed
255+
256+
# The saved record lists only ext-a.
257+
rec = next(r for r in load_records(tmp_path) if r.bundle_id == "demo")
258+
keys = {(c.kind, c.id) for c in rec.contributed_components}
259+
assert ("extensions", "ext-a") in keys
260+
assert ("extensions", "ext-b") not in keys
261+
262+
263+
def test_update_keeps_component_still_needed_by_sibling_bundle(tmp_path: Path):
264+
"""A dropped component still owned by another bundle stays installed."""
265+
make_project(tmp_path)
266+
installer = FakeInstaller()
267+
268+
man_sib = _bundle("sibling", ["ext-b"])
269+
install_bundle(tmp_path, _plan(man_sib), installer, manifest=man_sib)
270+
271+
man_v1 = _bundle("demo", ["ext-a", "ext-b"])
272+
install_bundle(tmp_path, _plan(man_v1), installer, manifest=man_v1)
273+
274+
man_v2 = _bundle("demo", ["ext-a"], version="2.0.0")
275+
install_bundle(
276+
tmp_path, _plan(man_v2), installer, manifest=man_v2, refresh=True
277+
)
278+
279+
# ext-b is still needed by 'sibling' -> not removed, stays installed.
280+
assert ("extensions", "ext-b") not in installer.remove_calls
281+
assert ("extensions", "ext-b") in installer.installed
282+
283+
# But demo's record no longer attributes it.
284+
rec = next(r for r in load_records(tmp_path) if r.bundle_id == "demo")
285+
assert ("extensions", "ext-b") not in {
286+
(c.kind, c.id) for c in rec.contributed_components
287+
}

0 commit comments

Comments
 (0)