Skip to content

fix(dev-up): recognize localized netstat states - #2531

Merged
Chris0Jeky merged 4 commits into
mainfrom
issue-2526/netstat-state-fallback
Sep 6, 2026
Merged

fix(dev-up): recognize localized netstat states#2531
Chris0Jeky merged 4 commits into
mainfrom
issue-2526/netstat-state-fallback

Conversation

@Chris0Jeky

Copy link
Copy Markdown
Owner

Summary

  • treat the positional netstat state column as localized and accept any non-empty state when the local port matches in both launchers
  • harden Bash epoch-millisecond detection and align timeout parsing for surrounding whitespace and signed zero
  • add direct Bash and PowerShell regression seams for all three cases

This follow-up is intentionally stacked on #2522 because the fallback path is introduced by that still-open PR. Retarget it to main only after the parent lands.

Verification

  • �ash -n scripts/dev-up.sh
  • PowerShell AST parse

ode --check scripts/ci/dev-up.test.mjs

  • focused localized-netstat/parser suite: 4 passed
  • full launcher suite: 86 passed; 2 unrelated Bash lifecycle tests timed out locally (
    eset seed teardown... and success uses exact marker...) with no tracked launcher descendants; the changed seams and existing port-release cases passed
  • git diff --check

Closes #2526
Refs #2522

@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, add credits to your account and enable them for code reviews in your settings.

@Chris0Jeky

Copy link
Copy Markdown
Owner Author

The connector usage-limit notice is informational, not a review finding. This PR is being checked through the independent coordinator review path; no code action is requested from this notice.

@Chris0Jeky

Copy link
Copy Markdown
Owner Author

Fresh-context review (SC-9 gate — Codex credits exhausted). This is the first review posted on this PR.

Verdict: FIX-FIRST. The localization fix is real and needed, but as written the fallback stops discriminating on state at all, so it reports a closing socket as a live listener. Verified by execution, not by reading.

