grpc: add server

[0x0ece]

WIP

The first commit has fixes to the protocol that affect the client.

The rest does not impact the client, in the sense that even features in shared files are gated out (e.g. dynamic tables are still set to 0 in the client).

[github-actions]

 ┌─ ⚡ PERF · 640cf44 vs main@a755cc6 ─────────────────────────────────
 │ SUITE                               BASELINE          NEW         Δ
 │ replay tps, mainnet               30,951 tps   31,043 tps  ·  +0.30%
 │ bench tps, localnet              760,901 tps  763,038 tps  ·  +0.28%
 │ snapshot load, testnet               14.91 s      14.97 s  ·  +0.46%
 │ mem total, mainnet                158.40 GiB   158.40 GiB  ·   0.00%
 │ mem total, testnet                 89.40 GiB    89.40 GiB  ·   0.00%
 │ mem total, ag mainnet             189.91 GiB   189.91 GiB  ·   0.00%
 │ mem total, ag testnet             120.91 GiB   120.91 GiB  ·   0.00%
!│ clean compile, firedancer             2.94 s       3.11 s  ▲  +5.88%
 │ binary size, firedancer             55.24 MB     55.25 MB  ·  +0.03%
 ├─────────────────────────────────────────────────────────────────────
@@ 0 REGRESSIONS · 1 WARNING · 0 IMPROVED · 8 NOISE @@
 └─────────────────────────────────────────────────────────────────────

history · 11 pushes

 ┌─ HISTORY · Δ vs main, per push, newest first ───────────────────────────────────────────
 │ HEAD         TPS    BENCH     SNAP    MEM·M    MEM·T     AG·M     AG·T  COMPILE   BINARY
!│ 640cf44   +0.30%   +0.28%   +0.46%    0.00%    0.00%    0.00%    0.00%   +5.88%   +0.03%
!│ 94b3b83   +1.37%   +0.22%   +0.83%    0.00%    0.00%    0.00%    0.00%   +5.60%   +0.03%
 │ 41ed245   +0.07%   +0.37%   -0.46%    0.00%    0.00%    0.00%    0.00%   +0.07%   +0.03%
 │ 43bb0a6   -0.17%   -0.03%   +0.49%    0.00%    0.00%    0.00%    0.00%   -1.57%   +0.03%
 │ d8c2862   -0.13%   +0.32%   +0.56%    0.00%    0.00%    0.00%    0.00%   +2.65%   +0.03%
 │ 149c22b   -0.20%   -0.16%   +0.62%    0.00%    0.00%    0.00%    0.00%   +1.59%   +0.03%
-│ 2788947   -0.25%   -0.11%   -0.36%    0.00%    0.00%    0.00%    0.00%   +6.12%   +0.03%
 │ 54cc05a   +0.28%   -0.12%   +0.52%    0.00%    0.00%    0.00%    0.00%   +1.64%   +0.02%
 │ 1e63a01   +0.60%   -0.15%   +1.21%    0.00%    0.00%    0.00%    0.00%   +0.88%   +0.02%
 │ e56a706   +0.22%   +0.34%   +0.37%    0.00%    0.00%    0.00%    0.00%   +1.55%   +0.02%
 │ 56f9db7   -0.13%   +0.31%   +0.97%    0.00%    0.00%    0.00%    0.00%   +2.74%   +0.02%
 └─────────────────────────────────────────────────────────────────────────────────────────

[Copilot]

🟡 Changes recommended

Unresolved critical and moderate correctness issues remain in gRPC request handling and HTTP/2 protocol behavior.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Adds a fixed-memory, callback-based gRPC server over HTTP/2 with HPACK dynamic-table support.

Changes:

• Adds gRPC server APIs, lifecycle, flow control, compression, and transports.
• Extends HPACK and HTTP/2 framing, continuation, and flow-control handling.
• Adds tests, fuzz targets, and build integration.

File summaries

File	Summary
src/waltz/h2/test_hpack.c	HPACK vectors and malformed-input tests
src/waltz/h2/test_h2_server.c	HTTP/2 server callback and header tests
src/waltz/h2/test_h2_server_sequences.c	Fragmentation and flow-control sequence tests
src/waltz/h2/test_h2_conn.c	HTTP/2 connection tests
src/waltz/h2/README.md	HTTP/2 behavior documentation
src/waltz/h2/fuzz_hpack_rd.c	HPACK decoder fuzz target
src/waltz/h2/fuzz_h2.c	HTTP/2 fuzz target
src/waltz/h2/fd_hpack.h	HPACK table APIs
src/waltz/h2/fd_hpack.c	Dynamic-table decoding
src/waltz/h2/fd_hpack_private.h	HPACK decoder internals
src/waltz/h2/fd_h2_stream.h	HTTP/2 stream state and lifecycle
src/waltz/h2/fd_h2_proto.h	HTTP/2 protocol flags
src/waltz/h2/fd_h2_conn.h	HTTP/2 connection state
src/waltz/h2/fd_h2_conn.c	Continuations, flow control, and header handling
src/waltz/grpc/Local.mk	Build and test integration
src/waltz/grpc/fuzz_grpc_server.c	gRPC server fuzz target
src/waltz/grpc/fd_grpc_server.h	Public gRPC server API
src/waltz/grpc/fd_grpc_server.c	gRPC server implementation
src/waltz/grpc/fd_grpc_server_private.h	Internal server state and buffers

