quic: write desired size needs update on maxstream

[martenrichter]

Without this update the streams can stall, if the chunks are close or bigger than the window size.
It was provoked by a very special timing, so probably hard to test,
Though I do not know, if this is also required for max data of the whole session.
Upstream says no:
ngtcp2/ngtcp2#2243

[nodejs-github-bot]

Review requested:

• @nodejs/quic

[martenrichter]

@jasnell @pimterry can you have a look?

[avivkeller]

Can you add a test?

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.13%. Comparing base (9024119) to head (cffb9bc).
⚠️ Report is 14 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #64768      +/-   ##
==========================================
- Coverage   90.15%   90.13%   -0.02%     
==========================================
  Files         743      744       +1     
  Lines      242407   242518     +111     
  Branches    45645    45700      +55     
==========================================
+ Hits       218532   218591      +59     
- Misses      15357    15434      +77     
+ Partials     8518     8493      -25     

see 40 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[martenrichter]

Can you add a test?

Not really. I found it in my webtransport test suite. But there it happens only, if I use node.js server and client. If I choose a Quiche client and a Node.js server, or a Node.js client and a Quiche server, it does not surface, as it depends on the particular packet size the particular Quic internals are generating at the low level. But I will later port all of my wt tests over, but therefore wt must be ready. (But this must not mean that these would reliably trigger the issue).

[pimterry]

Not really. I found it in my webtransport test suite.

I wanted to understand, so I did some digging. I think the trigger is actually quite clear: UpdateWriteDesiredSize was only called on ack, but the stream window is extended independently of acks. A peer can ack all our data, wait (all data acked but zero window available) and then extend the window size to allow more data later - so we do need to update desired size based on the window update alone 👍. Good find @martenrichter! This is tricky to trigger as it's easily obscured by buffering.

We can repro it reliably with something like this:

import { createPrivateKey } from 'node:crypto';
import { readFile } from 'node:fs/promises';
import { setTimeout as sleep } from 'node:timers/promises';
import { listen, connect } from 'node:quic';
import { drainableProtocol } from 'stream/iter';

const keys = 'test/fixtures/keys';
const key = createPrivateKey(await readFile(`${keys}/agent1-key.pem`));
const cert = await readFile(`${keys}/agent1-cert.pem`);

const WINDOW = 4096;
// Fills the window exactly: HTTP/3 spends 11 of those bytes on framing (8 for
// the HEADERS frame below, 3 for the DATA frame header). The send buffer then
// empties at the same moment the window reaches zero, leaving nothing in
// flight to ack. Any other size leaves bytes queued, and the ack for those
// wakes the writer instead, hiding the bug.
const BODY = WINDOW - 11;

let letServerRead;
const serverMayRead = new Promise((resolve) => { letServerRead = resolve; });

const endpoint = await listen((session) => {
  session.onstream = async (stream) => {
    await serverMayRead;
    for await (const _ of stream) { /* reading extends the window */ }
  };
}, {
  sni: { '*': { keys: [key], certs: [cert] } },
  transportParams: {
    initialMaxStreamDataBidiRemote: WINDOW,
    initialMaxData: 1024 * 1024,
  },
  onheaders() { this.sendHeaders({ ':status': '200' }); },
});

const session = await connect(endpoint.address, {
  servername: 'localhost',
  verifyPeer: 'manual',
});
await session.opened;

// Budget well above the window, so the window is what stops the writer.
const stream = await session.createBidirectionalStream({ budget: 1024 * 1024 });
stream.sendHeaders({
  ':method': 'POST',
  ':path': '/',
  ':scheme': 'https',
  ':authority': 'localhost',
}, { terminal: false });

const writer = stream.writer;
writer.writeSync(new Uint8Array(BODY));

// Long enough for every byte to be acked. The peer acks as data arrives,
// whether or not its application has read any of it, so by now the window is
// exhausted, the send buffer is empty, and no further ACK can arrive.
await sleep(500);
console.log('all acked, window exhausted');

const watchdog = setTimeout(() => {
  console.log('STALLED: no drain after MAX_STREAM_DATA');
  process.exit(1);
}, 5000);

letServerRead();                 // extend the window, with no ack attached
console.log('window extended');
await writer[drainableProtocol]();
console.log('writer drain');

clearTimeout(watchdog);
console.log('done');
process.exit(0);

This creates a server which doesn't read, so it'll ack everything without extending the window. The client writes the exact amount to fill the window but leave nothing in its buffers (otherwise the window update triggers more writes, and then gets acks). Then after a 500ms pause, the server reads and triggers a window update.

Fixed client will trigger a drain and complete OK, bad client will stall forever. We should be able to build a test around that directly.

The fix here covers this for HTTP/3, but doesn't totally fix the issue because we have two independent implementations of this with the current application structure, so we'll need to also separately fix the equivalent issue again for pure QUIC in the default app.

[martenrichter]

And we may also need it for session window, but I think we also need a callback from ngtcp2 for this.
Btw. that is the reason why there was so little progress from the last 3 weekends, it was horrible to debug (and I had a second error in the client code that was interfering).