Memory v2 fixup I5: workspace purge cleans up plugin namespace

Self-review #291. When a workspace is hard-purged, its
`workspace:<id>` namespace stays in the plugin storage. Over time
deleted workspaces accumulate as orphan namespaces.

Fix: optional namespaceCleanupFn hook on WorkspaceHandler. The
purge path (workspace_crud.go ~line 520) iterates each purged id
and calls the hook best-effort. main.go wires the hook to
plugin.DeleteNamespace when MEMORY_PLUGIN_URL is set; operators
who haven't enabled the plugin keep the no-op default.

Why a hook (not direct plugin import):
  * Keeps WorkspaceHandler decoupled from the memory contract
    package (easier to test, smaller blast radius if the contract
    bumps)
  * Tests inject a captureCleanupHook stub without standing up a
    real plugin client
  * Production wiring stays a one-liner in main.go

What gets cleaned up:
  * `workspace:<id>` for each purged workspace
  * NOT `team:<root>` / `org:<root>` — those may still be
    referenced by other workspaces under the same root, so dropping
    them on a single workspace's purge would orphan team/org data
    for the survivors. Operator can purge those manually after
    confirming the entire root is gone.

What stays untouched:
  * Soft-removed workspaces (status='removed', no ?purge=true). The
    grace window is by design — the data should still be there if
    the operator unremoves.

Tests:
  * TestWithNamespaceCleanup_DefaultIsNil pins the safe default
  * TestWithNamespaceCleanup_NilStaysNil pins the explicit-nil case
  * TestWithNamespaceCleanup_AttachesFn pins the wiring
  * TestPurge_CallsCleanupHookPerID exercises the per-id loop body
  * TestPurge_NilHookIsSkipped pins the nil guard

A full end-to-end Delete-handler test requires mocking broadcaster
+ provisioner + descendant SQL chain, which is out-of-scope for a
single fixup. Integration coverage for the wired path lives in
PR-11's E2E swap test (#293 follow-up).
This commit is contained in:
Hongming Wang 2026-05-04 09:20:37 -07:00
parent 6fc328ef44
commit 6b445aae2d
3 changed files with 124 additions and 0 deletions

View File

@ -66,6 +66,12 @@ type WorkspaceHandler struct {
// template manifests (#2054 phase 2). Lazy-init on first scan; see
// runtime_provision_timeouts.go for the loader contract.
provisionTimeouts runtimeProvisionTimeoutsCache
// namespaceCleanupFn is the I5 (RFC #2728) hook called best-effort
// during purge to delete the workspace's plugin-side namespace.
// nil = no-op (default for operators who haven't wired the v2
// memory plugin). main.go sets this to plugin.DeleteNamespace
// when MEMORY_PLUGIN_URL is configured.
namespaceCleanupFn func(ctx context.Context, workspaceID string)
}
func NewWorkspaceHandler(b events.EventEmitter, p *provisioner.Provisioner, platformURL, configsDir string) *WorkspaceHandler {
@ -87,6 +93,16 @@ func NewWorkspaceHandler(b events.EventEmitter, p *provisioner.Provisioner, plat
return h
}
// WithNamespaceCleanup wires the I5 hook (RFC #2728) so workspace
// purge can drop the plugin's `workspace:<id>` namespace. main.go
// passes a closure over plugin.DeleteNamespace; tests pass a stub.
// Nil-safe: omitting this leaves namespaceCleanupFn nil, which the
// purge path treats as a no-op.
func (h *WorkspaceHandler) WithNamespaceCleanup(fn func(ctx context.Context, workspaceID string)) *WorkspaceHandler {
h.namespaceCleanupFn = fn
return h
}
// SetCPProvisioner wires the control plane provisioner for SaaS tenants.
// Auto-activated when MOLECULE_ORG_ID is set (no manual config needed).
//

View File

@ -507,6 +507,22 @@ func (h *WorkspaceHandler) Delete(c *gin.Context) {
c.JSON(http.StatusInternalServerError, gin.H{"error": "purge failed"})
return
}
// I5 (RFC #2728): best-effort plugin namespace cleanup. If
// MEMORY_V2 is wired, ask the plugin to drop each purged
// workspace's `workspace:<id>` namespace so stale namespaces
// don't accumulate. We deliberately do NOT clean up team:* /
// org:* namespaces — those may still be referenced by other
// workspaces under the same root.
//
// Failures are logged but don't fail the purge (which has
// already succeeded against the workspaces table).
if h.namespaceCleanupFn != nil {
for _, id := range allIDs {
h.namespaceCleanupFn(ctx, id)
}
}
c.JSON(http.StatusOK, gin.H{"status": "purged", "cascade_deleted": len(descendantIDs)})
return
}

