Skip to content

fix(pegboard): reject empty actor kv values#4808

Closed
NathanFlurry wants to merge 1 commit intoengine-stabilize/actor2-validation-lifecyclefrom
engine-stabilize/reject-empty-actor-kv-values
Closed

fix(pegboard): reject empty actor kv values#4808
NathanFlurry wants to merge 1 commit intoengine-stabilize/actor2-validation-lifecyclefrom
engine-stabilize/reject-empty-actor-kv-values

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

Copy link
Copy Markdown
Member Author

NathanFlurry commented Apr 27, 2026

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

This stack of pull requests is managed by Graphite. Learn more about stacking.

@claude
Copy link
Copy Markdown

claude Bot commented Apr 27, 2026

PR Review: fix(pegboard): reject empty actor kv values

Overview: This is a minimal one-line fix that adds an ensure! guard rejecting empty byte slices ([]) in validate_entries, preventing them from being written to actor KV storage.


Correctness

The fix is correct and closes a real gap. Without it:

  1. (0..value.len()).step_by(VALUE_CHUNK_SIZE) in the put path produces an empty iterator for a zero-length value, so zero chunk keys are written to FDB.
  2. The metadata key is still written (it's set unconditionally before the chunk loop).
  3. On read, EntryBuilder::build() already guards against this with ensure!(!self.value.is_empty(), "empty value at key"), so the read-back would silently drop the entry or error. The new guard catches the problem earlier at write time with a cleaner error message surfaced to the caller.

The existing entry.rs check ("empty value at key") becomes a defence-in-depth fallback for the read path; the new check is the canonical rejection point on write.


Minor suggestions

  • Error message style. The surrounding ensure! messages use sentence-case ("A maximum of 128 key-value entries is allowed"). Consider "value cannot be empty""value cannot be empty (use kv.delete to remove a key)" or at minimum keep consistent with the project's sentence capitalisation. This is cosmetic only.

  • Error code vs plain string. The KvStorageQuotaExceeded check above returns a typed errors::Actor::* error. If there is (or should be) a dedicated errors::Actor::KvEmptyValue variant, this would produce a machine-readable error code the client SDK can pattern-match on rather than a plain string. Not strictly required but worth considering for consistency.

  • Tests. The PR description acknowledges missing tests. A unit test in utils.rs (or the tests/ directory per project conventions) covering:

    • validate_entries with one empty value → expect Err
    • validate_entries with one non-empty value → expect Ok
    • would make this regression-proof without relying only on the existing entry.rs read-path guard.

Verdict

The change is correct and the logic is sound. The suggestions above are non-blocking. Good candidate to merge after the description checklist is filled in and, ideally, a small test is added.

@NathanFlurry NathanFlurry force-pushed the engine-stabilize/runner-config-fresh-peer-list branch from 6953d69 to 9c1aabc Compare April 27, 2026 07:30
@NathanFlurry NathanFlurry force-pushed the engine-stabilize/reject-empty-actor-kv-values branch from 5cc0e15 to 91328b0 Compare April 27, 2026 07:31
@NathanFlurry NathanFlurry changed the base branch from engine-stabilize/runner-config-fresh-peer-list to graphite-base/4808 April 27, 2026 08:31
@NathanFlurry NathanFlurry force-pushed the engine-stabilize/reject-empty-actor-kv-values branch from 91328b0 to ff28b88 Compare April 27, 2026 08:31
@NathanFlurry NathanFlurry changed the base branch from graphite-base/4808 to engine-stabilize/actor2-validation-lifecycle April 27, 2026 08:32
@NathanFlurry NathanFlurry force-pushed the engine-stabilize/actor2-validation-lifecycle branch from bea788e to ffd9202 Compare April 27, 2026 09:56
@NathanFlurry NathanFlurry force-pushed the engine-stabilize/reject-empty-actor-kv-values branch from ff28b88 to a35b876 Compare April 27, 2026 09:56
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