amadan

oficina / meet Public

room.js: an attempt that cannot offer hands the ask back

Codex S4-12 (P2), and the far side of S4-8's own fix. The ask survived
round exhaustion — and then the retry spent it. The timer fired while a
remote exchange was in flight, `negotiate` cleared the flag at the top
of its round, slept through its yield, woke to a connection that was
still not stable, and walked away: pending false, timers none, offers
none. A return to `stable` had nothing left to collect.

The rule is that an attempt which settles nothing owes the ask back.

`politeYield` answers with the reason rather than a bare no, because
the three are not the same afterwards: a connection that has gone owes
nothing, a negotiation somebody else made is finished with, and an
exchange still *in flight* has settled nothing of ours. That last one
puts `renegotiate` back and leaves. So does an offer that never went
out for any other reason — an impolite side finding the connection
mid-exchange, where `setLocalDescription` throws and the attempt used
to end in silence.

And the timer defers rather than consuming: a retry that lands
mid-exchange puts itself off instead of spending the ask on an attempt
that cannot offer, bounded by NEGOTIATE_DEFERS so it stops waking every
300 ms — the ask outlives the timer either way, because the
connection's own return to `stable` is what collects it.

Five tests, and the harness now keeps the timer callback rather than
only counting schedulings, which is what let the first version of this
look fine. Both halves mutation-verified: dropping the deferral fails
three, dropping the hand-back fails four, and no test fails for both
reasons.

node --check, 212 node tests, make ci.
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 ad13608897b121ef44078cd67f4dafef863c9b63 parent a4d1ec1
2 files changed, +163 −17
  • internal/meet/static/room.js +56 −11
  • test/browser/room-lane.test.mjs +107 −6
