oficina / meet Public

room.js: fix round 2 - a roster cannot close what it never knew

The gate's defect had three doors, not two:

- onRoster puts the roster in a slot later rosters overwrite, and
  applyRoster reads it after the await, so a superseded roster is
  never applied.
- a close from reconcile no longer forgets §4.6's counters: seq
  belongs to a sender's participation, and only gone, lost and a
  departure of our own end one.
- and a roster cannot close a connection built after the hub sent it.
  This is the door that was killing the ICE scenarios: the hub does
  not re-send a roster when someone joins, it sends `peer` to the
  people already there. B's empty roster waits on the /ice fetch, A's
  `peer` lands inside it, and B closed the only connection it had with
  nothing left to rebuild it. Every connection carries the tick it was
  built on now, and reconcile skips anything newer than the roster it
  is applying.

The renewal schedule takes the earlier of two readings: the ratio
(margin x 5, x 6), which is immune to clock skew and wrong at short
lives where the server's truncation eats the margin, and the clock
(renewAt - fetchedAt), which is exact when the clocks agree and only
ever early when they do not. Guards read expiresAt; a floor under the
delay stops a marginless answer becoming a spin. ice.lastTryAt is
stamped on every failed attempt and the visibility catch-up reads it.

Three restarts that will not land on a good credential is not an
expired credential, and no longer borrows its sentence: record.trouble
is 'expired' or 'stuck', and the second says "Could not renew this
connection through the relay."

Two more, neither asked for and both needed:

- accept reads the generation before the replay cursor. A message from
  a newer generation is measured against a cursor the connection it
  replaced built, so calling it a replay is calling it something it
  cannot be. An older generation is still dropped.
- adoptServers sets the configuration on a floor-built connection but
  restarts only one that has not begun negotiating. Restarting across
  an offer in flight cost the ice scenario its media in one run: both
  sides connected, the remote track there, not one frame.

negotiation.test.mjs is 43 tests, and every new case was run against
the un-fixed source first. The three t.Skip markers are gone; the note
above `skipped` is kept as the clearest account of the defect on this
branch.

make browser is 26/26 with no skips in 318 s. Roughly one run in three
still loses one scenario — a different one each time — at the first
pairing, with both sides stable/new, no candidates and no frames: ICE
gathering that never started, which is the environment symptom the
Makefile already documents. Each of them passes in isolation every
time; staletarget 3 for 3. A negotiation nudge tried against it did
not move it and was reverted. task-4-report.md has the signature and
what to try next.

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 1 month ago 67f229241a990cf712ea78946d57d1d022c97c86 parent 0bd131a

4 files changed, +543 −61

docs/superpowers/copy/2026-09-10-slice-2-strings.md +2 −1
internal/meet/static/room.js +195 −48
internal/meettest/browser_test.go +12 −12
test/browser/negotiation.test.mjs +334 −0

CI passed — run details

diff --git a/docs/superpowers/copy/2026-09-10-slice-2-strings.md b/docs/superpowers/copy/2026-09-10-slice-2-strings.md

index 451b358..cc6ce1f 100644

--- a/docs/superpowers/copy/2026-09-10-slice-2-strings.md

+++ b/docs/superpowers/copy/2026-09-10-slice-2-strings.md

@@ -185,7 +185,7 @@ narrowed what can reach it:

The relay slice (`docs/superpowers/plans/2026-09-10-meet-slice-3-relay.md`).

Copy review is waived this round per suite discussion 5; every new string

-is catalogued here instead. Three new strings are user-facing, all in

+is catalogued here instead. Four new strings are user-facing, all in

`room.js`; two strings already in this file are listed beside them,

because slice 3 changes when one of them is shown and adds a second

path to the other.

@@ -194,6 +194,7 @@ path to the other.

| --- | --- | --- |

| Meet could not reach the relay. Calls may not connect until it can. | `room.js`, `iceWarning()` — `#ice-warning`, a `role="status"` paragraph the script creates once beside `#status` | Spec §5.3 step 1. Shown from the first failed `/ice` fetch until one succeeds; the client retries every minute behind it. Its own live region rather than `#status`, so it cannot take the call's own sentence with it. Says what happened and what it may cost, and does not diagnose the network — §5.5's rule. **Shown only where a relay is configured** (`data-relay`): on the floor there is no relay this instance could reach, and the sentence would be untrue. A 429 is not a failure and never shows it: the server named a retry time and the client waits it out inside the same budget. |

| The relay credential expired for this connection. | `room.js`, `tileNote()` — a tile's `.meet-tile-note` | Spec §5.3 step 3. Shown per connection, not per call: the must-succeed-by is the old credential's expiry, and the browser cannot see which credential an allocation is using, so this is the honest signal and not a diagnosis. The existing "Retry" beside it runs a fresh fetch and then another ICE restart. |

+| Could not renew this connection through the relay. | `room.js`, `tileNote()` — a tile's `.meet-tile-note` | Spec §5.3 step 2's other ending, and the reason it is not the sentence above. Three ICE restarts with a *good* new credential in hand, none of them proven within thirty seconds: the credential is fine and the path is not, so "expired" would be a sentence about the wrong thing. Same Retry beside it. |

| Could not connect, even through the relay. | `room.js`, `tileNote()` | Spec §5.5, in the spec's own words. Replaces "Could not connect." on a failed tile when `#room` carries `data-relay="1"` **and this connection was actually given the list** — a connection still running on the empty list never went near a relay, and the sentence would be a claim about something it never tried. The README's TCP-443-only note (slice 6) is where the possible causes are named; the tile does not diagnose. |

