Skip to content

fix(whatsapp): tolerate localized Windows netstat and taskkill output - #63628

Closed
LceAn wants to merge 1 commit into
NousResearch:mainfrom
LceAn:win-whatsapp-netstat-encoding
Closed

fix(whatsapp): tolerate localized Windows netstat and taskkill output#63628
LceAn wants to merge 1 commit into
NousResearch:mainfrom
LceAn:win-whatsapp-netstat-encoding

Conversation

@LceAn

@LceAn LceAn commented Jul 13, 2026

Copy link
Copy Markdown

What & why

On Windows, netstat and taskkill can emit localized text using the active code page. With text=True and a UTF-8-forced process environment, Python may raise UnicodeDecodeError before the WhatsApp bridge cleanup output is parsed. The outer cleanup guard can then silently leave a stale bridge process or port behind.

Change

  • Add errors="replace" to the Windows netstat call in _kill_port_process().
  • Add the same decoding policy to the text-mode taskkill call in _terminate_bridge_process().
  • Assert the exact decoding kwargs in the Windows mock coverage.

Replacing undecodable status text is safe here: the parser only consumes ASCII port/state/PID tokens, and taskkill output is diagnostic.

Validation

  • Focused Windows netstat/taskkill and POSIX listener tests: 6 passed.
  • ruff and git diff --check pass.

Related #63223 work is complementary rather than duplicate.

@alt-glitch alt-glitch added type/bug Something isn't working comp/plugins Plugin system and bundled plugins platform/whatsapp WhatsApp Business adapter platform/windows Native Windows-specific behavior or breakage sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows P3 Low — cosmetic, nice to have labels Jul 13, 2026
@LceAn

LceAn commented Jul 13, 2026

Copy link
Copy Markdown
Author

Pinging for a review — happy to reword or split if it helps.

Worth flagging that this one fixes a functional failure, not just test noise: on Chinese Windows the _kill_port_process netstat call crashes inside _readerthread before its output is parsed, so the except Exception: pass swallows it and the stale-bridge kill silently no-ops — the port is never released. The fix lets the parse actually run.

It also turns tests/gateway/test_whatsapp_bridge_pidfile.py::test_kill_port_spares_client_process from FAILED → PASSED on Windows (verified on Win 11 / Python 3.13.14). Single-kwarg change (errors="replace"), scoped to the one reproduced site; the other text=True calls in the file are left alone to avoid churning existing mock-arg assertions.

This is the plugins/ instance of the subprocess-encoding class tracked in #47939 (covered elsewhere by #45099 / #52859).

@jbbottoms

Copy link
Copy Markdown
Contributor

Nice pin, @LceAn — the errors="replace" is right, and the ASCII-only downstream parse (LISTENING + port suffix) makes it safe.

One sibling in the same file this PR doesn't cover, same GBK class: _terminate_bridge_process() (master adapter.py:219) runs taskkill /PID <pid> /T [/F] with capture_output=True, text=True and no errors=. taskkill prints localized status on Chinese Windows (成功: / 错误:), so the background reader thread hits the same UnicodeDecodeError under a UTF-8-forced decode — including the LANG=C.UTF-8 that scripts/run_tests.sh sets.

Two things make it a touch hotter than the netstat site:

  • it's only wrapped in except FileNotFoundError, not except Exception: pass — so the decode error propagates out and crashes the bridge-teardown path instead of silently no-opping the kill;
  • on a non-zero return it reads result.stderr or result.stdout straight into raise OSError(details), so the localized bytes are exactly what it's trying to surface.

