Merge pull request #2927 from Molecule-AI/fix/org-import-polish-2872

fix(org-import): polish — wrap-safe ErrNoRows, bounded lookup, godoc (#2872)

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 {

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