| Retry | `room.js`, `tile()` | Already catalogued for the failed tile. Now also the control on an expired tile, where it means "fetch a new credential and restart", not "restart". One label, because from a person's side it is one thing: try this connection again. |

| Could not connect. | `room.js`, `tileNote()` | Already catalogued. Unchanged, and still what a failed tile says on an instance with no relay (§5.5's floor). |

diff --git a/internal/meet/static/room.js b/internal/meet/static/room.js

index 6d973ed..1b6a96c 100644

--- a/internal/meet/static/room.js

+++ b/internal/meet/static/room.js

@@ -96,6 +96,10 @@

const RENEW_ATTEMPTS = 3;

const RENEW_WINDOW = 30000;

const RENEW_POLL = 500;

+ // The floor under a renewal delay. Nothing the server can send should

+ // schedule a renewal sooner than this: a list with no margin left in

+ // it would otherwise fetch, schedule for now, and fetch again.

+ const MIN_RENEW_DELAY = 1000;

const PEER_ID = /^[0-9a-f]{32}$/;

const HAS_SINK_ID = typeof HTMLMediaElement !== 'undefined' &&

typeof HTMLMediaElement.prototype.setSinkId === 'function';

@@ -145,6 +149,7 @@

margin: 0, // expiresAt - renewAt: a duration both clocks agree on

fetchedAt: 0, // when the list we hold arrived

failedSince: 0, // the first failure of the current run, or 0

+ lastTryAt: 0, // the most recent failed attempt, for the catch-up

fetches: 0, // *successful* fetches, for the gate

lastFetchAt: 0,

};

@@ -153,6 +158,12 @@

let waiting = []; // the admission queue, members only (§4.3)

let lastRoster = null;

+ // The newest roster this browser has been sent and has not applied

+ // yet. Only §5.2's fetch can leave anything in it for longer than a

+ // microtask.

+ let pendingRoster = null;

+ let pendingRosterAt = 0; // the tick that roster arrived on

+ let tick = 0;

let local = null; // the local MediaStream, or null

let prefs = loadPrefs();

let es = null; // the live EventSource

@@ -559,6 +570,12 @@

// ---------------------------------------------------------------

