Skip to content

Commit 8acb506

Browse files
Dumbrisclaude
andauthored
fix(ci): stop the Windows recovered-panic heap crash and the Pass-2 temp-dir race (#1288)
* fix(httpapi): stop the recovered nil-config panic in add-from-registry A controller that reports success without a server config (the test doubles do) was dereferenced in handleAddFromRegistry; chi's recoverer turned the fault into a bare 500. Under Go 1.26 on windows/amd64 the recovered hardware exception corrupts the Go heap (golang/go#81238), so the internal/httpapi test binary died later with "found pointer to free object" / "unexpected fault address" on unrelated PRs (#1282, #1285). Return a JSON 500 instead, the way redactedRegistrySummary already tolerates a nil entry, and pin it with a test. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(scanner): wait for the Pass-2 job before tearing down temp dirs TestServiceStartScanDeepOnRunsSourceResolutionAndPass2 failed on ubuntu CI with "TempDir RemoveAll cleanup: directory not empty": the Pass-2 goroutine writes tools.json into the server working dir after ResolveFullSource returns and before it registers its engine job, so an idle engine can mean "Pass 1 cleared, Pass 2 not started yet" and the test's teardown raced the write. Wait for the Pass-2 job to reach a terminal status in storage (its last write on every exit path) first. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
1 parent b1777e8 commit 8acb506

3 files changed

Lines changed: 56 additions & 2 deletions

File tree

‎internal/httpapi/registry_add_test.go‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -54,3 +54,21 @@ func TestAddFromRegistry_SlashServerIDUnescaped(t *testing.T) {
5454
assert.Equal(t, "microsoft/markitdown", controller.gotServerID, "serverId must be percent-decoded before registry lookup")
5555
assert.Equal(t, "github-mcp", controller.gotRegistryID, "registry id must be percent-decoded before lookup")
5656
}
57+
58+
// TestAddFromRegistry_NilConfigIsAnError pins the nil-tolerance of the success
59+
// branch: a controller that returns no config and no error yields a JSON 500,
60+
// never a recovered nil-pointer panic (which corrupts the heap on
61+
// windows/amd64 under Go 1.26 — golang/go#81238 — and took the whole test
62+
// binary down with it).
63+
func TestAddFromRegistry_NilConfigIsAnError(t *testing.T) {
64+
ctrl := &adminConfigController{ServerController: &MockServerController{}, apiKey: "admin-secret"}
65+
srv := NewServer(ctrl, zaptest.NewLogger(t).Sugar(), nil)
66+
67+
req := httptest.NewRequest(http.MethodPost, "/api/v1/registries/reg1/servers/srv1/add", nil)
68+
req.Header.Set("X-API-Key", "admin-secret")
69+
w := httptest.NewRecorder()
70+
srv.ServeHTTP(w, req)
71+
72+
assert.Equal(t, http.StatusInternalServerError, w.Code)
73+
assert.Contains(t, w.Body.String(), "no server configuration")
74+
}

‎internal/httpapi/server.go‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5836,6 +5836,17 @@ func (s *Server) handleAddFromRegistry(w http.ResponseWriter, r *http.Request) {
58365836
s.writeRegistryAddError(w, r, status, rerr)
58375837
return
58385838
}
5839+
if cfg == nil {
5840+
// A controller that reports success without a server config (the
5841+
// test doubles do) used to be dereferenced here; chi's recoverer
5842+
// turned that into a bare 500 — and on windows/amd64 the recovered
5843+
// hardware fault corrupts the Go heap under Go 1.26 (golang/go#81238),
5844+
// so the httpapi test binary then died in a later GC. Same
5845+
// nil-tolerance as redactedRegistrySummary.
5846+
logger.Errorw("Add from registry returned no server config", "registry", registryID, "server", serverID)
5847+
s.writeError(w, r, http.StatusInternalServerError, "registry returned no server configuration")
5848+
return
5849+
}
58395850

58405851
// Issue #1148, round 8: the MCP twin of this handler
58415852
// (`upstream_servers add_from_registry`) has sourced this echo from the

‎internal/security/scanner/service_test.go‎

Lines changed: 27 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2505,11 +2505,36 @@ func TestServiceStartScanDeepOnRunsSourceResolutionAndPass2(t *testing.T) {
25052505
t.Errorf("deep scan on: Pass 2 (ResolveFullSource) must run, got %d call(s)", got)
25062506
}
25072507

2508-
// Drain the engine so the background Pass-2 goroutine finishes before the
2509-
// test tears down its temp dirs (keeps -race teardown quiet).
2508+
// Drain Pass 2 before the test tears down its temp dirs. Waiting for the
2509+
// engine to go idle is not enough: the Pass-2 goroutine writes tools.json
2510+
// into workDir (exportToolDefinitions) AFTER ResolveFullSource returns and
2511+
// BEFORE it registers its job with the engine, so an idle engine can mean
2512+
// "Pass 1 cleared, Pass 2 not yet started" — and TempDir's RemoveAll then
2513+
// raced the write ("directory not empty", flaky on ubuntu CI). The Pass-2
2514+
// job is saved to storage on every exit path (completed, or the failed
2515+
// placeholder), so that is the terminal signal.
2516+
waitForPass2Saved(t, store, "srv-on")
25102517
waitForScanIdle(t, svc, "srv-on")
25112518
}
25122519

2520+
// waitForPass2Saved polls storage until a Pass-2 job for server has reached a
2521+
// terminal status — the goroutine's last write, on every path startPass2 can
2522+
// take.
2523+
func waitForPass2Saved(t *testing.T, store *mockStorage, server string) {
2524+
t.Helper()
2525+
deadline := time.Now().Add(10 * time.Second)
2526+
for time.Now().Before(deadline) {
2527+
jobs, _ := store.ListScanJobs(server)
2528+
for _, j := range jobs {
2529+
if j.ScanPass == ScanPassSupplyChainAudit && j.Status != ScanJobStatusRunning && j.Status != ScanJobStatusPending {
2530+
return
2531+
}
2532+
}
2533+
time.Sleep(20 * time.Millisecond)
2534+
}
2535+
t.Fatalf("Pass 2 for %s did not reach a terminal status in time", server)
2536+
}
2537+
25132538
// TestApplySecurityConfigDefaultConfigGatesDeepScanOff locks the audit's FIX-1
25142539
// invariant against the real default config: config.DefaultConfig() never
25152540
// initializes Config.Security (it stays nil), and the server wiring passes

0 commit comments

Comments
 (0)