feat: add HTTP/3 support and related functionality - #929
niteshpurohit wants to merge 9 commits into
Conversation
- Implemented HTTP/3 session management in `http3.cpp` to handle HTTP/3 connections. - Added QUIC session management in `quic.cpp` to support QUIC protocol operations. - Introduced arena memory management in `arena_memory.hpp` for efficient memory allocation. - Updated `dependency.cpp` to include new dependency operations for QUIC and HTTP/3. - Created tests for HTTP/3 functionality in `http3.cpp` and `http3_contract.cpp` to ensure correctness. - Enhanced error handling and logging for QUIC and HTTP/3 operations. - Ensured compatibility with existing QUIC connection ID and event structures. closes: #61
There was a problem hiding this comment.
🟡 Changes recommended
Critical QUIC crypto, validation, and arena-lifetime issues remain unresolved.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds QUIC and HTTP/3 adapter support using ngtcp2/nghttp3, with arena allocation, dependency integration, tests, and CI updates.
Changes:
- Added QUIC and HTTP/3 contracts and implementations.
- Added arena-backed memory management and dependency wiring.
- Added tests, build configuration, and CI coverage.
File summaries
| File | Description |
|---|---|
tests/adapters/http3.cpp |
HTTP/3 and QUIC smoke tests |
tests/adapters/http3_contract.cpp |
Contract assertions |
src/core/contract/laghu/core/contract.hpp |
Dependency operation identifiers |
src/adapters/quic.cpp |
QUIC session implementation |
src/adapters/private/laghu/adapters/internal/arena_memory.hpp |
Arena-backed allocator |
src/adapters/http3.cpp |
HTTP/3 session implementation |
src/adapters/dependency.cpp |
Dependency operation mapping |
src/adapters/contract/laghu/adapters/quic.hpp |
QUIC public contract |
src/adapters/contract/laghu/adapters/http3.hpp |
HTTP/3 public contract |
CMakeLists.txt |
Build targets and tests |
cmake/LaghuDependencies.cmake |
Dependency include configuration |
.github/workflows/toolchain.yml |
CI build and test coverage |
Review details
Suppressed comments (1)
src/adapters/contract/laghu/adapters/http3.hpp:43
Http3Output::bytesis a borrowed view into nghttp3-owned storage, but the contract does not state its lifetime. Callers can reasonably retain it pastnext_output()oracknowledge_output(), after which a later session operation may invalidate it. Document the same next-operation lifetime guarantee used byHttp2Session::next_output().
struct Http3Output final {
std::int64_t stream_id{-1};
core::ByteView bytes{};
bool fin{};
};
- Files reviewed: 12/12 changed files
- Comments generated: 10
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
- Added new enums for QUIC key direction to improve clarity in key management. - Refactored QuicConnectionId to use a class structure for better encapsulation of its properties. - Introduced new crypto callback functions to handle QUIC cryptographic operations. - Enhanced error handling in QUIC session management to ensure proper resource cleanup. - Updated tests to validate the behavior of QUIC sessions after arena resets, ensuring stability. - Improved memory management and allocation checks in QUIC and HTTP/3 session handling.
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved critical and moderate correctness, safety, lifetime, and resource-cleanup issues remain.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (9)
src/adapters/contract/laghu/adapters/http3.hpp:22
Http3Eventforwards header/data views backed by nghttp3 callback buffers, but the copyable event type does not document that those views expire when the sink callback returns. Consumers may otherwise retain a dangling view; document the callback-only lifetime or make events own their payloads.
struct Http3Event final {
src/adapters/contract/laghu/adapters/quic.hpp:54
QuicEvent::datais populated from ngtcp2 callback buffers, which are only valid during the sink invocation. Because the event is copyable and the contract does not state that lifetime, consumers can retain a danglingByteView; document the callback-only lifetime (as the HTTP/2 event contract does) or provide owned storage.
struct QuicEvent final {
src/adapters/http3.cpp:193
nghttp3_conn_writev_streamcan return zero vectors withfinset for an empty DATA/body completion. Returning a default output here discards the stream ID and FIN, so the caller cannot callnghttp3_conn_add_write_offset(..., 0)and the stream never gets its required zero-byte FIN acknowledged. Preservestream/finwhencount == 0and return an empty view for that case.
if (count == 0) return Http3Output{};
const auto view = *core::ByteView::from(std::span<const std::byte>{
reinterpret_cast<const std::byte*>(vector.base), vector.len});
return Http3Output{stream, view, fin != 0};
src/adapters/http3.cpp:178
- The final
uniargument tonghttp3_conn_read_stream2is hard-coded to false, so the adapter cannot feed control, QPACK encoder, or QPACK decoder streams as unidirectional streams. Those streams are essential to HTTP/3 and will be classified as bidirectional/request streams; derive the flag from the QUIC stream ID (bit 1) or expose it in the contract.
const auto result = nghttp3_conn_read_stream2(static_cast<nghttp3_conn*>(connection_), stream,
reinterpret_cast<const std::uint8_t*>(data.data()), data.size(), fin ? 1 : 0, 0);
src/adapters/quic.cpp:552
- The session configures ngtcp2 with
limits.maximum_packet_bytes, but this guard only requires the hard-coded 1200-byte minimum. ngtcp2 requires the destination buffer to be at leastmax_tx_udp_payload_size; thus a session configured for 1500 bytes accepts a 1200-byte buffer and invokes the native API outside its documented contract. Retain the configured maximum inStateand validate against that value.
if (output.size() < NGTCP2_MAX_UDP_PAYLOAD_SIZE) {
return std::unexpected{core_error(core::ErrorCode::invalid_range,
"QUIC output is smaller than the minimum packet buffer")};
src/adapters/quic.cpp:450
NGTCP2_MAX_UDP_PAYLOAD_SIZEis the protocol's upper bound (65527 bytes), not the minimum allowed packet size. This rejects normal MTU-sized configurations such as the 1500-byte limit used by the new tests, soQuicSession::createfails before a QUIC session can be established. Validate against the QUIC minimum packet size (or otherwise allow the configured maximum) instead.
limits.maximum_packet_bytes < NGTCP2_MAX_UDP_PAYLOAD_SIZE ||
crypto.random_fill == nullptr || crypto.start == nullptr ||
crypto.receive == nullptr || crypto.retry == nullptr || crypto.update == nullptr) {
src/adapters/quic.cpp:480
write_packetpassesstream_datadirectly to ngtcp2, but this callback table does not registeracked_stream_data_offset. ngtcp2 may retain the submitted bytes for retransmission until they are acknowledged or the stream closes, so a caller that releases or reuses theByteViewafter this method returns can cause corrupted retransmissions; the contract currently provides no lifetime rule or acknowledgement signal. Either copy/retain the data in adapter-owned storage or expose and wire an acknowledgement/lifetime contract before accepting borrowed buffers here.
callbacks.recv_stream_data = stream_data;
callbacks.stream_open = stream_opened;
callbacks.stream_reset = stream_reset;
callbacks.recv_stop_sending = stop_sending_received;
callbacks.stream_close2 = stream_closed;
src/adapters/quic.cpp:51
- Stream-limit and flow-control failures are classified as
corrupt_datahere.open_bidirectional_stream()can returnNGTCP2_ERR_STREAM_ID_BLOCKED, and writes can be blocked byNGTCP2_ERR_STREAM_DATA_BLOCKED; mapping these to exhaustion is necessary so callers can wait/retry instead of treating normal backpressure as peer corruption.
[[nodiscard]] core::DependencyStatus status_for(int code) noexcept {
if (code == NGTCP2_ERR_NOMEM) return core::DependencyStatus::exhaustion;
if (code == NGTCP2_ERR_INVALID_ARGUMENT) return core::DependencyStatus::invalid_input;
if (code == NGTCP2_ERR_CRYPTO) return core::DependencyStatus::crypto;
return core::DependencyStatus::corrupt_data;
src/adapters/quic.cpp:619
- After an arena reset, this deliberately skips
ngtcp2_conn_del, so ngtcp2 never invokesdelete_aead/delete_cipherfor key contexts that carry caller-provided destroy callbacks. The public API and the test both allow sessions to outlive a reset, which leaks those external crypto resources; add a reset-time cleanup path or prevent reset while a QUIC session is still live.
void QuicSession::release() noexcept {
if (connection_ != nullptr && arena_ != nullptr && arena_->generation() == generation_) {
ngtcp2_conn_del(static_cast<ngtcp2_conn*>(connection_));
}
- Files reviewed: 12/12 changed files
- Comments generated: 1
- Review effort level: Lite
- Added ArenaPin class to manage memory pinning in BoundedArena. - Updated Http3Session and QuicSession constructors to accept ArenaPin for better memory management. - Modified session creation methods to ensure proper pinning and unpinning of resources. - Improved error handling for session cleanup during arena resets. - Enhanced test cases to validate session behavior during arena resets.
There was a problem hiding this comment.
🟡 Changes recommended
Critical compile-time, acknowledgement, and arena-lifetime issues remain unresolved.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (5)
CMakeLists.txt:250
- These new HTTP/3/QUIC sources are not added to the build-identity input manifest.
cmake/LaghuBuildIdentity.cmake:114-119explicitly hashes the HTTP/2 contract and implementation, but has no corresponding HTTP/3, QUIC, or arena-memory entries, so changing this library can leave the reported build identity and any cache/determinism checks unchanged. Add the new public/private sources to that manifest.
add_library(laghu_quic STATIC
src/adapters/quic.cpp
src/adapters/contract/laghu/adapters/http3.hpp:59
- This constructor stores an
ArenaPin, whose destructor dereferences the arena, so the arena must outlive everyHttp3Session. The public contract does not state that precondition (unlikeHttp2Sessionatsrc/adapters/contract/laghu/adapters/http2.hpp:145); destroying the arena first leaves the session destructor with a dangling pointer. Document this lifetime requirement on the API.
[[nodiscard]] static core::Result<Http3Session> create(
Http3Role role, core::WorkerId worker, core::BoundedArena& arena,
Http3Limits limits, Http3EventSink events = {},
DependencyLogSink log_sink = {}) noexcept;
src/adapters/contract/laghu/adapters/quic.hpp:165
- This constructor stores an
ArenaPin, whose destructor dereferences the arena, so the arena must outlive everyQuicSession. The public contract does not state that precondition (unlikeHttp2Sessionatsrc/adapters/contract/laghu/adapters/http2.hpp:145); destroying the arena first leaves the session destructor with a dangling pointer. Document this lifetime requirement on the API.
[[nodiscard]] static core::Result<QuicSession> create(
QuicRole role, const QuicConnectionId& destination,
const QuicConnectionId& source, core::BoundedArena& arena,
QuicLimits limits, QuicCryptoCallbacks crypto,
QuicEventSink events = {}, DependencyLogSink log_sink = {}) noexcept;
src/adapters/contract/laghu/adapters/quic.hpp:174
- HTTP/3 needs three locally-created unidirectional streams (control, QPACK encoder, and QPACK decoder), but this contract exposes only
open_bidirectional_stream(). Because ngtcp2 types are private andHttp3Session::bind_streams()accepts IDs that must already exist, an API consumer has no way to establish the HTTP/3 control plane; the smoke test's hard-coded IDs mask this gap. Expose unidirectional stream creation (and cover the QUIC-to-HTTP/3 setup path) before treating the HTTP/3 adapter as usable.
[[nodiscard]] core::Result<std::int64_t> open_bidirectional_stream() noexcept;
src/adapters/quic.cpp:458
NGTCP2_MAX_UDP_PAYLOAD_SIZEis the protocol's upper bound (65527 bytes), not the minimum packet size. Comparing against it rejects normal MTU-sized configurations such as the 1500-byte limit used by every QUIC construction test, soQuicSession::createfails before any native session is created. Validate againstNGTCP2_MIN_INITIAL_DGRAM_SIZEinstead (or the intended lower bound).
limits.maximum_packet_bytes < NGTCP2_MAX_UDP_PAYLOAD_SIZE ||
- Files reviewed: 13/13 changed files
- Comments generated: 3
- Review effort level: Lite
- Added support for unidirectional streams in QuicSession. - Introduced new methods for acknowledging stream data in Http3Session. - Updated input hash generation to include HTTP/3 and QUIC headers. - Improved memory management checks in BoundedArena destructor. - Enhanced tests to validate new functionality in HTTP/3 and QUIC.
There was a problem hiding this comment.
🟡 Changes recommended
Critical QUIC/HTTP/3 lifecycle and configuration issues, plus a transmit-timestamp issue, remain unresolved.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (1)
src/adapters/quic.cpp:576
ngtcp2_conn_writev_streamrequiresngtcp2_conn_update_pkt_tx_timeafter packet generation so pacing andexpiry_ns()advance. This adapter never updates the connection after a successful write, so repeated writes can use stale pacing/expiry state; update the transmit timestamp after the packet is handed off (or expose an explicit send-completion operation).
const auto result = ngtcp2_conn_writev_stream(static_cast<ngtcp2_conn*>(connection_), nullptr,
nullptr, reinterpret_cast<std::uint8_t*>(output.data()), output.size(), &consumed,
flags, stream_id, stream_data.empty() ? nullptr : &vector,
stream_data.empty() ? 0U : 1U, now_ns);
- Files reviewed: 15/15 changed files
- Comments generated: 3
- Review effort level: Lite
- Added `packet_transmitted` method to `QuicSession` for managing packet transmission state. - Introduced `terminal` and `packet_pending_transmit` flags in `State` struct to track session status. - Updated error handling in `receive_packet` and `write_packet` methods to set terminal state on errors. - Modified tests to validate new functionality and ensure proper session cleanup on arena reset.
There was a problem hiding this comment.
🔵 Needs a closer look
Fix the reversed maximum_packet_bytes validation in src/adapters/quic.cpp.
Review details
Suppressed comments (1)
src/adapters/quic.cpp:466
- This comparison rejects every normal QUIC packet buffer (for example the 1500-byte buffers used by the new tests) and accepts values above ngtcp2's maximum instead.
maximum_packet_bytesshould be bounded above byNGTCP2_MAX_UDP_PAYLOAD_SIZE(and reject zero), otherwise session creation fails for valid MTUs and can pass an invalid setting to ngtcp2.
limits.maximum_packet_bytes < NGTCP2_MAX_UDP_PAYLOAD_SIZE ||
- Files reviewed: 15/15 changed files
- Comments generated: 0 new
- Review effort level: Lite
- Added validation for maximum packet size in QuicSession creation to ensure it does not exceed the defined limits. - Updated test cases to check for valid and invalid session creation scenarios, improving robustness against incorrect configurations.
There was a problem hiding this comment.
🟡 Changes recommended
QUIC CID routing, terminal expiry handling, path updates, and arena reuse have unresolved issues.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (4)
Previously missed (1) — in code that hasn't changed since the last review.
tests/adapters/http3.cpp:131
- The QUIC portion of this test suite never exercises a successful packet path: the crypto callbacks either do nothing or explicitly return false,
receive_packetis tested only with malformed input, and the write test expects failure. As a result, the newly added key installation, encryption/decryption, stream, expiry, and packet-transmission callbacks can regress without detection. Add a loopback test with deterministic keys/crypto that successfully writes, receives, and acknowledges a packet.
src/adapters/private/laghu/adapters/internal/arena_memory.hpp:23
free_listis owned by each session'sArenaMemory, so blocks returned byarena_freeduringngtcp2_conn_delremain reachable only from the session state that is about to be discarded. Creating another QUIC/HTTP/3 session in the same generation cannot reuse those blocks and keeps advancing the monotonic arena offset until it exhausts, even though the native allocations were freed. Move the reusable allocation bookkeeping to an arena/session-generation-owned structure, or otherwise reclaim/reuse these blocks across sessions before relying on this allocator for connection churn.
struct ArenaMemory final {
core::BoundedArena* arena{};
core::WorkerId worker;
ArenaAllocation* free_list{};
};
src/adapters/quic.cpp:666
ngtcp2_conn_handle_expirycan return terminal expiry errors such asNGTCP2_ERR_IDLE_CLOSE, but this path never marksState::terminal(unlike receive/write). The error is returned whilerequire_valid()remains successful, allowing later operations to call a closed native connection; classify expiry terminal errors and transition the session to terminal before returning.
core::Result<void> QuicSession::handle_expiry(std::uint64_t now_ns) noexcept {
if (const auto valid = require_valid(); !valid.has_value()) return valid;
const int result = ngtcp2_conn_handle_expiry(static_cast<ngtcp2_conn*>(connection_), now_ns);
if (const auto random = require_random(*static_cast<State*>(state_)); !random.has_value()) {
return random;
}
if (result != 0) return std::unexpected{native_error(core::DependencyOperation::quic_expiry,
result, static_cast<State*>(state_)->log)};
src/adapters/quic.cpp:488
- The session's
ngtcp2_pathis permanently built from zeroedsockaddr_invalues, andreceive_packetprovides no way to supply or update the actual local/remote addresses. This prevents ngtcp2 from validating address changes and makes QUIC migration/path ownership impossible even though the contract is intended to preserve that policy; include the packet path (or an explicit path-update operation) in the transport contract.
state->path = {
{reinterpret_cast<sockaddr*>(&state->local), sizeof(state->local)},
{reinterpret_cast<sockaddr*>(&state->remote), sizeof(state->remote)}, nullptr};
- Files reviewed: 15/15 changed files
- Comments generated: 1
- Review effort level: Lite
- Added a new NativeMemoryPool class to manage memory allocations for adapter sessions. - Updated Http3Session and QuicSession to utilize NativeMemoryPool instead of BoundedArena directly. - Refactored memory allocation logic to improve memory management and reuse of freed blocks. - Enhanced error handling during session creation to ensure proper memory allocation. - Introduced tests for validating memory reuse and encrypted QUIC packet handling.
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved QUIC and HTTP/3 correctness and error-handling issues remain, including a critical RNG-failure path.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (3)
src/adapters/contract/laghu/adapters/quic.hpp:188
connection_idsis declared optional here, butQuicSession::createrejects any nullconnection_ids.writeatsrc/adapters/quic.cpp:533, so a caller using the documented defaults can never construct a session. Since the adapter requires this callback for its NEW_CONNECTION_ID path, make the sink a required parameter (and likewise remove the precedingeventsdefault to preserve valid C++ default-argument ordering), or implement a defined no-sink policy instead of rejecting the default.
QuicEventSink events = {}, QuicConnectionIdSink connection_ids = {},
src/adapters/http3.cpp:195
nghttp3_conn_read_stream2returns a status code, not the number of input bytes consumed; successful calls therefore return 0 here even after acceptingdata. Callers using theResult<std::size_t>contract will treat every successful non-empty packet as unconsumed and may retry or fail to advance their receive buffer. Returndata.size()on success (or change the contract toResult<void>).
return static_cast<std::size_t>(result);
src/adapters/quic.cpp:352
- The
hp_maskcallback's destination is the QUIC header-protection mask, which isNGTCP2_HP_MASKLENbytes (5), whileNGTCP2_HP_SAMPLELENis the 16-byte input sample. Passing a 16-byte view makes every header-protection implementation write past ngtcp2's destination buffer, as the test callback does here, causing memory corruption. Use the mask-length constant foroutputand retain the sample-length constant forsample_view.
const auto output = *core::MutableByteView::from(std::span<std::byte>{
- Files reviewed: 16/16 changed files
- Comments generated: 1
- Review effort level: Lite
- Updated QuicSession::create to handle random failure during session creation. - Modified receive_packet in Http3Session to return the size of data received. - Enhanced address handling in Quic to use memcpy for better safety. - Added a new test case for constructor random failure rejection.
There was a problem hiding this comment.
🟡 Changes recommended
Three moderate issues remain unresolved in the HTTP/3 and QUIC implementations.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (2)
src/adapters/http3.cpp:103
maximum_field_section_bytesis only passed to nghttp3 as its receive setting;native_headers()accepts up to 64 fields without checking the aggregate name/value size. A caller can therefore submit a header block larger than the configured bound, allowing unbounded QPACK/native allocation and making this limit ineffective for outbound requests/responses. Track the configured limit and reject an overflow-safe aggregate before constructing thenghttp3_nvarray, as the HTTP/2 adapter does for header submissions.
for (const auto& header : headers) {
if (header.name.empty()) {
return std::unexpected{core_error(core::ErrorCode::invalid_input,
"HTTP/3 header name must not be empty")};
}
result.values[result.size] = {
const_cast<std::uint8_t*>(reinterpret_cast<const std::uint8_t*>(header.name.data())),
const_cast<std::uint8_t*>(reinterpret_cast<const std::uint8_t*>(header.value.data())),
header.name.size(), header.value.size(), NGHTTP3_NV_FLAG_NONE};
++result.size;
src/adapters/quic.cpp:671
- Once
write_packetreturns a positive packet, the caller must callpacket_transmitted()before generating another packet so ngtcp2's transmit timestamp/state stays synchronized with the packet actually sent. This method does not reject a secondwrite_packet()whilepacket_pending_transmitis true, so the second packet can overwrite the pending state and the first packet may never be accounted for. Reject writes while a packet is pending (or explicitly support multiple outstanding packets with corresponding transmit bookkeeping).
core::Result<QuicPacketWrite> QuicSession::write_packet(
core::MutableByteView output, std::int64_t stream_id, core::ByteView stream_data,
bool fin, std::uint64_t now_ns) noexcept {
if (const auto valid = require_valid(); !valid.has_value()) return std::unexpected{valid.error()};
const auto required = static_cast<State*>(state_)->maximum_packet_bytes;
if (output.size() < required) {
return std::unexpected{core_error(core::ErrorCode::invalid_range,
"QUIC output is smaller than the configured packet buffer")};
}
ngtcp2_ssize consumed{-1};
const ngtcp2_vec vector{reinterpret_cast<std::uint8_t*>(const_cast<std::byte*>(stream_data.data())),
stream_data.size()};
const std::uint32_t flags = fin ? NGTCP2_WRITE_STREAM_FLAG_FIN : NGTCP2_WRITE_STREAM_FLAG_NONE;
ngtcp2_path_storage output_path{};
ngtcp2_path_storage_zero(&output_path);
const auto result = ngtcp2_conn_writev_stream(static_cast<ngtcp2_conn*>(connection_),
- Files reviewed: 16/16 changed files
- Comments generated: 1
- Review effort level: Lite
- Added maximum field section bytes to State for better control over header size. - Updated native_headers function to validate header sizes against the maximum limit. - Enhanced Http3Session creation to check for valid field-section limits. - Improved error handling for oversized headers in tests to ensure robustness.


http3.cppto handle HTTP/3 connections.quic.cppto support QUIC protocol operations.arena_memory.hppfor efficient memory allocation.dependency.cppto include new dependency operations for QUIC and HTTP/3.http3.cppandhttp3_contract.cppto ensure correctness.closes: #61