amadan

oficina / meet Public

client: a 404 is not always ours, and four other holes

S2-3 (P2): a signal 404 can mean the target left, not that this
participation ended. recoverFromNotFound now probes the sender's own
participation first: 204 keeps the call and reconciles the target from
the next roster, 410 is terminal with the tombstone's reason, 404 is the
fresh-entry ladder it always was. The old behaviour closed a live call
and then met 409 from its own participation, and showed the takeover
screen for a tab that did not exist.

S2-5 (P2): requestReset is bounded retry with Retry-After, the 429/5xx
backoff and the signed-out and 404 arms, instead of a discarded POST.
After an outbound deadline the reset is the only thing that can rebuild
the pair, and losing it left the call down for good.

S2-6 (P2): §4.6's thirty seconds is an absolute per-message deadline
covering requests, backoff and 429s, with an AbortController on the
fetch at the deadline. Each of those could slip it before.

S2-7 (P2): perfect negotiation gains isSettingRemoteAnswerPending, the
W3C guard, factored into offerCollision so it can be gated without a
browser (test/browser/negotiation.test.mjs, run by make browser).

S2-8 (P3): join and the pump check signedOut before the 200 — a session
that expired can answer with a redirect to sign-in, which fetch follows
into a 200 carrying the sign-in page.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CzvnuKhLzudvq7jZ7SvVt2
Paul Campbell pushed by paul@keymail.dev 5e8db6cbba7e21ae191f55bf0803e6531956c7e8 parent 3c8c6ac
3 files changed, +324 −44
  • Makefile +6 −0
  • internal/meet/static/room.js +240 −44
  • test/browser/negotiation.test.mjs +78 −0
