feat(forward): SSHForwardTracker hierarchy + per-connection tracker_factory for local and remote forwards

[AlexMKX]

Summary

Adds an optional, observer-only hook API for watching the lifecycle of
forwarded connections (open / close / bytes in each direction), driven by a
per-connection tracker_factory. The motivating use case is idle-based
auto-shutdown of a forwarding daemon, but the same hooks support connection
counting and passive traffic metrics.

This reflects the design worked out in the discussion below (superseding the
original single ForwardTracker Protocol proposal).

API

A small class hierarchy mirroring the existing forwarder classes:

• SSHForwardTracker — base class. No-op default hooks shared by all forward
  types:
  • connection_lost(exc) — the forwarded connection closed (exc=None on clean
    close). Fires exactly once per connection.
  • forward_local_bytes(data) — a block forwarded local → tunnel.
  • forward_remote_bytes(data) — a block forwarded tunnel → local.
• SSHPortForwardTracker(SSHForwardTracker) — when the listener is a TCP
  port. Adds connection_made(forwarder, orig_host, orig_port).
• SSHPathForwardTracker(SSHForwardTracker) — when the listener is a UNIX
  domain socket. Adds connection_made(forwarder) (no address args).

A new tracker_factory: Callable[[], SSHForwardTracker] | None = None keyword is
added to the eight forwarding methods, invoked once per accepted
connection. The tracker type follows the listener endpoint, not the
destination:

method	tracker type
forward_local_port	SSHPortForwardTracker
forward_local_port_to_path	SSHPortForwardTracker
forward_local_path	SSHPathForwardTracker
forward_local_path_to_port	SSHPathForwardTracker
forward_remote_port	SSHPortForwardTracker
forward_remote_port_to_path	SSHPortForwardTracker
forward_remote_path	SSHPathForwardTracker
forward_remote_path_to_port	SSHPathForwardTracker

The byte hooks name the host where the data was generated, so for a remote
forward the mapping is mirrored: forward_remote_bytes sees what the client of
the remote listener sent, and forward_local_bytes sees what the local
destination sent back.

Semantics (per discussion)

• Observer-only. Byte hooks return nothing and never alter the forwarded
  data; transform/parsing use cases should drop down to create_connection() /
  open_connection() instead.
• No opt-in machinery for byte hooks — rely on the parent's no-op defaults.
  Hooks fire once per SSH message (not per byte), behind SSH decryption, so the
  Python call cost is negligible in any realistic workload.
• Single connection_lost(exc), symmetric with the single connection_made
  (no per-direction split).
• Every hook has a no-op default, so a subclass overrides only what it needs.
  Exceptions raised by a hook or by the factory are caught and discarded so a
  buggy tracker can never break forwarding.
• connection_lost is guarded so it fires exactly once even though
  _forward() notifies manually on a channel-open failure and the local
  transport's later close would otherwise notify again.

Scope

Local and remote forwarding. forward_socks is still left for a follow-up;
the hierarchy + factory accept it without rework.

Note on review history: this PR originally covered local forwarding only,
and your 1 Aug "these changes all look good" applies to that state, up to
2bf4164. Commits bc19857..96624f4 add the remote support you suggested as
the next step, so that part has not been reviewed yet. The local half is
refactored — the tracker plumbing moved into a shared SSHTrackedForwarder
base so the remote forwarders reuse it instead of duplicating it — but is
intended to be behaviour-preserving, and the existing local tracker tests pass
unchanged.

One lint-config change came with it: connection.py was 26 lines under
max-module-lines, so pylintrc goes 10000 → 11000. Happy to drop that and
take the C0302 instead if you would rather keep the config untouched.

Trackers on a remote forward are created when the listener connection is
accepted, so a connection whose local destination cannot be opened still
produces exactly one tracker, with connection_made followed by
connection_lost(ChannelOpenError). That is the question raised in the thread
below — easy to flip if you would prefer a tracker only once both ends are up.

Docs

A dedicated "Forward Tracker Classes" section in api.rst (factory pattern,
example, and the list of tracker-aware methods). Per your note, no changes.rst
entry is included.

Tests

