From 9f35f0c434ac091106dc80af8016d517a1a1b8d1 Mon Sep 17 00:00:00 2001 From: LiangshouX Date: Thu, 29 Oct 2026 00:57:36 +0800 Subject: [PATCH] fix(dsh): address the environment review on 603ea41 --- integrations/dsh/docs/environment.md | 26 +- integrations/dsh/docs/environment_ZH.md | 20 +- integrations/dsh/src/client/index.tsx | 44 ++- integrations/dsh/src/client/status-page.tsx | 8 +- .../dsh/src/environment/client/dependency.tsx | 21 +- .../dsh/src/environment/client/locales.ts | 24 ++ .../dsh/src/environment/client/python.tsx | 7 +- .../dsh/src/environment/client/rpc.ts | 32 +- .../dsh/src/environment/client/service.tsx | 174 ++++++++-- .../dsh/src/environment/client/tab.tsx | 63 ++-- .../dsh/src/environment/endpoint-key.ts | 21 ++ .../dsh/src/environment/host/gateway.ts | 2 +- .../dsh/src/environment/host/install.ts | 12 +- integrations/dsh/src/environment/host/log.ts | 18 +- .../dsh/src/environment/host/manager.ts | 45 ++- .../dsh/src/environment/host/service.ts | 87 ++--- integrations/dsh/src/environment/types.ts | 23 ++ integrations/dsh/tests/env-manager.test.mjs | 22 +- integrations/dsh/tests/env-reme.test.mjs | 22 +- integrations/dsh/tests/env-service.test.mjs | 21 +- integrations/dsh/tests/env-tab.test.mjs | 327 ++++++++++++++++++ 21 files changed, 855 insertions(+), 164 deletions(-) create mode 100644 integrations/dsh/src/environment/endpoint-key.ts diff --git a/integrations/dsh/docs/environment.md b/integrations/dsh/docs/environment.md index 82b0a077..da1e6a32 100644 --- a/integrations/dsh/docs/environment.md +++ b/integrations/dsh/docs/environment.md @@ -120,18 +120,23 @@ processes it manages when the Host exits. The plugin owns a small service adapte **after** the OS acknowledged the spawn, so a missing interpreter leaves no state behind. - A service is reported as managed — and offered a **Stop** button — only when the record matches the endpoint on screen, the pid is still alive, its image is still a Python or `reme` executable, **and** its command line still - carries the recorded `workspace_dir` and `service.port`. The command line is what makes a recycled pid safe: an - image name alone cannot prove which launch a pid belongs to. A record for port 2333 never claims a service on - port 2444. When the evidence cannot be read, ownership stays unproven: the process is shown as foreign, Stop - refuses to touch it, and a later Start drops the stale record instead of being blocked forever. + carries the recorded `workspace_dir` and `service.port`. The command line rules out most recycled pids (a longer + workspace path or another port never matches), though it cannot tell a pid apart from a user who starts the exact + same command by hand. A record for port 2333 never claims a service on port 2444. +- When the evidence cannot be read (localized `tasklist`/`ps`, missing permissions), ownership stays unproven. Start + and Stop both leave the process and the record alone: Stop refuses, and Start reports an error instead of dropping + the record. Discarding it would orphan a live service that the tab could never stop again. Verify the process + yourself, stop it, then retry — or remove `service.json` if it is really gone. - A service that occupies the port but fails the health check is reported as a conflict; if it is provably this plugin's service, the tab still offers **Stop**. - **Stop** tree-kills the recorded process (process group on POSIX, `taskkill /T /F` on Windows), waits for it to exit, escalates (POSIX: `SIGTERM` then `SIGKILL`; Windows: force `taskkill` again), and clears the record only after the pid is gone. A service started elsewhere stays untouched and is only observed. +- A recorded launch that is still alive but never became healthy is shown as **Last launch not ready**, with Stop as + the way out; it is not shown as "Starting" unless a start from this tab is actually in flight. - **Start** is serialized: two requests cannot spawn two services, and a second request observes the launch already in flight. Before replacing a recorded launch that never became healthy, Start only kills it when - ownership is proven; otherwise it drops the record without touching the process. + ownership is proven; an unproven record is kept and reported, never silently dropped. - **Auto-start** (`autoStart`, off by default) pulls the service up when DSH starts, if it is not already running. Python probes and pip installs are bounded Host operations; the service is the only process that must outlive DSH. @@ -164,9 +169,14 @@ the file is left unchanged. **The service reports “Port occupied”.** Something else listens on the endpoint. Change the port in the service URL (Memory settings) or stop the other program. -**The service does not become healthy.** The Start action polls for 120 seconds and then reports a timeout. Use -**View log** (`$DSH_HOME`-independent: the workspace `service.log`) for the startup trace — a missing `reme-ai`, -an invalid `.env`, or a port conflict all appear there. +**The service does not become healthy.** The Start action polls for 120 seconds. If the port is answered but +`/health_check` never passes, the card switches to the conflict state so **Stop** stays available. Use **View log** +(the workspace `service.log`) for the startup trace — a missing `reme-ai`, an invalid `.env`, or a port conflict all +appear there. + +**The service shows “Last launch not ready”.** A launch recorded by the plugin is still alive but never answered +`/health_check` (for example DSH closed while it was booting). Use **Stop** to clear it, then start again. If Stop +reports that the identity cannot be verified, confirm the process yourself before ending it. **The service shows as foreign.** It was started outside the plugin, so it is observed but not managed. Stop it in its own terminal if you want the tab to manage the next launch. diff --git a/integrations/dsh/docs/environment_ZH.md b/integrations/dsh/docs/environment_ZH.md index 3124d97b..9f09c11e 100644 --- a/integrations/dsh/docs/environment_ZH.md +++ b/integrations/dsh/docs/environment_ZH.md @@ -100,15 +100,20 @@ ReMe 在服务启动时读取 workspace 目录下的 `.env`；标签页从不把 - 启动在操作系统确认 spawn 之后才记录到 `$DSH_HOME/reme-memory/service.json`（pid、地址、workspace、命令行、启动时间）， 所以解释器缺失时不会留下残留状态。 - 只有记录与当前地址一致、pid 仍存活、进程映像仍是 Python 或 `reme`，**且命令行仍包含记录的 `workspace_dir` 与 - `service.port`** 时，服务才被判定为“由本插件托管”并给出**停止**按钮。命令行是防止 pid 复用的关键证据：只看映像名 - 无法证明 pid 属于哪次启动。2333 端口上的记录绝不会声称管理 2444 端口上的服务。证据读不到时按失败关闭处理：显示为 - 外部进程，停止操作不会碰它；之后点启动会丢弃这条过期记录，而不是永久卡住。 + `service.port`** 时，服务才被判定为“由本插件托管”并给出**停止**按钮。命令行能排除大多数 pid 复用（更长的 + workspace 路径、另一个端口都不会命中），但无法区分“用户照抄同一命令行手工启动”的同一服务。2333 端口上的记录绝不会 + 声称管理 2444 端口上的服务。 +- 证据读不到时（`tasklist`/`ps` 输出本地化、权限不足等）归属保持“未证明”：启动与停止都不会碰进程、也不会丢弃记录—— + 停止会拒绝，启动会返回错误。丢弃记录会让一个仍在运行的进程变成标签页再也停不掉的孤儿；请自行确认该进程后结束它 + 再重试，若确实已消失则删除 `service.json`。 - 服务占用端口但健康检查失败时显示为“端口被占用”；若能证明是本插件启动的，标签页仍给出**停止**按钮。 - **停止**会树杀记录的进程（POSIX 上按进程组、Windows 上为 `taskkill /T /F`），等待其退出，必要时升级（POSIX： `SIGTERM` 后 `SIGKILL`；Windows：再次强制 `taskkill`），并**在 pid 真正消失之后**才清除记录。别处启动的服务不受影响， 只会被观察。 +- 记录中的进程仍存活但从未就绪时，显示为**上次启动未就绪**，并给出**停止**按钮；只有本标签页发起的启动仍在进行时才会 + 显示“启动中”。 - **启动**是串行化的：两个请求不会拉起两个服务，后到的请求会观察已在进行的启动。替换“一直没有就绪”的旧记录时，只有 - 归属被证明才会杀掉它；证明不了就只丢弃记录，不碰进程。 + 归属被证明才会杀掉它；证明不了就保留记录并报错，不会静默丢弃。 - **自动启动**（`autoStart`，默认关闭）会在 DSH 启动时把尚未运行的服务拉起。 Python 探测与 pip 安装都是有界的 Host 操作；服务是唯一需要比 DSH 活得更久的进程。 @@ -137,8 +142,11 @@ Python 探测与 pip 安装都是有界的 Host 操作；服务是唯一需要 **服务显示“端口被占用”。** 该端口被其它程序占用。请在服务地址（记忆设置）中换一个端口，或停止占用它的程序。 -**服务迟迟未就绪。** 启动会轮询 120 秒后报超时。用**查看日志**（workspace 下的 `service.log`）看启动过程——缺少 `reme-ai`、 -`.env` 不合法、端口冲突都会体现在那里。 +**服务迟迟未就绪。** 启动会轮询 120 秒；如果端口有响应但 `/health_check` 一直不通过，卡片会切到“端口被占用”状态，**停止**按钮 +始终可用。用**查看日志**（workspace 下的 `service.log`）看启动过程——缺少 `reme-ai`、`.env` 不合法、端口冲突都会体现在那里。 + +**服务显示“上次启动未就绪”。** 本插件记录的进程仍存活，但一直没有通过 `/health_check`（例如启动途中 DSH 被关闭）。点**停止** +清掉它，再重新启动。若停止时提示无法验证进程身份，请先自行确认该进程再结束它。 **服务显示为外部启动。** 它不是由本插件启动的，因此只被观察而不被管理。若希望在标签页里管理，先在它自己的终端里停止，再从 这里启动。 diff --git a/integrations/dsh/src/client/index.tsx b/integrations/dsh/src/client/index.tsx index 5c74ae5d..6c1624e9 100644 --- a/integrations/dsh/src/client/index.tsx +++ b/integrations/dsh/src/client/index.tsx @@ -1,4 +1,11 @@ -import { useEffect, useReducer, useState, useSyncExternalStore } from "react"; +import { + useCallback, + useEffect, + useReducer, + useRef, + useState, + useSyncExternalStore, +} from "react"; import { SettingsFormModel } from "@deepseek-ai/dsh-client-ui-primitives"; import type { SettingsFieldState } from "@deepseek-ai/dsh-client-ui-primitives"; import type { ConfigForm } from "@deepseek-ai/dsh-client-ui-settings/client"; @@ -289,21 +296,40 @@ function ReMeSettingsCard({ envT, view, }: ReMeCardProps): JSX.Element | null { - const snapshot = useSyncExternalStore( - (listener) => scope.subscribe(listener), - () => scope.getSnapshot(), + const subscribe = useCallback( + (listener: () => void) => scope.subscribe(listener), + [scope], ); + const getSnapshot = useCallback(() => scope.getSnapshot(), [scope]); + const snapshot = useSyncExternalStore(subscribe, getSnapshot); // DSH's form model stages the drafts, marks the ones that are invalid and // writes every staged edit as one revision-fenced mutation, so this page owns // only its controls and the field specs behind them. const [model] = useState( () => new SettingsFormModel(scope, SETTINGS_SPECS), ); + const disposeTimer = useRef | undefined>( + undefined, + ); const [, publish] = useReducer((tick: number) => tick + 1, 0); const [busy, setBusy] = useState(false); const [saved, setSaved] = useState(false); const [tab, setTab] = useState<"memory" | "environment">("memory"); - useEffect(() => () => model.dispose(), [model]); + useEffect(() => { + // React StrictMode mounts, cleans up and mounts again in development. + // Disposing the model immediately would hand the remounted card a dead + // one, so the cleanup is deferred and cancelled by the next mount. + if (disposeTimer.current !== undefined) { + clearTimeout(disposeTimer.current); + disposeTimer.current = undefined; + } + return () => { + disposeTimer.current = setTimeout(() => { + disposeTimer.current = undefined; + model.dispose(); + }, 0); + }; + }, [model]); if (view === "summary") return <>{t("description")}; if (snapshot.status === "unavailable") return null; @@ -358,7 +384,9 @@ function ReMeSettingsCard({ type="button" role="tab" id="reme-tab-memory" - aria-controls="reme-tabpanel-memory" + aria-controls={ + tab === "memory" ? "reme-tabpanel-memory" : undefined + } aria-selected={tab === "memory"} className={tab === "memory" ? "active" : ""} onClick={() => setTab("memory")} @@ -369,7 +397,9 @@ function ReMeSettingsCard({ type="button" role="tab" id="reme-tab-environment" - aria-controls="reme-tabpanel-environment" + aria-controls={ + tab === "environment" ? "reme-tabpanel-environment" : undefined + } aria-selected={tab === "environment"} className={tab === "environment" ? "active" : ""} onClick={() => setTab("environment")} diff --git a/integrations/dsh/src/client/status-page.tsx b/integrations/dsh/src/client/status-page.tsx index 062efea2..8abf8e69 100644 --- a/integrations/dsh/src/client/status-page.tsx +++ b/integrations/dsh/src/client/status-page.tsx @@ -294,10 +294,12 @@ export function ReMeStatusPage({ rpc, t, }: ReMeStatusPageProps): JSX.Element | null { - const snapshot = useSyncExternalStore( - (listener) => scope.subscribe(listener), - () => scope.getSnapshot(), + const subscribe = useCallback( + (listener: () => void) => scope.subscribe(listener), + [scope], ); + const getSnapshot = useCallback(() => scope.getSnapshot(), [scope]); + const snapshot = useSyncExternalStore(subscribe, getSnapshot); const settings = snapshot.value; const [tab, setTab] = useState("overview"); const [data, setData] = useState({ loading: true, error: "" }); diff --git a/integrations/dsh/src/environment/client/dependency.tsx b/integrations/dsh/src/environment/client/dependency.tsx index 427d9682..e09281d4 100644 --- a/integrations/dsh/src/environment/client/dependency.tsx +++ b/integrations/dsh/src/environment/client/dependency.tsx @@ -253,6 +253,9 @@ export function DependencySection({ const { checkData, checkError, checking, install, now } = dependency; const [confirming, setConfirming] = useState(false); const [mirror, setMirror] = useState(""); + // The confirm copy follows the CTA: a Reinstall downloads again with + // `--force-reinstall`, so the first-install wording would be misleading. + const reinstall = checkData?.installed === true && checkData.versionOk; const badge = !checkData ? undefined @@ -307,9 +310,13 @@ export function DependencySection({

) : null} {checkData?.error ? ( -

- {`${t("checkRemeFailed")}: ${checkData.error}`} -

+ <> +

{t("checkRemeFailed")}

+
+ {t("errorsTitle")} + 
{checkData.error}
+
+ ) : null} {checkData ? (
@@ -387,7 +394,11 @@ export function DependencySection({ {confirming ? (
-

{fill(t("installConfirm"), { path: pythonPath })}

+

+ {fill(t(reinstall ? "reinstallConfirm" : "installConfirm"), { + path: pythonPath, + })} +

- {current && connection.phase === "fail" && connection.error ? ( + {current && connectionError ? (

- {connection.error} + {connectionError}

) : null} {settingsErrorText ? ( diff --git a/integrations/dsh/src/environment/endpoint-key.ts b/integrations/dsh/src/environment/endpoint-key.ts new file mode 100644 index 00000000..bf643abe --- /dev/null +++ b/integrations/dsh/src/environment/endpoint-key.ts @@ -0,0 +1,21 @@ +/** + * Canonical identity of an endpoint, shared by the Host (service records) and + * the tab (matching an answer to the URL on screen). + * + * Loopback aliases and default ports compare equal, so saving + * `localhost:2333` does not make the service this plugin started on + * `127.0.0.1:2333` look foreign. This module stays browser-safe: no Node + * built-ins, because the client imports it too. + */ +export function serviceEndpointKey(endpoint: string): string { + try { + const url = new URL(endpoint.trim()); + const hostname = url.hostname.toLowerCase(); + const host = hostname === "localhost" ? "127.0.0.1" : hostname; + const port = url.port || (url.protocol === "https:" ? "443" : "80"); + const path = url.pathname.replace(/\/+$/, ""); + return `${url.protocol}//${host}:${port}${path}`; + } catch { + return endpoint.trim(); + } +} diff --git a/integrations/dsh/src/environment/host/gateway.ts b/integrations/dsh/src/environment/host/gateway.ts index 08a8d5dc..5d8243b5 100644 --- a/integrations/dsh/src/environment/host/gateway.ts +++ b/integrations/dsh/src/environment/host/gateway.ts @@ -11,9 +11,9 @@ import type { ServiceLog, ServiceStartResult, ServiceState, + ServiceStopResult, } from "../types.js"; import type { ReMeEnvManager } from "./manager.js"; -import type { ServiceStopResult } from "./service.js"; declare module "@deepseek-ai/cordis" { interface Context { diff --git a/integrations/dsh/src/environment/host/install.ts b/integrations/dsh/src/environment/host/install.ts index a5a5a2cf..ad290d72 100644 --- a/integrations/dsh/src/environment/host/install.ts +++ b/integrations/dsh/src/environment/host/install.ts @@ -3,7 +3,7 @@ import { closeSync, fstatSync, mkdirSync, openSync, readSync } from "node:fs"; import { join } from "node:path"; import type { InstallPhase, InstallRequest, InstallStatus } from "../types.js"; -import { openAppendLog, pruneInstallLogs } from "./log.js"; +import { INSTALL_LOG_KEEP, openAppendLog, pruneInstallLogs } from "./log.js"; import { isAbsolutePath } from "./paths.js"; export interface InstallChild { @@ -212,7 +212,6 @@ export class InstallJobs { 36, )}-${(this.sequence += 1).toString(36)}`; mkdirSync(this.options.logDir, { recursive: true }); - pruneInstallLogs(this.options.logDir); const logPath = join(this.options.logDir, `${id}.log`); const child = this.options.spawner.start({ exe: pythonPath, @@ -231,6 +230,15 @@ export class InstallJobs { }; this.jobs.set(id, job); this.latest = id; + // Prune once the new log exists, so the steady state really is `keep` + // files rather than one more than the documented number. + pruneInstallLogs( + this.options.logDir, + INSTALL_LOG_KEEP, + [...this.jobs.values()] + .filter((running) => running.phase === "running") + .map((running) => running.logPath), + ); child.onExit((code) => this.finish(job, code)); return this.snapshot(job); } diff --git a/integrations/dsh/src/environment/host/log.ts b/integrations/dsh/src/environment/host/log.ts index a6aeea74..6cb8fb1c 100644 --- a/integrations/dsh/src/environment/host/log.ts +++ b/integrations/dsh/src/environment/host/log.ts @@ -6,7 +6,7 @@ import { rmSync, statSync, } from "node:fs"; -import { dirname, join } from "node:path"; +import { basename, dirname, join } from "node:path"; /** Rotate a log once it grows past this size; only one backup is kept. */ export const LOG_ROTATE_BYTES = 5 * 1024 * 1024; @@ -35,12 +35,18 @@ export function openAppendLog( /** * Keep only the newest `keep` per-job logs. Each install writes a new file, so * size-based rotation alone never bounds the directory. + * + * `active` are logs a running job is still writing; they are never removed. + * Each removal is attempted on its own, because one locked file (Windows) must + * not abort the rest of the housekeeping. */ export function pruneInstallLogs( directory: string, keep = INSTALL_LOG_KEEP, + active: readonly string[] = [], ): void { try { + const keepSet = new Set(active.map((path) => basename(path))); const entries = readdirSync(directory).filter( (name) => name.startsWith("install-") && name.endsWith(".log"), ); @@ -54,8 +60,14 @@ export function pruneInstallLogs( } }) .sort((left, right) => right.mtime - left.mtime); - for (const old of byAge.slice(keep)) - rmSync(join(directory, old.name), { force: true }); + for (const old of byAge.slice(keep)) { + if (keepSet.has(old.name)) continue; + try { + rmSync(join(directory, old.name), { force: true }); + } catch { + // A log another job still holds open is retried next time. + } + } } catch { // Pruning is housekeeping; a missing directory is not an error. } diff --git a/integrations/dsh/src/environment/host/manager.ts b/integrations/dsh/src/environment/host/manager.ts index 0860a51b..3d2e75fb 100644 --- a/integrations/dsh/src/environment/host/manager.ts +++ b/integrations/dsh/src/environment/host/manager.ts @@ -18,7 +18,7 @@ import { startService, stopService, } from "./service.js"; -import type { ServiceIO, ServiceStopResult } from "./service.js"; +import type { ServiceIO } from "./service.js"; import { envSummary, mergeEnvFile, @@ -39,6 +39,7 @@ import type { ServiceLog, ServiceStartResult, ServiceState, + ServiceStopResult, } from "../types.js"; const DEFAULT_ENDPOINT = "http://127.0.0.1:2333"; @@ -177,6 +178,7 @@ export class ReMeEnvManager { ok: health.ok, version: health.version, error: health.error, + ...(health.code === undefined ? {} : { code: health.code }), checkedAt: new Date().toISOString(), }; } @@ -257,7 +259,10 @@ export class ReMeEnvManager { serviceLog(workspaceDir: string): ServiceLog { const dir = resolveWorkspaceDir(workspaceDir, homedir()); const logPath = join(dir, "service.log"); - return { logPath, lines: tailLog(logPath, 40) }; + // Rotation moves the previous launch's crash output to `service.log.1`; if + // the current file is still empty, that backup is the only diagnostic. + const lines = tailLog(logPath, 40) || tailLog(`${logPath}.1`, 40); + return { logPath, lines }; } /** DSH-startup best effort: start a stopped local service when configured. */ @@ -397,8 +402,9 @@ export class ReMeEnvManager { } // A record for a *different* endpoint that is still alive would be // overwritten by the new launch, leaving that endpoint unable to stop it. - // Only a provably owned process blocks the new start; an unprovable record - // is stale and is discarded, just like the same-endpoint case below. + // A provably owned or an unverifiable process blocks the new start; only a + // provably foreign record is discarded, just like the same-endpoint case + // below. const recorded = readServiceState(this.serviceStatePath); if ( recorded && @@ -422,11 +428,25 @@ export class ReMeEnvManager { error: `A service for ${recorded.endpoint} is still running; stop it from that endpoint first`, code: "other-endpoint-running", }; + // An unreadable image/command line is not proof the process is gone: + // dropping the record would orphan a live service this plugin can no + // longer stop. Keep it and report why the start is blocked. + if (other === "unknown") + return { + started: false, + alreadyRunning: false, + command: "", + logPath, + startedAt: now, + error: + "The recorded service identity could not be verified; leaving it untouched", + code: "unverified-record", + }; removeServiceState(this.serviceStatePath); } // A previous launch that never became healthy may still be alive. Stop it - // when ownership is proven; when it cannot be proven, drop the stale record - // without killing anything so a recycled pid is never touched. + // when ownership is proven; an unprovable record is kept and reported, + // because dropping it would orphan a live process the tab cannot stop. if (current.pid !== undefined) { const ownership = await serviceOwnership( this.serviceIO, @@ -451,6 +471,19 @@ export class ReMeEnvManager { }`, code: "previous-not-stopped", }; + } else if (ownership === "unknown") { + // PowerShell/ps failed to describe a live process. Discarding the + // record here would orphan it: the tab could never stop or replace it. + return { + started: false, + alreadyRunning: false, + command: "", + logPath, + startedAt: now, + error: + "The recorded service identity could not be verified; leaving it untouched", + code: "unverified-record", + }; } else { removeServiceState(this.serviceStatePath); } diff --git a/integrations/dsh/src/environment/host/service.ts b/integrations/dsh/src/environment/host/service.ts index 8b86da84..2abaa49a 100644 --- a/integrations/dsh/src/environment/host/service.ts +++ b/integrations/dsh/src/environment/host/service.ts @@ -12,14 +12,18 @@ import { createConnection } from "node:net"; import { basename, dirname } from "node:path"; import { safeError } from "../../endpoint.js"; +import { serviceEndpointKey } from "../endpoint-key.js"; import type { + HealthErrorCode, ServiceStartResult, ServiceState, - ServiceStopReasonCode, + ServiceStopResult, } from "../types.js"; import { openAppendLog } from "./log.js"; import { isWindowsPath } from "./paths.js"; +export { serviceEndpointKey } from "../endpoint-key.js"; + /** * The local ReMe service is a *detached* process: it outlives DSH, which is the * point of starting it from here, so it is deliberately not owned by DSH's @@ -43,15 +47,8 @@ export interface ServiceHealth { version: string; /** Non-empty when the endpoint answered but not as a ReMe service. */ error: string; -} - -export interface ServiceStopResult { - stopped: boolean; - pid?: number; - /** Why an unmanaged or missing service was left alone. */ - reason?: string; - /** Stable reason the client localizes; `reason` stays for diagnostics. */ - code?: ServiceStopReasonCode; + /** Stable reason the client localizes instead of showing `error`. */ + code?: HealthErrorCode; } export interface ServiceIO { @@ -84,24 +81,8 @@ export function isValidPid(pid: unknown): pid is number { } /** - * Canonical identity of an endpoint for service-record matching. Loopback - * aliases and default ports compare equal, so saving `localhost:2333` does not - * make the service this plugin started on `127.0.0.1:2333` look foreign. + * Only loopback HTTP endpoints may be managed as a local service. */ -export function serviceEndpointKey(endpoint: string): string { - try { - const url = new URL(endpoint.trim()); - const hostname = url.hostname.toLowerCase(); - const host = hostname === "localhost" ? "127.0.0.1" : hostname; - const port = url.port || (url.protocol === "https:" ? "443" : "80"); - const path = url.pathname.replace(/\/+$/, ""); - return `${url.protocol}//${host}:${port}${path}`; - } catch { - return endpoint.trim(); - } -} - -/** Only loopback HTTP endpoints may be managed as a local service. */ export function parseLocalEndpoint(endpoint: string): { host: string; port: number; @@ -296,10 +277,12 @@ function commandMatches(command: string, persisted: PersistedService): boolean { const marker = `workspace_dir=${normalizeCommandText(workspace, foldCase)}`; const index = text.indexOf(marker); if (index < 0) return false; - // Require a boundary so `C:\a` does not match another service using `C:\ab`. - // A backslash remains a boundary for the `\"` form older shells produce. + // Require an argument boundary. A path separator is *not* one: the record + // `C:\a` must not match a launch for `C:\a\b` (POSIX `/repo` vs `/repo/sub`). + // The `\"` form Windows shows for the wrapper is folded by + // `normalizeCommandText`, so the quote itself is the boundary there. const next = text[index + marker.length] ?? ""; - if (next && !/[\s"',\]\\]/.test(next)) return false; + if (next && !/[\s"',\]]/.test(next)) return false; return new RegExp(`service\\.port=${port}(?!\\d)`).test(text); } @@ -659,16 +642,27 @@ export function createServiceIO(): ServiceIO { metadata?: { health?: { version?: unknown } }; }; if (!response.ok) - return { ok: false, version: "", error: `HTTP ${response.status}` }; + return { + ok: false, + version: "", + error: `HTTP ${response.status}`, + code: "http-status" as const, + }; const health = body.metadata?.health; if (body.success === false) - return { ok: false, version: "", error: "ReMe reported an error" }; + return { + ok: false, + version: "", + error: "ReMe reported an error", + code: "reme-error" as const, + }; // A generic HTTP server answering 200 with `{}` is not ReMe. if (typeof health !== "object" || health === null) return { ok: false, version: "", error: "The endpoint did not answer with a ReMe health report", + code: "not-reme" as const, }; const version = health.version; return { @@ -677,7 +671,12 @@ export function createServiceIO(): ServiceIO { error: "", }; } catch (error) { - return { ok: false, version: "", error: safeError(error) }; + return { + ok: false, + version: "", + error: safeError(error), + code: "unreachable" as const, + }; } finally { clearTimeout(timer); } @@ -697,7 +696,7 @@ export function createServiceIO(): ServiceIO { }, async spawnService({ exe, args, cwd, logPath, env }) { const fd = openAppendLog(logPath); - let child: ReturnType; + let child: ReturnType | undefined; try { child = spawn(exe, args, { shell: false, @@ -707,36 +706,40 @@ export function createServiceIO(): ServiceIO { env, stdio: ["ignore", fd, fd], }); + const started = child; await new Promise((resolve, reject) => { // A spawn acknowledgement that never arrives must not block the // start queue (and therefore a queued Stop) forever. const timer = setTimeout(() => { try { - child.kill(); + started.kill(); } catch { // Already gone. } reject(new Error("spawn did not acknowledge within 10 s")); }, 10_000); timer.unref(); - child.once("spawn", () => { + started.once("spawn", () => { clearTimeout(timer); resolve(); }); - child.once("error", (error: Error) => { + started.once("error", (error: Error) => { clearTimeout(timer); reject(error); }); }); } finally { // The child owns its inherited handle; the parent copy is no longer - // needed, and an unref'd child survives this Host exiting. + // needed. The listener and `unref()` must run on the timeout path too: + // otherwise a late acknowledgement keeps the child referenced and its + // `error` event unhandled, and the Host cannot exit. closeSync(fd); + if (child !== undefined) { + child.on("error", () => undefined); + child.unref(); + } } - // A failed executable is reported here rather than as an unhandled - // `error` event that would exit the Host. - child.on("error", () => undefined); - child.unref(); + if (child === undefined) throw new Error("spawn did not start"); return { pid: child.pid }; }, killTree(pid, force = false) { diff --git a/integrations/dsh/src/environment/types.ts b/integrations/dsh/src/environment/types.ts index 691a5f43..3a2f7c80 100644 --- a/integrations/dsh/src/environment/types.ts +++ b/integrations/dsh/src/environment/types.ts @@ -114,6 +114,13 @@ export interface LlmTestResult { error: string; } +/** Stable reason a health probe failed, so the client can localize it. */ +export type HealthErrorCode = + | "http-status" + | "not-reme" + | "reme-error" + | "unreachable"; + /** Whether the configured endpoint answers, probed from the DSH Host. */ export interface ConnectionHealth { endpoint: string; @@ -121,6 +128,8 @@ export interface ConnectionHealth { version: string; /** Non-empty when the endpoint answered but not as a ReMe service. */ error: string; + /** Stable reason; the client shows localized copy instead of `error`. */ + code?: HealthErrorCode; checkedAt: string; } @@ -141,6 +150,7 @@ export type ServiceStartErrorCode = | "conflict" | "other-endpoint-running" | "previous-not-stopped" + | "unverified-record" | "no-python" | "not-installed" | "version-too-old" @@ -170,3 +180,16 @@ export interface ServiceLog { logPath: string; lines: string; } + +/** + * Outcome of one Stop request. It crosses the `remeEnv` boundary, so it lives + * here rather than beside the Host-only process adapter. + */ +export interface ServiceStopResult { + stopped: boolean; + pid?: number; + /** Why an unmanaged or missing service was left alone. */ + reason?: string; + /** Stable reason the client localizes; `reason` stays for diagnostics. */ + code?: ServiceStopReasonCode; +} diff --git a/integrations/dsh/tests/env-manager.test.mjs b/integrations/dsh/tests/env-manager.test.mjs index 6f31d04c..ebba6765 100644 --- a/integrations/dsh/tests/env-manager.test.mjs +++ b/integrations/dsh/tests/env-manager.test.mjs @@ -160,7 +160,7 @@ test("stops a previous launch that never became healthy before replacing it", as assert.equal(calls.spawn.length, 1); }); -test("an unprovable previous record is discarded without killing it", async (t) => { +test("an unprovable previous record blocks the start instead of orphaning it", async (t) => { const { manager, calls, directory, io } = managerIn(t); manager.serviceStatus = async (endpoint) => ({ phase: "stopped", @@ -179,17 +179,20 @@ test("an unprovable previous record is discarded without killing it", async (t) startedAt: "2026-10-27T00:00:00.000Z", }); // The process exists, but its command line cannot be read, so ownership is - // unproven: never kill it, just drop the stale record and start. + // unproven. Dropping the record would orphan it: it must stay, and the start + // must report why instead of spawning a second service. io.processCommandLine = async () => ""; const result = await manager.serviceStart( "http://127.0.0.1:2334", "D:\\env\\python.exe", join(directory, "workspace"), ); - assert.equal(result.started, true); + assert.equal(result.started, false); + assert.equal(result.code, "unverified-record"); assert.deepEqual(calls.killTree, []); - assert.equal(calls.spawn.length, 1); - assert.equal(readServiceState(statePath)?.pid, 5001); + assert.equal(calls.spawn.length, 0); + // Once the evidence is readable again, the same record can be stopped. + assert.equal(readServiceState(statePath)?.pid, 6000); }); test("refuses to overwrite a live record owned by another endpoint", async (t) => { @@ -259,6 +262,15 @@ test("probes health from the Host and rejects a non-http endpoint", async (t) => await assert.rejects(() => manager.health("file:///etc/passwd"), /http/); }); +test("shows the rotated service log when the current one is empty", (t) => { + const { manager, directory } = managerIn(t); + writeFileSync(join(directory, "service.log"), ""); + writeFileSync(join(directory, "service.log.1"), "Traceback: boom\n"); + const log = manager.serviceLog(directory); + assert.equal(log.logPath, join(directory, "service.log")); + assert.match(log.lines, /boom/); +}); + test("validates the merged .env, not just the submitted values", async (t) => { const { manager, directory } = managerIn(t); const envPath = join(directory, ".env"); diff --git a/integrations/dsh/tests/env-reme.test.mjs b/integrations/dsh/tests/env-reme.test.mjs index b3e9dc05..43ca21d2 100644 --- a/integrations/dsh/tests/env-reme.test.mjs +++ b/integrations/dsh/tests/env-reme.test.mjs @@ -1,8 +1,8 @@ import assert from "node:assert/strict"; import test from "node:test"; -import { mkdtempSync, rmSync, writeFileSync } from "node:fs"; +import { mkdtempSync, readdirSync, rmSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; -import { join } from "node:path"; +import { basename, join } from "node:path"; import { checkReme, @@ -180,7 +180,10 @@ test("tails a log with CRLF and progress carriage returns", (t) => { function fakeSpawner() { const children = []; const spawner = { - start() { + start(options) { + // The real spawner opens the job log before spawning, so pruning must see + // the new file too. + if (options?.logPath) writeFileSync(options.logPath, ""); const child = { pid: 4000 + children.length, killed: false, @@ -236,6 +239,19 @@ test("finds the reme CLI in a POSIX bin directory", () => { assert.equal(cli, "/home/u/venv/bin/reme"); }); +test("keeps exactly the documented number of install logs", (t) => { + const { jobs, directory } = makeJobs(t); + for (let index = 0; index < 24; index += 1) + writeFileSync(join(directory, `install-old-${index}.log`), "x"); + const started = jobs.start({ pythonPath: "C:\\env\\python.exe" }); + const remaining = readdirSync(directory).filter((name) => + name.endsWith(".log"), + ); + // The steady state is the documented count, not one more than it. + assert.equal(remaining.length, 20); + assert.ok(remaining.includes(basename(started.logPath))); +}); + test("tracks a successful pip job through exit", (t) => { const { jobs, children, directory } = makeJobs(t); const started = jobs.start({ pythonPath: "C:\\env\\python.exe" }); diff --git a/integrations/dsh/tests/env-service.test.mjs b/integrations/dsh/tests/env-service.test.mjs index e5cbfedd..2e3e6c2f 100644 --- a/integrations/dsh/tests/env-service.test.mjs +++ b/integrations/dsh/tests/env-service.test.mjs @@ -266,10 +266,13 @@ test("a recycled pid that is no longer the service is not managed", async (t) => false, ); // Matching is boundary-aware: a longer workspace path or port is another - // launch, not this one. + // launch, not this one. A path separator is not a boundary either, so a + // record for `...\reme-x` never claims a service started in `...\reme-x\sub`. const state = readServiceState(statePath); for (const command of [ `reme start workspace_dir=${state.workspaceDir}2 service.port=2334`, + `reme start workspace_dir=${state.workspaceDir}\\sub service.port=2334`, + `reme start workspace_dir=${state.workspaceDir}/sub service.port=2334`, `reme start workspace_dir=${state.workspaceDir} service.port=23340`, ]) { const { io: longer } = fakeIO({ @@ -465,10 +468,26 @@ test("real IO accepts only a ReMe health report", async (t) => { const good = await io.fetchHealth(reme, ""); assert.equal(good.ok, true); assert.equal(good.version, "0.4.2.1"); + assert.equal(good.code, undefined); const empty = await io.fetchHealth(generic, ""); assert.equal(empty.ok, false); assert.match(empty.error, /ReMe health report/); + // The client localizes the code instead of showing this English sentence. + assert.equal(empty.code, "not-reme"); + + const failing = await listening((_request, response) => { + response.statusCode = 503; + response.end("{}"); + }, t); + const httpError = await io.fetchHealth(failing, ""); + assert.equal(httpError.code, "http-status"); + + const unhappy = await listening((_request, response) => { + response.end(JSON.stringify({ success: false })); + }, t); + const reported = await io.fetchHealth(unhappy, ""); + assert.equal(reported.code, "reme-error"); // The Authorization header is actually sent when a token is configured. let authorization = ""; diff --git a/integrations/dsh/tests/env-tab.test.mjs b/integrations/dsh/tests/env-tab.test.mjs index 13f13b36..32a6f903 100644 --- a/integrations/dsh/tests/env-tab.test.mjs +++ b/integrations/dsh/tests/env-tab.test.mjs @@ -519,6 +519,9 @@ test("a start that died before listening reports immediately", async (t) => { }); const copy = texts(renderer.toJSON()).join(" "); assert.ok(copy.includes(envZh.startFailedStopped)); + // The message is already a complete sentence; prefixing it would read + // "Service operation failed: The service exited…". + assert.equal(copy.includes(`${envZh.serviceError}:`), false); // Ready to retry without waiting out the 120 s deadline. assert.ok(buttonByText(renderer, envZh.startService)); }); @@ -630,6 +633,12 @@ test("Reinstall forces a reinstall while install and upgrade use the right flags await act(async () => { buttonByText(reinstall.renderer, envZh.reinstall).props.onClick(); }); + // The confirmation copy follows the Reinstall CTA, not the first-install one. + assert.ok( + texts(reinstall.renderer.toJSON()) + .join(" ") + .includes(envZh.reinstallConfirm.split("{path}")[0]), + ); await act(async () => { buttonByText(reinstall.renderer, envZh.confirmInstall).props.onClick(); }); @@ -739,3 +748,321 @@ test("switching the endpoint aborts the previous start poll", async (t) => { assert.ok(copy.includes("PID 1111"), "the new endpoint's pid is shown"); assert.equal(copy.includes("PID 5000"), false); }); + +test("a live record that never listened offers Stop instead of 'Starting'", async (t) => { + // The Host reports a recorded launch that is still alive but not listening + // as `stopped` + pid. Mapping that to `starting` on every read used to lock + // the card: Start disabled, no Stop, and Recheck repeated the same answer. + const stale = { + phase: "stopped", + managed: false, + pid: 6000, + endpoint: "http://127.0.0.1:2333", + logPath: "C:\\Users\\test\\.reme\\service.log", + }; + const { renderer, restore } = await mount( + {}, + { responses: { "remeEnv/serviceStatus": stale } }, + ); + t.after(restore); + const copy = texts(renderer.toJSON()).join(" "); + assert.equal(copy.includes(envZh.serviceBadgeStarting), false); + assert.ok(copy.includes(envZh.serviceBadgeStale)); + assert.ok(copy.includes(envZh.staleNote)); + const start = renderer.root.findAll( + (node) => + node.type === "button" && + textOf(node.props.children) === envZh.startService, + ); + assert.equal(start.length, 0, "Start is replaced by the Stop way out"); + const stop = buttonByText(renderer, envZh.stopService); + assert.equal(stop.props.disabled, false, "the recorded pid can be stopped"); +}); + +test("an owned conflict during a start keeps polling until healthy", async (t) => { + const stop = (pid) => ({ + phase: "stopped", + managed: false, + ...(pid === undefined ? {} : { pid }), + endpoint: "http://127.0.0.1:2333", + logPath: "C:\\Users\\test\\.reme\\service.log", + }); + const conflict = { + phase: "conflict", + managed: true, + pid: 6000, + endpoint: "http://127.0.0.1:2333", + logPath: "C:\\Users\\test\\.reme\\service.log", + }; + const running = { + phase: "running", + managed: true, + pid: 6000, + version: "0.4.0.9", + endpoint: "http://127.0.0.1:2333", + logPath: "C:\\Users\\test\\.reme\\service.log", + }; + let statusCalls = 0; + const { renderer, restore } = await mount( + {}, + { + responses: { + "remeEnv/serviceStatus": () => { + statusCalls += 1; + if (statusCalls === 1) return stop(); + if (statusCalls === 2) return conflict; + return running; + }, + "remeEnv/serviceStart": { + started: true, + alreadyRunning: false, + command: "", + logPath: running.logPath, + startedAt: new Date().toISOString(), + }, + }, + }, + ); + t.after(restore); + await act(async () => { + buttonByText(renderer, envZh.startService).props.onClick(); + }); + await act(async () => { + await new Promise((resolve) => setTimeout(resolve, 1_200)); + }); + // One owned conflict is our own launch booting; it must not become a Stop + // offer that would kill a service which is about to answer. + assert.ok( + texts(renderer.toJSON()).join(" ").includes(envZh.serviceBadgeStarting), + ); + await act(async () => { + await new Promise((resolve) => setTimeout(resolve, 1_100)); + }); + const copy = texts(renderer.toJSON()).join(" "); + assert.ok(copy.includes("ReMe 0.4.0.9")); + assert.equal(copy.includes(envZh.conflictManagedNote), false); +}); + +test("a Host conflict refusal shows the conflict card, not an error", async (t) => { + const stopped = { + phase: "stopped", + managed: false, + endpoint: "http://127.0.0.1:2333", + logPath: "C:\\Users\\test\\.reme\\service.log", + }; + const conflict = { + phase: "conflict", + managed: false, + endpoint: "http://127.0.0.1:2333", + logPath: stopped.logPath, + }; + let statusCalls = 0; + const { renderer, restore } = await mount( + {}, + { + responses: { + "remeEnv/serviceStatus": () => { + statusCalls += 1; + return statusCalls === 1 ? stopped : conflict; + }, + "remeEnv/serviceStart": { + started: false, + alreadyRunning: false, + error: "Another process is already listening on this endpoint", + code: "conflict", + command: "", + logPath: stopped.logPath, + startedAt: new Date().toISOString(), + }, + }, + }, + ); + t.after(restore); + await act(async () => { + buttonByText(renderer, envZh.startService).props.onClick(); + }); + await act(async () => { + await new Promise((resolve) => setTimeout(resolve, 50)); + }); + const copy = texts(renderer.toJSON()).join(" "); + assert.ok(copy.includes(envZh.conflictNote)); + assert.equal(copy.includes(envZh.startErrorConflict), false); +}); + +test("a slow status answer does not overwrite a finished start", async (t) => { + const stopped = { + phase: "stopped", + managed: false, + endpoint: "http://127.0.0.1:2333", + logPath: "C:\\Users\\test\\.reme\\service.log", + }; + const running = { + phase: "running", + managed: true, + pid: 7000, + version: "0.4.0.9", + endpoint: "http://127.0.0.1:2333", + logPath: stopped.logPath, + }; + let statusCalls = 0; + const { renderer, restore } = await mount( + {}, + { + responses: { + "remeEnv/serviceStatus": () => { + statusCalls += 1; + if (statusCalls === 1) return stopped; + // The manual Recheck answers late, after the start poll has settled. + if (statusCalls === 2) + return new Promise((resolve) => + setTimeout(() => resolve(stopped), 1_500), + ); + return running; + }, + "remeEnv/serviceStart": { + started: true, + alreadyRunning: false, + command: "", + logPath: stopped.logPath, + startedAt: new Date().toISOString(), + }, + }, + }, + ); + t.after(restore); + await act(async () => { + buttonByText(renderer, envZh.startService).props.onClick(); + }); + const rechecks = renderer.root.findAll( + (node) => + node.type === "button" && textOf(node.props.children) === envZh.recheck, + ); + await act(async () => { + rechecks.at(-1).props.onClick(); + }); + await act(async () => { + await new Promise((resolve) => setTimeout(resolve, 2_300)); + }); + const copy = texts(renderer.toJSON()).join(" "); + assert.ok(copy.includes("ReMe 0.4.0.9")); + assert.equal(copy.includes(envZh.serviceBadgeStopped), false); +}); + +test("a Stop result for the previous endpoint is dropped", async (t) => { + const OLD = "http://127.0.0.1:2333"; + const NEW = "http://127.0.0.1:2444"; + const oldRunning = { + phase: "running", + managed: true, + pid: 5000, + version: "0.4.0.9", + endpoint: OLD, + logPath: "C:\\Users\\test\\.reme\\service.log", + }; + const newRunning = { + ...oldRunning, + pid: 1111, + version: "9.9.9", + endpoint: NEW, + }; + let release; + const stopResult = new Promise((resolve) => { + release = resolve; + }); + const { renderer, setValue, restore } = await mount( + {}, + { + responses: { + "remeEnv/serviceStatus": (input) => + input?.endpoint === NEW ? newRunning : oldRunning, + "remeEnv/serviceStop": () => stopResult, + }, + }, + ); + t.after(restore); + await act(async () => { + buttonByText(renderer, envZh.stopService).props.onClick(); + }); + await act(async () => { + buttonByText(renderer, envZh.stopService).props.onClick(); + }); + await act(async () => { + setValue({ endpoint: NEW }); + }); + await act(async () => { + release({ + stopped: false, + reason: "could not be verified", + code: "unverified", + }); + }); + await act(async () => { + await new Promise((resolve) => setTimeout(resolve, 30)); + }); + const copy = texts(renderer.toJSON()).join(" "); + assert.equal(copy.includes(envZh.stopUnknownIdentity), false); + const stops = renderer.root.findAll( + (node) => + node.type === "button" && + textOf(node.props.children) === envZh.stopService, + ); + assert.ok(stops.length > 0); + assert.equal(stops[0].props.disabled, false); +}); + +test("localizes a Host health failure code", async (t) => { + const { renderer, restore } = await mount( + { endpoint: "http://10.0.0.5:2333" }, + { + responses: { + "remeEnv/health": { + endpoint: "http://10.0.0.5:2333", + ok: false, + version: "", + error: "The endpoint did not answer with a ReMe health report", + code: "not-reme", + checkedAt: new Date().toISOString(), + }, + }, + }, + ); + t.after(restore); + const copy = texts(renderer.toJSON()).join(" "); + assert.ok(copy.includes(envZh.healthErrorNotReme)); + assert.equal( + copy.includes("did not answer with a ReMe health report"), + false, + ); +}); + +test("a Python refresh keeps the previous interpreter list on screen", async (t) => { + let pythonCalls = 0; + const { renderer, restore } = await mount( + {}, + { + responses: { + "remeEnv/python": () => { + pythonCalls += 1; + // The refresh never answers; the previous detection must stay. + return pythonCalls === 1 + ? RESPONSES["remeEnv/python"] + : new Promise(() => {}); + }, + }, + }, + ); + t.after(restore); + await act(async () => { + buttonByText(renderer, envZh.changeInterpreter).props.onClick(); + }); + assert.equal(classesOf(renderer.toJSON()).get("reme-env-option"), 6); + await act(async () => { + buttonByText(renderer, envZh.refresh).props.onClick(); + }); + await act(async () => { + await new Promise((resolve) => setTimeout(resolve, 30)); + }); + const copy = texts(renderer.toJSON()).join(" "); + assert.equal(copy.includes(envZh.emptyTitle), false); + assert.equal(classesOf(renderer.toJSON()).get("reme-env-option"), 6); +});