connect_pool: unguarded hackney_conn:get_state/1 crashes checkout when a pooled HTTP/2/3 connection terminates mid-call

[kivra-kenand]

Environment

• hackney: 4.7.2 (also present in 4.5.x)
• Erlang/OTP: 28
• Observed in production under HTTP/2 connection reuse to a remote that idle-closes / GOAWAYs pooled connections.

Summary

When a pooled HTTP/2 (or HTTP/3) connection is being reused, connect_pool/4 validates it with hackney_conn:get_state/1, which is a bare gen_statem:call/2. If that pooled connection process is terminating at the exact moment of checkout (idle teardown, server GOAWAY, keepalive close), the gen_statem:call exits. The surrounding case only handles return values, so the exit propagates and crashes the caller of hackney:connect/4 instead of falling back to a fresh connection.

Affected code

src/hackney.erl, HTTP/2 checkout in connect_pool/4 (~L200):

  case PoolHandler:checkout_h2(Host, Port, Transport, Options2) of
    {ok, H2Pid} ->
      case hackney_conn:get_state(H2Pid) of      %% <-- exits if H2Pid is terminating
        {ok, connected} -> {ok, H2Pid};
        _ -> PoolHandler:unregister_h2(H2Pid, Options2),
             connect_pool_new(...)
      end;
    none -> connect_pool_new(...)
  end

The same unguarded pattern exists for HTTP/3 in try_h3_connection/5 (~L251), using hackney_conn:get_state(H3Pid).

hackney_conn:get_state/1 (src/hackney_conn.erl):

  get_state(Pid) -> gen_statem:call(Pid, get_state).

Why this looks like an oversight

The same file already defends against this exact race for the sibling calls. maybe_register_h2/6 and maybe_upgrade_ssl/3 wrap their hackney_conn:* calls with the comment:

  %% Wrapped in try to handle a race where the connection terminates before the call

The two get_state checkout calls just weren't given the same treatment.

Symptom (caller-side)

hackney:connect/4 exits (rather than returning {error, _}) with a reason of the form:

  {noproc,  {gen_statem, call, [<Pid>, get_state | _]}}
  {normal,  {gen_statem, call, [<Pid>, get_state | _]}}
  {shutdown,{gen_statem, call, [<Pid>, get_state | _]}}

Minimal reproduction (eunit, hermetic)

Inject a pool handler whose checkout_h2/4 returns an already-terminated pid; everything else delegates to hackney_pool. Drive the public API against a closed local port so the fallback path returns cleanly:

  setup: application:ensure_all_started(hackney),
         application:set_env(hackney, pool_handler, my_race_pool).

  %% my_race_pool:checkout_h2/4 -> {ok, DeadPid}   (DeadPid already terminated)
  %% all other callbacks delegate to hackney_pool

  {ok, L} = gen_tcp:listen(0, []), {ok, P} = inet:port(L), gen_tcp:close(L),                                                                
  Result = (catch hackney:connect(hackney_ssl, "127.0.0.1", P,
                                  [{protocols,[http2]}, {pool, default},
                                   {connect_timeout,1000}])),
  %% actual (buggy): EXIT {noproc,{gen_statem,call,[_,get_state|_]}}
  %% expected:       {error, _}

Expected behaviour

A pooled connection that terminates during the checkout liveness probe should be treated as unusable — unregister it and fall through to connect_pool_new/6 — not crash the caller. This matches what the code already does for the non-connected return value.

Proposed fix

Wrap both get_state checkout calls, mirroring the existing race handling. Narrowed to exit (a bad get_state is only ever a gen_statem:call exit; error/throw would be a genuine bug and should propagate):

  GetState = try hackney_conn:get_state(H2Pid)
             catch exit:_ -> {error, terminated} end,
  case GetState of
    {ok, connected} -> {ok, H2Pid};
    _ -> PoolHandler:unregister_h2(H2Pid, Options2),
         connect_pool_new(...)
  end

Same for the HTTP/3 site. (Could also tighten the existing maybe_register_h2 / maybe_upgrade_ssl catch _:_ guards to exit:_ for consistency.)

[benoitc]

Sorry but in plain words and human sentences what is this issue about? What are you fixing?

[kivra-kenand]

Sorry but in plain words and human sentences what is this issue about? What are you fixing?

We are getting a lot of connections terminating (without retries) after upgrading to 4.x and it seems to boil down to the missing try-catches noted above.