go-e2e-sandbox-pool-leak

Answer

Root cause. The public registry delete handler DELETE /api/v1/apps/{id} only removes the app record from the registry; it never calls sandbox.DestroyPool. The admin delete handler performs both steps, so public deletes leave the backing sandbox containers (labeled uhlp.app_id=<uuid>) running — a permanent leak. The fix is to make both routes share one teardown path that removes the pool.

1. Shared delete helper (destroy pool + registry, retryable ordering)

Destroy the pool before deleting the registry record. If pool teardown fails, the record survives and the client can retry — an unrecoverable leak is worse than a transient 500.

// internal/api/registry.go

// ErrAppNotFound is returned when the app record (or its UUID) does not exist.
var ErrAppNotFound = errors.New("app not found")

// deleteAppWithPool tears down the app's sandbox pool and then removes the
// registry record. Pool teardown happens first so a failed destroy leaves the
// record in place and the delete is retryable (DestroyPool is idempotent).
func (s *Server) deleteAppWithPool(ctx context.Context, id string) error {
    app, err := s.registry.Get(ctx, id)
    if errors.Is(err, ErrNotFound) {
        return ErrAppNotFound
    }
    if err != nil {
        return fmt.Errorf("registry get %q: %w", id, err)
    }
    if err := s.sandbox.DestroyPool(ctx, app.UUID); err != nil {
        // App record intentionally kept so the caller can retry the delete.
        return fmt.Errorf("destroy sandbox pool for app %s: %w", app.UUID, err)
    }
    if err := s.registry.Delete(ctx, id); err != nil {
        return fmt.Errorf("registry delete %q: %w", id, err)
    }
    return nil
}

2. Public route now uses the shared helper

// Public: DELETE /api/v1/apps/{id}
func (s *Server) publicDeleteApp(w http.ResponseWriter, r *http.Request) {
    id := chi.URLParam(r, "id")
    switch err := s.deleteAppWithPool(r.Context(), id); {
    case errors.Is(err, ErrAppNotFound):
        http.Error(w, `{"error":"app not found"}`, http.StatusNotFound)
    case err != nil:
        s.log.Error("public delete app", "id", id, "err", err)
        http.Error(w, `{"error":"failed to delete app"}`, http.StatusInternalServerError)
    default:
        w.WriteHeader(http.StatusNoContent) // 204
    }
}

3. Admin route delegates to the same helper (removes duplication)

// Admin: DELETE /api/v1/admin/apps/{id}
func (s *Server) adminDeleteApp(w http.ResponseWriter, r *http.Request) {
    id := chi.URLParam(r, "id")
    // Same teardown path as the public route; admin auth already enforced by middleware.
    switch err := s.deleteAppWithPool(r.Context(), id); {
    case errors.Is(err, ErrAppNotFound):
        http.Error(w, `{"error":"app not found"}`, http.StatusNotFound)
    case err != nil:
        http.Error(w, `{"error":"failed to delete app"}`, http.StatusInternalServerError)
    default:
        w.WriteHeader(http.StatusNoContent)
    }
}

4. sandbox.DestroyPool — keyed by the UUID label, idempotent

The label is uhlp.app_id=<uuid> — the app UUID, never the app name.

// internal/sandbox/pool.go
const appLabelKey = "uhlp.app_id"

// DestroyPool removes every container backing appID's sandbox pool.
// Idempotent: no matching containers yields nil. appID must be the app UUID.
func (m *Manager) DestroyPool(ctx context.Context, appID string) error {
    if appID == "" {
        return errors.New("sandbox: DestroyPool requires non-empty app UUID")
    }
    containers, err := m.docker.ContainerList(ctx, types.ContainerListOptions{
        All:     true, // include stopped/exited containers — they still leak
        Filters: filters.NewArgs(filters.Arg("label", appLabelKey+"="+appID)),
    })
    if err != nil {
        return fmt.Errorf("list containers for %s: %w", appID, err)
    }
    for _, c := range containers {
        if err := m.docker.ContainerRemove(ctx, c.ID,
            types.ContainerRemoveOptions{Force: true}); err != nil && !isNotFound(err) {
            return fmt.Errorf("remove container %s: %w", c.ID, err)
        }
    }
    m.pools.Delete(appID) // in-memory pool registry
    return nil
}

func isNotFound(err error) bool {
    var e client.NotFound
    return errors.As(err, &e)
}

5. Defense-in-depth: reconciliation sweeper

A periodic sweeper guarantees convergence even if a handler crashes mid-delete or docker was down at delete time:

// internal/sandbox/reconcile.go
// Runs every 5m: destroys pools whose app record no longer exists in the registry.
func (s *Server) reconcileSandboxPools(ctx context.Context) {
    ticker := time.NewTicker(5 * time.Minute)
    defer ticker.Stop()
    for {
        select {
        case <-ctx.Done():
            return
        case <-ticker.C:
            containers, err := s.sandbox.ListAll(ctx) // ContainerList with label key only
            if err != nil {
                s.log.Error("reconcile list", "err", err)
                continue
            }
            for _, c := range containers {
                appID := c.Labels[appLabelKey]
                if _, err := s.registry.Get(ctx, appID); errors.Is(err, ErrNotFound) {
                    s.log.Info("reconcile: destroying orphaned pool", "app_id", appID)
                    _ = s.sandbox.DestroyPool(ctx, appID)
                }
            }
        }
    }
}

6. E2E test — the correct detection pattern (UUID label)

// e2e/delete_test.go

// containersForApp returns container IDs matching uhlp.app_id=<uuid>.
// NOTE: the label carries the app UUID, NOT the app name. Filtering by name
// matches nothing and silently false-passes the leak check.
func containersForApp(t *testing.T, appID string) []string {
    t.Helper()
    out, err := exec.Command("docker", "ps", "-a",
        "--filter", "label=uhlp.app_id="+appID,
        "--format", "{{.ID}}").Output()
    require.NoError(t, err)
    var ids []string
    for _, l := range strings.Split(strings.TrimSpace(string(out)), "\n") {
        if l != "" {
            ids = append(ids, l)
        }
    }
    return ids
}

func TestE2E_PublicDeleteDestroysSandboxPool(t *testing.T) {
    app := createApp(t, apiClient) // returns {uuid, name, ...}

    // Sanity pre-condition: the pool actually exists under the UUID label.
    // Guards against a malformed filter silently matching nothing (the bug
    // the first harness attempt hit when it used the app name as the label).
    require.NotEmpty(t, containersForApp(t, app.UUID),
        "precondition failed: pool not running under uhlp.app_id=%s", app.UUID)

    resp := apiClient.Delete("/api/v1/apps/" + app.UUID)
    require.Equal(t, http.StatusNoContent, resp.StatusCode)

    // Assert: zero containers remain after the public delete.
    assert.Empty(t, containersForApp(t, app.UUID),
        "sandbox pool leaked after public app delete (containers: %v)",
        containersForApp(t, app.UUID))

    // Assert: app record is gone too.
    require.Equal(t, http.StatusNotFound,
        apiClient.Get("/api/v1/apps/"+app.UUID).StatusCode)
}

func TestE2E_AdminDeleteDestroysSandboxPool(t *testing.T) { /* same shape, admin client */ }