`bitreq`: Evict dead entries from async connection pool by tnull · Pull Request #564 · rust-bitcoin/corepc

tnull · 2026-04-22T10:28:30Z

Fixes#562.

Validate pool entries on acquire and clean up after every send:

Remove entries whose keep-alive deadline has passed or whose inner
state has been poisoned, rather than blindly cloning the Arc.
Evict on send failure, Connection: close, or malformed
Keep-Alive.
Refresh LRU position on a cache hit.

Pre-send insertion of a fresh Arc on a miss is kept so concurrent
callers arriving during the first round-trip can share the socket for
pipelining; reusable_until and the post-send eviction together
ensure they never observe a known-dead Arc.

Expose a `pub(crate)` view of whether an `AsyncConnection` is still healthy and, if so, until when. `None` means the inner `next_request_id` has been poisoned (every failure path in `send` already sets this); `Some(t)` is the current `socket_new_requests_timeout`, which `send` already refreshes from the server's `Keep-Alive: timeout=N` header. No behavioural change; groundwork for the pool to check entry validity without duplicating tracking. Co-Authored-By: HAL 9000

Validate pool entries on acquire and clean up after every send: - Remove entries whose keep-alive deadline has passed or whose inner state has been poisoned, rather than blindly cloning the `Arc`. - Evict on send failure, `Connection: close`, or malformed `Keep-Alive`. - Refresh LRU position on a cache hit. Pre-send insertion of a fresh `Arc` on a miss is kept so concurrent callers arriving during the first round-trip can share the socket for pipelining; `reusable_until` and the post-send eviction together ensure they never observe a known-dead `Arc`. Co-Authored-By: HAL 9000

Drives `a, b, a, c, a` through a `Client` with capacity 2, backed by three `tokio::net::TcpListener` servers so each URL resolves to a distinct `ConnectionKey`. On the fix, step 3 promotes `a` to most-recent, step 4's c-insert evicts `b`, and step 5 reuses the warm `a` entry — three TCP accepts. Pre-fix, `a` stays at the front of the LRU queue and gets evicted at step 4 instead — four accepts. Covers one regression only: the LRU refresh on hit. The other architectural improvement on this branch — explicit pool-layer eviction on failure / `Connection: close` — produces the same externally-observable behaviour as the pre-fix code because `AsyncConnection::send`'s interior `retry_new_connection!` machinery compensates by swapping poisoned inner state on the next use. That change is defended on architectural grounds (explicit pool-layer logic instead of reliance on the inner retry) rather than via a black-box regression test. Co-Authored-By: HAL 9000

storopoli · 2026-04-22T15:40:17Z

