Skip to content

fix: preserve client tunnel data during async relay setup - #7

Merged
LukeGus merged 2 commits into
Termix-SSH:dev-1.3.0from
zyjohn-design:fix/client-tunnel-first-data-race
Nov 7, 2026
Merged

LukeGus merged 2 commits into
Termix-SSH:dev-1.3.0from
zyjohn-design:fix/client-tunnel-first-data-race

Conversation

@zyjohn-design

Copy link
Copy Markdown

Overview

A SOCKS5 client can send its first application bytes immediately after CONNECT succeeds. The desktop socket is still flowing while openRelay() awaits link lookup, so those bytes can be lost before the forwarding handler is attached, leaving HTTP requests pending.

Changes Made

  • Pause the local socket before async relay setup, then resume after installing the data handler. Existing initialData and the ready-message queue preserve byte order.
  • Add a deterministic regression using a flowing Node stream and a deferred getLink() Promise. It verifies bytes arriving during link lookup, initialData, subsequent bytes, and waiting for relay readiness.
  • Add pause/resume to the existing socket mock. No UI, manifest or protocol changes.

Related Issues

Fixes #6

Validation

  • Regression on original source: fails because the first chunk is missing.
  • With fix: npm run test passes (29 files, 153 tests).
  • tsc -p . passes with TypeScript 5.9.3 installed separately; the repo does not currently declare the tsc executable.
  • npm run build, npm run validate, and npm run format:check pass.
  • A local desktop hotfix also allowed the previously stuck browser login to complete.

Checklist

  • Code follows the project style
  • Works on mobile and desktop (no UI change; desktop tunnel backend only)
  • Tests pass and new code has tests
  • Added the change to CHANGELOG.md (left for the release version; validator rejects Unreleased and requires the newest notes to match the manifest)
  • I have read CONTRIBUTING.md

Suggested release note: Client tunnels keep incoming data while preparing the relay, so the first SOCKS5 request is not lost while link lookup is pending.

@termix-triage-bot

termix-triage-bot Bot commented Nov 2, 2026 •

Copy link
Copy Markdown

AI review | Needs changes

The fix correctly prevents data loss during async relay setup, but leaves the socket paused if setup fails, causing connection hangs.

  • major src/backend/c2s-local/runtime.ts:240 socket.pause() is called at the start of openRelay, but socket.resume() is only reached if all async setup succeeds. If openSocket() or any intermediate step throws, the socket remains paused indefinitely, causing the client connection to hang and leak resources. Wrap the setup logic in a try/finally block to guarantee resume() is called, or explicitly resume/destroy on error.

Reviewed 011bf06. AI can be wrong, a maintainer still decides. Comment /termix review to run it again.

@zyjohn-design

Copy link
Copy Markdown
Author

The automated review raised a paused-socket leak on setup errors. The existing production callers already destroy the client socket on rejection:

  • handleLocalConnection() catches openRelay() rejection and calls socket.destroy().
  • handleDynamicConnection() catches rejection and calls fail(), which writes the SOCKS failure and destroys the socket.

I kept the existing caller-owned error handling and added two regression cases in 011bf06: both LOCAL and DYNAMIC connections must destroy their client socket when asynchronous link lookup rejects after pausing it. Resuming an unhandled stream on an error path would not help forwarding.

Validation after this addition: 29 test files / 155 tests pass; typecheck, manifest validation and formatting pass. The first-payload regression still fails on original source and passes with the fix.

@LukeGus
LukeGus changed the base branch from main to dev-1.3.0 November 7, 2026 03:16
@LukeGus
LukeGus merged commit 39a864d into Termix-SSH:dev-1.3.0 Nov 7, 2026
2 checks passed
LukeGus added a commit that referenced this pull request Nov 7, 2026
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.

Client SOCKS5 tunnels can drop the first payload during async relay setup

2 participants