amadan

oficina / meet Public

room.js: an ask made during a yield is not lost with it

Found by the browser gate, in the same run that first exercised the
fresh-baseline checks: `mutualshare` round 1, the polite side's screen
announced and never arriving — a caption with nothing under it, a
`sendonly` transceiver still unassociated three rounds later, and one
inbound stream where there should have been two.

The serializer from the previous commit did it. The polite side adds
its screen, sleeps in its yield, and answers the other side's offer
while asleep. That exchange fires `negotiationneeded` for the polite
side's *own* screen — which was dropped, because an attempt was in
flight — and then the attempt woke, found the generation moved by the
exchange it had slept through, and abandoned its offer. Nobody was left
to offer the screen, and nothing would ever ask again.

So an event that arrives mid-attempt is remembered on the connection,
and an attempt that abandons its offer goes round again for it. Bounded
by NEGOTIATE_ROUNDS, because going round and giving way are the same
two steps and the yield allowance runs out: four rounds, and in
practice two.

Two node tests, the first mutation-verified — restoring the plain
`if (entry.negotiating) return;` fails it, and only it: an exchange plus
a re-entrant `negotiate` during the yield must still end in one offer,
and an abandoned yield nobody asked to repeat must still end in none.

node --check, 182 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 f982bdf2149ba058e51d5bda74eedb0aec63e55d parent 6f1be79
2 files changed, +108 −20
  • internal/meet/static/room.js +58 −19
  • test/browser/room-lane.test.mjs +50 −1
