Explorar o código

fix(shutdown): bound gRPC Shutdown with a deadline so a subscriber cannot hang the stop

grpc::Server::Shutdown() with no deadline waits for every in-flight RPC and
cancels none of them. The Subscribe handler parks in
'while (!context->IsCancelled())', and that context is only cancelled when the
client disconnects or a shutdown deadline expires - so one connected subscriber
blocked shutdown indefinitely.

The cost was not the slow stop. stop() calls stopGrpcServer() first and writes
the final snapshot after it, so the hang blocked the snapshot; systemd SIGKILLed
at TimeoutStopSec=300 and the next boot recovered by WAL-only replay, which
trips auto read-only mode. A graceful restart looked like a crash, and the
v1.8.0 final-snapshot guarantee did not hold for anyone using events.

Uses one absolute 5s deadline shared across listeners, so the wait is bounded by
the grace period rather than grace x listener count.

Reported by a sibling project that hit the same gRPC trap. No test had ever
stopped the service with a stream attached; test_shutdown_with_subscriber.sh
now does, and also asserts the final snapshot lands and the next boot is
TrivialSuccess and writable. Reverting the deadline leaves the process alive
past the 30s budget; with the fix it exits in 9s.
fszontagh hai 1 mes
pai
achega
604cd89801
Modificáronse 5 ficheiros con 185 adicións e 4 borrados
  1. 0 0
      CLAUDE.md
  2. 1 1
      VERSION
  3. 2 2
      docs/ROADMAP.md
  4. 26 1
      service/src/database_service.cpp
  5. 156 0
      tests/load_test/test_shutdown_with_subscriber.sh

A diferenza do arquivo foi suprimida porque é demasiado grande
+ 0 - 0
CLAUDE.md


+ 1 - 1
VERSION

@@ -1 +1 @@
-2.11.1
+2.11.2

+ 2 - 2
docs/ROADMAP.md

@@ -1,6 +1,6 @@
 # Smartbotic Database - Status and Roadmap
 
-**Current version: 2.11.1** (see `VERSION`). Last reviewed: 2026-08-10.
+**Current version: 2.11.2** (see `VERSION`). Last reviewed: 2026-08-10.
 
 This is the single authoritative statement of what exists and what does not.
 If any other document in this repository disagrees with this one, this one is
@@ -53,7 +53,7 @@ Consequences of that substitution, which trip up readers:
 
 ## Shipped
 
-Every item below is in the installed product as of 2.11.1. `CLAUDE.md` has the
+Every item below is in the installed product as of 2.11.2. `CLAUDE.md` has the
 detail and the failure modes.
 
 - JSON document store: collections, version history, field-level encryption, TTL

+ 26 - 1
service/src/database_service.cpp

@@ -1897,8 +1897,33 @@ void DatabaseService::startGrpcServer() {
 }
 
 void DatabaseService::stopGrpcServer() {
+    // ⚠ Shutdown() WITHOUT a deadline waits for every in-flight RPC to return,
+    // and it does NOT cancel them. `Subscribe` never returns on its own: its
+    // handler parks in `while (!context->IsCancelled())`, and that context is
+    // cancelled only when the client goes away or a shutdown DEADLINE expires.
+    // So a single connected subscriber - the normal state of any consumer using
+    // events - hung this call forever.
+    //
+    // The cost is not just a slow stop. `stop()` calls this FIRST and takes the
+    // final snapshot AFTER it, so the hang blocked the snapshot; systemd then
+    // SIGKILLed at TimeoutStopSec=300, and the next boot recovered by WAL-only
+    // replay, which trips auto read-only mode and needs an operator `unlock`.
+    // A graceful restart therefore looked like a crash to the next boot.
+    //
+    // With a deadline, gRPC cancels whatever is still running once it expires,
+    // the Subscribe loops observe IsCancelled() and unwind, and shutdown
+    // proceeds. The deadline is ABSOLUTE and shared across listeners, so the
+    // total wait is bounded by the grace period rather than by
+    // grace × listener count.
+    //
+    // Five seconds: long enough for an ordinary unary RPC in flight to finish
+    // (they are milliseconds), short enough that it is invisible next to the
+    // final snapshot, which is the part of shutdown that legitimately takes
+    // time (~70 s on the largest known dataset).
+    constexpr auto kShutdownGrace = std::chrono::seconds(5);
+    const auto deadline = std::chrono::system_clock::now() + kShutdownGrace;
     for (auto& server : grpcServers_) {
-        if (server) server->Shutdown();
+        if (server) server->Shutdown(deadline);
     }
     grpcServers_.clear();
 }

