test: add Kafka CLI smoke tests for the broker, topics, configs, produce, consume, offsets and restarts

[solace-wkourlas]

Stacked on #862: review that one first. This PR adds the next set of smoke tests on top of its harness.

What this adds

Tests that run the Kafka CLI tools the way a user does, on every storage engine:

• broker: ApiVersions names node 111, and the cluster id is the one the broker started with.
• topics: describe, auto-create on produce, delete and re-create.
• configs: describe, add and delete topic configs, and broker defaults.
• produce: every acks setting, compression codecs, tombstones, max.message.bytes, idempotent and concurrent producers, LogAppendTime.
• consume: reading from an offset, a group resuming where it stopped, a pattern subscription that picks up a topic created later.
• offsets: lookups by time.
• restart: topics, records, committed offsets, topic configs and SCRAM users survive a SIGTERM restart on PostgreSQL and SQLite, and an in-memory broker starts again empty.
• storage_url: an unparsable or invalid storage URL stops the broker with an error that names it.

The harness now has one module per Kafka tool. Each command whose output a test reads has its own Output<Marker> type, so a reader can only be called on its own command's output. Brokers can restart on the same storage, and tests can start isolated brokers.

The CI change gives a smoke leg that fails before just smoke runs (setup, toolchain) a FAIL row, so it still shows in the report. run.sh also fails the leg if no nextest status line parses, instead of showing a green leg with no tests.

.claude/rules/smoke-tests.md sets the rules for writing a smoke test.

Ignored tests

A test that fails because of a broker bug is ignored with the bug in its reason, so CI stays green and the fix enables it. Every reason names the issue or PR that fixes it, e.g. #798 for the five produce tests that read the latest offset on in-memory storage, and #849 for the max-timestamp lookup.

Testing

• just clippy-equivalent for nisshi-smoke-test (all features, and each engine feature alone), cargo fmt, the crate's unit tests, rustdoc with warnings denied, ShellCheck on run.sh and actionlint on ci.yml all pass.
• just smoke postgres, sqlite and memory on this tree, rebased on main: every test that isn't ignored passes, and the shared broker passes its checks. Local runs also run the ignored tests: postgres 55 of 64 pass, sqlite 58 of 67, memory 45 of 58, and every failure is an ignored test.
• The maintenance_interval, timestamp-lookup and acks=0 tests were ignored for bugs that Fail clearly on bad storage config, lake-without-registry, and unauthenticated clients #850, fix: ListOffsets by timestamp should answer offset -1, not 0, when no record matches #838 and Produce with acks=0 no longer gets an unsolicited response; failure closes the connection #845 fixed, and are now enabled. acks=0 stays ignored on in-memory storage for Object-store ListOffsets(Latest) returns the newest batch's base offset + 1, not the high watermark #798, like the other produce tests.
• s3 is unchanged: it still runs only topic_lifecycle.

🤖 Generated with Claude Code

[solace-aross]

Leaving comments, not a verdict yet. The design is good, and a few items are worth settling before this goes in.

What I checked and found sound:

• Crash detection survived the harness rewrite. --restart=on-failure is gone, so a crashed broker now stays exited, and check_container_running reads State.Running and the exit code, then saves the broker's logs. That covers what the old RestartCount check caught.
• The CI change gives a leg that fails during setup a FAIL row, and run.sh fails a leg whose nextest log has no parseable status line. Both close ways a leg could look green with no tests.
• smoke-tests.md reads well. Checking only what a test's doc comment states is the right rule for a suite that reads human-oriented tool output.
• Disclosure check: no Solace-internal references, roadmap or hiring language, ticket IDs or customer names in the title, body, commits, diff or the two new rules files.

Findings:

