amadan

oficina / meet Public

client: one recovery, one budget, and only a 404 enters afresh

S2-11 (P2): a 404 whose probe said our own participation is fine started
a reset to the stale target; that reset was answered 404 too, probed,
and started another, each with a fresh thirty seconds — four hundred and
fifty POSTs in thirteen seconds under the gate. A target the hub has
404'd is now marked stale: nothing is sent to it and no second reset is
asked for until a roster settles it, a reset's own target-404 never asks
for another (fromReset), one reset per target is in flight at a time,
and the recovery's retries share the budget of whatever asked for the
diagnosis.

S2-12 (P2): a probe answering 429, 5xx or nothing at all fell through to
the fresh-entry ladder, which is exactly the S2-3 failure — close a live
call, then meet 409 from its own participation. Only the probe's own 404
enters afresh now; a transient answer is retried on the stream backoff
inside the shared budget, and a recovery that gets no answer leaves the
participation alone.

The negotiation note: negotiation.test.mjs now also drives onAnswer and
onOffer through a connection whose setRemoteDescription can be held
open, so an offer arriving while an answer is being applied is shown to
be answered rather than discarded. Removing the flag assignment fails
it; the six decision cases alone did not.
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 dc614c9cc12c83e0d10242b956ff5a9cdac20ca4 parent de597c6
2 files changed, +188 −34
  • internal/meet/static/room.js +102 −33
  • test/browser/negotiation.test.mjs +86 −1
