fix(tunnel): verify process identity before stopping cloudflared

[latenighthackathon]

Summary

stopService signalled whatever PID was recorded in cloudflared.pid after only an isAlive check. If the recorded cloudflared had exited and the OS recycled its PID to an unrelated process, that process could receive SIGTERM or SIGKILL. This verifies the live PID still names cloudflared before either signal and removes a stale PID file without signalling a bystander.

Related Issue

Closes #7643

Changes

• Add an injectable process-control seam so PID reuse and signalling are tested deterministically without spawning or signalling host processes.
• Revalidate the recorded PID's cloudflared identity before SIGTERM.
• Revalidate identity again immediately before SIGKILL escalation.
• Remove the stale PID file without signalling when the PID belongs to another process.
• Add regression coverage for both an already-recycled PID and recycling between SIGTERM and escalation.
• Add deterministic coverage for persistent cloudflared escalation (SIGTERM → SIGKILL) and PID-file cleanup.

Type of Change

• Code change (feature, bug fix, or refactor)
• Code change with doc updates
• Doc only (prose changes, no code sample modifications)
• Doc only (includes code sample changes)

Quality Gates

• Tests added or updated for changed behavior
• Existing tests cover changed behavior — justification:
• Tests not applicable — justification:
• Docs updated for user-facing behavior changes
• Docs not applicable — justification: Internal service-lifecycle safety behavior; commands, configuration, and supported platforms are unchanged.
• Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging)
• Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: Codex Desktop completed a serialized nine-category review of exact signed/Verified head 5700f6a22d2dd791b63bba7ddd7cf460800f1299 on current base 94c0b07efb7bcc5d9ddd1fd590a2da26514bbcec. Categories 1–8 PASS. Category 9 has a non-blocking residual warning: command-line identity lookup and POSIX signalling are not one OS-atomic process-handle operation. This is the explicit conservative compatibility boundary accepted by cloudflared stop can SIGKILL an unrelated process when the recorded PID has been recycled #7643; the exact Advisor reports no blockers, CodeRabbit’s actionable threads are resolved, and the deterministic tests cover a pre-signal bystander plus recycling before escalation. Exact diff fingerprint: 44af53152b9cd4b61a10557334ac4bea0a101a6288f19c932e24eef295d450bc (git diff --binary origin/main...HEAD | shasum -a 256).
• Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue:

Documentation Writer Review

• Documentation writer reviewed the completed changes
• Result: no-docs-needed
• Evidence: Exact head 5700f6a22d2dd791b63bba7ddd7cf460800f1299 changes internal tunnel process-lifecycle safety, regression coverage, and a typed readiness test fixture. User-facing commands, configuration, supported platforms, and documented tunnel behavior are unchanged. Focused tunnel tests pass 38/38, the readiness fixture passes 2/2, and CLI build/typecheck plus normal commit/pre-push hooks pass.
• Agent: Codex Desktop (serialized main task)

DGX Station Hardware Evidence

• Tested on DGX Station
• Tested commit: Not applicable; the DGX Station test-only fixture update carries the required immutable source-revision field and does not change Station runtime behavior.
• Station profile/scenario: Not applicable.
• Result: Not applicable.
• Supporting evidence: Not applicable.

Verification

• PR description includes a Signed-off-by: line and every commit appears as Verified in GitHub
• Normal pre-commit, commit-msg, and pre-push hooks passed
• Targeted behavior tests pass for the current change set — tunnel service tests 38/38; Station release-marker tests 2/2; CLI build and typecheck pass at exact head 5700f6a22.
• Applicable broad gate passed — fresh exact-head GitHub CI/E2E/automated review is running after the test-only repair.
• Quality Gates section completed with required justifications or waivers
• No secrets, API keys, or credentials committed
• npm run docs builds without warnings (doc changes only)
• Doc pages follow the style guide (doc changes only)
• New doc pages include SPDX header and frontmatter (new pages only)

Signed-off-by: latenighthackathon latenighthackathon@users.noreply.github.com