1. At least one ignore reason looks stale. timestamp_after_last_record_returns_minus_1 in tests/smoke/offsets.rs is ignored with "the broker answers offset 0 when no record is at or after the timestamp". The CHANGELOG at this head already says ListOffsets by timestamp answers -1 in that case. I could not run it, so I do not know whether it holds on all three engines. Please un-ignore it and run it. If it fails on one engine, name that engine in the reason.
2. Link the PRs, not only issues. Two reasons describe fixes that are already in flight: the acks=0 response (#845) and the ListOffsets version advertising (#849). Naming those PRs in the reasons means the fix un-ignores its own test. The remaining ignored tests need issue numbers before merge; you say they are coming, and I would hold the approval for them, because an ignored test with prose only is a bug with no owner.
3. .claude/rules/naming.md is a workspace-wide naming policy (it applies to every *.rs, *.sh and justfile), added inside a smoke-test PR. It sets rules such as "spell words out" and "name a test for the behaviour it requires" that existing code does not follow. I would move it to its own PR so the team can agree on it separately. That is Andrea's and the maintainers' call, not a blocker from me.
4. Order: this is stacked on #862, which still has an open change request from Peter. The diff here will shrink once #862 merges, and it is easier to review then.

Not reviewed in depth: the broker.rs harness rewrite, the kafka_cli/* modules and the eight new test files, about 4,000 of the diff lines. I did not run the suite (no Docker where I reviewed). Please post your local postgres, sqlite and memory results when they finish, and I will look again.

[solace-wkourlas]

Thanks for going through this, @solace-aross! The results of the smoke tests are as follows:

• postgres: 54 of 63 passed. The 9 failures are all ignored tests.
• sqlite: 57 of 66 passed. The 9 failures are all ignored tests.
• memory: 44 of 57 passed. The 13 failures are all ignored tests.

These counts include the ignored tests, because they run locally too. The extra failures on memory are the four produce tests that are waiting for #798.

You were right about the timestamp test, which now passes on all three engines. While checking that, I found the two maintenance_interval tests in storage_url.rs were in the same situation after #850. The broker now stops on a bad interval, but it logs the error through tracing, and the harness only looked for an "Error:" line. The harness now checks every error line the broker prints. Both tests are un-ignored and pass on all three engines.

I also moved naming.md to its own PR, #903, so it can be discussed separately.

[solace-aross]

Thanks for the follow-up. Re-read d0e609a and 7dbb5f6 against my earlier comments.

Resolved:

• Timestamp and maintenance_interval tests are un-ignored and the harness now checks every error line the broker prints. Local results on all three engines are in your comment.
• naming.md is out of the diff (moved to #903, now merged to main).
• Issue numbers are on the ignores for #798, #904, #905, #906 and #907.

Still open, and the reason I have not approved yet:

1. Three ignored tests name no issue or PR: acks_0_stores_every_record (produce.rs:106), max_timestamp_lookup_fails_at_once_as_unsupported (offsets.rs:76, #849 is open) and alter_partitions_keeps_records_in_place (topics.rs:116, CreatePartitions).
2. acks_0_stores_every_record is likely stale once rebased. #845 merged to main, and this branch does not contain 8543d06 yet. Un-ignore it after the rebase and check it passes on all three engines; if it does not, name the engine.
3. This still sits on #862, which carries Peter's change request at 28fd07a, so the order of merging is #862 first.

Happy to approve once 1 and 2 are in. I have still not run the suite or read the broker.rs rewrite line by line.

[solace-aross]

Approving at 0133a0b. My open points from the last round are in:

• The CreatePartitions test is removed (b7177b2), so no ignore is left without an issue. #849 is now named on the max-timestamp test.
• acks_0_stores_every_record runs again after #845 (bc193cb). It is ignored only on the memory engine, with #798 named, like the other produce tests waiting on that.
• #862 is now the base, rebased onto current main, so the merge order is #862 first.

I read the new commits (fail-closed results, leftover-broker cleanup, the container start timeout, the 20 minute step timeout) and they look sound. I have not run the suite locally, so the sqlite, postgres and s3 legs in CI are what confirm the acks=0 test passes on those engines.

One thing to fix before merge: typos fails on unparseable in tests/smoke/storage_url.rs (lines 23 and 31, from the first commit). Renaming the two tests to unparsable_* clears it. CI was still running when I approved, so the smoke legs had not reported yet.

[solace-aross]

Approve with notes. Re-approving after the push to 1268ced.

The typos note is fixed: both smoke tests are now unparsable_*, and typos passes. That commit is the only change since my approval at 0133a0b.

Notes:

• CodeQL fails on this head with two critical alerts (rust/hard-coded-cryptographic-value) at nisshi-smoke-test/tests/smoke/restart.rs lines 143 and 180. Both are the literal "secret" used as the SCRAM password in test fixtures, so they look like false positives. Someone with security-events access needs to dismiss them as "used in tests", or the tests can generate the password (for example from unique_name), before the check goes green.
• The smoke legs are skipped on this PR run, so acks=0 on the sqlite, postgres and s3 engines is not confirmed by CI yet. test (postgres:17), clippy, fmt and both compat legs pass.

[solace-aross]

Approving at bc12723. The only change since my approval at 1268ced is how the restart tests get their SCRAM passwords. They now call a new random_password() helper, a random u128 in decimal, instead of the literal "secret". That removes the two CodeQL alerts at the source rather than dismissing them. The helper uses rand, which is already a workspace dependency, so Cargo.lock gains an edge and no new crate. The wrong-password test gets a different password, since a collision between two random u128 values is negligible. The unparsable_* rename is still in.

CodeQL and typos passed on the previous head. On this head clippy, Analyze (rust), both compat legs and third-party-license were still running when I approved. The sqlite, postgres and s3 smoke legs are skipped on this PR because the base is the stacked branch, so they first run once it lands on main.