feat: implement service to service discovery - #7
Conversation
📝 WalkthroughWalkthroughThe PR introduces Serf-based cluster membership discovery by adding direct dependencies (hashicorp/serf, travisjeffery/go-dynaport), implementing a new Membership component with event-driven join/leave handling, and providing comprehensive integration tests for multi-node cluster scenarios. Changes
Sequence DiagramsequenceDiagram
participant App as Application
participant Mem as Membership
participant Serf as Serf Cluster
participant Handler as Handler (Implementation)
App->>Mem: New(handler, config)
Mem->>Serf: setupSerf() - Initialize & Join
Mem->>Mem: Start eventHandler() goroutine
Serf->>Mem: Emit MemberJoin Event
Mem->>Mem: eventHandler() receives event
Mem->>Handler: handleJoin(member)
Handler->>Handler: Custom join logic
Serf->>Mem: Emit MemberLeave Event
Mem->>Mem: eventHandler() receives event
Mem->>Handler: handleLeave(member)
Handler->>Handler: Custom leave logic
App->>Mem: Leave()
Mem->>Serf: Gracefully depart cluster
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 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)
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: 5
🤖 Fix all issues with AI agents
In `@go.mod`:
- Around line 10-12: The go.mod lists github.com/travisjeffery/go-dynaport
v1.0.0 which appears unmaintained; review usages of go-dynaport in the codebase
(search for imports of "github.com/travisjeffery/go-dynaport" and functions it
exposes) and either (a) replace it with a maintained alternative that provides
dynamic port allocation, (b) vendor a minimal fork/patch of go-dynaport in the
repo and add tests, or (c) add a TODO and create an issue documenting the
maintenance risk and rationale for keeping it; ensure any replacement preserves
behavior where used (adjust call sites accordingly) and update go.mod/go.sum and
CI to reflect the change.
In `@internal/discovery/membership_test.go`:
- Around line 68-94: Add test cleanup to shut down all Serf nodes created by
TestMembership: after assembling the members via setupMember, register a
t.Cleanup that iterates over the members slice returned by setupMember and for
each member calls serf.Leave() (if not already left) and then the Serf
shutdown/close method (e.g., Shutdown() or Close()) to terminate connections and
goroutines; update TestMembership to use this t.Cleanup so all serf instances
are closed regardless of test outcome.
In `@internal/discovery/membership.go`:
- Around line 39-55: The New function creates a logger then calls
logger.Named("membership") but ignores the returned logger so the
Membership.logger is not namespaced; update New to assign the result of
logger.Named("membership") (e.g., logger = logger.Named("membership")) before
constructing the Membership struct so the namespace takes effect for the
Membership.logger used in methods like setupSerf and elsewhere.
- Around line 163-165: The Leave method currently only calls m.serf.Leave(),
leaving Serf goroutines/sockets running; update Membership.Leave to call
m.serf.Leave() first and then m.serf.Shutdown() to fully clean up Serf
resources. Ensure you capture and return errors from both calls (e.g., if
Leave() returns an error return it, otherwise return any Shutdown() error) so
failures from either m.serf.Leave() or m.serf.Shutdown() are propagated by
Membership.Leave.
- Around line 96-124: In eventHandler, add handling for serf.EventMemberFailed
and serf.EventMemberReap so failed/reaped nodes are treated like leaves: for
those cases perform the same type assertion to serf.MemberEvent as in
EventMemberLeave, iterate me.Members, skip local members using
m.isLocal(member), and call m.handleLeave(member) for each non-local member;
update the switch in the eventHandler method to include these two event types
alongside EventMemberLeave.
🧹 Nitpick comments (1)
internal/discovery/membership.go (1)
57-94: Consider checkinglen(m.StartJoinAddrs) > 0instead ofnilfor better clarity.The current nil check works, but checking the slice length is more idiomatic and defensive against empty slices produced by flag parsers. While
serf.Joinhandles empty slices gracefully (returning a no-op), explicitly checkinglen(m.StartJoinAddrs) > 0makes the intent clearer and avoids an unnecessary function call.🔧 Suggested fix
- if m.StartJoinAddrs != nil { + if len(m.StartJoinAddrs) > 0 { _, err := m.serf.Join(m.StartJoinAddrs, true) if err != nil { return err } }
| github.com/hashicorp/serf v0.10.2 | ||
| github.com/stretchr/testify v1.11.1 | ||
| github.com/travisjeffery/go-dynaport v1.0.0 |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
github.com/hashicorp/serf v0.10.2 security advisories CVE
💡 Result:
github.com/hashicorp/serf v0.10.2 (tagged Jan 14, 2025) currently has no published GitHub Security Advisories, and therefore no CVEs are listed against the Serf repository/module on GitHub for that version. [1] [2]
Sources:
[1] hashicorp/serf tags (shows v0.10.2) (github.com)
[2] hashicorp/serf security page (“There aren’t any published security advisories”) (github.com)
Citations:
🌐 Web query:
github.com/travisjeffery/go-dynaport v1.0.0 security advisories
💡 Result:
- GitHub Security Advisories:
travisjeffery/go-dynaportcurrently has no published security advisories (the repo Security page states “There aren’t any published security advisories” and shows Security 0). [1] - Version context: On pkg.go.dev, the module’s latest tagged version is v1.0.0 (published Dec 18, 2017). [2]
Conclusion: There are no security advisories published for go-dynaport v1.0.0. [1]
Sources
[1] GitHub repository security overview for travisjeffery/go-dynaport (github.com)
[2] pkg.go.dev module page for github.com/travisjeffery/go-dynaport (pkg.go.dev)
Citations:
- 1: https://github.com/travisjeffery/go-dynaport/security
- 2: https://pkg.go.dev/github.com/travisjeffery/go-dynaport?utm_source=openai
🌐 Web query:
hashicorp serf latest release version 2024 2025
💡 Result:
HashiCorp Serf’s latest release in the 2024–2025 window is v0.10.2, released on January 14, 2025. [1][2]
There were no Serf releases in 2024 (the prior release before v0.10.2 was v0.10.1 on October 4, 2022). [1]
Sources: GitHub tags/releases for hashicorp/serf [1], Go package index (pkg.go.dev) [2].
🌐 Web query:
travisjeffery go-dynaport latest version release
💡 Result:
The latest tagged release of github.com/travisjeffery/go-dynaport is v1.0.0, published December 18, 2017. [1]
To use it:
go get github.com/travisjeffery/go-dynaport@v1.0.0Flag go-dynaport maintenance concern: latest version (v1.0.0) is from December 2017 with no recent updates.
Both dependencies are free of published security advisories. Serf v0.10.2 (released January 14, 2025) is current. However, go-dynaport v1.0.0 is the latest available version but was released in 2017 and shows no signs of active maintenance. Consider whether this dependency is still appropriate for production use or if an actively maintained alternative exists.
🤖 Prompt for AI Agents
In `@go.mod` around lines 10 - 12, The go.mod lists
github.com/travisjeffery/go-dynaport v1.0.0 which appears unmaintained; review
usages of go-dynaport in the codebase (search for imports of
"github.com/travisjeffery/go-dynaport" and functions it exposes) and either (a)
replace it with a maintained alternative that provides dynamic port allocation,
(b) vendor a minimal fork/patch of go-dynaport in the repo and add tests, or (c)
add a TODO and create an issue documenting the maintenance risk and rationale
for keeping it; ensure any replacement preserves behavior where used (adjust
call sites accordingly) and update go.mod/go.sum and CI to reflect the change.
| func TestMembership(t *testing.T) { | ||
| m, h := setupMember(t, nil) | ||
| m, _ = setupMember(t, m) | ||
| m, _ = setupMember(t, m) | ||
|
|
||
| require.EventuallyWithT(t, func(c *assert.CollectT) { | ||
| // expect 2 join events for the 2 new members | ||
| assert.Equal(c, 2, len(h.joins), "expected 2 join events") | ||
|
|
||
| for _, m := range m { | ||
| assert.Equal(c, 3, len(m.serf.Members()), "expected all nodes to have 3 members") | ||
| } | ||
|
|
||
| }, time.Second*5, time.Millisecond*100, "expected all nodes joined the cluster") | ||
|
|
||
| // leave the last node and expect a leave event | ||
| require.NoError(t, m[2].serf.Leave()) | ||
|
|
||
| // the remaining nodes should have 2 alive members | ||
| require.EventuallyWithT(t, func(c *assert.CollectT) { | ||
| assert.Equal(c, 1, len(h.leaves), "expected 1 leave event") | ||
| // Notes: member leaves are not immediately reflected in the member list | ||
| for _, m := range m[:2] { | ||
| assert.Equal(c, 2, m.membersAlive(), "expected remaining nodes to have 2 alive members") | ||
| } | ||
| }, time.Second*5, time.Millisecond*100, "expected remaining nodes to have 2 alive members") | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "membership_test.go" | head -20Repository: Devin-Yeung/proglog
Length of output: 102
🏁 Script executed:
find . -type f -name "*.go" -path "*/discovery/*" | head -20Repository: Devin-Yeung/proglog
Length of output: 137
🏁 Script executed:
git ls-files | grep -E "discovery.*\.go$" | head -20Repository: Devin-Yeung/proglog
Length of output: 133
🏁 Script executed:
cat -n ./internal/discovery/membership_test.goRepository: Devin-Yeung/proglog
Length of output: 3301
🏁 Script executed:
cat -n ./internal/discovery/membership.goRepository: Devin-Yeung/proglog
Length of output: 5774
Add cleanup to shut down Serf nodes after the test.
Currently only the last node leaves, leaving two Serf instances running until test exit, which can leak goroutines/sockets and cause flakes in a larger suite.
🧹 Suggested fix
func TestMembership(t *testing.T) {
m, h := setupMember(t, nil)
m, _ = setupMember(t, m)
m, _ = setupMember(t, m)
+
+ t.Cleanup(func() {
+ for _, member := range m {
+ _ = member.Leave()
+ }
+ })
require.EventuallyWithT(t, func(c *assert.CollectT) {
// expect 2 join events for the 2 new members
assert.Equal(c, 2, len(h.joins), "expected 2 join events")
@@
- require.NoError(t, m[2].serf.Leave())
+ require.NoError(t, m[2].Leave())🤖 Prompt for AI Agents
In `@internal/discovery/membership_test.go` around lines 68 - 94, Add test cleanup
to shut down all Serf nodes created by TestMembership: after assembling the
members via setupMember, register a t.Cleanup that iterates over the members
slice returned by setupMember and for each member calls serf.Leave() (if not
already left) and then the Serf shutdown/close method (e.g., Shutdown() or
Close()) to terminate connections and goroutines; update TestMembership to use
this t.Cleanup so all serf instances are closed regardless of test outcome.
| func New(handler Handler, config Config) (*Membership, error) { | ||
| logger, err := zap.NewDevelopment() | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| logger.Named("membership") | ||
|
|
||
| c := &Membership{ | ||
| Config: config, | ||
| handler: handler, | ||
| logger: logger, | ||
| } | ||
| if err := c.setupSerf(); err != nil { | ||
| return nil, err | ||
| } | ||
| return c, nil | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the full context of this file
cat -n internal/discovery/membership.go | head -100Repository: Devin-Yeung/proglog
Length of output: 3180
🏁 Script executed:
# Search for all usages of logger in this file
rg "logger" internal/discovery/membership.go -A 2 -B 2Repository: Devin-Yeung/proglog
Length of output: 539
🏁 Script executed:
# Check if there are any method calls on the logger variable after it's created
rg "logger\." internal/discovery/membership.goRepository: Devin-Yeung/proglog
Length of output: 107
Assign the named logger so the namespace takes effect.
logger.Named("membership") returns a new logger but the result is ignored. The unmodified logger is then assigned to the struct without the namespace.
🔧 Suggested fix
- logger.Named("membership")
+ logger = logger.Named("membership")📝 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.
| func New(handler Handler, config Config) (*Membership, error) { | |
| logger, err := zap.NewDevelopment() | |
| if err != nil { | |
| return nil, err | |
| } | |
| logger.Named("membership") | |
| c := &Membership{ | |
| Config: config, | |
| handler: handler, | |
| logger: logger, | |
| } | |
| if err := c.setupSerf(); err != nil { | |
| return nil, err | |
| } | |
| return c, nil | |
| } | |
| func New(handler Handler, config Config) (*Membership, error) { | |
| logger, err := zap.NewDevelopment() | |
| if err != nil { | |
| return nil, err | |
| } | |
| logger = logger.Named("membership") | |
| c := &Membership{ | |
| Config: config, | |
| handler: handler, | |
| logger: logger, | |
| } | |
| if err := c.setupSerf(); err != nil { | |
| return nil, err | |
| } | |
| return c, nil | |
| } |
🤖 Prompt for AI Agents
In `@internal/discovery/membership.go` around lines 39 - 55, The New function
creates a logger then calls logger.Named("membership") but ignores the returned
logger so the Membership.logger is not namespaced; update New to assign the
result of logger.Named("membership") (e.g., logger = logger.Named("membership"))
before constructing the Membership struct so the namespace takes effect for the
Membership.logger used in methods like setupSerf and elsewhere.
| func (m *Membership) eventHandler() { | ||
| for e := range m.events { | ||
| switch e.EventType() { | ||
| case serf.EventMemberJoin: | ||
| // type assertion never fails because only MemberEvent could have this event type | ||
| me := e.(serf.MemberEvent) | ||
| for _, member := range me.Members { | ||
| // gossip protocol will propagate the message to all nodes include the local node | ||
| // so we need to ignore the event if it's about the local node | ||
| if m.isLocal(member) { | ||
| continue | ||
| } | ||
| m.handleJoin(member) | ||
| } | ||
| case serf.EventMemberLeave: | ||
| // type assertion never fails because only MemberEvent could have this event type | ||
| me := e.(serf.MemberEvent) | ||
| for _, member := range me.Members { | ||
| // gossip protocol will propagate the message to all nodes include the local node | ||
| // so we need to ignore the event if it's about the local node | ||
| if m.isLocal(member) { | ||
| continue | ||
| } | ||
| m.handleLeave(member) | ||
| } | ||
| default: | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's explore the repository structure
fd -t f -e go | head -20Repository: Devin-Yeung/proglog
Length of output: 548
🏁 Script executed:
# Look for imports and usage of serf in the codebase
rg -t go "serf\." --no-heading | head -30Repository: Devin-Yeung/proglog
Length of output: 1693
🏁 Script executed:
# Search for EventMember types specifically
rg -t go "EventMember" --no-headingRepository: Devin-Yeung/proglog
Length of output: 187
🏁 Script executed:
# Check the membership.go file context
cat -n internal/discovery/membership.go | head -150Repository: Devin-Yeung/proglog
Length of output: 5085
🏁 Script executed:
# Search for any references to Failed or Reap event types
rg -t go "EventMemberFailed|EventMemberReap|Failed|Reap" --no-headingRepository: Devin-Yeung/proglog
Length of output: 45
🏁 Script executed:
# Search for event type constants in imports or comments
rg -t go "serf\.Event" --no-heading -A 2 -B 2Repository: Devin-Yeung/proglog
Length of output: 1279
🏁 Script executed:
# Check go.mod for Serf dependency version
cat go.mod | grep serfRepository: Devin-Yeung/proglog
Length of output: 97
🏁 Script executed:
# Look at the test file to understand expected behavior
cat -n internal/discovery/membership_test.goRepository: Devin-Yeung/proglog
Length of output: 3301
🌐 Web query:
Serf EventMemberFailed EventMemberReap event types hashicorp serf
💡 Result:
In HashiCorp Serf (github.com/hashicorp/serf/serf), these are membership event types:
EventMemberFailed(string:"member-failed") — emitted when Serf detects a node has failed (i.e., becomes unresponsive and transitions to the failed state). Serf will keep the failed member around and periodically try to reconnect. [1], [2]EventMemberReap(string:"member-reap") — emitted when Serf removes (“reaps”) a node that was previously failed or left from the membership list. The timing is controlled byreconnect_timeout(for failed nodes) andtombstone_timeout(for left nodes). [3], [2]
Sources:
[1] https://pkg.go.dev/github.com/hashicorp/serf/serf (EventType constants) (pkg.go.dev)
[2] https://git.baserock.org/cgit/delta/docker.git/commit/vendor/src/github.com/hashicorp/serf/serf?h=v18.04.0-ce-rc2&id=f2614f2107c838d014d31b806e3b8a9f1395cb2b (event type string mappings) (git.baserock.org)
[3] https://sources.debian.org/src/golang-github-hashicorp-serf/0.10.1-1/CHANGELOG.md (member-reap semantics and timeouts) (sources.debian.org)
Citations:
- 1: https://pkg.go.dev/github.com/hashicorp/serf/serf?utm_source=openai
- 2: https://git.baserock.org/cgit/delta/docker.git/commit/vendor/src/github.com/hashicorp/serf/serf?h=v18.04.0-ce-rc2&id=f2614f2107c838d014d31b806e3b8a9f1395cb2b&utm_source=openai
- 3: https://sources.debian.org/src/golang-github-hashicorp-serf/0.10.1-1/CHANGELOG.md?utm_source=openai
🌐 Web query:
Serf v0.10.2 member event types documentation
💡 Result:
In Serf v0.10.2, the membership (“member”) event types you can receive (e.g., via event_handlers, serf monitor, or the RPC event stream) are:
member-join— a node joined the cluster. [1]member-leave— a node left gracefully. [1]member-failed— a node is suspected/marked failed (unresponsive). [1]member-update— a node’s member data changed (typically tags/metadata). [1]member-reap— Serf removed a failed or left node after the relevant timeout(s). [2]
In event handlers, Serf exposes the event type in SERF_EVENT, and this value can be one of member-join, member-leave, member-failed, member-update, member-reap (plus non-member types like user and query). [3]
Sources: [1], [2], [3]
[1] (iankent.uk)
[2] (sources.debian.org)
[3] (dev.to)
Citations:
- 1: https://iankent.uk/blog/getting-started-with-hashicorp-serf/?utm_source=openai
- 2: https://sources.debian.org/src/golang-github-hashicorp-serf/0.10.1-1/CHANGELOG.md?utm_source=openai
- 3: https://dev.to/satrobit/nginx-loadbalancer-cluster-with-automatic-configuration-and-node-failure-detection-using-serf-9pp?utm_source=openai
Add handling for failed and reaped member events.
Serf emits EventMemberFailed when nodes become unresponsive and EventMemberReap when they are removed from the membership list after timeout. Currently, these events are silently ignored, preventing the handler from being notified when nodes fail. All three events (leave, failed, reap) represent node removal and should trigger the same handleLeave logic.
Suggested fix
- case serf.EventMemberLeave:
+ case serf.EventMemberLeave, serf.EventMemberFailed, serf.EventMemberReap:
// type assertion never fails because only MemberEvent could have this event type
me := e.(serf.MemberEvent)
for _, member := range me.Members {
// gossip protocol will propagate the message to all nodes include the local node
// so we need to ignore the event if it's about the local node📝 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.
| func (m *Membership) eventHandler() { | |
| for e := range m.events { | |
| switch e.EventType() { | |
| case serf.EventMemberJoin: | |
| // type assertion never fails because only MemberEvent could have this event type | |
| me := e.(serf.MemberEvent) | |
| for _, member := range me.Members { | |
| // gossip protocol will propagate the message to all nodes include the local node | |
| // so we need to ignore the event if it's about the local node | |
| if m.isLocal(member) { | |
| continue | |
| } | |
| m.handleJoin(member) | |
| } | |
| case serf.EventMemberLeave: | |
| // type assertion never fails because only MemberEvent could have this event type | |
| me := e.(serf.MemberEvent) | |
| for _, member := range me.Members { | |
| // gossip protocol will propagate the message to all nodes include the local node | |
| // so we need to ignore the event if it's about the local node | |
| if m.isLocal(member) { | |
| continue | |
| } | |
| m.handleLeave(member) | |
| } | |
| default: | |
| } | |
| } | |
| } | |
| func (m *Membership) eventHandler() { | |
| for e := range m.events { | |
| switch e.EventType() { | |
| case serf.EventMemberJoin: | |
| // type assertion never fails because only MemberEvent could have this event type | |
| me := e.(serf.MemberEvent) | |
| for _, member := range me.Members { | |
| // gossip protocol will propagate the message to all nodes include the local node | |
| // so we need to ignore the event if it's about the local node | |
| if m.isLocal(member) { | |
| continue | |
| } | |
| m.handleJoin(member) | |
| } | |
| case serf.EventMemberLeave, serf.EventMemberFailed, serf.EventMemberReap: | |
| // type assertion never fails because only MemberEvent could have this event type | |
| me := e.(serf.MemberEvent) | |
| for _, member := range me.Members { | |
| // gossip protocol will propagate the message to all nodes include the local node | |
| // so we need to ignore the event if it's about the local node | |
| if m.isLocal(member) { | |
| continue | |
| } | |
| m.handleLeave(member) | |
| } | |
| default: | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@internal/discovery/membership.go` around lines 96 - 124, In eventHandler, add
handling for serf.EventMemberFailed and serf.EventMemberReap so failed/reaped
nodes are treated like leaves: for those cases perform the same type assertion
to serf.MemberEvent as in EventMemberLeave, iterate me.Members, skip local
members using m.isLocal(member), and call m.handleLeave(member) for each
non-local member; update the switch in the eventHandler method to include these
two event types alongside EventMemberLeave.
| // Leave makes the node leave the Serf cluster gracefully. | ||
| func (m *Membership) Leave() error { | ||
| return m.serf.Leave() |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
git ls-files | grep -E "(membership|serf)" | head -20Repository: Devin-Yeung/proglog
Length of output: 133
🏁 Script executed:
cat -n internal/discovery/membership.go | head -200Repository: Devin-Yeung/proglog
Length of output: 5774
🏁 Script executed:
# Check if there's Serf documentation or usage elsewhere
rg "serf\.(Leave|Shutdown)" --type go -B 2 -A 2Repository: Devin-Yeung/proglog
Length of output: 684
🏁 Script executed:
# Look for how Serf is initialized and cleaned up
rg "serf\.Serf|NewSerf" --type go -B 2 -A 5Repository: Devin-Yeung/proglog
Length of output: 484
🏁 Script executed:
# Check if there are tests that show expected behavior
find . -name "*membership*test*" -type fRepository: Devin-Yeung/proglog
Length of output: 102
🏁 Script executed:
cat -n internal/discovery/membership_test.goRepository: Devin-Yeung/proglog
Length of output: 3301
🏁 Script executed:
# Check if there's any cleanup or close method
rg "Close|Shutdown" internal/discovery/membership.goRepository: Devin-Yeung/proglog
Length of output: 45
🏁 Script executed:
# Look for Serf package documentation or comments about shutdown
rg "Leave|Shutdown" --type go | grep -i "serf\|event\|goroutine" | head -20Repository: Devin-Yeung/proglog
Length of output: 438
🏁 Script executed:
# Check how Membership is used elsewhere in the codebase
rg "\.Leave\(\)" --type go -B 3 -A 3Repository: Devin-Yeung/proglog
Length of output: 1039
🏁 Script executed:
# Look at how the membership object is managed in main or initialization
rg "New.*Membership|membership\.New" --type go -B 2 -A 2Repository: Devin-Yeung/proglog
Length of output: 348
🌐 Web query:
Serf Leave() vs Shutdown() Hashicorp goroutine cleanup
💡 Result:
In github.com/hashicorp/serf/serf, Leave() and Shutdown() do different things:
(*Serf).Leave(): graceful cluster exit — it broadcasts a “leaving” intent so peers mark the node as left (not failed). It’s safe to call multiple times, and is best-effort if the leave broadcast times out. It is not the “resource cleanup” call. [1](*Serf).Shutdown(): process cleanup / goroutine+network teardown — it “stopp[s] all network activity and background maintenance” (i.e., the stuff that keeps goroutines alive). This is not graceful for cluster membership, so it should be preceded byLeave()or peers will treat it as a failure. It’s safe to call multiple times. [1]
So for goroutine cleanup you want Shutdown() (optionally after Leave()). The usual pattern is:
_ = s.Leave() // graceful membership (best effort)
_ = s.Shutdown() // stop background goroutines / networking
<-s.ShutdownCh() // optional: wait until shutdown completesShutdownCh() exists specifically to wait for shutdown completion. [1]
Related behavior in Consul (built on Serf/memberlist): consul leave is described as “a graceful leave and shutdown,” i.e., it aims to make peers see “left” instead of “failed,” reinforcing the same idea: graceful leave is about cluster semantics, while shutdown is about stopping the agent. [2]
Sources:
[1] Serf Go docs for Leave(), Shutdown(), ShutdownCh() (pkg.go.dev) (pkg.go.dev)
[2] Consul leave command docs (HashiCorp Developer) (developer.hashicorp.com)
Citations:
- 1: https://pkg.go.dev/github.com/hashicorp/serf/serf
- 2: https://developer.hashicorp.com/consul/commands/leave?utm_source=openai
🏁 Script executed:
# Check if there are any imports or documentation about Serf behavior
rg "github.com/hashicorp/serf" --type go -A 3 -B 1Repository: Devin-Yeung/proglog
Length of output: 284
🏁 Script executed:
# Look for any cleanup patterns or defer statements in the codebase
rg "defer.*Close|defer.*Shutdown" --type go -B 2 -A 1Repository: Devin-Yeung/proglog
Length of output: 892
🏁 Script executed:
# Check the actual event channel closing to understand cleanup
rg "close.*events|events.*close" --type go -B 2 -A 2Repository: Devin-Yeung/proglog
Length of output: 45
🏁 Script executed:
# Search for any documentation or comments about goroutine cleanup
rg "goroutine|eventHandler" --type go -B 2 -A 5Repository: Devin-Yeung/proglog
Length of output: 1007
Call Shutdown() after Leave() to properly cleanup Serf resources.
Calling only Leave() keeps the Serf instance running with its event goroutine and sockets alive. According to Serf's design, Leave() is for graceful cluster membership exit (best-effort), while Shutdown() is required to stop background goroutines and network activity. The standard pattern is to call Leave() first, then Shutdown().
Suggested fix
func (m *Membership) Leave() error {
- return m.serf.Leave()
+ if err := m.serf.Leave(); err != nil {
+ return err
+ }
+ return m.serf.Shutdown()
}📝 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.
| // Leave makes the node leave the Serf cluster gracefully. | |
| func (m *Membership) Leave() error { | |
| return m.serf.Leave() | |
| // Leave makes the node leave the Serf cluster gracefully. | |
| func (m *Membership) Leave() error { | |
| if err := m.serf.Leave(); err != nil { | |
| return err | |
| } | |
| return m.serf.Shutdown() | |
| } |
🤖 Prompt for AI Agents
In `@internal/discovery/membership.go` around lines 163 - 165, The Leave method
currently only calls m.serf.Leave(), leaving Serf goroutines/sockets running;
update Membership.Leave to call m.serf.Leave() first and then m.serf.Shutdown()
to fully clean up Serf resources. Ensure you capture and return errors from both
calls (e.g., if Leave() returns an error return it, otherwise return any
Shutdown() error) so failures from either m.serf.Leave() or m.serf.Shutdown()
are propagated by Membership.Leave.
2 participants
Summary by CodeRabbit
New Features
Tests