Skip to content

fix(devices): stop wrapper-launched tailnet watchers; skip alias-only network tests on macOS - #3329

Merged
DoodleBears merged 4 commits into
mainfrom
claude/project-thread-jfi3qh
Nov 3, 2026
Merged

DoodleBears merged 4 commits into
mainfrom
claude/project-thread-jfi3qh

Conversation

@DoodleBears

@DoodleBears DoodleBears commented Nov 2, 2026 •

Copy link
Copy Markdown
Contributor

Requested by Vain · project thread

Why the change

The full Core suite fails locally on a Mac with Tailscale (30 failures) while CI and Linux are green; one of the two causes is a real Host shutdown hang.

Issue / Discussion: found while validating #1950; no tracking issue.

Special things to note

  • Merge risk: two-way door, small blast radius. Runtime change is limited to how the tailnet watcher subprocess is spawned and stopped (bot-tailnet-watch.ts); the rest is test-only. Review focus: process-group kill (process.kill(-pid)) and its fallback for injected children; Windows behavior is unchanged (no detached, plain child.kill()).
  • Verification:
    • Mac reproduction on clean main (Node 24.21.0, 8 CPUs, Tailscale running, tailscale on PATH = 2-line /bin/sh wrapper without exec): 30 failed in the full run, 27 serially, all explained by the two causes below. Running Hosts / UDP 43288 were ruled out (removing tailscale from PATH cleared cause B with those Hosts still up).
    • New regression test stops a wrapper-launched watcher and the process it started when disposed times out (10 s) without the fix and passes with it (Linux).
    • Simulated macOS loopback (only 127.0.0.1 bindable): the 4 affected files report 17 skipped, 1 passed, 0 failed — the same 17 tests that failed on the Mac.
    • Full vitest run packages/core/test on Linux: 433 files / 4087 tests passed. oxfmt --check, oxlint, tsc --noEmit clean.
    • Not yet run: the Core suite on the Mac against this branch.
  • CI test plan: affected (core). No new config file or test directory; the new packages/core/test/loopback-aliases.ts helper sits in an existing test directory.

Change outline

A. Real bug: tailnet watcher dispose hangs behind a shell wrapper (plugin.test.ts ×7–9, dm-turn.test.ts ×3, bot-group-review-rounds ×1 under load; all Hook timed out in 10000ms in ctx.fiber.dispose())

before: spawn(tailscale debug watch-ipn) → sh wrapper → real Tailscale (child, no exec)
        dispose → child.kill() → only sh dies; real watcher reparented to PID 1, keeps stdout
        → 'close' never fires → await exits never resolves → dispose hangs (and the watcher leaks)
after:  spawnOwnedProcess(): detached (own process group) on POSIX
        stopOwnedProcess(): kill(-pid, SIGTERM) → wrapper and real watcher both stop
        on 'exit': destroy stdout so 'close' can't be held open by a leftover descendant

The same hang applies to a real Host with this wrapper on PATH (shutdown / plugin reload), not just tests.

B. Environment: tests bind 127.0.0.x that macOS lacks (17 tests in bot-tailnet-bridge, bot-network-continuity, network-withdrawal, network-lifecycle; network-listener-unavailable / EADDRNOTAVAIL)

