Skip to content

should override PORT #606

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
IamTaoChen wants to merge 17 commits into
base: main
Choose a base branch
from
+93 −8
Open

should override PORT #606

Changes from all commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
50298c6
should override PORT
IamTaoChen Dec 15, 2025
ed54aa9
add -autokill
IamTaoChen Dec 15, 2025
d9807e0
rm autokill
IamTaoChen Dec 15, 2025
1e78f97
use curl check again
IamTaoChen Dec 15, 2025
cf3fbed
add health_check_with_retries and check_port_owned_by_user
IamTaoChen Dec 17, 2025
582adaa
fix %{http_code}
IamTaoChen Dec 17, 2025
469f1cd
fix $PORT
IamTaoChen Dec 17, 2025
6bc4558
fix check_port_owned_by_user
IamTaoChen Dec 17, 2025
e39a012
debug
IamTaoChen Dec 17, 2025
484936a
test
IamTaoChen Dec 17, 2025
b88d30a
test
IamTaoChen Dec 17, 2025
9561764
fix
IamTaoChen Dec 17, 2025
97d3340
format warn and debug
IamTaoChen Dec 17, 2025
d59ed92
rewrite health_check_with_retries
IamTaoChen Dec 17, 2025
ec95aae
fix
IamTaoChen Dec 17, 2025
24919cf
cleanup
IamTaoChen Dec 17, 2025
b26906f
Merge branch 'main' into main
IamTaoChen Dec 17, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
101 changes: 93 additions & 8 deletions registry/coder/modules/kasmvnc/run.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,22 @@
#!/usr/bin/env bash

# DEBUG=1
set -eo pipefail

error() {
printf "💀 ERROR: %s\n" "$@"
printf "💀 ERROR: %s\n" "$*" >&2
exit 1
}

warn() {
printf "⚠️ WARN: %s\n" "$*" >&2
}

debug() {
[[ "$${DEBUG:-0}" == "1" ]] || return 0
printf "🐛 DEBUG: %s\n" "$*" >&2
}

# Function to check if KasmVNC is already installed
check_installed() {
if command -v kasmvncserver &> /dev/null; then
Expand Down Expand Up @@ -282,6 +292,68 @@ patch_kasm_http_files() {
fix_server_index_file "$homedir"
}

Copy link

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
# 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.

Copilot uses AI. Check for mistakes.
health_check_with_retries() {
local max_attempts=3
Copy link

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
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-)
Comment on lines +303 to +305
Copy link

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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)

Copilot uses AI. Check for mistakes.
Copy link

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
check_tool=(busybox wget -O-)
check_tool=(busybox wget --spider -q)

Copilot uses AI. Check for mistakes.
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
}
Comment on lines +295 to +322
Copy link

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copilot uses AI. Check for mistakes.


Copy link

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
# 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).

Copilot uses AI. Check for mistakes.
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"
Copy link

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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".

Suggested change
warn "lsof not found, skip port ownership check"
warn "lsof not found, skipping port ownership check"

Copilot uses AI. Check for mistakes.
return 1
Copy link

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
return 1
return 0

Copilot uses AI. Check for mistakes.
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
}
Comment on lines +325 to +354
Copy link

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.


if [[ "${SUBDOMAIN}" == "false" ]]; then
echo "🩹 Patching up webserver files to support path-sharing..."
patch_kasm_http_files
Expand All @@ -292,18 +364,31 @@ VNC_LOG="/tmp/kasmvncserver.log"
printf "🚀 Starting KasmVNC server...\n"

set +e
kasmvncserver -select-de "${DESKTOP_ENVIRONMENT}" -disableBasicAuth > "$VNC_LOG" 2>&1
kasmvncserver -select-de "${DESKTOP_ENVIRONMENT}" -websocketPort "${PORT}" -disableBasicAuth > "$VNC_LOG" 2>&1
RETVAL=$?
set -e

if [[ $RETVAL -ne 0 ]]; then
echo "ERROR: Failed to start KasmVNC server. Return code: $RETVAL"
if [[ -f "$VNC_LOG" ]]; then
echo "Full logs:"
cat "$VNC_LOG"
else
echo "ERROR: Log file not found: $VNC_LOG"

debug "KasmVNC error code: $RETVAL"

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
Comment on lines +375 to +387
Copy link

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
fi

echo "ERROR: Failed to start KasmVNC server. Return code: $RETVAL"
[[ -f "$VNC_LOG" ]] && cat "$VNC_LOG"
exit 1
fi

Expand Down