Add more tests for open session - #704
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The new concurrent SessionOpenInit test currently doesn’t pin the expected rejection status for losing threads, which can mask unintended failure modes and weaken the regression signal.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR strengthens TBOR session-handshake coverage by adding multi-threaded regression tests that ensure losing SessionOpenInit / SessionOpenFinish attempts do not roll back the winning session state, and updates the spec coverage document to reflect current backend-neutral test naming and the expanded open-session suite.
Changes:
- Add a multi-threaded
SessionOpenFinish“single winner” test that verifies the winning Active session remains usable (viaPskChange) after concurrent losers. - Add a native-only multi-threaded
SessionOpenInit“single winner” test that verifies the winning Pending session remains finishable after concurrent losers. - Update
SPEC_COVERAGE.mdto reflect backend-neutral test names and add the new open-session concurrency/capacity coverage rows.
File summaries
| File | Description |
|---|---|
| ddi/tbor/types/tests/SPEC_COVERAGE.md | Updates spec coverage matrix/test naming notes and documents the new concurrency/capacity coverage for session open. |
| ddi/tbor/types/tests/commands/open_session.rs | Adds multi-threaded regression tests for SessionOpenInit/SessionOpenFinish rollback behavior and wires required imports. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| let (winners, rejections): (Vec<_>, Vec<_>) = results.into_iter().partition(Result::is_ok); | ||
| assert_eq!( | ||
| winners.len(), | ||
| 1, | ||
| "exactly one concurrent SessionOpenInit must succeed", | ||
| ); | ||
| assert_eq!( | ||
| rejections.len(), | ||
| THREAD_COUNT - 1, | ||
| "every non-winning SessionOpenInit must be rejected", | ||
| ); | ||
|
|
||
| let pending = winners.into_iter().next().unwrap().unwrap(); | ||
| let session = ctx | ||
| .session_open_finish(pending) | ||
| .expect("losing inits must not destroy the winning Pending session"); | ||
| ctx.session_close(session.session_id) | ||
| .expect("close winning session"); | ||
| } |
| fn duplicate_pending(pending: &PendingHandshake) -> PendingHandshake { | ||
| PendingHandshake { | ||
| session_id: pending.session_id, | ||
| psk_id: pending.psk_id, | ||
| session_type: pending.session_type, | ||
| exported: pending.exported.clone(), | ||
| pk_init: pending.pk_init, | ||
| pk_resp: pending.pk_resp, | ||
| pk_hsm: pending.pk_hsm, | ||
| } | ||
| } |
There was a problem hiding this comment.
🟢 Approval recommended
Changes are isolated to tests and documentation, and the new concurrency assertions concretely validate the intended rollback invariants without altering production code paths.
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0 new
- Review effort level: Lite
Add multi-thread tests to check the proper rollback behavior in FW (failure thread should not rollback the sucessful active session)