From d7978e120148773cc94ada72b696d913ed03a9a2 Mon Sep 17 00:00:00 2001 From: Antoine Toussaint Date: Fri, 11 Sep 2026 22:00:41 -0400 Subject: [PATCH 1/6] fix: recover ephemeral containers across run scopes without crossing homes Persist canonical home/workspace ownership on each container alongside its exact run scope. Recover only explicitly ephemeral containers with dead owners across invocation scopes; retain stateful, ledger-owned and foreign containers and never remove volumes. Acknowledge the inherited identity over authenticated agent gRPC and retain the startup parent across reparenting, so callers can reject older agents before provisioning and a parent crash cannot erase labels. Based on the CLI's original Core pin plus scoped-recovery commits to avoid introducing the unrelated Buildx capability rollout. --- agents/agents.go | 1 + agents/container_recovery.go | 23 +++++ agents/container_recovery_test.go | 60 +++++++++++++ agents/testdata/recoveryagent/main.go | 18 ++++ docs/session-ledger.md | 21 ++++- runners/dockerrun/container_recovery_scope.go | 68 +++++++++++--- .../container_recovery_scope_test.go | 28 +++--- runners/dockerrun/disposable_recovery.go | 58 ++++++++++++ .../disposable_recovery_integration_test.go | 89 +++++++++++++++++++ runners/dockerrun/disposable_recovery_test.go | 57 ++++++++++++ runners/dockerrun/docker_runner.go | 5 ++ services/service.go | 12 ++- 12 files changed, 413 insertions(+), 27 deletions(-) create mode 100644 agents/container_recovery.go create mode 100644 agents/container_recovery_test.go create mode 100644 agents/testdata/recoveryagent/main.go create mode 100644 runners/dockerrun/disposable_recovery.go create mode 100644 runners/dockerrun/disposable_recovery_integration_test.go create mode 100644 runners/dockerrun/disposable_recovery_test.go diff --git a/agents/agents.go b/agents/agents.go index 04211dc18..1172d44ac 100644 --- a/agents/agents.go +++ b/agents/agents.go @@ -442,6 +442,7 @@ func Serve(reg PluginRegistration) { // insecure default. authUnaryInterceptor(expectedToken), principalUnaryInterceptor(), + containerRecoveryInterceptor(), runtimeLoadTracker(&runtimeLoaded), agentRPCInterceptor(), ), diff --git a/agents/container_recovery.go b/agents/container_recovery.go new file mode 100644 index 000000000..20c04096e --- /dev/null +++ b/agents/container_recovery.go @@ -0,0 +1,23 @@ +package agents + +import ( + "context" + + agentv0 "github.com/codefly-dev/core/generated/go/codefly/services/agent/v0" + "github.com/codefly-dev/core/runners/dockerrun" + "google.golang.org/grpc" + "google.golang.org/grpc/metadata" +) + +func containerRecoveryInterceptor() grpc.UnaryServerInterceptor { + return func(ctx context.Context, req any, info *grpc.UnaryServerInfo, handler grpc.UnaryHandler) (any, error) { + if info.FullMethod == agentv0.Agent_GetAgentInformation_FullMethodName { + if scope := dockerrun.InheritedContainerRecoveryScope(); scope != "" { + if err := grpc.SetHeader(ctx, metadata.Pairs(dockerrun.ContainerRecoveryScopeHeader, scope)); err != nil { + return nil, err + } + } + } + return handler(ctx, req) + } +} diff --git a/agents/container_recovery_test.go b/agents/container_recovery_test.go new file mode 100644 index 000000000..dffd522df --- /dev/null +++ b/agents/container_recovery_test.go @@ -0,0 +1,60 @@ +package agents + +import ( + "bytes" + "context" + "fmt" + "os" + "os/exec" + "path/filepath" + "testing" + "time" + + agentv0 "github.com/codefly-dev/core/generated/go/codefly/services/agent/v0" + "github.com/codefly-dev/core/runners/dockerrun" + "github.com/stretchr/testify/require" + "google.golang.org/grpc" + "google.golang.org/grpc/credentials/insecure" + "google.golang.org/grpc/metadata" +) + +func TestAgentAcknowledgesInheritedContainerRecoveryOverGRPC(t *testing.T) { + binary := filepath.Join(t.TempDir(), "recoveryagent") + output, err := exec.Command("go", "build", "-o", binary, "./testdata/recoveryagent").CombinedOutput() + require.NoError(t, err, "%s", output) + t.Setenv(dockerrun.ContainerRecoveryScopeEnvironment, "") + scope, err := dockerrun.NewContainerRecoveryScope(t.TempDir(), t.TempDir(), "test") + require.NoError(t, err) + require.NoError(t, dockerrun.SetContainerRecoveryScope(scope)) + for _, valid := range []bool{true, false} { + t.Run(fmt.Sprint(valid), func(t *testing.T) { + marker := os.Getenv(dockerrun.ContainerRecoveryScopeEnvironment) + if !valid { + marker = "0:invalid" + } + command := exec.Command(binary) + command.Env = append(os.Environ(), "CODEFLY_AGENT_TOKEN=recovery-test", "CODEFLY_AGENT_UDS_PATH=", dockerrun.ContainerRecoveryScopeEnvironment+"="+marker) + stdout, err := command.StdoutPipe() + require.NoError(t, err) + var stderr bytes.Buffer + command.Stderr = &stderr + require.NoError(t, command.Start()) + t.Cleanup(func() { _ = command.Process.Kill(); _ = command.Wait() }) + endpoint := readHandshakeEndpoint(t, stdout, &stderr) + conn, err := grpc.NewClient(endpoint, grpc.WithTransportCredentials(insecure.NewCredentials())) + require.NoError(t, err) + t.Cleanup(func() { _ = conn.Close() }) + ctx, cancel := context.WithTimeout(t.Context(), 10*time.Second) + defer cancel() + ctx = metadata.AppendToOutgoingContext(ctx, AuthMetadataKey, "recovery-test") + var headers metadata.MD + _, err = agentv0.NewAgentClient(conn).GetAgentInformation(ctx, &agentv0.AgentInformationRequest{}, grpc.Header(&headers)) + require.NoError(t, err) + if valid { + require.Equal(t, []string{dockerrun.InheritedContainerRecoveryScope()}, headers.Get(dockerrun.ContainerRecoveryScopeHeader)) + } else { + require.Empty(t, headers.Get(dockerrun.ContainerRecoveryScopeHeader)) + } + }) + } +} diff --git a/agents/testdata/recoveryagent/main.go b/agents/testdata/recoveryagent/main.go new file mode 100644 index 000000000..aba09d2be --- /dev/null +++ b/agents/testdata/recoveryagent/main.go @@ -0,0 +1,18 @@ +package main + +import ( + "context" + + "github.com/codefly-dev/core/agents" + agentv0 "github.com/codefly-dev/core/generated/go/codefly/services/agent/v0" +) + +type server struct { + agentv0.UnimplementedAgentServer +} + +func (server) GetAgentInformation(context.Context, *agentv0.AgentInformationRequest) (*agentv0.AgentInformation, error) { + return &agentv0.AgentInformation{}, nil +} + +func main() { agents.Serve(agents.PluginRegistration{Agent: server{}}) } diff --git a/docs/session-ledger.md b/docs/session-ledger.md index 344502e45..2ec1e4727 100644 --- a/docs/session-ledger.md +++ b/docs/session-ledger.md @@ -295,8 +295,8 @@ a silent forward-compatibility trick. The legacy PID-based startup sweep requires a `codefly.recovery-scope` label before checking owner liveness. The label hashes canonical Codefly home, -workspace path and resolved naming scope. A different home, workspace or scope -cannot claim a container, even when its creator PID is gone. Containers without +workspace path and resolved naming scope. This exact-scope sweep cannot claim +a different home, workspace or scope, even when its creator PID is gone. Containers without this label, or with missing/malformed owner PIDs, require explicit owner recovery; startup does not infer ownership from container names. @@ -325,6 +325,23 @@ environment retains its scope from first use so another flow changing the proces marker cannot redirect its cleanup. Recovery groups authorize orphan sweeping only; they never authorize adopting another invocation's container. +The agent's `GetAgentInformation` response includes the +`codefly-container-recovery-scope` gRPC header with the validated inherited scope. +The CLI must require this acknowledgement before Docker provisioning. The +startup parent identity is retained after reparenting so a CLI crash cannot make +an already-spawned agent create unlabeled containers. + +Containers also carry `codefly.recovery-namespace`, a hash of the canonical +home and workspace without the invocation's naming scope. This durable identity +lets `ReapDisposableContainers` recover explicitly ephemeral containers after +an SDK/test invocation dies, even when the next invocation has a fresh scope. +Unlike a recovery group, it needs no session metadata and survives a successor +that picked an entirely unrelated naming scope. It checks the original agent PID +and preserves live owners, stateful containers, and session-ledger containers. +A different home/workspace or a container without both ownership labels is never +eligible. No on-disk registry or inference from names is needed; failed removals +leave the labels available for retry. + Within the same scope, live owners and running stateful containers are retained. Stopped containers and running ephemeral containers with dead owners can be removed, without requesting volume removal. Containers carrying an invocation diff --git a/runners/dockerrun/container_recovery_scope.go b/runners/dockerrun/container_recovery_scope.go index 69c4d6b5d..930278208 100644 --- a/runners/dockerrun/container_recovery_scope.go +++ b/runners/dockerrun/container_recovery_scope.go @@ -16,15 +16,35 @@ import ( const LabelCodeflyRecoveryScope = "codefly.recovery-scope" const LabelCodeflyRecoveryGroup = "codefly.recovery-group" +const LabelCodeflyRecoveryNamespace = "codefly.recovery-namespace" const ContainerRecoveryScopeEnvironment = "CODEFLY_CONTAINER_RECOVERY_SCOPE" +// ContainerRecoveryScopeHeader acknowledges the ownership identity inherited +// by an agent. Older agents omit it and cannot promise scoped recovery. +const ContainerRecoveryScopeHeader = "codefly-container-recovery-scope" + // Capture the launching parent before serving any requests. A parent dying // during a request must not revoke the child's already inherited ownership. // If it died before initialization, validation fails and creation is refused. var containerRecoveryParentPID = os.Getppid() -// ContainerRecoveryScope binds cleanup to a home, workspace and resolved naming scope. -type ContainerRecoveryScope struct{ id, group string } +// InheritedContainerRecoveryScope is the validated identity used by container +// creation and by the agent's read-only gRPC ownership acknowledgement. It is +// empty unless both the exact scope and the durable namespace are inherited: +// an older CLI that projects only a scope cannot promise cross-scope recovery. +func InheritedContainerRecoveryScope() string { + scope, err := inheritedContainerRecoveryScope() + if err != nil || scope.id == "" || scope.namespace == "" { + return "" + } + return scope.id + ":" + scope.namespace +} + +// ContainerRecoveryScope binds cleanup to a home, workspace and resolved naming +// scope. group additionally covers the disposable siblings of one SDK +// invocation, and namespace is the durable canonical home/workspace identity +// that survives a run choosing an entirely fresh naming scope. +type ContainerRecoveryScope struct{ id, group, namespace string } func NewContainerRecoveryScope(home, workspace, namingScope string) (ContainerRecoveryScope, error) { paths := []string{home, workspace} @@ -41,7 +61,10 @@ func NewContainerRecoveryScope(home, workspace, namingScope string) (ContainerRe return ContainerRecoveryScope{}, err } } - scope := ContainerRecoveryScope{id: recoveryScopeHash(paths, namingScope)} + scope := ContainerRecoveryScope{ + id: recoveryScopeHash(paths, namingScope), + namespace: recoveryNamespaceHash(paths), + } // Only an SDK invocation in disposable mode can delegate recovery across // invocation names. Never infer this from a name suffix alone: a regular // developer scope may happen to have the same spelling. @@ -68,13 +91,24 @@ func recoveryScopeHash(paths []string, namingScope string) string { return hex.EncodeToString(sum[:]) } +// recoveryNamespaceHash covers every naming scope under one canonical +// home/workspace pair, so an aborted run's disposable containers stay +// recoverable by a successor that picked a completely different scope. +func recoveryNamespaceHash(paths []string) string { + data, _ := json.Marshal(paths) + sum := sha256.Sum256(data) + return hex.EncodeToString(sum[:]) +} + // SetContainerRecoveryScope projects ownership to directly spawned agents. // Older agents omit the label and are deliberately ineligible for startup cleanup. func SetContainerRecoveryScope(scope ContainerRecoveryScope) error { - if scope.id == "" { + if scope.id == "" || scope.namespace == "" { return fmt.Errorf("container recovery scope is unresolved") } - marker := strconv.Itoa(os.Getpid()) + ":" + scope.id + // pid:scope:namespace[:group] — group is optional, so it trails the + // namespace every resolved scope carries. + marker := strconv.Itoa(os.Getpid()) + ":" + scope.id + ":" + scope.namespace if scope.group != "" { marker += ":" + scope.group } @@ -94,16 +128,24 @@ func inheritedContainerRecoveryScope() (ContainerRecoveryScope, error) { if err != nil || pid <= 1 || (pid != os.Getpid() && pid != containerRecoveryParentPID) { return ContainerRecoveryScope{}, fmt.Errorf("container recovery marker does not belong to this process or its launching parent") } - id, group, grouped := strings.Cut(identity, ":") - decoded, err := hex.DecodeString(id) - if err != nil || len(decoded) != sha256.Size { + digests := strings.Split(identity, ":") + if len(digests) > 3 { return ContainerRecoveryScope{}, fmt.Errorf("invalid container recovery identity") } - if grouped { - decoded, err = hex.DecodeString(group) - if err != nil || len(decoded) != sha256.Size { - return ContainerRecoveryScope{}, fmt.Errorf("invalid container recovery group") + for _, digest := range digests { + decoded, decodeErr := hex.DecodeString(digest) + if decodeErr != nil || len(decoded) != sha256.Size { + return ContainerRecoveryScope{}, fmt.Errorf("invalid container recovery identity") } } - return ContainerRecoveryScope{id: id, group: group}, nil + // An older CLI projects only the exact scope. Preserve its label, but never + // acknowledge cross-scope recovery without a validated namespace. + scope := ContainerRecoveryScope{id: digests[0]} + if len(digests) > 1 { + scope.namespace = digests[1] + } + if len(digests) > 2 { + scope.group = digests[2] + } + return scope, nil } diff --git a/runners/dockerrun/container_recovery_scope_test.go b/runners/dockerrun/container_recovery_scope_test.go index 437e9e368..2ed68a501 100644 --- a/runners/dockerrun/container_recovery_scope_test.go +++ b/runners/dockerrun/container_recovery_scope_test.go @@ -150,16 +150,22 @@ func TestContainerRecoveryScopeAgentProcess(t *testing.T) { config, _, err := env.desiredContainerConfigs(t.Context()) require.NoError(t, err) require.Equal(t, scope.id, config.Labels[LabelCodeflyRecoveryScope]) + require.Equal(t, scope.namespace, config.Labels[LabelCodeflyRecoveryNamespace]) for _, tc := range []struct { - name, marker, want string - wantError bool + name, marker, want, namespace, group string + wantError bool }{ - {"direct child", os.Getenv(ContainerRecoveryScopeEnvironment), scope.id, false}, - {"legacy", "", "", false}, - {"stale parent", "999999999:" + scope.id, "", true}, - {"malformed identity", strconv.Itoa(os.Getpid()) + ":bad", "", true}, - {"malformed group", strconv.Itoa(os.Getpid()) + ":" + scope.id + ":bad", "", true}, - {"malformed marker", "bad", "", true}, + {"direct child", os.Getenv(ContainerRecoveryScopeEnvironment), scope.id, scope.namespace, scope.group, false}, + {"legacy", "", "", "", "", false}, + {"stale parent", "999999999:" + scope.id, "", "", "", true}, + {"malformed identity", strconv.Itoa(os.Getpid()) + ":bad", "", "", "", true}, + // An older CLI projects the exact scope alone. It stays labeled, but + // without a namespace it never delegates cross-scope recovery. + {"old CLI exact scope", strconv.Itoa(os.Getpid()) + ":" + scope.id, scope.id, "", "", false}, + {"scope and namespace", strconv.Itoa(os.Getpid()) + ":" + scope.id + ":" + scope.namespace, scope.id, scope.namespace, "", false}, + {"malformed namespace", strconv.Itoa(os.Getpid()) + ":" + scope.id + ":bad", "", "", "", true}, + {"malformed group", strconv.Itoa(os.Getpid()) + ":" + scope.id + ":" + scope.namespace + ":bad", "", "", "", true}, + {"malformed marker", "bad", "", "", "", true}, } { t.Run(tc.name, func(t *testing.T) { t.Setenv(ContainerRecoveryScopeEnvironment, tc.marker) @@ -175,9 +181,8 @@ func TestContainerRecoveryScopeAgentProcess(t *testing.T) { labels := map[string]string{} require.NoError(t, json.Unmarshal(output, &labels)) require.Equal(t, tc.want, labels[LabelCodeflyRecoveryScope]) - if tc.want != "" { - require.Equal(t, scope.group, labels[LabelCodeflyRecoveryGroup]) - } + require.Equal(t, tc.namespace, labels[LabelCodeflyRecoveryNamespace]) + require.Equal(t, tc.group, labels[LabelCodeflyRecoveryGroup]) }) } } @@ -219,5 +224,6 @@ func TestContainerRecoveryScopeSurvivesParentExit(t *testing.T) { require.NoError(t, json.Unmarshal(output, &labels)) require.Equal(t, scope.id, labels[LabelCodeflyRecoveryScope]) require.Equal(t, scope.group, labels[LabelCodeflyRecoveryGroup]) + require.Equal(t, scope.namespace, labels[LabelCodeflyRecoveryNamespace]) require.Equal(t, labelTrue, labels[LabelCodeflyEphemeral]) } diff --git a/runners/dockerrun/disposable_recovery.go b/runners/dockerrun/disposable_recovery.go new file mode 100644 index 000000000..b34d284c9 --- /dev/null +++ b/runners/dockerrun/disposable_recovery.go @@ -0,0 +1,58 @@ +package dockerrun + +import ( + "context" + "errors" + "fmt" + "time" + + "github.com/containerd/errdefs" + "github.com/docker/docker/api/types/container" + "github.com/docker/docker/api/types/filters" +) + +// ReapDisposableContainers recovers ephemeral containers across invocation +// scopes, using their durable canonical home/workspace namespace label. It +// never broadens recovery of stateful, legacy or session-ledger containers. +func ReapDisposableContainers(ctx context.Context, scope ContainerRecoveryScope) error { + if scope.id == "" || scope.namespace == "" { + return errors.New("container recovery scope is unresolved") + } + cli, _, err := newDockerClient() + if err != nil { + return ctx.Err() + } + defer func() { _ = cli.Close() }() + if err = pingDockerClient(ctx, cli); err != nil { + return ctx.Err() // Docker retains the ownership labels for the next run. + } + listCtx, cancel := context.WithTimeout(ctx, 5*time.Second) + containers, err := cli.ContainerList(listCtx, container.ListOptions{All: true, Filters: filters.NewArgs( + filters.Arg("label", LabelCodeflyOwner+"="+labelTrue), + filters.Arg("label", LabelCodeflyRecoveryNamespace+"="+scope.namespace), + filters.Arg("label", LabelCodeflyEphemeral+"="+labelTrue), + )}) + cancel() + if err != nil { + return fmt.Errorf("list disposable containers: %w", err) + } + var failures []error + for _, c := range containers { + if !disposableContainerInNamespace(c, scope) { + continue + } + removeCtx, removeCancel := context.WithTimeout(ctx, 10*time.Second) + err := cli.ContainerRemove(removeCtx, c.ID, container.RemoveOptions{Force: true}) + removeCancel() + if err != nil && !errdefs.IsNotFound(err) { + failures = append(failures, fmt.Errorf("recover disposable container %s: %w", c.ID, err)) + } + } + return errors.Join(append(failures, ctx.Err())...) +} + +func disposableContainerInNamespace(c container.Summary, scope ContainerRecoveryScope) bool { + return scope.namespace != "" && c.Labels[LabelCodeflyRecoveryNamespace] == scope.namespace && + c.Labels[LabelCodeflyEphemeral] == labelTrue && + staleContainerInScope(c, ContainerRecoveryScope{id: c.Labels[LabelCodeflyRecoveryScope]}) +} diff --git a/runners/dockerrun/disposable_recovery_integration_test.go b/runners/dockerrun/disposable_recovery_integration_test.go new file mode 100644 index 000000000..02ac06400 --- /dev/null +++ b/runners/dockerrun/disposable_recovery_integration_test.go @@ -0,0 +1,89 @@ +//go:build !skip_infra + +package dockerrun + +import ( + "context" + "os" + "os/exec" + "strconv" + "testing" + + "github.com/docker/docker/api/types/container" + "github.com/docker/docker/api/types/mount" + "github.com/docker/docker/api/types/volume" + "github.com/docker/docker/errdefs" + "github.com/stretchr/testify/require" +) + +func TestDisposableRecoveryAcrossScopesPreservesOtherOwnersAndVolumes(t *testing.T) { + backend := newBackend(t) + home, workspace := t.TempDir(), t.TempDir() + old, err := NewContainerRecoveryScope(home, workspace, "old-disposable") + require.NoError(t, err) + next, err := NewContainerRecoveryScope(home, workspace, "new-disposable") + require.NoError(t, err) + foreignHome, err := NewContainerRecoveryScope(t.TempDir(), workspace, "old-disposable") + require.NoError(t, err) + foreignWorkspace, err := NewContainerRecoveryScope(home, t.TempDir(), "old-disposable") + require.NoError(t, err) + unregistered, err := NewContainerRecoveryScope(home, workspace, "unregistered") + require.NoError(t, err) + process := exec.Command("true") + require.NoError(t, process.Run()) + deadPID := strconv.Itoa(process.Process.Pid) + vol, err := backend.client.VolumeCreate(t.Context(), volume.CreateOptions{Name: uniqueName(t)}) + require.NoError(t, err) + t.Cleanup(func() { _ = backend.client.VolumeRemove(context.Background(), vol.Name, true) }) + for _, tc := range []struct { + name string + scope ContainerRecoveryScope + ephemeral, ledgered, live, running, keep bool + }{ + {name: "orphan", scope: old, ephemeral: true, running: true}, + {name: "stopped orphan", scope: old, ephemeral: true}, + {name: "stateful", scope: old, keep: true}, + {name: "ledger", scope: old, ephemeral: true, ledgered: true, keep: true}, + {name: "live agent", scope: old, ephemeral: true, live: true, keep: true}, + {name: "foreign home", scope: foreignHome, ephemeral: true, keep: true}, + {name: "foreign workspace", scope: foreignWorkspace, ephemeral: true, keep: true}, + {name: "another ephemeral scope", scope: unregistered, ephemeral: true}, + {name: "legacy", ephemeral: true, keep: true}, + } { + t.Run(tc.name, func(t *testing.T) { + labels := map[string]string{LabelCodeflyOwner: labelTrue, LabelCodeflySession: deadPID, LabelCodeflyRecoveryScope: tc.scope.id, LabelCodeflyRecoveryNamespace: tc.scope.namespace} + if tc.ephemeral { + labels[LabelCodeflyEphemeral] = labelTrue + } + if tc.ledgered { + labels[LabelCodeflyInvocation] = "retained-session" + } + if tc.live { + labels[LabelCodeflySession] = strconv.Itoa(os.Getpid()) + } + created, err := backend.client.ContainerCreate(t.Context(), &container.Config{Image: "alpine:latest", Cmd: []string{"sleep", "300"}, Labels: labels}, &container.HostConfig{NetworkMode: "none", Mounts: []mount.Mount{{Type: mount.TypeVolume, Source: vol.Name, Target: "/data"}}}, nil, nil, uniqueName(t)) + require.NoError(t, err) + t.Cleanup(func() { + _ = backend.client.ContainerRemove(context.Background(), created.ID, container.RemoveOptions{Force: true}) + }) + if tc.running { + require.NoError(t, backend.client.ContainerStart(t.Context(), created.ID, container.StartOptions{})) + } + // Cancellation must leave ownership and resources available for retry. + cancelled, cancel := context.WithCancel(t.Context()) + cancel() + require.Error(t, ReapDisposableContainers(cancelled, next)) + _, err = backend.client.ContainerInspect(t.Context(), created.ID) + require.NoError(t, err) + require.NoError(t, ReapDisposableContainers(t.Context(), next)) + _, err = backend.client.ContainerInspect(t.Context(), created.ID) + if tc.keep { + require.NoError(t, err) + } else { + require.True(t, errdefs.IsNotFound(err), "%v", err) + } + _, err = backend.client.VolumeInspect(t.Context(), vol.Name) + require.NoError(t, err, "recovery must never delete volumes") + }) + } +} diff --git a/runners/dockerrun/disposable_recovery_test.go b/runners/dockerrun/disposable_recovery_test.go new file mode 100644 index 000000000..fa22ae226 --- /dev/null +++ b/runners/dockerrun/disposable_recovery_test.go @@ -0,0 +1,57 @@ +package dockerrun + +import ( + "os" + "os/exec" + "strconv" + "testing" + + "github.com/docker/docker/api/types/container" + "github.com/stretchr/testify/require" +) + +func TestDisposableRecoveryUsesDurableNamespaceAndEphemeralOwnership(t *testing.T) { + home, workspace := t.TempDir(), t.TempDir() + previous, err := NewContainerRecoveryScope(home, workspace, "previous") + require.NoError(t, err) + next, err := NewContainerRecoveryScope(home, workspace, "next") + require.NoError(t, err) + require.NotEqual(t, previous.id, next.id) + require.Equal(t, previous.namespace, next.namespace) + foreignHome, err := NewContainerRecoveryScope(t.TempDir(), workspace, "previous") + require.NoError(t, err) + foreignWorkspace, err := NewContainerRecoveryScope(home, t.TempDir(), "previous") + require.NoError(t, err) + owner := exec.Command("true") + require.NoError(t, owner.Run()) + for _, tc := range []struct { + name string + scope ContainerRecoveryScope + ephemeral, ledgered, live, want bool + }{ + {"previous disposable", previous, true, false, false, true}, + {"previous stateful", previous, false, false, false, false}, + {"live owner", previous, true, false, true, false}, + {"ledger", previous, true, true, false, false}, + {"foreign home", foreignHome, true, false, false, false}, + {"foreign workspace", foreignWorkspace, true, false, false, false}, + {"legacy exact scope only", ContainerRecoveryScope{id: previous.id}, true, false, false, false}, + {"missing exact scope", ContainerRecoveryScope{namespace: previous.namespace}, true, false, false, false}, + } { + t.Run(tc.name, func(t *testing.T) { + labels := map[string]string{LabelCodeflyOwner: labelTrue, LabelCodeflyRecoveryScope: tc.scope.id, LabelCodeflyRecoveryNamespace: tc.scope.namespace, LabelCodeflySession: strconv.Itoa(owner.Process.Pid)} + if tc.ephemeral { + labels[LabelCodeflyEphemeral] = labelTrue + } + if tc.live { + labels[LabelCodeflySession] = strconv.Itoa(os.Getpid()) + } + if tc.ledgered { + labels[LabelCodeflyInvocation] = "session" + } + for _, state := range []string{"running", "exited"} { + require.Equal(t, tc.want, disposableContainerInNamespace(container.Summary{State: state, Labels: labels}, next)) + } + }) + } +} diff --git a/runners/dockerrun/docker_runner.go b/runners/dockerrun/docker_runner.go index 2ce6f43a3..e8cfcfe7b 100644 --- a/runners/dockerrun/docker_runner.go +++ b/runners/dockerrun/docker_runner.go @@ -409,6 +409,11 @@ func (docker *DockerEnvironment) desiredContainerConfigs(ctx context.Context) (* if scope.group != "" { containerConfig.Labels[LabelCodeflyRecoveryGroup] = scope.group } + // The durable home/workspace namespace is what lets a successor run + // recover this container after choosing an entirely fresh naming scope. + if scope.namespace != "" { + containerConfig.Labels[LabelCodeflyRecoveryNamespace] = scope.namespace + } } hostConfig := docker.createHostConfig(ctx) fingerprint, err := containerConfigFingerprint(containerConfig, hostConfig) diff --git a/services/service.go b/services/service.go index 8fcf68b05..9c774d95a 100644 --- a/services/service.go +++ b/services/service.go @@ -10,7 +10,10 @@ import ( "github.com/codefly-dev/core/agents/services" "github.com/codefly-dev/core/failures" runners "github.com/codefly-dev/core/runners/base" + "github.com/codefly-dev/core/runners/dockerrun" "github.com/codefly-dev/core/wool" + "google.golang.org/grpc" + "google.golang.org/grpc/metadata" "google.golang.org/protobuf/proto" "github.com/codefly-dev/core/agents/communicate" @@ -37,6 +40,9 @@ type Instance struct { Agent *services.ServiceAgent Info *agentv0.AgentInformation + // ContainerRecoveryScope is the agent's authenticated gRPC acknowledgement, + // not a claim inferred from the CLI's own Core version. + ContainerRecoveryScope string Builder *BuilderInstance Runtime *RuntimeInstance @@ -406,13 +412,17 @@ func Load(ctx context.Context, workspace *resources.Workspace, module *resources } instance.ProcessInfo.AgentPID = agent.ProcessInfo.PID - info, err := agent.GetAgentInformation(ctx, &agentv0.AgentInformationRequest{}) + var headers metadata.MD + info, err := agent.GetAgentInformation(ctx, &agentv0.AgentInformationRequest{}, grpc.Header(&headers)) if err != nil { return nil, w.Wrapf(err, "cannot get agent information: %v", service.Agent) } instance.Capabilities = info.Capabilities instance.Info = info + if values := headers.Get(dockerrun.ContainerRecoveryScopeHeader); len(values) == 1 { + instance.ContainerRecoveryScope = values[0] + } // Double-check under lock: another goroutine may have loaded the same // identity while we were spawning. Prefer the cached one so callers share From ead39e3905d1d1d1618e9bc4faed69df3844c4bb Mon Sep 17 00:00:00 2001 From: Antoine Toussaint Date: Fri, 11 Sep 2026 22:11:09 -0400 Subject: [PATCH 2/6] fix: bind container recovery to the caller host and PID namespace Identical home/workspace paths on a shared Docker daemon do not imply the same owner host. Include a stable host identity and Linux PID namespace in recovery ownership, require both labels in every sweep, and preserve foreign-host containers even when their numeric PID is absent locally. Linux requires a persistent machine identity rather than silently falling back to a boot ID. Cover host collisions in the selection and real-Docker regression tests. --- docs/session-ledger.md | 34 ++++++++++------- runners/dockerrun/container_recovery_host.go | 37 +++++++++++++++++++ runners/dockerrun/container_recovery_scope.go | 21 +++++++++-- .../container_recovery_scope_test.go | 7 +++- runners/dockerrun/container_sweep.go | 8 +++- runners/dockerrun/disposable_recovery.go | 2 +- .../disposable_recovery_integration_test.go | 3 ++ runners/dockerrun/disposable_recovery_test.go | 3 ++ runners/dockerrun/session_backend_test.go | 4 +- 9 files changed, 96 insertions(+), 23 deletions(-) create mode 100644 runners/dockerrun/container_recovery_host.go diff --git a/docs/session-ledger.md b/docs/session-ledger.md index 2ec1e4727..bf5b3af57 100644 --- a/docs/session-ledger.md +++ b/docs/session-ledger.md @@ -295,10 +295,10 @@ a silent forward-compatibility trick. The legacy PID-based startup sweep requires a `codefly.recovery-scope` label before checking owner liveness. The label hashes canonical Codefly home, -workspace path and resolved naming scope. This exact-scope sweep cannot claim -a different home, workspace or scope, even when its creator PID is gone. Containers without -this label, or with missing/malformed owner PIDs, require explicit owner recovery; -startup does not infer ownership from container names. +workspace path and resolved naming scope. This exact-scope sweep cannot claim a +different home, workspace or scope, even when its creator PID is gone. +Containers without this label, or with missing/malformed owner PIDs, require +explicit owner recovery; startup does not infer ownership from container names. Disposable SDK invocations additionally delegate orphan cleanup through a durable `codefly.recovery-group` label. Its identity includes the same canonical home and @@ -331,16 +331,22 @@ The CLI must require this acknowledgement before Docker provisioning. The startup parent identity is retained after reparenting so a CLI crash cannot make an already-spawned agent create unlabeled containers. -Containers also carry `codefly.recovery-namespace`, a hash of the canonical -home and workspace without the invocation's naming scope. This durable identity -lets `ReapDisposableContainers` recover explicitly ephemeral containers after -an SDK/test invocation dies, even when the next invocation has a fresh scope. -Unlike a recovery group, it needs no session metadata and survives a successor -that picked an entirely unrelated naming scope. It checks the original agent PID -and preserves live owners, stateful containers, and session-ledger containers. -A different home/workspace or a container without both ownership labels is never -eligible. No on-disk registry or inference from names is needed; failed removals -leave the labels available for retry. +Containers also carry `codefly.recovery-namespace`, a hash of the stable caller +host identity and canonical home/workspace, without the invocation's naming +scope. The host identity (including the caller's Linux PID namespace) prevents +local PID checks from claiming another host or PID namespace's containers on a +shared Docker daemon. Linux requires a persistent machine ID; recovery never +falls back to a boot ID that would strand containers after reboot. + +This durable identity lets `ReapDisposableContainers` recover explicitly +ephemeral containers after an SDK/test invocation dies, even when the next +invocation has a fresh scope. Where a recovery group delegates within one SDK +session's naming scope, the namespace needs no session metadata and survives a +successor that picked an entirely unrelated scope. It checks the original agent +PID and preserves live owners, stateful containers, and session-ledger +containers. A different home/workspace or a container without both ownership +labels is never eligible for either sweep. No on-disk registry or inference from +names is needed; failed removals leave the labels available for retry. Within the same scope, live owners and running stateful containers are retained. Stopped containers and running ephemeral containers with dead owners can be diff --git a/runners/dockerrun/container_recovery_host.go b/runners/dockerrun/container_recovery_host.go new file mode 100644 index 000000000..9853b1f6c --- /dev/null +++ b/runners/dockerrun/container_recovery_host.go @@ -0,0 +1,37 @@ +package dockerrun + +import ( + "context" + "encoding/hex" + "fmt" + "os" + "runtime" + "strings" + "time" + + "github.com/shirou/gopsutil/v3/host" +) + +func containerRecoveryHostID() (string, error) { + const linuxOS = "linux" + if runtime.GOOS == linuxOS { + pidNamespace, err := os.Readlink("/proc/self/ns/pid") + if err != nil { + return "", fmt.Errorf("identify local PID namespace: %w", err) + } + // gopsutil's Linux fallback is a boot ID. It changes on reboot and + // would silently strand containers, so require a persistent identity. + for _, path := range []string{"/etc/machine-id", "/var/lib/dbus/machine-id", "/sys/class/dmi/id/product_uuid"} { + data, err := os.ReadFile(path) // #nosec G304 -- fixed operating-system identity paths. + id := strings.ToLower(strings.ReplaceAll(strings.TrimSpace(string(data)), "-", "")) + decoded, decodeErr := hex.DecodeString(id) + if err == nil && decodeErr == nil && len(decoded) == 16 && strings.Trim(id, "0") != "" && strings.Trim(id, "f") != "" { + return id + ":" + pidNamespace, nil + } + } + return "", fmt.Errorf("no persistent machine ID; provision /etc/machine-id before using Docker recovery") + } + ctx, cancel := context.WithTimeout(context.Background(), 3*time.Second) + defer cancel() + return host.HostIDWithContext(ctx) +} diff --git a/runners/dockerrun/container_recovery_scope.go b/runners/dockerrun/container_recovery_scope.go index 930278208..c53a693a9 100644 --- a/runners/dockerrun/container_recovery_scope.go +++ b/runners/dockerrun/container_recovery_scope.go @@ -47,6 +47,17 @@ func InheritedContainerRecoveryScope() string { type ContainerRecoveryScope struct{ id, group, namespace string } func NewContainerRecoveryScope(home, workspace, namingScope string) (ContainerRecoveryScope, error) { + hostID, err := containerRecoveryHostID() + if err != nil { + return ContainerRecoveryScope{}, fmt.Errorf("resolve container recovery host: %w", err) + } + return newContainerRecoveryScope(home, workspace, namingScope, hostID) +} + +func newContainerRecoveryScope(home, workspace, namingScope, hostID string) (ContainerRecoveryScope, error) { + if hostID == "" { + return ContainerRecoveryScope{}, fmt.Errorf("container recovery requires a stable host identity") + } paths := []string{home, workspace} for i, path := range paths { if path == "" { @@ -63,7 +74,7 @@ func NewContainerRecoveryScope(home, workspace, namingScope string) (ContainerRe } scope := ContainerRecoveryScope{ id: recoveryScopeHash(paths, namingScope), - namespace: recoveryNamespaceHash(paths), + namespace: recoveryNamespaceHash(hostID, paths), } // Only an SDK invocation in disposable mode can delegate recovery across // invocation names. Never infer this from a name suffix alone: a regular @@ -93,9 +104,11 @@ func recoveryScopeHash(paths []string, namingScope string) string { // recoveryNamespaceHash covers every naming scope under one canonical // home/workspace pair, so an aborted run's disposable containers stay -// recoverable by a successor that picked a completely different scope. -func recoveryNamespaceHash(paths []string) string { - data, _ := json.Marshal(paths) +// recoverable by a successor that picked a completely different scope. The host +// identity is part of it: identical paths on a shared Docker daemon do not +// imply the same owner host, and a foreign host's PIDs are not ours to judge. +func recoveryNamespaceHash(hostID string, paths []string) string { + data, _ := json.Marshal(append([]string{hostID}, paths...)) sum := sha256.Sum256(data) return hex.EncodeToString(sum[:]) } diff --git a/runners/dockerrun/container_recovery_scope_test.go b/runners/dockerrun/container_recovery_scope_test.go index 2ed68a501..215f71881 100644 --- a/runners/dockerrun/container_recovery_scope_test.go +++ b/runners/dockerrun/container_recovery_scope_test.go @@ -64,6 +64,8 @@ func TestContainerRecoveryScopeIsolation(t *testing.T) { home, workspace := t.TempDir(), t.TempDir() scope, err := NewContainerRecoveryScope(home, workspace, "candidate") require.NoError(t, err) + otherHost, err := newContainerRecoveryScope(home, workspace, "candidate", "different-host") + require.NoError(t, err) otherHome, err := NewContainerRecoveryScope(t.TempDir(), workspace, "candidate") require.NoError(t, err) otherScope, err := NewContainerRecoveryScope(home, workspace, "foreign") @@ -85,6 +87,7 @@ func TestContainerRecoveryScopeIsolation(t *testing.T) { {name: "same scope stateful reuse", scope: scope, pid: deadPID, state: "running"}, {name: "same scope live owner", scope: scope, pid: strconv.Itoa(os.Getpid()), state: "exited", ephemeral: true}, {name: "same scope ledger", scope: scope, pid: deadPID, state: "exited", ledgered: true}, + {name: "foreign host stopped", scope: otherHost, pid: deadPID, state: "exited", ephemeral: true}, {name: "foreign home stopped", scope: otherHome, pid: deadPID, state: "exited"}, {name: "foreign scope ephemeral", scope: otherScope, pid: deadPID, state: "running", ephemeral: true}, {name: "foreign workspace stopped", scope: otherWorkspace, pid: deadPID, state: "exited"}, @@ -95,7 +98,7 @@ func TestContainerRecoveryScopeIsolation(t *testing.T) { {name: "negative PID", scope: scope, pid: "-1", state: "exited"}, } { t.Run(tc.name, func(t *testing.T) { - labels := map[string]string{LabelCodeflyOwner: "true", LabelCodeflyRecoveryScope: tc.scope.id, LabelCodeflySession: tc.pid} + labels := map[string]string{LabelCodeflyOwner: "true", LabelCodeflyRecoveryScope: tc.scope.id, LabelCodeflyRecoveryNamespace: tc.scope.namespace, LabelCodeflySession: tc.pid} if tc.ephemeral { labels[LabelCodeflyEphemeral] = "true" } @@ -125,6 +128,8 @@ func TestContainerRecoveryScopeCanonicalPaths(t *testing.T) { require.Equal(t, first, second) _, err = NewContainerRecoveryScope("", workspace, "") require.Error(t, err) + _, err = newContainerRecoveryScope(home, workspace, "", "") + require.ErrorContains(t, err, "stable host identity") _, err = NewContainerRecoveryScope(filepath.Join(root, "missing"), workspace, "") require.Error(t, err) require.Error(t, SetContainerRecoveryScope(ContainerRecoveryScope{})) diff --git a/runners/dockerrun/container_sweep.go b/runners/dockerrun/container_sweep.go index 53c0f44a7..e1278e1b3 100644 --- a/runners/dockerrun/container_sweep.go +++ b/runners/dockerrun/container_sweep.go @@ -53,7 +53,7 @@ func shouldReapContainer(state string, ownerAlive, ephemeral, ledgered bool) boo // explicitly labeled for the same SDK recovery group. // Legacy containers without scope labels require explicit owner recovery. func ReapStaleContainers(ctx context.Context, scope ContainerRecoveryScope) error { - if scope.id == "" { + if scope.id == "" || scope.namespace == "" { return fmt.Errorf("container recovery scope is unresolved") } w := wool.Get(ctx).In("base.ReapStaleContainers") @@ -138,7 +138,11 @@ func ReapStaleContainers(ctx context.Context, scope ContainerRecoveryScope) erro } func staleContainerInScope(c container.Summary, scope ContainerRecoveryScope) bool { - if scope.id == "" || c.Labels[LabelCodeflyOwner] != labelTrue || c.Labels[LabelCodeflyRecoveryScope] == "" { + // Both ownership labels are required: the durable namespace proves the + // container belongs to this host's home/workspace, and the scope (or the + // SDK recovery group) proves it belongs to this run's cleanup authority. + if scope.id == "" || scope.namespace == "" || c.Labels[LabelCodeflyOwner] != labelTrue || + c.Labels[LabelCodeflyRecoveryScope] == "" || c.Labels[LabelCodeflyRecoveryNamespace] != scope.namespace { return false } if c.Labels[LabelCodeflyRecoveryScope] != scope.id && diff --git a/runners/dockerrun/disposable_recovery.go b/runners/dockerrun/disposable_recovery.go index b34d284c9..1df506dea 100644 --- a/runners/dockerrun/disposable_recovery.go +++ b/runners/dockerrun/disposable_recovery.go @@ -54,5 +54,5 @@ func ReapDisposableContainers(ctx context.Context, scope ContainerRecoveryScope) func disposableContainerInNamespace(c container.Summary, scope ContainerRecoveryScope) bool { return scope.namespace != "" && c.Labels[LabelCodeflyRecoveryNamespace] == scope.namespace && c.Labels[LabelCodeflyEphemeral] == labelTrue && - staleContainerInScope(c, ContainerRecoveryScope{id: c.Labels[LabelCodeflyRecoveryScope]}) + staleContainerInScope(c, ContainerRecoveryScope{id: c.Labels[LabelCodeflyRecoveryScope], namespace: scope.namespace}) } diff --git a/runners/dockerrun/disposable_recovery_integration_test.go b/runners/dockerrun/disposable_recovery_integration_test.go index 02ac06400..6c1bbe245 100644 --- a/runners/dockerrun/disposable_recovery_integration_test.go +++ b/runners/dockerrun/disposable_recovery_integration_test.go @@ -23,6 +23,8 @@ func TestDisposableRecoveryAcrossScopesPreservesOtherOwnersAndVolumes(t *testing require.NoError(t, err) next, err := NewContainerRecoveryScope(home, workspace, "new-disposable") require.NoError(t, err) + foreignHost, err := newContainerRecoveryScope(home, workspace, "old-disposable", "different-host") + require.NoError(t, err) foreignHome, err := NewContainerRecoveryScope(t.TempDir(), workspace, "old-disposable") require.NoError(t, err) foreignWorkspace, err := NewContainerRecoveryScope(home, t.TempDir(), "old-disposable") @@ -45,6 +47,7 @@ func TestDisposableRecoveryAcrossScopesPreservesOtherOwnersAndVolumes(t *testing {name: "stateful", scope: old, keep: true}, {name: "ledger", scope: old, ephemeral: true, ledgered: true, keep: true}, {name: "live agent", scope: old, ephemeral: true, live: true, keep: true}, + {name: "foreign host", scope: foreignHost, ephemeral: true, keep: true}, {name: "foreign home", scope: foreignHome, ephemeral: true, keep: true}, {name: "foreign workspace", scope: foreignWorkspace, ephemeral: true, keep: true}, {name: "another ephemeral scope", scope: unregistered, ephemeral: true}, diff --git a/runners/dockerrun/disposable_recovery_test.go b/runners/dockerrun/disposable_recovery_test.go index fa22ae226..093070de1 100644 --- a/runners/dockerrun/disposable_recovery_test.go +++ b/runners/dockerrun/disposable_recovery_test.go @@ -18,6 +18,8 @@ func TestDisposableRecoveryUsesDurableNamespaceAndEphemeralOwnership(t *testing. require.NoError(t, err) require.NotEqual(t, previous.id, next.id) require.Equal(t, previous.namespace, next.namespace) + foreignHost, err := newContainerRecoveryScope(home, workspace, "previous", "different-host") + require.NoError(t, err) foreignHome, err := NewContainerRecoveryScope(t.TempDir(), workspace, "previous") require.NoError(t, err) foreignWorkspace, err := NewContainerRecoveryScope(home, t.TempDir(), "previous") @@ -33,6 +35,7 @@ func TestDisposableRecoveryUsesDurableNamespaceAndEphemeralOwnership(t *testing. {"previous stateful", previous, false, false, false, false}, {"live owner", previous, true, false, true, false}, {"ledger", previous, true, true, false, false}, + {"foreign host", foreignHost, true, false, false, false}, {"foreign home", foreignHome, true, false, false, false}, {"foreign workspace", foreignWorkspace, true, false, false, false}, {"legacy exact scope only", ContainerRecoveryScope{id: previous.id}, true, false, false, false}, diff --git a/runners/dockerrun/session_backend_test.go b/runners/dockerrun/session_backend_test.go index 567f7a536..821765bda 100644 --- a/runners/dockerrun/session_backend_test.go +++ b/runners/dockerrun/session_backend_test.go @@ -291,6 +291,7 @@ func TestReapStaleContainersKeepsLedgeredContainers(t *testing.T) { labels[LabelCodeflyOwner] = "true" labels[LabelCodeflySession] = deadOwner labels[LabelCodeflyRecoveryScope] = scope.id + labels[LabelCodeflyRecoveryNamespace] = scope.namespace labels[LabelCodeflyName] = name _, err := backend.client.ContainerCreate(ctx, &container.Config{Image: "alpine:latest", Labels: labels, Cmd: []string{"true"}}, @@ -350,6 +351,7 @@ func TestReapStaleContainersIsolatesHomeAndScope(t *testing.T) { labels := map[string]string{LabelCodeflyOwner: "true", LabelCodeflySession: tc.pid} if tc.scope.id != "" { labels[LabelCodeflyRecoveryScope] = tc.scope.id + labels[LabelCodeflyRecoveryNamespace] = tc.scope.namespace } if tc.ephemeral { labels[LabelCodeflyEphemeral] = "true" @@ -406,7 +408,7 @@ func TestReapStaleContainersRecoversDisposableSiblings(t *testing.T) { {"missing owner", old, "", true, false, true}, } { t.Run(tc.name, func(t *testing.T) { - labels := map[string]string{LabelCodeflyOwner: labelTrue, LabelCodeflyRecoveryScope: tc.scope.id, LabelCodeflyRecoveryGroup: tc.scope.group, LabelCodeflySession: tc.pid} + labels := map[string]string{LabelCodeflyOwner: labelTrue, LabelCodeflyRecoveryScope: tc.scope.id, LabelCodeflyRecoveryGroup: tc.scope.group, LabelCodeflyRecoveryNamespace: tc.scope.namespace, LabelCodeflySession: tc.pid} if tc.ephemeral { labels[LabelCodeflyEphemeral] = labelTrue } From b590e215c7e7e93612112e192f9b38110be386e3 Mon Sep 17 00:00:00 2001 From: Antoine Toussaint Date: Fri, 11 Sep 2026 22:18:07 -0400 Subject: [PATCH 3/6] fix: preserve containers owned by namespace init processes PID liveness helpers reject system PIDs, which must not be interpreted as proof that a namespace's live PID 1 is dead. Keep those containers and reject zero-PID inherited ownership markers. The real Linux container test runs as PID 1 and covers this regression. --- runners/dockerrun/container_recovery_scope_test.go | 5 +++++ runners/dockerrun/container_sweep.go | 4 +++- 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/runners/dockerrun/container_recovery_scope_test.go b/runners/dockerrun/container_recovery_scope_test.go index 215f71881..cee85bf99 100644 --- a/runners/dockerrun/container_recovery_scope_test.go +++ b/runners/dockerrun/container_recovery_scope_test.go @@ -95,6 +95,7 @@ func TestContainerRecoveryScopeIsolation(t *testing.T) { {name: "missing PID", scope: scope, state: "exited"}, {name: "malformed PID", scope: scope, pid: "invalid", state: "exited"}, {name: "zero PID", scope: scope, pid: "0", state: "exited"}, + {name: "init PID", scope: scope, pid: "1", state: "exited", ephemeral: true}, {name: "negative PID", scope: scope, pid: "-1", state: "exited"}, } { t.Run(tc.name, func(t *testing.T) { @@ -190,6 +191,10 @@ func TestContainerRecoveryScopeAgentProcess(t *testing.T) { require.Equal(t, tc.group, labels[LabelCodeflyRecoveryGroup]) }) } + t.Run("zero owner cannot claim namespace init", func(t *testing.T) { + t.Setenv(ContainerRecoveryScopeEnvironment, "0:"+scope.id+":"+scope.namespace) + require.Empty(t, InheritedContainerRecoveryScope()) + }) } func TestContainerRecoveryScopeSurvivesParentExit(t *testing.T) { diff --git a/runners/dockerrun/container_sweep.go b/runners/dockerrun/container_sweep.go index e1278e1b3..c989f7c41 100644 --- a/runners/dockerrun/container_sweep.go +++ b/runners/dockerrun/container_sweep.go @@ -150,7 +150,9 @@ func staleContainerInScope(c container.Summary, scope ContainerRecoveryScope) bo return false } pid, err := strconv.Atoi(c.Labels[LabelCodeflySession]) - if err != nil || pid <= 0 { + // IsProcessAlive intentionally rejects system PIDs. That is not evidence + // that PID 1 is dead: an agent/library caller may be its namespace's init. + if err != nil || pid <= 1 { return false } return shouldReapContainer(c.State, base.IsProcessAlive(pid), c.Labels[LabelCodeflyEphemeral] == labelTrue, c.Labels[LabelCodeflyInvocation] != "") From 5f366d1346b76eaeda3b7d5265917566356d2060 Mon Sep 17 00:00:00 2001 From: Antoine Toussaint Date: Fri, 11 Sep 2026 22:35:45 -0400 Subject: [PATCH 4/6] Reject container reuse that would retain another recovery owner Validate immutable ownership labels before reuse or fingerprint replacement so an adopter cannot lose a live ephemeral container to recovery. Preserve inherited ephemeral intent after parent exit and cover both paths with real Docker and subprocess tests. --- docs/session-ledger.md | 8 +- runners/dockerrun/container_recovery_reuse.go | 29 ++++++ .../container_recovery_reuse_test.go | 97 +++++++++++++++++++ .../container_sweep_decision_test.go | 4 +- runners/dockerrun/docker_runner.go | 14 ++- 5 files changed, 146 insertions(+), 6 deletions(-) create mode 100644 runners/dockerrun/container_recovery_reuse.go create mode 100644 runners/dockerrun/container_recovery_reuse_test.go diff --git a/docs/session-ledger.md b/docs/session-ledger.md index bf5b3af57..167f2e0c1 100644 --- a/docs/session-ledger.md +++ b/docs/session-ledger.md @@ -329,7 +329,7 @@ The agent's `GetAgentInformation` response includes the `codefly-container-recovery-scope` gRPC header with the validated inherited scope. The CLI must require this acknowledgement before Docker provisioning. The startup parent identity is retained after reparenting so a CLI crash cannot make -an already-spawned agent create unlabeled containers. +an already-spawned agent create unlabeled containers or lose its ephemeral intent. Containers also carry `codefly.recovery-namespace`, a hash of the stable caller host identity and canonical home/workspace, without the invocation's naming @@ -348,6 +348,12 @@ containers. A different home/workspace or a container without both ownership labels is never eligible for either sweep. No on-disk registry or inference from names is needed; failed removals leave the labels available for retry. +Provisioning checks ownership before reusing, replacing, starting or following +logs from an existing container. It refuses foreign/unverified ownership and +cross-agent adoption of ephemeral containers: Docker labels are immutable, so +adoption would leave the old creator recorded and let recovery delete a live +adopter's container. Stateful reuse within the same verified scope remains valid. + Within the same scope, live owners and running stateful containers are retained. Stopped containers and running ephemeral containers with dead owners can be removed, without requesting volume removal. Containers carrying an invocation diff --git a/runners/dockerrun/container_recovery_reuse.go b/runners/dockerrun/container_recovery_reuse.go new file mode 100644 index 000000000..97b143159 --- /dev/null +++ b/runners/dockerrun/container_recovery_reuse.go @@ -0,0 +1,29 @@ +package dockerrun + +import ( + "fmt" + + "github.com/docker/docker/api/types/container" +) + +// Labels are immutable. Adopting an ephemeral container would leave its old +// creator recorded as owner, letting recovery remove it while its adopter lives. +// Check ownership before fingerprint-driven replacement as well as reuse. +func validateContainerRecoveryReuse(existing, desired *container.Config) error { + if existing == nil { + return fmt.Errorf("existing container has no verifiable configuration") + } + if desired.Labels[LabelCodeflyRecoveryScope] == "" { + if existing.Labels[LabelCodeflyRecoveryScope] != "" || existing.Labels[LabelCodeflyRecoveryNamespace] != "" { + return fmt.Errorf("unscoped caller cannot claim a container with recovery ownership") + } + return nil // Callers that do not participate in scoped recovery. + } + if existing.Labels[LabelCodeflyOwner] != labelTrue || existing.Labels[LabelCodeflyRecoveryScope] != desired.Labels[LabelCodeflyRecoveryScope] || existing.Labels[LabelCodeflyRecoveryNamespace] != desired.Labels[LabelCodeflyRecoveryNamespace] { + return fmt.Errorf("existing container has foreign or unverified recovery ownership") + } + if (existing.Labels[LabelCodeflyEphemeral] == labelTrue || desired.Labels[LabelCodeflyEphemeral] == labelTrue) && existing.Labels[LabelCodeflySession] != desired.Labels[LabelCodeflySession] { + return fmt.Errorf("ephemeral container belongs to another agent; its ownership cannot be transferred") + } + return nil +} diff --git a/runners/dockerrun/container_recovery_reuse_test.go b/runners/dockerrun/container_recovery_reuse_test.go new file mode 100644 index 000000000..00c1586aa --- /dev/null +++ b/runners/dockerrun/container_recovery_reuse_test.go @@ -0,0 +1,97 @@ +//go:build !skip_infra + +package dockerrun + +import ( + "context" + "os" + "os/exec" + "strconv" + "testing" + + "github.com/codefly-dev/core/resources" + "github.com/docker/docker/api/types/container" + "github.com/stretchr/testify/require" +) + +func TestContainerRecoveryRejectsAdoptionBeforeReuseOrReplacement(t *testing.T) { + backend := newBackend(t) + t.Setenv(ContainerRecoveryScopeEnvironment, "") + scope, err := NewContainerRecoveryScope(t.TempDir(), t.TempDir(), "reuse") + require.NoError(t, err) + require.NoError(t, SetContainerRecoveryScope(scope)) + owner := exec.Command("true") + require.NoError(t, owner.Run()) + for _, tc := range []struct { + name string + ephemeral, sameOwner, foreign, legacy, changed, unscoped bool + // reject names the refusal expected, or is empty when reuse is allowed. + // An unscoped caller is turned away by name-based lookup before reuse + // validation is ever reached, so it reports that earlier refusal. + reject string + }{ + {name: "ephemeral cannot transfer", ephemeral: true, reject: "recover its owner explicitly"}, + {name: "own ephemeral can restart", ephemeral: true, sameOwner: true}, + {name: "stateful can survive agent restart"}, + {name: "foreign cannot reuse", foreign: true, reject: "recover its owner explicitly"}, + {name: "foreign cannot replace", foreign: true, changed: true, reject: "recover its owner explicitly"}, + {name: "legacy cannot adopt", legacy: true, reject: "recover its owner explicitly"}, + {name: "unscoped cannot adopt", unscoped: true, reject: "refusing adoption or replacement"}, + } { + t.Run(tc.name, func(t *testing.T) { + name := uniqueName(t) + newEnvironment := func() *DockerEnvironment { + t.Helper() + env, err := NewDockerHeadlessEnvironment(t.Context(), resources.NewDockerImage("alpine:latest"), name) + require.NoError(t, err) + t.Cleanup(func() { _ = env.client.Close() }) + env.WithPause() + if tc.ephemeral { + env.WithEphemeral() + } + return env + } + env := newEnvironment() + config, hostConfig, err := env.desiredContainerConfigs(t.Context()) + require.NoError(t, err) + config.Labels[LabelCodeflySession] = strconv.Itoa(owner.Process.Pid) + if tc.sameOwner { + config.Labels[LabelCodeflySession] = strconv.Itoa(os.Getpid()) + } + if tc.foreign { + config.Labels[LabelCodeflyRecoveryNamespace] = "foreign" + } + if tc.legacy { + delete(config.Labels, LabelCodeflyRecoveryNamespace) + } + if tc.changed { + config.Labels[LabelCodeflyConfig] = "old-configuration" + } + created, err := backend.client.ContainerCreate(t.Context(), config, hostConfig, nil, nil, env.name) + require.NoError(t, err) + t.Cleanup(func() { + _ = backend.client.ContainerRemove(context.Background(), created.ID, container.RemoveOptions{Force: true}) + }) + reuser := env + if tc.unscoped { + // Recovery ownership is fixed on an environment's first use, so + // an unscoped caller is a fresh environment that never inherited + // a marker — not this one with the ambient marker pulled out + // from under its already-resolved identity. + t.Setenv(ContainerRecoveryScopeEnvironment, "") + reuser = newEnvironment() + } + err = reuser.GetContainer(t.Context()) + if tc.reject != "" { + require.ErrorContains(t, err, tc.reject) + } else { + require.NoError(t, err) + } + inspected, err := backend.client.ContainerInspect(t.Context(), created.ID) + require.NoError(t, err, "ownership rejection must not delete or replace the old container") + if tc.reject != "" { + require.False(t, inspected.State.Running, "ownership rejection must precede starting the old container") + } + }) + } +} diff --git a/runners/dockerrun/container_sweep_decision_test.go b/runners/dockerrun/container_sweep_decision_test.go index 18e1f1f2e..2bc47acfa 100644 --- a/runners/dockerrun/container_sweep_decision_test.go +++ b/runners/dockerrun/container_sweep_decision_test.go @@ -62,8 +62,8 @@ func TestEphemeralContainersFlag(t *testing.T) { // in-process flag off, a marker equal to our parent's PID still counts. ephemeralContainers.Store(false) _ = os.Setenv(EphemeralContainersEnvironment, strconv.Itoa(os.Getppid())) - if !EphemeralContainers() { - t.Fatal("agent process did not inherit the ephemeral environment marker from its live parent") + if EphemeralContainers() != (os.Getppid() > 0) { + t.Fatal("only a real parent may supply an inherited ephemeral marker") } // A stale marker naming some unrelated process is deliberately ignored, so diff --git a/runners/dockerrun/docker_runner.go b/runners/dockerrun/docker_runner.go index e8cfcfe7b..acb868732 100644 --- a/runners/dockerrun/docker_runner.go +++ b/runners/dockerrun/docker_runner.go @@ -210,7 +210,11 @@ func (docker *DockerEnvironment) GetContainer(ctx context.Context) error { if err != nil { return w.Wrapf(err, "cannot check if container is present") } + owned := !exists defer func() { + if !owned { + return + } logContext := context.Background() err := docker.GetLogs(logContext) if err != nil { @@ -223,6 +227,10 @@ func (docker *DockerEnvironment) GetContainer(ctx context.Context) error { if inspectErr != nil { return w.Wrapf(inspectErr, "cannot inspect existing container") } + if ownershipErr := validateContainerRecoveryReuse(inspect.Config, containerConfig); ownershipErr != nil { + return w.Wrapf(ownershipErr, "cannot reuse container %s; recover its owner explicitly before retrying", docker.instance.ID) + } + owned = true actualFingerprint := "" if inspect.Config != nil && inspect.Config.Labels != nil { actualFingerprint = inspect.Config.Labels[LabelCodeflyConfig] @@ -623,14 +631,14 @@ func SetEphemeralContainers(v bool) { // EphemeralContainers reports whether this process spawns ephemeral containers. // It is true when this process enabled the mode in-process, or when it inherited -// the marker from its launching parent. Reparenting after a CLI crash must not -// turn a disposable container into a retained stateful container. +// the marker from its startup parent. Reparenting must not turn disposable +// resources into stateful ones. Descendants still reject a grandparent's marker. func EphemeralContainers() bool { if ephemeralContainers.Load() { return true } marker := os.Getenv(EphemeralContainersEnvironment) - return marker != "" && marker == strconv.Itoa(containerRecoveryParentPID) + return containerRecoveryParentPID > 0 && marker == strconv.Itoa(containerRecoveryParentPID) } func (docker *DockerEnvironment) createHostConfig(_ context.Context) *container.HostConfig { From e7b1f63c982f819f379ac611165602f35a088624 Mon Sep 17 00:00:00 2001 From: Antoine Toussaint Date: Sat, 12 Sep 2026 08:31:14 -0400 Subject: [PATCH 5/6] test: expect a fenced no-op teardown after a refused adoption MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Teardown was fenced to the generation Init acquired (#462) after the ownership guard test landed (#464), so a refused Init now leaves the environment with no acquired container and Shutdown returns cleanly instead of repeating the ownership refusal. The assertion still demanded that second error, failing on main independently of this branch. Assert the fenced behaviour instead: Shutdown is a silent no-op. The property the case exists for — teardown must not rediscover and delete the foreign container Init refused to claim — is unchanged and still proven by the inspection that follows. Co-Authored-By: Claude Opus 5 (1M context) --- runners/dockerrun/container_ownership_test.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/runners/dockerrun/container_ownership_test.go b/runners/dockerrun/container_ownership_test.go index 0c8dbe1d9..b352d136e 100644 --- a/runners/dockerrun/container_ownership_test.go +++ b/runners/dockerrun/container_ownership_test.go @@ -81,7 +81,11 @@ func TestContainerOwnershipGuardsReuseReplacementAndShutdown(t *testing.T) { require.Nil(t, second.instance) // Failed initialization must not let teardown rediscover and delete // the very foreign container that initialization refused to claim. - require.ErrorContains(t, second.Shutdown(ctx), "refusing adoption or replacement") + // Teardown is now fenced to the generation Init acquired (#462), so a + // refused Init leaves nothing to tear down: Shutdown is a silent + // no-op rather than a second ownership refusal. The container's + // survival is asserted below. + require.NoError(t, second.Shutdown(ctx)) inspected, err := first.client.ContainerInspect(ctx, originalID) require.NoError(t, err) require.True(t, inspected.State.Running) From 0ccfffed82d32eb082fdcd167e3cbf858fbeb018 Mon Sep 17 00:00:00 2001 From: Antoine Toussaint Date: Sat, 12 Sep 2026 13:00:54 -0400 Subject: [PATCH 6/6] fix: stop container recovery ownership from stranding what it should collect MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A self-review of the rebased recovery work found that every cross-version transition of the new ownership labels leaked containers silently, and that resolving ownership could refuse to run at all. Each fix below addresses the cause, not the symptom. Pre-upgrade containers were unreapable. staleContainerInScope required the namespace label to equal this scope's, so a container carrying only the exact scope — everything created before the label shipped — matched nothing and Docker cannot add a label after creation. An absent label is now read as "predates the namespace" and keeps the exact-scope authority it always had, while a label that is present and different stays foreign. The same absence is tolerated on the reuse path, which otherwise refused to reuse pre-upgrade stateful containers and would have failed runs holding real databases. The process marker was positional and untagged, and two revisions gave the field after the exact scope different meanings: a recovery group in one, the durable namespace in the other. A reader guessing wrong stamped a container with ownership no sweep could match, and the gRPC acknowledgement could not detect it because the misparsed value is well-formed. The marker now carries a v2 layout tag; an untagged marker is honored only for the exact scope and is refused outright if it carries trailing fields, so a mixed pair fails loudly at container creation instead of leaking. Resolving a scope hard-failed on hosts with no persistent machine ID, which is alpine, debian-slim and ubuntu images alike (verified). prepareContainerRecovery returns that error and aborts the run, so every containerized Docker run broke. The namespace is now optional: an unidentifiable host narrows recovery to the exact scope instead of refusing to resolve. InheritedContainerRecoveryScope echoes the empty namespace so the CLI's compatibility gate, which compares the agent's acknowledgement against its own computation, does not read a compatible agent as one that failed to understand the marker. ReapDisposableContainers discarded the Docker client and ping errors and returned nil, indistinguishable from a successful empty sweep — the leak it exists to collect resumed with no signal at all. It now traces both, warns when no durable identity narrows it to a no-op, and states the cross-scope rules directly instead of calling staleContainerInScope with a scope built from the container's own labels, which made that predicate's scope comparison tautological and hid what was really gating removal. Retire the recovery group. It has never had a consumer: the CLI was compiled against a Core revision without it, never references the label, and calls ReapDisposableContainers immediately before ReapStaleContainers with an early return on error — so a naming-scope prefix could only ever authorize removing containers the durable namespace had already collected. Keeping two mechanisms for one job also forced an arbitrary asymmetry about which one may act without host proof. validateContainerRecoveryReuse repeated ownership checks IsContainerPresent had already made, leaving three of five conditions unreachable and two error messages for one refusal; it now asserts only the host namespace and the immutability of an ephemeral owner. A duplicated acknowledgement header is refused rather than silently dropped, which had made an agent claiming two identities look like one claiming none. Marker parse errors name the field that was malformed again. Also reverts two comment rewrites of untouched main prose, and stops the ownership test asserting that Shutdown is silent rather than that the foreign container survives. Co-Authored-By: Claude Opus 5 (1M context) --- docs/session-ledger.md | 79 +++++++---- runners/dockerrun/container_ownership_test.go | 10 +- runners/dockerrun/container_recovery_host.go | 18 ++- runners/dockerrun/container_recovery_reuse.go | 30 +++-- .../container_recovery_reuse_test.go | 5 +- runners/dockerrun/container_recovery_scope.go | 114 ++++++++-------- .../container_recovery_scope_test.go | 124 +++++++++--------- runners/dockerrun/container_sweep.go | 41 ++---- runners/dockerrun/disposable_recovery.go | 44 ++++++- runners/dockerrun/disposable_recovery_test.go | 13 ++ runners/dockerrun/docker_runner.go | 9 +- runners/dockerrun/session_backend_test.go | 54 -------- services/container_recovery_test.go | 33 +++++ services/service.go | 21 ++- 14 files changed, 332 insertions(+), 263 deletions(-) create mode 100644 services/container_recovery_test.go diff --git a/docs/session-ledger.md b/docs/session-ledger.md index 167f2e0c1..8348cdd1f 100644 --- a/docs/session-ledger.md +++ b/docs/session-ledger.md @@ -295,18 +295,10 @@ a silent forward-compatibility trick. The legacy PID-based startup sweep requires a `codefly.recovery-scope` label before checking owner liveness. The label hashes canonical Codefly home, -workspace path and resolved naming scope. This exact-scope sweep cannot claim a -different home, workspace or scope, even when its creator PID is gone. -Containers without this label, or with missing/malformed owner PIDs, require -explicit owner recovery; startup does not infer ownership from container names. - -Disposable SDK invocations additionally delegate orphan cleanup through a durable -`codefly.recovery-group` label. Its identity includes the same canonical home and -workspace and the caller's naming scope, excluding only the current SDK session's -verified invocation suffix. This delegation is issued only in ephemeral mode with -matching session metadata. A later disposable invocation in that group can remove -dead-owner ephemeral siblings; live owners, stateful siblings and ledgered -containers remain protected. Ordinary naming scopes never gain this delegation. +workspace path and resolved naming scope. A different home, workspace or scope +cannot claim a container, even when its creator PID is gone. Containers without +this label, or with missing/malformed owner PIDs, require explicit owner recovery; +startup does not infer ownership from container names. The CLI sets the scope after resolving its run environment and before spawning agents. `SetContainerRecoveryScope` projects a PID-bound process marker to direct @@ -322,8 +314,8 @@ replacement or shutdown. A scoped caller cannot claim an unscoped container, and an unscoped caller cannot claim a scoped one. A mismatch is an error requiring explicit owner recovery, even when runtime configurations match. Each Docker environment retains its scope from first use so another flow changing the process -marker cannot redirect its cleanup. Recovery groups authorize orphan sweeping -only; they never authorize adopting another invocation's container. +marker cannot redirect its cleanup. Cross-scope recovery authorizes orphan +sweeping only; it never authorizes adopting another invocation's container. The agent's `GetAgentInformation` response includes the `codefly-container-recovery-scope` gRPC header with the validated inherited scope. @@ -333,20 +325,51 @@ an already-spawned agent create unlabeled containers or lose its ephemeral inten Containers also carry `codefly.recovery-namespace`, a hash of the stable caller host identity and canonical home/workspace, without the invocation's naming -scope. The host identity (including the caller's Linux PID namespace) prevents -local PID checks from claiming another host or PID namespace's containers on a -shared Docker daemon. Linux requires a persistent machine ID; recovery never -falls back to a boot ID that would strand containers after reboot. - -This durable identity lets `ReapDisposableContainers` recover explicitly -ephemeral containers after an SDK/test invocation dies, even when the next -invocation has a fresh scope. Where a recovery group delegates within one SDK -session's naming scope, the namespace needs no session metadata and survives a -successor that picked an entirely unrelated scope. It checks the original agent -PID and preserves live owners, stateful containers, and session-ledger -containers. A different home/workspace or a container without both ownership -labels is never eligible for either sweep. No on-disk registry or inference from -names is needed; failed removals leave the labels available for retry. +scope. The host identity prevents local PID checks from claiming another host's +containers on a shared Docker daemon. On Linux it requires a persistent machine +ID, never a boot ID that would strand containers after reboot, and it includes +the caller's PID namespace so a PID number from one namespace is never read as +liveness evidence in another. + +That PID namespace is a same-namespace proof, not a durable name: the kernel +recycles nsfs inodes, so sequential containers can reuse one and concurrent +containers never share one. Both directions err toward retention, so a mismatch +only ever declines to reap — but cross-scope recovery between concurrent +containerized runs on a shared daemon is consequently unavailable. + +A host that cannot prove a durable identity resolves a scope with no namespace. +Cross-scope recovery is then unavailable and `ReapDisposableContainers` says so, +while the exact-scope sweep continues unchanged; resolution never fails for this +reason, because the common Linux container base images provide no machine ID at +all. A namespace that is present and different is a foreign host and is never +eligible for any sweep or for reuse. + +The namespace lets `ReapDisposableContainers` recover explicitly ephemeral +containers after an SDK/test invocation dies, even when the next invocation has a +fresh scope. It needs no SDK session metadata and survives a successor that +picked an entirely unrelated scope. It checks the original agent PID and +preserves live owners, stateful containers, and session-ledger containers. No +on-disk registry or inference from names is needed; failed removals leave the +labels available for retry. + +Containers created before the namespace label existed carry only the exact +scope. Docker cannot add a label to a container that already exists, so the +exact-scope sweep and reuse both continue to accept a missing namespace label — +refusing it would strand every pre-upgrade container permanently and refuse to +reuse retained databases. Cross-scope authority is withheld from them, since +outside the exact scope the hash proves nothing about home, workspace or host. + +The process marker is tagged with its field layout (`pid:v2:scope:namespace`). A +marker written before the tag existed is honored only for the exact scope, and +one carrying untagged trailing fields is refused: two revisions gave the field +after the scope different meanings, and a reader that guesses wrong stamps a +container with ownership no sweep can match. + +The exact-scope sweep no longer delegates across naming scopes. That authority +belongs solely to `ReapDisposableContainers` and the durable namespace, which +subsumes what a naming-scope prefix could ever authorize: the CLI runs both +sweeps, so a prefix-scoped delegation could only ever remove containers the +namespace sweep had already collected. Provisioning checks ownership before reusing, replacing, starting or following logs from an existing container. It refuses foreign/unverified ownership and diff --git a/runners/dockerrun/container_ownership_test.go b/runners/dockerrun/container_ownership_test.go index b352d136e..dc02c5e57 100644 --- a/runners/dockerrun/container_ownership_test.go +++ b/runners/dockerrun/container_ownership_test.go @@ -81,11 +81,11 @@ func TestContainerOwnershipGuardsReuseReplacementAndShutdown(t *testing.T) { require.Nil(t, second.instance) // Failed initialization must not let teardown rediscover and delete // the very foreign container that initialization refused to claim. - // Teardown is now fenced to the generation Init acquired (#462), so a - // refused Init leaves nothing to tear down: Shutdown is a silent - // no-op rather than a second ownership refusal. The container's - // survival is asserted below. - require.NoError(t, second.Shutdown(ctx)) + // Teardown is fenced to the generation Init acquired (#462), so a + // refused Init leaves nothing to tear down. What matters is that the + // foreign container survives, which the inspection below asserts; + // whether Shutdown reports anything is not part of that contract. + _ = second.Shutdown(ctx) inspected, err := first.client.ContainerInspect(ctx, originalID) require.NoError(t, err) require.True(t, inspected.State.Running) diff --git a/runners/dockerrun/container_recovery_host.go b/runners/dockerrun/container_recovery_host.go index 9853b1f6c..35a1126e9 100644 --- a/runners/dockerrun/container_recovery_host.go +++ b/runners/dockerrun/container_recovery_host.go @@ -15,12 +15,22 @@ import ( func containerRecoveryHostID() (string, error) { const linuxOS = "linux" if runtime.GOOS == linuxOS { + // The PID namespace proves "same namespace as the container's recorded + // owner", which is what makes a local liveness check meaningful. It is + // not a durable name: the kernel recycles nsfs inodes, so sequential + // containers can reuse one and concurrent containers never share one. + // Both directions only ever decline to reap, so a stale or distinct + // inode costs recovery, never a wrongly deleted container. pidNamespace, err := os.Readlink("/proc/self/ns/pid") if err != nil { return "", fmt.Errorf("identify local PID namespace: %w", err) } // gopsutil's Linux fallback is a boot ID. It changes on reboot and // would silently strand containers, so require a persistent identity. + // Absence is reported as no identity rather than an error: alpine, + // debian-slim and ubuntu images ship no usable machine ID, and failing + // here would stop every containerized run instead of narrowing recovery + // to the exact scope. ReapDisposableContainers traces the consequence. for _, path := range []string{"/etc/machine-id", "/var/lib/dbus/machine-id", "/sys/class/dmi/id/product_uuid"} { data, err := os.ReadFile(path) // #nosec G304 -- fixed operating-system identity paths. id := strings.ToLower(strings.ReplaceAll(strings.TrimSpace(string(data)), "-", "")) @@ -29,9 +39,13 @@ func containerRecoveryHostID() (string, error) { return id + ":" + pidNamespace, nil } } - return "", fmt.Errorf("no persistent machine ID; provision /etc/machine-id before using Docker recovery") + return "", nil } ctx, cancel := context.WithTimeout(context.Background(), 3*time.Second) defer cancel() - return host.HostIDWithContext(ctx) + id, err := host.HostIDWithContext(ctx) + if err != nil { + return "", nil // Same degrade: no durable identity, not a failed run. + } + return id, nil } diff --git a/runners/dockerrun/container_recovery_reuse.go b/runners/dockerrun/container_recovery_reuse.go index 97b143159..d74313791 100644 --- a/runners/dockerrun/container_recovery_reuse.go +++ b/runners/dockerrun/container_recovery_reuse.go @@ -6,23 +6,29 @@ import ( "github.com/docker/docker/api/types/container" ) -// Labels are immutable. Adopting an ephemeral container would leave its old -// creator recorded as owner, letting recovery remove it while its adopter lives. -// Check ownership before fingerprint-driven replacement as well as reuse. +// validateContainerRecoveryReuse judges the reuse and fingerprint-replacement +// paths that name-based lookup cannot. IsContainerPresent has already proven +// Codefly ownership and an identical exact scope before this runs, so repeating +// those comparisons here only produced unreachable branches and a second error +// message for a refusal it had already made. What genuinely remains is the +// host-binding namespace and the immutability of an ephemeral owner: labels +// cannot be rewritten, so adopting an ephemeral container would leave its old +// creator recorded and let recovery remove it while its adopter lives. func validateContainerRecoveryReuse(existing, desired *container.Config) error { if existing == nil { return fmt.Errorf("existing container has no verifiable configuration") } - if desired.Labels[LabelCodeflyRecoveryScope] == "" { - if existing.Labels[LabelCodeflyRecoveryScope] != "" || existing.Labels[LabelCodeflyRecoveryNamespace] != "" { - return fmt.Errorf("unscoped caller cannot claim a container with recovery ownership") - } - return nil // Callers that do not participate in scoped recovery. + // A container created before the namespace label existed carries none, and + // Docker cannot add one after creation. Treating that absence as foreign + // would refuse to reuse every pre-upgrade container — including the stateful + // databases the reuse path exists to preserve across CLI restarts. A label + // that is present and different is a genuinely foreign host or PID namespace. + if existingNamespace := existing.Labels[LabelCodeflyRecoveryNamespace]; existingNamespace != "" && + existingNamespace != desired.Labels[LabelCodeflyRecoveryNamespace] { + return fmt.Errorf("existing container belongs to another host or PID namespace") } - if existing.Labels[LabelCodeflyOwner] != labelTrue || existing.Labels[LabelCodeflyRecoveryScope] != desired.Labels[LabelCodeflyRecoveryScope] || existing.Labels[LabelCodeflyRecoveryNamespace] != desired.Labels[LabelCodeflyRecoveryNamespace] { - return fmt.Errorf("existing container has foreign or unverified recovery ownership") - } - if (existing.Labels[LabelCodeflyEphemeral] == labelTrue || desired.Labels[LabelCodeflyEphemeral] == labelTrue) && existing.Labels[LabelCodeflySession] != desired.Labels[LabelCodeflySession] { + if (existing.Labels[LabelCodeflyEphemeral] == labelTrue || desired.Labels[LabelCodeflyEphemeral] == labelTrue) && + existing.Labels[LabelCodeflySession] != desired.Labels[LabelCodeflySession] { return fmt.Errorf("ephemeral container belongs to another agent; its ownership cannot be transferred") } return nil diff --git a/runners/dockerrun/container_recovery_reuse_test.go b/runners/dockerrun/container_recovery_reuse_test.go index 00c1586aa..59676740b 100644 --- a/runners/dockerrun/container_recovery_reuse_test.go +++ b/runners/dockerrun/container_recovery_reuse_test.go @@ -35,7 +35,10 @@ func TestContainerRecoveryRejectsAdoptionBeforeReuseOrReplacement(t *testing.T) {name: "stateful can survive agent restart"}, {name: "foreign cannot reuse", foreign: true, reject: "recover its owner explicitly"}, {name: "foreign cannot replace", foreign: true, changed: true, reject: "recover its owner explicitly"}, - {name: "legacy cannot adopt", legacy: true, reject: "recover its owner explicitly"}, + // Docker cannot add a namespace label to a container that already exists, + // so a pre-upgrade container must stay reusable — refusing it would strand + // the retained databases this path exists to preserve across restarts. + {name: "pre-upgrade container still reusable", legacy: true}, {name: "unscoped cannot adopt", unscoped: true, reject: "refusing adoption or replacement"}, } { t.Run(tc.name, func(t *testing.T) { diff --git a/runners/dockerrun/container_recovery_scope.go b/runners/dockerrun/container_recovery_scope.go index c53a693a9..8610742ce 100644 --- a/runners/dockerrun/container_recovery_scope.go +++ b/runners/dockerrun/container_recovery_scope.go @@ -10,15 +10,20 @@ import ( "path/filepath" "strconv" "strings" - - "github.com/codefly-dev/core/sdk/session" ) const LabelCodeflyRecoveryScope = "codefly.recovery-scope" -const LabelCodeflyRecoveryGroup = "codefly.recovery-group" const LabelCodeflyRecoveryNamespace = "codefly.recovery-namespace" const ContainerRecoveryScopeEnvironment = "CODEFLY_CONTAINER_RECOVERY_SCOPE" +// containerRecoveryMarkerVersion tags the marker's field layout. Two Core +// revisions gave the field after the exact scope different meanings — a +// recovery group in one, the durable namespace in the other — and a reader that +// guesses wrong stamps a container with ownership no sweep can ever match, +// leaking exactly what this recovery exists to collect. An untagged marker is +// therefore honored only for the one field every revision agreed on. +const containerRecoveryMarkerVersion = "v2" + // ContainerRecoveryScopeHeader acknowledges the ownership identity inherited // by an agent. Older agents omit it and cannot promise scoped recovery. const ContainerRecoveryScopeHeader = "codefly-container-recovery-scope" @@ -29,22 +34,23 @@ const ContainerRecoveryScopeHeader = "codefly-container-recovery-scope" var containerRecoveryParentPID = os.Getppid() // InheritedContainerRecoveryScope is the validated identity used by container -// creation and by the agent's read-only gRPC ownership acknowledgement. It is -// empty unless both the exact scope and the durable namespace are inherited: -// an older CLI that projects only a scope cannot promise cross-scope recovery. +// creation and by the agent's read-only gRPC ownership acknowledgement. It +// echoes the complete inherited identity, including an empty namespace when the +// CLI's host has no durable identity: the caller compares it against what it +// projected, so conflating "agent did not understand" with "cross-scope recovery +// is unavailable here" would report a compatible agent as incompatible. func InheritedContainerRecoveryScope() string { scope, err := inheritedContainerRecoveryScope() - if err != nil || scope.id == "" || scope.namespace == "" { + if err != nil || scope.id == "" { return "" } return scope.id + ":" + scope.namespace } // ContainerRecoveryScope binds cleanup to a home, workspace and resolved naming -// scope. group additionally covers the disposable siblings of one SDK -// invocation, and namespace is the durable canonical home/workspace identity -// that survives a run choosing an entirely fresh naming scope. -type ContainerRecoveryScope struct{ id, group, namespace string } +// scope; namespace is the durable canonical host/home/workspace identity that +// survives a run choosing an entirely fresh naming scope. +type ContainerRecoveryScope struct{ id, namespace string } func NewContainerRecoveryScope(home, workspace, namingScope string) (ContainerRecoveryScope, error) { hostID, err := containerRecoveryHostID() @@ -55,9 +61,6 @@ func NewContainerRecoveryScope(home, workspace, namingScope string) (ContainerRe } func newContainerRecoveryScope(home, workspace, namingScope, hostID string) (ContainerRecoveryScope, error) { - if hostID == "" { - return ContainerRecoveryScope{}, fmt.Errorf("container recovery requires a stable host identity") - } paths := []string{home, workspace} for i, path := range paths { if path == "" { @@ -72,26 +75,14 @@ func newContainerRecoveryScope(home, workspace, namingScope, hostID string) (Con return ContainerRecoveryScope{}, err } } - scope := ContainerRecoveryScope{ - id: recoveryScopeHash(paths, namingScope), - namespace: recoveryNamespaceHash(hostID, paths), - } - // Only an SDK invocation in disposable mode can delegate recovery across - // invocation names. Never infer this from a name suffix alone: a regular - // developer scope may happen to have the same spelling. - if EphemeralContainers() { - invocation := session.FromEnvironment(os.Environ()) - if invocation != nil { - identity, err := hex.DecodeString(invocation.ID) - if err == nil && len(identity) == 16 { - suffix := invocation.Scope() - if namingScope == suffix { - scope.group = recoveryScopeHash(paths, "") - } else if prefix, ok := strings.CutSuffix(namingScope, "-"+suffix); ok { - scope.group = recoveryScopeHash(paths, prefix) - } - } - } + scope := ContainerRecoveryScope{id: recoveryScopeHash(paths, namingScope)} + // A host that cannot prove a durable identity gets no namespace, so + // cross-scope recovery is unavailable while the exact-scope sweep keeps + // working. Refusing to resolve a scope at all would stop every run on a + // Linux host without a provisioned machine ID — which is every common + // container base image. + if hostID != "" { + scope.namespace = recoveryNamespaceHash(hostID, paths) } return scope, nil } @@ -116,15 +107,12 @@ func recoveryNamespaceHash(hostID string, paths []string) string { // SetContainerRecoveryScope projects ownership to directly spawned agents. // Older agents omit the label and are deliberately ineligible for startup cleanup. func SetContainerRecoveryScope(scope ContainerRecoveryScope) error { - if scope.id == "" || scope.namespace == "" { + if scope.id == "" { return fmt.Errorf("container recovery scope is unresolved") } - // pid:scope:namespace[:group] — group is optional, so it trails the - // namespace every resolved scope carries. - marker := strconv.Itoa(os.Getpid()) + ":" + scope.id + ":" + scope.namespace - if scope.group != "" { - marker += ":" + scope.group - } + // pid:v2:scope:namespace. The namespace is empty on a host with no durable + // identity. + marker := strings.Join([]string{strconv.Itoa(os.Getpid()), containerRecoveryMarkerVersion, scope.id, scope.namespace}, ":") return os.Setenv(ContainerRecoveryScopeEnvironment, marker) } @@ -141,24 +129,42 @@ func inheritedContainerRecoveryScope() (ContainerRecoveryScope, error) { if err != nil || pid <= 1 || (pid != os.Getpid() && pid != containerRecoveryParentPID) { return ContainerRecoveryScope{}, fmt.Errorf("container recovery marker does not belong to this process or its launching parent") } - digests := strings.Split(identity, ":") - if len(digests) > 3 { - return ContainerRecoveryScope{}, fmt.Errorf("invalid container recovery identity") - } - for _, digest := range digests { - decoded, decodeErr := hex.DecodeString(digest) - if decodeErr != nil || len(decoded) != sha256.Size { + fields := strings.Split(identity, ":") + if fields[0] != containerRecoveryMarkerVersion { + // A marker written before the layout was tagged. Only the exact scope + // had a single agreed meaning across those revisions, so refuse anything + // trailing it rather than guess whether it is a group or a namespace. + if len(fields) != 1 { + return ContainerRecoveryScope{}, fmt.Errorf("untagged container recovery marker carries ambiguous fields") + } + if err := validRecoveryDigest(fields[0]); err != nil { return ContainerRecoveryScope{}, fmt.Errorf("invalid container recovery identity") } + return ContainerRecoveryScope{id: fields[0]}, nil + } + digests := fields[1:] + if len(digests) != 2 { + return ContainerRecoveryScope{}, fmt.Errorf("container recovery marker has %d fields, want scope and namespace", len(digests)) + } + if err := validRecoveryDigest(digests[0]); err != nil { + return ContainerRecoveryScope{}, fmt.Errorf("invalid container recovery identity") } - // An older CLI projects only the exact scope. Preserve its label, but never - // acknowledge cross-scope recovery without a validated namespace. scope := ContainerRecoveryScope{id: digests[0]} - if len(digests) > 1 { + // The namespace is deliberately optional — a host with no durable identity + // projects the exact scope alone — but must be a digest when present. + if digests[1] != "" { + if err := validRecoveryDigest(digests[1]); err != nil { + return ContainerRecoveryScope{}, fmt.Errorf("invalid container recovery namespace") + } scope.namespace = digests[1] } - if len(digests) > 2 { - scope.group = digests[2] - } return scope, nil } + +func validRecoveryDigest(digest string) error { + decoded, err := hex.DecodeString(digest) + if err != nil || len(decoded) != sha256.Size { + return fmt.Errorf("not a sha256 digest") + } + return nil +} diff --git a/runners/dockerrun/container_recovery_scope_test.go b/runners/dockerrun/container_recovery_scope_test.go index cee85bf99..40c975884 100644 --- a/runners/dockerrun/container_recovery_scope_test.go +++ b/runners/dockerrun/container_recovery_scope_test.go @@ -11,55 +11,10 @@ import ( "time" "github.com/codefly-dev/core/resources" - "github.com/codefly-dev/core/sdk/session" "github.com/docker/docker/api/types/container" "github.com/stretchr/testify/require" ) -func disposableRecoveryScope(t *testing.T, home, workspace, label, identity string) ContainerRecoveryScope { - t.Helper() - t.Setenv(EphemeralContainersEnvironment, strconv.Itoa(os.Getppid())) - t.Setenv(session.IDEnvironment, identity) - t.Setenv(session.SecretEnvironment, "test-secret") - invocation := &session.Session{ID: identity} - name := invocation.Scope() - if label != "" { - name = label + "-" + name - } - scope, err := NewContainerRecoveryScope(home, workspace, name) - require.NoError(t, err) - require.NotEmpty(t, scope.group) - return scope -} - -func TestDisposableRecoveryGroupRequiresInvocationAndMode(t *testing.T) { - home, workspace := t.TempDir(), t.TempDir() - first := disposableRecoveryScope(t, home, workspace, "tests", strings.Repeat("a", 32)) - second := disposableRecoveryScope(t, home, workspace, "tests", strings.Repeat("b", 32)) - require.NotEqual(t, first.id, second.id) - require.Equal(t, first.group, second.group) - otherLabel := disposableRecoveryScope(t, home, workspace, "other", strings.Repeat("c", 32)) - require.NotEqual(t, first.group, otherLabel.group) - otherHome := disposableRecoveryScope(t, t.TempDir(), workspace, "tests", strings.Repeat("d", 32)) - require.NotEqual(t, first.group, otherHome.group) - otherWorkspace := disposableRecoveryScope(t, home, t.TempDir(), "tests", strings.Repeat("e", 32)) - require.NotEqual(t, first.group, otherWorkspace.group) - for _, tc := range []struct{ name, identity, marker, scope string }{ - {"reusable", strings.Repeat("a", 32), "", "tests-saaaaaaaaaaaa"}, - {"not an invocation", "", strconv.Itoa(os.Getppid()), "tests-saaaaaaaaaaaa"}, - {"malformed invocation", "a", strconv.Itoa(os.Getppid()), "tests-sa"}, - {"different invocation", strings.Repeat("b", 32), strconv.Itoa(os.Getppid()), "tests-saaaaaaaaaaaa"}, - } { - t.Run(tc.name, func(t *testing.T) { - t.Setenv(session.IDEnvironment, tc.identity) - t.Setenv(EphemeralContainersEnvironment, tc.marker) - scope, err := NewContainerRecoveryScope(home, workspace, tc.scope) - require.NoError(t, err) - require.Empty(t, scope.group) - }) - } -} - func TestContainerRecoveryScopeIsolation(t *testing.T) { home, workspace := t.TempDir(), t.TempDir() scope, err := NewContainerRecoveryScope(home, workspace, "candidate") @@ -72,6 +27,9 @@ func TestContainerRecoveryScopeIsolation(t *testing.T) { require.NoError(t, err) otherWorkspace, err := NewContainerRecoveryScope(home, t.TempDir(), "candidate") require.NoError(t, err) + // Pre-upgrade containers carry the exact scope and no namespace at all. + preUpgrade := ContainerRecoveryScope{id: scope.id} + preUpgradeForeignScope := ContainerRecoveryScope{id: otherScope.id} child := exec.Command("true") require.NoError(t, child.Run()) deadPID := strconv.Itoa(child.Process.Pid) @@ -92,6 +50,11 @@ func TestContainerRecoveryScopeIsolation(t *testing.T) { {name: "foreign scope ephemeral", scope: otherScope, pid: deadPID, state: "running", ephemeral: true}, {name: "foreign workspace stopped", scope: otherWorkspace, pid: deadPID, state: "exited"}, {name: "legacy no scope", pid: deadPID, state: "exited", ephemeral: true}, + // Docker cannot add a label to an existing container, so refusing these + // would strand every container created before the namespace shipped. + {name: "pre-upgrade same scope stopped", scope: preUpgrade, pid: deadPID, state: "exited", want: true}, + {name: "pre-upgrade same scope ephemeral", scope: preUpgrade, pid: deadPID, state: "running", ephemeral: true, want: true}, + {name: "pre-upgrade cross scope stays", scope: preUpgradeForeignScope, pid: deadPID, state: "running", ephemeral: true}, {name: "missing PID", scope: scope, state: "exited"}, {name: "malformed PID", scope: scope, pid: "invalid", state: "exited"}, {name: "zero PID", scope: scope, pid: "0", state: "exited"}, @@ -113,6 +76,16 @@ func TestContainerRecoveryScopeIsolation(t *testing.T) { require.False(t, staleContainerInScope(c, scope)) }) } + t.Run("host without durable identity", func(t *testing.T) { + degraded := ContainerRecoveryScope{id: scope.id} + require.False(t, staleContainerInScope(container.Summary{State: "exited", Labels: map[string]string{ + LabelCodeflyOwner: "true", LabelCodeflyRecoveryScope: scope.id, + LabelCodeflyRecoveryNamespace: scope.namespace, LabelCodeflySession: deadPID, + }}, degraded), "a caller that cannot prove a host identity must not reap a container bound to one") + require.True(t, staleContainerInScope(container.Summary{State: "exited", Labels: map[string]string{ + LabelCodeflyOwner: "true", LabelCodeflyRecoveryScope: scope.id, LabelCodeflySession: deadPID, + }}, degraded), "exact-scope recovery must survive the loss of a durable host identity") + }) } func TestContainerRecoveryScopeCanonicalPaths(t *testing.T) { @@ -129,8 +102,14 @@ func TestContainerRecoveryScopeCanonicalPaths(t *testing.T) { require.Equal(t, first, second) _, err = NewContainerRecoveryScope("", workspace, "") require.Error(t, err) - _, err = newContainerRecoveryScope(home, workspace, "", "") - require.ErrorContains(t, err, "stable host identity") + // Every common Linux container base image supplies no machine ID, so an + // unidentifiable host must narrow recovery rather than fail to resolve. + degraded, err := newContainerRecoveryScope(home, workspace, "", "") + require.NoError(t, err) + require.Equal(t, first.id, degraded.id) + require.Empty(t, degraded.namespace) + require.NoError(t, SetContainerRecoveryScope(degraded)) + require.NoError(t, ReapStaleContainers(t.Context(), degraded)) _, err = NewContainerRecoveryScope(filepath.Join(root, "missing"), workspace, "") require.Error(t, err) require.Error(t, SetContainerRecoveryScope(ContainerRecoveryScope{})) @@ -150,28 +129,38 @@ func TestContainerRecoveryScopeAgentProcess(t *testing.T) { t.Setenv(ContainerRecoveryScopeEnvironment, "") scope, err := NewContainerRecoveryScope(t.TempDir(), t.TempDir(), "agent") require.NoError(t, err) - scope.group = strings.Repeat("c", 64) require.NoError(t, SetContainerRecoveryScope(scope)) env := &DockerEnvironment{name: "test", image: &resources.DockerImage{Name: "alpine", Tag: "latest"}} config, _, err := env.desiredContainerConfigs(t.Context()) require.NoError(t, err) require.Equal(t, scope.id, config.Labels[LabelCodeflyRecoveryScope]) require.Equal(t, scope.namespace, config.Labels[LabelCodeflyRecoveryNamespace]) + pid := strconv.Itoa(os.Getpid()) + tagged := func(digests ...string) string { + return pid + ":" + containerRecoveryMarkerVersion + ":" + strings.Join(digests, ":") + } for _, tc := range []struct { - name, marker, want, namespace, group string - wantError bool + name, marker, want, namespace string + wantError bool }{ - {"direct child", os.Getenv(ContainerRecoveryScopeEnvironment), scope.id, scope.namespace, scope.group, false}, - {"legacy", "", "", "", "", false}, - {"stale parent", "999999999:" + scope.id, "", "", "", true}, - {"malformed identity", strconv.Itoa(os.Getpid()) + ":bad", "", "", "", true}, - // An older CLI projects the exact scope alone. It stays labeled, but - // without a namespace it never delegates cross-scope recovery. - {"old CLI exact scope", strconv.Itoa(os.Getpid()) + ":" + scope.id, scope.id, "", "", false}, - {"scope and namespace", strconv.Itoa(os.Getpid()) + ":" + scope.id + ":" + scope.namespace, scope.id, scope.namespace, "", false}, - {"malformed namespace", strconv.Itoa(os.Getpid()) + ":" + scope.id + ":bad", "", "", "", true}, - {"malformed group", strconv.Itoa(os.Getpid()) + ":" + scope.id + ":" + scope.namespace + ":bad", "", "", "", true}, - {"malformed marker", "bad", "", "", "", true}, + {"direct child", os.Getenv(ContainerRecoveryScopeEnvironment), scope.id, scope.namespace, false}, + {"legacy", "", "", "", false}, + {"stale parent", "999999999:" + containerRecoveryMarkerVersion + ":" + scope.id + ":" + scope.namespace, "", "", true}, + {"malformed identity", tagged("bad", scope.namespace), "", "", true}, + // A revision older than the layout tag projects the exact scope alone. It + // stays labeled, but never delegates cross-scope recovery. + {"untagged exact scope", pid + ":" + scope.id, scope.id, "", false}, + // A revision older than the tag also wrote pid:scope:group here. Reading + // that group as a namespace stamped the container with ownership no sweep + // could ever match, leaking it permanently — refuse instead of guessing. + {"untagged trailing field", pid + ":" + scope.id + ":" + strings.Repeat("d", 64), "", "", true}, + {"scope and namespace", tagged(scope.id, scope.namespace), scope.id, scope.namespace, false}, + // A host with no durable identity projects an empty namespace field. + {"scope without namespace", tagged(scope.id, ""), scope.id, "", false}, + {"missing namespace field", tagged(scope.id), "", "", true}, + {"malformed namespace", tagged(scope.id, "bad"), "", "", true}, + {"trailing field", tagged(scope.id, scope.namespace, strings.Repeat("e", 64)), "", "", true}, + {"malformed marker", "bad", "", "", true}, } { t.Run(tc.name, func(t *testing.T) { t.Setenv(ContainerRecoveryScopeEnvironment, tc.marker) @@ -188,13 +177,20 @@ func TestContainerRecoveryScopeAgentProcess(t *testing.T) { require.NoError(t, json.Unmarshal(output, &labels)) require.Equal(t, tc.want, labels[LabelCodeflyRecoveryScope]) require.Equal(t, tc.namespace, labels[LabelCodeflyRecoveryNamespace]) - require.Equal(t, tc.group, labels[LabelCodeflyRecoveryGroup]) }) } t.Run("zero owner cannot claim namespace init", func(t *testing.T) { - t.Setenv(ContainerRecoveryScopeEnvironment, "0:"+scope.id+":"+scope.namespace) + t.Setenv(ContainerRecoveryScopeEnvironment, tagged(scope.id, scope.namespace)) + require.NotEmpty(t, InheritedContainerRecoveryScope()) + t.Setenv(ContainerRecoveryScopeEnvironment, "0:"+containerRecoveryMarkerVersion+":"+scope.id+":"+scope.namespace) require.Empty(t, InheritedContainerRecoveryScope()) }) + t.Run("acknowledgement echoes an empty namespace", func(t *testing.T) { + // A compatible agent on a host with no durable identity must not look + // like an agent that failed to understand the marker at all. + t.Setenv(ContainerRecoveryScopeEnvironment, tagged(scope.id, "")) + require.Equal(t, scope.id+":", InheritedContainerRecoveryScope()) + }) } func TestContainerRecoveryScopeSurvivesParentExit(t *testing.T) { @@ -224,7 +220,6 @@ func TestContainerRecoveryScopeSurvivesParentExit(t *testing.T) { t.Setenv(ContainerRecoveryScopeEnvironment, "") scope, err := NewContainerRecoveryScope(t.TempDir(), t.TempDir(), "crash") require.NoError(t, err) - scope.group = strings.Repeat("a", 64) require.NoError(t, SetContainerRecoveryScope(scope)) cmd := exec.CommandContext(t.Context(), os.Args[0], "-test.run=^TestContainerRecoveryScopeSurvivesParentExit$") cmd.Env = append(os.Environ(), "RECOVERY_PARENT_EXIT_ROLE=parent", "RECOVERY_PARENT_EXIT_READY="+filepath.Join(t.TempDir(), "ready")) @@ -233,7 +228,6 @@ func TestContainerRecoveryScopeSurvivesParentExit(t *testing.T) { labels := map[string]string{} require.NoError(t, json.Unmarshal(output, &labels)) require.Equal(t, scope.id, labels[LabelCodeflyRecoveryScope]) - require.Equal(t, scope.group, labels[LabelCodeflyRecoveryGroup]) require.Equal(t, scope.namespace, labels[LabelCodeflyRecoveryNamespace]) require.Equal(t, labelTrue, labels[LabelCodeflyEphemeral]) } diff --git a/runners/dockerrun/container_sweep.go b/runners/dockerrun/container_sweep.go index c989f7c41..93927ff2b 100644 --- a/runners/dockerrun/container_sweep.go +++ b/runners/dockerrun/container_sweep.go @@ -49,11 +49,12 @@ func shouldReapContainer(state string, ownerAlive, ephemeral, ledgered bool) boo return true } -// ReapStaleContainers recovers this exact scope and dead disposable siblings -// explicitly labeled for the same SDK recovery group. +// ReapStaleContainers recovers containers labeled for this exact scope. Recovery +// across scopes belongs to ReapDisposableContainers, which is authorized by the +// durable namespace label rather than by a naming-scope prefix. // Legacy containers without scope labels require explicit owner recovery. func ReapStaleContainers(ctx context.Context, scope ContainerRecoveryScope) error { - if scope.id == "" || scope.namespace == "" { + if scope.id == "" { return fmt.Errorf("container recovery scope is unresolved") } w := wool.Get(ctx).In("base.ReapStaleContainers") @@ -89,25 +90,6 @@ func ReapStaleContainers(ctx context.Context, scope ContainerRecoveryScope) erro if err != nil { return fmt.Errorf("cannot list codefly containers: %w", err) } - if scope.group != "" { - siblings, listErr := cli.ContainerList(listCtx, container.ListOptions{ - All: true, - Filters: filters.NewArgs( - filters.Arg("label", LabelCodeflyOwner+"="+labelTrue), - filters.Arg("label", LabelCodeflyRecoveryGroup+"="+scope.group), - filters.Arg("label", LabelCodeflyEphemeral+"="+labelTrue), - ), - }) - if listErr != nil { - return fmt.Errorf("cannot list disposable sibling containers: %w", listErr) - } - for _, sibling := range siblings { - if sibling.Labels[LabelCodeflyRecoveryScope] != scope.id { - containers = append(containers, sibling) - } - } - } - reaped := 0 for _, c := range containers { if !staleContainerInScope(c, scope) { @@ -138,15 +120,16 @@ func ReapStaleContainers(ctx context.Context, scope ContainerRecoveryScope) erro } func staleContainerInScope(c container.Summary, scope ContainerRecoveryScope) bool { - // Both ownership labels are required: the durable namespace proves the - // container belongs to this host's home/workspace, and the scope (or the - // SDK recovery group) proves it belongs to this run's cleanup authority. - if scope.id == "" || scope.namespace == "" || c.Labels[LabelCodeflyOwner] != labelTrue || - c.Labels[LabelCodeflyRecoveryScope] == "" || c.Labels[LabelCodeflyRecoveryNamespace] != scope.namespace { + if scope.id == "" || c.Labels[LabelCodeflyOwner] != labelTrue || c.Labels[LabelCodeflyRecoveryScope] != scope.id { return false } - if c.Labels[LabelCodeflyRecoveryScope] != scope.id && - (scope.group == "" || c.Labels[LabelCodeflyRecoveryGroup] != scope.group || c.Labels[LabelCodeflyEphemeral] != labelTrue) { + // A namespace label naming another host's home/workspace is never ours, and + // neither is one we cannot verify because this host has no durable identity. + // A container created before the label existed carries none at all: Docker + // cannot add a label to a container that already exists, so refusing those + // would strand every pre-upgrade container forever. For them the exact scope + // hash — home, workspace and naming scope — remains the authority it was. + if namespace := c.Labels[LabelCodeflyRecoveryNamespace]; namespace != scope.namespace && namespace != "" { return false } pid, err := strconv.Atoi(c.Labels[LabelCodeflySession]) diff --git a/runners/dockerrun/disposable_recovery.go b/runners/dockerrun/disposable_recovery.go index 1df506dea..3a45a5516 100644 --- a/runners/dockerrun/disposable_recovery.go +++ b/runners/dockerrun/disposable_recovery.go @@ -4,27 +4,44 @@ import ( "context" "errors" "fmt" + "strconv" "time" "github.com/containerd/errdefs" "github.com/docker/docker/api/types/container" "github.com/docker/docker/api/types/filters" + + "github.com/codefly-dev/core/runners/base" + "github.com/codefly-dev/core/wool" ) // ReapDisposableContainers recovers ephemeral containers across invocation // scopes, using their durable canonical home/workspace namespace label. It // never broadens recovery of stateful, legacy or session-ledger containers. func ReapDisposableContainers(ctx context.Context, scope ContainerRecoveryScope) error { - if scope.id == "" || scope.namespace == "" { + if scope.id == "" { return errors.New("container recovery scope is unresolved") } + w := wool.Get(ctx).In("base.ReapDisposableContainers") + if scope.namespace == "" { + // The host could not prove a durable identity, so nothing here can be + // attributed across scopes. Say so: the leak this sweep exists to collect + // silently resumes otherwise. + w.Warn("cross-scope disposable recovery unavailable without a durable host identity; only exact-scope recovery will run") + return nil + } cli, _, err := newDockerClient() if err != nil { + // Docker not reachable — nothing to reap, not an error. Trace it anyway: + // a misconfigured daemon otherwise reads as "swept, nothing found". + w.Trace("docker client unavailable — skipping disposable recovery", wool.ErrField(err)) return ctx.Err() } defer func() { _ = cli.Close() }() if err = pingDockerClient(ctx, cli); err != nil { - return ctx.Err() // Docker retains the ownership labels for the next run. + // Docker retains the ownership labels for the next run. + w.Trace("docker engine unavailable — skipping disposable recovery", wool.ErrField(err)) + return ctx.Err() } listCtx, cancel := context.WithTimeout(ctx, 5*time.Second) containers, err := cli.ContainerList(listCtx, container.ListOptions{All: true, Filters: filters.NewArgs( @@ -51,8 +68,25 @@ func ReapDisposableContainers(ctx context.Context, scope ContainerRecoveryScope) return errors.Join(append(failures, ctx.Err())...) } +// disposableContainerInNamespace states the cross-scope rules directly rather +// than borrowing staleContainerInScope with a scope built from the container's +// own labels, which made that predicate's exact-scope comparison tautological +// and hid which checks were really gating removal. func disposableContainerInNamespace(c container.Summary, scope ContainerRecoveryScope) bool { - return scope.namespace != "" && c.Labels[LabelCodeflyRecoveryNamespace] == scope.namespace && - c.Labels[LabelCodeflyEphemeral] == labelTrue && - staleContainerInScope(c, ContainerRecoveryScope{id: c.Labels[LabelCodeflyRecoveryScope], namespace: scope.namespace}) + if scope.namespace == "" || + c.Labels[LabelCodeflyOwner] != labelTrue || + c.Labels[LabelCodeflyRecoveryScope] == "" || + c.Labels[LabelCodeflyRecoveryNamespace] != scope.namespace || + c.Labels[LabelCodeflyEphemeral] != labelTrue { + return false + } + pid, err := strconv.Atoi(c.Labels[LabelCodeflySession]) + // IsProcessAlive intentionally rejects system PIDs. That is not evidence + // that PID 1 is dead: an agent/library caller may be its namespace's init. + if err != nil || pid <= 1 { + return false + } + // Ephemeral is established above; a ledgered container stays with + // sessionledger recovery, which can read its retention record. + return shouldReapContainer(c.State, base.IsProcessAlive(pid), true, c.Labels[LabelCodeflyInvocation] != "") } diff --git a/runners/dockerrun/disposable_recovery_test.go b/runners/dockerrun/disposable_recovery_test.go index 093070de1..348c696e8 100644 --- a/runners/dockerrun/disposable_recovery_test.go +++ b/runners/dockerrun/disposable_recovery_test.go @@ -57,4 +57,17 @@ func TestDisposableRecoveryUsesDurableNamespaceAndEphemeralOwnership(t *testing. } }) } + t.Run("host without durable identity reaps nothing and reports why", func(t *testing.T) { + // Resolution now degrades instead of failing, so this sweep has to say it + // cannot run rather than return a silent success that reads as "nothing + // to collect" while the leak it exists for resumes. + degraded := ContainerRecoveryScope{id: previous.id} + require.False(t, disposableContainerInNamespace(container.Summary{State: "running", Labels: map[string]string{ + LabelCodeflyOwner: labelTrue, LabelCodeflyRecoveryScope: previous.id, + LabelCodeflyRecoveryNamespace: previous.namespace, LabelCodeflyEphemeral: labelTrue, + LabelCodeflySession: strconv.Itoa(owner.Process.Pid), + }}, degraded)) + require.NoError(t, ReapDisposableContainers(t.Context(), degraded)) + require.Error(t, ReapDisposableContainers(t.Context(), ContainerRecoveryScope{})) + }) } diff --git a/runners/dockerrun/docker_runner.go b/runners/dockerrun/docker_runner.go index acb868732..fd5cff68b 100644 --- a/runners/dockerrun/docker_runner.go +++ b/runners/dockerrun/docker_runner.go @@ -414,10 +414,7 @@ func (docker *DockerEnvironment) desiredContainerConfigs(ctx context.Context) (* containerConfig := docker.createContainerConfig(ctx) if scope.id != "" { containerConfig.Labels[LabelCodeflyRecoveryScope] = scope.id - if scope.group != "" { - containerConfig.Labels[LabelCodeflyRecoveryGroup] = scope.group - } - // The durable home/workspace namespace is what lets a successor run + // The durable host/home/workspace namespace is what lets a successor run // recover this container after choosing an entirely fresh naming scope. if scope.namespace != "" { containerConfig.Labels[LabelCodeflyRecoveryNamespace] = scope.namespace @@ -631,8 +628,8 @@ func SetEphemeralContainers(v bool) { // EphemeralContainers reports whether this process spawns ephemeral containers. // It is true when this process enabled the mode in-process, or when it inherited -// the marker from its startup parent. Reparenting must not turn disposable -// resources into stateful ones. Descendants still reject a grandparent's marker. +// the marker from its launching parent. Reparenting after a CLI crash must not +// turn a disposable container into a retained stateful container. func EphemeralContainers() bool { if ephemeralContainers.Load() { return true diff --git a/runners/dockerrun/session_backend_test.go b/runners/dockerrun/session_backend_test.go index 821765bda..d5bf8e6b4 100644 --- a/runners/dockerrun/session_backend_test.go +++ b/runners/dockerrun/session_backend_test.go @@ -10,7 +10,6 @@ import ( "os/exec" "path/filepath" "strconv" - "strings" "testing" "time" @@ -379,56 +378,3 @@ func TestReapStaleContainersIsolatesHomeAndScope(t *testing.T) { require.NoError(t, err) require.Equal(t, "retained bytes", string(content)) } - -func TestReapStaleContainersRecoversDisposableSiblings(t *testing.T) { - backend := newBackend(t) - ctx := t.Context() - home, workspace := t.TempDir(), t.TempDir() - old := disposableRecoveryScope(t, home, workspace, "tests", strings.Repeat("a", 32)) - current := disposableRecoveryScope(t, home, workspace, "tests", strings.Repeat("b", 32)) - foreign := disposableRecoveryScope(t, t.TempDir(), workspace, "tests", strings.Repeat("c", 32)) - otherScope := disposableRecoveryScope(t, home, workspace, "other", strings.Repeat("d", 32)) - otherWorkspace := disposableRecoveryScope(t, home, t.TempDir(), "tests", strings.Repeat("e", 32)) - process := exec.Command("true") - require.NoError(t, process.Run()) - deadPID := strconv.Itoa(process.Process.Pid) - for _, tc := range []struct { - name string - scope ContainerRecoveryScope - pid string - ephemeral, ledgered, keep bool - }{ - {"orphan", old, deadPID, true, false, false}, - {"live", old, strconv.Itoa(os.Getpid()), true, false, true}, - {"stateful", old, deadPID, false, false, true}, - {"ledgered", old, deadPID, true, true, true}, - {"foreign home", foreign, deadPID, true, false, true}, - {"foreign scope", otherScope, deadPID, true, false, true}, - {"foreign workspace", otherWorkspace, deadPID, true, false, true}, - {"missing owner", old, "", true, false, true}, - } { - t.Run(tc.name, func(t *testing.T) { - labels := map[string]string{LabelCodeflyOwner: labelTrue, LabelCodeflyRecoveryScope: tc.scope.id, LabelCodeflyRecoveryGroup: tc.scope.group, LabelCodeflyRecoveryNamespace: tc.scope.namespace, LabelCodeflySession: tc.pid} - if tc.ephemeral { - labels[LabelCodeflyEphemeral] = labelTrue - } - if tc.ledgered { - labels[LabelCodeflyInvocation] = "ledger" - } - created, err := backend.client.ContainerCreate(ctx, &container.Config{Image: "alpine:latest", Cmd: []string{"sleep", "300"}, Labels: labels}, nil, nil, nil, "") - require.NoError(t, err) - t.Cleanup(func() { - _ = backend.client.ContainerRemove(context.Background(), created.ID, container.RemoveOptions{Force: true}) - }) - require.NoError(t, backend.client.ContainerStart(ctx, created.ID, container.StartOptions{})) - require.NoError(t, ReapStaleContainers(ctx, current)) - inspected, err := backend.client.ContainerInspect(ctx, created.ID) - if tc.keep { - require.NoError(t, err) - require.True(t, inspected.State.Running) - } else { - require.True(t, errdefs.IsNotFound(err), "orphan survived: %v", err) - } - }) - } -} diff --git a/services/container_recovery_test.go b/services/container_recovery_test.go new file mode 100644 index 000000000..8bba53e9e --- /dev/null +++ b/services/container_recovery_test.go @@ -0,0 +1,33 @@ +package services + +import ( + "testing" + + "github.com/codefly-dev/core/runners/dockerrun" + "github.com/stretchr/testify/require" + "google.golang.org/grpc/metadata" +) + +func TestAcknowledgedContainerRecoveryScope(t *testing.T) { + header := dockerrun.ContainerRecoveryScopeHeader + t.Run("absent", func(t *testing.T) { + scope, err := acknowledgedContainerRecoveryScope(metadata.MD{}) + require.NoError(t, err) + require.Empty(t, scope) + }) + t.Run("single", func(t *testing.T) { + scope, err := acknowledgedContainerRecoveryScope(metadata.Pairs(header, "scope:namespace")) + require.NoError(t, err) + require.Equal(t, "scope:namespace", scope) + }) + t.Run("empty namespace is still an acknowledgement", func(t *testing.T) { + scope, err := acknowledgedContainerRecoveryScope(metadata.Pairs(header, "scope:")) + require.NoError(t, err) + require.Equal(t, "scope:", scope) + }) + t.Run("two identities are refused, not silently dropped", func(t *testing.T) { + scope, err := acknowledgedContainerRecoveryScope(metadata.Pairs(header, "one", header, "two")) + require.ErrorContains(t, err, "acknowledged 2 container recovery identities") + require.Empty(t, scope) + }) +} diff --git a/services/service.go b/services/service.go index 9c774d95a..93601f14e 100644 --- a/services/service.go +++ b/services/service.go @@ -420,8 +420,9 @@ func Load(ctx context.Context, workspace *resources.Workspace, module *resources instance.Capabilities = info.Capabilities instance.Info = info - if values := headers.Get(dockerrun.ContainerRecoveryScopeHeader); len(values) == 1 { - instance.ContainerRecoveryScope = values[0] + instance.ContainerRecoveryScope, err = acknowledgedContainerRecoveryScope(headers) + if err != nil { + return nil, w.Wrapf(err, "cannot trust container recovery acknowledgement: %v", service.Agent) } // Double-check under lock: another goroutine may have loaded the same @@ -528,3 +529,19 @@ func UpdateAgent(ctx context.Context, service *resources.Service) (*UpdateInform } return info, nil } + +// acknowledgedContainerRecoveryScope reads the agent's ownership +// acknowledgement. Exactly one value is the contract: silently ignoring a second +// made an agent that acknowledged two different identities indistinguishable +// from one that acknowledged none, which is the very condition this header +// exists to detect before Docker provisioning. +func acknowledgedContainerRecoveryScope(headers metadata.MD) (string, error) { + values := headers.Get(dockerrun.ContainerRecoveryScopeHeader) + if len(values) > 1 { + return "", fmt.Errorf("agent acknowledged %d container recovery identities: %v", len(values), values) + } + if len(values) == 0 { + return "", nil + } + return values[0], nil +}