Return {error, closed} for calls to a connection in closed state

NEWS.md

@@ -1,5 +1,18 @@
 # NEWS
 
+unreleased
+----------
+
+### Fixed
+
+- A request that races a peer-initiated close now returns `{error, closed}`
+  instead of `{error, invalid_state}`. A connection that sees the peer close
+  stays alive briefly so late calls get an answer, and during that window every
+  call without a handler answered with the generic `invalid_state`. The same
+  race therefore had two answers: `{error, closed}` once the connection process
+  was gone, `{error, invalid_state}` while it lingered. Callers can now tell a
+  closed connection from a misuse of the API (#932, #933, thanks @kpy3).
+
 4.7.4 - 2026-08-12
 ------------------
 

src/hackney_conn.erl

@@ -598,12 +598,12 @@ release_to_pool(Pid) ->
 %% This updates the process being monitored - if the new owner crashes,
 %% the connection will terminate. Used by the pool when checking out
 %% a connection to a new requester.
--spec set_owner(pid(), pid()) -> ok | {error, invalid_state}.
+-spec set_owner(pid(), pid()) -> ok | {error, closed | invalid_state}.
 set_owner(Pid, NewOwner) ->
     set_owner(Pid, NewOwner, 5000).
 
 %% @doc Set a new owner, waiting at most `Timeout'. @see get_state/2
--spec set_owner(pid(), pid(), timeout()) -> ok | {error, invalid_state}.
+-spec set_owner(pid(), pid(), timeout()) -> ok | {error, closed | invalid_state}.
 set_owner(Pid, NewOwner, Timeout) ->
     gen_statem:call(Pid, {set_owner, NewOwner}, Timeout).
 
@@ -1824,8 +1824,8 @@ closed(enter, _OldState, #conn_data{socket = Socket, transport = Transport, pool
     %% late-arriving {call, From, {request, _}} messages from workers that
     %% raced the pool checkout race a terminating gen_statem — which
     %% surfaces as `exit:{normal, _}` in the caller (issue #836). Stay
-    %% alive briefly so those late calls get a proper `{error, {closed, _}}`
-    %% reply via handle_common's invalid_state fallback, then stop.
+    %% alive briefly so those late calls get a proper `{error, closed}`
+    %% reply via handle_common's closed-state fallback, then stop.
     case PoolPid of
         undefined ->
             {keep_state, Data#conn_data{socket = undefined}};
@@ -2002,6 +2002,14 @@ handle_common({call, From}, checkin_info, _State, Data) ->
 handle_common({call, From}, get_protocol, _State, #conn_data{protocol = Protocol}) ->
     {keep_state_and_data, [{reply, From, Protocol}]};
 
+handle_common({call, From}, _, closed, _Data) ->
+    %% #932: a call that reaches a connection already in `closed' (a request
+    %% that raced a peer-initiated close after checkout) is a closed
+    %% connection, not a misuse of the API. Reply with the same
+    %% `{error, closed}' safe_call/3 gives when the process is already gone,
+    %% so the race has one answer instead of two.
+    {keep_state_and_data, [{reply, From, {error, closed}}]};
+
 handle_common({call, From}, _, _State, _Data) ->
     {keep_state_and_data, [{reply, From, {error, invalid_state}}]};
 

test/hackney_conn_tests.erl

@@ -29,14 +29,18 @@ hackney_conn_test_() ->
       {"connect timeout", fun test_connect_timeout/0},
       {"connect to invalid host", fun test_connect_invalid/0},
       {"owner death stops connection", fun test_owner_death/0},
-      {"set_owner on closed connection returns invalid_state (#850)",
+      {"set_owner on closed connection returns closed (#850, #932)",
        fun test_set_owner_closed_returns_error/0},
       {"set_owner_async stops a closed pooled connection (#850)",
        fun test_set_owner_async_closed_pooled_stops/0},
       {"request on a dead connection returns {error, closed} (#861)",
        fun test_request_dead_conn_returns_error/0},
       {"body on a dead connection returns {error, closed} (#861)",
-       fun test_body_dead_conn_returns_error/0}
+       fun test_body_dead_conn_returns_error/0},
+      {"request on a connection in closed returns {error, closed} (#932)",
+       fun test_request_closed_conn_returns_closed/0},
+      {"accessors still answer in closed (#932)",
+       fun test_closed_conn_accessors_still_answer/0}
      ]}.
 
 %% Integration tests - use embedded Cowboy server
@@ -256,15 +260,15 @@ test_owner_death() ->
 
 %% #850: when a checkout races a server-side close, the pool calls set_owner on
 %% a connection that has just transitioned to `closed`. It must get
-%% {error, invalid_state} back (so the pool can fall through to a fresh
-%% connection) rather than crash. A non-pooled connection has no grace timer,
-%% so it stays in `closed` to answer.
+%% {error, closed} back (so the pool can fall through to a fresh connection)
+%% rather than crash. A non-pooled connection has no grace timer, so it stays
+%% in `closed` to answer.
 test_set_owner_closed_returns_error() ->
     {Pid, ListenSock} = connected_conn(#{}),
     ?assertEqual({ok, connected}, hackney_conn:get_state(Pid)),
     ok = hackney_conn:close(Pid),
     ?assertEqual({ok, closed}, hackney_conn:get_state(Pid)),
-    ?assertEqual({error, invalid_state}, hackney_conn:set_owner(Pid, self())),
+    ?assertEqual({error, closed}, hackney_conn:set_owner(Pid, self())),
     hackney_conn:stop(Pid),
     gen_tcp:close(ListenSock).
 
@@ -285,6 +289,46 @@ test_set_owner_async_closed_pooled_stops() ->
 %% #861: a pooled connection can stop between checkout and the call, so a
 %% request to an already-dead connection must return {error, closed} rather
 %% than letting exit:{normal,_}/noproc crash the caller.
+%% #932: a request that races a peer-initiated close lands on a connection
+%% that is still alive in `closed' (the #836 grace window). It used to get the
+%% generic {error, invalid_state}, so the same race answered {error, closed}
+%% when the process had already stopped and {error, invalid_state} when it had
+%% not. Both answers are now {error, closed}.
+test_request_closed_conn_returns_closed() ->
+    {Pid, ListenSock} = connected_conn(#{}),
+    ok = hackney_conn:close(Pid),
+    ?assertEqual({ok, closed}, hackney_conn:get_state(Pid)),
+    ?assertEqual({error, closed},
+                 hackney_conn:request(Pid, <<"GET">>, <<"/">>, [], <<>>)),
+    ?assertEqual({error, closed}, hackney_conn:body(Pid)),
+    hackney_conn:stop(Pid),
+    gen_tcp:close(ListenSock).
+
+%% #932: only calls with no handler answer {error, closed}. The accessors the
+%% pool and hackney read after a response, which is when the peer's close
+%% typically lands, must keep answering. `hackney:location/1' feeds
+%% response_headers straight to hackney_headers, so an error tuple there
+%% crashes it.
+test_closed_conn_accessors_still_answer() ->
+    {Pid, ListenSock} = connected_conn(#{}),
+    ok = hackney_conn:close(Pid),
+    ?assertEqual({ok, closed}, hackney_conn:get_state(Pid)),
+    ?assertEqual(undefined, hackney_conn:response_headers(Pid)),
+    %% reads response_headers when no location is stored
+    ?assertEqual(undefined, hackney:location(Pid)),
+    ?assertEqual(ok, hackney_conn:set_location(Pid, <<"http://127.0.0.1/x">>)),
+    ?assertEqual(<<"http://127.0.0.1/x">>, hackney_conn:get_location(Pid)),
+    ?assertEqual(<<"http://127.0.0.1/x">>, hackney:location(Pid)),
+    ?assertEqual(http1, hackney_conn:get_protocol(Pid)),
+    ?assertEqual(false, hackney_conn:is_upgraded_ssl(Pid)),
+    ?assertEqual(false, hackney_conn:is_no_reuse(Pid)),
+    Info = hackney_conn:checkin_info(Pid),
+    ?assert(is_map(Info)),
+    ?assertEqual(false, maps:get(ready, Info)),
+    ?assertEqual(ok, hackney_conn:close(Pid)),
+    hackney_conn:stop(Pid),
+    gen_tcp:close(ListenSock).
+
 test_request_dead_conn_returns_error() ->
     Pid = dead_conn_pid(),
     ?assertEqual({error, closed},