Skip to content

feat: add docker_enabled option for agents to enable Docker-in-Docker… - #417

Merged
pikann merged 2 commits into
masterfrom
feature/add-docker-enabled-option-for-agents
Aug 19, 2026
Merged

feat: add docker_enabled option for agents to enable Docker-in-Docker…#417
pikann merged 2 commits into
masterfrom
feature/add-docker-enabled-option-for-agents

Conversation

@pikann

@pikann pikann commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

Summary

New conversations were slow to start. Digging into sandbox.Manager.Start, every cold start paid for two containers back to back and unconditionally:

  1. A privileged per-conversation docker:dind sidecar (network create + container create/start + poll docker info until dockerd answers) — run to full completion...
  2. ...before the actual goose sandbox container was even created, even though the two don't depend on each other beyond the sidecar's network ID (available almost immediately, long before dockerd itself is ready).

And that sidecar ran for every conversation, whether or not the agent ever executes a Docker command.

This PR fixes both:

  • Parallelize sidecar startup with the sandbox container's own boot. startDindSidecar now only creates the network and starts the container; the docker info readiness wait runs in a goroutine alongside the sandbox container's create/start/health-poll, joined right before Start returns a Handle. Cuts cold-start latency from dind_boot + sandbox_boot to roughly max(dind_boot, sandbox_boot).
  • Make the sidecar opt-in. New docker_enabled column on agents (default false). Agents that don't set it skip the sidecar, the private network, and the DOCKER_HOST env var entirely — no privileged container spun up for conversations that never touch Docker.

Details

  • services/api/migrations/000041_add_agent_docker_enabled.sqldocker_enabled BOOLEAN NOT NULL DEFAULT FALSE.
  • services/api: threaded DockerEnabled through the full agent CRUD path — domain entity, repo (all 3 INSERTs, the UPDATE, and both row↔entity mappers), service (CreateAgent/UpdateAgent/CreateGlobalAgent/UpdateGlobalAgent), DTOs, and handlers. LLM-only, same as SystemPrompt/GitCommitterName — ACP agents don't run through agent-runner's sandbox at all.
  • services/agent-runner:
    • agent.Config, the repo's agentRecord/SELECT, and executor.coldStart carry DockerEnabled down to sandbox.Config.
    • sandbox.Manager.Start skips startDindSidecar, the NetworkConnect to its network, and the DOCKER_HOST env line when DockerEnabled is false.
    • The dind-readiness goroutine runs against a context.WithCancel(ctx) that's cancelled on every return path (not just the happy one) — otherwise a sandbox-container failure after the goroutine starts would leave it polling docker info against a container about to be force-removed for up to dindReadyTimeout (90s), with nothing left reading the result.
    • Updated test/e2e/dind_test.go's two Docker-access tests to pass DockerEnabled: true explicitly, since the sidecar they exercise is no longer on by default.
  • Web: docker_enabled added to the Agent type and all four create/update payload shapes in agent-api.ts; a "Docker access" toggle in the agent Overview tab (next to git committer identity, LLM-type agents only); i18n strings added to all 9 locales.

Deliberately not included: pre-pulling the docker:dind image at install/upgrade time. Since the setting defaults off, most installs will never start that container at all, so warming it unconditionally would undercut the point of making it opt-in.

Test plan

  • go build ./... && go vet ./... && go test ./internal/... — clean on both services/api and services/agent-runner
  • npx tsc -b --noEmit and npx biome check — clean on apps/web
  • All 9 edited locale JSON files parse
  • test/e2e/dind_test.go's TestSandboxRunsAsRootWithDockerAccess / TestSandboxDindSidecarsAreIsolatedPerConversation (needs PACA_E2E=1 + real Docker — not run in this environment)
  • Manual: toggle "Docker access" on an agent in the web UI, start a conversation, confirm docker commands work inside the sandbox; confirm it's off by default for existing/new agents

… sidecar

