diff --git a/controller/Dockerfile b/controller/Dockerfile index 49252ebe..7c6f7d7b 100644 --- a/controller/Dockerfile +++ b/controller/Dockerfile @@ -20,7 +20,7 @@ ARG CI_FLEET_COMMIT=unknown RUN CGO_ENABLED=0 go build -trimpath -ldflags="-s -w -X main.version=${CI_FLEET_VERSION} -X main.commitSHA=${CI_FLEET_COMMIT}" -o /out/ci-fleet-controller . \ && touch -d "@${SOURCE_DATE_EPOCH}" /out/ci-fleet-controller -FROM debian:13.7-slim@sha256:a29215f6a35e51e22adffa17f89e9d2ef06214e64a2bad10d765c46aea49f11f +FROM debian:13.7-slim@sha256:62bd423aeee912639ca8ee681c2b762cb3e25137e0a92b20be455898a9f034ad ARG SOURCE_DATE_EPOCH=1786752000 ARG DEBIAN_SNAPSHOT=20261012T000000Z ARG CI_FLEET_COMMIT=unknown diff --git a/controller/lifecycle_test.go b/controller/lifecycle_test.go index cd935f42..d0bbca21 100644 --- a/controller/lifecycle_test.go +++ b/controller/lifecycle_test.go @@ -12,6 +12,7 @@ import ( "sync" "sync/atomic" "testing" + "time" "github.com/actions/scaleset" "github.com/actions/scaleset/listener" @@ -200,3 +201,85 @@ func TestUntrackedRunnerEventsRetainUnrelatedAndDockerErrors(t *testing.T) { }) } } + +func TestExitWatcherDoesNotRemoveAgainWhileCompletionResponseIsPending(t *testing.T) { + const name, id = "ci-fleet-example-0123abcd", "runner-id" + waiting, exited, release := make(chan struct{}), make(chan struct{}), make(chan struct{}) + var waitOnce, exitOnce, releaseOnce sync.Once + var removals atomic.Int32 + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + path := strings.TrimPrefix(r.URL.Path, "/v1.48") + switch { + case path == "/containers/"+id+"/wait": + waitOnce.Do(func() { close(waiting) }) + select { + case <-exited: + json.NewEncoder(w).Encode(map[string]int{"StatusCode": 0}) + case <-r.Context().Done(): + } + case path == "/containers/"+id+"/json": + w.WriteHeader(http.StatusNotFound) + json.NewEncoder(w).Encode(map[string]string{"message": "No such container"}) + case path == "/containers/"+id+"/logs": + w.WriteHeader(http.StatusOK) + case path == "/containers/"+id && r.Method == http.MethodDelete: + if removals.Add(1) == 1 { + exitOnce.Do(func() { close(exited) }) + // Docker is already absent, but completion still awaits its response. + <-release + } + w.WriteHeader(http.StatusNoContent) + default: + t.Errorf("unexpected Docker request: %s %s", r.Method, r.URL) + w.WriteHeader(http.StatusInternalServerError) + } + })) + defer server.Close() + docker, err := dockerclient.NewClientWithOpts(dockerclient.WithHost(server.URL), dockerclient.WithHTTPClient(server.Client()), dockerclient.WithVersion("1.48")) + if err != nil { + t.Fatal(err) + } + defer docker.Close() + scaler := &Scaler{runners: newRunnerState(), dockerClient: docker, logger: slog.New(slog.DiscardHandler), + config: Config{FleetInstance: "example", MaxRunners: 1, StatusFile: t.TempDir() + "/status.json"}} + scaler.runners.addBusy(name, id) + ctx, cancel := context.WithCancel(context.Background()) + defer func() { + releaseOnce.Do(func() { close(release) }) + exitOnce.Do(func() { close(exited) }) + scaler.runners.forget(name, id) + cancel() + }() + watchDone := make(chan struct{}) + go func() { scaler.watchRunner(ctx, name, id); close(watchDone) }() + select { + case <-waiting: + case <-time.After(2 * time.Second): + t.Fatal("exit watcher did not start") + } + completion := make(chan error, 1) + go func() { + completion <- scaler.HandleJobCompleted(context.Background(), &scaleset.JobCompleted{RunnerName: name}) + }() + select { + case <-watchDone: + case <-time.After(2 * time.Second): + t.Fatal("exit watcher did not accept confirmed container absence") + } + if !scaler.runners.contains(name, id) { + t.Fatal("tracking was discarded while completion was pending") + } + releaseOnce.Do(func() { close(release) }) + select { + case err := <-completion: + if err != nil { + t.Fatal(err) + } + case <-time.After(2 * time.Second): + t.Fatal("completion did not finish") + } + if removals.Load() != 1 || scaler.runners.count() != 0 || scaler.runners.contains(name, id) { + t.Fatalf("duplicate removal or retained tracking: removals=%d runners=%d", removals.Load(), scaler.runners.count()) + } +} diff --git a/controller/scaler.go b/controller/scaler.go index 555f123a..b98f072e 100644 --- a/controller/scaler.go +++ b/controller/scaler.go @@ -219,6 +219,7 @@ func (s *Scaler) watchRunner(ctx context.Context, name, id string) { } // Docker can wake a not-running waiter while the container is restarting. current, err := s.dockerClient.ContainerInspect(ctx, id) + // A not-found inspection already proves this exact container is retired. if !errdefs.IsNotFound(err) { if err != nil || current.State == nil { s.logger.Warn("inspect exited runner", "runner", name, "error", err) @@ -228,11 +229,11 @@ func (s *Scaler) watchRunner(ctx context.Context, name, id string) { if current.State.Running || current.State.Restarting || current.State.Paused { continue } - } - if err := s.logAndRemove(ctx, name, id, false); err != nil { - s.logger.Warn("retry exited runner cleanup", "runner", name, "error", err) - time.Sleep(time.Minute) - continue + if err := s.logAndRemove(ctx, name, id, false); err != nil { + s.logger.Warn("retry exited runner cleanup", "runner", name, "error", err) + time.Sleep(time.Minute) + continue + } } if !s.runners.markExited(name, id) { return diff --git a/docs/CONTROLLER-DESIGN.md b/docs/CONTROLLER-DESIGN.md index f7607cfa..32e29edd 100644 --- a/docs/CONTROLLER-DESIGN.md +++ b/docs/CONTROLLER-DESIGN.md @@ -63,6 +63,12 @@ runner still require the instance-specific name check and Docker's confirmation that the container is absent. Contradictory events and uncertain Docker results remain failures; restarting alone does not fix their cause. +When Docker confirms that the exact tracked container ID is absent, the exit +watcher records its exit without issuing another removal. This avoids a duplicate +delete while a completion removal's response is still pending. Tracking remains +available until completion finishes; inspection and removal errors retain their +existing retry behavior. + A batch canceled before admission performs no work. Once admitted, a real batch finishes acquisition, callbacks, cleanup and reconciliation without inheriting caller cancellation, preserving the previous listener's cleanup behavior. The diff --git a/docs/MANAGED-IMAGES.md b/docs/MANAGED-IMAGES.md index 970c299e..c0c9fed3 100644 --- a/docs/MANAGED-IMAGES.md +++ b/docs/MANAGED-IMAGES.md @@ -156,7 +156,8 @@ Compose selections are unchanged. | Input | Immutable selection | Evidence | | --- | --- | --- | | Runner Debian 13.7 slim, reviewed 2026-10-30 | `sha256:62bd423aeee912639ca8ee681c2b762cb3e25137e0a92b20be455898a9f034ad` | Docker Hub manifest bytes match the digest and include Linux amd64 and arm64; base configs were created 2026-10-20 | -| Controller Debian 13.7 slim, reviewed 2026-10-12 | `sha256:a29215f6a35e51e22adffa17f89e9d2ef06214e64a2bad10d765c46aea49f11f` | Docker Hub registry manifest for `library/debian:13.7-slim`; also the previous runner selection | +| Controller Debian 13.7 slim, reviewed 2026-10-30 | `sha256:62bd423aeee912639ca8ee681c2b762cb3e25137e0a92b20be455898a9f034ad` | Docker Hub manifest bytes match the digest and include Linux amd64 and arm64; base configs were created 2026-10-20 | +| Debian 13.7 slim, reviewed 2026-10-12 for both images | `sha256:a29215f6a35e51e22adffa17f89e9d2ef06214e64a2bad10d765c46aea49f11f` | Docker Hub registry manifest for `library/debian:13.7-slim`; retained historical review | | Go 1.27.3 Bookworm build image | `sha256:972b4110ba0f47539a975c31e52c64765e1ff19f2d74469ae4a595c6676e80a5` | Docker Hub registry manifest for `library/golang:1.27.3-bookworm`; [upstream patch notes](https://go.dev/doc/devel/release#go1.27.3) | | Debian packages | signed snapshot `20261012T000000Z` | `snapshot.debian.org/archive/debian/20261012T000000Z/dists/trixie/InRelease` | | Build timestamps | `SOURCE_DATE_EPOCH=1786752000` | BuildKit reproducible-build timestamp input declared in every stage |