mirror of
https://github.com/maziggy/bambuddy.git
synced 2026-10-18 21:47:39 +02:00
fix(updates): install the discovered release tag, not hardcoded origin/main
The in-app updater ran `git fetch origin main && git reset --hard origin/main` regardless of which version the GitHub releases API reported as latest. So whenever the latest release lived on a branch other than main — e.g. during a beta cycle when 0.2.4b1 sits on its own branch and main still points at the previous stable — clicking Apply Update appeared to succeed but the user actually stayed pinned to old main HEAD. Fix: extract `_discover_target_release(db)` mirroring the same release-API + include_beta_updates selection the GUI's update-check already uses, pass the resolved tag (e.g. `v0.2.4b1`) into `_perform_update(target_ref)`, and run `git fetch --prune --tags origin && git reset --hard <target_ref>`. The fetch now pulls --tags so a tag ref is locally resolvable; the reset takes the caller's ref instead of a hardcoded branch. apply_update now returns a clear error if no release resolves, instead of silently kicking off an update that can't land.
This commit is contained in:
@@ -383,8 +383,61 @@ async def check_for_updates(
|
|||||||
}
|
}
|
||||||
|
|
||||||
|
|
||||||
async def _perform_update():
|
async def _discover_target_release(db: AsyncSession) -> str | None:
|
||||||
"""Perform the actual update using git fetch and reset."""
|
"""Look up the tag we should install from GitHub releases.
|
||||||
|
|
||||||
|
Same selection logic the GUI's update-check uses: respect
|
||||||
|
`include_beta_updates`, skip prereleases when the user opted out, take
|
||||||
|
the first matching release. Returns the raw tag name (e.g. `v0.2.4b1`)
|
||||||
|
so the git ref is unambiguous, or None if there's no release to install.
|
||||||
|
|
||||||
|
The previous in-app updater path was hardcoded to `git fetch origin main
|
||||||
|
&& git reset --hard origin/main`, which silently no-ops whenever main
|
||||||
|
isn't where the latest release lives — e.g. during a beta release cycle
|
||||||
|
where the next stable hasn't been merged to main yet. Anchoring to the
|
||||||
|
release tag instead lets the GUI install whatever GitHub says is latest.
|
||||||
|
"""
|
||||||
|
result = await db.execute(select(Settings).where(Settings.key == "include_beta_updates"))
|
||||||
|
beta_setting = result.scalar_one_or_none()
|
||||||
|
include_beta = beta_setting and beta_setting.value.lower() == "true"
|
||||||
|
|
||||||
|
try:
|
||||||
|
async with httpx.AsyncClient() as client:
|
||||||
|
response = await client.get(
|
||||||
|
f"https://api.github.com/repos/{GITHUB_REPO}/releases?per_page=20",
|
||||||
|
headers={"Accept": "application/vnd.github.v3+json"},
|
||||||
|
timeout=10.0,
|
||||||
|
)
|
||||||
|
response.raise_for_status()
|
||||||
|
releases = response.json()
|
||||||
|
except (httpx.HTTPError, ValueError) as exc:
|
||||||
|
logger.error("Could not fetch GitHub releases for update target: %s", exc)
|
||||||
|
return None
|
||||||
|
|
||||||
|
for release in releases:
|
||||||
|
tag = release.get("tag_name", "")
|
||||||
|
if not tag:
|
||||||
|
continue
|
||||||
|
if include_beta:
|
||||||
|
return tag
|
||||||
|
# Skip prereleases (parsed from version, not GitHub flag — GitHub's
|
||||||
|
# is_prerelease flag isn't always set on dailies).
|
||||||
|
parsed = parse_version(tag)
|
||||||
|
if parsed[4] == 0:
|
||||||
|
return tag
|
||||||
|
return None
|
||||||
|
|
||||||
|
|
||||||
|
async def _perform_update(target_ref: str):
|
||||||
|
"""Perform the actual update using git fetch and reset.
|
||||||
|
|
||||||
|
`target_ref` is whatever git ref the caller wants to land on — typically
|
||||||
|
a release tag like `v0.2.4b1` resolved by `_discover_target_release`,
|
||||||
|
but accepts any ref `git reset --hard` understands (`origin/main`, a
|
||||||
|
branch, a sha). Tag-based refs are the production path because they pin
|
||||||
|
the install to a specific release artifact instead of whatever happens
|
||||||
|
to be on a moving branch.
|
||||||
|
"""
|
||||||
global _update_status
|
global _update_status
|
||||||
|
|
||||||
try:
|
try:
|
||||||
@@ -447,13 +500,18 @@ async def _perform_update():
|
|||||||
"error": None,
|
"error": None,
|
||||||
}
|
}
|
||||||
|
|
||||||
# Fetch from origin
|
# Fetch branches AND tags from origin so any ref the caller passes
|
||||||
|
# (release tag like `v0.2.4b1`, a branch like `main`, or a sha) is
|
||||||
|
# locally resolvable for the reset below. `--tags` is required —
|
||||||
|
# plain `git fetch origin` doesn't bring tags by default, so a
|
||||||
|
# release tag would not be resolvable.
|
||||||
process = await asyncio.create_subprocess_exec(
|
process = await asyncio.create_subprocess_exec(
|
||||||
git_path,
|
git_path,
|
||||||
*git_config,
|
*git_config,
|
||||||
"fetch",
|
"fetch",
|
||||||
|
"--prune",
|
||||||
|
"--tags",
|
||||||
"origin",
|
"origin",
|
||||||
"main",
|
|
||||||
cwd=str(base_dir),
|
cwd=str(base_dir),
|
||||||
stdout=asyncio.subprocess.PIPE,
|
stdout=asyncio.subprocess.PIPE,
|
||||||
stderr=asyncio.subprocess.PIPE,
|
stderr=asyncio.subprocess.PIPE,
|
||||||
@@ -478,13 +536,18 @@ async def _perform_update():
|
|||||||
"error": None,
|
"error": None,
|
||||||
}
|
}
|
||||||
|
|
||||||
# Hard reset to origin/main (clean update, no merge conflicts)
|
# Hard reset to the target ref (clean update, no merge conflicts).
|
||||||
|
# `target_ref` is typically a release tag like `v0.2.4b1` resolved
|
||||||
|
# from the GitHub releases API by `_discover_target_release`. The
|
||||||
|
# local branch name doesn't change — only HEAD moves. Falling back
|
||||||
|
# to `origin/main` here was the source of the "in-app updater can't
|
||||||
|
# reach beta releases" bug.
|
||||||
process = await asyncio.create_subprocess_exec(
|
process = await asyncio.create_subprocess_exec(
|
||||||
git_path,
|
git_path,
|
||||||
*git_config,
|
*git_config,
|
||||||
"reset",
|
"reset",
|
||||||
"--hard",
|
"--hard",
|
||||||
"origin/main",
|
target_ref,
|
||||||
cwd=str(base_dir),
|
cwd=str(base_dir),
|
||||||
stdout=asyncio.subprocess.PIPE,
|
stdout=asyncio.subprocess.PIPE,
|
||||||
stderr=asyncio.subprocess.PIPE,
|
stderr=asyncio.subprocess.PIPE,
|
||||||
@@ -593,6 +656,7 @@ async def _perform_update():
|
|||||||
@router.post("/apply")
|
@router.post("/apply")
|
||||||
async def apply_update(
|
async def apply_update(
|
||||||
background_tasks: BackgroundTasks,
|
background_tasks: BackgroundTasks,
|
||||||
|
db: AsyncSession = Depends(get_db),
|
||||||
_: User | None = RequirePermissionIfAuthEnabled(Permission.SETTINGS_UPDATE),
|
_: User | None = RequirePermissionIfAuthEnabled(Permission.SETTINGS_UPDATE),
|
||||||
):
|
):
|
||||||
"""Apply available update (git pull + rebuild)."""
|
"""Apply available update (git pull + rebuild)."""
|
||||||
@@ -617,8 +681,22 @@ async def apply_update(
|
|||||||
),
|
),
|
||||||
}
|
}
|
||||||
|
|
||||||
|
# Discover which release tag to install. Resolved here (where we have
|
||||||
|
# a DB session) and passed into the background task; the BG task can't
|
||||||
|
# reuse this request's session since FastAPI closes it on response.
|
||||||
|
target_ref = await _discover_target_release(db)
|
||||||
|
if target_ref is None:
|
||||||
|
return {
|
||||||
|
"success": False,
|
||||||
|
"message": (
|
||||||
|
"Could not determine a release to install. Either GitHub is "
|
||||||
|
"unreachable or no release matches your update channel "
|
||||||
|
"(check the include_beta_updates setting)."
|
||||||
|
),
|
||||||
|
}
|
||||||
|
|
||||||
# Start update in background
|
# Start update in background
|
||||||
background_tasks.add_task(_perform_update)
|
background_tasks.add_task(_perform_update, target_ref)
|
||||||
|
|
||||||
_update_status = {
|
_update_status = {
|
||||||
"status": "downloading",
|
"status": "downloading",
|
||||||
|
|||||||
@@ -23,9 +23,16 @@ class TestUpdatesAPI:
|
|||||||
|
|
||||||
@pytest.mark.asyncio
|
@pytest.mark.asyncio
|
||||||
async def test_apply_update_non_docker(self, async_client: AsyncClient):
|
async def test_apply_update_non_docker(self, async_client: AsyncClient):
|
||||||
"""Test non-Docker path - mock _perform_update to prevent side effects."""
|
"""Test non-Docker path - mock _perform_update + _discover_target_release
|
||||||
|
to prevent side effects (network call to GitHub releases API + actual
|
||||||
|
git/pip subprocesses)."""
|
||||||
with (
|
with (
|
||||||
patch("backend.app.api.routes.updates._is_docker_environment", return_value=False),
|
patch("backend.app.api.routes.updates._is_docker_environment", return_value=False),
|
||||||
|
patch(
|
||||||
|
"backend.app.api.routes.updates._discover_target_release",
|
||||||
|
new_callable=AsyncMock,
|
||||||
|
return_value="v9.9.9",
|
||||||
|
),
|
||||||
patch("backend.app.api.routes.updates._perform_update", new_callable=AsyncMock),
|
patch("backend.app.api.routes.updates._perform_update", new_callable=AsyncMock),
|
||||||
):
|
):
|
||||||
response = await async_client.post("/api/v1/updates/apply")
|
response = await async_client.post("/api/v1/updates/apply")
|
||||||
@@ -118,7 +125,7 @@ class TestUpdatesAPI:
|
|||||||
side_effect=fake_create_subprocess_exec,
|
side_effect=fake_create_subprocess_exec,
|
||||||
),
|
),
|
||||||
):
|
):
|
||||||
await updates_module._perform_update()
|
await updates_module._perform_update("v0.2.4b1")
|
||||||
|
|
||||||
# The updater MUST NOT have run `git remote set-url origin <https>`
|
# The updater MUST NOT have run `git remote set-url origin <https>`
|
||||||
# because origin already pointed at the right repo over SSH.
|
# because origin already pointed at the right repo over SSH.
|
||||||
@@ -167,7 +174,7 @@ class TestUpdatesAPI:
|
|||||||
side_effect=fake_create_subprocess_exec,
|
side_effect=fake_create_subprocess_exec,
|
||||||
),
|
),
|
||||||
):
|
):
|
||||||
await updates_module._perform_update()
|
await updates_module._perform_update("v0.2.4b1")
|
||||||
|
|
||||||
set_url_calls = [c for c in calls if "set-url" in c["args"] and "origin" in c["args"]]
|
set_url_calls = [c for c in calls if "set-url" in c["args"] and "origin" in c["args"]]
|
||||||
assert set_url_calls, "Updater must rewrite origin when it points at a fork."
|
assert set_url_calls, "Updater must rewrite origin when it points at a fork."
|
||||||
@@ -176,6 +183,121 @@ class TestUpdatesAPI:
|
|||||||
f"Expected origin to be reset to canonical HTTPS URL; got: {rewritten_to}"
|
f"Expected origin to be reset to canonical HTTPS URL; got: {rewritten_to}"
|
||||||
)
|
)
|
||||||
|
|
||||||
|
@pytest.mark.asyncio
|
||||||
|
async def test_perform_update_resets_to_target_ref_not_hardcoded_main(self, tmp_path):
|
||||||
|
"""Regression for the hardcoded-`origin/main` limitation: the in-app
|
||||||
|
updater must reset to the caller-supplied target ref (typically a
|
||||||
|
release tag like `v0.2.4b1` discovered from the GitHub releases API)
|
||||||
|
so beta releases that don't live on main can actually be installed.
|
||||||
|
Pre-fix, `_perform_update` issued `git reset --hard origin/main`
|
||||||
|
verbatim and silently no-op'd whenever the latest release wasn't on
|
||||||
|
main — leaving a 0.2.3.x user clicking *Apply Update* stranded on
|
||||||
|
0.2.3.x. Also asserts the fetch step uses `--tags` so a tag ref is
|
||||||
|
actually resolvable post-fetch."""
|
||||||
|
from backend.app.api.routes import updates as updates_module
|
||||||
|
|
||||||
|
app_dir = tmp_path / "app"
|
||||||
|
data_dir = tmp_path / "app" / "data"
|
||||||
|
app_dir.mkdir()
|
||||||
|
data_dir.mkdir()
|
||||||
|
(app_dir / "requirements.txt").write_text("fastapi\n")
|
||||||
|
|
||||||
|
calls: list[dict] = []
|
||||||
|
|
||||||
|
async def fake_create_subprocess_exec(*args, **kwargs):
|
||||||
|
calls.append({"args": args, "cwd": kwargs.get("cwd")})
|
||||||
|
proc = MagicMock()
|
||||||
|
if "get-url" in args and "origin" in args:
|
||||||
|
proc.communicate = AsyncMock(return_value=(b"git@github.com:maziggy/bambuddy.git\n", b""))
|
||||||
|
else:
|
||||||
|
proc.communicate = AsyncMock(return_value=(b"", b""))
|
||||||
|
proc.returncode = 0
|
||||||
|
return proc
|
||||||
|
|
||||||
|
with (
|
||||||
|
patch.object(updates_module.settings, "base_dir", data_dir),
|
||||||
|
patch.object(updates_module.settings, "app_dir", app_dir),
|
||||||
|
patch.object(updates_module, "_find_executable", return_value="/usr/bin/git"),
|
||||||
|
patch.object(
|
||||||
|
updates_module.asyncio,
|
||||||
|
"create_subprocess_exec",
|
||||||
|
side_effect=fake_create_subprocess_exec,
|
||||||
|
),
|
||||||
|
):
|
||||||
|
await updates_module._perform_update("v0.2.4b1")
|
||||||
|
|
||||||
|
# Reset target must be the caller-supplied ref, not "origin/main".
|
||||||
|
reset_calls = [c for c in calls if "reset" in c["args"] and "--hard" in c["args"]]
|
||||||
|
assert reset_calls, "git reset must be invoked"
|
||||||
|
reset_target = reset_calls[0]["args"][-1]
|
||||||
|
assert reset_target == "v0.2.4b1", (
|
||||||
|
f"Expected reset target to be the caller-supplied ref 'v0.2.4b1'; "
|
||||||
|
f"got {reset_target!r}. Regression to a hardcoded 'origin/main' "
|
||||||
|
"would re-introduce the in-app-updater-can't-install-betas bug."
|
||||||
|
)
|
||||||
|
|
||||||
|
# Fetch must include --tags so v0.2.4b1 (a tag) is locally resolvable.
|
||||||
|
fetch_calls = [c for c in calls if "fetch" in c["args"]]
|
||||||
|
assert fetch_calls
|
||||||
|
assert "--tags" in fetch_calls[0]["args"], (
|
||||||
|
"Fetch must use --tags so release-tag refs (the production path "
|
||||||
|
"for tag-based updates) are resolvable for the subsequent reset. "
|
||||||
|
f"Captured fetch call: {fetch_calls[0]['args']}"
|
||||||
|
)
|
||||||
|
|
||||||
|
@pytest.mark.asyncio
|
||||||
|
async def test_apply_update_passes_discovered_release_to_perform_update(self, async_client: AsyncClient):
|
||||||
|
"""End-to-end glue: the route handler calls `_discover_target_release`
|
||||||
|
to pick the tag (respecting include_beta_updates), then schedules
|
||||||
|
`_perform_update` with that tag — not with no arg, not with main."""
|
||||||
|
from backend.app.api.routes import updates as updates_module
|
||||||
|
|
||||||
|
captured_ref: list[str] = []
|
||||||
|
|
||||||
|
async def fake_perform_update(target_ref):
|
||||||
|
captured_ref.append(target_ref)
|
||||||
|
|
||||||
|
async def fake_discover(_db):
|
||||||
|
return "v0.2.4b1"
|
||||||
|
|
||||||
|
with (
|
||||||
|
patch.object(updates_module, "_is_docker_environment", return_value=False),
|
||||||
|
patch.object(updates_module, "_perform_update", side_effect=fake_perform_update),
|
||||||
|
patch.object(updates_module, "_discover_target_release", side_effect=fake_discover),
|
||||||
|
):
|
||||||
|
response = await async_client.post("/api/v1/updates/apply")
|
||||||
|
|
||||||
|
assert response.json()["success"] is True
|
||||||
|
assert captured_ref == ["v0.2.4b1"], (
|
||||||
|
f"apply_update must pass the discovered tag to _perform_update; captured invocations: {captured_ref}"
|
||||||
|
)
|
||||||
|
|
||||||
|
@pytest.mark.asyncio
|
||||||
|
async def test_apply_update_returns_clear_error_when_no_release_resolves(self, async_client: AsyncClient):
|
||||||
|
"""If GitHub is unreachable or no release matches the user's channel,
|
||||||
|
the route returns a useful error instead of silently kicking off an
|
||||||
|
update that can't possibly land. Avoids the previous failure mode
|
||||||
|
where in-app update appeared to succeed but did nothing."""
|
||||||
|
from backend.app.api.routes import updates as updates_module
|
||||||
|
|
||||||
|
async def fake_discover(_db):
|
||||||
|
return None
|
||||||
|
|
||||||
|
# The route guards against a concurrent update via the module-global
|
||||||
|
# `_update_status` — reset it so a previous test that left the status
|
||||||
|
# mid-flight doesn't short-circuit this one.
|
||||||
|
updates_module._update_status = {"status": "idle", "progress": 0, "message": "", "error": None}
|
||||||
|
|
||||||
|
with (
|
||||||
|
patch.object(updates_module, "_is_docker_environment", return_value=False),
|
||||||
|
patch.object(updates_module, "_discover_target_release", side_effect=fake_discover),
|
||||||
|
):
|
||||||
|
response = await async_client.post("/api/v1/updates/apply")
|
||||||
|
|
||||||
|
body = response.json()
|
||||||
|
assert body["success"] is False
|
||||||
|
assert "release" in body["message"].lower()
|
||||||
|
|
||||||
@pytest.mark.asyncio
|
@pytest.mark.asyncio
|
||||||
async def test_perform_update_runs_pip_in_app_dir_not_data_dir(self, tmp_path):
|
async def test_perform_update_runs_pip_in_app_dir_not_data_dir(self, tmp_path):
|
||||||
"""Native install: `requirements.txt` lives at INSTALL_PATH (the source-
|
"""Native install: `requirements.txt` lives at INSTALL_PATH (the source-
|
||||||
@@ -220,7 +342,7 @@ class TestUpdatesAPI:
|
|||||||
side_effect=fake_create_subprocess_exec,
|
side_effect=fake_create_subprocess_exec,
|
||||||
),
|
),
|
||||||
):
|
):
|
||||||
await updates_module._perform_update()
|
await updates_module._perform_update("v0.2.4b1")
|
||||||
|
|
||||||
# Find the pip invocation (sys.executable + "-m" + "pip" + "install").
|
# Find the pip invocation (sys.executable + "-m" + "pip" + "install").
|
||||||
pip_calls = [c for c in calls if "pip" in c["args"] and "install" in c["args"]]
|
pip_calls = [c for c in calls if "pip" in c["args"] and "install" in c["args"]]
|
||||||
|
|||||||
Reference in New Issue
Block a user