Skip to content

Commit 9a1f3a4

Browse files
Fix Go FFI inprocess e2e CI failures
Resolve the remaining inprocess-cell failures in the go-sdk-tests matrix: - Anchor all repo-relative test paths to source dirs, not the process cwd. The in-process transport os.Chdir's the whole test process into a per-test temp workdir, so cwd-relative resolution of the replay proxy (server.ts) and MCP server scripts (*.mjs) broke for every test after the first in-process one. Add testharness.RepoPath and use it for the proxy, CLI, and MCP paths. - Fix a data race in ffihost.Host: `disposed` was written under disposeMu but read under callbackMu. Unify all disposed/serverID/connectionID/activeCallbacks access under a single mutex, which also fixes a real drain-ordering bug where a late onOutbound callback could feed the receive buffer after it was closed. - Re-arm foreign signal handlers with SA_ONSTACK on linux too (not just darwin). libuv's SA_ONSTACK-less SIGCHLD handler makes the Go runtime abort when the test process reaps its own os/exec child (e.g. an MCP OAuth server) while libnode is loaded. Resolve sigaction through the runtime handle (with a libc dlopen fallback). - Make the OAuth MCP server's stderr buffer thread-safe (syncBuffer): os/exec writes to it on a goroutine while the test reads String(), a -race violation. - Skip only the readCheckpoint "unknown checkpoint" scenario in-process, where the native runtime decodes the id as u32 and rejects the large sentinel, matching Rust's explicit skip. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: a96d1a82-c80e-40f1-a3df-a9708429b68b
1 parent 8bb0a99 commit 9a1f3a4

12 files changed

Lines changed: 213 additions & 59 deletions

