Skip to content

Commit 233858d

Browse files
ClaydeCodeclaude
ClaydeCode
and
claude
committed
Serialize revive against uninstall teardown with a per-app op lock
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) <noreply@anthropic.com>
1 parent 74e608f commit 233858d

3 files changed

Lines changed: 112 additions & 61 deletions

File tree

‎shard_core/service/app_installation/worker.py‎

Lines changed: 31 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212
from shard_core.database import installed_apps as db_installed_apps
1313
from shard_core.data_model.app_meta import Status
1414
from shard_core.service.app_tools import (
15+
app_op_lock,
1516
get_installed_apps_path,
1617
docker_create_app_containers,
1718
docker_stop_app,
@@ -123,31 +124,36 @@ async def _install_app_from_existing_zip(app_name: str):
123124

124125

125126
async def _uninstall_app(app_name: str):
126-
try:
127-
installed_app = await get_app_from_db(app_name)
128-
if installed_app.status == Status.PAUSED:
129-
# unfreeze while the status still says PAUSED — once it flips to
130-
# UNINSTALLING nothing knows the containers are frozen, and a
131-
# frozen stack can be neither stopped nor removed
132-
await docker_unpause_app(app_name)
133-
await update_app_status(app_name, Status.UNINSTALLING)
134-
except KeyError:
135-
log.warning(f"during uninstallation of {app_name}: app not found in database")
136-
137-
try:
138-
await docker_stop_app(app_name)
139-
await docker_shutdown_app(app_name)
140-
except Exception as e:
141-
log.error(f"Error while shutting down app {app_name}: {e!r}")
142-
143-
log.debug(f"deleting app data for {app_name}")
144-
shutil.rmtree(Path(get_installed_apps_path() / app_name), ignore_errors=True)
145-
log.debug(f"removing app {app_name} from database")
146-
async with db_conn() as conn:
147-
await db_installed_apps.remove(conn, app_name)
148-
await write_traefik_dyn_config()
149-
await signals.on_apps_update.send_async()
150-
log.info(f"uninstalled {app_name}")
127+
# Held across the whole teardown so a concurrent wake/control-tick revive
128+
# (start_app) can't recreate a container between our stop and row removal.
129+
async with app_op_lock(app_name):
130+
try:
131+
installed_app = await get_app_from_db(app_name)
132+
if installed_app.status == Status.PAUSED:
133+
# unfreeze while the status still says PAUSED — once it flips to
134+
# UNINSTALLING nothing knows the containers are frozen, and a
135+
# frozen stack can be neither stopped nor removed
136+
await docker_unpause_app(app_name)
137+
await update_app_status(app_name, Status.UNINSTALLING)
138+
except KeyError:
139+
log.warning(
140+
f"during uninstallation of {app_name}: app not found in database"
141+
)
142+
143+
try:
144+
await docker_stop_app(app_name)
145+
await docker_shutdown_app(app_name)
146+
except Exception as e:
147+
log.error(f"Error while shutting down app {app_name}: {e!r}")
148+
149+
log.debug(f"deleting app data for {app_name}")
150+
shutil.rmtree(Path(get_installed_apps_path() / app_name), ignore_errors=True)
151+
log.debug(f"removing app {app_name} from database")
152+
async with db_conn() as conn:
153+
await db_installed_apps.remove(conn, app_name)
154+
await write_traefik_dyn_config()
155+
await signals.on_apps_update.send_async()
156+
log.info(f"uninstalled {app_name}")
151157

152158

153159
async def _reinstall_app(app_name: str):

‎shard_core/service/app_tools.py‎

Lines changed: 50 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22
import json
33
import logging
44
import time
5+
from collections import defaultdict
56
from pathlib import Path
67
from typing import Literal
78

@@ -39,6 +40,15 @@ async def docker_create_app_containers(name: str):
3940
# started, or the control tick recreates containers mid-uninstall/reinstall.
4041
_REVIVABLE_STATUS = (Status.STOPPED, Status.RUNNING, Status.DOWN, Status.PAUSED)
4142

43+
# Per-app op lock serializing revive against teardown: start_app and the
44+
# uninstall worker both take it, so a wake-on-access or control-tick revive can
45+
# never recreate a container the uninstall worker just removed (issue #185).
46+
_app_op_locks: dict[str, asyncio.Lock] = defaultdict(asyncio.Lock)
47+
48+
49+
def app_op_lock(name: str) -> asyncio.Lock:
50+
return _app_op_locks[name]
51+
4252

4353
async def get_app_container_state(name: str) -> ContainerState:
4454
"""Real docker state of an app's containers: running | paused | exited | missing.
@@ -106,48 +116,52 @@ async def start_app(name: str) -> None:
106116
on the real container state, not the stored status, so an app that exited
107117
out-of-band while the database still says PAUSED still starts (via up) rather
108118
than erroring on unpause. Idempotent: an already-running stack is a no-op.
109-
"""
110-
async with db_conn() as conn:
111-
app = await db_installed_apps.get_by_name(conn, name)
112-
db_status = app["status"] if app else None
113-
if db_status not in _REVIVABLE_STATUS:
114-
log.debug(f"app {name=} has {db_status=}, skipping start")
115-
return
116119
117-
state = await get_app_container_state(name)
118-
119-
if state == "running":
120-
if db_status != Status.RUNNING:
121-
log.warning(
122-
f"app {name=} container is running but db says {db_status=}; reconciling"
123-
)
120+
Holds the per-app op lock so a revive can never race the uninstall worker's
121+
teardown and recreate a container it just removed (issue #185).
122+
"""
123+
async with app_op_lock(name):
124+
async with db_conn() as conn:
125+
app = await db_installed_apps.get_by_name(conn, name)
126+
db_status = app["status"] if app else None
127+
if db_status not in _REVIVABLE_STATUS:
128+
log.debug(f"app {name=} has {db_status=}, skipping start")
129+
return
130+
131+
state = await get_app_container_state(name)
132+
133+
if state == "running":
134+
if db_status != Status.RUNNING:
135+
log.warning(
136+
f"app {name=} container is running but db says {db_status=}; reconciling"
137+
)
138+
await _mark_running(name)
139+
return
140+
141+
if state == "paused":
142+
if db_status != Status.PAUSED:
143+
log.warning(
144+
f"app {name=} container is paused but db says {db_status=}; unpausing"
145+
)
146+
log.debug(f"unpausing app {name=}")
147+
try:
148+
await _do_unpause(name)
149+
except SubprocessError:
150+
# a partially-paused stack (some containers already exited) can't
151+
# be revived by unpause — fall back to a plain start
152+
log.warning(f"unpause failed for {name=}, starting instead")
153+
await _compose_up(name)
124154
await _mark_running(name)
125-
return
155+
return
126156

127-
if state == "paused":
128-
if db_status != Status.PAUSED:
157+
# exited / created / missing
158+
if db_status in (Status.RUNNING, Status.PAUSED):
129159
log.warning(
130-
f"app {name=} container is paused but db says {db_status=}; unpausing"
160+
f"app {name=} container is {state} but db says {db_status=}; starting it"
131161
)
132-
log.debug(f"unpausing app {name=}")
133-
try:
134-
await _do_unpause(name)
135-
except SubprocessError:
136-
# a partially-paused stack (some containers already exited) can't be
137-
# revived by unpause — fall back to a plain start
138-
log.warning(f"unpause failed for {name=}, starting instead")
139-
await _compose_up(name)
162+
log.debug(f"starting app {name=}")
163+
await _compose_up(name)
140164
await _mark_running(name)
141-
return
142-
143-
# exited / created / missing
144-
if db_status in (Status.RUNNING, Status.PAUSED):
145-
log.warning(
146-
f"app {name=} container is {state} but db says {db_status=}; starting it"
147-
)
148-
log.debug(f"starting app {name=}")
149-
await _compose_up(name)
150-
await _mark_running(name)
151165

152166

153167
async def docker_pause_app(name: str):

‎tests/test_app_compose_pinning.py‎

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
project "core" — stopping shard_core itself (issue #160).
66
"""
77

8+
import asyncio
89
from pathlib import Path
910
from unittest.mock import AsyncMock, patch
1011

@@ -345,3 +346,33 @@ async def test_start_app_recovers_stale_containers(
345346
assert commands[1][-1] == "down"
346347
assert commands[2][-2:] == ("up", "-d")
347348
assert await _status("stale_app") == Status.RUNNING
349+
350+
351+
async def test_start_app_is_serialized_by_the_per_app_op_lock(
352+
db, tmp_path, subprocess_mock
353+
):
354+
"""start_app and an uninstall teardown must not interleave: both hold the
355+
per-app op lock, so a revive can't recreate a container after the uninstall
356+
worker removed it (issue #185 follow-up). While the lock is held, start_app
357+
blocks and issues no compose command."""
358+
_app_dir(tmp_path, "locked_app")
359+
await _insert_app("locked_app", Status.RUNNING)
360+
361+
lock = app_tools.app_op_lock("locked_app")
362+
assert app_tools.app_op_lock("locked_app") is lock # one lock per app name
363+
364+
with (
365+
settings_override({"path_root": str(tmp_path)}),
366+
patch.object(
367+
app_tools, "get_app_container_state", new=AsyncMock(return_value="missing")
368+
),
369+
):
370+
await lock.acquire()
371+
try:
372+
task = asyncio.create_task(app_tools.start_app("locked_app"))
373+
await asyncio.sleep(0.05)
374+
assert not task.done() # blocked on the lock, no compose up yet
375+
subprocess_mock.assert_not_called()
376+
finally:
377+
lock.release()
378+
await asyncio.wait_for(task, timeout=1)

0 commit comments

Comments
 (0)