diff --git a/internal/meet/static/room.js b/internal/meet/static/room.js
index 231bdf0..2e87e07 100644
--- a/internal/meet/static/room.js
+++ b/internal/meet/static/room.js
@@ -1016,6 +1016,10 @@
yields: 0,
negotiation: 0,
negotiating: false,
+ // 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.
+ renegotiate: false,
// §5.1's camera watch (Codex S4-5): the last framesEncoded read
// off this connection's own camera m-line, how many polls in a
// row have reported that same number, and how many times the
@@ -1250,31 +1254,66 @@
// remote exchange completing is exactly the thing that fires it — and
// the second attempt used to skip the yield, because the first had
// already spent it, and then both of them offered.
+ // An attempt abandoned by its yield is not the end of the story, and
+ // this is where the first draft of the serializer was wrong — the
+ // browser gate's mutualshare caught it. `negotiationneeded` fires for
+ // the polite side's own screen while its attempt is asleep; the
+ // attempt then wakes, finds the generation moved by the exchange it
+ // slept through, and abandons its offer. If that event was simply
+ // dropped because an attempt was in flight, the screen it was about
+ // is never offered at all — the transceiver sits unassociated for the
+ // rest of the call and the far side sees a caption with nothing in
+ // it. So an event that arrives mid-attempt is remembered, and an
+ // attempt that abandons its offer runs again for it.
+ //
+ // Bounded, because "go round again" and "give way" are the same two
+ // steps: each round can spend a yield, and the allowance runs out.
+ const NEGOTIATE_ROUNDS = 4;
+
async function negotiate(id, entry) {
- if (pcs.get(id) !== entry || entry.negotiating) return;
+ if (pcs.get(id) !== entry) return;
+ if (entry.negotiating) {
+ entry.renegotiate = true;
+ return;
+ }
entry.negotiating = true;
try {
- if (entry.polite && entry.yields < POLITE_YIELDS) {
- entry.yields += 1;
- if (!(await politeYield(id, entry, entry.negotiation))) return;
- }
- try {
- entry.makingOffer = true;
- await entry.pc.setLocalDescription();
- // This side's own offer: the negotiation is ours, the
- // generation moves with it, and the next one starts with the
- // whole allowance again.
- entry.negotiation += 1;
- entry.yields = 0;
- send(id, 'offer', { description: describe(entry.pc.localDescription) });
- } catch {
- // A failed local description is not fatal: the pair recovers
- // through a reset if it never connects.
- } finally {
- entry.makingOffer = false;
+ for (let round = 0; round < NEGOTIATE_ROUNDS; round++) {
+ entry.renegotiate = false;
+ 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.
+ if (!entry.renegotiate || pcs.get(id) !== entry || state.stopped) return;
+ continue;
+ }
+ }
+ try {
+ entry.makingOffer = true;
+ await entry.pc.setLocalDescription();
+ // This side's own offer: the negotiation is ours, the
+ // generation moves with it, and the next one starts with the
+ // whole allowance again.
+ entry.negotiation += 1;
+ entry.yields = 0;
+ send(id, 'offer', { description: describe(entry.pc.localDescription) });
+ } 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.
+ return;
}
} finally {
entry.negotiating = false;
+ entry.renegotiate = false;
}
}
diff --git a/test/browser/room-lane.test.mjs b/test/browser/room-lane.test.mjs
index 8e6319a..644a5cf 100644
--- a/test/browser/room-lane.test.mjs
+++ b/test/browser/room-lane.test.mjs
@@ -484,8 +484,11 @@ function pin(name) {
const POLITE_YIELD = pin('POLITE_YIELD');
const POLITE_YIELDS = pin('POLITE_YIELDS');
+const NEGOTIATE_ROUNDS = pin('NEGOTIATE_ROUNDS');
+
const negotiator = new Function(
- 'pcs', 'send', 'describe', 'sleep', 'state', 'POLITE_YIELD', 'POLITE_YIELDS', 'console',
+ 'pcs', 'send', 'describe', 'sleep', 'state',
+ 'POLITE_YIELD', 'POLITE_YIELDS', 'NEGOTIATE_ROUNDS', 'console',
[lift(src, 'negotiate'), lift(src, 'politeYield'), lift(src, 'answeredExchange')].join('\n') +
'\nreturn { negotiate, answeredExchange };',
);
@@ -507,6 +510,7 @@ function negotiating(opts) {
yields: o.yields || 0,
negotiation: 0,
negotiating: false,
+ renegotiate: false,
makingOffer: false,
pc: {
signalingState: o.signalingState || 'stable',
@@ -529,6 +533,7 @@ function negotiating(opts) {
{ stopped: !!o.stopped },
POLITE_YIELD,
POLITE_YIELDS,
+ NEGOTIATE_ROUNDS,
quiet,
);
box = {
@@ -625,6 +630,50 @@ test('two negotiations while one sleeps produce one offer', async () => {
assert.deepEqual(n.sent, [['peer1', 'offer', true]]);
});
+test('a negotiation asked for during an abandoned yield still happens', async () => {
+ // The browser gate found this one, and it is the serializer's own
+ // trap. In `mutualshare`, the polite side adds its screen, sleeps in
+ // its yield, and answers the other side's offer while asleep. That
+ // exchange fires `negotiationneeded` for the polite side's *own*
+ // screen — which the serializer dropped, because an attempt was in
+ // flight — and then the attempt woke, saw the generation had moved,
+ // and abandoned its offer. Nobody was left to offer the screen: the
+ // transceiver sat unassociated for the rest of the call, and the far
+ // side showed a caption with nothing under it.
+ let once = true;
+ const n = negotiating({
+ polite: true,
+ duringYield: (entry, pcs, box) => {
+ if (!once) return;
+ once = false;
+ box.answeredExchange(entry);
+ // Exactly what the browser does next, into a negotiate that is
+ // still in flight.
+ box.negotiate('peer1', entry);
+ },
+ });
+ await n.negotiate('peer1', n.entry);
+ assert.deepEqual(n.sent, [['peer1', 'offer', true]],
+ 'the change asked for during the yield was never offered');
+ assert.equal(n.waits.length, 2, 'the second round gives way of its own accord');
+ assert.equal(n.entry.negotiating, false, 'the serializer must not stay latched');
+ assert.equal(n.entry.renegotiate, false);
+});
+
+test('an abandoned yield that nobody asked to repeat offers nothing', async () => {
+ // The other half: without a fresh ask, an attempt taken over by an
+ // exchange stays taken over. Otherwise every superseded yield would
+ // come back round and offer anyway, which is the extra offer the
+ // generation exists to prevent.
+ const n = negotiating({
+ polite: true,
+ duringYield: (entry, pcs, box) => box.answeredExchange(entry),
+ });
+ await n.negotiate('peer1', n.entry);
+ assert.deepEqual(n.sent, []);
+ assert.equal(n.waits.length, 1, 'it went round again with nothing to go round for');
+});
+
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