- Introduced a new boolean field `docker_enabled` in the Agent model to allow agents to opt-in for a Docker-in-Docker sidecar during conversations.
- Updated the frontend to include a toggle for Docker access in the agent detail view.
- Enhanced localization files to support new labels and hints for Docker access.
- Modified the agent API and database schema to accommodate the new field.
- Implemented necessary changes in the agent runner service to manage Docker sidecar lifecycle based on the new configuration.
- Added migration script to safely add the `docker_enabled` column to the agents table.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ No critical issues — minor suggestions inline.

Reviewed changes — a clean, well-documented feature thread. docker_enabled is correctly wired through the full stack, and I specifically verified the two things that could have been subtle bugs:

  • Sandbox concurrency refactor (sandbox.go): moving waitForDindReady into a cancellable goroutine is sound — no goroutine leak (buffered chan, 90s internal deadline, dindCtx cancelled on every return path via defer cancelDindWait()), no nil-dind panic on teardown (Stop guards with if h.dind != nil), and no double-cleanup on the not-ready error path (cleanup() removes only the sandbox container; the deferred stopDindSidecar removes the sidecar container + network).
  • ACP handling: the UI toggle is correctly inside the !isAcp branch, matching the backend's LLM-only semantics.
  • Migration 000041 is additive with DEFAULT FALSE, consistent with repo convention.
Inline comment summary

The only actionable point is a coverage gap on the disabled path (see inline comment) and a cross-service deploy-ordering note below.

ℹ️ Cross-service migration ordering

agent-runner's FindByID now SELECTs docker_enabled, so a cold start fails — every execution, not just Docker ones — if the new agent-runner binary is live before migration 000041 is applied to the shared database. Since api and agent-runner are separate deployments sharing one schema, the migration must land before the runner rolls out.

Technical details
# Deploy ordering: agent-runner reads docker_enabled

## Affected sites
- services/agent-runner/internal/repository/postgres/agent_repository.go:85 — SELECT now references `agents.docker_enabled`

## Required outcome
- Ensure migration 000041 is applied (api-side migrate step) before the new agent-runner binary serves traffic, or cold starts fail on a missing-column error.

Pullfrog  | Fix all ➔Fix 👍s ➔View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

Comment thread services/agent-runner/test/e2e/dind_test.go
…isabled

This commit introduces a new end-to-end test that verifies the behavior of the sandbox when Docker is disabled. The test checks that no dind sidecar container or associated network is created for a conversation where Docker is not enabled. It also ensures that the sandbox container does not have the DOCKER_HOST environment variable set, confirming that the system behaves correctly in this scenario.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes — the new commit e615afff adds TestSandboxDindSidecarNotStartedWhenDisabled, the negative-path e2e test that closes the coverage gap this PR's previous review flagged:

  • Added TestSandboxDindSidecarNotStartedWhenDisabled: starts a DockerEnabled: false (default) conversation and asserts the paca-dind-<conv> sidecar container and paca-sbx-net-<conv> private network do not exist, and that the sandbox container's env carries no DOCKER_HOST. Gated consistently with its siblings (testing.Short + PACA_E2E=1 + checkDockerAvailable), reuses newSandboxManager / rawUpstreamGooseImage, and cleans up via mgr.Stop.
  • Verified the black-box container/network names match dindContainerName / conversationNetworkName in dind.go (the replication is intentional — the test is in package e2e_test and can't reach the unexported helpers).
  • Brought in github.com/containerd/errdefs (direct) plus indirect dep bumps via go mod tidy; go vet ./test/e2e/... and go build ./... are clean.

All three assertions requested in the prior review thread (no container, no network, no DOCKER_HOST) are covered, and no new issues surfaced in the delta.

Pullfrog  | View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

@pikann
pikann merged commit 4f9d029 into master Aug 19, 2026
10 checks passed
@pikann
pikann deleted the feature/add-docker-enabled-option-for-agents branch August 19, 2026 09:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant