diff --git a/.gitea/workflows/ci.yml b/.gitea/workflows/ci.yml index 8438221b3..fb5305243 100644 --- a/.gitea/workflows/ci.yml +++ b/.gitea/workflows/ci.yml @@ -49,7 +49,7 @@ on: # `merge_group` (GitHub merge-queue trigger) dropped — Gitea has no merge # queue. The .github/ original retains it; this Gitea-side copy drops it. -# Cancel in-progress CI runs when a new commit arrives on the same ref. +# Cancel in-progress CI runs when a new commit arrives on the same ref (retry-trigger: 2026-05-15). # Stale runs queue up otherwise. PR refs and main/staging refs each get # their own group because github.ref differs. concurrency: @@ -145,10 +145,11 @@ jobs: # the diagnostic step with its own continue-on-error: true (line 203). # Flip confirmed by CI / Platform (Go) status = success on main HEAD 363905d3. continue-on-error: false - # Job-level ceiling. The go test step below runs with a per-step 10m timeout; - # this cap catches any step that leaks past that. Set well above 10m so - # the per-step timeout is the active constraint. - timeout-minutes: 15 + # Job-level ceiling. The go test step below runs with a per-step 70m timeout; + # this cap catches any step that leaks past that. Set well above 70m so + # the per-step timeout is the active constraint. Raised to 75m + # to account for golangci-lint ~17m + test suite ~20-30m on cold runner (mc#1099). + timeout-minutes: 75 defaults: run: working-directory: workspace-server @@ -172,16 +173,22 @@ jobs: - if: always() name: Install golangci-lint run: go install github.com/golangci/golangci-lint/v2/cmd/golangci-lint@v2.12.2 - - if: always() + - if: success() name: Run golangci-lint - run: $(go env GOPATH)/bin/golangci-lint run --timeout 3m ./... - - if: always() - name: Diagnostic — per-package verbose 60s + # mc#1099: --no-config bypasses .golangci.yaml ceiling; --timeout 30m + # is the active constraint. Cold runner: fetch-depth:0 clone (5-10m) + Go + # toolchain (5-10m) + mod download (2-5m) + build + vet + install lint + # (5m) = ~15-20m before linting even starts. 30m gives headroom. + run: $(go env GOPATH)/bin/golangci-lint run --no-config --timeout 30m ./... + - if: success() + name: Diagnostic — per-package verbose 600s + # mc#1099: step-level ceiling above the 600s Go timeout for cold-runner headroom. + timeout-minutes: 20 run: | set +e - go test -race -v -timeout 60s ./internal/handlers/... 2>&1 | tee /tmp/test-handlers.log + go test -race -v -timeout 600s ./internal/handlers/... 2>&1 | tee /tmp/test-handlers.log handlers_exit=$? - go test -race -v -timeout 60s ./internal/pendinguploads/... 2>&1 | tee /tmp/test-pu.log + go test -race -v -timeout 600s ./internal/pendinguploads/... 2>&1 | tee /tmp/test-pu.log pu_exit=$? echo "::group::handlers exit=$handlers_exit (last 100 lines)" tail -100 /tmp/test-handlers.log @@ -193,11 +200,12 @@ jobs: continue-on-error: true - if: always() name: Run tests with race detection and coverage - # Explicit timeout: cold runner cache causes OOM kills at ~4m39s on the - # full ./... suite with race detection + coverage. A 10m per-step timeout - # lets the suite complete on cold cache (~5-7m) while failing cleanly - # instead of OOM-killing. The job-level timeout (15m) is a backstop. - run: go test -race -timeout 10m -coverprofile=coverage.out ./... + # mc#1099: cold runner (~5-20m) + race detector (3-5x overhead) can push + # the suite past 10m. Per-step ceiling must exceed Go-level timeout so + # Go's timeout fires first (clean interrupt) rather than the step ceiling + # (SIGKILL). Job-level ceiling (75m) is the outer backstop. + timeout-minutes: 70 + run: go test -race -timeout 60m -coverprofile=coverage.out ./... - if: always() name: Per-file coverage report diff --git a/workspace-server/internal/handlers/activity_test.go b/workspace-server/internal/handlers/activity_test.go index ffb93d701..4013e13b1 100644 --- a/workspace-server/internal/handlers/activity_test.go +++ b/workspace-server/internal/handlers/activity_test.go @@ -63,6 +63,35 @@ func TestSessionSearchReturnsActivityAndMemory(t *testing.T) { } } +func TestSessionSearch_DBError(t *testing.T) { + mock := setupTestDB(t) + setupTestRedis(t) + broadcaster := newTestBroadcaster() + handler := NewActivityHandler(broadcaster) + + // Simulate a DB query failure — handler must return 500. + // q=x ensures the ILIKE filter is included, so the handler passes 3 args: + // [workspaceID, "%x%", limit] instead of just [workspaceID, limit]. + mock.ExpectQuery("WITH session_items AS"). + WithArgs("ws-123", "%x%", 50). + WillReturnError(context.DeadlineExceeded) + + w := httptest.NewRecorder() + c, _ := gin.CreateTestContext(w) + c.Request = httptest.NewRequest("GET", "/workspaces/ws-123/session-search?q=x", nil) + c.Request.Header.Set("Content-Type", "application/json") + c.Params = gin.Params{{Key: "id", Value: "ws-123"}} + + handler.SessionSearch(c) + + if w.Code != http.StatusInternalServerError { + t.Errorf("expected 500, got %d: %s", w.Code, w.Body.String()) + } + if err := mock.ExpectationsWereMet(); err != nil { + t.Errorf("unmet sqlmock expectations: %v", err) + } +} + // ---------- Activity List source filter ---------- func TestActivityList_SourceCanvas(t *testing.T) { diff --git a/workspace-server/internal/handlers/delegation_test.go b/workspace-server/internal/handlers/delegation_test.go index fcd17eec8..ea39f3a23 100644 --- a/workspace-server/internal/handlers/delegation_test.go +++ b/workspace-server/internal/handlers/delegation_test.go @@ -523,6 +523,42 @@ func TestDelegationRecord_InsertsActivityLogRow(t *testing.T) { } } +func TestDelegationRecord_DBInsertFails(t *testing.T) { + mock := setupTestDB(t) + setupTestRedis(t) + broadcaster := newTestBroadcaster() + wh := NewWorkspaceHandler(broadcaster, nil, "http://localhost:8080", t.TempDir()) + h := NewDelegationHandler(wh, broadcaster) + + // activity_logs INSERT fails — handler must return 500. + mock.ExpectExec("INSERT INTO activity_logs"). + WithArgs( + "550e8400-e29b-41d4-a716-446655440000", // workspace_id + "550e8400-e29b-41d4-a716-446655440000", // source_id + "550e8400-e29b-41d4-a716-446655440001", // target_id + sqlmock.AnyArg(), // summary + sqlmock.AnyArg(), // request_body (jsonb) + sqlmock.AnyArg(), // response_body (jsonb) + ). + WillReturnError(context.DeadlineExceeded) + + w := httptest.NewRecorder() + c, _ := gin.CreateTestContext(w) + c.Params = gin.Params{{Key: "id", Value: "550e8400-e29b-41d4-a716-446655440000"}} + body := `{"target_id":"550e8400-e29b-41d4-a716-446655440001","task":"hello","delegation_id":"del-xyz"}` + c.Request = httptest.NewRequest("POST", "/delegations/record", bytes.NewBufferString(body)) + c.Request.Header.Set("Content-Type", "application/json") + + h.Record(c) + + if w.Code != http.StatusInternalServerError { + t.Errorf("expected 500, got %d: %s", w.Code, w.Body.String()) + } + if err := mock.ExpectationsWereMet(); err != nil { + t.Errorf("unmet expectations: %v", err) + } +} + func TestDelegationRecord_RejectsInvalidUUID(t *testing.T) { setupTestDB(t) setupTestRedis(t) diff --git a/workspace-server/internal/handlers/handlers_test.go b/workspace-server/internal/handlers/handlers_test.go index 958858f02..22ef02ecd 100644 --- a/workspace-server/internal/handlers/handlers_test.go +++ b/workspace-server/internal/handlers/handlers_test.go @@ -79,14 +79,18 @@ func newTestBroadcaster() *events.Broadcaster { // for the duration of the test, so httptest.NewServer's loopback URLs // don't trip the SSRF guard. The 169.254 metadata, RFC-1918, TEST-NET, // CGNAT, and link-local guards stay active — only 127.0.0.0/8 and ::1 -// are relaxed. Always paired with t.Cleanup to restore; multiple -// parallel tests won't race because Go test flips it sequentially per -// test unless t.Parallel() is used, and these tests don't parallelize. +// are relaxed. Protected by loopbackMu so concurrent tests don't race. func allowLoopbackForTest(t *testing.T) { t.Helper() + loopbackMu.Lock() prev := testAllowLoopback testAllowLoopback = true - t.Cleanup(func() { testAllowLoopback = prev }) + t.Cleanup(func() { + loopbackMu.Lock() + defer loopbackMu.Unlock() + testAllowLoopback = prev + }) + loopbackMu.Unlock() } // expectBudgetCheck adds the sqlmock expectation for the budget-check diff --git a/workspace-server/internal/handlers/ssrf.go b/workspace-server/internal/handlers/ssrf.go index 2e795e900..56448cb89 100644 --- a/workspace-server/internal/handlers/ssrf.go +++ b/workspace-server/internal/handlers/ssrf.go @@ -7,6 +7,7 @@ import ( "os" "path/filepath" "strings" + "sync" ) // devModeAllowsLoopback reports whether the SSRF defence should permit @@ -35,13 +36,20 @@ func devModeAllowsLoopback() bool { // loopback URLs and fake hostnames (*.example) don't trigger SSRF // rejections. Production code never mutates this. var ssrfCheckEnabled = true +var ssrfMu sync.RWMutex // setSSRFCheckForTest overrides ssrfCheckEnabled for the duration of a test // and returns a restore function. Use with defer in *_test.go only. func setSSRFCheckForTest(enabled bool) func() { + ssrfMu.Lock() + defer ssrfMu.Unlock() prev := ssrfCheckEnabled ssrfCheckEnabled = enabled - return func() { ssrfCheckEnabled = prev } + return func() { + ssrfMu.Lock() + defer ssrfMu.Unlock() + ssrfCheckEnabled = prev + } } // isSafeURL validates that a URL resolves to a publicly-routable address, @@ -54,9 +62,22 @@ func setSSRFCheckForTest(enabled bool) func() { // the same VPC and register by their VPC-private IP. Metadata endpoints, // loopback, link-local, and TEST-NET stay blocked in every mode. func isSafeURL(rawURL string) error { - if !ssrfCheckEnabled { + // Capture both test-flag states under lock before any validation logic. + // Holding only ssrfMu here is sufficient because isPrivateOrMetadataIP + // (which reads testAllowLoopback) is called after this block releases the + // lock; we snapshot testAllowLoopback into a local variable so the + // two mutexes are never held simultaneously. + ssrfMu.RLock() + enabled := ssrfCheckEnabled + ssrfMu.RUnlock() + if !enabled { return nil } + + loopbackMu.RLock() + allowLoopback := testAllowLoopback + loopbackMu.RUnlock() + u, err := url.Parse(rawURL) if err != nil { return fmt.Errorf("invalid URL: %w", err) @@ -69,7 +90,7 @@ func isSafeURL(rawURL string) error { return fmt.Errorf("empty hostname") } if ip := net.ParseIP(host); ip != nil { - if (ip.IsLoopback() && !testAllowLoopback && !devModeAllowsLoopback()) || ip.IsUnspecified() || ip.IsLinkLocalUnicast() || ip.IsLinkLocalMulticast() || ip.IsInterfaceLocalMulticast() { + if (ip.IsLoopback() && !allowLoopback && !devModeAllowsLoopback()) || ip.IsUnspecified() || ip.IsLinkLocalUnicast() || ip.IsLinkLocalMulticast() || ip.IsInterfaceLocalMulticast() { return fmt.Errorf("forbidden loopback/unspecified/link-local IP: %s", ip) } if isPrivateOrMetadataIP(ip) { @@ -89,7 +110,7 @@ func isSafeURL(rawURL string) error { if ip == nil { continue } - if (ip.IsLoopback() && !testAllowLoopback && !devModeAllowsLoopback()) || ip.IsUnspecified() || ip.IsLinkLocalUnicast() || ip.IsLinkLocalMulticast() || ip.IsInterfaceLocalMulticast() { + if (ip.IsLoopback() && !allowLoopback && !devModeAllowsLoopback()) || ip.IsUnspecified() || ip.IsLinkLocalUnicast() || ip.IsLinkLocalMulticast() || ip.IsInterfaceLocalMulticast() { return fmt.Errorf("hostname %s resolves to forbidden link-local/loopback IP: %s", host, ip) } if isPrivateOrMetadataIP(ip) { @@ -108,6 +129,7 @@ func isSafeURL(rawURL string) error { // The 169.254 metadata, RFC-1918, TEST-NET, CGNAT, and link-local // guards are NOT relaxed by this flag — only loopback. var testAllowLoopback = false +var loopbackMu sync.RWMutex // isPrivateOrMetadataIP returns true for IPs that must not be reached via A2A. // @@ -167,7 +189,10 @@ func isPrivateOrMetadataIP(ip net.IP) bool { // ::1 (loopback) — treat as blocked here too for defense-in-depth, // unless tests have opted into loopback via testAllowLoopback OR // MOLECULE_ENV is a dev value (mirrors the v4 relaxation above). - if ip.IsLoopback() && !testAllowLoopback && !devModeAllowsLoopback() { + loopbackMu.RLock() + allowLB := testAllowLoopback + loopbackMu.RUnlock() + if ip.IsLoopback() && !allowLB && !devModeAllowsLoopback() { return true } // Link-local fe80::/10 — always blocked.