From 5aff3bde704cfc179504b3a0d5d18c29d88beb18 Mon Sep 17 00:00:00 2001 From: Joseph Mearman Date: Wed, 16 Sep 2026 05:59:33 +0100 Subject: [PATCH] fix(core): actually re-advertise on discovery visibility resume pauseAllAdvertisements/pauseAdvertisementsForBackend discarded the real AdvertiseOptions a caller passed to advertise(), replacing them with an inert placeholder. resumeAllAdvertisements/resumeAdvertisementsForBackend then only deleted the paused bookkeeping entry and never called backend.startAdvertising again, so toggling mesh visibility from quiet/dark back to discoverable silently left the mesh unadvertised on every backend, with getVisibility() reporting "discoverable" the whole time. DiscoveryManager now keeps the real opts alongside each active and paused advertisement, and resume genuinely calls backend.startAdvertising(opts) again. The caller-visible advertisement id is now minted by DiscoveryManager itself (via nanoid, distinct from whatever id a backend returns internally) and kept stable across a pause/resume cycle, so a caller holding an id from before a visibility change can still address the same logical advertisement afterwards -- a backend is free to return a different internal id on every startAdvertising call, and DiscoveryManager no longer depends on backend id determinism to keep working correctly. --- src/core/discovery.ts | 82 +++++++++++++-------- src/test/visibility.integration.test.ts | 95 ++++++++++++++++++++++++- 2 files changed, 147 insertions(+), 30 deletions(-) diff --git a/src/core/discovery.ts b/src/core/discovery.ts index b57763b6..15ab7502 100644 --- a/src/core/discovery.ts +++ b/src/core/discovery.ts @@ -8,6 +8,7 @@ */ import type { MeshVisibility } from "./types.js"; +import { nanoid } from "./nanoid.js"; // --------------------------------------------------------------------------- // Discovered mesh @@ -58,12 +59,23 @@ export interface DiscoveryBackend { // Discovery manager // --------------------------------------------------------------------------- +/** An advertisement DiscoveryManager currently considers live -- the caller-visible external id, which backend to it, the backend's own internal id (needed to address stopAdvertising/a future re-advertisement), and the original opts (needed to genuinely re-advertise on resume, not just forget the advertisement ever happened). */ +interface ActiveAdvertisement { + backendName: string; + backendId: string; + opts: AdvertiseOptions; +} + export class DiscoveryManager { private readonly backends = new Map(); - private readonly activeAdvertisements = new Map(); + /** Keyed by the stable, caller-visible external id DiscoveryManager itself mints -- deliberately never the backend's own returned id, since a backend is free to return a different id on every startAdvertising call (a real, un-mocked backend calling it deterministically is incidental, not a contract this class may rely on) and a caller must be able to keep using the same id it was given across a pause/resume cycle. */ + private readonly activeAdvertisements = new Map< + string, + ActiveAdvertisement + >(); private meshVisibility: MeshVisibility = "discoverable"; private readonly perAdapterVisibility = new Map(); - /** Advertisements that were paused due to visibility changes. */ + /** Advertisements paused due to a visibility change, keyed by the same external id -- carries the real original opts (not a placeholder) so resuming can genuinely call startAdvertising again. */ private readonly pausedAdvertisements = new Map< string, { backendName: string; opts: AdvertiseOptions } @@ -74,7 +86,7 @@ export class DiscoveryManager { this.backends.set(backend.name, backend); } - /** Start advertising on a specific backend. Returns an advertisement ID. */ + /** Start advertising on a specific backend. Returns a stable external advertisement id, distinct from whatever id the backend itself returns internally. */ async advertise( backendName: string, opts: Readonly, @@ -85,8 +97,9 @@ export class DiscoveryManager { `Unknown discovery backend: "${backendName}". Available: ${[...this.backends.keys()].join(", ")}`, ); } - const id = await backend.startAdvertising(opts); - this.activeAdvertisements.set(id, backendName); + const backendId = await backend.startAdvertising(opts); + const id = nanoid(); + this.activeAdvertisements.set(id, { backendName, backendId, opts }); return id; } @@ -154,16 +167,15 @@ export class DiscoveryManager { } private async pauseAllAdvertisements(): Promise { - // Capture current ads before clearing - for (const [id, backendName] of this.activeAdvertisements) { - // We've lost the original opts — but we can just stop the ad + // Capture current ads' real opts before clearing, so resume can genuinely re-advertise rather than merely forgetting the pause happened. + for (const [id, active] of this.activeAdvertisements) { this.pausedAdvertisements.set(id, { - backendName, - opts: { name: "resumed", port: 0 }, + backendName: active.backendName, + opts: active.opts, }); - const backend = this.backends.get(backendName); + const backend = this.backends.get(active.backendName); if (backend) { - await backend.stopAdvertising(id).catch(() => { + await backend.stopAdvertising(active.backendId).catch(() => { /* intentionally empty — best-effort stop */ }); } @@ -174,15 +186,15 @@ export class DiscoveryManager { private async pauseAdvertisementsForBackend( backendName: string, ): Promise { - for (const [id, bn] of this.activeAdvertisements) { - if (bn === backendName) { + for (const [id, active] of this.activeAdvertisements) { + if (active.backendName === backendName) { this.pausedAdvertisements.set(id, { backendName, - opts: { name: "resumed", port: 0 }, + opts: active.opts, }); const backend = this.backends.get(backendName); if (backend) { - await backend.stopAdvertising(id).catch(() => { + await backend.stopAdvertising(active.backendId).catch(() => { /* intentionally empty — best-effort stop */ }); } @@ -200,32 +212,44 @@ export class DiscoveryManager { } private async resumeAllAdvertisements(): Promise { - // Restart backends first (they may have been stopped in "dark" mode) Note: backends reinitialise their sockets on next startAdvertising/discover call. - for (const [id] of this.pausedAdvertisements) { + // Backends reinitialise their own sockets/timers on the next startAdvertising call -- nothing extra needed here beyond actually calling it, which is the whole fix: resuming used to just forget the pause happened rather than genuinely re-advertising. + for (const [id, entry] of this.pausedAdvertisements) { this.pausedAdvertisements.delete(id); - // We can't fully resume without original opts — the caller must re-advertise. Mark as not paused so new advertise calls work. + const backend = this.backends.get(entry.backendName); + if (!backend) continue; + const backendId = await backend.startAdvertising(entry.opts); + this.activeAdvertisements.set(id, { + backendName: entry.backendName, + backendId, + opts: entry.opts, + }); } - return Promise.resolve(); } private async resumeAdvertisementsForBackend( backendName: string, ): Promise { for (const [id, entry] of this.pausedAdvertisements) { - if (entry.backendName === backendName) { - this.pausedAdvertisements.delete(id); - } + if (entry.backendName !== backendName) continue; + this.pausedAdvertisements.delete(id); + const backend = this.backends.get(backendName); + if (!backend) continue; + const backendId = await backend.startAdvertising(entry.opts); + this.activeAdvertisements.set(id, { + backendName, + backendId, + opts: entry.opts, + }); } - return Promise.resolve(); } - /** Stop a previously started advertisement. */ + /** Stop a previously started advertisement, addressed by its stable external id. */ async stopAdvertising(id: string): Promise { - const backendName = this.activeAdvertisements.get(id); - if (backendName === undefined) return; - const backend = this.backends.get(backendName); + const active = this.activeAdvertisements.get(id); + if (active === undefined) return; + const backend = this.backends.get(active.backendName); if (!backend) return; - await backend.stopAdvertising(id); + await backend.stopAdvertising(active.backendId); this.activeAdvertisements.delete(id); } diff --git a/src/test/visibility.integration.test.ts b/src/test/visibility.integration.test.ts index a3d34b4c..be184571 100644 --- a/src/test/visibility.integration.test.ts +++ b/src/test/visibility.integration.test.ts @@ -9,14 +9,42 @@ * - Actions route through CommsTool correctly */ -import { test, describe, expect } from "vitest"; +import { test, describe, expect, vi } from "vitest"; import { MeshStore } from "../core/mesh-store.js"; import { CommsTool } from "../core/tool.js"; import { buildAction } from "../core/bridge.js"; import { DiscoveryManager } from "../core/discovery.js"; +import type { DiscoveryBackend, AdvertiseOptions } from "../core/discovery.js"; import type { MeshVisibility } from "../core/types.js"; import { wireTestTransport } from "./test-transport.js"; +/** A fake DiscoveryBackend whose startAdvertising returns a NEW, incrementing id every call -- deliberately unlike the real mdns/tailscale backends' own deterministic `${name}-${port}` ids, so a test exercising it proves DiscoveryManager itself preserves a stable external advertisement id across pause/resume, rather than merely benefiting from a backend's own incidental determinism. */ +function fakeBackend(name: string): DiscoveryBackend & { + startCalls: AdvertiseOptions[]; + stopCalls: string[]; +} { + let counter = 0; + const startCalls: AdvertiseOptions[] = []; + const stopCalls: string[] = []; + return { + name, + startCalls, + stopCalls, + startAdvertising: vi.fn(async (opts: Readonly) => { + await Promise.resolve(); + startCalls.push({ ...opts }); + counter += 1; + return `${name}-internal-${String(counter)}`; + }), + stopAdvertising: vi.fn(async (id: string) => { + await Promise.resolve(); + stopCalls.push(id); + }), + discover: vi.fn(async () => Promise.resolve([])), + stop: vi.fn(async () => Promise.resolve()), + }; +} + const TEST_PORT = 19881; // --------------------------------------------------------------------------- @@ -66,6 +94,71 @@ describe("DiscoveryManager visibility", () => { }); }); +// --------------------------------------------------------------------------- +// Pause/resume actually re-advertises (regression coverage for the bug the wire-mesh migration plan names: "quiet -> discoverable silently leaves you unadvertised" -- pauseAllAdvertisements/resumeAllAdvertisements used to discard the original AdvertiseOptions and never call backend.startAdvertising again on resume). +// --------------------------------------------------------------------------- + +describe("DiscoveryManager pause/resume genuinely re-advertises", () => { + test("setVisibility(quiet) then setVisibility(discoverable) calls startAdvertising again with the original opts", async () => { + const dm = new DiscoveryManager(); + const backend = fakeBackend("mdns"); + dm.registerBackend(backend); + const opts: AdvertiseOptions = { name: "my-mesh", port: 19876 }; + + const id = await dm.advertise("mdns", opts); + expect(backend.startCalls).toHaveLength(1); + + await dm.setVisibility("quiet"); + expect(backend.stopCalls).toContain("mdns-internal-1"); + expect(dm.isPaused(id)).toBe(true); + + await dm.setVisibility("discoverable"); + + expect(backend.startCalls).toHaveLength(2); + expect(backend.startCalls[1]).toEqual(opts); + expect(dm.isPaused(id)).toBe(false); + }); + + test("the external advertisement id stays stable across a pause/resume cycle, even though the backend returns a new internal id each call", async () => { + const dm = new DiscoveryManager(); + const backend = fakeBackend("mdns"); + dm.registerBackend(backend); + const opts: AdvertiseOptions = { name: "my-mesh", port: 19876 }; + + const id = await dm.advertise("mdns", opts); + await dm.setVisibility("quiet"); + await dm.setVisibility("discoverable"); + + // The caller's original id must still resolve to the (now-different) live backend advertisement. + await dm.stopAdvertising(id); + expect(backend.stopCalls[backend.stopCalls.length - 1]).toBe( + "mdns-internal-2", + ); + }); + + test("per-adapter quiet/dark then discoverable re-advertises only that adapter with its own original opts", async () => { + const dm = new DiscoveryManager(); + const mdns = fakeBackend("mdns"); + const tailscale = fakeBackend("tailscale"); + dm.registerBackend(mdns); + dm.registerBackend(tailscale); + const mdnsOpts: AdvertiseOptions = { name: "mesh-a", port: 19876 }; + const tsOpts: AdvertiseOptions = { name: "mesh-b", port: 19877 }; + + await dm.advertise("mdns", mdnsOpts); + await dm.advertise("tailscale", tsOpts); + + await dm.setVisibility("quiet", "mdns"); + expect(mdns.stopCalls).toHaveLength(1); + expect(tailscale.stopCalls).toHaveLength(0); + + await dm.setVisibility("discoverable", "mdns"); + expect(mdns.startCalls).toHaveLength(2); + expect(mdns.startCalls[1]).toEqual(mdnsOpts); + expect(tailscale.startCalls).toHaveLength(1); + }); +}); + // --------------------------------------------------------------------------- // MeshStore delegation // ---------------------------------------------------------------------------