From f840986927c074b47c20c364b02f39022730f54e Mon Sep 17 00:00:00 2001 From: JUN Date: Sat, 12 Sep 2026 13:40:33 +0900 Subject: [PATCH 1/3] docs: plan operations fixes and remote verification slices --- devlog/_plan/260912_operations/000_plan.md | 34 +++++++++++++++++++ .../260912_operations/001_roadmap_audit.md | 7 ++++ devlog/_plan/260912_operations/010_update.md | 11 ++++++ .../_plan/260912_operations/020_listeners.md | 13 +++++++ devlog/_plan/260912_operations/030_totals.md | 11 ++++++ .../260912_operations/040_client_usage.md | 13 +++++++ devlog/_plan/260912_operations/050_pairing.md | 13 +++++++ .../_plan/260912_operations/060_transport.md | 7 ++++ .../260912_operations/070_verification.md | 9 +++++ 9 files changed, 118 insertions(+) create mode 100644 devlog/_plan/260912_operations/000_plan.md create mode 100644 devlog/_plan/260912_operations/001_roadmap_audit.md create mode 100644 devlog/_plan/260912_operations/010_update.md create mode 100644 devlog/_plan/260912_operations/020_listeners.md create mode 100644 devlog/_plan/260912_operations/030_totals.md create mode 100644 devlog/_plan/260912_operations/040_client_usage.md create mode 100644 devlog/_plan/260912_operations/050_pairing.md create mode 100644 devlog/_plan/260912_operations/060_transport.md create mode 100644 devlog/_plan/260912_operations/070_verification.md diff --git a/devlog/_plan/260912_operations/000_plan.md b/devlog/_plan/260912_operations/000_plan.md new file mode 100644 index 0000000000..f8ad96b84b --- /dev/null +++ b/devlog/_plan/260912_operations/000_plan.md @@ -0,0 +1,34 @@ +# Operations delivery roadmap + +Operators need accurate startup failures, safe update retry cleanup, readable usage, and actionable connected-client guidance. This unit carries the remaining reviewed contributions and repairs their current callers. The original stop-refusal implementation is already present on dev; it is not applied twice. + +Loop: satisfy-spec HOTL, triggered by the operations lane assignment. Goal: reviewable PRs with final-tip hosted evidence and a durable handoff. Non-goals: merging, closing originals, releases, live service/config changes, local product suites/build/typecheck/install. Scope: this managed worktree and explicitly authorized GitHub PR writes; existing credentials only. No user token/time/agent-count cap. Stop only after every disposition, acceptance row and final-tip result is recorded, or an actual unavailable external gate is documented. Memory artifact: this unit and ignored .tmp/operations/handoff.md. Escalation: real tool/access denial or scope beyond assigned issues; main owns code and decisions. Native architect role is unavailable; supported inherited design reviews plus separate A audits follow explicit user direction. + +## Work-phase map + +| ID | Document | Outcome | Dependency | +| --- | --- | --- | --- | +| roadmap | 000 + all decade docs | Docs-only source-grounded roadmap | none | +| update | 010_update.md | Retire observed exited pinned children (#4185) | roadmap | +| listeners | 020_listeners.md | Name auxiliary bind failures and malformed edits (#4236 residual) | roadmap | +| totals | 030_totals.md | Keep readable usage and disclose omissions (#4111) | roadmap | +| client-usage | 040_client_usage.md | Hub usage scoped to connected client (#4205) | roadmap; preserve totals contract if shared | +| pairing | 050_pairing.md | Hub identity and origin-specific browser authentication (#4206/#4208) | roadmap | +| transport | 060_transport.md | Reviewed local management catalog read (#4315/#4317) | roadmap | +| verification | 070_verification.md | Exact tips, hosted results, credits, handoff | all implementations | + +Independent changes use independent branches/PRs from the fetched dev baseline plus the common roadmap checkpoint. Only actual shared-code dependencies become an ordinary manual chain. No native stack registration. Intermediate auto-CI stays enabled; final cumulative tips are the acceptance unit. + +## Source disposition + +- #4170 OPEN at 4d72ef010363b80cd78f65148a5228d6797a3117, but dev contains 1ada8f5ff1 and further refusalNextStep behavior in src/lib/process-control.ts. No duplicate carry. Missing-message incident and actual scheduler behavior are not proven resolved by wording. +- #4185 OPEN at 2602f3ceca4b93237436911dcd8dffc35b3b5e57; current src/update/job.ts still uses lastChild.pid alone. +- #4111 OPEN at 2f07acb58b3e73f48cea38334f301b430a8634cd. Readable totals differ from connected-client routing. +- #4317 OPEN / CHANGES_REQUESTED at 27f577aa795f3b968c070a50eded48411bd7e985. Independent security review and own caller-complete patch required. Sensitive analysis stays in ignored scratch. +- #4236 latest public comment identifies only auxiliary listener bind diagnosis and malformed edit reporting as outstanding. #4249/#4250/#4251/#4252/#4254/#4255 are already landed and must not be replayed. + +## Evidence policy + +Local suites of every size, typecheck, builds and install: NOT RUN by explicit user instruction. Regression code goes to GitHub-hosted Cross-platform CI, whose pull_request trigger has no base filter (.github/workflows/ci.yml:3). Docs consistency uses git diff --check and file-map inspection, not claimed test coverage. Source review is not execution. Each final source SHA is paired with its actual run ID/URL/conclusion; skipped/cancelled runs are not passing proof. Every C/D records this limitation and defers behavioral acceptance to final hosted results. + +Existing tree conventions: src/cli owns commands, src/server owns listener/management boundaries, src/update owns update retries, gui/src owns React presentation, tests mirrors domains, structure/manifest.json maps owning docs. No new dependencies, service layer, or settings are needed. diff --git a/devlog/_plan/260912_operations/001_roadmap_audit.md b/devlog/_plan/260912_operations/001_roadmap_audit.md new file mode 100644 index 0000000000..d99db8768d --- /dev/null +++ b/devlog/_plan/260912_operations/001_roadmap_audit.md @@ -0,0 +1,7 @@ +# Roadmap audit outcome + +Independent design reviewers accepted the update, listener, usage, pairing and local-catalog designs after reflection. Native architect selection was unavailable; user-directed inherited model review was used without a native-role claim. Two reflection calls initially reported model capacity errors; same-handle retries completed. + +Independent A reviewer identified nested managementIngress degradation hidden by schema catch and an incomplete CLI regression path. Both were folded in and re-audited. Final verdict: GO-WITH-FIXES (blockers=0); remaining deps.managementOrigin wording corrected before lock. This audit certifies the plan only, not implementation or test behavior. + +The first cycle is docs-only. All eight roadmap files exist, source/test paths were reviewed, and git diff --cached --check exits zero. Product suites/build/typecheck/install NOT RUN by explicit instruction. Next direction: implement the observed-child cleanup slice from 010, then the independent listener slice. Final-tip hosted CI owns behavior acceptance. No retired update architecture or already-carried stop implementation is replayed. diff --git a/devlog/_plan/260912_operations/010_update.md b/devlog/_plan/260912_operations/010_update.md new file mode 100644 index 0000000000..53cb5d8719 --- /dev/null +++ b/devlog/_plan/260912_operations/010_update.md @@ -0,0 +1,11 @@ +# Pinned child retirement + +Class C4: process termination boundary. Dependency: roadmap. Reuse the existing RestartIo and restartAfterUpdate retry loop; no new process supervisor. + +MODIFY src/update/job.ts: RestartIo gains injectable spawnDetachedStartFn, preparePortForPinnedStartFn, waitForGhostListenClearFn and killProxyFn, matching existing functions. Replace Date.now in the pinned retry window with existing io.now. Replace both lastChild.pid-only kills with one closure accepting the ChildProcess: return when pid absent, exitCode non-null, or signalCode non-null; otherwise existing isAlive then kill. Capture the exact spawned child and attach once(exit) to clear lastChild only when lastChild === child. Keep healthy-probe early return. No persisted schema or serialization changes; these are process-local IO seams consumed in the retry loop only. + +MODIFY tests/update/update-job.test.ts: carry #4185 deterministic fake EventEmitter children through actual three retries, using injected clock and no real kill. Cases: successful exit, nonzero exit, signal exit, event retirement, live timeout cleanup, same-PID late old event, healthy last attempt. MODIFY tests/windows/windows-deploy-close-regressions.test.ts: replace obsolete exact PID-expression oracle with reference to behavior regression; keep wrapper ownership assertions. MODIFY structure/runtime.md to state observed-child retirement. Existing process code is otherwise unchanged. + +Exact starting patch: public PR #4185 head 2602f3ceca4b93237436911dcd8dffc35b3b5e57, reviewed source diff retained locally in .tmp/operations/pr-4185.diff. Before: numeric PID may remain after child exit. After: recorded exit/signal or matching exit event retires cleanup authority. This does not make all OS signalling atomic against PID reuse. + +Planned hosted activation checks (not yet executed): seven tests in update/update-job.test.ts exercise both cleanup sites. Local execution NOT RUN. Source check: both sites use the same child-aware closure; no unrelated test weakened. Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com>. diff --git a/devlog/_plan/260912_operations/020_listeners.md b/devlog/_plan/260912_operations/020_listeners.md new file mode 100644 index 0000000000..393ba74e8a --- /dev/null +++ b/devlog/_plan/260912_operations/020_listeners.md @@ -0,0 +1,13 @@ +# Auxiliary bind diagnosis and malformed edit reporting + +Class C3; dependency roadmap. Existing owners: src/server/ports.ts: isAddrInUse, src/server/index.ts: startup transaction, src/cli/index.ts: handleStart retry loop, src/config.ts: loadConfig warnings. + +MODIFY src/server/ports.ts: add AuxiliaryListenerBindError carrying listener name (unauthenticatedLoopbackListener or hub.managementIngress), port, hostname and original cause. Its message names the precise config key/address and directs correction of that effective auxiliary address (including a port-less companion). MODIFY src/server/index.ts: keep synchronous rollback of opened listeners, wrap only each auxiliary Bun.serve failure after rollback. MODIFY src/cli/index.ts: reject AuxiliaryListenerBindError before generic isAddrInUse retry logic. Public-listener EADDRINUSE retains soft/hard-pin behavior; auxiliary collisions never re-pick the public port. Fields are transient Error data, created by both auxiliary catches, consumed by CLI; no serialization. + +MODIFY src/config.ts: reuse load-time degraded-field warning family. When raw unauthenticatedLoopbackListener is present and validated value disappeared, emit field-specific warning without raw values; keep valid providers/keys and file bytes. The same pure warning is exposed by validFileConfigDiagnostics, so doctor/status also report degradation. Existing write-time loopbackListenerPortError stays strict. Whole-hub malformed blocks already warn, but nested hub.managementIngress can be silently swallowed before that check. Add a raw-versus-validated nested-ingress warning to the same owner and diagnostics. Cover string enabled and invalid port for BOTH listener blocks, preserving providers/file bytes. + +MODIFY existing tests/server/ports.test.ts and tests/server/loopback-listener-integration.test.ts : occupied loopback and management ports must report their config key and actual port, preserve cause and release every successful bind; non-EADDRINUSE errors identify bind failure without claiming the port is busy. Public conflict remains classified normally. Extend tests/config/config-load-degrade.test.ts and tests/server/loopback-listener-admission.test.ts with string enabled and out-of-range port: warning present, provider preserved, file unchanged. No local socket or service operation: tests only written, run remotely. + +MODIFY directly relevant structure/runtime.md, structure/config.md, structure/ops/service-and-sidecars.md and public hub/loopback guidance; link the canonical contract from other mapped ownership docs only where applicable. Review all conditional activation rows on hosted CI. Source inspection is not runtime proof. + +Design OPS-LIST-01..04 accepted with amendments. NEW tests/cli/cli-start-auxiliary-bind.test.ts, registered in both test-layout files, uses isolated CLI subprocess to cover soft and hard-pinned startup: failure names auxiliary key/address, exit nonzero, no public repick/wait branch. Existing management rollback fixture occupies management first then selects distinct public/loopback ports. Warning helper runs on all three load return paths plus read-only diagnostics; controls cover absent and valid-disabled entries and malformed secret-shaped input without echoing raw values. diff --git a/devlog/_plan/260912_operations/030_totals.md b/devlog/_plan/260912_operations/030_totals.md new file mode 100644 index 0000000000..71d2615772 --- /dev/null +++ b/devlog/_plan/260912_operations/030_totals.md @@ -0,0 +1,11 @@ +# Readable usage totals with explicit omissions + +Class C3; dependency roadmap. Adopt public #4111 final diff (2f07acb58b3e73f48cea38334f301b430a8634cd) after current-base and latest-review audit; preserve luvs01 credit. Source diff and metadata are in ignored .tmp/operations/pr-4111.diff/json, fetched directly from GitHub. + +MODIFY src/server/management/usage-aggregate-cache.ts: replace four oversizedRows throws with retained usageIncomplete boolean, set on full scan, OR on append, preserve in resultFrom; cache API-key snapshots with diagnostic. MODIFY api-key-usage.ts: keep readable accumulator output and attach usageIncomplete:true / usageIncompleteReason:oversized_rows instead of throwing. MODIFY logs-usage-routes.ts: serialize diagnostics on filtered and unfiltered summaries. MODIFY usage-summary-cache.ts CachedUsageSummary and oauth-account-routes.ts GET /api/keys to retain/serialize flags. Other IO/mutation errors still fail. + +MODIFY src/cli/usage-report.ts: warnings precede totals/no-match branch; incomplete no-match says skipped records may match. MODIFY gui/src/usage-summary-resource.ts shared optional diagnostic type; NEW components/usage-incomplete-notice.tsx; extend consumers Usage, dashboard overview, Models, AddProviderModal, ProviderWorkspaceShell, ApiKeysWorkspace/ListPanel and ApiKeys. Incomplete keys do not claim inactivity; warnings survive consumer caching. Add all locale keys. Full field chain: scanner oversizedRows -> retained aggregate boolean/API key snapshot -> route JSON/cache -> shared GUI/CLI input types -> every totals/ranking/key activity consumer. + +MODIFY existing tests/cli/cli-usage-report.test.ts, tests/server/api-usage.test.ts, tests/server/api-key-attribution.test.ts, tests/usage/usage-aggregate-cache.test.ts and GUI usage/custom-range/model-picker/key-workspace tests; NEW gui/tests/usage-incomplete-consumers.test.tsx. Activation: good + oversized + good rows yields readable sums and warning; append oversized sticky flag, full clean rewrite clears it, missing filter matches stays uncertain, loading/stale consumers retain warning. Do not turn IO errors into zero totals. + +MODIFY structure/gui-and-management-api.md and relevant mapped contract pointers; public management API, CLI agents and web-dashboard guides in all existing translated paths from original diff. Hosted full CI and dashboard evidence certify final tip; local suites/build/typecheck NOT RUN. This does not implement hub client-scoped CLI usage (#4205). diff --git a/devlog/_plan/260912_operations/040_client_usage.md b/devlog/_plan/260912_operations/040_client_usage.md new file mode 100644 index 0000000000..389c303f8b --- /dev/null +++ b/devlog/_plan/260912_operations/040_client_usage.md @@ -0,0 +1,13 @@ +# Connected client usage + +Class C4 for credential scope; dependency roadmap and only shared usage contract if needed. Existing src/cli/observe.ts:155 usage currently calls runtimeRequest('/api/usage'), whose owner src/cli/runtime-api.ts always resolves a local endpoint. Connected machine listener does not expose that route. + +MODIFY observe.ts usage dispatch to inspect existing client connection state before choosing the endpoint; standalone keeps runtimeRequest unchanged. Reuse the existing client-to-hub request owner and add a dedicated client-authenticated /v1/usage read, with only the enrolled client credential. Fail closed on invalid/mismatched state. Retain range/surface/provider/model and inclusive custom-window options, JSON versus human output, endpoint error messages and hub key scope. Do not send the local admin token to the hub or expose global management usage. If the existing hub endpoint supports fewer selectors, reject unsupported options explicitly until it is extended with the same authenticated scope. + +MODIFY existing client/hub API owner only where the read contract requires it; extend CLI usage and hub admission regression owners. Activation: connected client succeeds with own-key data despite local /api/usage absence; second client data excluded; revoked/bad credentials refuse; standalone still uses local management; custom-window contract preserved. Exact file map: NEW src/server/hub-usage.ts route handler and src/remote/hub-usage.ts shared response contract; MODIFY src/server/index.ts dispatch near /v1/models, src/server/auth-cors.ts configured-key identity resolver, src/client/hub-client.ts authenticated client read, src/cli/observe.ts usage dispatch, src/cli/usage-report.ts source/scope label. The route requires explicit configured-key admission even on loopback, derives apiKeyId from that identity, rejects caller-selected key IDs, uses filtered aggregation and no hub-wide summary cache. It omits private account attribution from the client DTO. Authentication/route enforcement tier: server code; caller-selected IDs cannot override the authenticated key; local admin/host control remains a residual outside client isolation. Final layer: server authorization. No claim of protection against the host owner. No local credential provisioning or running client changes. + +MODIFY public connected-client/CLI usage guide and structure/runtime.md / gui-and-management-api.md canonical scope. Any pre-disclosure details stay in scratch. Hosted regressions only; local execution NOT RUN. + +Accepted design OPS-USAGE-02/03/04. NEW tests/server/hub-usage.test.ts and tests/clients/hub-usage.test.ts with entries in scripts/test-layout/layout.json explicit and tests/fixtures/test-layout-expected.json; NEW tests/cli/cli-usage-hub.test.ts. Tests use two client keys, loopback and remote admissions, invalid state, custom window, unsupported endpoint, bad response, expired/revoked credentials. Full implementation follows source confirmation before B. + +Reflection amendments: getFilteredUsageAggregate in src/server/management/usage-aggregate-cache.ts is the aggregation owner. Client DTO preserves #4111 incomplete flags; CLI suppresses advice to remove filters for account totals because that scope never exports accounts. Public files: docs-site/src/content/docs/guides/remote-hub.md and reference/cli/agents.md. All three new tests register in scripts/test-layout/layout.json and tests/fixtures/test-layout-expected.json. diff --git a/devlog/_plan/260912_operations/050_pairing.md b/devlog/_plan/260912_operations/050_pairing.md new file mode 100644 index 0000000000..2b05bff2ba --- /dev/null +++ b/devlog/_plan/260912_operations/050_pairing.md @@ -0,0 +1,13 @@ +# Pending browser authentication guidance + +Class C3; dependency roadmap. Reuse existing connected-client state and browser-auth components. Target #4206 and #4208 together because both concern the same pending local-dashboard pairing journey. + +MODIFY owning dashboard pending-auth component and bootstrap state: distinguish a reachable connected machine awaiting hub browser authentication from a stopped standalone proxy. Show configured hub identity/origin, explain that machine enrollment and browser session are separate, offer the current origin-specific existing pairing/authentication action. Preserve revoked/expired/unreachable states and their existing retry actions; do not suggest ocx start while the local runtime is reachable. Derive the next action from current origin + configured hub instead of a hardcoded localhost URL. No credentials appear in visible copy/URLs. + +MODIFY all gui/src/i18n locale dictionaries with meaningful labels. Extend existing pending-auth/dashboard tests for local origin, remote hub origin, pending, authenticated, expired/revoked and unavailable standalone; positive browser auth transitions into connected dashboard. Exact files: gui/src/App.tsx, api.ts, pages/dashboard-core-poll.ts, pages/use-dashboard-data.ts and pages/Dashboard.tsx consume a classified authentication/error state instead of a boolean. Existing connect-pairing.ts and connect-pairing-transport.ts own hub identity and origin-specific action. Define the error classification in api.ts at response ingress; consume in polling and Dashboard; reset on authenticated success and pairing completion. No persistence/serialization for this UI state. Keep cached data with stale labeling when auth fails; do not erase a known hub into standalone offline. Public hub/browser-pairing guidance is updated with the same distinction. No service restart or live auth reconfiguration. + +Hosted component suite and screenshot artifact of the rendered pending state required for final delivery; local GUI tests/build NOT RUN. Static source or mockup is not rendered application evidence. + +Accepted OPS-PAIR-01/02. Cases include browser session expiry and post-pairing refresh, local and hub origin guidance, code versus API/admin-key explanation, and operator handoff text. Prefer existing component tests; new test files only where needed. + +Reflection amendments: reuse existing api.ts SESSION_UNAVAILABLE_EVENT and App sharedSessionReady; subscribe in App, emit on terminal 401 expiry (not aborted requests), reset/read refresh on successful pairing. Do not create duplicate auth state. Poll classification and pairing errors distinguish HTTP auth refusal, transport/network, and invalid responses; aborted work does not show a failure. diff --git a/devlog/_plan/260912_operations/060_transport.md b/devlog/_plan/260912_operations/060_transport.md new file mode 100644 index 0000000000..a884509d22 --- /dev/null +++ b/devlog/_plan/260912_operations/060_transport.md @@ -0,0 +1,7 @@ +# Local management catalog read + +Class C4; dependency roadmap. Scope #4315 and the current CHANGES_REQUESTED review on #4317. Public source starting points: src/cli/opencode.ts fetchOpencodeProxyModels/cmdOpencode, src/lib/admin-secrets.ts, src/lib/local-destinations.ts, associated providers/opencode-cli tests. Reuse existing transport owner after caller search; avoid an opencode-only ad hoc credential client. + +The executable security design and negative-case audit live only in ignored .tmp/operations/060_transport_private.md. That file must be completed and independently reviewed before B; no pre-disclosure reasoning is copied into public planning history. Public deliverable is the implementation, regression tests and shipped contract text only. Required review dimensions: local destination selection, redirect and proxy-environment behavior, credential separation and all current callers. Original contributor credit: Cortes Ventures . No fallback that substitutes a data credential for admin authentication. + +Hosted regression execution plus independent security source audit bind the final patch SHA. Review state is refreshed before handoff; this work cannot approve or merge the original PR. Local suites/build/typecheck/install NOT RUN. diff --git a/devlog/_plan/260912_operations/070_verification.md b/devlog/_plan/260912_operations/070_verification.md new file mode 100644 index 0000000000..e5af9101e3 --- /dev/null +++ b/devlog/_plan/260912_operations/070_verification.md @@ -0,0 +1,9 @@ +# Final tips and handoff + +Dependency: each implementation. No new behavior by default; append a separate PABCD repair cycle when actual final-tip CI failure identifies a necessary delta. + +For each independently mergeable branch: record git rev-parse HEAD, original source PR disposition, included commits, gh pr view headRefOid/baseRefName, successful native-membership read (or unknown), and gh run view for the exact Cross-platform CI run. Manual chains only when later work consumes earlier code; verify lower SHA ancestry at the final tip and record bottom-to-top order. Do not cancel auto-CI or change workflow/protection. No merge/auto-merge, closure, release or user service operation. + +A local receipt may run git diff --check and read-only hosted-result assertions; it is not a local test result. Local suites, typecheck/build/install are NOT RUN. Final behavior acceptance comes from GitHub-hosted test runs at the final SHA and independent review; author reports/old green CI are not substituted. + +Update ignored .tmp/operations/handoff.md as soon as each artifact exists. Include outstanding issue acceptance, original author trailers, unresolved maintainer objections, exact run links/conclusions and cycle ledger pointers. Publish template-complete PR bodies with truthful verification, screenshots for changed dashboard UI and no private investigation notes. Parent owns all integration decisions. From e090ad65cd619e095cbd98eac17ccb4824b256c7 Mon Sep 17 00:00:00 2001 From: JUN Date: Sat, 12 Sep 2026 13:42:29 +0900 Subject: [PATCH 2/3] fix(update): retire observed exited pinned-start children Carry PR #4185 with child-object ownership and deterministic retry coverage. Local product tests NOT RUN; hosted final-tip CI follows. Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com> --- src/update/job.ts | 38 ++++-- structure/runtime.md | 5 + tests/update/update-job.test.ts | 116 ++++++++++++++++++ .../windows-deploy-close-regressions.test.ts | 3 +- 4 files changed, 150 insertions(+), 12 deletions(-) diff --git a/src/update/job.ts b/src/update/job.ts index b7e51ff8c5..dd89d4028f 100644 --- a/src/update/job.ts +++ b/src/update/job.ts @@ -983,6 +983,11 @@ export interface RestartIo { spawnStart?: (job: UpdateJobState, installer: Installer, port?: number, launcher?: string) => void; /** The package launcher verified after a pnpm group switch or rollback. */ packageLauncherPathFn?: () => string; + /** Exercise pinned-start retries without spawning, reclaiming, or killing real processes. */ + spawnDetachedStartFn?: typeof spawnDetachedStart; + preparePortForPinnedStartFn?: typeof preparePortForPinnedStart; + waitForGhostListenClearFn?: typeof waitForGhostListenClear; + killProxyFn?: typeof killProxy; serviceInstalledFn?: () => boolean; /** * After a service reinstall exits 0, only trust the service path when this is true. @@ -1332,9 +1337,20 @@ async function restartAfterUpdate( } } const attempts = 3; + const now = io.now ?? (() => Date.now()); + const spawnPinnedStart = io.spawnDetachedStartFn ?? spawnDetachedStart; + const preparePort = io.preparePortForPinnedStartFn ?? preparePortForPinnedStart; + const waitForGhost = io.waitForGhostListenClearFn ?? waitForGhostListenClear; // Longer than published hard-pin reclaim (30s) so a slow start can still report healthy. const perAttemptHealthMs = 70_000; let lastChild: ChildProcess | null = null; + const killSpawnAttempt = (child: ChildProcess | null): void => { + // A numeric PID can be reused once this particular child has exited. + if (!child?.pid || child.exitCode !== null || child.signalCode !== null) return; + if (aliveFn(child.pid)) { + try { (io.killProxyFn ?? killProxy)(child.pid); } catch { /* best-effort */ } + } + }; for (let attempt = 1; attempt <= attempts; attempt++) { if (attempt > 1) { updateJob( @@ -1343,13 +1359,11 @@ async function restartAfterUpdate( `Pinned start attempt ${attempt - 1} did not become healthy on port ${port}; ` + `retrying (${attempt}/${attempts}).`, ); - if (lastChild?.pid && aliveFn(lastChild.pid)) { - try { killProxy(lastChild.pid); } catch { /* best-effort */ } - } + killSpawnAttempt(lastChild); lastChild = null; } - preparePortForPinnedStart(job, port, listPids, aliveFn, verifyOcx); - const ready = await waitForGhostListenClear( + preparePort(job, port, listPids, aliveFn, verifyOcx); + const ready = await waitForGhost( port, hostname, listPids, @@ -1365,17 +1379,19 @@ async function restartAfterUpdate( ); continue; } - lastChild = spawnDetachedStart(job, job.installer, port, launcher); - const healthDeadline = Date.now() + perAttemptHealthMs; - while (Date.now() < healthDeadline) { + const child = spawnPinnedStart(job, job.installer, port, launcher); + lastChild = child; + child.once("exit", () => { + if (lastChild === child) lastChild = null; + }); + const healthDeadline = now() + perAttemptHealthMs; + while (now() < healthDeadline) { if (await probe(port, hostname)) return; await sleep(500); } } // Exhausted retries: do not leave a hung pinned-start child owning the port. - if (lastChild?.pid && aliveFn(lastChild.pid)) { - try { killProxy(lastChild.pid); } catch { /* best-effort */ } - } + killSpawnAttempt(lastChild); } /** Compact listen-holder summary for update-job logs when reclaim fails. */ diff --git a/structure/runtime.md b/structure/runtime.md index 49a5fb6483..847cb4195a 100644 --- a/structure/runtime.md +++ b/structure/runtime.md @@ -91,6 +91,11 @@ scans may proceed if verification succeeds or the holder exits. The allowlist na eligibility and supplies no identity evidence by itself. This contract uses the existing verifier; it does not add process-instance proof or change the classification cache. +Pinned post-update retries in `src/update/job.ts` retire a child after an observed exit or signal. +Both retry and final-timeout cleanup check the retained child object; a late exit from an older +child cannot clear its replacement. A healthy child remains running. This does not provide an +atomic OS guarantee against unobserved PID reuse. + > Decision record: [ADR-0003](decisions/ADR-0003-lifecycle.md) An installed Codex shim is checked on ordinary CLI startup with a regular-file/1 MiB state bound plus diff --git a/tests/update/update-job.test.ts b/tests/update/update-job.test.ts index 9ba42b8150..c5ac2b523e 100644 --- a/tests/update/update-job.test.ts +++ b/tests/update/update-job.test.ts @@ -1,4 +1,6 @@ import { afterEach, beforeEach, describe, expect, test } from "bun:test"; +import type { ChildProcess } from "node:child_process"; +import { EventEmitter } from "node:events"; import { mkdirSync, readFileSync, rmSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; @@ -42,6 +44,120 @@ afterEach(() => { removeTreeWithRetry(dir); }); +describe("pinned-start child cleanup", () => { + // exitCode/signalCode are readonly on ChildProcess, but these fakes must move a child from + // "running" to "exited" mid-test. Redeclare them as mutable rather than widening each + // assignment with a cast, so the transitions stay type-checked. + type FakeChild = EventEmitter & Pick & { + exitCode: number | null; + signalCode: NodeJS.Signals | null; + }; + + async function exhaustRetries(options: { + spawned?: (child: FakeChild) => void; + healthWait?: (children: FakeChild[]) => void; + reusePid?: boolean; + healthyOnLastAttempt?: boolean; + } = {}) { + let now = 0; + const children: FakeChild[] = []; + const killed: number[] = []; + const livenessChecks: number[] = []; + const job: UpdateJobState = { + id: "pinned-child-cleanup", status: "restarting", + startedAt: new Date().toISOString(), updatedAt: new Date().toISOString(), + currentVersion: "2.49.0", latestVersion: "2.50.0", channel: "latest", + installer: "npm", restart: true, command: "", releaseNotesUrl: "", log: [], + }; + writeFileSync(updateJobPath(), JSON.stringify(job)); + await restartAfterUpdateForTests(job, { port: 19111, hostname: "127.0.0.1" }, { + serviceInstalledFn: () => false, + waitForPort: async () => true, + listListenPidsFn: () => [], + preparePortForPinnedStartFn: () => {}, + waitForGhostListenClearFn: async () => ({ ok: true, accessDenied: false }), + probeProxyIdentity: async () => null, + probeProxy: async () => !!options.healthyOnLastAttempt && children.length === 3, + now: () => now, + sleepMs: async ms => { + options.healthWait?.(children); + now += ms; + }, + isAliveFn: pid => { + livenessChecks.push(pid); + // A reused numeric PID may be live even after our own child has exited. + return true; + }, + spawnDetachedStartFn: () => { + const child: FakeChild = Object.assign(new EventEmitter(), { + pid: options.reusePid ? 4241 : 4241 + children.length, exitCode: null, signalCode: null, + }); + children.push(child); + options.spawned?.(child); + return child as ChildProcess; + }, + killProxyFn: pid => { killed.push(pid); }, + }); + expect(children).toHaveLength(3); + return { killed, livenessChecks }; + } + + test.each([ + { name: "successful exit", exitCode: 0, signalCode: null }, + { name: "failed exit", exitCode: 1, signalCode: null }, + { name: "signal exit", exitCode: null, signalCode: "SIGTERM" as const }, + ])("never reuses a child PID after $name", async ({ exitCode, signalCode }) => { + const result = await exhaustRetries({ + spawned: child => { child.exitCode = exitCode; child.signalCode = signalCode; }, + }); + expect(result.killed).toEqual([]); + expect(result.livenessChecks).toEqual([]); + }); + + test("retires a child when its exit event is observed during the health wait", async () => { + const observed = new Set(); + const result = await exhaustRetries({ + healthWait: children => { + const child = children.at(-1)!; + if (observed.has(child)) return; + observed.add(child); + // Keep the fixture fields unset to exercise the event retirement independently. + child.emit("exit", 0, null); + }, + }); + expect(observed.size).toBe(3); + expect(result.killed).toEqual([]); + expect(result.livenessChecks).toEqual([]); + }); + + test("still cleans up live children before retries and after the final health timeout", async () => { + const result = await exhaustRetries(); + expect(result.killed).toEqual([4241, 4242, 4243]); + expect(result.livenessChecks).toEqual([4241, 4242, 4243]); + }); + + test("a previous child's late exit does not retire the current live child", async () => { + const observed = new Set(); + const result = await exhaustRetries({ + reusePid: true, + healthWait: children => { + const previous = children.at(-2); + if (!previous || observed.has(previous)) return; + observed.add(previous); + previous.exitCode = 0; + previous.emit("exit", 0, null); + }, + }); + expect(observed.size).toBe(2); + expect(result.killed).toEqual([4241, 4241, 4241]); + }); + + test("leaves the current child running when its health probe succeeds", async () => { + const result = await exhaustRetries({ healthyOnLastAttempt: true }); + expect(result.killed).toEqual([4241, 4242]); + }); +}); + describe("GUI update check", () => { test("surfaces an npm update with the launcher-safe command", () => { const result = checkForUpdate("latest", { diff --git a/tests/windows/windows-deploy-close-regressions.test.ts b/tests/windows/windows-deploy-close-regressions.test.ts index e3bae4fc0b..d9f80c8d19 100644 --- a/tests/windows/windows-deploy-close-regressions.test.ts +++ b/tests/windows/windows-deploy-close-regressions.test.ts @@ -59,7 +59,8 @@ describe("update-job restart avoids the shell-less .cmd EINVAL (Windows, bun/sou // matching rules themselves are covered by windows-service-wrappers.test.ts. expect(src).toContain("killWindowsSchedulerWrappers"); expect(read("src/lib/windows-service-wrappers.ts")).toContain("$_.ProcessId -eq $PID"); - expect(src).toContain("lastChild?.pid && aliveFn(lastChild.pid)"); + // Pinned-child cleanup is exercised through the retry loop in update/update-job.test.ts, + // including exited children, live retry/final cleanup, and successful health probes. }); }); From f37894711158fa8215d26bed642389148ac395f6 Mon Sep 17 00:00:00 2001 From: JUN Date: Sat, 12 Sep 2026 14:01:36 +0900 Subject: [PATCH 3/3] fix(update): retire failed-spawn children on terminal events --- devlog/_plan/260912_operations/010_update.md | 2 ++ .../260912_operations/080_update_events.md | 13 ++++++++++ src/update/job.ts | 10 +++++-- structure/runtime.md | 5 ++-- tests/update/update-job.test.ts | 26 +++++++++++++++++-- 5 files changed, 50 insertions(+), 6 deletions(-) create mode 100644 devlog/_plan/260912_operations/080_update_events.md diff --git a/devlog/_plan/260912_operations/010_update.md b/devlog/_plan/260912_operations/010_update.md index 53cb5d8719..29a7cffd4d 100644 --- a/devlog/_plan/260912_operations/010_update.md +++ b/devlog/_plan/260912_operations/010_update.md @@ -9,3 +9,5 @@ MODIFY tests/update/update-job.test.ts: carry #4185 deterministic fake EventEmit Exact starting patch: public PR #4185 head 2602f3ceca4b93237436911dcd8dffc35b3b5e57, reviewed source diff retained locally in .tmp/operations/pr-4185.diff. Before: numeric PID may remain after child exit. After: recorded exit/signal or matching exit event retires cleanup authority. This does not make all OS signalling atomic against PID reuse. Planned hosted activation checks (not yet executed): seven tests in update/update-job.test.ts exercise both cleanup sites. Local execution NOT RUN. Source check: both sites use the same child-aware closure; no unrelated test weakened. Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com>. + +Follow-up080 extends exact-child retirement to error/close/exit and releases its own handlers; failed spawn error+close without exit is covered separately. Local tests remain NOT RUN. diff --git a/devlog/_plan/260912_operations/080_update_events.md b/devlog/_plan/260912_operations/080_update_events.md new file mode 100644 index 0000000000..05896e305e --- /dev/null +++ b/devlog/_plan/260912_operations/080_update_events.md @@ -0,0 +1,13 @@ +# Update child terminal-event follow-up + +User-requested review repair for PR4343 discussion3995158280. Previous totalsD directs this repair before client usage. Scope remains update cleanup; no live child/process operation locally. Class C4, satisfy-spec, existing credential scope, no time/token bound. Local tests/build/typecheck/install NOT RUN; source review now and hosted final4343 tip later. No merge. + +Source finding verified: src/update/job.ts spawnDetachedStart returns child_process.spawn result and records error, while retry ownership watches exit alone. Node child-process contract permits error without exit and guarantees close after failed spawn. Existing cleanup already skips absent PID; the defect is incomplete terminal-event retirement, not proof of a wrong-PID kill on failed spawn. + +MODIFY src/update/job.ts: one identity-checked retire handler clears lastChild only if it is still this child, and removes its own exit/error/close listeners to release closure references. Register once on all three. Existing spawn error logging, healthy probe behavior and live-child cleanup remain. This production owner does not call child.kill/send or supply AbortSignal; its error event is spawn failure. No global process supervisor or schema changes. + +MODIFY tests/update/update-job.test.ts: extend fake-child retry harness with failed spawn pid undefined, error then close without exit; assert retries continue and no liveness/kill calls, plus own terminal listeners are released. Parameterize late old-child terminal event across exit/error/close to prove object identity protects current live child. Keep live cleanup and healthy controls. A repeated terminal callback is idempotent. + +MODIFY structure/runtime.md observed-child paragraph and010 plan with terminal-event semantics. Publish --no-verify, independent source re-audit, reply with exact head and NOT RUN distinction, resolve the authorized review thread. Behavioral acceptance stays OPEN until final hosted CI. + +Source: https://nodejs.org/api/child_process.html#event-error and #event-close, opened2026-09-12. Public review: https://github.com/lidge-jun/opencodex/pull/4343#discussion_r3995158280. diff --git a/src/update/job.ts b/src/update/job.ts index dd89d4028f..16310cad82 100644 --- a/src/update/job.ts +++ b/src/update/job.ts @@ -1381,9 +1381,15 @@ async function restartAfterUpdate( } const child = spawnPinnedStart(job, job.installer, port, launcher); lastChild = child; - child.once("exit", () => { + const retireChild = () => { if (lastChild === child) lastChild = null; - }); + child.removeListener("exit", retireChild); + child.removeListener("error", retireChild); + child.removeListener("close", retireChild); + }; + child.once("exit", retireChild); + child.once("error", retireChild); + child.once("close", retireChild); const healthDeadline = now() + perAttemptHealthMs; while (now() < healthDeadline) { if (await probe(port, hostname)) return; diff --git a/structure/runtime.md b/structure/runtime.md index 847cb4195a..69bb8fcea6 100644 --- a/structure/runtime.md +++ b/structure/runtime.md @@ -91,9 +91,10 @@ scans may proceed if verification succeeds or the holder exits. The allowlist na eligibility and supplies no identity evidence by itself. This contract uses the existing verifier; it does not add process-instance proof or change the classification cache. -Pinned post-update retries in `src/update/job.ts` retire a child after an observed exit or signal. +Pinned post-update retries in `src/update/job.ts` retire a child after an observed exit, signal, spawn error, or close event. Both retry and final-timeout cleanup check the retained child object; a late exit from an older -child cannot clear its replacement. A healthy child remains running. This does not provide an +child cannot clear its replacement. Retirement removes only its own event listeners and preserves +separate logging. A healthy child remains running. This does not provide an atomic OS guarantee against unobserved PID reuse. > Decision record: [ADR-0003](decisions/ADR-0003-lifecycle.md) diff --git a/tests/update/update-job.test.ts b/tests/update/update-job.test.ts index c5ac2b523e..d3eb621ce7 100644 --- a/tests/update/update-job.test.ts +++ b/tests/update/update-job.test.ts @@ -136,7 +136,7 @@ describe("pinned-start child cleanup", () => { expect(result.livenessChecks).toEqual([4241, 4242, 4243]); }); - test("a previous child's late exit does not retire the current live child", async () => { + test.each(["exit", "error", "close"])("a previous child's late %s does not retire the current live child", async event => { const observed = new Set(); const result = await exhaustRetries({ reusePid: true, @@ -145,13 +145,35 @@ describe("pinned-start child cleanup", () => { if (!previous || observed.has(previous)) return; observed.add(previous); previous.exitCode = 0; - previous.emit("exit", 0, null); + if (event === "error") previous.emit(event, new Error("late spawn error")); + else previous.emit(event, 0, null); }, }); expect(observed.size).toBe(2); expect(result.killed).toEqual([4241, 4241, 4241]); }); + test("failed spawns retire on error and close without an exit event", async () => { + const observed = new Set(); + const result = await exhaustRetries({ + spawned: child => { Object.defineProperty(child, "pid", { value: undefined }); }, + healthWait: children => { + const child = children.at(-1)!; + if (observed.has(child)) return; + observed.add(child); + child.emit("error", new Error("spawn ENOENT")); + // Retirement also releases this attempt's closure; an absent-PID check alone + // would pass the kill assertions while keeping the exit handler installed. + expect(child.listenerCount("exit")).toBe(0); + expect(child.listenerCount("close")).toBe(0); + child.emit("close", -1, null); + }, + }); + expect(observed.size).toBe(3); + expect(result.killed).toEqual([]); + expect(result.livenessChecks).toEqual([]); + }); + test("leaves the current child running when its health probe succeeds", async () => { const result = await exhaustRetries({ healthyOnLastAttempt: true }); expect(result.killed).toEqual([4241, 4242]);