diff --git a/internal/meet/static/room.js b/internal/meet/static/room.js
index 80e1e0d..61ed76c 100644
--- a/internal/meet/static/room.js
+++ b/internal/meet/static/room.js
@@ -102,6 +102,13 @@
const lastIn = new Map(); // peer id -> highest seq received (resume cursor)
const queue = new Map(); // peer id -> [{seq, body}] outbound, in order
const inflight = new Set();// peer ids with a POST in flight
+ // Targets the hub has answered 404 for while our own participation is
+ // alive: they have left, and our roster has not caught up yet. Nothing
+ // is sent to them and no reset is asked for until a roster settles it —
+ // otherwise every message to a departed peer starts its own 404 →
+ // probe → reset → 404 cycle, each with a fresh budget (Codex S2-11).
+ const stale = new Set();
+ const resetting = new Set(); // targets with a reset already in flight
const names = new Map(); // peer id -> display name, from the server only
const frames = new Map(); // peer id -> frames rendered, for the test hook
const tiles = new Map(); // tile key -> {figure, video}
@@ -649,6 +656,11 @@
state.admitted = true;
state.joined = true;
state.full = false;
+ // The roster is the authority on who is here (§4.5), so it settles
+ // every target a 404 marked stale: the ones it lists get their
+ // connection back through ensurePC below, and the ones it does not
+ // are closed a few lines further down.
+ stale.clear();
const present = new Set();
for (const rp of roster.peers || []) {
if (!rp || !rp.peer) continue;
@@ -806,6 +818,7 @@
}
queue.delete(id);
if (forget) {
+ stale.delete(id);
gen.delete(id);
seqOut.delete(id);
lastIn.delete(id);
@@ -952,6 +965,11 @@
function send(to, type, payload) {
if (!state.peer) return;
+ // A target the hub has already said it does not have. Queueing for
+ // it would buy one more 404 and one more recovery round each time,
+ // and ICE alone produces a dozen candidates a renegotiation; the
+ // roster is what settles this, and it clears the mark.
+ if (stale.has(to)) return;
const seq = (seqOut.get(to) || 0) + 1;
seqOut.set(to, seq);
const q = queue.get(to) || [];
@@ -1144,10 +1162,21 @@
// 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) {
+ async function resetPair(to, budget) {
const peer = state.peer;
if (!peer) return;
- const deadline = Date.now() + SEND_DEADLINE;
+ // One reset per target at a time. Two in flight can only ask the hub
+ // for two generations, and the second is stale before it is sent.
+ if (resetting.has(to)) return;
+ resetting.add(to);
+ try {
+ await resetLoop(to, peer, budget || Date.now() + SEND_DEADLINE);
+ } finally {
+ resetting.delete(to);
+ }
+ }
+
+ async function resetLoop(to, peer, deadline) {
let wait = BACKOFF_MIN;
for (;;) {
if (state.stopped || state.peer !== peer) return;
@@ -1158,7 +1187,10 @@
}
if (res.status === 200) return;
if (res.status === 404) {
- await recoverFromNotFound(res.data.boot, to);
+ // Whatever this 404 turns out to be, it is not worth another
+ // reset: `fromReset` is what stops the cycle, and the budget is
+ // the same one, not a fresh thirty seconds (Codex S2-11).
+ await recoverFromNotFound(res.data.boot, to, { deadline, fromReset: true });
return;
}
if (res.status !== 429 && res.status !== 0 && res.status < 500) {
@@ -1185,9 +1217,19 @@
// 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) {
+ // 404 — and only 404 — is the fresh-entry ladder, which is what this
+ // function always was.
+ //
+ // A 429 or a 5xx from the probe proves nothing either way, and used
+ // to fall through to the fresh-entry ladder and re-create exactly the
+ // failure above (Codex S2-12). It is retried instead, on the stream's
+ // own backoff, inside one budget shared with whatever asked for the
+ // diagnosis — a recovery that cannot get an answer inside thirty
+ // seconds leaves the participation alone and lets the stream ladder,
+ // which is running anyway, be the thing that decides.
+ async function recoverFromNotFound(boot, to, opts) {
+ const o = opts || {};
+ const deadline = o.deadline || Date.now() + SEND_DEADLINE;
if (state.stopped) return;
if (boot) state.boot = boot;
const peer = state.peer;
@@ -1195,35 +1237,62 @@
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;
+ const path = '/m/' + encodeURIComponent(sqid) + '/stream?probe=1&peer=' + encodeURIComponent(peer);
+ let wait = BACKOFF_MIN;
+ for (;;) {
+ let res;
+ try {
+ res = await probe(path);
+ } catch {
+ // The probe could not be made at all: the same "nothing is
+ // proved" as a 5xx, and treated the same way.
+ res = { status: 0, data: {}, signedOut: false, retryAfter: null };
+ }
+ 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) {
+ staleTarget(to, !!o.fromReset);
+ return;
+ }
+ if (res.status === 404) {
+ enterAfresh();
+ return;
+ }
+ // 429, 5xx, a network failure, or a status the contract does not
+ // name: try the diagnosis again rather than acting on it.
+ const ms = res.status === 429 ? retryAfter(res.retryAfter) : wait;
+ wait = Math.min(wait * 2, STREAM_BACKOFF_MAX);
+ if (!(await waitWithin(ms, deadline))) 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);
+ }
+
+ // staleTarget is what a 404 means once the probe has said this
+ // browser's own participation is fine: the peer we were writing to has
+ // gone, and our roster has not caught up. Nothing queued for it can be
+ // delivered, and nothing new may be queued until a roster settles it.
+ //
+ // The single reset is worth asking for once, while the roster still
+ // lists the target, because a `pair` is how a live peer that merely
+ // lost its own participation comes back. Asking again after that reset
+ // has itself been answered 404 is the cycle S2-11 found — twelve
+ // rounds and counting — so `fromReset` skips straight to closing the
+ // connection and waiting for the roster, which recreates it through
+ // `reconcile` if the peer is still there.
+ function staleTarget(to, fromReset) {
+ if (!to) return;
+ queue.set(to, []);
+ const first = !stale.has(to);
+ stale.add(to);
+ if (first && !fromReset && rosterGen(to)) {
+ requestReset(to);
return;
}
- // 404, and anything the contract does not name: this participation
- // really is gone.
- enterAfresh();
+ closePC(to, false);
+ renderTiles();
}
// enterAfresh is the fresh-entry ladder: drop every connection, forget
@@ -1524,7 +1593,7 @@
return;
}
if (res.status === 404) {
- recoverFromNotFound(res.data.boot);
+ await recoverFromNotFound(res.data.boot);
return;
}
if (res.status !== 200) button.disabled = false;
diff --git a/test/browser/negotiation.test.mjs b/test/browser/negotiation.test.mjs
index 1dd6847..5cf393a 100644
--- a/test/browser/negotiation.test.mjs
+++ b/test/browser/negotiation.test.mjs
@@ -30,8 +30,11 @@ const roomJS = fileURLToPath(new URL('../../internal/meet/static/room.js', impor
// 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 + '(');
+ let start = source.indexOf('function ' + name + '(');
assert.notEqual(start, -1, name + ' is not in room.js any more — this test is stale, not passing');
+ // `async` is part of the declaration, and lifting a function that
+ // awaits without it is a syntax error rather than a test.
+ if (source.slice(start - 6, start) === 'async ') start -= 6;
let depth = 0;
for (let i = source.indexOf('{', start); i < source.length; i++) {
if (source[i] === '{') depth += 1;
@@ -76,3 +79,85 @@ test('an answer being applied while we are making an offer is still glare', () =
test('a remote offer already applied is glare', () => {
assert.equal(offerCollision(entry(false, false), 'have-remote-offer'), true);
});
+
+// The decision above is only half of it: the flag has to be *set*, by
+// the answer handler, around the await that makes the window. So the two
+// handlers are lifted out too and driven against a fake connection whose
+// setRemoteDescription can be held open — an answer still being applied
+// when the next offer arrives, which is the race Codex S2-7 named and
+// which the six cases above would pass without.
+const handlers = new Function(
+ 'accept', 'send', 'describe', 'drainCandidates',
+ [lift(src, 'offerCollision'), lift(src, 'onOffer'), lift(src, 'onAnswer')].join('\n') +
+ '\nreturn { onOffer, onAnswer };',
+);
+
+// fakePC is the part of RTCPeerConnection these two handlers touch. The
+// answer's setRemoteDescription is deliberately slow, and the state it
+// leaves behind while it is in flight is the real one: signalingState
+// stays at have-local-offer until the answer is applied.
+function fakePC() {
+ const applied = [];
+ let release = null;
+ const pc = {
+ signalingState: 'have-local-offer',
+ remoteDescription: null,
+ localDescription: { type: 'answer', sdp: 'local' },
+ applied,
+ hold: () => new Promise((resolve) => { release = resolve; }),
+ async setRemoteDescription(desc) {
+ applied.push(desc.type);
+ if (desc.type === 'answer') {
+ await new Promise((resolve) => { release = resolve; });
+ pc.signalingState = 'stable';
+ } else {
+ pc.signalingState = 'have-remote-offer';
+ }
+ pc.remoteDescription = desc;
+ },
+ async setLocalDescription() {
+ pc.signalingState = 'stable';
+ },
+ finishAnswer() {
+ release();
+ },
+ };
+ return pc;
+}
+
+test('an offer arriving while an answer is being applied is answered, not discarded', async () => {
+ const pc = fakePC();
+ const e = {
+ pc,
+ polite: false, // the impolite side: the one that discards on glare
+ makingOffer: false,
+ ignoreOffer: false,
+ isSettingRemoteAnswerPending: false,
+ pending: [],
+ descSeq: 0,
+ };
+ const sent = [];
+ const { onOffer, onAnswer } = handlers(
+ () => e,
+ (to, type) => sent.push(type),
+ (d) => d,
+ async () => {},
+ );
+
+ // The answer goes in and is left half-applied: its promise is still
+ // pending, and signalingState is still have-local-offer.
+ const answering = onAnswer({ from: 'other', seq: 1, payload: { description: { type: 'answer', sdp: 'a' } } });
+ assert.equal(e.isSettingRemoteAnswerPending, true, 'the answer handler must raise the flag before it awaits');
+ assert.equal(pc.signalingState, 'have-local-offer');
+
+ // And the next offer arrives in exactly that window.
+ await onOffer({ from: 'other', seq: 2, payload: { description: { type: 'offer', sdp: 'o' } } });
+
+ assert.equal(e.ignoreOffer, false, 'the impolite side treated a legitimate offer as glare');
+ assert.ok(pc.applied.includes('offer'), 'the offer was never applied: ' + JSON.stringify(pc.applied));
+ assert.deepEqual(sent, ['answer'], 'the offer was not answered: ' + JSON.stringify(sent));
+
+ pc.finishAnswer();
+ await answering;
+ assert.equal(e.isSettingRemoteAnswerPending, false, 'the flag must be cleared once the answer is applied');
+});