Document QUIC_PARAM_CONN_PATH_STATUS - #86
Conversation
The parameter had no row in the connection parameter table and no section; its only mention was a passing reference from the PATH_STATISTICS section. Written from the implementation: - Set-only. It appears in QuicConnParamSet and nowhere in QuicConnParamGet, so the current status cannot be read back through it. - Requires multipath, else QUIC_STATUS_INVALID_STATE, and an exact BufferLength. - Selects the path by PathId against Paths[i].PathID->ID, skipping paths with no path ID yet; an unknown id is QUIC_STATUS_INVALID_PARAMETER. - Locally, QuicConnChoosePath draws only from paths that are active and not closing, so marking one backup takes it out of the send rotation. - On the wire it sends PATH_AVAILABLE or PATH_BACKUP, the PATH_STATUS frames of draft-ietf-quic-multipath, with a per-path-ID sequence number. Setting the status it already has sends nothing, since the send flag is only raised when the value changes. - The peer can flip it on us: an incoming frame updates the path and raises QUIC_CONNECTION_EVENT_PATH_STATUS_CHANGED, with older sequence numbers ignored. Noted so applications follow the event rather than assume the status is whatever they last set. Also corrects the numbering of three rows. The number in that column is the parameter's low byte in decimal, which every other row follows, but REMOVE_PATH, ADD_CANDIDATE_ADDRESS and REMOVE_CANDIDATE_ADDRESS were each one low -- they were numbered sequentially after NETWORK_STATISTICS (0x20) was inserted out of numeric order. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review of #86 found five inaccuracies in the section as written. Path selection was described as if active status were the only input: - With no path active at all, QuicConnChoosePath still returns &Connection->Paths[0] -- the random pick only replaces it when ActivePathCount > 0. Marking every path backup does not stop sending. - Under multipath QuicPathSetActive sets IsActive unconditionally and never raises SendStatus or the PATH_AVAILABLE flag. It runs from the fallback when the active path is removed (path.c) and when a new path finishes validation (connection.c), so a path the application marked backup can be promoted again with the peer still believing it is backup. - Selection happens once per QuicSendFlush, not per packet, and a flush from pacing reuses Send->PacingPath without choosing at all. The receive paragraph was wrong twice: - QUIC_CONNECTION_EVENT_PATH_STATUS_CHANGED is raised only by the two peer frame handlers. Setting the parameter locally raises nothing, so telling applications to "follow that event" to learn the status was incomplete. - StatusRecvSeq is stored as lastAccepted + 1 and the check is Frame.StatusSequenceNumber < StatusRecvSeq, so a frame equal to the last accepted is dropped too. "Older than" is now "not greater than". Finally, the parameter and QUIC_PATH_STATUS both sit behind QUIC_API_ENABLE_PREVIEW_FEATURES, which the section now states. It is not marked in the table row: all fifteen preview-guarded connection parameters are unmarked there, so tagging one would read as if the others were not preview. Worth a sweep of its own. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
All five findings were real — verified each against the source before changing anything. Fixed in 001b35d. Path selection (findings 1–3). The section described active status as if it were the only input to selection. Three separate corrections:
Selection is per Receive paragraph (finding 4). Both parts confirmed. Preview guard (finding 5). Confirmed — both the parameter and the struct are inside I did not add Re-verified after the change: all 39 connection rows match their values in |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Both were mine, and neither reproduces on Linux -- 1350 migration runs and 30 idle-DestCid runs are clean. The evidence is the ETL trace and a comparison against control branches, not a local repro. MigrationShareBinding/17 asserted in the XDP BVT. The trace ends: [S][TX][121] SH ... (Payload 1206 bytes) PING / PADDING Len:1201 [pack] Created in batch ... [ lib] ASSERT, src\core\send.c:1759 - Result. That line is upstream's CXPLAT_DBG_ASSERT(Result) in QuicSendPathChallenges, which assumes a PATH_CHALLENGE frame always fits. Moving MTU probes out of the send loop put them before path challenges; on a server still amplification limited after migration, a full-size probe spends the allowance the challenge then needs, the datagram comes back too small for a nine-byte frame, and the assert fires. Previously probes were built in the main loop, which runs after challenges, so challenges had the allowance first. Challenges and responses now go first, and a path with a challenge or response pending is not probed at all. Ordering alone would not settle it, since either can be queued for a later flush. Misc.IdleDestCidChange failed both kernel BVTs. QuicPacketBuilderInitialize stamps Send->LastFlushTime, which is the clock the idle DestCid update measures its idle period against. The congestion control check added in the previous commit sat after the builder was initialized, so a probe that gave up still reset that clock -- and since the flag is re-raised, it did so on every flush, and the update never fired. QuicCongestionControlCanSend is asked before the builder exists instead. Not pre-existing: the documentation-only branch behind #86 and the P2-only branch behind #88 both pass all four kernel BVT jobs. IdleDestCidChange matched none of the sweep filters used until now, so it had never been run locally. It is in the sweep from here on.
1 participant
Description
QUIC_PARAM_CONN_PATH_STATUShad no row in the connection parameter table and no section of its own. Its only appearance indocs/Settings.mdwas a passing reference from theQUIC_PARAM_CONN_PATH_STATISTICSsection, which assumed the reader already knew what it was.Written from the implementation:
QuicConnParamSetand nowhere inQuicConnParamGet, so the current status cannot be read back through it. Worth stating, since the name reads like something you could query.QUIC_STATUS_INVALID_STATEotherwise) and an exactBufferLength.PathIdagainstPaths[i].PathID->ID, skipping paths that have no path ID yet — one added before the handshake is confirmed. An unknown id isQUIC_STATUS_INVALID_PARAMETER.QUIC_API_ENABLE_PREVIEW_FEATURES, along with theQUIC_PATH_STATUSstruct.What the status actually does
The interesting part, and the part an early draft of this section got wrong in three ways.
It steers path selection, per flush. Once multipath is negotiated and the handshake is confirmed, each
QuicSendFlushpicks one path at random from those active and not closing, and builds that flush's packets on it. Not per packet — and a flush triggered by pacing reusesSend->PacingPathwithout choosing at all.Marking everything backup does not stop sending.
QuicConnChoosePathopens withQUIC_PATH* Path = &Connection->Paths[0];and only replaces it insideif (ActivePathCount > 0). With no active path it returns the first one and sending continues there.The status is not durable against internal changes. Under multipath
QuicPathSetActiveis justPath->IsActive = TRUE;— noSendStatus, noQUIC_CONN_SEND_FLAG_PATH_AVAILABLE. It runs from the fallback when the active path is removed (path.c) and when a new path finishes validation (connection.c), so a path the application marked backup can be promoted again with nothing sent to the peer and no event raised. This is the one most likely to surprise: it is natural to assume a status holds until you change it.On the wire, and from the peer
The change is announced with a PATH_AVAILABLE or PATH_BACKUP frame — the PATH_STATUS frames of draft-ietf-quic-multipath — carrying the path ID and a per-path-ID sequence number. Setting the status a path already has sends nothing, since the send flag is raised only when the value changes.
The peer can flip it on us. An incoming frame updates the path and raises
QUIC_CONNECTION_EVENT_PATH_STATUS_CHANGED; a frame whose sequence number is not greater than the last accepted for that path ID is ignored —StatusRecvSeqis stored aslastAccepted + 1and tested with<, so an equal one is dropped as well.That event fires only for peer-initiated changes; setting the parameter locally raises nothing. So an application tracking a path's status needs both what it set and what arrives on the event — and, given the promotion caveat, neither is a complete record. The section says so rather than leaving the reader to find out.
Also: three misnumbered rows
The number in that column is the parameter's low byte in decimal — every row follows it. Checking all connection rows against
msquic.hmechanically turned up three that were one low:QUIC_PARAM_CONN_REMOVE_PATH0x05000021QUIC_PARAM_CONN_ADD_CANDIDATE_ADDRESS0x05000022QUIC_PARAM_CONN_REMOVE_CANDIDATE_ADDRESS0x05000023They drifted when
QUIC_PARAM_CONN_NETWORK_STATISTICS(0x05000020, so 32) was inserted out of numeric order and the rows after it were numbered sequentially instead of from their values. Fixed because leaving them wrong immediately above a newly added correct row is worse than the small scope increase.Left alone
The
(preview)marker on the table row. All fifteen preview-guarded connection parameters are unmarked there —CIBIR_ID,VERSION_SETTINGS,NETWORK_STATISTICS,CLOSE_ASYNC, the bound and observed address ones, all four path ones,UNCONNECTED_UDP_SOCKET,PATH_STATISTICSand this one. Marking a single row would read as if the other fourteen were not preview. The requirement is stated in the section prose instead, which is what protects someone copying the struct; the table is a sweep of its own.Testing
Documentation only — no code change, so nothing to run.
Checked mechanically: every
QUIC_PARAM_CONN_*row matches the value inmsquic.h(0 mismatches across 39 rows), the table is one unbroken block with no stray blank line, and everySee [...](#anchor)link resolves to a heading in the file.Documentation
This is the documentation.