feat!: report permanent reconnect failure via OnReconnectAborted - #23
Merged
Conversation
v0.9.0 made OnDisconnect fire a second time when automatic reconnection permanently gave up, but left callers no way to tell that call apart from the ordinary one preceding every reconnect attempt: both can carry a *amqp.Error, and since MaxReconnectAttempts defaults to 0 the ErrMaxReconnects sentinel never appears, so the auth-abort case was recognizable only by re-implementing dialErrorIsPermanent's reply-code classification against amqp091-go directly. A caller therefore had to choose between treating every blip as fatal and never noticing the connection was permanently dead — the exact failure the v0.9.0 fail-fast gate existed to surface. Overloading one callback with two meanings was the mistake, so split them rather than papering over it with a marker error. OnDisconnect goes back to what its name says: fired once per lost connection, before a retry, never terminally. The new OnReconnectAborted fires at most once, when the loop gives up, and receives the cause unwrapped — ErrMaxReconnects, or the rejected dial error that errors.As unwraps to its *amqp.Error. The callback identity is the signal, so no sentinel is needed. The abort callback is read at dispatch time rather than captured when the connection dropped: with unlimited attempts the loop can run for a long time, and a callback registered in that window must still be honored. safeOnDisconnect generalizes to safeCallback, naming the callback in its panic log. Integration tests assert both halves of the contract: the abort lands on OnReconnectAborted exactly once, and the connection-lost notification stays on OnDisconnect. BREAKING CHANGE: code relying on OnDisconnect firing on terminal give-up must move to OnReconnectAborted; ErrReconnectAborted is removed.
WalkthroughThe connection API now exposes ChangesReconnection callback lifecycle
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Broker
participant ReconnectLoop
participant Callbacks
participant Application
Broker-->>ReconnectLoop: connection loss or dial result
ReconnectLoop->>Callbacks: invoke OnDisconnect with loss error
ReconnectLoop->>Broker: retry connection
Broker-->>ReconnectLoop: permanent error or retry exhaustion
ReconnectLoop->>Callbacks: invoke OnReconnectAborted with terminal cause
Callbacks->>Application: deliver callback error
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://webproxy.poorya-velaei-d67.workers.dev/https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CHANGELOG.md`:
- Around line 8-42: Update the 0.10.0 section in CHANGELOG.md by adding a
distinct “Removed” subsection and documenting the removal of the previously
public ErrReconnectAborted sentinel error as a breaking user-facing change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 94a7fa4f-5499-4b3b-80f6-d25bd5f3b435
📒 Files selected for processing (6)
CHANGELOG.mdREADME.mdconfig_test.goexamples/basic/main.gointegration_test.gorabbitmq.go
Comment on lines
+8
to
+42
| ## [0.10.0] - 2026-07-28 | ||
|
|
||
| ### Added | ||
|
|
||
| - **`Connection.OnReconnectAborted` — a dedicated callback for "the connection | ||
| is never coming back".** It fires at most once, when automatic reconnection | ||
| permanently gives up, and receives the cause: `ErrMaxReconnects` when the | ||
| attempt budget ran out, otherwise the rejected dial error, which `errors.As` | ||
| unwraps to its `*amqp.Error`. Closing the connection yourself with `Close` is | ||
| not an abort and does not fire it. | ||
|
|
||
| ```go | ||
| conn.OnDisconnect(func(err error) { | ||
| health.Degraded(err) // reconnecting, with backoff | ||
| }) | ||
| conn.OnReconnectAborted(func(err error) { | ||
| health.Fatal(err) // never coming back on its own | ||
| }) | ||
| ``` | ||
|
|
||
| ### Changed | ||
|
|
||
| - **BREAKING: `OnDisconnect` no longer doubles as the terminal notification.** | ||
| v0.9.0 invoked it a second time when reconnection gave up, which left callers | ||
| no way to tell that call apart from the ordinary one preceding every reconnect | ||
| attempt: both can carry a `*amqp.Error`, and with the default | ||
| `MaxReconnectAttempts` of 0 the `ErrMaxReconnects` sentinel never appears, so | ||
| the auth-abort case was distinguishable only by re-implementing the library's | ||
| reply-code classification against `amqp091-go` directly. | ||
|
|
||
| `OnDisconnect` is now exactly what its name says: fired once per lost | ||
| connection, before reconnection is attempted, never terminally. Code that | ||
| needs the terminal signal moves to `OnReconnectAborted`; code that only logs | ||
| disconnects needs no change. | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Missing "Removed" entry for ErrReconnectAborted.
This release adds OnReconnectAborted (Added) and changes OnDisconnect semantics (Changed), but per the PR objectives ErrReconnectAborted — a previously public sentinel error — is also removed in this release. Keep a Changelog reserves a distinct ### Removed section specifically for "features or functionality that have been permanently removed," separate from Changed. Omitting it here means a reader scanning only "Removed" (or a changelog-parsing tool) could miss this breaking removal entirely.
As per coding guidelines, "Update CHANGELOG.md in Keep a Changelog format for user-facing changes."
📝 Suggested addition
+### Removed
+
+- **BREAKING: `ErrReconnectAborted` sentinel error removed.** Terminal
+ reconnect failures are now reported via `OnReconnectAborted`'s callback
+ argument (`ErrMaxReconnects` or the underlying dial error) instead of a
+ dedicated sentinel.
+
### Changed📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| ## [0.10.0] - 2026-07-28 | |
| ### Added | |
| - **`Connection.OnReconnectAborted` — a dedicated callback for "the connection | |
| is never coming back".** It fires at most once, when automatic reconnection | |
| permanently gives up, and receives the cause: `ErrMaxReconnects` when the | |
| attempt budget ran out, otherwise the rejected dial error, which `errors.As` | |
| unwraps to its `*amqp.Error`. Closing the connection yourself with `Close` is | |
| not an abort and does not fire it. | |
| ```go | |
| conn.OnDisconnect(func(err error) { | |
| health.Degraded(err) // reconnecting, with backoff | |
| }) | |
| conn.OnReconnectAborted(func(err error) { | |
| health.Fatal(err) // never coming back on its own | |
| }) | |
| ``` | |
| ### Changed | |
| - **BREAKING: `OnDisconnect` no longer doubles as the terminal notification.** | |
| v0.9.0 invoked it a second time when reconnection gave up, which left callers | |
| no way to tell that call apart from the ordinary one preceding every reconnect | |
| attempt: both can carry a `*amqp.Error`, and with the default | |
| `MaxReconnectAttempts` of 0 the `ErrMaxReconnects` sentinel never appears, so | |
| the auth-abort case was distinguishable only by re-implementing the library's | |
| reply-code classification against `amqp091-go` directly. | |
| `OnDisconnect` is now exactly what its name says: fired once per lost | |
| connection, before reconnection is attempted, never terminally. Code that | |
| needs the terminal signal moves to `OnReconnectAborted`; code that only logs | |
| disconnects needs no change. | |
| ## [0.10.0] - 2026-07-28 | |
| ### Added | |
| - **`Connection.OnReconnectAborted` — a dedicated callback for "the connection | |
| is never coming back".** It fires at most once, when automatic reconnection | |
| permanently gives up, and receives the cause: `ErrMaxReconnects` when the | |
| attempt budget ran out, otherwise the rejected dial error, which `errors.As` | |
| unwraps to its `*amqp.Error`. Closing the connection yourself with `Close` is | |
| not an abort and does not fire it. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@CHANGELOG.md` around lines 8 - 42, Update the 0.10.0 section in CHANGELOG.md
by adding a distinct “Removed” subsection and documenting the removal of the
previously public ErrReconnectAborted sentinel error as a breaking user-facing
change.
Source: Coding guidelines
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
2 participants
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.
Problem
v0.9.0 added a fail-fast gate so the reconnect loop stops instead of re-submitting
rejected credentials forever. It surfaced that terminal event by invoking
OnDisconnecta second time — but gave callers no way to tell that call apartfrom the ordinary one that precedes every reconnect attempt:
*amqp.Error, so the error type is not a signal;MaxReconnectAttemptsdefaults to0(unlimited), so theErrMaxReconnectssentinel never appears on the default configuration;
unexported
dialErrorIsPermanentreply-code classification (403/530) againstamqp091-godirectly.So a caller had to pick between treating every transient blip as fatal, or never
noticing the connection was permanently dead — the exact failure the v0.9.0 gate
was built to surface.
Change
Overloading one callback with two meanings was the mistake, so this splits them
rather than papering over it with a marker error:
OnDisconnectOnReconnectAbortedOnReconnectAbortedreceives the cause unwrapped:ErrMaxReconnectswhen theattempt budget ran out, otherwise the rejected dial error, which
errors.Asunwraps to its
*amqp.Error. The callback identity is the signal, so no markersentinel is needed. Closing the connection yourself with
Closeis not an abortand does not fire it.
Implementation notes:
dropped. With unlimited attempts the retry loop can run for a long time, and a
callback registered during that window must still be honored.
safeOnDisconnectgeneralizes tosafeCallback(name, fn, err), naming thecallback in its panic log. Both callbacks stay synchronous and panic-contained.
Breaking change
Code relying on
OnDisconnectfiring on terminal give-up must move toOnReconnectAborted, andErrReconnectAbortedis removed. Code that only logsdisconnects needs no change. The affected behavior shipped in v0.9.0 and has no
known users.
Testing
go test -raceunit suite green; new coverage for callback dispatch, theunwrapped abort causes, independent registration slots, and late registration.
tests now assert both halves of the contract: the abort lands on
OnReconnectAbortedexactly once, and the connection-lost notification stays onOnDisconnect.golangci-lint run --build-tags=integration: 0 issues.Docs updated: README (callback table, examples, error list), package docs on both
callbacks,
examples/basic/main.go, CHANGELOG (0.10.0).Summary by CodeRabbit
OnReconnectAbortedcallback for when automatic reconnection permanently stops.OnDisconnectnow fires only when a connection is lost before reconnection attempts begin.OnReconnectAborted.