(The inner taskkill at adapter.py:101 is fine — it's bytes-mode, no text=True.)

Same one-line remedy — errors="replace" on the _terminate_bridge_process call. Happy to leave it to you to fold into this PR, or into the #45099 / #52859 errors='replace' sweep if the maintainers would rather keep the plugins/ instances together. Just flagging so the same-file sibling doesn't get left behind.

@teknium1

Copy link
Copy Markdown
Contributor

Thanks for isolating a real Windows cleanup failure. Current main still has the reported netstat call with text=True and no decoding error policy at plugins/platforms/whatsapp/adapter.py:89-93; the proposed replacement policy is safe because the parser only acts on ASCII tokens at adapter.py:94-104.

Problems

  • The same decode failure class remains in _terminate_bridge_process(): Windows taskkill uses capture_output=True, text=True without errors= at plugins/platforms/whatsapp/adapter.py:223-228, and decoded stderr/stdout is used at line 237. disconnect() reaches that helper at lines 791 and 797.
  • The existing netstat test at tests/gateway/test_whatsapp_connect.py:471-501 does not assert the decoding kwargs.

Suggested changes

  • Fold errors="replace" into the taskkill call and update its exact mock expectation at tests/gateway/test_whatsapp_connect.py:590-595.
  • Assert errors="replace" on the netstat call so this Windows regression is covered without requiring a localized host.

Automated hermes-sweeper review.

@teknium1 teknium1 added sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users labels Jul 16, 2026
@alt-glitch alt-glitch added the needs-decision Awaiting maintainer decision before any implementation label Jul 16, 2026
@LceAn
LceAn force-pushed the win-whatsapp-netstat-encoding branch from 3d999bd to dbc512f Compare July 18, 2026 08:06
@alt-glitch alt-glitch removed the needs-decision Awaiting maintainer decision before any implementation label Jul 18, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Related to #63223: the live diff now covers both the netstat and taskkill Windows text-decoding sites and includes an exact kwargs test. This is related repair work, not a duplicate.

@LceAn

LceAn commented Jul 18, 2026

Copy link
Copy Markdown
Author

Updated per review. Added errors="replace" to both the netstat and _terminate_bridge_process() taskkill text calls, updated the exact taskkill mock expectation, and added stable netstat kwargs assertions (including the decoding policy).

Focused validation: the two mock tests plus the native-listener regression all pass (3 passed). The canonical two-file run is currently blocked at collection by the existing Windows clean-env HOME/USERPROFILE issue, before these tests execute; the direct focused run passes.

@alt-glitch alt-glitch added the needs-decision Awaiting maintainer decision before any implementation label Jul 18, 2026
@LceAn
LceAn force-pushed the win-whatsapp-netstat-encoding branch from dbc512f to c211080 Compare July 20, 2026 02:59
@LceAn LceAn changed the title fix(whatsapp): decode netstat output with errors=replace on Windows fix(whatsapp): decode localized netstat and taskkill output on Windows Jul 20, 2026
@LceAn

LceAn commented Jul 20, 2026

Copy link
Copy Markdown
Author

Maintenance update: rebased onto current main, normalized the touched files back to LF, and refreshed the title/body to match the live patch. The reviewed netstat + taskkill decoding fixes and exact kwargs coverage remain intact; the diff is now a focused +11/-3. The related #63223 work is complementary, not duplicate.

@alt-glitch alt-glitch removed the needs-decision Awaiting maintainer decision before any implementation label Jul 20, 2026
Decode both Windows netstat and taskkill text output with
errors="replace" so localized bytes cannot abort stale-port cleanup or
bridge teardown. Add stable mock assertions for the netstat decoding
policy and update the exact taskkill expectation.

Addresses review feedback on NousResearch#63628.
@LceAn
LceAn force-pushed the win-whatsapp-netstat-encoding branch from c211080 to d2563f0 Compare July 20, 2026 05:13
@LceAn LceAn changed the title fix(whatsapp): decode localized netstat and taskkill output on Windows fix(whatsapp): tolerate localized Windows netstat and taskkill output Jul 20, 2026
@LceAn

LceAn commented Jul 20, 2026

Copy link
Copy Markdown
Author

Second-pass maintenance: retained the minimal errors="replace" policy after checking UTF-8 mode and locale behavior. Both Windows text-decoding sites and their exact kwargs coverage remain in the pushed head; focused tests pass.

@teknium1

Copy link
Copy Markdown
Contributor

Closing as superseded: both production hunks are strictly subsumed by the encoding='utf-8', errors='replace' guards that shipped on main in PR #70875 (whatsapp adapter netstat/taskkill sites included — e.g. adapter.py:91), and the PR's test assertions would now fail against main's stronger kwargs. Verified per-hunk against current main before closing. Thanks for the report and fix — the volume of independent PRs on this bug class is what escalated it to the class-wide close-out.

@teknium1 teknium1 closed this Jul 24, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/plugins Plugin system and bundled plugins P3 Low — cosmetic, nice to have platform/whatsapp WhatsApp Business adapter platform/windows Native Windows-specific behavior or breakage sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants