Harden reuse-container mode against stale mounts, OOM, and leftovers

Persistent reuse containers froze their bind mounts and create-time flags,
so a wiped/re-cloned checkout poisoned every later job with a "container
breakout" exit 128, OOM'd containers were reused, and unused containers
accumulated and exhausted host memory. Add three guards:

- Create-flag fingerprint stamped as a label and re-checked before reuse:
  covers every flag frozen at container creation (volumes, tmpfs, network,
  devices, caps, resource limits, ...) plus each bind-mount source's host
  inode. Recreates the container on any create-flag mismatch (e.g. a tmpfs
  that is now a bind volume) or a wiped/re-created bind-mount source. Flags
  re-applied per `docker exec` (-t, -i, env, workdir, user) and labels are
  excluded. Replaces the per-job `docker exec --workdir true` probe.
- Out-of-band tainted marker: an in-container job (e.g. Cinder on OOM)
  touches /var/run/buildkite-docker-reuse/tainted to opt its container
  out of reuse on the next job, via a fresh per-container scratch mount.
- Per-spawn-slot cleanup that discards leftover/mismatched persistent
  containers at job start, for both reuse and non-reuse jobs, scoped via
  labels so it never touches another agent's container.

Also validate the reuse container name against Docker's naming rules to
prevent path traversal, document the behavior in the README, and extend
the bats suite.

Author: rsennewald-zoox

README.md

@@ -331,6 +331,22 @@ The container name is derived from the image name and the agent's spawn index. I
 
 Default: `false`
 
