From 7e3e22138bb7110d9316dfe67fd2dadb6b84bc66 Mon Sep 17 00:00:00 2001 From: Anso Date: Sun, 17 May 2026 01:16:21 -0400 Subject: [PATCH] fix(mesh): name actual bridge kind in proxy-bridge rejection (#1075) * fix(mesh): distinguish proxy bridge from pilot tunnel in registerProxyBridge rejection When central's MeshProxyTunnelDialer dials a node whose slot is already held by a peer-initiated proxy bridge, the registration is correctly refused, but the activity-log message read "pilot tunnel already registered for node N; proxy bridge refused" regardless of which kind of bridge was actually in the slot. No pilot tunnel exists in proxy-mode remotes by definition, so the wording pointed operators at the wrong subsystem. Use the existing bridgeKinds index (already consulted correctly by the sister method replaceOrRegisterProxyBridge) to emit a kind-accurate message: "proxy bridge already registered for node N; concurrent dial refused" when the existing bridge is a peer-initiated proxy bridge, keeping the original "pilot tunnel" wording for the pilot-agent case so existing log filters and the sister test still match. Pure cosmetic; the rejection behavior is unchanged. New regression test asserts the discriminator in both directions. Also tightens the test-fixture beforeEach to clear bridgeKinds alongside bridges so future tests cannot read a stale kind. * chore: ignore local trailer directory in git repository * refactor(mesh): centralize bridge-kind type and conflict-message formatter Followup tidy on PilotTunnelManager after /simplify flagged two related items in the F-R-2 change: - Extract `BridgeKind = 'pilot' | 'proxy'` as a module-scoped type alias so the kind taxonomy lives in one place. `bridgeKinds` and `injectBridgeForTest` now consume it; the inline union literal is gone. - Extract `formatBridgeConflict(nodeId, existingKind)` as a private helper. Both `registerProxyBridge` and `replaceOrRegisterProxyBridge` call it from their "slot already held" rejection paths, removing the byte-identical "pilot tunnel already registered for node N; proxy bridge refused" string copy across the two sites. - Trim the 6-line WHAT comment above the rejection throw to a 2-line WHY so the call site stays scannable. No behavior change; the three relevant test suites stay green. --- .gitignore | 1 + .../pilot-tunnel-manager-ensure.test.ts | 41 +++++++++++++++- backend/src/services/PilotTunnelManager.ts | 47 +++++++++++++------ 3 files changed, 72 insertions(+), 17 deletions(-) diff --git a/.gitignore b/.gitignore index ed4dc788..167c8ff4 100644 --- a/.gitignore +++ b/.gitignore @@ -60,6 +60,7 @@ frontend/DESIGN.md # Local-only projects (separate repos) demo-video/ +trailer/ website/ # Internal engineering docs (not published) diff --git a/backend/src/__tests__/pilot-tunnel-manager-ensure.test.ts b/backend/src/__tests__/pilot-tunnel-manager-ensure.test.ts index 6091c907..b95f6a37 100644 --- a/backend/src/__tests__/pilot-tunnel-manager-ensure.test.ts +++ b/backend/src/__tests__/pilot-tunnel-manager-ensure.test.ts @@ -34,9 +34,13 @@ beforeEach(() => { DatabaseService.getInstance().getDb().prepare("DELETE FROM nodes WHERE name LIKE 'pilot-mgr-test-%'").run(); MeshProxyTunnelDialer.resetForTest(0); // idle-close disabled; clears any prior bridges // Drop any bridges held over from a prior test in the same file. + // Both maps must clear together: every production write path keeps + // `bridges` and `bridgeKinds` in lockstep, and a stale kind entry + // would mislead the kind-discriminator in registerProxyBridge. type BridgeMap = Map; - const bridges = (PilotTunnelManager.getInstance() as unknown as { bridges: BridgeMap }).bridges; - bridges.clear(); + const inst = PilotTunnelManager.getInstance() as unknown as { bridges: BridgeMap; bridgeKinds: BridgeMap }; + inst.bridges.clear(); + inst.bridgeKinds.clear(); }); function makeFakeBridge(nodeId: number): { bridge: import('../services/PilotTunnelBridge').PilotTunnelBridge; close: () => void } { @@ -91,6 +95,39 @@ describe('PilotTunnelManager.ensureBridge', () => { } }); + it('registerProxyBridge rejection names the existing bridge kind (proxy vs pilot)', () => { + const mgr = PilotTunnelManager.getInstance(); + + // Existing peer-initiated proxy bridge: rejection must say "proxy bridge", + // not "pilot tunnel". Locks in the F-R-2 discriminator. + const { bridge: existingProxy, close: closeProxy } = makeFakeBridge(404); + const { bridge: rejected, close: closeRejected } = makeFakeBridge(404); + try { + mgr.registerProxyBridge(404, existingProxy); + expect(() => mgr.registerProxyBridge(404, rejected)) + .toThrow(/proxy bridge already registered for node 404/); + expect(mgr.getBridge(404)).toBe(existingProxy); + } finally { + closeProxy(); + closeRejected(); + } + + // Existing pilot-agent tunnel: rejection keeps the original "pilot tunnel" + // wording so log filters and the sister regex in pilot-tunnel-manager-replace + // still match the pilot case. + const { bridge: pilot, close: closePilot } = makeFakeBridge(505); + const { bridge: rejectedProxy, close: closeRejectedProxy } = makeFakeBridge(505); + try { + mgr.injectBridgeForTest(505, pilot, 'pilot'); + expect(() => mgr.registerProxyBridge(505, rejectedProxy)) + .toThrow(/pilot tunnel already registered for node 505/); + expect(mgr.getBridge(505)).toBe(pilot); + } finally { + closePilot(); + closeRejectedProxy(); + } + }); + it('registerProxyBridge emits proxy-bridge-up and proxy-bridge-down on lifecycle', async () => { const mgr = PilotTunnelManager.getInstance(); const up: number[] = []; diff --git a/backend/src/services/PilotTunnelManager.ts b/backend/src/services/PilotTunnelManager.ts index 5ebce4e5..d2d2dab0 100644 --- a/backend/src/services/PilotTunnelManager.ts +++ b/backend/src/services/PilotTunnelManager.ts @@ -33,6 +33,17 @@ export class PilotTunnelCapacityError extends Error { } } +/** + * Discriminator for the two flavors of bridge that share the `bridges` + * map: `'pilot'` is set by `registerTunnel` (agent-initiated long-lived + * tunnel); `'proxy'` is set by `registerProxyBridge` / + * `replaceOrRegisterProxyBridge` (central- or peer-initiated short-lived + * proxy bridge). Used by the rejection-message formatter and by + * `replaceOrRegisterProxyBridge` so a peer-initiated dial can supersede a + * previous proxy bridge but never shadow a live pilot tunnel. + */ +export type BridgeKind = 'pilot' | 'proxy'; + /** * PilotTunnelManager: singleton registry of active mesh-capable bridges. * @@ -68,15 +79,7 @@ export class PilotTunnelCapacityError extends Error { export class PilotTunnelManager extends EventEmitter { private static instance: PilotTunnelManager; private bridges: Map = new Map(); - /** - * Parallel kind index for the `bridges` map. `'pilot'` is set by - * `registerTunnel` (agent-initiated long-lived tunnel); `'proxy'` is - * set by `registerProxyBridge` and `replaceOrRegisterProxyBridge` - * (central-initiated short-lived bridge). Used by - * `replaceOrRegisterProxyBridge` so a peer-initiated dial can supersede - * a previous proxy bridge but never shadow a live pilot tunnel. - */ - private bridgeKinds: Map = new Map(); + private bridgeKinds: Map = new Map(); private softWarned = false; private constructor() { @@ -111,7 +114,7 @@ export class PilotTunnelManager extends EventEmitter { * Bypasses capacity / lifecycle hooks so unit tests can prime the * registry without owning a real WebSocket. */ - public injectBridgeForTest(nodeId: number, bridge: PilotTunnelBridge, kind: 'pilot' | 'proxy'): void { + public injectBridgeForTest(nodeId: number, bridge: PilotTunnelBridge, kind: BridgeKind): void { this.bridges.set(nodeId, bridge); this.bridgeKinds.set(nodeId, kind); } @@ -258,10 +261,9 @@ export class PilotTunnelManager extends EventEmitter { public registerProxyBridge(nodeId: number, bridge: PilotTunnelBridge): void { const existing = this.bridges.get(nodeId); if (existing) { - // A pilot tunnel for this node already exists. Proxy bridges - // should not silently shadow them; refuse the registration so - // the dialer can surface a clear error. - throw new Error(`pilot tunnel already registered for node ${nodeId}; proxy bridge refused`); + // Kind-accurate rejection so the dialer logs the actual conflict + // instead of always blaming a pilot tunnel. + throw new Error(this.formatBridgeConflict(nodeId, this.bridgeKinds.get(nodeId))); } if (this.bridges.size >= PILOT_TUNNEL_HARD_LIMIT) { PilotMetrics.increment('tunnels_rejected_capacity'); @@ -289,7 +291,7 @@ export class PilotTunnelManager extends EventEmitter { public replaceOrRegisterProxyBridge(nodeId: number, bridge: PilotTunnelBridge): void { const existingKind = this.bridgeKinds.get(nodeId); if (existingKind === 'pilot') { - throw new Error(`pilot tunnel already registered for node ${nodeId}; proxy bridge refused`); + throw new Error(this.formatBridgeConflict(nodeId, existingKind)); } if (existingKind === 'proxy') { const old = this.bridges.get(nodeId); @@ -312,6 +314,21 @@ export class PilotTunnelManager extends EventEmitter { this.bridgeKinds.set(nodeId, 'proxy'); } + /** + * Single source of truth for the "slot already held" rejection + * message thrown by `registerProxyBridge` and + * `replaceOrRegisterProxyBridge`. Picks the bridge-kind subject and + * the rejection tail so a stale call site cannot drift from the + * other. `undefined` collapses to the pilot branch defensively; + * production code always sets `bridgeKinds` whenever `bridges` is + * set, so this is unreachable in practice. + */ + private formatBridgeConflict(nodeId: number, existingKind: BridgeKind | undefined): string { + return existingKind === 'proxy' + ? `proxy bridge already registered for node ${nodeId}; concurrent dial refused` + : `pilot tunnel already registered for node ${nodeId}; proxy bridge refused`; + } + /** * Force-close a tunnel (e.g., on node deletion). */