fix(bin): replace lsof port preflight with ss/netstat - #3105
fix(bin): replace lsof port preflight with ss/netstat#3105bitflicker64 wants to merge 11 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests.
Additional details and impacted files@@ Coverage Diff @@
## master #3105 +/- ##
============================================
- Coverage 39.19% 32.30% -6.89%
- Complexity 264 374 +110
============================================
Files 770 770
Lines 65779 65779
Branches 8726 8726
============================================
- Hits 25779 21249 -4530
- Misses 37247 42111 +4864
+ Partials 2753 2419 -334 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@copilot review |
imbajin
left a comment
There was a problem hiding this comment.
Blocking: yes. Summary: The new probe can miss or stall on real port conflicts, and the corrected command detection exposes a broken curl-only path. Evidence: static review of 6395c0c; bash -n passed; codecov/project is failing on this head.
imbajin
left a comment
There was a problem hiding this comment.
Blocking: yes. Summary: The fallback probe can execute configuration text as shell code and misparses a supported IPv6 URL form; the no-lsof branch matrix also remains untested. Evidence: exact-head static review, bash -n on all three changed scripts, and controlled reproductions for command execution and [::1]:8080 parsing.
imbajin
left a comment
There was a problem hiding this comment.
Blocking: yes. Summary: The listener-table probes can reject valid binds or silently skip collision detection, and the PD startup test still does not enforce its new lsof-free cleanup path. Evidence: exact-head static inspection, targeted shell reproductions, the 19-test check_port suite, and successful visible checks.
There was a problem hiding this comment.
Blocking: yes — 6.0/10 on current head d315ae8. The main direction and CI are good, but the following issues still need to be addressed before merge.
New current-head findings:
- Hostname inputs bypass collision detection:
ss -n/netstat -nreport numeric addresses, so a supported value such aslocalhostis missed. - The real-listener test can pass without creating its listener: fixed port 54321 may already be occupied and Python bind success is not verified.
Existing comments that remain applicable on d315ae8 (not reposted; +1 used for deduplication):
- No hard deadline without the external
timeoutcommand: the raw/dev/tcpfallback can still stall on dropped SYN packets. - URL normalization is still incomplete: bracketed IPv6 was fixed, but leading/trailing whitespace accepted by
ServerOptionsstill bypasses this check. - Endpoint/dual-stack conflict semantics remain incomplete: the old IPv6-hextet false positive was fixed, but IPv4 and
IPV6_V6ONLYIPv6 wildcard listeners are still treated as unconditional cross-family conflicts. fusercleanup remains unenforced: Linux startup tests usefuserwithout checking that it exists and suppress cleanup failures.
Please do not treat these linked threads as fully resolved merely because some are outdated or marked resolved against older revisions; the specific residual behaviors above were revalidated on the current head.
|
fixing remaining issues |
9d75341 to
745a8bb
Compare
…-test suite Replace lsof with layered probe (ss → netstat → /dev/tcp) in server check_port to eliminate startup hang in Kubernetes pods where ulimit -n can be ~1 billion causing lsof to scan the entire FD table. Core changes (server util.sh): - ss -ltn: kernel socket table query, no FD scan - netstat -ltn / netstat -an: fallback for systems without ss - bash /dev/tcp with timeout 1: last resort, null-command probe - Hostname resolution (getent/dscacheutil) for localhost → numeric IP - Address-family-aware wildcard matching (no cross-family false conflicts) - ss non-zero exit fallthrough to netstat - No-timeout watchdog with 2s hard deadline - URL whitespace stripping (handles ServerOptions edge case) - IPv6 bracket notation parsing - Octal-safe port validation via $((10#port)) - curl -fL for HTTP error detection (was curl -L) PD and Store util.sh: - Remove dead check_port (never called by PD/Store startup scripts) - Fix command_available return-value inversion bug - Fix curl download path from broken concatenation to basename output - curl -fL for HTTP error detection CI and cleanup: - fuser preflight check in server-ci.yml (Linux port cleanup) - OS-aware cleanup in test-start-hugegraph*.sh (lsof on macOS, fuser on Linux) - Warning when neither tool available Tests (test-check-port.sh, 34 tests, 0 failures): - ss branch: IPv4/IPv6 occupied/free, wildcard, dot-escaping guard - netstat branch: Linux/BSD formats, IP octet false-positive guard - /dev/tcp: timeout mock, real ephemeral-port listener with liveness check - Host/port conflict: cross-family wildcard, specific IP vs loopback - Tool failure fallthrough: ss→netstat, ss+netstat→/dev/tcp - Hostname collision: localhost resolution, unresolved fallthrough - URL normalization + IPv6 hextet guard - Edge cases: empty URL, port >65535, port 0, non-numeric - download() curl fallback: PD, Store, failure propagation - SKIP exit-code handling for setup failures Closes apache#3105
937624c to
6f87b8f
Compare
imbajin
left a comment
There was a problem hiding this comment.
Blocking: yes. Summary: This head reverts a merged query-correctness fix and also leaves new URL parsing, resolver timeout, and test-cleanup gaps. Evidence: exact-head static review by 6 independent lanes; bash -n passed; focused shell suite reported 33 pass, 0 fail, 1 environment-limited skip; visible exact-head checks are green.
4cf5886 to
8307426
Compare
8307426 to
9fdc941
Compare
|
Pushed a follow-up addressing the outstanding threads on the current head. Status per open item: Fixed:
Documented, not changed:
Re-requesting review on the latest commit — let me know if any of the above needs more than a comment. |
There was a problem hiding this comment.
Blocking: yes — 5.5/10 on current head 9fdc941d.
Why 5.5/10
Only two new inline comments were added because the other blockers already have review threads; the score reflects the whole current implementation, not the number of new comments.
actual port conflict / stalled resolver
|
v
probe output is missing, ambiguous, or not bind-equivalent
|
v
check_port returns "free" — or never returns
|
v
Java bind fails with EADDRINUSE / startup hangs
‼️ Endpoint correctness: dual-stack semantics and the wildcard fallback can still produce a falsefree.⚠️ Bounded execution: the resolver deadline is not a guaranteed TERM-to-KILL deadline.⚠️ Configuration and CI: URL/default-port parsing remains partial, and PD/Store cleanup prerequisites do not match the implementation.⚠️ Fallback confidence: a successful but unusable listener table is treated asfree, while the production no-timeoutwatchdog is not exercised.
Expected design
Make every probe return occupied, free, or unknown:
- Normalize and validate the configured endpoint once; explicitly reject unsupported ambiguous forms such as unbracketed IPv6.
- Parse listener records into address-family/address/port tuples and apply wildcard plus
v6onlybind semantics. - Treat execution failure and empty/unparseable output as
unknown, then continue to a bounded fallback. - Put both resolution and connection attempts under a TERM-to-KILL deadline.
- Test the actual production branches and align CI prerequisites with the cleanup tool being used.
Only validated, bind-equivalent evidence should produce free.
9fdc941 to
1f3f01e
Compare
|
All three 5.5/10 items addressed in
Also fixed a regression from authority parsing: invalid ports like 35/35 tests pass. Re-requesting review. |
imbajin
left a comment
There was a problem hiding this comment.
Blocking: yes. Summary: The no-timeout port probe passes its arguments in the wrong order, and its new regression test is invalid; the latest-head server CI is failing. Evidence: exact-head static inspection; a controlled helper invocation printed 8080|2; HugeGraph-Server CI reported 34 passed and 1 failed.
- Localize read_property variables (match PD/Store) - Use if ! download / if ! tar in ensure_package_exist so atomic download failures are detected without a fragile $? check Also merges upstream master so the branch is no longer diverged (RISC-V libatomic + CI from apache#3102 kept intact with check_port).
Status since imbajin’s last reviewAddressing the blocking items from the later review rounds (including heads Port check correctness
URL / authority parsing
download() / PD–Store
CI / tests / cleanup
Also
Local: Happy to take another look when you have time — thanks for the thorough reviews. |
There was a problem hiding this comment.
Blocking: yes — 5.5/10 on current head 986c95f1.
The implementation has improved, but the preflight still has verified false-positive and false-negative behavior. Below is the complete current-head status so resolved threads do not hide residual cases.
Fixed on this head
- Shell configuration values are passed as positional parameters instead of being reparsed as code.
- Authority parsing now stops at
/,?, or#; surrounding whitespace and bracketed IPv6 are handled. - Hostnames and equivalent IPv6 spellings are normalized before listener comparison.
- Nonzero
ss/netstatexecution falls through instead of being treated as “free”. - The no-
timeoutargument-order regression, invalidlocaldeclarations, and listener cleanup were fixed. - Normal downloads use a temporary file and rename only after successful transfer.
- The accidental rollback of the merged count-optimization fix was removed.
Still required
‼️ macOS false occupied:netstat -ltnincludes non-listening states, while the parser scans every token in every row. An unrelatedESTABLISHEDpeer port can block startup. BSD wildcard rows also carry their family intcp4/tcp6; discarding that field creates cross-family false positives.‼️ Linux dual-stack false free: the dual-stack thread remains applicable.[::]:PORTcan also occupy IPv4 whenIPV6_V6ONLY=0, but the implementation and test currently assume the families are independent.⚠️ Wildcard false free: the/dev/tcpfallback checks loopback only, so a listener bound only to another local interface is missed before a wildcard bind.⚠️ Unknown is treated as free: a successful but empty or unparseable listener table can still setport_checked=1; this is reproducible for thess/ Linux-netstatpaths.⚠️ URL normalization is still partial: the default-port thread still applies to uppercase schemes and scheme-less values accepted byServerOptions. Ambiguous unbracketed IPv6 should be explicitly rejected rather than guessed.⚠️ Resolution is not always bounded: the resolver deadline thread still has a no-timeoutpath that can rungetentsynchronously.⚠️ CI prerequisites disagree with the implementation: the PD/Store workflow still checkslsof, while Linux cleanup usesfuser.⚠️ The production no-timeoutpath lacks end-to-end coverage: the existing watchdog test tests the helper, notcheck_port → run_with_deadlinewith a real occupied/free endpoint.⚠️ Verified Store download can report success after installation failure:mvfailure is overwritten byreturn 0.⚠️ The configured startup timeout is not a hard deadline:wait_for_startup()calls synchronouscurlwithout--connect-timeout/--max-time, so one blackholed request can exceed the outer timeout indefinitely.⚠️ PID-based temporary names are not exclusive:...tmp.$$is shared by concurrent background subshells. Current callers are sequential, so either usemktempas a small fix or explicitly keep concurrency unsupported; no locking framework is needed.- 🧹 Scope and hygiene: unrelated
process_num/process_id/ memory and shared-helper contract changes have no focused compatibility tests, andtest-check-port.sh:319contains trailing whitespace.
Minimal design for this PR
Keep this as a best-effort startup preflight; the real server bind remains authoritative.
- Normalize only enough to obtain a validated port: case-insensitive
http/https, explicit/default port, scheme-less value, and bracketed IPv6. Reject ambiguous input with a clear warning. - Detect any LISTEN socket on that port, matching the original conservative behavior:
- Linux: filtered
ss -H -ltn(orfuserfallback). - macOS/BSD:
netstat -an -p tcp, inspecting onlyLISTENrows and the local-address column.
- Linux: filtered
- Return
busy,free, orunknown. Tool failure and unparseable output areunknown; warn and let the server perform the authoritative bind. - Remove DNS resolution, address canonicalization, cross-family endpoint matching,
/dev/tcp, and its watchdog from this path. Port-level detection makes the dual-stack and wildcard cases conservative without reproducing kernel socket semantics in Bash. - In the same PR, check
mv, usemktemp, bound the startupcurlby the remaining deadline, align CI prerequisites with the selected OS tool, and revert or split unrelated helper refactors. - Replace the broad mock matrix with a compact contract suite: URL-to-port cases; Linux
ssbusy/free/failure; BSDLISTENversusESTABLISHED;unknownfallback; one real ephemeral listener; rename failure; and a blocking startup request.
This handles the current blockers in one PR without adding a new abstraction or expanding support for rare socket configurations.
Rework the startup port check to the minimal design agreed in review. The preflight is best effort - the server's own bind stays authoritative - so it now answers one question only: is anything already listening on this port? check_port: - Parse just enough of the configured URL to obtain a validated port: case-insensitive scheme, explicit or default port, scheme-less value, bracketed IPv6. Ambiguous unbracketed IPv6 is rejected with a warning instead of guessed. - Detect listeners per OS, inspecting only LISTEN rows and only the local-address column: ss -H -ltn on Linux, netstat -an -p tcp on BSD. Splitting on the last separator keeps IPv6 hextets from being read as the port. - Return busy, free or unknown. A probe that is missing, fails, or yields no recognisable listener row is unknown: warn and let the bind decide. - Drop DNS resolution, address canonicalisation, cross-family endpoint matching, /dev/tcp and its watchdog. Port-level detection covers the dual-stack and wildcard cases conservatively without reproducing kernel socket semantics in shell, and removes the last unbounded call from the startup path. This fixes the macOS false positive where netstat -ltn returned non-listening rows and an unrelated ESTABLISHED peer port aborted startup. Also in this pass: - store: a failed atomic rename no longer reports success, so LD_PRELOAD is never exported for a library that did not install. Same fix applied to download() in all three copies. - download(): mktemp instead of a PID-derived name, which is shared by concurrent background subshells. - wait_for_startup(): bound each probe with --connect-timeout / --max-time by the time left in the deadline, so one blackholed request cannot outlive the configured timeout. - pd-store-ci: preflight checked lsof while cleanup used fuser; align it. - Revert unrelated process_num / process_id / free_memory / get_ip changes. netstat is used as the Linux fallback rather than fuser: fuser cannot distinguish "not found" from "insufficient privileges", which would break the busy/free/unknown contract. Tests: test-check-port.sh replaced with a compact contract suite - URL to port, ss busy/free/failure, BSD LISTEN vs ESTABLISHED vs TIME_WAIT, unknown fallback, a real ephemeral listener, rename failure, and a bounded startup probe. 41 passed, 0 failed. Verified non-vacuous by mutation: dropping the LISTEN filter, downgrading unknown to free, and reading the foreign-address column each fail the suite.
Converging on port-only detection removed capability on purpose. Mark each gap in place with a namespaced TODO so it can be found and picked up later rather than rediscovered: - port-only matching ignores the listener's address, so a listener on one local address reports the port busy even when the server would bind a different one. This is the old `lsof -i :PORT` behaviour and fails safe, but it can refuse a bind that would have succeeded. - a host with genuinely zero LISTEN sockets is indistinguishable from a restricted or unparseable table; both report unknown and warn. - with neither ss nor netstat present there is no probe left, so the check is permanently unknown. A dependency-free fallback needs a bounded connect, which is exactly what was removed here. - unbracketed IPv6 and a scheme-less value with no port are skipped rather than guessed. - wait_for_startup overshoot is now bounded but not zero: the loop still sleeps between probes before re-reading the clock. - the Linux and BSD detection branches are mock-driven, so only the host's own branch runs against a real kernel on any one runner. Also fix a real parsing defect found while auditing those gaps: a URL carrying userinfo (http://user:pass@host:port) was matched by the unbracketed-IPv6 check, so it skipped the preflight and printed a misleading warning about bracket notation. Strip userinfo from the authority before that test, and cover the userinfo forms in the suite. Suite: 44 passed, 0 failed.
|
Thanks for the detailed design in the last review — I took it in full rather than patching around the edges. Current head: What changed
Worth noting this strengthens the original goal rather than only preserving it: there is no longer any unbounded call left in the startup path, not merely no All 12 items from your list are addressed — the resolution table is in the PR description. Items 2, 3, 6 and 8 are resolved by removal rather than by more shell. One deviationYou suggested Size
The convergence commit itself is +426 / −1297. What this gives upPort-only detection trades away real capability, so rather than leave that implicit I marked each gap in place with a namespaced
Tests
I checked it is not vacuous by mutating the implementation — dropping the Your macOS repro, before and after on this head: Auditing those gaps also turned up a defect worth mentioning: a URL with userinfo ( Out of scopeTwo findings are deliberately not addressed, with reasons on their threads: the |
imbajin
left a comment
There was a problem hiding this comment.
Blocking: yes. Summary: Oversized numeric ports can wrap into valid values before range validation. Evidence: exact-head static review and a controlled reproduction where 18446744073709551617 is accepted as port 1; the 44-case shell suite otherwise passes.
parse_port_from_url() converted the digit string with $((10#$port)) before bounding it. Bash evaluates in 64 bits and wraps silently, so an out-of-range value could re-enter 1..65535 and be accepted as a real port: 18446744073709551617 parsed as 1, and 18446744073709559616 as 8000. The preflight would then abort startup naming a port that was never configured. Normalise the leading-zero form textually instead, reject anything longer than five digits, and only then convert and range-check. Java reads 08080 as 8080, so that form is still accepted. Covers both wrapping values, an all-zero port, and a long leading-zero form in the URL-parsing table.
imbajin
left a comment
There was a problem hiding this comment.
Blocking: yes. Summary: The official images no longer have a usable occupied-port preflight, fallback parsing can still skip a working probe, and startup can exceed its configured deadline. Evidence: six independent exact-head review lanes, Docker dependency and shell branch traces, 48/48 check_port contract tests passing, git diff --check passing, and all visible latest-head workflows passing.
The server images install lsof, which check_port no longer calls, and the eclipse-temurin:11-jre-jammy base ships neither ss nor netstat. The preflight was therefore permanently inconclusive in every official image: a duplicate start was no longer refused, so it could overwrite bin/pid before the second JVM failed to bind, leaving the first process outside the stop script's reach. base image as shipped: port_listen_state 8080 -> unknown base image + iproute2: port_listen_state 8080 -> busy Replace lsof with iproute2 in both server images; no shipped script calls lsof any more. Assert the result in the image build, which already runs on any PR touching a Dockerfile. port_listen_state also ended its search on the first probe, so an ss response that ran but could not be parsed returned "unknown" even with a working netstat behind it. Capture the parser result, answer only on busy or free, and fall through otherwise. `ss -H` exiting zero with no output is the one case where empty is an answer rather than a failure - -H means there is no header to print - so read it as "no listeners" instead of warning on every clean start. wait_for_startup bounded each curl but not the loop: it slept a flat 2s and only then re-read the clock, so a 1s timeout ran for 2s. Recompute the remainder before pausing, cap the pause to it, and stop once it is spent. Drop the /dev/tcp wording from the server CI comments; no such branch exists. Suite is 53 passed, 0 failed on macOS bash 3.2 and on Linux bash 5.2 against a real ss and a real listener. Each fix is mutation-checked: restoring the early return fails only the two fallback cases, restoring unknown-on-empty fails one, restoring the flat sleep fails one.
Self-review pass over the port-preflight change. `process_num` and `process_id` in the PD and Store `util.sh` still carried a changed contract - a boolean return and an echoed pid - while the server copy had already been reverted. Neither has a caller in PD or Store, but the review asked for unrelated helper refactors to be reverted or split, so all three copies now match master's contract again. The PD and Store images installed `lsof` solely for the dead `check_port` this change removes; nothing in `hg-pd-dist` or `hg-store-dist` calls it any more. Unlike the server images they run no preflight, so nothing replaces it. Also in this pass: - `test-check-port.sh`: the tally file is now covered by a trap for the whole run, section 5 registers its trap before forking the listener rather than after, and it restores that handler instead of clearing it globally. Verified against the previous head: a SIGTERM mid-run leaked one temp file, now none. - `test-check-port.sh`: `sleep 0.5` -> `sleep 1`. Fractional sleep is not POSIX, and the suite runs `set -u` without `-e`, so a busybox sleep would fail silently and burn all ten readiness iterations at once. - `docker-build-ci.yml`: assert `free` or `busy` rather than "not unknown", so an empty result cannot satisfy the check. - `port_listen_state`: record that the port must already be normalised, since it is matched as text. - Correct the port-cleanup comments in the server and PD startup tests, which credited fuser on the branch that uses lsof for macOS. Suite is 53 passed, 0 failed on macOS bash 3.2 and on Linux bash 5.2 against a real ss; the five implementation mutations all still fail it.
|
Self-review on
53 passed, 0 failed on macOS |
imbajin
left a comment
There was a problem hiding this comment.
Blocking: yes. Summary: The port-preflight deadline can still be exceeded, and concurrent checksum installs can remove a valid replacement; CI coverage also has trigger and false-skip gaps. Evidence: six independent exact-head review lanes, controlled deadline reproduction, static concurrency trace, 53/53 check_port assertions passed, git diff --check passed, visible Actions passed but Codecov project status failed.
Honor the startup deadline at the probe boundary, preserve verified Store downloads across concurrent replacement failures, and make the focused CI wiring fail closed. Add deterministic regressions for both race boundaries and trigger the image probe when server util.sh changes.
|
@imbajin Ready for re-review at The four findings from your latest review are addressed in
Codecov is intentionally treated as non-blocking. |
imbajin
left a comment
There was a problem hiding this comment.
Blocking: yes. Summary: The implementation passes its focused suite, but the test harness does not lock down the Linux listener filter or the startup timeout and return contract. Evidence: exact-head static review, 60/60 focused shell assertions passing, git diff --check, and all visible latest-head checks passing.
There was a problem hiding this comment.
Pull request overview
This PR updates HugeGraph’s distribution/CI shell tooling to avoid startup hangs in containerized environments by removing the lsof -i :PORT preflight (which can be unbounded with huge FD limits) and replacing it with an ss/netstat-based, best-effort “is anything listening on this port?” check. It also removes now-dead check_port copies from PD/Store, updates Docker images/CI prerequisites accordingly, and adds a contract-style shell test suite to pin the new semantics.
Changes:
- Rewrote the server
check_portpath to parse ports robustly and detect listeners viass/netstatwith a 3-state contract (busy/free/unknown). - Removed dead PD/Store
check_portand tightened related helper logic (downloads, temp files, quoting, and startup probing behavior). - Updated Dockerfiles and GitHub Actions workflows/tests to match the new runtime dependencies and to validate the port-probe behavior.
Reviewed changes
Copilot reviewed 11 out of 15 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
hugegraph-server/hugegraph-dist/src/assembly/static/bin/util.sh |
Replaces lsof-based port preflight with parse_port_from_url + port_listen_state; tightens startup probe deadline behavior; makes downloads more atomic. |
hugegraph-server/hugegraph-dist/src/assembly/travis/test-check-port.sh |
Adds contract tests for URL parsing, ss/netstat parsing, unknown-handling, bounded startup probing, and concurrent replacement safety. |
hugegraph-server/hugegraph-dist/src/assembly/travis/test-start-hugegraph.sh |
Switches port cleanup to fuser on Linux while retaining lsof for macOS; adjusts prereq checks and exports. |
hugegraph-server/hugegraph-dist/src/assembly/travis/test-start-hugegraph-store.sh |
Similar port cleanup/prereq logic updates; ensures stop is invoked during cleanup. |
hugegraph-server/hugegraph-dist/src/assembly/travis/test-start-hugegraph-pd.sh |
Similar port cleanup/prereq logic updates; ensures stop is invoked during cleanup. |
hugegraph-server/Dockerfile |
Replaces lsof with iproute2 so the server image includes ss for the new preflight. |
hugegraph-server/Dockerfile-hstore |
Same as above for the hstore server image. |
hugegraph-pd/hg-pd-dist/src/assembly/static/bin/util.sh |
Removes dead check_port; adjusts helper implementations (quoting, download, temp handling). |
hugegraph-pd/Dockerfile |
Removes lsof since PD no longer has a port preflight dependency. |
hugegraph-store/hg-store-dist/src/assembly/static/bin/util.sh |
Removes dead check_port; adjusts helper implementations (quoting, download, temp handling). |
hugegraph-store/hg-store-dist/src/assembly/static/bin/start-hugegraph-store.sh |
Quotes download_and_verify args and LD_PRELOAD; minor robustness in ulimit and process detection. |
hugegraph-store/Dockerfile |
Removes lsof since Store no longer has a port preflight dependency. |
.github/workflows/server-ci.yml |
Adds test-check-port.sh to CI and updates prereq checks (drops lsof, adds fuser requirement for Linux cleanup). |
.github/workflows/pd-store-ci.yml |
Aligns prereq checks with the new cleanup approach (fuser on Linux). |
.github/workflows/docker-build-ci.yml |
Ensures Docker build CI runs when server util.sh changes and asserts server images can produce a definitive port_listen_state result. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Purpose of the PR
Fix a startup hang in kind/Kubernetes environments — the case that surfaced while bringing up the Helm chart. In a pod where
ulimit -nis very large,lsof -i :PORTwalks an enormous file descriptor table andstart-hugegraph.shstalls before the server ever binds.The fix replaces that
lsofcall in the server'scheck_port, and removes the deadcheck_portcopies from PD/Store.Design
Following the minimal design agreed in review, the preflight answers exactly one question: is anything already listening on this port? It is best effort — the server's own bind remains authoritative.
Parse only enough of the configured URL to obtain a validated port: case-insensitive scheme, explicit or default port, scheme-less value, bracketed IPv6. Ambiguous unbracketed IPv6 is rejected with a warning rather than guessed.
Detect per OS, inspecting only
LISTENrows and only the local-address column:ss -H -ltn, falling back tonetstat -ltnnetstat -an -p tcpSplitting the address on its last separator keeps IPv6 hextets (
[2001:db8:8080::1]:9090) from being read as the port.Report
busy,freeorunknown. A probe that is missing, fails, or yields no recognisable listener row isunknown: warn and let the bind decide. Only positive evidence blocks startup.Port-level detection covers the dual-stack and wildcard cases conservatively — matching the original
lsof -i :PORTsemantics — without reproducing kernel socket behaviour in shell.This also strengthens the original fix. DNS resolution, address canonicalization, cross-family endpoint matching,
/dev/tcpand its watchdog are all gone, so there is no longer any unbounded call left in the startup path — not just nolsof.Main Changes
hugegraph-server/.../bin/util.sh—check_portrewritten as above (parse_port_from_url+port_listen_state);normalize_addr()andrun_with_deadline()deleted;wait_for_startup()bounds each probe with--connect-timeout/--max-timeand caps its retry pause by the time left in the deadline.hugegraph-server/Dockerfile,Dockerfile-hstore—lsofreplaced withiproute2. The base image ships neitherssnornetstat, so without this the preflight is permanentlyunknownin every official image and a duplicate start is no longer refused;lsofwas left over from the probe this PR removes and no shipped script calls it.hugegraph-pd/Dockerfile,hugegraph-store/Dockerfile—lsofremoved. Its only consumer was the deadcheck_portthis PR deletes from the PD/Storeutil.sh; nothing inhg-pd-distorhg-store-distcalls it any more. These two run no preflight, so unlike the server images nothing replaces it..github/workflows/docker-build-ci.yml— the built server images must be able to answerport_listen_state, so a missing socket-table tool fails the build rather than silently disabling the preflight..github/workflows/server-ci.yml— comments described anss/netstat//dev/tcpchain; the/dev/tcpbranch does not exist.hugegraph-pd/.../util.sh,hugegraph-store/.../util.sh— deadcheck_portremoved;command_availableinversion fixed; atomicdownload(); a failed rename no longer reports success, soLD_PRELOADis never exported for a library that did not install;mktempinstead of a PID-derived temp name..github/workflows/pd-store-ci.yml— preflight checkedlsofwhile cleanup usedfuser; aligned.Review items
All items from the review rounds are addressed:
LISTENrows and local-address column only/dev/tcpfallbackunknownis a distinct state that never blockspd-store-ci.ymltimeoutcoveragemvfailure swallowedmktempprocess_num/process_id/free_memory/get_ipreverted to master in all threeutil.shcopieslsofreplaced withiproute2in both server images, asserted in the image buildssskips thenetstatfallbackbusy/free, otherwise the next one is tried/dev/tcpbranch that does not existssmock ignored argv and non-LISTEN rows-H -ltnargv is enforced and a non-LISTEN local-port regression addedA self-review pass on top of that found a few more, none of them raised in review:
process_num/process_idstill carried a changed contract while the server copy was already revertedlsoffor thecheck_portthis PR deletessleep 0.5in the listener-readiness loopset -uwithout-ea busyboxsleepfails silently and burns all ten iterations. Nowsleep 1unknown", which an empty result also satisfiesfreeorbusyfuseron the macOS branch that useslsofport_listen_statealso now records that its port argument must already be normalisedOne deviation:
netstatis used as the Linux fallback rather thanfuser.fusercannot distinguish "not found" from "insufficient privileges", which would collapsefreeandunknowninto the same result and break the contract.Size
Converging on the port-only design shrank the change substantially:
masterutil.shtest-check-port.shTests
test-check-port.shis a compact contract suite: URL-to-port cases; exact Linuxss -H -ltnargv, non-LISTEN filtering, busy/free/failure and thenetstatfallback; BSDLISTENvsESTABLISHEDvsTIME_WAIT; theunknownfallback; one real ephemeral listener; rename failure; bounded startup failure and timeout values; the deterministic deadline boundary; and the coordinated Store replacement race.62 passed, 0 failed on the current merged head with macOS
bash3.2, including a real listener, the deadline boundary, and the coordinated Store race. The latest-head Linux and image results are provided by the triggered CI runs.Verified non-vacuous by mutation — each of these makes the suite fail:
LISTENrow filter-lfrom the exactss -H -ltninvocation$((10#$port))before bounding its digit countunknowntofreessbranch before thenetstatfallback is triedss -Hresponse asunknowninstead offreesleep 2in the startup retry loop--max-time 5under a one-second remaining deadlineAdditional current-head gates passed: both explicit missing-target invocations exit 1, every changed shell file passes
bash -n, all changed workflows parse, the 38-module Maven clean compile succeeds, the repository-wide EditorConfig check succeeds, andgit diff --checkis clean. One full implementer self-review and two fresh independent post-fix read-only reviews found no actionable defects.The macOS false positive was reproduced before the change and re-checked after: with 37
ESTABLISHEDconnections to:443and no listener on 443,check_port "http://0.0.0.0:443"previously exited 1 and now correctly reports the port free.The image regression was reproduced the same way —
util.shsourced inside the real base image, against a real listener:Known limitations
Converging on the port-only design traded away capability on purpose. Each of these is marked in the code with a greppable
TODO(check_port)so it can be picked up later:127.0.0.1:8080reports the port busy even when the server would bind192.168.1.5:8080— refuses a bind that would have succeeded. Fails safe, and matches whatlsof -i :PORTdid.port_listen_statenetstatbranchesss -Hresolves this (zero exit, no output, no header to lose), but bothnetstatbranches print headers the parser drops, so an idle host still warns thereport_listen_statessnornetstatpresentunknownand never detects a busy port. The published images now carryiproute2for this reason, but a hand-built minimal image can still ship neither. A dependency-free fallback needs a bounded connect — which is exactly what was removed here, so adding one back means re-solving the hang this PR fixes.port_listen_state::1:8080)parse_port_from_url127.0.0.1)parse_port_from_urlwait_for_shutdownhas the same overshootwait_for_startuphadwait_for_shutdowndocker-build-ci.ymltest-check-port.shNone of these can block a legitimate startup except the first, and that one is the pre-existing
lsofbehaviour rather than a new regression.Out of scope
Two findings raised on this PR are not addressed here, deliberately:
libjemalloc_aarch64.soMD5 mismatch instart-hugegraph-store.sh— theexpected_md5value is pre-existingmastercontent; this PR only added quoting around thedownload_and_verifyarguments. Changing a published checksum belongs in its own change.cluster-test/MultiClusterDeployTest.testServerNodesDeployment(org mirror only) — fails with an identical signature on branches that do not touch this code, and passes onmaster, so it is not a regression from this PR.Documentation Status
Doc - No Need