+#### Reuse safety and lifecycle
+
+A persistent container freezes its bind mounts and other create-time flags, so the plugin guards against reusing one that is no longer correct for the current job:
+
+- **Create-flag fingerprint.** At creation the container is labelled with a fingerprint of every flag frozen at container-creation time — volumes, `tmpfs`, `network`, devices, capabilities, resource limits, and so on — plus each bind-mount source's host inode. Flags that are re-applied on each `docker exec` (`-t`, `-i`, environment, `workdir`, `user`) and the per-job labels are excluded, since those can legitimately differ between jobs. Before reusing, the plugin recomputes the fingerprint and recreates the container if it differs. This catches two cases: a job whose create-time flags changed (for example a `tmpfs` mount that is now a bind `volume`, or a different `network`), and — importantly — a bind-mount source that was wiped and re-created on the host (for example a checkout that a failed `git clean` caused the agent to delete and re-clone). Without this, a reused container can run with the wrong mounts, and a stale bind mount makes every subsequent `docker exec` fail with `current working directory is outside of container mount namespace root ... possible container breakout detected` (exit 128). A container created before this feature (no fingerprint label) is treated as a mismatch and recreated once.
+
+- **Per-slot cleanup.** Persistent containers are labelled with `com.buildkite.docker-plugin.reuse=true` and the agent's spawn slot. At the start of every job (whether or not it uses `reuse-container`), the plugin discards this slot's persistent containers that the job does not need — a non-reuse job discards any leftover, and a reuse job discards any container other than the one it wants — so unused containers do not accumulate and exhaust host memory. Cleanup is scoped strictly to the agent's own spawn slot and is skipped when the agent name has no numeric `-%spawn` suffix (so it can never disturb another agent's container).
+
+- **Tainted marker.** A job running inside the container can opt the container out of reuse by creating the file `/var/run/buildkite-docker-reuse/tainted` (the plugin bind-mounts a fresh, world-writable host scratch dir there for each container). On the next job the plugin sees the marker and recreates the container instead of reusing it. This lets the job's own logic decide a container is no longer safe to reuse — for example a CI runner that detects an out-of-memory condition — without the plugin needing to interpret exit codes. Anything written into the `tainted` file is logged as the recreation reason.
+
+Notes and limitations:
+
+- Reuse safety applies on Unix agents; on Windows the fingerprint compares flags without bind-mount inode annotations.
+- The fingerprint covers create-time flags only (those frozen for the container's lifetime). Flags re-applied on each `docker exec` — TTY/interactive, environment, `workdir`, and `user` — are intentionally excluded, so changing them does not force a recreate.
+- The tainted-marker mechanism relies on the in-container job writing the marker; a job killed so abruptly that it cannot write the file will not mark the container as tainted (in which case the container is typically still healthy, and if its main process died it is recreated anyway).
+
 ### `reuse-container-name` (optional, string)
 
 Override the auto-generated container name used by the `reuse-container` feature. Use this when the default name derivation does not produce unique names (for example, when multiple agents on the same host do not use the standard `-%spawn` agent name suffix).

commands/run.sh

@@ -597,10 +597,53 @@ elif [[ ${#command[@]} -gt 0 ]] ; then
   done
 fi
 
+# Constants shared by the reuse-container path. The tainted dir is a fixed
+# contract between this plugin and the in-container job (e.g. Cinder), not a
+# user-facing option: a job touches "${reuse_tainted_target}/tainted" to opt its
+# container out of reuse. The host base is overridable only via an internal env
+# var so tests can redirect it away from the real /var/tmp.
+reuse_tainted_target="/var/run/buildkite-docker-reuse"
+reuse_tainted_base="${BUILDKITE_PLUGIN_DOCKER_REUSE_TAINTED_BASE:-/var/tmp/buildkite-docker-reuse}"
+
+# Discard persistent reuse containers from previous jobs that this job does not
+# need, scoped to this agent's spawn slot. Runs for both reuse and non-reuse
+# jobs (a non-reuse job should not leave a stale persistent container hogging
+# memory), but only when a numeric spawn slot is determinable -- otherwise we
+# cannot safely tell this agent's containers apart from another agent's.
+if ! is_windows ; then
+  reuse_spawn_slot="$(get_spawn_slot)"
+  if [[ -n "${reuse_spawn_slot}" ]]; then
+    keep_container=""
+    if [[ "${BUILDKITE_PLUGIN_DOCKER_REUSE_CONTAINER:-false}" =~ ^(true|on|1)$ ]]; then
+      keep_container="$(get_reuse_container_name "${image}")"
+    fi
+    cleanup_foreign_reuse_containers "${reuse_spawn_slot}" "${keep_container}"
+  fi
+fi
+
 if [[ "${BUILDKITE_PLUGIN_DOCKER_REUSE_CONTAINER:-false}" =~ ^(true|on|1)$ ]]; then
   # --- Reuse-container path ---
   container_name=$(get_reuse_container_name "${image}")
 
+  # Enforce Docker's container-name rules before deriving any host path from the
+  # name. This rejects path-traversal values (e.g. an explicit reuse-container-
+  # name of ".." or "a/b") that would otherwise make the "rm -rf" of the tainted
+  # scratch dir below operate outside its base.
+  if [[ ! "${container_name}" =~ ^[a-zA-Z0-9][a-zA-Z0-9_.-]*$ ]]; then
+    echo "+++ Error: Invalid reuse container name '${container_name}'." >&2
+    echo "    Must match Docker's naming rules: ^[a-zA-Z0-9][a-zA-Z0-9_.-]*$" >&2
+    exit 1
+  fi
+
+  host_tainted_dir="${reuse_tainted_base}/${container_name}"
+
+  # Fingerprint of this job's create-time flags (+ bind-mount source inodes).
+  # Used to detect, before reuse, both a flag mismatch (e.g. a changed tmpfs,
+  # volume, network, ...) and a wiped/re-created bind-mount source (the stale-
+  # mount breakout). Computed from run_flags, excluding per-exec flags, labels,
+  # and the internal tainted-marker mount.
+  current_fingerprint="$(compute_reuse_fingerprint "${reuse_tainted_target}" "${run_flags[@]}")"
+
   # Build exec_args by extracting only the flags that docker exec supports
   # from the run_flags snapshot (tty, interactive, env, workdir, user).
   exec_args=()
@@ -649,6 +692,31 @@ if [[ "${BUILDKITE_PLUGIN_DOCKER_REUSE_CONTAINER:-false}" =~ ^(true|on|1)$ ]]; t
     if [[ -n "${expected_image_id}" ]] && [[ "${container_image_id}" == "${expected_image_id}" ]]; then
       echo "--- :docker: Reusing existing container ${container_name} (${image})"
       need_create=false
+
+      # Compare the create-flag fingerprint stamped at creation against this
+      # job's. A difference means either a create-time flag changed (mismatch,
+      # e.g. a tmpfs/volume/network change) or a bind-mount source was wiped and
+      # re-created on the host (staleness, e.g. a re-cloned checkout). The latter
+      # is the breakout that fails every subsequent `docker exec --workdir` with
+      # "current working directory is outside of container mount namespace root"
+      # (exit 128). A missing/empty label (a container from before this feature)
+      # is treated as a mismatch.
+      stored_fingerprint="$(docker inspect --format '{{ index .Config.Labels "com.buildkite.docker-plugin.fingerprint" }}' "${container_name}" 2>/dev/null || true)"
+      if [[ "${stored_fingerprint}" != "${current_fingerprint}" ]]; then
+        echo "+++ :docker: Create-flag fingerprint changed for ${container_name}; recreating container"
+        echo "    (a create-time flag changed, or a bind-mount source was wiped and re-created on the host)"
+        docker rm -f "${container_name}" >/dev/null 2>&1 || true
+        need_create=true
+      # Honor an out-of-band tainted marker written by the in-container job (e.g.
+      # Cinder's error analysis on OOM) to opt this container out of reuse.
+      elif [[ -f "${host_tainted_dir}/tainted" ]]; then
+        echo "+++ :docker: Container ${container_name} is tainted; recreating container"
+        if [[ -s "${host_tainted_dir}/tainted" ]]; then
+          echo "    reason: $(tr -d '\n' < "${host_tainted_dir}/tainted")"
+        fi
+        docker rm -f "${container_name}" >/dev/null 2>&1 || true
+        need_create=true
+      fi
     else
       echo "+++ WARNING: Container image mismatch for ${container_name}"
       echo "    Expected image: ${image} (${expected_image_id:-unknown})"
@@ -680,6 +748,27 @@ if [[ "${BUILDKITE_PLUGIN_DOCKER_REUSE_CONTAINER:-false}" =~ ^(true|on|1)$ ]]; t
       i=$((i+1))
     done
 
+    # Discovery + fingerprint labels. The reuse/spawn-slot labels let later jobs
+    # find and scope-clean this slot's containers; the fingerprint records the
+    # create-time flags (+ inodes) this container was built with. Labels are
+    # excluded from the fingerprint, so adding them here does not perturb it.
+    create_flags+=("--label" "com.buildkite.docker-plugin.reuse=true")
+    if [[ -n "${reuse_spawn_slot:-}" ]]; then
+      create_flags+=("--label" "com.buildkite.docker-plugin.spawn-slot=${reuse_spawn_slot}")
+    fi
+    create_flags+=("--label" "com.buildkite.docker-plugin.fingerprint=${current_fingerprint}")
+
+    # Out-of-band tainted-marker channel: a fresh, per-container host scratch dir bind-
+    # mounted into the container. Reset on every creation so a new container
+    # never inherits a stale marker, and world-writable so the in-container job
+    # (which may run as a different uid) can create the marker file. Unix only.
+    if ! is_windows ; then
+      rm -rf "${host_tainted_dir}"
+      mkdir -p "${host_tainted_dir}"
+      chmod 0777 "${host_tainted_dir}"
+      create_flags+=("--volume" "${host_tainted_dir}:${reuse_tainted_target}")
+    fi
+
     echo "--- :docker: Creating persistent container ${container_name} (${image})"
     echo -ne '\033[90m$\033[0m docker run -d --name ' >&2
     echo -n "${container_name} " >&2

lib/shared.bash

@@ -76,6 +76,19 @@ function is_macos() {
   [[ "$OSTYPE" =~ ^(darwin) ]]
 }
 
+# Returns the agent's numeric spawn index (the trailing "-N" of
+# BUILDKITE_AGENT_NAME), or nothing if it cannot be determined. This is the
+# per-slot key that scopes reuse containers and their cleanup to one agent on a
+# shared host. Computed from BUILDKITE_AGENT_NAME directly so it stays correct
+# even when an explicit reuse-container-name is used.
+function get_spawn_slot() {
+  local agent_name="${BUILDKITE_AGENT_NAME:-}"
+  local spawn_suffix="${agent_name##*-}"
+  if [[ "${spawn_suffix}" =~ ^[0-9]+$ ]]; then
+    echo "${spawn_suffix}"
+  fi
+}
+
 # Returns a stable container name for the reuse-container feature.
 # Uses the explicit override if set, otherwise derives from the image name
 # and the agent's spawn index (to isolate containers per agent on a host).
@@ -90,18 +103,126 @@ function get_reuse_container_name() {
   local sanitized="${image//[^a-zA-Z0-9_.-]/-}"
   local name="${sanitized}"
 
-  local spawn_suffix="${BUILDKITE_AGENT_NAME##*-}"
-  if [[ "${spawn_suffix}" =~ ^[0-9]+$ ]]; then
+  local spawn_suffix
+  spawn_suffix="$(get_spawn_slot)"
+  if [[ -n "${spawn_suffix}" ]]; then
     name="${name}-${spawn_suffix}"
   else
-    echo "Warning: Could not extract numeric spawn index from BUILDKITE_AGENT_NAME '${BUILDKITE_AGENT_NAME}'." >&2
+    echo "Warning: Could not extract numeric spawn index from BUILDKITE_AGENT_NAME '${BUILDKITE_AGENT_NAME:-}'." >&2
     echo "  Multiple agents on the same host may share container name '${name}'." >&2
     echo "  Set 'reuse-container-name' to specify an explicit container name." >&2
   fi
 
   echo "${name}"
 }
 
+# Returns the inode number of a host path, portably across GNU and BSD/macOS
+# (avoids the GNU-only `stat -c %i`). Prints "missing" when the path does not
+# exist, which still yields a deterministic, comparable fingerprint entry.
+function get_host_inode() {
+  local path="$1"
+  if [[ -e "${path}" ]]; then
+    # `ls -d -i` prints "<inode> <path>" for the path itself (not contents).
+    # shellcheck disable=SC2012  # `find -printf` is GNU-only; ls keeps this
+    # portable to BSD/macOS, and we only read the leading inode field.
+    ls -di "${path}" 2>/dev/null | awk '{print $1}'
+  else
+    echo "missing"
+  fi
+}
+
+# Computes a deterministic fingerprint of the create-time flags a reuse
+# container is (re)created with, so a later job can detect when it would reuse a
+# container that was built with different flags (mismatch) or with a bind-mount
+# source that has since been wiped and re-created on the host (staleness, e.g. a
+# re-cloned checkout). This covers EVERY flag frozen at container creation
+# (volumes, tmpfs, network, devices, caps, resources, etc.), so changing any of
+# them forces a fresh container.
+#
+# Excluded (deliberately):
+#   - flags re-applied on each `docker exec` and therefore allowed to differ
+#     between jobs without a recreate: -t, -i, --env, --env-file, --workdir, -u;
+#   - all --label values (volatile per-job metadata and the plugin's own labels);
+#   - the plugin-internal tainted-marker --volume (identified by <tainted_target>).
+# For each remaining bind-mount --volume the host source inode is appended
+# (absolute source, Unix only) so a wiped/re-created source is detected.
+#
+# Flags are kept in their (deterministic) build order and joined with ",". The
+# raw string is returned (not hashed) to avoid depending on sha256sum vs shasum.
+#
+# Usage: compute_reuse_fingerprint <tainted_target> <flag>...
+function compute_reuse_fingerprint() {
+  local tainted_target="$1"; shift
+  local -a flags=("$@")
+  local -a entries=()
+  local i=0
+
+  while [[ $i -lt ${#flags[@]} ]]; do
+    local flag="${flags[$i]}"
+    case "${flag}" in
+      -t|-i)
+        # Per-exec flag (no value); not frozen.
+        ;;
+      --workdir|-u|--env|--env-file|--label)
+        # Per-exec or volatile; skip the flag and its value.
+        i=$((i+1))
+        ;;
+      --volume)
+        local spec="${flags[$((i+1))]}"
+        i=$((i+1))
+        local rest="${spec#*:}"
+        local dst="${rest%%:*}"
+        if [[ -n "${tainted_target}" && "${dst}" == "${tainted_target}" ]]; then
+          : # plugin-internal tainted-marker mount; not part of user config
+        else
+          local src="${spec%%:*}"
+          if [[ "${src}" == /* ]] && ! is_windows; then
+            entries+=("--volume=${spec}#$(get_host_inode "${src}")")
+          else
+            entries+=("--volume=${spec}")
+          fi
+        fi
+        ;;
+      *)
+        # Any other create-time flag (or a value token of one): keep verbatim,
+        # in order, so it contributes to the fingerprint.
+        entries+=("${flag}")
+        ;;
+    esac
+    i=$((i+1))
+  done
+
+  if [[ ${#entries[@]} -gt 0 ]]; then
+    local IFS=','
+    echo "${entries[*]}"
+  fi
+}
+
+# Removes persistent reuse containers belonging to this agent's spawn slot that
+# this job does not need, freeing their memory. Scoped per slot via labels so it
+# never touches another agent's container on a shared host.
+#   <slot>  the agent spawn slot (from get_spawn_slot).
+#   <keep>  the one container name to preserve (the desired reuse container for
+#           this job); pass "" for non-reuse jobs to discard all slot containers.
+function cleanup_foreign_reuse_containers() {
+  local slot="$1" keep="$2"
+  local names
+  names="$(docker ps -a \
+    --filter "label=com.buildkite.docker-plugin.reuse=true" \
+    --filter "label=com.buildkite.docker-plugin.spawn-slot=${slot}" \
+    --format '{{.Names}}' 2>/dev/null || true)"
+
+  [[ -z "${names}" ]] && return 0
+
+  local name
+  while IFS= read -r name; do
+    [[ -z "${name}" ]] && continue
+    [[ -n "${keep}" && "${name}" == "${keep}" ]] && continue
+    echo "--- :docker: Discarding persistent container ${name} (not needed by this job on slot ${slot})"
+    docker rm -f "${name}" >/dev/null 2>&1 || true
+  done <<< "${names}"
+}
+
 # Returns the image ID (digest) of the image a container was created from.
 function get_container_image_id() {
   local container_name="$1"