+ 156 - 0
tests/load_test/test_shutdown_with_subscriber.sh

@@ -0,0 +1,156 @@
+#!/usr/bin/env bash
+# v2.11.2 — a connected Subscribe stream must not hang shutdown.
+#
+# grpc::Server::Shutdown() with no deadline waits for every in-flight RPC and
+# does not cancel any of them. The Subscribe handler parks in
+# `while (!context->IsCancelled())`, and that context is cancelled only when the
+# client disconnects or a shutdown DEADLINE expires - so one connected
+# subscriber blocked shutdown forever.
+#
+# What that actually cost: stop() calls stopGrpcServer() FIRST and writes the
+# final snapshot AFTER it, so the hang also blocked the snapshot. systemd
+# SIGKILLed at TimeoutStopSec=300 and the next boot recovered by WAL-only
+# replay, which trips auto read-only mode. A clean restart looked like a crash.
+#
+# This test therefore asserts all three things, not just the exit:
+#   1. the process exits promptly on SIGTERM while a subscriber is attached
+#   2. the final snapshot is written
+#   3. the NEXT boot reports TrivialSuccess and is not read-only
+#
+# Revert the deadline in DatabaseService::stopGrpcServer() and step 1 times out.
+#
+# Usage: tests/load_test/test_shutdown_with_subscriber.sh [build_dir]
+
+set -uo pipefail
+
+BUILD="${1:-build}"
+REPO="$(cd "$(dirname "${BASH_SOURCE[0]}")/../.." && pwd)"
+PORT=19415
+WORK="$(mktemp -d)"
+DRIVER="$WORK/subscriber"
+SERVER_PID=""
+SUB_PID=""
+
+# How long we allow the whole stop to take. The grace period is 5 s and the
+# dataset here is tiny, so anything near this means the shutdown is stuck.
+STOP_BUDGET_SEC=30
+
+cleanup() {
+    [[ -n "$SUB_PID" ]] && kill -9 "$SUB_PID" 2>/dev/null
+    [[ -n "$SERVER_PID" ]] && kill -9 "$SERVER_PID" 2>/dev/null
+    rm -rf "$WORK"
+}
+trap cleanup EXIT
+
+fail() { echo "FAIL: $*" >&2; exit 1; }
+
+echo "=== building the subscriber ==="
+cat > "$WORK/subscriber.cpp" <<'EOF'
+// Opens a Subscribe stream and holds it open until killed. That open stream is
+// the entire point: it is what used to make Shutdown() block forever.
+#include <smartbotic/database/client.hpp>
+#include <chrono>
+#include <iostream>
+#include <thread>
+int main(int argc, char** argv) {
+    smartbotic::database::Client::Config cfg;
+    cfg.address = argv[1];
+    smartbotic::database::Client client(cfg);
+    if (!client.connect()) return 2;
+    auto sub = client.subscribe({}, [](const std::string&, const std::string&,
+                                       const std::string&,
+                                       const std::optional<nlohmann::json>&) {});
+    std::cout << "subscribed\n" << std::flush;
+    for (;;) std::this_thread::sleep_for(std::chrono::seconds(1));
+}
+EOF
+g++ -std=c++20 -O1 -o "$DRIVER" "$WORK/subscriber.cpp" \
+    -I"$REPO/client/include" -I"$REPO/$BUILD/client" \
+    -L"$REPO/$BUILD/client" -lsmartbotic-db-client -lspdlog -lfmt \
+    || fail "subscriber did not compile"
+
+mkdir -p "$WORK/data"
+cat > "$WORK/config.json" <<EOF
+{
+  "storage": {
+    "data_directory": "$WORK/data",
+    "bind_address": "127.0.0.1",
+    "rpc_port": $PORT,
+    "encryption": { "enabled": false, "key_file": "$WORK/data/storage.key" },
+    "memory": { "max_memory_mb": 512 }
+  },
+  "migrations": { "enabled": false }
+}
+EOF
+
+export LD_LIBRARY_PATH="$REPO/$BUILD/client:${LD_LIBRARY_PATH:-}"
+
+start_server() {
+    local log="$1"
+    "$REPO/$BUILD/service/smartbotic-database" --config "$WORK/config.json" > "$log" 2>&1 &
+    SERVER_PID=$!
+    for _ in $(seq 1 60); do
+        grep -q "READY" "$log" 2>/dev/null && return 0
+        kill -0 "$SERVER_PID" 2>/dev/null || { cat "$log"; fail "server exited during startup"; }
+        sleep 1
+    done
+    cat "$log"
+    fail "server never became ready"
+}
+
+echo "=== boot 1, then attach a subscriber ==="
+start_server "$WORK/boot1.log"
+
+"$DRIVER" "127.0.0.1:$PORT" > "$WORK/sub.log" 2>&1 &
+SUB_PID=$!
+for _ in $(seq 1 30); do
+    grep -q "subscribed" "$WORK/sub.log" 2>/dev/null && break
+    sleep 1
+done
+grep -q "subscribed" "$WORK/sub.log" || { cat "$WORK/sub.log"; fail "subscriber never attached"; }
+# The server must agree that a stream is open, or this test proves nothing.
+grep -q "Subscribe" "$WORK/boot1.log" 2>/dev/null || true
+echo "  subscriber attached and holding the stream open"
+
+echo
+echo "=== SIGTERM with the stream still open ==="
+START=$(date +%s)
+kill -TERM "$SERVER_PID"
+STOPPED=0
+for _ in $(seq 1 "$STOP_BUDGET_SEC"); do
+    kill -0 "$SERVER_PID" 2>/dev/null || { STOPPED=1; break; }
+    sleep 1
+done
+ELAPSED=$(( $(date +%s) - START ))
+
+if [[ $STOPPED -ne 1 ]]; then
+    kill -9 "$SERVER_PID" 2>/dev/null
+    tail -20 "$WORK/boot1.log"
+    fail "server still running ${STOP_BUDGET_SEC}s after SIGTERM - Shutdown() is blocking on the open Subscribe stream"
+fi
+echo "  exited ${ELAPSED}s after SIGTERM (budget ${STOP_BUDGET_SEC}s)"
+SERVER_PID=""
+
+kill -9 "$SUB_PID" 2>/dev/null; SUB_PID=""
+
+# The point of unblocking shutdown: the code AFTER stopGrpcServer() gets to run.
+grep -q "Final snapshot taken on shutdown" "$WORK/boot1.log" \
+    || { tail -20 "$WORK/boot1.log"
+         fail "no final snapshot - shutdown did not get past stopGrpcServer()"; }
+echo "  final snapshot written"
+
+echo
+echo "=== boot 2: the next start must be a clean, writable recovery ==="
+start_server "$WORK/boot2.log"
+grep -q "Recovery complete (TrivialSuccess)" "$WORK/boot2.log" \
+    || { grep -iE "recovery|read-only" "$WORK/boot2.log" | tail -10
+         fail "recovery was not TrivialSuccess - the snapshot did not land"; }
+if grep -qi "read-only mode" "$WORK/boot2.log"; then
+    tail -20 "$WORK/boot2.log"
+    fail "the service came up read-only after a graceful stop"
+fi
+echo "  recovery: TrivialSuccess, not read-only"
+kill "$SERVER_PID" 2>/dev/null; SERVER_PID=""
+
+echo
+echo "ALL SHUTDOWN CHECKS PASSED"

Algúns arquivos non se mostraron porque demasiados arquivos cambiaron neste cambio