diff --git a/internal/meet/static/room.js b/internal/meet/static/room.js
index b741b6b..ea00181 100644
--- a/internal/meet/static/room.js
+++ b/internal/meet/static/room.js
@@ -1031,8 +1031,10 @@
negotiation: 0,
negotiating: false,
// A retry scheduled for an ask the rounds ran out on, so a second
- // one is never stacked on top of it.
+ // one is never stacked on top of it, and how many times that
+ // retry has found the connection busy and put itself off.
negotiateTimer: 0,
+ negotiateDefers: 0,
// A `negotiationneeded` that arrived while an attempt was in
// flight, kept rather than dropped: an attempt that abandons its
// offer would otherwise take that change to the grave with it.
@@ -1231,11 +1233,20 @@
// two apart: it moves on every exchange, so a yield whose generation
// has changed is a yield whose negotiation somebody else already
// made. Offering anyway is the extra offer that produced.
+ //
+ // It answers with the reason rather than a bare no, because the three
+ // are not the same afterwards (Codex S4-12). A connection that has
+ // gone owes nothing. A negotiation somebody else made is finished
+ // with. But an exchange still *in flight* has settled nothing: this
+ // side's own reason for offering is as outstanding as it was, and an
+ // attempt that walks away from it without saying so is where the ask
+ // went missing.
async function politeYield(id, entry, generation) {
await sleep(POLITE_YIELD);
- if (pcs.get(id) !== entry || state.stopped) return false;
- if (entry.negotiation !== generation) return false;
- return entry.pc.signalingState === 'stable';
+ if (pcs.get(id) !== entry || state.stopped) return 'gone';
+ if (entry.negotiation !== generation) return 'superseded';
+ if (entry.pc.signalingState !== 'stable') return 'busy';
+ return '';
}
// answeredExchange records one offer/answer exchange this side did
@@ -1295,6 +1306,12 @@
// steps: each round can spend a yield, and the allowance runs out.
const NEGOTIATE_ROUNDS = 4;
+ // How many times a kept ask's retry may find the connection busy and
+ // put itself off again. After that it stops asking and waits: the
+ // connection's own `signalingstatechange` is what picks the ask up,
+ // and a timer that keeps waking every 300 ms adds nothing to it.
+ const NEGOTIATE_DEFERS = 10;
+
async function negotiate(id, entry) {
if (pcs.get(id) !== entry) return;
if (entry.negotiating) {
@@ -1308,14 +1325,25 @@
const generation = entry.negotiation;
if (entry.polite && entry.yields < POLITE_YIELDS) {
entry.yields += 1;
- if (!(await politeYield(id, entry, generation))) {
- // Abandoned. Something asked for a negotiation while this
- // one slept, and this is the only thing that will answer
- // it: the ask is outstanding and the event is long gone.
+ const why = await politeYield(id, entry, generation);
+ if (why === 'gone') return;
+ if (why === 'busy') {
+ // An exchange is in flight and has settled nothing of ours.
+ // The ask goes back on the connection for the return to
+ // `stable` to collect (Codex S4-12): walking away from it
+ // here is how a preserved request went missing, and this
+ // attempt may be the only thing that was holding it.
+ entry.renegotiate = true;
+ return;
+ }
+ if (why === 'superseded') {
+ // Somebody else made this negotiation. Only an ask that
+ // arrived *while* we slept is still outstanding.
if (!entry.renegotiate || pcs.get(id) !== entry || state.stopped) return;
continue;
}
}
+ let offered = false;
try {
entry.makingOffer = true;
await entry.pc.setLocalDescription();
@@ -1325,15 +1353,19 @@
entry.negotiation += 1;
entry.yields = 0;
send(id, 'offer', { description: describe(entry.pc.localDescription) });
+ offered = true;
} catch {
// A failed local description is not fatal: the pair recovers
// through a reset if it never connects.
} finally {
entry.makingOffer = false;
}
- // An offer went out (or failed to). Either way it was made from
- // the connection as it stands, so it carries every local change
- // asked for up to this moment.
+ // An offer that went out was made from the connection as it
+ // stands, so it carries every local change asked for up to this
+ // moment. One that did not — an impolite side finding the
+ // connection mid-exchange, say — has settled nothing, and the
+ // ask goes back for the quiet moment to collect.
+ if (!offered) entry.renegotiate = true;
return;
}
} finally {
@@ -1367,6 +1399,19 @@
entry.negotiateTimer = setTimeout(() => {
entry.negotiateTimer = 0;
if (!entry.renegotiate || pcs.get(id) !== entry || state.stopped) return;
+ // A retry that lands mid-exchange must not spend the ask on an
+ // attempt that cannot offer (Codex S4-12): it defers instead, and
+ // the connection's own return to `stable` is what collects it.
+ // The timer comes back a bounded number of times for the case
+ // where nothing announces that return; after that the ask simply
+ // waits, because the stable handler is not going anywhere.
+ if (entry.negotiating || entry.pc.signalingState !== 'stable') {
+ if (entry.negotiateDefers >= NEGOTIATE_DEFERS) return;
+ entry.negotiateDefers += 1;
+ keepNegotiation(id, entry);
+ return;
+ }
+ entry.negotiateDefers = 0;
negotiate(id, entry).catch((err) => {
console.error('meet: negotiate', err);
});
diff --git a/test/browser/room-lane.test.mjs b/test/browser/room-lane.test.mjs
index a6fd933..dc234e2 100644
--- a/test/browser/room-lane.test.mjs
+++ b/test/browser/room-lane.test.mjs
@@ -486,12 +486,15 @@ const POLITE_YIELDS = pin('POLITE_YIELDS');
const NEGOTIATE_ROUNDS = pin('NEGOTIATE_ROUNDS');
+const NEGOTIATE_DEFERS = pin('NEGOTIATE_DEFERS');
+
const negotiator = new Function(
'pcs', 'send', 'describe', 'sleep', 'state',
- 'POLITE_YIELD', 'POLITE_YIELDS', 'NEGOTIATE_ROUNDS', 'setTimeout', 'console',
+ 'POLITE_YIELD', 'POLITE_YIELDS', 'NEGOTIATE_ROUNDS', 'NEGOTIATE_DEFERS',
+ 'setTimeout', 'console',
[lift(src, 'negotiate'), lift(src, 'politeYield'), lift(src, 'answeredExchange'),
lift(src, 'keepNegotiation')].join('\n') +
- '\nreturn { negotiate, answeredExchange };',
+ '\nreturn { negotiate, answeredExchange, keepNegotiation };',
);
// A connection as `negotiate` sees one, over a fake pc whose
@@ -506,6 +509,7 @@ function negotiating(opts) {
const sent = [];
const waits = [];
const scheduled = [];
+ const timers = [];
const pcs = new Map();
const entry = {
polite: !!o.polite,
@@ -514,6 +518,7 @@ function negotiating(opts) {
negotiating: false,
renegotiate: false,
negotiateTimer: 0,
+ negotiateDefers: 0,
makingOffer: false,
pc: {
signalingState: o.signalingState || 'stable',
@@ -537,23 +542,35 @@ function negotiating(opts) {
POLITE_YIELD,
POLITE_YIELDS,
NEGOTIATE_ROUNDS,
- // The retry the rounds hand their outstanding work to, recorded
- // rather than run: what this file gates is that the ask survives
- // and is scheduled, not what the timer eventually does with it.
+ NEGOTIATE_DEFERS,
+ // The retry the rounds hand their outstanding work to. The callback
+ // is kept as well as counted, because what it does when it runs is
+ // the whole of Codex S4-12 and a test that only counts schedulings
+ // never finds out.
(fn, ms) => {
scheduled.push(ms);
- return scheduled.length;
+ timers.push(fn);
+ return timers.length;
},
quiet,
);
box = {
negotiate: built.negotiate,
answeredExchange: built.answeredExchange,
+ keepNegotiation: built.keepNegotiation,
entry,
pcs,
sent,
waits,
scheduled,
+ timers,
+ // fire runs the retry the way a timer would: the entry's own handle
+ // is cleared first, exactly as the shipped callback does.
+ fire: () => {
+ const fn = timers.shift();
+ entry.negotiateTimer = 0;
+ return fn ? fn() : undefined;
+ },
};
return box;
}
@@ -709,6 +726,90 @@ test('an ask the rounds run out on is kept, not dropped', async () => {
assert.equal(n.entry.negotiating, false, 'the serializer must not stay latched');
});
+test('the retry does not spend the ask on a connection mid-exchange', async () => {
+ // Codex S4-12. The ask survived round exhaustion; then the timer ran
+ // while a remote exchange was in flight, `negotiate` cleared the flag
+ // at the top of its round, slept, woke to a connection that was still
+ // not stable, and walked away — pending=false, timers=0, offers=0.
+ // A return to `stable` has nothing left to collect.
+ const n = negotiating({
+ polite: true,
+ duringYield: (entry, pcs, box) => {
+ box.answeredExchange(entry);
+ box.negotiate('peer1', entry);
+ },
+ });
+ await n.negotiate('peer1', n.entry);
+ assert.equal(n.entry.renegotiate, true, 'the rounds did not keep the ask');
+ assert.equal(n.timers.length, 1);
+
+ // The exchange the retry lands in the middle of.
+ n.entry.pc.signalingState = 'have-remote-offer';
+ await n.fire();
+
+ assert.equal(n.entry.renegotiate, true, 'the retry spent the ask on an attempt that could not offer');
+ assert.deepEqual(n.sent, [], 'an offer went out into an exchange');
+ assert.equal(n.timers.length, 1, 'nothing is coming back for the ask');
+ assert.equal(n.entry.negotiateDefers, 1);
+});
+
+test('a deferred retry offers once the connection is quiet again', async () => {
+ const n = negotiating({ polite: true, yields: POLITE_YIELDS });
+ n.entry.renegotiate = true;
+ n.keepNegotiation('peer1', n.entry);
+ n.entry.pc.signalingState = 'have-remote-offer';
+ await n.fire();
+ assert.deepEqual(n.sent, []);
+
+ // The exchange completes and the connection settles.
+ n.entry.pc.signalingState = 'stable';
+ await n.fire();
+ assert.deepEqual(n.sent, [['peer1', 'offer', true]], 'the kept ask was never answered');
+ assert.equal(n.entry.renegotiate, false);
+ assert.equal(n.entry.negotiateDefers, 0, 'the deferral count must not carry into the next ask');
+});
+
+test('an exchange that outlasts the yield leaves the ask behind it', async () => {
+ // The other door onto the same defect, and the one that needs no
+ // timer: an attempt whose yield wakes into an exchange that is still
+ // in flight. Nothing about that settled this side's own reason for
+ // offering.
+ const n = negotiating({
+ polite: true,
+ duringYield: (entry) => { entry.pc.signalingState = 'have-remote-offer'; },
+ });
+ await n.negotiate('peer1', n.entry);
+ assert.deepEqual(n.sent, []);
+ assert.equal(n.entry.renegotiate, true, 'the ask went with the attempt');
+ assert.equal(n.timers.length, 1, 'and nothing was scheduled to bring it back');
+});
+
+test('an impolite side that cannot offer keeps its ask too', async () => {
+ // No yield at all on this side: `setLocalDescription` throws on a
+ // connection mid-exchange, which used to end the attempt silently.
+ const n = negotiating({ polite: false });
+ n.entry.pc.setLocalDescription = async () => {
+ throw new Error('InvalidStateError');
+ };
+ await n.negotiate('peer1', n.entry);
+ assert.deepEqual(n.sent, []);
+ assert.equal(n.entry.renegotiate, true);
+ assert.equal(n.timers.length, 1);
+});
+
+test('a retry gives up rescheduling but never gives up the ask', async () => {
+ const n = negotiating({ polite: true, yields: POLITE_YIELDS });
+ n.entry.renegotiate = true;
+ n.keepNegotiation('peer1', n.entry);
+ n.entry.pc.signalingState = 'have-remote-offer';
+ for (let i = 0; i < NEGOTIATE_DEFERS + 2; i++) {
+ if (n.timers.length) await n.fire();
+ }
+ assert.equal(n.entry.renegotiate, true, 'the ask must outlive the timer');
+ assert.equal(n.timers.length, 0, 'the timer went on waking for ever');
+ assert.equal(n.entry.negotiateDefers, NEGOTIATE_DEFERS);
+});
+
test('a kept ask schedules one retry, not one per round', async () => {
// The retry is a place to pick the ask up again, not a queue: the
// flag is cleared at the top of every round, so a scheduled attempt