feat(sync): start a connected team's engine when an agent turns up - #775
Merged
Conversation
This was referenced Aug 14, 2026
fujibee
force-pushed
the
fix/774-autostart-engine
branch
from
August 14, 2026 20:25
aec2e48 to
13c0eec
Compare
A machine restart leaves every sync engine dead and nothing restarts one. The agent keeps working, send keeps committing locally, and nothing reaches the other machines until a person happens to type remote sync start. #765 made that visible, which was the right first step and asks a person to do what the machine can do (#774). Two trigger points, both places an agent already establishes what it is: session-start.sh, where the monitor is started, and actas-claim.sh, where a session takes on a role and therefore a team. ONE ENGINE PER (MACHINE, TEAM) IS NOT ENFORCED HERE, DELIBERATELY. cmd_sync_start already takes the per-team lock, answers 'Sync engine already running' under it, and returns 0; the pidfile is per team. So this calls that command and inherits the invariant. Checking the pidfile here instead would put a second answer to 'is it running?' outside the lock that makes the first one true — and two answers diverge exactly when several sessions open at once, which is the case this feature exists for. The binding check is inherited the same way: the command refuses an unbound or disconnected team by name. A consequence worth stating: no mutation of this file can turn the 'exactly one engine' assertion red, because that property is not implemented here. The race test proves the inheritance, not an implementation. Had I written my own liveness check, that assertion WOULD be mutable — and its being mutable is what a second answer looks like from the outside. NOTHING HERE MAY FAIL A SESSION. Every path returns 0; a start that fails prints what the command said and the session continues, because an agent that will not open because a sync engine refused is worse than an engine that is down. A team already running produces no output at all — starting is a side effect nobody asked for in this moment, and a line would then appear on every session start for the rest of the machine's life. Tests, six, with the concurrent case as the centre: five callers race, every one returns 0, exactly one reports starting, four are silent, and one engine is alive — counted from the process table, not from the pidfile, which can only ever name one and so is the wrong witness. Mutations: treat 'already running' as a start 2 red (incl. the race) propagate the command's exit code 2 red drop 2>&1, losing the stated reason 1 red Interacts with #773: an engine that exits on a server refusal will now be restarted every session and exit again. That issue is next and is not made worse by this landing — today the engine is simply dead instead.
…s enforceable Three review findings, and the first one is this issue one level up. P1-1: the six cases drove agmsg_sync_autostart alone. Deleting the invocation from either trigger left all six green — and #774 IS the triggers, not the helper. The helper working shows nothing about it being called, and the two wirings are different code: session-start awks remote.sh status for connected teams, actas-claim array-ifies TEAMS after the claim. Four cases now drive the real scripts against a fake remote: a connected team is started, a disconnected one is never offered to the command, and the thing the session actually needs still comes out (the Monitor directive; status=ok). That is the third time tonight I tested a command and not its wiring. It is also what the per-trigger deletion mutation is for, and it earned its keep twice over: - 'actas does not wait for a start that hangs' passed with the invocation DELETED. Nothing to wait for is also fast, so the case could not tell a bound from an absence — it was measuring the feature's absence and calling it a bound. - the repair for that was itself timing-fragile: it grepped for the recorded call immediately after the helper gave up waiting, which passes on an idle machine and fails under load. Now it waits for the record, with the session's own bound measured separately from the outside. P1-2: 'nothing here may fail a session' held only for exit status. Both triggers ran sync start synchronously — actas before printing status=ok, session-start before the Monitor directive — and cmd_sync_start waits out a readiness loop of its own before giving up, per team, serially, with no bound if the child hangs. A release-blocker fix that can stop a session from starting is not a fix. The source comment even said 'Best-effort, and bounded' while being unbounded in time. Each start now runs in the background under a whole-call budget (AGMSG_SYNC_AUTOSTART_TIMEOUT_S, 5s). When it expires the child is LEFT RUNNING rather than killed — it may be seconds from success, and killing it could leave a half-made pidfile — and what stops is the waiting. The session says a start is in flight and goes on. A consequence, tested separately rather than folded in: a start that FAILS is only reported as a failure if the command notices within the budget. Past it, the honest sentence is 'still in flight'. Those are different facts. P1-3: five non-terminal [[ ... ]] assertions cannot fail a test under macOS bash 3.2, which is what CI runs. Replaced with printf | grep -q, the form the checker measures as enforced on both interpreters. Mutations, per trigger, each turning only its own side red: session-start's invocation removed its 2 cases red, actas 2 green actas-claim's invocation removed its 2 cases red, session-start 2 green
…he deliberate children Three things, and the first two are the CI failures on the previous head. 1. I removed five unenforceable assertions and added one in the same head. tests/test_sync_autostart.bats had a non-terminal '! grep -q' for the negative (a disconnected team is never offered to the command). A leading '!' does not trip errexit on either interpreter, so it reports ok whatever it finds. This harness already has 'refute'; used it. 2. The warning's runnable remedy was printed with a FOUR-space indent. #765 prints two, and tests/test_delivery.bats extracts the command with sed -n 's/^ bash //p' and then RUNS it. So the deeper indent hid the operator's remedy from the check that proves the remedy is runnable. Back to two spaces, with the reason written where someone might 'tidy' it again. That test is not a stale test. It pins the #761/#765 ruling — do not start anything, make the absence visible — which #774 REVERSES. What #765 built is not discarded: its warning, wording and remedy are what remain when the start fails, which is the case that test drives. The test name and its comment now say a decision was reversed, so this does not read as a test edited to fit new output. Its budget is raised so the failure path is deterministic: under the 5s default the command may not have finished failing when the hook stops waiting, and 'still in flight' is then the honest sentence — a different fact, tested separately. 3. The 'does not wait' cases leave a sync start child running on purpose, and nothing reaped it. A CI shard runs many files in one process tree, so a fake that loops forever becomes somebody else's flake — which fits both OSes failing the same shard numbers. teardown now kills them. My own measurement error made 2 reachable: I reported test_delivery as 'ok' having looked at tail -3. The last three lines being ok is not a suite passing.
…g it The session-start case passed under --filter and failed in the full file. It relied on there being no engine to start, which is true when the file runs alone and not when it runs with its neighbours: what a start does depends on what other tests left behind. That is the same cross-test coupling this branch fixes in its own suite, arriving from the other direction — my children leaking outward there, other tests' leavings breaking my premise here. Both come from a shard running many files in one process tree. So the condition is stated rather than assumed: an unusable interpreter makes sync start fail immediately and for a named reason, which is what the warning under test is about. The case now measures what the operator is told when a start fails, and nothing about what happened to run before it. A --filter pass is not a suite pass, in either direction.
… fails The comment that records the #761/#765 -> #774 reversal contradicted itself. One paragraph said the start fails because the fixture has no engine to start; two paragraphs later it said the failure is forced and inherited from nothing, and the code exports an unusable interpreter. The first sentence was the superseded explanation, left in place while the paragraph around it was rewritten. It is also the explanation this branch threw out: inheriting the failure from the fixture is what made the case pass under --filter and fail in the full file. A durable record of a reversed decision cannot hold both accounts. The forced condition is now the only cause given, with the inherited one described as what it replaced and why. Same shape as the PR body drifting behind its head, one level down: an artifact edited in layers, each layer true when written.
…caller's streams The CI shards were red for three reasons. Two were the feature, not the tests. 1. A bare kill -0. tests/test_instance_id.bats forbids it outside scripts/lib/instance-id.sh, because liveness has to go through _agmsg_pid_alive, which is EPERM-aware and cross-checks ps. My poll asked whether the background child was alive. The question was wrong anyway. What this needs to know is whether the child has FINISHED, and kill -0 succeeds for one that has exited and not been reaped. The child now writes its exit status to a sentinel as its last act and this polls for that file. No pid is examined at all. 2. THE ABANDONED CHILD HELD THE CALLER'S STDOUT. A start that outruns the budget is deliberately left running — and it inherited the streams of whatever called the hook. Anything that CAPTURES that output (run in a test, , a piped hook) then waits for EOF, and a start that hangs hangs the session. That is the requirement this budget exists for, broken where no exit code and no timeout could see it. It surfaced as a suite whose cases were all green and which never finished. The child is now detached: ( ... ) </dev/null >/dev/null 2>&1 & 3. teardown reaped by the name the fake was WRITTEN as, while the hanging one runs as a copy at /remote.sh. Nothing was killed and the child outlived the file. Reaped by / now, which cannot reach anything outside the test's own tree. Also: the cases whose subject is not timing now drive a fake remote.sh that answers instantly. Using the real command made them slow and load-dependent, and raising their budgets only bought a suite that was slow AND fragile. The real command is kept for the race case, where inheriting its lock is the whole point. tests/test_sync_autostart.bats: 10 tests, 0 failures, exit 0.
…not just 0/1/2 The shard check found it: 'every background spawn under scripts/ closes bats fd 3 and fd 4' failed on ubuntu 3/4 and macos 3/4. I had already found that the abandoned child held the caller's stdout and detached it with </dev/null >/dev/null 2>&1. That was necessary and not sufficient. bats hands a harness pipe down on fd 3 and 4, and a child holding those keeps the shard alive after every case in it has passed — the same symptom, one layer further out. scripts/lib/close-fds.sh exists for exactly this leak, and its own comment records the last time: the codex bridge closed 3 and 4 by name while remote-sync.sh had the range close, so the bridge kept the harness pipe and hung a shard. I fell into the same hole from a different spawn path. agmsg_close_inherited_fds is called INSIDE the subshell, which closes the child's copies and leaves this shell's own descriptors alone — the pattern that file's comment prescribes, and the reason the rule has no exceptions to audit. tests/test_close_fds.bats: 4 tests, 0 failures.
fujibee
force-pushed
the
fix/774-autostart-engine
branch
from
August 14, 2026 23:24
0891051 to
83be882
Compare
This was referenced Aug 14, 2026
Merged
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
1 participant
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.
Declared reviewers: 1
Closes #774. Refs #765, #773, #595.
Landing on
integration/remote. Head089105132e273e8ce8c0165717fdd64680a5b67c.What this does
When an agent turns up and belongs to a connected remote team, that team's sync engine is started if one is not already running. Two trigger points, both places an agent already establishes what it is:
scripts/session-start.sh— where the monitor is startedscripts/actas-claim.sh— where a session takes on a role, and therefore a teamscripts/lib/sync-autostart.shis the one function both call.A restart leaves every engine dead and nothing restarts one. The agent keeps working and
sendkeeps committing locally, so nothing looks wrong — the messages simply do not arrive. #765 made that visible; a warning still asks a person to do what the machine can do.One engine per (machine, team) is NOT enforced here, deliberately
cmd_sync_startalready takesagmsg_lock_acquire "$TEAMS_DIR/<team>", answersSync engine already running (pid N).under that lock, and returns 0. The pidfile is per team. So this calls that command and inherits the invariant.Checking the pidfile here instead would put a second answer to "is it running?" outside the lock that makes the first one true — and two answers diverge exactly when several sessions open at once, which is the case this feature exists for. The binding check is inherited the same way: the command refuses an unbound or disconnected team by name.
No mutation of
sync-autostart.shcan turn the "exactly one engine" assertion red, because that property is not implemented here. The race test proves the inheritance.There is no refusal check here, and there was one (#773)
Earlier heads of this PR read
remote.sh statusbefore each start and skipped a team whose server had refused. That is removed, and the reason is a measurement rather than a review count.The check existed to stop a restart loop: the engine used to exit when the server refused, so auto-start would raise it again next session and it would exit again.
#792ended that. On the landed engine a refusal is recorded, logged, and then:It keeps the loop. So starting a refused team costs one quiet process that reports the reason through
status— there is no loop left to prevent.The check was not free. It put a second command in the path where a session prints its Monitor directive and
actasprintsstatus=ok, and bounding it correctly took a background wrapper, a self-imposed deadline, a shared clock, a grace period and a reaping rule — seven rounds of review on machinery whose purpose had already been removed by another PR. Reading the engine settled it; the round count only prompted the reading.Removed with it: three cases asserting a refused team is never started, and two bounding and reaping the lookup. The suite records why they are gone, so "there used to be a check" does not read as an oversight.
Nothing here may fail a session — including by being slow
The first version returned 0 on every path and still ran
sync startsynchronously:actasdid not printstatus=ok, and session start did not emit the Monitor directive, until the engine was ready. A release-blocker fix that can stop a session from starting is not a fix (raised in review).Each start now runs in the background under a whole-call budget,
AGMSG_SYNC_AUTOSTART_TIMEOUT_S, default 5s. When it expires the child is left running rather than killed — it may be seconds from success — and what stops is the waiting.A team already running produces no output at all. The #765 warning survives, in its voice, for teams that could not be started.
Tests
tests/test_sync_autostart.bats, 10 cases — the whole file at this head, named so the count cannot drift from the contents:The two triggers, driven for real — the last four. The first version of this PR drove
agmsg_sync_autostartalone, and deleting the invocation from either trigger left every case green, while #774 is the triggers. Those four run the real scripts against a fakeremote.sh: each trigger starts a connected team's engine and still produces its own output (the Monitor directive;status=ok), and neither is held up by async startthat hangs.The helper, including the case this feature exists for: several sessions race for the per-team lock — every one returns 0, exactly one reports starting, the rest are silent, and one engine is alive, counted from the process table rather than the pidfile, which can only ever name one and is the wrong witness.
What the command said reaches the operator: a refusal from
sync startis repeated rather than replaced, and a failed start does not fail the caller while still carrying the command's own sentence.What CI caught that local runs had not
Six defects reached earlier heads, each found by a CI run rather than by any local one, and each only after the ones before it were fixed:
A new unenforceable assertion in the same head that removed five. A non-terminal
! grep -qreportsokwhatever it finds under macOS bash 3.2.refutenow; the checker is back at its baseline.The runnable remedy was printed with a four-space indent.
#765prints two, andtests/test_delivery.batsextracts the command withsed -n 's/^ bash //p'and then runs it. The deeper indent hid the operator's remedy from the check that proves it is runnable.The "does not wait" cases leave a child running on purpose, and nothing reaped it. A CI shard runs many files in one process tree, so a fake that loops forever becomes somebody else's flake.
teardownkills them now, matched by the path they actually run under.The spawn line did not close fd 3 and fd 4 where the repo-wide check reads.
tests/test_spawn_fd_guard.bats— new on the destination since this branch was last rebased — reads the line a background job is started on, and this one delegated toagmsg_close_inherited_fdsinside the subshell instead. The call does the stronger thing (whatever the runtime handed down, not just 3 and 4), and the redirections are on the line now as well: belt and braces, and visible to a reader without following a function.A forced condition was forced through the machine rather than through the command. The session-start case in
tests/test_delivery.batsmadesync startfail by pointingAGMSG_NODEat a path that does not exist, on the reasoning that an unusable interpreter fails "immediately and for a named reason". That is a claim about a machine, and on a macOS runner it was false: the command had not returned after sixty seconds, so the hook printed "a start is still in flight" — a different fact, tested elsewhere — and the case failed for a reason unrelated to what it asserts.remote.shis replaced for that half of the test by one that refuses at once, and restored before the half that needs a start to succeed. Mutation: make the stub succeed instead, and the case reddens.And the child held the caller's streams. Detaching 0/1/2 was necessary and not sufficient: bats hands a harness pipe down on fd 3 and 4, and a child that keeps them open holds the shard after every case has passed.
agmsg_close_inherited_fdsruns inside the subshell.Measurements, at this head
Every suite run in full and read from its failure count:
Drift
Rebased onto the current destination, which had moved twice. Measured per file, from the repo root:
Measured empty rather than structural: the destination has advanced and did not touch this surface. Re-measured immediately before landing.
#774 closes; #761 does not
Both triggers run when an agent turns up. An unattended restart — the machine comes back with no session on it — starts nothing, so #761 stays open and this says so rather than letting the closing keyword decide it.
The red on
ubuntu-latest 4/4not ok 265 watch: relaunch with the SAME instance id replaces the previous watcher, failing at_wait_pidfile "$pf" "$w2"—tests/test_watch.bats:495.That is #595, filed 2026-08-01, naming this assertion and this call site. This branch's first commit is dated 2026-08-14. It is a race in
scripts/watch.sh's pidfile handover, reproduced under a forced interleaving and fixed in #803.Attribution, measured rather than asserted:
watch.shandtest_watch.bats— followingsource, and deriving the storage drivers from the directorystorage.shloads them from, rather than naming the ones I thought of — does not contain any file this branch changes. Controls:lib/storage.sh,drivers/storage/sqlite.sh,drivers/storage/sqlite-sync.shanddrivers/storage/jsonl.share all found by that instrument, so it is not blind.