HarperFast / HarperFast/symphony

idleTimeoutMs is a maximum connection duration, not an idle timeout

Open
#34 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
Rust
Stars
1
Forks
0
PR merge metrics
No merged PRs in 30d

Description

Pre-existing on `main`, surfaced by the CI review bot on #33 (which only touched the error mapping around this code, not the timeout itself).

`proxy_conn.rs::forward` wraps the whole copy future in `tokio::time::timeout`:

```rust
match timeout(idle, body).await {
Ok(result) => result.map_err(|_| ErrorKind::Stream),
Err(_) => Err(ErrorKind::IdleTimeout),
}
```

`tokio::time::timeout` is a hard deadline — it does not reset on I/O activity. So `idleTimeoutMs` terminates any connection once its **total** duration exceeds the value, however busy it has been. A large file transfer, a long-lived MQTT subscription, or a streaming response all get cut at the deadline mid-flight.

The default is 60s, so out of the box any connection lasting longer than a minute is killed.

## Impact

host-manager already works around this without naming it — it sets `idleTimeoutMs: 0` on every listener, with a comment that 60s "would reap idle keep-alive sockets" and is below the lifetime of long-lived MQTT/replication connections. That disables the feature entirely rather than fixing it, so today there is effectively no idle timeout in the Fabric deployment.

Anyone who does set a non-zero `idleTimeoutMs` — following the README, which documents it as "Close connections silent for this many ms" — gets a duration cap instead.

## Fix sketch

A real idle timeout needs a deadline that resets on activity. Options:

- Wrap the streams in an adapter that bumps a shared `Instant` on every successful read/write, with a periodic checker task — composes with the existing `CountingStream`, which already sits on the client side of the copy.
- Or drive the copy manually with `tokio::select!` against a `tokio::time::Sleep` that gets `reset()` on each transfer.

Either way `ErrorKind::IdleTimeout` (added in #33 for the exported metrics) then means what its label says. Until this is fixed, that metric counts duration-cap terminations.

## Note

Not fixed in #33 deliberately — it is a behaviour change on the proxy's hottest path and unrelated to the metrics work. #33 documents the current semantics where the metric is defined.

Contributor guide

Open the contributing guide

Research direction

Read proxy_conn.rs::forward and the README's idleTimeoutMs documentation first; compare the documented idle semantics with the timeout wrapping described here. Trace how CountingStream and ErrorKind::IdleTimeout participate in the copy path, then validate that a non-zero timeout resets on activity and that long active transfers are not terminated at the configured duration.

Written by the indexing model from the issue text.

Assessment

Tech stack
rust
Domain
networking
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
45/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.