Skip to content

fix(container): scope shutdown and boot reconciliation to the owning worker - #70

Merged
mariuspruvot merged 1 commit into
mainfrom
fix/worker-scoped-cleanup
Aug 1, 2026
Merged

fix(container): scope shutdown and boot reconciliation to the owning worker#70
mariuspruvot merged 1 commit into
mainfrom
fix/worker-scoped-cleanup

Conversation

@mariuspruvot

Copy link
Copy Markdown
Owner

The most serious item left from the 2026-08-01 audit: with --workers 4 in production, one worker restarting destroyed its peers' live sessions.

The bug

Both lifecycle hooks operated on every unfinished session, while all four workers share one Docker socket:

  • cleanup_all_running (shutdown) → stopped every running container on the host and marked those sessions CANCELLED
  • reconcile_stale_sessions (boot) → marked every still-RUNNING row FAILED

So a single worker restarting cancelled three other workers' sessions mid-stream, and its boot hook then failed whatever survived. Unlike the webhook reaper, which claims rows atomically, these two were not idempotent — they were destructive to peers.

Neither had a single test. That is how it survived a full audit round: grep for either name in tests/ returned nothing.

The fix

Shutdown is scoped by ownership. Every runner now carries a helprs.boot_id label identifying the process that started it, and cleanup_own_running asks Docker which containers carry this process's label. Docker is the authority on what is actually running; the database only records what was intended.

Boot reconciliation stops guessing. reconcile_stale_sessions asks container_is_running per session rather than treating "unfinished" as evidence of death. At boot, most RUNNING rows belong to a peer that never stopped and is still streaming to a user. A row with no container_id never got that far, so nothing is running for it either way — those are still failed.

This is the better signal regardless of worker count: it answers "is this session actually alive?" instead of a proxy for it.

One subtlety worth reviewing

current_boot_id() recomputes when the PID changes instead of caching once:

if _boot_id is None or _boot_id[0] != pid:
    _boot_id = (pid, f"{pid}-{uuid4().hex[:8]}")

uvicorn's --workers mode may fork after import, and a plain module-level constant or lru_cache would then be inherited by every child — two workers sharing a boot id would each stop the other's containers, reintroducing the exact bug through the mechanism meant to fix it. The PID check makes it correct under both fork and spawn, with no plumbing through the lifespan.

Protocol changes

DockerClient gains container_is_running and list_runners, plus a frozen RunnerContainer dataclass so the boundary returns a typed pair rather than a dict. AioDockerClient.list_runners filters server-side on the label and skips any container whose session label is missing or unparseable — guessing which row it belongs to could cancel the wrong session.

Verification

  • 419 tests pass (was 411), ruff + ruff format + mypy clean
  • Eight new tests in tests/modules/container/test_cleanup.py, covering the multi-worker case explicitly: a peer's session is left alone on shutdown, a live peer session is not failed at boot, the live and the dead are separated, an already-finished session is not re-cancelled, an orphan container with no row is skipped
  • All eight were run against the previous implementation first — four of them fail on it, which is the check that the coverage is real rather than decorative

…worker

Production runs `--workers 4` against one shared Docker socket, and both
lifecycle hooks operated on "every unfinished session". So whenever any one
worker restarted or crashed, its shutdown hook CANCELLED every live session
on the host and its boot hook marked the survivors FAILED. Unlike the
reaper, these were not idempotent -- they were actively destructive to
peers. Neither function had a single test, which is how it survived.

Shutdown is now scoped by ownership. Every runner carries a `helprs.boot_id`
label identifying the process that started it, and `cleanup_own_running`
asks Docker which containers carry this process's label. Docker is the
authority on what is actually running; the database only records what was
intended.

Boot reconciliation stops guessing. `reconcile_stale_sessions` asks
`container_is_running` per session instead of treating "unfinished" as
evidence of death -- at boot, most RUNNING rows belong to a peer that never
stopped and is still streaming to a user.

`current_boot_id` recomputes when the PID changes rather than caching once,
because uvicorn's --workers mode may fork after import and two workers
sharing a boot id would each stop the other's containers -- reintroducing
the bug through the mechanism meant to fix it.

Adds `container_is_running` and `list_runners` to the DockerClient protocol
and its doubles, plus `RunnerContainer` so the boundary returns a typed pair
rather than a dict.

Eight new tests, run against the previous behaviour first: four of them fail
on it.
@helprs-prod

helprs-prod Bot commented Aug 1, 2026

Copy link
Copy Markdown

helPRs session created for this PR.

Skill: challenge-me | Open session

@mariuspruvot
mariuspruvot merged commit 1539806 into main Aug 1, 2026
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant