oficina / meet Public

room.js: fix round 1 - reachable watchdog, honest selected pair

- expireUnrenewed was unreachable on the path it exists for: it
  compared entry.listAt with ice.fetchedAt, which only advances on a
  successful fetch, so with the fetch still failing every connection
  in the call was exempt and nothing was ever marked. renewAll reads
  the expiring credential's stamp before the fetch and passes it in;
  entries strictly newer than it are exempt, the renewed and running
  skips stay.
- selectedPairUfrag now takes transport.selectedCandidatePairId as the
  only selection when a transport report is present. Chromium keeps
  the previous generation's pair in the report after a restart, still
  succeeded and still nominated, so scanning for one of those let a
  renewal read as proven by the connection it had just replaced.
  nominated/selected remain the fallback for Firefox's shape.
- Retry on an expired tile erased the sentence and then did nothing
  when the fetch failed. retryRenewal takes the sentence's own state,
  restores it, and falls through to a bare restartIce.
- The warning is a claim about a relay, so it is shown only where one
  is configured, and state().ice.warning mirrors the sentence. The
  failed tile says "even through the relay" only when this connection
  was actually given the list (!entry.floor).
- The schedule is durations from the fetch, not instants on the
  server's clock: margin = expiresAt - renewAt, delay = margin * 5,
  must-succeed-by = fetchedAt + margin * 6. The absolute values stay
  in state() for the gate.
- catchUpRenewal respects an outstanding failure's own minute; a /ice
  404 the ladder could not decide arms the minute retry; proveRenewed
  stops when the connection is replaced under it; the no-restartIce
  arm marks the connection expired if nothing rebuilt it inside the
  window.
- /ice spends /signal's bucket, so a 429 is "not now", not a failure:
  fetchIce waits Retry-After inside one SEND_DEADLINE budget and
  shows no warning unless the wait runs out of it.
- negotiation.test.mjs: the watchdog over a fake entry set, three
  selected-pair cases, the replaced-connection cancel, and a bounded
  helper that stops the runaway poll so a widened deadline check
  fails the test instead of hanging the run. 24/24, each new case
  verified failing against the unfixed source.

