-
Notifications
You must be signed in to change notification settings - Fork 30
Wrap the pre-session v2 policy change with timeout #615
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.
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds timeout protection for the pre-session v2 policy exchange to prevent potential hangs during migration, addressing issue #603. The changes refactor timeout constant definitions to be scoped closer to their usage points.
Key changes:
- Wraps the pre-session policy exchange with a 60-second timeout
- Moves timeout constant definitions from global scope to function scope
- Consolidates timeout constant declarations for better code organization
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/migtd/src/spdm/mod.rs | Removes global SPDM_TIMEOUT constant definition |
| src/migtd/src/migration/session.rs | Adds timeout wrapper for policy exchange and moves timeout constants to local scope |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const PRE_SESSION_TIMEOUT: Duration = Duration::from_secs(60); // 60 seconds | ||
| #[cfg(feature = "policy_v2")] | ||
| let remote_policy = Box::pin(with_timeout( | ||
| PRE_SESSION_TIMEOUT, | ||
| pre_session_data_exchange(&mut transport), | ||
| )) | ||
| .await??; |
Copilot
AI
Nov 21, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The constant PRE_SESSION_TIMEOUT is defined inside the function but before a feature gate. Consider moving it inside the #[cfg(feature = "policy_v2")] block since it's only used within that feature context, similar to how SPDM_TIMEOUT is placed inside its feature gate at line 994-995.
| const PRE_SESSION_TIMEOUT: Duration = Duration::from_secs(60); // 60 seconds | |
| #[cfg(feature = "policy_v2")] | |
| let remote_policy = Box::pin(with_timeout( | |
| PRE_SESSION_TIMEOUT, | |
| pre_session_data_exchange(&mut transport), | |
| )) | |
| .await??; | |
| { | |
| const PRE_SESSION_TIMEOUT: Duration = Duration::from_secs(60); // 60 seconds | |
| let remote_policy = Box::pin(with_timeout( | |
| PRE_SESSION_TIMEOUT, | |
| pre_session_data_exchange(&mut transport), | |
| )) | |
| .await??; | |
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remote_policy will be used later, connot warp it with {} false suggestion.
But later SPDM_TIMEOUT could adopt this style. Updated.
b778f36 to
02d3f1a
Compare
Wrap the pre-session v2 policy change with a timeout to prevent potential
hang during pre migration.
02d3f1a to
38d9ba6
Compare
Wrap the pre-session v2 policy change with a timeout to prevent potential
hang during pre migration.
Fix problem 1 of issue: #603