some RPC client/server cleanup #2709
Conversation
sawka
commented
Dec 22, 2025
sawka
commented
- announce and unannounce should propagate
- route the sendmessage command
- add timeouts for sending to OutputCh (appropriately)
- add ctx for SendCancel function
WalkthroughThe diff makes cancellation context-aware and non-blocking across the RPC/WebSocket stack. Several cancellation callbacks (client RpcOpts.StreamCancelFn and web handler streamCancelFn) now accept a context and return an error; the web handler invokes cancellation with a 5-second timeout context. Routing and announce/unannounce handling were adjusted (including a changed handleUnannounceMessage signature) and message send paths were hardened to avoid blocking by using select/timeouts and guards for empty request IDs. Multiple send/cancel flows were updated to return errors on timeout. Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Repository UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
⏰ 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). (3)
🔇 Additional comments (5)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
pkg/web/web.gopkg/wshrpc/wshclient/wshclientutil.gopkg/wshutil/wshadapter.gopkg/wshutil/wshrouter.gopkg/wshutil/wshrpc.go
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-01-22T22:27:25.739Z
Learnt from: esimkowitz
Repo: wavetermdev/waveterm PR: 1790
File: pkg/remote/connparse/connparse.go:76-82
Timestamp: 2025-01-22T22:27:25.739Z
Learning: The GetRpcContext() method in wshutil package is guaranteed to never return nil due to type constraints, making nil checks unnecessary.
Applied to files:
pkg/wshrpc/wshclient/wshclientutil.gopkg/wshutil/wshrpc.go
📚 Learning: 2025-01-22T01:28:41.417Z
Learnt from: esimkowitz
Repo: wavetermdev/waveterm PR: 1790
File: pkg/remote/fileshare/wshfs/wshfs.go:122-122
Timestamp: 2025-01-22T01:28:41.417Z
Learning: The RpcClient in pkg/remote/fileshare/wshfs/wshfs.go is initialized and handled downstream by either main-server or wshcmd-connserver, as documented in the package comment.
Applied to files:
pkg/wshrpc/wshclient/wshclientutil.gopkg/web/web.go
📚 Learning: 2025-01-29T04:21:11.649Z
Learnt from: esimkowitz
Repo: wavetermdev/waveterm PR: 1725
File: pkg/remote/fileshare/wavefs/wavefs.go:441-494
Timestamp: 2025-01-29T04:21:11.649Z
Learning: The `CopyRemote` function in WaveTerm's file operations has proper error handling that prevents partial writes by failing fast and using context cancellation. Each step (path cleaning, file operations, tar reading/writing) is guarded by error checks that prevent proceeding with writes on error.
Applied to files:
pkg/web/web.go
🧬 Code graph analysis (4)
pkg/wshutil/wshadapter.go (1)
pkg/wshrpc/wshrpctypes.go (1)
Command_Message(60-60)
pkg/web/web.go (1)
pkg/wshrpc/wshrpctypes.go (2)
RespOrErrorUnion(193-196)FileData(508-513)
pkg/wshutil/wshrpc.go (1)
pkg/panichandler/panichandler.go (1)
PanicHandler(25-43)
pkg/wshutil/wshrouter.go (1)
pkg/wshrpc/wshrpctypes.go (1)
Command_RouteUnannounce(59-59)
🪛 golangci-lint (2.5.0)
pkg/wshrpc/wshclient/wshclientutil.go
[error] 66-66: : # github.com/wavetermdev/waveterm/pkg/wshrpc/wshclient
pkg/wshrpc/wshclient/wshclientutil.go:66:24: cannot use func(ctx context.Context) error {…} (value of type func(ctx context.Context) error) as func() value in assignment
(typecheck)
⏰ 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). (3)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
- GitHub Check: Build for TestDriver.ai
🔇 Additional comments (13)
pkg/wshutil/wshadapter.go (1)
98-105: LGTM! Prevents message loops for unimplemented message command.This correctly suppresses the out-of-band debug message when
Command_Messagehandler is missing, avoiding potential infinite loops while still returning the proper error response.pkg/web/web.go (2)
258-258: LGTM! Context-aware stream cancellation signature.The updated signature aligns with the broader context-aware cancellation pattern introduced in this PR.
272-278: Context-aware cancellation with appropriate timeout.Using
context.Background()for the cancellation timeout is correct since the request context is already done at this point. The 5-second timeout provides a reasonable window for the cancel operation to complete.pkg/wshutil/wshrouter.go (3)
171-181: LGTM! Conditional announcement routing with upstream propagation.The logic correctly avoids self-registration when source matches the local route, and properly forwards announcements upstream.
183-192: LGTM! Unannounce messages now propagated upstream.This addresses the PR objective to ensure unannounce messages are forwarded upstream, with proper lock release before the upstream send.
207-222: Improved routing hierarchy with announced route fallback.The cascading lookup (direct route → announced local route → upstream) provides robust message routing with clear fallback behavior.
pkg/wshutil/wshrpc.go (7)
316-318: Good guard: prevents registering handlers for empty request IDs.This fixes the bug mentioned in the PR objectives where cleanup/close could fail when reqId is empty.
507-525: Context-aware cancellation with proper cleanup.The updated
SendCancelcorrectly uses select for non-blocking send, respects context cancellation, and ensures finalization in both success and timeout paths.
611-625: Message routing to source with non-blocking send.The
Routefield correctly targets the source, and the select prevents blocking if the output channel is full or context is cancelled.
627-656: Non-blocking response sending with proper guards.The empty
reqIdguard correctly handles no-response requests, and the select-based send with timeout error propagation prevents deadlocks while informing callers of failures.
658-679: Consistent non-blocking error response handling.The guards and select-based send follow the same pattern as
SendResponse, ensuring error responses don't block indefinitely.
695-705: Finalize ensures completion with proper cleanup.Calling
SendResponse(nil, true)when not already done ensures proper protocol completion, with the unregister happening regardless.
751-758: Non-blocking request initiation with timeout handling.The select-based send prevents blocking on a full output channel, with proper finalization and error return on timeout.
| opts.StreamCancelFn = func(ctx context.Context) error { | ||
| return reqHandler.SendCancel(ctx) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find RpcOpts type definition and StreamCancelFn signature
rg -n "type RpcOpts|StreamCancelFn" --type=go -A 5 -B 2Repository: wavetermdev/waveterm
Length of output: 2005
Type mismatch: StreamCancelFn signature incompatible with RpcOpts field definition.
The code assigns func(ctx context.Context) error to opts.StreamCancelFn, but the field in RpcOpts (pkg/wshrpc/wshrpctypes.go:376) is defined as func() with no parameters and no return value. This type mismatch will prevent compilation. The RpcOpts.StreamCancelFn field definition must be updated to func(context.Context) error to match the new signature.
🧰 Tools
🪛 golangci-lint (2.5.0)
[error] 66-66: : # github.com/wavetermdev/waveterm/pkg/wshrpc/wshclient
pkg/wshrpc/wshclient/wshclientutil.go:66:24: cannot use func(ctx context.Context) error {…} (value of type func(ctx context.Context) error) as func() value in assignment
(typecheck)
🤖 Prompt for AI Agents
In pkg/wshrpc/wshclient/wshclientutil.go around lines 66-68, opts.StreamCancelFn
is assigned a func(ctx context.Context) error but RpcOpts.StreamCancelFn is
declared as func() in pkg/wshrpc/wshrpctypes.go (around line 376); update the
RpcOpts.StreamCancelFn field type to func(context.Context) error to match the
new signature, then update any other callers/assignments of StreamCancelFn to
accept and return the context/error signature (or wrap them where appropriate),
and run go build/go test to ensure all usages compile.