The trailer names Opus 5, not Fable 5.1: it is what wrote both
rounds, and this session's attribution block says so. The record is
the only thing at stake and a human can rewrite both commits onto the
Fable line if the ledger's consistency matters more.
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 9fed86cbace19f74d5a291ea803fb86f463d8a4d parent b5c29b3
3 files changed, +356 −95
  • docs/superpowers/copy/2026-09-10-slice-2-strings.md +10 −7
  • internal/meet/static/room.js +220 −83
  • test/browser/negotiation.test.mjs +126 −5

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 421886b..451b358 100644
--- a/docs/superpowers/copy/2026-09-10-slice-2-strings.md
+++ b/docs/superpowers/copy/2026-09-10-slice-2-strings.md
@@ -185,21 +185,24 @@ 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. Four are user-facing, all in `room.js`, and
-one existing string is now reached by a second path.
+is catalogued here instead. Three 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.
| String | Where | Note |
| --- | --- | --- |
-| 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. |
+| 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 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"` — i.e. when this instance has a relay configured, so "even through the relay" is a true claim about this deployment and not a guess. The README's TCP-443-only note (slice 6) is where the possible causes are named; the tile does not diagnose. |
+| 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). |
Developer-facing, in `console.error` prefixes rather than on a screen,
-listed only so this file stays a complete record of the branch. Both
-print an error object and never a response body: the `/ice` answer
-carries a credential, and nothing about the servers reaches the console.
+listed only so this file stays a complete record of the branch. All
+three print an error object and never a response body: the `/ice`
+answer carries a credential, and nothing about the servers reaches the
+console.
| String | Where | Note |
| --- | --- | --- |
diff --git a/internal/meet/static/room.js b/internal/meet/static/room.js
index 21e814b..6d973ed 100644
--- a/internal/meet/static/room.js
+++ b/internal/meet/static/room.js
@@ -142,6 +142,7 @@
servers: [], // the RTCIceServer list, [] on the floor (§5.5)
expiresAt: 0, // the credential's hard deadline on the relay
renewAt: 0, // when the server asks us to come back
+ 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
fetches: 0, // *successful* fetches, for the gate
@@ -1131,49 +1132,83 @@
const peer = state.peer;
if (state.stopped || !peer) return false;
const path = '/m/' + encodeURIComponent(sqid) + '/ice?peer=' + encodeURIComponent(peer);
- let res;
- try {
- res = await probe(path, Date.now() + SEND_DEADLINE);
- } catch {
- icePrimed = true;
- iceFailed();
- return false;
- }
- if (state.stopped || state.peer !== peer) return false;
- if (res.signedOut) {
- terminal('signedout');
- return false;
- }
- if (res.status === 404) {
- // Not a relay failure: a 404 here says this participation is gone,
- // and /signal's own 404 says no more than that either. So it goes
- // through the same probe-first recovery — this browser's own
- // participation is the authority on what the 404 meant — and the
- // next roster fetches again, because whatever comes back from that
- // ladder is a call that has not got a list yet.
- icePrimed = false;
- firstIce = null;
- await recoverFromNotFound(res.data.boot);
- return false;
- }
- if (res.status !== 200 || !Array.isArray(res.data.iceServers)) {
+ // One budget for the whole exchange, however many requests it takes
+ // — the shape §4.6 already gives one outbound message, and the same
+ // reason applies: /ice spends /signal's own per-session, per-room
+ // bucket, so a renewal that lands inside a burst of signalling gets
+ // a 429 and a Retry-After rather than an answer.
+ const deadline = Date.now() + SEND_DEADLINE;
+ for (;;) {
+ let res;
+ try {
+ res = await probe(path, deadline);
+ } catch {
+ icePrimed = true;
+ iceFailed();
+ return false;
+ }
+ if (state.stopped || state.peer !== peer) return false;
+ if (res.signedOut) {
+ terminal('signedout');
+ return false;
+ }
+ if (res.status === 429) {
+ // Not a failure: the server said "not now, come back in n". Honour
+ // it inside this budget and say nothing — a warning about the
+ // relay would be untrue, and the person's own call is what filled
+ // that bucket. Only a wait that runs out of budget is a failure.
+ if (!(await waitWithin(retryAfter(res.retryAfter), deadline))) {
+ icePrimed = true;
+ iceFailed();
+ return false;
+ }
+ continue;
+ }
+ if (res.status === 404) {
+ // Not a relay failure: a 404 here says this participation is
+ // gone — a resume window that has lapsed included, now that
+ // /ice gates on the same liveness /signal does — and /signal's
+ // own 404 says no more than that either. So it goes through
+ // the same probe-first recovery — this browser's own
+ // participation is the authority on what the 404 meant — and the
+ // next roster fetches again, because whatever comes back from that
+ // ladder is a call that has not got a list yet.
+ icePrimed = false;
+ firstIce = null;
+ await recoverFromNotFound(res.data.boot);
+ // The ladder is allowed to decide nothing at all: a probe that
+ // cannot answer inside its budget leaves this participation alone
+ // (§4.5), and a call left with no list and no retry pending would
+ // simply sit there. So unless it ended the call or entered afresh
+ // — both of which change what is checked here — the minute retry
+ // covers it.
+ if (!state.stopped && state.peer === peer) iceFailed();
+ return false;
+ }
+ if (res.status !== 200 || !Array.isArray(res.data.iceServers)) {
+ icePrimed = true;
+ iceFailed();
+ return false;
+ }
icePrimed = true;
- iceFailed();
- return false;
+ ice.servers = res.data.iceServers;
+ ice.expiresAt = millis(res.data.expiresAt);
+ ice.renewAt = millis(res.data.renewAt);
+ // The margin the server left itself: expiry minus renew time, on
+ // its clock, where the two subtract to a duration both machines
+ // agree on. Everything below is scheduled from it and from
+ // `fetchedAt`, which is this browser's own clock — see renewDelay.
+ ice.margin = Math.max(0, ice.expiresAt - ice.renewAt);
+ ice.fetchedAt = Date.now();
+ ice.lastFetchAt = ice.fetchedAt;
+ ice.fetches += 1;
+ ice.failedSince = 0;
+ clearTimeout(iceRetryTimer);
+ iceWarning(false);
+ scheduleRenewal();
+ adoptServers();
+ return true;
}
- icePrimed = true;
- ice.servers = res.data.iceServers;
- ice.expiresAt = millis(res.data.expiresAt);
- ice.renewAt = millis(res.data.renewAt);
- ice.fetchedAt = Date.now();
- ice.lastFetchAt = ice.fetchedAt;
- ice.fetches += 1;
- ice.failedSince = 0;
- clearTimeout(iceRetryTimer);
- iceWarning(false);
- scheduleRenewal();
- adoptServers();
- return true;
}
// The wire carries unix seconds; everything in this file works in
@@ -1214,6 +1249,10 @@
function iceWarning(show) {
const el = warningEl();
if (!el) return;
+ // On the floor the sentence would be untrue: there is no relay this
+ // instance could reach, /ice answers an empty list, and a fetch that
+ // fails there costs the call nothing a person could act on (§5.5).
+ if (show && !relayConfigured) return;
if (show) {
// Unhidden first, then written: a live region announces the change
// it can see, and text set into a hidden element is not one.
@@ -1266,16 +1305,42 @@
// `renewAt` already behind us runs at once.
function scheduleRenewal() {
clearTimeout(renewTimer);
- if (state.stopped || !ice.renewAt || !ice.servers.length) return;
- renewTimer = setTimeout(renew, Math.max(0, ice.renewAt - Date.now()));
+ if (state.stopped || !ice.margin || !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
+ // 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.
+ function renewDelay() {
+ return ice.margin * 5;
+ }
+
+ function mustSucceedBy() {
+ return ice.fetchedAt + ice.margin * 6;
}
// §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.renewAt || !ice.servers.length) return;
- if (Date.now() >= ice.renewAt) renew();
+ if (state.stopped || !ice.margin || !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 (Date.now() - ice.fetchedAt >= renewDelay()) renew();
}
function renew() {
@@ -1286,23 +1351,27 @@
// renewAll is §5.3's sequence for the whole mesh: fetch, then renew
// every live connection. The deadline it hands each of them is the
- // *old* credential's expiry (step 3) — the one honest signal, because
- // the browser cannot see which credential an allocation is using — and
- // it survives the minute retries, because a failed fetch leaves
- // `ice.expiresAt` where it was.
+ // *old* credential's — step 3's must-succeed-by, and the one honest
+ // signal, because the browser cannot see which credential an
+ // allocation is using — so both it and the stamp that names the
+ // credential it belongs to are read *before* the fetch. A failed fetch
+ // leaves neither where the watchdog could confuse them: `fetchedAt`
+ // only ever advances on a success, which is exactly why reading it
+ // afterwards made the watchdog unreachable (round-1 review, item 1).
async function renewAll() {
if (state.stopped || renewing) return;
renewing = true;
renewDue = true;
- const mustSucceedBy = ice.expiresAt;
- armMustSucceedBy(mustSucceedBy);
+ const expiring = ice.fetchedAt;
+ const deadline = mustSucceedBy();
+ armMustSucceedBy(deadline, expiring);
try {
// A failed fetch has already armed the warning and the minute
// retry, and that retry comes back here.
if (!(await fetchIce())) return;
renewDue = false;
for (const id of Array.from(pcs.keys())) {
- renewOne(id, mustSucceedBy).catch((err) => {
+ renewOne(id, deadline).catch((err) => {
console.error('meet: renew', err);
});
}
@@ -1318,19 +1387,28 @@
// restart is still in flight is left alone — it is the thing that
// would clear this, and interrupting it would show the sentence to
// someone whose call is about to come back.
- function armMustSucceedBy(at) {
+ function armMustSucceedBy(at, expiring) {
clearTimeout(expiryTimer);
if (!at) return;
- expiryTimer = setTimeout(expireUnrenewed, Math.max(0, at - Date.now()));
- }
-
- function expireUnrenewed() {
+ expiryTimer = setTimeout(() => expireUnrenewed(expiring), Math.max(0, at - Date.now()));
+ }
+
+ // expireUnrenewed is the verdict itself. `expiring` is the stamp of
+ // the credential that has just run out, so "newer than that" means
+ // "built on a list this deadline was never about" — a peer who joined
+ // after the fetch, whose connection has the current credential and
+ // nothing to renew. It has to be the *expiring* stamp and not
+ // `ice.fetchedAt`: on the path this exists for, the fetch is still
+ // failing and `ice.fetchedAt` has not moved, so comparing against it
+ // exempted every connection in the call and nothing was ever marked.
+ function expireUnrenewed(expiring) {
if (state.stopped) return;
for (const [id, entry] of pcs) {
- // Built on the list we hold now: this connection never had the
- // credential that just expired.
- if (entry.listAt >= ice.fetchedAt) continue;
+ if (entry.listAt > expiring) continue;
const record = renewals.get(id);
+ // A restart still in flight is the thing that would clear this,
+ // and interrupting it would show the sentence to someone whose
+ // call is about to come back.
if (record && (record.renewed || record.running)) continue;
markRelayExpired(id);
}
@@ -1358,7 +1436,7 @@
// the must-succeed-by is read *between* attempts rather than during
// one — cutting a restart that is about to land would show a person
// the expired sentence for a call that was coming back.
- async function renewOne(id, mustSucceedBy) {
+ async function renewOne(id, deadline) {
const entry = pcs.get(id);
if (!entry || state.stopped) return false;
const record = renewalRecord(id);
@@ -1386,11 +1464,20 @@
} catch {
// No restartIce at all: a fresh generation is the only way to
// put a new credential under this pair, and the `pair` that
- // answers rebuilds the connection from nothing.
+ // 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.
requestReset(id);
+ await sleep(RENEW_WINDOW);
+ if (state.stopped || pcs.get(id) !== entry) return false;
+ markRelayExpired(id);
return false;
}
- const proven = await proveRenewed(entry, oldUfrag, from, Date.now() + RENEW_WINDOW);
+ const proven = await proveRenewed(id, entry, oldUfrag, from, Date.now() + RENEW_WINDOW);
if (state.stopped || pcs.get(id) !== entry) return false;
if (proven !== null) {
record.renewed = true;
@@ -1401,7 +1488,10 @@
renderTiles();
return true;
}
- if (mustSucceedBy && Date.now() > mustSucceedBy) break;
+ // 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;
}
markRelayExpired(id);
return false;
@@ -1415,18 +1505,38 @@
// must-succeed-by — the deadline it would carry is already behind us,
// and a person who asked for this is owed the three attempts, not an
// instant repeat of the same sentence.
- async function retryRenewal(id) {
- if (!(await fetchIce())) return;
- await renewOne(id, 0);
+ //
+ // A fetch that fails is the case worth writing out. `restartIce` has
+ // already cleared the sentence off the tile, so returning here left a
+ // person looking at a tile that said nothing was wrong while nothing
+ // had been done about it. The sentence goes back up — it is still
+ // 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) {
+ if (await fetchIce()) {
+ await renewOne(id, 0);
+ return;
+ }
+ if (wasExpired) {
+ renewalRecord(id).relayExpired = true;
+ renderTiles();
+ }
+ const entry = pcs.get(id);
+ if (entry) bareRestart(id, entry);
}
// proveRenewed polls getStats until the decision below says the
// restart landed, or until the window runs out. Half a second between
// polls: an ICE restart is a round trip and a handful of candidate
// exchanges, and asking faster only costs the call CPU.
- async function proveRenewed(entry, oldUfrag, from, deadline) {
+ async function proveRenewed(id, entry, oldUfrag, from, deadline) {
for (;;) {
- if (state.stopped) return null;
+ // A connection replaced under us — a `pair`, a roster, an
+ // unreachable target — is not the connection this proof is about,
+ // and polling its stats for another thirty seconds proves nothing
+ // about either of them.
+ if (state.stopped || pcs.get(id) !== entry) return null;
const proven = renewedUfrag(await statsList(entry.pc), oldUfrag,
entry.pc.signalingState, entry.desc > from);
if (proven !== null) return proven;
@@ -1467,24 +1577,39 @@
// username fragment on the local candidate of the selected, succeeded
// candidate pair: the fragment when the browser names it, '' when the
// pair is there and the fragment is not, and null when there is no
- // such pair at all. Chromium marks the pair through the transport's
- // selectedCandidatePairId and through `nominated`, Firefox through
- // `selected`; all three are read, so the same function answers on any
- // of them. Pure, like the decision above.
+ // such pair at all. Pure, like the decision above.
+ //
+ // Where a browser names the selected pair itself — Chromium's
+ // transport.selectedCandidatePairId — that answer wins outright, and a
+ // pair it does not name is not consulted however succeeded and
+ // nominated it looks. Chromium keeps the *previous* generation's pair
+ // in the report for a while after a restart, still succeeded and still
+ // nominated, and scanning for one of those was how a restart could
+ // read as proven by the connection it had just replaced (round-1
+ // review, item 2). `nominated`/`selected` are the fallback for a
+ // report with no transport in it, which is what Firefox sends.
function selectedPairUfrag(reports) {
const locals = new Map();
const pairs = [];
- let selectedId = '';
+ const selected = new Set();
+ let transports = 0;
for (const r of reports || []) {
if (!r || !r.type) continue;
if (r.type === 'local-candidate') locals.set(r.id, r);
else if (r.type === 'candidate-pair') pairs.push(r);
- else if (r.type === 'transport' && r.selectedCandidatePairId) selectedId = r.selectedCandidatePairId;
+ else if (r.type === 'transport') {
+ transports += 1;
+ if (r.selectedCandidatePairId) selected.add(r.selectedCandidatePairId);
+ }
}
+ // A transport that has not named a pair yet has not selected one:
+ // `selected` is empty, nothing matches, and the answer is null.
+ const chosen = transports
+ ? pairs.filter((p) => selected.has(p.id))
+ : pairs.filter((p) => p.nominated || p.selected);
let found = null;
- for (const p of pairs) {
+ for (const p of chosen) {
if (p.state !== 'succeeded') continue;
- if (!(p.nominated || p.selected || (selectedId && p.id === selectedId))) continue;
const local = locals.get(p.localCandidateId);
const ufrag = (local && local.usernameFragment) || '';
// A pair that names the fragment is worth more than one that does
@@ -2046,7 +2171,12 @@
const record = renewals.get(id);
if (record && record.relayExpired) return ICE_EXPIRED;
if (!entry.failed) return '';
- return relayConfigured ? FAILED_RELAY : FAILED_PLAIN;
+ // Both halves have to be true for the longer sentence: this
+ // instance has a relay, and *this connection* was given it. A
+ // connection still running on the empty list never went near one,
+ // and saying it failed through the relay would be a claim about
+ // something it never tried.
+ return relayConfigured && !entry.floor ? FAILED_RELAY : FAILED_PLAIN;
}
// tile builds or updates one <figure> per participant. Nothing here
@@ -2165,16 +2295,20 @@
if (record) record.relayExpired = false;
renderTiles();
if (expired || iceStale()) {
- retryRenewal(id).catch((err) => {
+ retryRenewal(id, expired).catch((err) => {
console.error('meet: renew', err);
});
return;
}
+ bareRestart(id, entry);
+ }
+
+ // bareRestart is §5.1's retry, unchanged: the restart, and a fresh
+ // generation from the hub for a browser that has no restartIce.
+ function bareRestart(id, entry) {
try {
entry.pc.restartIce();
} catch {
- // No restartIce: ask the hub for a fresh generation instead,
- // which rebuilds the connection from nothing.
requestReset(id);
}
}
@@ -2182,7 +2316,8 @@
// 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.renewAt && Date.now() >= ice.renewAt);
+ return !!(ice.servers.length && ice.margin &&
+ Date.now() - ice.fetchedAt >= renewDelay());
}
// applySink routes a remote tile's audio to the speaker the holding
@@ -2422,7 +2557,9 @@
lastFetchAt: ice.lastFetchAt,
expiresAt: ice.expiresAt,
renewAt: ice.renewAt,
- warning: !!ice.failedSince,
+ // The sentence that is actually showing: on the floor there
+ // is no relay to fail and none is shown (§5.5).
+ warning: !!(ice.failedSince && relayConfigured),
servers: ice.servers.length,
},
// Per connection: how many restarts §5.3 step 2 has spent, and
diff --git a/test/browser/negotiation.test.mjs b/test/browser/negotiation.test.mjs
index 243f189..48ac2e1 100644
--- a/test/browser/negotiation.test.mjs
+++ b/test/browser/negotiation.test.mjs
@@ -287,6 +287,53 @@ test('a pair that is not succeeded, or not selected, is not proof', () => {
assert.equal(renewedUfrag(report('new1', { nominated: false, unselected: true }), 'old0', 'stable', true), null);
});
+// Round-1 review, item 2. Chromium keeps the previous generation's pair
+// in the report for a while after a restart — still succeeded, still
+// nominated — so a scan for "a succeeded nominated pair" could be
+// answered by the very connection the restart replaced, and a renewal
+// would read as proven by its own predecessor. Where the browser names
+// the selected pair, that answer is the only one.
+test('the transport names the selected pair, and a stale nominated one does not overrule it', () => {
+ const reports = [
+ // Listed first, so a scan in report order would find this one.
+ { type: 'candidate-pair', id: 'stale', state: 'succeeded', nominated: true, localCandidateId: 'Lold' },
+ { type: 'transport', id: 'T', selectedCandidatePairId: 'fresh' },
+ { type: 'candidate-pair', id: 'fresh', state: 'succeeded', nominated: true, localCandidateId: 'Lnew' },
+ { type: 'local-candidate', id: 'Lold', usernameFragment: 'old0' },
+ { type: 'local-candidate', id: 'Lnew', usernameFragment: 'new1' },
+ ];
+ assert.equal(selectedPairUfrag(reports), 'new1', 'the stale pair answered for the fresh one');
+ assert.equal(renewedUfrag(reports, 'old0', 'stable', true), 'new1');
+});
+
+test('a transport-selected pair needs no nomination flag of its own', () => {
+ assert.equal(selectedPairUfrag([
+ { type: 'transport', id: 'T', selectedCandidatePairId: 'P' },
+ { type: 'candidate-pair', id: 'P', state: 'succeeded', localCandidateId: 'L' },
+ { type: 'local-candidate', id: 'L', usernameFragment: 'u1' },
+ ]), 'u1');
+});
+
+test('a transport that has selected nothing, or selected a pair that has not succeeded, is not a selection', () => {
+ const pair = (state) => ([
+ { type: 'transport', id: 'T', selectedCandidatePairId: state ? 'P' : '' },
+ { type: 'candidate-pair', id: 'P', state: state || 'succeeded', nominated: true, localCandidateId: 'L' },
+ { type: 'local-candidate', id: 'L', usernameFragment: 'u1' },
+ ]);
+ assert.equal(selectedPairUfrag(pair('')), null, 'a transport with no selection must not fall back to nominated');
+ assert.equal(selectedPairUfrag(pair('in-progress')), null);
+});
+
+test('without a transport report, nomination is the selection', () => {
+ // Firefox's shape: no transport, `selected` or `nominated` on the
+ // pair itself. The fallback has to stay, or the proof never lands
+ // there at all.
+ assert.equal(selectedPairUfrag([
+ { type: 'candidate-pair', id: 'P', state: 'succeeded', nominated: true, localCandidateId: 'L' },
+ { type: 'local-candidate', id: 'L', usernameFragment: 'ff1' },
+ ]), 'ff1');
+});
+
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.
@@ -314,7 +361,7 @@ test('the reader tells "no such pair" from "a pair with no fragment"', () => {
// between polls is the right interval for a real ICE restart and the
// wrong one for a test, and the interval is not what is under test here.
const prover = new Function(
- 'state', 'RENEW_POLL',
+ 'state', 'pcs', 'RENEW_POLL',
[lift(src, 'proveRenewed'), lift(src, 'renewedUfrag'), lift(src, 'selectedPairUfrag'),
lift(src, 'statsList'), lift(src, 'sleep')].join('\n') +
'\nreturn { proveRenewed };',
@@ -335,17 +382,91 @@ function fakeStatsPC(reports) {
};
}
+// bounded runs a poll against a hard cap, so that a proof which stops
+// at its own deadline is a pass and one that does not is a *failure*
+// rather than a test run that never ends. The cap's own timer is
+// unref'd, and `stop` is what ends the runaway poll behind it: an
+// unbounded loop with a sleep in it holds the event loop open for ever,
+// so without this the assertion would fail and the process would still
+// never exit (round-1 review, item 10).
+function bounded(promise, ms, stop) {
+ return Promise.race([promise, new Promise((resolve) => {
+ const t = setTimeout(() => resolve('NEVER STOPPED'), ms);
+ if (t.unref) t.unref();
+ })]).finally(stop);
+}
+
test('the proof polls until the restart lands', async () => {
const pc = fakeStatsPC([report('old0'), report('old0'), report('new1')]);
- const { proveRenewed } = prover({ stopped: false }, 10);
- const got = await proveRenewed({ pc, desc: 1 }, 'old0', 0, Date.now() + 5000);
+ const entry = { pc, desc: 1 };
+ const st = { stopped: false };
+ const { proveRenewed } = prover(st, new Map([['peer', entry]]), 10);
+ const got = await bounded(proveRenewed('peer', entry, 'old0', 0, Date.now() + 5000),
+ 3000, () => { st.stopped = true; });
assert.equal(got, 'new1');
assert.ok(pc.asked() >= 3, 'it must have kept asking: ' + pc.asked());
});
test('the proof gives up at its deadline, and says so with null', async () => {
const pc = fakeStatsPC([report('old0')]);
- const { proveRenewed } = prover({ stopped: false }, 10);
- const got = await proveRenewed({ pc, desc: 1 }, 'old0', 0, Date.now() + 60);
+ const entry = { pc, desc: 1 };
+ const st = { stopped: false };
+ const { proveRenewed } = prover(st, new Map([['peer', entry]]), 10);
+ const got = await bounded(proveRenewed('peer', entry, 'old0', 0, Date.now() + 60),
+ 3000, () => { st.stopped = true; });
assert.equal(got, null, 'a restart that never landed must not read as renewed');
});
+
+test('the proof stops when the connection is replaced under it', async () => {
+ // A `pair`, a roster or an unreachable target rebuilds the
+ // connection: the entry in `pcs` is a different object, and thirty
+ // more seconds of polling the old one prove nothing about either.
+ const pc = fakeStatsPC([report('old0')]);
+ const entry = { pc, desc: 1 };
+ const pcs = new Map([['peer', { pc: fakeStatsPC([report('new1')]), desc: 9 }]]);
+ const st = { stopped: false };
+ const { proveRenewed } = prover(st, pcs, 10);
+ const got = await bounded(proveRenewed('peer', entry, 'old0', 0, Date.now() + 30000),
+ 3000, () => { st.stopped = true; });
+ assert.equal(got, null);
+ assert.equal(pc.asked(), 0, 'it must not even have asked the connection that is gone');
+});
+
+// ── §5.3 step 3's watchdog ────────────────────────────────────────────
+//
+// Round-1 review, item 1. The must-succeed-by exists for one path: the
+// fetch keeps failing, the old credential's deadline passes, and the
+// connections still on it are owed the sentence. `ice.fetchedAt` only
+// advances on a *successful* fetch, so on exactly that path it is still
+// the expiring credential's own stamp — and a watchdog that exempted
+// "built at or after ice.fetchedAt" exempted every connection in the
+// call and marked nothing, ever. The stamp is captured before the fetch
+// and passed in instead.
+const watchdog = new Function(
+ 'state', 'pcs', 'renewals', 'markRelayExpired',
+ lift(src, 'expireUnrenewed') + '\nreturn { expireUnrenewed };',
+);
+
+test('the must-succeed-by marks the connections that credential was about, and only those', () => {
+ const expiring = 1000; // the fetch stamp of the credential that ran out
+ const pcs = new Map([
+ ['on-it', { listAt: expiring }],
+ ['older', { listAt: 400 }],
+ ['joined-since', { listAt: expiring + 1 }],
+ ['restarting', { listAt: expiring }],
+ ['already-renewed', { listAt: expiring }],
+ ]);
+ const renewals = new Map([
+ ['restarting', { running: true, renewed: false }],
+ ['already-renewed', { running: false, renewed: true }],
+ ]);
+ const marked = [];
+ const { expireUnrenewed } = watchdog(
+ { stopped: false }, pcs, renewals, (id) => marked.push(id),
+ );
+
+ expireUnrenewed(expiring);
+
+ assert.deepEqual(marked.sort(), ['older', 'on-it'],
+ 'a connection built on the expiring credential, with nothing running, is the whole point');
+});