CASESessionManager: don't release an in-flight CASE setup#72255
Open
woody-apple wants to merge 1 commit into
Open
CASESessionManager: don't release an in-flight CASE setup#72255woody-apple wants to merge 1 commit into
woody-apple wants to merge 1 commit into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request modifies CASESessionManager::ReleaseSession to skip releasing a session if it is currently in the process of being established, introducing a helper method IsEstablishingSession in OperationalSessionSetup. However, the reviewer pointed out a critical issue: silently skipping the release can lead to use-after-free crashes, resource leaks, and an inability to cancel in-flight connection attempts.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #72255 +/- ##
==========================================
+ Coverage 55.52% 55.58% +0.06%
==========================================
Files 1629 1630 +1
Lines 111092 111133 +41
Branches 13415 13405 -10
==========================================
+ Hits 61683 61777 +94
+ Misses 49409 49356 -53 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
PR #72255: Size comparison from 8ad5f7b to a667d15 Full report (35 builds for bl602, bl616, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, psoc6, qpg, realtek, stm32, telink)
|
|
PR #72255: Size comparison from 8ad5f7b to be9ffbd Full report (5 builds for cc32xx, realtek, stm32)
|
|
PR #72255: Size comparison from 8ad5f7b to 1a3c634 Full report (35 builds for bl602, bl616, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, psoc6, qpg, realtek, stm32, telink)
|
woody-apple
force-pushed
the
case-no-teardown-in-flight
branch
2 times, most recently
from
d903222 to
9a3e4e5
Compare
|
PR #72255: Size comparison from 4894db9 to a9d164a Full report (35 builds for bl602, bl616, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, psoc6, qpg, realtek, stm32, telink)
|
woody-apple
force-pushed
the
case-no-teardown-in-flight
branch
from
a9d164a to
69da3aa
Compare
|
PR #72255: Size comparison from 4894db9 to 5d1a7b3 Full report (20 builds for bl602, bl616, bl702, bl702l, cc13x4_26x4, cc32xx, psoc6, qpg, realtek, stm32)
|
woody-apple
force-pushed
the
case-no-teardown-in-flight
branch
from
5d1a7b3 to
23ffd84
Compare
|
PR #72255: Size comparison from 8a162c6 to 231a796 Full report (35 builds for bl602, bl616, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, psoc6, qpg, realtek, stm32, telink)
|
woody-apple
force-pushed
the
case-no-teardown-in-flight
branch
from
231a796 to
23456c2
Compare
|
PR #72255: Size comparison from 8a162c6 to 23456c2 Full report (35 builds for bl602, bl616, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, psoc6, qpg, realtek, stm32, telink)
|
woody-apple
force-pushed
the
case-no-teardown-in-flight
branch
from
23456c2 to
3fdad6f
Compare
woody-apple
force-pushed
the
case-no-teardown-in-flight
branch
from
3fdad6f to
d428651
Compare
woody-apple
force-pushed
the
case-no-teardown-in-flight
branch
from
d428651 to
23456c2
Compare
Signed-off-by: Justin Wood <woody@apple.com>
woody-apple
force-pushed
the
case-no-teardown-in-flight
branch
from
23456c2 to
f8543c1
Compare
|
PR #72255: Size comparison from 8a162c6 to f8543c1 Full report (35 builds for bl602, bl616, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, psoc6, qpg, realtek, stm32, telink)
|
Contributor
Author
|
Withdrawing. The PR's own analysis establishes there is no use-after-free here (~OperationalSessionSetup teardown is safe), and the only in-tree ReleaseSession callers behave identically through the existing path, so the cooperative-release change is effectively inert. Closing; can revisit if a real retry/backoff churn case is demonstrated. |
Contributor
Author
|
(FYI: This is Claude) Reopening — my earlier withdrawal rationale was incorrect. The claim that the change is "inert / callers behave identically" is false: both in-tree callers of |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
CASESessionManager::ReleaseSession(peerId)destroys the peer'sOperationalSessionSetupeven when it is mid-handshake, discarding the retry state (attempt count, busy-delay, exponential backoff). A higher layer calling ReleaseSession during cold-start can repeatedly tear down in-flight Sigma1/Sigma2 attempts, so each new attempt starts fresh, the longer backoff intervals are never reached, and a busy peer keeps returning BUSY for tens of seconds. This gates ReleaseSession on a newOperationalSessionSetup::IsEstablishingSession()accessor so in-flight setups are left to complete or follow their own retry cycle; established/uninitialized setups still release as before. See commit message for details.Testing
Built locally and ran the CASE / OperationalSessionSetup unit tests. Manual repro: trigger a higher-layer ReleaseSession during an in-flight CASE handshake to a slow/busy peer and confirm the retry/backoff state is preserved (no fresh-ephemeral restart loop), while established and uninitialized sessions still release as before.
Testing