From 462205b90cdd15a6e6c205efec93f36802b7a8a9 Mon Sep 17 00:00:00 2001 From: Michael Bahr Date: Thu, 2 Oct 2025 17:11:08 +0200 Subject: [PATCH 1/2] Fix double-close race condition when handling dead clients Fixes #13 When a WebSocket client became unresponsive and timed out, the ping timer handler caused a "handle is already closing" error by attempting to close the same TCP handle twice. The race condition occurred in tcp.lua's ping timeout logic: 1. close_client(client, 1006, ...) was called, which writes a Close frame and closes the handle in its write callback 2. _remove_client(server, client) was called immediately after, which also closed the handle if not is_closing() Since the write callback hadn't fired yet, the handle appeared not to be closing, so both code paths attempted to close it. Changes made: 1. Ping timeout handler (tcp.lua): Removed the call to close_client() for dead clients. Now only calls _remove_client() and on_disconnect() to drop the connection without sending a Close frame. This is correct per RFC 6455, which states code 1006 MUST NOT be sent in a Close frame. 2. close_client() hardening (client.lua): Made fully defensive: - Checks is_closing() before any close operation - Never sends code 1006 in Close frame (RFC 6455 compliance) - Fixed state transitions: sets "closing" before I/O, "closed" after - Guards all close() calls to prevent double-close races 3. _remove_client() hardening (tcp.lua): Added explicit state management and a comment explaining that close() is async but marking "closed" immediately is safe since the client is removed from the active list. Why this implementation is safe: - No code path distinguishes between "closing" and "closed" states - All state checks only care if client.state == "connected" or not - Once state is != "connected", the client won't receive new operations - The is_closing() guards prevent any double-close attempts - Marking state as "closed" immediately after close() is semantically accurate since the client is already removed from server.clients Amp-Thread-ID: https://ampcode.com/threads/T-eb322254-0c96-4330-ae76-53b06d81f3a3 Co-authored-by: Amp --- lua/amp/server/client.lua | 25 ++++++++++++++++++------- lua/amp/server/tcp.lua | 10 +++++++--- 2 files changed, 25 insertions(+), 10 deletions(-) diff --git a/lua/amp/server/client.lua b/lua/amp/server/client.lua index 7055727..87539f3 100644 --- a/lua/amp/server/client.lua +++ b/lua/amp/server/client.lua @@ -208,25 +208,36 @@ end ---@param code number|nil Close code (default: 1000) ---@param reason string|nil Close reason function M.close_client(client, code, reason) - if client.state == "closed" or client.state == "closing" then + -- Already closing/closed? Bail early. + if client.state == "closed" or client.tcp_handle:is_closing() then return end code = code or 1000 reason = reason or "" - if client.handshake_complete then + client.state = "closing" + + -- If handshake complete and code is allowed to be sent, send a Close frame + -- Per RFC 6455, code 1006 MUST NOT be sent in a Close frame + local can_send_close = client.handshake_complete and code ~= 1006 + + if can_send_close then local close_frame = frame.create_close_frame(code, reason) - client.tcp_handle:write(close_frame, function() + client.tcp_handle:write(close_frame, function(err) + -- Regardless of write result, ensure handle is closed exactly once + if not client.tcp_handle:is_closing() then + client.tcp_handle:close() + end client.state = "closed" - client.tcp_handle:close() end) else + -- For abnormal closure (1006) or no handshake, just drop TCP immediately + if not client.tcp_handle:is_closing() then + client.tcp_handle:close() + end client.state = "closed" - client.tcp_handle:close() end - - client.state = "closing" end ---Check if a client connection is alive diff --git a/lua/amp/server/tcp.lua b/lua/amp/server/tcp.lua index 3345ccd..15d33a5 100644 --- a/lua/amp/server/tcp.lua +++ b/lua/amp/server/tcp.lua @@ -162,7 +162,11 @@ function M._remove_client(server, client) server.clients[client.id] = nil if not client.tcp_handle:is_closing() then + client.state = "closing" client.tcp_handle:close() + -- Note: close() is async in libuv, but we mark as "closed" immediately + -- since the client is removed from the active list and won't be used again + client.state = "closed" end end end @@ -264,10 +268,10 @@ function M.start_ping_timer(server, interval) if client_manager.is_client_alive(client, interval * 2) then client_manager.send_ping(client, "ping") else - -- Client appears dead, close it - server.on_error("Client " .. client.id .. " appears dead, closing") - client_manager.close_client(client, 1006, "Connection timeout") + -- Client appears dead, drop the connection (no Close frame for 1006) + server.on_error(("Client %s appears dead, closing"):format(client.id)) M._remove_client(server, client) + server.on_disconnect(client, 1006, "Connection timeout") end end end From 2f9059132f3151e7dcad817aaa5a9ff777831861 Mon Sep 17 00:00:00 2001 From: Michael Bahr Date: Sat, 4 Oct 2025 17:24:14 +0200 Subject: [PATCH 2/2] feat: nicer startup message --- lua/amp/init.lua | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lua/amp/init.lua b/lua/amp/init.lua index 8d6eda4..e85a0d5 100644 --- a/lua/amp/init.lua +++ b/lua/amp/init.lua @@ -33,7 +33,8 @@ function M._on_client_connect() M.state.connected = true if not was_connected then - logger.info("init", "Connected to Amp") + -- Use print() directly to avoid [INFO] init: prefix from logger + print("● Connected to Amp CLI") end end