feat: simplify keepalive protocol interface

[cryptodj413]

ref: #108

---

Summary by cubic

Simplified the keepalive protocol interface by replacing message-handling callbacks with optional notification hooks and making the server auto-reply. Added Start/Stop methods for better lifecycle control and safe timer cancellation.

• Refactors

  • Removed KeepAliveFunc, KeepAliveResponseFunc, and DoneFunc; added OnKeepAliveReceived and OnKeepAliveResponseReceived (connectionId, cookie).
  • Server now always sends keep-alive responses; handleDone only logs.
  • NewServer uses a default config when nil is passed.

• New Features

  • Start/Stop added to KeepAlive, Client, and Server; client Stop cancels pending timers, server Stop halts the protocol.

^{Written for commit 162c37c. Summary will update on new commits.}

Summary by CodeRabbit

• New Features
  • Added Stop method to gracefully terminate the keep-alive protocol
  • Improved event handling mechanism with cleaner and more efficient callback signatures
  • Enhanced protocol lifecycle management with explicit start and stop capabilities

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

📝 Walkthrough

Walkthrough

The changes refactor the keep-alive protocol package's public API by replacing explicit function type fields in Config (KeepAliveFunc, KeepAliveResponseFunc, DoneFunc) with callback fields (OnKeepAliveReceived, OnKeepAliveResponseReceived). Callbacks are modified to receive ConnectionId and cookie values instead of CallbackContext. CallbackContext is expanded to include ConnectionId, Client, and Server fields. The Client and Server types gain Stop() methods, and the KeepAlive struct gains Start() and Stop() methods. Configuration builder functions are renamed accordingly, and Server.handleDone() signature changes from returning error to void.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Pre-merge checks and finishing touches

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'feat: simplify keepalive protocol interface' accurately summarizes the main changes: the PR refactors the keepalive protocol by replacing explicit callback function types with simplified callback fields and renaming options, directly addressing the simplification objective.

✨ Finishing touches

• 📝 Generate docstrings

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[cubic-dev-ai]

No issues found across 3 files

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

protocol/keepalive/keepalive.go (1)

115-123: Consider adding a Start() method to Server for symmetry.

Client.Start() uses sync.Once for idempotent startup, but Server lacks a custom Start() method—calling k.Server.Start() invokes the embedded Protocol.Start() directly. If KeepAlive.Start() is called multiple times, the client is protected but the server may not be. Consider adding a Server.Start() method with similar sync.Once protection for consistent behavior.

#!/bin/bash
# Check if Protocol.Start() has built-in idempotency protection
ast-grep --pattern $'func ($_ *Protocol) Start() {
  $$$
}'

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 7f3be1a and 162c37c.

📒 Files selected for processing (3)

• protocol/keepalive/client.go
• protocol/keepalive/keepalive.go
• protocol/keepalive/server.go

🧰 Additional context used

🧬 Code graph analysis (3)

protocol/keepalive/server.go (3)

protocol/keepalive/keepalive.go (1)

• NewConfig (139-149)

protocol/keepalive/messages.go (1)

• NewMsgKeepAliveResponse (78-86)

protocol/handshake/server.go (1)

• Server (26-30)

protocol/keepalive/keepalive.go (4)

connection.go (1)

• ConnectionId (56-56)

connection/id.go (1)

• ConnectionId (22-25)

protocol/keepalive/client.go (1)

• Client (26-33)

protocol/keepalive/server.go (1)

• Server (24-28)

protocol/keepalive/client.go (3)

protocol/handshake/client.go (1)

• Client (26-31)

connection.go (1)

• ConnectionId (56-56)

connection/id.go (1)

• ConnectionId (22-25)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)

• GitHub Check: cubic · AI code reviewer

🔇 Additional comments (10)

protocol/keepalive/client.go (2)

118-127: LGTM! Clean implementation of Stop().

The method properly acquires the mutex before stopping and nilling the timer, then releases before calling Protocol.Stop(). This avoids holding the lock during potentially blocking operations.

---

163-167: LGTM! Callback invocation is correct.

The simplified callback signature (ConnectionId, uint16) aligns with the new interface design. The nil check on c.config is defensive since NewClient guarantees a non-nil config, but it's harmless.

protocol/keepalive/server.go (4)

66-67: Verify that ignoring the handleDone result is intentional.

The previous implementation likely returned an error from handleDone(). Now handleDone() returns nothing and its call result is ignored while other message types still return errors via err. Confirm this is the intended behavior—if handleDone could fail (e.g., during cleanup), any error would now be silently ignored.

---

88-97: Good simplification—server now always responds.

The callback is now purely for notification, and the keep-alive response is always sent automatically. This is a cleaner design that removes the burden of response handling from callback implementers. Users who previously relied on KeepAliveFunc to conditionally suppress responses will need to adapt.

---

32-35: LGTM! Consistent nil-config handling.

Mirrors the same pattern used in NewClient, ensuring both constructors safely default when no config is provided.

---

110-113: LGTM!

Clean delegation to the underlying protocol. Unlike the client, the server has no timer to clean up.

protocol/keepalive/keepalive.go (4)

90-97: LGTM! Cleaner callback interface.

The new signatures func(connection.ConnectionId, uint16) are simpler and provide exactly the information callers typically need. Removing CallbackContext from the callback signature reduces coupling while still providing the ConnectionId and cookie value.

---

125-133: LGTM! Clean shutdown logic.

The nil checks prevent nil pointer dereferences. Client.Stop() properly cleans up its timer. Ensure Protocol.Stop() (called by both) handles being invoked multiple times gracefully.

---

151-163: LGTM! Clear option function naming.

The WithOnKeepAliveReceived and WithOnKeepAliveResponseReceived names clearly indicate they set notification callbacks, aligning well with the simplified interface design.

---

99-104: LGTM!

CallbackContext is retained for internal use (logging, state tracking) while callbacks receive only the necessary ConnectionId and cookie values. This is a clean separation of concerns.