From a8f62d443fc46a3d813b62b64b4271d0afbd863f Mon Sep 17 00:00:00 2001 From: song Date: Wed, 14 Oct 2026 15:42:23 +0800 Subject: [PATCH 1/2] fix(extensions): keep a disabled extension disabled across upgrade and rollback `install_extension` serves both `install` and `upgrade` and always wrote `enabled: True`; `rollback_extension` did the same. A provider the operator had switched off therefore came back on as a side effect of any revision change, with no test or documentation stating that contract. Preserve the existing enablement decision when an entry already exists: a fresh install still activates the provider, while upgrade and rollback change only the active revision. The operation payload now reports the resulting state instead of a constant. Behavior change, disclosed: before, `extension upgrade`/`rollback` on a disabled extension re-enabled it; now it stays disabled until an explicit `extension enable`. The reference documents the rule; two regression tests cover the disabled and enabled paths through install, disable, upgrade, rollback and status readback. Signed-off-by: song --- docs/reference/extensions.md | 4 +- loopx/extensions/runtime.py | 13 ++-- tests/extensions/test_extension_runtime.py | 69 ++++++++++++++++++++++ 3 files changed, 81 insertions(+), 5 deletions(-) diff --git a/docs/reference/extensions.md b/docs/reference/extensions.md index 649e5fc805..921e802a81 100644 --- a/docs/reference/extensions.md +++ b/docs/reference/extensions.md @@ -302,7 +302,9 @@ names the exact repair command. For a separately distributed provider, pass `--manifest `. `upgrade` validates and probes the new manifest before changing the active revision. `rollback` probes the previous revision before switching back. A -failed probe leaves the current revision untouched. Activation state contains +failed probe leaves the current revision untouched. Neither operation changes +the enablement decision: a disabled extension stays disabled across an upgrade +or rollback until an explicit `enable`. Activation state contains validated manifest snapshots and revision ids in the private LoopX runtime root; it does not contain provider output or credentials. diff --git a/loopx/extensions/runtime.py b/loopx/extensions/runtime.py index 9c52db6413..a2c37b43c5 100644 --- a/loopx/extensions/runtime.py +++ b/loopx/extensions/runtime.py @@ -239,6 +239,10 @@ def install_extension( previous_revision = ( str(existing.get("active_revision")) if isinstance(existing, dict) else None ) + # A fresh install activates the provider. An upgrade changes only the + # active revision; the operator's enable/disable decision is separate + # state and survives the revision change. + enabled = bool(existing.get("enabled")) if isinstance(existing, dict) else True if execute: revisions = ( list(existing.get("revisions") or []) @@ -257,7 +261,7 @@ def install_extension( ) extensions[extension_id] = { "id": extension_id, - "enabled": True, + "enabled": enabled, "active_revision": revision, "rollback_revision": previous_revision, "revisions": revisions, @@ -275,7 +279,7 @@ def install_extension( "version": provider["version"], "revision": revision, "previous_revision": previous_revision, - "enabled": True if execute else None, + "enabled": enabled if execute else None, "doctor": doctor, "rollback_available": previous_revision is not None, } @@ -438,7 +442,8 @@ def rollback_extension( current_entry["active_revision"] = target_revision current_entry["rollback_revision"] = previous_revision _record_doctor(current_entry, target_revision, doctor) - current_entry["enabled"] = True + # Rollback switches the active revision only; it does not undo an + # operator's disable decision. _write_state(path, current_state) return { "ok": True, @@ -450,7 +455,7 @@ def rollback_extension( "version": target.get("version"), "revision": target_revision, "previous_revision": previous_revision, - "enabled": True if execute else bool(entry.get("enabled")), + "enabled": bool(entry.get("enabled")), "doctor": doctor, } diff --git a/tests/extensions/test_extension_runtime.py b/tests/extensions/test_extension_runtime.py index 107138affd..0551146bb0 100644 --- a/tests/extensions/test_extension_runtime.py +++ b/tests/extensions/test_extension_runtime.py @@ -1605,6 +1605,75 @@ def test_declared_manifest_for_an_installed_id_must_repeat_the_active_revision( extension_catalog_entries([v2], state_file=state_file) +def test_upgrade_and_rollback_preserve_a_disabled_extension(tmp_path: Path) -> None: + """Changing the active revision must not undo an operator's disable. + + Before this guard, both `upgrade` and `rollback` wrote `enabled: True` + unconditionally, so a provider the operator had switched off came back + on as a side effect of a revision change. + """ + provider = _provider(tmp_path / "provider") + v1 = _manifest(tmp_path / "v1.toml", entrypoint=provider, version="1.0.0") + v2 = _manifest(tmp_path / "v2.toml", entrypoint=provider, version="2.0.0") + state_file = tmp_path / "runtime" / "extensions.json" + + installed = install_extension(v1, state_file=state_file, execute=True) + assert installed["enabled"] is True + assert disable_extension( + "test-semantic-extension", state_file=state_file, execute=True + )["changed"] is True + + upgraded = install_extension( + v2, state_file=state_file, operation="upgrade", execute=True + ) + assert upgraded["enabled"] is False + assert upgraded["revision"] != installed["revision"] + entry = json.loads(state_file.read_text(encoding="utf-8"))["extensions"][ + "test-semantic-extension" + ] + assert entry["enabled"] is False + assert entry["active_revision"] == upgraded["revision"] + with pytest.raises(ValueError, match="is disabled"): + resolve_extension_activation("test-semantic-extension", state_file=state_file) + + rolled_back = rollback_extension( + "test-semantic-extension", state_file=state_file, execute=True + ) + assert rolled_back["enabled"] is False + assert rolled_back["revision"] == installed["revision"] + entry = json.loads(state_file.read_text(encoding="utf-8"))["extensions"][ + "test-semantic-extension" + ] + assert entry["enabled"] is False + assert extension_status(state_file=state_file)["extensions"][0]["enabled"] is False + + # The explicit enable path remains the only way back on. + assert enable_extension( + "test-semantic-extension", state_file=state_file, execute=True + )["changed"] is True + assert resolve_extension_activation( + "test-semantic-extension", state_file=state_file + )["revision"] == installed["revision"] + + +def test_upgrade_and_rollback_keep_an_enabled_extension_enabled(tmp_path: Path) -> None: + provider = _provider(tmp_path / "provider") + v1 = _manifest(tmp_path / "v1.toml", entrypoint=provider, version="1.0.0") + v2 = _manifest(tmp_path / "v2.toml", entrypoint=provider, version="2.0.0") + state_file = tmp_path / "runtime" / "extensions.json" + + install_extension(v1, state_file=state_file, execute=True) + upgraded = install_extension( + v2, state_file=state_file, operation="upgrade", execute=True + ) + assert upgraded["enabled"] is True + rolled_back = rollback_extension( + "test-semantic-extension", state_file=state_file, execute=True + ) + assert rolled_back["enabled"] is True + assert extension_status(state_file=state_file)["extensions"][0]["enabled"] is True + + def test_failed_upgrade_keeps_the_active_revision(tmp_path: Path) -> None: ready = _provider(tmp_path / "ready-provider") broken = _provider(tmp_path / "broken-provider", doctor_exit=2) From 5f6c425a6fc9a681d1e8cc746019cde47f171cac Mon Sep 17 00:00:00 2001 From: song Date: Thu, 15 Oct 2026 08:19:06 +0800 Subject: [PATCH 2/2] fix(extensions): report the enablement committed by rollback `rollback_extension` reads the entry before the doctor probe and re-reads it under the state lock afterwards. Persisted state already kept an enable or disable that completed during the probe, but the result still carried the pre-probe `enabled`, so a real CLI caller saw the rollback JSON disagree with `extension list` and could not tell whether a follow-up enable/disable was needed. Capture `enabled` from the entry committed under the existing lock and return that; dry-run keeps the preview value. No provider is re-enabled, no new state or CAS rule is added. `install_extension` already derives the upgrade receipt from the entry read under its lock and needs no change. Regression: both race directions through real `python -m loopx.cli` subprocesses, with a synthetic provider whose doctor blocks at a file barrier until a second CLI `disable`/`enable` has committed; asserts the rollback result, saved state, and `list` readback agree. Signed-off-by: song --- docs/reference/extensions.md | 4 +- loopx/extensions/runtime.py | 10 +- tests/extensions/test_extension_runtime.py | 129 +++++++++++++++++++++ 3 files changed, 140 insertions(+), 3 deletions(-) diff --git a/docs/reference/extensions.md b/docs/reference/extensions.md index 921e802a81..f4bd11ddb4 100644 --- a/docs/reference/extensions.md +++ b/docs/reference/extensions.md @@ -304,7 +304,9 @@ For a separately distributed provider, pass `--manifest `. revision. `rollback` probes the previous revision before switching back. A failed probe leaves the current revision untouched. Neither operation changes the enablement decision: a disabled extension stays disabled across an upgrade -or rollback until an explicit `enable`. Activation state contains +or rollback until an explicit `enable`. The operation result reports the +enablement state it committed, so an `enable` or `disable` that completes while +the probe runs shows in the result as well as in `list`. Activation state contains validated manifest snapshots and revision ids in the private LoopX runtime root; it does not contain provider output or credentials. diff --git a/loopx/extensions/runtime.py b/loopx/extensions/runtime.py index a2c37b43c5..e68af52f11 100644 --- a/loopx/extensions/runtime.py +++ b/loopx/extensions/runtime.py @@ -424,6 +424,9 @@ def rollback_extension( f"extension `{extension_id}` rollback doctor is not ready: {doctor['status']}" ) previous_revision = str(entry.get("active_revision") or "") + # Dry-run previews the enablement read before the probe; the execute path + # replaces it with the value committed under the lock below. + enabled = bool(entry.get("enabled")) if execute: with exclusive_file_lock(path): current_state = _read_state(path) @@ -443,7 +446,10 @@ def rollback_extension( current_entry["rollback_revision"] = previous_revision _record_doctor(current_entry, target_revision, doctor) # Rollback switches the active revision only; it does not undo an - # operator's disable decision. + # operator's enable/disable decision, including one that completed + # while the probe ran. The result must report what was committed + # here, not the pre-probe snapshot. + enabled = bool(current_entry.get("enabled")) _write_state(path, current_state) return { "ok": True, @@ -455,7 +461,7 @@ def rollback_extension( "version": target.get("version"), "revision": target_revision, "previous_revision": previous_revision, - "enabled": bool(entry.get("enabled")), + "enabled": enabled, "doctor": doctor, } diff --git a/tests/extensions/test_extension_runtime.py b/tests/extensions/test_extension_runtime.py index 0551146bb0..5a8372435f 100644 --- a/tests/extensions/test_extension_runtime.py +++ b/tests/extensions/test_extension_runtime.py @@ -1674,6 +1674,135 @@ def test_upgrade_and_rollback_keep_an_enabled_extension_enabled(tmp_path: Path) assert extension_status(state_file=state_file)["extensions"][0]["enabled"] is True +def _barrier_provider(path: Path, *, gate: Path, started: Path, release: Path) -> Path: + """A provider whose doctor blocks at an observable barrier. + + Without ``gate`` the doctor passes at once, so install and upgrade are + unaffected. Once ``gate`` exists the doctor writes ``started`` and then + waits for ``release``, which lets a test complete another real CLI call + while the probe is in flight. + """ + path.write_text( + f"""#!{sys.executable} +import sys +import time +from pathlib import Path + +if "--doctor" not in sys.argv: + raise SystemExit(1) +if Path({str(gate)!r}).exists(): + Path({str(started)!r}).write_text("started", encoding="utf-8") + deadline = time.monotonic() + 30 + while not Path({str(release)!r}).exists(): + if time.monotonic() > deadline: + raise SystemExit(3) + time.sleep(0.02) +raise SystemExit(0) +""", + encoding="utf-8", + ) + path.chmod(0o755) + return path + + +@pytest.mark.parametrize( + ("concurrent_command", "enabled_before", "enabled_after"), + [("disable", True, False), ("enable", False, True)], +) +def test_rollback_result_reports_the_enablement_committed_under_the_lock( + tmp_path: Path, + concurrent_command: str, + enabled_before: bool, + enabled_after: bool, +) -> None: + """An enable/disable that lands during the rollback probe is preserved + and reported. + + The rollback doctor runs outside the state lock. The committed state + already kept the concurrent decision, but the result carried the + pre-probe snapshot, so a caller of the real CLI saw `enabled` disagree + with `extension list` and could not tell whether a follow-up + enable/disable was needed. Both directions use real `loopx.cli` + subprocesses and a doctor that blocks at a file barrier. + """ + extension_id = "test-semantic-extension" + gate, started, release = ( + tmp_path / name for name in ("gate", "doctor-started", "release") + ) + blocking = _barrier_provider( + tmp_path / "blocking-provider", gate=gate, started=started, release=release + ) + v1 = _manifest(tmp_path / "v1.toml", entrypoint=blocking, version="1.0.0") + v1.write_text( + v1.read_text(encoding="utf-8").replace( + "timeout_seconds = 5", "timeout_seconds = 60" + ), + encoding="utf-8", + ) + # The active revision keeps an unblocked doctor so a concurrent `enable` + # (which probes the active revision) cannot wait on the same barrier. + v2 = _manifest( + tmp_path / "v2.toml", entrypoint=_provider(tmp_path / "plain-provider"), + version="2.0.0", + ) + state_file = tmp_path / "runtime" / "extensions.json" + installed = install_extension(v1, state_file=state_file, execute=True) + upgraded = install_extension( + v2, state_file=state_file, operation="upgrade", execute=True + ) + if not enabled_before: + disable_extension(extension_id, state_file=state_file, execute=True) + + def cli(*arguments: str) -> list[str]: + return [ + sys.executable, "-m", "loopx.cli", "--format", "json", "extension", + *arguments, "--state-file", str(state_file), + ] + + gate.write_text("", encoding="utf-8") + with subprocess.Popen( + cli("rollback", extension_id, "--execute"), + stdout=subprocess.PIPE, stderr=subprocess.PIPE, text=True, + ) as rollback: + try: + deadline = time.monotonic() + 30 + while not started.exists(): + assert rollback.poll() is None, rollback.communicate()[1] + assert time.monotonic() < deadline, "rollback doctor never reached the barrier" + time.sleep(0.02) + # The rollback is inside its probe and does not hold the lock. + concurrent = subprocess.run( + cli(concurrent_command, extension_id, "--execute"), + capture_output=True, text=True, check=False, timeout=60, + ) + assert concurrent.returncode == 0, concurrent.stderr + concurrent_payload = json.loads(concurrent.stdout) + assert concurrent_payload["changed"] is True + assert concurrent_payload["enabled"] is enabled_after + assert concurrent_payload["active_revision"] == upgraded["revision"] + finally: + release.write_text("", encoding="utf-8") + stdout, stderr = rollback.communicate(timeout=60) + assert rollback.returncode == 0, stderr + payload = json.loads(stdout) + assert payload["ok"] is True + assert payload["operation"] == "rollback" + assert payload["revision"] == installed["revision"] + assert payload["previous_revision"] == upgraded["revision"] + assert payload["enabled"] is enabled_after + + entry = json.loads(state_file.read_text(encoding="utf-8"))["extensions"][extension_id] + assert entry["active_revision"] == installed["revision"] + assert entry["enabled"] is enabled_after + listing = subprocess.run( + cli("list"), capture_output=True, text=True, check=False, timeout=60, + ) + assert listing.returncode == 0, listing.stderr + [listed] = json.loads(listing.stdout)["extensions"] + assert listed["active_revision"] == installed["revision"] + assert listed["enabled"] is payload["enabled"] is enabled_after + + def test_failed_upgrade_keeps_the_active_revision(tmp_path: Path) -> None: ready = _provider(tmp_path / "ready-provider") broken = _provider(tmp_path / "broken-provider", doctor_exit=2)