Skip to content

refactor(handlers): widen WorkspaceHandler.provisioner to LocalProvisionerAPI interface (#2369) - #2390

Merged
hongmingwang-moleculeai merged 1 commit into
stagingfrom
auto/local-provisioner-api-interface
Apr 30, 2026
Merged

hongmingwang-moleculeai merged 1 commit into
stagingfrom
auto/local-provisioner-api-interface

Conversation

@HongmingWang-Rabbit

Copy link
Copy Markdown
Contributor

Closes #2369.

Why

`WorkspaceHandler` typed its CP-side via `provisioner.CPProvisionerAPI` (interface, mockable) but its Docker-side via `*provisioner.Provisioner` (concrete, harder to mock). This PR closes that asymmetry.

What

  • New `provisioner.LocalProvisionerAPI` interface — the 7 methods handlers actually call: `Start`, `Stop`, `IsRunning`, `ExecRead`, `RemoveVolume`, `VolumeHasFile`, `WriteAuthTokenToVolume`. Compile-time assertion that `*Provisioner` satisfies it. Mirror of `cp_provisioner.go`'s `CPProvisionerAPI` block.
  • `WorkspaceHandler.provisioner` and `TeamHandler.provisioner` re-typed to the interface. Constructor parameter type is unchanged — the assignment widens — so 200+ callers of `NewWorkspaceHandler`/`NewTeamHandler` are unaffected.
  • Constructors gain an `if p != nil { h.provisioner = p }` guard. Without this, passing `nil` (the test fixture pattern) yields a typed-nil interface where `h.provisioner != nil` evaluates as true — the classic Go "why is my nil error not nil" trap. Inline doc-comment cites the Go FAQ.
  • Hardened the 5 `Provisioner` methods that lacked nil-receiver guards (`Start`, `ExecRead`, `WriteAuthTokenToVolume`, `RemoveVolume`, `VolumeHasFile`). Each now returns `ErrNoBackend` on nil receiver instead of panicking. Symmetric with `Stop`/`IsRunning` (already hardened in [testing] Harden Provisioner.{Stop,IsRunning} against nil Docker/HTTP client (backends.md drift-risk #6) #1813). Defensive cleanup: even if a future caller bypasses the constructor's nil-elision, behavior degrades cleanly instead of panic.
  • Extended `TestZeroValuedBackends_NoPanic` with 5 new nil-receiver subtests for the newly-hardened methods. Defense-in-depth.

Drift gate

The compile-time assertion `var _ LocalProvisionerAPI = (*Provisioner)(nil)` blocks any future method-signature drift on `*Provisioner` that would silently break handlers — same protection `CPProvisionerAPI` has had since #1814.

Verification

  • `go build ./...` — clean
  • `go test ./...` — all 19 packages green (~1500 tests)
  • `go test ./internal/provisioner/ -run TestZeroValuedBackends_NoPanic -v` — 11 subtests pass (was 6 before; +5 for the new nil-receiver paths)
  • Hand-verified: `NewWorkspaceHandler(b, nil, ...)` produces a handler where `h.provisioner == nil` evaluates true (the typed-nil trap is sidestepped by constructor)

Test plan

  • Existing 200+ tests calling `NewWorkspaceHandler(..., nil, ...)` still pass without modification
  • New nil-receiver subtests pass for all 5 newly-hardened methods
  • Compile-time assertion catches drift if any method signature on `*Provisioner` changes
  • Build clean across the full server suite

🤖 Generated with Claude Code

…ionerAPI interface (#2369)

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) <noreply@anthropic.com>
Merged via the queue into staging with commit 904cf31 Apr 30, 2026
19 checks passed
HongmingWang-Rabbit pushed a commit that referenced this pull request Jun 12, 2026
… error + add registry-load-fail test (#2248 follow-up)

Researcher review RC 9329 on PR #2390 found two blockers:

1. org_import.go swallowed the new fail-closed config-generation error.
   In createWorkspaceTree, after the workspace DB row/layout/broadcast are
   already persisted, cfgErr only logged and continued. That left a silent
   stuck provisioning workspace during org import. Fixed by calling
   markProvisionFailed before continuing, so the canvas surfaces the failure
   card immediately.

2. Registry-unavailable/load-error fail-closed path was not covered.
   Converted providerRegistry from a named function to a variable function
   (mirrors the resolveInstanceID testability pattern) so tests can swap in
   a mock. Added TestDeriveProvider_RegistryLoadErrorFailClosed asserting
   that a registry load error blocks provisioning.

Scope still limited to workspace config/provisioning caller plumbing and tests.
HongmingWang-Rabbit pushed a commit that referenced this pull request Jun 12, 2026
deriveDefaultConfigProviderFromManifest references providers.Manifest,
providers.Provider, and providers.RuntimeNativeSet but workspace_provision.go
never imported the internal/providers package. This is a compile error
introduced in the #2248 follow-up branch.

Researcher review 9329 blocker #2.
HongmingWang-Rabbit pushed a commit that referenced this pull request Jun 12, 2026
Empty commit to re-run CI jobs that were absent/pending on the
previous infra outage. No code changes.
HongmingWang-Rabbit pushed a commit that referenced this pull request Jun 12, 2026
Serialized merge by gitea-merge-queue after current-main, genuine approvals, and required CI checks were green.
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.

Extract LocalProvisionerAPI interface to mirror CPProvisionerAPI (handler-side abstraction)

1 participant