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 // ---------------------------------------------------------------------------