[SLOP(claude-opus-4-8-high)] feat(universaldb): graceful postgres leader handoff on shutdown

[MasterPtato]

No description provided.

[MasterPtato]

Stack for rivet-dev/rivet

• #5552 [slopfix] fix(universaldb): match fdb conflict semantics in …
• #5551 [slopfix] fix(universaldb,util,pegboard-envoy): index dedup …
• #5398 feat(kitchen-sink): add churnDb actor and seed driver for co …
• #5397 chore: Clean up
• #5382 [slopfix] docs(self-hosting): correct postgres config format …
• #5356 [slopfix] fix(pegboard): decrement kv_storage_used metric on …
• #5354 [slopfix] refactor(universaldb,ups): single/multi-node udb o …
• #5353 [slopfix] perf(universaldb): fold postgres drain_batch into …
• #5342 [SLOP(claude-opus-4-8-high)] perf(universaldb): batch leader …
• #5341 [slopfix] docs(self-hosting): promote postgres from experime …
• #5340 [slopfix] fix(self-host): repair dev compose generation, kit …
• #5337 [SLOP(claude-opus-4-8-high)] feat(universaldb): graceful pos … 👈
• #5336 [slopfix] test(universaldb): postgres leader failover + abor …
• #5335 [SLOP(claude-opus-4-8-high)] feat(ups): table-backed postgre …
• #5329 [SLOP(claude-opus-4-8-high)] feat(universaldb): postgres lea …
• #5327 [SLOP(claude-opus-4-8)] feat(pegboard-envoy): rate-limit env …
• #5326 [SLOP(claude-opus-4-8)] feat(util): add rate limiter primiti …
• #5322 [SLOP(claude-opus-4-8)] feat(rivetkit-core): add serde durat …
• #5321 Add observation for serde_bare
• #5320 Add observation and ser/de metrics for serde calls
• #5314 [SLOP(claude-opus-4-8-medium)] chore: add profiling cargo pr …
• #5306 [SLOP(claude-opus-4-8)] fix(kitchen-sink): serve built front …
• main

Get stack: forklift get 5337
Push local edits: forklift submit
Merge when ready: forklift merge 5337

_{change mvxpzxsl}

[claude]

Review

Nice feature and the failover test is a good addition. Two correctness concerns worth a look before merge, plus one process nit.

1. The graceful udb.shutdown() call can be cancelled before it runs (force-abort path)

engine/packages/service-manager/src/lib.rs:

if abort {
    tokio::time::sleep(Duration::from_millis(50)).await;
    rivet_runtime::shutdown().await; // notifies the process-wide SHUTDOWN signal
    break;
}
...
// (after the loop)
pools.udb()?.shutdown().await;
rivet_runtime::TermSignal::stop();

start() runs entirely inside the future passed to rivet_runtime::run(), which races it against SHUTDOWN.notified():

tokio::select! {
    _ = notify.notified() => { ... None }
    res = f => Some(res),
}

tokio::select! drops the other branch as soon as one resolves. In the abort path, rivet_runtime::shutdown() fires the notify before the new pools.udb()?.shutdown().await call runs. Once that notification is observed on the next poll of the outer select!, f (the whole start() call tree, including the pending Postgres queries inside pools.udb()?.shutdown().await) is liable to be dropped mid-flight — so the graceful leader handoff this PR adds is exactly the thing most likely to get skipped on a hard/triple-signal abort, silently falling back to TTL-based failover instead.

This doesn't affect the normal single-SIGTERM graceful path (rivet_runtime::shutdown() is never called there before the udb shutdown runs), only the 3x-signal abort branch — but that branch was clearly written with graceful intent (// Give time for services to handle final abort + a 50ms sleep), so it's worth fixing. Simplest fix: move pools.udb()?.shutdown().await (and TermSignal::stop()) to run before rivet_runtime::shutdown() in the abort branch, or restructure so rivet_runtime::shutdown() is only called once as the very last step.

2. resolver_handle.abort() doesn't guarantee the in-flight renew has actually stopped

engine/packages/universaldb/src/driver/postgres/database.rs:

fn shutdown<'a>(&'a self) -> BoxFut<'a, ()> {
    Box::pin(async move {
        // Stop renewing the lease before releasing it so a racing renew cannot re-extend it.
        self.resolver_handle.abort();
        self.gc_handle.abort();
        resolver::handoff(&self.shared).await;
    })
}

resolver::handoff's doc comment states "Renewal must already be stopped before calling this, otherwise a racing renew could re-extend the lease" — but JoinHandle::abort() is a fire-and-forget cancellation request, not a synchronous stop. If the resolver task's lease::renew(...) UPDATE was already written to the socket (bytes in flight) at the moment abort() is called, Postgres can still execute and commit that renew (re-extending expires_at) after handoff()'s release() runs on its own connection, since release() doesn't check expires_at or ordering, just leader_addr. That silently undoes the graceful handoff and leaves the standby waiting out the full TTL anyway — the exact failure mode this PR is meant to eliminate.

Since shutdown(&self) can't get &mut access to resolver_handle to .await it (trait methods are all &self, presumably because DatabaseDriverHandle is an Arc<dyn DatabaseDriver>), actually waiting for the abort to land would need a small refactor (e.g. Mutex<Option<JoinHandle<()>>> so shutdown can .take() and await it before calling handoff). This is a narrow, low-probability race (needs a renew request in flight at the exact moment of shutdown, out of a 3s renew interval), but it's a real gap relative to the invariant the code comment claims.

3. PR title violates the repo's own convention

The title [SLOP(claude-opus-4-8-high)] feat(universaldb): graceful postgres leader handoff on shutdown includes a [SLOP(...)] prefix. Per CLAUDE.md: "Never indicate that a change was written by a coding agent: no model name, no agent name, no [SLOP(...)] prefix in the title, body, or PR text." Worth renaming to a plain conventional-commit title before merge.

Minor / non-blocking

• PostgresDatabaseDriver::shutdown() duplicates the abort() calls already present in Drop; harmless since abort() is idempotent, but could be factored into a shared helper.
• shutdown() only aborts resolver_handle/gc_handle; the PgListener reconnect task and PostgresShared::cache_refresh_task (both raw tokio::spawn, untracked by any handle) keep running until the whole process exits. Fine given the only call site is immediately before process exit, but worth a comment if shutdown() is ever expected to fully quiesce the driver for reuse.
• The new test_postgres_graceful_handoff test is a solid, real-infra integration test (matches the repo's no-mocking testing policy) and correctly exercises Database::shutdown() directly, but doesn't exercise the service-manager SIGTERM/abort wiring, so it wouldn't have caught issue [SVC-2555] Set up issue templates #1 above.

Everything else (lease fencing via leader_addr, the ELECTION_CHANNEL NOTIFY wakeup as a latency optimization over the ELECTION_RETRY poll, re-subscribing on RecvError::Closed) looks correct and well reasoned.