feat(cli)!: detach and persist meshes on up - #880
Conversation
`cotal up` always detaches the stack from the invoking process and always writes a durable self-hosted mesh record at provision time. A stopped stack stays listed as recorded at its root rather than denied as nonexistent. `--foreground` remains for debugging; `--detach` is a no-op. Claiming a self-hosted space from another root is refused; Windows detached spawns set windowsHide.
Live up-stack cells prove persist and default detach first so their named mutations kill those assertions rather than an earlier ENOENT or a spawnSync SIGTERM that still exits 0. Registry smoke covers other-root self-hosted claim, list tags, and sweep keeping offline self-hosted records.
rev880 rejected #880 because Node spawn({detached:true}) on Windows is DETACHED_PROCESS|CREATE_NEW_PROCESS_GROUP only - libuv will not set CREATE_BREAKAWAY_FROM_JOB, so a GHA job object still kills the stack while the self-hosted record lists it as running. Notes, not the finished wire: POSIX stays Node detached+unref; Windows must probe IsProcessInJob + BREAKAWAY_OK and either spawn with CREATE_BREAKAWAY_FROM_JOB or throw naming --foreground. Do not refuse every win32 host (a local console is usually fine). Call sites still use the weaker spawn until that probe is real.
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| @@ -1,9 +1,10 @@ | |||
| import { spawn, spawnSync } from "node:child_process"; | |||
| import { spawnSync } from "node:child_process"; | |||
…sist # Conflicts: # extensions/connector-core/src/docs-bundle.generated.ts
|
rev880b review at REJECT @ 83b595dBlocker: the Windows probe confuses Established by reading: exact PR head and merge-base diff; native calls and every branch; Microsoft job and process-creation flag contracts; Ran on macOS, not Windows: Residuals, non-blocking this round: the live suite does not assert a same-stack second |
|
rev880c review seat, exact head REJECT @ 83b595d Blocking wrong result:
The round-one Windows blocker is closed by contract inspection. Windows evidence boundary: I could not execute Windows native process creation on this macOS host. I established the native contract by reading. Windows CI does execute the pure decision seam through the sharded Lifecycle evidence: the live suite establishes bare Mutation evidence run:
Containment proof, run before any stack-start/teardown exercise:
|
|
Parking the review on this PR rather than leaving it looking stalled, and the reason is worth Three review seats on this lane have now taken down the operator's live mesh while trying to grade The mechanism is this PR's territory. Grading "does That is filed and analysed in #884. It is not a defect in this PR, and this PR is not blocked on it. What it does mean: the behavioural half of this review cannot be executed safely on a machine that No action needed from anyone here. Re-seating once the exercise is redesigned. |
|
Branch updated with current The merge was clean, and that was the thing to be careful about.
Nothing in the merge output can show that. The entry was moved to the end of the file. Verified by computation rather than by diff, and re-derived independently of the report that produced it:
Also checked while updating:
No suites were run and the feature was not exercised as part of this update; it was scoped to merge-and-push only. Merging to |
|
TERMINAL: APPROVE Graded exact head I reviewed Blocking classes:
Named gap: the exact-head Linux CI run Named residual: the mutation configurations are strong and name the correct first-failing cells, but CI runs the underlying suites, not these two feature-specific mutation-proof configurations themselves. That is not a hold because the cells are non-vacuous by direct control-flow inspection, but retaining published mutation-proof output with the PR would make the evidence easier to audit. |
|
CI gap clarification: GitHub Actions is experiencing a major service incident. The exact-head CI workflow at |
|
CONTRACT lens verdict for
Validated at that SHA with all Artifact accounting: the root Named gaps: I did not run |
|
BLOCKERS — lens A: SAFETY / ADVERSARIAL Graded exact SHA
What I checked safely:
NAMED GAPS / NOT CHECKED: I did not run |
|
APPROVE — lens C: TESTS (coverage, gate wiring, mutation), exact SHA Evidence:
NAMED GAPS / RESIDUALS:
|
|
Lens A addendum after broad safe gates, same exact SHA
Additional safe whole-result evidence:
The named lifecycle gaps from the terminal verdict remain. No forbidden lifecycle verb or live suite was run. |
Review panel result for
|
| Lens | Verdict |
|---|---|
| Safety / adversarial | BLOCKERS (3) |
| Contract (release, docs, generated artifacts) | BLOCKED (2) |
| Tests (coverage, gate wiring, mutation) | BLOCKER (superseded its own earlier APPROVE) |
A hash is only clear when every lens clears it. The tests lens initially approved, then withdrew that in favour of a blocker on finding 2 after reproducing it; its other results below still stand. Each finding below was re-derived independently against the code before being accepted.
Blocking
-
A live stack can be double-brokered.
claimSpacereturns atup.ts:2188whenorigin === "self-hosted"and the canonical roots match — before theisReachable(existing.server)probe at 2198. Same root with a different server therefore skips the liveness check entirely. Two things sharpen this: the docstring at 2176 describes the refresh path as "same server + root", so the code accepts a strictly wider case than its own contract claims; and the guards disagree on equality —up.ts:633and:822compareheld.root === rootraw while 2188 comparesrealpathSafe, so a canonically-equal but raw-different root (on darwin,/tmpvs/private/tmp) slips the first and is waved through by the second. The self-hosted fixtures inmeshes-registry.smoke.tsare allserver: DEAD(line 630), so the added test passes for both the safe and the unsafe behavior. -
smoke:flag-inventoryis red at this sha.up.ts:171registersforeground, but the golden inbin/smoke/flag-inventory.smoke.ts:33still lists onlydetach:boolean. That suite is gated atci-suites.txt:167— executable index 150, so shard 2 fails. The suite exists specifically to force conscious acknowledgement of a public flag change, so failing it is the gate doing its job. Confirmed through the real CLI composition root:cotal __complete up --returns--server,--detachand--foreground. -
The changeset omits
@cotal-ai/connector-core, whosedocs-bundle.generated.tschanged. The fixed group bumps it regardless, so the version is unaffected and what is lost is the CHANGELOG entry. -
docs/cli.md:161overstates the registry behavior. It sayscotal up"always records the mesh as self-hosted", butup.ts:2242preservesorigin: "manual"on a refresh. Accurate narrowing: a launch that starts the broker records it as self-hosted; a refresh that merely finds one already answering preserves an operator-owned manual record.
Raised, not blocking
- Spawn-to-record orphan windows. Broker spawn at
up.ts:2030, pid file at 2054, registry write at 2090 — a launcher death in between leaves an unrecorded detached broker, with related windows in the delivery, manager and auth paths. The pattern predates this PR, but makingupdetached by default promotes it from an opt-in risk to the default one. Flagging for a scope call: fix here, or track separately.
--detach was checked and is not a defect: it is an announced deprecation, with the no-op documented in the flag metadata (up.ts:170), an explicit refusal of --detach --foreground (up.ts:202), and a docs entry (cli.md:151).
On CI
Only docs has run at this sha; unit, smoke and live have not executed at all, against a queue backlog. That is worth stating plainly because an empty check list reads like a pass — it is the absence of a red, not the presence of one. Finding 2 also means a green full matrix is not reachable at this sha regardless of the backlog, so the head has to move.
What the tests lens verified
Recorded because it narrows what the fix has to re-establish: windows-detached-spawn does execute on darwin (3/3, both declared mutations killed) and is gated; ci-suites.txt is a strict prefix-preserving append — all 385 prior entries unchanged and in order, the new suite at index 385, no other suite changing shard; meshes-registry passes 164/164; and mutations deleting the self-hosted stale-sweep and root-teardown protections each killed their named cells and restored green.
|
BLOCKER — this supersedes my earlier APPROVE. Lens C: TESTS (public acceptance, coverage, gate wiring, mutation), exact SHA
The PR adds the public Positive controls and boundaries:
NAMED GAPS / RESIDUALS remain unchanged: I did not run forbidden |
Probe a live self-hosted endpoint before allowing a same-root server move, and use canonical root identity throughout the already-running guards. Add live registry coverage and a mutation ledger for the refusal. Update the foreground flag inventory, changeset package set, and mesh provenance documentation with its generated bundle.
|
CONTRACT delta re-grade for Bounded to the two prior CONTRACT findings and the single
I did not re-read or re-run the unchanged PR surface, and did not grade the safety implementation/tests or flag-inventory golden beyond what was necessary to establish package coverage. Prior named gaps remain unchanged: no stack verbs, no live suites, no native Windows breakaway verification. |
|
APPROVE — bounded delta-only re-grade, lens A: SAFETY / ADVERSARIAL Graded exact SHA Finding 1 is closed. The canonicalized earlier guards are correct, not merely consistent. When an answering server's registry root is an alias of the current root, routing into the already-running refresh branch is the safe result: pidfiles, store, auth, logs, and control-plane repair are root-scoped, and treating the alias as foreign would either auto-port around the live same-root stack or issue the wrong foreign-listener remedy. At the default-port fallback, the canonical same-root case now refuses instead of allocating another broker against the same root. Coverage discriminates. Finding 2 is intentionally outside this delta verdict. Prior named gaps carry forward unchanged. |
|
APPROVE — delta-only lens C: TESTS, exact SHA
No new gaps beyond the previously named forbidden live-up and native Windows residuals. |
Round 2 result for
|
| Lens | Round 1 | Round 2 at 18d7b4b2 |
|---|---|---|
| Safety / adversarial | BLOCKERS (3) | APPROVE |
| Contract | BLOCKED (2) | APPROVE |
| Tests | BLOCKER | APPROVE |
What the fix established
claimSpaceno longer authorizes a second broker over a live stack. It returns early only for the same server and the same canonical root. A self-hosted record at the same root but a different server now probes the recorded endpoint and refuses while it answers; a dead endpoint still returns, so the auto-port restart survives. The docstring was rewritten in the same commit, so the contract and the code no longer disagree. That gap was half the original finding.- One spelling of the root rule.
up.ts:630,up.ts:819andensureRootForSpacenow use the registry's sharedcanonicalRoot, and the localrealpathSafecopy is deleted. A broker recorded under a root alias is correctly treated as this root's own already-running stack, since pidfiles, store, auth, logs and control repair are all root-scoped. - The coverage gap is closed with a discriminating pair, not an extra assertion: a live-endpoint cell that asserts the refusal names the endpoint, a cell proving the record survives the refusal, and the pre-existing dead-endpoint cell renamed to mark it as the control. Registry suite 166/166, up from 164. Mutation fixture
claim-space-live-root.json: deleting only the liveness guard reddens exactly the named live-endpoint cell and leaves the dead-endpoint control green, with a named assertion failure rather than a crash. - Changeset now covers every package with a first-party change (
cotal-ai,@cotal-ai/cli,@cotal-ai/workspace,@cotal-ai/connector-core), anddocs/cli.mdstates the actual rule: a launch that starts the broker records self-hosted; a refresh that merely finds one answering preserves an operator-owned manual record.
CI
Green at this sha: live, unit, docs, both CodeQL analyses, every Windows shard plus soak, and smoke shards 2 and 3. Shard 2 confirms the repair that forced this round: flag-inventory smoke passed (43 commands) and meshes registry smoke: 166 checks passed.
Smoke shards 0 and 1 fail, and they are inherited from main, not introduced here. Compared frontier to frontier rather than by shard number, since a shard aborts at its first failing suite and equal shard numbers prove nothing:
this PR origin/main
shard 0 FAILED at pnpm smoke:artifact-store = FAILED at pnpm smoke:artifact-store
shard 1 FAILED at pnpm smoke:backup-inventory = FAILED at pnpm smoke:backup-inventory
Both are the backup-inventory family tracked in #666. ci-ok is red because those two are.
Worth recording separately, since it is a main problem rather than this PR's: shard 2 intermittently hangs at smoke:opencode-events-release and is killed at a 30-minute limit. When that happens the shard is reported cancelled, which discards suites that demonstrably passed earlier in the same job. The passes are still readable in the job log. A single-job re-run completed cleanly here.
The one open item
The spawn-to-record orphan windows raised by the safety lens (broker spawn at up.ts:2030, pid file at 2054, registry write at 2090, with related windows in the delivery, manager and auth paths) were deliberately left out of this fold. The pattern predates this PR, but making up detached by default promotes it from an opt-in risk to the default one. That is a call about what ships rather than a defect in the fix, so it is left for the maintainer: address it here, or track it separately and merge.
Blocker found after the round-2 approvals: Windows teardown is inert, and
|
Forward kill signals to the native detached pid and expose active null exit state so teardown guards behave like real ChildProcess handles. Fail loudly when native Windows exit cannot be observed, and cover both regressions through the injected non-live contract seam and mutation ledger.
Normalize an already-gone Windows pid to the ChildProcess kill contract, keep pid cleanup in finally, and retain the bind or readiness failure as the primary error when cleanup fails. Extend the non-live Windows contract smoke and mutation ledger across both throwing cleanup paths.
The suite exists and nothing runs it, so gate-inventory fails. The chain file bin/smoke/ci-suites.txt is frozen by position until PR #880 lands: its shard walk is round-robin by index, so a mid-file insert re-shards every later suite across CI runners. Declare the suite ungated with the freeze as its reason instead, and align the detached-spawn live assertions with the new compact provenance row.
Blocking: a stopped self-hosted mesh keeps the
|
|
Blocking the current head pending a Windows process-identity redesign. The detached launcher closes the native process handle and retains only a PID. Later A successor needs a stable process capability or recorded creation identity with atomic verify-and-terminate semantics, explicit legacy-record behavior, and real Windows validation. No current-main integration was attempted after confirming this blocker. Live |
|
Tracking the required stable Windows process-identity redesign in #969. This PR remains blocked on that prerequisite. |
davidfarah2003
left a comment
davidfarah2003
left a comment
There was a problem hiding this comment.
LANDING-QUEUE REVIEW: BLOCKERS
- Body contradicts the shipped code on the central design claim. The body says the mechanism is Node
spawn({ detached: true, windowsHide: true })plusunref(), with "no platform-conditional fallback", and that "Windows detached spawns setwindowsHide".implementations/cli/src/lib/detached-spawn.ts:130is exactly a platform conditional —if (process.platform === "win32") return windows.spawn(command, args, opts);— andwindowsHide: trueappears only in the non-win32 branch on the next line. The win32 path never sets it;nativeWindowsLauncherpassesDETACHED_PROCESS(0x8) toCreateProcessinstead. The whole ~100-line PowerShell + inline-C# P/Invoke launcher atdetached-spawn.ts:76-127(IsProcessInJob,QueryInformationJobObject,CreateProcesswithCREATE_BREAKAWAY_FROM_JOB) goes unmentioned in the body.docs/cli.mdanddocs/run-a-mesh.mddo document the breakaway probe, so the docs are ahead of the body — which makes this a body defect, not a docs one, but a reader of the body is told the opposite of what lands. - Evidence under-reports the shipped proof. Three mutation fixtures ship; one is named.
bin/smoke/mutations/windows-detached-spawn.json(13 mutations, cells C01-C16) andbin/smoke/mutations/claim-space-live-root.json(1 mutation) carry no reported kill counts, and the brand-new 16-cell suitebin/smoke/windows-detached-spawn.smoke.tshas no reported run at all — Evidence lists onlysmoke:up-stack:live29,smoke:meshes-registry164, typecheck, changeset status,check:docsbundle. The body closes that section with "Named cells, nothing else," which is precisely the claim the two silent fixtures break. - ci-suites comment is attached to the wrong suite. The suite name
smoke:windows-detached-spawnis correctly at the absolute tail (bin/smoke/ci-suites.txt:548), but its two-line rationale comment was inserted mid-file atbin/smoke/ci-suites.txt:533-534— betweensmoke:control-transport-dial(532) and the existingboot-self-heal-gaterationale block (535-538). It is shard-neutral (ci-suites.mjsstrips comments before indexing), but it now reads asboot-self-heal-gate's rationale, 14 lines from the suite it describes. - Verified clean otherwise: no AI/tool attribution anywhere in the diff;
.changeset/tidy-tigers-detach.mdisminoracross cotal-ai / cli / connector-core / workspace, satisfying (g) for the!subject; the doc sweep is complete and both-polarity guarded —website/scripts/check-dist.mjsfails on both a survivingnpx cotal-ai up --detachand a missing--foreground.self-hostedorigin is threaded consistently throughpruneMesh,removeMeshesByRoot,localMeshesForRootandrender.ts. - (i) Queue interaction: PR #1057 ("refuse to signal a recycled pid") rewrites the same three files this PR converts to
spawnDetached—auth-proc.ts,delivery-proc.ts,manager-proc.ts— and is the direct fix for the "HELD DESIGN BLOCKER: the native launcher closespi.hProcessand returns a bare pid, so this smoke does not prove process identity remains pinned across pid reuse" thatwindows-detached-spawn.jsonholds out of scope. These two want an explicit landing order. Textual tail conflict inci-suites.txtwith #921, #981, #1053, #1054; #1052/#1034/#1033 restructure the file andshard.mjsunder it.
Sequencing note: three other open PRs are independently fixing the defect blocking this oneThe 2026-08-29 block here is PID identity — the detached launcher closes the native handle, keeps a
They are not merely adjacent. Measured from each PR's merge base with and several more by two of the three ( So whichever lands first forces the other two to refold on the exact files carrying their design, and What this needs, and it is not more reviewA decision about which design is canonical, taken once, before any of the three merges. The Review capacity spent grading them independently is largely wasted: two of the three verdicts will be Current state of the three, for whoever picks this up
Filing this as an observation on the blocked PR rather than as a new issue, because nothing here is |
|
Measured state of this PR against current This PR cannot currently produce any CI evidenceIt is in a conflicted state, and a pull request whose merge ref cannot be computed mints zero The conflicts, measured rather than assumed
This is a semantic merge in production code, not a mechanical rebase. An earlier characterisation of the conflicted backlog as a cheap clerical pass was mine, and it was wrong. I withdrew it after measuring. One of the seven is not real work: The overlap with #1069 and #1057, which is the part that constrains orderingAll three PRs conflict with
All three are editing process lifecycle in the same place. Whichever lands first, the other two conflict again immediately and must rebase onto the result. These need an order. Resolving them in parallel guarantees rework. A note on which order, since the assumption may be backwardsThe sequencing has been discussed as though minimising rework puts this PR late, which would conflict with the ci-suites freeze waiting on it. The measurement suggests the opposite may hold:
So the freeze and the rework argument may point the same way rather than opposite ways, which would remove the tension rather than requiring a trade to be made. What that does not settle, and why this is a note rather than a recommendation: this PR is a breaking change and currently carries no review decision. Landing it first is a review question before it is a sequencing question, and that is not mine to answer. The measurement above is offered so that whoever does answer it is working from the current tree rather than from an assumption about it. |
Merge hazard not visible in this PR's diff: it would resurrect two files main deletedThis branch is 1143 commits behind Both were removed by
Restoring the file while main carries its replacement at the same shard index is the failure mode that None of this appears in the diff of this PR, because a PR diff is computed against the merge base, Method, so it can be re-run: Also note the PR is Given 1143 commits and an eight-day-old approval set, the rebase is large enough that the existing |
Retraction: my resurrection finding above is wrong. There is no resurrection on this PR.I ran a check that cannot answer the question I asked it, and published the result. Correcting it The decisive test, which I should have run first: compute the merge and look in the result. Every file I listed is absent from the merged tree. Git resolves them correctly. Why my check was wrong.
The discriminator is What was correct in the original comment and still stands: the branch is behind main by the stated Apologies for the noise, and for the review time it would have cost. The class of error is worth |
|
Status note after seven days at the same head, so the stall is recorded rather than inferred. I am not lifting the block and I am not adding one. The block from 2026-08-29 is live, not stale. The head is still Two structural facts worth having on the thread, because together they explain why this has not moved and would stop the next person re-staffing it into the same wall. 1. It is 2. The validation the block asks for cannot be produced on the fleet this lane runs on. The successor needs "real Windows validation" and exercise of So the honest shape of what is needed is a successor that (a) records a creation identity rather than a bare PID and gives Nothing here changes the verdict. It is a note about why the verdict has not been actionable, and what a lane would need before it could be. |
State of this PR, so the approvals above are not read as merge-readinessMeasured 2026-09-08 against Three terminal APPROVEs sit on this thread at that exact sha, dated 2026-08-26, on the CONTRACT, SAFETY and TESTS lenses. They are not void. The head has not moved, so each still binds the commit it names, and nothing here disputes the review work that produced them. What they cannot do is carry a merge. They grade a head; a merge commit is a third artifact, and with 1562 commits of divergence and a conflicting merge the resulting tree would bear little resemblance to the one those three seats read. A verdict on So the position is: reviewed, and not mergeable. Not "approved and waiting", which is how a reader scanning for green ticks would take it. What a path forward needsEither a refold onto current That choice belongs to the author and to whoever owns the No lane is currently staffed on this PR. |
1 participant
cotal upalways detaches the stack from the invoking process and always writes a durable self-hosted mesh record at provision time. The right path is the only path: an agent session, SSH hangup, or CI runner is a hostile parent, not a load-bearing one.Defects
Two operator-visible failures on a live broker:
up-started mesh vanished fromcotal meshesonce its processes died. Targeting it denied that the mesh existed (no mesh named "X" is running) instead of naming the recorded root and how to restart.cotal upran the stack as children of the invoker. Killing the invoker took the broker with it.Behavior
spawn({ detached: true, windowsHide: true })plusunref()— nosetsidbinary, no platform-conditional fallback.--foregroundremains for debugging;--detachis a deprecated no-op.meshes adduses (~/.cotal/meshes/), originself-hosted, with its root.downand sweep keep that record. A dead stack lists as self-hosted/offline; targeting it saysmesh "X" is recorded at <root> but not running - run cotal up there to restart.windowsHide.Evidence
COTAL_HOME): afterdown,cotal meshesprintedno meshes registeredandcotal psdenied existence; bareupprinted Ctrl-C copy and SIGTERM of the invoker left 0 survivors.self-hosted, dead list isself-hosted · offline, targeting names the root and restart command; bareupexits 0 and the broker reparents to pid 1.pnpm smoke:up-stack:live29 checks;pnpm smoke:meshes-registry164 checks;pnpm typecheck;pnpm changeset status0;pnpm check:docsbundle.bin/smoke/mutations/up-daemonize-persist.json): persist skip KILLED onup persists a self-hosted mesh record at provision time(2/29); detach revert KILLED onbare up exits 0 while the stack remains alive (detached by default)(3/29). Named cells, nothing else. Suite command rebuilds because CLI/live smokes resolve product code throughdist/.smoke:artifact-storeandsmoke:gate-inventory(main CI baseline red at cb7f384: shard 0 smoke:artifact-store (#666/#356 mechanism), shard 1 gate-inventory (ungated smoke:jcode-private-lifecycle) #868).smoke:mutation-fixturesis also red on main (dead anchors in auth/connector-core fixtures); this PR's two anchors are present and unique.Fixes #864