From 423d58d42c83c482816052abb1763eedd12bbb9a Mon Sep 17 00:00:00 2001 From: Hongming Wang Date: Tue, 5 May 2026 13:20:54 -0700 Subject: [PATCH] =?UTF-8?q?fix(org-import):=20polish=20=E2=80=94=20wrap-sa?= =?UTF-8?q?fe=20ErrNoRows,=20bounded=20lookup,=20godoc?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three small hardening passes from #2872's optional/important findings, batched into one polish PR: 1. errors.Is(err, sql.ErrNoRows) instead of err == sql.ErrNoRows. The bare equality breaks if any future caller wraps the error via fmt.Errorf("…: %w", err) — the no-rows happy path would fall through to the "real DB error" branch and abort the import. errors.Is unwraps. New test TestLookupExistingChild_WrappedNoRows_TreatedAsNotFound pins the fix; verified the test fails on the old `==` shape (build break on unused-import + assertion failure once import dropped). 2. Bounded 5s timeout on lookupExistingChild instead of context.Background(). The createWorkspaceTree call site runs in goroutines spawned from the /org/import handler, so plumbing the request context here would cascade-cancel into provisionWorkspaceAuto and abort in-flight EC2 provisioning if the client disconnected mid-import — that's the wrong tradeoff. A short bounded timeout protects the per-row SELECT against a wedged DB without taking the drop-everything-on-disconnect behaviour. The lookup is a single ~10ms query; 5s leaves 500x headroom for transient slow paths. 3. Godoc clarifications on the skip-path block. - /org/import is ADDITIVE-ONLY, never destructive. Children present in the existing tree but absent from the new template are preserved (no DELETE on diff). - Skip-path does NOT propagate updates to existing nodes — a re-import that adds an initial_memory or schedule to an existing workspace is silently dropped. Document the limitation so future operators know to delete-and-re-import or reach for a future /org/sync route. Verification: - go build ./... → clean - go test ./internal/handlers/... → all passing (TestLookup* + TestCreateWorkspaceTree* + TestClass1* + TestGate*) - 4 lookup tests + 1 new wrap-safety test → 5/5 PASS - Full handlers suite → green Refs molecule-core#2872 (Optional findings — wrap-safety + ctx, godoc clarifications for additive-only + skip-path-update-limitation) Out of scope (deferred): - PR-D partial unique index migration + ON CONFLICT — sequenced after Phase 4 cleanup verified clean per #2872 plan - PR-E full createWorkspaceTree integration test for partial-match — needs heavier sqlmock scaffolding for downstream workspaces_audit/canvas_layouts/secrets/channels INSERTs; follow-up Co-Authored-By: Claude Opus 4.7 (1M context) --- .../internal/handlers/org_import.go | 29 +++++++++++++++-- .../handlers/org_import_idempotency_test.go | 32 +++++++++++++++++++ 2 files changed, 59 insertions(+), 2 deletions(-) diff --git a/workspace-server/internal/handlers/org_import.go b/workspace-server/internal/handlers/org_import.go index 639c8ba9..3dfe2fbd 100644 --- a/workspace-server/internal/handlers/org_import.go +++ b/workspace-server/internal/handlers/org_import.go @@ -7,6 +7,7 @@ import ( "context" "database/sql" "encoding/json" + "errors" "fmt" "log" "os" @@ -79,7 +80,16 @@ func (h *OrgHandler) createWorkspaceTree(ws OrgWorkspace, parentID *string, absX } } - ctxLookup := context.Background() + // 5s timeout bounds the lookup independently of any HTTP request + // context. createWorkspaceTree runs in goroutines spawned from the + // /org/import handler, so plumbing the request context here would + // cascade-cancel into provisionWorkspaceAuto and abort in-flight + // EC2 provisioning if the client disconnected mid-import — that's + // the wrong behaviour. A short bounded timeout protects the + // per-row SELECT against a wedged DB without taking the + // drop-everything-on-disconnect tradeoff. + ctxLookup, cancelLookup := context.WithTimeout(context.Background(), 5*time.Second) + defer cancelLookup() // Idempotency: if a workspace with the same (parent_id, name) already // exists, skip the INSERT + canvas_layouts + broadcast + provisioning. // This is what makes /org/import safe to call multiple times — the @@ -91,6 +101,15 @@ func (h *OrgHandler) createWorkspaceTree(ws OrgWorkspace, parentID *string, absX // (parent exists, some children missing) backfill the missing children // instead of either no-op'ing the whole subtree or duplicating the // existing children. + // + // /org/import is ADDITIVE-ONLY, never destructive. Children present + // in the existing tree but absent from the new template are + // preserved (no DELETE on diff). Skip-path also does NOT propagate + // updates to existing nodes — a re-import that adds an + // initial_memory or schedule to an existing workspace is silently + // dropped (the function bypasses seedInitialMemories, schedule SQL, + // channel config for skipped rows). To force-update an existing + // tree, delete and re-import or use a future /org/sync route. existingID, existing, lookupErr := h.lookupExistingChild(ctxLookup, ws.Name, parentID) if lookupErr != nil { return fmt.Errorf("idempotency check for %s: %w", ws.Name, lookupErr) @@ -605,6 +624,12 @@ func (h *OrgHandler) createWorkspaceTree(ws OrgWorkspace, parentID *string, absX // // On sql.ErrNoRows: returns ("", false, nil) — caller should INSERT. // On a real DB error: returns ("", false, err) — caller propagates. +// +// errors.Is is wrap-safe — a future caller wrapping the error +// (database/sql can wrap driver errors with %w in some setups) would +// silently break a `err == sql.ErrNoRows` equality check, causing the +// no-rows path to fall through to the "real DB error" branch and +// abort the import. errors.Is unwraps. func (h *OrgHandler) lookupExistingChild(ctx context.Context, name string, parentID *string) (string, bool, error) { var existingID string err := db.DB.QueryRowContext(ctx, ` @@ -614,7 +639,7 @@ func (h *OrgHandler) lookupExistingChild(ctx context.Context, name string, paren AND status != 'removed' LIMIT 1 `, name, parentID).Scan(&existingID) - if err == sql.ErrNoRows { + if errors.Is(err, sql.ErrNoRows) { return "", false, nil } if err != nil { diff --git a/workspace-server/internal/handlers/org_import_idempotency_test.go b/workspace-server/internal/handlers/org_import_idempotency_test.go index cefc6e74..1f2955cb 100644 --- a/workspace-server/internal/handlers/org_import_idempotency_test.go +++ b/workspace-server/internal/handlers/org_import_idempotency_test.go @@ -2,7 +2,9 @@ package handlers import ( "context" + "database/sql" "errors" + "fmt" "go/ast" "go/parser" "go/token" @@ -123,6 +125,36 @@ func TestLookupExistingChild_DBError_Propagates(t *testing.T) { } } +// TestLookupExistingChild_WrappedNoRows_TreatedAsNotFound — pins the +// wrap-safety of the errors.Is(err, sql.ErrNoRows) check. The previous +// `err == sql.ErrNoRows` equality would fall through to the +// "real DB error" branch on a wrapped no-rows error, aborting the +// import for what is in fact the no-rows happy path. driver/sql +// wrapping is currently a non-issue but a future driver change or a +// caller that wraps the result via fmt.Errorf("…: %w", err) would +// silently break the equality check. errors.Is unwraps. +func TestLookupExistingChild_WrappedNoRows_TreatedAsNotFound(t *testing.T) { + mock := setupTestDB(t) + parent := "parent-1" + wrapped := fmt.Errorf("driver-wrapped: %w", sql.ErrNoRows) + mock.ExpectQuery(`SELECT id FROM workspaces`). + WithArgs("Alpha", &parent). + WillReturnError(wrapped) + + h := &OrgHandler{} + id, found, err := h.lookupExistingChild(context.Background(), "Alpha", &parent) + + if err != nil { + t.Fatalf("expected wrapped no-rows to be treated as not-found (err=nil), got: %v", err) + } + if found { + t.Errorf("expected found=false on wrapped no-rows, got found=true") + } + if id != "" { + t.Errorf("expected empty id on wrapped no-rows, got %q", id) + } +} + // workspacesInsertRE matches a SQL literal that begins (after optional // leading whitespace) with `INSERT INTO workspaces` followed by `(` — // requiring the open-paren rules out lookalikes like