fix(tui): honor active Gateway runtime port in TUI connection#42496
fix(tui): honor active Gateway runtime port in TUI connection#42496fuleinist wants to merge 4 commits into
Conversation
Fixes openclaw#42461 - Apply applyGatewayRuntimePortEnvOverride() before resolving TUI Gateway URL - Ensures TUI connects to the correct port when Gateway runs on custom port - Added port field to gateway lock payload for runtime port discovery
Greptile SummaryThis PR fixes the TUI connecting to the wrong port when the Gateway is started with a custom Key changes:
Issues found:
Confidence Score: 3/5
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| export async function applyGatewayRuntimePortEnvOverride( | ||
| env: NodeJS.ProcessEnv = process.env, | ||
| ): Promise<void> { | ||
| // Skip if already set | ||
| if (env.OPENCLAW_GATEWAY_PORT?.trim()) { | ||
| return; | ||
| } | ||
|
|
||
| // Dynamic import to avoid circular dependency | ||
| const { readGatewayLockPayload } = await import("../infra/gateway-lock.js"); | ||
| const payload = await readGatewayLockPayload(env); | ||
|
|
||
| if (payload?.port && Number.isFinite(payload.port) && payload.port > 0) { | ||
| env.OPENCLAW_GATEWAY_PORT = String(payload.port); | ||
| } | ||
| } |
There was a problem hiding this comment.
No liveness validation before applying the port override
applyGatewayRuntimePortEnvOverride reads the port from the lock file without verifying the process recorded in the lock (payload.pid) is actually still alive. If the gateway process crashes without releasing the lock (e.g., SIGKILL), this function will set OPENCLAW_GATEWAY_PORT to the stale port, causing the TUI to try connecting to a port where nothing is listening, instead of falling back to the configured/default port where a freshly started gateway might be running.
Consider checking isPidAlive(payload.pid) (already exported from ../shared/pid-alive.js) before applying the port override:
import { isPidAlive } from "../shared/pid-alive.js";
// inside applyGatewayRuntimePortEnvOverride, after reading payload:
if (
payload?.port &&
Number.isFinite(payload.port) &&
payload.port > 0 &&
typeof payload.pid === "number" &&
isPidAlive(payload.pid)
) {
env.OPENCLAW_GATEWAY_PORT = String(payload.port);
}Note: this would require re-exporting or exposing LockPayload.pid from readGatewayLockPayload, which is already returned in the current implementation.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/config/paths.ts
Line: 298-313
Comment:
**No liveness validation before applying the port override**
`applyGatewayRuntimePortEnvOverride` reads the port from the lock file without verifying the process recorded in the lock (`payload.pid`) is actually still alive. If the gateway process crashes without releasing the lock (e.g., `SIGKILL`), this function will set `OPENCLAW_GATEWAY_PORT` to the stale port, causing the TUI to try connecting to a port where nothing is listening, instead of falling back to the configured/default port where a freshly started gateway might be running.
Consider checking `isPidAlive(payload.pid)` (already exported from `../shared/pid-alive.js`) before applying the port override:
```ts
import { isPidAlive } from "../shared/pid-alive.js";
// inside applyGatewayRuntimePortEnvOverride, after reading payload:
if (
payload?.port &&
Number.isFinite(payload.port) &&
payload.port > 0 &&
typeof payload.pid === "number" &&
isPidAlive(payload.pid)
) {
env.OPENCLAW_GATEWAY_PORT = String(payload.port);
}
```
Note: this would require re-exporting or exposing `LockPayload.pid` from `readGatewayLockPayload`, which is already returned in the current implementation.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
This has been addressed. The current code in applyGatewayRuntimePortEnvOverride now checks isPidAlive(payload.pid) before applying the port, and additionally verifies that payload.startTime matches the current process start time (via readLinuxStartTime) to guard against PID reuse. This was added in commit ea8d038.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5520a6c553
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (env.OPENCLAW_GATEWAY_PORT?.trim()) { | ||
| return; |
There was a problem hiding this comment.
applyGatewayRuntimePortEnvOverride only treats OPENCLAW_GATEWAY_PORT as an existing override, so when a user sets the still-supported legacy CLAWDBOT_GATEWAY_PORT, this function will still read the lock file and write OPENCLAW_GATEWAY_PORT, which then takes precedence in resolveGatewayPort. In that environment, TUI can connect to the lock-file port instead of the explicitly configured legacy env port, which is a regression from prior behavior where CLAWDBOT_GATEWAY_PORT was respected.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This has been addressed. The check now includes CLAWDBOT_GATEWAY_PORT alongside OPENCLAW_GATEWAY_PORT so that legacy env var users are not overridden by the lock file port. Added in commit ea8d038.
…PID in TUI port override - In acquireGatewayLock, pass lastPayload?.port (existing gateway's port) to resolveGatewayOwnerStatus instead of opts.port (new gateway's port). This prevents incorrectly declaring a live gateway as dead when two gateways target different ports. - In applyGatewayRuntimePortEnvOverride, verify the PID is still alive before applying the port from the lock file. This prevents using a stale lock from a crashed gateway. - Update tests to reflect correct behavior: lock files now include port field, and liveness checks use the lock holder's port. Fixes review feedback from PR openclaw#42496.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f7b538fa25
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| return null; | ||
| } | ||
| const startTime = typeof parsed.startTime === "number" ? parsed.startTime : undefined; | ||
| const port = typeof parsed.port === "number" ? parsed.port : undefined; |
There was a problem hiding this comment.
readLockPayload now accepts any numeric port from disk, and acquireGatewayLock passes that value into resolveGatewayOwnerStatus, which probes it via net.createConnection. If a stale/corrupted lock contains an out-of-range port (for example 70000 or -1), Node throws ERR_SOCKET_BAD_PORT, and that exception bubbles out of lock acquisition instead of falling back to normal stale-lock handling. This can block gateway startup until the lock file is manually deleted, so the parsed port should be range-validated (1-65535) or ignored when invalid.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This has been addressed. readLockPayload now validates port range: the port must be a finite number, an integer, greater than 0, and <= 65535 before it is stored in the parsed payload. Out-of-range ports (e.g., 70000, -1) are treated as missing. Added in commit ea8d038.
Review Feedback Addressed ✅The initial Greptile review comments have been addressed in commit
Additional Suggestions from Codex ReviewThere are two remaining issues from the Codex review that could be addressed: 1. Preserve legacy
|
|
I can implement these two follow-up tweaks, but I can't push to your fork (0xsline doesn't have permission to If you want, you can cherry-pick this commit onto your branch:
Diff touches:
If you'd like me to push it, enable “Allow edits from maintainers” (or push from the PR author account). |
Two review fixes for PR openclaw#42496: 1. readLockPayload: validate port range (1-65535) before returning. Prevents ERR_SOCKET_BAD_PORT when a stale/corrupted lock contains an out-of-range port. 2. applyGatewayRuntimePortEnvOverride: also check legacy CLAWDBOT_GATEWAY_PORT env var when deciding whether to apply the lock file port override, preserving backward compatibility. Co-Authored-By: Claude Opus 4.6 <[email protected]>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ea8d038128
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (isPidAlive(payload.pid)) { | ||
| env.OPENCLAW_GATEWAY_PORT = String(payload.port); |
There was a problem hiding this comment.
applyGatewayRuntimePortEnvOverride treats a lock as current if isPidAlive(payload.pid) is true, but that does not protect against PID reuse. If a stale lock remains and the PID has been recycled by an unrelated process, this code still writes OPENCLAW_GATEWAY_PORT from stale data and resolveGatewayConnection can target the wrong port. The lock payload already carries startTime for anti-reuse checks in src/infra/gateway-lock.ts, so this path should validate owner identity (start time/cmdline) before trusting the runtime-port override.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This has been addressed. The current code in applyGatewayRuntimePortEnvOverride now reads the lock payload and verifies that payload.startTime matches the current process start time via readLinuxStartTime before applying the port. If the PID has been recycled (startTime mismatch), the function returns early without setting the stale port. Added in commit 8ed5e99.
| typeof parsed.port === "number" && | ||
| Number.isFinite(parsed.port) && | ||
| parsed.port > 0 && | ||
| parsed.port <= 65535 |
There was a problem hiding this comment.
The new lock payload parser accepts any finite number in range, so a malformed lock with "port": 18789.5 is treated as valid. Later, resolveGatewayOwnerStatus passes that value to net.createConnection, which throws ERR_SOCKET_BAD_PORT for non-integer ports and aborts lock acquisition instead of falling back to stale-lock handling. Treat non-integer port values as invalid (for example with Number.isInteger) when parsing the lock payload.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This has been addressed. The port validation in readLockPayload now includes Number.isInteger(parsed.port) — a port value like 18789.5 is now treated as invalid and discarded, preventing ERR_SOCKET_BAD_PORT exceptions during lock acquisition. Added in commit 8ed5e99.
…non-integer ports
- Guard applyGatewayRuntimePortEnvOverride against PID reuse: when a lock
carries startTime, compare it against the current process start time
read from /proc/{pid}/stat before trusting the runtime-port override.
If the times differ the lock is stale (PID recycled) and is ignored.
- Add Number.isInteger check in readLockPayload so malformed locks with
fractional ports (e.g. 18789.5) are treated as missing, preventing
ERR_SOCKET_BAD_PORT from net.createConnection during owner probes.
- Export readLinuxStartTime so it can be used by paths.ts for the
startTime verification above.
Addresses chatgpt-codex-connector[bot] P2 comments on openclaw#42496.
Co-Authored-By: Claude Opus 4.6 <[email protected]>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8ed5e99fb1
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (payload.startTime !== undefined) { | ||
| const currentStartTime = readLinuxStartTime(payload.pid); | ||
| if (currentStartTime !== payload.startTime) { | ||
| return; // Stale lock — PID was recycled | ||
| } |
There was a problem hiding this comment.
applyGatewayRuntimePortEnvOverride still trusts any live PID when payload.startTime is absent, and then sets OPENCLAW_GATEWAY_PORT from the lock; this can misroute TUI connections on macOS/Windows if a stale lock's PID has been recycled by an unrelated process. Fresh evidence: acquireGatewayLock only records startTime on Linux (src/infra/gateway-lock.ts, platform check around lock payload creation), so the new PID-reuse guard is skipped on non-Linux and stale lock ports can still be applied.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Acknowledged. On Linux this is fully addressed via the startTime verification (commit 8ed5e99). On macOS/Windows where startTime is not recorded in the lock, I proposed an additional guard using readLinuxCmdline + isGatewayArgv to verify the recycled PID still points to a gateway process — this would eliminate the remaining window where a stale lock could misroute a TUI connection on non-Linux platforms. However, I do not have push access to this repository, so I cannot commit this fix directly. Please consider applying it manually or granting push access.
|
This pull request has been automatically marked as stale due to inactivity. |
|
Codex automated review: keeping this open. Keep this PR open. Current main still does not let Best possible solution: Review and finish this PR rather than close it. The likely path is to keep the lock-file runtime-port design, persist a validated active port in the Gateway lock payload, apply it before TUI builds connection details, add the 48789 regression test, and share or extend the existing Gateway owner-identity checks so stale locks are not trusted on macOS/Windows. What I checked:
Remaining risk / open question:
Codex Review notes: model gpt-5.5, reasoning high; reviewed against aac83e00cfe7. |
|
staled |
3 participants
Summary
Fixes #42461
The TUI connection path now applies the active Gateway runtime port override before resolving the connection target. This ensures that when the Gateway is running on a custom port (e.g., 48789), the TUI automatically connects to that port instead of defaulting to 18789.
Changes
portfield to the gateway lock payload insrc/infra/gateway-lock.tsapplyGatewayRuntimePortEnvOverride()function insrc/config/paths.tsto read the lock file and setOPENCLAW_GATEWAY_PORTenv varapplyGatewayRuntimePortEnvOverride()at the start ofresolveGatewayConnection()insrc/tui/gateway-chat.tsTesting
Fixes #42461