Skip to content

Defer destination CID replacement #6298

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Guillaume Hetier (guhetier) wants to merge 10 commits into from
+211 −143
Open

Defer destination CID replacement #6298

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
48078cf
Defer path CID replacement
guhetier Aug 24, 2026
3d6d972
Prioritize CID assignment for migration
guhetier Aug 26, 2026
8bb2fa7
Use renamed active path helper
guhetier Aug 27, 2026
b737d55
Harden deferred CID replacement
guhetier Sep 3, 2026
d296e37
Address deferred CID review feedback
guhetier Sep 3, 2026
2562630
Handle paths without destination CIDs
guhetier Sep 4, 2026
9f413ce
Define destination CID update before use
guhetier Sep 4, 2026
a90fe03
Preserve CID state until replacement succeeds
guhetier Sep 4, 2026
45f2178
Avoid retiring destination CIDs twice
guhetier Sep 5, 2026
2decdde
Make destination CID retirement idempotent
guhetier Sep 5, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
122 changes: 18 additions & 104 deletions src/core/connection.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1047,6 +1047,10 @@ QuicConnRetireCid(
_In_ QUIC_CID_LIST_ENTRY* DestCid
)
{
if (DestCid->CID.Retired) {
return;
}

QuicTraceEvent(
ConnDestCidRemoved,
"[conn][%p] (SeqNum=%llu) Removed Destination CID: %!CID!",
Expand Down Expand Up @@ -1076,6 +1080,10 @@ QuicConnRetireCurrentDestCid(
_In_ QUIC_PATH* Path
)
{
if (Path->DestCid == NULL) {
return TRUE;
}

if (Path->DestCid->CID.Length == 0) {
QuicTraceLogConnVerbose(
ZeroLengthCidRetire,
Expand All @@ -1096,7 +1104,7 @@ QuicConnRetireCurrentDestCid(
CXPLAT_DBG_ASSERT(Path->DestCid != NewDestCid);
QUIC_CID_LIST_ENTRY* OldDestCid = Path->DestCid;
QUIC_CID_CLEAR_PATH(Path->DestCid);
QuicConnRetireCid(Connection, Path->DestCid);
QuicConnRetireCid(Connection, OldDestCid);
Path->DestCid = NewDestCid;
QUIC_CID_SET_PATH(Connection, Path->DestCid, Path);
QUIC_CID_VALIDATE_NULL(Connection, OldDestCid);
Expand All @@ -1112,7 +1120,7 @@ QuicConnOnRetirePriorToUpdated(
_In_ QUIC_CONNECTION* Connection
)
{
BOOLEAN ReplaceRetiredCids = FALSE;
BOOLEAN RetiredUsedCid = FALSE;

for (CXPLAT_LIST_ENTRY* Entry = Connection->DestCids.Flink;
Entry != &Connection->DestCids;
Expand All @@ -1127,78 +1135,11 @@ QuicConnOnRetirePriorToUpdated(
continue;
}

if (DestCid->CID.UsedLocally) {
ReplaceRetiredCids = TRUE;
}

QUIC_CID_CLEAR_PATH(DestCid);
RetiredUsedCid |= DestCid->CID.UsedLocally;
QuicConnRetireCid(Connection, DestCid);
}

return ReplaceRetiredCids;
}

_IRQL_requires_max_(PASSIVE_LEVEL)
BOOLEAN
QuicConnReplaceRetiredCids(
_In_ QUIC_CONNECTION* Connection
)
{
QUIC_PATH_SET* PathSet = &Connection->Paths;
CXPLAT_DBG_ASSERT(PathSet->Count <= QUIC_MAX_PATH_COUNT);
for (uint8_t i = 0; i < PathSet->Count; ++i) {
QUIC_PATH* Path = &PathSet->Paths[i];
if (Path->DestCid == NULL || !Path->DestCid->CID.Retired) {
continue;
}

QUIC_CID_VALIDATE_NULL(Connection, Path->DestCid); // Previously cleared on retire.
QUIC_CID_LIST_ENTRY* NewDestCid = QuicConnGetUnusedDestCid(Connection);
if (NewDestCid == NULL) {
if (Path->IsActive) {
QuicTraceEvent(
ConnError,
"[conn][%p] ERROR, %s.",
Connection,
"Active path has no replacement for retired CID");
QuicConnSilentlyAbort(Connection); // Must silently abort because we can't send anything now.
return FALSE;
}
QuicTraceLogConnWarning(
NonActivePathCidRetired,
Connection,
"Non-active path has no replacement for retired CID.");
//
// A path pending deferred activation is still considered non-active here and
// may be removed. CID replacement will be deferred in the next stack layer.
//
CXPLAT_DBG_ASSERT(i != 0);
QuicPathRemove(Connection, i--);
continue;
}

CXPLAT_DBG_ASSERT(NewDestCid != Path->DestCid);
Path->DestCid = NewDestCid;
QUIC_CID_SET_PATH(Connection, NewDestCid, Path);
Path->DestCid->CID.UsedLocally = TRUE;
Path->InitiatedCidUpdate = TRUE;
QuicPathValidate(Path);
}

#if DEBUG
for (CXPLAT_LIST_ENTRY* Entry = Connection->DestCids.Flink;
Entry != &Connection->DestCids;
Entry = Entry->Flink) {
QUIC_CID_LIST_ENTRY* DestCid =
CXPLAT_CONTAINING_RECORD(
Entry,
QUIC_CID_LIST_ENTRY,
Link);
CXPLAT_DBG_ASSERT(!DestCid->CID.Retired || DestCid->AssignedPath == NULL);
}
#endif

return TRUE;
return RetiredUsedCid;
}

_IRQL_requires_max_(DISPATCH_LEVEL)
Expand Down Expand Up @@ -5058,10 +4999,10 @@ QuicConnRecvFrames(
break; // Ignore frame if we are closed.
}

BOOLEAN ReplaceRetiredCids = FALSE;
BOOLEAN RetiredUsedCid = FALSE;
if (Connection->RetirePriorTo < Frame.RetirePriorTo) {
Connection->RetirePriorTo = Frame.RetirePriorTo;
ReplaceRetiredCids = QuicConnOnRetirePriorToUpdated(Connection);
RetiredUsedCid = QuicConnOnRetirePriorToUpdated(Connection);
}

if (QuicConnGetDestCidFromSeq(Connection, Frame.Sequence, FALSE) == NULL) {
Expand All @@ -5076,7 +5017,7 @@ QuicConnRecvFrames(
"Allocation of '%s' failed. (%llu bytes)",
"new DestCid",
sizeof(QUIC_CID_LIST_ENTRY) + Frame.Length);
if (ReplaceRetiredCids) {
if (RetiredUsedCid) {
QuicConnSilentlyAbort(Connection);
} else {
QuicConnFatalError(Connection, QUIC_STATUS_OUT_OF_MEMORY, NULL);
Expand Down Expand Up @@ -5109,7 +5050,7 @@ QuicConnRecvFrames(
"[conn][%p] ERROR, %s.",
Connection,
"Peer exceeded CID limit");
if (ReplaceRetiredCids) {
if (RetiredUsedCid) {
QuicConnSilentlyAbort(Connection);
} else {
QuicConnTransportError(Connection, QUIC_ERROR_PROTOCOL_VIOLATION);
Expand All @@ -5118,10 +5059,6 @@ QuicConnRecvFrames(
}
}

if (ReplaceRetiredCids && !QuicConnReplaceRetiredCids(Connection)) {
return FALSE;
}

AckEliciting = TRUE;
break;
}
Expand Down Expand Up @@ -5527,31 +5464,6 @@ QuicConnRecvPostProcessing(
// sent back out.
//

if (CurrentPath->DestCid == NULL ||
(PeerUpdatedCid && CurrentPath->DestCid->CID.Length != 0)) {
//
// TODO - What if the peer (client) only sends a single CID and
// rebinding happens? Should we support using the same CID over?
//
QUIC_CID_LIST_ENTRY* NewDestCid = QuicConnGetUnusedDestCid(Connection);
if (NewDestCid == NULL) {
QuicTraceEvent(
ConnError,
"[conn][%p] ERROR, %s.",
Connection,
"No unused CID for new path");
CurrentPath->GotValidPacket = FALSE; // Don't have a new CID to use!!!
CurrentPath->DestCid = NULL;
return;
}
CXPLAT_DBG_ASSERT(NewDestCid != CurrentPath->DestCid);
CurrentPath->DestCid = NewDestCid;
QUIC_CID_SET_PATH(Connection, CurrentPath->DestCid, CurrentPath);
CurrentPath->DestCid->CID.UsedLocally = TRUE;
}

CXPLAT_DBG_ASSERT(CurrentPath->DestCid != NULL);
QuicPathValidate(CurrentPath);
CurrentPath->SendChallenge = TRUE;
CurrentPath->PathValidationStartTime = CxPlatTimeUs64();

Expand Down Expand Up @@ -5990,6 +5902,8 @@ QuicConnRecvDatagrams(
//
QuicPathUpdateActive(Connection);

QuicPathUpdateDestCids(PathSet, Connection);

if (!Connection->State.UpdateWorker && Connection->State.Connected &&
!Connection->State.ShutdownComplete && RecvState.UpdatePartitionId) {
//
Expand Down
9 changes: 9 additions & 0 deletions src/core/connection.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1260,6 +1260,15 @@ QuicConnGenerateNewSourceCids(
_In_ BOOLEAN ReplaceExistingCids
);

//
// Get an unused destination CID from the list provided by the peer.
//
_IRQL_requires_max_(PASSIVE_LEVEL)
QUIC_CID_LIST_ENTRY*
QuicConnGetUnusedDestCid(
_In_ const QUIC_CONNECTION* Connection
);

//
// Retires the currently used destination connection ID.
//
Expand Down
95 changes: 93 additions & 2 deletions src/core/path.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,36 @@
_In_ uint32_t PathId
);

_IRQL_requires_max_(PASSIVE_LEVEL)
static
BOOLEAN
QuicPathUpdateDestCid(
_In_ QUIC_CONNECTION* Connection,
_Inout_ QUIC_PATH* Path
)
{
if (Path->DestCid != NULL && !Path->DestCid->CID.Retired) {
return TRUE;
}

QUIC_CID_LIST_ENTRY* NewDestCid = QuicConnGetUnusedDestCid(Connection);
if (NewDestCid == NULL) {
return FALSE;
}

if (Path->DestCid != NULL) {
QUIC_CID_CLEAR_PATH(Path->DestCid);
Path->DestCid = NULL;
}

Path->DestCid = NewDestCid;
QUIC_CID_SET_PATH(Connection, NewDestCid, Path);
Path->DestCid->CID.UsedLocally = TRUE;
Path->InitiatedCidUpdate = TRUE;
QuicPathValidate(Path);
return TRUE;
}

_IRQL_requires_max_(PASSIVE_LEVEL)
void
QuicPathUpdateActive(
Expand All @@ -90,6 +120,18 @@
return;
}

//
// A path needs a usable destination CID before it can become active.
//
uint8_t NextActivePathIndex;
QUIC_PATH* NextActivePath =
QuicConnGetPathByID(Connection, PathSet->NextActivePathId, &NextActivePathIndex);
CXPLAT_DBG_ASSERT(NextActivePath != NULL);
if (!QuicPathUpdateDestCid(Connection, NextActivePath)) {
PathSet->NextActivePathId = QuicPathGetActive(PathSet)->ID;
return;
}

QuicPathSetActive(Connection, PathSet->NextActivePathId);

QUIC_PATH* ActivePath = QuicPathGetActive(PathSet);
Expand Down Expand Up @@ -206,6 +248,55 @@
return TRUE;
}

_IRQL_requires_max_(PASSIVE_LEVEL)
void
QuicPathUpdateDestCids(
_In_ QUIC_PATH_SET* PathSet,
_In_ QUIC_CONNECTION* Connection
)
{
for (uint8_t i = 0; i < PathSet->Count; ++i) {
QUIC_PATH* Path = &PathSet->Paths[i];
if (QuicPathUpdateDestCid(Connection, Path)) {
continue;
}

if (Path->IsActive) {
QuicTraceEvent(
ConnError,
"[conn][%p] ERROR, %s.",
Connection,
"Active path has no replacement for retired CID");
QuicConnSilentlyAbort(Connection);
return;
}

QuicTraceLogConnWarning(
NonActivePathCidRetired,
Connection,
"Non-active path has no replacement for retired CID.");
CXPLAT_DBG_ASSERT(i != 0);
QuicPathRemove(Connection, i);
//
// Reprocess this index because removal shifted the remaining paths down.
//
--i;

@guhetier Guillaume Hetier (guhetier) Sep 4, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Intentional and needed for now, will be fixed with a refactor of QuicPathRemove later in the stack

}

#if DEBUG
for (CXPLAT_LIST_ENTRY* Entry = Connection->DestCids.Flink;
Entry != &Connection->DestCids;
Entry = Entry->Flink) {
QUIC_CID_LIST_ENTRY* DestCid =
CXPLAT_CONTAINING_RECORD(
Entry,
QUIC_CID_LIST_ENTRY,
Link);
CXPLAT_DBG_ASSERT(!DestCid->CID.Retired || DestCid->AssignedPath == NULL);
}
#endif
}

_IRQL_requires_max_(PASSIVE_LEVEL)
void
QuicPathSetAllowance(
Expand Down Expand Up @@ -446,8 +537,8 @@
QuicCongestionControlReset(&Connection->CongestionControl, FALSE);
}
Connection->Paths.NextActivePathId = ActivePath->ID;
CXPLAT_DBG_ASSERT(Path->DestCid != NULL);
CXPLAT_DBG_ASSERT(!Path->DestCid->CID.Retired);
CXPLAT_DBG_ASSERT(ActivePath->DestCid != NULL);
CXPLAT_DBG_ASSERT(!ActivePath->DestCid->CID.Retired);
}

_IRQL_requires_max_(PASSIVE_LEVEL)
Expand Down
7 changes: 7 additions & 0 deletions src/core/path.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -268,6 +268,13 @@ QuicPathRemove(
_In_ uint8_t Index
);

_IRQL_requires_max_(PASSIVE_LEVEL)
void
QuicPathUpdateDestCids(
_In_ QUIC_PATH_SET* PathSet,
_In_ QUIC_CONNECTION* Connection
);

_IRQL_requires_max_(PASSIVE_LEVEL)
void
QuicPathSetAllowance(
Expand Down
18 changes: 0 additions & 18 deletions src/generated/linux/connection.c.clog.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -375,24 +375,6 @@ tracepoint(CLOG_CONNECTION_C, NoReplacementCidForRetire , arg1);\



/*----------------------------------------------------------
// Decoder Ring for NonActivePathCidRetired
// [conn][%p] Non-active path has no replacement for retired CID.
// QuicTraceLogConnWarning(
NonActivePathCidRetired,
Connection,
"Non-active path has no replacement for retired CID.");
// arg1 = arg1 = Connection = arg1
----------------------------------------------------------*/
#ifndef _clog_3_ARGS_TRACE_NonActivePathCidRetired
#define _clog_3_ARGS_TRACE_NonActivePathCidRetired(uniqueId, arg1, encoded_arg_string)\
tracepoint(CLOG_CONNECTION_C, NonActivePathCidRetired , arg1);\

#endif




/*----------------------------------------------------------
// Decoder Ring for IgnoreUnreachable
// [conn][%p] Ignoring received unreachable event (inline)
Expand Down
Loading
Loading