Repository navigation
Fail a bad login at once and clean up inside the overall timeout (#109, #132) - #142
Merged
Merged
Conversation
When a handshake step got an error (RAKP2 HMAC mismatch for a wrong password, an RMCP+ status code such as "Unauthorized name", a reply that does not decode), the waiting state reported it but stayed in place, so the retry loops of IpmiAsyncConnector failed three more times with "Illegal connection state: Rakp1Waiting" and the caller never saw the real cause; wrong credentials were sent four times in a row (#109). - The five *Waiting states now roll the machine back to the state their Timeout transition targets before they report an error, so the same step can be tried again. - The handshake loops only send a step again on a ConnectionException, that is when no reply came; any other failure is the BMC's answer and is thrown at once. Utils.execute() only bounded the runner's call(); IpmiClient then ran close() on the calling thread, after the deadline, while the worker could still be using the connector (#132). The worker now runs call() and close() itself, on a daemon thread, so the cleanup is covered by the deadline and never races with the worker; on a timeout the caller waits up to one second for that cleanup before throwing. close() is idempotent. A FakeBmc test helper (a loopback UDP endpoint with a pluggable responder) backs the new tests: every IpmiClient method returns within the timeout plus the grace and leaves no library thread alive against a BMC that never answers; a reply that does not decode fails the cipher-suites step once, without a resend, and the step can be retried. Verified on the Lenovo IMM and the GIGABYTE BMC: a wrong password fails in 3.4 s with "Authentication check failed", an unknown user in 0.3 s with "Unauthorized name", each sent once; sensor walks unchanged. Fixes #109, fixes #132. Co-Authored-By: Claude Fable 5.1 <[email protected]>
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. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b9324eb71e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
- Handshake steps are sent again on a transient completion code (busy, out of resources, initializing, internal timeout) as well as on a missing reply; CompletionCode.isTransient() now holds the list the sync connector already used. - The worker closes the runner with try-with-resources, so a failing close() is attached as suppressed to the real failure instead of replacing it. - When the worker outlives the cleanup grace (a call that cannot be interrupted), a WARN says the port is released when it returns, and the docs no longer promise the port back within one second. - Javadoc on the FakeBmc accessors. Co-Authored-By: Claude Fable 5.1 <[email protected]>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d31430be65
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Co-Authored-By: Claude Fable 5.1 <[email protected]>
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
1 participant
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.
#109: a failed login left the state machine stuck
When a handshake step got an error, the
*Waitingstate emitted anErrorActionbut stayed in place.IpmiAsyncConnectorthen retried the step three more times, each failing withIllegal connection state: Rakp1Waiting, which is what the caller got; the real cause was only anERRORlog line, and wrong credentials went out four times (account lockouts).*Waitingstates roll the machine back (to what theirTimeouttransition targets) before reporting an error, so the step can be tried again.ConnectionException(no reply). Any other failure is the BMC's answer and is thrown at once:IllegalArgumentException: Authentication check failedfor a wrong password or BMC key,IPMIException: Unauthorized name.for an unknown user, and so on.No dedicated
AuthenticationExceptionwas added: the cause now reaches the caller as is, and the docs list the messages. Say so if you want a typed exception for MetricsHub to classify on.#132: cleanup ran on the calling thread, after the deadline
Utils.execute()only boundedcall(); the try-with-resources inIpmiClientthen ranclose()on the calling thread while the worker could still be using the connector. Now the worker runscall()andclose()itself (on a daemon thread namedipmi-client), so the cleanup is inside the deadline and never races with the worker. On a timeout the caller waits up to 1 s for the worker to close the session and release the port, then throws.AbstractIpmiRunner.close()is idempotent.Close Session is still not confirmed: a lost Close Session leaves the BMC session to expire on its own (typically 60 s). Confirming it needs a new waiting state for the Authcap stage; left out as a separate change if it proves necessary.
Tests
New
FakeBmchelper (loopback UDP endpoint with a pluggable responder):IpmiClientTest: against a BMC that never answers, each of the fiveIpmiClientmethods throwsTimeoutExceptionwithin the timeout plus the grace, and no library thread is left alive.IpmiConnectorTest: a reply that does not decode fails the cipher-suites step once, with no resend, and the step can be tried again (noIllegal connection state).mvn verifyon JDK 17: 43 tests, 0 checkstyle / PMD / CPD / SpotBugs findings.Live
ExecutionException→IllegalArgumentException: Authentication check failedafter 3.4 s, oneERRORline (credentials sent once).IPMIException: Unauthorized name.after 0.26 s.TimeoutExceptionat 5.05 s, JVM gone at 5.23 s.Fixes #109, fixes #132.
🤖 Generated with Claude Code