‎go/internal/e2e/mcp_and_agents_e2e_test.go‎

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -115,10 +115,7 @@ func TestMCPServersE2E(t *testing.T) {
115115
t.Run("should pass literal env values to MCP server subprocess", func(t *testing.T) {
116116
ctx.ConfigureForTest(t)
117117

118-
mcpServerPath, err := filepath.Abs("../../../test/harness/test-mcp-server.mjs")
119-
if err != nil {
120-
t.Fatalf("Failed to resolve test-mcp-server path: %v", err)
121-
}
118+
mcpServerPath := testharness.RepoPath("test", "harness", "test-mcp-server.mjs")
122119
mcpServerDir := filepath.Dir(mcpServerPath)
123120

124121
mcpServers := map[string]copilot.MCPServerConfig{

‎go/internal/e2e/mcp_oauth_e2e_test.go‎

Lines changed: 22 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,6 @@ import (
66
"net/http"
77
"os"
88
"os/exec"
9-
"path/filepath"
109
"slices"
1110
"strings"
1211
"sync"
@@ -306,17 +305,14 @@ type oauthMCPRequest struct {
306305
func startOAuthMCPServer(t *testing.T) string {
307306
t.Helper()
308307

309-
serverPath, err := filepath.Abs("../../../test/harness/test-mcp-oauth-server.mjs")
310-
if err != nil {
311-
t.Fatalf("Failed to resolve OAuth MCP server path: %v", err)
312-
}
308+
serverPath := testharness.RepoPath("test", "harness", "test-mcp-oauth-server.mjs")
313309
cmd := exec.Command("node", serverPath)
314310
cmd.Env = append(os.Environ(), "EXPECTED_TOKEN="+expectedMCPOAuthToken)
315311
stdout, err := cmd.StdoutPipe()
316312
if err != nil {
317313
t.Fatalf("Failed to pipe OAuth MCP server stdout: %v", err)
318314
}
319-
var stderr strings.Builder
315+
var stderr syncBuffer
320316
cmd.Stderr = &stderr
321317
if err := cmd.Start(); err != nil {
322318
t.Fatalf("Failed to start OAuth MCP server: %v", err)
@@ -362,6 +358,26 @@ func stringValue(value *string) string {
362358
return *value
363359
}
364360

361+
// syncBuffer is a minimal io.Writer whose contents can be read concurrently.
362+
// os/exec writes to cmd.Stderr on a separate goroutine, so reading a plain
363+
// strings.Builder while the process is running is a data race (caught by -race).
364+
type syncBuffer struct {
365+
mu sync.Mutex
366+
buf strings.Builder
367+
}
368+
369+
func (b *syncBuffer) Write(p []byte) (int, error) {
370+
b.mu.Lock()
371+
defer b.mu.Unlock()
372+
return b.buf.Write(p)
373+
}
374+
375+
func (b *syncBuffer) String() string {
376+
b.mu.Lock()
377+
defer b.mu.Unlock()
378+
return b.buf.String()
379+
}
380+
365381
func fetchOAuthMCPRequests(t *testing.T, baseURL string) []oauthMCPRequest {
366382
t.Helper()
367383

‎go/internal/e2e/mcp_server_helpers_test.go‎

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -8,16 +8,14 @@ import (
88
"time"
99

1010
copilot "github.com/github/copilot-sdk/go"
11+
"github.com/github/copilot-sdk/go/internal/e2e/testharness"
1112
"github.com/github/copilot-sdk/go/rpc"
1213
)
1314

1415
func testMCPServers(t *testing.T, serverNames ...string) map[string]copilot.MCPServerConfig {
1516
t.Helper()
1617

17-
mcpServerPath, err := filepath.Abs("../../../test/harness/test-mcp-server.mjs")
18-
if err != nil {
19-
t.Fatalf("Failed to resolve test-mcp-server path: %v", err)
20-
}
18+
mcpServerPath := testharness.RepoPath("test", "harness", "test-mcp-server.mjs")
2119

2220
mcpServerDir := filepath.Dir(mcpServerPath)
2321
mcpServers := make(map[string]copilot.MCPServerConfig, len(serverNames))

‎go/internal/e2e/pre_mcp_tool_call_hook_e2e_test.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@ func TestPreMCPToolCallHookE2E(t *testing.T) {
1515
client := ctx.NewClient()
1616
t.Cleanup(func() { client.ForceStop() })
1717

18-
testHarnessDir, _ := filepath.Abs("../../../test/harness")
18+
testHarnessDir := testharness.RepoPath("test", "harness")
1919
metaEchoServer := filepath.Join(testHarnessDir, "test-mcp-meta-echo-server.mjs")
2020

2121
metaEchoConfig := func() map[string]copilot.MCPServerConfig {

‎go/internal/e2e/rpc_workspace_checkpoints_e2e_test.go‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,11 @@ func TestRPCWorkspaceCheckpointsE2E(t *testing.T) {
3333
})
3434

3535
t.Run("should return nil or empty content for unknown checkpoint", func(t *testing.T) {
36+
// In-process, session.workspaces.readCheckpoint is answered by the native
37+
// runtime, which decodes the checkpoint number as a u32 and rejects the
38+
// large sentinel this test uses. Covered by the default (stdio) transport.
39+
// Mirrors Rust's should_return_null_or_empty_content_for_unknown_checkpoint.
40+
testharness.SkipIfInProcess(t, "readCheckpoint decodes the id as u32 in-process")
3641
session := createWorkspaceRPCSession(t, client)
3742
defer session.Disconnect()
3843

‎go/internal/e2e/testharness/context.go‎

Lines changed: 5 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -33,13 +33,11 @@ func CLIPath() string {
3333
// 1.0.64-1 the @github/copilot package is a thin loader; the runnable
3434
// index.js ships in the installed platform package
3535
// (e.g. @github/copilot-linux-x64).
36-
base, err := filepath.Abs("../../../nodejs/node_modules/@github")
37-
if err == nil {
38-
matches, _ := filepath.Glob(filepath.Join(base, "copilot-*", "index.js"))
39-
if len(matches) > 0 {
40-
cliPath = matches[0]
41-
return
42-
}
36+
base := RepoPath("nodejs", "node_modules", "@github")
37+
matches, _ := filepath.Glob(filepath.Join(base, "copilot-*", "index.js"))
38+
if len(matches) > 0 {
39+
cliPath = matches[0]
40+
return
4341
}
4442
})
4543
return cliPath

‎go/internal/e2e/testharness/helper.go‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,11 +3,32 @@ package testharness
33
import (
44
"context"
55
"errors"
6+
"path/filepath"
7+
"runtime"
68
"time"
79

810
copilot "github.com/github/copilot-sdk/go"
911
)
1012

13+
// RepoPath resolves a path relative to the repository root, anchored to this
14+
// source file's directory rather than the process working directory. The
15+
// in-process (FFI) transport os.Chdir's the whole test process into a per-test
16+
// temp workdir (the shared runtime host inherits the process cwd), so any
17+
// cwd-relative resolution (e.g. filepath.Abs("../../../test/...")) would break
18+
// for every test after the first in-process one. This helper stays correct
19+
// regardless of the current working directory.
20+
func RepoPath(elem ...string) string {
21+
_, callerFile, _, ok := runtime.Caller(0)
22+
if !ok {
23+
// Fall back to a cwd-relative join; only correct before any chdir.
24+
return filepath.Join(append([]string{"..", "..", ".."}, elem...)...)
25+
}
26+
// This file lives at go/internal/e2e/testharness/, so the repo root is four
27+
// levels up from its directory.
28+
repoRoot := filepath.Join(filepath.Dir(callerFile), "..", "..", "..", "..")
29+
return filepath.Join(append([]string{repoRoot}, elem...)...)
30+
}
31+
1132
// GetFinalAssistantMessage waits for and returns the final assistant message from a session turn.
1233
// If alreadyIdle is true, skip waiting for session.idle (useful for resumed sessions where the
1334
// idle event was ephemeral and not persisted in the event history).

‎go/internal/e2e/testharness/proxy.go‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -38,8 +38,11 @@ func (p *CapiProxy) Start() (string, error) {
3838
return p.proxyURL, nil
3939
}
4040

41-
// The harness server is in the shared test directory
42-
serverPath := "../../../test/harness/server.ts"
41+
// The harness server is in the shared test directory. Anchor the path to
42+
// the repo root (not the process cwd), because the in-process (FFI)
43+
// transport os.Chdir's into a per-test temp workdir, which would otherwise
44+
// break the cwd-relative resolution.
45+
serverPath := RepoPath("test", "harness", "server.ts")
4346

4447
p.cmd = exec.Command("npx", "tsx", serverPath)
4548
p.cmd.Dir = "." // Will be resolved relative to test execution

‎go/internal/ffihost/ffihost.go‎

Lines changed: 35 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,7 @@ const symbolPrefix = "copilot_runtime_"
5353

5454
// ffiLibrary binds the copilot_runtime_* C ABI exports of a loaded cdylib.
5555
type ffiLibrary struct {
56+
handle uintptr
5657
hostStart func(argv unsafe.Pointer, argvLen uintptr, env unsafe.Pointer, envLen uintptr) uint32
5758
hostShutdown func(serverID uint32) bool
5859
connectionOpen func(serverID uint32, cb uintptr, userData uintptr, a unsafe.Pointer, aLen uintptr, b unsafe.Pointer, bLen uintptr, c unsafe.Pointer, cLen uintptr) uint32
@@ -95,7 +96,7 @@ func loadLibrary(libraryPath string) (lib *ffiLibrary, err error) {
9596
}
9697
}()
9798

98-
bound := &ffiLibrary{}
99+
bound := &ffiLibrary{handle: handle}
99100
purego.RegisterLibFunc(&bound.hostStart, handle, symbolPrefix+"host_start")
100101
purego.RegisterLibFunc(&bound.hostShutdown, handle, symbolPrefix+"host_shutdown")
101102
purego.RegisterLibFunc(&bound.connectionOpen, handle, symbolPrefix+"connection_open")
@@ -119,22 +120,24 @@ type Host struct {
119120
extraArgs []string
120121
lib *ffiLibrary
121122

123+
// mu guards disposed, activeCallbacks, serverID, and connectionID. A single
124+
// mutex is used so that publishing disposed=true and draining in-flight
125+
// callbacks are serialized against onOutbound's disposed-check/increment;
126+
// this prevents a callback from feeding the receive buffer after it is
127+
// closed (and avoids a data race on disposed observed by the -race detector).
128+
mu sync.Mutex
122129
serverID uint32
123130
connectionID uint32
131+
disposed bool
132+
// activeCallbacks counts outbound native callbacks currently executing.
133+
activeCallbacks int
124134

125135
recv *receiveBuffer
126136

127-
disposeMu sync.Mutex
128-
disposed bool
129-
130137
// callbackHandle is the purego callback trampoline address. purego keeps the
131138
// underlying Go closure alive for the process lifetime, so it (and this Host,
132139
// which the closure captures) must not be relied upon to be freed.
133140
callbackHandle uintptr
134-
// Serializes teardown against in-flight native callbacks so the receive
135-
// buffer is not fed after it is closed.
136-
callbackMu sync.Mutex
137-
activeCallbacks int
138141
}
139142

140143
// Create resolves the cdylib next to the CLI entrypoint and prepares the host.
@@ -182,12 +185,14 @@ func (h *Host) Start() error {
182185
}
183186

184187
// host_start spawned the worker child via libuv's uv_spawn, which installs a
185-
// SIGCHLD handler without SA_ONSTACK on its first call. On macOS the Go runtime
186-
// aborts ("non-Go code set up signal handler without SA_ONSTACK flag") when it
187-
// later reaps one of its own os/exec children, so re-add SA_ONSTACK to that
188-
// foreign handler now that it exists (no-op off Darwin, and before the first
189-
// spawn there is nothing to fix — hence here rather than at library load).
190-
rearmForeignSignalHandlers()
188+
// SIGCHLD handler without SA_ONSTACK on its first call. The Go runtime aborts
189+
// ("non-Go code set up signal handler without SA_ONSTACK flag") when it later
190+
// reaps one of its own os/exec children (e.g. a test-spawned MCP server) and
191+
// the delivered SIGCHLD lands on a non-signal stack. Re-add SA_ONSTACK to that
192+
// foreign handler now that it exists (implemented on darwin+linux; a no-op on
193+
// other platforms, and before the first spawn there is nothing to fix — hence
194+
// here rather than at library load).
195+
rearmForeignSignalHandlers(h.lib.handle)
191196

192197
h.callbackHandle = purego.NewCallback(h.onOutbound)
193198
h.connectionID = h.lib.connectionOpen(h.serverID, h.callbackHandle, 0, nil, 0, nil, 0, nil, 0)
@@ -233,18 +238,18 @@ func (h *Host) buildEnv() []byte {
233238
// are copied out before returning. Nothing may panic across the FFI boundary.
234239
func (h *Host) onOutbound(userData uintptr, bytesPtr uintptr, bytesLen uintptr) uintptr {
235240
_ = userData
236-
h.callbackMu.Lock()
241+
h.mu.Lock()
237242
if h.disposed {
238-
h.callbackMu.Unlock()
243+
h.mu.Unlock()
239244
return 0
240245
}
241246
h.activeCallbacks++
242-
h.callbackMu.Unlock()
247+
h.mu.Unlock()
243248

244249
defer func() {
245-
h.callbackMu.Lock()
250+
h.mu.Lock()
246251
h.activeCallbacks--
247-
h.callbackMu.Unlock()
252+
h.mu.Unlock()
248253
// Never let a panic unwind into native code.
249254
_ = recover()
250255
}()
@@ -263,10 +268,10 @@ func (h *Host) onOutbound(userData uintptr, bytesPtr uintptr, bytesLen uintptr)
263268
}
264269

265270
func (h *Host) writeFrame(frame []byte) (int, error) {
266-
h.disposeMu.Lock()
271+
h.mu.Lock()
267272
closed := h.disposed || h.connectionID == 0
268273
connID := h.connectionID
269-
h.disposeMu.Unlock()
274+
h.mu.Unlock()
270275
if closed {
271276
return 0, fmt.Errorf("the in-process runtime connection is closed")
272277
}
@@ -285,27 +290,30 @@ func (h *Host) writeFrame(frame []byte) (int, error) {
285290
// resources. It is idempotent and waits for any in-flight outbound callback to
286291
// finish before closing the receive buffer.
287292
func (h *Host) Dispose() {
288-
h.disposeMu.Lock()
293+
h.mu.Lock()
289294
if h.disposed {
290-
h.disposeMu.Unlock()
295+
h.mu.Unlock()
291296
return
292297
}
298+
// Publish disposed under the same lock onOutbound uses to check it, so no new
299+
// callback can pass the check and increment activeCallbacks after the drain
300+
// loop below observes zero.
293301
h.disposed = true
294302
connID := h.connectionID
295303
serverID := h.serverID
296304
h.connectionID = 0
297305
h.serverID = 0
298-
h.disposeMu.Unlock()
306+
h.mu.Unlock()
299307

300308
// Stop accepting new callbacks and wait for in-flight ones to drain before
301309
// closing the receive buffer they feed.
302310
for {
303-
h.callbackMu.Lock()
311+
h.mu.Lock()
304312
if h.activeCallbacks == 0 {
305-
h.callbackMu.Unlock()
313+
h.mu.Unlock()
306314
break
307315
}
308-
h.callbackMu.Unlock()
316+
h.mu.Unlock()
309317
runtime.Gosched()
310318
}
311319

‎go/internal/ffihost/sigonstack_darwin.go‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -31,15 +31,15 @@ const (
3131
// without SA_ONSTACK flag" when such a signal (notably SIGCHLD, signal 20 on
3232
// Darwin) is delivered while a Go-managed child process is reaped. libuv
3333
// installs a SIGCHLD handler without SA_ONSTACK, which poisons every subsequent
34-
// os/exec child reaped by Go in the same process (only observed on macOS; Linux
35-
// does not enforce this).
34+
// os/exec child reaped by Go in the same process (enforced by the Go runtime on
35+
// both macOS and Linux; the Linux variant lives in sigonstack_linux.go).
3636
//
3737
// We preserve each foreign handler and merely OR in SA_ONSTACK, so libuv's child
3838
// watching keeps working while the Go runtime stays happy. Handlers left at
3939
// SIG_DFL/SIG_IGN and Go's own handlers (which already carry SA_ONSTACK) are
4040
// untouched. Best-effort: any failure is silently ignored, since the worst case
4141
// is the pre-existing crash.
42-
func rearmForeignSignalHandlers() {
42+
func rearmForeignSignalHandlers(_ uintptr) {
4343
handle, err := purego.Dlopen("/usr/lib/libSystem.B.dylib", purego.RTLD_NOW|purego.RTLD_GLOBAL)
4444
if err != nil || handle == 0 {
4545
return

0 commit comments

Comments
 (0)