Skip to content

fix(broker): self-heal route registration - #85

Open
zikolach wants to merge 5 commits into
mainfrom
feature/self-heal-broker-route-registration
Open

fix(broker): self-heal route registration#85
zikolach wants to merge 5 commits into
mainfrom
feature/self-heal-broker-route-registration

Conversation

@zikolach

Copy link
Copy Markdown
Owner

Summary

  • make live broker route registration self-healing and observable
  • close the supervisor cleanup race that can split clients across replacement brokers
  • guarantee autonomous multi-client reconnect and restart convergence
  • distinguish local binding state from broker transport and route synchronization health

OpenSpec

  • self-heal-broker-route-registration
  • 0/45 implementation tasks complete at draft creation
  • strict validation passes

Incident

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

@zikolach
zikolach marked this pull request as ready for review July 27, 2026 16:48
Copilot AI review requested due to automatic review settings July 27, 2026 16:48

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread extensions/relay/broker/reconciliation.ts
Comment thread extensions/relay/broker/reconciliation.ts
Comment thread extensions/relay/broker/protocol.ts
Copilot AI review requested due to automatic review settings July 27, 2026 17:01

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.

Comment thread extensions/relay/broker/tunnel-runtime.ts
Comment thread extensions/relay/broker/tunnel-runtime.ts
Comment thread extensions/relay/broker/process.js
Copilot AI review requested due to automatic review settings July 27, 2026 23:09

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/parsing state.json again. In the registerRoute handler we load state and then call stripRevokedBindingFromRoute(...) 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() calls request(), which calls ensureConnected(), which itself always runs resyncRoutes(). On a cold start (no socket yet), this can trigger reconciliation twice (once inside ensureConnected(), then again here). Also, if the broker doesn't support reconcileRoutes, 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,

Comment thread extensions/relay/broker/tunnel-runtime.ts
Copilot AI review requested due to automatic review settings July 28, 2026 06:53

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 the reconcileRoutes action, but it does not persist that capability result. Because this method currently uses request() (which destroys/reconnects the socket on any ok:false response), a legacy broker can trigger unnecessary disconnect/reconnect churn and repeated unsupported probes. Cache the unsupported result (reconciliationSupported = false) and avoid using request() 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants