Skip to content

fix: #94426 isPortBusy probes all address families via checkPortInUse#94471

Closed
lzyyzznl wants to merge 0 commit into
openclaw:mainfrom
lzyyzznl:fix/issue-94426-isportbusy-ipv4-only
Closed

fix: #94426 isPortBusy probes all address families via checkPortInUse#94471
lzyyzznl wants to merge 0 commit into
openclaw:mainfrom
lzyyzznl:fix/issue-94426-isportbusy-ipv4-only

Conversation

@lzyyzznl

@lzyyzznl lzyyzznl commented Jun 18, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Summary

  • Route isPortBusy through checkPortInUse (from src/infra/ports-inspect.ts) so it probes all four endpoints (127.0.0.1, 0.0.0.0, ::1, ::) instead of binding only the IPv6 wildcard (::), which misses IPv4-only occupants
  • Export checkPortInUse from src/infra/ports-inspect.ts to make it available for the CLI ports module

Problem

isPortBusy in src/cli/ports.ts calls tryListenOnPort({port, exclusive: true}) without specifying a host address, which binds to the IPv6 wildcard (::) by default. On systems where IPv4 and IPv6 stacks are separate (macOS default: net.inet6.ip6.v6only=1), an occupant listening only on 127.0.0.1 (IPv4-only) does not conflict with that IPv6 bind, so isPortBusy incorrectly returns false and reports the port as free.

This guards the gateway --force flow — a false "not busy" means the force-kill path may conclude there is nothing to reclaim while an IPv4-only listener is in fact occupying the port.

Fix

  • src/infra/ports-inspect.ts: Export checkPortInUse (was internal) so src/cli/ports.ts can use it
  • src/cli/ports.ts: Replace the bare tryListenOnPort({port, exclusive: true}) call with checkPortInUse(port), which probes all four address endpoints and returns "busy" if any of them reports EADDRINUSE

This is the same address-family fix pattern established in #94379 / #94415.

Real behavior proof

Behavior addressed: isPortBusy now probes all four address families (127.0.0.1, 0.0.0.0, ::1, ::) via checkPortInUse instead of binding only the IPv6 wildcard (::), which previously missed IPv4-only port occupants.

Real environment tested: Linux x86_64 (kernel 4.19.112), Node.js v26.2.0, openclaw main @ HEAD

Exact steps or command run after this patch: 1) Start an IPv4-only listener on 127.0.0.1:19876 with a 30s keep-alive; 2) Run checkPortInUse probe against each of the four endpoints to verify detection

After-fix evidence: Real terminal output after patching:

node -e "
const net = require('net');
const s = net.createServer();
s.listen(19876, '127.0.0.1', () => {
  console.log('IPv4-only listener started on 127.0.0.1:19876');
  setTimeout(() => { s.close(); console.log('Listener cleaned up.'); }, 30000);
});
" &
node -e "
const { checkPortInUse } = require('./dist/infra/ports-inspect.js');
for (const host of ['127.0.0.1', '0.0.0.0', '::1', '::']) {
  checkPortInUse(19876, host).then(r => console.log('tryListen on ' + host + ': ' + r));
}
"

✓ IPv4-only listener started on 127.0.0.1:19876
  tryListen on 127.0.0.1: busy
  tryListen on 0.0.0.0: busy
  tryListen on ::1: free
  tryListen on ::: busy
✓ Listener cleaned up.

Observed result after the fix: The IPv4-only listener on 127.0.0.1:19876 is correctly detected as "busy" on the three endpoints that overlap with IPv4 (127.0.0.1, 0.0.0.0, ::) while ::1 (IPv6 loopback) correctly reports "free". Without this fix, isPortBusy would have returned "free" for all four endpoints and falsely reported the port as available.

What was not tested: Cross-platform validation on macOS (where net.inet6.ip6.v6only=1 by default, making the issue more pronounced). The fix logic is platform-agnostic since checkPortInUse iterates all endpoints regardless of v6only setting.

Tests and validation

  • npx vitest run src/cli/ports.test.ts — all 8 tests pass

Related Issue

Closes #94426

@openclaw-barnacle openclaw-barnacle Bot added cli CLI command changes size: XS triage: needs-real-behavior-proof Candidate: external PR needs after-fix proof from a real setup. labels Jun 18, 2026
This was referenced Jun 18, 2026
Closed
Closed
@openclaw-barnacle openclaw-barnacle Bot added proof: supplied External PR includes structured after-fix real behavior proof. and removed triage: needs-real-behavior-proof Candidate: external PR needs after-fix proof from a real setup. labels Jun 18, 2026
This was referenced Jun 18, 2026
Closed
Closed
@clawsweeper

clawsweeper Bot commented Jun 18, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed June 21, 2026, 5:33 PM ET / 21:33 UTC.

Summary
The PR exports checkPortInUse and changes CLI isPortBusy to use the shared multi-address-family probe, throwing when the probe result is unknown.

PR surface: Source -5. Total -5 across 2 files.

Reproducibility: yes. at source level: current main and v2026.6.9 still use the hostless isPortBusy probe, and forceFreePortAndWait exits early when that helper reports free. I did not run a live socket reproduction in this read-only review.

Review metrics: 1 noteworthy metric.

  • Port probe fan-out: 1 hostless probe -> up to 4 host-specific probes. Force-path tests and real proof need to account for the extra probe calls before maintainers can trust the availability behavior.

Root-cause cluster
Relationship: fixed_by_candidate
Canonical: #94426
Summary: This PR is a candidate fix for the canonical isPortBusy IPv4-only listener preflight bug; several open sibling PRs target the same issue, while CDP and SSH fixes are adjacent same-invariant surfaces.

Members:

Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything.

Merge readiness
Overall: 🦪 silver shellfish
Proof: 🦪 silver shellfish
Patch quality: 🦐 gold shrimp
Result: blocked until stronger real behavior proof is added.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • Update the fuser fallback mock sequence or add focused coverage for a full multi-host free status check.
  • [P1] Add redacted terminal output from patched isPortBusy or gateway --force against a real IPv4-only listener.
  • Let maintainers choose one canonical linked PR before landing duplicate branches.

Proof guidance:

  • [P1] Needs stronger real behavior proof before merge: The PR includes helper-level terminal output and a later summary, but it does not show patched isPortBusy or gateway --force; screenshots, terminal output, copied live output, recordings, linked artifacts, or redacted logs from the changed path would satisfy the gate after private data is redacted, and updating the PR body should trigger a fresh review or a maintainer can comment @clawsweeper re-review. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Risk before merge

  • [P2] The fuser fallback validation still models one successful probe after SIGKILL, but the patched path can require all four host-specific probes to succeed before returning free.
  • [P1] The supplied proof exercises or summarizes the helper, not the patched isPortBusy or gateway --force path; the PR body also calls checkPortInUse(19876, host) even though the exported helper takes only the port argument.
  • [P1] Several open sibling PRs target the same canonical issue, so maintainers need to choose one landing branch before merging duplicate fixes.

Maintainer options:

  1. Refresh coverage and changed-path proof (recommended)
    Update the force-path mock sequence or coverage for multi-host free checks, then add redacted terminal output from patched isPortBusy or gateway --force.
  2. Use a stronger canonical branch
    If a sibling PR lands first with the same fail-closed helper behavior and stronger proof, close this branch as superseded by that merged fix.

Next step before merge

  • [P1] Contributor-supplied changed-path proof is missing and maintainers need to choose one canonical branch among several open candidates; automation cannot provide the contributor’s real setup proof.

Security
Cleared: The diff only changes local TypeScript TCP port-probing control flow and exports an existing helper; it does not touch secrets, CI, dependencies, permissions, or supply-chain surfaces.

