Browse Source

feat(systemd): Type=notify so dependents wait for recovery (v1.7.5)

Pre-1.7.5 the unit was Type=simple — dependents using
After=smartbotic-database.service launched the moment the process
spawned, hit DB reads that timed out against the in-recovery
database, and silently fell back to defaults. On the shadowman
side this surfaced as settings.instance_type reading null on
tools-service startup, causing dev_mode to default to false —
dev-only tools and the plugin-dev skill silently disappeared
until someone manually restarted shadowman-tools after the DB
was fully up.

Fix: Type=notify + NotifyAccess=main + TimeoutStartSec=300.
The service code already calls sd_notify(READY=1) at the correct
point (after recovery, migrations, and gRPC start); switching the
unit type makes systemd actually wait for that signal before
activating the unit and unblocking dependents.
EXTEND_TIMEOUT_USEC=600000000 at each phase boundary protects
against systemd's start watchdog mid-recovery on huge datasets;
STATUS=... notifications make systemctl status show where the
service is during a slow boot.

Documents the lifecycle contract in CLAUDE.md so any future
consumer service knows the protocol.
fszontagh 2 tháng trước cách đây
mục cha
commit
5381d74803
4 tập tin đã thay đổi với 84 bổ sung3 xóa
  1. 38 0
      CLAUDE.md
  2. 1 1
      VERSION
  3. 17 1
      packaging/deb/systemd/smartbotic-database.service
  4. 28 1
      service/src/main.cpp

+ 38 - 0
CLAUDE.md

@@ -88,6 +88,44 @@ The submodule fallback still works for projects that haven't switched (`BUILD_SH
 
 Bump `VERSION` for API/protocol changes. Deb revision (`-N`) for packaging-only changes.
 
