Repository navigation
feat(connections): promote a new homeserver to primary while the old bridge runs - #5966
Conversation
…bridge runs Adding a homeserver and making it primary failed on a terminal-run core: set_primary refuses while the legacy bridge holds its lease, and disconnect refuses the legacy row. promote(instance) does the whole move in one operation: it starts the new row's bridge, stops the old owner's bridge through the existing stop path, marks the old row parked and flips the primary under one hold of the record lock. A refused stop or a failed start rolls back; a repeat is a no-op. A parked row stays in the record. The plan skips it; for the legacy row the engine owns the flag, so REMOTE_TASK_TOKEN can stay set: the launchd wrapper waits instead of launching (and keeps the agent-bridge supervisor up), the bare launch skips it, and the bridge stands down (rc 75) when it finds its row parked right after taking its lease. unpark clears the flag. Surfaces: connections.promote / unpark / legacy_parked in the module; promote, unpark and legacy-parked in the CLI; connection.promote and connection.unpark in the runtime API; POST <instance>/promote and <instance>/unpark over HTTP. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Coverage Gate✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 94%. Diff CoverageDiff: origin/main...HEAD, staged and unstaged changes
Summary
src/connections.pyLines 794-802 794 if not isinstance(removed, list) or not all(isinstance(x, str) for x in removed):
795 raise SchemaError(f"connections.json: '{REMOVED_KEY}' is not a list of names")
796 parked = payload.get(LEGACY_PARKED_KEY, False)
797 if not isinstance(parked, bool):
! 798 raise SchemaError(f"connections.json: '{LEGACY_PARKED_KEY}' is not a boolean")
799 return {"rows": rows, REMOVED_KEY: sorted(set(removed)), LEGACY_PARKED_KEY: parked}
800
801
802 def _read_record(path: Path) -> dict:Lines 1271-1279 1271 with locked_file(_lock_path(workspace), create_mode=0o600):
1272 rec = _read_record(path)
1273 row = _find(rec["rows"], "")
1274 if row.get("disabled"):
! 1275 raise DisabledCannotBePrimary("")
1276 if not bridge_pid("", workspace):
1277 raise LegacyBridgeNotRunning()
1278 was_parked, was_primary = bool(row.get("parked")), row["primary"]
1279 old = _owner_row(rec["rows"], workspace)Lines 1304-1312 1304 return
1305 if derive_state(sidecar, {})[1] == "auth_rejected":
1306 raise NewPrimaryNotConnected(instance, "auth_rejected")
1307 if not pid or _exited(pid):
! 1308 raise NewPrimaryNotConnected(instance, "exited")
1309 if time.monotonic() >= deadline:
1310 raise NewPrimaryNotConnected(instance, "timeout")
1311 time.sleep(0.2) |
…as no refusal branch Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
sonichi
left a comment
There was a problem hiding this comment.
Changes needed at 8b487118 (agent-assisted, Blue for Chi's seat). The locking and parking are solid. One blocker: after a promote, no bridge carries the proactive room.
Blocker: the new primary's bridge starts without REMOTE_PROACTIVE_ROOM.
promotecallsstart(instance, …)(src/connections.py:1162) while the row is still a secondary.- The primary flip happens only later, under the lock (
:1184-1185). launch_envsets the room only whenentry.get("primary")(:1560), so the new bridge launches with an empty room. Then the old bridge, the only one that had the room, is stopped and parked.- Unlike
set_primary, the result carries norelaunch_needed, so no caller knows to fix it. That breaks the "exactly one bridge carries the room" invariant indocs/connections.md. - Proactive delivery stops until the next full restart.
- Promoting back to the legacy row has the same issue. That bridge was launched as a secondary, with its room stripped by the wrapper, and promote skips the
LegacyPrimaryRunningguard thatset_primaryuses for exactly this case. - Evidence: a background reviewer ran a probe at this head. With
REMOTE_PROACTIVE_ROOMinjected, promote returnedprimary=True,relaunch_needed=None, and the spawned bridge's room was''. I confirmed the ordering by reading the code; my own re-run of the probe couldn't import the test helpers. - Fix: relaunch the new primary's bridge after the flip (and the legacy row's when promoting back to it), or at minimum return
relaunch_needed. Add a test that asserts the room on the spawned environment.
Should-fix:
- "Started" isn't "connected".
launch.okwithlease == "held"(:1160-1167) means the new bridge took its local lease, not that the gateway accepted its token. If the new token then gets a 401, the old bridge is already stopped and parked, and the owner has no working primary. Before stopping the old bridge, wait with a deadline for the new bridge's status to show connected, and roll back otherwise. - A legacy bridge can survive a promote. When a named row already covers the same server,
import_from_env(:1342-1346) gives the env token no row._owner_row(:1127) then can't see a running legacy bridge, so promote neither stops nor parks it. If it kept the room at startup, two bridges send nudges.
Nit: the wrapper's post-exit legacy-parked → continue check (gateway-bridge-wrapper.sh:217) is untested. Deleting it still passes gateway-bridge-wrapper-self-heal.test.sh.
What holds (verified by reading):
- One lock hold: stop, park and flip happen under a single hold of the record lock, and
legacy_parkedtakes the same lock. - The old bridge really stops: it goes through
_stop_rowand covers alias names. - Parked rows stay down: they are skipped everywhere a bridge could start, including
launch_plan,plan_entry,env_only(via_refuse_down),start_added,startup-runtime.sh:540, the wrapper and the bridge's stand-down check. - Edge cases: a racing disconnect is refused and rolled back, a repeated promote is a no-op, and a disabled row is refused.
- HTTP: a
Refusalmaps to 409, and the new two-segment POST doesn't collide with existing routes. - Earlier risk areas: none of the five from the prior connections review are reintroduced.
Tests run by the reviewer at this head:
connections.test.py231 OK;runtime-api-connections-view.test.py22 OK;agent-api-connections.test.pyPASSconnections-cli.test.py17 OK;remote-gateway-singleton.test.py24 OKgateway-bridge-wrapper-self-heal.test.shPASS;startup-runtime-connections-launch-plan.test.sh27/0- Mutation: replacing
_stop_rowwithstopped = 0fails 8Promotetests.
Code-owner note: this touches src/runtime-api/, which only qingyun-wu and keweichen own, so it needs one of them in addition.
…or connected, parks a rowless legacy bridge
- Flip the primary first, then stop any running secondary bridge and start it, so launch_env
hands it REMOTE_PROACTIVE_ROOM; relaunch_needed is returned on every surface.
- Stop the old owner only after the new bridge's own status shows a gateway poll
(CONNECT_TIMEOUT_S); a timeout, 401 or exit is new_primary_not_connected and rolls back.
- promote('') reports relaunch_needed when the legacy bridge settled without the room.
- A legacy bridge with no row (a named row covers its server) is stopped and recorded as
legacy_parked; unpark '' clears it.
- HTTP: the fixed segment `legacy` addresses the legacy row; status_for carries `legacy`.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
sonichi
left a comment
There was a problem hiding this comment.
Approve at 5b7fe5a9 (agent-assisted, Blue for Chi's seat). All three findings are fixed, and I checked each one by running the tests.
- Blocker, fixed.
- The flip (
r["primary"] = r is row) now happens in the first transaction, beforestart(), solaunch_envgives the new bridgeREMOTE_PROACTIVE_ROOM. - A running secondary is stopped and relaunched as primary.
relaunch_neededis now reported for thealready_runningandalias_runningcases, and for promoting back to legacy.- Mutation check: I removed that early flip and re-ran
connections.test.py. Exactly the two new ordering tests fail (…flips_before_the_new_bridge_launches…and…running_secondary_is_relaunched…).
- The flip (
- (a), fixed.
_await_connectedwaits up to 60 s for this pid's own sidecar to reportconnected.- It fails immediately on
auth_rejected, and on exit or timeout it raisesNewPrimaryNotConnected(409). _undo_promoteand_restart_secondaryrestore the old primary, the parked flag and a secondary that was stopped for the relaunch.- The old bridge is stopped only after this, under the lock.
- (b), fixed.
- When no legacy row exists, the legacy bridge is stopped and the record-level
legacy_parkedis set. - The covered-legacy case stays up when the new bridge never connects, and that is tested.
- When no legacy row exists, the legacy bridge is stopped and the record-level
- Ran at this head:
connections.test.py239 OK;runtime-api-connections-view.test.py22 OK;agent-api-connections.test.pyPASSconnections-cli.test.py17 OK;remote-gateway-singleton.test.py24 OKgateway-bridge-wrapper-self-heal.test.shPASS;startup-runtime-connections-launch-plan.test.sh27/0
Note, not blocking: for the up-to-60 s connect wait, both bridges carry the room. The old one still has it from its own launch, and the new one has it as primary. If proactive delivery goes through the outbox claim, a result is still delivered once. If anything posts nudges outside the outbox, it could double for that window. Worth one line in docs/connections.md, next to "exactly one bridge carries the room", so the transition isn't read as a violation.
Code owner: this still touches src/runtime-api/, so it needs qingyun-wu or keweichen as well.
…wait (promote_conflict) The second lock hold now checks the row is still the primary and the row to park is still there and unparked. Otherwise it stops the bridge this call started, restores the row's parked flag, leaves the other writer's primary in place, and raises promote_conflict (409). Docs: during the connect wait both bridges hold the proactive room in their environment. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@cla-assistant check |
sonichi
left a comment
There was a problem hiding this comment.
Re-approve at 559789d7 (agent-assisted, Blue for Chi's seat).
- Conflict guard. The second lock hold now raises
PromoteConflict(aRefusal, so 409promote_conflict) in three cases:- the row is gone or no longer primary;
- the row to park was removed;
- the row to park was already parked by another writer.
- Rollback on conflict.
_undo_promote(..., restore_primary=False)stops only the bridge this call started and leaves the other writer's primary standing._restart_secondarythen brings back a secondary this call had stopped for the relaunch. Every other failure still restoresbefore. - Docs.
docs/connections.mdnow states that both bridges holdREMOTE_PROACTIVE_ROOMduring the connect wait (up to 60 s), and that only the oneowns_unaddressednames posts there. That answers my note: the env overlap doesn't mean double posting. - Ran at this head:
connections.test.py: 241 OK, including the two new conflict tests (a primary flip, and a third-party park during the wait).runtime-api-connections-view.test.py: 22 OK.agent-api-connections.test.py: PASS.connections-cli.test.py: 17 OK.
Code owner for src/runtime-api/: qingyun-wu.
What changed, and why
On a terminal-run core, the legacy unsuffixed ag2.space bridge is usually the primary and is running. So "add s.space, then make it primary" fails today:
set_primaryis refused withlegacy_primary_runningwhile that bridge holds its lease.disconnectrefuses the legacy row withlegacy_row_unmanaged.The only way through was to stop everything by hand, promote, and restart.
This PR adds one engine operation,
promote(instance), that moves the primary while the old primary's bridge is still running:parkedflag and makes it the only primary first, solaunch_envgives its bridgeREMOTE_PROACTIVE_ROOM.started. A failure raisesnew_primary_not_started.CONNECT_TIMEOUT_S(60 s, past the bridge's first long poll) for the new bridge's own status file, written by that pid, to showconnected: true, meaning the gateway accepted its token. A 401, an exit or the deadline raisesnew_primary_not_connected. Steps 2 and 3 roll back on failure (primary andparkedflags restored, the started bridge stopped, a stopped secondary started again); the old bridge is never touched.parked. A legacy bridge with no row of its own (a named row covers its server, soimport-envgave it none) is stopped too and recorded as a record-levellegacy_parked. First it checks that the row is still the primary and that the row to park is still there and unparked. If another writer changed either during the wait, it raisespromote_conflict(409): the bridge this call started is stopped, the row'sparkedflag is restored, and the other writer's primary stands.promote(''), back to the legacy row, needs that bridge running and runs step 4 only. That bridge settled the proactive room when it started, so when a named row was primary the answer carriesrelaunch_needed: true(restart it withbash src/restart.sh).Ownership stays single throughout. Delivery is claim-based, and since #5960 every bridge reads
owns_unaddressedfrom the record on each pass. So before the flip only the old bridge owns unaddressed messages and cards, after it only the new one does, and until the new bridge polls nobody does. Messages wait during that window; none are lost.A refused stop (
bridge_still_runningorbridge_lease_indeterminate) rolls back the same way: the primary flips back, a bridge this call started is stopped, and aparkedflag it cleared is restored. Promoting the current primary is a no-op (changed: false), so retrying after any failure is safe.Parked, not deleted. The row, its vault key and its launch record all stay. For the legacy row the engine owns the flag, so
REMOTE_TASK_TOKENcan stay set:legacy-parkedbefore each launch; while the row is parked it launches nothing and raises no owner alert, but keeps the agent-bridge supervisor running, and it resumes by itself after un-park;startup-runtime.shskips the bridge with a ⊘ line;connections.legacy_parkedunder the record lock right after it takes its lease (a new host-injectedSTAND_DOWN_GATE) and exits 75 when parked, so a launch that raced the promote never polls.Named parked rows are skipped by
launch_plan,plan_entryandstart_added.unpark(instance)clears the flag and starts a named row's bridge as a secondary. The legacy row's launchers resume it.set_primaryon a parked row is refused withparked_cannot_be_primary;reconnectalso clears the flag.API and CLI shape
src/connections.py):promote(instance, *, repo, logs_dir, environ, workspace, vault, spawn, kill, python)returns the row plusprevious(the parked instance,""for legacy,Noneif nothing was parked),stopped_pid,legacy_stopped_pid,launch,changedandrelaunch_needed.unpark(instance, *, repo, logs_dir, ...)returns the row pluslaunch.legacy_parked(workspace)returns a bool.legacy_bridge_not_running,new_primary_not_started,new_primary_not_connected,promote_conflict,parked_cannot_be_primary.scripts/connections.py):promote INSTANCE --repo DIR --logs-dir DIRunpark INSTANCE --repo DIR --logs-dir DIR(''names the legacy row)legacy-parked(exit 0 when parked, else 1)connection.promote {instance}returns{connection, previous, stopped_pid, launch, changed, relaunch_needed}.connection.unpark {instance}returns{connection, launch}.POST /api/v1/connections/<instance>/promoteandPOST /api/v1/connections/<instance>/unpark, behind the same local API gate. Refusals are 409 with the code. The fixed segmentlegacyaddresses the legacy row (instance"", which an empty path segment cannot carry), e.g.POST /api/v1/connections/legacy/unpark;legacyis reserved as a slug.parkedboolean.status_forspells it out, and the derived state isparkedwith reasonparked.status_foralso carrieslegacy: truefor the legacy row.legacy_parkedboolean for a rowless legacy bridge.Where to look first
src/connections.py:promote,_promote_legacy,_await_connected,_undo_promote,_owner_row,unpark,legacy_parked, and theparkedhandling inlaunch_plan/plan_entry/derive_state.src/launchd/gateway-bridge-wrapper.sh(the parked wait loop) andsrc/startup-runtime.sh(the bare-launch skip).packages/ag2-sparrow/ag2_sparrow/remote_gateway_bridge.py: theSTAND_DOWN_GATEhook, called after the lease heartbeat starts and before the proactive room is settled.src/remote-gateway-bridge.pyinjects it for the legacy, non-seat bridge only.Behavior this proves (before/after)
Scripted repro, run in a temp workspace: the legacy row is primary and its bridge holds a real lease (a production-writer lease child), and s.space is added.
At the parent commit (e50b019):
At this head:
Tests
All of these were run locally and pass:
python3 tests/connections.test.py: 241 tests OK. The newPromoteclass covers:REMOTE_PROACTIVE_ROOMin its env;relaunch_neededon a promote back to the legacy row;promote_conflict, with that move standing and nothing stopped or parked, and a parked target keeping its flag on a conflict;unpark ''clearing it;plan_entry(including env-only) andstart_addedskipping parked rows, and a legacy bridge that takes its lease later readinglegacy_parkedunder the lock;set_primaryrefusing a parked row;python3 tests/remote-gateway-singleton.test.py: 24 OK. The bridge stands down with exit 75 when the gate gives a reason, after the heartbeat and before the room settles; it keeps running when the gate says nothing or raises.bash tests/gateway-bridge-wrapper-self-heal.test.sh: PASS. With the row parked, the wrapper stays alive, launches nothing, keeps the agent-bridge supervisor up and writes no alert, and it launches the bridge after un-park.bash tests/startup-runtime-connections-launch-plan.test.sh: 27 passed. A parked legacy row is not launched and the skip is reported; named lanes still launch.bash tests/gateway-bridge-wrapper-primary-room.test.sh: all ok.python3 tests/runtime-api-connections-view.test.py: 22 OK.connection.promoteoff a running legacy bridge, thenconnection.unpark "", thenlegacy_bridge_not_runningas CONFLICT.python3 tests/agent-api-connections.test.py: all ok. The new routes need the bearer; promote, unpark, promote back and the no-op repeat over real HTTP, andPOST /connections/legacy/unparkreaching instance"".python3 tests/connections-cli.test.py: 17 OK.ruff checkon the changed Python files, andbash scripts/review-checks.sh --diff <(git diff origin/main...HEAD)(PASS).Edge cases checked
bridge_still_running, and the promote rolls back.Docs
docs/connections.mdis updated: theparkedfield, the refusal table (three new codes;promoteadded todisabled_cannot_be_primary,bridge_still_runningandbridge_lease_indeterminate), a new "Promote: one operation, old primary running" section, the state table, and the CLI, runtime API and HTTP surface lists.docs/catalog.jsonis already verified for today.Risks and gaps
scripts/connections.py promote <inst> --repo . --logs-dir <logs>, send a DM on the new homeserver and confirm the reply, then rununpark ''.REMOTE_PROACTIVE_ROOMin their environment. Only the oneowns_unaddressednames posts there.relaunch_neededinstead.🤖 Generated with Claude Code