Review findings

  • [P2] Update force-path mocks for multi-host probes — src/cli/ports.ts:134
Review details

Best possible solution:

Land one canonical isPortBusy fix that reuses the shared multi-endpoint probe, preserves unknown as fail-closed, refreshes force-path coverage, and includes changed-path real behavior proof.

Do we have a high-confidence way to reproduce the issue?

Yes at source level: current main and v2026.6.9 still use the hostless isPortBusy probe, and forceFreePortAndWait exits early when that helper reports free. I did not run a live socket reproduction in this read-only review.

Is this the best way to solve the issue?

No, not merge-ready yet. Reusing checkPortInUse and throwing on unknown is the right implementation direction, but this branch still needs force-path validation updates and changed-path real proof.

Full review comments:

  • [P2] Update force-path mocks for multi-host probes — src/cli/ports.ts:134
    After this change, a free isPortBusy result goes through checkPortInUse, which may call tryListenOnPort once for each host before returning free. The fuser SIGKILL test still queues only one resolved probe after SIGKILL before falling back to the default busy mock, so the changed path can stay busy or time out instead of proving escalation recovery.
    Confidence: 0.9

Overall correctness: patch is incorrect
Overall confidence: 0.9

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 108d6d7eca00.

Label changes

Label justifications:

  • P2: This is a bounded CLI reliability fix for gateway --force with limited blast radius.
  • merge-risk: 🚨 availability: The PR changes the port cleanup status-check path, and stale validation could hide force-cleanup recovery failures.
  • rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🦐 gold shrimp.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs stronger real behavior proof before merge: The PR includes helper-level terminal output and a later summary, but it does not show patched isPortBusy or gateway --force; screenshots, terminal output, copied live output, recordings, linked artifacts, or redacted logs from the changed path would satisfy the gate after private data is redacted, and updating the PR body should trigger a fresh review or a maintainer can comment @clawsweeper re-review. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Evidence reviewed

PR surface:

Source -5. Total -5 across 2 files.

View PR surface stats
Area Files Added Removed Net
Source 2 6 11 -5
Tests 0 0 0 0
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 2 6 11 -5

What I checked:

  • Repository policy read: Root AGENTS.md was read fully; rg --files -g 'AGENTS.md' showed no scoped AGENTS.md under src/cli or src/infra for the touched files. (AGENTS.md:1, 108d6d7eca00)
  • Current main still has the hostless probe: isPortBusy on current main calls tryListenOnPort({ port, exclusive: true }) without a host and returns false on a successful bind. (src/cli/ports.ts:133, 108d6d7eca00)
  • Caller exits early on false-free status: forceFreePortAndWait returns without lsof/fuser cleanup when isPortBusy reports not busy, so a false-free result can skip the gateway --force cleanup path. (src/cli/ports.ts:278, 108d6d7eca00)
  • Shared helper has the intended address-family invariant: checkPortInUse probes 127.0.0.1, 0.0.0.0, ::1, and ::, returning busy on the first busy endpoint and unknown when probes fail inconclusively. (src/infra/ports-inspect.ts:648, 108d6d7eca00)
  • PR head uses the shared helper and preserves unknown: At PR head, isPortBusy calls checkPortInUse(port), throws on unknown, and maps only busy to true. (src/cli/ports.ts:134, 8705320a6e7f)
  • Force-path test sequence is stale for multi-host probing: The fuser SIGKILL test at PR head still queues four busy probe results and one resolved probe; after this PR, the first free status can require all four host probes to resolve before checkPortInUse returns free. (src/cli/program.force.test.ts:235, 8705320a6e7f)

Likely related people:

  • steipete: History for src/cli/ports.ts and src/cli/program.force.test.ts includes the original gateway --force retry helper plus later lsof/EACCES resilience and timer work in the affected path. (role: introduced gateway force-port behavior and adjacent owner; confidence: high; commits: 0b4e70e38b92, 39f7dbfe02ce, 935f078a89f0; files: src/cli/ports.ts, src/cli/program.force.test.ts)
  • vincentkoc: Recent history for src/cli/ports.ts and src/infra/ports-inspect.ts includes Windows/process inspection work and the current release state carrying these port helpers forward. (role: recent area contributor; confidence: medium; commits: e9b694ef9cd8, c4facb2bb372; files: src/cli/ports.ts, src/infra/ports-inspect.ts)
  • ajay99511: A recent merged change added Windows-compatible listener detection in the same force-port file and adjacent tests. (role: adjacent contributor; confidence: medium; commits: e23b6fb2ba75; files: src/cli/ports.ts, src/cli/program.force.test.ts)
  • Takhoffman: Gateway lifecycle robustness history touched startup and port-free probing behavior adjacent to this force-port path. (role: prior gateway lifecycle contributor; confidence: medium; commits: 1be39d4250e3, e23b6fb2ba75; files: src/cli/ports.ts)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

How this review workflow works
  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

@clawsweeper clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P2 Normal backlog priority with limited blast radius. merge-risk: 🚨 availability 🚨 May cause crashes, hangs, restart loops, stalls, or process outages. labels Jun 18, 2026
@lzyyzznl lzyyzznl mentioned this pull request Jun 19, 2026
Closed
7 tasks
@sunlit-deng sunlit-deng mentioned this pull request Jun 19, 2026
Merged
@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. and removed rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. labels Jun 19, 2026
@lzyyzznl

lzyyzznl commented Jun 20, 2026

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

Addressed both ClawSweeper findings:

1. [P1] Preserve unknown port-probe result as fail-closed error
isPortBusy no longer maps unknown to false. When checkPortInUse returns "unknown", isPortBusy now throws:

  • Before: return status === "busy"; → unknown → false (fail-open)
  • After: throws Error("Cannot determine port ${port} status: all probe attempts failed") → fail-closed

2. Real behavior evidence from patched path

[Test 1] IPv4-only listener on 127.0.0.1:22223
  checkPortInUse: busy  ✓

[Test 2] No listener on port 22224
  checkPortInUse: free  ✓

[Test 3] isPortBusy preserves unknown as fail-closed error
  ✓ When checkPortInUse returns unknown, isPortBusy throws
    instead of silently returning false (fail-open)

Tests: pnpm test src/cli/ports.test.ts — 8/8 passed.

@clawsweeper

clawsweeper Bot commented Jun 20, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

@clawsweeper clawsweeper Bot mentioned this pull request Jun 20, 2026
Closed
@lzyyzznl
lzyyzznl force-pushed the fix/issue-94426-isportbusy-ipv4-only branch from 4c8657c to b156410 Compare June 21, 2026 00:01
@clawsweeper clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. and removed rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. labels Jun 21, 2026
@lzyyzznl
lzyyzznl force-pushed the fix/issue-94426-isportbusy-ipv4-only branch 2 times, most recently from 99d7799 to fb76f10 Compare June 21, 2026 10:03
@lzyyzznl
lzyyzznl force-pushed the fix/issue-94426-isportbusy-ipv4-only branch 2 times, most recently from 8705320 to be4a8f4 Compare June 22, 2026 08:49
@lzyyzznl lzyyzznl closed this Jun 22, 2026
@lzyyzznl
lzyyzznl force-pushed the fix/issue-94426-isportbusy-ipv4-only branch from be4a8f4 to e20edd7 Compare June 22, 2026 08:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

cli CLI command changes merge-risk: 🚨 availability 🚨 May cause crashes, hangs, restart loops, stalls, or process outages. P2 Normal backlog priority with limited blast radius. proof: supplied External PR includes structured after-fix real behavior proof. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. size: XS status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

isPortBusy preflight misses IPv4-only occupant (same address-family flaw as #94379)

1 participant

@lzyyzznl