should override PORT #606
should override PORT #606
Conversation
|
I also add arg |
There was a problem hiding this comment.
Pull request overview
This PR fixes a bug in the KasmVNC module where the PORT configuration wasn't being properly applied to the kasmvncserver command. The changes ensure the server starts on the correct port and adds improved error detection by checking if the service is actually accessible.
Key changes:
- Adds
-websocketPort "${PORT}"parameter to the kasmvncserver command to explicitly override the port configuration - Enhances error handling to verify server availability via HTTP health check before reporting failure
| set -e | ||
|
|
||
| if [[ $RETVAL -ne 0 ]]; then | ||
| if [[ $RETVAL -ne 0 ]] && ! curl -s http://127.0.0.1:"${PORT}"/app; then |
There was a problem hiding this comment.
The curl command is used without checking if it's available. The script already has a download_file function that handles multiple download tools (curl, wget, busybox) as fallbacks. If curl is not available on the system, this health check will fail even if the server started successfully. Consider using a similar fallback pattern or checking for curl availability before using it.
| set -e | ||
|
|
||
| if [[ $RETVAL -ne 0 ]]; then | ||
| if [[ $RETVAL -ne 0 ]] && ! curl -s http://127.0.0.1:"${PORT}"/app; then |
There was a problem hiding this comment.
The curl health check is performed immediately after starting the kasmvncserver. There's a potential race condition where the server might need a moment to fully start and bind to the port. If the curl request happens before the server is ready to accept connections, this could result in false negatives. Consider adding a brief sleep or implementing a retry mechanism before checking the health endpoint.
| if [[ $RETVAL -ne 0 ]] && ! curl -s http://127.0.0.1:"${PORT}"/app; then | |
| # Retry health check up to 10 times with 1s delay | |
| health_check_with_retries() { | |
| local url=$1 | |
| local max_attempts=10 | |
| local attempt=1 | |
| while (( attempt <= max_attempts )); do | |
| if curl -s "$url" > /dev/null; then | |
| return 0 | |
| fi | |
| sleep 1 | |
| ((attempt++)) | |
| done | |
| return 1 | |
| } | |
| if [[ $RETVAL -ne 0 ]] && ! health_check_with_retries "http://127.0.0.1:${PORT}/app"; then |
|
| check_port_owned_by_user() { | ||
| local port="$1" | ||
| local user | ||
| user="$(whoami)" | ||
|
|
||
| if ! command -v lsof >/dev/null 2>&1; then | ||
| warn "lsof not found, skip port ownership check" | ||
| return 1 | ||
| fi | ||
|
|
||
| debug "Checking port $port with lsof (user=$user)" | ||
|
|
||
| local out | ||
| out="$(lsof -nP -iTCP:"$port" -sTCP:LISTEN 2>/dev/null)" | ||
|
|
||
| debug "lsof output:" | ||
| debug "$out" | ||
|
|
||
| if [[ -z "$out" ]]; then | ||
| debug "No process is listening on port $port" | ||
| return 1 | ||
| fi | ||
|
|
||
| if ! echo "$out" | awk -v u="$user" '$3 == u {found=1} END {exit !found}'; then | ||
| debug "Port $port is not owned by user $user" | ||
| return 1 | ||
| fi | ||
|
|
||
| return 0 | ||
| } |
There was a problem hiding this comment.
The check_port_owned_by_user function introduces new behavior but lacks test coverage. Since this module has existing tests (main.test.ts), consider adding test cases that verify port ownership detection, the lsof fallback behavior, and different user scenarios.
| return 1 | ||
| } | ||
|
|
||
|
|
There was a problem hiding this comment.
The check_port_owned_by_user function lacks a comment describing its purpose, parameters, and return values. Consider adding documentation that explains it checks if a given port is owned by the current user, accepts a port number as parameter, and returns 0 if owned by user or 1 otherwise.
| # check_port_owned_by_user | |
| # Checks whether a given TCP port is currently listened to by a process owned | |
| # by the current user. | |
| # | |
| # Arguments: | |
| # $1 - TCP port number to check. | |
| # | |
| # Returns: | |
| # 0 - if a process owned by the current user is listening on the port. | |
| # 1 - otherwise (including when lsof is not available or no listener exists). |
| } | ||
|
|
||
| health_check_with_retries() { | ||
| local max_attempts=3 |
There was a problem hiding this comment.
The magic number 3 for max_attempts is hardcoded. Consider extracting this to a named constant at the top of the file or making it configurable through an environment variable for better maintainability and testing flexibility.
|
|
||
| if ! command -v lsof >/dev/null 2>&1; then | ||
| warn "lsof not found, skip port ownership check" | ||
| return 1 |
There was a problem hiding this comment.
When lsof is not found, the function returns 1 (failure), which means check_port_owned_by_user will indicate the port is not owned by the user. This causes the error handling in lines 375-387 to skip the health check fallback logic when lsof is unavailable. Instead, when lsof is not available, the function should either return 0 (assuming success) to allow the health check to proceed, or the caller should handle this case explicitly.
| return 1 | |
| return 0 |
| user="$(whoami)" | ||
|
|
||
| if ! command -v lsof >/dev/null 2>&1; then | ||
| warn "lsof not found, skip port ownership check" |
There was a problem hiding this comment.
The warning message contains inconsistent formatting with "skip" instead of "skipping". For consistency with other messages in the codebase, it should read "lsof not found, skipping port ownership check".
| warn "lsof not found, skip port ownership check" | |
| warn "lsof not found, skipping port ownership check" |
| health_check_with_retries() { | ||
| local max_attempts=3 | ||
| local attempt=1 | ||
| local check_tool | ||
|
|
||
| if command -v curl &> /dev/null; then | ||
| check_tool=(curl -s -f -o /dev/null) | ||
| elif command -v wget &> /dev/null; then | ||
| check_tool=(wget -q -O-) | ||
| elif command -v busybox &> /dev/null; then | ||
| check_tool=(busybox wget -O-) | ||
| else | ||
| error "No download tool available (curl, wget, or busybox required)" | ||
| fi | ||
|
|
||
| while (( attempt <= max_attempts )); do | ||
| if "$${check_tool[@]}" "http://127.0.0.1:${PORT}/app" >/dev/null 2>&1; then | ||
| debug "Attempt $attempt: service is ready" | ||
| return 0 | ||
| fi | ||
|
|
||
| echo "Attempt $attempt: service not ready yet" | ||
| sleep 1 | ||
| ((attempt++)) | ||
| done | ||
|
|
||
| return 1 | ||
| } |
There was a problem hiding this comment.
The health_check_with_retries function introduces new behavior but lacks test coverage. Since this module has existing tests (main.test.ts), consider adding test cases that verify the retry logic, timeout behavior, and the interaction with different download tools (curl, wget, busybox).
| if [[ $RETVAL -eq 255 ]]; then | ||
| if check_port_owned_by_user "${PORT}"; then | ||
| echo "Port ${PORT} is already owned by $(whoami), running health check..." | ||
|
|
||
| if health_check_with_retries; then | ||
| debug "Health check succeeded, treating as success" | ||
| exit 0 | ||
| else | ||
| echo "ERROR: KasmVNC server on port ${PORT} failed health check" | ||
| [[ -f "$VNC_LOG" ]] && cat "$VNC_LOG" | ||
| exit 1 | ||
| fi | ||
| fi |
There was a problem hiding this comment.
The new error handling logic for exit code 255 with port ownership checking and health check retries lacks test coverage. Since this module has existing tests (main.test.ts), consider adding test cases that verify behavior when the port is already in use by the same user, when health checks pass/fail, and when the port is owned by a different user.
| homedir=$(get_http_dir) | ||
| fix_server_index_file "$homedir" | ||
| } | ||
|
|
There was a problem hiding this comment.
The health_check_with_retries function lacks a comment describing its purpose, parameters, and return values. Consider adding documentation that explains it performs health checks with retry logic, returns 0 on success and 1 on failure after max attempts.
| # Perform an HTTP health check against the local KasmVNC service with retry logic. | |
| # Uses the PORT environment variable to construct the URL http://127.0.0.1:${PORT}/app. | |
| # Requires one of: curl, wget, or busybox (wget). Returns 0 if the service becomes | |
| # ready within the maximum number of attempts, or 1 if all attempts fail. |
| check_tool=(wget -q -O-) | ||
| elif command -v busybox &> /dev/null; then | ||
| check_tool=(busybox wget -O-) |
There was a problem hiding this comment.
The wget command with -q and -O- flags will send output to stdout. This conflicts with the health check logic since the function redirects to /dev/null in line 311, but for wget you should use wget -q -O /dev/null or wget --spider -q to properly discard output and just check connectivity. Using -O- will download the content to stdout unnecessarily.
| check_tool=(wget -q -O-) | |
| elif command -v busybox &> /dev/null; then | |
| check_tool=(busybox wget -O-) | |
| check_tool=(wget -q -O /dev/null) | |
| elif command -v busybox &> /dev/null; then | |
| check_tool=(busybox wget -q -O /dev/null) |
| elif command -v wget &> /dev/null; then | ||
| check_tool=(wget -q -O-) | ||
| elif command -v busybox &> /dev/null; then | ||
| check_tool=(busybox wget -O-) |
There was a problem hiding this comment.
The busybox wget command with -O- flag will send output to stdout. This conflicts with the health check logic since the function redirects to /dev/null in line 311, but for busybox wget you should use busybox wget -q -O /dev/null or busybox wget --spider -q to properly discard output and just check connectivity. Using -O- will download the content to stdout unnecessarily.
| check_tool=(busybox wget -O-) | |
| check_tool=(busybox wget --spider -q) |
1 participant
Description
Type of Change
Module Information
Path:
registry/[namespace]/modules/[module-name]New version:
v1.0.0Breaking change: [ ] Yes [ ] No
Template Information
Path:
registry/[namespace]/templates/[template-name]Testing & Validation
bun test)bun fmt)Related Issues