From a009e76da25b56e55522329ad7b047e77db7d06b Mon Sep 17 00:00:00 2001 From: lxowalle Date: Wed, 8 Apr 2026 16:48:55 +0800 Subject: [PATCH] fix isolation startup lifecycle and MCP transport wrapping --- pkg/isolation/platform_linux.go | 3 +++ pkg/isolation/platform_other.go | 3 +++ pkg/isolation/platform_windows.go | 16 +++++++++++++++- pkg/isolation/runtime.go | 25 +++++++------------------ pkg/isolation/runtime_test.go | 25 +++++++++++++++++++++++++ pkg/mcp/isolated_command_transport.go | 6 ++++-- pkg/mcp/manager.go | 7 ------- 7 files changed, 57 insertions(+), 28 deletions(-) diff --git a/pkg/isolation/platform_linux.go b/pkg/isolation/platform_linux.go index 429a6f6c6..d8b9e417c 100644 --- a/pkg/isolation/platform_linux.go +++ b/pkg/isolation/platform_linux.go @@ -99,6 +99,9 @@ func postStartPlatformIsolation(cmd *exec.Cmd, isolation config.IsolationConfig, return nil } +func cleanupPendingPlatformResources(cmd *exec.Cmd) { +} + // buildLinuxBwrapArgs translates the mount plan into the bubblewrap command // line that re-executes the original process inside the isolated mount view. func buildLinuxBwrapArgs( diff --git a/pkg/isolation/platform_other.go b/pkg/isolation/platform_other.go index 9c517f847..d8d06e2ec 100644 --- a/pkg/isolation/platform_other.go +++ b/pkg/isolation/platform_other.go @@ -17,3 +17,6 @@ func applyPlatformIsolation(cmd *exec.Cmd, isolation config.IsolationConfig, roo func postStartPlatformIsolation(cmd *exec.Cmd, isolation config.IsolationConfig, root string) error { return nil } + +func cleanupPendingPlatformResources(cmd *exec.Cmd) { +} diff --git a/pkg/isolation/platform_windows.go b/pkg/isolation/platform_windows.go index 7cd86cd4e..9434976f7 100644 --- a/pkg/isolation/platform_windows.go +++ b/pkg/isolation/platform_windows.go @@ -101,9 +101,9 @@ func postStartPlatformIsolation(cmd *exec.Cmd, isolation config.IsolationConfig, } return fmt.Errorf("open process for job assignment: %w", err) } - defer windows.CloseHandle(proc) if err := windows.AssignProcessToJobObject(job, proc); err != nil { + _ = windows.CloseHandle(proc) _ = windows.CloseHandle(job) if resources.token != 0 { _ = resources.token.Close() @@ -120,6 +120,20 @@ func postStartPlatformIsolation(cmd *exec.Cmd, isolation config.IsolationConfig, return nil } +func cleanupPendingPlatformResources(cmd *exec.Cmd) { + if cmd == nil { + return + } + resourcesAny, ok := windowsPendingResources.LoadAndDelete(cmd) + if !ok { + return + } + resources, _ := resourcesAny.(windowsProcessResources) + if resources.token != 0 { + _ = resources.token.Close() + } +} + func reapWindowsProcessResources(pid int, proc windows.Handle, job windows.Handle) { _, _ = windows.WaitForSingleObject(proc, windows.INFINITE) _ = windows.CloseHandle(proc) diff --git a/pkg/isolation/runtime.go b/pkg/isolation/runtime.go index f2c6217ae..83c86b514 100644 --- a/pkg/isolation/runtime.go +++ b/pkg/isolation/runtime.go @@ -42,7 +42,6 @@ type UserEnv struct { var ( isolationMu sync.RWMutex currentIsolation = config.DefaultConfig().Isolation - currentWorkspace = config.DefaultConfig().WorkspacePath() ) // Configure updates the process-wide isolation state used by subsequent child @@ -53,11 +52,9 @@ func Configure(cfg *config.Config) { if cfg == nil { defaults := config.DefaultConfig() currentIsolation = defaults.Isolation - currentWorkspace = defaults.WorkspacePath() return } currentIsolation = cfg.Isolation - currentWorkspace = filepath.Clean(cfg.WorkspacePath()) } // CurrentConfig returns the currently active isolation settings. @@ -67,14 +64,6 @@ func CurrentConfig() config.IsolationConfig { return currentIsolation } -// CurrentWorkspace returns the workspace path currently associated with the -// configured runtime state. -func CurrentWorkspace() string { - isolationMu.RLock() - defer isolationMu.RUnlock() - return currentWorkspace -} - // ResolveInstanceRoot resolves the instance root used to build the isolated // filesystem and redirected user environment. func ResolveInstanceRoot() (string, error) { @@ -111,11 +100,7 @@ func InstanceDirs(root string) []string { filepath.Join(root, "runtime-user-env", "cache"), filepath.Join(root, "runtime-user-env", "state"), } - workspace := CurrentWorkspace() - if workspace == "" { - workspace = filepath.Join(root, pkg.WorkspaceName) - } - dirs = append(dirs, workspace) + dirs = append(dirs, filepath.Join(root, pkg.WorkspaceName)) if runtime.GOOS == "windows" { dirs = append(dirs, filepath.Join(root, "runtime-user-env", "AppData", "Roaming"), @@ -291,8 +276,10 @@ func BuildLinuxMountPlan(root string, overrides []config.ExposePath) []MountRule // BuildWindowsAccessRules derives the host-path access policy used by the // Windows restricted-token backend. func BuildWindowsAccessRules(root string, overrides []config.ExposePath) []AccessRule { - rules := []AccessRule{{Path: root, Mode: "rw"}} - for _, item := range MergeExposePaths(nil, overrides) { + merged := MergeExposePaths(nil, overrides) + rules := make([]AccessRule, 0, len(merged)+1) + rules = append(rules, AccessRule{Path: root, Mode: "rw"}) + for _, item := range merged { rules = append(rules, AccessRule{Path: item.Source, Mode: item.Mode}) } return rules @@ -367,6 +354,7 @@ func Start(cmd *exec.Cmd) error { return err } if err := cmd.Start(); err != nil { + cleanupPendingPlatformResources(cmd) return err } isolation := CurrentConfig() @@ -393,6 +381,7 @@ func Run(cmd *exec.Cmd) error { return err } if err := cmd.Start(); err != nil { + cleanupPendingPlatformResources(cmd) return err } isolation := CurrentConfig() diff --git a/pkg/isolation/runtime_test.go b/pkg/isolation/runtime_test.go index 5be9324db..0b6859319 100644 --- a/pkg/isolation/runtime_test.go +++ b/pkg/isolation/runtime_test.go @@ -7,6 +7,7 @@ import ( "runtime" "testing" + "github.com/sipeed/picoclaw/pkg" "github.com/sipeed/picoclaw/pkg/config" ) @@ -35,6 +36,30 @@ func TestPrepareInstanceRoot_CreatesDirectories(t *testing.T) { } } +func TestInstanceDirs_UsesInstanceWorkspaceNotGlobalState(t *testing.T) { + root := filepath.Join(t.TempDir(), "instance") + cfg := config.DefaultConfig() + cfg.Isolation.Enabled = true + cfg.Agents.Defaults.Workspace = filepath.Join(t.TempDir(), "external-workspace") + Configure(cfg) + t.Cleanup(func() { Configure(config.DefaultConfig()) }) + + dirs := InstanceDirs(root) + wantWorkspace := filepath.Join(root, pkg.WorkspaceName) + found := false + for _, dir := range dirs { + if dir == wantWorkspace { + found = true + } + if dir == cfg.WorkspacePath() { + t.Fatalf("InstanceDirs() should not depend on process-wide workspace state: %q", dir) + } + } + if !found { + t.Fatalf("InstanceDirs() missing instance workspace dir %q", wantWorkspace) + } +} + func TestIsSupportedOn(t *testing.T) { tests := []struct { goos string diff --git a/pkg/mcp/isolated_command_transport.go b/pkg/mcp/isolated_command_transport.go index ee6f6ebbf..f54b4af8b 100644 --- a/pkg/mcp/isolated_command_transport.go +++ b/pkg/mcp/isolated_command_transport.go @@ -220,5 +220,7 @@ func (c *isolatedIOConn) Close() error { return c.closeErr } -var _ sdkmcp.Transport = (*isolatedCommandTransport)(nil) -var _ sdkmcp.Connection = (*isolatedIOConn)(nil) +var ( + _ sdkmcp.Transport = (*isolatedCommandTransport)(nil) + _ sdkmcp.Connection = (*isolatedIOConn)(nil) +) diff --git a/pkg/mcp/manager.go b/pkg/mcp/manager.go index f7a0b6b68..f589f82a9 100644 --- a/pkg/mcp/manager.go +++ b/pkg/mcp/manager.go @@ -16,7 +16,6 @@ import ( "github.com/modelcontextprotocol/go-sdk/mcp" "github.com/sipeed/picoclaw/pkg/config" - "github.com/sipeed/picoclaw/pkg/isolation" "github.com/sipeed/picoclaw/pkg/logger" ) @@ -366,12 +365,6 @@ func (m *Manager) ConnectServer( env = append(env, fmt.Sprintf("%s=%s", k, v)) } cmd.Env = env - // Apply the shared isolation preparation before the MCP SDK takes over the - // stdio transport so stdio servers see the same isolated environment. - if err := isolation.PrepareCommand(cmd); err != nil { - return fmt.Errorf("prepare stdio MCP isolation: %w", err) - } - transport = &isolatedCommandTransport{Command: cmd} default: return fmt.Errorf(