Skip to content

fix(core): actually re-advertise on discovery visibility resume #143

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Mearman merged 1 commit into from
Sep 16, 2026
Merged

fix(core): actually re-advertise on discovery visibility resume #143

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
82 changes: 53 additions & 29 deletions src/core/discovery.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
*/

import type { MeshVisibility } from "./types.js";
import { nanoid } from "./nanoid.js";

// ---------------------------------------------------------------------------
// Discovered mesh
Expand Down Expand Up @@ -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<string, DiscoveryBackend>();
private readonly activeAdvertisements = new Map<string, string>();
/** 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<string, MeshVisibility>();
/** 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 }
Expand All @@ -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<AdvertiseOptions>,
Expand All @@ -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;
}

Expand Down Expand Up @@ -154,16 +167,15 @@ export class DiscoveryManager {
}

private async pauseAllAdvertisements(): Promise<void> {
// 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 */
});
}
Expand All @@ -174,15 +186,15 @@ export class DiscoveryManager {
private async pauseAdvertisementsForBackend(
backendName: string,
): Promise<void> {
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 */
});
}
Expand All @@ -200,32 +212,44 @@ export class DiscoveryManager {
}

private async resumeAllAdvertisements(): Promise<void> {
// 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<void> {
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<void> {
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);
}

Expand Down
95 changes: 94 additions & 1 deletion src/test/visibility.integration.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<AdvertiseOptions>) => {
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;

// ---------------------------------------------------------------------------
Expand Down Expand Up @@ -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
// ---------------------------------------------------------------------------
Expand Down