feat(pegboard): actor startup KV preloading#4452
Conversation
…g skipped entries
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
🚅 Deployed to the rivet-pr-4452 environment in rivet-frontend
|
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
PR Review: feat(pegboard): actor startup KV preloadingThis is a well-architected optimization. The three-way preload return semantics (Uint8Array / null / undefined), budget tracking in a single FDB snapshot transaction, and WriteCollector batching are solid design choices. The adversarial review bugfixes (US-016 through US-020) show thorough self-review. Some issues to address: Bug: get-key budget check adds key to requested_get_keys before value-size checkFile: engine/packages/pegboard/src/actor_kv/preload.rs In the get-keys loop, a key is pushed to requested_get_keys unconditionally before checking whether the entry value fits within the budget. If a key exists in FDB but its value exceeds the remaining budget, the key lands in requested_get_keys but not in entries. On the TypeScript side, preload.get(KEY) returns null ("requested but not found"), causing the subsystem to treat it as absent instead of falling back to KV. For PERSIST_DATA specifically, this would start the actor with missing state. Fix: only add to requested_get_keys after confirming the entry fits budget. Keys absent from FDB (builder stays None) should still be recorded since "not found" is a valid preloaded answer: Minor: unwrap_or_default() silently uses empty actor nameFile: engine/packages/pegboard/src/workflows/actor/runtime.rs, insert_and_send_commands The .unwrap_or_default() call when extracting the actor name from commands silently falls back to an empty string. This is guarded by has_start_cmd above, so find_map should always succeed. An empty actor name produces wrong preload config with no error. Prefer an explicit error: Nit: expectNoKvRoundTrips only warns once per startupFile: rivetkit-typescript/packages/rivetkit/src/actor/instance/mod.ts The warnUnexpectedKvRoundTrip method resets the flag to false after the first warning, which silences all subsequent unexpected KV round-trips during startup. If multiple subsystems fall back to KV (e.g., after a preload regression), only the first will be visible in logs. Consider logging without resetting, or deduplicating by method name. Nit: .ralph/ planning artifacts in the repoFiles: .ralph/prd.json, .ralph/progress.txt, scripts/ralph/progress.txt These look like AI-assisted planning artifacts. Unless the team intentionally tracks them, they probably belong in .gitignore rather than committed to source. Minor: prefix_range trailing-zero assumptionFile: engine/packages/pegboard/src/actor_kv/preload.rs The prefix_range function silently no-ops if the trailing 0 byte is absent when popping from the packed key. A silent no-op produces an incorrect range. If the null terminator is guaranteed by tuple encoding, an assertion would catch regressions immediately: OverallThe implementation is solid. The critical item is the get-key budget / requested_get_keys tracking bug - the rest are minor. The version migration plumbing (v7 to v8 converters, backward-compat preloaded_kv: None for old runners) is correct, and the missed-commands reconnect path injecting fresh preloaded KV is an important correctness detail that's handled well. |
Code Review: feat(pegboard): actor startup KV preloadingThis is a substantial and well-implemented feature. The adversarial review pass (US-016–US-020) already caught and fixed several critical correctness issues. Here are remaining observations. Issues1. Fragile prefix range construction in The if let Some(&0) = start.last() {
start.pop();
}If the tuple encoding changes or an empty prefix is passed, the computed range is wrong with no error or assertion. Consider asserting the byte is present or returning a 2. Preloaded KV fetch can fail actor startup with no fallback
3. If a subsystem makes multiple unexpected KV reads, only the first is reported (the flag is cleared after the first hit). This makes the detection less useful when multiple regressions are introduced at once. Consider logging all occurrences during startup and clearing the flag only at the end of 4.
5. Missed-command preload in
6.
Positive observations
Minor
|
- Add unit tests for PreloadMap (29 tests) and WriteCollector (4 tests) - Use symbol for warnUnexpectedKvRoundTrip to keep it internal - Add sync comments between Rust preload.rs and TS keys.ts - Add CLAUDE.md rule about keeping prefix bytes in sync - Remove ralph artifacts and unrelated error json - Add .gitignore entries for ralph generated files Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Summary
CommandStartActorprotocol message (v8)PreloadMapinterface with binary search for efficient subsystem consumption,WriteCollectorfor batching new-actor initialization writes, and#expectNoKvRoundTripsdetection flagImplementation
Engine (Rust)
engine/packages/config/src/config/pegboard.rsPreloadedKvtype and v7/v8 convertersbatch_preload()function inactor_kv/preload.rswith single FDB snapshot transactionInsertAndSendCommandsactivity (not persisted in workflow history)prepopulateActorNamesmetadataTypeScript (RivetKit)
PreloadMapwith binary search (no hex string conversion)PreloadMapbefore issuing KV readsActorInstance.start()WriteCollectorbatches new-actor init writes into singlekvBatchPut#expectNoKvRoundTripsflag warns on unexpected KV reads during startupBugfixes (from adversarial review)
requested_prefixes/requested_get_keysincluding skipped entries (critical: prevented silent data loss for actors exceeding preload budgets)entry_sizeto include metadata in budget calculationclearPreload()on SQLite VFS store after migration (768 KB memory leak fix)actor_preload_configsfield from runner connPreloadMaptwice from same dataDocs
limits.mdxandconfiguration.mdxwith preload limitsTest plan
pnpm test driver-file-system)pnpm test driver-engine)cargo build -p pegboardsucceedscargo build -p rivet-runner-protocolsucceedscargo build -p rivet-pegboard-runnersucceeds🤖 Generated with Claude Code