+## systemd integration (Type=notify, v1.7.5+)
+
+The service uses **`Type=notify`** in its systemd unit and signals readiness
+via `sd_notify(READY=1)` only after recovery + migrations complete and the
+gRPC listener is up (see `service/src/main.cpp` around the `service.start()`
+call). This is the lifecycle contract for dependents:
+
+- `After=smartbotic-database.service` + `Type=notify` on this unit means
+  systemd blocks the dependent's startup until the DB sends `READY=1`. A
+  dependent doing `db.get(...)` immediately on launch sees a fully-recovered
+  DB, never an in-recovery one. Pre-1.7.5 the unit was `Type=simple`, so
+  `sd_notify` was decoration and consumers could observe an in-recovery DB
+  and silently fall back to defaults — surfaced on the shadowman side as
+  `instance_type` reading null and `Dev mode enabled` never logging until
+  manual restart.
+- `TimeoutStartSec=300` covers slow recoveries on large datasets. The
+  service emits `EXTEND_TIMEOUT_USEC=600000000` (10 min) at each phase
+  boundary as belt-and-braces against systemd's start watchdog.
+- `STATUS=...` notifications are emitted at each phase ("Initializing —
+  running recovery and migrations" → "Starting gRPC server" → "Ready") so
+  `systemctl status smartbotic-database` shows where the service is during
+  a slow boot.
+- `sd_notify(STOPPING=1)` is emitted on graceful shutdown so dependents
+  observe a draining state distinct from a crash. The graceful-shutdown
+  path also takes a final snapshot (v1.8.0 hardening) so the next boot's
+  recovery is `TrivialSuccess` rather than `WalOnlyReplay`.
+
+**Dependents (shadowman-* services and any other consumer)** should:
+- Set `After=smartbotic-database.service` and `Wants=smartbotic-database.service`.
+- Set their own `Type=notify` if they have a meaningful "ready" boundary.
+- Treat critical-config DB reads as fail-loud: a null result on a setting
+  the service depends on is a startup-ordering bug, not a default fallback.
+
+**Operator-facing tooling** (deb postinst, ansible, etc.) should not layer
+its own port-poll or sleep loops on top of the unit. `systemctl restart
+smartbotic-database` (or `systemctl start ... && systemctl start dependent`)
+already blocks correctly until READY.
+
 ## Conventions
 
 - C++20

+ 1 - 1
VERSION

@@ -1 +1 @@
-1.7.4
+1.7.5

+ 17 - 1
packaging/deb/systemd/smartbotic-database.service

@@ -3,7 +3,23 @@ Description=Smartbotic Database Service
 After=network.target
 
 [Service]
-Type=simple
+# v1.7.5: Type=notify so dependents block at startup until the service is
+# *actually* ready (recovery complete + migrations applied + gRPC listening),
+# not just "process spawned." Pre-1.7.5 this was Type=simple, which made
+# `After=smartbotic-database.service` on consumers ineffective: tools,
+# gateway, server etc. would launch immediately and silently fall back to
+# defaults when their startup DB reads timed out against an in-recovery
+# database. The service code already calls `sd_notify(READY=1)` at the
+# correct point in `service/src/main.cpp` after `service.start()`; with
+# Type=notify systemd actually waits for that signal before considering
+# the unit active and unblocking dependents.
+Type=notify
+NotifyAccess=main
+# Recovery on a large dataset (700k+ docs, multi-MB WAL) routinely takes
+# 60–90s. Default systemd start timeout (90s) is too tight; bump to 5 min.
+# The service emits `EXTEND_TIMEOUT_USEC=` during long recovery anyway, but
+# the static cap is the safety net.
+TimeoutStartSec=300
 User=smartbotic-db
 Group=smartbotic-db
 ExecStart=/usr/bin/smartbotic-database --config /etc/smartbotic-database/config.json

+ 28 - 1
service/src/main.cpp

@@ -223,6 +223,20 @@ int main(int argc, char* argv[]) {
         std::signal(SIGINT, signalHandler);
         std::signal(SIGTERM, signalHandler);
 
+#ifdef HAVE_SYSTEMD
+        // v1.7.5: extend the systemd start timeout while recovery is
+        // running. Default TimeoutStartSec=90s + a 5-min unit cap is
+        // enough for most datasets, but very large databases (millions
+        // of docs, multi-GB WAL) can exceed that. EXTEND_TIMEOUT_USEC
+        // pushes systemd's clock back a fixed interval — we re-issue
+        // it on each major step so the watchdog never fires mid-replay.
+        // 600 seconds (10 min) per step is generous and only affects
+        // the start phase; once READY is sent, normal WatchdogSec
+        // semantics take over.
+        sd_notify(0, "EXTEND_TIMEOUT_USEC=600000000");
+        sd_notify(0, "STATUS=Initializing — running recovery and migrations");
+#endif
+
         // Initialize service. Recovery-refused is reported via the
         // recovery_outcome_ state (banner printed by DatabaseService) and
         // maps to exit code 10.
@@ -240,12 +254,25 @@ int main(int argc, char* argv[]) {
             service.setReadOnly(true, "started with --read-only flag");
         }
 
+#ifdef HAVE_SYSTEMD
+        sd_notify(0, "EXTEND_TIMEOUT_USEC=600000000");
+        sd_notify(0, "STATUS=Starting gRPC server");
+#endif
+
         // Start service
         service.start();
 
 #ifdef HAVE_SYSTEMD
-        // Notify systemd we're ready
+        // v1.7.5: only notify READY *after* full initialization +
+        // service.start() — recovery is done, migrations are applied,
+        // gRPC is listening. With the unit set to Type=notify,
+        // dependents (shadowman-tools, -gateway, -server) block in
+        // their own systemd start until this READY arrives, so they
+        // can never observe an in-recovery DB and silently fall back
+        // to defaults. Pre-1.7.5 the unit was Type=simple — sd_notify
+        // was effectively decoration.
         sd_notify(0, "READY=1");
+        sd_notify(0, "STATUS=Ready");
         spdlog::info("Notified systemd: READY");
 #endif