Summary by CodeRabbit

• Bug Fixes
  • Improved tunnel shutdown safety by verifying the recorded process is the expected service before sending termination signals.
  • Prevented unrelated processes from being stopped when a process ID has been recycled.
  • Stale process ID records are now cleaned up automatically, including scenarios where the PID’s liveness changes during shutdown.
  • Ensured escalation during shutdown does not go beyond the intended first termination signal unless the process still matches expectations.

[coderabbitai]

📝 Walkthrough

Walkthrough

Changes

PID termination safety

Layer / File(s)	Summary
Add process-control seam and shutdown identity guards src/lib/tunnel/services.ts	stopService validates that the recorded PID belongs to cloudflared before signaling, rechecks identity before escalation, and supports injected process-control operations through stopAll.
Test recycled PID handling src/lib/tunnel/services.test.ts	Scripted process-control tests verify unrelated processes receive no signals and recycled PIDs do not receive SIGKILL while stale PID files are removed.

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

Suggested reviewers: cv, thabhelo, brandonpelfrey

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	The changes match #7643 by verifying cloudflared identity before signaling, removing stale PID files, and rechecking before SIGKILL.
Out of Scope Changes check	✅ Passed	The diff stays focused on the stop-path fix and its regression tests, with no obvious unrelated changes.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main change: verifying process identity before stopping cloudflared.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/cloudflared-stop-verify-pid-identity

---

_{Comment @coderabbitai help to get the list of available commands.}

[github-code-quality]

Code Coverage Overview

Languages: TypeScript

TypeScript / code-coverage/plugin

The overall coverage in commit 5700f6a in the fix/cloudflared-stop... branch remains at 96%, unchanged from commit 94c0b07 in the main branch.

TypeScript / code-coverage/cli

The overall coverage in commit 5700f6a in the fix/cloudflared-stop... branch remains at 81%, unchanged from commit 94c0b07 in the main branch.

Show a code coverage summary of the most impacted files.

File	main 94c0b07	fix/cloudflared-stop... 5700f6a	+/-
src/lib/shields/index.ts	69%	69%	0%
src/lib/sandbox...rce-identity.ts	88%	88%	0%
src/lib/tunnel/services.ts	73%	75%	+2%
src/lib/domain/.../connect-env.ts	89%	97%	+8%

---

_{Updated July 31, 2026 16:01 UTC}

[coderabbitai]