New tests in tests/test_forward.py covering both tracker subclasses,
connection_made / connection_lost ordering, per-connection factory
invocation, both byte-hook directions, "lost fires exactly once" on a denied
forward, and exception isolation for both a buggy factory and buggy hooks —
each mirrored for the remote methods, including the *_to_path / *_to_port
cross cases.

The byte-direction tests deliberately use a distinct request and response
rather than an echo server: an echo-based test passes even if the two byte
hooks are wired backwards, which was verified by mutation. There is also a
deterministic test that a cancelled destination open still delivers
connection_lost.

[ronf]

Thanks for the PR. Could you say a little more about the use case you have in mind for this? I'm not really seeing how this would be able to do things like byte counts or idle tracking if you are only sending connection_made and connection_lost messages through the tracker.

I'm wondering if some of this could be handled by using the existing accept_handler. It only runs right now on a new client connection coming in, giving you the client IP address and port and even lets you decide based on that whether to allow the forwarded connection or not. It doesn't currently run when a connection closes (either cleanly or with an error), but that could be added as something like an error_handler. In the future, support for a progress_handler could also be added here to report on bytes transferred, much like what exists in AsyncSSH right now for the SFTP/SCP file copy functions.

[AlexMKX]

Hello @ronf, thank you for the questions.

The main use case I have in mind is handling idle forwarded connections. This came from a practical need in this tool: https://github.com/AlexMKX/garuda-tunnel

It is basically a “poor man's Teleport” for automating Kubernetes access through Terraform or an Ansible Kubernetes inventory source. It runs as a daemon, exposes local ports through a bastion, and uses AsyncSSH under the hood.

For safety and operational visibility, I would like to track the number of live forwarded connections and apply an idle timeout to traffic going through the mapped ports. A per-byte counter would be better, of course, but for this use case an active connection count is a reasonable first approximation without inviting a whole committee of extra machinery into the room.

I agree that accept_handler is close to this area. The main gap is that it currently only covers the beginning of the connection. Having something symmetrical for connection close, perhaps an error_handler or close_handler, would probably solve a good part of this. A future progress_handler for byte counts would also be useful and would fit nicely with what AsyncSSH already does for SFTP/SCP transfers.

So I am not strongly attached to the exact tracker API shape from the PR. The important part for me is having a lightweight way to observe connection lifecycle events for forwarded connections, and later possibly traffic progress, without each user of port forwarding having to rebuild that logic around AsyncSSH internals.

[ronf]

Thanks for the added context... I'm in the middle of some other feature work at the moment, but once that's wrapped up I'll take a closer look at this, probably in the next couple of weeks.

I'm thinking that you could still have a ForwardTracker class at the application level if you wanted here, and instantiate that with details of the connection it is associated with. The arguments to accept_handler and close_handler in forward_local_port() could point at the bound methods of that specific ForwardTracker object. The same could apply in the future for tracking byte counts.

In fact, you could probably have a factory method on ForwardTracker that actually called forward_local_port() with all the necessary bound methods as arguments. This will need to be an async method so you can't use the constructor directly, but this should work as an async class method if you wanted to avoid needing to call the constructor first yourself.

[ronf]

I had a chance to get back to this today.

My main concern about what we previously discussed is the lack of the ability to identify specific connections with this approach, especially if we want to extend it later to report specific byte counts being transferred on the connections. For local port forwarding (or even port to path), one could argue that orig_host and orig_port should uniquely identify a specific connection if you had different tracker instances per SSHListener instance, but there's no equivalent of that if we were to extend this to cover forwarding of UNIX domain socket connections. That's actually part of why I never implemented an accept handler for the forward_local_path() and forward_local_path_to_port() cases. There was nothing for the handler to use to decide which connections to allow and which to reject.

One possible way to address this would be to pass in a factory function when setting up forwarding. A new instance of this would be created per forwarded connection, and once forwarding is successfully set up for a connection, the factory function would be called to create a new tracking instance for that connection, and connection_made (and later connection_lost) would be called on these instances.

With this approach, other forms of monitoring should be pretty easy to add. For instance, there could be methods for data_sent() and data_received(), providing the actual data being forwarded, not just byte counts. I'm also thinking the call to connection_made() could possibly provide an SSHForwarder instance, allowing this to be used later to do something like forcibly close a specific connection from the tracker.

