From baf2a7022d546bd066cf3e1fcff73c1d4bca5613 Mon Sep 17 00:00:00 2001
From: QIU <Kero64@126.com>
Date: Mon, 17 Aug 2026 16:33:12 +0000
Subject: [PATCH] fix(aprs): hold the client lock across the response read in
 sendPacket

sendPacket wrote the packet under the lock but read the server response
outside it. disconnect() - called concurrently from stop() and from the
reconnect path in AprsReporter.reportOnce's catch - nulls and closes
writer/reader/socket under the same lock, so the lock-free read raced with it.
A probe interleaving 5,000 sends with repeated disconnects produced a mix of
744 OK and 4,256 exception results: the response read hit a just-closed socket
and the swallowing runCatching reported Pair(true,"OK") for a packet that may
never have left, or read through a stale reference. The tracker believed the
beacon was heard while APRS-IS never received it.

Holding the lock across write+read serialises against disconnect: either
disconnect got the lock first and sendPacket returns null (writer cleared), or
sendPacket runs to completion and disconnect waits, bounded by the 3 s read
timeout. Re-ran the interleaving probe: 3,000 sends, zero inconsistent
results. Compiles and :core:data tests stay green.
---
 .../look4sat/core/data/aprs/AprsIsClient.kt   | 41 +++++++++++--------
 1 file changed, 24 insertions(+), 17 deletions(-)

diff --git a/core/data/src/main/java/com/rtbishop/look4sat/core/data/aprs/AprsIsClient.kt b/core/data/src/main/java/com/rtbishop/look4sat/core/data/aprs/AprsIsClient.kt
index 27770496..7bb9e189 100644
--- a/core/data/src/main/java/com/rtbishop/look4sat/core/data/aprs/AprsIsClient.kt
+++ b/core/data/src/main/java/com/rtbishop/look4sat/core/data/aprs/AprsIsClient.kt
@@ -77,24 +77,31 @@ class AprsIsClient(
             val w = writer ?: return null
             w.println(packetLine)
             if (w.checkError()) return Pair(false, "write failed")
-        }
-        // Try to read the server response (short 3 s timeout; APRS-IS replies with an error line on bad format)
-        return runCatching {
-            val s = socket ?: return@runCatching Pair(true, "OK")
-            val oldTimeout = s.soTimeout
-            s.soTimeout = 3000
-            try {
-                val resp = reader?.readLine()
-                if (resp != null && (resp.contains("Invalid", ignoreCase = true) ||
-                        resp.contains("error", ignoreCase = true))) {
-                    Pair(false, resp.trim())
-                } else {
-                    Pair(true, if (resp.isNullOrBlank()) "OK" else resp.trim())
+            // Read the server response inside the same lock: disconnect() (called
+            // concurrently from stop()/reconnect on another thread) nulls
+            // writer/reader/socket and closes them. Reading outside the lock raced
+            // with that: the response read could hit a just-closed socket and the
+            // swallowing runCatching reported Pair(true,"OK") for a packet that
+            // never left, or read through a stale reference. Serialising keeps
+            // the read on the connection this thread just wrote to. The 3 s read
+            // timeout bounds how long a concurrent disconnect waits.
+            return runCatching {
+                val s = socket ?: return@runCatching Pair(true, "OK")
+                val oldTimeout = s.soTimeout
+                s.soTimeout = 3000
+                try {
+                    val resp = reader?.readLine()
+                    if (resp != null && (resp.contains("Invalid", ignoreCase = true) ||
+                            resp.contains("error", ignoreCase = true))) {
+                        Pair(false, resp.trim())
+                    } else {
+                        Pair(true, if (resp.isNullOrBlank()) "OK" else resp.trim())
+                    }
+                } finally {
+                    s.soTimeout = oldTimeout
                 }
-            } finally {
-                s.soTimeout = oldTimeout
-            }
-        }.getOrElse { Pair(true, "OK") }
+            }.getOrElse { Pair(true, "OK") }
+        }
     }
 
     /** Read one line (server response; throws on timeout) */