room.js: codex round 1 - renewal bound to the credential it renews
S3-1: expireUnrenewed exempted any connection whose `renewed` flag was true, and nothing ever cleared it. One successful renewal exempted a connection from every later deadline, so a second credential could expire in silence. The watchdog reads entry.listAt instead — the stamp of the list a connection is actually on, which moves only on proof. S3-2: retryRenewal fetched for the whole call, renewed only its own tile, and reset the shared timers on the way past, leaving every other connection on a credential about to die. renewOutdated hands a list that has just arrived to every connection still on an older one, and both the scheduled renewal and a tile's Retry go through it. S3-3: with no baseline fragment — including after a rejected getStats — any succeeded selected pair counted as proof, and '' passed a !== null check, so a connection could be stamped onto a credential it had never gathered on. No baseline is no proof now, and '' is not evidence: the poller takes its own baseline from the first reading after the restart and then requires a different one, or stays unproven. S3-4: a successful fetch followed by one failed restart near the old deadline reported the credential expired, after one attempt of three, with the replacement in hand. renewOne has no deadline: a fresh credential buys all three restarts and the verdict is stuck. Expired belongs to the watchdog, the only place that knows the other case. S3-5: the gate asserted the client's own cached list length, which would read the same with iceServers: [] in every constructor. The connection trap now records every configuration installed — count, credentialed count and an FNV-1a digest, never a credential — and the scenarios assert a real connection received the two-entry list, that none was left on the empty one, and that the renewal installed a different credential. §13's successful-fetch/failed-restart ending gets its own scenario, icestuck: three attempts, stuck, not expired. S3-6: a recording slog.Handler replaces the substring search — exactly one INFO record with exactly one attribute. S3-7: one test exhausts the bucket through POST /signal and then asks /ice; another sends an oversized room id and asserts the log names it invalid and that no bucket was minted. W3-7: per-peer candidate counts (gathered, posted, received, added, pending) in state() and diagnose(). W3-12: a margin under a second is declined rather than scheduled, so a development TTL cannot turn the client into a 1 Hz loop against its own server; state().ice.schedulable says so. And the residual flake is settled, by the counters, in three losing runs: one browser gathers no ICE candidate at all and never leaves `gathering`, while the other gathers, posts every one, and adds every one it receives. There is nothing left for the client to do on either side. It is the environment symptom the Makefile already documents, reaching one Chromium context at a time, and it correlates with load — every losing run took 560-590s against a healthy 318-420s. 63 node tests, each new one verified failing against the unfixed source. make browser twice: 26 of 27 with staletarget lost to the flake, then 27/27 in 417.65s.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CzvnuKhLzudvq7jZ7SvVt2
8 files changed,
+769
−78
docs/superpowers/copy/2026-09-10-slice-2-strings.md+1 −1docs/superpowers/reviews/2026-09-10-slice-3-record.md+82 −6internal/meet/app_relay_test.go+64 −6internal/meet/static/room.js+143 −49internal/meettest/browser_test.go+1 −0internal/meettest/ice_test.go+86 −4test/browser/call.mjs+174 −3test/browser/negotiation.test.mjs+218 −9
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 cc6ce1f..f730c84 100644 |
| --- a/docs/superpowers/copy/2026-09-10-slice-2-strings.md |
| +++ b/docs/superpowers/copy/2026-09-10-slice-2-strings.md |
| @@ -194,7 +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 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. Codex round 1 (S3-4) tightened which of the two a person sees: with a replacement credential in hand this is now the *only* possible sentence, where the old deadline could previously rename it "expired" after a single attempt. | |
| | 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/docs/superpowers/reviews/2026-09-10-slice-3-record.md b/docs/superpowers/reviews/2026-09-10-slice-3-record.md| index fd47e47..d0ad91c 100644 |
| --- a/docs/superpowers/reviews/2026-09-10-slice-3-record.md |
| +++ b/docs/superpowers/reviews/2026-09-10-slice-3-record.md |
| @@ -136,6 +136,19 @@ full diff. |
| are unexported, so the tripwire reads the ratio off the wire, which |
| is the form the client actually depends on). Verified against a |
| deliberately moved constant. |
| +- **A margin too small to schedule on. Decided, not carried.** A |
| + development-only `X-Meet-Test-Ice-TTL` under about six seconds leaves |
| + a renew-to-expiry margin below a second, and the client's own |
| + one-second floor under the renewal delay would then turn that list |
| + into a fetch roughly once a second for as long as the tab stayed open |
| + — a client that can be made to hammer its own server, even if only a |
| + developer can ask it to. The choice was between clamping the client |
| + and recording a gate rule that no scenario may use such a TTL; the |
| + Codex fix wave **clamped the client**, because a rule that lives only |
| + in a document is a rule the next scenario breaks. `schedulable()` is |
| + false below a one-second margin: nothing is timed, and |
| + `state().ice.schedulable` says so rather than the client looking idle |
| + for no reason. Node-tested (W3-12). |
| - **The residual gate flake, `connectionState: "new"`.** Roughly one |
| full `make browser` run in three loses one scenario — a different one |
| each time — always at the first pairing, always with the same |
| @@ -166,17 +179,59 @@ full diff. |
| already used for its own `getStats` walk, now installed on every |
| context. It changes no assertion. |
| - Round 3 also closed the likeliest client-side cause the signature |
| - admits: `createPC` registered the connection before attaching its |
| + **What the fix-round-3 diagnostic then showed** (whole-branch review |
| + W3-7, from a lost `playfallback` run and confirmed by a lost |
| + `staletarget` one): negotiation is *not* where it stops. Both sides |
| + report a local offer and a remote answer applied, `signalingState` |
| + stable, two `sendrecv` transceivers, and live tracks on the senders — |
| + and `iceConnectionState: "new"` all the same, with `iceGatheringState` |
| + `complete` on one side and still `gathering` on the other. So the |
| + fault is inside ICE, after a negotiation that finished properly, and |
| + the reading in the paragraph above — "nothing was ever offered" — is |
| + wrong for these runs. The Codex fix wave adds the next four numbers, |
| + per peer, to `state()` and to `diagnose()`: candidates gathered |
| + (`onicecandidate`), candidates posted (`send` of type `candidate`), |
| + candidates received, candidates added (`addIceCandidate`), and the |
| + pending buffer's length. Counts only — an address is not something |
| + the client publishes. Between them they say which of the four ways |
| + ICE can stall this is, in one losing run. |
| + |
| + Round 3 also closed the likeliest client-side cause the *earlier* |
| + reading admitted: `createPC` registered the connection before attaching its |
| listeners (so a `negotiationneeded` fired by `addTrack` could be lost |
| outright, leaving exactly a `stable`/`new` connection that never |
| offers), and its recvonly fallback was all-or-nothing, so a browser |
| that came down `startLocal`'s ladder to audio alone offered no video |
| m-line and the peer's tile could never get a frame. Whether either was |
| - the flake is unproved; both were real. **Destination if it survives: |
| - the next diagnostics pass** — run the gate on CI hardware and see |
| - whether the rate changes, then read the `pcState` lines from a losing |
| - run before touching the client again. |
| + the flake is unproved; both were real, and W3-7's reading now says |
| + neither was it. |
| + |
| + **Settled, in the Codex fix wave itself.** The counts were read from |
| + three losing runs — `ice`, `icerenewal` and `staletarget`, on separate |
| + gate runs — and all three say the same thing: |
| + |
| + | side | gathered | posted | received | added | pending | gatheringState | |
| + | --- | --- | --- | --- | --- | --- | --- | |
| + | A | 4 | 4 | 0 | 0 | 0 | complete | |
| + | B | 0 | 0 | 4 | 4 | 0 | **gathering** | |
| + |
| + One browser gathers **no ICE candidate at all** and never leaves |
| + `gathering`. The other gathers, posts every one it gathers, and the |
| + first adds every one it receives. There is nothing for the client to |
| + do differently: it sends what the browser gives it and applies what |
| + arrives. The stall is inside one Chromium's own candidate gathering, |
| + which is the Makefile's documented environment symptom — "Chromium |
| + gathers zero ICE candidates and every call fails with frames stuck at |
| + 0" — reaching one context at a time instead of all of them. |
| + |
| + It correlates with load: every losing run of this wave took 560-590 s |
| + against a healthy 318-420 s, one hit `go test`'s own ten-minute |
| + timeout, and the wave ran while a whole-branch reviewer was working |
| + the same tree. **No further client change is owed.** What is owed, if |
| + anyone wants it gone: run the gate where nothing else is running, and |
| + give `assertPair` a failure message that names `gathered: 0` for what |
| + it is, so the next person spends a minute on it rather than a |
| + fix round. |
| --- |
| @@ -219,3 +274,24 @@ above, after the deploy that enables it. |
| slice 2's record already gives. The residual flake in §3 above is why a |
| red run here is worth a second run before it is worth debugging, exactly |
| as slice 2's `rosterpair` flake was. |
| + |
| +## Codex round 1 |
| + |
| +`codex exec` read the branch at `6b316cc` and asked for changes: no P1, |
| +five P2 and two P3. The whole-branch reviewer read the same tree |
| +concurrently and numbered its own findings W3-1..W3-12, of which |
| +W3-1..W3-6 are the same defects. One commit answered all of them. |
| + |
| +| Finding | What it was | Disposition | Test that would have caught it | |
| +| --- | --- | --- | --- | |
| +| S3-1 / W3-1 (P2) | `expireUnrenewed` exempted any connection with `renewed=true`, and nothing ever cleared that flag: one successful renewal exempted a connection from every later deadline, so a second credential could expire in silence | Fixed. The watchdog reads `entry.listAt` — the stamp of the list a connection is actually on, which moves only on proof — and exempts only a connection on a list newer than the expiring one | `a renewal in an earlier cycle does not exempt a connection from a later one` | |
| +| S3-2 / W3-2 (P2) | `retryRenewal` fetched for the whole call and renewed only its own tile, while resetting the shared timers: every other connection stayed on a credential about to die | Fixed. `renewOutdated(expiring)` hands a list that has just arrived to every connection still on an older one, and both the scheduled renewal and a tile's Retry go through it | `a Retry on one tile renews every connection the fetch re-credentialed` | |
| +| S3-3 / W3-3 (P2) | With no baseline fragment — including after a rejected `getStats` — any succeeded selected pair counted as proof, and `''` passed a `!== null` check; a connection could be stamped onto a credential it had never gathered on | Fixed. No baseline is no proof, and `''` is not evidence. The poller takes its own baseline from the first reading after the restart and then requires a different one; if none can be read the restart stays unproven | `nothing recorded before the restart is not proof of anything`, `a connection that never moves is unproven, whatever it started with` | |
| +| S3-4 / W3-4 (P2) | A successful fetch followed by one failed restart near the old deadline reported "expired" after one attempt of three, with a replacement credential in hand | Fixed. `renewOne` no longer takes a deadline: with a fresh credential it spends all three restarts and its verdict is always `stuck`. `expired` belongs to the watchdog, the only place that knows the other case | `a fresh credential buys three restarts and the verdict is stuck, never expired`, and the `icestuck` scenario | |
| +| S3-5 / W3-5, W3-6 (P2) | The gate asserted the client's cached list length, not that any connection received it: `ice` would pass with `iceServers: []` in every constructor, and `icerenewal` with `setConfiguration` deleted | Fixed. The connection trap records every configuration installed — count, credentialed count, and an FNV-1a digest, never a credential — and the two scenarios assert that a real connection got the two-entry list and that the renewal installed a *different* one. The §13 successful-fetch/failed-restart ending gets its own scenario, `icestuck` | `ice`, `icerenewal`, `icestuck` | |
| +| S3-6 (P3) | The startup-logging test matched substrings, so duplicate lines, a WARN, or an extra decoded attribute would all have passed | Fixed. A recording `slog.Handler` keeps whole records; the test asserts exactly one INFO record with exactly one attribute, `regions=1` | `TestBuildLogsOneINFOLineWithTheRegionCountAndNotTheKey`, verified against a WARN and against an extra attribute | |
| +| S3-7 (P3) | The burst test exercised `/ice` alone, so a limiter of its own would have passed it; the malformed-room case passed with `validSqid` removed, because the liveness gate refuses that room a step later | Fixed. One test exhausts the budget entirely through `POST /signal` and then asks `/ice`; another sends an oversized room id and asserts the log names it `invalid` and that no bucket was minted | `TestIceSpendsTheSignalEndpointsOwnBucket`, `TestIceNeverLogsAMalformedRoomOrMintsABucketForOne`, both verified against the guards removed | |
| +| W3-7 | The residual flake's `pcState` showed negotiation complete and ICE at `new`, so the fault is in ICE | Instrumented, then **settled** — see the flake entry above. Per-peer candidate counts (gathered, posted, received, added, pending) in `state()` and `diagnose()`; three losing runs then showed one browser gathering zero candidates | no assertion changed | |
| +| W3-12 | A sub-six-second development TTL leaves a margin under a second, which the delay floor would turn into a 1 Hz fetch loop | Fixed, not carried: `schedulable()` declines below a one-second margin and `state().ice.schedulable` says so | `a margin under a second is not a schedule` | |
| + |
| +Commit: `6b316cc..30bd06a`. |
diff --git a/internal/meet/app_relay_test.go b/internal/meet/app_relay_test.go| index f31b167..b851233 100644 |
| --- a/internal/meet/app_relay_test.go |
| +++ b/internal/meet/app_relay_test.go |
| @@ -7,6 +7,7 @@ import ( |
| "log/slog" |
| "path/filepath" |
| "strings" |
| + "sync" |
| "testing" |
| "amadan.net/rastrillo/rastrillo/db" |
| @@ -55,7 +56,8 @@ func TestSetRelayFlipsRelayConfigured(t *testing.T) { |
| // survived. |
| func TestBuildLogsOneINFOLineWithTheRegionCountAndNotTheKey(t *testing.T) { |
| var buf bytes.Buffer |
| - logger := slog.New(slog.NewTextHandler(&buf, nil)) |
| + rec := &recordingHandler{inner: slog.NewTextHandler(&buf, nil)} |
| + logger := slog.New(rec) |
| cfg := minimalConfig(t) |
| cfg.Logger = logger |
| cfg.Relay = relayVector(t) |
| @@ -70,15 +72,71 @@ func TestBuildLogsOneINFOLineWithTheRegionCountAndNotTheKey(t *testing.T) { |
| if !a.relayConfigured() || len(a.relayRegions()) != 1 { |
| t.Fatalf("build did not store the parsed relay: configured=%v regions=%d", a.relayConfigured(), len(a.relayRegions())) |
| } |
| - log := buf.String() |
| - if !strings.Contains(log, "meet: relay configured") || !strings.Contains(log, "regions=1") { |
| - t.Fatalf("want one INFO line naming the region count, got:\n%s", log) |
| - } |
| - if strings.Contains(log, "uMhwW4P") { |
| + // Records, not substrings. A substring search says nothing about how |
| + // many lines there were, what level they were at, or what else they |
| + // carried: two identical lines, a WARN, or a line that also logged a |
| + // decoded region would all have passed (Codex S3-6). |
| + var configured []slog.Record |
| + for _, r := range rec.records() { |
| + if r.Message == "meet: relay configured" { |
| + configured = append(configured, r) |
| + } |
| + } |
| + if len(configured) != 1 { |
| + t.Fatalf("want exactly one \"meet: relay configured\" record, got %d:\n%s", len(configured), buf.String()) |
| + } |
| + got := configured[0] |
| + if got.Level != slog.LevelInfo { |
| + t.Fatalf("the relay line is %s, not INFO", got.Level) |
| + } |
| + attrs := map[string]string{} |
| + got.Attrs(func(a slog.Attr) bool { |
| + attrs[a.Key] = a.Value.String() |
| + return true |
| + }) |
| + if len(attrs) != 1 || attrs["regions"] != "1" { |
| + t.Fatalf("want exactly one attribute, regions=1; got %v", attrs) |
| + } |
| + if log := buf.String(); strings.Contains(log, "uMhwW4P") { |
| t.Fatal("the relay key reached the log") |
| } |
| } |
| +// recordingHandler keeps every record whole so a test can ask how many |
| +// there were, at what level, and with which attributes — the three |
| +// questions a rendered line cannot answer. It forwards to a real |
| +// handler as well, so a failure can still print what was written. |
| +type recordingHandler struct { |
| + inner slog.Handler |
| + mu sync.Mutex |
| + kept []slog.Record |
| +} |
| + |
| +func (h *recordingHandler) Enabled(ctx context.Context, l slog.Level) bool { |
| + return h.inner.Enabled(ctx, l) |
| +} |
| + |
| +func (h *recordingHandler) Handle(ctx context.Context, r slog.Record) error { |
| + h.mu.Lock() |
| + h.kept = append(h.kept, r.Clone()) |
| + h.mu.Unlock() |
| + return h.inner.Handle(ctx, r) |
| +} |
| + |
| +func (h *recordingHandler) WithAttrs(as []slog.Attr) slog.Handler { |
| + return &recordingHandler{inner: h.inner.WithAttrs(as)} |
| +} |
| + |
| +func (h *recordingHandler) WithGroup(name string) slog.Handler { |
| + return &recordingHandler{inner: h.inner.WithGroup(name)} |
| +} |
| + |
| +func (h *recordingHandler) records() []slog.Record { |
| + h.mu.Lock() |
| + defer h.mu.Unlock() |
| + return append([]slog.Record(nil), h.kept...) |
| +} |
| + |
| // TestSetRelayIsTypeConsistentWithBuildsInitialStore: build() stores a |
| // nil []relay.Region into a.relay when no relay is configured, and |
| // SetRelay later stores a real []relay.Region into the same |
diff --git a/internal/meet/static/room.js b/internal/meet/static/room.js| index 22a8007..81cf123 100644 |
| --- a/internal/meet/static/room.js |
| +++ b/internal/meet/static/room.js |
| @@ -100,6 +100,16 @@ |
| // 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; |
| + // And the floor under the margin itself. A list whose renew time and |
| + // expiry are less than a second apart cannot be renewed on any |
| + // sensible schedule: the delay floor above would turn it into a fetch |
| + // roughly once a second for as long as the tab stayed open. The |
| + // server clamps what it will mint, so this can only arrive through |
| + // the development-only TTL header — but a client that answers a |
| + // nonsense schedule with a 1 Hz loop is a client that can be made to |
| + // hammer its own server, so it declines instead, and says so in |
| + // state().ice rather than looking idle for no reason (W3-12). |
| + const MIN_MARGIN = 1000; |
| const PEER_ID = /^[0-9a-f]{32}$/; |
| const HAS_SINK_ID = typeof HTMLMediaElement !== 'undefined' && |
| typeof HTMLMediaElement.prototype.setSinkId === 'function'; |
| @@ -926,6 +936,17 @@ |
| // reconcile reads it: a roster older than this cannot be talking |
| // about this connection. |
| bornAt: tick, |
| + // The four numbers that say where ICE stopped, and the one place |
| + // they exist: a connection whose negotiation completed and whose |
| + // transport never left `new` (the residual gate flake) is either |
| + // gathering nothing, posting nothing, receiving nothing, or |
| + // adding nothing, and only these tell those four apart. Counts, |
| + // never candidates: an address is not something this file |
| + // publishes. |
| + gathered: 0, |
| + posted: 0, |
| + received: 0, |
| + added: 0, |
| }; |
| pcs.set(id, entry); |
| @@ -953,6 +974,7 @@ |
| pc.addEventListener('icecandidate', ({ candidate }) => { |
| if (pcs.get(id) !== entry || !candidate) return; |
| + entry.gathered += 1; |
| send(id, 'candidate', { candidate: candidate.toJSON() }); |
| }); |
| @@ -1172,6 +1194,7 @@ |
| if (!entry) return; |
| const candidate = msg.payload && msg.payload.candidate; |
| if (!candidate) return; |
| + entry.received += 1; |
| // §4.6: a candidate whose seq precedes the current description's |
| // belongs to a description that has been replaced. |
| if (msg.seq && entry.descSeq && msg.seq < entry.descSeq) return; |
| @@ -1192,6 +1215,7 @@ |
| async function addCandidate(entry, candidate) { |
| try { |
| await entry.pc.addIceCandidate(candidate); |
| + entry.added += 1; |
| } catch (err) { |
| // The W3C reference swallows exactly this: an ignored offer's |
| // candidates cannot apply, and that is expected, not an error. |
| @@ -1455,7 +1479,7 @@ |
| // `renewAt` already behind us runs at once. |
| function scheduleRenewal() { |
| clearTimeout(renewTimer); |
| - if (state.stopped || !ice.expiresAt || !ice.servers.length) return; |
| + if (state.stopped || !schedulable()) return; |
| renewTimer = setTimeout(renew, Math.max(0, renewDelay() - (Date.now() - ice.fetchedAt))); |
| } |
| @@ -1503,7 +1527,7 @@ |
| // 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.expiresAt || !ice.servers.length) return; |
| + if (state.stopped || !schedulable()) 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 |
| @@ -1520,12 +1544,12 @@ |
| }); |
| } |
| - // 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 — 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 |
| + // renewAll is §5.3's sequence for the whole mesh: fetch, then hand |
| + // what came back to every connection still on the old list. Both the |
| + // deadline it arms the watchdog with — step 3's must-succeed-by, the |
| + // one honest signal, because the browser cannot see which credential |
| + // an allocation is using — and the stamp naming the credential that |
| + // deadline 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). |
| @@ -1534,23 +1558,36 @@ |
| renewing = true; |
| renewDue = true; |
| const expiring = ice.fetchedAt; |
| - const deadline = mustSucceedBy(); |
| - armMustSucceedBy(deadline, expiring); |
| + armMustSucceedBy(mustSucceedBy(), 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, deadline).catch((err) => { |
| - console.error('meet: renew', err); |
| - }); |
| - } |
| + renewOutdated(expiring); |
| } finally { |
| renewing = false; |
| } |
| } |
| + // renewOutdated hands a list that has just arrived to every |
| + // connection still on an older one. Every path that produces a list |
| + // comes through here — the scheduled renewal and a tile's Retry |
| + // alike — because a fetch is a fetch: one credential is minted for |
| + // the call, and renewing only the connection whose Retry happened to |
| + // make the request left every other connection on a credential about |
| + // to die, with the shared timers already reset out from under them |
| + // (Codex S3-2). |
| + function renewOutdated(expiring) { |
| + for (const id of Array.from(pcs.keys())) { |
| + const entry = pcs.get(id); |
| + if (!entry || entry.listAt > expiring) continue; |
| + renewOne(id).catch((err) => { |
| + console.error('meet: renew', err); |
| + }); |
| + } |
| + } |
| + |
| // armMustSucceedBy sets §5.3 step 3's verdict for the connections that |
| // have nothing running: with the fetch still failing a minute at a |
| // time, the old credential's deadline passes and their allocation dies |
| @@ -1575,12 +1612,22 @@ |
| function expireUnrenewed(expiring) { |
| if (state.stopped) return; |
| for (const [id, entry] of pcs) { |
| + // The one exemption that means anything: this connection is on a |
| + // list *newer* than the one that just expired, because it was |
| + // built on it or was proven onto it. `listAt` is that list's |
| + // stamp and it moves only on proof, so it answers "which |
| + // credential is this connection actually using" — which is the |
| + // question. A `renewed` flag answered "did this connection ever |
| + // renew, once, at any time", and stayed true through every later |
| + // failure: one successful renewal exempted a connection from |
| + // every deadline for the rest of the call, and nobody was ever |
| + // told the second credential had died (Codex S3-1). |
| 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; |
| + if (record && record.running) continue; |
| markRelayExpired(id); |
| } |
| } |
| @@ -1622,18 +1669,22 @@ |
| // 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, deadline) { |
| + // renewOne is reached only with a fresh credential in hand — every |
| + // caller has just fetched one — so the deadline plays no part in it. |
| + // Its whole budget is three restarts, and its verdict on failure is |
| + // always `stuck`: the credential is fine and the path is not. Cutting |
| + // the budget short at the old deadline and calling the result |
| + // "expired" was a sentence about the wrong thing, told after one |
| + // attempt of the three (Codex S3-4). `expired` belongs to the |
| + // watchdog, which is the only place that knows the other case: no |
| + // fresh credential arrived at all. |
| + async function renewOne(id) { |
| const entry = pcs.get(id); |
| if (!entry || state.stopped) return false; |
| const record = renewalRecord(id); |
| 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; |
| @@ -1664,8 +1715,7 @@ |
| requestReset(id); |
| await sleep(RENEW_WINDOW); |
| if (state.stopped || pcs.get(id) !== entry) return false; |
| - if (deadline && Date.now() > deadline) markRelayExpired(id); |
| - else markRenewalStuck(id); |
| + markRenewalStuck(id); |
| return false; |
| } |
| const proven = await proveRenewed(id, entry, oldUfrag, from, Date.now() + RENEW_WINDOW); |
| @@ -1679,27 +1729,17 @@ |
| 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) { |
| - ranOut = true; |
| - break; |
| - } |
| } |
| - if (ranOut) markRelayExpired(id); |
| - else markRenewalStuck(id); |
| + markRenewalStuck(id); |
| return false; |
| } finally { |
| record.running = false; |
| } |
| } |
| - // retryRenewal is the control §5.3 step 3 puts on an expired tile: a |
| - // fresh fetch, then step 2 again for that one connection. It passes no |
| - // 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. |
| + // retryRenewal is the control §5.3 step 3 puts on a tile in trouble: |
| + // a fresh fetch, then step 2 again — for this connection and for |
| + // every other one the fetch just re-credentialed. |
| // |
| // A fetch that fails is the case worth writing out. `restartIce` has |
| // already cleared the sentence off the tile, so returning here left a |
| @@ -1709,8 +1749,13 @@ |
| // Retry would have given it, which is the one thing left that might |
| // help. |
| async function retryRenewal(id, wasTrouble) { |
| + const expiring = ice.fetchedAt; |
| if (await fetchIce()) { |
| - await renewOne(id, 0); |
| + // Not just this one. The fetch this Retry made replaced the |
| + // credential every connection was using, and reset the shared |
| + // schedule with it, so every connection still on the old list is |
| + // renewed here (Codex S3-2). |
| + renewOutdated(expiring); |
| return; |
| } |
| if (wasTrouble) { |
| @@ -1726,15 +1771,25 @@ |
| // 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(id, entry, oldUfrag, from, deadline) { |
| + // The generation this connection was on before the restart, when it |
| + // could be read. When it could not, the first fragment readable |
| + // afterwards stands in for it, and the proof is then the ordinary |
| + // one: a *different* fragment after that. It is a weaker starting |
| + // point and it can honestly fail to prove a restart that did land — |
| + // but failing to prove costs a sentence and a Retry, where wrongly |
| + // proving is a call running on a credential nobody ever checked. |
| + let baseline = oldUfrag || ''; |
| for (;;) { |
| // 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, |
| + const reports = await statsList(entry.pc); |
| + const proven = renewedUfrag(reports, baseline, |
| entry.pc.signalingState, entry.desc > from); |
| if (proven !== null) return proven; |
| + if (!baseline) baseline = selectedPairUfrag(reports) || ''; |
| const left = deadline - Date.now(); |
| if (left <= 0) return null; |
| await sleep(Math.min(RENEW_POLL, left)); |
| @@ -1758,14 +1813,21 @@ |
| // so callers test `!== null`, never truthiness. |
| function renewedUfrag(reports, oldUfrag, signalingState, negotiated) { |
| if (!negotiated || signalingState !== 'stable') return null; |
| + // No baseline, no proof. Without knowing which generation this |
| + // connection was on, a selected pair that has not moved at all |
| + // looks exactly like one that has — and the old reading took that |
| + // as proof, cleared the warning, and stamped the connection as |
| + // renewed onto a credential it had never gathered on (Codex S3-3). |
| + // A getStats that rejected before the restart arrived here the same |
| + // way. The caller establishes a baseline from the first reading it |
| + // can take *after* the restart and comes back; if none can be read, |
| + // the restart stays unproven, which is the honest answer. |
| + if (!oldUfrag) return null; |
| const ufrag = selectedPairUfrag(reports); |
| - if (ufrag === null) return null; |
| - // Nothing was recorded before the restart — no pair yet, or a |
| - // browser that will not name the fragment — so §5.3 step 2's |
| - // fallback applies: any succeeded selected pair after the restart's |
| - // answer counts. |
| - if (!oldUfrag) return ufrag; |
| - return ufrag && ufrag !== oldUfrag ? ufrag : null; |
| + // And a pair the browser will not name is not evidence either: '' |
| + // says "there is a pair", not "there is a new one". |
| + if (!ufrag) return null; |
| + return ufrag !== oldUfrag ? ufrag : null; |
| } |
| // selectedPairUfrag reads one getStats report and answers the ICE |
| @@ -1850,6 +1912,10 @@ |
| function send(to, type, payload) { |
| if (!state.peer) return; |
| + if (type === 'candidate') { |
| + const entry = pcs.get(to); |
| + if (entry) entry.posted += 1; |
| + } |
| // 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 |
| @@ -2512,8 +2578,14 @@ |
| // 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.expiresAt && |
| - Date.now() - ice.fetchedAt >= renewDelay()); |
| + return !!(schedulable() && Date.now() - ice.fetchedAt >= renewDelay()); |
| + } |
| + |
| + // schedulable: there is a credential, there are servers to renew it |
| + // for, and the margin the server left is one a schedule can be built |
| + // on. Pure enough to gate without a browser. |
| + function schedulable() { |
| + return !!(ice.expiresAt && ice.servers.length && ice.margin >= MIN_MARGIN); |
| } |
| // applySink routes a remote tile's audio to the speaker the holding |
| @@ -2695,6 +2767,20 @@ |
| } |
| } |
| + function candidateState() { |
| + const out = {}; |
| + for (const [id, entry] of pcs) { |
| + out[id] = { |
| + gathered: entry.gathered, |
| + posted: entry.posted, |
| + received: entry.received, |
| + added: entry.added, |
| + pending: entry.pending.length, |
| + }; |
| + } |
| + return out; |
| + } |
| + |
| // renewalState is a plain object rather than a Map: the gate reads it |
| // by peer id, and this is the one shape that survives being handed |
| // straight out of page.evaluate(). |
| @@ -2746,6 +2832,11 @@ |
| frames: new Map(frames), |
| signaling, |
| connection, |
| + // Where ICE got to, per connection: gathered, posted, |
| + // received, added, and what is still buffered waiting for a |
| + // remote description. Four counts and a length — never an |
| + // address. |
| + candidates: candidateState(), |
| // §5.2 and §5.3, for the gate. The three clocks are |
| // milliseconds since the epoch, converted once in fetchIce, so |
| // they compare with Date.now(); `fetches` counts the fetches |
| @@ -2761,6 +2852,9 @@ |
| // is no relay to fail and none is shown (§5.5). |
| warning: !!(ice.failedSince && relayConfigured), |
| servers: ice.servers.length, |
| + // False when the list carries a margin too small to schedule |
| + // on: nothing is timed, and that is deliberate (W3-12). |
| + schedulable: schedulable(), |
| }, |
| // Per connection: how many restarts §5.3 step 2 has spent, and |
| // whether one of them was proven. `ufrag` is the ICE username |
diff --git a/internal/meettest/browser_test.go b/internal/meettest/browser_test.go| index f4ad9ec..43dff5e 100644 |
| --- a/internal/meettest/browser_test.go |
| +++ b/internal/meettest/browser_test.go |
| @@ -79,6 +79,7 @@ var scenarios = []string{ |
| "ice", |
| "icerenewal", |
| "icefail", |
| + "icestuck", |
| "guest", |
| "removed", |
| "allowlist", |
diff --git a/internal/meettest/ice_test.go b/internal/meettest/ice_test.go| index 7f46bd5..702c5d8 100644 |
| --- a/internal/meettest/ice_test.go |
| +++ b/internal/meettest/ice_test.go |
| @@ -408,10 +408,59 @@ func TestIceRefusesAnUnboundSessionBeforeTheStoreOrTheLimiter(t *testing.T) { |
| } |
| } |
| -// TestIceRateLimitsOneSessionsBurst proves /ice spends the same |
| -// session+room bucket signal_test.go's own rate-limit test exercises |
| -// for POST /signal — sharing one limiter, per item 1, not a limiter of |
| -// its own. |
| +// TestIceSpendsTheSignalEndpointsOwnBucket is the shared-bucket claim |
| +// itself, which the burst test below cannot make: a limiter of /ice's |
| +// own would pass that one exactly as a shared one does. So this |
| +// exhausts the budget entirely through POST /signal and then asks /ice |
| +// for a list — one request, on a participation that is admitted and |
| +// would otherwise be answered 200 (Codex S3-7). |
| +func TestIceSpendsTheSignalEndpointsOwnBucket(t *testing.T) { |
| + x, sqid, _ := memberWithMeeting(t) |
| + if err := x.ctl.SetRelay(relayContract(t)); err != nil { |
| + t.Fatal(err) |
| + } |
| + c := x.browser(t) |
| + c.signIn("paul@example.com") |
| + s := c.stream("/m/" + sqid + "/stream") |
| + defer s.close() |
| + peer, _ := s.hello() |
| + if resp, b := c.signal(sqid, peer, `{"type":"join"}`); resp.StatusCode != http.StatusOK { |
| + t.Fatalf("member join: %d %s", resp.StatusCode, b) |
| + } |
| + // One list before the burst, so the 429 below cannot be a refusal |
| + // this participation would have got anyway. |
| + if rec := c.getStream("/m/" + sqid + "/ice?peer=" + peer); rec.Code != http.StatusOK { |
| + t.Fatalf("ice before the burst: %d %s", rec.Code, rec.Body.String()) |
| + } |
| + |
| + // Now spend the budget on the *other* endpoint. A candidate to a |
| + // peer that is not there is refused on its merits, and every refusal |
| + // still costs a token: the limiter runs in front of the handler. |
| + spent := 0 |
| + for i := 0; i < 200; i++ { |
| + resp, _ := c.signal(sqid, peer, `{"type":"candidate","to":"`+strings.Repeat("b", 32)+`","seq":1,"gen":1,"payload":{}}`) |
| + spent++ |
| + if resp.StatusCode == http.StatusTooManyRequests { |
| + break |
| + } |
| + } |
| + if spent >= 200 { |
| + t.Fatal("200 signal posts never hit the rate limit, so nothing was exhausted") |
| + } |
| + |
| + rec := c.getStream("/m/" + sqid + "/ice?peer=" + peer) |
| + if rec.Code != http.StatusTooManyRequests { |
| + t.Fatalf("ice after %d signal posts exhausted the bucket: %d %s — /ice has a limiter of its own", |
| + spent, rec.Code, rec.Body.String()) |
| + } |
| + if rec.Header().Get("Retry-After") == "" { |
| + t.Fatal("429 without Retry-After") |
| + } |
| +} |
| + |
| +// TestIceRateLimitsOneSessionsBurst proves /ice is behind the limiter at |
| +// all: a hundred requests in a row are refused before the hundredth. |
| +// The shared-bucket half is the test above. |
| func TestIceRateLimitsOneSessionsBurst(t *testing.T) { |
| x, sqid, _ := memberWithMeeting(t) |
| if err := x.ctl.SetRelay(relayContract(t)); err != nil { |
| @@ -598,3 +647,36 @@ func TestIceNeverLogsAMalformedPeerVerbatim(t *testing.T) { |
| t.Fatalf("the malformed peer reached the log:\n%s", buf.String()) |
| } |
| } |
| + |
| +// TestIceNeverLogsAMalformedRoomOrMintsABucketForOne is the room half of |
| +// the same guard, and it is written to fail if validSqid is taken out of |
| +// the handler. The old assertion could not: it checked only the status, |
| +// and an unchecked sqid is refused a step later anyway when |
| +// LiveForSession finds no participation there (Codex S3-7). Two things |
| +// only the shape guard can give: a log line that names the room as |
| +// `invalid` rather than repeating four kilobytes of someone else's |
| +// text, and a limiter that mints no bucket for a room id that cannot |
| +// exist — the key is session+room, so an unchecked one is an unbounded |
| +// space of keys anybody can fill. |
| +func TestIceNeverLogsAMalformedRoomOrMintsABucketForOne(t *testing.T) { |
| + var buf bytes.Buffer |
| + x := newHarnessWithLogger(t, meet.EnvDevelopment, slog.New(slog.NewTextHandler(&buf, &slog.HandlerOptions{Level: slog.LevelDebug})), "paul@example.com") |
| + c := x.browser(t) |
| + c.signIn("paul@example.com") |
| + |
| + needle := "forged-room-line\n" + strings.Repeat("q", 4096) |
| + rec := c.getStream("/m/" + url.PathEscape(needle) + "/ice?peer=" + strings.Repeat("a", 32)) |
| + if rec.Code != http.StatusNotFound { |
| + t.Fatalf("malformed room: %d %s", rec.Code, rec.Body.String()) |
| + } |
| + log := buf.String() |
| + if strings.Contains(log, "forged-room-line") || strings.Contains(log, strings.Repeat("q", 64)) { |
| + t.Fatalf("the malformed room reached the log:\n%s", log) |
| + } |
| + if strings.Contains(log, "ice refused") && !strings.Contains(log, "room=invalid") { |
| + t.Fatalf("a refusal was logged without naming the room `invalid`:\n%s", log) |
| + } |
| + if n := x.ctl.SignalLimiterSize(); n != 0 { |
| + t.Fatalf("the limiter minted a bucket for a room id that cannot exist: %d keys", n) |
| + } |
| +} |
diff --git a/test/browser/call.mjs b/test/browser/call.mjs| index 11e4487..6a405e1 100644 |
| --- a/test/browser/call.mjs |
| +++ b/test/browser/call.mjs |
| @@ -237,6 +237,7 @@ function state(page) { |
| frames: Object.fromEntries(s.frames), |
| signaling: Object.fromEntries(s.signaling), |
| connection: Object.fromEntries(s.connection), |
| + candidates: s.candidates, |
| // §5.2 and §5.3. Both are plain objects in the page — `ice` |
| // carries counts and three clocks in milliseconds since the |
| // epoch, `renewals` is keyed by peer id — so they survive |
| @@ -322,6 +323,11 @@ async function diagnose(page, label) { |
| connection: Object.fromEntries(s.connection), |
| ice: s.ice, |
| renewals: s.renewals, |
| + // Where ICE got to, per peer. The pcState above says |
| + // negotiation finished; these say whether anything was |
| + // ever gathered, sent, received or added afterwards, which |
| + // is the next question and the one nothing else answers. |
| + candidates: s.candidates, |
| } |
| : null, |
| tiles, |
| @@ -470,6 +476,7 @@ async function biteCut(cut, ms = 20000) { |
| // out before a new one landed. |
| 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.'; |
| // iceRoute is the instrument the three ICE scenarios share. Every |
| // GET /m/{sqid}/ice from this context goes through it, which buys three |
| @@ -541,15 +548,56 @@ const pcTap = () => { |
| // Wrapping the proxy in another proxy would work and reads like a bug. |
| if (window.__pcs) return; |
| window.__pcs = []; |
| + // And every ICE configuration the page actually installs, which is a |
| + // different fact from the list room.js says it holds: state().ice |
| + // would read the same if the constructor were handed an empty array |
| + // and setConfiguration were deleted (Codex S3-5). Recorded by shape |
| + // and by digest — a 32-bit FNV-1a of the usernames and credentials — |
| + // so the gate can say "a different credential is installed" without |
| + // anything anywhere printing one. |
| + window.__iceConfigs = []; |
| + const digest = (servers) => { |
| + const text = servers.map((v) => (v.username || '') + '|' + (v.credential || '')).join('\n'); |
| + let h = 2166136261; |
| + for (let i = 0; i < text.length; i++) { |
| + h ^= text.charCodeAt(i); |
| + h = Math.imul(h, 16777619); |
| + } |
| + return (h >>> 0).toString(16); |
| + }; |
| + const note = (at, config) => { |
| + const servers = (config && config.iceServers) || []; |
| + window.__iceConfigs.push({ |
| + at, |
| + servers: servers.length, |
| + credentialed: servers.filter((v) => v.credential).length, |
| + digest: digest(servers), |
| + }); |
| + }; |
| window.RTCPeerConnection = new Proxy(Real, { |
| construct(target, args) { |
| const pc = Reflect.construct(target, args); |
| window.__pcs.push(pc); |
| + note('construct', args[0]); |
| + const real = pc.setConfiguration && pc.setConfiguration.bind(pc); |
| + if (real) { |
| + pc.setConfiguration = (config) => { |
| + note('setConfiguration', config); |
| + return real(config); |
| + }; |
| + } |
| return pc; |
| }, |
| }); |
| }; |
| +// iceConfigs is what the trap recorded: one entry per configuration the |
| +// page installed, in order, with a digest standing in for the |
| +// credentials it carried. |
| +function iceConfigs(page) { |
| + return page.evaluate(() => window.__iceConfigs || []); |
| +} |
| + |
| // selectedUfrags is §5.3 step 2's reading taken independently of the |
| // client's: the transport names the selected pair, the pair names its |
| // local candidate, and the candidate carries the ICE generation's |
| @@ -592,8 +640,9 @@ function warningRegion(page) { |
| } |
| // tileNotes is every sentence currently under a tile. There are only |
| -// three it can ever be — the expired credential and the two failure |
| -// sentences — so the whole list goes into the assertion's message. |
| +// four it can ever be — the expired credential, the renewal that would |
| +// not land, and the two failure sentences — so the whole list goes into |
| +// the assertion's message. |
| function tileNotes(page) { |
| return page.evaluate(() => |
| Array.from(document.querySelectorAll('#tiles figure .meet-tile-note')) |
| @@ -830,7 +879,27 @@ const run = { |
| for (const [page, label, seen] of [[pageA, 'member A', iceA], [pageB, 'member B', iceB]]) { |
| const s = await state(page); |
| check(s.ice.servers === 2, |
| - label + ' built its connections with the two servers /ice minted (servers=' + s.ice.servers + ')'); |
| + label + ' holds the two servers /ice minted (servers=' + s.ice.servers + ')'); |
| + // And the connections were actually built with them. The line |
| + // above is the client's own cached count and would read the same |
| + // if every RTCPeerConnection were constructed with an empty list; |
| + // this is what the constructor received (Codex S3-5). |
| + const configs = await iceConfigs(page); |
| + const built = configs.filter((c) => c.at === 'construct'); |
| + const floor = built.filter((c) => c.servers === 0); |
| + const installed = configs.filter((c) => c.at === 'setConfiguration' && c.servers === 2); |
| + check(built.length > 0 && configs.some((c) => c.servers === 2 && c.credentialed === 1), |
| + label + ' put the two-entry list, one credentialed, into a real connection (' + |
| + JSON.stringify(configs) + ')'); |
| + // Built with it, or given it before it could need it. Both are |
| + // §5.2: the first roster waits for the fetch, but a `peer` that |
| + // lands inside that fetch builds on the floor's empty list and |
| + // adopts the real one at its next quiet moment. What must never |
| + // happen is a connection left on the empty list — which is what |
| + // the client's own cached count could not tell anyone. |
| + check(floor.length <= installed.length, |
| + label + ' left no connection on the empty list (' + floor.length + ' built on the floor, ' + |
| + installed.length + ' given the list afterwards)'); |
| check(s.ice.fetches === 1, |
| label + ' fetched the list once for the call, not once per connection (fetches=' + s.ice.fetches + ')'); |
| check(s.ice.expiresAt > Date.now() && s.ice.renewAt > Date.now() && s.ice.renewAt < s.ice.expiresAt, |
| @@ -954,6 +1023,26 @@ const run = { |
| const after = await selectedUfrags(pageA); |
| check(after.length === 1 && after[0] && after[0] !== before[0], |
| "the gate's own walk of the selected pair agrees (" + JSON.stringify(before) + ' -> ' + JSON.stringify(after) + ')'); |
| + |
| + // And the renewal installed the *replacement* credential, rather |
| + // than restarting on the one it already had: a fresh generation |
| + // and flowing media would look exactly the same if renewal's |
| + // setConfiguration call were deleted, because host candidates |
| + // carry this call either way (Codex S3-5). Digests, never |
| + // credentials. |
| + const configs = await iceConfigs(pageA); |
| + const credentialed = configs.filter((c) => c.servers === 2 && c.credentialed === 1); |
| + const digests = Array.from(new Set(credentialed.map((c) => c.digest))); |
| + // Two credentialed configurations with *different* digests: the |
| + // first list this connection was given, and the replacement the |
| + // renewal installed. Which of them arrived through the |
| + // constructor and which through setConfiguration is not the |
| + // claim — a connection built during the first fetch takes its |
| + // first list through adopt — and the digests never leave the |
| + // page as anything but digests. |
| + check(credentialed.length >= 2 && digests.length >= 2, |
| + 'the renewal installed a credential different from the one this connection had (' + |
| + JSON.stringify(configs.map((c) => c.at + ':' + c.servers + ':' + c.digest)) + ')'); |
| } |
| // The call is what matters, not the restart: frames rising on both |
| @@ -1013,6 +1102,88 @@ const run = { |
| // testing a knob rather than the client, so proving the warning clears |
| // would cost this scenario seventy seconds of waiting. The plan's own |
| // ruling says the same; the recovery timing stays unproved here. |
| + // §13's other renewal ending, and the one no scenario reached: the |
| + // fetch works, the restart does not. room.js must spend all three |
| + // attempts and then say the connection is stuck — never that the |
| + // credential expired, which it plainly has not, and which is what the |
| + // client used to say after one attempt when the old deadline happened |
| + // to fall inside the window (Codex S3-4, S3-5). |
| + // |
| + // The instrument is a page-side stub: A's restartIce does nothing at |
| + // all. Everything else is real — the fetch is served normally, the |
| + // list is installed, the exchange is untouched — so what is being |
| + // tested is the state machine's own verdict on a restart that will |
| + // not land. |
| + async icestuck() { |
| + const ctxA = await newContext('member A', { cookie: cookieA }); |
| + const ctxB = await newContext('member B', { cookie: cookieB }); |
| + await ctxA.addInitScript(() => { |
| + const proto = window.RTCPeerConnection && window.RTCPeerConnection.prototype; |
| + if (proto) proto.restartIce = function noRestart() {}; |
| + }); |
| + const iceA = await iceRoute(ctxA, { ttl: '12s' }); |
| + const pageA = await openRoom(ctxA); |
| + const pageB = await openRoom(ctxB); |
| + |
| + const pair = await assertPair(pageA, pageB, 'member A', 'member B'); |
| + if (!pair) { |
| + await ctxA.close(); |
| + await ctxB.close(); |
| + return; |
| + } |
| + const peerB = pair.b.peer; |
| + const framesBefore = (await state(pageA)).frames[peerB] || 0; |
| + |
| + // The renewal itself works: a second list arrives. |
| + const renewing = await until(async () => { |
| + const s = await state(pageA); |
| + return s.ice.fetches >= 2 ? s : false; |
| + }, 25000); |
| + check(renewing, 'A fetched a replacement list (' + iceA.served + ' served) — ' + |
| + (await diagnose(pageA, 'member A'))); |
| + check(!(await state(pageA)).ice.warning, |
| + 'and nothing claims the relay is unreachable: it answered (' + |
| + JSON.stringify((await state(pageA)).ice) + ')'); |
| + |
| + // Three restarts, thirty seconds each, and none of them lands. |
| + // That is 90 seconds before the verdict, so the wait is generous. |
| + const verdict = await until(async () => { |
| + const r = (await state(pageA)).renewals[peerB]; |
| + return r && r.trouble ? r : false; |
| + }, 120000); |
| + check(verdict, 'A reached a verdict on the connection it could not renew — ' + |
| + (await diagnose(pageA, 'member A'))); |
| + if (verdict) { |
| + check(verdict.trouble === 'stuck', |
| + 'the verdict is that the connection is stuck, not that the credential expired (' + |
| + JSON.stringify(verdict) + ')'); |
| + check(verdict.expired === false, |
| + 'and it is not marked expired: a replacement credential was in hand throughout (' + |
| + JSON.stringify(verdict) + ')'); |
| + check(verdict.attempts === 3, |
| + 'it spent the whole three-restart budget first (' + verdict.attempts + ')'); |
| + check(verdict.renewed === false, 'and nothing was proven (' + JSON.stringify(verdict) + ')'); |
| + const notes = await tileNotes(pageA); |
| + check(notes.includes(ICE_STUCK) && !notes.includes(ICE_EXPIRED), |
| + "A's tile says it could not renew this connection, and does not say expired (" + |
| + JSON.stringify(notes) + ')'); |
| + } |
| + |
| + // And the call itself is untouched: a restart that never happened |
| + // costs nothing on a network where the media was already flowing. |
| + const rising = await until(async () => { |
| + const s = await state(pageA); |
| + return (s.frames[peerB] || 0) > framesBefore && s.connection[peerB] === 'connected' ? s : false; |
| + }, 15000); |
| + check(rising, 'the call is still connected and still receiving video (' + framesBefore + ' -> ' + |
| + (rising ? rising.frames[peerB] : 'stuck') + ') — ' + (await diagnose(pageA, 'member A'))); |
| + |
| + await leaveRoom(pageA); |
| + await leaveRoom(pageB); |
| + await ctxA.close(); |
| + await ctxB.close(); |
| + }, |
| + |
| async icefail() { |
| const ctxA = await newContext('member A', { cookie: cookieA }); |
| const ctxB = await newContext('member B', { cookie: cookieB }); |
diff --git a/test/browser/negotiation.test.mjs b/test/browser/negotiation.test.mjs| index 1ec4ca0..7aab2e8 100644 |
| --- a/test/browser/negotiation.test.mjs |
| +++ b/test/browser/negotiation.test.mjs |
| @@ -347,13 +347,23 @@ test('the fallback prefers a nominated pair that names a fragment over one that |
| ]), '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. |
| - assert.equal(renewedUfrag(report('new1'), null, 'stable', true), 'new1'); |
| - assert.equal(renewedUfrag(report(''), null, 'stable', true), ''); |
| - // And it is still bounded by the exchange having completed. |
| - assert.equal(renewedUfrag(report('new1'), null, 'stable', false), null); |
| +test('nothing recorded before the restart is not proof of anything', () => { |
| + // This used to be the fallback: with no baseline, any succeeded |
| + // selected pair counted. But an unchanged pair looks exactly like a |
| + // new one when there is nothing to compare it with, so that reading |
| + // could clear the warning and stamp a connection as renewed onto a |
| + // credential it had never gathered on (Codex S3-3). A rejected |
| + // getStats before the restart arrived here the same way. The poller |
| + // takes its own baseline from the first reading after the restart |
| + // instead — see "the proof takes a baseline after the restart". |
| + assert.equal(renewedUfrag(report('new1'), null, 'stable', true), null); |
| + assert.equal(renewedUfrag(report(''), null, 'stable', true), null); |
| + assert.equal(renewedUfrag(report('new1'), '', 'stable', true), null); |
| +}); |
| + |
| +test('a pair the browser will not name is not evidence', () => { |
| + // '' says "there is a pair", not "there is a new one". |
| + assert.equal(renewedUfrag(report(''), 'old0', 'stable', true), null); |
| }); |
| test('the reader tells "no such pair" from "a pair with no fragment"', () => { |
| @@ -467,11 +477,11 @@ test('the must-succeed-by marks the connections that credential was about, and o |
| ['older', { listAt: 400 }], |
| ['joined-since', { listAt: expiring + 1 }], |
| ['restarting', { listAt: expiring }], |
| - ['already-renewed', { listAt: expiring }], |
| + ['proven-onto-the-new-one', { listAt: expiring + 1 }], |
| ]); |
| const renewals = new Map([ |
| ['restarting', { running: true, renewed: false }], |
| - ['already-renewed', { running: false, renewed: true }], |
| + ['proven-onto-the-new-one', { running: false, renewed: true }], |
| ]); |
| const marked = []; |
| const { expireUnrenewed } = watchdog( |
| @@ -484,6 +494,30 @@ test('the must-succeed-by marks the connections that credential was about, and o |
| 'a connection built on the expiring credential, with nothing running, is the whole point'); |
| }); |
| +test('a renewal in an earlier cycle does not exempt a connection from a later one', () => { |
| + // Codex S3-1. `renewed` is a boolean about the past: it goes true on |
| + // the first successful renewal and nothing ever puts it back, so a |
| + // watchdog that read it exempted that connection from every deadline |
| + // for the rest of the call. What matters is which credential the |
| + // connection is *on*, and `listAt` is that: it moves only on proof. |
| + // |
| + // Cycle 1 succeeds, so listAt moves to the first renewal's list. |
| + // Cycle 2's fetch then fails, and the credential that expires is that |
| + // same list — which this connection is still using. |
| + const cycleOne = 2000; |
| + const pcs = new Map([['them', { listAt: cycleOne }]]); |
| + const renewals = new Map([['them', { running: false, renewed: true, attempts: 1 }]]); |
| + const marked = []; |
| + const { expireUnrenewed } = watchdog( |
| + { stopped: false }, pcs, renewals, (id) => marked.push(id), |
| + ); |
| + |
| + expireUnrenewed(cycleOne); |
| + |
| + assert.deepEqual(marked, ['them'], |
| + 'the second credential expired under a connection that had renewed once, and nobody was told'); |
| +}); |
| + |
| // ── the roster the fetch came back to (§4.5, §4.6, §5.2) ────────────── |
| // |
| @@ -933,3 +967,178 @@ test('no local stream asks to receive both', () => { |
| assert.equal(hasLiveTrack('video'), false); |
| assert.equal(hasLiveTrack('audio'), false); |
| }); |
| + |
| + |
| +// ── the renewal state machine, driven (§5.3 step 2, §15.14) ─────────── |
| +// |
| +// The pure decisions above are read by a loop, and the loop is where |
| +// three of Codex's round-1 findings lived: which connections a fetch is |
| +// for, how many attempts a fresh credential buys, and which of the two |
| +// sentences the failure earns. So the loop itself is lifted and driven |
| +// over fake connections, with the windows shortened to milliseconds. |
| +const machine = new Function( |
| + 'state', 'pcs', 'renewals', 'ice', 'fetchIce', 'requestReset', 'renderTiles', |
| + 'bareRestart', 'RENEW_ATTEMPTS', 'RENEW_WINDOW', 'RENEW_POLL', 'console', |
| + [lift(src, 'renewOne'), lift(src, 'renewOutdated'), lift(src, 'retryRenewal'), |
| + lift(src, 'renewalRecord'), lift(src, 'markTrouble'), lift(src, 'markRenewalStuck'), |
| + lift(src, 'markRelayExpired'), lift(src, 'proveRenewed'), lift(src, 'renewedUfrag'), |
| + lift(src, 'selectedPairUfrag'), lift(src, 'selectedUfrag'), lift(src, 'statsList'), |
| + lift(src, 'sleep')].join('\n') + |
| + '\nreturn { renewOne, renewOutdated, retryRenewal, proveRenewed };', |
| +); |
| + |
| +// A connection whose getStats walks a list of fragments, one reading at |
| +// a time, and whose restartIce completes an exchange the way a real one |
| +// does. Give it ['old0'] and it never moves: the restart does not land. |
| +function renewable(ufrags, listAt) { |
| + const entry = { desc: 0, listAt: listAt || 0, floor: false, makingOffer: false }; |
| + let i = 0; |
| + entry.calls = []; |
| + entry.pc = { |
| + signalingState: 'stable', |
| + setConfiguration: (c) => entry.calls.push('setConfiguration:' + ((c.iceServers || []).length)), |
| + restartIce: () => { |
| + entry.calls.push('restartIce'); |
| + entry.desc += 1; |
| + }, |
| + getStats: () => { |
| + const u = ufrags[Math.min(i, ufrags.length - 1)]; |
| + i += 1; |
| + return Promise.resolve(new Map(report(u).map((r) => [r.id, r]))); |
| + }, |
| + }; |
| + return entry; |
| +} |
| + |
| +// The machine, wired to a mesh and a fetch that behaves as told. |
| +function mesh(entries, fetchResult) { |
| + const pcs = new Map(Object.entries(entries)); |
| + const renewals = new Map(); |
| + const ice = { servers: [1, 2], fetchedAt: 1000, expiresAt: 0, renewAt: 0, margin: 0 }; |
| + const resets = []; |
| + const fetchIce = async () => { |
| + if (!fetchResult) return false; |
| + ice.fetchedAt += 1000; // a new list, with a stamp of its own |
| + return true; |
| + }; |
| + const api = machine( |
| + { stopped: false }, pcs, renewals, ice, fetchIce, |
| + (id) => resets.push(id), () => {}, (id) => resets.push('bare:' + id), |
| + 3, 60, 10, console, |
| + ); |
| + return { ...api, pcs, renewals, ice, resets }; |
| +} |
| + |
| +test('a fresh credential buys three restarts and the verdict is stuck, never expired', async () => { |
| + // Codex S3-4. A renewal that starts shortly before the old deadline — |
| + // after a tab suspension, say — used to spend one attempt of its |
| + // three, read the deadline, and report the credential expired. It has |
| + // a replacement credential in its hand while it says so. The |
| + // credential is fine; the path is not; that is a different sentence |
| + // and it is worth all three attempts. |
| + const them = renewable(['old0']); |
| + const m = mesh({ them }); |
| + |
| + const ok = await m.renewOne('them'); |
| + |
| + assert.equal(ok, false); |
| + const record = m.renewals.get('them'); |
| + assert.equal(record.attempts, 3, 'the full budget, not one attempt cut short by a deadline'); |
| + assert.equal(record.trouble, 'stuck'); |
| + assert.equal(record.renewed, false); |
| + assert.equal(them.listAt, 0, 'and nothing was stamped: this connection is on no new credential'); |
| +}); |
| + |
| +test('a restart that lands is proven, stamped and quiet', async () => { |
| + const them = renewable(['old0', 'old0', 'new1']); |
| + const m = mesh({ them }); |
| + |
| + assert.equal(await m.renewOne('them'), true); |
| + const record = m.renewals.get('them'); |
| + assert.equal(record.renewed, true); |
| + assert.equal(record.ufrag, 'new1'); |
| + assert.equal(record.trouble, ''); |
| + assert.equal(record.attempts, 1, 'one restart was enough'); |
| + assert.equal(them.listAt, m.ice.fetchedAt, 'and the connection is stamped onto the list it took'); |
| +}); |
| + |
| +test('a Retry on one tile renews every connection the fetch re-credentialed', async () => { |
| + // Codex S3-2. The fetch a Retry makes replaces the credential the |
| + // whole call was using and resets the shared schedule with it, so |
| + // renewing only the tile that was clicked left every other connection |
| + // on a credential about to die with nothing coming to fix it. |
| + const a = renewable(['a0', 'a0', 'a1']); |
| + const b = renewable(['b0', 'b0', 'b1']); |
| + const m = mesh({ a, b }, true); |
| + |
| + await m.retryRenewal('a', 'expired'); |
| + // renewOutdated does not await its renewals — the mesh renews in |
| + // parallel, as it does in the page — so wait for both to settle. |
| + await new Promise((resolve) => setTimeout(resolve, 400)); |
| + |
| + assert.equal(m.renewals.get('a').renewed, true, 'the tile that was clicked'); |
| + assert.equal(m.renewals.get('b').renewed, true, 'and the one that was not'); |
| + assert.equal(b.listAt, m.ice.fetchedAt, 'both are stamped onto the new list'); |
| +}); |
| + |
| +test('a connection already on the new list is not renewed again', async () => { |
| + const a = renewable(['a0', 'a0', 'a1'], 1000); |
| + const joined = renewable(['j0'], 9999); // built after the fetch |
| + const m = mesh({ a, joined }); |
| + |
| + m.renewOutdated(1000); |
| + await new Promise((resolve) => setTimeout(resolve, 400)); |
| + |
| + assert.equal(m.renewals.get('a').renewed, true); |
| + assert.equal(m.renewals.has('joined'), false, 'nothing was asked of it, so it has no record'); |
| + assert.deepEqual(joined.calls, [], 'and its connection was not touched'); |
| +}); |
| + |
| +test('the proof takes a baseline after the restart when it had none before', async () => { |
| + // Codex S3-3, the other half: with no baseline the poller does not |
| + // give up, it takes the first fragment it can read afterwards and |
| + // then requires a different one. A connection that moves is still |
| + // proven. |
| + const them = renewable(['', 'base', 'base', 'moved']); |
| + const m = mesh({ them }); |
| + them.desc = 1; // the exchange has completed |
| + |
| + const proven = await m.proveRenewed('them', them, null, 0, Date.now() + 2000); |
| + assert.equal(proven, 'moved'); |
| +}); |
| + |
| +test('a connection that never moves is unproven, whatever it started with', async () => { |
| + const them = renewable(['', 'stuck0']); |
| + const m = mesh({ them }); |
| + them.desc = 1; |
| + |
| + const proven = await m.proveRenewed('them', them, null, 0, Date.now() + 200); |
| + assert.equal(proven, null, 'an unchanged pair with no baseline must not read as a new generation'); |
| +}); |
| + |
| +// ── a margin too small to schedule on (W3-12) ───────────────────────── |
| +// |
| +// The delay has a floor of a second, so a list whose renew time and |
| +// expiry are less than a second apart would be fetched about once a |
| +// second for as long as the tab stayed open. The server clamps what it |
| +// mints, so only the development TTL header can produce one — but a |
| +// client that answers a nonsense schedule by hammering its own server |
| +// is a client that can be made to do it, so it declines instead. |
| +const scheduling = new Function( |
| + 'ice', 'MIN_MARGIN', |
| + lift(src, 'schedulable') + '\nreturn { schedulable };', |
| +); |
| + |
| +test('an ordinary list is schedulable', () => { |
| + assert.equal(scheduling({ expiresAt: 9, servers: [1], margin: 1800000 }, 1000).schedulable(), true); |
| + assert.equal(scheduling({ expiresAt: 9, servers: [1], margin: 1000 }, 1000).schedulable(), true); |
| +}); |
| + |
| +test('a margin under a second is not a schedule', () => { |
| + assert.equal(scheduling({ expiresAt: 9, servers: [1], margin: 999 }, 1000).schedulable(), false); |
| + assert.equal(scheduling({ expiresAt: 9, servers: [1], margin: 0 }, 1000).schedulable(), false); |
| +}); |
| + |
| +test('the floor has nothing to say about a call with no relay', () => { |
| + assert.equal(scheduling({ expiresAt: 0, servers: [], margin: 0 }, 1000).schedulable(), false); |
| +}); |