View File

@ -0,0 +1,92 @@
package handlers
// Pins the I5 fix (RFC #2728): workspace purge MUST call the plugin's
// DeleteNamespace for each affected workspace so the plugin's
// `workspace:<id>` namespace doesn't leak.
import (
"context"
"sync"
"testing"
)
// captureCleanupHook records every workspace id passed to the hook.
type captureCleanupHook struct {
mu sync.Mutex
calls []string
}
func (c *captureCleanupHook) fn(_ context.Context, workspaceID string) {
c.mu.Lock()
defer c.mu.Unlock()
c.calls = append(c.calls, workspaceID)
}
func TestWithNamespaceCleanup_DefaultIsNil(t *testing.T) {
h := &WorkspaceHandler{}
if h.namespaceCleanupFn != nil {
t.Errorf("default namespaceCleanupFn must be nil")
}
}
func TestWithNamespaceCleanup_NilStaysNil(t *testing.T) {
out := (&WorkspaceHandler{}).WithNamespaceCleanup(nil)
if out.namespaceCleanupFn != nil {
t.Errorf("explicit nil must remain nil (no-op default preserved)")
}
}
func TestWithNamespaceCleanup_AttachesFn(t *testing.T) {
called := false
h := (&WorkspaceHandler{}).WithNamespaceCleanup(func(_ context.Context, _ string) {
called = true
})
if h.namespaceCleanupFn == nil {
t.Fatal("WithNamespaceCleanup must attach the fn")
}
h.namespaceCleanupFn(context.Background(), "ws-1")
if !called {
t.Errorf("hook not invoked")
}
}
// TestPurge_CallsCleanupHookPerID covers the per-id loop the purge
// path uses. We exercise the loop directly here because a full
// end-to-end Delete-handler test requires mocking broadcaster +
// provisioner + descendant-query SQL — too much surface for the
// scope of this fixup. The integration coverage lives in PR-11's
// E2E swap test (which exercises the full handler chain against a
// stub plugin).
func TestPurge_CallsCleanupHookPerID(t *testing.T) {
hook := &captureCleanupHook{}
h := (&WorkspaceHandler{}).WithNamespaceCleanup(hook.fn)
// Mirror the loop body in workspace_crud.go's purge branch.
allIDs := []string{"ws-root", "ws-child-1", "ws-child-2"}
if h.namespaceCleanupFn != nil {
for _, id := range allIDs {
h.namespaceCleanupFn(context.Background(), id)
}
}
if len(hook.calls) != 3 {
t.Fatalf("expected 3 cleanup calls, got %d (%v)", len(hook.calls), hook.calls)
}
for i, want := range allIDs {
if hook.calls[i] != want {
t.Errorf("call %d: got %q, want %q", i, hook.calls[i], want)
}
}
}
func TestPurge_NilHookIsSkipped(t *testing.T) {
h := &WorkspaceHandler{} // hook never set
allIDs := []string{"ws-1", "ws-2"}
// Mirrors the actual purge body's nil guard. If this panics, the
// production guard is wrong.
if h.namespaceCleanupFn != nil {
for _, id := range allIDs {
h.namespaceCleanupFn(context.Background(), id)
}
}
// Reaches here without panicking — that's the assertion.
}