Skip to content

Commit 0ef7d6f

Browse files
ralyodioclaude
andcommitted
fix(dns): use the port holder enable said it would use
Preflight finds a bridge this run did not start, asks it the clearnet question, and on a good answer prints that it "is being used as-is". Then `enable` started a second daemon anyway. `startDaemon` decides "already running" from our own pidfile, so a holder it did not record is invisible to it. Both bridges then bind -- the socket is created with reuseAddr -- and the kernel hands the query to whichever took the more specific address. On the machine that found this, ours took 127.0.0.1 while the holder had 0.0.0.0, so the bridge the note promised would serve was the one receiving nothing. `holderForwards` was computed, returned and printed, and never read again before the start. This gates the start on it. The existing test was already named for this -- "a holder that forwards is used, not refused" -- and only asserted the note was printed, which is why the second daemon went unnoticed. It now asserts the bridge is not started, and a new test covers the other half: a holder that fails the clearnet question is the stale-bridge case, so --force past it must still start ours. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 parent 49239a2 commit 0ef7d6f

2 files changed

Lines changed: 54 additions & 5 deletions

File tree

src/dns.mjs

Lines changed: 18 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2312,10 +2312,24 @@ export async function dnsCommand(args = [], out = console.log, deps = {}) {
23122312
// which on catch-all routing is a window where every lookup on the machine
23132313
// goes to a port with nothing behind it. It also left nothing to verify
23142314
// against: there is no answer to ask for until the bridge exists.
2315-
const started = await startBridge({ port: wanted, registryBase, entry: cliEntry() });
2316-
out(started.alreadyRunning
2317-
? ` ok bridge already running (pid ${started.pid})`
2318-
: ` ok bridge started on ${DEFAULT_HOST}:${wanted} (pid ${started.pid})`);
2315+
// Unless a bridge this run did not start already holds the port and
2316+
// forwards. Preflight has just said out loud that it is being used as-is,
2317+
// and starting ours anyway makes that line a lie: `startDaemon` decides
2318+
// "already running" from our pidfile alone, so a stranger on the port is
2319+
// invisible to it and it spawns a second daemon. Both then bind — the
2320+
// socket is created with reuseAddr — and the kernel delivers to whichever
2321+
// took the more specific address, so the holder the note promised would
2322+
// serve is silently shadowed by the bridge it said would not be started.
2323+
// Honoring the note is the whole of the fix.
2324+
const reusing = cleared.holder && cleared.holderForwards ? cleared.holder : null;
2325+
const started = reusing
2326+
? { started: false, pid: reusing.pid, alreadyRunning: true, reused: true }
2327+
: await startBridge({ port: wanted, registryBase, entry: cliEntry() });
2328+
out(started.reused
2329+
? ` ok using the bridge already on ${DEFAULT_HOST}:${wanted} (pid ${reusing.pid || "?"}) — not starting a second one`
2330+
: started.alreadyRunning
2331+
? ` ok bridge already running (pid ${started.pid})`
2332+
: ` ok bridge started on ${DEFAULT_HOST}:${wanted} (pid ${started.pid})`);
23192333

23202334
const outcome = await applyWith(plan, {
23212335
verify: () => verify({ moshpit: moshpitProbe }),

test/dns-enable-rollback.test.mjs

Lines changed: 36 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -110,6 +110,7 @@ test("a holder that forwards is used, not refused — or --force becomes the nor
110110
// would train everyone to pass --force, which is how a safety check stops
111111
// being one. Behaviour is the test: it answers a clearnet name.
112112
const lines = [];
113+
let startedOurs = false;
113114
const code = await dnsCommand(["enable"], (l) => lines.push(String(l)), {
114115
...noSystem(),
115116
preflight: (o) => preflightEnable({
@@ -118,10 +119,44 @@ test("a holder that forwards is used, not refused — or --force becomes the nor
118119
listeners: async () => [{ address: "0.0.0.0", port: 5354, pid: 2471795, process: "bun" }],
119120
forwards: async () => true,
120121
}),
122+
startBridge: async () => { startedOurs = true; return { started: true, pid: 1, alreadyRunning: false }; },
121123
applyWith: async () => ({ saved: { ok: true }, applied: { ok: true, results: [] }, verified: { ok: true, checks: [] }, rolledBack: null, backups: [] }),
122124
});
123125
assert.equal(code, 0);
124-
assert.match(lines.join("\n"), /held by pid 2471795, which this run did not start it forwards/);
126+
const out = lines.join("\n");
127+
assert.match(out, /held by pid 2471795, which this run did not start it forwards/);
128+
// "Used as-is" has to mean it. This assertion is the one this test was missing
129+
// while it was named for it: the note printed, and then a second daemon was
130+
// started anyway. `startDaemon` reads our pidfile to decide "already running",
131+
// so it cannot see the holder; both bind, because the socket sets reuseAddr,
132+
// and the kernel gives the query to the more specific bind. On the machine
133+
// that found this, ours took 127.0.0.1 while the holder had 0.0.0.0 — so the
134+
// bridge the note promised would serve was the one getting nothing.
135+
assert.equal(startedOurs, false, "the holder is used as-is, so there is nothing to start");
136+
assert.match(out, /using the bridge already on 127\.0\.0\.1:5354 \(pid 2471795\)/);
137+
assert.doesNotMatch(out, /bridge started on/);
138+
});
139+
140+
test("a holder that does not forward is still not reused — --force starts ours over it", async () => {
141+
// The other half of the gate. A holder that fails the clearnet question is the
142+
// stale-bridge case, and forcing past it means deliberately putting a working
143+
// bridge in front of it — so the start still has to happen.
144+
const lines = [];
145+
let startedOurs = false;
146+
const code = await dnsCommand(["enable", "--force"], (l) => lines.push(String(l)), {
147+
...noSystem(),
148+
preflight: (o) => preflightEnable({
149+
...o,
150+
dropins: async () => [],
151+
listeners: async () => [{ address: "127.0.0.1", port: 5354, pid: 4242, process: "node" }],
152+
forwards: async () => false,
153+
}),
154+
startBridge: async () => { startedOurs = true; return { started: true, pid: 7, alreadyRunning: false }; },
155+
applyWith: async () => ({ saved: { ok: true }, applied: { ok: true, results: [] }, verified: { ok: true, checks: [] }, rolledBack: null, backups: [] }),
156+
});
157+
assert.equal(code, 0);
158+
assert.equal(startedOurs, true);
159+
assert.match(lines.join("\n"), /bridge started on 127\.0\.0\.1:5354 \(pid 7\)/);
125160
});
126161

127162
test("a wildcard bind is caught too — it is the same query, taken by the same stranger", () => {

0 commit comments

Comments
 (0)