HYPERFLEET-641: Add Health() method, optimize integration tests, fix goroutine leak #12
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a Publisher Health() API and broker-specific health checks. The Publisher interface gains Health(); the concrete publisher stores an optional healthCheck func and optional io.Closer healthCloser. New RabbitMQ health check inspects AMQP publisher Closed()/IsConnected(); Google Pub/Sub health check reuses a persistent pubsub.Client and performs a lightweight ListTopics (page size 1, 3s timeout). NewPublisher wires healthCheck and healthCloser; Close() closes healthCloser. Tests, README health documentation, and go.mod updates for pubsub/v2 were added. Sequence Diagram(s)sequenceDiagram
participant Client
participant Publisher
participant HealthCheck as HealthCheckFunc
participant Broker as RabbitMQ/GooglePubSub
participant PubSubClient as Pub/Sub Client
Client->>Publisher: Health()
activate Publisher
Publisher->>HealthCheck: invoke()
activate HealthCheck
alt RabbitMQ flow
HealthCheck->>Broker: type assert to *amqp.Publisher
Broker-->>HealthCheck: publisher ref
HealthCheck->>Broker: Closed() / IsConnected()
Broker-->>HealthCheck: state/result
else Google Pub/Sub flow
HealthCheck->>PubSubClient: ListTopics(ctx, PageSize=1, Timeout=3s)
PubSubClient-->>HealthCheck: iterator
HealthCheck->>PubSubClient: iterator.Next()
PubSubClient-->>HealthCheck: iterator.Done or error
end
HealthCheck-->>Publisher: nil or error
deactivate HealthCheck
Publisher-->>Client: nil or error
deactivate Publisher
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@broker/broker.go`:
- Around line 78-86: The publisher `pub` created by newGooglePubSubPublisher is
leaked when pubsub.NewClient fails; update the error path after calling
pubsub.NewClient (and before returning) to close/cleanup the previously created
publisher `pub` (or call its Close/Stop method) so resources are released, e.g.,
call the publisher's close method and handle/aggregate any close error before
returning the fmt.Errorf from the failing pubsub.NewClient call; reference
newGooglePubSubPublisher, pub, pubsub.NewClient, and healthClient to find the
exact spot to insert the cleanup.
🧹 Nitpick comments (1)
broker/publisher.go (1)
58-62: Consider adding a nil check forhealthCheckto prevent potential panics.If
healthCheckis nil (e.g., in test scenarios or future broker types), callingp.healthCheck()will panic. A defensive nil check would improve robustness.🛡️ Proposed defensive fix
func (p *publisher) Health() error { + if p.healthCheck == nil { + return nil // No health check configured + } return p.healthCheck() }
rafabene
force-pushed
the
HYPERFLEET-641
branch
from
b29ff73 to
ae23849
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@broker/publisher.go`:
- Around line 58-62: The Health() method on publisher currently calls
p.healthCheck() unguarded and will panic if healthCheck is nil; modify
publisher.Health() to first check if p == nil or p.healthCheck == nil and return
a clear error (e.g., errors.New or fmt.Errorf like "health check not
configured") instead of invoking the nil function, otherwise call and return
p.healthCheck(); reference the publisher type and the Health() and healthCheck
symbols when making the change.
ae23849 to
b2eb802
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@README.md`:
- Around line 117-119: The README note about Google Pub/Sub ends with a
truncated sentence; update the sentence referencing the Google Pub/Sub publisher
(via Watermill/Google Cloud SDK) to finish the thought—e.g., "The app should
call Close() to avoid leaking resources/goroutines"—so it clearly states what
Close() prevents (background goroutines or resources) and mention the Close()
method explicitly for clarity.
b2eb802 to
de19564
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@broker/rabbitmq.go`:
- Around line 28-42: newRabbitMQHealthCheck may panic if pub is a nil
*amqp.Publisher because the type assertion succeeds but amqpPub can be nil;
after the type assertion in newRabbitMQHealthCheck (checking
pub.(*amqp.Publisher)), add an explicit nil check for amqpPub and return a
descriptive error if nil before calling amqpPub.Closed() or
amqpPub.IsConnected(); reference the types message.Publisher and *amqp.Publisher
and the methods Closed and IsConnected when implementing the guard.
🧹 Nitpick comments (1)
broker/health_test.go (1)
12-37: Consider adding a test case for nilhealthCheck.The
Health()method includes a guard forp.healthCheck == nilreturning"health check not configured", but there's no test covering this path. Adding this would improve coverage of the defensive nil check.💡 Suggested test case
t.Run("nil healthCheck returns error", func(t *testing.T) { p := &publisher{ logger: mockLogger, healthCheck: nil, } err := p.Health() assert.Error(t, err) assert.Contains(t, err.Error(), "health check not configured") })
|
After merging, tag |
rafabene
force-pushed
the
HYPERFLEET-641
branch
from
de19564 to
47783c6
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@README.md`:
- Around line 120-134: Add a note to the Health Check section explaining that
the Google Pub/Sub Health() implementation uses the ListTopics API and therefore
requires the IAM permission pubsub.topics.list on the project; clarify that this
is separate from publish permissions (e.g., pubsub.topics.publish) so a
principal that can publish but lacks pubsub.topics.list will cause
Publisher.Health() to fail and may misconfigure readiness probes.
🧹 Nitpick comments (1)
broker/health_test.go (1)
39-59: Clarify the non-AMQP publisher comment.✏️ Suggested tweak
- // Pass a nil publisher wrapped in the message.Publisher interface - // to trigger the type assertion failure + // Pass a non-AMQP publisher to trigger the type assertion failure
- Add Health() error to Publisher interface for readiness probes - RabbitMQ: in-memory check via IsConnected()/Closed() (no network call) - Google Pub/Sub: lightweight ListTopics API call (page size 1, 3s timeout) - Fix publisher resource leak when health check client creation fails - Guard Health() against nil healthCheck to prevent panic - Guard RabbitMQ health check against nil *amqp.Publisher to prevent panic - Fix truncated sentence in README for Google Pub/Sub Close() note - Document pubsub.topics.list IAM permission requirement for Health()
- GO-2026-4341: memory exhaustion in net/url query parameter parsing - GO-2026-4340: TLS handshake messages processed at incorrect encryption level - GO-2026-4337: unexpected TLS session resumption
rafabene
force-pushed
the
HYPERFLEET-641
branch
from
47783c6 to
b5816a6
Compare
- Share testcontainers across tests via TestMain (4 containers total instead of ~25), significantly reducing test execution time - Fix goroutine leak in subscriber Close() by cancelling router contexts before waiting for goroutines to finish - Fix GenerateSubscriptionName to include topic name, preventing PubSub subscription conflicts across different topics - Use unique topic/subscription names per test to avoid interference when sharing containers - Add configurable log level to NewTestLogger (WithLevel option), set tests to WARN level to suppress Watermill INFO-level chatter - Add -p 1 to Makefile test-integration target for streaming output
rafabene
changed the title
|
@coderabbitai resume |
1 participant
Summary
Health() errormethod to thePublisherinterface for readiness probesIsConnected()/Closed()(no network call)ListTopicsAPI call (page size 1, 3s timeout) using a persistent clientTestMain(4 containers total instead of ~25)Close()by cancelling router contextsGenerateSubscriptionNameto include topic name, preventing PubSub subscription conflictsNewTestLogger(WithLeveloption), tests use WARN level-p 1totest-integrationMakefile target for streaming outputTest plan
Health()with healthy/unhealthy publishermake testpassingmake lintpassing (0 issues)govulncheckpassing (0 vulnerabilities)