fix(remote): sync start keeps the record of an engine it did not stop (#831) by fujibee · Pull Request #840 · fujibee/agmsg

fujibee · 2026-08-16T07:07:31Z

Before pressing: strip every closing keyword from the squash body. Both
commits on this branch contain one aimed at this PR's issue — the first states
it, the second quotes it while retracting it, and GitHub reads the string
either way. --amend is not available here. It never fired while the base was
integration/remote; now that the base is main it would, and it must not:
direction 2 is untouched (see the last section). Nothing matching
(close|closes|closed|fix|fixes|fixed|resolve|resolves|resolved) #<this issue>
may survive into the squash body.

sync start left running sync engines that nothing could name. This is
direction 1 of #831.

What was happening

Measured on Windows 11: readiness could not be confirmed, sync start reported
failure, deleted the pidfile and left. The engine kept running and kept pulling.
Three invocations left three live engines; a follow-up reproduction reached six.
status reported stopped for every one of them, and the pidfile was the only
thing that had ever named them.

Two probes disagree on that platform and the code believed the wrong one. kill -0
cannot see a live engine there (#652), so _remote_sync_engine_status answered
stale; the reap refused to signal a pid it could not confirm was ours, returned
"nothing to stop", and the caller deleted the record anyway.

And even had it decided to signal, it would not have worked. The engine is
launched as bash remote-sync.sh, which runs node. An MSYS kill reaches the
MSYS-side process and the native node.exe under it survives — #831 measured
exactly that, on engines a kill had already been aimed at.

The change

"Gone" requires every probe to say so — compat_pid_gone. Under Git Bash
the pid these shells minted is an MSYS pid, which tasklist does not report at
all (asking it about one is how #567 lost every codex bridge), so the Windows
side is asked about the WINPID, which is the same process under the name that
subsystem uses.

The asymmetry is the point: a probe that wrongly says alive costs a signal
aimed at a pid whose ownership still has to be proven. A probe that wrongly says
gone leaves a live engine with nobody to stop it.

And "could not ask" is not "gone". The Windows side is reached through a
WINPID lookup and tasklist, and either can be missing, fail, or answer
something unparseable. The first version of this let all of those fall through
to gone — so a single POSIX false-gone was still enough to declare a live native
engine dead. That is #652's collapse rebuilt inside the function written to
prevent it, and review caught it. Each of them answers not gone;
tasklist's exit status is checked, not only its output.

The signal goes through whatever the host uses to end a tree —
compat_signal_pid_tree. POSIX signal, then on msys taskkill /PID <winpid> /T,
with /F on the second pass after the polite attempt has been made and waited
on. Windows has no signal to deliver, so the tree is ended by pid.

The WINPID is resolved before the signal, and the same mapping is carried
through. ps -l -p <msys-pid> stops answering once the MSYS side has exited,
so a kill sent first can remove the only means of naming the native process
still running underneath. That is not hypothetical — it is the reported symptom,
the kill returns and node.exe is still there, produced by the order of two
lines. The reap resolves it once while the cmdline check still proves the process
is its own, and that one mapping is used for the signal, the taskkill and the
confirmation that it went.

Ownership is still the cmdline, and never the number. A pid stops naming the
same process the moment that process exits, and /T on a recycled number ends a
stranger's tree. Every pass re-reads _remote_sync_engine_status, which answers
running only while the cmdline names this team's engine; an unproven pid is
reported as "could not stop", not signalled. Overriding a false "gone" is what
brings more pids to that check, which is why the check is the one that survives
reuse.

Then the caller is simple again: records are cleared when the process is not
there (reap 0 — this call stopped it — or 2 — every probe says gone), and
kept when it could not be stopped (1), which is reported.

Two diagnostics were wrong and are fixed:

The give-up message asserted the engine could not reach the server and that
nothing was syncing for the team. Neither was measured, and the engines the
text was written for were reaching the server and pulling the whole time. It
now says what is known: it never became ready, and this command did not stop it.
The manual way out was kill <pid> — the thing sync start abandons the engine it spawned: orphans accumulate and tear the shared log #831 measured does not end the
node process. On msys it now prints the taskkill form, with its WINPID, and
says why.

Controls

tests/test_sync_start_orphans.bats. The end-to-end cases drive the real
cmd_sync_start against a real engine.

The Windows cases drive the Windows route on the CI host: MSYSTEM set, and
a uname / ps / tasklist / taskkill / powershell.exe on PATH that speak
the shapes Git Bash speaks — the cmdline the ownership check reads comes from the
real process, so that check is the real one and not a stubbed yes.

MSYSTEM alone is not the switch. _agmsg_detect_platform asks uname -s,
so on a host answering Darwin the branch under test never executes and every
Windows assertion passes vacuously. That happened while writing these, and the
uname stub is what makes them mean anything.

What they are worth: that the route is chosen and that the process dies
through it. They are not a measurement of native node.exe dying on real
Windows. Only the Windows machine closes that, and it is asked to.

Existing-user impact: YES. On Windows this ends a process tree that the
previous behaviour left running, and it changes which records survive a failed
sync start and what the failure prints. Under the current gate that puts it on
the side needing koit's explicit GO, which it waits for after any CLEARED.

Fifteen mutations, each applied alone to a pristine copy, bash -n checked, then
the whole file run. All fifteen were measured against this 14-case file:

mutation	reddens
`compat_pid_gone` skips the Windows side entirely	1, 2, 4, 5, 6, 8
POSIX signal only, no `taskkill`	1, 2, 7
status back to the POSIX probe alone	1, 2
clear the records even when it could not stop it	11, 13
*resolve the WINPID after* the kill (the old order)**	7
do not carry the mapping into the disappearance check	1
an unobtainable WINPID falls through to gone	5
a missing `tasklist` falls through to gone	6
a `tasklist` that runs and fails falls through to gone	8
treat every platform as msys	3
drop the missing-dependency guard	9
the give-up message names a cause again	12
offer `kill` on Windows too	13
the reap drops its ownership check	14
lower the shipped readiness ceiling	10

Fifteen rows; eleven land on exactly one case; every one of the fourteen
cases is reddened by at least one row.

An earlier version of this table mixed rows measured against a 12-case file with
rows measured against this one. Cases 1–6 kept their numbers across that change,
so the mismatch was invisible by inspection — and on re-running, two of those
rows in fact redden a case that did not exist when they were first measured. A
row is quoted against the file it ran on.

Two rows exist because the first attempt at them came back green. Restoring
the old signal order left every case passing: the reap supplies the WINPID, which
defends compat_signal_pid_tree from its own ordering, so any case reaching it
through the reap is blind to that bug by construction. The case that sees it
calls the function directly with no mapping passed in. And the platform decision
was made twice — a guard on the lookup and another before the taskkill — so
removing either alone changed nothing and neither was held; it is made once now.

The fixture hid one of them. Its ps returned 900000 + pid whatever the
process state, so the pid-to-WINPID mapping was immortal and the code could
always find one no matter when it asked. It now answers only for a pid that is
still there, which is what MSYS2's ps does.

Measured on this head: this file 14/14, and check-enforced-assertions.sh
finds nothing in it.

What this does NOT do

#831 stays open. It names two independent directions:

this one;
each engine writing to its own log, or atomic appends. Untouched. Every
engine still appends to run/remote-sync.<team>.log, lines still tear into
each other, and the readiness poll still greps that shared file.

It also does not reduce the number of engines that can be started — two callers
that both misread still start two. What it stops is one of them being left with
nothing pointing at it.

A cost it adds, named: on msys, when the POSIX probe says gone,
compat_pid_gone pays a ps and a tasklist per call, and the readiness poll
can turn up to its ceiling. _compat_get_winpid is deliberately not memoised
— a cache there would be keyed on a pid, which is the reuse hazard this change is
built to survive. That failure path is the one #779 is already open about.

Refs #831

Reported from the field: repeated `sync start` attempts left sync engines running that nothing pointed at. Three invocations, three live engines, and `status` reporting stopped for every one of them. The give-up path is where they came from. When the engine never becomes ready the starter reaps it and clears its records, and `_remote_sync_engine_reap_owned` answered 0 -- "reaped" -- for a pid it had only READ as gone, having signalled nothing. On Windows a live engine reads as dead (#652; #505 gives one way, a DWORD pid above the POSIX ceiling that kill(1) will not parse), so the records of a RUNNING engine were deleted. The pidfile is the only thing naming it, so from then on no `status` could see it and no `stop` could reach it, while it kept retrying on a backoff. So the reap now separates its own act from a reading: 0 it signalled the engine and the engine went 2 the engine read as gone; nothing was signalled 1 it could not stop it and `cmd_sync_start` clears the pidfile and the cycle stamp on 0 only. On 2 it leaves them and says which situation it is in: a stale record is what `status` already knows how to describe, and a live process with no record is not something the operator can act on at all. #832 fixed the probe that misread. This does not stand on that fix, because the harm does not depend on which misreading happens. Two smaller things came with it. Both call sites capture the status with `|| rc=$?` rather than calling bare -- this file runs under `set -e`, where a bare call returning non-zero exits the script instead of reaching the message below it. And the "could not retake the registry lock" line no longer says "the engine is stopped" on a path where nothing was signalled; that is the one thing that path does not establish, and the line is all the operator gets. Six cases in tests/test_sync_start_orphans.bats, driven against a real engine with the local liveness probe forced to the reading Windows produces. Each has its negative control in the file: 2 is answered for a reading, 0 for an act, records survive when blind and are cleared when honest. Closes #831

The previous commit ended with `Closes #831`, and that was wrong. #831 names two independent directions and this branch is the first of them. The second -- every engine appending to one shared log, so lines tear into each other and every tool that reads that log reads fragments -- is untouched, and no case in the file says anything about it. The keyword does not fire from `integration/remote` anyway, so nothing was closed; the claim was still in the record, where the next reader would have believed it. It also does not reduce the number of engines that can exist. Two `sync start` calls that both misread still start two. What it stops is the second half of the harm, where the record of one is deleted and nothing can name it after. The scope now sits in the test file's own header rather than only in a PR body, because that is where someone reads it when they come looking for a case that is not there. Refs #831

The previous two commits on this branch hardened the give-up path -- an engine that could not be stopped kept its record instead of becoming anonymous -- and called that direction 1 of #831. Review was right that it is not. Direction 1 asks that the engine be reaped before failure is reported, and that the reap work on Windows, which is where every measured orphan came from. Keeping a name on an orphan is a smaller claim than not leaving one. Two things were missing, and they are the same shape: a reading was allowed to end the matter. "Gone" was one probe's answer. Under Git Bash `kill -0` cannot see a live engine (#652), so `_remote_sync_engine_status` reported `stale` before it ever reached the check that proves ownership, and the reap declined to signal a pid it could not confirm. `compat_pid_gone` now requires every probe available to agree, and on msys asks the Windows side about the WINPID -- not the MSYS pid, which `tasklist` does not report at all and which answered "dead" for every running codex bridge in #567. And the signal was `kill` alone. The engine runs as `bash remote-sync.sh`, which runs `node`; the MSYS signal reaches the shell and the native node.exe under it survives, which is how the engines in #831 outlived a kill already aimed at them. `compat_signal_pid_tree` sends the POSIX signal first and unconditionally, then on msys ends the tree by pid: `taskkill /PID <winpid> /T`, with `/F` only on the second pass, after the polite attempt has been made and waited on. Ownership is unchanged, and deliberately so. A pid stops naming the same process the moment that process exits, and `/T` on a recycled number ends a stranger's tree; overriding a false "gone" brings more pids to that check, so the check has to be the one that survives reuse. It is the cmdline, re-read every pass, and an unproven pid is still reported as "could not stop" rather than signalled. That leaves the caller simple: the records are cleared when the process is not there, kept when it could not be stopped, and the difference is reported. Two diagnostics said more than was measured. The give-up message asserted the engine could not reach the server and that nothing was syncing for the team -- the engines it was written for were reaching the server and pulling the whole time. And the only manual way out it offered was `kill <pid>`, which #831 measured does not end that process; on msys it now prints the taskkill form with the WINPID, and says why. The readiness poll's ceiling gains a test seam, defaulting to the shipped 1600 turns. Reaching the give-up path costs the whole ceiling, and the regressions that drive it are not about that number; one case leaves the seam unset and asserts the sixteen-second floor, so the default stays on the path of something. Ten cases in tests/test_sync_start_orphans.bats, ten mutations, each landing on its own assertion. The Windows cases drive the Windows route on a POSIX host -- `uname`, `ps`, `tasklist`, `taskkill` and `powershell.exe` stubbed to the shapes Git Bash speaks, with the ownership cmdline coming from the real process. That measures that the route is chosen and that the engine dies through it. It is not a measurement of native node.exe dying on real Windows; the Windows machine is asked for that. MSYSTEM alone does not select the platform -- `_agmsg_detect_platform` asks `uname -s` -- so the first version of these cases passed without ever entering the branch under test. Refs #831

Two defects in the previous commit, both found in review, and both the same shape as the bug this branch exists to fix. `compat_pid_gone` let every way of failing to reach the Windows side fall through to "gone": no WINPID, no `tasklist`, a `tasklist` that ran and exited non-zero. So a single POSIX false-gone was enough to declare a live native engine dead -- which is #652's collapse, rebuilt inside the function written to prevent it. Each of those now answers "not gone", `tasklist`'s exit status is checked and not only its text, and the caller's cmdline check is what still keeps a dead pid from reading as a running engine. `compat_signal_pid_tree` sent the POSIX signal and then looked up the WINPID. `ps -l -p <msys-pid>` stops answering once the MSYS side has exited, so the kill could remove the only means of naming the native process still running underneath -- the reported symptom, "the kill returns and node.exe is still there", produced by the order of two lines. The mapping is now resolved before the signal, and `_remote_sync_engine_reap_owned` resolves it once while the cmdline check still proves the process is its own, then carries that same mapping through the signal, the taskkill and the confirmation that it went. The platform decision in `compat_signal_pid_tree` was made twice -- a guard on the lookup and another before the taskkill -- so either could be removed with nothing to show for it, and no control held it. It is made once now. The fixture hid the ordering defect: its `ps` returned `900000 + pid` whatever the process state, so the mapping was immortal and the code could always find a WINPID no matter when it asked. It answers only for a pid that is still there, which is what MSYS2's `ps` does. Four cases were missing and none of them were reachable through the reap: - The ordering, driven on `compat_signal_pid_tree` directly with no WINPID passed in. Every case that reaches it through the reap is blind to this by construction, because the reap supplies the mapping and thereby defends the function from its own order. Restoring the old order left twelve cases green. - "Cannot ask" split by which line failed: no WINPID, no `tasklist`, and a `tasklist` present that exits non-zero -- that last being the one that most looks like an answer. Fourteen cases, fourteen mutations, every case reddened by at least one and eight landing on exactly one. Refs #831

…ing the test Three CI failures on the previous head, all of this branch's doing, and the first is the one worth reading. Removing the unmeasured cause from the give-up message took a MEASURED fact out with it. The line said the engine could not reach the server -- not established, correctly deleted -- and, in the same breath, that it will keep retrying on a backoff, which is the engine's own documented loop and the thing that turns "a command failed" into "something is still running on this machine". #731's test was pinning exactly that half. A deletion removes somebody's premise, and this one was not swept for readers before it was made; the test found what a grep would have. The consequence is restored, the cause is still gone. The other two were my new cases failing on ubuntu and passing on macOS, which is the shape that should have been suspicious on its own. Forcing the msys branch on a POSIX host sends `compat_get_cmdline` to `/proc/<pid>/cmdline` when one exists. macOS has none, so the stubbed Windows lookup answered. Linux has one, so it short-circuited past every stub and returned the real cmdline with a TRAILING SPACE -- `tr '\0' ' '` converts the final NUL -- and the ownership check's `*" run --team <team>"` suffix stopped matching. Ownership then could not be proven, the reap declined, and the engine survived: exactly the two assertions that failed. Measured directly rather than inferred: that suffix matches the cmdline without the trailing space and does not match it with one. So `_AGMSG_COMPAT_NO_PROC=1` now rides with `MSYSTEM` at all nine stub sites, which is what that variable exists for -- a native Windows process has no MSYS /proc entry either, so this is the fixture modelling the platform rather than the host it happens to run on. 14/14 here, and test_remote_status_liveness 31/31.

fujibee added 2 commits August 16, 2026 00:02

fujibee changed the base branch from integration/remote to main August 16, 2026 07:59

fujibee added 3 commits August 16, 2026 02:04

Navigation Menu

fix(remote): sync start keeps the record of an engine it did not stop (#831) - #840

fix(remote): sync start keeps the record of an engine it did not stop (#831)#840
fujibee wants to merge 5 commits into
mainfrom
fix/831-sync-start-does-not-abandon-its-engine

fujibee commented Aug 16, 2026 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

fujibee commented Aug 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What was happening

The change

Controls

What this does NOT do

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

fujibee commented Aug 16, 2026 •

edited

Loading