+ /// Removes any pool entry for `key`. No-op if the slot is already empty.
+ fn evict(&self, key: &ConnectionKey) {
+ let mut state = self.r#async.lock().unwrap();
+ state.connections.remove(key);


I think this should evict by identity, not just by ConnectionKey.
There is a race where an older poisoned Arc can finish later and remove a newer healthy entry for the same key:
tasks A and B are both using the old pooled connection for K
A fails and marks the old connection dead
task C opens and inserts a fresh connection for K
B reaches this cleanup path later and remove(key) drops C's fresh entry
That doesn't break in-flight requests, but it does create avoidable reconnect churn under load. I think evict needs to compare the currently pooled Arc against the one that just finished and only remove on Arc::ptr_eq.
Permalinks:
send_async cleanup: https://github.com/tnull/corepc/blob/708d9c388aa148eb3f11a2b3e5d72d361ab116bf/bitreq/src/client.rs#L87-L95
evict: https://github.com/tnull/corepc/blob/708d9c388aa148eb3f11a2b3e5d72d361ab116bf/bitreq/src/client.rs#L146-L153

TheBlueMatt

I'm not sure I'm understanding the fix here - if we hit a dead connection when we go to request we should fix the Connection, not build a new one in the Client?

tnull · 2026-07-13T08:20:43Z

Spent some time trying to reproduce #562 and couldn't really. This PR is now also somewhat superseded by #661 (w.r.t. keep-alive handling), so going ahead closing this.

tnull · 2026-04-22T10:28:30Z

Fixes#562.

Validate pool entries on acquire and clean up after every send:

Remove entries whose keep-alive deadline has passed or whose inner
state has been poisoned, rather than blindly cloning the Arc.
Evict on send failure, Connection: close, or malformed
Keep-Alive.
Refresh LRU position on a cache hit.

Pre-send insertion of a fresh Arc on a miss is kept so concurrent
callers arriving during the first round-trip can share the socket for
pipelining; reusable_until and the post-send eviction together
ensure they never observe a known-dead Arc.

Expose a `pub(crate)` view of whether an `AsyncConnection` is still healthy and, if so, until when. `None` means the inner `next_request_id` has been poisoned (every failure path in `send` already sets this); `Some(t)` is the current `socket_new_requests_timeout`, which `send` already refreshes from the server's `Keep-Alive: timeout=N` header. No behavioural change; groundwork for the pool to check entry validity without duplicating tracking. Co-Authored-By: HAL 9000

Validate pool entries on acquire and clean up after every send: - Remove entries whose keep-alive deadline has passed or whose inner state has been poisoned, rather than blindly cloning the `Arc`. - Evict on send failure, `Connection: close`, or malformed `Keep-Alive`. - Refresh LRU position on a cache hit. Pre-send insertion of a fresh `Arc` on a miss is kept so concurrent callers arriving during the first round-trip can share the socket for pipelining; `reusable_until` and the post-send eviction together ensure they never observe a known-dead `Arc`. Co-Authored-By: HAL 9000

Drives `a, b, a, c, a` through a `Client` with capacity 2, backed by three `tokio::net::TcpListener` servers so each URL resolves to a distinct `ConnectionKey`. On the fix, step 3 promotes `a` to most-recent, step 4's c-insert evicts `b`, and step 5 reuses the warm `a` entry — three TCP accepts. Pre-fix, `a` stays at the front of the LRU queue and gets evicted at step 4 instead — four accepts. Covers one regression only: the LRU refresh on hit. The other architectural improvement on this branch — explicit pool-layer eviction on failure / `Connection: close` — produces the same externally-observable behaviour as the pre-fix code because `AsyncConnection::send`'s interior `retry_new_connection!` machinery compensates by swapping poisoned inner state on the next use. That change is defended on architectural grounds (explicit pool-layer logic instead of reliance on the inner retry) rather than via a black-box regression test. Co-Authored-By: HAL 9000

storopoli · 2026-04-22T15:40:17Z

+ /// Removes any pool entry for `key`. No-op if the slot is already empty.
+ fn evict(&self, key: &ConnectionKey) {
+ let mut state = self.r#async.lock().unwrap();
+ state.connections.remove(key);


I think this should evict by identity, not just by ConnectionKey.
There is a race where an older poisoned Arc can finish later and remove a newer healthy entry for the same key:
tasks A and B are both using the old pooled connection for K
A fails and marks the old connection dead
task C opens and inserts a fresh connection for K
B reaches this cleanup path later and remove(key) drops C's fresh entry
That doesn't break in-flight requests, but it does create avoidable reconnect churn under load. I think evict needs to compare the currently pooled Arc against the one that just finished and only remove on Arc::ptr_eq.
Permalinks:
send_async cleanup: https://github.com/tnull/corepc/blob/708d9c388aa148eb3f11a2b3e5d72d361ab116bf/bitreq/src/client.rs#L87-L95
evict: https://github.com/tnull/corepc/blob/708d9c388aa148eb3f11a2b3e5d72d361ab116bf/bitreq/src/client.rs#L146-L153

TheBlueMatt

I'm not sure I'm understanding the fix here - if we hit a dead connection when we go to request we should fix the Connection, not build a new one in the Client?

tnull · 2026-07-13T08:20:43Z

Spent some time trying to reproduce #562 and couldn't really. This PR is now also somewhat superseded by #661 (w.r.t. keep-alive handling), so going ahead closing this.

tnull · 2026-04-22T10:28:30Z

Fixes#562.

Validate pool entries on acquire and clean up after every send:

Remove entries whose keep-alive deadline has passed or whose inner
state has been poisoned, rather than blindly cloning the Arc.
Evict on send failure, Connection: close, or malformed
Keep-Alive.
Refresh LRU position on a cache hit.

Pre-send insertion of a fresh Arc on a miss is kept so concurrent
callers arriving during the first round-trip can share the socket for
pipelining; reusable_until and the post-send eviction together
ensure they never observe a known-dead Arc.

Expose a `pub(crate)` view of whether an `AsyncConnection` is still healthy and, if so, until when. `None` means the inner `next_request_id` has been poisoned (every failure path in `send` already sets this); `Some(t)` is the current `socket_new_requests_timeout`, which `send` already refreshes from the server's `Keep-Alive: timeout=N` header. No behavioural change; groundwork for the pool to check entry validity without duplicating tracking. Co-Authored-By: HAL 9000

Validate pool entries on acquire and clean up after every send: - Remove entries whose keep-alive deadline has passed or whose inner state has been poisoned, rather than blindly cloning the `Arc`. - Evict on send failure, `Connection: close`, or malformed `Keep-Alive`. - Refresh LRU position on a cache hit. Pre-send insertion of a fresh `Arc` on a miss is kept so concurrent callers arriving during the first round-trip can share the socket for pipelining; `reusable_until` and the post-send eviction together ensure they never observe a known-dead `Arc`. Co-Authored-By: HAL 9000

Drives `a, b, a, c, a` through a `Client` with capacity 2, backed by three `tokio::net::TcpListener` servers so each URL resolves to a distinct `ConnectionKey`. On the fix, step 3 promotes `a` to most-recent, step 4's c-insert evicts `b`, and step 5 reuses the warm `a` entry — three TCP accepts. Pre-fix, `a` stays at the front of the LRU queue and gets evicted at step 4 instead — four accepts. Covers one regression only: the LRU refresh on hit. The other architectural improvement on this branch — explicit pool-layer eviction on failure / `Connection: close` — produces the same externally-observable behaviour as the pre-fix code because `AsyncConnection::send`'s interior `retry_new_connection!` machinery compensates by swapping poisoned inner state on the next use. That change is defended on architectural grounds (explicit pool-layer logic instead of reliance on the inner retry) rather than via a black-box regression test. Co-Authored-By: HAL 9000

storopoli · 2026-04-22T15:40:17Z

+ /// Removes any pool entry for `key`. No-op if the slot is already empty.
+ fn evict(&self, key: &ConnectionKey) {
+ let mut state = self.r#async.lock().unwrap();
+ state.connections.remove(key);


I think this should evict by identity, not just by ConnectionKey.
There is a race where an older poisoned Arc can finish later and remove a newer healthy entry for the same key:
tasks A and B are both using the old pooled connection for K
A fails and marks the old connection dead
task C opens and inserts a fresh connection for K
B reaches this cleanup path later and remove(key) drops C's fresh entry
That doesn't break in-flight requests, but it does create avoidable reconnect churn under load. I think evict needs to compare the currently pooled Arc against the one that just finished and only remove on Arc::ptr_eq.
Permalinks:
send_async cleanup: https://github.com/tnull/corepc/blob/708d9c388aa148eb3f11a2b3e5d72d361ab116bf/bitreq/src/client.rs#L87-L95
evict: https://github.com/tnull/corepc/blob/708d9c388aa148eb3f11a2b3e5d72d361ab116bf/bitreq/src/client.rs#L146-L153

TheBlueMatt

I'm not sure I'm understanding the fix here - if we hit a dead connection when we go to request we should fix the Connection, not build a new one in the Client?

tnull · 2026-07-13T08:20:43Z

Spent some time trying to reproduce #562 and couldn't really. This PR is now also somewhat superseded by #661 (w.r.t. keep-alive handling), so going ahead closing this.

tnull · 2026-04-22T10:28:30Z

Fixes#562.

Validate pool entries on acquire and clean up after every send:

Remove entries whose keep-alive deadline has passed or whose inner
state has been poisoned, rather than blindly cloning the Arc.
Evict on send failure, Connection: close, or malformed
Keep-Alive.
Refresh LRU position on a cache hit.

Pre-send insertion of a fresh Arc on a miss is kept so concurrent
callers arriving during the first round-trip can share the socket for
pipelining; reusable_until and the post-send eviction together
ensure they never observe a known-dead Arc.

Expose a `pub(crate)` view of whether an `AsyncConnection` is still healthy and, if so, until when. `None` means the inner `next_request_id` has been poisoned (every failure path in `send` already sets this); `Some(t)` is the current `socket_new_requests_timeout`, which `send` already refreshes from the server's `Keep-Alive: timeout=N` header. No behavioural change; groundwork for the pool to check entry validity without duplicating tracking. Co-Authored-By: HAL 9000

Validate pool entries on acquire and clean up after every send: - Remove entries whose keep-alive deadline has passed or whose inner state has been poisoned, rather than blindly cloning the `Arc`. - Evict on send failure, `Connection: close`, or malformed `Keep-Alive`. - Refresh LRU position on a cache hit. Pre-send insertion of a fresh `Arc` on a miss is kept so concurrent callers arriving during the first round-trip can share the socket for pipelining; `reusable_until` and the post-send eviction together ensure they never observe a known-dead `Arc`. Co-Authored-By: HAL 9000

Drives `a, b, a, c, a` through a `Client` with capacity 2, backed by three `tokio::net::TcpListener` servers so each URL resolves to a distinct `ConnectionKey`. On the fix, step 3 promotes `a` to most-recent, step 4's c-insert evicts `b`, and step 5 reuses the warm `a` entry — three TCP accepts. Pre-fix, `a` stays at the front of the LRU queue and gets evicted at step 4 instead — four accepts. Covers one regression only: the LRU refresh on hit. The other architectural improvement on this branch — explicit pool-layer eviction on failure / `Connection: close` — produces the same externally-observable behaviour as the pre-fix code because `AsyncConnection::send`'s interior `retry_new_connection!` machinery compensates by swapping poisoned inner state on the next use. That change is defended on architectural grounds (explicit pool-layer logic instead of reliance on the inner retry) rather than via a black-box regression test. Co-Authored-By: HAL 9000

storopoli · 2026-04-22T15:40:17Z

+ /// Removes any pool entry for `key`. No-op if the slot is already empty.
+ fn evict(&self, key: &ConnectionKey) {
+ let mut state = self.r#async.lock().unwrap();
+ state.connections.remove(key);


I think this should evict by identity, not just by ConnectionKey.
There is a race where an older poisoned Arc can finish later and remove a newer healthy entry for the same key:
tasks A and B are both using the old pooled connection for K
A fails and marks the old connection dead
task C opens and inserts a fresh connection for K
B reaches this cleanup path later and remove(key) drops C's fresh entry
That doesn't break in-flight requests, but it does create avoidable reconnect churn under load. I think evict needs to compare the currently pooled Arc against the one that just finished and only remove on Arc::ptr_eq.
Permalinks:
send_async cleanup: https://github.com/tnull/corepc/blob/708d9c388aa148eb3f11a2b3e5d72d361ab116bf/bitreq/src/client.rs#L87-L95
evict: https://github.com/tnull/corepc/blob/708d9c388aa148eb3f11a2b3e5d72d361ab116bf/bitreq/src/client.rs#L146-L153

TheBlueMatt

I'm not sure I'm understanding the fix here - if we hit a dead connection when we go to request we should fix the Connection, not build a new one in the Client?

tnull · 2026-07-13T08:20:43Z

Spent some time trying to reproduce #562 and couldn't really. This PR is now also somewhat superseded by #661 (w.r.t. keep-alive handling), so going ahead closing this.

tnull · 2026-04-22T10:28:30Z

Fixes#562.

Validate pool entries on acquire and clean up after every send:

Remove entries whose keep-alive deadline has passed or whose inner
state has been poisoned, rather than blindly cloning the Arc.
Evict on send failure, Connection: close, or malformed
Keep-Alive.
Refresh LRU position on a cache hit.

Pre-send insertion of a fresh Arc on a miss is kept so concurrent
callers arriving during the first round-trip can share the socket for
pipelining; reusable_until and the post-send eviction together
ensure they never observe a known-dead Arc.

Expose a `pub(crate)` view of whether an `AsyncConnection` is still healthy and, if so, until when. `None` means the inner `next_request_id` has been poisoned (every failure path in `send` already sets this); `Some(t)` is the current `socket_new_requests_timeout`, which `send` already refreshes from the server's `Keep-Alive: timeout=N` header. No behavioural change; groundwork for the pool to check entry validity without duplicating tracking. Co-Authored-By: HAL 9000

Validate pool entries on acquire and clean up after every send: - Remove entries whose keep-alive deadline has passed or whose inner state has been poisoned, rather than blindly cloning the `Arc`. - Evict on send failure, `Connection: close`, or malformed `Keep-Alive`. - Refresh LRU position on a cache hit. Pre-send insertion of a fresh `Arc` on a miss is kept so concurrent callers arriving during the first round-trip can share the socket for pipelining; `reusable_until` and the post-send eviction together ensure they never observe a known-dead `Arc`. Co-Authored-By: HAL 9000

Drives `a, b, a, c, a` through a `Client` with capacity 2, backed by three `tokio::net::TcpListener` servers so each URL resolves to a distinct `ConnectionKey`. On the fix, step 3 promotes `a` to most-recent, step 4's c-insert evicts `b`, and step 5 reuses the warm `a` entry — three TCP accepts. Pre-fix, `a` stays at the front of the LRU queue and gets evicted at step 4 instead — four accepts. Covers one regression only: the LRU refresh on hit. The other architectural improvement on this branch — explicit pool-layer eviction on failure / `Connection: close` — produces the same externally-observable behaviour as the pre-fix code because `AsyncConnection::send`'s interior `retry_new_connection!` machinery compensates by swapping poisoned inner state on the next use. That change is defended on architectural grounds (explicit pool-layer logic instead of reliance on the inner retry) rather than via a black-box regression test. Co-Authored-By: HAL 9000

storopoli · 2026-04-22T15:40:17Z

+ /// Removes any pool entry for `key`. No-op if the slot is already empty.
+ fn evict(&self, key: &ConnectionKey) {
+ let mut state = self.r#async.lock().unwrap();
+ state.connections.remove(key);


I think this should evict by identity, not just by ConnectionKey.
There is a race where an older poisoned Arc can finish later and remove a newer healthy entry for the same key:
tasks A and B are both using the old pooled connection for K
A fails and marks the old connection dead
task C opens and inserts a fresh connection for K
B reaches this cleanup path later and remove(key) drops C's fresh entry
That doesn't break in-flight requests, but it does create avoidable reconnect churn under load. I think evict needs to compare the currently pooled Arc against the one that just finished and only remove on Arc::ptr_eq.
Permalinks:
send_async cleanup: https://github.com/tnull/corepc/blob/708d9c388aa148eb3f11a2b3e5d72d361ab116bf/bitreq/src/client.rs#L87-L95
evict: https://github.com/tnull/corepc/blob/708d9c388aa148eb3f11a2b3e5d72d361ab116bf/bitreq/src/client.rs#L146-L153

TheBlueMatt

I'm not sure I'm understanding the fix here - if we hit a dead connection when we go to request we should fix the Connection, not build a new one in the Client?

tnull · 2026-07-13T08:20:43Z

Spent some time trying to reproduce #562 and couldn't really. This PR is now also somewhat superseded by #661 (w.r.t. keep-alive handling), so going ahead closing this.

tnull · 2026-04-22T10:28:30Z

Fixes#562.

Validate pool entries on acquire and clean up after every send:

Remove entries whose keep-alive deadline has passed or whose inner
state has been poisoned, rather than blindly cloning the Arc.
Evict on send failure, Connection: close, or malformed
Keep-Alive.
Refresh LRU position on a cache hit.

Pre-send insertion of a fresh Arc on a miss is kept so concurrent
callers arriving during the first round-trip can share the socket for
pipelining; reusable_until and the post-send eviction together
ensure they never observe a known-dead Arc.

Expose a `pub(crate)` view of whether an `AsyncConnection` is still healthy and, if so, until when. `None` means the inner `next_request_id` has been poisoned (every failure path in `send` already sets this); `Some(t)` is the current `socket_new_requests_timeout`, which `send` already refreshes from the server's `Keep-Alive: timeout=N` header. No behavioural change; groundwork for the pool to check entry validity without duplicating tracking. Co-Authored-By: HAL 9000

Validate pool entries on acquire and clean up after every send: - Remove entries whose keep-alive deadline has passed or whose inner state has been poisoned, rather than blindly cloning the `Arc`. - Evict on send failure, `Connection: close`, or malformed `Keep-Alive`. - Refresh LRU position on a cache hit. Pre-send insertion of a fresh `Arc` on a miss is kept so concurrent callers arriving during the first round-trip can share the socket for pipelining; `reusable_until` and the post-send eviction together ensure they never observe a known-dead `Arc`. Co-Authored-By: HAL 9000

Drives `a, b, a, c, a` through a `Client` with capacity 2, backed by three `tokio::net::TcpListener` servers so each URL resolves to a distinct `ConnectionKey`. On the fix, step 3 promotes `a` to most-recent, step 4's c-insert evicts `b`, and step 5 reuses the warm `a` entry — three TCP accepts. Pre-fix, `a` stays at the front of the LRU queue and gets evicted at step 4 instead — four accepts. Covers one regression only: the LRU refresh on hit. The other architectural improvement on this branch — explicit pool-layer eviction on failure / `Connection: close` — produces the same externally-observable behaviour as the pre-fix code because `AsyncConnection::send`'s interior `retry_new_connection!` machinery compensates by swapping poisoned inner state on the next use. That change is defended on architectural grounds (explicit pool-layer logic instead of reliance on the inner retry) rather than via a black-box regression test. Co-Authored-By: HAL 9000

storopoli · 2026-04-22T15:40:17Z

+ /// Removes any pool entry for `key`. No-op if the slot is already empty.
+ fn evict(&self, key: &ConnectionKey) {
+ let mut state = self.r#async.lock().unwrap();
+ state.connections.remove(key);


I think this should evict by identity, not just by ConnectionKey.
There is a race where an older poisoned Arc can finish later and remove a newer healthy entry for the same key:
tasks A and B are both using the old pooled connection for K
A fails and marks the old connection dead
task C opens and inserts a fresh connection for K
B reaches this cleanup path later and remove(key) drops C's fresh entry
That doesn't break in-flight requests, but it does create avoidable reconnect churn under load. I think evict needs to compare the currently pooled Arc against the one that just finished and only remove on Arc::ptr_eq.
Permalinks:
send_async cleanup: https://github.com/tnull/corepc/blob/708d9c388aa148eb3f11a2b3e5d72d361ab116bf/bitreq/src/client.rs#L87-L95
evict: https://github.com/tnull/corepc/blob/708d9c388aa148eb3f11a2b3e5d72d361ab116bf/bitreq/src/client.rs#L146-L153

TheBlueMatt

I'm not sure I'm understanding the fix here - if we hit a dead connection when we go to request we should fix the Connection, not build a new one in the Client?

tnull · 2026-07-13T08:20:43Z

Spent some time trying to reproduce #562 and couldn't really. This PR is now also somewhat superseded by #661 (w.r.t. keep-alive handling), so going ahead closing this.

tnull · 2026-04-22T10:28:30Z

Fixes#562.

Validate pool entries on acquire and clean up after every send:

Remove entries whose keep-alive deadline has passed or whose inner
state has been poisoned, rather than blindly cloning the Arc.
Evict on send failure, Connection: close, or malformed
Keep-Alive.
Refresh LRU position on a cache hit.

Pre-send insertion of a fresh Arc on a miss is kept so concurrent
callers arriving during the first round-trip can share the socket for
pipelining; reusable_until and the post-send eviction together
ensure they never observe a known-dead Arc.

Expose a `pub(crate)` view of whether an `AsyncConnection` is still healthy and, if so, until when. `None` means the inner `next_request_id` has been poisoned (every failure path in `send` already sets this); `Some(t)` is the current `socket_new_requests_timeout`, which `send` already refreshes from the server's `Keep-Alive: timeout=N` header. No behavioural change; groundwork for the pool to check entry validity without duplicating tracking. Co-Authored-By: HAL 9000

Validate pool entries on acquire and clean up after every send: - Remove entries whose keep-alive deadline has passed or whose inner state has been poisoned, rather than blindly cloning the `Arc`. - Evict on send failure, `Connection: close`, or malformed `Keep-Alive`. - Refresh LRU position on a cache hit. Pre-send insertion of a fresh `Arc` on a miss is kept so concurrent callers arriving during the first round-trip can share the socket for pipelining; `reusable_until` and the post-send eviction together ensure they never observe a known-dead `Arc`. Co-Authored-By: HAL 9000

Drives `a, b, a, c, a` through a `Client` with capacity 2, backed by three `tokio::net::TcpListener` servers so each URL resolves to a distinct `ConnectionKey`. On the fix, step 3 promotes `a` to most-recent, step 4's c-insert evicts `b`, and step 5 reuses the warm `a` entry — three TCP accepts. Pre-fix, `a` stays at the front of the LRU queue and gets evicted at step 4 instead — four accepts. Covers one regression only: the LRU refresh on hit. The other architectural improvement on this branch — explicit pool-layer eviction on failure / `Connection: close` — produces the same externally-observable behaviour as the pre-fix code because `AsyncConnection::send`'s interior `retry_new_connection!` machinery compensates by swapping poisoned inner state on the next use. That change is defended on architectural grounds (explicit pool-layer logic instead of reliance on the inner retry) rather than via a black-box regression test. Co-Authored-By: HAL 9000

storopoli · 2026-04-22T15:40:17Z

+ /// Removes any pool entry for `key`. No-op if the slot is already empty.
+ fn evict(&self, key: &ConnectionKey) {
+ let mut state = self.r#async.lock().unwrap();
+ state.connections.remove(key);


I think this should evict by identity, not just by ConnectionKey.
There is a race where an older poisoned Arc can finish later and remove a newer healthy entry for the same key:
tasks A and B are both using the old pooled connection for K
A fails and marks the old connection dead
task C opens and inserts a fresh connection for K
B reaches this cleanup path later and remove(key) drops C's fresh entry
That doesn't break in-flight requests, but it does create avoidable reconnect churn under load. I think evict needs to compare the currently pooled Arc against the one that just finished and only remove on Arc::ptr_eq.
Permalinks:
send_async cleanup: https://github.com/tnull/corepc/blob/708d9c388aa148eb3f11a2b3e5d72d361ab116bf/bitreq/src/client.rs#L87-L95
evict: https://github.com/tnull/corepc/blob/708d9c388aa148eb3f11a2b3e5d72d361ab116bf/bitreq/src/client.rs#L146-L153

TheBlueMatt

I'm not sure I'm understanding the fix here - if we hit a dead connection when we go to request we should fix the Connection, not build a new one in the Client?

tnull · 2026-07-13T08:20:43Z

Spent some time trying to reproduce #562 and couldn't really. This PR is now also somewhat superseded by #661 (w.r.t. keep-alive handling), so going ahead closing this.

tnull · 2026-04-22T10:28:30Z

Fixes#562.

Validate pool entries on acquire and clean up after every send:

Remove entries whose keep-alive deadline has passed or whose inner
state has been poisoned, rather than blindly cloning the Arc.
Evict on send failure, Connection: close, or malformed
Keep-Alive.
Refresh LRU position on a cache hit.

Pre-send insertion of a fresh Arc on a miss is kept so concurrent
callers arriving during the first round-trip can share the socket for
pipelining; reusable_until and the post-send eviction together
ensure they never observe a known-dead Arc.

Expose a `pub(crate)` view of whether an `AsyncConnection` is still healthy and, if so, until when. `None` means the inner `next_request_id` has been poisoned (every failure path in `send` already sets this); `Some(t)` is the current `socket_new_requests_timeout`, which `send` already refreshes from the server's `Keep-Alive: timeout=N` header. No behavioural change; groundwork for the pool to check entry validity without duplicating tracking. Co-Authored-By: HAL 9000

Validate pool entries on acquire and clean up after every send: - Remove entries whose keep-alive deadline has passed or whose inner state has been poisoned, rather than blindly cloning the `Arc`. - Evict on send failure, `Connection: close`, or malformed `Keep-Alive`. - Refresh LRU position on a cache hit. Pre-send insertion of a fresh `Arc` on a miss is kept so concurrent callers arriving during the first round-trip can share the socket for pipelining; `reusable_until` and the post-send eviction together ensure they never observe a known-dead `Arc`. Co-Authored-By: HAL 9000

Drives `a, b, a, c, a` through a `Client` with capacity 2, backed by three `tokio::net::TcpListener` servers so each URL resolves to a distinct `ConnectionKey`. On the fix, step 3 promotes `a` to most-recent, step 4's c-insert evicts `b`, and step 5 reuses the warm `a` entry — three TCP accepts. Pre-fix, `a` stays at the front of the LRU queue and gets evicted at step 4 instead — four accepts. Covers one regression only: the LRU refresh on hit. The other architectural improvement on this branch — explicit pool-layer eviction on failure / `Connection: close` — produces the same externally-observable behaviour as the pre-fix code because `AsyncConnection::send`'s interior `retry_new_connection!` machinery compensates by swapping poisoned inner state on the next use. That change is defended on architectural grounds (explicit pool-layer logic instead of reliance on the inner retry) rather than via a black-box regression test. Co-Authored-By: HAL 9000

storopoli · 2026-04-22T15:40:17Z

+ /// Removes any pool entry for `key`. No-op if the slot is already empty.
+ fn evict(&self, key: &ConnectionKey) {
+ let mut state = self.r#async.lock().unwrap();
+ state.connections.remove(key);


I think this should evict by identity, not just by ConnectionKey.
There is a race where an older poisoned Arc can finish later and remove a newer healthy entry for the same key:
tasks A and B are both using the old pooled connection for K
A fails and marks the old connection dead
task C opens and inserts a fresh connection for K
B reaches this cleanup path later and remove(key) drops C's fresh entry
That doesn't break in-flight requests, but it does create avoidable reconnect churn under load. I think evict needs to compare the currently pooled Arc against the one that just finished and only remove on Arc::ptr_eq.
Permalinks:
send_async cleanup: https://github.com/tnull/corepc/blob/708d9c388aa148eb3f11a2b3e5d72d361ab116bf/bitreq/src/client.rs#L87-L95
evict: https://github.com/tnull/corepc/blob/708d9c388aa148eb3f11a2b3e5d72d361ab116bf/bitreq/src/client.rs#L146-L153

TheBlueMatt

I'm not sure I'm understanding the fix here - if we hit a dead connection when we go to request we should fix the Connection, not build a new one in the Client?

tnull · 2026-07-13T08:20:43Z

Spent some time trying to reproduce #562 and couldn't really. This PR is now also somewhat superseded by #661 (w.r.t. keep-alive handling), so going ahead closing this.

tnull added 2 commits April 22, 2026 12:26

tnull requested review from TheBlueMatt and oleonardolima as code owners April 22, 2026 10:28

tnull force-pushed the 2026-04-bitreq-async-pool-fix branch from 04a56f3 to 708d9c3Compare April 22, 2026 10:58

storopoli suggested changes Apr 22, 2026
View reviewed changes

TheBlueMatt reviewed Apr 24, 2026
View reviewed changes

tnull mentioned this pull request Jul 11, 2026
bitreq::Client only reuses an HTTP/1.1 connection when the response explicitly contains Connection: keep-alive#659
Closed

tnull closed this Jul 13, 2026

tnull added 2 commits April 22, 2026 12:26

tnull requested review from TheBlueMatt and oleonardolima as code owners April 22, 2026 10:28

tnull force-pushed the 2026-04-bitreq-async-pool-fix branch from 04a56f3 to 708d9c3Compare April 22, 2026 10:58

storopoli suggested changes Apr 22, 2026
View reviewed changes

TheBlueMatt reviewed Apr 24, 2026
View reviewed changes

tnull mentioned this pull request Jul 11, 2026
bitreq::Client only reuses an HTTP/1.1 connection when the response explicitly contains Connection: keep-alive#659
Closed

tnull closed this Jul 13, 2026

tnull added 2 commits April 22, 2026 12:26

tnull requested review from TheBlueMatt and oleonardolima as code owners April 22, 2026 10:28

tnull force-pushed the 2026-04-bitreq-async-pool-fix branch from 04a56f3 to 708d9c3Compare April 22, 2026 10:58

storopoli suggested changes Apr 22, 2026
View reviewed changes

TheBlueMatt reviewed Apr 24, 2026
View reviewed changes

tnull mentioned this pull request Jul 11, 2026
bitreq::Client only reuses an HTTP/1.1 connection when the response explicitly contains Connection: keep-alive#659
Closed

tnull closed this Jul 13, 2026

tnull added 2 commits April 22, 2026 12:26

tnull requested review from TheBlueMatt and oleonardolima as code owners April 22, 2026 10:28

tnull force-pushed the 2026-04-bitreq-async-pool-fix branch from 04a56f3 to 708d9c3Compare April 22, 2026 10:58

storopoli suggested changes Apr 22, 2026
View reviewed changes

TheBlueMatt reviewed Apr 24, 2026
View reviewed changes

tnull mentioned this pull request Jul 11, 2026
bitreq::Client only reuses an HTTP/1.1 connection when the response explicitly contains Connection: keep-alive#659
Closed

tnull closed this Jul 13, 2026

tnull added 2 commits April 22, 2026 12:26

tnull requested review from TheBlueMatt and oleonardolima as code owners April 22, 2026 10:28

tnull force-pushed the 2026-04-bitreq-async-pool-fix branch from 04a56f3 to 708d9c3Compare April 22, 2026 10:58

storopoli suggested changes Apr 22, 2026
View reviewed changes

TheBlueMatt reviewed Apr 24, 2026
View reviewed changes

tnull mentioned this pull request Jul 11, 2026
bitreq::Client only reuses an HTTP/1.1 connection when the response explicitly contains Connection: keep-alive#659
Closed

tnull closed this Jul 13, 2026

tnull added 2 commits April 22, 2026 12:26

tnull requested review from TheBlueMatt and oleonardolima as code owners April 22, 2026 10:28

tnull force-pushed the 2026-04-bitreq-async-pool-fix branch from 04a56f3 to 708d9c3Compare April 22, 2026 10:58

storopoli suggested changes Apr 22, 2026
View reviewed changes

TheBlueMatt reviewed Apr 24, 2026
View reviewed changes

tnull mentioned this pull request Jul 11, 2026
bitreq::Client only reuses an HTTP/1.1 connection when the response explicitly contains Connection: keep-alive#659
Closed

tnull closed this Jul 13, 2026

tnull added 2 commits April 22, 2026 12:26

tnull requested review from TheBlueMatt and oleonardolima as code owners April 22, 2026 10:28

tnull force-pushed the 2026-04-bitreq-async-pool-fix branch from 04a56f3 to 708d9c3Compare April 22, 2026 10:58

storopoli suggested changes Apr 22, 2026
View reviewed changes

TheBlueMatt reviewed Apr 24, 2026
View reviewed changes

tnull mentioned this pull request Jul 11, 2026
bitreq::Client only reuses an HTTP/1.1 connection when the response explicitly contains Connection: keep-alive#659
Closed

tnull closed this Jul 13, 2026

tnull added 2 commits April 22, 2026 12:26

tnull requested review from TheBlueMatt and oleonardolima as code owners April 22, 2026 10:28

tnull force-pushed the 2026-04-bitreq-async-pool-fix branch from 04a56f3 to 708d9c3Compare April 22, 2026 10:58

storopoli suggested changes Apr 22, 2026
View reviewed changes

TheBlueMatt reviewed Apr 24, 2026
View reviewed changes

tnull mentioned this pull request Jul 11, 2026
bitreq::Client only reuses an HTTP/1.1 connection when the response explicitly contains Connection: keep-alive#659
Closed

tnull closed this Jul 13, 2026

Uh oh!

Conversation

tnull commented Apr 22, 2026

Uh oh!

storopoliApr 22, 2026

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

tnull commented Jul 13, 2026• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

tnull commented Apr 22, 2026

Uh oh!

storopoliApr 22, 2026

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

tnull commented Jul 13, 2026• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

tnull commented Apr 22, 2026

Uh oh!

storopoliApr 22, 2026

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

tnull commented Jul 13, 2026• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

tnull commented Apr 22, 2026

Uh oh!

storopoliApr 22, 2026

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

tnull commented Jul 13, 2026• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

tnull commented Jul 13, 2026•
edited
Loading

tnull commented Jul 13, 2026•
edited
Loading

tnull commented Jul 13, 2026•
edited
Loading

tnull commented Jul 13, 2026•
edited
Loading

tnull commented Jul 13, 2026•
edited
Loading

tnull commented Jul 13, 2026•
edited
Loading