Skip to content

feat(kitchen-sink): add churnDb actor and seed driver for compaction read-hotshard repro#5398

Open
MasterPtato wants to merge 1 commit into
stack/chore-clean-up-mnloxsnmfrom
stack/feat-kitchen-sink-add-churndb-actor-and-seed-driver-for-compaction-read-hotshard-repro-yluvtyol
Open

feat(kitchen-sink): add churnDb actor and seed driver for compaction read-hotshard repro#5398
MasterPtato wants to merge 1 commit into
stack/chore-clean-up-mnloxsnmfrom
stack/feat-kitchen-sink-add-churndb-actor-and-seed-driver-for-compaction-read-hotshard-repro-yluvtyol

Conversation

@MasterPtato

Copy link
Copy Markdown
Contributor

No description provided.

@MasterPtato

Copy link
Copy Markdown
Contributor Author

Stack for rivet-dev/rivet

Current stack:

Dependencies:

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

change yluvtyol

@claude

claude Bot commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

Reviewed the new churnDb actor and seed-churn-dbs.ts driver script for the compaction read-hot-shard repro. This is manual load-test/ops tooling under examples/kitchen-sink, so I focused on correctness of the script itself rather than production-code concerns.

Issues

1. Header comment documents stale defaults that don't match the code (examples/kitchen-sink/scripts/seed-churn-dbs.ts:34-36)
The "All knobs are env vars" block says:

CHURN_TARGET_TXIDS      target committed-txn (delta) count per DB (default 18000)
CHURN_WORKING_SET_ROWS  rows in the bounded working set (default 64)
CHURN_ROW_BYTES         payload bytes per row (default 8192)

but the actual code defaults are 25_000, 16, and 256 respectively (seed-churn-dbs.ts:105-111, mirrored in churn-db.ts:71-74). Since the whole point of this script is precisely tuning per-commit DELTA size to stay under one FDB shard while going deep on txid count, someone re-running this later off the comment's stated defaults (64 rows x 8192 bytes ~ 512KiB/commit) would get a very different, and much larger, per-commit footprint than what's actually seeded (16 rows x 256 bytes ~ 4KiB/commit). Worth reconciling the comment with the code before this gets used as the source of truth for a repro run.

Also, CHURN_BUDGET_MS and CHURN_COMMIT_DELAY_MS are real, functional env knobs (seed-churn-dbs.ts:135-138) but aren't listed in that same doc block.

2. budgetMs isn't clamped against the actor's actionTimeout (examples/kitchen-sink/src/actors/testing/churn-db.ts:159 vs DEFAULT_BUDGET_MS/ChurnInput.budgetMs)
The action comment states "churn() self-limits with budgetMs and returns done=false before this fires," relying on the invariant budgetMs < actionTimeout (300_000ms). That holds for the default (120s) but nothing enforces it. If a caller sets CHURN_BUDGET_MS to 300s or more, churn() would hit the hard action timeout instead of gracefully returning done: false, breaking the resumable-call design the driver script depends on. Consider clamping budgetMs to something safely below actionTimeout (e.g. Math.min(budgetMs, actionTimeout - margin)) rather than trusting callers to respect the assumption.

3. Number(flag(...)) || envNum(...) silently discards an explicit 0 (examples/kitchen-sink/scripts/seed-churn-dbs.ts:104-118)
Every numeric CLI flag (--count, --target-txids, --working-set-rows, --row-bytes, --concurrency, --max-calls) is parsed as Number(flag(...)) || envNum(...). If a flag is explicitly passed as 0, Number("0") is 0, which is falsy, so the || silently falls through to the env/default value instead of using 0 or rejecting it via the envNum validation path (which would throw for non-positive values). Low impact since 0 isn't a meaningful value for most of these knobs, but it's a real inconsistency: a user explicitly passing --count 0 expecting a validation error (matching envNum's throw-on-invalid behavior) would instead get 3 DBs seeded silently.

Notes (non-blocking)

  • Error handling, rollback-on-failure, and the BEGIN/try/COMMIT/catch-ROLLBACK pattern all look correct and consistent with the rest of the codebase.
  • Endpoint credential redaction before logging (endpointForLog) is a nice touch.
  • Comment style is good: explains the why (shard-sizing rationale, throttling rationale) rather than restating the code, consistent with repo conventions.
  • No automated test coverage, but this is a manual ops/repro script, so that's expected and fine.

Overall this is solid, well-commented load-test tooling; the main thing worth fixing before it's relied on for the actual repro run is reconciling the doc comment with the real defaults (#1), since a mismatch there could send someone chasing the wrong shard-sizing shape.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant