feat: serve lib decoupling

[LeeroyHannigan]

What

Decouples the ExtendDB server into library crates so a backend can be added crate-only, with no edits to any core crate. Four commits:

1. refactor(storage) replace inventory link-time registration with an explicit BackendRegistry installed once into a process-global OnceLock (set_registry). Backends now expose one register(&mut BackendRegistry) instead of six inventory::submit! blocks. Lookup free-function signatures are unchanged, so all call sites are untouched.
2. refactor(config) extract AppConfig + loading into a new backend-agnostic extenddb-config crate (breaks the would-be server ↔ app cycle).
3. refactor(server) promote the server orchestration to a public extenddb_server::serve(config, listener, port, run_dir, foreground, git_hash); move the generic background workers into extenddb-server.
4. refactor(app) lift the full CLI + dispatch into a new extenddb-app crate exposing run(BuildInfo); crates/bin becomes the Postgres thin bin and the reference template for backend authors.

First-party and third-party backends now follow the identical serve(config, registry) path, no privileged wiring for Postgres. Out of scope: the open-once/shared-handle lifecycle contract (only exclusive-lock embedded engines need it; no-op for Postgres/SQLite).

Why

The server orchestration (component assembly, cache wiring, AppState, worker spawning, TLS, PID lifecycle) lived in the bin crate, and backend registration relied on inventory link-time collection. A third party with only their backend crate + the published extenddb-* libraries could not run a server without forking bin. This makes the server a library call so a backend is a crate plus a ~10-line main.

Closes #168

Testing done

• cargo build --workspace, cargo fmt --all -- --check, cargo clippy --workspace --all-targets -- -D warnings (the CI gate): all clean.
• cargo test --workspace: 585 passed, 0 failed (unit + doc).
• Integration on a fresh Postgres deployment (mirrors integration.yml): comprehensive 326 passed / 0 failed; rust-integration transaction/idempotency/GSI/data-plane/management paths pass. The only integration failures are pre-existing capacity_throttling cases that are environment-sensitive locally and green in CI.
• End-to-end against live Postgres: version, serve + /health, a SigV4-signed CreateTable/PutItem/GetItem/DeleteTable round-trip, and every CLI subcommand — init, migrate, settings, manage, catalog-check, verify, status, stop, destroy.
• No regressions: a differential run of pre-refactor main through the identical local harness fails on exactly the same environment-sensitive tests, confirming the remaining local failures are not introduced by this change.
• Note: cargo clippy -- -W clippy::pedantic reports warnings, but they are pre-existing project-wide style (missing # Errors docs, backticks, function length) present on main; this PR's moves don't add net-new pedantic findings.

Checklist

• I have read CONTRIBUTING.md
• All tests pass (cargo test --workspace)
• Code is formatted (cargo fmt --check)
• Clippy is clean — CI gate cargo clippy --all-targets -- -D warnings passes (-W clippy::pedantic emits only pre-existing project-wide warnings, unchanged by this PR)
• I have added or updated tests for new functionality (registry-backed lookups covered by existing suites; added a pid_file_path test in extenddb-config)
• I have updated documentation if behavior changed — not yet: the required-backend change (below) needs a note in the config reference / sample docs; flagging as a follow-up
• Breaking changes are noted below (if any)
• If this changes the wire protocol, Storage trait, auth model, on-disk format, or public CLI surface, an RFC has been accepted or is linked below. Otherwise, an ADR captures the decision (link below).

ADR / RFC: RFC #168 (serve/library decoupling + registry). Public CLI surface is unchanged; the new public library surface (serve, BackendRegistry) is governed by RFC #168.

Breaking changes

StorageConfig deserialization no longer defaults the backend to "postgres". The [storage] backend key is now required, a backend-agnostic config crate cannot bake in a default. Configs that set backend explicitly (all shipped/sample configs do) are unaffected; a config omitting it now gets a clear error instead of a silent Postgres default.

---

By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache License 2.0 and I agree to the Developer Certificate of
Origin (DCO). See CONTRIBUTING.md for details.

[pdf-amzn]

Clean, well-motivated refactoring. The new crate topology makes sense and the inventory → explicit BackendRegistry migration eliminates a class of silent linker failures. Commit structure is excellent — each commit is independently reviewable and behavior-preserving.

One discussion point: serve() signature — git_hash: &str

The current signature is:

pub async fn serve(
    app_config: config::AppConfig,
    std_listener: TcpListener,
    port: u16,
    run_dir: String,
    foreground: bool,
    git_hash: &str,
) -> anyhow::Result<()>

Since this is a public library API, the git_hash lifetime is worth a quick discussion. All realistic callers pass env!("EXTENDDB_GIT_HASH") which is 'static, so &'static str would be the tighter contract. On the other hand, &str is strictly more permissive today and doesn't break anyone — it only becomes a problem if you later want to narrow to &'static str (breaking change) or if the function needs to store it beyond the call (would need String/'static).

Not blocking — happy to discuss offline to sort out what makes sense here

Minor non-blocking observations (no action needed now)

• port is derivable from std_listener.local_addr() — passing both creates a consistency coupling
• 6 positional params approaching ergonomic threshold; a ServeConfig struct would be more future-proof
• BackendRegistry::register_* silently overwrites on duplicate key — a tracing::warn would help catch wiring bugs
• REDACTED_CONFIG_KEYS comment says "keep in sync" — consider exporting from one place

Everything else looks solid. Nice work.

[pdf-amzn]

Non-blocking observations (9 items)

All of these are suggestions for future improvement — none should block merge.

---

1. port is redundant with std_listener

serve() takes both std_listener: TcpListener and port: u16, but the port is derivable from std_listener.local_addr().unwrap().port(). Passing both creates a logical coupling where the caller must ensure consistency. Consider extracting port from the listener internally.

2. 6 positional params → consider a config struct

Six positional parameters (one of which is a bool flag) is at the ergonomic threshold for a public library API. Before v1.0, consider:

pub struct ServeParams {
    pub app_config: AppConfig,
    pub listener: TcpListener,
    pub run_dir: String,
    pub foreground: bool,
    pub git_hash: &'static str,
}

pub async fn serve(params: ServeParams) -> anyhow::Result<()>

More future-proof (adding a field is non-breaking with #[non_exhaustive]) and self-documenting at call sites.

3. Workers lack a CancellationToken

Worker loops (loop { sleep(...).await; ... }) in workers.rs rely on tokio runtime drop for shutdown. This works today but makes graceful drain impossible (e.g., flushing final metrics before exit). A CancellationToken passed to each worker would allow ordered shutdown. Follow-up item.

4. env!("CARGO_PKG_VERSION") in app crate

The app crate uses env!("CARGO_PKG_VERSION") directly in print_version and the serve banner. This gives the app crate's version, which is fine while all workspace crates stay version-synchronized. But for full decoupling, consider threading the version through BuildInfo alongside git_hash and build_time.

5. postgres feature flag is vestigial

[features]
default = ["postgres"]
postgres = ["extenddb-storage-postgres"]

Since main.rs unconditionally calls extenddb_storage_postgres::register(), disabling the feature causes a compile error. Either remove the feature flag or gate the register call behind #[cfg(feature = "postgres")].

6. Silent overwrite on duplicate backend name

BackendRegistry::register_* methods use HashMap::insert which silently overwrites on duplicate key. If two backends accidentally register with the same name, the second wins silently. Consider:

if self.bootstrappers.contains_key(name) {
    tracing::warn!("Backend '{name}' already registered — overwriting");
}

Or even debug_assert!(!self.bootstrappers.contains_key(name)) for catching wiring bugs in dev.

7. Stale comment: # Database plugin registry

In Cargo.toml, the section comment # Database plugin registry now has no content below it (the inventory dep was the only thing there). Remove or repurpose.

8. Duplicate log_to_syslog_raw code

The syslog FFI helper appears in two forms: as a function in crates/server/src/serve.rs and as inline unsafe in the panic hook in crates/app/src/cmd_serve.rs. Since extenddb-server is a dep of extenddb-app, the app crate could import and reuse the server's helper, eliminating the duplication.

9. REDACTED_CONFIG_KEYS — export from one place

crates/config/src/display.rs defines REDACTED_CONFIG_KEYS with a comment: "keep in sync with REDACTED_KEYS in crates/server/src/console/pages/settings_pages.rs". Since extenddb-server depends on extenddb-config, the server could import the canonical list from the config crate, turning a manual-sync hazard into a compile-time guarantee.

[LeeroyHannigan]

Thanks Paul, this was a useful list. All nine are addressed. Pushed as fd43714, 3a78255, 0e2ddfb, 2b9ab1b, 0b388dd.

1, 2 (port redundancy, param count). serve() now takes a single #[non_exhaustive] ServeParams, built via ServeParams::new(..).with_log_target(..). #[non_exhaustive] blocks struct literals from other crates, so the constructor is required rather than optional. port is gone from the signature and read back from the bound listener, which also resolves an ephemeral (port 0) bind correctly.

git_hash lifetime. Tightened to &'static str as part of BuildInfo. Agreed on the reasoning: every caller already passes a compile-time env!, and narrowing later would be breaking for anyone who wrote a backend against the library.

3 (no CancellationToken). Every worker now takes a token and returns at its next tick. serve() cancels once the HTTP server stops accepting, then awaits the handles with a 5s bound. ServerRuntimeHooks::spawn_workers returns its JoinHandles so backend workers join the same drain instead of each backend inventing its own shutdown path. extenddb-storage re-exports CancellationToken and a sleep_or_shutdown helper so a third-party backend needs no tokio-util dependency and there is one implementation of the select! rather than one per worker.

Your point about flushing final metrics was correct: metrics_flush_worker was discarding its partial bucket on every shutdown. It now drains everything on the cancellation pass. insert_metrics upserts additively, so a partial bucket flushed at shutdown accumulates rather than overwriting after a restart.

Covered by three tests in 0b388dd, each mutation checked so they are not vacuous: reverting drain_age to FLUSH_INTERVAL fails the final-flush test, and reverting a worker to loop { sleep } fails the cancellation test. Live check: SIGTERM drained 13 workers (6 generic, 7 Postgres) in 38ms.

4 (CARGO_PKG_VERSION in the app crate). BuildInfo gains version, read from the bin crate, and the banner plus console version string use it. Worth noting serve() had the same bug: it reported the server crate's version. One caveat, BuildInfo lives in the server crate so extenddb-app can re-export it without a new crate. Build provenance is arguably not a server concept, so if you would rather it sat somewhere neutral, say so and I will move it.

5 (vestigial postgres feature). Removed rather than cfg-gated. With the feature off the bin would register no backend, so every command would fail at runtime with an unknown-backend error. The thin bin exists to wire exactly one backend, so the dependency is not optional.

6 (silent overwrite on duplicate name). Registration runs before the tracing subscriber exists, so a tracing::warn would be invisible. The registry records collisions and set_registry fails with RegistryError::DuplicateRegistrations, surfacing through main's ? before any request is served. RegistryAlreadySet folds into that enum, which is a public rename, safe only because the registry is new in this PR. Unit tested both ways.

7 (stale Cargo.toml comment). Removed. That was left behind by my inventory removal.

8 (duplicate log_to_syslog_raw). The app's panic hook now calls the server crate's copy. log_to_syslog_raw is pub and documented for that use, since tracing is unusable in the hook.

9 (REDACTED_CONFIG_KEYS single source). should_redact is exported from extenddb-config and imported by the console; the console's copy is deleted along with the "keep in sync" comment. This one fails in the unsafe direction if it drifts, so making it a compile-time guarantee was the right call.

Two things I have not resolved. The 5s drain bound stacks on the existing graceful window, so a worker wedged in a slow backend call could extend shutdown; the happy path is 38ms but I have not measured the pathological case against a 10s container grace period. Happy to lower the bound if you think that matters.

Verification: 620 unit tests and 408 Rust integration tests pass, clippy clean with no suppressions, and each of the five commits compiles standalone.

[pdf-amzn]

Design question: registry vs. single-backend setter

The BackendRegistry + HashMap approach supports multiple backends registered in one binary, with config-driven dispatch via [storage] backend = "postgres". But looking at the architecture — one [storage] section, one backend per deployment, no multi-tenant routing — this capability isn't used today, and the SQLite dev mode (#182) is a separate-binary use case.

Would a simpler single-backend design work equally well?

// In extenddb-storage:
pub struct Backend {
    pub bootstrapper: BootstrapperFactory,
    pub storage_config: StorageConfigDeserializer,
    pub operations: &'static dyn OperationsEngine,
    pub settings_store: SettingsStoreFactory,
    pub diagnostics_store: DiagnosticsStoreFactory,
    pub server_components: ServerComponentsFactory,
}

pub fn set_backend(backend: Backend) -> Result<(), BackendAlreadySet> { ... }

Thin bin becomes:

fn main() {
    extenddb_storage::set_backend(extenddb_storage_postgres::backend());
    extenddb_app::run(...)
}

What this would give you:

• Compile-time guarantee: no "unknown backend" errors possible
• No string-keyed dispatch — the backend = "postgres" config key becomes redundant (there's only one compiled in)
• Trivial public API: one struct, one function
• Same library decoupling goal achieved (backend plugged in by thin bin, not hard-wired)

What you'd lose:

• Multi-backend-in-one-binary (e.g. a hypothetical extenddb-multi registering both postgres + sqlite, config selects at runtime)
• The config file driving backend selection — each binary is its backend

Not blocking — the registry as implemented is clean and the extra complexity is bounded (~100 lines). This is more of a "is the HashMap dispatch earning its keep?" question. If there's a concrete plan for multi-backend binaries or config-time selection, the registry makes sense. If every deployment is always one-backend-per-binary anyway, set_backend is simpler and eliminates a class of runtime errors entirely.

Curious whether RFC #168 considered this alternative or specifically chose multi-backend support for a reason I'm not seeing.