Fail clearly on bad storage config, lake-without-registry, and unauthenticated clients

[solace-aross]

Updated after review (0766f20): Builder::storage stays infallible in both crates; the maintenance interval options are parsed and stripped by the broker only. Intervals parse with human_units then humantime; bare numbers, zero and values over 365 days are rejected. The pre-auth rejection is logged at ERROR with the client address in broker.rs, and the frame.rs lines are debug!. Details below that disagree with this predate the update.

Summary

The broker used to panic or hang on bad input instead of failing clearly. This PR makes it fail fast with a specific message in every case, and logs a clear warning instead of silently hanging when a client sends a request before authenticating.

Problems and fixes

1. A maintenance_interval/transaction_maintenance_interval storage-URL option that doesn't parse, or parses to zero, used to be silently ignored (defaulting), or later panicked in tokio::time::interval (which panics on a zero period). Builder::storage now validates both options up front and returns an error naming the option and value; the CLI prints a specific message and exits non-zero instead of a panic or a silent wrong default.

2. Every storage engine now rejects a query option it doesn't recognise (a typo like maintainance_interval, or an option that belongs to a different engine, e.g. vacuum_into on a non-sqlite engine) instead of silently passing it through. Postgres already rejected unknown options natively via tokio-postgres's own parser; added a regression test for it instead of new code.

3. A data lake subcommand (iceberg, delta, parquet) without --schema-registry used to panic on .unwrap(). --schema-registry is now validated synchronously in Arg::build() before the broker starts, so the error comes from argument parsing, not a panic.

   The first design for this made --schema-registry a subcommand-local argument duplicated under each lake subcommand. That's wrong: clap only parses a subcommand-local flag positioned after the subcommand name, and this repo's own usage (justfile, compose.yaml, docs) puts --schema-registry before the subcommand. A reproduction confirmed the ordering bug. The fix instead keeps --schema-registry as a single top-level arg (global = true), which parses correctly in both positions. schema_registry_before_subcommand_parses is a regression test for this specific ordering.