diff --git a/Makefile b/Makefile
index 6927e6d..75d535d 100644
--- a/Makefile
+++ b/Makefile
@@ -128,8 +128,14 @@ ci: vet fmt-check test migration-check
# -count=1 because call.mjs is not an input to Go's test cache: node
# opens it, not the test, so an edited browser script would otherwise be
# answered with a cached pass from before the edit.
+# negotiation.test.mjs runs first because it needs no browser at all: it
+# lifts one pure function out of room.js and checks §5.1's glare
+# decision against the W3C truth table, in forty milliseconds. Named
+# explicitly rather than by directory — node's test runner treats
+# everything under test/ as a test file, and call.mjs is not one.
browser:
go vet -tags browser ./internal/meettest/
+ node --test test/browser/negotiation.test.mjs
npm install --no-save playwright@1.62.1
npx playwright install chromium
go test -tags browser -count=1 -run TestCallInARealBrowser -v ./internal/meettest/
diff --git a/internal/meet/static/room.js b/internal/meet/static/room.js
index 424f02f..80e1e0d 100644
--- a/internal/meet/static/room.js
+++ b/internal/meet/static/room.js
@@ -509,10 +509,19 @@
renderStatus();
}
- async function join() {
+ async function join(attempt) {
+ const round = attempt || 0;
const peer = state.peer;
const res = await postSignal({ peer, type: 'join' });
if (state.stopped || state.peer !== peer) return;
+ // signedOut before the status, because it can be true *of* a 200: a
+ // session that has expired answers 303 to /signin, fetch follows
+ // it, and the sign-in page comes back 200 — which read as a status
+ // alone marked the join a success (Codex S2-8).
+ if (res.signedOut) {
+ terminal('signedout');
+ return;
+ }
if (res.status === 200) {
state.joined = true;
// A guest gets nothing back from its own join: no roster, no
@@ -521,12 +530,15 @@
renderStatus();
return;
}
- if (res.signedOut) {
- terminal('signedout');
- return;
- }
if (res.status === 404) {
- recoverFromNotFound(res.data.boot);
+ await recoverFromNotFound(res.data.boot);
+ // The probe kept this participation, so the 404 raced something
+ // rather than naming the end of it. `join` is idempotent: one
+ // more go, and then it is the stream's ladder's problem.
+ if (round === 0 && !state.stopped && state.peer === peer && !state.joined) {
+ await sleep(BACKOFF_MIN);
+ if (!state.stopped && state.peer === peer && !state.joined) join(1);
+ }
return;
}
// 429 and the transient failures are retried on the same peer, with
@@ -535,7 +547,7 @@
// knock speaks for itself.
if (res.status === 429 || res.status >= 500 || res.status === 0) {
await sleep(res.status === 429 ? retryAfter(res.retryAfter) : BACKOFF_MIN);
- if (!state.stopped && state.peer === peer && !state.joined) join();
+ if (!state.stopped && state.peer === peer && !state.joined) join(round);
}
}
@@ -714,6 +726,12 @@
polite: String(state.peer) < String(id),
makingOffer: false,
ignoreOffer: false,
+ // §5.1's third negotiation flag, and the one this file was
+ // missing (Codex S2-7). It is true only while an answer is being
+ // applied, which is a state that looks exactly like glare from
+ // the outside — signalingState is still have-local-offer — and is
+ // not.
+ isSettingRemoteAnswerPending: false,
pending: [],
descSeq: 0,
gen: g,
@@ -822,16 +840,39 @@
return pcs.get(from) || null;
}
+ // offerCollision is the W3C perfect-negotiation decision, written out
+ // as its own function because it is the one piece of §5.1 that can be
+ // reasoned about — and gated — without a browser (see
+ // test/browser/negotiation.test.mjs, which reads it out of this file).
+ //
+ // readyForOffer = !makingOffer &&
+ // (signalingState === "stable" || isSettingRemoteAnswerPending)
+ // offerCollision = !readyForOffer
+ //
+ // The second half of that disjunction is the whole point: while an
+ // answer is being applied, signalingState is still have-local-offer,
+ // and without this an offer arriving in that window read as glare and
+ // was discarded by the impolite side (Codex S2-7).
+ function offerCollision(entry, signalingState) {
+ const readyForOffer = !entry.makingOffer &&
+ (signalingState === 'stable' || entry.isSettingRemoteAnswerPending);
+ return !readyForOffer;
+ }
+
async function onOffer(msg) {
const entry = accept(msg);
if (!entry) return;
const desc = msg.payload && msg.payload.description;
if (!desc || desc.type !== 'offer') return;
- const collision = entry.makingOffer || entry.pc.signalingState !== 'stable';
- entry.ignoreOffer = !entry.polite && collision;
- if (entry.ignoreOffer) return;
+ if (offerCollision(entry, entry.pc.signalingState)) {
+ entry.ignoreOffer = !entry.polite;
+ if (entry.ignoreOffer) return;
+ } else {
+ entry.ignoreOffer = false;
+ }
try {
// The polite peer's implicit rollback lives inside this call.
+ entry.isSettingRemoteAnswerPending = false; // this one is an offer
await entry.pc.setRemoteDescription(desc);
entry.ignoreOffer = false;
entry.descSeq = msg.seq || entry.descSeq;
@@ -851,7 +892,17 @@
if (!desc || desc.type !== 'answer') return;
if (entry.pc.signalingState !== 'have-local-offer') return;
try {
- await entry.pc.setRemoteDescription(desc);
+ // The W3C flag, set exactly where the reference sets it: around
+ // setRemoteDescription, true because this description is an
+ // answer. An offer that arrives while this is in flight is a
+ // legitimate offer, not glare, and offerCollision reads this to
+ // tell the two apart.
+ entry.isSettingRemoteAnswerPending = true;
+ try {
+ await entry.pc.setRemoteDescription(desc);
+ } finally {
+ entry.isSettingRemoteAnswerPending = false;
+ }
// The reference sets ignoreOffer on every description it applies,
// and an answer that lands means the collision it was set for is
// over: candidate failures after this point are real again.
@@ -912,31 +963,52 @@
// pump owns one POST at a time per target, in order, and does not
// start the next message until this one is accepted. Every status the
// wire contract names has its own arm here.
+ //
+ // §4.6's thirty seconds is an absolute deadline per message, stamped
+ // when the message reaches the head of the queue and covering
+ // everything that follows: the requests, the backoff between them,
+ // and the 429s. Each of those used to be able to slip it (Codex
+ // S2-6) — a 429 skipped the check entirely, a sleep could overshoot
+ // it, and a fetch that never settled meant the check was never
+ // reached at all — so a pair could sit "unreachable" for ever with
+ // nothing rebuilding it. The fetch is aborted at the deadline for
+ // that last one.
async function pump(to) {
if (inflight.has(to)) return;
inflight.add(to);
- let failingSince = 0;
let wait = BACKOFF_MIN;
try {
for (;;) {
const q = queue.get(to);
if (!q || !q.length || state.stopped) return;
- const res = await postSignal(q[0].body);
+ const head = q[0];
+ if (!head.deadline) head.deadline = Date.now() + SEND_DEADLINE;
+ if (Date.now() >= head.deadline) {
+ unreachable(to);
+ return;
+ }
+ const res = await postSignal(head.body, { deadline: head.deadline });
if (queue.get(to) !== q) return; // the queue was dropped underneath us
+ // signedOut is read before the status, because it can be true
+ // *of* a 200: an expired session answers 303 to /signin, fetch
+ // follows it, and the sign-in page comes back 200 (Codex S2-8).
+ if (res.signedOut) {
+ terminal('signedout');
+ return;
+ }
if (res.status === 200) {
q.shift();
- failingSince = 0;
wait = BACKOFF_MIN;
continue;
}
- if (res.signedOut) {
- terminal('signedout');
- return;
- }
if (res.status === 429) {
// Retry-After is whole seconds, and the same message is
- // retried with the same seq.
- await sleep(retryAfter(res.retryAfter));
+ // retried with the same seq — but never past this message's
+ // own deadline.
+ if (!(await waitWithin(retryAfter(res.retryAfter), head.deadline))) {
+ unreachable(to);
+ return;
+ }
continue;
}
if (res.status === 409) {
@@ -953,30 +1025,22 @@
return; // `conflict` belongs to admit, which is not queued here
}
if (res.status === 404) {
- recoverFromNotFound(res.data.boot);
+ await recoverFromNotFound(res.data.boot, to);
return;
}
if (res.status === 400 || res.status === 413) {
// Malformed, or over 64 KB: never retried. Drop that one
- // message and carry on with the rest of the queue — with the
- // failure clock cleared, because this one was answered.
+ // message and carry on with the rest of the queue — the next
+ // one gets a deadline of its own, because this one was
+ // answered.
q.shift();
- failingSince = 0;
wait = BACKOFF_MIN;
continue;
}
- if (!failingSince) failingSince = Date.now();
- if (Date.now() - failingSince > SEND_DEADLINE) {
- // §4.6: the target is unreachable. Close the connection, ask
- // the hub for a fresh generation, and let `pair` or the next
- // roster build it again.
- queue.set(to, []);
- closePC(to, false);
- renderTiles();
- requestReset(to);
+ if (!(await waitWithin(wait, head.deadline))) {
+ unreachable(to);
return;
}
- await sleep(wait);
wait = Math.min(wait * 2, BACKOFF_MAX);
}
} finally {
@@ -990,6 +1054,17 @@
}
}
+ // unreachable is §4.6's verdict on one target: the message at the head
+ // of its queue never got through inside its deadline. Close the
+ // connection, drop what was queued for it, and ask the hub for a fresh
+ // generation — `pair` or the next roster builds it again.
+ function unreachable(to) {
+ queue.set(to, []);
+ closePC(to, false);
+ renderTiles();
+ requestReset(to);
+ }
+
function rosterGen(id) {
if (!lastRoster) return 0;
for (const rp of lastRoster.peers || []) if (rp.peer === id) return rp.gen || 0;
@@ -1005,36 +1080,157 @@
return new Promise((resolve) => setTimeout(resolve, ms));
}
+ // waitWithin sleeps for ms, or until the deadline, whichever comes
+ // first, and answers whether there is any budget left afterwards. A
+ // backoff is not allowed to be the reason a deadline is missed.
+ async function waitWithin(ms, deadline) {
+ const left = deadline - Date.now();
+ if (left <= 0) return false;
+ await sleep(Math.min(ms, left));
+ return Date.now() < deadline;
+ }
+
// postSignal is the one place this file talks upstream. csrf.Protect
// is origin-based, so a same-origin fetch needs no token.
- async function postSignal(body, keepalive) {
+ //
+ // `deadline` is an absolute instant, not a duration: a request that
+ // has not settled by then is aborted, so a hanging fetch cannot hold
+ // a queue open past the moment its caller had promised to give up
+ // (Codex S2-6). An aborted fetch throws, and lands on the same
+ // status 0 as any other network failure — which the caller then reads
+ // as "out of budget" on its next pass.
+ async function postSignal(body, opts) {
+ const o = opts || {};
const path = '/m/' + encodeURIComponent(sqid) + '/signal';
+ let stop = 0;
+ let controller = null;
+ if (o.deadline && typeof AbortController === 'function') {
+ controller = new AbortController();
+ stop = setTimeout(() => controller.abort(), Math.max(0, o.deadline - Date.now()));
+ }
try {
const res = await fetch(path, {
method: 'POST',
credentials: 'same-origin',
headers: { 'Content-Type': 'application/json' },
body: JSON.stringify(body),
- keepalive: !!keepalive,
+ keepalive: !!o.keepalive,
+ signal: controller ? controller.signal : undefined,
});
return await readRefusal(res, 200, path);
} catch {
return { status: 0, data: {}, signedOut: false, retryAfter: null };
+ } finally {
+ if (stop) clearTimeout(stop);
}
}
+ // requestReset is the fire-and-forget face of resetPair: every caller
+ // asks for a reset and carries on, and a rejection escaping an async
+ // function nobody awaits would go where neither a person nor a gate
+ // could see it. Same shape as the pageshow handler's.
function requestReset(to) {
- postSignal({ peer: state.peer, type: 'reset', to });
+ resetPair(to).catch((err) => {
+ console.error('meet: reset', err);
+ });
}
- // recoverFromNotFound is the 404 from either endpoint: this
- // participation no longer exists. `boot` splits a process restart
- // from a lapsed resume window; the recovery is the same either way,
- // and the difference is only worth carrying so the fresh stream
- // starts from the right boot id.
- function recoverFromNotFound(boot) {
+ // resetPair asks the hub for a fresh generation with this target.
+ // The answer used to be discarded (Codex S2-5), so a 429 or a 5xx
+ // lost the reset silently — and the reset that follows an outbound
+ // deadline is the only thing that rebuilds a connection this browser
+ // has just closed, which left the pair down with nothing coming to
+ // fix it. Bounded by the same thirty seconds one queued message gets,
+ // and by the same rules: Retry-After on a 429, doubling backoff on
+ // the transient failures, terminal on a session that has been signed
+ // out, and the 404 ladder.
+ async function resetPair(to) {
+ const peer = state.peer;
+ if (!peer) return;
+ const deadline = Date.now() + SEND_DEADLINE;
+ let wait = BACKOFF_MIN;
+ for (;;) {
+ if (state.stopped || state.peer !== peer) return;
+ const res = await postSignal({ peer, type: 'reset', to }, { deadline });
+ if (res.signedOut) {
+ terminal('signedout');
+ return;
+ }
+ if (res.status === 200) return;
+ if (res.status === 404) {
+ await recoverFromNotFound(res.data.boot, to);
+ return;
+ }
+ if (res.status !== 429 && res.status !== 0 && res.status < 500) {
+ // 400, 409 and anything else the contract names: a reset that
+ // is refused on its merits is not retried. The roster is the
+ // other way this pair converges (§4.6).
+ return;
+ }
+ const ms = res.status === 429 ? retryAfter(res.retryAfter) : wait;
+ wait = Math.min(wait * 2, BACKOFF_MAX);
+ if (!(await waitWithin(ms, deadline))) return;
+ }
+ }
+
+ // recoverFromNotFound is a 404 from either endpoint. It used to read
+ // as "this participation no longer exists" and close everything, but
+ // a 404 from `/signal` says no such thing (Codex S2-3): the *target*
+ // may simply have left, which is the ordinary case while this
+ // browser's own stream is down and the `gone` event cannot reach it.
+ // Acting on that as if it were our own ending closed a live call and
+ // then met a 409 from our own still-live participation, and showed a
+ // takeover screen for a tab that does not exist.
+ //
+ // So the sender's own participation is probed, and that is the
+ // authority: 204 keeps the call and lets the next roster reconcile
+ // the target; 410 is terminal with the reason the tombstone carries;
+ // 404 is the fresh-entry ladder, which is what this function always
+ // was.
+ async function recoverFromNotFound(boot, to) {
if (state.stopped) return;
if (boot) state.boot = boot;
+ const peer = state.peer;
+ if (!peer) {
+ enterAfresh();
+ return;
+ }
+ let res;
+ try {
+ res = await probe('/m/' + encodeURIComponent(sqid) + '/stream?probe=1&peer=' + encodeURIComponent(peer));
+ } catch {
+ // The probe could not be made at all. Nothing is proved, so
+ // nothing is torn down: the stream's own ladder is already
+ // running, and the next roster reconciles whatever this was.
+ return;
+ }
+ if (state.stopped || state.peer !== peer) return;
+ if (refused(res)) return;
+ if (res.status === 410) {
+ terminal(res.data.reason || 'ended');
+ return;
+ }
+ if (res.status === 204) {
+ // Still ours. The 404 was about the target, so drop what was
+ // queued for it — none of it can ever be delivered — and ask for
+ // a fresh generation only if the roster still lists it. If it
+ // does not, `gone` or the next roster has already closed it, and
+ // this browser asks for nothing.
+ if (!to) return;
+ queue.set(to, []);
+ if (rosterGen(to)) requestReset(to);
+ return;
+ }
+ // 404, and anything the contract does not name: this participation
+ // really is gone.
+ enterAfresh();
+ }
+
+ // enterAfresh is the fresh-entry ladder: drop every connection, forget
+ // the peer, and open a new stream. `boot` has already been recorded by
+ // the caller, so the fresh stream starts from the right one.
+ function enterAfresh() {
+ if (state.stopped) return;
closeStream();
closeAllPCs();
state.peer = null;
@@ -1372,7 +1568,7 @@
clearTimeout(reconnectTimer);
clearTimeout(fullTimer);
clearInterval(statsTimer);
- if (state.peer) postSignal({ peer: state.peer, type: 'leave' }, true);
+ if (state.peer) postSignal({ peer: state.peer, type: 'leave' }, { keepalive: true });
closeStream();
closeAllPCs();
stopLocal();
diff --git a/test/browser/negotiation.test.mjs b/test/browser/negotiation.test.mjs
new file mode 100644
index 0000000..1dd6847
--- /dev/null
+++ b/test/browser/negotiation.test.mjs
@@ -0,0 +1,78 @@
+// negotiation.test.mjs — the one piece of room.js that can be gated
+// without a browser.
+//
+// node --test test/browser/negotiation.test.mjs
+//
+// room.js is an IIFE that wants a document, so it cannot be imported.
+// `offerCollision` is a pure function of two arguments, though, and its
+// source is lifted out of the file by name and evaluated here. That
+// keeps the thing under test the shipped source — not a copy of it that
+// can drift — and it fails loudly if the function is renamed or
+// reshaped rather than passing on a stale duplicate.
+//
+// What it gates is spec §5.1's perfect-negotiation decision, exactly as
+// the W3C writes it:
+//
+// readyForOffer = !makingOffer &&
+// (signalingState === "stable" || isSettingRemoteAnswerPending)
+// offerCollision = !readyForOffer
+//
+// The `isSettingRemoteAnswerPending` half was missing (Codex S2-7): an
+// offer arriving while an answer was still being applied read as glare,
+// and the impolite side discarded a legitimate offer.
+import { test } from 'node:test';
+import assert from 'node:assert/strict';
+import { readFileSync } from 'node:fs';
+import { fileURLToPath } from 'node:url';
+
+const roomJS = fileURLToPath(new URL('../../internal/meet/static/room.js', import.meta.url));
+
+// lift returns the source of one top-level-ish `function name(...) {...}`
+// by matching braces from its declaration.
+function lift(source, name) {
+ const start = source.indexOf('function ' + name + '(');
+ assert.notEqual(start, -1, name + ' is not in room.js any more — this test is stale, not passing');
+ let depth = 0;
+ for (let i = source.indexOf('{', start); i < source.length; i++) {
+ if (source[i] === '{') depth += 1;
+ else if (source[i] === '}') {
+ depth -= 1;
+ if (depth === 0) return source.slice(start, i + 1);
+ }
+ }
+ assert.fail('unbalanced braces reading ' + name + ' out of room.js');
+}
+
+const src = readFileSync(roomJS, 'utf8');
+// eslint-disable-next-line no-new-func
+const offerCollision = new Function(lift(src, 'offerCollision') + '; return offerCollision;')();
+
+const entry = (makingOffer, isSettingRemoteAnswerPending) => ({ makingOffer, isSettingRemoteAnswerPending });
+
+test('a stable connection is ready for an offer', () => {
+ assert.equal(offerCollision(entry(false, false), 'stable'), false);
+});
+
+test('an offer of our own in flight is glare', () => {
+ assert.equal(offerCollision(entry(true, false), 'stable'), true);
+ assert.equal(offerCollision(entry(true, false), 'have-local-offer'), true);
+});
+
+test('our own unanswered offer is glare', () => {
+ assert.equal(offerCollision(entry(false, false), 'have-local-offer'), true);
+});
+
+test('an answer being applied is not glare, whatever the signaling state says', () => {
+ // The regression itself: have-local-offer with an answer half applied
+ // is the state a legitimate follow-up offer arrives in.
+ assert.equal(offerCollision(entry(false, true), 'have-local-offer'), false);
+ assert.equal(offerCollision(entry(false, true), 'stable'), false);
+});
+
+test('an answer being applied while we are making an offer is still glare', () => {
+ assert.equal(offerCollision(entry(true, true), 'have-local-offer'), true);
+});
+
+test('a remote offer already applied is glare', () => {
+ assert.equal(offerCollision(entry(false, false), 'have-remote-offer'), true);
+});