Repository navigation
fix: one Closing behaviour for sm and peer - #125
Merged
Merged
Conversation
Part 2 of the protocol contract decided in #108. sm.StateMachine, sm.Client and peer.Manager handled a connection in Closing (after a DPR was sent or accepted) differently, and none of them followed RFC 6733 §5.6, whose Closing state defines only DPA, timeout and disconnect. - Once Closing is latched, answers to requests sent before it still complete. New requests and other base commands are dropped before validation and dispatch, with no handler, hook, reply or watchdog credit; the Manager's early error-answer path no longer answers them. - A request counts as sent before Closing if its write acquired the connection's write gate first. Gate acquisition registers answer ownership under the same lock as Closing, so a later request is rejected with ErrConnectionClosing and never written, and frames stay whole. Cancellation and request expiry remove that ownership. - A crossed or repeated valid DPR is answered without extending the original deadline, and any correlated DPA closes the transport, whatever its Result-Code (§5.4). - A DPR that arrives while this node's requests on the connection still await answers is answered with DIAMETER_UNABLE_TO_COMPLY (5012), as §5.4 recommends for messages likely in flight; the DPR's sender still closes. - sm.Client stops its watchdog when Closing starts, so no DWR follows a DPR, and the disconnect deadlines are armed before waiting for watchdog work, so a stalled DWR write can no longer hold Closing open. - The Manager freezes which requests may still be answered when Closing starts and disposes of reservations that were never written. Refs #108
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configuration
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
gomaja
added a commit
that referenced
this pull request
Oct 15, 2026
Step 2 of the plan adopted in #99. sm.StateMachine, sm.Client and peer.Manager each decide the base protocol themselves; #108 unified their behaviour, and this adds the single implementation they will move onto in #110-#112. Nothing calls it yet. diam/internal/protocol is a deterministic state machine for one connection generation: New, Handle(Input) []Effect and an immutable Snapshot. It owns no sockets, goroutines, timers or callbacks and depends only on diam, diam/internal/base, diam/internal/watchdog and the dictionary types. - Phases PreCER, WaitCEA, Open, Closing, Closed and Retired, with the #108 rules: answer correlation and peer binding, base validation in the #127 order, the Closing admission transaction and publication boundary of #125, DPA handling and the in-flight 5012 (RFC 6733 §§3, 5.3-5.6, 7). - A validated CER becomes a candidate that the caller activates, so the Manager can hold the CEA during election (§5.6.4) while StateMachine activates at once. - Inputs carry received messages with their receipt time, local intents, timer expiries, allocation and write outcomes (skipped publication is reported apart from failure) and retirement. Effects ask the caller to send, admit, discard, notify, arm or stop timers, freeze admission and close. - Identifiers come from an allocator the caller supplies, so the Manager keeps one reservation space; reservations outlive answers until every writer returns. - The watchdog runs through diam/internal/watchdog with receipt-time decisions (RFC 3539 §3.4.1, Appendix A). The first validated DPR cause and a rejected CEA's parsed answer and error are kept for the front ends. Tests cover every phase and input pair against the #108 record, fuzz input sequences against an independent transition model (including delayed allocation and write completion), and mutate each correlation, Closing and watchdog rule. Refs #109
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.
Part 2 of the protocol contract decided in #108. sm.StateMachine,
sm.Client and peer.Manager handled a connection in Closing (after a DPR
was sent or accepted) differently, and none of them followed RFC 6733
§5.6, whose Closing state defines only DPA, timeout and disconnect.
complete. New requests and other base commands are dropped before
validation and dispatch, with no handler, hook, reply or watchdog
credit; the Manager's early error-answer path no longer answers them.
connection's write gate first. Gate acquisition registers answer
ownership under the same lock as Closing, so a later request is
rejected with ErrConnectionClosing and never written, and frames stay
whole. Cancellation and request expiry remove that ownership.
original deadline, and any correlated DPA closes the transport,
whatever its Result-Code (§5.4).
await answers is answered with DIAMETER_UNABLE_TO_COMPLY (5012), as
§5.4 recommends for messages likely in flight; the DPR's sender still
closes.
DPR, and the disconnect deadlines are armed before waiting for watchdog
work, so a stalled DWR write can no longer hold Closing open.
starts and disposes of reservations that were never written.
Refs #108
Validation
make checkpasses locally for all modules: format, test (Go 1.27.2 and1.26.9), race, vet, lint, vuln, windows, secrets and codeql.
early closure on a crossed DPR and 2001 DPAs while requests were in flight;
here there are none of the first three and the in-flight DPAs carry 5012.
request boundary error in the Manager and in
WriteToWithRetry, and aframe-splitting bug in an intermediate design; the final version writes every
frame whole and mutating any guarded behaviour fails a named test.
Summary by cubic
Unifies Closing behaviour across
sm.StateMachine,sm.Client, andpeer.Managerso all follow RFC 6733 §5.6: once Closing is latched, new requests and other base commands are dropped before validation and dispatch, while answers to requests sent before Closing still complete.Behaviour changes
ErrConnectionClosing.DIAMETER_UNABLE_TO_COMPLY(5012).Watchdog and
peer.Managersm.Clientstops its watchdog when Closing starts so no DWR follows a DPR, and disconnect deadlines are armed before waiting for watchdog work.peer.Managerfreezes which requests may still be answered when Closing starts and disposes of admission reservations that were never written.Written for commit 58bf711. Summary will update on new commits.