fix(broker): self-heal route registration - #85
Conversation
There was a problem hiding this comment.
Pull request overview
Implements the first slice of the “self-heal broker route registration” OpenSpec change by introducing a broker reconciliation protocol contract, normalization helpers, and synchronization-state tracking, plus corresponding OpenSpec docs and unit tests.
Changes:
- Add broker route reconciliation request/response types, limits, and client synchronization-state modeling.
- Add normalization/validation helpers for reconciliation snapshots and helpers to derive synchronization state after connect/disconnect/response.
- Add OpenSpec change proposal/design/specs/tasks and unit tests covering validation and basic synchronization behavior.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/relay/broker-reconciliation.test.ts | Adds unit tests for reconciliation request validation and synchronization-state helpers. |
| openspec/changes/self-heal-broker-route-registration/tasks.md | Tracks implementation tasks for the new reconciliation/restart convergence workstream. |
| openspec/changes/self-heal-broker-route-registration/specs/relay-runtime-status-line/spec.md | Specifies status-line requirements for distinguishing binding vs transport vs route sync health. |
| openspec/changes/self-heal-broker-route-registration/specs/relay-communication-diagnostics/spec.md | Specifies secret-safe, bounded diagnostics for reconciliation and restart convergence. |
| openspec/changes/self-heal-broker-route-registration/specs/relay-broker-topology/spec.md | Specifies autonomous reconciliation and restart convergence/ownership-safety requirements. |
| openspec/changes/self-heal-broker-route-registration/proposal.md | Documents motivation, intended behavior, and impact scope for the change. |
| openspec/changes/self-heal-broker-route-registration/design.md | Captures lifecycle design decisions, safety constraints, and planned testing approach. |
| openspec/changes/self-heal-broker-route-registration/.openspec.yaml | Declares the OpenSpec change metadata. |
| extensions/relay/broker/reconciliation.ts | Implements normalization/validation helpers and client sync-state transitions. |
| extensions/relay/broker/protocol.ts | Adds reconciliation protocol types, limits, and synchronization-health types. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (3)
extensions/relay/broker/tunnel-runtime.ts:506
applyReconciliationResponse()currently throws on any non-legacy but unrecognized response payload. A malformed/forward-incompatible broker reply should not crash the live runtime; instead treat it as "unsupported" (fall back to legacy) or mark synchronization as unknown/reconnecting.
const response = normalizeBrokerReconciliationResponse(value);
if (!response) {
if (value === undefined || value === true) return false;
throw new Error("Invalid broker route reconciliation response.");
}
extensions/relay/broker/process.js:2719
stripRevokedBindingFromRoute()can now accept the already-loaded state to avoid reading/parsingstate.jsonagain. In theregisterRoutehandler we load state and then callstripRevokedBindingFromRoute(...)without passing it, which introduces an extra file read per registration.
const state = await loadState();
const route = await stripRevokedBindingFromRoute(routeWithPersistedTelegramBinding(message.route, state));
extensions/relay/broker/tunnel-runtime.ts:495
reconcileRoutes()callsrequest(), which callsensureConnected(), which itself always runsresyncRoutes(). On a cold start (no socket yet), this can trigger reconciliation twice (once insideensureConnected(), then again here). Also, if the broker doesn't supportreconcileRoutes, this path should fall back to legacy registration rather than bubbling the request error.
private async reconcileRoutes(): Promise<void> {
this.synchronizationState = synchronizationStateAfterConnect([...this.routes.keys()], this.synchronizationState);
const result = await this.request("reconcileRoutes", {
clientId: this.clientId,
observedBrokerEpoch: this.synchronizationState.brokerEpoch,
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
extensions/relay/broker/tunnel-runtime.ts:520
reconcileRoutes()falls back to legacy registration when the broker rejects thereconcileRoutesaction, but it does not persist that capability result. Because this method currently usesrequest()(which destroys/reconnects the socket on anyok:falseresponse), a legacy broker can trigger unnecessary disconnect/reconnect churn and repeated unsupported probes. Cache the unsupported result (reconciliationSupported = false) and avoid usingrequest()for the reconciliation probe so protocol-level rejections don’t force reconnection.
private async reconcileRoutes(): Promise<void> {
if (this.reconciliationSupported === false) {
await this.reconcileRoutesLegacy();
return;
}
this.synchronizationState = synchronizationStateAfterConnect([...this.routes.keys()], this.synchronizationState);
let result: unknown;
try {
result = await this.request("reconcileRoutes", {
clientId: this.clientId,
observedBrokerEpoch: this.synchronizationState.brokerEpoch,
routes: [...this.routes.values()].map((route) => this.serializeRoute(route)),
});
} catch (error) {
if (!isUnsupportedReconciliationAction(error)) throw error;
await this.reconcileRoutesLegacy();
return;
}
if (!this.applyReconciliationResponse(result)) await this.reconcileRoutesLegacy();
openspec/changes/self-heal-broker-route-registration/tasks.md:37
- The PR description summary claims the supervisor cleanup race is closed and autonomous multi-client restart convergence is guaranteed, but this PR’s implementation and OpenSpec task list still show restart convergence work as pending (e.g., section 4.* and 7.* are unchecked). Please either scope the PR description to the reconciliation protocol/client changes actually included here or include the restart/cleanup fixes in this PR so the summary matches what ships.
## 4. Shared Broker Restart Convergence
- [ ] 4.1 Add an intentional-restart preparation request that captures bounded expected client/route counts and notifies connected clients without persisting live descriptors.
- [ ] 4.2 Make notified clients retain live route maps, clear synchronization, and enter autonomous reconnect before the old socket closes.
- [ ] 4.3 Move broker kill, ownership verification, control-file cleanup, and replacement startup under broker-scope supervisor serialization.
- [ ] 4.4 Make pid/socket cleanup compare the selected old PID/epoch with current authoritative control state and join any replacement already started by another client.
- [ ] 4.5 Prevent a client connected to a superseded broker from killing the different live broker named by authoritative scope state.
- [ ] 4.6 Reconcile the initiating client's routes and poll bounded broker health for expected client/route convergence.
- [ ] 4.7 Return explicit complete, partial-timeout, in-progress, and failed restart outcomes while keeping recovered routes available.
- [ ] 4.8 Update `/relay restart` copy to explain shared-machine impact and report secret-safe convergence counts and repair guidance.
- [ ] 4.9 Add multi-client process tests for complete recovery, reconnect-during-cleanup, delayed/exited clients, concurrent restart callers, no orphan brokers, and no-manual-reload behavior.
Summary
OpenSpec
self-heal-broker-route-registrationIncident
A live Sigma Pi session retained an active binding and broker socket while Telegram showed its route offline. Investigation also found that restart cleanup can race another client's replacement startup and delete the replacement broker's shared control files.
Validation
openspec validate self-heal-broker-route-registration --strict