very large refactor of wshrouter #2732
Conversation
…d router stuff (inject command), fix rpccontext, and connserver routing
WalkthroughThis pull request implements a significant refactoring of the routing, RPC, and authentication systems. Key changes include: reworking the router from route-based to link-based architecture with trust levels and control plane support; introducing a new JWT-based authentication system via the Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
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: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
pkg/util/shellutil/shellquote.go (1)
24-51: Newline handling was intentionally removed fromHardQuote— verify this behavior is correct.The most recent commit removed explicit newline escaping from
HardQuote(previously:case '\n': buf.append(buf, '\\', '\n')). This creates a functional difference:
HardQuote: Does NOT escape newlines (literal\nin double quotes)HardQuotePowerShell: Escapes newlines as`nHardQuoteFish: Does NOT escape newlines (documented in code comment)Since
HardQuoteis used withsh -cto execute commands (seegenconn.go:154,conncontroller.go:315), literal newlines in the quoted string could affect command parsing. While bash accepts literal newlines inside double quotes, this differs from PowerShell and may complicate debugging.No test cases cover newline handling for any quoting function. Please confirm whether this removal was intentional and ensure it doesn't break command construction when environment variables or other inputs unexpectedly contain newlines.
frontend/app/store/wps.ts (1)
43-58: Missing null check forWpsRpcClientbefore RPC calls.The
updateWaveEventSubfunction usesWpsRpcClientdirectly in RPC calls, but there's no guard against it beingundefined. IfupdateWaveEventSubis called beforesetWpsRpcClientinitializes the client, this will throw a runtime error.🔎 Proposed fix
function updateWaveEventSub(eventType: string) { + if (WpsRpcClient == null) { + console.warn("WpsRpcClient not initialized, skipping event subscription update"); + return; + } const subjects = waveEventSubjects.get(eventType); if (subjects == null) { RpcApi.EventUnsubCommand(WpsRpcClient, eventType, { noresponse: true }); return; }pkg/wshutil/wshproxy.go (1)
43-52: TheingressLinkIdparameter cannot be used in this implementation.Unlike the parallel
WshRpc.SendRpcMessagewhich uses this parameter to constructRpcInputChType,WshRpcProxysends only raw bytes toToRemoteCh. Since the channel type ischan []byte, theingressLinkIdis dropped and lost. EitherWshRpcProxyneeds a different channel type that can carry link ID information, or this parameter should not be part of the interface contract if it cannot be used by all implementations.
♻️ Duplicate comments (3)
cmd/wsh/cmd/wshcmd-view.go (1)
56-59: Validation pattern matches other commands.This follows the same TabId retrieval and validation pattern as other commands in this PR. See the refactoring suggestion in
wshcmd-launch.gofor a way to consolidate this logic.cmd/wsh/cmd/wshcmd-web.go (1)
115-118: Validation pattern matches other commands.This follows the same TabId retrieval and validation pattern as other commands in this PR. See the refactoring suggestion in
wshcmd-launch.gofor a way to consolidate this logic.cmd/wsh/cmd/wshcmd-run.go (1)
136-139: Validation pattern matches other commands.This follows the same TabId retrieval and validation pattern as other commands in this PR. See the refactoring suggestion in
wshcmd-launch.gofor a way to consolidate this logic.
🧹 Nitpick comments (11)
pkg/util/utilfn/utilfn.go (1)
1019-1023: Good refactoring to parameterize output destination.The change from hardcoded
os.Stdoutto anio.Writerparameter improves flexibility and testability.Consider returning the write error to allow callers to handle write failures (e.g., disk full scenarios):
🔎 Proposed enhancement
-func DumpGoRoutineStacks(w io.Writer) { +func DumpGoRoutineStacks(w io.Writer) error { buf := make([]byte, 1<<20) n := runtime.Stack(buf, true) - w.Write(buf[:n]) + _, err := w.Write(buf[:n]) + return err }Taskfile.yml (1)
598-603: LGTM! Consider completing the migration for consistency.The platform-specific tsunami binary cleanup commands are correct. However, note that this PR creates a mixed approach: some tasks now use explicit platform-specific commands while others still use the
RMRFtemplate variable (defined at line 11 and used in tasks likeclean,dev:clearconfig,artifacts:download, etc.).Consider either:
- Completing the migration by converting remaining RMRF usages to explicit commands
- Documenting why certain cleanup operations warrant explicit commands vs. template-based approach
frontend/app/store/wshrouter.ts (1)
149-156: Optional chaining inconsistency.Line 155 uses optional chaining (
this.upstreamClient?.recvRpcMessage), while lines 53 and 142 call the same method without it. Since the constructor guaranteesupstreamClientis non-null (lines 39-41), the optional chaining is unnecessary.🔎 Suggested fix for consistency
- this.upstreamClient?.recvRpcMessage(unannounceMsg); + this.upstreamClient.recvRpcMessage(unannounceMsg);pkg/wshutil/wshrouter_controlimpl.go (1)
105-168: Consider extracting common token validation logic.
AuthenticateTokenCommand(root branch, lines 121-142) andAuthenticateTokenVerifyCommand(lines 170-202) share nearly identical logic for retrieving the token swap entry, validating the RPC context, and building the return data.🔎 Suggested refactor to reduce duplication
Extract common logic into a helper:
func (impl *WshRouterControlImpl) validateAndConsumeTokenSwap(token string) (*wshrpc.CommandAuthenticateRtnData, *wshrpc.RpcContext, error) { entry := shellutil.GetAndRemoveTokenSwapEntry(token) if entry == nil { return nil, nil, fmt.Errorf("no token entry found") } _, err := validateRpcContextFromAuth(entry.RpcContext) if err != nil { return nil, nil, err } if entry.RpcContext.IsRouter { return nil, nil, fmt.Errorf("cannot auth router via token") } if entry.RpcContext.RouteId == "" { return nil, nil, fmt.Errorf("no routeid") } rtnData := wshrpc.CommandAuthenticateRtnData{ Env: entry.Env, InitScriptText: entry.ScriptText, RpcContext: entry.RpcContext, } return &rtnData, entry.RpcContext, nil }Then both methods can call this helper.
Also applies to: 170-202
cmd/wsh/cmd/wshcmd-secret.go (1)
179-185: LGTM! TabId requirement properly enforced.The addition of WAVETERM_TABID validation and propagation to the block creation data is correct and consistent with the broader PR changes.
Optional: Consider a helper to reduce duplication
The pattern of retrieving and validating
WAVETERM_TABIDappears across multiple command files. If not already present, consider a helper function:func requireTabIdFromEnv() (string, error) { tabId := getTabIdFromEnv() if tabId == "" { return "", fmt.Errorf("no WAVETERM_TABID env var set") } return tabId, nil }This would consolidate the validation logic in one place.
cmd/wsh/cmd/wshcmd-launch.go (1)
51-54: Consider consolidating validation into the helper function.The pattern of retrieving and validating
WAVETERM_TABIDis repeated across multiple command files (launch, view, web, run). WhilegetTabIdFromEnv()centralizes retrieval, the validation logic remains duplicated. Consider having the helper return an error when the tabId is empty, reducing repetition.Example consolidated helper
In
wshcmd-root.go, refactor the helper to:func getValidTabIdFromEnv() (string, error) { tabId := os.Getenv("WAVETERM_TABID") if tabId == "" { return "", fmt.Errorf("WAVETERM_TABID environment variable not set") } return tabId, nil }Then simplify call sites to:
- tabId := getTabIdFromEnv() - if tabId == "" { - return fmt.Errorf("no WAVETERM_TABID env var set") - } + tabId, err := getValidTabIdFromEnv() + if err != nil { + return err + }pkg/wcore/wcore.go (1)
216-225: Consider security implications of logging the public key.The public key is logged to stdout in PEM format. While public keys are meant to be shared, logging them to stdout may expose them in production logs where they could be collected by log aggregation systems. Consider:
- Whether this logging is only needed during development
- Using debug-level logging instead of standard logging
- Adding a flag to control key logging behavior
pkg/web/ws.go (1)
291-295: Validate stableId format/length to prevent abuse.While authentication is performed, the
stableIdis used directly in logging and as a map key. Consider adding basic validation (e.g., max length, allowed characters) to prevent potential log injection or memory issues from excessively long IDs.pkg/wavejwt/wavejwt.go (2)
63-67: Consider returning a copy to prevent mutation of internal state.
GetPublicKey()returns the internal slice directly. While Ed25519 public keys are typically treated as immutable, returning a copy would be more defensive.🔎 Proposed fix
func GetPublicKey() []byte { globalLock.Lock() defer globalLock.Unlock() + if publicKey == nil { + return nil + } + rtn := make([]byte, len(publicKey)) + copy(rtn, publicKey) - return publicKey + return rtn }
114-131: Sign mutates the input claims struct.The function modifies
claims.IssuedAt,claims.Issuer, andclaims.ExpiresAtin place if they're nil/empty. This side effect could be surprising to callers. Consider documenting this behavior or working on a copy.pkg/wshutil/wshrouter.go (1)
519-529: Panic for programming error is acceptable but consider error return.Using
panicinRegisterUpstreamfor root router is reasonable for catching programming errors early. However, if this could be called dynamically based on configuration, an error return might be safer.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (63)
.gitignoreTaskfile.ymlcmd/server/main-server.gocmd/wsh/cmd/wshcmd-connserver.gocmd/wsh/cmd/wshcmd-createblock.gocmd/wsh/cmd/wshcmd-debug.gocmd/wsh/cmd/wshcmd-editconfig.gocmd/wsh/cmd/wshcmd-editor.gocmd/wsh/cmd/wshcmd-launch.gocmd/wsh/cmd/wshcmd-root.gocmd/wsh/cmd/wshcmd-run.gocmd/wsh/cmd/wshcmd-secret.gocmd/wsh/cmd/wshcmd-ssh.gocmd/wsh/cmd/wshcmd-term.gocmd/wsh/cmd/wshcmd-view.gocmd/wsh/cmd/wshcmd-wavepath.gocmd/wsh/cmd/wshcmd-web.godb/migrations-wstore/000009_mainserver.down.sqldb/migrations-wstore/000009_mainserver.up.sqlemain/emain-menu.tsemain/emain.tsfrontend/app/store/wps.tsfrontend/app/store/ws.tsfrontend/app/store/wshclientapi.tsfrontend/app/store/wshrouter.tsfrontend/app/store/wshrpcutil-base.tsfrontend/app/store/wshrpcutil.tsfrontend/types/gotypes.d.tspkg/baseds/baseds.gopkg/blockcontroller/shellcontroller.gopkg/remote/conncontroller/conncontroller.gopkg/shellexec/shellexec.gopkg/tsgen/tsgen.gopkg/util/packetparser/packetparser.gopkg/util/shellutil/shellquote.gopkg/util/shellutil/tokenswap.gopkg/util/sigutil/sigusr1_notwindows.gopkg/util/utilfn/utilfn.gopkg/waveapp/waveapp.gopkg/wavebase/wavebase.gopkg/wavejwt/wavejwt.gopkg/waveobj/wtype.gopkg/wcloud/wcloud.gopkg/wcore/wcore.gopkg/web/ws.gopkg/wshrpc/wshclient/barerpcclient.gopkg/wshrpc/wshclient/wshclient.gopkg/wshrpc/wshrpctypes.gopkg/wshrpc/wshserver/wshserver.gopkg/wshrpc/wshserver/wshserverutil.gopkg/wshutil/wshadapter.gopkg/wshutil/wshcmdreader.gopkg/wshutil/wshmultiproxy.gopkg/wshutil/wshproxy.gopkg/wshutil/wshrouter.gopkg/wshutil/wshrouter_controlimpl.gopkg/wshutil/wshrpc.gopkg/wshutil/wshrpcio.gopkg/wshutil/wshutil.gopkg/wsl/wsl-unix.gopkg/wsl/wsl-win.gopkg/wslconn/wslconn.gopkg/wstore/wstore_dbops.go
💤 Files with no reviewable changes (5)
- pkg/wavebase/wavebase.go
- pkg/wstore/wstore_dbops.go
- pkg/util/shellutil/tokenswap.go
- pkg/wshutil/wshmultiproxy.go
- cmd/wsh/cmd/wshcmd-debug.go
🧰 Additional context used
🧠 Learnings (4)
📚 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:
cmd/wsh/cmd/wshcmd-view.gofrontend/app/store/wshrpcutil-base.tspkg/wshrpc/wshclient/wshclient.gopkg/wslconn/wslconn.gopkg/wshrpc/wshserver/wshserverutil.gopkg/wshrpc/wshclient/barerpcclient.gopkg/waveapp/waveapp.gopkg/wshutil/wshadapter.gopkg/remote/conncontroller/conncontroller.gocmd/wsh/cmd/wshcmd-root.gopkg/wshutil/wshrouter_controlimpl.gopkg/web/ws.gofrontend/app/store/wshrpcutil.tsfrontend/app/store/wps.tspkg/blockcontroller/shellcontroller.gopkg/wshutil/wshutil.gocmd/server/main-server.gopkg/wshutil/wshproxy.gopkg/wshrpc/wshrpctypes.gopkg/wshutil/wshrpc.gocmd/wsh/cmd/wshcmd-connserver.gopkg/wshutil/wshrouter.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:
cmd/wsh/cmd/wshcmd-wavepath.go
📚 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/wshserver/wshserverutil.gopkg/wshrpc/wshclient/barerpcclient.gopkg/waveapp/waveapp.gopkg/wshutil/wshadapter.gocmd/wsh/cmd/wshcmd-root.gopkg/wshutil/wshrouter_controlimpl.gopkg/blockcontroller/shellcontroller.gofrontend/types/gotypes.d.tspkg/wshutil/wshutil.gocmd/server/main-server.gopkg/wshutil/wshproxy.gopkg/wshutil/wshrpc.gocmd/wsh/cmd/wshcmd-connserver.go
📚 Learning: 2025-10-21T05:09:26.916Z
Learnt from: sawka
Repo: wavetermdev/waveterm PR: 2465
File: frontend/app/onboarding/onboarding-upgrade.tsx:13-21
Timestamp: 2025-10-21T05:09:26.916Z
Learning: In the waveterm codebase, clientData is loaded and awaited in wave.ts before React runs, ensuring it is always available when components mount. This means atoms.client will have data on first render.
Applied to files:
frontend/app/store/wps.ts
🧬 Code graph analysis (30)
pkg/util/sigutil/sigusr1_notwindows.go (1)
pkg/util/utilfn/utilfn.go (1)
DumpGoRoutineStacks(1019-1023)
cmd/wsh/cmd/wshcmd-view.go (1)
pkg/wshrpc/wshrpctypes.go (1)
CommandCreateBlockData(429-438)
pkg/waveobj/wtype.go (1)
pkg/waveobj/metamap.go (1)
MetaMapType(8-8)
frontend/app/store/wshrpcutil-base.ts (1)
frontend/app/store/wps.ts (1)
setWpsRpcClient(151-151)
cmd/wsh/cmd/wshcmd-run.go (1)
pkg/wshrpc/wshrpctypes.go (1)
CommandCreateBlockData(429-438)
cmd/wsh/cmd/wshcmd-createblock.go (1)
pkg/wshrpc/wshrpctypes.go (1)
CommandCreateBlockData(429-438)
pkg/wshutil/wshcmdreader.go (1)
pkg/baseds/baseds.go (1)
RpcInputChType(11-14)
frontend/app/store/wshclientapi.ts (3)
frontend/app/store/wshclient.ts (1)
WshClient(159-159)pkg/remote/fileshare/wshfs/wshfs.go (1)
WshClient(22-22)pkg/wshrpc/wshrpctypes.go (3)
CommandAuthenticateTokenData(398-400)RpcOpts(375-381)CommandAuthenticateRtnData(391-396)
pkg/wslconn/wslconn.go (4)
pkg/wavebase/wavebase.go (1)
IsDevMode(112-114)pkg/remote/conncontroller/conncontroller.go (1)
ConnServerCmdTemplate(74-78)pkg/util/shellutil/shellquote.go (1)
HardQuote(24-51)pkg/wshutil/wshrouter.go (1)
MakeConnectionRouteId(93-95)
cmd/wsh/cmd/wshcmd-editor.go (1)
pkg/wshrpc/wshrpctypes.go (1)
CommandCreateBlockData(429-438)
pkg/wshrpc/wshserver/wshserverutil.go (5)
pkg/wshutil/wshrpc.go (1)
MakeWshRpc(233-235)pkg/wshrpc/wshrpctypes.go (1)
RpcContext(383-389)cmd/wsh/cmd/wshcmd-root.go (1)
RpcContext(33-33)pkg/wshrpc/wshclient/barerpcclient.go (1)
WshServerImpl(18-18)pkg/wshrpc/wshserver/wshserver.go (1)
WshServerImpl(71-71)
pkg/util/packetparser/packetparser.go (1)
pkg/baseds/baseds.go (1)
RpcInputChType(11-14)
pkg/wshrpc/wshclient/barerpcclient.go (3)
pkg/wshrpc/wshrpctypes.go (1)
RpcContext(383-389)cmd/wsh/cmd/wshcmd-root.go (1)
RpcContext(33-33)pkg/wshutil/wshrouter.go (1)
DefaultRouter(121-121)
cmd/wsh/cmd/wshcmd-web.go (1)
pkg/wshrpc/wshrpctypes.go (1)
CommandCreateBlockData(429-438)
pkg/waveapp/waveapp.go (5)
frontend/app/store/wshclientapi.ts (1)
AuthenticateCommand(21-23)pkg/wshrpc/wshclient/wshclient.go (1)
AuthenticateCommand(33-36)cmd/wsh/cmd/wshcmd-root.go (1)
RpcClient(32-32)pkg/wshrpc/wshrpctypes.go (1)
RpcOpts(375-381)pkg/wshutil/wshrouter.go (1)
ControlRoute(26-26)
cmd/wsh/cmd/wshcmd-root.go (3)
pkg/wshrpc/wshrpctypes.go (4)
RpcContext(383-389)CommandAuthenticateTokenData(398-400)RpcOpts(375-381)CommandResolveIdsData(420-423)pkg/wshutil/wshutil.go (1)
SetupDomainSocketRpcClient(276-298)pkg/wshrpc/wshclient/wshclient.go (2)
SetPeerInfoCommand(662-665)ResolveIdsCommand(614-617)
emain/emain-menu.ts (1)
frontend/app/store/wps.ts (1)
waveEventSubscribe(152-152)
cmd/wsh/cmd/wshcmd-secret.go (1)
pkg/wshrpc/wshrpctypes.go (1)
CommandCreateBlockData(429-438)
pkg/wshutil/wshrouter_controlimpl.go (8)
pkg/wshutil/wshrouter.go (4)
WshRouter(75-91)LinkKind_Router(50-50)LinkKind_Leaf(49-49)ControlRootRoute(27-27)pkg/wshutil/wshrpc.go (3)
GetRpcSourceFromContext(83-89)GetRpcResponseHandlerFromContext(99-105)GetWshRpcFromContext(75-81)pkg/baseds/baseds.go (1)
NoLinkId(9-9)pkg/wshutil/wshproxy.go (1)
WshRpcProxy(15-21)pkg/wshutil/wshutil.go (1)
ValidateAndExtractRpcContextFromToken(329-335)cmd/wsh/cmd/wshcmd-root.go (1)
RpcContext(33-33)pkg/util/shellutil/tokenswap.go (1)
GetAndRemoveTokenSwapEntry(88-97)pkg/util/utilfn/marshal.go (1)
ReUnmarshal(36-42)
pkg/web/ws.go (4)
pkg/baseds/baseds.go (3)
LinkId(7-7)RpcInputChType(11-14)NoLinkId(9-9)pkg/wshutil/wshproxy.go (2)
WshRpcProxy(15-21)MakeRpcProxy(23-30)pkg/wshutil/wshrouter.go (1)
DefaultRouter(121-121)pkg/eventbus/eventbus.go (2)
RegisterWSChannel(36-43)UnregisterWSChannel(45-49)
frontend/app/store/wshrpcutil.ts (1)
frontend/app/store/wps.ts (1)
setWpsRpcClient(151-151)
frontend/app/store/wps.ts (3)
frontend/app/store/wshclient.ts (1)
WshClient(159-159)frontend/app/store/wshclientapi.ts (1)
RpcApi(707-707)pkg/wps/wpstypes.go (1)
SubscriptionRequest(39-43)
pkg/blockcontroller/shellcontroller.go (4)
pkg/wshrpc/wshrpctypes.go (1)
RpcContext(383-389)pkg/wshutil/wshrouter.go (2)
MakeRandomProcRouteId(105-107)MakeControllerRouteId(97-99)pkg/wshutil/wshutil.go (1)
MakeClientJWTToken(300-317)pkg/wshutil/wshproxy.go (1)
MakeRpcProxy(23-30)
frontend/types/gotypes.d.ts (2)
pkg/wshrpc/wshrpctypes.go (1)
RpcContext(383-389)cmd/wsh/cmd/wshcmd-root.go (1)
RpcContext(33-33)
pkg/wshrpc/wshserver/wshserver.go (5)
frontend/app/store/wshclientapi.ts (2)
GetJwtPublicKeyCommand(316-318)CreateBlockCommand(116-118)pkg/wshrpc/wshclient/wshclient.go (2)
GetJwtPublicKeyCommand(384-387)CreateBlockCommand(147-150)pkg/wavejwt/wavejwt.go (1)
GetPublicKeyBase64(69-75)pkg/wshrpc/wshrpctypes.go (1)
CommandCreateBlockData(429-438)pkg/waveobj/metaconsts.go (2)
MetaKey_View(9-9)MetaKey_File(13-13)
cmd/server/main-server.go (4)
pkg/wshutil/wshrouter.go (4)
DefaultRouter(121-121)DefaultRoute(24-24)MakeConnectionRouteId(93-95)NewWshRouter(123-137)pkg/wshutil/wshrpc.go (2)
MakeWshRpc(233-235)ServerImpl(39-41)cmd/wsh/cmd/wshcmd-root.go (1)
RpcContext(33-33)pkg/wavebase/wavebase.go (2)
WaveVersion(25-25)BuildTime(26-26)
pkg/wshrpc/wshrpctypes.go (5)
frontend/app/store/wshclientapi.ts (6)
AuthenticateTokenVerifyCommand(31-33)DisposeCommand(151-153)RouteAnnounceCommand(521-523)RouteUnannounceCommand(526-528)SetPeerInfoCommand(551-553)GetJwtPublicKeyCommand(316-318)pkg/wshrpc/wshclient/wshclient.go (6)
AuthenticateTokenVerifyCommand(45-48)DisposeCommand(189-192)RouteAnnounceCommand(626-629)RouteUnannounceCommand(632-635)SetPeerInfoCommand(662-665)GetJwtPublicKeyCommand(384-387)cmd/wsh/cmd/wshcmd-root.go (1)
RpcContext(33-33)pkg/waveobj/waveobj.go (1)
ORef(30-34)pkg/waveobj/metamap.go (1)
MetaMapType(8-8)
pkg/wshutil/wshrpc.go (1)
pkg/baseds/baseds.go (2)
LinkId(7-7)RpcInputChType(11-14)
cmd/wsh/cmd/wshcmd-connserver.go (8)
pkg/panichandler/panichandler.go (1)
PanicHandler(25-43)pkg/baseds/baseds.go (2)
NoLinkId(9-9)LinkId(7-7)cmd/wsh/cmd/wshcmd-root.go (1)
RpcContext(33-33)pkg/util/packetparser/packetparser.go (1)
Parse(48-72)frontend/app/store/wshclientapi.ts (1)
GetJwtPublicKeyCommand(316-318)pkg/wshrpc/wshclient/wshclient.go (1)
GetJwtPublicKeyCommand(384-387)pkg/wavejwt/wavejwt.go (1)
SetPublicKey(53-61)pkg/wshrpc/wshremote/sysinfo.go (1)
RunSysInfoLoop(64-72)
pkg/wshutil/wshrouter.go (5)
pkg/baseds/baseds.go (3)
LinkId(7-7)RpcInputChType(11-14)NoLinkId(9-9)pkg/wshrpc/wshrpctypes.go (2)
Command_Message(64-64)CommandMessageData(407-409)pkg/panichandler/panichandler.go (1)
PanicHandler(25-43)pkg/wps/wps.go (1)
Broker(46-50)pkg/wps/wpstypes.go (2)
WaveEvent(27-33)Event_RouteGone(19-19)
🪛 golangci-lint (2.5.0)
cmd/wsh/cmd/wshcmd-connserver.go
[error] 250-250: ineffectual assignment to logWriter
(ineffassign)
pkg/wshutil/wshrouter.go
[error] 415-415: ineffectual assignment to exitReason
(ineffassign)
[error] 425-425: ineffectual assignment to exitReason
(ineffassign)
⏰ 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 (go)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Build for TestDriver.ai
🔇 Additional comments (110)
.gitignore (1)
19-19: LGTM!The addition of
manifests/to the ignore list is appropriate and follows the existing directory exclusion patterns. This aligns with the PR's Windows/WSL manifest handling improvements.pkg/wshutil/wshadapter.go (2)
56-76: LGTM! Clean simplification of function signature.The removal of the
rpcCtxparameter simplifies the function and aligns with the PR's goal of unified RPC context handling. The function now focuses solely on re-marshalling command data, which is its core responsibility.
107-107: Call site correctly matches updated function signature.The call at line 107 properly uses the new 2-parameter signature for
recodeCommandData. Search of the codebase confirms this is the only call site, and it has been correctly updated.pkg/util/sigutil/sigusr1_notwindows.go (1)
17-17: Exported constant added for dump file path.The path
/tmp/waveterm-usr1-dump.logis appropriate for Unix-like systems where/tmpis standard.emain/emain-menu.ts (2)
387-392: LGTM! Good refactoring to control subscription timing.Moving the event subscription from module load time to an explicit initialization function is a solid improvement. This gives better control over when the workspace update listener is registered and integrates cleanly with the startup sequence.
500-500: LGTM!The export statement correctly exposes both functions for use in other modules.
emain/emain.ts (2)
26-26: LGTM!The import correctly adds the new initialization function alongside existing imports.
353-353: LGTM! Correct initialization sequence.The menu event subscriptions are properly initialized after the RPC client setup, ensuring that the workspace update handler can successfully make RPC calls when triggered.
Taskfile.yml (3)
179-180: LGTM! Platform-specific cleanup commands are correct.The explicit platform-specific removal commands for
wavesrv*binaries are syntactically correct and appropriately handle non-existent files. The transition from template-based to explicit commands improves clarity for each platform's build process.Also applies to: 226-227, 239-240
263-268: LGTM! Platform-filtered cleanup approach is well-structured.The use of
platformsfilters to conditionally execute platform-specific removal commands is the idiomatic Task approach and correctly handles the cleanup ofwsh*binaries across all target architectures.
533-538: LGTM! Consistent platform-specific cleanup.The platform-specific removal of
package.jsonbefore runningnpm initfollows the established pattern and correctly handles both Unix-like and Windows platforms.frontend/app/store/wshrouter.ts (2)
11-11: LGTM! Clear constant for control route.The introduction of
ControlRouteNamewith the$prefix clearly identifies this as a reserved control route, consistent with the PR's control-plane routing refactor.
47-54: Consistent control route integration across lifecycle messages.The addition of
route: ControlRouteNameto announce, reannounce, and unannounce messages properly routes control-plane messages through the control route via the routing infrastructure. The backend RpcMessage struct defines the Route field for routing/forwarding, and the RouteAnnounceCommand and RouteUnannounceCommand handlers are correctly implemented to process these messages when they arrive at the control plane.pkg/wsl/wsl-unix.go (1)
65-67: LGTM! Critical bug fix.The method now correctly assigns to
c.Stderrinstead of incorrectly writing toc.Stdout. This fix ensures error output is properly routed to stderr, which is essential for tools that parse stdout and stderr separately.pkg/wsl/wsl-win.go (1)
104-106: LGTM! Cross-platform parity achieved.This fix mirrors the correction in
pkg/wsl/wsl-unix.go, ensuringSetStderrcorrectly assigns toc.c.Stderron Windows. The cross-platform consistency is now established.pkg/wcloud/wcloud.go (1)
143-147: LGTM! Clean observability control.The
verboseparameter allows selective suppression of request logging. The conditional logging on lines 145-147 is well-placed and maintains existing behavior for most callers (passingtrue), while allowingSendDiagnosticPingto suppress logs by passingfalse.pkg/waveobj/wtype.go (1)
31-31: LGTM! Proper wave object type registration.The
MainServertype is correctly registered inValidOTypesandAllWaveObjTypes, following the established pattern for wave object types. TheGetOType()implementation is consistent with other types.Also applies to: 43-43, 310-319
pkg/tsgen/tsgen.go (1)
502-504: LGTM! Appropriate exclusion of sensitive type.Skipping TypeScript generation for
MainServerprevents exposing JWT key fields to the frontend. This is the correct approach sinceMainServercontains sensitive authentication data (JwtPrivateKey,JwtPublicKey) that should remain server-side only.frontend/app/store/ws.ts (4)
21-26: Good fix for the reconnect handler removal.The previous code likely referenced
this.reconnectHandlerswhich doesn't exist on the class. This correctly references the module-levelreconnectHandlersarray.
40-62: Consistent rename from routeId to stableId.The rename is applied consistently across the class property, constructor parameter, and assignment.
77-79: Good URL encoding practice.Using
encodeURIComponenton the stableId ensures special characters are properly escaped in the query string.
222-234: Smart guard to drop stale routing messages when offline.Dropping
routeannounceandrouteunannounceRPC messages when the WebSocket is not open prevents queueing outdated routing information that would be invalid upon reconnection. The guard correctly allows other RPC messages to be queued.frontend/types/gotypes.d.ts (2)
1002-1009: RpcContext type correctly mirrors the Go struct.The TypeScript type aligns with the Go definition in
pkg/wshrpc/wshrpctypes.go:382-388. Therouteidis correctly required while other fields are optional, matching theomitemptyJSON tags in Go.
197-202: CommandAuthenticateRtnData updated to use RpcContext.The transition from separate
routeid/authtokenfields to the unifiedRpcContexttype aligns with the broader authentication refactoring in this PR.pkg/wshutil/wshrouter_controlimpl.go (3)
17-21: Clean struct and interface implementation.The
WshRouterControlImplstruct properly holds a reference to the router and implements theWshServerImplmarker interface.
23-53: RouteAnnounce and RouteUnannounce handlers are well-structured.Both methods follow a consistent pattern: validate source, get response handler, check ingress link, then perform the route operation. Error messages are clear and actionable.
204-218: Validation logic is clear and handles edge cases.The
validateRpcContextFromAuthfunction correctly enforces that routers cannot have a routeId while leaves must have one.pkg/wshutil/wshrpc.go (7)
43-47: AbstractRpcClient interface properly extended.The interface now includes
GetPeerInfo()for debugging and updatesSendRpcMessageto acceptingressLinkIdfor link tracking.
49-62: WshRpc struct correctly updated for RpcInputChType.The
InputChtype change tochan baseds.RpcInputChTypeenables ingress link tracking throughout the RPC flow.
120-132: RpcMessage simplified by removing AuthToken.The removal of
AuthTokenfromRpcMessagealigns with the JWT-based authentication refactoring. The newDataTypefield addition appears intentional for type discrimination.
207-235: Constructors properly initialize with RpcInputChType channels.
MakeWshRpcWithChannelscorrectly creates the typed channel, andMakeWshRpcprovides a convenient wrapper. The channel initialization on line 210 correctly usesbaseds.RpcInputChType.
289-341: Request handling correctly propagates ingressLinkId.The
ingressLinkIdis properly threaded throughhandleRequestInternalto theRpcResponseHandler, enabling downstream code to identify the originating link.
343-397: Server loop correctly processes RpcInputChType.The loop properly extracts
MsgBytesfor unmarshaling (line 372) andIngressLinkIdfor request handling (line 384). The debug logging on line 361 correctly accessesinputVal.MsgBytes.
609-611: GetIngressLinkId accessor completes the ingress tracking API.This accessor allows command handlers to retrieve the ingress link ID for routing decisions.
db/migrations-wstore/000009_mainserver.down.sql (1)
1-1: Standard down migration.The
DROP TABLE IF EXISTScorrectly reverses the corresponding up migration and handles the case where the table may not exist.pkg/wshutil/wshrpcio.go (1)
19-23: Correctly adapted to use RpcInputChType.The function now wraps line bytes in
RpcInputChTypebefore sending. TheIngressLinkIddefaults to0(NoLinkId), which is appropriate for stream-based input where there's no link context to track.db/migrations-wstore/000009_mainserver.up.sql (1)
1-5: Standard migration for MainServer persistence.The table structure follows the waveobj pattern with
oid(UUID primary key),version(for optimistic locking), anddata(JSON blob). TheIF NOT EXISTSclause ensures idempotency.cmd/wsh/cmd/wshcmd-wavepath.go (1)
59-68: LGTM! Consistent TabId handling.The WAVETERM_TABID validation and propagation to
PathCommandDatafollows the established pattern and is correctly implemented.cmd/wsh/cmd/wshcmd-term.go (1)
59-73: LGTM! TabId requirement properly integrated.The validation and propagation of
WAVETERM_TABIDto the terminal block creation data is correct and aligns with the PR's authentication and routing refactor.cmd/wsh/cmd/wshcmd-editor.go (1)
58-64: LGTM! Editor command properly requires TabId.The addition of
WAVETERM_TABIDvalidation and propagation to the editor block creation is correct and consistent with the broader authentication refactor.frontend/app/store/wshrpcutil.ts (2)
4-4: LGTM! Import correctly updated.The addition of
setWpsRpcClientto the import statement is necessary for wiring the RPC client to the WPS subsystem.
22-22: LGTM! WPS client wiring properly positioned.The call to
setWpsRpcClient(TabRpcClient)is correctly placed after client instantiation and before route registration, ensuring the WPS subsystem has access to the RPC client for event handling.cmd/wsh/cmd/wshcmd-launch.go (1)
57-62: LGTM!The
TabIdis correctly populated in theCommandCreateBlockDatastructure after validation. The integration is clean and consistent with the new tab-context requirement.cmd/wsh/cmd/wshcmd-view.go (1)
64-74: LGTM!Both preview paths (web URL and local file) correctly populate the
TabIdfield in their respectiveCommandCreateBlockDatastructures. The implementation ensures tab context is consistently provided regardless of the preview type.Also applies to: 91-101
cmd/wsh/cmd/wshcmd-web.go (1)
120-130: LGTM!The
TabIdfield is correctly populated in the web block creation path. The implementation is consistent with other command files and properly integrates the tab context requirement.frontend/app/store/wshrpcutil-base.ts (2)
4-4: LGTM!The import of
setWpsRpcClientis clean and correctly grouped with the existingwpsReconnectHandlerimport from the same module.
114-127: LGTM!The
setWpsRpcClient(electronClient)call is correctly placed in the initialization sequence. The ordering (router setup → WebSocket init → WPS client config → route registration → reconnect handlers) follows a logical initialization flow.cmd/wsh/cmd/wshcmd-run.go (1)
141-153: LGTM!The
TabIdfield is correctly populated in theCommandCreateBlockDatastructure for the run command. The integration is clean and maintains the existing logic while adding the required tab context.cmd/wsh/cmd/wshcmd-editconfig.go (1)
41-47: LGTM! Proper validation of required environment variable.The TabId retrieval and validation logic is correct. The error handling ensures that the required
WAVETERM_TABIDenvironment variable is present before proceeding with block creation.cmd/wsh/cmd/wshcmd-createblock.go (1)
37-47: LGTM! Consistent implementation with other commands.The TabId validation and propagation follows the same pattern as other block-creation commands, ensuring consistency across the codebase.
pkg/wshrpc/wshclient/barerpcclient.go (1)
32-33: LGTM! Aligns with the new link-based routing architecture.The migration from explicit channel wiring and
RegisterRoutetoMakeWshRpcwithRegisterTrustedLeafis consistent with the PR's objectives to refactor routing to a link-based model with trust levels. The emptyRpcContext{}is appropriate for a bare client that doesn't require specific route or block context.pkg/wshutil/wshcmdreader.go (1)
69-69: LGTM! Channel type migration is correct.The migration to
baseds.RpcInputChTypewraps the escape sequence bytes appropriately. TheIngressLinkIdfield is left at its zero value, which appears intentional for this local PTY buffer processing context where ingress link tracking isn't required.pkg/waveapp/waveapp.go (1)
166-189: Validate RouteId from RPC context.The code validates
BlockIdat lines 171-173 but doesn't validateRouteIdbefore assigning it at line 188. Since theRouteIdis now extracted from the JWT context and may be used for routing decisions, consider adding validation to ensure it's not empty.🔎 Suggested validation
client.RpcContext = rpcCtx if client.RpcContext == nil || client.RpcContext.BlockId == "" { return fmt.Errorf("no block id in rpc context") } + if client.RpcContext.RouteId == "" { + return fmt.Errorf("no route id in rpc context") + }pkg/wshrpc/wshserver/wshserverutil.go (1)
22-27: LGTM! Simplified RPC client construction.The change correctly adapts to the new MakeWshRpc signature, removing manual channel management and simplifying initialization. The empty RpcContext is appropriate for the main server RPC client singleton.
pkg/baseds/baseds.go (1)
1-14: LGTM! Clean shared type definitions.The new
LinkIdtype,NoLinkIdconstant, andRpcInputChTypestruct provide well-structured primitives for link tracking across the routing and RPC systems. The design is minimal and appropriate for shared infrastructure.pkg/wcore/wcore.go (1)
198-214: LGTM! Robust key initialization with comprehensive error handling.The key decoding and loading logic correctly handles all error cases. The
wavejwt.SetPrivateKeyandSetPublicKeycalls likely validate key formats internally, which should catch any corrupted data.pkg/wslconn/wslconn.go (3)
69-73: LGTM! Template updated for name-driven routing.The
ConnServerCmdTemplatecorrectly adds--routerand--connflags, aligning with the new connection naming and routing architecture. The additional placeholder supports optional flags like--dev.
270-274: LGTM! Dev mode flag correctly integrated.The dev mode detection and flag construction is clean. The
devFlagvariable is safely constructed (either empty or--dev), and the connection name is properly shell-quoted.
342-342: The route ID consistency is correct—no action needed. The connserver is started withconn.GetName()(line 274), which becomes the--connparameter that the connserver uses to register viaMakeConnectionRouteId(). This ensures both the registration and wait call use the same route ID format:"conn:wsl://distro_name".Likely an incorrect or invalid review comment.
cmd/wsh/cmd/wshcmd-ssh.go (1)
57-60: This is not an ssh-specific breaking change—it's part of the broader TabId architecture used across all wsh commands that create blocks.The
WAVETERM_TABIDrequirement applies consistently across 13+ commands (ssh, term, run, createblock, editconfig, editor, etc.), all following the same pattern. The environment variable is automatically set byshellcontroller.gowhen commands execute within a Wave terminal block, so users invokingwsh ssh --newfrom the Wave UI will haveWAVETERM_TABIDprovided transparently. The requirement only affects programmatic or out-of-context invocations, which appear to be outside the intended usage scope.pkg/util/packetparser/packetparser.go (1)
22-48: LGTM - Clean migration to RpcInputChType.The refactor correctly wraps raw packet bytes in
baseds.RpcInputChType{MsgBytes: ...}. TheIngressLinkIdfield is appropriately left unset here since the packet parser operates at a lower level and doesn't have link context - the ingress link will be set by upstream routing components.pkg/remote/conncontroller/conncontroller.go (1)
287-313: LGTM - Connserver startup refactored with RpcContext and dev mode support.The changes correctly:
- Construct an
RpcContextwithRouteId,SockName, andConnfor JWT token creation- Add the
--connflag with properly quoted connection name viashellutil.HardQuote- Conditionally append
--devflag for development modeThe command template update aligns with the connserver CLI changes in this PR.
pkg/wshrpc/wshclient/wshclient.go (1)
44-48: LGTM - Generated RPC client wrappers follow established patterns.The new commands (
AuthenticateTokenVerifyCommand,GetJwtPublicKeyCommand,SetPeerInfoCommand) are correctly generated following the existing pattern of usingsendRpcRequestCallHelperwith appropriate type parameters.Also applies to: 383-387, 661-665
frontend/app/store/wps.ts (1)
9-13: LGTM - Clean dependency injection pattern for RPC client.The
setWpsRpcClientsetter allows the RPC client to be injected after initialization, decoupling the WPS module from the client setup lifecycle. This aligns with the broader RPC wiring changes inwshrpcutil-base.ts.pkg/wshrpc/wshserver/wshserver.go (3)
73-75: LGTM - Clean implementation of GetJwtPublicKeyCommand.Returns the base64-encoded public key via
wavejwt.GetPublicKeyBase64(). This aligns with the new JWT-based authentication flow introduced in this PR.
473-486: LGTM - Input validation added for block deletion commands.Adding explicit checks for empty
BlockIdin bothDeleteSubBlockCommandandDeleteBlockCommandis a good defensive measure that provides clearer error messages than downstream failures.
1348-1356: This concern is not valid. TabId validation occurs downstream increateBlockObj(pkg/wcore/block.go, lines 132-135), which retrieves the tab and returns an error "tab not found" if the TabId is invalid or empty. The CLI caller (the only caller found) also validates TabId upfront by reading it from the WAVETERM_TABID environment variable and returning an error if absent. There is no code path that allowsCreateBlockCommandto be called with an empty or invalid TabId.Likely an incorrect or invalid review comment.
frontend/app/store/wshclientapi.ts (1)
30-33: LGTM - Generated TypeScript RPC API matches Go backend.The new commands (
AuthenticateTokenVerifyCommand,GetJwtPublicKeyCommand,SetPeerInfoCommand) are correctly generated with appropriate type signatures that match their Go counterparts inwshclient.go.Also applies to: 315-318, 550-553
cmd/wsh/cmd/wshcmd-root.go (1)
205-207: Usage ofgetTabIdFromEnvhelper is correctly implemented.The helper is actively used across 11 different command files (createblock, secret, web, editconfig, view, term, ssh, run, launch, wavepath, and editor), confirming it's being properly utilized by other commands as expected.
pkg/web/ws.go (3)
35-42: LGTM: Clean struct and map refactor for link-based routing.The
StableConnInfostruct correctly encapsulates connection identity with both the WebSocket connection ID and the router link ID. The map key change from route ID to stable ID aligns with the broader refactor.
260-275: Good defensive handling in registerConn.The implementation properly handles the case of replacing an existing connection for the same stable ID by unregistering the old link before registering the new one. This prevents link leaks.
277-289: Proper ownership check in unregisterConn.The check ensuring
curConnInfo.ConnId != wsConnIdprevents a race where a newer connection could be inadvertently unregistered by an older connection's cleanup path.pkg/wavejwt/wavejwt.go (2)
41-51: LGTM: Proper Ed25519 key generation.Uses
crypto/randfor secure random generation and returns raw key bytes in theKeyPairstruct.
129-130: Verify 1-year default expiry is appropriate for all token use cases.A 365-day expiry is quite long for JWTs. If some tokens should be short-lived (e.g., for ephemeral shell sessions), consider requiring callers to explicitly set
ExpiresAtor using different defaults based on token purpose.cmd/server/main-server.go (3)
460-465: LGTM: Proper router initialization sequence.Creating the router before other initialization steps and immediately setting it as root ensures the routing infrastructure is ready for subsequent registrations.
389-397: LGTM: Clean trusted leaf registration for internal routes.The main RPC client and local connection are correctly registered as trusted leaves with their respective route IDs. The
MakeWshRpccall properly constructs anRpcContextwith the connection name.
551-555: Good: InitMainServer error handling aborts startup.Failing to initialize the main server (which sets up JWT keys) correctly prevents the server from running in an invalid state.
pkg/blockcontroller/shellcontroller.go (5)
423-434: LGTM: Consistent RpcContext construction for WSL.The context is properly populated with RouteId, SockName, BlockId, and Conn fields before JWT token generation.
456-467: LGTM: Consistent RpcContext construction for SSH.Follows the same pattern as WSL, correctly using
conn.Opts.String()for the Conn field.
483-493: LGTM: Consistent RpcContext construction for Local.The Local branch correctly omits the Conn field (empty for local connections).
518-522: Good: Error handling added to RegisterTrustedLeaf.Returning an error when controller route registration fails prevents the shell from running without proper RPC routing.
599-600: LGTM: Link-based cleanup in wait loop.Using
UnregisterLink(controllerLinkId)in the deferred cleanup ensures the link is properly removed when the shell process exits.pkg/wshutil/wshproxy.go (2)
14-21: LGTM: Clean struct update for typed channels and peer info.The
FromRemoteChtype change tobaseds.RpcInputChTypeand addition ofPeerInfoalign with the link-based routing refactor.
54-57: LGTM: RecvRpcMessage correctly extracts message bytes.The function properly unwraps
MsgBytesfrom the typed channel input.pkg/wshutil/wshutil.go (6)
204-208: LGTM: Channel type and RPC constructor update.Using
baseds.RpcInputChTypeandMakeWshRpcWithChannelsaligns with the typed channel architecture.
300-317: Good dev-mode validation for RpcContext invariants.The panic checks ensure that routers don't have route IDs and non-routers always have route IDs. This catches configuration errors early in development.
319-327: LGTM: Clean helper for claims-to-context conversion.Centralizes the mapping between JWT claims and RpcContext.
360-376: LGTM: Link-based lifecycle for stdio client.Properly registers a trusted router link, and the cleanup function correctly unregisters the link and closes the channel.
406-436: LGTM: Proper link lifecycle management for domain socket clients.Using
atomic.Int32for the link ID container andRegisterUntrustedLinkfor domain socket clients (which need JWT verification) is the correct approach.
276-277: Good: Home directory expansion for socket paths.Expanding
~in socket paths before connecting prevents connection failures when paths contain home directory references.cmd/wsh/cmd/wshcmd-connserver.go (5)
68-99: LGTM: Proper panic handling and link lifecycle in listener handler.The defer at line 69-71 catches panics, and the atomic-based link ID tracking with cleanup on connection close prevents link leaks.
120-129: LGTM: Clean RPC client setup with proper context construction.The route ID is derived from the connection name, and the RpcContext is properly populated. Registration as a trusted leaf is correct for the connserver's own RPC client.
171-187: Good: JWT public key fetch and validation sequence.Fetching the public key from the upstream server and setting it locally enables JWT validation for incoming domain socket connections. The base64 decode and size validation (in
SetPublicKey) ensure the key is valid.
266-271: Good: Early validation of required --conn parameter.Failing early when the required parameter is missing provides a clear error message and prevents running in an invalid state.
219-224: Good: Panic protection added to sysinfo loop.The deferred panic handler ensures the goroutine doesn't crash the entire process if something goes wrong.
pkg/wshrpc/wshrpctypes.go (6)
53-63: Well-organized control plane command constants.The command constants are clearly documented with their routing context (e.g.,
$control,$control:root). This improves code clarity for the new routing architecture.
200-208: LGTM on new interface methods.The new
AuthenticateTokenVerifyCommand,SetPeerInfoCommand, andGetJwtPublicKeyCommandmethods align with the frontend client API and server-side helpers shown in the relevant code snippets.
380-380: Improved cancellation signature.Changing
StreamCancelFnfromfunc()tofunc(context.Context) errorallows for context-aware cancellation and proper error propagation from the cancellation handler.
383-389: Clean RpcContext struct design.The explicit
RpcContextstruct with clear fields (SockName,RouteId,BlockId,Conn,IsRouter) replaces the previous reflection-based tag injection approach. This is more maintainable and type-safe.
391-396: Appropriate return data structure.Including
RpcContextas a pointer in the authentication return data allows optional context binding while maintaining backward compatibility with the existingEnvandInitScriptTextfields.
407-409: Consistent tag cleanup across command data types.The removal of
wshcontexttags in favor of plainjsontags across all command data structures is consistent with the shift to explicitRpcContextfields. This simplifies serialization and eliminates the need for reflection-based context injection.Also applies to: 411-413, 415-418, 420-423, 429-431, 445-449, 457-462, 523-527, 534-536, 711-717, 809-811, 827-832
pkg/wshutil/wshrouter.go (9)
23-37: Clean routing constant organization.The control plane routing constants (
ControlRoute,ControlRootRoute,ControlPrefix) are well-structured and the prefixes provide clear namespace separation.
49-73: Good encapsulation with internal types.The
linkMeta,rpcRoutingInfo, andmessageWraptypes provide clean abstractions for the link-based routing model. TheName()method onlinkMetais useful for logging.
75-91: Well-structured router state.The refactored
WshRouterstruct clearly separates concerns: link management (linkMap), route resolution (routeMap), RPC tracking (rpcMap), and upstream buffering. The use of a separate mutex for upstream buffering (upstreamBufLock) avoids contention with the main lock.
400-457: Static analysis false positive - defer captures exitReason by reference.The golangci-lint warnings about "ineffectual assignment to exitReason" at lines 415 and 425 are false positives. The
deferat line 411 capturesexitReasonby reference, so the assignments beforebreakcorrectly update what gets logged when the deferred function executes.The recv loop implementation is solid with proper panic handling and clear exit reason logging.
459-491: Good defensive copying pattern.Both
getLinkMetaandgetLinkForRoutereturn copies oflinkMetarather than pointers to map values. This correctly prevents data races when the caller uses the returned struct outside the lock.
531-547: Control plane registration looks correct.The control plane is properly registered as a trusted leaf, and the source route is correctly bound. The root router also binds
$control:rootinSetAsRootRouter.
633-664: Thorough validation in bindRouteLocally.Good defensive checks for: NoLinkId, non-bindable routes, missing links, untrusted links, and link kind mismatches. The source route conflict check prevents accidental rebinding.
695-701: Broker integration for route lifecycle.The
unsubscribeFromBrokerproperly cleans up subscriptions and publishes aRouteGoneevent. The panic handler protects against broker-related panics.
703-714: Clear unauthenticated error handling.The error response correctly identifies the unauthenticated link and the attempted command, providing useful debugging information.
| func serverRun(cmd *cobra.Command, args []string) error { | ||
| var logFile *os.File | ||
| logWriter := io.Writer(os.Stderr) | ||
| if connServerDev { | ||
| var err error | ||
| logFile, err = os.OpenFile("/tmp/connserver.log", os.O_CREATE|os.O_WRONLY|os.O_APPEND, 0644) | ||
| if err != nil { | ||
| fmt.Fprintf(os.Stderr, "failed to open log file: %v\n", err) | ||
| log.SetFlags(log.LstdFlags | log.Lmicroseconds) | ||
| log.SetPrefix(fmt.Sprintf("[PID:%d] ", os.Getpid())) | ||
| } else { | ||
| defer logFile.Close() | ||
| logWriter = io.MultiWriter(os.Stderr, logFile) | ||
| log.SetOutput(logWriter) | ||
| log.SetFlags(log.LstdFlags | log.Lmicroseconds) | ||
| log.SetPrefix(fmt.Sprintf("[PID:%d] ", os.Getpid())) | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix ineffectual assignment to logWriter.
The static analysis correctly identifies that logWriter is assigned at line 250 but never used if the else branch is taken. The variable should only be declared where it's used.
🔎 Proposed fix
func serverRun(cmd *cobra.Command, args []string) error {
var logFile *os.File
- logWriter := io.Writer(os.Stderr)
if connServerDev {
var err error
logFile, err = os.OpenFile("/tmp/connserver.log", os.O_CREATE|os.O_WRONLY|os.O_APPEND, 0644)
if err != nil {
fmt.Fprintf(os.Stderr, "failed to open log file: %v\n", err)
log.SetFlags(log.LstdFlags | log.Lmicroseconds)
log.SetPrefix(fmt.Sprintf("[PID:%d] ", os.Getpid()))
} else {
defer logFile.Close()
- logWriter = io.MultiWriter(os.Stderr, logFile)
- log.SetOutput(logWriter)
+ log.SetOutput(io.MultiWriter(os.Stderr, logFile))
log.SetFlags(log.LstdFlags | log.Lmicroseconds)
log.SetPrefix(fmt.Sprintf("[PID:%d] ", os.Getpid()))
}
}🧰 Tools
🪛 golangci-lint (2.5.0)
[error] 250-250: ineffectual assignment to logWriter
(ineffassign)
| wshclient.AuthenticateCommand(RpcClient, jwtToken, &wshrpc.RpcOpts{Route: wshutil.ControlRoute}) | ||
| blockId := os.Getenv("WAVETERM_BLOCKID") | ||
| if blockId != "" { | ||
| peerInfo := fmt.Sprintf("domain:block:%s", blockId) | ||
| wshclient.SetPeerInfoCommand(RpcClient, peerInfo, &wshrpc.RpcOpts{Route: wshutil.ControlRoute}) | ||
| } |
There was a problem hiding this comment.
AuthenticateCommand error is silently ignored.
The return value and potential error from AuthenticateCommand are not handled. If authentication fails, the RPC client will continue operating in an unauthenticated state without any indication to the caller.
🔎 Proposed fix
- wshclient.AuthenticateCommand(RpcClient, jwtToken, &wshrpc.RpcOpts{Route: wshutil.ControlRoute})
+ _, err = wshclient.AuthenticateCommand(RpcClient, jwtToken, &wshrpc.RpcOpts{Route: wshutil.ControlRoute})
+ if err != nil {
+ return fmt.Errorf("error authenticating: %v", err)
+ }🤖 Prompt for AI Agents
In cmd/wsh/cmd/wshcmd-root.go around lines 164 to 169, the call to
wshclient.AuthenticateCommand ignores its return value and any error; change it
to capture the error, check if err != nil, log the authentication failure (with
the err) and abort startup (e.g., return the error or call os.Exit(1)) so the
RPC client does not continue unauthenticated; only proceed to set peerInfo when
authentication succeeds.
| blockId := os.Getenv("WAVETERM_BLOCKID") | ||
| if blockId == "" { | ||
| return nil, fmt.Errorf("no WAVETERM_BLOCKID env var set") | ||
| } | ||
| rtnData, err := wshclient.ResolveIdsCommand(RpcClient, wshrpc.CommandResolveIdsData{ | ||
| BlockId: blockId, | ||
| Ids: []string{id}, | ||
| }, &wshrpc.RpcOpts{Timeout: 2000}) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n 'resolveSimpleId|resolveBlockArg' --type=go -C3Repository: wavetermdev/waveterm
Length of output: 5714
🏁 Script executed:
# Read the full resolveSimpleId implementation
sed -n '179,210p' cmd/wsh/cmd/wshcmd-root.goRepository: wavetermdev/waveterm
Length of output: 1069
🏁 Script executed:
# Check if getThisBlockMeta exists or was removed
rg 'getThisBlockMeta' --type=goRepository: wavetermdev/waveterm
Length of output: 46
🏁 Script executed:
# Check where WAVETERM_BLOCKID is set or used
rg 'WAVETERM_BLOCKID' --type=go -B2 -A2Repository: wavetermdev/waveterm
Length of output: 1505
🏁 Script executed:
# Check if there's documentation about wsh or how it's meant to be used
find . -name "README*" -o -name "*.md" | grep -i wsh | head -5Repository: wavetermdev/waveterm
Length of output: 46
🏁 Script executed:
# Look at the initialization code in wshcmd-root.go to understand the full context
sed -n '1,150p' cmd/wsh/cmd/wshcmd-root.go | tail -50Repository: wavetermdev/waveterm
Length of output: 1639
🏁 Script executed:
# Check the full context of how environment is handled and wsh is invoked
sed -n '130,175p' cmd/wsh/cmd/wshcmd-root.goRepository: wavetermdev/waveterm
Length of output: 2006
🏁 Script executed:
# Check git history to see if getThisBlockMeta existed and what it did
git log --all --oneline --grep="getThisBlockMeta" 2>/dev/null || echo "Git history not available"Repository: wavetermdev/waveterm
Length of output: 46
🏁 Script executed:
# Check if there are any examples or tests showing wsh usage patterns
find . -type f -name "*.go" -path "*/test*" -o -path "*/example*" | xargs grep -l "wsh" 2>/dev/null | head -5Repository: wavetermdev/waveterm
Length of output: 87
🏁 Script executed:
# Check the test file to understand how wsh is used
cat cmd/test/test-main.go | head -100Repository: wavetermdev/waveterm
Length of output: 1385
🏁 Script executed:
# Let's look for any pattern of how this worked before - check if there are conditionals or fallback patterns
rg -A5 -B5 'resolveSimpleId.*this' --type=goRepository: wavetermdev/waveterm
Length of output: 46
resolveSimpleId requires WAVETERM_BLOCKID environment variable for relative ID resolution.
When resolving relative identifiers (e.g., "this" or simple names), resolveSimpleId now requires WAVETERM_BLOCKID to be set. Full ORef format identifiers (e.g., "block:xxx") continue to work without it. This affects all commands that use resolveBlockArg() and rely on the default "this" resolution when no explicit block argument is provided—including getmeta, readfile, setmeta, web, debug, setvar, getvar, and deleteblock.
In normal waveterm execution, WAVETERM_BLOCKID is automatically set by the shell controller. The constraint only impacts usage patterns where wsh is invoked outside of a waveterm block context with relative identifiers.
🤖 Prompt for AI Agents
cmd/wsh/cmd/wshcmd-root.go around lines 187 to 194, resolveSimpleId currently
always requires WAVETERM_BLOCKID which breaks full ORef identifiers; change the
logic to first detect if the provided id is already a full ORef (e.g., starts
with "block:" or matches the ORef pattern) and, if so, call ResolveIdsCommand
directly without reading WAVETERM_BLOCKID, otherwise (for relative ids like
"this") keep requiring WAVETERM_BLOCKID; also update the error returned when
WAVETERM_BLOCKID is missing to clearly state it is required only for relative
IDs.
| if jwtToken != "" && cmdOpts.ForceJwt { | ||
| conn.Debugf(ctx, "adding JWT token to environment\n") | ||
| cmdCombined = fmt.Sprintf(`%s=%s %s`, wavebase.WaveJwtTokenVarName, jwtToken, cmdCombined) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the file to confirm the line numbers and code
wc -l pkg/shellexec/shellexec.goRepository: wavetermdev/waveterm
Length of output: 94
🏁 Script executed:
# Read the section around line 268 to see the change
sed -n '260,280p' pkg/shellexec/shellexec.goRepository: wavetermdev/waveterm
Length of output: 1007
🏁 Script executed:
# Check line 447 for StartRemoteShellProc JWT handling
sed -n '440,460p' pkg/shellexec/shellexec.goRepository: wavetermdev/waveterm
Length of output: 955
🏁 Script executed:
# Check line 514 for StartLocalShellProc JWT handling
sed -n '505,525p' pkg/shellexec/shellexec.goRepository: wavetermdev/waveterm
Length of output: 991
🏁 Script executed:
# Find all calls to StartWslShellProc
rg -n 'StartWslShellProc\s*\(' --type=go -A 2 -B 2Repository: wavetermdev/waveterm
Length of output: 911
🏁 Script executed:
# Search for CommandOptsType struct definition and ForceJwt field
rg -n 'type CommandOptsType' --type=go -A 15Repository: wavetermdev/waveterm
Length of output: 1293
🏁 Script executed:
# Check the context around the caller at line 435 in shellcontroller.go
sed -n '420,450p' pkg/blockcontroller/shellcontroller.goRepository: wavetermdev/waveterm
Length of output: 1236
🏁 Script executed:
# Search for all StartWslShellProc calls with broader context
rg -n 'StartWslShellProc' --type=go -B 10 -A 5Repository: wavetermdev/waveterm
Length of output: 6895
🏁 Script executed:
# Look for where cmdOpts is populated in shellcontroller.go
rg -n 'CommandOptsType\s*\{' pkg/blockcontroller/shellcontroller.go -A 10Repository: wavetermdev/waveterm
Length of output: 46
🏁 Script executed:
# Look earlier in the function to see how cmdOpts is initialized
sed -n '350,415p' pkg/blockcontroller/shellcontroller.goRepository: wavetermdev/waveterm
Length of output: 2692
🏁 Script executed:
# Search for ForceJwt assignments in shellcontroller.go
rg -n 'ForceJwt' pkg/blockcontroller/shellcontroller.go -B 3 -A 3Repository: wavetermdev/waveterm
Length of output: 261
🏁 Script executed:
# Search for where ForceJwt might be set to true in the WSL code path
rg -n 'ForceJwt\s*=' --type=go -B 5 -A 5Repository: wavetermdev/waveterm
Length of output: 984
🏁 Script executed:
# Check if there's logic to set ForceJwt based on JWT presence
rg -n 'cmdOpts.ForceJwt' --type=go -B 10 -A 2Repository: wavetermdev/waveterm
Length of output: 4106
🏁 Script executed:
# Look at the entire WSL path to see if ForceJwt is set before the call
sed -n '414,446p' pkg/blockcontroller/shellcontroller.goRepository: wavetermdev/waveterm
Length of output: 1271
Critical issue: JWT tokens are prepared but not injected for WSL Shell blocks.
While the consistency across the three shell functions is correct (all require ForceJwt flag), the calling code has a bug: on line 434, a JWT token is created and stored in swapToken.Env, but cmdOpts.ForceJwt is never set to true before calling StartWslShellProc at line 435. This means the prepared JWT will silently fail to inject because the condition if jwtToken != "" && cmdOpts.ForceJwt at line 268 will evaluate to false (ForceJwt defaults to false for Shell blocks).
Add cmdOpts.ForceJwt = true in the WSL WSH path (around line 433-434) after confirming the JWT token is created, to ensure the token is actually injected into the shell environment.
🤖 Prompt for AI Agents
In pkg/shellexec/shellexec.go around lines 433-434 (and relevant to the JWT
check at lines 268-271), the code creates and stores a JWT in swapToken.Env but
never sets cmdOpts.ForceJwt, so the injection conditional (if jwtToken != "" &&
cmdOpts.ForceJwt) prevents the token from being added to the WSL shell
environment; after confirming the JWT was successfully created for the WSL/WSH
path, set cmdOpts.ForceJwt = true before invoking StartWslShellProc (only in the
WSL/WSH branch) so the existing environment-injection code at lines ~268-271
will include the JWT in the shell command.
|
|
||
| var ( | ||
| safePattern = regexp.MustCompile(`^[a-zA-Z0-9_/.-]+$`) | ||
| safePattern = regexp.MustCompile(`^[a-zA-Z0-9_@:,+=/.-]+$`) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n pkg/util/shellutil/shellquote.go | head -60Repository: wavetermdev/waveterm
Length of output: 1460
🏁 Script executed:
# Find usages of HardQuote and related functions
rg -n 'HardQuote|SoftQuote' --type=go pkg/util/shellutil/Repository: wavetermdev/waveterm
Length of output: 1324
🏁 Script executed:
# Look for test files to understand expected behavior
fd -e _test.go shellquoteRepository: wavetermdev/waveterm
Length of output: 46
🏁 Script executed:
# Search for HardQuote usage across entire codebase
rg -l 'HardQuote' --type=go | head -20Repository: wavetermdev/waveterm
Length of output: 315
🏁 Script executed:
cat -n pkg/util/shellutil/shellquote_test.goRepository: wavetermdev/waveterm
Length of output: 3088
🏁 Script executed:
# Check if HardQuote is used in any shell command execution contexts
rg -B5 -A5 'HardQuote.*\(' pkg/genconn/genconn.go pkg/shellexec/shellexec.go pkg/wslconn/wslconn.goRepository: wavetermdev/waveterm
Length of output: 2744
🏁 Script executed:
# Look at tokenswap.go more carefully to understand the context
cat -n pkg/util/shellutil/tokenswap.go | head -140Repository: wavetermdev/waveterm
Length of output: 4520
🏁 Script executed:
# Check if test file has any tests for strings starting with dash
grep -n '^\s*input:\s*"-' pkg/util/shellutil/shellquote_test.goRepository: wavetermdev/waveterm
Length of output: 46
🏁 Script executed:
# Let's verify the exact behavior by checking what HardQuote does with "-rf"
# Look at the pattern more carefully
cat -n pkg/util/shellutil/shellquote.go | sed -n '15,35p'Repository: wavetermdev/waveterm
Length of output: 600
Flag injection risk: strings starting with - are returned unquoted by safePattern.
The expanded safePattern regex ^[a-zA-Z0-9_@:,+=/.-]+$ matches strings starting with - and returns them unquoted. While not traditional command injection, this causes flag injection vulnerabilities in command contexts.
For example, in pkg/genconn/genconn.go line 73, if opts.Cwd is a string like -rf, the code generates cd -rf && ... where cd misinterprets -rf as flags rather than a directory name. Similar issues occur in environment variable assignments in tokenswap.go and elsewhere.
Additionally, the test file contains no test case for dash-prefixed strings, indicating a coverage gap in validation.
Recommendation: Either remove - from safePattern or quote all strings that start with -.
🤖 Prompt for AI Agents
In pkg/util/shellutil/shellquote.go around line 16, safePattern currently allows
strings beginning with '-' which leads to flag injection (unquoted dash-prefixed
strings are returned raw); update the logic so dash-prefixed values are always
quoted: either remove '-' from the safePattern character class or add a
conditional that detects strings starting with '-' and forces quoting, and add
unit tests covering dash-prefixed inputs to verify they are quoted.
| file, err := os.Create(DumpFilePath) | ||
| if err != nil { | ||
| continue | ||
| } | ||
| utilfn.DumpGoRoutineStacks(file) | ||
| file.Close() |
There was a problem hiding this comment.
Add logging for file creation failures.
When os.Create fails (e.g., permission issues, disk full), the error is silently ignored, making it difficult to diagnose why stack dumps aren't being generated.
🔎 Proposed improvements
Improvement 1: Log file creation errors
for range sigCh {
file, err := os.Create(DumpFilePath)
if err != nil {
+ log.Printf("failed to create stack dump file: %v\n", err)
continue
}
utilfn.DumpGoRoutineStacks(file)
file.Close()
}Improvement 2 (optional): Check close error
utilfn.DumpGoRoutineStacks(file)
- file.Close()
+ if err := file.Close(); err != nil {
+ log.Printf("failed to close stack dump file: %v\n", err)
+ }📝 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.
| file, err := os.Create(DumpFilePath) | |
| if err != nil { | |
| continue | |
| } | |
| utilfn.DumpGoRoutineStacks(file) | |
| file.Close() | |
| file, err := os.Create(DumpFilePath) | |
| if err != nil { | |
| log.Printf("failed to create stack dump file: %v\n", err) | |
| continue | |
| } | |
| utilfn.DumpGoRoutineStacks(file) | |
| file.Close() |
| file, err := os.Create(DumpFilePath) | |
| if err != nil { | |
| continue | |
| } | |
| utilfn.DumpGoRoutineStacks(file) | |
| file.Close() | |
| file, err := os.Create(DumpFilePath) | |
| if err != nil { | |
| continue | |
| } | |
| utilfn.DumpGoRoutineStacks(file) | |
| if err := file.Close(); err != nil { | |
| log.Printf("failed to close stack dump file: %v\n", err) | |
| } |
| file, err := os.Create(DumpFilePath) | |
| if err != nil { | |
| continue | |
| } | |
| utilfn.DumpGoRoutineStacks(file) | |
| file.Close() | |
| file, err := os.Create(DumpFilePath) | |
| if err != nil { | |
| log.Printf("failed to create stack dump file: %v\n", err) | |
| continue | |
| } | |
| utilfn.DumpGoRoutineStacks(file) | |
| if err := file.Close(); err != nil { | |
| log.Printf("failed to close stack dump file: %v\n", err) | |
| } |
🤖 Prompt for AI Agents
In pkg/util/sigutil/sigusr1_notwindows.go around lines 27 to 32, the code
currently ignores errors returned by os.Create and by file.Close; modify it to
log the os.Create error when it fails (including the error value and the path),
skip writing the dump when create fails, and after
utilfn.DumpGoRoutineStacks(file) attempt to close the file and log any Close()
error as well so failures are visible for debugging.
| type MainServer struct { | ||
| OID string `json:"oid"` | ||
| Version int `json:"version"` | ||
| Meta MetaMapType `json:"meta"` | ||
| JwtPrivateKey string `json:"jwtprivatekey"` // base64 | ||
| JwtPublicKey string `json:"jwtpublickey"` // base64 | ||
| } | ||
|
|
||
| func (*MainServer) GetOType() string { | ||
| return OType_MainServer | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for all usages of JwtPrivateKey to understand how it's handled
rg -n 'JwtPrivateKey' --type=go -B2 -A2Repository: wavetermdev/waveterm
Length of output: 1319
🏁 Script executed:
#!/bin/bash
# Search for encryption-related functions or patterns
rg -n 'encrypt|Encrypt|cipher|Cipher|crypto' --type=go | grep -i 'key\|private' | head -20Repository: wavetermdev/waveterm
Length of output: 612
🏁 Script executed:
#!/bin/bash
# Look for database operations and field handling around MainServer
rg -n 'MainServer' --type=go -B1 -A5 | head -50Repository: wavetermdev/waveterm
Length of output: 2393
🏁 Script executed:
#!/bin/bash
# Search for any secret management, key rotation, or vault-related code
rg -n 'secret|Secret|vault|Vault|rotate|Rotate' --type=go | head -30Repository: wavetermdev/waveterm
Length of output: 2506
Encrypt JWT private key at rest in database.
The JwtPrivateKey is stored in the database as a base64-encoded string without encryption. Base64 encoding is not encryption and provides no protection if the database is compromised. Implement one of the following:
- Encrypt the private key at rest using database encryption or application-level encryption
- Use the existing secret management system instead of direct database storage
- Add a key rotation mechanism with secure key derivation
This is necessary to prevent private key exposure in case of database breach.
🤖 Prompt for AI Agents
In pkg/waveobj/wtype.go around lines 298–308, the JwtPrivateKey field is stored
as a base64 string which is not secure; replace direct storage with an
encrypted-at-rest approach: change the model to hold an encrypted blob or a
reference to the secret manager (e.g., EncryptedJwtPrivateKey or
JwtPrivateKeyRef), implement application-side encryption using authenticated
encryption (AES-GCM) with a KMS/secret-manager data key (store ciphertext,
nonce/iv, and tag or store a secret-manager pointer), and provide
SetJwtPrivateKey/DecryptJwtPrivateKey methods that encrypt on write and decrypt
on read; ensure keys are not logged, support key rotation by storing key-version
metadata, and persist only ciphertext (optionally base64-encoded) or a
secret-manager identifier instead of plaintext/base64 private key.
| func (router *WshRouter) runUpstreamBufferLoop() { | ||
| defer func() { | ||
| panichandler.PanicHandler("WshRouter:runUpstreamBufferLoop", recover()) | ||
| }() | ||
| for { | ||
| router.upstreamBufLock.Lock() | ||
| for len(router.upstreamBuf) == 0 { | ||
| router.upstreamBufCond.Wait() | ||
| } | ||
| for { | ||
| msgBytes, ok := rpc.RecvRpcMessage() | ||
| if !ok { | ||
| break | ||
| msg := router.upstreamBuf[0] | ||
| router.upstreamBuf = router.upstreamBuf[1:] | ||
| router.upstreamBufLock.Unlock() | ||
|
|
||
| upstream := router.getUpstreamClient() | ||
| if upstream != nil { | ||
| upstream.SendRpcMessage(msg.msgBytes, baseds.NoLinkId, msg.debugStr) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Potential memory growth and missing exit condition in upstream buffer loop.
Two concerns with runUpstreamBufferLoop:
-
The slicing pattern
router.upstreamBuf = router.upstreamBuf[1:]doesn't release memory from the underlying array. For a long-running server with high message throughput, this can cause gradual memory growth. -
The loop has no exit condition. If the router is shut down or upstream is permanently gone, this goroutine will spin indefinitely.
🔎 Suggested improvement for memory management
func (router *WshRouter) runUpstreamBufferLoop() {
defer func() {
panichandler.PanicHandler("WshRouter:runUpstreamBufferLoop", recover())
}()
for {
router.upstreamBufLock.Lock()
for len(router.upstreamBuf) == 0 {
router.upstreamBufCond.Wait()
}
msg := router.upstreamBuf[0]
- router.upstreamBuf = router.upstreamBuf[1:]
+ // Copy to avoid retaining old array memory
+ router.upstreamBuf = append([]messageWrap(nil), router.upstreamBuf[1:]...)
router.upstreamBufLock.Unlock()
upstream := router.getUpstreamClient()
if upstream != nil {
upstream.SendRpcMessage(msg.msgBytes, baseds.NoLinkId, msg.debugStr)
}
}
}Committable suggestion skipped: line range outside the PR's diff.
the PR spiraled and ended up being much larger than anticipated.
it is a refactor of wshrouter to have it track "links" as opposed to just routes. this lets us simplify a lot of things when it comes to multi-level routing.
also adds more debugging, more logging, some windows fixes, other wsl fixes