+ const aliases = await loopbackAliasesAvailable(['127.0.0.12', '127.0.0.13']);
- it.each([...])(
+ it.skipIf(!aliases).each([...])(

The bridge identifies peers by source address, so these tests need distinct loopback IPs; Linux CI keeps running them. On macOS they skip with a console hint (sudo ifconfig lo0 alias 127.0.0.N to run them locally).

A bilingual changes/ fragment for the shutdown fix follows in the next commit (it needs this PR's number).


Generated by Claude Code

Summary by CodeRabbit

  • Bug Fixes
    • Improved cleanup when the tailnet watcher is launched through a shell wrapper. Stopping the watcher now also stops the Tailscale process started by the wrapper, and shutdown no longer waits indefinitely for an output stream held open by a leftover process.
    • Network continuity checks now skip when required loopback addresses are unavailable, preventing environment-specific test failures.

…nly tests on macOS

Core tests failed only on a Mac with Tailscale installed:

- watchTailnet spawned `tailscale debug watch-ipn`; when `tailscale` on PATH is a
  /bin/sh wrapper without exec, child.kill() stopped only the wrapper, the real
  watcher kept the stdout pipe open, 'close' never fired and plugin dispose hung
  (hook timeouts in plugin.test.ts, dm-turn.test.ts). Spawn the watcher in its own
  process group, signal the group, and drop stdout once the child has exited.
- Real-bridge tailnet tests bind 127.0.0.x addresses that macOS lacks without
  lo0 aliases (EADDRNOTAVAIL -> network-listener-unavailable). Probe the
  addresses and skip with a hint instead of failing.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Lpx6qjn5hiharccxWBTehc
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Lpx6qjn5hiharccxWBTehc
@DoodleBears DoodleBears self-assigned this Nov 2, 2026
@coderabbitai

coderabbitai Bot commented Nov 2, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review.

Recent review info
Run configuration
  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: defaults
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 916cdcb4-720a-4aeb-8605-968c1c5e643e
Commits

Reviewing files that changed from the base of the PR and between 5c32be7 and 5d2c1b8.

Files selected for processing: 2
  • packages/core/test/bot-tailnet-bridge.test.ts
  • packages/core/test/bot-tailnet-watch.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 1 remain after this review.

Walkthrough

The tailnet watcher now manages process groups on non-Windows platforms and stops owned processes on failure and shutdown. Network integration tests check required loopback aliases and skip when those aliases are unavailable.

Changes: Tailnet watcher shutdown

Layer / File(s) Summary
Owned process lifecycle
packages/core/src/devices/bot-tailnet-watch.ts, packages/core/test/bot-tailnet-watch.test.ts, changes/3329-tailnet-watch-wrapper.md
The watcher starts an owned process and stops its process group on non-Windows platforms. Failure and shutdown use the same stop helper. A test checks that disposing a watcher also stops a child launched through a shell wrapper. English and Chinese release notes describe the change.

Loopback-dependent integration tests

Layer / File(s) Summary
Loopback alias availability check
packages/core/test/loopback-aliases.ts
The helper attempts to bind each supplied address, closes successful listeners, warns about unavailable addresses, and returns whether all addresses are available.
Gate network tests on available aliases
packages/core/test/bot-network-continuity.test.ts, packages/core/test/bot-tailnet-bridge.test.ts, packages/core/test/network-lifecycle.test.ts, packages/core/test/network-withdrawal.test.ts
The network tests check the loopback addresses they require and skip when any are unavailable. Their existing test scenarios and assertions remain in place.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix

Possibly related PRs

  • BotHarness/DeepSeekBot#3262: Adds the original tailnet watcher and network tests updated in this pull request.
  • BotHarness/DeepSeekBot#472: Removes Core source comments under the source policy; this pull request also includes source-comment cleanup in the Core tailnet watcher.
Merge Risk: Minimal · up to 5d2c1

No merge-blocking risk is identified from the reviewed changes.

Pre-merge checks: Passed 4 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat… Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks: 4 passed
Check name Status Explanation
Title check Passed The title clearly identifies both primary changes: stopping wrapper-launched tailnet watchers and skipping loopback-alias-dependent tests on macOS.
Description check Passed The description includes the required Why, Special things to note, verification results, CI test plan, and Change outline sections. It explains the causes, implementation, risks, test coverage, and re…
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Full details: Description check

Explanation

The description includes the required Why, Special things to note, verification results, CI test plan, and Change outline sections. It explains the causes, implementation, risks, test coverage, and remaining macOS verification gap.

Full details: Docstring Coverage

Explanation

Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 5 functions across 7 files.

Resolution

Write docstrings for the functions missing them to satisfy the coverage threshold.

Finishing Touches
Generate docstrings
  • Commit to this branch
  • Create a new PR
Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

Share

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Lpx6qjn5hiharccxWBTehc

@coderabbitai coderabbitai Bot left a comment •

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Fix comments: Fix CodeRabbit comments on this PR

…t zombies as stopped

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Lpx6qjn5hiharccxWBTehc
@DoodleBears
DoodleBears merged commit b39b9d4 into main Nov 3, 2026
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants