Skip to content

fix(pegboard): skip protocol version keys in runner pool backfill#4620

Merged
MasterPtato merged 1 commit intomainfrom
04-12-fix_pegboard_skip_protocol_version_keys_in_runner_pool_backfill
Apr 13, 2026
Merged

fix(pegboard): skip protocol version keys in runner pool backfill#4620
MasterPtato merged 1 commit intomainfrom
04-12-fix_pegboard_skip_protocol_version_keys_in_runner_pool_backfill

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

claude bot commented Apr 13, 2026

PR Review: fix(pegboard): skip protocol version keys in runner pool backfill

Summary

This PR fixes a bug in runner_pool_backfill.rs where a range scan over the runner_config_subspace was inadvertently pulling in ProtocolVersionKey entries alongside the intended DataKey entries. Without the fix, the backfill would attempt to deserialize a ProtocolVersionKey value (a u16) as a RunnerConfig, causing the activity to fail.


Correctness

runner_config.rs - ProtocolVersionKey unpack validation

The new validation is correct. DataKey packs as a 5-element tuple (RUNNER, CONFIG, DATA, namespace_id, name) while ProtocolVersionKey packs as a 6-element tuple with the additional PROTOCOL_VERSION constant appended. Because of the length difference:

  • Attempting to unpack a DataKey entry as a ProtocolVersionKey will fail (only 5 elements, 6 expected) so is_ok() returns false and the entry is processed normally.
  • Attempting to unpack a ProtocolVersionKey entry will succeed after the new data != PROTOCOL_VERSION guard so is_ok() returns true and the entry is skipped.

No false positives or false negatives appear possible.

runner_pool_backfill.rs - skip ordering

The new_last_key is updated before the skip check (line 101 vs lines 103-108). This is correct. Pagination must advance past the current entry regardless of whether it is processed or skipped. If new_last_key were only set for non-skipped entries, a chunk full of protocol-version keys could loop infinitely.


Minor observations

Error message could include context (runner_config.rs line 274)

The current message is a static string. Including the actual vs expected value would make debugging easier if the constant ever changes.

Unrelated formatting changes in database.rs

The database.rs diff is purely cosmetic: reordered imports and collapsed multi-line expressions. Per CLAUDE.md, cargo fmt should not be run automatically, and mixing formatting-only changes into a bug-fix PR adds diff noise. Consider omitting these or landing them as a separate commit.


Overall

The fix is correct, well-scoped, and addresses the root cause. The two observations above are minor.

Generated with Claude Code (https://claude.com/claude-code)

@NathanFlurry NathanFlurry marked this pull request as ready for review April 13, 2026 05:00
@NathanFlurry NathanFlurry force-pushed the 04-12-fix_pegboard_skip_protocol_version_keys_in_runner_pool_backfill branch from 412eec8 to 0aff89a Compare April 13, 2026 05:38
@MasterPtato MasterPtato force-pushed the 04-12-fix_pegboard_create_missing_normal_runner_configs branch from 5c3d2c5 to a142b3e Compare April 13, 2026 19:35
@MasterPtato MasterPtato force-pushed the 04-12-fix_pegboard_skip_protocol_version_keys_in_runner_pool_backfill branch from 0aff89a to f99981c Compare April 13, 2026 19:35
@github-actions
Copy link
Copy Markdown
Contributor

Preview packages published to npm

Install with:

npm install rivetkit@pr-4620

All packages published as 0.0.0-pr.4620.0ecee0c with tag pr-4620.

Engine binary is shipped via @rivetkit/engine-cli on linux-x64-musl, linux-arm64-musl, darwin-x64, and darwin-arm64. Windows users should use the release installer or set RIVET_ENGINE_BINARY.

Docker images:

docker pull rivetdev/engine:slim-0ecee0c
docker pull rivetdev/engine:full-0ecee0c
Individual packages
npm install rivetkit@pr-4620
npm install @rivetkit/react@pr-4620
npm install @rivetkit/rivetkit-native@pr-4620
npm install @rivetkit/sqlite-wasm@pr-4620
npm install @rivetkit/workflow-engine@pr-4620

Copy link
Copy Markdown
Contributor

MasterPtato commented Apr 13, 2026

Merge activity

  • Apr 13, 8:40 PM UTC: A user started a stack merge that includes this pull request via Graphite.
  • Apr 13, 8:44 PM UTC: Graphite rebased this pull request as part of a merge.
  • Apr 13, 8:44 PM UTC: @MasterPtato merged this pull request with Graphite.

@MasterPtato MasterPtato changed the base branch from 04-12-fix_pegboard_create_missing_normal_runner_configs to graphite-base/4620 April 13, 2026 20:41
@MasterPtato MasterPtato changed the base branch from graphite-base/4620 to main April 13, 2026 20:42
@MasterPtato MasterPtato force-pushed the 04-12-fix_pegboard_skip_protocol_version_keys_in_runner_pool_backfill branch from f99981c to 2573bfc Compare April 13, 2026 20:43
@MasterPtato MasterPtato merged commit 49c97f4 into main Apr 13, 2026
18 of 26 checks passed
@MasterPtato MasterPtato deleted the 04-12-fix_pegboard_skip_protocol_version_keys_in_runner_pool_backfill branch April 13, 2026 20:44
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.

2 participants