Repository navigation
Commit 7164587
fix(cli): os start forwards SIGTERM/SIGINT to its serve child and reaps it on exit (#21161)
Fixes #21114
Clause-②: no
## What changed
`os start` now supervises its `serve` child through
`ServeRestartCoordinator`, the forwarding mechanism `os dev` already
runs, used as is (`packages/cli/src/commands/start.ts`):
- SIGTERM or SIGINT sent to the `start` process is forwarded to the
child (`beginShutdown`). The child's exit then ends the parent with the
child's exit code.
- Whatever else ends the parent, the child is sent SIGTERM on the way
out (`killChildOnParentExit` on `process.on('exit')`).
- A child that exits on its own still ends the parent with `code ?? 0`,
which is the one handler `start` had before.
Also in this PR: the pin
`packages/cli/test/start-signal-forwarding.e2e.test.ts` and
`.changeset/21114-start-signal-forwarding.md` (`@objectstack/cli`
patch).
## Why the coordinator is used as is, and nothing is extracted
Triage asked for one mechanism, "extracted to a shared helper if
needed". It is not needed here:
- `start` has no restart semantics, so it never calls `requestRestart`.
The other paths are what `start` needs: `start()`, `beginShutdown`,
`killChildOnParentExit`, and the child-exit path (`state === 'running'`
ends the parent with `code ?? 0`).
- The class's restart-only messages cannot print from `start`. They are
gated on `spawnCount > 1` or are inside `requestRestart`. The
spawn-failure message reads `failed to start server`, because
`restartIndex` is 0.
- `dev-restart.ts` and `dev.ts` are unchanged, and the coordinator's own
unit tests (`src/utils/dev-restart.test.ts`) already cover
`beginShutdown` and `killChildOnParentExit`.
The only text the two commands now share is the three-line subscription
(`process.on` for `SIGINT`, `SIGTERM` and `exit`). It stays at each call
site, as the dispatch directed for the no-extraction route. What a
signal *does* lives in the coordinator alone.
## Measured, before and after
Run on `examples/app-showcase` with the built entry: `node
packages/cli/bin/run.js start -p PORT --no-ui &` (control: `dev -p PORT
--no-watch &`). The signal went to the parent's pid alone, on a random
high port. The before tree is `origin/main` `7a606a9a34`; the after tree
is `20eaab626f`, where `start.ts` is byte-identical to this head.
| command | signal | tree | parent exit (shell) | `serve` child after |
port after | `/api/v1/health` after |
|---|---|---|---|---|---|---|
| `os start` | SIGTERM | before | 143 | alive, PPID 1 | bound | 200 |
| `os start` | SIGINT | before | 130 | alive, PPID 1 | bound | 200 |
| `os start` | SIGTERM | after | 0 | gone | free | no answer |
| `os start` | SIGINT | after | 0 | gone | free | no answer |
| `os dev` (control) | SIGTERM | before | 0 | gone | free | no answer |
| `os dev` (control) | SIGINT | before | 0 | gone | free | no answer |
| `os dev` (control) | SIGTERM | after | 0 | gone | free | no answer |
| `os dev` (control) | SIGINT | after | 0 | gone | free | no answer |
On the before tree the orphans had to be killed by their own pid. On the
after tree every recorded pid was gone without help.
The Ctrl-C shape was measured on the after tree too (SIGINT to the whole
process group). For both `start` and `dev`, the parent and the child
were gone and the port was free. Both logged one `Shutdown already in
progress, ignoring SIGINT` line, because the child receives the signal
from the group and again from the forward. The changeset states this.
## The pin
`test/start-signal-forwarding.e2e.test.ts` boots `os start` and `os dev`
on the same minimal artifact. For SIGTERM and for SIGINT it signals the
parent pid alone, never the group, and asserts that the `serve` child is
gone and the port is free. Both readings have a positive control on the
same boot before the signal: exactly one `serve` child is seen alive,
and the port reads bound.
- **Entry.** The pin spawns `bin/run-dev.js` under the tsx loader,
passed as `--import`, so the spawned pid is the CLI parent itself. The
built entry would have added a seventh spawner to the six-file
population that `check:cli-test-child-env` pins. The subject is the
supervisor wiring in `src/commands/start.ts`, which both entries run.
The built entry is covered by the hand measurement above.
- **Child selection, measured.** On a cold tsx transform cache the
loader runs an `esbuild --service` process as a second child of the CLI
parent. That process outlives the parent by a moment: it was alive at
the parent's exit and gone 2 s later, in 2 of 2 probes with
`TSX_DISABLE_CACHE=1`. One early run of the pin went red on exactly
that. The probe now selects the child whose argv carries the `serve`
token, and the pin is green with the cache forced cold.
- **Tier.** The `.e2e` name puts the pin in the nightly tier
(`OS_TEST_TIERS=nightly`), next to
`start-port-banner-agreement.e2e.test.ts`. It does not run in this PR's
CI. Each run boots 4 kernels, about 16 s each on a shared box.
**Ablation** (source mode: the pin reads `src/`, so no build leg). The
fix was committed first. `scripts/ablation-replace.mjs` replaced the
three `process.on` lines with a planted marker statement and confirmed
the change on disk (anchor 1 to 0, blob `b6d246505be6` to
`1055d17fdbeb`). It ran the pin, then restored the file (blob back to
`b6d246505be6`, `git diff HEAD` empty, porcelain empty). Results at head
`4d2d7f9bc6`:
- ablated: `os start` SIGTERM and SIGINT both red on both readings
(`left its serve child running`, `left port N bound`); `os dev` control
green on both signals; 2 failed, 2 passed;
- restored: 4 passed.
An earlier built-entry version of the pin was ablated the same way on
`dist/`, with `scripts/ablation-dist-preflight.mjs` confirming the
marker present and then absent. It gave the same direction on
`b299389161` and on the merged `a096821511`.
## Verification
All readings below were taken at head `4d2d7f9bc6` unless another commit
is named. This branch merged `origin/main` `c6954d6d09` before the last
commits, and the build state was refreshed after the merge.
- **Pin**, nightly tier, run locally with `OS_TEST_TIERS=nightly pnpm
--filter @objectstack/cli exec vitest run --project integration
test/start-signal-forwarding.e2e.test.ts`: 4 passed. With the transform
cache forced cold (`TSX_DISABLE_CACHE=1`): 4 passed. Ablated: 2 failed,
2 passed. Restored: 4 passed.
- **Typecheck.** `pnpm --filter @objectstack/cli typecheck` exited 0
(`check:test-typecheck: OK`).
- **`unit` tier.** `pnpm --filter @objectstack/cli exec vitest run
--project unit`: 242 files and 3432 tests passed at `a096821511`. Since
then only the e2e pin changed, and it is outside the `unit` project. At
`4d2d7f9bc6`, `test/vitest-tiers-partition.test.ts`,
`src/utils/port-contract-single-source.test.ts` and
`src/utils/dev-restart.test.ts` were re-run: 3 files and 51 tests
passed. The `integration` tier is left to CI, because this diff touches
no spawn entry and no integration-layer file other than the new nightly
pin.
- **Gates.** `node scripts/pm/dispatch-gates.mjs --repo
objectstack-ai/objectstack --commands` derived 63 commands. Each was
run, and every one exited 0. `--ran`, with an exit code on every line,
reported `63 derived, 63 run, 0 NOT-MEASURED, 0 UNRUN`, a derived zero.
`check:dual-build-cjs-loads` first answered PREREQUISITE NOT MET: 8
packages outside the cli closure had no `dist/`. They were built, and it
was re-run as part of the 63. Derivation residual: the tree was 9
commits behind `origin/main` `70dae533c5`, and one derivation input,
`scripts/doc-authoring-prose-id.baseline.json`, changed upstream.
- **Lint**, a proven narrowing rather than the full `pnpm lint`, which
is left to CI:
1. Population: both changed TypeScript files are in eslint's own linted
population. `--print-config` resolves 6 and 5 rules for them, and
neither file is ignored.
2. Count: `--format json` read 2 files, 0 errors and 0 warnings.
3. Invariance: the config enables no type-aware linting
(`parserOptions.project` and `projectService` are undefined for both
files). Its only inputs on disk are `scripts/slot-lookup-baseline.json`
and `scripts/query-options-erasure-baseline.json`, and neither is in
this diff, so no verdict on an untouched file can change.
## Acceptance notes
- **`AGENTS.md` written by turbo.** The dev-dependency bump at
`840ec9dab3` moved `turbo` from 2.10.10 to 2.11.5. That version appends
a managed "turborepo agent rules" block, wrapped in HTML comment
markers, to `AGENTS.md` on repository-scoped commands when it detects an
AI agent. `turbo.json` sets no `agentGuidance` opt-out. Measured in this
worktree: the first `pnpm exec turbo run build` after merging `main`
left ` M AGENTS.md` (11 added lines).
`scripts/ablation-dist-preflight.mjs --absent` then answered exit 3 on
its tree reading. The file was restored from HEAD and is not part of
this diff. It is reported to the seat for filing, not fixed here.
- **Port-door anchor.** `src/utils/port-contract-single-source.test.ts`
anchors on the text `const child = spawn(` in `start.ts` to keep the
port door ahead of the spawn. The spawn keeps that spelling inside the
coordinator's `spawnChild`, with a comment saying why, so that
structural pin keeps measuring the same order.
- **Coordinator docblock.** `ServeRestartCoordinator`'s docblock in
`dev-restart.ts` still describes only `os dev`. It was left alone
because `dev-restart.ts` is outside this card's surface on the
no-extraction route. `start.ts` names the coordinator and the pin
instead.
- **Behaviour on signal.** After SIGTERM or SIGINT, `os start` now waits
for the server's graceful shutdown and exits 0. Before, it died on the
signal (shell status 143 or 130) while the server kept running. This
matches `os dev`.
---
_Generated by [Claude
Code](https://claude.ai/code/session_01VvcEokUG1tvVxkceYfR5XB)_
---------
Co-authored-by: Claude <noreply@anthropic.com>1 parent c35436c commit 7164587
3 files changed
Lines changed: 362 additions & 12 deletions
File tree
- .changeset
- packages/cli
- src/commands
- test
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
| 1 | + | |
| 2 | + | |
| 3 | + | |
| 4 | + | |
| 5 | + | |
| 6 | + | |
| 7 | + | |
| 8 | + | |
| 9 | + | |
| 10 | + | |
| 11 | + | |
| 12 | + | |
| 13 | + | |
| 14 | + | |
| 15 | + | |
| 16 | + | |
| 17 | + | |
| 18 | + | |
| 19 | + | |
| 20 | + | |
| 21 | + | |
| 22 | + | |
| 23 | + | |
| 24 | + | |
| 25 | + | |
| 26 | + | |
| 27 | + | |
| 28 | + | |
| 29 | + | |
| 30 | + | |
| 31 | + | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
13 | 13 | | |
14 | 14 | | |
15 | 15 | | |
| 16 | + | |
16 | 17 | | |
17 | 18 | | |
18 | 19 | | |
| |||
445 | 446 | | |
446 | 447 | | |
447 | 448 | | |
448 | | - | |
449 | | - | |
450 | | - | |
451 | | - | |
452 | | - | |
453 | | - | |
454 | | - | |
455 | | - | |
456 | | - | |
457 | | - | |
458 | | - | |
459 | | - | |
| 449 | + | |
| 450 | + | |
| 451 | + | |
| 452 | + | |
| 453 | + | |
| 454 | + | |
| 455 | + | |
| 456 | + | |
| 457 | + | |
| 458 | + | |
| 459 | + | |
| 460 | + | |
| 461 | + | |
| 462 | + | |
| 463 | + | |
| 464 | + | |
| 465 | + | |
| 466 | + | |
| 467 | + | |
| 468 | + | |
| 469 | + | |
| 470 | + | |
| 471 | + | |
| 472 | + | |
| 473 | + | |
| 474 | + | |
| 475 | + | |
| 476 | + | |
| 477 | + | |
| 478 | + | |
| 479 | + | |
| 480 | + | |
| 481 | + | |
| 482 | + | |
| 483 | + | |
| 484 | + | |
| 485 | + | |
| 486 | + | |
| 487 | + | |
| 488 | + | |
| 489 | + | |
| 490 | + | |
| 491 | + | |
| 492 | + | |
460 | 493 | | |
461 | 494 | | |
462 | 495 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
| 1 | + | |
| 2 | + | |
| 3 | + | |
| 4 | + | |
| 5 | + | |
| 6 | + | |
| 7 | + | |
| 8 | + | |
| 9 | + | |
| 10 | + | |
| 11 | + | |
| 12 | + | |
| 13 | + | |
| 14 | + | |
| 15 | + | |
| 16 | + | |
| 17 | + | |
| 18 | + | |
| 19 | + | |
| 20 | + | |
| 21 | + | |
| 22 | + | |
| 23 | + | |
| 24 | + | |
| 25 | + | |
| 26 | + | |
| 27 | + | |
| 28 | + | |
| 29 | + | |
| 30 | + | |
| 31 | + | |
| 32 | + | |
| 33 | + | |
| 34 | + | |
| 35 | + | |
| 36 | + | |
| 37 | + | |
| 38 | + | |
| 39 | + | |
| 40 | + | |
| 41 | + | |
| 42 | + | |
| 43 | + | |
| 44 | + | |
| 45 | + | |
| 46 | + | |
| 47 | + | |
| 48 | + | |
| 49 | + | |
| 50 | + | |
| 51 | + | |
| 52 | + | |
| 53 | + | |
| 54 | + | |
| 55 | + | |
| 56 | + | |
| 57 | + | |
| 58 | + | |
| 59 | + | |
| 60 | + | |
| 61 | + | |
| 62 | + | |
| 63 | + | |
| 64 | + | |
| 65 | + | |
| 66 | + | |
| 67 | + | |
| 68 | + | |
| 69 | + | |
| 70 | + | |
| 71 | + | |
| 72 | + | |
| 73 | + | |
| 74 | + | |
| 75 | + | |
| 76 | + | |
| 77 | + | |
| 78 | + | |
| 79 | + | |
| 80 | + | |
| 81 | + | |
| 82 | + | |
| 83 | + | |
| 84 | + | |
| 85 | + | |
| 86 | + | |
| 87 | + | |
| 88 | + | |
| 89 | + | |
| 90 | + | |
| 91 | + | |
| 92 | + | |
| 93 | + | |
| 94 | + | |
| 95 | + | |
| 96 | + | |
| 97 | + | |
| 98 | + | |
| 99 | + | |
| 100 | + | |
| 101 | + | |
| 102 | + | |
| 103 | + | |
| 104 | + | |
| 105 | + | |
| 106 | + | |
| 107 | + | |
| 108 | + | |
| 109 | + | |
| 110 | + | |
| 111 | + | |
| 112 | + | |
| 113 | + | |
| 114 | + | |
| 115 | + | |
| 116 | + | |
| 117 | + | |
| 118 | + | |
| 119 | + | |
| 120 | + | |
| 121 | + | |
| 122 | + | |
| 123 | + | |
| 124 | + | |
| 125 | + | |
| 126 | + | |
| 127 | + | |
| 128 | + | |
| 129 | + | |
| 130 | + | |
| 131 | + | |
| 132 | + | |
| 133 | + | |
| 134 | + | |
| 135 | + | |
| 136 | + | |
| 137 | + | |
| 138 | + | |
| 139 | + | |
| 140 | + | |
| 141 | + | |
| 142 | + | |
| 143 | + | |
| 144 | + | |
| 145 | + | |
| 146 | + | |
| 147 | + | |
| 148 | + | |
| 149 | + | |
| 150 | + | |
| 151 | + | |
| 152 | + | |
| 153 | + | |
| 154 | + | |
| 155 | + | |
| 156 | + | |
| 157 | + | |
| 158 | + | |
| 159 | + | |
| 160 | + | |
| 161 | + | |
| 162 | + | |
| 163 | + | |
| 164 | + | |
| 165 | + | |
| 166 | + | |
| 167 | + | |
| 168 | + | |
| 169 | + | |
| 170 | + | |
| 171 | + | |
| 172 | + | |
| 173 | + | |
| 174 | + | |
| 175 | + | |
| 176 | + | |
| 177 | + | |
| 178 | + | |
| 179 | + | |
| 180 | + | |
| 181 | + | |
| 182 | + | |
| 183 | + | |
| 184 | + | |
| 185 | + | |
| 186 | + | |
| 187 | + | |
| 188 | + | |
| 189 | + | |
| 190 | + | |
| 191 | + | |
| 192 | + | |
| 193 | + | |
| 194 | + | |
| 195 | + | |
| 196 | + | |
| 197 | + | |
| 198 | + | |
| 199 | + | |
| 200 | + | |
| 201 | + | |
| 202 | + | |
| 203 | + | |
| 204 | + | |
| 205 | + | |
| 206 | + | |
| 207 | + | |
| 208 | + | |
| 209 | + | |
| 210 | + | |
| 211 | + | |
| 212 | + | |
| 213 | + | |
| 214 | + | |
| 215 | + | |
| 216 | + | |
| 217 | + | |
| 218 | + | |
| 219 | + | |
| 220 | + | |
| 221 | + | |
| 222 | + | |
| 223 | + | |
| 224 | + | |
| 225 | + | |
| 226 | + | |
| 227 | + | |
| 228 | + | |
| 229 | + | |
| 230 | + | |
| 231 | + | |
| 232 | + | |
| 233 | + | |
| 234 | + | |
| 235 | + | |
| 236 | + | |
| 237 | + | |
| 238 | + | |
| 239 | + | |
| 240 | + | |
| 241 | + | |
| 242 | + | |
| 243 | + | |
| 244 | + | |
| 245 | + | |
| 246 | + | |
| 247 | + | |
| 248 | + | |
| 249 | + | |
| 250 | + | |
| 251 | + | |
| 252 | + | |
| 253 | + | |
| 254 | + | |
| 255 | + | |
| 256 | + | |
| 257 | + | |
| 258 | + | |
| 259 | + | |
| 260 | + | |
| 261 | + | |
| 262 | + | |
| 263 | + | |
| 264 | + | |
| 265 | + | |
| 266 | + | |
| 267 | + | |
| 268 | + | |
| 269 | + | |
| 270 | + | |
| 271 | + | |
| 272 | + | |
| 273 | + | |
| 274 | + | |
| 275 | + | |
| 276 | + | |
| 277 | + | |
| 278 | + | |
| 279 | + | |
| 280 | + | |
| 281 | + | |
| 282 | + | |
| 283 | + | |
| 284 | + | |
| 285 | + | |
| 286 | + | |
0 commit comments