[AlexMKX]

Factory-per-connection makes sense — it solves the UNIX-socket identity gap and scales cleanly to byte counts and force-close, which the aggregate observer can't. For my idle use case I'd just have the factory bump a shared counter and return a per-connection object whose connection_lost decrements it.

Before I rework the PR: could you pin the intended shape? Specifically — the parameter name on forward_local_port (tracker_factory?), whether the factory is sync or async, and what connection_made receives (an SSHForwarder, plus orig addr?). I'll adapt the PR (and the docs/tests) to match once the signature is settled.

[ronf]

Yes - tracker_factory seems like a good name for the new argument, and it should default to None as in your original proposal here. This factory function should be synchronous. While there are places where callbacks can be either synchronous or asynchronous, the methods being mirrored here in things like SSHConnection SSHSession are generally synchronous, so I'd go with that as the initial plan here unless you can think of a need that one of these callbacks would need to be async.

To handle both TCP and UNIX domain socket cases, I think we may need two different protocols, though there could be a common parent for most of the methods. The difference would be in either connection_made() or the constructor to take arguments for orig_host and orig_port in the TCP case but no additional arguments in the UNIX domain socket case. I'm thinking probably extra arguments to connection_made() after the SSHForwarder instance, as you said.

In terms of naming on the tracker class, I would have SSHForwardTracker as a parent class with SSHPortForwardTracker and SSHPathForwardTracker as subclasses, mirroring the existing SSHPortForwarder and SSHPathForwarder classes.

We should be able to share a tracker for both forward_local_port() and forward_local_port_to_path(), and similarly share the other tracker for forward_local_path() and forward_local_path_to_port(). I haven't had a chance to think much about whether we'd also want this trackers to work for the equivalent forward_remote methods, but at first thought it seems like we should be able to make all four of those methods work as well here, using the same trackers for both the local & remote cases.

We could also look into supporting forward_socks() in the future, but I think that's something which can wait until we have the base cases working. I could even see splitting up the work to start with only local forwarding with remote coming later, as long as we fill in enough of the design to know it CAN be added later without major re-work.

In terms if the callback methods, I'm thinking:

• connection_made(forwarder: SSHForwarder, orig_host: str, orig_port: int) for TCP or connection_made(forwarder: SSHForwarder) for UNIX domain sockets when a new tunneled connection is opened.
• connection_lost(exc: Optional[Exception]) for reporting a clean close or an error on a tunneled connection.
• forward_local_bytes(data: bytes) for forwarding bytes from a local socket to the SSH connection
• forward_remote_bytes(data: bytes) for forwarding remote bytes from the SSH connection to a local socket
• close() to forcibly close the tunnel.

I'm thinking for now we probably don't need to separately track EOF for tunneled connections. While it's possible for half-open connections to exist, it's not common, especially for forwarding. There MIGHT be value in reporting the optional exception differently for the two directions, since technically we could get a clean close on one side but an exception on the other, or two different exceptions for the two sides. I don't know how useful this would be, but if we did this, I'm thinking connection_lost() would get changed to local_connection_lost() and remote_connection_lost(). Generally, these calls would happen in quick succession, but the ordering of the two would reflect which side closed first (in addition to letting us report different exceptions).

[ronf]

Oh - one other thing. I would not include the changes.rst file in the PR. A new changes.rst entry would only appear when a new release is cut, and would be a much less detailed than what was proposed here. That said, we could add some documentation to the new SSHForwardTracker classes, and maybe even carve out a dedicated section in api.rst for discussing this tracker functionality.

[AlexMKX]

Thanks for your detailed response @ronf .
Sounds great — happy to proceed with this design. tracker_factory + per-connection tracker with the SSHForwardTracker / SSHPortForwardTracker / SSHPathForwardTracker hierarchy reads cleanly and gives a clear migration path for the remote-forward and forward_socks cases later.

A few things I'd like to confirm before I rework the PR:

1. forward_local_bytes / forward_remote_bytes semantics: is this purely an observer (returns None, asyncssh ignores the return value), or a transform (return value replaces what gets forwarded)? Observer fits my idle-tracking use case; transform would be powerful but has very different implications. I'd lean observer for the initial API.