4. A client that sends a request before completing authentication used to have its connection silently dropped, with no indication of why, and in testing this hung for the test harness's full retry window. Checked this against Apache Kafka at the 3.9.0 source level (not a live comparison): Kafka's own SASL-handshake enforcement behaves the same way (closes the connection if the first request isn't a SASL handshake), and the original "hang" was the test harness's own timeout being shorter than the client's default retry window, not a Nisshi bug. The fix: log one specific warn! (client address via the existing tracing span, api_key, api_name) at the point of rejection, instead of two redundant generic error! dumps further down the stack.

Duration grammar change (disclosure)

Validating maintenance_interval/transaction_maintenance_interval required picking a parser. These moved from humantime's grammar to human_units's, to match the grammar already used by sibling options (batch_max_delay, busy_timeout) in the same URL query-string context. human_units's grammar is narrower:

• Before (humantime): compound durations (1h30m) and more unit spellings (min, sec, us) parsed.
• After (human_units): only <integer><ns|μs|ms|s|m|h|d> or a bare integer (seconds).

Concretely: maintenance_interval=5min and maintenance_interval=1h30m parsed fine before this change and now produce a startup error. This is a real, user-visible behavior change beyond the ticket's literal ask (which wanted validation added, not necessarily a narrower grammar). It's likely an improvement regardless — anyone currently using a compound duration got a silent wrong default before, and now gets an immediate, clear startup error — but it's a different outcome for the exact same input, so reviewers should know about it going in.

Other known, accepted gaps (disclosed, not fixed here)

• nisshi-service/src/stream.rs:816 still logs a generic connection-ending error! in addition to the new, specific warn!. This is pre-existing, not introduced by this ticket, and isn't being touched: that layer's error type is fully generic/opaque, and specializing it would need a broader change affecting the proxy and client error paths too. Net result as shipped: one new specific WARN plus the pre-existing generic ERROR, down from two generic ERRORs before this change.
• nisshi-storage-sql/src/limbo.rs:1075 (and :4070) call .unwrap() on a non-UTF-8 database path. Low severity (experimental, turso-backed engine); flagged here rather than split into a separate ticket.

Follow-up tickets filed

• Follow-up — a recognised storage option with a value that fails to parse still falls back to a default silently on dynostore and the sqlite engine (separate, already-scoped-out gap from this ticket's validation).
• Follow-up — --listener-url with a non-IP hostname silently falls back to the unspecified (wildcard) address instead of resolving the hostname or failing clearly, in nisshi-broker/src/broker.rs's Broker::listen. An operator who configures a specific listen interface silently gets the wildcard address instead, which is a misconfiguration-tolerated-as-network-exposure-surface issue.

Review history

This is round 2 of 2 (capped). Round 1 found a dead UnsupportedStorageUrl(Url) error variant in nisshi-storage; round 2 found a second, separate dead variant of the same name in nisshi-broker's own Error enum (also from the tansu rename, zero constructors) — deleted in this round. Round 2 also found the two new storage error variants were falling through to the CLI's generic "Unknown error occurred" catch-all instead of a specific message — added two match arms following the existing LakeRequiresSchemaRegistry pattern.

Test output

Targeted suites (nisshi-cli, nisshi-storage + nisshi-storage-sql/-dynostore/-null/-slatedb, nisshi-broker including unauthenticated::*, nisshi-service), run via cargo nextest run --workspace --all-features --all-targets filtered to these packages: 573 passed, 0 failed (1 slow, 39 skipped by backend/feature gating).

unauthenticated::* specifically:

PASS [0.110s] nisshi-broker::it unauthenticated::api_versions_exempt_before_authentication
PASS [0.111s] nisshi-broker::it unauthenticated::request_before_authentication_closes_connection
Summary: 2 tests run: 2 passed, 0 skipped

cargo clippy --workspace --all-features --all-targets -- -D warnings: clean (0 errors).
cargo fmt --all -- --check: clean.

The new unauthenticated.rs test was mutation-tested twice: once reproducing the original "hangs" bug (replacing the rejection with a never-resolving future) — confirmed the test correctly fails after a bounded timeout instead of hanging forever; once reproducing a "silently bypasses auth" bug (disabling the check entirely) — confirmed the test correctly fails because it gets back a real, unexpected successful response instead of a closed connection. Both mutations correctly left api_versions_exempt_before_authentication passing, since ApiVersions is supposed to work regardless of authentication state.

Live binary runs confirmed both new CLI error messages print clearly and exit non-zero, e.g.:

$ nisshi broker --storage-engine 'memory://nisshi/?maintenance_interval=0s'
ERROR nisshi: storage option maintenance_interval=0s is not a valid non-zero duration (expected e.g. 10m, 90s, 500ms)

$ nisshi broker --storage-engine 'memory://nisshi/?typo_option=1'
ERROR nisshi: storage option typo_option is not recognised by the memory engine

🤖 Generated with Claude Code

https://claude.ai/code/session_01D2qdgPhMyLGZN5dLR8CVHs

[sgamelin]

Thanks for this. The lake-without-registry fix and the global = true reasoning look right, and the pre-authentication change matches Kafka. I checked that at 3.9.1, the version our descriptors pin, rather than 3.9.0: SaslServerAuthenticator.handleKafkaRequest throws IllegalSaslStateException for any request other than ApiVersions or SaslHandshake, and the connection closes (SaslServerAuthenticator.java#L520-L523).

Main points, inline:

1. None of the new storage-option validation has a test.
2. nisshi_storage::Builder::storage accepts the two interval options, and then every engine rejects them.
3. The duration grammar change breaks configs that start today. The description's reason for it isn't accurate.
4. The default log filter hides the new warn!.

Also:

• CHANGELOG: [Unreleased] is empty. This PR changes startup in three ways users will notice: unknown storage options are fatal, the duration grammar is narrower, and a bare integer now means seconds instead of being ignored. It also makes Builder::storage fallible in two published crates. Could you add ### Changed entries?
• Postgres: tokio-postgres rejects the unknown option, so the error reaches the generic "Unknown error occurred" arm in nisshi.rs, not the new message. That's fine, but the new test in pg.rs passes on any error from Postgres::builder. Asserting that the error names vacuum_into would pin what it claims.
• --schema-registry: schema_registry_before_subcommand_parses also passes without global = true, because the flag was already top-level. What global = true adds is the flag after the subcommand (parquet --location ... --schema-registry ...), and a test for that order would pin it.

Merge coordination with other open PRs:

• #844 adds its own nisshi_cli::Error::Server(..) arm to the same match in nisshi.rs. Git merges both cleanly. But #844's arm comes first and ends in _ =>, so this PR's arm becomes unreachable, and unreachable_patterns then fails clippy with -D warnings. Whichever PR merges second has to fold both inner matches into one Server arm and choose one fallback: #844 prints the inner error, and this PR prints err.
• #845 and #826: #845 (tests/it/produce_acks_zero.rs) and #826 (tests/it/common/wire.rs) add Broker::builder()...storage(...) calls without ?. Git won't flag a conflict there, so whichever lands second has to add the ?.
• #849 conflicts in the nisshi_storage::Error enum. Keep both sets of new variants and drop UnsupportedStorageUrl. #849 also deletes get_telemetry_subscriptions.rs, whose doctest this PR edits, so take the deletion. #835 rewrites delete_records.rs and removes the test this PR edits, so take #835's file. All three resolutions are mechanical.

[solace-aross]

Summary points, in 0766f20:

• CHANGELOG ### Changed entries for unknown storage options, interval validation (bare numbers and the 365-day cap), lake without registry, and the pre-auth log line.
• The pg test now asserts the error names vacuum_into.
• Added schema_registry_after_subcommand_parses; it fails without global = true.
• With both builders infallible again, Produce with acks=0 no longer gets an unsolicited response; failure closes the connection #845 and fix: answer OFFSET_OUT_OF_RANGE for a fetch offset outside the partition #826 no longer need a ?, and the fix: implement DeleteRecords on lite, limbo and dynostore; fix Postgres column names #835 and doctest conflicts are gone. fix(storage): fail broker startup when S3/GCS storage isn't usable #844 (nisshi.rs Server arm) and Cap and advertise per-route Kafka API version ranges #849 (nisshi_storage::Error) still need the merges you described.

[sgamelin]

Thanks for this round. Every earlier point is addressed in 7d37ca0:

• Tests: each of the four mutations I listed now fails a test, apart from part of the last one (inline). The new tests pass at dc5bb6d.
• Interval options: they now live only in nisshi-broker, and both Builder::storage methods are infallible again, as on main.
• Duration grammar: 1h30m and 5min parse as on main, a bare number fails, and the 365-day cap stays well below tokio's Instant overflow.
• Logging: a rejected pre-authentication connection now logs one ERROR line with the peer address under the default filter.
• The rest: the CHANGELOG entries, the pg assertion and the after-subcommand test are in.

Approving. Three optional points, inline.

Merge coordination with other open PRs:

• #844: still needs the two Server arms in nisshi.rs folded into one, as in my earlier review.
• #845: conflicts in the connection-error match in broker.rs. Keep both new arms.
• #827: conflicts in the pg.rs tests. Keep both tests.
• #849: still conflicts in nisshi_storage::Error. Keep both sets of variants.
• #826 and #835: now merge cleanly.