function handle(type, msg) {

+ // Every downstream event moves this on. It is not a clock and not a

+ // sequence number the server would recognise — it is only this

+ // browser's own count of what it has been told, and it exists so

+ // that a roster can be compared with the connections built since it

+ // arrived (see reconcile).

+ tick += 1;

switch (type) {

case 'hello': return onHello(msg);

case 'roster': return onRoster(msg);

@@ -733,7 +750,17 @@

// neither a person nor a gate could see it. Same shape as the

// pageshow handler's.

function onRoster(msg) {

- applyRoster(msg).catch((err) => {

+ // The newest roster wins, and the slot is what makes that true

+ // across §5.2's await: a roster that arrives while the first /ice

+ // fetch is out overwrites the one waiting to be applied, rather

+ // than queueing behind it. Reconciling a superseded roster closes

+ // connections the hub has since named — which is exactly how the

+ // gate's three ICE scenarios died (Task 5's report): a `peer` event

+ // ten milliseconds after the roster built a connection the captured

+ // roster did not carry.

+ pendingRoster = msg.roster || { peers: [] };

+ pendingRosterAt = tick;

+ applyRoster().catch((err) => {

console.error('meet: roster', err);

});

}

@@ -743,14 +770,22 @@

// roster reconciles straight away. A fetch that fails does not hold

// the room up: it leaves the floor's empty list in place, shows the

// warning and arms the minute retry, and the roster is applied.

- async function applyRoster(msg) {

- const roster = msg.roster || { peers: [] };

+ //

+ // Whatever it applies is read *after* the await, never before. Two

+ // rosters that arrive during one fetch resume in order: the first

+ // takes the newest of them and empties the slot, and the second finds

+ // it empty and has nothing left to do.

+ async function applyRoster() {

if (!icePrimed) await iceReady();

if (state.stopped) return;

- reconcile(roster);

+ const roster = pendingRoster;

+ const at = pendingRosterAt;

+ pendingRoster = null;

+ if (!roster) return;

+ reconcile(roster, at);

}

- function reconcile(roster) {

+ function reconcile(roster, at) {

rosters += 1;

lastRoster = roster;

state.admitted = true;

@@ -775,10 +810,27 @@

present.add(rp.peer);

ensurePC(rp.peer, rp.gen || 1);

}

- for (const id of Array.from(pcs.keys())) {

- // A connection to an id the roster does not carry is closed: this

- // is how a browser that missed a `gone` still lets go.

- if (!present.has(id)) closePC(id, true);

+ for (const [id, entry] of pcs) {

+ if (present.has(id)) continue;

+ // A roster can only speak for the room as it was when the hub

+ // sent it. A connection built *since* — from a `peer` event that

+ // arrived while §5.2's fetch was still out, which is the case

+ // that cost the gate three scenarios (Task 5's report) — is not

+ // something this roster ever knew about, and closing it leaves a

+ // call with a peer it will never hear from again: no further

+ // roster is coming, because nothing has changed as far as the hub

+ // is concerned.

+ if (at && entry.bornAt > at) continue;

+ // Otherwise: a connection to an id the roster does not carry is

+ // closed, which is how a browser that missed a `gone` still lets

+ // go. The counters are *not* dropped with it, though — §4.6 gives

+ // `seq` a sender's whole participation, and a roster's silence is

+ // not the hub telling us that participation ended. `gone`, `lost`

+ // and a departure of our own are; they forget. Dropping them here

+ // meant a connection closed and rebuilt started numbering at 1

+ // into a peer whose cursor was already at 3, and every message of

+ // the new one was discarded as a replay.

+ closePC(id, false);

}

waiting = (roster.waiting || []).map((w) => {

names.set(w.peer, w.name || '');

@@ -870,6 +922,10 @@

// restart's offer/answer exchange has completed" is read without

// guessing from signalingState alone.

desc: 0,

+ // What this browser had been told when the connection was built.

+ // reconcile reads it: a roster older than this cannot be talking

+ // about this connection.

+ bornAt: tick,

};

pcs.set(id, entry);

@@ -969,12 +1025,26 @@

const from = msg.from;

if (!from) return null;

const seq = msg.seq || 0;

- if (seq && seq <= (lastIn.get(from) || 0)) return null;

- if (seq) lastIn.set(from, seq);

const g = msg.gen || 0;

const mine = gen.get(from) || 0;

+ // The generation is read before the cursor, and that order is

+ // load-bearing. A message from a generation *older* than ours

+ // belongs to a pair that has been reset out from under it and is

+ // gone whatever its seq. One from a *newer* generation is the other

+ // side of a reset we have not seen yet — and the cursor it would be

+ // measured against was built by the connection that reset, so it

+ // cannot honestly call that message a replay. Reading the cursor

+ // first meant the first message of a rebuilt pair could be

+ // discarded for ever, which is the line Task 5's report found under

+ // the stale-roster defect.

if (g && g < mine) return null;

- if (g && g > mine) rebuildPC(from, g);

+ if (g && g > mine) {

+ rebuildPC(from, g);

+ if (seq) lastIn.set(from, seq);

+ return pcs.get(from) || null;

+ }

+ if (seq && seq <= (lastIn.get(from) || 0)) return null;

+ if (seq) lastIn.set(from, seq);

return pcs.get(from) || null;

}

@@ -1101,6 +1171,7 @@

const ICE_WARNING = 'Meet could not reach the relay. Calls may not connect until it can.';

const ICE_EXPIRED = 'The relay credential expired for this connection.';

+ const ICE_STUCK = 'Could not renew this connection through the relay.';

const FAILED_RELAY = 'Could not connect, even through the relay.';

const FAILED_PLAIN = 'Could not connect.';

@@ -1203,6 +1274,7 @@

ice.lastFetchAt = ice.fetchedAt;

ice.fetches += 1;

ice.failedSince = 0;

+ ice.lastTryAt = 0;

clearTimeout(iceRetryTimer);

iceWarning(false);

scheduleRenewal();

@@ -1229,6 +1301,11 @@

// fetch is the case that makes it matter.

function iceFailed() {

if (state.stopped) return;

+ // Every failure, not just the first: the catch-up on a tab coming

+ // back has to know when this browser last actually tried, and

+ // `failedSince` is the start of the run and answers a different

+ // question (whether the sentence is up).

+ ice.lastTryAt = Date.now();

if (!ice.failedSince) ice.failedSince = Date.now();

iceWarning(true);

clearTimeout(iceRetryTimer);

@@ -1286,15 +1363,35 @@

if (!ice.servers.length) return;

for (const entry of pcs.values()) {

if (!entry.floor) continue;

+ try {

+ entry.pc.setConfiguration({ iceServers: ice.servers });

+ } catch {

+ // An older browser without setConfiguration: nothing to adopt

+ // into, and the connection keeps what it was built with.

+ continue;

+ }

+ // And a restart only for a connection that has not started

+ // negotiating. Interrupting one that is mid-offer — which is

+ // exactly what a connection built ten milliseconds before the

+ // list lands is — cost the `ice` scenario its media in one gate

+ // run out of four: both sides reached `connected`, the remote

+ // track arrived, and not one frame ever came down it. A

+ // connection left on the floor is not stuck, and not silent about

+ // it: it gathers host candidates, which is §5.5 and connects on a

+ // permissive network, and `entry.floor` staying true is what

+ // makes its tile say "Could not connect." rather than claiming it

+ // tried the relay. Its Retry restarts it — with these servers,

+ // which are in the configuration either way — and so does the

+ // first renewal, which is a restart this file actually proves.

+ if (entry.desc || entry.makingOffer || entry.pc.signalingState !== 'stable') continue;

entry.floor = false;

entry.listAt = ice.fetchedAt;

try {

- entry.pc.setConfiguration({ iceServers: ice.servers });

entry.pc.restartIce();

} catch {

- // No setConfiguration, or no restartIce: the connection keeps

- // the host candidates it has, which is §5.5's floor and still a

- // call on a permissive network.

+ // No restartIce: the first offer this connection makes will

+ // gather on the servers anyway, because they are already in the

+ // configuration.

}

@@ -1305,41 +1402,62 @@

// `renewAt` already behind us runs at once.

function scheduleRenewal() {

clearTimeout(renewTimer);

- if (state.stopped || !ice.margin || !ice.servers.length) return;

+ if (state.stopped || !ice.expiresAt || !ice.servers.length) return;

renewTimer = setTimeout(renew, Math.max(0, renewDelay() - (Date.now() - ice.fetchedAt)));

}

// renewDelay and mustSucceedBy are §5.3's two instants, measured as

- // durations from the moment this browser took the list — never as

+ // durations from the moment this browser took the list rather than as

// points on the server's clock. A browser whose clock is minutes off

// the server's would otherwise renew at the wrong time, and one far

// enough ahead would decide the credential had expired before it ever

- // used it. What both machines do agree on is how long the credential

- // lasts: the server sends renewAt and expiresAt from one clock, and

- // their difference is the margin it left itself. The contract's own

- // shape — renew at five sixths of the life — makes the rest

- // arithmetic: the margin is a sixth of the life, so the delay is five

- // of them and the whole life is six.

+ // used it.

+ //

+ // There are two ways to read the duration and neither is right on its

+ // own, so both are taken and the earlier wins.

+ //

+ // The *ratio* reading uses the margin the server left itself —

+ // expiresAt minus renewAt, a subtraction of two instants on one clock,

+ // so it survives any skew at all. The contract's shape (renew at five

+ // sixths of the life) makes the rest arithmetic: the margin is a sixth

+ // of the life, the delay five of them, the life six. But the server

+ // sends whole seconds, and truncation eats the margin at short lives:

+ // a twelve-second life gives 10 and 12 — margin 2, and 2 x 5 = 10, the

+ // truth — while a ten-second life gives 8 and 10, where the same

+ // arithmetic says 10 and would renew at the very moment the credential

+ // died.

+ //

+ // The *clock* reading subtracts this browser's fetch stamp from the

+ // server's instants. It is exact when the clocks agree and wrong by

+ // the skew when they do not — but wrong in a direction that only

+ // matters one way: a browser running ahead of the server gets a

+ // shorter answer and renews early, which costs nothing, and one

+ // running behind gets a longer one, which the ratio reading then caps.

+ //

+ // So: the earlier of the two, and a floor under the delay so that a

+ // server answering with no margin at all cannot turn the schedule into

+ // a spin.

function renewDelay() {

- return ice.margin * 5;

+ return Math.max(MIN_RENEW_DELAY,

+ Math.min(ice.margin * 5, ice.renewAt - ice.fetchedAt));

}

function mustSucceedBy() {

- return ice.fetchedAt + ice.margin * 6;

+ return ice.fetchedAt + Math.min(ice.margin * 6, ice.expiresAt - ice.fetchedAt);

}

// §5.3's last paragraph: a suspended tab stops its timers, so a tab

// coming back to visible compares the clock with `renewAt` rather than

// trusting one, and runs the sequence at once if it is overdue.

function catchUpRenewal() {

- if (state.stopped || !ice.margin || !ice.servers.length) return;

+ if (state.stopped || !ice.expiresAt || !ice.servers.length) return;

// A fetch that is already failing has a retry of its own running on

// its own minute, and a tab coming back to visible is not a reason

// to jump that queue. The comparison is with the clock rather than

// with the timer because that timer is exactly what a suspended tab

// stops: a failure older than the retry interval means the minute

// has passed with nothing to run it, and this is what runs it.

- if (ice.failedSince && Date.now() - ice.failedSince < ICE_RETRY) return;

+ if (ice.failedSince && Date.now() - ice.lastTryAt < ICE_RETRY) return;

if (Date.now() - ice.fetchedAt >= renewDelay()) renew();

}

@@ -1414,17 +1532,32 @@

}

+ // The two verdicts a renewal can end on, and they are not

+ // interchangeable. `expired` is the must-succeed-by: the credential

+ // this connection is using ran out before a new one landed, which is

+ // §5.3 step 3's honest signal. `stuck` is the other ending — three

+ // ICE restarts with a good new credential in hand, none of them

+ // proven — where the credential is fine and the path is not. Saying

+ // "expired" there would be a sentence about the wrong thing.

function markRelayExpired(id) {

+ markTrouble(id, 'expired');

+ }

+ function markRenewalStuck(id) {

+ markTrouble(id, 'stuck');

+ }

+ function markTrouble(id, kind) {

const record = renewalRecord(id);

if (record.renewed) return;

- record.relayExpired = true;

+ record.trouble = kind;

renderTiles();

}

function renewalRecord(id) {

let record = renewals.get(id);

if (!record) {

- record = { attempts: 0, renewed: false, ufrag: '', at: 0, running: false, relayExpired: false };

+ record = { attempts: 0, renewed: false, ufrag: '', at: 0, running: false, trouble: '' };

renewals.set(id, record);

}

return record;

@@ -1443,6 +1576,11 @@

if (record.running) return false;

record.running = true;

record.renewed = false;

+ // Which of the two endings this run reaches. The deadline is the

+ // only thing that makes "expired" true; three restarts that would

+ // not land on a good credential is a different fact and gets a

+ // different sentence.

+ let ranOut = false;

try {

for (let attempt = 1; attempt <= RENEW_ATTEMPTS; attempt++) {

if (state.stopped || pcs.get(id) !== entry) return false;

@@ -1467,14 +1605,14 @@

// answers rebuilds the connection from nothing — a rebuild

// this record does not survive, which is what "the connection

// is gone from under us" means below. If nothing has rebuilt

- // it by the end of the window, nothing is going to: the

- // connection is still on a credential that has run out, and

- // the person is owed the sentence rather than a tile that

- // looks fine and carries no media.

+ // it by the end of the window, nothing is going to, and the

+ // person is owed a sentence rather than a tile that looks

+ // fine and carries no media.

requestReset(id);

await sleep(RENEW_WINDOW);

if (state.stopped || pcs.get(id) !== entry) return false;

- markRelayExpired(id);

+ if (deadline && Date.now() > deadline) markRelayExpired(id);

+ else markRenewalStuck(id);

return false;

}

const proven = await proveRenewed(id, entry, oldUfrag, from, Date.now() + RENEW_WINDOW);

@@ -1484,16 +1622,20 @@

record.ufrag = proven;

record.at = Date.now();

entry.listAt = ice.fetchedAt;

- record.relayExpired = false;

+ record.trouble = '';

renderTiles();

return true;

}

// Read between attempts, never during one: `mustSucceedBy` is

// the whole call's deadline, and a restart that is about to

// land is worth more than a sentence delivered on time.

- if (deadline && Date.now() > deadline) break;

+ if (deadline && Date.now() > deadline) {

+ ranOut = true;

+ break;

+ }

}

- markRelayExpired(id);

+ if (ranOut) markRelayExpired(id);

+ else markRenewalStuck(id);

return false;

} finally {

record.running = false;

@@ -1513,13 +1655,13 @@

// true — and the connection still gets the plain restart the ordinary

// Retry would have given it, which is the one thing left that might

// help.

- async function retryRenewal(id, wasExpired) {

+ async function retryRenewal(id, wasTrouble) {

if (await fetchIce()) {

await renewOne(id, 0);

return;

}

- if (wasExpired) {

- renewalRecord(id).relayExpired = true;

+ if (wasTrouble) {

+ renewalRecord(id).trouble = wasTrouble;

renderTiles();

}

const entry = pcs.get(id);

@@ -2169,7 +2311,8 @@

// data-relay is where the server states it.

function tileNote(id, entry) {

const record = renewals.get(id);

- if (record && record.relayExpired) return ICE_EXPIRED;

+ if (record && record.trouble === 'expired') return ICE_EXPIRED;

+ if (record && record.trouble === 'stuck') return ICE_STUCK;

if (!entry.failed) return '';

// Both halves have to be true for the longer sentence: this

// instance has a relay, and *this connection* was given it. A

@@ -2291,11 +2434,11 @@

if (!entry) return;

entry.failed = false;

const record = renewals.get(id);

- const expired = !!(record && record.relayExpired);

- if (record) record.relayExpired = false;

+ const trouble = (record && record.trouble) || '';

+ if (record) record.trouble = '';

renderTiles();

- if (expired || iceStale()) {

- retryRenewal(id, expired).catch((err) => {

+ if (trouble || iceStale()) {

+ retryRenewal(id, trouble).catch((err) => {

console.error('meet: renew', err);

});

return;

@@ -2316,7 +2459,7 @@

// iceStale: we hold a list, and the server's own renew time for it has

// passed. A retry then is a renewal, not a restart.

function iceStale() {

- return !!(ice.servers.length && ice.margin &&

+ return !!(ice.servers.length && ice.expiresAt &&

Date.now() - ice.fetchedAt >= renewDelay());

}

@@ -2509,7 +2652,11 @@

attempts: r.attempts,

renewed: !!r.renewed,

ufrag: r.ufrag || '',

- expired: !!r.relayExpired,

+ // `expired` is the must-succeed-by's own verdict and nothing

+ // else; `trouble` also carries a renewal that could not land on

+ // a credential that was perfectly good.

+ expired: r.trouble === 'expired',

+ trouble: r.trouble || '',

};

}

return out;

diff --git a/internal/meettest/browser_test.go b/internal/meettest/browser_test.go

index bd36090..5f615bb 100644

--- a/internal/meettest/browser_test.go

+++ b/internal/meettest/browser_test.go

@@ -108,7 +108,11 @@ var scenarios = []string{

// scenario back in the gate — that, and nothing else, is what "fixed"

// means.

-// All three ICE scenarios share one defect, and it is not in them:

+// FIXED in Task 4's fix round 2 — see task-4-report.md. The account

+// below is left as written because it is what the fix was made against,

+// and because the last paragraph of it is still true of `accept`.

+//

+// All three ICE scenarios shared one defect, and it was not in them:

// room.js applyRoster captures the roster, awaits the /ice fetch

// (§5.2's ordering), and then reconciles the roster it captured

@@ -148,17 +152,13 @@ var scenarios = []string{

// build connections at once and adopt the servers when the list lands —

// and, under it, an `accept` that lets a new generation past the cursor

// it is renumbering.

-var skipped = map[string]string{

- "ice": "Task 4 fix round: FAIL member A settled negotiation and connected to member B " +

- "— signaling stable, connection connecting on one side; have-local-offer, connection new on the other, " +

- "then FAIL member A decoded a video frame from member B (frames {}). See the note above skipped.",

- "icerenewal": "Task 4 fix round: FAIL member A settled negotiation and connected to member B, " +

- "then FAIL A is on its first list before the renewal (5), FAIL the second list is a fetch and not a re-read (0 ms later), " +

- "FAIL the proof is a new ICE generation (UBi5 -> UBi5). See the note above skipped.",

- "icefail": "Task 4 fix round: FAIL member A is admitted and has a peer connection " +

- "— status \"You are the only one here.\", no connection at all after the stale roster closed it. " +

- "See the note above skipped.",

+// Empty, and the account above is kept because it is the clearest one

+// this branch has of a defect that cost two slices a flake each. Task

+// 4's fix round 2 closed both halves: applyRoster reconciles the roster

+// held when the fetch returns, and a close from reconcile no longer

+// forgets §4.6's counters for a peer the hub may still name. The three

+// ICE scenarios and staletarget have run in the gate ever since.

+var skipped = map[string]string{}

func TestCallInARealBrowser(t *testing.T) {

rig := newBrowserRig(t)

diff --git a/test/browser/negotiation.test.mjs b/test/browser/negotiation.test.mjs

index 48ac2e1..ba8c2a4 100644

--- a/test/browser/negotiation.test.mjs

+++ b/test/browser/negotiation.test.mjs

@@ -334,6 +334,19 @@ test('without a transport report, nomination is the selection', () => {

]), 'ff1');

});

+test('the fallback prefers a nominated pair that names a fragment over one that does not', () => {

+ // Two nominated pairs and only the second names its fragment. Taking

+ // the first would answer '' — "a pair, but no generation" — which

+ // renewedUfrag reads as proof for a connection that had nothing

+ // recorded before the restart. The rule is: keep looking.

+ assert.equal(selectedPairUfrag([

+ { type: 'candidate-pair', id: 'P1', state: 'succeeded', nominated: true, localCandidateId: 'L1' },

+ { type: 'candidate-pair', id: 'P2', state: 'succeeded', nominated: true, localCandidateId: 'L2' },

+ { type: 'local-candidate', id: 'L1' },

+ { type: 'local-candidate', id: 'L2', usernameFragment: 'ff2' },

+ ]), 'ff2');

+});

test('nothing recorded before the restart falls back to any selected succeeded pair', () => {

// §5.3 step 2's fallback, for a connection that had no selected pair

// to read, or a browser that will not name fragments.

@@ -470,3 +483,324 @@ test('the must-succeed-by marks the connections that credential was about, and o

assert.deepEqual(marked.sort(), ['older', 'on-it'],

'a connection built on the expiring credential, with nothing running, is the whole point');

});

+// ── the roster the fetch came back to (§4.5, §4.6, §5.2) ──────────────

+//

+// §5.2 puts one await in front of the first connection this browser

+// builds, and that await is long enough for the room to change under it.

+// Two rules keep the call alive across it, and the gate's three ICE

+// scenarios died for want of both (Task 5's report):

+//

+// 1. what is reconciled is the roster held when the fetch *returns*,

+// not the one captured before it. A superseded roster does not name

+// the peer that arrived during the fetch, and reconciling it closes

+// a connection the hub has since named.

+// 2. a close from `reconcile` does not forget §4.6's counters. `seq`

+// belongs to the sender's participation, not to a connection, and a

+// connection closed and rebuilt that starts again at 1 is answered

+// by a peer whose cursor is already at 3 — which discards it as a

+// replay, for ever.

+const rosterPath = new Function(

+ 'state', 'icePrimed', 'iceReady', 'reconcile', 'pendingRoster',

+ 'pendingRosterAt', 'tick', 'console',

+ [lift(src, 'onRoster'), lift(src, 'applyRoster')].join('\n') +

+ '\nreturn { onRoster };',

+);

+test('the roster applied is the newest one, not the one the fetch started with', async () => {

+ const applied = [];

+ let release = null;

+ const fetching = new Promise((resolve) => { release = resolve; });

+ const { onRoster } = rosterPath(

+ { stopped: false },

+ false, // icePrimed: the first fetch is still out

+ () => fetching,

+ (roster) => applied.push(roster),

+ null, 0, 0,

+ console,

+ );

+ onRoster({ roster: { peers: [{ peer: 'a' }] } });

+ // A `peer` joins ten milliseconds later, and the hub sends the roster

+ // again. Both land while the fetch is out.

+ onRoster({ roster: { peers: [{ peer: 'a' }, { peer: 'b' }] } });

+ release();

+ await new Promise((resolve) => setTimeout(resolve, 0));

+ assert.equal(applied.length, 1, 'a superseded roster must not be applied at all');

+ assert.deepEqual(applied[0].peers.map((p) => p.peer), ['a', 'b'],

+ 'the roster that names the peer who arrived during the fetch is the one that wins');

+});

+// reconcile and closePC, lifted together and driven over real Maps: the

+// forget rule is a property of the pair of them, and stubbing either

+// would be testing the stub.

+const reconciler = new Function(

+ 'state', 'stale', 'names', 'pcs', 'gen', 'seqOut', 'lastIn', 'queue', 'frames',

+ 'renewals', 'ensurePC', 'renderTiles', 'renderQueue', 'renderStatus',

+ 'rosters', 'lastRoster', 'waiting',

+ [lift(src, 'reconcile'), lift(src, 'closePC')].join('\n') +

+ '\nreturn { reconcile };',

+);

+function reconcileOver(pcs, seqOut, lastIn) {

+ const built = [];

+ const { reconcile } = reconciler(

+ { stopped: false, peer: 'me', kind: 'member' },

+ new Set(), new Map(), pcs, new Map(), seqOut, lastIn, new Map(), new Map(),

+ new Map(), (id, g) => built.push([id, g]), () => {}, () => {}, () => {},

+ 0, null, [],

+ );

+ return { reconcile, built };

+// A connection, as reconcile sees one: what tick it was built on, and a

+// close it can report.

+function connection(bornAt, closed, id) {

+ return { bornAt, pc: { close() { closed.push(id); } } };

+test('a peer the roster stops naming loses its connection and keeps its counters', () => {

+ const closed = [];

+ const pcs = new Map([['peer1', connection(1, closed, 'peer1')]]);

+ const seqOut = new Map([['peer1', 7]]);

+ const lastIn = new Map([['peer1', 3]]);

+ const { reconcile } = reconcileOver(pcs, seqOut, lastIn);

+ reconcile({ peers: [{ peer: 'me' }] }, 2);

+ assert.deepEqual(closed, ['peer1'], 'the connection is still closed — that part was right');

+ assert.equal(seqOut.get('peer1'), 7,

+ 'seq belongs to the sender participation (§4.6), not to the connection');

+ assert.equal(lastIn.get('peer1'), 3, 'and so does the replay cursor');

+});

+test('a peer the roster still names keeps its connection and its counters', () => {

+ const closed = [];

+ const pcs = new Map([['peer1', connection(1, closed, 'peer1')]]);

+ const seqOut = new Map([['peer1', 7]]);

+ const lastIn = new Map([['peer1', 3]]);

+ const { reconcile, built } = reconcileOver(pcs, seqOut, lastIn);

+ reconcile({ peers: [{ peer: 'me' }, { peer: 'peer1', gen: 1 }] }, 2);

+ assert.deepEqual(closed, [], 'a present peer must not be closed');

+ assert.ok(pcs.has('peer1'), 'the connection survives a roster that names it');

+ assert.deepEqual(built, [['peer1', 1]], 'and goes through the one door into the mesh');

+ assert.equal(seqOut.get('peer1'), 7);

+ assert.equal(lastIn.get('peer1'), 3);

+});

+// ── the renewal schedule (§5.3 step 4) ────────────────────────────────

+//

+// Two readings of the same schedule, and the earlier of them wins. The

+// ratio reading is immune to clock skew and wrong at short lives, where

+// the server's truncation to whole seconds eats the margin; the clock

+// reading is exact when the clocks agree and skewed when they do not,

+// but only ever in a direction that renews early. Neither is safe alone.

+const schedule = new Function(

+ 'ice', 'MIN_RENEW_DELAY',

+ [lift(src, 'renewDelay'), lift(src, 'mustSucceedBy')].join('\n') +

+ '\nreturn { renewDelay, mustSucceedBy };',

+);

+// answered builds what /ice sends for a life in seconds — including the

+// server's own truncation, which is where the short lives go wrong —

+// read by a browser whose clock is `skew` ms ahead of the server's.

+const SERVER_NOW = 1700000000000; // an exact second, as Unix() gives

+function answered(life, skew) {

+ const secs = SERVER_NOW / 1000;

+ const expiresAt = (secs + life) * 1000;

+ const renewAt = (secs + Math.floor(life * 5 / 6)) * 1000;

+ return {

+ servers: [1],

+ expiresAt,

+ renewAt,

+ margin: Math.max(0, expiresAt - renewAt),

+ fetchedAt: SERVER_NOW + (skew || 0),

+ };

+function scheduleFor(ice) {

+ const { renewDelay, mustSucceedBy } = schedule(ice, 1000);

+ return { delay: renewDelay(), life: mustSucceedBy() - ice.fetchedAt };

+test('the three-hour life schedules where the contract says', () => {

+ // 10800 s: renew at 9000, expire at 10800, and the truncation is a

+ // no-op because both divide.

+ assert.deepEqual(scheduleFor(answered(10800)), { delay: 9000000, life: 10800000 });

+});

+test('a twelve-second life survives truncation because it divides too', () => {

+ // The gate's own icerenewal TTL: 10 and 12, margin 2, and 2 x 5 = 10.

+ assert.deepEqual(scheduleFor(answered(12)), { delay: 10000, life: 12000 });

+});

+test('a ten-second life is where the ratio reading alone would renew too late', () => {

+ // renewAt is 8 (8.333 truncated), so the margin reads 2 and the ratio

+ // says 10 — the exact moment the credential dies. The clock reading

+ // says 8, and the earlier of the two is what is used.

+ assert.deepEqual(scheduleFor(answered(10)), { delay: 8000, life: 10000 });

+});

+test('a two-second life renews at its floor and not before', () => {

+ // renewAt is 1 (1.667 truncated): the clock says 1000 ms, the ratio

+ // says 5000, and the floor is 1000 — so 1000, inside a life of 2000.

+ const got = scheduleFor(answered(2));

+ assert.deepEqual(got, { delay: 1000, life: 2000 });

+ assert.ok(got.delay < got.life, 'a renewal scheduled after the expiry is not a renewal');

+});

+test('a browser running behind the server is capped by the ratio, not stretched by the clock', () => {

+ // 60 s behind: the clock reading says 9060 s, which would renew a

+ // minute after the server expected. The ratio reading caps it.

+ assert.equal(scheduleFor(answered(10800, -60000)).delay, 9000000);

+});

+test('a browser running ahead of the server renews early, which costs nothing', () => {

+ const got = scheduleFor(answered(10800, 60000));

+ assert.equal(got.delay, 8940000, 'the clock reading is the earlier one here, and early is safe');

+ assert.equal(got.life, 10740000);

+});

+test('a server that leaves no margin at all cannot turn the schedule into a spin', () => {

+ const ice = { servers: [1], expiresAt: SERVER_NOW + 2000, renewAt: SERVER_NOW, margin: 0, fetchedAt: SERVER_NOW };

+ assert.equal(scheduleFor(ice).delay, 1000, 'the floor is what stops fetch-schedule-fetch');

+});

+test('a roster cannot close a connection built after the hub sent it', () => {

+ // The door the "newest roster" rule does not cover, and the one that

+ // actually cost the gate three scenarios: the hub does not re-send a

+ // roster when someone joins — it sends `peer` to the people already

+ // there. So a peer arriving while §5.2's fetch is out builds a

+ // connection, no newer roster ever comes, and the roster waiting on

+ // the fetch does not name it. Closing it there leaves a call with a

+ // participant it will never hear from again.

+ const closed = [];

+ const pcs = new Map([

+ ['before', connection(1, closed, 'before')],

+ ['during', connection(3, closed, 'during')],

+ ]);

+ const { reconcile } = reconcileOver(pcs, new Map(), new Map());

+ reconcile({ peers: [{ peer: 'me' }] }, 2);

+ assert.deepEqual(closed, ['before'],

+ 'a connection older than the roster is the only one it can speak for');

+ assert.ok(pcs.has('during'), 'the connection built during the fetch survives');

+});

+// ── adopting the list a connection was built without (§5.2) ───────────

+//

+// The first fetch is one await long, and a `peer` can land inside it. It

+// builds a connection on the floor's empty list, and when the list

+// arrives that connection is given it. What it is NOT given is an ICE

+// restart on top of a negotiation already in flight: doing that cost the

+// `ice` scenario its media in one gate run out of four — both sides

+// reached `connected`, the remote track arrived, and no frame ever came

+// down it.

+const adopter = new Function(

+ 'ice', 'pcs',

+ lift(src, 'adoptServers') + '\nreturn { adoptServers };',

+);

+function adoptable(state, desc) {

+ const calls = [];

+ return {

+ floor: true,

+ desc: desc || 0,

+ makingOffer: false,

+ listAt: 0,

+ calls,

+ pc: {

+ signalingState: state,

+ setConfiguration: (c) => calls.push(['setConfiguration', c.iceServers.length]),

+ restartIce: () => calls.push(['restartIce']),

+ },

+ };

+test('a connection that has not started negotiating takes the servers and restarts', () => {

+ const entry = adoptable('stable', 0);

+ const ice = { servers: [1, 2], fetchedAt: 99 };

+ adopter(ice, new Map([['p', entry]])).adoptServers();

+ assert.deepEqual(entry.calls, [['setConfiguration', 2], ['restartIce']]);

+ assert.equal(entry.floor, false, 'this one really is on the list now');

+ assert.equal(entry.listAt, 99);

+});

+test('a connection mid-negotiation takes the servers and is left alone', () => {

+ for (const entry of [adoptable('have-local-offer', 0), adoptable('stable', 1)]) {

+ const ice = { servers: [1, 2], fetchedAt: 99 };

+ adopter(ice, new Map([['p', entry]])).adoptServers();

+ assert.deepEqual(entry.calls, [['setConfiguration', 2]],

+ 'an ICE restart across an offer in flight is what broke the media');

+ assert.equal(entry.floor, true,

+ 'and the tile must not claim a relay this connection never gathered through');

+ }

+});

+test('there is nothing to adopt on the floor', () => {

+ const entry = adoptable('stable', 0);

+ adopter({ servers: [], fetchedAt: 99 }, new Map([['p', entry]])).adoptServers();

+ assert.deepEqual(entry.calls, []);

+});

+// ── §4.6's inbound rules, in the order they have to be read ───────────

+//

+// The cursor and the generation both refuse messages, and which is

+// consulted first decides whether a pair that has just been reset can

+// ever speak again. A message from a newer generation is measured

+// against a cursor the *previous* connection built, so calling it a

+// replay is calling it something it cannot be.

+const inbound = new Function(

+ 'gen', 'lastIn', 'pcs', 'rebuildPC',

+ lift(src, 'accept') + '\nreturn { accept };',

+);

+function inboundOver(g, cursor) {

+ const rebuilt = [];

+ const pcs = new Map([['them', { id: 'pc' }]]);

+ const gen = new Map([['them', g]]);

+ const lastIn = new Map([['them', cursor]]);

+ const { accept } = inbound(gen, lastIn, pcs, (from, to) => {

+ rebuilt.push([from, to]);

+ gen.set(from, to);

+ });

+ return { accept, rebuilt, lastIn };

+test('a message at or below the cursor is a replay and is dropped', () => {

+ const { accept } = inboundOver(1, 3);

+ assert.equal(accept({ from: 'them', seq: 3, gen: 1 }), null);

+});

+test('a message above the cursor is taken, and moves it', () => {

+ const { accept, lastIn } = inboundOver(1, 3);

+ assert.ok(accept({ from: 'them', seq: 4, gen: 1 }));

+ assert.equal(lastIn.get('them'), 4);

+});

+test('a message from an older generation is dropped whatever its seq', () => {

+ const { accept } = inboundOver(2, 3);

+ assert.equal(accept({ from: 'them', seq: 99, gen: 1 }), null);

+});

+test('a message from a newer generation is not a replay, whatever the cursor says', () => {

+ // The line under the stale-roster defect: seq 1 from a pair that has

+ // just been reset, against a cursor of 3 from the connection it

+ // replaced. Read cursor-first, this is discarded — and so is

+ // everything after it, because the cursor never moves again.

+ const { accept, rebuilt, lastIn } = inboundOver(1, 3);

+ assert.ok(accept({ from: 'them', seq: 1, gen: 2 }),

+ 'the first message of a rebuilt pair must get through');

+ assert.deepEqual(rebuilt, [['them', 2]], 'and it rebuilds the pair under the new generation');

+ assert.equal(lastIn.get('them'), 1, 'the cursor follows the generation that is speaking now');

+});