Преглед изворни кода

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 пре 1 месец
родитељ
комит
7b0274f9b6
5 измењених фајлова са 185 додато и 4 уклоњено
  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

Разлика између датотеке није приказан због своје велике величине
+ 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
 # 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.
 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
 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
 ## 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.
 detail and the failure modes.
 
 
 - JSON document store: collections, version history, field-level encryption, TTL
 - 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() {
 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_) {
     for (auto& server : grpcServers_) {
-        if (server) server->Shutdown();
+        if (server) server->Shutdown(deadline);
     }
     }
     grpcServers_.clear();
     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"

Неке датотеке нису приказане због велике количине промена