Review details

Suppressed comments (11)

src/waltz/grpc/fd_grpc_server.c:1228

• fd_grpc_server_finish is documented as callable from callbacks, including stream_hdr. At this point the stream is still in HEADERS; finish changes it to FINISH, but this unconditional assignment overwrites that state and then invokes stream_open, losing the handler's requested status. Preserve FINISH (and drop subsequent request data) before transitioning to ACTIVE.

  stream->state = FD_GRPC_SERVER_STREAM_ACTIVE;

src/waltz/grpc/fd_grpc_server.c:1867

• The shutdown path resets only tx_queue; a stream that owns a large slot still has large_idx and large_rem pending. The next flush therefore emits the oversized response before the UNAVAILABLE trailers and can remain blocked on flow control, contrary to the shutdown guarantee and the comment that pending sends are dropped. Release the large slot and reset the queue accounting here as well.

      fd_h2_rbuf_init( s->tx_queue, s->tx_queue->buf0, s->tx_queue->bufsz );

src/waltz/grpc/fd_grpc_server.c:1590

• conn_release keeps conn->active true while it invokes stream_close callbacks. Those callbacks are explicitly allowed to call fd_grpc_server_conn_close, which re-enters conn_release; the nested release can decrement conn_cnt and invoke close callbacks a second time, and the outer release then repeats the teardown. Mark release-in-progress before notifying streams or otherwise guard this path so callback re-entry is idempotent.

    if( s->state!=FD_GRPC_SERVER_STREAM_FREE ) {
      fd_grpc_server_stream_end( s, FD_GRPC_SERVER_CLOSE_CONN_LOST );
    }

src/waltz/grpc/fd_grpc_server.c:1516

• stream_msg is allowed to call fd_grpc_server_conn_close through the documented re-entrant callback contract. In that case fd_grpc_server_rx_data returns with this stream released, but the next line still calls fd_grpc_server_rx_fin( stream ) for END_STREAM, using a stream whose lifetime has ended. Check that the connection and stream are still active after delivering the message before processing END_STREAM.

  fd_grpc_server_rx_data( stream, data, data_sz );
  if( flags & FD_H2_FLAG_END_STREAM ) fd_grpc_server_rx_fin( stream );

src/waltz/grpc/fd_grpc_server.c:1114

