Fix shutdown abort, transfer error reporting, log query params, and transferProgress payload

api/openapi.yaml

@@ -1301,7 +1301,7 @@ paths:
         | `propertyChanged` | `OnPropertyChangedCodes` | `{cameraId, codes: [...]}` | One or more property values changed on the camera |
         | `afStatus` | `OnWarning` (`CrWarningExt_AFStatus`) | `{state: "focused" \| "unlocked" \| "tracking", source?}` | Autofocus state changed |
         | `downloadComplete` | `OnCompleteDownload` | `{cameraId, filename, savedPath}` | Auto-transferred image saved to host (remote mode + matching `still-image-store-destination`) |
-        | `transferProgress` | `OnNotifyRemoteTransferResult` (or filesystem polling fallback) | `{cameraId, contentId, fileId, percent, savedPath?}` | Explicit file-pull progress (remote-transfer mode). On macOS V2.01 the SDK callback is unreliable; the server falls back to filesystem polling and emits synthetic events |
+        | `transferProgress` | `OnNotifyRemoteTransferResult` (or filesystem polling fallback) | `{cameraId, contentId, fileId, percent, notify, savedPath?, filename?, synthetic?}` | Explicit file-pull progress (remote-transfer mode). `filename` duplicates `savedPath` and is retained for existing consumers — prefer `savedPath`. `synthetic: true` marks an event produced by the filesystem-polling fallback rather than the SDK callback; on SDK V2.02 (macOS/Linux) the real callback fires reliably and `synthetic` is absent |
         | `contentsTransfer` | `OnContentsTransfer` | `{cameraId, files: [...]}` | Contents-transfer mode file list update (printable-ASCII filenames only) |
         | `operationResult` | `OnCompleteOperation` | `{cameraId, operation, result}` | Generic SDK operation completed (e.g. control codes, non-shutter commands) |
         | `error` | `OnError` | `{cameraId, code, message}` | SDK error occurred |

api/server/src/CameraWebApp.cpp

@@ -10,12 +10,17 @@
 // Global server instance for signal handling (atomic for safe concurrent access)
 std::atomic<cli::CameraWebServer*> g_server{nullptr};
 
