amadan
room.js: the round bound stops the sleeping, not the negotiation
Codex S4-8 (P2). Let a remote exchange complete during *every* yield, each one firing a fresh `negotiationneeded`: `answeredExchange` hands the spent yield back, so every round gives way again and the rounds run out. The `finally` then cleared `renegotiate` — waits 4, offers 0, renegotiate false — and the last ask, for local work that still needed an offer, was gone with nothing left to make it. The lost-request defect f982bdf fixed, reached through the bound f982bdf added. NEGOTIATE_ROUNDS bounds how long one attempt may go round. It does not decide that outstanding work stops mattering. So an ask the rounds ran out on stays on the connection, and `keepNegotiation` gives it a place to be picked up: the connection's next return to `stable` (which is the usual reason the rounds ran out in the first place) or, for a connection that is already stable and quiet and will announce nothing, a timer. One retry at a time — `negotiate` clears the flag at the top of every round, so a scheduled attempt that finds nothing outstanding does nothing at all — and a connection that goes away keeps no ask and schedules nothing. Three tests, the first two mutation-verified: putting the `entry.renegotiate = false` back in the `finally` fails them and only them. The first reproduces Codex's trace exactly (waits 4, offers 0) and now asserts the ask survives it. node --check, 197 node tests, make ci.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CzvnuKhLzudvq7jZ7SvVt2
2 files changed,
+117
−2
internal/meet/static/room.js+50 −0test/browser/room-lane.test.mjs+67 −2
diff --git a/internal/meet/static/room.js b/internal/meet/static/room.js| index 288b662..6401e56 100644 |
| --- a/internal/meet/static/room.js |
| +++ b/internal/meet/static/room.js |
| @@ -1016,6 +1016,9 @@ |
| yields: 0, |
| 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. |
| + negotiateTimer: 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. |
| @@ -1146,6 +1149,14 @@ |
| // far side means for something else. |
| addShareTo(entry); |
| adoptWhenQuiet(id, entry); |
| + // And it is when an ask the negotiation rounds ran out on gets |
| + // its turn (Codex S4-8): a connection that was mid-negotiation is |
| + // the usual reason they ran out, and this is it settling. |
| + if (entry.renegotiate && !entry.negotiating && pc.signalingState === 'stable') { |
| + negotiate(id, entry).catch((err) => { |
| + console.error('meet: negotiate', err); |
| + }); |
| + } |
| }); |
| // And now the media. Our own tracks, and a transceiver for every |
| @@ -1313,8 +1324,39 @@ |
| } |
| } finally { |
| entry.negotiating = false; |
| + // The bound limits how long one attempt may go round; it does not |
| + // decide that outstanding work stops mattering (Codex S4-8). If |
| + // something asked for a negotiation that this attempt never got |
| + // to, the ask stays on the connection and is picked up at its |
| + // next quiet moment. |
| + keepNegotiation(id, entry); |
| + } |
| + } |
| + |
| + // keepNegotiation is what happens to an ask the rounds ran out on. |
| + // The request stays true and a retry is scheduled: on the next return |
| + // to `stable` if the connection is mid-negotiation (which is the |
| + // usual reason the rounds ran out), and on a timer otherwise, because |
| + // a connection that is already stable and quiet will not announce |
| + // anything for the retry to hang off. |
| + // |
| + // A retry is scheduled once at a time: `negotiate` clears the flag at |
| + // the top of every round, so a scheduled one that finds nothing |
| + // outstanding does nothing at all. |
| + function keepNegotiation(id, entry) { |
| + if (!entry.renegotiate) return; |
| + if (pcs.get(id) !== entry || state.stopped) { |
| entry.renegotiate = false; |
| + return; |
| } |
| + if (entry.negotiateTimer) return; |
| + entry.negotiateTimer = setTimeout(() => { |
| + entry.negotiateTimer = 0; |
| + if (!entry.renegotiate || pcs.get(id) !== entry || state.stopped) return; |
| + negotiate(id, entry).catch((err) => { |
| + console.error('meet: negotiate', err); |
| + }); |
| + }, POLITE_YIELD); |
| } |
| // hasLiveTrack answers whether the local stream has a track of this |
| @@ -1339,6 +1381,14 @@ |
| } catch { |
| // Already closed. |
| } |
| + // A retry scheduled for a connection that is going away has |
| + // nothing left to negotiate; the timer's own guard would refuse |
| + // it anyway, and clearing it here keeps nothing pending. |
| + if (entry.negotiateTimer) { |
| + clearTimeout(entry.negotiateTimer); |
| + entry.negotiateTimer = 0; |
| + } |
| + entry.renegotiate = false; |
| pcs.delete(id); |
| } |
| queue.delete(id); |
diff --git a/test/browser/room-lane.test.mjs b/test/browser/room-lane.test.mjs| index 644a5cf..da819b4 100644 |
| --- a/test/browser/room-lane.test.mjs |
| +++ b/test/browser/room-lane.test.mjs |
| @@ -488,8 +488,9 @@ const NEGOTIATE_ROUNDS = pin('NEGOTIATE_ROUNDS'); |
| const negotiator = new Function( |
| 'pcs', 'send', 'describe', 'sleep', 'state', |
| - 'POLITE_YIELD', 'POLITE_YIELDS', 'NEGOTIATE_ROUNDS', 'console', |
| - [lift(src, 'negotiate'), lift(src, 'politeYield'), lift(src, 'answeredExchange')].join('\n') + |
| + 'POLITE_YIELD', 'POLITE_YIELDS', 'NEGOTIATE_ROUNDS', 'setTimeout', 'console', |
| + [lift(src, 'negotiate'), lift(src, 'politeYield'), lift(src, 'answeredExchange'), |
| + lift(src, 'keepNegotiation')].join('\n') + |
| '\nreturn { negotiate, answeredExchange };', |
| ); |
| @@ -504,6 +505,7 @@ function negotiating(opts) { |
| const o = opts || {}; |
| const sent = []; |
| const waits = []; |
| + const scheduled = []; |
| const pcs = new Map(); |
| const entry = { |
| polite: !!o.polite, |
| @@ -511,6 +513,7 @@ function negotiating(opts) { |
| negotiation: 0, |
| negotiating: false, |
| renegotiate: false, |
| + negotiateTimer: 0, |
| makingOffer: false, |
| pc: { |
| signalingState: o.signalingState || 'stable', |
| @@ -534,6 +537,13 @@ 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. |
| + (fn, ms) => { |
| + scheduled.push(ms); |
| + return scheduled.length; |
| + }, |
| quiet, |
| ); |
| box = { |
| @@ -543,6 +553,7 @@ function negotiating(opts) { |
| pcs, |
| sent, |
| waits, |
| + scheduled, |
| }; |
| return box; |
| } |
| @@ -674,6 +685,60 @@ test('an abandoned yield that nobody asked to repeat offers nothing', async () = |
| assert.equal(n.waits.length, 1, 'it went round again with nothing to go round for'); |
| }); |
| +test('an ask the rounds run out on is kept, not dropped', async () => { |
| + // Codex S4-8, and the far end of f982bdf's fix. Let a remote exchange |
| + // complete during *every* yield, each one firing a fresh |
| + // `negotiationneeded`: `answeredExchange` hands the spent yield back, |
| + // so every round gives way again, and the rounds run out. The shipped |
| + // code then cleared the flag in its `finally` — waits 4, offers 0, |
| + // renegotiate false — and the last ask, for local work that still |
| + // needed an offer, was gone with nothing left to make it. |
| + 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.waits.length, NEGOTIATE_ROUNDS, 'the bound on synchronous rounds is gone'); |
| + assert.deepEqual(n.sent, [], 'an offer went out into an exchange that was still moving'); |
| + assert.equal(n.entry.renegotiate, true, 'the outstanding ask was thrown away with the rounds'); |
| + assert.deepEqual(n.scheduled, [POLITE_YIELD], 'nothing was scheduled to answer it'); |
| + assert.equal(n.entry.negotiating, false, 'the serializer must not stay latched'); |
| +}); |
| + |
| +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 |
| + // that finds nothing outstanding does nothing at all. |
| + const n = negotiating({ |
| + polite: true, |
| + duringYield: (entry, pcs, box) => { |
| + box.answeredExchange(entry); |
| + box.negotiate('peer1', entry); |
| + }, |
| + }); |
| + await n.negotiate('peer1', n.entry); |
| + await n.negotiate('peer1', n.entry); |
| + assert.equal(n.scheduled.length, 1, 'retries stacked up: ' + JSON.stringify(n.scheduled)); |
| +}); |
| + |
| +test('a connection that went away keeps no ask and schedules nothing', async () => { |
| + const n = negotiating({ |
| + polite: true, |
| + duringYield: (entry, pcs, box) => { |
| + box.answeredExchange(entry); |
| + box.negotiate('peer1', entry); |
| + pcs.delete('peer1'); |
| + }, |
| + }); |
| + await n.negotiate('peer1', n.entry); |
| + assert.deepEqual(n.scheduled, []); |
| + assert.equal(n.entry.renegotiate, false); |
| +}); |
| + |
| test('a negotiation after a lost yield is protected again', async () => { |
| // The reset defect the Task 4 re-review carried (item 1): a yield lost |
| // to an exchange that happened to satisfy this side's own change left |