• max_header_list_sz is enforced here only against encoded field-block bytes. HPACK indexed fields can make a block well below that bound expand to many times the configured decoded header-list size, so a peer can bypass the intended per-request header/resource limit and force all of those fields through the loop and application callbacks. If this parameter is the hard request limit documented by the server API, accumulate decoded name/value bytes plus 32 per field and reject once it is exceeded.

  while( !fd_hpack_rd_done( hpack_rd ) ) {
    uchar *     scratch = server->hpack_scratch;
    fd_h2_hdr_t hdr[1];
    uint err = fd_hpack_rd_next( hpack_rd, hdr, &scratch, server->hpack_scratch+scratch_sz );

src/waltz/grpc/fd_grpc_server.c:1351

• FD_GRPC_SERVER_ACCEPT_UNARY is documented as accepting one request message, but this path never records or rejects completed messages. A client can send two complete gRPC messages before END_STREAM and both are delivered to stream_msg (and an END_STREAM with no message is also accepted). Track the unary message count and finish with a protocol/status error unless exactly one message is received.

  server->metrics.rx_msg_cnt++;
  if( stream->flags & FD_GRPC_SERVER_STREAM_FLAG_APP_OPEN ) {
    server->callbacks->stream_msg( server->app_ctx, stream, msg, msg_sz );
  }

src/waltz/grpc/fd_grpc_server.c:1159

• The API documents that paths longer than FD_GRPC_SERVER_PATH_MAX cannot match a route, but this truncates the value and passes the prefix to stream_open. A service with a route equal to that prefix will therefore accept a longer, distinct path. Preserve an overlength indicator and reject it before route matching instead of silently truncating it for dispatch.

      case FD_H2_HDR_PATH:
        if( FD_UNLIKELY( !value_len ) ) { malformed = 1; break; }
        stream->path_len = (ushort)fd_ulong_min( value_len, FD_GRPC_SERVER_PATH_MAX );
        fd_memcpy( stream->path, value, stream->path_len );

src/waltz/grpc/fd_grpc_server.c:1189

• A valid grpc-timeout can parse to a positive value close to LONG_MAX, but adding it to the current wallclock can overflow. For example, 2562047H is accepted by the parser and can make stream->deadline negative, causing an immediate deadline-exceeded result instead of a long deadline. Saturate the addition at LONG_MAX.

    case FD_GRPC_SERVER_HDR_TIMEOUT: {
      long timeout = fd_grpc_server_parse_timeout( value, value_len );
      if( timeout!=LONG_MAX ) stream->deadline = server->now + timeout;
      continue;

src/waltz/grpc/fd_grpc_server.c:576

• This reports positive space whenever the byte ring has room, even if stream_tx_queue_msg_max has already been reached. fd_grpc_server_send then returns FD_GRPC_SERVER_ERR_AGAIN for a queue-sized message, contradicting the API guarantee that zero means the next such send will not fit and can prevent correct backpressure handling. Include the pending-message limit in this result.

ulong
fd_grpc_server_stream_tx_free_sz( fd_grpc_server_stream_t const * stream ) {
  ulong free_sz = fd_h2_rbuf_free_sz( stream->tx_queue );
  return free_sz<sizeof(fd_grpc_hdr_t) ? 0UL : free_sz-sizeof(fd_grpc_hdr_t);

src/waltz/grpc/fd_grpc_server.c:1206

• content-length is a recognized HTTP/2 header but falls through to stream_hdr without being parsed or compared with the total DATA bytes. HTTP/2 requires a request with this field to be treated as malformed when the value is invalid or does not equal the DATA length, so a request advertising 0 while carrying a gRPC body is currently accepted and dispatched.

    default:
      break;
    }

    server->callbacks->stream_hdr( server->app_ctx, stream, name, name_len, value, value_len );

src/waltz/h2/fd_h2_conn.c:220

• SETTINGS_MAX_HEADER_LIST_SIZE limits the decompressed header list (the sum of each name, value, and 32-byte entry overhead), but this accounts only for encoded HPACK bytes. A peer can insert a value once and then repeat a dynamic index many times, so a small field block can expand far beyond the advertised limit and still reach the callbacks. Enforce the decoded header-list accounting while decoding, rather than using payload_sz here.

fd_h2_rx_hdrs_account( fd_h2_conn_t * conn,
                       ulong          payload_sz ) {
  ulong hdrs_sz = (ulong)conn->rx_hdrs_sz + payload_sz;
  if( FD_UNLIKELY( hdrs_sz > conn->self_settings.max_header_list_size ) ) {
    fd_h2_conn_error( conn, FD_H2_ERR_ENHANCE_YOUR_CALM );
    return 0;

• Files reviewed: 20/20 changed files
• Comments generated: 6
• Review effort level: Lite (auto)

Note

Copilot is running an experiment and ran this review at Lite.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🟡 Changes recommended

Unresolved critical and moderate correctness and safety issues remain.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (11)

src/waltz/grpc/fd_grpc_server.c:1175

• FD_GRPC_SERVER_PATH_MAX is documented as making longer paths unmatchable and UNIMPLEMENTED, but this stores only the prefix and later passes it to stream_open. A route equal to that prefix can therefore accept a request whose actual :path has extra bytes. Preserve an overlength marker and reject it as UNIMPLEMENTED, or make route matching see the full length instead of silently truncating it.

        if( FD_UNLIKELY( !value_len ) ) { malformed = 1; break; }
        stream->path_len = (ushort)fd_ulong_min( value_len, FD_GRPC_SERVER_PATH_MAX );
        fd_memcpy( stream->path, value, stream->path_len );

src/waltz/grpc/fd_grpc_server.c:2192

• Passing timeout_millis directly to poll can block past every server timer deadline. With -1 (or a timeout longer than a deadline), a quiet connection will not run deadline, keepalive, or idle handling until socket activity, contrary to the documented fd_grpc_server_poll timer behavior. Bound the poll timeout by the next timer deadline or service timers before blocking.

    ready = poll( pfd, (nfds_t)pfd_cnt, timeout_millis );

src/waltz/grpc/fd_grpc_server.c:1957

• Resetting only tx_queue does not release an in-flight large-message slot. fd_grpc_server_stream_flush still sees large_idx and will continue sending that oversized response during shutdown, contrary to the comment that queued messages are dropped; with a stalled client this can also keep the connection until the drain timeout. Release the large slot before clearing the queue.

      fd_h2_rbuf_init( s->tx_queue, s->tx_queue->buf0, s->tx_queue->bufsz );

src/waltz/grpc/fd_grpc_server.c:1200

• The prefix-only check accepts values such as application/grpc-malicious as valid gRPC content types, routing non-gRPC requests into the gRPC message parser. Accept the exact application/grpc value or a non-empty +format suffix rather than any string beginning with the prefix.

      content_type = ( value_len>=sizeof(FD_GRPC_SERVER_CONTENT_TYPE)-1UL ) &&
                     fd_memeq( value, FD_GRPC_SERVER_CONTENT_TYPE, sizeof(FD_GRPC_SERVER_CONTENT_TYPE)-1UL );

src/waltz/grpc/fd_grpc_server.c:1626

• Reducing SETTINGS_INITIAL_WINDOW_SIZE is allowed to make an existing stream's send window negative; RFC 9113 requires pausing that stream until it becomes positive, not a connection FLOW_CONTROL_ERROR. This unsigned-window check makes a legal settings reduction tear down the connection.

    long wnd = (long)s->h2->tx_wnd + delta;
    if( FD_UNLIKELY( wnd<0L || wnd>0x7fffffffL ) ) {
      fd_h2_conn_error( h2, FD_H2_ERR_FLOW_CONTROL );
      return;

src/waltz/grpc/fd_grpc_server.c:1759

• The callback is documented as re-entrant, so it may close conn itself. If conn_open does that and returns zero, this condition is false and the function returns an already inactive connection at line 1760, even though the caller believes initialization succeeded.

  if( FD_UNLIKELY( server->callbacks->conn_open( server->app_ctx, conn ) ) ) {
    conn->sock = -1;
    fd_grpc_server_conn_release( conn );
    return NULL;
  }

src/waltz/grpc/fd_grpc_server.c:580

• This helper reports available queue capacity without considering stream_tx_queue_msg_max. Once the message-count limit is reached it can return a positive value even though the next queue-sized fd_grpc_server_send is guaranteed to return FD_GRPC_SERVER_ERR_AGAIN, making the public backpressure query misleading.

fd_grpc_server_stream_tx_free_sz( fd_grpc_server_stream_t const * stream ) {
  ulong free_sz = fd_h2_rbuf_free_sz( stream->tx_queue );
  return free_sz<sizeof(fd_grpc_hdr_t) ? 0UL : free_sz-sizeof(fd_grpc_hdr_t);

src/waltz/h2/README.md:42

• These lines say the client passes a NULL table to fd_hpack_rd_init, but the implementation initializes a zero-sized rx_hpack and calls fd_hpack_rd_init_dtable with its address. The behavior for dynamic references is equivalent, but the documented API path is inaccurate and omits how size updates are handled; describe the zero-sized table/init_dtable path instead.

applied the limit by then.  A client therefore passes a NULL dynamic
table to `fd_hpack_rd_init`.

src/waltz/h2/fd_h2_conn.c:268

• These fields are initialized before stream admission, but the refusal returns below do not advance rx_stream_next. If the refused stream's slot is freed before a higher peer stream arrives, the same stream ID can be presented again and accepted as a fresh stream, despite HTTP/2 stream IDs being single-use and increasing. Advance the receive next ID when a new peer HEADERS stream is refused as well, while retaining rx_stream_id to drop its CONTINUATION frames.

  conn->rx_stream_id = stream_id;
  conn->rx_hdrs_sz   = 0U;
  if( FD_UNLIKELY( !( frame_flags & FD_H2_FLAG_END_HEADERS ) ) ) {
    conn->flags |= FD_H2_CONN_FLAGS_CONTINUATION;
  }

src/waltz/h2/fd_h2_conn.c:344

• When the initial HEADERS carries END_STREAM without END_HEADERS, fd_h2_rx_headers has already moved the stream to CLOSING_RX. The unconditional fd_h2_stream_rx_headers call above treats CLOSING_RX as illegal, so the valid CONTINUATION sequence exercised by h2_server_continuation raises a connection error instead of completing the field block. Handle continuation chunks separately so the final chunk can increment the header sequence without reopening or rejecting the already half-closed receive side.

  cb->headers( conn, stream, payload, payload_sz, frame_flags | FD_H2_VFLAG_CONTINUATION );

src/waltz/h2/fd_h2_conn.c:220

• This applies the advertised SETTINGS_MAX_HEADER_LIST_SIZE to raw HPACK bytes, although RFC 9113 defines that limit using decoded field name/value sizes. The server already performs the decoded-size check later, so a valid compressed field block can be rejected solely because its wire representation is larger than the setting.

  if( FD_UNLIKELY( hdrs_sz > conn->self_settings.max_header_list_size ) ) {
    fd_h2_conn_error( conn, FD_H2_ERR_ENHANCE_YOUR_CALM );

• Files reviewed: 20/20 changed files
• Comments generated: 7
• Review effort level: Lite (auto)

Note

Copilot is running an experiment and ran this review at Lite.

[Copilot]

🟡 Changes recommended

Refused-stream HPACK state, stream reuse, shutdown descriptors, routing, timers, and unary cardinality have unresolved correctness issues.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (1)

src/waltz/grpc/fd_grpc_server.c:1263

• FD_GRPC_SERVER_ACCEPT_UNARY promises a single request message, but this flag is only consulted by the deadline timer; every framed message is still delivered to stream_msg until the peer half-closes. A unary handler that does not finish on its first callback can therefore receive multiple requests. Track the first delivered message and reject a second one (or revise the public API if unary is not meant to enforce cardinality).

  server->metrics.stream_open_cnt++;
  stream->flags |= FD_GRPC_SERVER_STREAM_FLAG_APP_OPEN;
  if( kind==FD_GRPC_SERVER_ACCEPT_UNARY ) stream->flags |= FD_GRPC_SERVER_STREAM_FLAG_UNARY;

• Files reviewed: 21/21 changed files
• Comments generated: 5
• Review effort level: Balanced

[Copilot]

🟡 Changes recommended

Unresolved critical and moderate findings affect HPACK safety, HTTP/2 state, and gRPC stream handling.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (7)

src/waltz/grpc/fd_grpc_server.c:1232

• HTTP/2 requests (including gRPC calls) require :authority in addition to :method, :scheme, and :path (apart from CONNECT, which this server does not accept). Because this mask omits FD_GRPC_SERVER_PSEUDO_AUTHORITY, a request without an authority reaches stream_open instead of being rejected as malformed.

  uint pseudo_need = FD_GRPC_SERVER_PSEUDO_METHOD |
                     FD_GRPC_SERVER_PSEUDO_SCHEME |
                     FD_GRPC_SERVER_PSEUDO_PATH;
  if( FD_UNLIKELY( ( pseudo_seen & pseudo_need )!=pseudo_need ) ) malformed = 1;

src/waltz/grpc/fd_grpc_server.c:1602

• closed_by==0 is also used for local RST_STREAMs generated by malformed-header and flow-control errors, but this maps every one to FD_GRPC_SERVER_CLOSE_FINISHED. The public API documents that reason as “trailers were sent”; these paths send no trailers, so handlers cannot distinguish successful completion from an aborted call. Add a distinct local-abort reason or update the contract and all callers consistently.

  fd_grpc_server_stream_t * stream = fd_grpc_server_stream_from_h2( h2_stream );
  fd_grpc_server_stream_end( stream, closed_by ? FD_GRPC_SERVER_CLOSE_CANCELLED
                                               : FD_GRPC_SERVER_CLOSE_FINISHED );

src/waltz/grpc/fd_grpc_server.h:39

• This summary lists stream_open before stream_hdr, but the callback contract below and the implementation invoke metadata callbacks before stream_open. Consumers following this list can initialize per-stream state too early or assume headers are complete when they are not; make the documented order match the actual API.

     stream_open                   request headers complete
     stream_hdr    (0 or more)     request metadata, before stream_open
     stream_msg    (0 or more)     one per complete request message
     stream_half_close             client sent END_STREAM

src/waltz/h2/fd_h2_conn.c:339

• When a stream is refused, the HEADERS block is dropped here without being decoded. HPACK's dynamic table is connection-scoped, so that block can contain incremental-indexing literals or table-size updates; dropping its HEADERS/CONTINUATION chunks leaves rx_hpack out of sync and can make a later valid stream fail with COMPRESSION_ERROR. Accumulate and decode the refused block through END_HEADERS without invoking stream callbacks before discarding it.

  fd_h2_stream_t * stream = cb->stream_query( conn, stream_id );
  if( FD_UNLIKELY( !stream ) ) return 1;

src/waltz/h2/fd_h2_conn.c:279

• When the concurrency limit or stream_create rejects a new stream, this returns without decoding its HEADERS field block. HPACK dynamic-table updates are connection-scoped, so an incremental-indexing literal on a refused stream still has to update rx_hpack; otherwise later valid streams are decoded against a different table state. Consume/decode the complete block without delivering it to the application before discarding the refused stream.

    if( FD_UNLIKELY( conn->stream_active_cnt[0] >= conn->self_settings.max_concurrent_streams ) ) {
      fd_h2_tx_rst_stream( rbuf_tx, stream_id, FD_H2_ERR_REFUSED_STREAM );
      return 1;
    }

src/waltz/h2/fd_hpack.c:350

• For a literal that uses a dynamic entry only for its name, fd_hpack_dtable_query copies the old entry's name and value into scratch, but the next lines replace hdr->value with the new literal value. The unnecessary old value can make a valid header fail with FD_HPACK_DTABLE_SZ_MAX scratch (for example, a 4090-byte dynamic entry whose 2-KiB name is reused with a 50-byte value). Use a name-only lookup/copy for this representation.

    uint err = fd_hpack_rd_indexed( rd, hdr, name_idx, scratch, scratch_end );
    if( FD_UNLIKELY( err ) ) return FD_H2_ERR_COMPRESSION;
    hdr->value     = (char const *)value_p;

src/waltz/h2/test_h2_server.c:141

• This callback still creates a fresh HPACK reader for each callback payload. With the new CONTINUATION behavior, a valid literal split across frames is decoded as an incomplete block and causes a compression GOAWAY; the README requires callers to concatenate chunks through END_HEADERS before decoding. Reassemble the chunks here and decode only the completed field block.

  if( FD_UNLIKELY( !fd_hpack_rd_init_dtable( hpack_rd, data, data_sz, &conn->rx_hpack ) ) ) {
    FD_LOG_WARNING(( "Error reading headers (bad HPACK table size update)" ));
    fd_h2_conn_error( conn, FD_H2_ERR_COMPRESSION );
    return;
  }

• Files reviewed: 20/20 changed files
• Comments generated: 3
• Review effort level: Lite (auto)

Note

Copilot is running an experiment and ran this review at Lite.

[Copilot]

🟡 Changes recommended

Unresolved HTTP/2 and HPACK handling issues, along with required callback and test fixes, block approval.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (5)

src/waltz/grpc/fd_grpc_server.h:38

• The documented call order contradicts itself: the list puts stream_open before stream_hdr, while the stream_hdr description and implementation require metadata callbacks before stream_open. This is a public lifecycle contract, so callers following the list can initialize handler state too late. Reorder these entries so stream_hdr precedes stream_open.

   Handlers see the following call order on a stream:

     stream_open                   request headers complete
     stream_hdr    (0 or more)     request metadata, before stream_open
     stream_msg    (0 or more)     one per complete request message

src/waltz/h2/fd_h2_conn.c:220

• SETTINGS_MAX_HEADER_LIST_SIZE applies to the decoded header list (name length + value length + 32 per field), not the encoded HPACK field-block bytes. Counting payload bytes here rejects valid blocks whose Huffman representation expands, and can also reject blocks containing legal dynamic-table size updates, before HPACK decoding can apply the correct limit. Use a separate encoded-buffer/resource bound and enforce the advertised setting after decoding.

  if( FD_UNLIKELY( hdrs_sz > conn->self_settings.max_header_list_size ) ) {
    fd_h2_conn_error( conn, FD_H2_ERR_ENHANCE_YOUR_CALM );

src/waltz/h2/fd_h2_conn.c:168

• The refill test loses valid small receive windows because stream_wnd_max/2U truncates to zero when the advertised initial window is 1. After receiving the one permitted byte, rx_wnd is 0 and the condition remains false forever, so the peer never gets a stream WINDOW_UPDATE and the stream stalls. Use a non-truncating half comparison (for example stream->rx_wnd*2U < stream_wnd_max) or otherwise handle the stream_wnd_max==1 boundary.

                   ( stream->rx_wnd < stream_wnd_max/2U     ) &
                   ( stream_wnd_max <= 0x7fffffffU          ) ) ) {

src/waltz/h2/fd_h2_conn.c:339

• Dropping the remainder when stream_query returns NULL has the same HPACK state-loss problem for a stream reset or refused while its header block is fragmented. The peer's CONTINUATION bytes still belong to the connection compression context; ignoring an incremental-indexing block leaves rx_hpack out of sync for later streams. Decode/discard the complete block rather than returning here.

  /* The stream was refused or reset while the field block was still in
     flight.  The peer was already told, so drop the rest of the block. */
  fd_h2_stream_t * stream = cb->stream_query( conn, stream_id );
  if( FD_UNLIKELY( !stream ) ) return 1;

src/waltz/h2/test_h2_server.c:141

• This callback now receives CONTINUATION chunks, but it still initializes and decodes HPACK independently for each callback. A literal split across HEADERS and CONTINUATION is therefore reported as a compression error on the first chunk, so the test server rejects valid fragmented requests. Buffer the chunks through END_HEADERS before the single HPACK decode, as required by the updated callback contract.

  if( FD_UNLIKELY( !fd_hpack_rd_init_dtable( hpack_rd, data, data_sz, &conn->rx_hpack ) ) ) {
    FD_LOG_WARNING(( "Error reading headers (bad HPACK table size update)" ));
    fd_h2_conn_error( conn, FD_H2_ERR_COMPRESSION );
    return;
  }

• Files reviewed: 20/20 changed files
• Comments generated: 3
• Review effort level: Lite (auto)

Note

Copilot is running an experiment and ran this review at Lite.

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[Copilot]

🟡 Changes recommended

Valid HTTP/2 traffic can be rejected or desynchronize HPACK state, alongside several lifecycle and API contract issues.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (2)

src/waltz/h2/fd_h2_conn.c:223

• SETTINGS_MAX_HEADER_LIST_SIZE limits the decoded field-list size (name + value + 32 per field), not the number of compressed bytes on the wire. A conforming peer can therefore send a Huffman representation whose wire size exceeds this value while its decoded list remains within the advertised limit; this check closes that valid connection. Keep a separate implementation limit for accumulated compressed bytes and enforce the advertised setting after HPACK decoding.

  ulong hdrs_sz = (ulong)conn->rx_hdrs_sz + payload_sz;
  if( FD_UNLIKELY( hdrs_sz > conn->self_settings.max_header_list_size ) ) {
    fd_h2_conn_error( conn, FD_H2_ERR_ENHANCE_YOUR_CALM );
    return 0;

src/waltz/h2/fd_h2_conn.c:280

• Refused streams still carry HPACK state. Dropping this block without decoding its incremental-indexing records desynchronizes rx_hpack, so after the active-stream count falls a valid later request that references one of those entries will fail with COMPRESSION_ERROR. HTTP/2 requires HEADERS/CONTINUATION processing sufficient to preserve the connection compression context even when the stream is refused; decode-and-discard the complete field block before continuing the connection.

    /* A refused block is dropped undecoded, so its dynamic table
       insertions are not applied: a peer that opens more streams than
       it was granted desyncs its own HPACK table. */

• Files reviewed: 19/19 changed files
• Comments generated: 5
• Review effort level: Balanced

[Copilot]

🟡 Changes recommended

Legal HTTP/2 requests can currently break HPACK synchronization or be rejected, and several server lifecycle semantics are incorrect.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (4)

src/waltz/grpc/fd_grpc_server.c:1538

• A legal HTTP/2 field block may span HEADERS and CONTINUATION frames. Treating the first fragment as a COMPRESSION_ERROR makes the server incompatible with conforming clients whenever their encoder splits a request field block. Reassemble all fragments through END_HEADERS before invoking the HPACK decoder instead of closing the connection.

  /* One frame carries one whole field block.  A peer that splits one
     across CONTINUATION frames is refused rather than reassembled. */
  if( FD_UNLIKELY( !( flags & FD_H2_FLAG_END_HEADERS ) ) ) {
    fd_h2_conn_error( h2, FD_H2_ERR_COMPRESSION );
    return;

src/waltz/h2/fd_h2_conn.c:280

• HPACK state is connection-wide and still has to be updated for a field block whose stream is refused. Dropping this block means a conforming peer can incrementally index a field here and reference it on its next accepted stream; this decoder will then use a different table and terminate the connection with COMPRESSION_ERROR. Consume/decode refused field blocks, including their CONTINUATION frames, while suppressing application delivery.

    /* A refused block is dropped undecoded, so its dynamic table
       insertions are not applied: a peer that opens more streams than
       it was granted desyncs its own HPACK table. */

src/waltz/grpc/fd_grpc_server.c:1880

• CLOSE_CANCELLED is documented as a client-originated RST_STREAM, but this timeout is a local server abort. Reporting CANCELLED prevents handlers from distinguishing peer cancellation from the server's stalled-send policy; use CLOSE_ABORTED.

      fd_grpc_server_stream_end( s, FD_GRPC_SERVER_CLOSE_CANCELLED );

src/waltz/h2/fd_h2_conn.c:222

• SETTINGS_MAX_HEADER_LIST_SIZE limits the decoded field-section size (name_len + value_len + 32 per field), not the HPACK wire payload. Comparing compressed bytes to the same value rejects legal blocks—for example, deliberately inefficient Huffman coding can be larger than its decoded field list. Keep a separate encoded-block/CONTINUATION safety limit, while enforcing this setting after HPACK decoding.

  ulong hdrs_sz = (ulong)conn->rx_hdrs_sz + payload_sz;
  if( FD_UNLIKELY( hdrs_sz > conn->self_settings.max_header_list_size ) ) {
    fd_h2_conn_error( conn, FD_H2_ERR_ENHANCE_YOUR_CALM );

• Files reviewed: 19/19 changed files
• Comments generated: 3
• Review effort level: Balanced

[Copilot]

🟡 Changes recommended

Valid fragmented or refused header blocks can desynchronize HPACK state, and queued responses can stall before polling.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (4)

src/waltz/h2/fd_h2_conn.c:223

• SETTINGS_MAX_HEADER_LIST_SIZE limits the decoded header-list size (name + value + 32 per field), not the encoded HEADERS/CONTINUATION payload. A conforming peer can use a valid Huffman representation whose wire size exceeds its decoded size, so this comparison can reject a header list that is within the advertised limit. Keep any encoded-block safety cap separate from this setting and enforce this setting after HPACK decoding, as the gRPC decoder already does.

  if( FD_UNLIKELY( hdrs_sz > conn->self_settings.max_header_list_size ) ) {
    fd_h2_conn_error( conn, FD_H2_ERR_ENHANCE_YOUR_CALM );
    return 0;

src/waltz/h2/fd_h2_conn.c:280

• Dropping a refused field block without applying its HPACK updates desynchronizes the connection compression context even for a conforming client. REFUSED_STREAM rejects application processing, but every received field block still has to be decoded to maintain the dynamic table; otherwise a later legal stream that references an insertion from this block is incorrectly terminated with COMPRESSION_ERROR. Decode-and-discard refused blocks, including their CONTINUATION fragments.

    /* A refused block is dropped undecoded, so its dynamic table
       insertions are not applied: a peer that opens more streams than
       it was granted desyncs its own HPACK table. */

src/waltz/grpc/fd_grpc_server.c:1552

• CONTINUATION is a normal HTTP/2 mechanism, and HPACK records may cross frame boundaries. This closes otherwise valid gRPC connections whenever a client fragments request headers or trailers. Reassemble the field block per connection until END_HEADERS, preserve the initial HEADERS END_STREAM state, and decode it once complete instead of treating fragmentation as a compression error.

  /* One frame carries one whole field block.  A peer that splits one
     across CONTINUATION frames is refused rather than reassembled. */
  if( FD_UNLIKELY( !( flags & FD_H2_FLAG_END_HEADERS ) ) ) {
    fd_h2_conn_error( h2, FD_H2_ERR_COMPRESSION );
    return;

src/waltz/grpc/fd_grpc_server.c:2182

• Pending application sends are held in per-stream queues, but these descriptors request POLLOUT only after bytes have reached rbuf_tx, and that flush happens after poll(). If a handler queues or finishes a response between poll calls, a long or infinite timeout can block before the response is moved to the socket buffer. Service the server before building the poll set so queued responses make the connection writable immediately.

    short events = POLLIN;
    if( fd_h2_rbuf_used_sz( conn->rbuf_tx ) ) events = (short)( events|POLLOUT );
    pfd[ pfd_cnt++ ] = (struct pollfd){ .fd = conn->sock, .events = events };

• Files reviewed: 19/19 changed files
• Comments generated: 2
• Review effort level: Balanced

[Copilot]

Copilot review overview

🟡 Changes recommended

Valid HTTP/2 continuations are rejected and refused streams can desynchronize connection-wide HPACK state.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 5 · 12

Open (17)

• When the initial HEADERS carries END_STREAM, fd_h2_rx_headers has already moved the stream to…
• Stream IDs may be skipped. If a peer opens stream 5 first, stream 3 is still idle, but this…
• The decoder bounds name_len, but it does not similarly bound value_len or check remaining bytes…
• stream_msg is allowed to call fd_grpc_server_conn_close per the public callback contract. That…
• A refused field block is discarded without being HPACK-decoded. Literals with incremental indexing…
• Prevent msg_sz capacity check overflow · New
• Service queued output before building the poll set · New
• Honor poll timeout when no descriptors exist · New
• Enforce grpc-timeout for streaming RPCs · New
• grpc-timeout defines a deadline for the whole RPC regardless of whether it is unary or streaming.…
• A HEADERS field block may legally be split across CONTINUATION frames, and the H2 layer now…
• Deleting a server with active connections closes the descriptors directly but never calls…
• Returning after sending REFUSED_STREAM skips decoding this field block, including any…
• SETTINGS_MAX_HEADER_LIST_SIZE is defined in terms of the decoded/uncompressed header-list size,…
• Shutdown leaves the owned listen socket open, and fd_grpc_server_poll continues polling and…
• When conn_cnt reaches the configured maximum, this condition omits the listen fd, so…
• FD_GRPC_SERVER_ACCEPT_UNARY promises one request message, but this flag is used only by the…

Resolved since last review (2)

• This bounds-check forms out+data_len before proving that the result is within the scratch object.…
• The timeout grammar permits 0n, for which stream->deadline equals now. Using a strict…

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[Copilot]

Copilot review overview

🟡 Changes recommended

Valid continuation handling, refused-stream HPACK synchronization, and fragmented DATA flow control remain incorrect.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 6 · 9

Open (15)

• Refill stream window only after consuming the full DATA frame · New
• When the initial HEADERS carries END_STREAM, fd_h2_rx_headers has already moved the stream to…
• Stream IDs may be skipped. If a peer opens stream 5 first, stream 3 is still idle, but this…
• The decoder bounds name_len, but it does not similarly bound value_len or check remaining bytes…
• stream_msg is allowed to call fd_grpc_server_conn_close per the public callback contract. That…
• A refused field block is discarded without being HPACK-decoded. Literals with incremental indexing…
• Enforce grpc-timeout for streaming RPCs
• grpc-timeout defines a deadline for the whole RPC regardless of whether it is unary or streaming.…
• A HEADERS field block may legally be split across CONTINUATION frames, and the H2 layer now…
• Deleting a server with active connections closes the descriptors directly but never calls…
• Returning after sending REFUSED_STREAM skips decoding this field block, including any…
• SETTINGS_MAX_HEADER_LIST_SIZE is defined in terms of the decoded/uncompressed header-list size,…
• Shutdown leaves the owned listen socket open, and fd_grpc_server_poll continues polling and…
• When conn_cnt reaches the configured maximum, this condition omits the listen fd, so…
• FD_GRPC_SERVER_ACCEPT_UNARY promises one request message, but this flag is used only by the…

Resolved since last review (1)

• Service queued output before building the poll set