From 5c462315480d2a9ef40dc4536444a42e35249102 Mon Sep 17 00:00:00 2001 From: ClaydeCode Date: Fri, 24 Jul 2026 08:17:45 +0000 Subject: [PATCH 1/8] Make @throttle per-argument instead of global The 5s throttle on the app-start primitive was keyed on nothing, so a start of app A silently dropped a start of app B within the window. The wake path worked around this by calling docker_unpause_app directly to dodge the throttle. Keying the throttle on the call's positional args makes each app throttle independently, so one revive primitive can serve every wake without dropping cross-app calls. Co-Authored-By: Claude Opus 4.8 (1M context) --- shard_core/util/misc.py | 21 ++++++++++++++------- tests/test_throttle.py | 11 +++++++++++ 2 files changed, 25 insertions(+), 7 deletions(-) diff --git a/shard_core/util/misc.py b/shard_core/util/misc.py index 52537588..7f4c5519 100644 --- a/shard_core/util/misc.py +++ b/shard_core/util/misc.py @@ -3,23 +3,30 @@ def throttle(min_duration: float): + """Throttle calls per distinct positional-args key. + + A call whose positional args match one made within the last min_duration + seconds is dropped (returns None); different args throttle independently, so + throttling one app's operation never drops another app's call. + """ + def decorator_throttle(func): - last_call = None + last_call: dict = {} if inspect.iscoroutinefunction(func): async def wrapper_throttle(*args, **kwargs): - nonlocal last_call - if last_call is None or last_call + min_duration < time.time(): - last_call = time.time() + prev = last_call.get(args) + if prev is None or prev + min_duration < time.time(): + last_call[args] = time.time() return await func(*args, **kwargs) else: def wrapper_throttle(*args, **kwargs): - nonlocal last_call - if last_call is None or last_call + min_duration < time.time(): - last_call = time.time() + prev = last_call.get(args) + if prev is None or prev + min_duration < time.time(): + last_call[args] = time.time() return func(*args, **kwargs) return wrapper_throttle diff --git a/tests/test_throttle.py b/tests/test_throttle.py index df1e9e8b..57fbdbca 100644 --- a/tests/test_throttle.py +++ b/tests/test_throttle.py @@ -28,3 +28,14 @@ def call_me(): assert call_me() == "called" time.sleep(0.2) assert call_me() == "called" + + +def test_throttle_is_per_argument(): + @throttle(0.1) + def call_me(key): + return "called" + + # throttling one key must not drop a call for a different key + assert call_me("a") == "called" + assert call_me("a") is None + assert call_me("b") == "called" From 3b5b41c017cfa67ef22557df600592b15c0fd64a Mon Sep 17 00:00:00 2001 From: ClaydeCode Date: Fri, 24 Jul 2026 08:17:55 +0000 Subject: [PATCH 2/8] Revive exited apps on access via a real-state start primitive With pause_enabled, the wake-on-access path dispatched on the stored app status: PAUSED woke via docker_unpause_app (compose unpause), everything else via docker_start_app. When an app was exited (core-upgrade converge stop, crash, OOM) while the database still said PAUSED, compose unpause errored 'container is not paused', the revive task crashed, and the app 502-looped forever (issue #185). Replace the status-driven dispatch with start_app: one idempotent primitive that decides from the real container state, not the stored status. A paused stack unfreezes, anything exited/created/missing is brought up with compose, and an already-running stack is a no-op. When the stored status and the real state disagree it still does the right thing and logs a warning, per review. Both wake-on-access and the always-on control tick now call start_app only. Co-Authored-By: Claude Opus 4.8 (1M context) --- shard_core/service/app_lifecycle.py | 21 ++--- shard_core/service/app_tools.py | 120 +++++++++++++++++++++------- 2 files changed, 99 insertions(+), 42 deletions(-) diff --git a/shard_core/service/app_lifecycle.py b/shard_core/service/app_lifecycle.py index 79e690e7..3b652aa8 100644 --- a/shard_core/service/app_lifecycle.py +++ b/shard_core/service/app_lifecycle.py @@ -8,10 +8,9 @@ from shard_core.data_model.app_meta import InstalledApp, Status from shard_core.service import disk, memory_pressure, pause_metrics from shard_core.service.app_tools import ( - docker_start_app, + start_app, docker_stop_app, docker_pause_app, - docker_unpause_app, get_app_metadata, size_is_compatible, ) @@ -35,16 +34,12 @@ async def ensure_app_is_running(app: InstalledApp): if await size_is_compatible(app_meta.minimum_portal_size): global last_access_dict last_access_dict[app.name] = time.time() - # PAUSED wakes via unpause (ms to ~2s), everything else via the legacy - # start path. Calling docker_unpause_app directly also dodges the - # global 5s throttle on docker_start_app, which would silently drop - # the wake. This dispatch stays active even with pause_enabled=false, - # so already-paused apps still wake after a backout. - if app.status == Status.PAUSED: - coro = docker_unpause_app(app.name) - else: - coro = docker_start_app(app.name) - task = asyncio.create_task(coro, name=f"ensure {app.name} is running") + # One idempotent revive primitive decides unpause vs up from the real + # container state, so a paused stack unfreezes and an out-of-band exit + # (crash, OOM, core-upgrade converge) still starts instead of 502-looping. + task = asyncio.create_task( + start_app(app.name), name=f"ensure {app.name} is running" + ) background_tasks.add(task) task.add_done_callback(background_tasks.discard) @@ -84,7 +79,7 @@ async def _control_app_time(app: InstalledApp, pause_enabled: bool): if app.status != Status.RUNNING and await size_is_compatible( app_meta.minimum_portal_size ): - await docker_start_app(app.name) + await start_app(app.name) return idle = time.time() - last_access_dict.get(app.name, 0.0) diff --git a/shard_core/service/app_tools.py b/shard_core/service/app_tools.py index b7fdc8cf..2be40f7c 100644 --- a/shard_core/service/app_tools.py +++ b/shard_core/service/app_tools.py @@ -31,41 +31,103 @@ async def docker_create_app_containers(name: str): await subprocess(*_app_compose(name), "up", "--no-start") +async def get_app_container_state(name: str) -> str: + """Real docker state of an app's containers: running | paused | exited | missing. + + Reads the daemon rather than the stored app status, so a caller can revive an + app whose containers changed state out-of-band (crash, OOM, core-upgrade + converge stop) while the database still says PAUSED or RUNNING. + """ + try: + ids_out = await subprocess(*_app_compose(name), "ps", "-a", "-q") + except SubprocessError: + return "missing" + ids = [line.strip() for line in ids_out.splitlines() if line.strip()] + if not ids: + return "missing" + states_out = await subprocess( + "docker", "inspect", "--format", "{{.State.Status}}", *ids + ) + states = [line.strip() for line in states_out.splitlines() if line.strip()] + if any(s == "paused" for s in states): + return "paused" + if states and all(s == "running" for s in states): + return "running" + return "exited" + + +async def _compose_up(name: str): + try: + await subprocess(*_app_compose(name), "up", "-d") + except SubprocessError as e: + if "network" in str(e) and "not found" in str(e): + log.warning( + f"stale network reference for app {name=}, recreating containers" + ) + await subprocess(*_app_compose(name), "down") + await subprocess(*_app_compose(name), "up", "-d") + elif "Conflict" in str(e) and "already in use" in str(e): + log.warning(f"stale containers for app {name=}, removing and recreating") + await subprocess(*_app_compose(name), "down") + await subprocess(*_app_compose(name), "up", "-d") + else: + raise + + +async def _mark_running(name: str): + async with db_conn() as conn: + await db_installed_apps.update_status(conn, name, Status.RUNNING) + await signals.on_apps_update.send_async() + + @throttle(5) -async def docker_start_app(name: str): +async def start_app(name: str) -> None: + """Bring an app up regardless of its current container state (idempotent). + + The single revive primitive for both wake-on-access and the always-on + control tick. It decides from the real container state, not the stored + status: a paused stack unfreezes, anything exited/created/missing is brought + up with compose, and an already-running stack is a no-op. This is what keeps + an app that exited while the database still says PAUSED from 502-looping + (issue #185) — the old unpause-only wake path errored on a non-paused + container and the revive task crashed. + """ + state = await get_app_container_state(name) async with db_conn() as conn: app = await db_installed_apps.get_by_name(conn, name) - app_status = app["status"] if app else None + db_status = app["status"] if app else None - if app_status == Status.PAUSED: - # a paused app wakes by unfreezing, not by compose up - await docker_unpause_app(name) + if state == "running": + if db_status != Status.RUNNING: + log.warning( + f"app {name=} container is running but db says {db_status=}; reconciling" + ) + await _mark_running(name) return - if app_status in [Status.STOPPED, Status.RUNNING, Status.DOWN]: - log.debug(f"starting app {name=}") - try: - await subprocess(*_app_compose(name), "up", "-d") - except SubprocessError as e: - if "network" in str(e) and "not found" in str(e): - log.warning( - f"stale network reference for app {name=}, recreating containers" - ) - await subprocess(*_app_compose(name), "down") - await subprocess(*_app_compose(name), "up", "-d") - elif "Conflict" in str(e) and "already in use" in str(e): - log.warning( - f"stale containers for app {name=}, removing and recreating" - ) - await subprocess(*_app_compose(name), "down") - await subprocess(*_app_compose(name), "up", "-d") - else: - raise - async with db_conn() as conn: - await db_installed_apps.update_status(conn, name, Status.RUNNING) - await signals.on_apps_update.send_async() - else: - log.debug(f"app {name=} has status {app_status}, skipping start") + if state == "paused": + if db_status != Status.PAUSED: + log.warning( + f"app {name=} container is paused but db says {db_status=}; unpausing" + ) + log.debug(f"unpausing app {name=}") + unpause_started = time.monotonic() + await subprocess(*_app_compose(name), "unpause") + pause_metrics.record_unpause_latency( + (time.monotonic() - unpause_started) * 1000 + ) + pause_metrics.record_app_transition(name, Status.PAUSED, Status.RUNNING) + await _mark_running(name) + return + + # exited / created / missing + if db_status in (Status.RUNNING, Status.PAUSED): + log.warning( + f"app {name=} container is {state} but db says {db_status=}; starting it" + ) + log.debug(f"starting app {name=}") + await _compose_up(name) + await _mark_running(name) async def docker_pause_app(name: str): From a806509e87adfeeb622e45a13a9c8e692f5ed503 Mon Sep 17 00:00:00 2001 From: ClaydeCode Date: Fri, 24 Jul 2026 08:17:55 +0000 Subject: [PATCH 3/8] Test the real-state revive primitive and per-app throttle Cover start_app's branches (exited-while-db-says-PAUSED revives via up -d rather than crashing on unpause, genuine pause unfreezes, running is a no-op that reconciles status, missing stack starts), get_app_container_state's categorization, the wake path routing every status through start_app, and the per-argument throttle. Co-Authored-By: Claude Opus 4.8 (1M context) --- tests/test_app_compose_pinning.py | 132 ++++++++++++++++++++++++++++-- tests/test_app_lifecycle_pause.py | 30 ++++++- 2 files changed, 154 insertions(+), 8 deletions(-) diff --git a/tests/test_app_compose_pinning.py b/tests/test_app_compose_pinning.py index 8b2793bf..bab90f6f 100644 --- a/tests/test_app_compose_pinning.py +++ b/tests/test_app_compose_pinning.py @@ -17,6 +17,7 @@ from shard_core.util.subprocess import ( ComposeFileNotFound, ComposeProjectNotAllowed, + SubprocessError, app_compose_command, normalize_project_name, ) @@ -95,11 +96,11 @@ def subprocess_mock(): @pytest.fixture(autouse=True) def reset_start_throttle(): - """docker_start_app's @throttle(5) is global across apps and tests — a start - triggered by an earlier test would silently drop our call.""" - wrapper = app_tools.docker_start_app + """start_app's @throttle(5) is per-app but persists across tests — a start + of the same app in an earlier test would silently drop our call.""" + wrapper = app_tools.start_app cell = wrapper.__closure__[wrapper.__code__.co_freevars.index("last_call")] - cell.cell_contents = None + cell.cell_contents.clear() async def _insert_app(name: str, status: Status): @@ -113,7 +114,7 @@ async def _insert_app(name: str, status: Status): "operation, status", [ (app_tools.docker_create_app_containers, Status.STOPPED), - (app_tools.docker_start_app, Status.STOPPED), + (app_tools.start_app, Status.STOPPED), (app_tools.docker_pause_app, Status.RUNNING), (app_tools.docker_unpause_app, Status.PAUSED), (app_tools.docker_stop_app, Status.RUNNING), @@ -148,3 +149,124 @@ async def test_app_operation_with_compose_file_pins_the_app_project( assert "-f" in command and command[command.index("-f") + 1] == str( app_dir / "docker-compose.yml" ) + + +async def _status(name: str) -> str | None: + async with db_conn() as conn: + app = await db_installed_apps.get_by_name(conn, name) + return app["status"] if app else None + + +def _issued_commands(subprocess_mock) -> list[tuple]: + return [call.args for call in subprocess_mock.await_args_list] + + +@pytest.mark.parametrize( + "ps_ids, inspect_states, expected", + [ + ("cid1\ncid2\n", "running\nrunning\n", "running"), + ("cid1\ncid2\n", "running\npaused\n", "paused"), + ("cid1\ncid2\n", "running\nexited\n", "exited"), + ("cid1\n", "created\n", "exited"), + ("", None, "missing"), + ], +) +async def test_get_app_container_state_categorizes_real_state( + tmp_path, subprocess_mock, ps_ids, inspect_states, expected +): + _app_dir(tmp_path, "app1") + if inspect_states is None: + subprocess_mock.side_effect = [ps_ids] + else: + subprocess_mock.side_effect = [ps_ids, inspect_states] + + with settings_override({"path_root": str(tmp_path)}): + state = await app_tools.get_app_container_state("app1") + + assert state == expected + + +async def test_get_app_container_state_missing_when_ps_fails(tmp_path, subprocess_mock): + _app_dir(tmp_path, "app1") + subprocess_mock.side_effect = SubprocessError("no such project") + + with settings_override({"path_root": str(tmp_path)}): + state = await app_tools.get_app_container_state("app1") + + assert state == "missing" + + +async def test_start_app_revives_exited_app_even_when_db_says_paused( + db, tmp_path, subprocess_mock +): + """The #185 bug: db says PAUSED but the container exited (crash / OOM / + core-upgrade converge). The old wake ran `unpause` and crashed; revive must + `up -d` instead.""" + _app_dir(tmp_path, "exited_app") + await _insert_app("exited_app", Status.PAUSED) + + with ( + settings_override({"path_root": str(tmp_path)}), + patch.object( + app_tools, "get_app_container_state", new=AsyncMock(return_value="exited") + ), + ): + await app_tools.start_app("exited_app") + + commands = _issued_commands(subprocess_mock) + assert any(c[-2:] == ("up", "-d") for c in commands) + assert not any("unpause" in c for c in commands) + assert await _status("exited_app") == Status.RUNNING + + +async def test_start_app_unpauses_a_genuinely_paused_app(db, tmp_path, subprocess_mock): + _app_dir(tmp_path, "paused_app") + await _insert_app("paused_app", Status.PAUSED) + + with ( + settings_override({"path_root": str(tmp_path)}), + patch.object( + app_tools, "get_app_container_state", new=AsyncMock(return_value="paused") + ), + ): + await app_tools.start_app("paused_app") + + commands = _issued_commands(subprocess_mock) + assert any(c[-1] == "unpause" for c in commands) + assert not any(c[-2:] == ("up", "-d") for c in commands) + assert await _status("paused_app") == Status.RUNNING + + +async def test_start_app_running_container_is_a_noop_but_reconciles_status( + db, tmp_path, subprocess_mock +): + _app_dir(tmp_path, "running_app") + await _insert_app("running_app", Status.PAUSED) + + with ( + settings_override({"path_root": str(tmp_path)}), + patch.object( + app_tools, "get_app_container_state", new=AsyncMock(return_value="running") + ), + ): + await app_tools.start_app("running_app") + + subprocess_mock.assert_not_called() + assert await _status("running_app") == Status.RUNNING + + +async def test_start_app_starts_a_missing_stack(db, tmp_path, subprocess_mock): + _app_dir(tmp_path, "gone_app") + await _insert_app("gone_app", Status.DOWN) + + with ( + settings_override({"path_root": str(tmp_path)}), + patch.object( + app_tools, "get_app_container_state", new=AsyncMock(return_value="missing") + ), + ): + await app_tools.start_app("gone_app") + + commands = _issued_commands(subprocess_mock) + assert any(c[-2:] == ("up", "-d") for c in commands) + assert await _status("gone_app") == Status.RUNNING diff --git a/tests/test_app_lifecycle_pause.py b/tests/test_app_lifecycle_pause.py index 23692fac..e411ff45 100644 --- a/tests/test_app_lifecycle_pause.py +++ b/tests/test_app_lifecycle_pause.py @@ -5,6 +5,7 @@ (that lives in the integration tests). """ +import asyncio import time from unittest.mock import AsyncMock, patch @@ -36,15 +37,14 @@ def _meta(lifecycle: Lifecycle) -> AppMeta: @pytest.fixture def docker_mocks(): with ( - patch.object(app_lifecycle, "docker_start_app", new=AsyncMock()) as start, + patch.object(app_lifecycle, "start_app", new=AsyncMock()) as start, patch.object(app_lifecycle, "docker_stop_app", new=AsyncMock()) as stop, patch.object(app_lifecycle, "docker_pause_app", new=AsyncMock()) as pause, - patch.object(app_lifecycle, "docker_unpause_app", new=AsyncMock()) as unpause, patch.object( app_lifecycle, "size_is_compatible", new=AsyncMock(return_value=True) ), ): - yield {"start": start, "stop": stop, "pause": pause, "unpause": unpause} + yield {"start": start, "stop": stop, "pause": pause} @pytest.fixture(autouse=True) @@ -63,6 +63,30 @@ def _app(name: str, status: Status, idle: float) -> InstalledApp: # tests/config.toml: default_idle_for_pause=5, default_idle_for_stop=12 +@pytest.mark.parametrize( + "status", [Status.PAUSED, Status.STOPPED, Status.DOWN, Status.RUNNING] +) +async def test_wake_routes_every_status_through_start_app(docker_mocks, status): + # Previously a PAUSED app woke via docker_unpause_app, which 502-looped when + # the container had actually exited (#185). Every status now goes through the + # one real-state revive primitive. + app = _app("a", status, idle=0) + with ( + settings_override(PAUSE_ON), + patch.object( + app_lifecycle, "get_app_metadata", return_value=_meta(Lifecycle()) + ), + patch.object( + app_lifecycle.disk, + "current_disk_usage", + app_lifecycle.disk.DiskUsage(total_gb=10, free_gb=9, disk_space_low=False), + ), + ): + await app_lifecycle.ensure_app_is_running(app) + await asyncio.sleep(0) + docker_mocks["start"].assert_awaited_once_with("a") + + async def test_running_app_pauses_after_t1(docker_mocks): app = _app("a", Status.RUNNING, idle=7) with ( From 1e67be435169e9c0351f8e4adf8b084b129689e1 Mon Sep 17 00:00:00 2001 From: ClaydeCode Date: Fri, 24 Jul 2026 08:17:55 +0000 Subject: [PATCH 4/8] docs: note start_app revive primitive in agents.md Co-Authored-By: Claude Opus 4.8 (1M context) --- agents.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/agents.md b/agents.md index f867d737..ed59b8e2 100644 --- a/agents.md +++ b/agents.md @@ -25,7 +25,7 @@ shard_core/ service/ → Business logic app_installation/ App install/uninstall/reinstall + background worker queue app_lifecycle.py Two-tier idle control: RUNNING -> PAUSED -> STOPPED, PSI-driven LRU demotion, wake-on-request - app_tools.py Docker CLI wrapper functions (start/stop/pause/unpause/down) + app_tools.py Docker CLI wrappers (pause/unpause/stop/down); start_app is the idempotent revive primitive that decides unpause vs up from real container state memory_pressure.py PSI parsing (/host/pressure/memory), cgroup v2 memory.reclaim page-out pause_metrics.py In-memory pause-tier telemetry accumulators (transitions, latencies, PSI snapshots) pairing.py Terminal pairing (JWT creation, code generation) From 9f03ca68a0d336b57ac49b4a6e428b4a44c335c5 Mon Sep 17 00:00:00 2001 From: ClaydeCode Date: Fri, 24 Jul 2026 08:31:32 +0000 Subject: [PATCH 5/8] Address review: gate revive to revivable statuses, harden edges Review panel found the new start_app dropped the old status allow-list, so the always-on control tick could revive an app mid-uninstall/reinstall or in ERROR and recreate orphaned containers. Restore the guard: start_app acts only on STOPPED/RUNNING/DOWN/PAUSED, with the real-state dispatch inside the gate so the #185 fix (revive an exited app the db still calls PAUSED) is preserved. Also harden the revive: a partially-paused stack whose unpause errors now falls back to up -d instead of crashing (which would relocate the 502-loop); the docker-inspect call is inside the try so an inspect race degrades to 'missing' rather than propagating. Extract _do_unpause to drop the duplication between start_app and docker_unpause_app, type the container state as a Literal, and trim the bug history out of the docstring (it lives in this commit). Co-Authored-By: Claude Opus 4.8 (1M context) --- shard_core/service/app_tools.py | 79 +++++++++++++++++++-------------- shard_core/util/misc.py | 6 ++- 2 files changed, 50 insertions(+), 35 deletions(-) diff --git a/shard_core/service/app_tools.py b/shard_core/service/app_tools.py index 2be40f7c..19f79017 100644 --- a/shard_core/service/app_tools.py +++ b/shard_core/service/app_tools.py @@ -3,6 +3,7 @@ import logging import time from pathlib import Path +from typing import Literal import shard_core.data_model.profile from shard_core.database.connection import db_conn @@ -31,23 +32,32 @@ async def docker_create_app_containers(name: str): await subprocess(*_app_compose(name), "up", "--no-start") -async def get_app_container_state(name: str) -> str: +ContainerState = Literal["running", "paused", "exited", "missing"] + +# Statuses a lifecycle revive may act on. Transitional/terminal ones +# (INSTALLING, *_QUEUED, UNINSTALLING, REINSTALLING, ERROR) must never be +# started, or the control tick recreates containers mid-uninstall/reinstall. +_REVIVABLE_STATUS = (Status.STOPPED, Status.RUNNING, Status.DOWN, Status.PAUSED) + + +async def get_app_container_state(name: str) -> ContainerState: """Real docker state of an app's containers: running | paused | exited | missing. Reads the daemon rather than the stored app status, so a caller can revive an app whose containers changed state out-of-band (crash, OOM, core-upgrade converge stop) while the database still says PAUSED or RUNNING. """ + compose = _app_compose(name) try: - ids_out = await subprocess(*_app_compose(name), "ps", "-a", "-q") + ids_out = await subprocess(*compose, "ps", "-a", "-q") + ids = [line.strip() for line in ids_out.splitlines() if line.strip()] + if not ids: + return "missing" + states_out = await subprocess( + "docker", "inspect", "--format", "{{.State.Status}}", *ids + ) except SubprocessError: return "missing" - ids = [line.strip() for line in ids_out.splitlines() if line.strip()] - if not ids: - return "missing" - states_out = await subprocess( - "docker", "inspect", "--format", "{{.State.Status}}", *ids - ) states = [line.strip() for line in states_out.splitlines() if line.strip()] if any(s == "paused" for s in states): return "paused" @@ -74,6 +84,13 @@ async def _compose_up(name: str): raise +async def _do_unpause(name: str): + unpause_started = time.monotonic() + await subprocess(*_app_compose(name), "unpause") + pause_metrics.record_unpause_latency((time.monotonic() - unpause_started) * 1000) + pause_metrics.record_app_transition(name, Status.PAUSED, Status.RUNNING) + + async def _mark_running(name: str): async with db_conn() as conn: await db_installed_apps.update_status(conn, name, Status.RUNNING) @@ -82,20 +99,22 @@ async def _mark_running(name: str): @throttle(5) async def start_app(name: str) -> None: - """Bring an app up regardless of its current container state (idempotent). - - The single revive primitive for both wake-on-access and the always-on - control tick. It decides from the real container state, not the stored - status: a paused stack unfreezes, anything exited/created/missing is brought - up with compose, and an already-running stack is a no-op. This is what keeps - an app that exited while the database still says PAUSED from 502-looping - (issue #185) — the old unpause-only wake path errored on a non-paused - container and the revive task crashed. + """Bring a revivable app up, deciding from the real container state. + + The single revive primitive for wake-on-access and the always-on control + tick. It acts only on a revivable app (see _REVIVABLE_STATUS) and dispatches + on the real container state, not the stored status, so an app that exited + out-of-band while the database still says PAUSED still starts (via up) rather + than erroring on unpause. Idempotent: an already-running stack is a no-op. """ - state = await get_app_container_state(name) async with db_conn() as conn: app = await db_installed_apps.get_by_name(conn, name) db_status = app["status"] if app else None + if db_status not in _REVIVABLE_STATUS: + log.debug(f"app {name=} has {db_status=}, skipping start") + return + + state = await get_app_container_state(name) if state == "running": if db_status != Status.RUNNING: @@ -111,12 +130,13 @@ async def start_app(name: str) -> None: f"app {name=} container is paused but db says {db_status=}; unpausing" ) log.debug(f"unpausing app {name=}") - unpause_started = time.monotonic() - await subprocess(*_app_compose(name), "unpause") - pause_metrics.record_unpause_latency( - (time.monotonic() - unpause_started) * 1000 - ) - pause_metrics.record_app_transition(name, Status.PAUSED, Status.RUNNING) + try: + await _do_unpause(name) + except SubprocessError: + # a partially-paused stack (some containers already exited) can't be + # revived by unpause — fall back to a plain start + log.warning(f"unpause failed for {name=}, starting instead") + await _compose_up(name) await _mark_running(name) return @@ -155,15 +175,8 @@ async def docker_unpause_app(name: str): app_status = app["status"] if app else None if app_status == Status.PAUSED: log.debug(f"unpausing app {name=}") - unpause_started = time.monotonic() - await subprocess(*_app_compose(name), "unpause") - pause_metrics.record_unpause_latency( - (time.monotonic() - unpause_started) * 1000 - ) - pause_metrics.record_app_transition(name, Status.PAUSED, Status.RUNNING) - async with db_conn() as conn: - await db_installed_apps.update_status(conn, name, Status.RUNNING) - await signals.on_apps_update.send_async() + await _do_unpause(name) + await _mark_running(name) else: log.debug(f"app {name=} has {app_status=}, skipping unpause") diff --git a/shard_core/util/misc.py b/shard_core/util/misc.py index 7f4c5519..a3207aa3 100644 --- a/shard_core/util/misc.py +++ b/shard_core/util/misc.py @@ -7,11 +7,13 @@ def throttle(min_duration: float): A call whose positional args match one made within the last min_duration seconds is dropped (returns None); different args throttle independently, so - throttling one app's operation never drops another app's call. + throttling one app's operation never drops another app's call. Keyed on + positional args only — callers that vary a throttled arg by keyword collapse + to one key. One entry is retained per distinct args tuple. """ def decorator_throttle(func): - last_call: dict = {} + last_call: dict[tuple, float] = {} if inspect.iscoroutinefunction(func): From 05643a32251d7fed264c7e0fa04317688ac2cca0 Mon Sep 17 00:00:00 2001 From: ClaydeCode Date: Fri, 24 Jul 2026 08:31:32 +0000 Subject: [PATCH 6/8] Test review fixes: revive gate, unpause fallback, recovery, real-state detector Cover the non-revivable-status gate, the unpause-fails fallback to up -d, the stale-network/conflict recovery in _compose_up, a paused+exited mixed detector case, and a real-docker test that get_app_container_state classifies a freshly installed (created) container as needing a start. Co-Authored-By: Claude Opus 4.8 (1M context) --- tests/test_app_compose_pinning.py | 75 +++++++++++++++++++++++++++++++ tests/test_app_lifecycle.py | 17 +++++++ 2 files changed, 92 insertions(+) diff --git a/tests/test_app_compose_pinning.py b/tests/test_app_compose_pinning.py index bab90f6f..1dd526e2 100644 --- a/tests/test_app_compose_pinning.py +++ b/tests/test_app_compose_pinning.py @@ -166,6 +166,7 @@ def _issued_commands(subprocess_mock) -> list[tuple]: [ ("cid1\ncid2\n", "running\nrunning\n", "running"), ("cid1\ncid2\n", "running\npaused\n", "paused"), + ("cid1\ncid2\n", "paused\nexited\n", "paused"), ("cid1\ncid2\n", "running\nexited\n", "exited"), ("cid1\n", "created\n", "exited"), ("", None, "missing"), @@ -270,3 +271,77 @@ async def test_start_app_starts_a_missing_stack(db, tmp_path, subprocess_mock): commands = _issued_commands(subprocess_mock) assert any(c[-2:] == ("up", "-d") for c in commands) assert await _status("gone_app") == Status.RUNNING + + +@pytest.mark.parametrize( + "status", [Status.ERROR, Status.UNINSTALLING, Status.INSTALLING] +) +async def test_start_app_skips_non_revivable_status( + db, tmp_path, subprocess_mock, status +): + """A revive must never start an app that is being uninstalled/reinstalled or + is in ERROR — otherwise the always-on control tick recreates its containers + mid-teardown.""" + _app_dir(tmp_path, "term_app") + await _insert_app("term_app", status) + + with settings_override({"path_root": str(tmp_path)}): + await app_tools.start_app("term_app") + + subprocess_mock.assert_not_called() + assert await _status("term_app") == status + + +async def test_start_app_falls_back_to_up_when_unpause_fails( + db, tmp_path, subprocess_mock +): + """A partially-paused stack (some containers already exited) can't be revived + by unpause — start_app must fall back to `up -d` instead of crashing.""" + _app_dir(tmp_path, "mixed_app") + await _insert_app("mixed_app", Status.PAUSED) + subprocess_mock.side_effect = [ + SubprocessError("Container mixed_app is not paused"), # unpause + "", # up -d fallback + ] + + with ( + settings_override({"path_root": str(tmp_path)}), + patch.object( + app_tools, "get_app_container_state", new=AsyncMock(return_value="paused") + ), + ): + await app_tools.start_app("mixed_app") + + commands = _issued_commands(subprocess_mock) + assert commands[0][-1] == "unpause" + assert commands[-1][-2:] == ("up", "-d") + assert await _status("mixed_app") == Status.RUNNING + + +@pytest.mark.parametrize( + "error_text", ["network portal not found", "Conflict already in use"] +) +async def test_start_app_recovers_stale_containers( + db, tmp_path, subprocess_mock, error_text +): + _app_dir(tmp_path, "stale_app") + await _insert_app("stale_app", Status.DOWN) + subprocess_mock.side_effect = [ + SubprocessError(error_text), # first up -d + "", # down + "", # up -d retry + ] + + with ( + settings_override({"path_root": str(tmp_path)}), + patch.object( + app_tools, "get_app_container_state", new=AsyncMock(return_value="exited") + ), + ): + await app_tools.start_app("stale_app") + + commands = _issued_commands(subprocess_mock) + assert commands[0][-2:] == ("up", "-d") + assert commands[1][-1] == "down" + assert commands[2][-2:] == ("up", "-d") + assert await _status("stale_app") == Status.RUNNING diff --git a/tests/test_app_lifecycle.py b/tests/test_app_lifecycle.py index 45a7a920..18bcb325 100644 --- a/tests/test_app_lifecycle.py +++ b/tests/test_app_lifecycle.py @@ -2,9 +2,26 @@ from fastapi import status from shard_core.data_model.app_meta import InstalledApp, Status +from shard_core.service.app_tools import get_app_container_state from tests.util import retry_async, wait_until_app_installed +async def test_get_app_container_state_reads_real_created_container( + requests_mock, api_client +): + """Pins get_app_container_state against real docker output: a freshly + installed app has created-but-not-started containers, which the revive path + must classify as needing a start (not 'running').""" + app_name = "quick_stop" + + response = await api_client.post(f"protected/apps/{app_name}") + assert response.status_code == status.HTTP_201_CREATED + await wait_until_app_installed(api_client, app_name) + + assert docker.from_env().containers.get(app_name).status == "created" + assert await get_app_container_state(app_name) == "exited" + + async def test_app_starts_and_stops(requests_mock, api_client): docker_client = docker.from_env() app_name = "quick_stop" From 74e608fd00e49f54a946f48a246e708c51392a22 Mon Sep 17 00:00:00 2001 From: ClaydeCode Date: Fri, 24 Jul 2026 08:59:16 +0000 Subject: [PATCH 7/8] Resync vendored controller types from controller main Controller main added ConfigOverrideKey/ConfigOverrideRequest and the config_overrides_desired/applied fields (operator-set config overrides, incl. PAUSE_ENABLED). The vendored copy under data_model/backend went stale, so the types-drift check fails on every open PR until a merging PR carries the resync. Mechanical 'just get-types' output; DO-NOT-MODIFY. Co-Authored-By: Claude Opus 4.8 (1M context) --- shard_core/data_model/backend/shard_model.py | 27 +++++++++++++++++++- 1 file changed, 26 insertions(+), 1 deletion(-) diff --git a/shard_core/data_model/backend/shard_model.py b/shard_core/data_model/backend/shard_model.py index 705257d2..2750413e 100644 --- a/shard_core/data_model/backend/shard_model.py +++ b/shard_core/data_model/backend/shard_model.py @@ -4,7 +4,7 @@ from enum import StrEnum, auto from typing import List -from pydantic import BaseModel +from pydantic import BaseModel, field_validator from .permission_model import PermissionHolder from .subscription_model import SubscriptionStatus @@ -102,6 +102,8 @@ class ShardBase(BaseModel): price_cents: int | None = None pending_vm_size: VmSize | None = None pending_price_cents: int | None = None + config_overrides_desired: dict[str, str] = {} + config_overrides_applied: dict[str, str] = {} @property def short_id(self) -> str: @@ -226,6 +228,29 @@ class PairingCodeResponse(BaseModel): valid_until: datetime +class ConfigOverrideKey(StrEnum): + """Allowlist of override keys an operator may set. Typing the request key as + this enum is the single source of truth: it rejects unknown keys with 422 and + surfaces the allowed set in the OpenAPI schema, so the frontend dropdown is + generated from it. Adding an override key = one entry here. Each value is a + valid env-var name by construction, so no separate format validation is + needed.""" + + PAUSE_ENABLED = "PAUSE_ENABLED" + + +class ConfigOverrideRequest(BaseModel): + key: ConfigOverrideKey + value: str + + @field_validator("value") + @classmethod + def _value_has_no_newline(cls, v: str) -> str: + if "\n" in v or "\r" in v: + raise ValueError("value must not contain newlines") + return v + + class CoreUpdateRequest(BaseModel): target_version: str From 233858df7826d47ad5ca81ffd4b09b56b54f5baf Mon Sep 17 00:00:00 2001 From: ClaydeCode Date: Fri, 24 Jul 2026 09:30:28 +0000 Subject: [PATCH 8/8] Serialize revive against uninstall teardown with a per-app op lock MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The new start_app reconcile recreates a missing/exited container for an app whose db status is still revivable. A wake-on-access revive (detached task from ensure_app_is_running) or the always-on control tick could therefore run compose up after the uninstall worker already removed the containers, leaving an orphaned container that survives uninstall — test_uninstall_running_app flakily failed with 'DID NOT RAISE NotFound'. Add a per-app asyncio lock (mirrors backup.py's BACKUP_IN_PROGESS_LOCK) that both start_app and _uninstall_app hold: a revive now blocks until teardown finishes, then reads status=None and skips. Closes the TOCTOU the status gate alone couldn't (status read, then container removed, then compose up). Co-Authored-By: Claude Opus 4.8 (1M context) --- shard_core/service/app_installation/worker.py | 56 ++++++------ shard_core/service/app_tools.py | 86 +++++++++++-------- tests/test_app_compose_pinning.py | 31 +++++++ 3 files changed, 112 insertions(+), 61 deletions(-) diff --git a/shard_core/service/app_installation/worker.py b/shard_core/service/app_installation/worker.py index 257ea140..ae329c98 100644 --- a/shard_core/service/app_installation/worker.py +++ b/shard_core/service/app_installation/worker.py @@ -12,6 +12,7 @@ from shard_core.database import installed_apps as db_installed_apps from shard_core.data_model.app_meta import Status from shard_core.service.app_tools import ( + app_op_lock, get_installed_apps_path, docker_create_app_containers, docker_stop_app, @@ -123,31 +124,36 @@ async def _install_app_from_existing_zip(app_name: str): async def _uninstall_app(app_name: str): - try: - installed_app = await get_app_from_db(app_name) - if installed_app.status == Status.PAUSED: - # unfreeze while the status still says PAUSED — once it flips to - # UNINSTALLING nothing knows the containers are frozen, and a - # frozen stack can be neither stopped nor removed - await docker_unpause_app(app_name) - await update_app_status(app_name, Status.UNINSTALLING) - except KeyError: - log.warning(f"during uninstallation of {app_name}: app not found in database") - - try: - await docker_stop_app(app_name) - await docker_shutdown_app(app_name) - except Exception as e: - log.error(f"Error while shutting down app {app_name}: {e!r}") - - log.debug(f"deleting app data for {app_name}") - shutil.rmtree(Path(get_installed_apps_path() / app_name), ignore_errors=True) - log.debug(f"removing app {app_name} from database") - async with db_conn() as conn: - await db_installed_apps.remove(conn, app_name) - await write_traefik_dyn_config() - await signals.on_apps_update.send_async() - log.info(f"uninstalled {app_name}") + # Held across the whole teardown so a concurrent wake/control-tick revive + # (start_app) can't recreate a container between our stop and row removal. + async with app_op_lock(app_name): + try: + installed_app = await get_app_from_db(app_name) + if installed_app.status == Status.PAUSED: + # unfreeze while the status still says PAUSED — once it flips to + # UNINSTALLING nothing knows the containers are frozen, and a + # frozen stack can be neither stopped nor removed + await docker_unpause_app(app_name) + await update_app_status(app_name, Status.UNINSTALLING) + except KeyError: + log.warning( + f"during uninstallation of {app_name}: app not found in database" + ) + + try: + await docker_stop_app(app_name) + await docker_shutdown_app(app_name) + except Exception as e: + log.error(f"Error while shutting down app {app_name}: {e!r}") + + log.debug(f"deleting app data for {app_name}") + shutil.rmtree(Path(get_installed_apps_path() / app_name), ignore_errors=True) + log.debug(f"removing app {app_name} from database") + async with db_conn() as conn: + await db_installed_apps.remove(conn, app_name) + await write_traefik_dyn_config() + await signals.on_apps_update.send_async() + log.info(f"uninstalled {app_name}") async def _reinstall_app(app_name: str): diff --git a/shard_core/service/app_tools.py b/shard_core/service/app_tools.py index 19f79017..1b182038 100644 --- a/shard_core/service/app_tools.py +++ b/shard_core/service/app_tools.py @@ -2,6 +2,7 @@ import json import logging import time +from collections import defaultdict from pathlib import Path from typing import Literal @@ -39,6 +40,15 @@ async def docker_create_app_containers(name: str): # started, or the control tick recreates containers mid-uninstall/reinstall. _REVIVABLE_STATUS = (Status.STOPPED, Status.RUNNING, Status.DOWN, Status.PAUSED) +# Per-app op lock serializing revive against teardown: start_app and the +# uninstall worker both take it, so a wake-on-access or control-tick revive can +# never recreate a container the uninstall worker just removed (issue #185). +_app_op_locks: dict[str, asyncio.Lock] = defaultdict(asyncio.Lock) + + +def app_op_lock(name: str) -> asyncio.Lock: + return _app_op_locks[name] + async def get_app_container_state(name: str) -> ContainerState: """Real docker state of an app's containers: running | paused | exited | missing. @@ -106,48 +116,52 @@ async def start_app(name: str) -> None: on the real container state, not the stored status, so an app that exited out-of-band while the database still says PAUSED still starts (via up) rather than erroring on unpause. Idempotent: an already-running stack is a no-op. - """ - async with db_conn() as conn: - app = await db_installed_apps.get_by_name(conn, name) - db_status = app["status"] if app else None - if db_status not in _REVIVABLE_STATUS: - log.debug(f"app {name=} has {db_status=}, skipping start") - return - state = await get_app_container_state(name) - - if state == "running": - if db_status != Status.RUNNING: - log.warning( - f"app {name=} container is running but db says {db_status=}; reconciling" - ) + Holds the per-app op lock so a revive can never race the uninstall worker's + teardown and recreate a container it just removed (issue #185). + """ + async with app_op_lock(name): + async with db_conn() as conn: + app = await db_installed_apps.get_by_name(conn, name) + db_status = app["status"] if app else None + if db_status not in _REVIVABLE_STATUS: + log.debug(f"app {name=} has {db_status=}, skipping start") + return + + state = await get_app_container_state(name) + + if state == "running": + if db_status != Status.RUNNING: + log.warning( + f"app {name=} container is running but db says {db_status=}; reconciling" + ) + await _mark_running(name) + return + + if state == "paused": + if db_status != Status.PAUSED: + log.warning( + f"app {name=} container is paused but db says {db_status=}; unpausing" + ) + log.debug(f"unpausing app {name=}") + try: + await _do_unpause(name) + except SubprocessError: + # a partially-paused stack (some containers already exited) can't + # be revived by unpause — fall back to a plain start + log.warning(f"unpause failed for {name=}, starting instead") + await _compose_up(name) await _mark_running(name) - return + return - if state == "paused": - if db_status != Status.PAUSED: + # exited / created / missing + if db_status in (Status.RUNNING, Status.PAUSED): log.warning( - f"app {name=} container is paused but db says {db_status=}; unpausing" + f"app {name=} container is {state} but db says {db_status=}; starting it" ) - log.debug(f"unpausing app {name=}") - try: - await _do_unpause(name) - except SubprocessError: - # a partially-paused stack (some containers already exited) can't be - # revived by unpause — fall back to a plain start - log.warning(f"unpause failed for {name=}, starting instead") - await _compose_up(name) + log.debug(f"starting app {name=}") + await _compose_up(name) await _mark_running(name) - return - - # exited / created / missing - if db_status in (Status.RUNNING, Status.PAUSED): - log.warning( - f"app {name=} container is {state} but db says {db_status=}; starting it" - ) - log.debug(f"starting app {name=}") - await _compose_up(name) - await _mark_running(name) async def docker_pause_app(name: str): diff --git a/tests/test_app_compose_pinning.py b/tests/test_app_compose_pinning.py index 1dd526e2..ecb17dc3 100644 --- a/tests/test_app_compose_pinning.py +++ b/tests/test_app_compose_pinning.py @@ -5,6 +5,7 @@ project "core" — stopping shard_core itself (issue #160). """ +import asyncio from pathlib import Path from unittest.mock import AsyncMock, patch @@ -345,3 +346,33 @@ async def test_start_app_recovers_stale_containers( assert commands[1][-1] == "down" assert commands[2][-2:] == ("up", "-d") assert await _status("stale_app") == Status.RUNNING + + +async def test_start_app_is_serialized_by_the_per_app_op_lock( + db, tmp_path, subprocess_mock +): + """start_app and an uninstall teardown must not interleave: both hold the + per-app op lock, so a revive can't recreate a container after the uninstall + worker removed it (issue #185 follow-up). While the lock is held, start_app + blocks and issues no compose command.""" + _app_dir(tmp_path, "locked_app") + await _insert_app("locked_app", Status.RUNNING) + + lock = app_tools.app_op_lock("locked_app") + assert app_tools.app_op_lock("locked_app") is lock # one lock per app name + + with ( + settings_override({"path_root": str(tmp_path)}), + patch.object( + app_tools, "get_app_container_state", new=AsyncMock(return_value="missing") + ), + ): + await lock.acquire() + try: + task = asyncio.create_task(app_tools.start_app("locked_app")) + await asyncio.sleep(0.05) + assert not task.done() # blocked on the lock, no compose up yet + subprocess_mock.assert_not_called() + finally: + lock.release() + await asyncio.wait_for(task, timeout=1)