Actionable comments posted: 3

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/lib/tunnel/services.test.ts`:
- Around line 494-513: Update the test around stopAll, the /proc state
inspection, and its assertions to use try/finally cleanup. Ensure
logSpy.mockRestore() and the bystander process termination via
process.kill(bpid, "SIGKILL") run in finally blocks even when stopAll,
inspection, or assertions throw, while preserving the existing process-state
checks.
- Around line 482-517: Replace the real sleep process, /proc inspection, and
process.kill cleanup in the test “does not signal a recycled PID whose process
is not cloudflared” with the tunnel adapter/fake pattern used by the surrounding
tests. Configure the fake process lookup to model a recycled PID belonging to a
non-cloudflared process, assert that stopAll does not signal it, and retain the
PID-file removal assertion without broad host interactions.

In `@src/lib/tunnel/services.ts`:
- Around line 352-361: Revalidate PID ownership immediately before the SIGKILL
escalation in the tunnel shutdown flow, using readProcessCommandLine and
commandLineNamesCloudflared as in the existing pre-SIGTERM check. If the live
PID no longer identifies cloudflared, log that it is not running, remove the
stale PID via removePid, and return without sending SIGKILL.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: e0f44edf-f2ab-4f39-beaf-8f8056a51fda

📥 Commits

Reviewing files that changed from the base of the PR and between 1bc0283 and 2ae67fb.

📒 Files selected for processing (2)

• src/lib/tunnel/services.test.ts
• src/lib/tunnel/services.ts

[github-actions]

PR Review Advisor — No blocking findings reported

Advisor assessment: No blocking advisor findings reported
Next action: No advisor follow-up needed.
Findings: 0 blockers · 0 warnings · 0 suggestions

Model lanes

• GPT-5.6 Terra (primary): Completed · high confidence · 0 blockers · 0 warnings · 0 suggestions
• Nemotron 3 Ultra (second opinion): Completed · high confidence · 0 blockers · 0 warnings · 0 suggestions
• Model comparison: normalized findings match; normalized E2E selections differ; severity counts match.

Nemotron output stays in workflow artifacts and does not change the assessment above.

E2E guidance

Advisory only. E2E / PR Gate selects and runs jobs independently.

Recommended E2E: None

1 optional E2E recommendation

• tunnel-lifecycle

Workflow run details

This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge.

[coderabbitai]

🧹 Nitpick comments (1)

src/lib/tunnel/services.ts (1)

146-162: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Consider consolidating the duplicated identity-check logic.

pidIsOurs (new) and the inline check in readCloudflaredState (Line 283: cmdline !== null && !commandLineNamesCloudflared(cmdline)) both implement the same "null-or-cloudflared" rule, but readCloudflaredState still calls readProcessCommandLine/process.kill directly instead of going through ProcessControl. Extracting a shared predicate (or having readCloudflaredState accept an injectable ProcessControl) would avoid the duplication and let that function benefit from the same deterministic test seam.

As per path instructions (src/lib/README.md), "align with the adapter/injection pattern where process/FS interactions are involved."

Also applies to: 282-285, 364-367

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/lib/tunnel/services.ts` around lines 146 - 162, Consolidate the
duplicated PID identity check by reusing pidIsOurs from readCloudflaredState and
related lifecycle paths. Route command-line lookup and signaling through the
injected ProcessControl interface instead of calling readProcessCommandLine or
process.kill directly, preserving the null-or-cloudflared rule and enabling
deterministic tests.

Source: Path instructions

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@src/lib/tunnel/services.ts`:
- Around line 146-162: Consolidate the duplicated PID identity check by reusing
pidIsOurs from readCloudflaredState and related lifecycle paths. Route
command-line lookup and signaling through the injected ProcessControl interface
instead of calling readProcessCommandLine or process.kill directly, preserving
the null-or-cloudflared rule and enabling deterministic tests.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 160ce322-dbab-4ff4-8209-8113a41afcaa

📥 Commits

Reviewing files that changed from the base of the PR and between 2ae67fb and 5468a3b.

📒 Files selected for processing (2)

• src/lib/tunnel/services.test.ts
• src/lib/tunnel/services.ts

[cjagwani]

I reviewed the current exact two-file diff. The identity checks are scoped to accepted issue #7643, and the fail-open behavior for an unreadable command line matches that issue’s explicit compatibility boundary. I’m keeping this as a comment, not a changes-requested review.

Two mechanical items remain before exact-head approval:

• the PR base is still a4a5aa2d8, while current main is 795de982e; maintainer edits are disabled, so please merge current main into this branch;
• please add the advisor-identified scripted ProcessControl case where PID 4242 remains live and names cloudflared through escalation, asserting SIGTERM then SIGKILL and PID-file removal. The rejection paths are covered, but this changed allowed branch is not.

Once that quiet refreshed head is available, I’ll babysit its focused test, current CI, signatures, and final approval.

[cjagwani]

Exact-head approval for f350d4b after the one-hour quiet check. All 50 GitHub checks are green; the focused tunnel suite passes 37/37 locally; CLI typecheck passes with the PR-pinned dependency tree; all seven commits are Verified; and there are no unresolved major findings. Security review PASS across credentials, input handling, authorization, dependencies, error exposure, data protection, configuration, tests, and system/process safety. The stop path now validates cloudflared identity before SIGTERM and revalidates before SIGKILL, with deterministic recycled-PID regressions. The conservative unreadable-command-line behavior is the explicit accepted #7643 contract. GitHub reports no conflict, and the user waiver applies to base-only refreshes.