A user container was created with no restart policy, so anything that stops the daemon stopped it for good — an `apt upgrade` pulling a new docker-ce SIGTERMs every container (exit 143) and only those carrying a policy come back. Skald's own process survives that, and `ensure()` runs only at boot, at login and off the lifecycle bus, so nothing noticed: every `docker exec` path then failed identically until someone logged in again. The per-user MCP servers respawn-looped on `container ... is not running`, and a connector's dependency install failed with the same line. Create with `--restart unless-stopped`, and reconcile an existing container's policy in place with `docker update`. `unless-stopped` rather than `always` because `stop_all()` stops these deliberately at shutdown: the flag Docker sets there is exactly the one this policy honours, so a daemon restart while Skald is down leaves them alone and the next boot's `ensure` starts them. The in-place reconcile is deliberately not a sixth `reusable()` axis. The policy is the one property Docker can change on a live container, so making it a recreate would throw away a running container — and every `docker exec` under it — to set a flag.
This commit is contained in:
@@ -76,6 +76,23 @@ pub const SKILLS_USERS_DIR: &str = "skills-users";
|
||||
pub const SKILLS_ROOT_DIR: &str = ".skills-root";
|
||||
/// Home mount point inside the container.
|
||||
pub const CONTAINER_HOME: &str = "/root";
|
||||
|
||||
/// Docker restart policy for a user's container.
|
||||
///
|
||||
/// Without one, a container created here is `restart=no`, so **anything that stops
|
||||
/// the daemon stops it for good**: `apt upgrade` pulling a new `docker-ce` SIGTERMs
|
||||
/// every container (exit 143) and only those with a policy come back. Skald's own
|
||||
/// process survives that — it needs no daemon to stay alive — and [`ensure`] runs
|
||||
/// only at boot, at login, and off the lifecycle bus, so nothing notices. What the
|
||||
/// user sees is every `docker exec` path failing identically until someone logs in
|
||||
/// again: the per-user MCP servers respawn-loop on `container … is not running`, and
|
||||
/// a connector's dependency install fails with the same line.
|
||||
///
|
||||
/// `unless-stopped`, not `always`, because [`ContainerManager::stop_all`] stops these
|
||||
/// deliberately at shutdown — the flag Docker sets there is exactly the one this
|
||||
/// policy honours, so a daemon restart while Skald is down leaves them alone and the
|
||||
/// next boot's `ensure` starts them. A later `docker start` clears it again.
|
||||
const RESTART_POLICY: &str = "unless-stopped";
|
||||
/// Grace window `docker stop` gives in-container processes (SIGTERM → SIGKILL)
|
||||
/// before force-killing — enough for a shell or MCP `docker exec` child to exit.
|
||||
const STOP_GRACE: Duration = Duration::from_secs(10);
|
||||
@@ -438,8 +455,9 @@ impl ContainerManager {
|
||||
/// mounts + `--user`, and starts it (if stopped). Self-healing: a container whose
|
||||
/// `--user` no longer matches the host uid:gid (e.g. an old root container from a
|
||||
/// previous binary), that predates `--init`, or that runs a superseded
|
||||
/// [`IMAGE_TAG`], is torn down and recreated. Idempotent — a no-op when a matching
|
||||
/// container is already running.
|
||||
/// [`IMAGE_TAG`], is torn down and recreated; a reused one additionally has its
|
||||
/// [`RESTART_POLICY`] reconciled in place, which is the one property that needs no
|
||||
/// recreate. Idempotent — a no-op when a matching container is already running.
|
||||
pub async fn ensure(&self, user_id: &str) -> Result<()> {
|
||||
let fs = build_user_fs(&self.system, user_id).await?;
|
||||
let wd = std::env::current_dir().context("failed to read working directory")?;
|
||||
@@ -464,8 +482,12 @@ impl ContainerManager {
|
||||
match container_state(name).await {
|
||||
// Reuse only if it runs as the expected user AND has tini as PID 1;
|
||||
// otherwise recreate below.
|
||||
ContainerState::Running if reusable(name, &want_user, &fs).await => return Ok(()),
|
||||
ContainerState::Running if reusable(name, &want_user, &fs).await => {
|
||||
ensure_restart_policy(name).await;
|
||||
return Ok(());
|
||||
}
|
||||
ContainerState::Stopped if reusable(name, &want_user, &fs).await => {
|
||||
ensure_restart_policy(name).await;
|
||||
docker(&["start", name]).await.context("docker start failed")?;
|
||||
return Ok(());
|
||||
}
|
||||
@@ -487,6 +509,9 @@ impl ContainerManager {
|
||||
// otherwise `execute_cmd`'s /stop reaper (and any command that leaves
|
||||
// orphans) would accumulate zombies under the idle `sleep infinity`.
|
||||
"--init".into(),
|
||||
// Survive a daemon restart (see `RESTART_POLICY`).
|
||||
"--restart".into(),
|
||||
RESTART_POLICY.into(),
|
||||
"--name".into(),
|
||||
name.clone(),
|
||||
"--workdir".into(),
|
||||
@@ -708,6 +733,41 @@ async fn reusable(name: &str, want_user: &Option<String>, fs: &UserFs) -> bool {
|
||||
&& skills_mounted(name, fs).await
|
||||
}
|
||||
|
||||
/// Brings an existing container's restart policy up to [`RESTART_POLICY`], in place.
|
||||
///
|
||||
/// Deliberately **not** a [`reusable`] axis: the policy is the one property Docker can
|
||||
/// change on a live container (`docker update`), so making it a recreate would throw
|
||||
/// away a running container — and every `docker exec` under it — to set a flag. Every
|
||||
/// other axis there is fixed at create time and has no such door.
|
||||
///
|
||||
/// Reads before writing so the common case (already correct) is one inspect and no
|
||||
/// mutation, and so nothing is logged on the boot pass of an already-reconciled box.
|
||||
/// Best-effort throughout: an unreadable inspect is treated as correct, because the
|
||||
/// only cost of skipping is the behaviour we had before this existed, while churning a
|
||||
/// working container on a docker hiccup is a real one.
|
||||
async fn ensure_restart_policy(name: &str) {
|
||||
let Ok(current) = docker(&["inspect", "-f", "{{.HostConfig.RestartPolicy.Name}}", name]).await
|
||||
else {
|
||||
return;
|
||||
};
|
||||
if current.trim() == RESTART_POLICY {
|
||||
return;
|
||||
}
|
||||
match docker(&["update", "--restart", RESTART_POLICY, name]).await {
|
||||
Ok(_) => tracing::info!(
|
||||
container = %name,
|
||||
from = %current.trim(),
|
||||
to = %RESTART_POLICY,
|
||||
"container restart policy updated"
|
||||
),
|
||||
Err(e) => tracing::warn!(
|
||||
container = %name,
|
||||
error = %e,
|
||||
"could not set the container restart policy — it will not survive a docker daemon restart"
|
||||
),
|
||||
}
|
||||
}
|
||||
|
||||
/// Gives the container's runtime `uid`/`gid` a passwd + shadow (+ group) entry, so
|
||||
/// tools that resolve the invoking user work despite the arbitrary numeric uid — and
|
||||
/// so `sudo` succeeds (without a shadow entry PAM's account phase fails with "account
|
||||
|
||||
Reference in New Issue
Block a user