Skip to content

feat(kitchen-sink): add load testing harness#5017

Draft
NathanFlurry wants to merge 1 commit into
counter-latency/client-protocol-polishfrom
counter-latency/kitchen-sink-load-harness
Draft

feat(kitchen-sink): add load testing harness#5017
NathanFlurry wants to merge 1 commit into
counter-latency/client-protocol-polishfrom
counter-latency/kitchen-sink-load-harness

Conversation

@NathanFlurry
Copy link
Copy Markdown
Member

Description

Please include a summary of the changes and the related issue. Please also include relevant motivation and context.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes.

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

@claude
Copy link
Copy Markdown
Contributor

claude Bot commented May 11, 2026

Code Review: feat(kitchen-sink): add load testing harness

Overview

This PR adds a comprehensive load testing harness to the kitchen-sink example:

  • Major rewrite of counter-latency.ts with subcommand-style CLI, worker health tracking, and SIGINT/SIGTERM handling
  • Two new actor types: tunnelStress (raw WebSocket stress) and loadTestAgent (SQLite-backed inference simulation)
  • New Dockerfile.local and build-push-kitchen-sink-local.sh for local image builds

Overall the PR is well-structured. A few issues worth addressing before merging.


Bugs / Correctness

1. Race condition in tunnel-stress.ts message handler

The message listener is async with no backpressure. A second message can arrive and increment messageCount before the KV write from the first finishes. Under high load, KV writes may overlap or commit out of order. If messageCount is meant to reflect committed KV entries this will not hold. Consider fire-and-forget (void c.kv.put(...)) if ordering does not matter, or serialize writes through a queue.

2. Magic string coupling for actor-stopped reconnect

ACTOR_STOPPED_CLOSE_REASON = "hack_force_close" is an internal engine detail baked into the test harness. If the engine changes this reason string, reconnect detection silently breaks with no compile error. Either export the constant from RivetKit or add a comment pointing to the specific source file in the engine so it is obvious when the string needs updating.


Code Quality

3. openWebSocket interface contract is ambiguous

In runConcurrentWorker, actorId is always resolved via workload.resolveActorId before calling openWebSocket. Both workload implementations then have a dead fallback client.foo.getOrCreate(...) branch that can never be reached. Declaring actorId: string (non-optional) in ConcurrentWorkload.openWebSocket would make the contract explicit and eliminate dead code.

4. requireActorId uses an unsafe cast

connection as { actorId?: string } silently succeeds even if the field is absent at runtime. If the client exposes a typed accessor, use that instead; otherwise add a comment explaining why the cast is necessary.

5. Undocumented backward-compat alias

values["message-interval-ms"] ?? values["increment-interval"] accepts --increment-interval as a legacy alias but this does not appear in concurrentUsage(). Either add it to the help text or remove it to avoid confusion.


Dockerfile / Build Script

6. tsx runtime in Dockerfile.local

Using tsx for on-the-fly compilation in Docker is not production-ready. A short "staging only" comment would prevent this pattern from being copied into production configurations.

7. 7 GB heap default

ENV NODE_OPTIONS=--max-old-space-size=7168 is presumably tuned for the 8 GB Cloud Run instance. Without context it looks arbitrary and will OOM on smaller machines. A comment referencing the target instance size would help.

8. mktemp -d followed immediately by rm -rf

mktemp -d creates the directory, which is then immediately deleted and recreated with mkdir -p. A fixed path like /tmp/rivet-kitchen-sink-COMMIT_SHA would be simpler and equally correct.


Minor

  • logPrefix takes a _worker parameter that is unused in both code paths. Safe to drop.
  • ping-pong-counter.ts is imported and registered in this diff's src/index.ts but does not appear in the diff itself. Confirm it is intentionally in scope for this PR rather than a prior stack commit.
  • Root package.json counter-latency shortcut is a nice DX touch.

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