2. Are the byte hooks meant to be always called when present, or optional/opt-in? For low-overhead aggregate trackers (just counting connections), routing every byte through a Python method has a real throughput cost. A way to declare "this tracker doesn't want byte hooks" — either by leaving the method as the parent's no-op default (detected via type(tracker).forward_local_bytes is SSHForwardTracker.forward_local_bytes), or via an explicit attribute — would let metrics-only trackers stay cheap. Happy to follow whatever convention you prefer.

3. connection_lost(exc) vs split local_connection_lost / remote_connection_lost: I'd lean toward keeping the single hook for now, since splitting is forward-compatible (the per-direction variants can be added later without breaking existing code, as long as the single hook fires on first-side-closed). Unless you'd prefer the split to land as part of the initial API, I'll go with one method.

For scope, I'll start with local forwarding (forward_local_port + forward_local_port_to_path sharing SSHPortForwardTracker; forward_local_path + forward_local_path_to_port sharing SSHPathForwardTracker) and leave the remote-forward wiring + forward_socks for follow-up PRs, designed so they slot in without major rework.

Good call on changes.rst — I'll drop it from the PR. For the docs side, I can add docstrings to SSHForwardTracker / SSHPortForwardTracker / SSHPathForwardTracker covering each method's contract (when it's called, what the args are, return-value semantics), plus a dedicated section in api.rst that explains the factory pattern, shows a minimal example, and notes which forward_* methods accept tracker_factory. If you have a preferred section name or placement (e.g., right after the existing forwarding methods, or its own top-level section), let me know — otherwise I'll pick a reasonable spot and you can move it during review.

[ronf]

forward_local_bytes / forward_remote_bytes semantics: is this purely an observer (returns None, asyncssh ignores the return value), or a transform (return value replaces what gets forwarded)? Observer fits my idle-tracking use case; transform would be powerful but has very different implications. I'd lean observer for the initial API.

I was thinking pure observer here. Once you start thinking about transformations or parsing information out of the streams, you're probably better off calling create_connection() on the SSHConnection with your own SSHTCPSessionFactory, or calling open_connection() to get back SSHReader and SSHWriter instance you can use to manipulate the stream. Admittedly, you this would require also manually setting up your own listener to accept new connections, but that can be done with only a few lines of code. The only messy part is trying to get a common listening port if your listen host resolves to multiple IPs.

Are the byte hooks meant to be always called when present, or optional/opt-in? For low-overhead aggregate trackers (just counting connections), routing every byte through a Python method has a real throughput cost. A way to declare "this tracker doesn't want byte hooks" — either by leaving the method as the parent's no-op default (detected via type(tracker).forward_local_bytes is SSHForwardTracker.forward_local_bytes), or via an explicit attribute — would let metrics-only trackers stay cheap. Happy to follow whatever convention you prefer.

Anything you did to query if a method was defined might not be much cheaper than making an unconditional call to an empty function body (which could be provided in the tracker parent class as you said). If we did find that the method lookup was expensive enough to matter, we could always have the tracker parent constructor scan for which methods exist and cache the result in a member variable. However, I'd want to measure the performance before attempting any optimizations of that sort. Keep in mind the byte callbacks are once per SSH message, not per byte, and applications transferring large amounts of data are probably using larger block sizes on writes. Also, all SSH messages have to go through a decryption step, which is likely to be FAR more expensive than a Python method call.

connection_lost(exc) vs split local_connection_lost / remote_connection_lost: I'd lean toward keeping the single hook for now, since splitting is forward-compatible (the per-direction variants can be added later without breaking existing code, as long as the single hook fires on first-side-closed). Unless you'd prefer the split to land as part of the initial API, I'll go with one method.

I'm torn on this one. If we were to split it, it seems a bit asymmetric to not also do the split on connection_made(), but we don't want to do that because we want one tracker object for both sides of the forwarding. Also, this would end up triggering two callbacks back to back in pretty much all cases, when the only value we're getting is information on which side closed first. I'm ok going without the split for now.

For scope, I'll start with local forwarding (forward_local_port + forward_local_port_to_path sharing SSHPortForwardTracker; forward_local_path + forward_local_path_to_port sharing SSHPathForwardTracker) and leave the remote-forward wiring + forward_socks for follow-up PRs, designed so they slot in without major rework.

Sounds good. I probably won't release the feature until at least local and remote are both done, but I like the idea of fully reviewing the local case first to make sure everything looks good. It keeps the size down and there'll be less throw-away work if changes are required.

As for forward_socks(), we'll see how cleanly this fits in, and we can make a call about whether to include that or not.

[AlexMKX]

All three confirmations make sense and I'll go with them as the basis for the rework:

1. Pure observer for byte hooks — clean boundary. Anyone wanting transform/parsing semantics drops down to create_connection() / open_connection().

2. No opt-in machinery for now; rely on no-op defaults in the parent class. The per-message granularity plus the cost of SSH decryption makes the Python method call effectively free in any realistic workload. Revisit only if benchmarks ever say otherwise.

3. Single connection_lost — agreed. The symmetry argument with single connection_made is the right way to look at it; splitting one side without the other would feel arbitrary, and the value of knowing which side closed first is too narrow to justify the API surface.

Scope: I'll send the rework as a local-only PR (forward_local_port + forward_local_port_to_path sharing SSHPortForwardTracker; forward_local_path + forward_local_path_to_port sharing SSHPathForwardTracker), structured so adding the remote-forward methods later is a small follow-up rather than a redesign. forward_socks I'll leave alone for now and we'll decide once the base case lands.

I'll start on it now. A few small implementation-detail questions are likely to come up — whether SSHForwardTracker is an abstract base or a concrete class with no-op defaults users can subclass selectively, what happens if tracker_factory() raises, doc placement in api.rst — but those will be more productive to raise on the actual diff than upfront. I'll ping when the PR is updated.

[ronf]

Generally speaking, all of the AsyncSSH factory classes are meant to be subclassed, with a default implementation of pretty much all of the methods so that only methods an application wants to change need to defined in the subclass.

In terms of placement in api.rst, I was thinking that there was already a top-level section for forwarding, but I see now that's not the case. Instead, there's a paragraph in the "Overview" section about forwarding methods, and then a "Forwarder Classes" section. The new tracker classes can probably be added in "Forwarder Classes" below SSHForwarder, with the details of each callback being described in the method docs, similar to what is done in SSHClient and SSHServer. It's probably also worth a quick mention of these new classes in the Overview section just after the existing forwarding summary.

All of the methods can be listed for the Port and Path versions of the tracker classes, but the implementation of those will mostly be coming from the definition in the parent class in most cases. The exception would be connection_made(), as its arguments will be different. This is similar to SSHTCPSession and SSHUNIXSession.

[AlexMKX]

Reworked the PR to match what we landed on above and force-pushed feat/forward-tracker:

• tracker_factory (per-connection) instead of a single shared tracker, with the SSHForwardTracker / SSHPortForwardTracker / SSHPathForwardTracker hierarchy mirroring the forwarder classes. connection_made(forwarder, orig_host, orig_port) for TCP, connection_made(forwarder) for UNIX.
• Observer-only forward_local_bytes / forward_remote_bytes (no-op defaults on the base, no opt-in machinery), single connection_lost(exc).
• Wired into all four local methods (forward_local_port, forward_local_port_to_path, forward_local_path, forward_local_path_to_port). Remote forwards and forward_socks deliberately left out for a follow-up.
• Dropped changes.rst; added a "Forward Tracker Classes" section to api.rst.

One implementation detail worth flagging for review: connection_lost clears the per-connection tracker on first call so it fires exactly once — _forward() notifies manually on a ChannelOpenError, and the local transport's subsequent close would otherwise deliver a second connection_lost. There's a regression test for that ("lost fires exactly once" on an accept_handler denial).

The PR description above is updated to the new design. Happy to adjust naming, hook signatures, or the api.rst placement on the diff.

[ronf]

This looks great! I've added inline comments, but they're all for pretty minor issues.

[ronf]

Just reviewed your update -- looks good! Based on the changes, I marked most of my previous comments as resolved. There's only a handful left.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.84%. Comparing base (fab96ac) to head (2bf4164).
⚠️ Report is 212 commits behind head on develop.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop     #807      +/-   ##
===========================================
- Coverage    99.85%   99.84%   -0.02%     
===========================================
  Files           95      102       +7     
  Lines        27784    32305    +4521     
  Branches      2908     3112     +204     
===========================================
+ Hits         27743    32254    +4511     
- Misses          38       45       +7     
- Partials         3        6       +3     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[AlexMKX]

Just reviewed your update -- looks good! Based on the changes, I marked most of my previous comments as resolved. There's only a handful left.

It should be better now. Are there anything to fix?

[ronf]

A few comments on the latest changes.

[ronf]

Rather than creating a lambda function here, I wonder if you could pass in one of the bound tracker methods rather than the tracker object itself as the first argument. You'd probably get less type safety, passing the arguments through an "args" argument, but it would avoid the need for the lambda.

An alternative would be to use a nested function definition for the wrapper instead of a lambda, like what's done in forward.py today for defining session_factory in the _forward() method. I think that's a bit easier to read.

[ronf]

Also, pylint is raising a no-self-use error on _notify_tracker(). One option would be to make that a @staticmethod.

[AlexMKX]

Switched to nested function definitions instead of lambdas, matching the session_factory style used in _forward(). Each notify() is annotated with the concrete tracker type at that call site (SSHPortForwardTracker / SSHPathForwardTracker for the connection_made hooks, _Tracker for the shared byte/connection_lost hooks in the base class).

[AlexMKX]

_notify_tracker is now a @staticmethod (generic over _Tracker, still called as self._notify_tracker(...)). Confirmed with pylint (using the repo pylintrc + the no_self_use extension, which is what surfaces this on newer pylint) that the no-self-use finding is gone.

[ronf]

Looks good. Some of the other function names here could probably also use some trimming.

[ronf]

These changes all look good and I'm seeing clean runs on mypy, pylint, and tox (including coverage).

If you're happy with this code I think the next step is to look into adding the "remote" versions of this tracker support. I'm in the middle of addressing some bug reports in the "develop" branch right now, but hopefully by the time the remote support is ready to go, things will quiet down and I'll be able to get this into the next feature release...

Thanks for all your work on this so far!

[AlexMKX]

Thanks! Yes, I am happy with the current state, so I will start on the remote versions.

One design question before I get too far, about the tracker lifecycle boundary: for a connection accepted by the remote listener whose local destination socket cannot be opened, should tracker_factory still be called, and should that tracker then get connection_made followed by connection_lost(...)? I lean towards yes, so the lifecycle mirrors local forwarding (where the tracker is created for the accepted socket, before the SSH channel open can fail) and there is exactly one tracker per accepted listener connection. Happy to go the other way if you would rather a tracker only appear once both ends are established.

[ronf]

Yeah, I think this makes sense. We'd want to know about the incoming connections even if the forwarding target isn't successful. Even if we deferred connection_made() until after finishing the outbound connection, there's no way to report the exception in connection_made(), so it's probably simpler to wait and report that exception in connection_lost(). That also means the generated callbacks look the same regardless of whether the failure is detected before or after the call to connection_made().

[AlexMKX]

Pushed the remote half. Commits bc19857..96624f4 add tracker_factory to the four forward_remote_* methods, with the tracker plumbing lifted into a shared base so the remote forwarders reuse it rather than duplicating it. I have updated the PR title and description to match — the old text said remote forwarding was out of scope, which is no longer true.

To be explicit about review state: your 1 Aug pass applies up to 2bf4164. Everything after that is new and unreviewed.

Two things worth flagging:

• pylintrc max-module-lines 10000 -> 11000, because connection.py was 26 lines under the cap and the four new signatures do not fit under it. Easy to revert if you would rather take the C0302.
• The tracker lifecycle question for a destination that cannot be opened is in the thread above. Current behaviour: the tracker is created when the listener connection is accepted, so it gets connection_made and then connection_lost(ChannelOpenError). Flipping it is a two-line change in each of the two helpers.

Gates on my side: 192 passed in test_forward.py, pylint 9.81 (unchanged message set), mypy clean on forward.py, sphinx builds warning-free.