From e081c8335fd723ab48bd3cf3bd2c17197a7ae18a Mon Sep 17 00:00:00 2001 From: Hongming Wang Date: Thu, 30 Apr 2026 09:18:16 -0700 Subject: [PATCH] refactor(handlers): widen WorkspaceHandler.provisioner to LocalProvisionerAPI interface (#2369) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Symmetric with the existing CPProvisionerAPI interface. Closes the asymmetry where the SaaS provisioner field was an interface (mockable in tests) but the Docker provisioner field was a concrete pointer (not). ## Changes - New ``provisioner.LocalProvisionerAPI`` interface — the 7 methods WorkspaceHandler / TeamHandler call on h.provisioner today: Start, Stop, IsRunning, ExecRead, RemoveVolume, VolumeHasFile, WriteAuthTokenToVolume. Compile-time assertion confirms *Provisioner satisfies it. Mirror of cp_provisioner.go's CPProvisionerAPI block. - ``WorkspaceHandler.provisioner`` and ``TeamHandler.provisioner`` re-typed from ``*provisioner.Provisioner`` to ``provisioner.LocalProvisionerAPI``. Constructor parameter type is unchanged — the assignment widens to the interface, so the 200+ callers of ``NewWorkspaceHandler`` / ``NewTeamHandler`` are unaffected. - Constructors gain a ``if p != nil`` guard before assigning to the interface field. Without this, ``NewWorkspaceHandler(..., nil, ...)`` (the test fixture pattern across 200+ tests) yields a typed-nil interface value where ``h.provisioner != nil`` evaluates *true*, and the SaaS-vs-Docker fork incorrectly routes nil-fixture tests into the Docker code path. Documented inline with reference to the Go FAQ. - Hardened the 5 Provisioner methods that lacked nil-receiver guards (Start, ExecRead, WriteAuthTokenToVolume, RemoveVolume, VolumeHasFile) — return ErrNoBackend on nil receiver instead of panicking on p.cli dereference. Symmetric with Stop/IsRunning (already hardened in #1813). Defensive cleanup so a future caller that bypasses the constructor's nil-elision still degrades cleanly. - Extended TestZeroValuedBackends_NoPanic with 5 new sub-tests covering the newly-hardened nil-receiver paths. Defense-in-depth: a future refactor that drops one of the nil-checks fails red here before reaching production. ## Why now - Provisioner orchestration has been touched in #2366 / #2368 — the interface symmetry is the natural follow-up captured in #2369. - Future work (CP fleet redeploy endpoint, multi-backend provisioners) wants this in place. Memory note ``project_provisioner_abstraction.md`` calls out pluggable backends as a north-star. - Memory note ``feedback_long_term_robust_automated.md`` — compile-time gates + ErrNoBackend symmetry > runtime panics. ## Verification - ``go build ./...`` clean. - ``go test ./...`` clean — 1300+ tests pass, including the previously-flaky Create-with-nil-provisioner paths that now exercise the constructor's nil-elision correctly. - ``go test ./internal/provisioner/ -run TestZeroValuedBackends_NoPanic -v`` — all 11 nil-receiver subtests green (was 6, +5 for the newly-hardened methods). Co-Authored-By: Claude Opus 4.7 (1M context) --- workspace-server/internal/handlers/team.go | 14 ++++-- .../internal/handlers/workspace.go | 24 ++++++++-- .../provisioner/backend_contract_test.go | 44 +++++++++++++++++++ .../provisioner/local_provisioner_api.go | 36 +++++++++++++++ .../internal/provisioner/provisioner.go | 15 +++++++ 5 files changed, 127 insertions(+), 6 deletions(-) create mode 100644 workspace-server/internal/provisioner/local_provisioner_api.go diff --git a/workspace-server/internal/handlers/team.go b/workspace-server/internal/handlers/team.go index 96da44a0..29a4a80c 100644 --- a/workspace-server/internal/handlers/team.go +++ b/workspace-server/internal/handlers/team.go @@ -25,20 +25,28 @@ import ( // NULL auth_token — same drift class as the SaaS bug fixed in #2366. type TeamHandler struct { broadcaster *events.Broadcaster - provisioner *provisioner.Provisioner + // provisioner is interface-typed (#2369) for the same reason as + // WorkspaceHandler.provisioner — Stop is the only call site here + // and it's on the LocalProvisionerAPI surface, so widening is free + // and symmetric with WorkspaceHandler. + provisioner provisioner.LocalProvisionerAPI wh *WorkspaceHandler platformURL string configsDir string } func NewTeamHandler(b *events.Broadcaster, p *provisioner.Provisioner, wh *WorkspaceHandler, platformURL, configsDir string) *TeamHandler { - return &TeamHandler{ + h := &TeamHandler{ broadcaster: b, - provisioner: p, wh: wh, platformURL: platformURL, configsDir: configsDir, } + // Avoid the typed-nil interface trap (see NewWorkspaceHandler note). + if p != nil { + h.provisioner = p + } + return h } // Expand handles POST /workspaces/:id/expand diff --git a/workspace-server/internal/handlers/workspace.go b/workspace-server/internal/handlers/workspace.go index ae70256b..11490c80 100644 --- a/workspace-server/internal/handlers/workspace.go +++ b/workspace-server/internal/handlers/workspace.go @@ -34,7 +34,15 @@ type WorkspaceHandler struct { // satisfies the interface — see the compile-time assertion in // internal/events/broadcaster.go. broadcaster events.EventEmitter - provisioner *provisioner.Provisioner + // provisioner narrowed from `*provisioner.Provisioner` to the + // provisioner.LocalProvisionerAPI interface (#2369) so tests can + // substitute a stub without standing up the real Docker daemon. + // Production callers still pass *provisioner.Provisioner via + // NewWorkspaceHandler, which satisfies the interface — see the + // compile-time assertion in internal/provisioner/local_provisioner_api.go. + // Mirrors cpProv's interface-typed field for symmetry across both + // backends. + provisioner provisioner.LocalProvisionerAPI // cpProv narrowed from `*provisioner.CPProvisioner` to the // provisioner.CPProvisionerAPI interface (#1814) so tests can // substitute a stub without standing up the real CP HTTP client + @@ -60,12 +68,22 @@ type WorkspaceHandler struct { } func NewWorkspaceHandler(b events.EventEmitter, p *provisioner.Provisioner, platformURL, configsDir string) *WorkspaceHandler { - return &WorkspaceHandler{ + h := &WorkspaceHandler{ broadcaster: b, - provisioner: p, platformURL: platformURL, configsDir: configsDir, } + // Only assign p when the concrete pointer is non-nil. Without this + // guard, a `NewWorkspaceHandler(..., nil, ...)` call (which all the + // non-Docker test fixtures use) yields a typed-nil interface — the + // `if h.provisioner != nil` checks scattered through the SaaS-vs- + // Docker fork would incorrectly evaluate as non-nil and route into + // the Docker path. Mirrors the pattern documented in the Go FAQ + // "Why is my nil error value not equal to nil?". + if p != nil { + h.provisioner = p + } + return h } // SetCPProvisioner wires the control plane provisioner for SaaS tenants. diff --git a/workspace-server/internal/provisioner/backend_contract_test.go b/workspace-server/internal/provisioner/backend_contract_test.go index a5948f54..b998882a 100644 --- a/workspace-server/internal/provisioner/backend_contract_test.go +++ b/workspace-server/internal/provisioner/backend_contract_test.go @@ -51,6 +51,14 @@ import ( // Kept minimal on purpose; expand only when a new cross-backend // behavior needs a contract test. Implementation-private methods stay // off this interface. +// +// NOTE: this is the SHARED-CONTRACT subset — the methods both backends +// must implement. The handler-facing surface is wider (LocalProvisionerAPI +// includes ExecRead / RemoveVolume / VolumeHasFile / WriteAuthTokenToVolume, +// which are Docker-specific because the CP backend delegates volume +// management to EC2 boot scripts). Don't widen Backend with Docker-only +// methods — split into per-backend interfaces if a method only one side +// implements. type Backend interface { Start(ctx context.Context, cfg WorkspaceConfig) (string, error) Stop(ctx context.Context, workspaceID string) error @@ -225,4 +233,40 @@ func TestZeroValuedBackends_NoPanic(t *testing.T) { t.Errorf("nil *CPProvisioner.Stop: got %v, want ErrNoBackend", err) } }) + + // Methods extended to the LocalProvisionerAPI interface in #2369 — + // every one needs the same nil-receiver guard so the typed-nil + // interface case (handler tests passing nil concrete *Provisioner + // to NewWorkspaceHandler, which the constructor now elides into a + // real nil interface) doesn't regress in the future. + t.Run("nil_Provisioner.Start", func(t *testing.T) { + var p *Provisioner + if _, err := p.Start(context.Background(), WorkspaceConfig{}); !errors.Is(err, ErrNoBackend) { + t.Errorf("nil *Provisioner.Start: got %v, want ErrNoBackend", err) + } + }) + t.Run("nil_Provisioner.ExecRead", func(t *testing.T) { + var p *Provisioner + if _, err := p.ExecRead(context.Background(), "any", "any"); !errors.Is(err, ErrNoBackend) { + t.Errorf("nil *Provisioner.ExecRead: got %v, want ErrNoBackend", err) + } + }) + t.Run("nil_Provisioner.RemoveVolume", func(t *testing.T) { + var p *Provisioner + if err := p.RemoveVolume(context.Background(), "any"); !errors.Is(err, ErrNoBackend) { + t.Errorf("nil *Provisioner.RemoveVolume: got %v, want ErrNoBackend", err) + } + }) + t.Run("nil_Provisioner.VolumeHasFile", func(t *testing.T) { + var p *Provisioner + if _, err := p.VolumeHasFile(context.Background(), "any", "config.yaml"); !errors.Is(err, ErrNoBackend) { + t.Errorf("nil *Provisioner.VolumeHasFile: got %v, want ErrNoBackend", err) + } + }) + t.Run("nil_Provisioner.WriteAuthTokenToVolume", func(t *testing.T) { + var p *Provisioner + if err := p.WriteAuthTokenToVolume(context.Background(), "any", "tok"); !errors.Is(err, ErrNoBackend) { + t.Errorf("nil *Provisioner.WriteAuthTokenToVolume: got %v, want ErrNoBackend", err) + } + }) } diff --git a/workspace-server/internal/provisioner/local_provisioner_api.go b/workspace-server/internal/provisioner/local_provisioner_api.go new file mode 100644 index 00000000..51996c93 --- /dev/null +++ b/workspace-server/internal/provisioner/local_provisioner_api.go @@ -0,0 +1,36 @@ +package provisioner + +import "context" + +// LocalProvisionerAPI is the contract WorkspaceHandler uses to talk to the +// local Docker provisioner. Mirrors CPProvisionerAPI so the handler types +// both backends symmetrically — no more "interface for SaaS, concrete for +// Docker" asymmetry that blocked easy mocking and made future +// pluggable-backend work harder than it needed to be (issue #2369). +// +// Method set is the union of methods WorkspaceHandler actually calls +// today. Adding a new handler call site that reaches into Provisioner +// means widening this interface explicitly — the same review-surfacing +// contract CPProvisionerAPI already enforces. +// +// Keep narrow: the *Provisioner type also exposes ListManagedContainerIDPrefixes, +// CopyTemplateToContainer, DockerClient, etc. Those are consumed by +// non-handler call sites (orphan sweeper, registry health watcher) which +// type their dependency as `*Provisioner` directly. Pulling them onto +// this interface would force every handler test to stub method bodies +// they never exercise. +type LocalProvisionerAPI interface { + Start(ctx context.Context, cfg WorkspaceConfig) (string, error) + Stop(ctx context.Context, workspaceID string) error + IsRunning(ctx context.Context, workspaceID string) (bool, error) + ExecRead(ctx context.Context, containerName, filePath string) ([]byte, error) + RemoveVolume(ctx context.Context, workspaceID string) error + VolumeHasFile(ctx context.Context, workspaceID, relPath string) (bool, error) + WriteAuthTokenToVolume(ctx context.Context, workspaceID, token string) error +} + +// Compile-time assertion: *Provisioner satisfies LocalProvisionerAPI. +// Catches a future method-signature drift at build time instead of at +// the NewWorkspaceHandler call site. Mirror of the assertion in +// cp_provisioner.go for CPProvisionerAPI. +var _ LocalProvisionerAPI = (*Provisioner)(nil) diff --git a/workspace-server/internal/provisioner/provisioner.go b/workspace-server/internal/provisioner/provisioner.go index dd94b08d..1de342b1 100644 --- a/workspace-server/internal/provisioner/provisioner.go +++ b/workspace-server/internal/provisioner/provisioner.go @@ -272,6 +272,9 @@ func InternalURL(workspaceID string) string { // Start provisions and starts a workspace container. func (p *Provisioner) Start(ctx context.Context, cfg WorkspaceConfig) (string, error) { + if p == nil || p.cli == nil { + return "", ErrNoBackend + } name := ContainerName(cfg.WorkspaceID) configVolume := ConfigVolumeName(cfg.WorkspaceID) @@ -807,6 +810,9 @@ func (p *Provisioner) CopyToContainer(ctx context.Context, containerID, dstPath // ExecRead runs "cat " in an existing container and returns the output. // Used to read config files from a running container before stopping it. func (p *Provisioner) ExecRead(ctx context.Context, containerName, filePath string) ([]byte, error) { + if p == nil || p.cli == nil { + return nil, ErrNoBackend + } exec, err := p.cli.ContainerExecCreate(ctx, containerName, container.ExecOptions{ Cmd: []string{"cat", filePath}, AttachStdout: true, @@ -891,6 +897,9 @@ func (p *Provisioner) ReadFromVolume(ctx context.Context, volumeName, filePath s // Uses a throwaway alpine container to write directly to the named volume, // bypassing the container lifecycle entirely. func (p *Provisioner) WriteAuthTokenToVolume(ctx context.Context, workspaceID, token string) error { + if p == nil || p.cli == nil { + return ErrNoBackend + } volName := ConfigVolumeName(workspaceID) resp, err := p.cli.ContainerCreate(ctx, &container.Config{ Image: "alpine", @@ -936,6 +945,9 @@ func (p *Provisioner) execInContainer(ctx context.Context, containerID string, c // Also removes the claude-sessions volume (best-effort, may not exist // for non claude-code runtimes). Issue #12. func (p *Provisioner) RemoveVolume(ctx context.Context, workspaceID string) error { + if p == nil || p.cli == nil { + return ErrNoBackend + } volName := ConfigVolumeName(workspaceID) if err := p.cli.VolumeRemove(ctx, volName, true); err != nil { return fmt.Errorf("failed to remove volume %s: %w", volName, err) @@ -1139,6 +1151,9 @@ var ErrNoConfigSource = fmt.Errorf("no config.yaml source: template path missing // volume read-only. Returns (false, nil) if the file is absent and // (false, err) only on Docker-level failures. func (p *Provisioner) VolumeHasFile(ctx context.Context, workspaceID, relPath string) (bool, error) { + if p == nil || p.cli == nil { + return false, ErrNoBackend + } volName := ConfigVolumeName(workspaceID) // Confirm the volume exists first — Docker auto-creates on bind otherwise. if _, err := p.cli.VolumeInspect(ctx, volName); err != nil {