This PR is T2 control-plane (scripts/ci/**), so under the ADR-0066 amendment of 2026-09-03 it merges only after the maintainer's own review plus this fresh-context review. It is also stacked on open PR #2522, which is currently CONFLICTING against main — it cannot land until that does.

MEDIUM — the relaxed match accepts every TCP state, not just localized LISTENING

scripts/dev-up.sh:431. The old code filtered on the state token (grep -i 'LISTEN'). The new awk keeps only a positional check — ends_with_port($2) && $4 != "" for the Windows shape, ends_with_port($4) && $6 != "" for BSD/Linux — and that column is non-empty for every TCP row. So the accepted set is not "the localized spellings of LISTENING", it is "any token at all": TIME_WAIT, ESTABLISHED, CLOSE_WAIT, FIN_WAIT_1/2, SYN_SENT, SYN_RECEIVED, LAST_ACK, CLOSING, in every locale.

Confirmed by replaying 10 realistic netstat captures through the head's own port_listener_inventory with ss/lsof stubbed away. scripts/dev-up.ps1:418 has identical semantics, so the two launchers agree — they are consistently wrong rather than divergent, which is the better of the two failure modes but still wrong.

Consequence for the launcher: a port left in TIME_WAIT by a just-stopped API now reads as "a live foreign owner is holding this port", and dev-up fails closed instead of waiting for the socket to drain. That is precisely the distinction the base PR #2522 exists to draw, so this change partially undoes its parent's purpose.

Fix that keeps the localization win. Discriminate on the peer-endpoint column rather than the state word — wildcard/zero peer endpoints are pure digits and punctuation and are not localized:

function wildcard_peer(value) { return value ~ /^(0\.0\.0\.0:(0|\*)|\[?::\]?:(0|\*)|\*:(\*|0)|\*\.\*)$/ }

and require it alongside the port match. That restores LISTEN-only semantics without reintroducing an English-only state token.

MEDIUM — the 186 lines of new test ratify the bug rather than catch it

scripts/ci/dev-up.test.mjs:1231. The bug is Windows-only (Windows netstat localizes the state column; Linux net-tools and BSD tokens are not localized), yet the bash fixture emits a Linux/BSD-shaped six-column row carrying a German-ish tokentcp 0 0 127.0.0.1:PORT 0.0.0.0:* ABHOEREN — a shape no real netstat ever produces. The PowerShell fixture at :687 is closer to real but omits the localized header and blank lines, and both use ASCII ABHOEREN rather than the real ABHÖREN, so the OEM-codepage path (netstat.exe writing CP437/CP850 bytes into a git-bash pipe, which is not valid UTF-8) is never exercised.

Most importantly there is no negative case: nothing asserts that a TIME_WAIT or ESTABLISHED row on the target port yields free. That is not a coverage oversight, it is the finding above — the suite as written silently ratifies the defect.

Suggested additions, table-driven off one (fixture, expected) array: a Windows-shape four/five-column fixture with the real header lines; TIME_WAIT-only and ESTABLISHED-only fixtures asserting free; and a two-token localized state (Italian IN ASCOLTO) asserting listening — that last case is confirmed working on both launchers today, so it is cheap coverage for a real hazard.

LOW — the PowerShell guard is unreachable dead code

scripts/dev-up.ps1:418. $fields = ($row -split '\s+') | Where-Object { $_ -ne "" } already drops empty tokens, -split '\s+' cannot emit a whitespace-only token, and line 415 has already asserted $fields.Count -ge 5. So [string]::IsNullOrWhiteSpace([string]$fields[3]) is always $false and the guard can never continue. Harmless on its own, but it reads to a future maintainer as though a state check survives. Either delete the vacuous half, or replace it with the peer-endpoint discriminator above — which is what the comment's word "conservative" implies but does not deliver.

LOW — two undeclared changes ride along, amending the still-open base PR's own code

The title, branch name and commit subject all say "recognize localized netstat states", but the sh diff also rewrites port_release_timeout_ms (:375-397 — whitespace trim, leading +, -0/-000 normalisation, suppressed warn for an all-whitespace value) and now_ms (:400-406 — ^[0-9]+$ tightened to ^[0-9]{12,}$).

Both are correct: I walked the parity table against the PowerShell parser (' 42 ', '-0', '+0', ' ', '-5', '2147483648', 11-digit, '1,000', '0x10', '0000000042' all now agree), and the now_ms guard genuinely closes an 8-hour-deadline hazard when date +%s%3N degrades to bare seconds. The issue is disclosure: these functions do not exist on main, so this PR silently amends #2522's unlanded code. No code change needed — amend the body to say so, and note it on #2522's thread, so the maintainer's SC-10 pass reviews the stack's combined effect rather than the base in isolation.

The red check — investigated, not a code defect

Smart CI / Required Gate is FAILURE at this head (job 100929506193). The failing step emits planner-error — pull-request planning requires merge SHA and tree SHA from the same fetched merge ref and, separately, trust-mismatch — plan says T3, the event re-derives T1.

Root cause is a stacked-PR base-resolution defect in Smart CI, not in this diff: smart-ci-shadow.yml:53 sets CONTROL_BASE: ${{ github.sha }}, and the job env shows CONTROL_BASE: ca6fe42f…, which is main's tip at 06:07:19Z — not this PR's actual base issue-1898/port-release-deadline @ d8d53002. The merge ref, built against the real base, cannot reconcile with that expected base. The same job passes on #2522 and #2532, so it is not systemic to the lane; it is specific to a PR whose base is not the default branch. This PR was created directly against the stacked base with no retarget event, so it is not staleness either.

All of ci-required is green here — only this advisory gate is red, and branch protection requires only the three security contexts. Filing the Smart CI defect separately against CI-03 #2327. Note for planning: #2506 does not close this. It targets the same error string, but its tests only ever pass baseRef: 'main', so the stacked case is untested; its receipt would also read "the base advanced from ca6fe42 to d8d5300", which is a false narrative — the base did not advance, it is a different branch; and it does nothing about the trust-mismatch.

Ordering

2531^ is exactly d8d53002, the current head of issue-1898/port-release-deadline, so the stack is clean today. #2522 must land first (global law 4: oldest first). Do not --delete-branch issue-1898/port-release-deadline while this PR still targets it — that cascade-closes this PR unreopenably. Retarget to main only after the parent has actually merged, then confirm the new base via the API before merging.

Chris0Jeky added a commit that referenced this pull request Sep 4, 2026
Resolves the single conflict this branch had accumulated. #2522 inserts 182
lines of new port-release tests immediately above the `high-volume stdout and
stderr cannot deadlock marker acceptance` declaration; main's 8c0779f
modified that exact declaration line (adding `timeout: 60_000`). Git cannot
order an insertion against a change to its anchor line, so it conflicted.

Resolution keeps both sides: every inserted line survives verbatim and main's
declaration — with its timeout — moves below the inserted block. #2522's own
copy of that declaration, which lacked the timeout, is dropped.

Merged rather than rebased on purpose: #2531's head has this branch's head
d8d5300 as an ancestor, so a force-push here would strand the stacked child.

Verified: the diff against main contains zero removed lines (pure insertion),
`node --check scripts/ci/dev-up.test.mjs` passes, and both of main's fixes
survive in the tree - `closeAllConnections` and `timeout: 60_000`.
@Chris0Jeky

Copy link
Copy Markdown
Owner Author

Merge-time trap: this PR's Closes #2526 is currently inert

Separate from the review above, and easy to miss because the body looks correct.

gh pr view 2531 --json closingIssuesReferences,baseRefName
  base=issue-1898/port-release-deadline   closes=0   []

The body's last lines do read Closes #2526 / Refs #2522, but GitHub registers closing references only on PRs targeting the default branch. Because this PR's base is issue-1898/port-release-deadline, the link graph holds nothing. Control measurement on a main-based PR for contrast: gh pr view 2541 --json closingIssuesReferences returns [1761].

#2526 is OPEN.

Why it matters. The link self-heals the moment the PR is retargeted to main — so in the intended sequence (land #2522, then gh pr edit 2531 --base main, confirm via the API, then merge) nothing goes wrong. But in the failure mode this stack is already exposed to — merging #2531 while it still points at its stacked base — the delta lands on issue-1898/port-release-deadline instead of main, the PR reports MERGED, and #2526 stays open with no record of why. That is the eighth block's dead-base trap plus a silently orphaned issue.

So the retarget step is load-bearing twice over: it is what gets the code to main, and it is what makes the issue close. Confirm baseRefName == main via the API before merging, then re-check closingIssuesReferences is non-empty.

No action needed on the body — the text is right, only its effect is suppressed by the base.

…ized state token (re-review HIGH-1); drained-connection negative fixtures in both twins
@Chris0Jeky

Copy link
Copy Markdown
Owner Author

Round 2 (coordinator, 2026-09-06, SC-10 delegated). Retargeted to main after #2522 landed (closingIssuesReferences = #2526 confirmed), updated, and the re-review's HIGH-1 addressed in 38710cbd8: both launchers now identify a listener by its foreign address being the wildcard peer (0.0.0.0:0/[::]:0 on Windows netstat; 0.0.0.0:*, :::*, *.* on net-tools/BSD), never by the localized state token, so drained TIME_WAIT/CLOSE_WAIT rows with a real peer no longer count. Negative fixtures: the Bash harness asserts a drained-only port is free (fails against the pre-fix awk) and the PowerShell fake carries a TIME_WAIT row with a real peer beside the listener. Local proof: the two netstat cases green; the full dev-up suite running; a scoped fresh-context verification of the fix diff is in flight. MEDIUM-2/3 from the re-review (the ASCII-substituted ABHOEREN fixture never exercises the OEM code page; the Bash timeout-parity test names a PowerShell parity it does not execute) are recorded on #2526 as follow-ups.

@Chris0Jeky

Copy link
Copy Markdown
Owner Author

Scoped verification of the fix diff (fresh-context, 2026-09-06): SHIP. Column indices confirmed for both netstat shapes; wild_peer covers 0.0.0.0:0, [::]:0, 0.0.0.0:*, :::*, [::]:*, *.*; IPv6 listeners still classify; no other test pins the old behaviour; the Bash negative fails against the pre-fix awk. One MEDIUM fixed on the spot (38710cbd8 → next commit): the PR's earlier ^tcp[46]?$ protocol filter rejected macOS/BSD dual-stack tcp46 rows, which main's grep -Ei LISTEN had matched, a fail-open on a Mac; now ^tcp[46]{0,2}$. Recorded for #2526, not fixed: the PowerShell fixture's TIME_WAIT row shares the listening port, so the PS twin has no mutation resistance of its own (the Bash harness carries the negative); netstat -ano -p TCP is IPv4-only on Windows, so the [::]:0 arm is unreachable there (pre-existing blindness, kept as defence). The hosted Frontend Unit (ubuntu-latest) red at 38710cbd8 was "Node helper: TERM closes an active frontend connection: frontend helper did not bind" (5 s bind wait, dev-up.test.mjs:1083), a Node-helper timing case untouched by this PR; re-run once at the new head.

@Chris0Jeky
Chris0Jeky merged commit c4cca76 into main Sep 6, 2026
35 checks passed
@Chris0Jeky
Chris0Jeky deleted the issue-2526/netstat-state-fallback branch September 6, 2026 14:13
@github-project-automation github-project-automation Bot moved this from Pending to Done in Taskdeck Execution Sep 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

[dev-up] netstat existence fallback matches the LISTEN state by English string (localized Windows yields a false free) — from PR #2522 round 2

1 participant