fix(proc): address kz8.2 review comments (kz8.2.1)#7
Merged
Conversation
Eight findings from Copilot's review of #6, addressed in one PR: 1. install.osacompile / install.lsregister now have their own LAUNCH_CATEGORIES entries; per-binary metering no longer collapses three distinct install helpers into install.plutil. 2. launch / launchSync distinguish "timeout" (our local timer fired) from "signal" (OS/external killer ended the child). A signal-killed no-timeout child no longer misreports as timeout in daemon-stats. 3. launchDetachedSync is now exported and returns the typed outcome synchronously. spawnDaemonDetachedReal uses it and returns result.ok, so ENOENT / EACCES on detached daemon spawns surfaces instead of being reported as success. As a bonus, launchDetachedSync now detects ENOENT (which spawn doesn't throw for — it surfaces as pid=undefined). 4. Rolling-window ring buffer cap bumped to 16384 (~273 sustained launches/sec before undercount kicks in); cap rationale documented at the constant. 5. execGitAsync takes args: string[] directly; the prior whitespace-split contract is gone. Paths with spaces / quoted args can no longer mis-tokenize at the boundary. 6. byCategory now only includes executed categories — symmetric with p50/p99 histograms and with the formatter's filter. Consumers wanting the full enum read LAUNCH_CATEGORIES. 7. Stats formatter column width is computed from active categories (max(13, ...lengths)) instead of a hardcoded padEnd(13) that misaligned for var-system.git, install.plutil, terminal-width, and others. 8. Timer's child.kill is guarded by !settled so a very-fast child that exited before setTimeout was scheduled doesn't get a stray SIGTERM. [LAW:single-enforcer] [LAW:one-type-per-behavior] [LAW:no-silent-fallbacks] [LAW:dataflow-not-control-flow] [LAW:types-are-the-program] Refs: brandon-process-discipline-kz8.2.1
There was a problem hiding this comment.
Pull request overview
This PR follows up on subprocess launch/metering review feedback by tightening launch result classification, detached daemon spawning, install categories, and daemon stats formatting.
Changes:
- Adds distinct launch categories and a
signalfailure reason. - Introduces
launchDetachedSyncand uses it for daemon spawning. - Converts git command execution to argv arrays and adjusts subprocess stats output/formatting.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
src/proc/launch.ts |
Updates launch result reasons and adds detached sync launch API. |
src/daemon/acquire.ts |
Uses detached sync launch for daemon spawning. |
src/daemon/stats.ts |
Changes rolling cap and sparse subprocess category snapshot behavior. |
src/daemon/client-stats.ts |
Adjusts subprocess category formatting and padding. |
src/segments/git.ts |
Switches git execution from command strings to argv arrays. |
src/install/index.ts |
Assigns more specific install subprocess categories. |
test/proc/launch.test.ts |
Adds launch signal and detached sync coverage. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+245
to
+254
| // spawnSync result, not in the surrounding control flow. Node sets | ||
| // `result.signal` whenever the child died from a signal — including but | ||
| // not limited to the timeout's SIGTERM. We can only attribute "timeout" | ||
| // when a timeout was actually requested; otherwise the signal came from | ||
| // somewhere else (OOM killer, ctrl-C through the tty group, etc.). | ||
| const hasTimeout = opts.timeoutMs !== undefined && opts.timeoutMs > 0; | ||
| const reason: "timeout" | "signal" | "non-zero" = result.signal | ||
| ? hasTimeout && result.signal === "SIGTERM" | ||
| ? "timeout" | ||
| : "signal" |
Comment on lines
+158
to
+160
| // actually executed — same shape as p50/p99 below. Consumers wanting the | ||
| // full closed list can read LAUNCH_CATEGORIES (exported from src/proc/launch) | ||
| // and treat missing keys as zero. |
Comment on lines
+162
to
+163
| for (const [cat, n] of this.subprocessCount) { | ||
| if (n > 0) byCategory[cat] = n; |
Comment on lines
+139
to
+141
| // Snapshot already includes only executed categories (stats.ts: | ||
| // snapshotSubprocesses keeps byCategory and the histograms symmetric). | ||
| const activeCats = Object.entries(s.subprocesses.byCategory); |
| category: "daemon-spawn", | ||
| }); | ||
| return true; | ||
| return result.ok; |
| // — that pattern discards the spawn outcome and reports success on failure. | ||
| // This function returns the typed result synchronously, and also meters | ||
| // through the stats handle so detached spawns show up in daemon-stats. | ||
| export function launchDetachedSync(opts: LaunchOpts): LaunchResult { |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Addresses all 8 inline comments from Copilot's review of #6 (kz8.2). PR #6 merged before the bot finished reviewing — this PR closes the loose end without re-opening the original ticket. Followup tracked as
brandon-process-discipline-kz8.2.1.The 8 findings, in commit order:
install category mislabel —
osacompileandlsregisterwere tagged asinstall.plutil. Addedinstall.osacompileandinstall.lsregistertoLAUNCH_CATEGORIESand tagged correctly. Per-binary metering now distinguishes the three install helpers.signal vs timeout — both
launchandlaunchSyncreported any signal-killed exit asreason: "timeout". Now distinguishes:"timeout"(our local timer fired and SIGTERMed the child),"signal"(OS/external killer — SIGKILL, SIGINT through tty group, SIGPIPE, SIGHUP, externalkill),"non-zero"(clean exit, non-zero code),"spawn-error"(failure before child started). Async path trackstimedOut: boolean; sync path compares againstopts.timeoutMs+result.signal === "SIGTERM".silent detached-spawn failure —
void launch({detached: true})inspawnDaemonDetachedRealdiscarded the Promise, so a failed daemon spawn (ENOENT, EACCES) was reported as success. Exported newlaunchDetachedSyncthat returnsLaunchResultsynchronously;spawnDaemonDetachedRealnow returnsresult.ok. Also:spawn()doesn't throw for ENOENT — it returns aChildProcesswithpid: undefinedand emits'error'asynchronously.launchDetachedSyncnow attaches a no-op error listener and treatspid === undefinedas spawn-error. A new test covers this case.rolling-window ring buffer cap — bumped
ROLLING_BUFFER_CAPfrom 4096 to 16384 (~273 sustained launches/sec before undercount kicks in). Cap rationale documented at the constant. The old comment "old entries get overwritten; 'last minute' truth is preserved" was wrong and has been corrected.execGitAsync(args: string[]) — the prior
command: string+ whitespace-split contract was fragile for any future caller passing a quoted arg or a path with spaces. Signature now takesargs: readonly string[]; 12 call sites updated to pass arrays. The runtimebin === "git"check is gone (the boundary type carries it).byCategory / histograms symmetry —
byCategorywas built from allLAUNCH_CATEGORIES(every category present, zeros included) whilep50DurationMs/p99DurationMsonly included executed categories. NowbyCategorymatches: only executed categories appear. Same shape across all three subprocess maps, and matches the formatter's already-existing filter. JSON consumers wanting the full closed list readLAUNCH_CATEGORIESfromsrc/proc/launch.dynamic category column padding —
padEnd(13)was too narrow forvar-system.git(14),install.plutil(14),install.osacompile(18),terminal-width(14), etc. Now computed fromMath.max(13, ...activeCats.map(c => c.length)).kill-after-settled race — timeout timer could fire and
SIGTERMan already-settled child (e.g. extremely fast/bin/truethat exits beforesetTimeoutis scheduled). Guarded withif (!settled) child.kill("SIGTERM")andlet timedOut = trueset before the kill so the close handler'sreasondiscrimination still works.All changes cite the relevant
[LAW:...]markers inline.Test plan
pnpm typecheck— cleanpnpm lint— cleanpnpm check:protocol— TS/Rust agreepnpm build— produces dist/pnpm test— 32/32 suites, 682/682 tests pass (was 669; added two new tests forlaunchDetachedSyncspawn-error + metering, and one for"signal"reason)Refs: brandon-process-discipline-kz8.2.1