conn updates 3 #1711
conn updates 3 #1711
Conversation
…code using a special context
…o enables blocklogger. fix small remote display name in password prompt
…w takes a logblockid.
|
Warning Rate limit exceeded@sawka has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 12 minutes and 55 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
WalkthroughThe pull request introduces a comprehensive set of changes across multiple packages and files in the WaveTerm project, focusing on enhancing logging, connection management, and debugging capabilities. The modifications span frontend and backend components, introducing new logging mechanisms, connection metadata, and debugging options. Key changes include the addition of a new The updates reflect a systematic approach to improving the application's observability, allowing for more granular logging and debugging of connection-related activities while maintaining the existing architectural patterns and error handling strategies. Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🔭 Outside diff range comments (1)
pkg/remote/conncontroller/conncontroller.go (1)
Line range hint
191-204: Avoid logging sensitive information such as domain socket namesLogging the generated domain socket name may expose sensitive information in logs. Consider removing the socket name from the log messages or obfuscating it to prevent potential security risks.
Apply this diff to remove the socket name from the log messages:
- conn.Infof(ctx, "generated domain socket name %s\n", sockName) + conn.Infof(ctx, "domain socket listener created\n")
🧹 Nitpick comments (12)
pkg/wshutil/wshrpcio.go (1)
65-78: Consider buffering the channel for better performance.The implementation is correct, but unbuffered channels can lead to potential blocking in high-throughput scenarios.
- ch := make(chan LineOutput) + ch := make(chan LineOutput, 1024) // Buffer size can be adjusted based on expected loadcmd/wsh/cmd/wshcmd-conn.go (1)
180-184: Inconsistent structure usage between functions.While other functions use
ConnExtData, this function usesConnRequest. Consider standardizing the structure usage across similar functions for better maintainability.- data := wshrpc.ConnRequest{ - Host: connName, - LogBlockId: RpcContext.BlockId, - } + data := wshrpc.ConnExtData{ + ConnName: connName, + LogBlockId: RpcContext.BlockId, + }pkg/wsl/wsl.go (1)
449-451: Consider a more descriptive variable name.The implementation correctly uses the config watcher and improves default value handling. However, the variable name
wshAskcould be more descriptive.Apply this diff for better clarity:
-wshAsk := wconfig.DefaultBoolPtr(config.Settings.ConnAskBeforeWshInstall, true) +shouldPromptForWshInstall := wconfig.DefaultBoolPtr(config.Settings.ConnAskBeforeWshInstall, true) -installErr := conn.CheckAndInstallWsh(ctx, conn.GetName(), &WshInstallOpts{NoUserPrompt: !wshAsk}) +installErr := conn.CheckAndInstallWsh(ctx, conn.GetName(), &WshInstallOpts{NoUserPrompt: !shouldPromptForWshInstall})frontend/app/store/wshclientapi.ts (1)
41-41: Add JSDoc comments for the modified and new methods.The implementation correctly updates the parameter types and adds the new method. Consider adding JSDoc comments to document the parameters and return types.
Add documentation like this:
/** * Ensures a connection is established with the specified connection data. * @param client The WebSocket client instance * @param data The connection extension data * @param opts Optional RPC options * @returns A promise that resolves when the connection is ensured */ ConnEnsureCommand(client: WshClient, data: ConnExtData, opts?: RpcOpts): Promise<void> /** * Reinstalls the Wave Shell Extensions for a connection. * @param client The WebSocket client instance * @param data The connection extension data * @param opts Optional RPC options * @returns A promise that resolves when the reinstallation is complete */ ConnReinstallWshCommand(client: WshClient, data: ConnExtData, opts?: RpcOpts): Promise<void> /** * Appends output data to a command controller. * @param client The WebSocket client instance * @param data The output data to append * @param opts Optional RPC options * @returns A promise that resolves when the output is appended */ ControllerAppendOutputCommand(client: WshClient, data: CommandControllerAppendOutputData, opts?: RpcOpts): Promise<void>Also applies to: 51-51, 60-64
pkg/wshrpc/wshserver/wshserver.go (3)
265-276: Add input validation for empty or malformed data.Consider validating the input data before processing to ensure robustness:
- Check if
data.Data64is empty- Validate if
data.BlockIdis emptyfunc (ws *WshServer) ControllerAppendOutputCommand(ctx context.Context, data wshrpc.CommandControllerAppendOutputData) error { + if data.BlockId == "" { + return fmt.Errorf("blockId cannot be empty") + } + if data.Data64 == "" { + return fmt.Errorf("data64 cannot be empty") + } outputBuf := make([]byte, base64.StdEncoding.DecodedLen(len(data.Data64))) nw, err := base64.StdEncoding.Decode(outputBuf, []byte(data.Data64))
614-627: Improve error handling for block retrieval.The error from
DBMustGetis silently ignored. Consider logging the error or propagating it to help with debugging.func termCtxWithLogBlockId(ctx context.Context, logBlockId string) context.Context { if logBlockId == "" { return ctx } block, err := wstore.DBMustGet[*waveobj.Block](ctx, logBlockId) if err != nil { + log.Printf("error getting block for logging context: %v", err) return ctx }
629-635: Maintain consistent error message formatting.The error messages in connection-related functions should follow a consistent format. Consider using a helper function to format connection-related errors.
+func formatConnError(connName string, err error) error { + return fmt.Errorf("connection error for %q: %v", connName, err) +} func (ws *WshServer) ConnEnsureCommand(ctx context.Context, data wshrpc.ConnExtData) error { ctx = termCtxWithLogBlockId(ctx, data.LogBlockId) if strings.HasPrefix(data.ConnName, "wsl://") { distroName := strings.TrimPrefix(data.ConnName, "wsl://") - return wsl.EnsureConnection(ctx, distroName) + if err := wsl.EnsureConnection(ctx, distroName); err != nil { + return formatConnError(data.ConnName, err) + } + return nil } - return conncontroller.EnsureConnection(ctx, data.ConnName) + if err := conncontroller.EnsureConnection(ctx, data.ConnName); err != nil { + return formatConnError(data.ConnName, err) + } + return nil }Also applies to: 680-699
frontend/types/gotypes.d.ts (1)
128-132: Add JSDoc comments for new type declarations.Consider adding documentation for the new types to improve code maintainability:
+/** + * Data structure for appending output to a block file. + */ type CommandControllerAppendOutputData = { blockid: string; data64: string; }; +/** + * Extended connection data with logging context. + */ type ConnExtData = { connname: string; logblockid?: string; };Also applies to: 295-299
frontend/app/block/blockframe.tsx (2)
359-363: Consistent timeout handling for connection operations.Consider extracting the timeout value to a constant to maintain consistency across all connection operations.
+const CONN_OPERATION_TIMEOUT = 60000; const handleTryReconnect = React.useCallback(() => { const prtn = RpcApi.ConnConnectCommand( TabRpcClient, { host: connName, logblockid: nodeModel.blockId }, - { timeout: 60000 } + { timeout: CONN_OPERATION_TIMEOUT } );
548-552: Consistent error handling for connection operations.The error handling for connection operations should be consistent. Consider extracting the error handling logic to a reusable function.
+const handleConnError = (operation: string, blockId: string, connName: string, error: any) => { + console.log(`error ${operation}`, blockId, connName, error); +}; RpcApi.ConnEnsureCommand( TabRpcClient, { connname: connName, logblockid: nodeModel.blockId }, { timeout: 60000 } -).catch((e) => { - console.log("error ensuring connection", nodeModel.blockId, connName, e); -}); +).catch((e) => handleConnError("ensuring", nodeModel.blockId, connName, e));Also applies to: 702-706
pkg/remote/sshclient.go (1)
156-156: Comprehensive logging implementation.The addition of debug logging provides better visibility into the SSH connection process. Consider adding log levels to control verbosity.
+const ( + LogLevelDebug = "debug" + LogLevelInfo = "info" +) +func logWithLevel(ctx context.Context, level string, format string, args ...interface{}) { + if level == LogLevelDebug { + blocklogger.Infof(ctx, "[conndebug] "+format, args...) + } +}Also applies to: 223-223, 238-238, 241-241, 640-640, 643-643, 647-647, 650-650, 656-656, 659-659
frontend/app/view/term/term.tsx (1)
685-723: LGTM! Consider adding documentation for the debug levels.The implementation of the "Debug Connection" menu item is clean and follows the existing patterns. The debug levels (Off, Info, Verbose) are well-structured with clear metadata values.
Consider adding code comments or documentation to explain:
- The purpose of each debug level
- The expected logging output for each level
- The impact on performance when debugging is enabled
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
cmd/server/main-server.go(2 hunks)cmd/wsh/cmd/wshcmd-conn.go(3 hunks)cmd/wsh/cmd/wshcmd-ssh.go(1 hunks)frontend/app/block/blockframe.tsx(7 hunks)frontend/app/store/keymodel.ts(2 hunks)frontend/app/store/wshclientapi.ts(3 hunks)frontend/app/view/preview/preview.tsx(1 hunks)frontend/app/view/term/term.tsx(1 hunks)frontend/types/gotypes.d.ts(5 hunks)pkg/blockcontroller/blockcontroller.go(5 hunks)pkg/blocklogger/blocklogger.go(1 hunks)pkg/remote/conncontroller/conncontroller.go(16 hunks)pkg/remote/connutil.go(0 hunks)pkg/remote/sshclient.go(8 hunks)pkg/waveobj/metaconsts.go(1 hunks)pkg/waveobj/wtypemeta.go(1 hunks)pkg/wconfig/settingsconfig.go(3 hunks)pkg/wshrpc/wshclient/wshclient.go(3 hunks)pkg/wshrpc/wshrpctypes.go(7 hunks)pkg/wshrpc/wshserver/wshserver.go(8 hunks)pkg/wshutil/wshrpcio.go(2 hunks)pkg/wsl/wsl.go(1 hunks)
💤 Files with no reviewable changes (1)
- pkg/remote/connutil.go
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build for TestDriver.ai
🔇 Additional comments (25)
pkg/remote/conncontroller/conncontroller.go (1)
280-284: Verify command execution safety to prevent command injectionEnsure that the construction of the command string and its execution via
sshSession.Startis secure and not susceptible to command injection attacks. Verify thatgenconn.HardQuoteproperly sanitizescmdStr.As a follow-up, you can run the following script to check the implementation of
genconn.HardQuote:✅ Verification successful
Command execution chain verified as secure
The command construction and execution chain is properly secured against injection:
- Template structure is safe and well-defined
- Path discovery uses proper command execution mechanisms
- All inputs are properly sanitized and shell-quoted
- Comprehensive test coverage verifies the quoting behavior
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that genconn.HardQuote properly sanitizes command strings. # Search for the definition of genconn.HardQuote rg 'func HardQuote' pkg/ # Search for all usages of genconn.HardQuote rg 'genconn\.HardQuote' pkg/Length of output: 227
Script:
#!/bin/bash # Get the implementation of HardQuote cat pkg/genconn/quote.go # Look for tests fd quote_test.go # If test file exists, show its contents cat pkg/genconn/quote_test.go 2>/dev/null || true # Check how cmdStr is constructed by looking at the context rg -B 10 'shWrappedCmdStr :=' pkg/remote/conncontroller/conncontroller.goLength of output: 4252
Script:
#!/bin/bash # Check ConnServerCmdTemplate definition and wshPath handling rg -B 5 'ConnServerCmdTemplate.*=' pkg/remote/conncontroller/conncontroller.go # Look for wshPath construction rg -B 10 'wshPath.*:=' pkg/remote/conncontroller/conncontroller.goLength of output: 754
Script:
#!/bin/bash # Get the complete ConnServerCmdTemplate definition rg -A 10 'ConnServerCmdTemplate.*=' pkg/remote/conncontroller/conncontroller.go # Find and show GetWshPath implementation ast-grep --pattern 'func GetWshPath($$$) $$$' # Show the actual implementation rg -B 5 'func GetWshPath' pkg/Length of output: 4026
cmd/wsh/cmd/wshcmd-ssh.go (1)
42-43: Addition ofLogBlockIdtoConnRequestThe
LogBlockIdfield is correctly added to theConnRequeststruct, ensuring that logging is appropriately tied to the block context.pkg/wshutil/wshrpcio.go (1)
60-63: LGTM! Well-structured type definition.The
LineOutputstruct effectively encapsulates both the line content and potential errors, following Go's idiomatic error handling pattern.pkg/waveobj/metaconsts.go (1)
98-98: LGTM! Follows established constant naming pattern.The new constant
MetaKey_TermConnDebugis well-placed and maintains consistency with other terminal-related metadata keys.cmd/wsh/cmd/wshcmd-conn.go (2)
131-135: LGTM! Proper use of ConnExtData structure.The implementation correctly initializes the ConnExtData structure with both the connection name and log block ID.
197-201: LGTM! Consistent with other implementations.The ConnExtData initialization matches the pattern used in connReinstallRun.
pkg/waveobj/wtypemeta.go (1)
99-99: LGTM! Well-documented field with clear value constraints.The
TermConnDebugfield is properly documented with allowed values (null, info, debug) and follows the established pattern for terminal-related fields.frontend/app/store/keymodel.ts (1)
149-153: LGTM! Clean implementation of delayed refocus.The implementation correctly wraps the existing
globalRefocusfunction with a timeout mechanism.pkg/wshrpc/wshclient/wshclient.go (3)
53-55: LGTM! Parameter type change aligns with interface update.The function signature change from
stringtowshrpc.ConnExtDataenhances the connection management by including additional metadata.
65-67: LGTM! Parameter type change aligns with interface update.The function signature change from
stringtowshrpc.ConnExtDataenhances the connection management by including additional metadata.
76-80: LGTM! New function for controller output management.The new function
ControllerAppendOutputCommandfollows the established pattern for RPC commands and properly handles output data.pkg/wconfig/settingsconfig.go (3)
118-120: LGTM! Enhanced configuration flexibility with pointer types.The change to pointer types for boolean fields allows for better representation of unset states in the configuration.
139-144: LGTM! Well-implemented helper function.The
DefaultBoolPtrfunction provides a clean way to handle nil pointer cases with default values.
317-318: LGTM! Improved documentation.The comment clarifies the intended usage of the function and recommends using the watcher for obtaining current configuration.
pkg/wshrpc/wshrpctypes.go (5)
121-121: LGTM! Interface updates enhance connection management.The interface changes properly reflect the new data structures and maintain consistency across the codebase.
Also applies to: 158-159
315-318: LGTM! Well-structured data type for output management.The new
CommandControllerAppendOutputDatastruct properly encapsulates the required fields for output data management.
468-471: LGTM! Enhanced connection configuration.The addition of
ConnWshPathand pointer types for boolean fields improves connection configuration flexibility.
498-500: LGTM! Improved error handling and logging.The addition of
LogBlockIdandNoWshReasonfields enhances debugging capabilities.Also applies to: 545-545
659-662: LGTM! Well-designed data structure for connection metadata.The new
ConnExtDatastruct properly encapsulates connection name and logging information.pkg/blockcontroller/blockcontroller.go (3)
Line range hint
379-387: Improved error handling for remote shell processes.The error handling has been enhanced to properly set WSH errors and fallback to non-WSH mode.
761-766: LGTM! Clean helper function for log formatting.The
formatConnNameForLogfunction provides consistent connection name formatting for logs.
794-796: LGTM! Enhanced logging for connection changes.The addition of detailed logging for connection changes improves debugging capabilities.
pkg/wshrpc/wshserver/wshserver.go (1)
22-22: Well-structured logging enhancements.The addition of the
blockloggerpackage and the new logging context mechanism provides better traceability for connection operations. The implementation is clean and maintainable.Also applies to: 241-241, 614-627
pkg/remote/sshclient.go (1)
81-88: Well-structured error message extraction.The
SimpleMessageFromPossibleConnectionErrorfunction effectively handles error message extraction. Consider adding a test case to verify the behavior.frontend/app/view/preview/preview.tsx (1)
367-367: LGTM! Parameter structure updated for consistency.The change to use a structured parameter for
connnameinConnEnsureCommandaligns with the broader refactoring effort across the codebase.
| conn.Infof(ctx, "normalized knownhosts address: %s\n", fmtAddr) | ||
| clientDisplayName := fmt.Sprintf("%s (%s)", conn.GetName(), fmtAddr) |
There was a problem hiding this comment.
Avoid logging sensitive information such as client addresses
Logging the normalized known hosts address, which includes the username and remote address, may expose sensitive information in logs. Consider removing or obfuscating this information in log messages to enhance security.
Apply this diff to modify the log message:
- conn.Infof(ctx, "normalized knownhosts address: %s\n", fmtAddr)
+ conn.Infof(ctx, "client connected: %s\n", conn.GetName())📝 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.
| conn.Infof(ctx, "normalized knownhosts address: %s\n", fmtAddr) | |
| clientDisplayName := fmt.Sprintf("%s (%s)", conn.GetName(), fmtAddr) | |
| conn.Infof(ctx, "client connected: %s\n", conn.GetName()) | |
| clientDisplayName := fmt.Sprintf("%s (%s)", conn.GetName(), fmtAddr) |
pkg/blocklogger/blocklogger.go
Outdated
| func InitBlockLogger() { | ||
| outputChan = make(chan wshrpc.CommandControllerAppendOutputData, outputBufferSize) | ||
| // Start the output runner | ||
| go outputRunner() | ||
| } |
There was a problem hiding this comment.
Prevent multiple initializations of InitBlockLogger
Calling InitBlockLogger multiple times could start multiple outputRunner goroutines and lead to race conditions. Consider adding a guard to ensure InitBlockLogger is only executed once.
Apply this diff to add a sync.Once guard:
+import (
+ // ...
+ "sync"
+)
+var initOnce sync.Once
func InitBlockLogger() {
+ initOnce.Do(func() {
outputChan = make(chan wshrpc.CommandControllerAppendOutputData, outputBufferSize)
// Start the output runner
go outputRunner()
+ })
}Committable suggestion skipped: line range outside the PR's diff.
pkg/wshutil/wshrpcio.go
Outdated
| func ReadLineWithTimeout(ch chan LineOutput, timeout time.Duration) (string, error) { | ||
| select { | ||
| case output := <-ch: | ||
| if output.Error != nil { | ||
| return "", output.Error | ||
| } | ||
| return output.Line, nil | ||
| case <-time.After(timeout): | ||
| return "", context.DeadlineExceeded | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Potential resource leak in timeout scenario.
When timeout occurs, the goroutine reading from the channel continues running. Consider adding context cancellation to clean up resources.
-func ReadLineWithTimeout(ch chan LineOutput, timeout time.Duration) (string, error) {
+func ReadLineWithTimeout(ctx context.Context, ch chan LineOutput, timeout time.Duration) (string, error) {
+ ctx, cancel := context.WithTimeout(ctx, timeout)
+ defer cancel()
select {
case output := <-ch:
if output.Error != nil {
return "", output.Error
}
return output.Line, nil
- case <-time.After(timeout):
+ case <-ctx.Done():
return "", context.DeadlineExceeded
}
}📝 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 ReadLineWithTimeout(ch chan LineOutput, timeout time.Duration) (string, error) { | |
| select { | |
| case output := <-ch: | |
| if output.Error != nil { | |
| return "", output.Error | |
| } | |
| return output.Line, nil | |
| case <-time.After(timeout): | |
| return "", context.DeadlineExceeded | |
| } | |
| } | |
| func ReadLineWithTimeout(ctx context.Context, ch chan LineOutput, timeout time.Duration) (string, error) { | |
| ctx, cancel := context.WithTimeout(ctx, timeout) | |
| defer cancel() | |
| select { | |
| case output := <-ch: | |
| if output.Error != nil { | |
| return "", output.Error | |
| } | |
| return output.Line, nil | |
| case <-ctx.Done(): | |
| return "", context.DeadlineExceeded | |
| } | |
| } |
cmd/server/main-server.go
Outdated
| @@ -297,6 +298,7 @@ func main() { | |||
| go stdinReadWatch() | |||
| go telemetryLoop() | |||
| configWatcher() | |||
| blocklogger.InitBlockLogger() | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for block logger initialization.
The InitBlockLogger() call should include error handling to ensure proper initialization and graceful failure handling.
Apply this diff:
-blocklogger.InitBlockLogger()
+if err := blocklogger.InitBlockLogger(); err != nil {
+ log.Printf("error initializing block logger: %v\n", err)
+ return
+}📝 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.
| blocklogger.InitBlockLogger() | |
| if err := blocklogger.InitBlockLogger(); err != nil { | |
| log.Printf("error initializing block logger: %v\n", err) | |
| return | |
| } |
| @@ -769,6 +775,7 @@ func ResyncController(ctx context.Context, tabId string, blockId string, rtOpts | |||
| } | |||
| if force { | |||
| StopBlockController(blockId) | |||
| time.Sleep(100 * time.Millisecond) // TODO see if we can remove this (the "process finished with exit code" message comes out after we start reconnecting otherwise) | |||
There was a problem hiding this comment.
💡 Codebase verification
Sleep statements should be replaced with proper synchronization mechanism
The sleep statements are used as a workaround for message ordering between process termination and reconnection. A more reliable solution would be to use the existing ShellProc.DoneCh channel to properly synchronize these events.
pkg/blockcontroller/blockcontroller.go:778: Replace sleep with proper synchronization usingShellProc.DoneChpkg/blockcontroller/blockcontroller.go:797: Replace sleep with proper synchronization usingShellProc.DoneCh
🔗 Analysis chain
Verify the necessity of sleep statements.
The sleep statements are added to ensure proper process message ordering, but they might introduce unnecessary delays.
Run the following script to check for potential race conditions or message ordering issues:
Also applies to: 797-797
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for similar sleep patterns in the codebase
# to understand if this is a common pattern or if there might be
# a better synchronization mechanism.
rg -A 5 "time.Sleep.*Millisecond.*process.*finished"
Length of output: 1186
Script:
#!/bin/bash
# Search for process exit handling and connection management
echo "=== Process Exit Handling ==="
rg -B 2 -A 2 "process.*finished.*exit.*code" --type go
echo -e "\n=== Connection Management ==="
rg -B 2 -A 2 "reconnect|CheckConnStatus" --type go
echo -e "\n=== Channel Usage (for potential sync mechanisms) ==="
ast-grep --pattern 'chan $_'
Length of output: 17267
No description provided.