From 4210cc94591d34f844320593661b058956849dbd Mon Sep 17 00:00:00 2001 From: wanghj Date: Mon, 6 Jul 2026 17:26:31 +0800 Subject: [PATCH 1/2] fix: harden Windows agent runtime handling - Default unset sandbox providers to Docker and update docs/tests for the new behavior. - Fix Windows Codex runtime liveness checks and app-server shutdown error reporting. - Stop agent lifecycle delivery before recreating runtimes so Feishu disconnects release old Codex state. - Require template-declared environment variables in the agent creation UI and backend save flow. - Update frontend and Go tests for status mapping, required env validation, and Windows lifecycle cleanup. --- README.md | 6 +- README.zh.md | 6 +- docs/config.md | 10 +-- docs/config.zh.md | 10 +-- internal/agent/service.go | 40 ++--------- internal/agent/service_profiles.go | 2 + internal/agent/service_test.go | 29 ++++---- internal/config/config_test.go | 24 +------ internal/config/sandbox_provider_default.go | 64 +---------------- .../config/sandbox_provider_default_test.go | 36 +--------- internal/localstore/migrate_ids.go | 4 +- internal/runtime/codex/appserver_manager.go | 6 +- .../runtime/codex/process_alive_windows.go | 13 +++- internal/runtime/codex/runtime.go | 43 +++--------- internal/runtime/codex/runtime_test.go | 20 ++---- internal/skill/local/upload.go | 2 +- internal/template/local_store.go | 3 +- internal/template/service.go | 4 +- .../ProfileControls/EnvKeyValueEditor.tsx | 25 +++++-- .../ProfileControls/ProfileControls.css | 21 ++++++ .../business/ProfileControls/types.ts | 1 + .../src/hooks/workspace/useAgentController.ts | 6 ++ web/app/src/models/agents.ts | 16 ++++- .../WorkspaceModals/AgentProfileModal.tsx | 4 +- web/app/src/shared/i18n/messages.ts | 2 + .../components/AgentProfileModal.test.tsx | 68 +++++++++++++++++++ .../components/EnvKeyValueEditor.test.tsx | 12 ++++ web/app/tests/models/agents.test.ts | 12 +++- 28 files changed, 236 insertions(+), 253 deletions(-) diff --git a/README.md b/README.md index 21bde8ce..a064f0ec 100644 --- a/README.md +++ b/README.md @@ -89,7 +89,7 @@ CSGClaw gives you one **Manager** and a set of specialized **Workers**, so inste **Workers** — role-specific executors (frontend, backend, testing, docs, research…). Specialization keeps context clean and reduces role confusion. -**Sandbox** — Worker execution is isolated by the configured sandbox provider. Official bundles default to **BoxLite** when they include a bundled `boxlite`, and default to **Docker** otherwise. +**Sandbox** — Worker execution is isolated by the configured sandbox provider. CSGClaw defaults to **Docker** when the provider is unset, and BoxLite remains available through explicit configuration. **Interface** — WebUI out of the box; Feishu, WeChat, Matrix, and other channels available as integrations. @@ -119,8 +119,8 @@ The key isn't that multiple agents exist — it's that **their collaboration is **PicoClaw by default, extensible by design.** CSGClaw uses PicoClaw as its lightweight default Agent Runtime, keeping the Manager fast to start and cheap to run. The runtime remains pluggable, so deployments can integrate alternatives such as OpenClaw when needed. -**BoxLite by default, sandbox-agnostic by design.** -Isolation is non-negotiable. BoxLite is the preferred default because it is lightweight, fast to start, and convenient for local-first workflows. When BoxLite is unavailable, Docker is fully supported as a mature cross-platform fallback, and teams can switch sandbox providers explicitly when their environment requires it. +**Docker by default, sandbox-agnostic by design.** +Isolation is non-negotiable. Docker is the default sandbox provider for broad cross-platform availability, while BoxLite remains supported for environments that prefer its lightweight local runtime. Teams can switch sandbox providers explicitly when their environment requires it. **WebUI first, channel-agnostic by design.** Many multi-agent systems are tightly coupled to one messaging protocol. CSGClaw ships with a built-in WebUI so you can start immediately, while keeping other channels (Feishu, WeChat, Matrix) as optional integrations — not assumptions. diff --git a/README.zh.md b/README.zh.md index 41594b52..6aaf408e 100644 --- a/README.zh.md +++ b/README.zh.md @@ -89,7 +89,7 @@ CSGClaw 提供一位 Manager 和一组可分工的 Worker，让你通过统一 **Worker** — 面向具体职责的执行单元（前端、后端、测试、文档、调研……）。角色分工让上下文更干净，协作更容易组织。 -**Sandbox** — Worker 执行环境由配置的 sandbox provider 隔离。官方 bundle 在内置 `boxlite` 时默认使用 **BoxLite**，未内置时默认使用 **Docker**。 +**Sandbox** — Worker 执行环境由配置的 sandbox provider 隔离。未显式配置 provider 时，CSGClaw 默认使用 **Docker**；BoxLite 仍可通过显式配置启用。 **Interface** — 默认提供 WebUI；飞书、微信、Matrix 等通道可按需接入。 @@ -119,8 +119,8 @@ Manager：已记录，Bob 先修接口，字段确认后 Alice 再更新展示 **默认选择 PicoClaw，同时保留运行时扩展能力。** CSGClaw 默认使用 PicoClaw 作为轻量化 Agent Runtime，让 Manager 启动更快、占用更低。运行时仍可插拔，需要时可以集成 OpenClaw 等其他实现。 -**默认选择 BoxLite，设计上不绑定单一 Sandbox。** -隔离不是可选项。BoxLite 之所以作为默认方案，是因为它轻量、启动快、对本地优先场景更方便；当 BoxLite 不可用时，CSGClaw 也完整支持 Docker 作为更成熟的跨平台回退方案，并允许按环境需要显式切换不同的 sandbox provider。 +**默认选择 Docker，设计上不绑定单一 Sandbox。** +隔离不是可选项。Docker 作为默认 sandbox provider 具备更广泛的跨平台可用性；BoxLite 仍然适合偏好轻量本地运行时的环境，并允许按需要显式切换不同的 sandbox provider。 **默认 WebUI，不绑定单一通道。** 很多多智能体系统把某种消息协议当作唯一入口。CSGClaw 自带 WebUI，让你可以立即开始；飞书、微信、Matrix 等通道作为可选集成存在，而不是预设前提。 diff --git a/docs/config.md b/docs/config.md index abff6dd2..310d7ca2 100644 --- a/docs/config.md +++ b/docs/config.md @@ -159,12 +159,9 @@ Official bundles use one of these layouts: - `csgclaw/bin/csgclaw` plus `csgclaw/bin/boxlite` - `csgclaw/bin/csgclaw` only -If `[sandbox].provider` is omitted or empty, CSGClaw chooses the default dynamically from the installed bundle: +If `[sandbox].provider` is omitted or empty, CSGClaw defaults to `docker`. -- bundled `boxlite` present: default to `boxlite` -- bundled `boxlite` absent: default to `docker` - -That means a generated config can keep the provider empty to follow the bundle default: +That means a generated config can keep the provider empty to follow the default: ```toml [sandbox] @@ -217,8 +214,7 @@ docker_cli_path = "/usr/local/bin/docker" Current platform expectations: -- Linux amd64, Linux arm64, and macOS arm64 official bundles include `boxlite`, so an empty provider resolves to `boxlite`. -- macOS amd64 and Windows amd64 official bundles do not include `boxlite`, so an empty provider resolves to `docker`. +- An empty provider resolves to `docker`. - Windows users should have Docker installed and reachable on `PATH`, or set `[sandbox].docker_cli_path` explicitly. ## Hub Configuration diff --git a/docs/config.zh.md b/docs/config.zh.md index 34542637..0bc4ac19 100644 --- a/docs/config.zh.md +++ b/docs/config.zh.md @@ -159,12 +159,9 @@ CSGClaw 通过配置的 sandbox provider 隔离 Worker 执行环境。当前内 - `csgclaw/bin/csgclaw` 加 `csgclaw/bin/boxlite` - 只有 `csgclaw/bin/csgclaw` -如果 `[sandbox].provider` 省略或为空，CSGClaw 会根据当前安装 bundle 动态选择默认值： +如果 `[sandbox].provider` 省略或为空，CSGClaw 默认使用 `docker`。 -- 内置了 `boxlite`：默认用 `boxlite` -- 没有内置 `boxlite`：默认用 `docker` - -这也是为什么生成出来的配置文件可以把 provider 留空，直接跟随 bundle 默认值： +这也是为什么生成出来的配置文件可以把 provider 留空，直接跟随默认值： ```toml [sandbox] @@ -217,8 +214,7 @@ docker_cli_path = "/usr/local/bin/docker" 当前平台上的默认行为如下： -- Linux amd64、Linux arm64、macOS arm64 的官方 bundle 内置 `boxlite`，因此 provider 留空时会解析成 `boxlite`。 -- macOS amd64 和 Windows amd64 的官方 bundle 不内置 `boxlite`，因此 provider 留空时会解析成 `docker`。 +- provider 留空时会解析成 `docker`。 - Windows 用户需要确保本地 Docker 可用并且能从 `PATH` 找到；如果路径特殊，可以显式设置 `[sandbox].docker_cli_path`。 ## Hub 配置 diff --git a/internal/agent/service.go b/internal/agent/service.go index fbb57c02..0470a947 100644 --- a/internal/agent/service.go +++ b/internal/agent/service.go @@ -13,7 +13,6 @@ import ( "strings" "sync" "sync/atomic" - "syscall" "time" "csgclaw/internal/codexcli" @@ -53,8 +52,6 @@ var locateCodexCLI = func() (string, error) { var defaultSandboxProvider sandbox.Provider = unconfiguredSandboxProvider{} var testDefaultServiceOption ServiceOption -const removeAllRetryAttempts = 12 - var errDefaultTemplateRuntimeMismatch = errors.New("default template runtime mismatch") type unconfiguredSandboxProvider struct{} @@ -770,7 +767,7 @@ func (s *Service) cleanupBootstrapManagerForRecreate(ctx context.Context, rt san if cleanupSkills != nil { defer cleanupSkills() } - if err := removeAllWithRetry(managerHome); err != nil { + if err := removeAll(managerHome); err != nil { return nil, fmt.Errorf("remove bootstrap manager home: %w", err) } if restoreSkills != nil { @@ -1501,7 +1498,7 @@ func (s *Service) Delete(ctx context.Context, id string) error { if err != nil { return err } - if err := removeAllWithRetry(agentHome); err != nil { + if err := removeAll(agentHome); err != nil { return fmt.Errorf("remove agent home: %w", err) } @@ -1684,37 +1681,8 @@ func sandboxNameForAgentID(agentID string) string { return agentruntime.SandboxNameForAgentID(canonicalAgentID(agentID)) } -func removeAllWithRetry(path string) error { - path = strings.TrimSpace(path) - if path == "" { - return fmt.Errorf("path is required") - } - - var lastErr error - for attempt := 0; attempt < removeAllRetryAttempts; attempt++ { - if err := osRemoveAll(path); err == nil || os.IsNotExist(err) { - return nil - } else { - lastErr = err - // Defensive retry: BoxLite runtime cleanup can briefly lag behind Close(), - // so agent home removal may transiently fail with "directory not empty". - // If runtime shutdown semantics improve later, prefer fixing that timing - // instead of relying on retries here. - if !isRetryableRemoveAllError(err) || attempt == removeAllRetryAttempts-1 { - return err - } - } - time.Sleep(time.Duration(attempt+1) * 50 * time.Millisecond) - } - return lastErr -} - -func isRetryableRemoveAllError(err error) bool { - if errors.Is(err, syscall.ENOTEMPTY) || errors.Is(err, syscall.EACCES) { - return true - } - lower := strings.ToLower(err.Error()) - return strings.Contains(lower, "directory not empty") || strings.Contains(lower, "permission denied") +func removeAll(path string) error { + return osRemoveAll(path) } func (s *Service) List() []Agent { diff --git a/internal/agent/service_profiles.go b/internal/agent/service_profiles.go index 7d4dabd2..77be1356 100644 --- a/internal/agent/service_profiles.go +++ b/internal/agent/service_profiles.go @@ -700,6 +700,8 @@ func (s *Service) recreate(ctx context.Context, id string, imageFor func(context } } + s.stopLifecycleAgent(got.ID) + if testCreateGatewayBoxHook != nil { rt, err := s.ensureRuntime(got.ID) if err != nil { diff --git a/internal/agent/service_test.go b/internal/agent/service_test.go index ea924ae4..99675b21 100644 --- a/internal/agent/service_test.go +++ b/internal/agent/service_test.go @@ -9,7 +9,6 @@ import ( "os" "path/filepath" "strings" - "syscall" "testing" "time" @@ -363,16 +362,19 @@ func (f *fakeInfoInstance) Info(context.Context) (sandbox.Info, error) { type fakeLifecycleObserver struct { ensureCalls []Agent stopCalls []string + events []string ensureErr error } func (f *fakeLifecycleObserver) EnsureAgent(_ context.Context, a Agent) error { f.ensureCalls = append(f.ensureCalls, a) + f.events = append(f.events, "ensure:"+a.ID) return f.ensureErr } func (f *fakeLifecycleObserver) StopAgent(agentID string) { f.stopCalls = append(f.stopCalls, agentID) + f.events = append(f.events, "stop:"+agentID) } type cancelOnWrite struct { @@ -1925,9 +1927,15 @@ func TestRecreateTriggersLifecycleObserver(t *testing.T) { if got.Avatar != "avatar/cartoon-7.png" { t.Fatalf("Recreate().Avatar = %q, want %q", got.Avatar, "avatar/cartoon-7.png") } + if len(observer.stopCalls) != 1 || observer.stopCalls[0] != "agent-alice" { + t.Fatalf("StopAgent() calls = %+v, want one call for agent-alice", observer.stopCalls) + } if len(observer.ensureCalls) != 1 || observer.ensureCalls[0].ID != "agent-alice" { t.Fatalf("EnsureAgent() calls = %+v, want one call for agent-alice", observer.ensureCalls) } + if got, want := strings.Join(observer.events, ","), "stop:agent-alice,ensure:agent-alice"; got != want { + t.Fatalf("lifecycle events = %q, want %q", got, want) + } } func TestRecreateProvisionsRuntimeBeforeNew(t *testing.T) { @@ -3910,7 +3918,7 @@ func TestDeleteRemovesRuntimeCacheByHomeDir(t *testing.T) { } } -func TestDeleteRetriesAgentHomeRemovalOnDirectoryNotEmpty(t *testing.T) { +func TestDeleteReturnsAgentHomeRemovalError(t *testing.T) { rt := &fakeRuntime{} SetTestHooks(func(_ *Service, _ string) (sandbox.Runtime, error) { return rt, nil }, nil) defer ResetTestHooks() @@ -3952,10 +3960,7 @@ func TestDeleteRetriesAgentHomeRemovalOnDirectoryNotEmpty(t *testing.T) { osRemoveAll = func(path string) error { removeCalls++ if path == agentHome && removeCalls == 1 { - return &os.PathError{Op: "unlinkat", Path: filepath.Join(agentHome, "boxlite", "images"), Err: syscall.ENOTEMPTY} - } - if path == agentHome && removeCalls == 2 { - return &os.PathError{Op: "unlinkat", Path: filepath.Join(agentHome, "boxlite", "images"), Err: syscall.EACCES} + return &os.PathError{Op: "unlinkat", Path: filepath.Join(agentHome, ".codex", "home", "logs_2.sqlite"), Err: errors.New("The process cannot access the file because it is being used by another process.")} } return os.RemoveAll(path) } @@ -3963,14 +3968,12 @@ func TestDeleteRetriesAgentHomeRemovalOnDirectoryNotEmpty(t *testing.T) { osRemoveAll = origRemoveAll }() - if err := svc.Delete(context.Background(), "u-alice"); err != nil { - t.Fatalf("Delete() error = %v", err) + err = svc.Delete(context.Background(), "u-alice") + if err == nil || !strings.Contains(err.Error(), "being used by another process") { + t.Fatalf("Delete() error = %v, want locked file error", err) } - if removeCalls < 3 { - t.Fatalf("osRemoveAll() calls = %d, want at least 3", removeCalls) - } - if _, err := os.Stat(agentHome); !os.IsNotExist(err) { - t.Fatalf("agent home still exists after delete: err=%v", err) + if removeCalls != 1 { + t.Fatalf("osRemoveAll() calls = %d, want 1", removeCalls) } } diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 70eb9ec5..3da729a9 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -73,9 +73,6 @@ func TestDefaultTasksDirUsesDomainSubdirectory(t *testing.T) { } func TestLoadUsesDefaultBootstrapTemplatesWhenSectionIsEmpty(t *testing.T) { - restore := stubSandboxProviderExecutablePath(t, filepath.Join(t.TempDir(), "bin", "csgclaw")) - defer restore() - dir := t.TempDir() path := filepath.Join(dir, "config.toml") content := `[server] @@ -720,9 +717,6 @@ models = ["minimax-m2.7"] } func TestLoadRejectsRemovedLegacyBoxLiteProvider(t *testing.T) { - restore := stubSandboxProviderExecutablePath(t, filepath.Join(t.TempDir(), "bin", "csgclaw")) - defer restore() - dir := t.TempDir() path := filepath.Join(dir, "config.toml") content := `[server] @@ -753,9 +747,6 @@ models = ["minimax-m2.7"] } func TestLoadReadsDockerSandboxConfig(t *testing.T) { - restore := stubSandboxProviderExecutablePath(t, filepath.Join(t.TempDir(), "bin", "csgclaw")) - defer restore() - dir := t.TempDir() path := filepath.Join(dir, "config.toml") content := `[server] @@ -800,7 +791,7 @@ func TestSandboxEffectiveDockerCLIPathDefault(t *testing.T) { } } -func TestLoadUsesBundledBoxLiteWhenSandboxProviderUnset(t *testing.T) { +func TestLoadUsesDockerWhenSandboxProviderUnsetEvenWithBundledBoxLite(t *testing.T) { dir := t.TempDir() binDir := filepath.Join(dir, "bundle", "bin") if err := os.MkdirAll(binDir, 0o755); err != nil { @@ -809,9 +800,6 @@ func TestLoadUsesBundledBoxLiteWhenSandboxProviderUnset(t *testing.T) { if err := os.WriteFile(filepath.Join(binDir, "boxlite"), []byte(""), 0o755); err != nil { t.Fatalf("WriteFile(boxlite) error = %v", err) } - restore := stubSandboxProviderExecutablePath(t, filepath.Join(binDir, "csgclaw")) - defer restore() - path := filepath.Join(dir, "config.toml") content := `[server] listen_addr = "127.0.0.1:18080" @@ -832,7 +820,7 @@ models = ["minimax-m2.7"] if err != nil { t.Fatalf("Load() error = %v", err) } - if got, want := cfg.Sandbox.Provider, BoxLiteProvider; got != want { + if got, want := cfg.Sandbox.Provider, DockerProvider; got != want { t.Fatalf("cfg.Sandbox.Provider = %q, want %q", got, want) } } @@ -846,9 +834,6 @@ func TestLoadKeepsExplicitDockerProviderWhenBundledBoxLiteIsPresent(t *testing.T if err := os.WriteFile(filepath.Join(binDir, "boxlite"), []byte(""), 0o755); err != nil { t.Fatalf("WriteFile(boxlite) error = %v", err) } - restore := stubSandboxProviderExecutablePath(t, filepath.Join(binDir, "csgclaw")) - defer restore() - path := filepath.Join(dir, "config.toml") content := `[server] listen_addr = "127.0.0.1:18080" @@ -1503,10 +1488,7 @@ models = ["gpt-test"] } } -func TestSaveKeepsSandboxProviderUnsetWhenItUsesDynamicDefault(t *testing.T) { - restore := stubSandboxProviderExecutablePath(t, filepath.Join(t.TempDir(), "bin", "csgclaw")) - defer restore() - +func TestSaveKeepsSandboxProviderUnsetWhenItUsesDefault(t *testing.T) { dir := t.TempDir() path := filepath.Join(dir, "config.toml") content := `[server] diff --git a/internal/config/sandbox_provider_default.go b/internal/config/sandbox_provider_default.go index 2c01f3be..4ac7d719 100644 --- a/internal/config/sandbox_provider_default.go +++ b/internal/config/sandbox_provider_default.go @@ -1,69 +1,7 @@ package config -import ( - "os" - "path/filepath" - "runtime" - "strings" -) - -// DefaultSandboxProvider remains the canonical bundled-BoxLite provider name -// for explicit configs. Unset providers are resolved dynamically at runtime. -const DefaultSandboxProvider = BoxLiteProvider - -var sandboxProviderExecutablePath = os.Executable +const DefaultSandboxProvider = DockerProvider func defaultSandboxProvider() string { - if hasBundledBoxLite() { - return BoxLiteProvider - } return DockerProvider } - -func hasBundledBoxLite() bool { - exe, err := sandboxProviderExecutablePath() - if err != nil { - return false - } - for _, exePath := range executablePathCandidates(exe) { - dir := filepath.Dir(exePath) - for _, name := range bundledBoxLiteCandidateNames() { - candidate := filepath.Join(dir, name) - if isExecutableFile(candidate) { - return true - } - } - } - return false -} - -func executablePathCandidates(exe string) []string { - exe = strings.TrimSpace(exe) - if exe == "" { - return nil - } - - candidates := []string{exe} - if resolved, err := filepath.EvalSymlinks(exe); err == nil && resolved != "" && resolved != exe { - candidates = append(candidates, resolved) - } - return candidates -} - -func bundledBoxLiteCandidateNames() []string { - if runtime.GOOS == "windows" { - return []string{"boxlite.exe", "boxlite"} - } - return []string{"boxlite"} -} - -func isExecutableFile(path string) bool { - info, err := os.Stat(path) - if err != nil || info.IsDir() { - return false - } - if runtime.GOOS == "windows" { - return true - } - return info.Mode()&0o111 != 0 -} diff --git a/internal/config/sandbox_provider_default_test.go b/internal/config/sandbox_provider_default_test.go index 06c22688..631bbd1c 100644 --- a/internal/config/sandbox_provider_default_test.go +++ b/internal/config/sandbox_provider_default_test.go @@ -1,45 +1,15 @@ package config -import ( - "os" - "path/filepath" - "testing" -) +import "testing" -func TestDefaultSandboxProviderUsesBundledBoxLiteWhenPresent(t *testing.T) { - dir := t.TempDir() - binDir := filepath.Join(dir, "bin") - if err := os.MkdirAll(binDir, 0o755); err != nil { - t.Fatalf("MkdirAll() error = %v", err) - } - if err := os.WriteFile(filepath.Join(binDir, "boxlite"), []byte(""), 0o755); err != nil { - t.Fatalf("WriteFile(boxlite) error = %v", err) - } - - restore := stubSandboxProviderExecutablePath(t, filepath.Join(binDir, "csgclaw")) - defer restore() - - if got, want := defaultSandboxProvider(), BoxLiteProvider; got != want { +func TestDefaultSandboxProviderUsesDockerEvenWhenBundledBoxLiteIsPresent(t *testing.T) { + if got, want := defaultSandboxProvider(), DockerProvider; got != want { t.Fatalf("defaultSandboxProvider() = %q, want %q", got, want) } } func TestDefaultSandboxProviderFallsBackToDockerWhenBundledBoxLiteIsAbsent(t *testing.T) { - restore := stubSandboxProviderExecutablePath(t, filepath.Join(t.TempDir(), "bin", "csgclaw")) - defer restore() - if got, want := defaultSandboxProvider(), DockerProvider; got != want { t.Fatalf("defaultSandboxProvider() = %q, want %q", got, want) } } - -func stubSandboxProviderExecutablePath(t *testing.T, path string) func() { - t.Helper() - previous := sandboxProviderExecutablePath - sandboxProviderExecutablePath = func() (string, error) { - return path, nil - } - return func() { - sandboxProviderExecutablePath = previous - } -} diff --git a/internal/localstore/migrate_ids.go b/internal/localstore/migrate_ids.go index a0e2f6d1..7b4bc31f 100644 --- a/internal/localstore/migrate_ids.go +++ b/internal/localstore/migrate_ids.go @@ -2496,7 +2496,7 @@ func moveDirContentsWithStableCollisions(src, dst string) error { return err } } - return os.Remove(src) + return os.RemoveAll(src) } func moveDirEntryWithStableCollisions(src, dst string) error { @@ -2647,7 +2647,7 @@ func mergeDirs(src, dst string) error { return err } } - return os.Remove(src) + return os.RemoveAll(src) } func arrayOfMaps(value any) []map[string]any { diff --git a/internal/runtime/codex/appserver_manager.go b/internal/runtime/codex/appserver_manager.go index 90bb9b14..2cf73a90 100644 --- a/internal/runtime/codex/appserver_manager.go +++ b/internal/runtime/codex/appserver_manager.go @@ -168,7 +168,9 @@ func (m *appServerManager) Stop(ctx context.Context, handle SessionHandle) error _ = live.stdin.Close() } if live.cmd != nil && live.cmd.Process != nil { - _ = stopProcess(live.cmd.Process.Pid) + if err := stopProcess(live.cmd.Process.Pid); err != nil { + return err + } } timeout := time.NewTimer(appServerStopTimeout) @@ -179,7 +181,7 @@ func (m *appServerManager) Stop(ctx context.Context, handle SessionHandle) error case <-ctx.Done(): return ctx.Err() case <-timeout.C: - return nil + return fmt.Errorf("codex app-server stop timeout after %s: runtime_id=%s", appServerStopTimeout, runtimeID) } } diff --git a/internal/runtime/codex/process_alive_windows.go b/internal/runtime/codex/process_alive_windows.go index 0eb9f47b..7ee781f9 100644 --- a/internal/runtime/codex/process_alive_windows.go +++ b/internal/runtime/codex/process_alive_windows.go @@ -3,17 +3,24 @@ package codex import ( - "os" "syscall" ) +const stillActive = 259 + func processAlivePID(pid int) bool { if pid <= 0 { return false } - proc, err := os.FindProcess(pid) + handle, err := syscall.OpenProcess(syscall.PROCESS_QUERY_INFORMATION, false, uint32(pid)) if err != nil { return false } - return proc.Signal(syscall.Signal(0)) == nil + defer syscall.CloseHandle(handle) + + var exitCode uint32 + if err := syscall.GetExitCodeProcess(handle, &exitCode); err != nil { + return false + } + return exitCode == stillActive } diff --git a/internal/runtime/codex/runtime.go b/internal/runtime/codex/runtime.go index 74cec0bf..e452a74f 100644 --- a/internal/runtime/codex/runtime.go +++ b/internal/runtime/codex/runtime.go @@ -29,7 +29,6 @@ const ( workspaceDirName = "workspace" homeDirName = "home" logPollInterval = 200 * time.Millisecond - removeAllRetryAttempts = 12 codexProxyProviderName = "proxy" codexModelProviderName = "codex" ) @@ -266,12 +265,14 @@ func (r *Runtime) Delete(ctx context.Context, h agentruntime.Handle) error { if runtimeID == "" { return fmt.Errorf("runtime id is required") } - _, _ = r.Stop(ctx, h) + if _, err := r.Stop(ctx, h); err != nil && !errors.Is(err, sandbox.ErrNotFound) && !errors.Is(err, os.ErrNotExist) { + return fmt.Errorf("stop runtime before delete: %w", err) + } dir, err := r.runtimeDirForHandle(h) if err != nil { return err } - if err := r.removeAllWithRetry(dir); err != nil && !errors.Is(err, os.ErrNotExist) { + if err := r.removeAll(dir); err != nil && !errors.Is(err, os.ErrNotExist) { return err } return nil @@ -939,35 +940,6 @@ func (r *Runtime) removeAll(path string) error { return os.RemoveAll(path) } -func (r *Runtime) removeAllWithRetry(path string) error { - path = strings.TrimSpace(path) - if path == "" { - return fmt.Errorf("path is required") - } - - var lastErr error - for attempt := 0; attempt < removeAllRetryAttempts; attempt++ { - if err := r.removeAll(path); err == nil || errors.Is(err, os.ErrNotExist) { - return nil - } else { - lastErr = err - if !isRetryableRemoveAllError(err) || attempt == removeAllRetryAttempts-1 { - return err - } - } - time.Sleep(time.Duration(attempt+1) * 50 * time.Millisecond) - } - return lastErr -} - -func isRetryableRemoveAllError(err error) bool { - if errors.Is(err, syscall.ENOTEMPTY) || errors.Is(err, syscall.EACCES) { - return true - } - lower := strings.ToLower(err.Error()) - return strings.Contains(lower, "directory not empty") || strings.Contains(lower, "permission denied") -} - func (r *Runtime) openFile(path string, flag int, mode os.FileMode) (*os.File, error) { if r.deps.OpenFile != nil { return r.deps.OpenFile(path, flag, mode) @@ -1158,6 +1130,13 @@ func stopProcess(pid int) error { if err := proc.Kill(); err != nil && !errors.Is(err, os.ErrProcessDone) { return err } + deadline = time.Now().Add(3 * time.Second) + for time.Now().Before(deadline) { + if !processAlive(pid) { + return nil + } + time.Sleep(50 * time.Millisecond) + } return nil } diff --git a/internal/runtime/codex/runtime_test.go b/internal/runtime/codex/runtime_test.go index d28420e0..1295d145 100644 --- a/internal/runtime/codex/runtime_test.go +++ b/internal/runtime/codex/runtime_test.go @@ -7,7 +7,6 @@ import ( "os" "path/filepath" "strings" - "syscall" "testing" "time" @@ -702,7 +701,7 @@ func TestBuildSessionEnvOnlyInjectsOpenAIAPIKey(t *testing.T) { } } -func TestDeleteRetriesRuntimeDirRemovalOnDirectoryNotEmpty(t *testing.T) { +func TestDeleteReturnsRuntimeDirRemovalError(t *testing.T) { root := t.TempDir() rt := New(Dependencies{ BinaryProvider: fakeBinaryProvider{path: "/tmp/codex"}, @@ -756,22 +755,17 @@ func TestDeleteRetriesRuntimeDirRemovalOnDirectoryNotEmpty(t *testing.T) { rt.deps.RemoveAll = func(path string) error { removeCalls++ if path == runtimeDir && removeCalls == 1 { - return &os.PathError{Op: "unlinkat", Path: filepath.Join(runtimeDir, "home", "tmp", "plugins-clone", "plugins"), Err: syscall.ENOTEMPTY} - } - if path == runtimeDir && removeCalls == 2 { - return &os.PathError{Op: "unlinkat", Path: filepath.Join(runtimeDir, "home", "tmp", "plugins-clone", "plugins"), Err: syscall.EACCES} + return &os.PathError{Op: "unlinkat", Path: filepath.Join(runtimeDir, "home", "logs_2.sqlite"), Err: errors.New("The process cannot access the file because it is being used by another process.")} } return os.RemoveAll(path) } - if err := rt.Delete(context.Background(), handle); err != nil { - t.Fatalf("Delete() error = %v", err) - } - if removeCalls < 3 { - t.Fatalf("RemoveAll() calls = %d, want at least 3", removeCalls) + err = rt.Delete(context.Background(), handle) + if err == nil || !strings.Contains(err.Error(), "being used by another process") { + t.Fatalf("Delete() error = %v, want locked file error", err) } - if _, err := os.Stat(runtimeDir); !errors.Is(err, os.ErrNotExist) { - t.Fatalf("runtime dir still exists after delete: err=%v", err) + if removeCalls != 1 { + t.Fatalf("RemoveAll() calls = %d, want 1", removeCalls) } } diff --git a/internal/skill/local/upload.go b/internal/skill/local/upload.go index e6bd27b3..0580fe04 100644 --- a/internal/skill/local/upload.go +++ b/internal/skill/local/upload.go @@ -51,7 +51,7 @@ func InstallArchiveWithOptions(root, filename string, archive []byte, options In if err != nil { return SkillSummary{}, fmt.Errorf("create upload temp dir: %w", err) } - defer os.RemoveAll(tempDir) + defer func() { _ = os.RemoveAll(tempDir) }() if err := extractArchive(archive, tempDir); err != nil { return SkillSummary{}, err diff --git a/internal/template/local_store.go b/internal/template/local_store.go index c63067ba..f30c6672 100644 --- a/internal/template/local_store.go +++ b/internal/template/local_store.go @@ -4,13 +4,12 @@ import ( "context" "errors" "fmt" + toml "github.com/pelletier/go-toml/v2" "os" "path/filepath" "slices" "strings" "time" - - toml "github.com/pelletier/go-toml/v2" ) const ( diff --git a/internal/template/service.go b/internal/template/service.go index 397073a4..b0601acc 100644 --- a/internal/template/service.go +++ b/internal/template/service.go @@ -150,7 +150,7 @@ func (s *Service) ListWorkspace(ctx context.Context, id, workspacePath string) ( return apitypes.WorkspaceListing{}, err } if normalizeRegistryKind(cfgStore.ref.Kind) != RegistryKindLocal { - defer os.RemoveAll(workspace.Path) + defer func() { _ = os.RemoveAll(workspace.Path) }() } if strings.TrimSpace(workspace.Path) == "" { if strings.TrimSpace(workspacePath) == "" { @@ -174,7 +174,7 @@ func (s *Service) ReadWorkspaceFile(ctx context.Context, id, workspacePath strin return apitypes.WorkspaceFile{}, err } if normalizeRegistryKind(cfgStore.ref.Kind) != RegistryKindLocal { - defer os.RemoveAll(workspace.Path) + defer func() { _ = os.RemoveAll(workspace.Path) }() } if strings.TrimSpace(workspace.Path) == "" { return apitypes.WorkspaceFile{}, ErrWorkspaceDirRequired diff --git a/web/app/src/components/business/ProfileControls/EnvKeyValueEditor.tsx b/web/app/src/components/business/ProfileControls/EnvKeyValueEditor.tsx index 01e5af53..f57ec2a3 100644 --- a/web/app/src/components/business/ProfileControls/EnvKeyValueEditor.tsx +++ b/web/app/src/components/business/ProfileControls/EnvKeyValueEditor.tsx @@ -1,4 +1,5 @@ import { Button, TextInput } from "@/components/ui"; +import { classNames } from "@/shared/lib/classNames"; import type { EnvKeyValueRow, Translator } from "./types"; export type EnvKeyValueEditorProps = { @@ -19,15 +20,26 @@ export function EnvKeyValueEditor({ rows = [], t, onChange }: EnvKeyValueEditorP return (
{items.map((row, index) => ( -
- update(index, { key: event.currentTarget.value })} - /> +
+
+ update(index, { key: event.currentTarget.value })} + /> + {row.required ? ( + + ) : null} +
update(index, { value: event.currentTarget.value })} />