feat(sync): start a connected team's engine when an agent turns up - #775
Open
fujibee wants to merge 6 commits into
Open
feat(sync): start a connected team's engine when an agent turns up#775fujibee wants to merge 6 commits into
fujibee wants to merge 6 commits into
Conversation
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.
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.
Landing on
integration/remote. Headefaa8547fb1ff4443aa38db832082dbb8b87ee30.This PR is not to be landed before
fujibee/agmsg#773. See the last section — the ordering is a condition, not a preference.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.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, before starting anything.
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. Had I written my own liveness check it would be mutable — and being mutable is what a second answer looks like from the outside.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.cmd_sync_startwaits out a readiness loop of its own before giving up, per team, serially, with no bound at all if the child hangs. A release-blocker fix that can stop a session from starting is not a fix (raised in review — 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, default 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 and the tool says different things.
A team already running produces no output at all. Starting is a side effect nobody asked for in that moment, and a line there would appear on every session start for the rest of the machine's life. The #765 warning survives, in its voice, for teams that could not be started — now with the reason the command gave.
Tests
tests/test_sync_autostart.bats, 10 cases.The two triggers, driven for real. The first version drove
agmsg_sync_autostartalone, and deleting the invocation from either trigger left all six green — while #774 is the triggers, not the helper. The helper working shows nothing about it being called, and the two wirings are different code. Four cases now run the real scripts against a fakeremote.sh: a connected team is started, a disconnected one is never offered to the command, the Monitor directive andstatus=okstill come out, and async startthat hangs does not hold either of them up.The helper, including the case this feature exists for: five callers race for the per-team lock — every one returns 0, exactly one reports starting, four 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.
Mutations
session-start's invocation removedactas-claim's invocation removedalready runningas a startstarted == 12>&1, losing the reason the command gaveAll restored; both trigger files verified free of mutation text before the commit.
What the first CI run caught, which local runs had not
Three defects reached the previous head. None was a flake, and two were mine to have found:
A new unenforceable assertion, in the same head that removed five.
tests/test_sync_autostart.batsused a non-terminal! grep -qfor "the disconnected team was never offered to the command". A leading!does not trip errexit on either interpreter, so it reportsokwhatever it finds.enforceable assertionscounted 615 against a baseline of 614 — exactly one. This harness already hasrefute; it is used now, and the checker is back at 614.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 the remedy is runnable. Two spaces again, with the reason written beside it.The "does not wait" cases leave a
sync startchild 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 operating systems failing the same shard numbers.teardownkills them now.And one measurement error of my own made the second reachable: I reported
test_deliveryas passing having looked attail -3. The last three lines beingokis not a suite passing. Every suite below is now read from its failure count.A test that passed under
--filterand failed in the full filetest_delivery.bats's session-start case relied on there being no engine to start — true when that file runs alone, and not when it runs with its neighbours, because 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.
The condition is now stated rather than inherited — an unusable interpreter makes
sync startfail immediately and for a named reason, which is what the warning under test is about. A--filterpass is not a suite pass, in either direction.The per-trigger mutation found two defects in my own tests
Worth reading, because both were invisible from a green run:
actas ... does not wait for a start that hangspassed with the invocation deleted. Nothing to wait for is also fast — the case could not tell a bound from an absence, so it was measuring the feature's absence and calling it a bound.Without the deletion mutation I would have written "four trigger cases added, the wiring is covered" — and one of the four was measuring its own absence.
Measurements, at this head
Every suite run in full and read from its failure count, not its tail:
The five non-terminal
[[ ... ]]this branch had added are gone, and so is the! grep -qthat replaced one of them. They cannot fail a test under macOS bash 3.2, which is what CI runs; they areprintf | grep -qandrefutenow, the two forms the checker measures as enforced on both interpreters.Drift, from the repo root immediately before the push, against
origin/integration/remote:Re-measured immediately before landing.
One thing found while building this
The first fake engine only slept, and the run hung:
cmd_sync_startdoes not return when the process exists — it waits for the engine'sstartup_nonceto reach the logfile. The tests use the same nonce-emitting fake node astest_remote_status_liveness.bats. Worth knowing before writing the next test that callssync start.Interaction with #773
An engine that exits on a server refusal will now be restarted on the next session, exit again, and be restarted again. #773 is now a blocker too and is designed against this one rather than after it: the moment this code attempts a start is the moment a recorded refusal can be surfaced, so the answer to "why isn't this syncing?" lands on the same surface an agent already reads.
CI on this head
enforceable assertionsis success. The one failure seen on the previous head wasbats (windows-latest, windows runtime (#567)), whose log istest_codex_monitor.batsteardown failing withrm: Directory not empty— a file this branch does not touch, and a known flake already filed as #662. It is not re-diagnosed here; what is owed is evidence that it is unrelated, which is a same-head re-run going green, and that is stated when it has been observed rather than assumed.A base comparison is not available for it:
origin/integration/remote's head carries a single check run, because the branch push does not run the pull-request matrix.#773 lands first, and that ordering is a condition rather than a preference. The asymmetry decides it:
A record with no reader is inactive. The other way round is an accident. So this PR waits, and the few lines that make the auto-start path read a recorded refusal — do not start a team the server has refused — land with #773 and are reviewed there.
This landing does not make the loop worse in itself: today such an engine is simply dead and stays dead until a person notices. What changes is that the loop becomes visible, which is the argument #773 already makes for not exiting on a refusal at all.