+// Set by signalHandler, consumed by main()'s run loop.
+std::atomic<int> g_signalReceived{0};
+
+// Async-signal-safe: records the signal and returns. It must NOT call stop()
+// directly — stop() does stream I/O, joins threads, and takes several mutexes,
+// none of which are legal in a signal handler. Doing so also risks a hard
+// deadlock: a signal delivered to a thread already holding the shutdown mutex
+// would block the handler forever and make Ctrl-C stop working. main() does the
+// actual teardown once it observes this flag.
 void signalHandler(int signal) {
-    std::cout << "\nReceived signal " << signal << ", shutting down web server..." << std::endl;
-    auto* server = g_server.load();
-    if (server) {
-        server->stop();
-    }
+    g_signalReceived.store(signal);
 }
 
 int main(int argc, char* argv[]) {
@@ -72,11 +77,23 @@ int main(int argc, char* argv[]) {
     std::cout << "Press Ctrl+C to stop the server..." << std::endl;
     std::cout << std::endl;
 
-    // Keep server running until interrupted
-    while (server.isRunning()) {
-        std::this_thread::sleep_for(std::chrono::seconds(1));
+    // Keep server running until interrupted, or until something else (the
+    // detached POST /api/server/shutdown thread) clears the running flag.
+    while (server.isRunning() && g_signalReceived.load() == 0) {
+        std::this_thread::sleep_for(std::chrono::milliseconds(200));
     }
 
+    if (const int sig = g_signalReceived.load()) {
+        std::cout << "\nReceived signal " << sig << ", shutting down web server..." << std::endl;
+    }
+
+    // Tear down explicitly rather than leaving it to ~CameraWebServer(). stop()
+    // is idempotent and serialized, so this is safe even when the detached
+    // shutdown thread is already inside it — we simply block until it is done.
+    // Doing it here (not in the destructor) keeps the join on a thread that is
+    // still fully constructed.
+    server.stop();
+
     // Null out global pointer before server goes out of scope (prevents signal handler dangling pointer)
     g_server.store(nullptr);
 

api/server/src/CameraWebServer.cpp

@@ -168,20 +168,40 @@ bool CameraWebServer::start() {
 }
 
 void CameraWebServer::stop() {
-    if (!m_running) return;
-
-    std::cout << "[Shutdown] Stopping web server..." << std::endl;
-    m_running = false;
-
-    if (m_serverSocket >= 0) {
-        close(m_serverSocket);
-        m_serverSocket = -1;
-    }
-
+    // Serialized, and deliberately WITHOUT an early return before the join.
+    //
+    // stop() is reached from three directions: the detached thread spawned by
+    // POST /api/server/shutdown, main()'s run loop, and ~CameraWebServer().
+    // The old `if (!m_running) return;` guard sat above the join, so the second
+    // caller returned early and skipped it — leaving m_serverThread joinable at
+    // destruction, which is std::terminate → SIGABRT → exit 134.
+    //
+    // The mutex matters as much as the missing join: without it the two callers
+    // could reach m_serverThread.join() concurrently, and joining the same
+    // thread from two threads is itself undefined. Holding it across the whole
+    // teardown also stops ~CameraWebServer() from destroying members while the
+    // detached thread is still inside here.
+    std::lock_guard<std::mutex> guard(m_shutdownMutex);
+    const bool wasRunning = m_running.exchange(false);
+
+    if (wasRunning) {
+        std::cout << "[Shutdown] Stopping web server..." << std::endl;
+
+        if (m_serverSocket >= 0) {
+            close(m_serverSocket);
+            m_serverSocket = -1;
+        }
+    }
+
+    // Always join. After the first caller joins, the thread is no longer
+    // joinable, so later callers fall through harmlessly.
     if (m_serverThread.joinable()) {
         m_serverThread.join();
     }
 
+    // The remainder is one-shot teardown; the first caller already ran it.
+    if (!wasRunning) return;
+
     // Close all SSE client sockets to unblock their keepalive loops
     {
         std::lock_guard<std::mutex> lock(m_sseClientsMutex);
@@ -314,21 +334,40 @@ void CameraWebServer::handleClient(int clientSocket) {
     close(clientSocket);
 }
 
+std::string HttpRequest::queryParam(const std::string& name, const std::string& fallback) const {
+    size_t pos = 0;
+    while (pos < query.size()) {
+        const size_t amp = query.find('&', pos);
+        const size_t end = (amp == std::string::npos) ? query.size() : amp;
+        const size_t eq = query.find('=', pos);
+        if (eq != std::string::npos && eq < end && query.compare(pos, eq - pos, name) == 0) {
+            std::string value = query.substr(eq + 1, end - eq - 1);
+            if (!value.empty()) return value;
+        }
+        if (amp == std::string::npos) break;
+        pos = amp + 1;
+    }
+    return fallback;
+}
+
 HttpRequest CameraWebServer::parseRequest(const std::string& request) {
     HttpRequest req;
 
     std::istringstream iss(request);
     std::string line;
-    
+
     // Parse request line (GET /path HTTP/1.1)
     if (std::getline(iss, line)) {
         std::istringstream requestLine(line);
         std::string httpVersion;
         requestLine >> req.method >> req.path >> httpVersion;
 
-        // Remove query parameters if present
+        // Split the query string off the path. Routing matches on the bare
+        // path, but the query is kept — it used to be discarded here, which is
+        // why handlers documenting `?lines=`/`?level=` could never honour them.
         size_t queryPos = req.path.find('?');
         if (queryPos != std::string::npos) {
+            req.query = req.path.substr(queryPos + 1);
             req.path = req.path.substr(0, queryPos);
         }
     }
@@ -1706,15 +1745,18 @@ void CameraWebServer::startLiveViewBroadcasting() {
 }
 
 void CameraWebServer::stopLiveViewBroadcasting() {
-    if (!m_broadcastingLiveView.load()) {
-        return;
-    }
-
-    m_broadcastingLiveView = false;
+    // Same shape as stop(), and the same hazard: the early return sat above the
+    // join, so a second caller could leave m_broadcastThread joinable and abort
+    // at destruction. Shares stop()'s mutex so the destructor's two teardown
+    // calls cannot interleave with the detached shutdown thread. Neither
+    // function calls the other, so there is no lock ordering to get wrong.
+    std::lock_guard<std::mutex> guard(m_shutdownMutex);
+    m_broadcastingLiveView.exchange(false);
+
     if (m_broadcastThread.joinable()) {
         m_broadcastThread.join();
+        std::cout << "Stopped live view WebSocket broadcasting" << std::endl;
     }
-    std::cout << "Stopped live view WebSocket broadcasting" << std::endl;
 }
 
 void CameraWebServer::liveViewBroadcastThread() {
@@ -2511,6 +2553,42 @@ HttpResponse CameraWebServer::handleApiListSDCardFiles(const std::string& camera
     return response;
 }
 
+namespace {
+
+// Maps a failed RemoteTransfer start into an HTTP status + a message that says
+// what actually happened. Shared by the full-file and thumbnail/screennail
+// paths so the two cannot drift.
+//
+// 0x8D03 (CrError_RemoteTransfer_GetContentsDataDisable) is the important one:
+// it does NOT mean the requested transfer failed, it means one is already in
+// flight. It clears on its own the moment the in-flight transfer finishes, so
+// it is a 409 the caller should retry — not a 400. Reporting it as a hard
+// failure is what led the investigation in #40 to conclude that a single stuck
+// transfer "latches" the subsystem; in fact every later request was correctly
+// reporting "busy" because the stuck one never completed.
+struct TransferError {
+    int statusCode;
+    std::string message;
+};
+
+TransferError classifyTransferError(const std::string& sdkMessage, const std::string& what) {
+    if (sdkMessage.find("0x00008D03") != std::string::npos) {
+        return {409,
+                "A transfer is already in progress on this camera, so the " + what +
+                " was not started. This is transient — retry once the current transfer "
+                "completes (watch for a transferProgress event). SDK error 0x00008D03."};
+    }
+    if (sdkMessage.find("0x00008D02") != std::string::npos) {
+        return {400,
+                "Failed to start " + what +
+                ". Confirm the file identifiers are valid for the current connection "
+                "mode and retry (SDK error 0x00008D02)."};
+    }
+    return {400, sdkMessage};
+}
+
+}  // namespace
+
 HttpResponse CameraWebServer::handleApiDownloadSDCardFile(
     const std::string& cameraId,
     const std::string& slotNumber,
@@ -2574,12 +2652,11 @@ HttpResponse CameraWebServer::handleApiDownloadSDCardFile(
         root["message"] = result.message.empty() ? "Download started" : result.message;
         response.statusCode = 202; // Accepted — download is async
     } else {
-        std::string message = result.error_message;
-        if (message.find("0x00008D02") != std::string::npos) {
-            message = "Failed to start file download. Confirm the file identifiers are valid for the current connection mode and retry (SDK error 0x00008D02).";
-        }
-        root["message"] = message;
-        response.statusCode = 400;
+        const auto classified = classifyTransferError(result.error_message, "file download");
+        root["message"] = classified.message;
+        root["retryable"] = (classified.statusCode == 409);
+        response.statusCode = classified.statusCode;
+        addLog(classified.statusCode == 409 ? "warn" : "error", classified.message, cameraId);
     }
 
     response.body = root.toStyledString();
@@ -2644,8 +2721,12 @@ HttpResponse CameraWebServer::handleApiDownloadCompressed(
         root["type"] = type;
         response.statusCode = 202;
     } else {
-        root["message"] = result.error_message;
-        response.statusCode = 400;
+        const auto classified = classifyTransferError(result.error_message, type + " download");
+        root["message"] = classified.message;
+        root["type"] = type;
+        root["retryable"] = (classified.statusCode == 409);
+        response.statusCode = classified.statusCode;
+        addLog(classified.statusCode == 409 ? "warn" : "error", classified.message, cameraId);
     }
 
     response.body = root.toStyledString();
@@ -3259,24 +3340,30 @@ HttpResponse CameraWebServer::handleApiServerLogs(const HttpRequest& request) {
     response.contentType = "application/json";
     response.statusCode = 200;
 
-    // Parse query parameters: ?lines=100&level=info
-    int maxLines = 100;
-    std::string minLevel = "info";
-
-    // Query params could be parsed from request.path if needed in the future
-
-    std::lock_guard<std::mutex> lock(m_logMutex);
-
-    // Filter by level
     auto levelPriority = [](const std::string& level) -> int {
         if (level == "debug") return 0;
         if (level == "info") return 1;
         if (level == "warn") return 2;
         if (level == "error") return 3;
-        return 1;
+        return -1;  // unrecognised
     };
 
-    int minPriority = levelPriority(minLevel);
+    // Parse query parameters: ?lines=100&level=info
+    int maxLines = 100;
+    try {
+        const int requested = std::stoi(request.queryParam("lines", "100"));
+        // Clamp rather than reject: a caller asking for more than we retain
+        // should get everything we have, not an error.
+        maxLines = std::max(1, std::min(requested, static_cast<int>(MAX_LOG_ENTRIES)));
+    } catch (const std::exception&) {
+        maxLines = 100;  // non-numeric ?lines= falls back to the default
+    }
+
+    const std::string requestedLevel = request.queryParam("level", "info");
+    int minPriority = levelPriority(requestedLevel);
+    if (minPriority < 0) minPriority = levelPriority("info");
+
+    std::lock_guard<std::mutex> lock(m_logMutex);
 
     std::ostringstream json;
     json << "{\n  \"success\": true,\n  \"logs\": [\n";
@@ -3300,7 +3387,9 @@ HttpResponse CameraWebServer::handleApiServerLogs(const HttpRequest& request) {
         }
     }
 
-    json << "\n  ],\n  \"total\": " << total << "\n}";
+    // `total` is everything retained; `returned` is what survived the lines/level
+    // filter. Without both, a filtered response looks like data loss.
+    json << "\n  ],\n  \"returned\": " << count << ",\n  \"total\": " << total << "\n}";
 
     response.body = json.str();
     return response;

api/server/src/CameraWebServer.h

@@ -33,10 +33,16 @@ namespace cli {
 
 struct HttpRequest {
     std::string method;
-    std::string path;
+    std::string path;   // query string stripped, so routing stays exact-match
+    std::string query;  // raw query string, without the leading '?'
     std::string body;
     std::string headers;
     std::string contentType;
+
+    // Value of `name` from the query string, or `fallback` if absent/empty.
+    // Percent-decoding is deliberately not attempted: the only consumers today
+    // are numeric/enum scalars, and a half-correct decoder is worse than none.
+    std::string queryParam(const std::string& name, const std::string& fallback = "") const;
 };
 
 struct HttpResponse {
@@ -80,6 +86,10 @@ class CameraWebServer {
     int m_serverSocket;
     std::atomic<bool> m_running;
     std::thread m_serverThread;
+    // Serializes stop() / stopLiveViewBroadcasting(), which are each reachable
+    // from the detached shutdown thread, main(), and the destructor. Guards the
+    // joins so no two callers can join the same std::thread concurrently.
+    std::mutex m_shutdownMutex;
     std::unique_ptr<CameraWebController> m_cameraController;
 
     // WebSocket support