Skip to content

feat(envoy-protocol): add sqlite preload hints messages#4889

Draft
NathanFlurry wants to merge 1 commit intomock-agentic-loop/sqlite-vfs-runtime-helpersfrom
mock-agentic-loop/envoy-protocol-v5-preload-hints
Draft

feat(envoy-protocol): add sqlite preload hints messages#4889
NathanFlurry wants to merge 1 commit intomock-agentic-loop/sqlite-vfs-runtime-helpersfrom
mock-agentic-loop/envoy-protocol-v5-preload-hints

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 May 3, 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
Contributor

claude Bot commented May 3, 2026

Code Review: feat(envoy-protocol): add sqlite preload hints messages

Overview

This PR bumps the envoy-protocol from v4 to v5, adding SqlitePreloadHints wire messages that allow actors to persist SQLite page preload hints back to the engine. It is a stacked draft PR targeting mock-agentic-loop/sqlite-vfs-runtime-helpers, scoped to protocol definitions only.


Protocol Design

The new types are well-structured:

  • SqlitePreloadHints supports both sparse pgnos and ranges — good, as it handles both random-access hot pages and sequential read patterns efficiently.
  • SqlitePersistPreloadHintsResponse properly adds SqliteFenceMismatch as a distinct error variant (separate from SqliteErrorResponse), matching the generation-fencing semantics in SqlitePersistPreloadHintsRequest.
  • Backward compat conversions are correct: v4→v5 is a no-op (convert_same_bytes) for all unchanged types, and v5→v4 returns a ProtocolCompatibilityError if the new request/response variant is encountered. The ProtocolCompatibilityFeature::SqlitePreloadHints entry correctly uses the plural "require" verb form (consistent with existing entries).

Issues

Critical: convert_same_bytes violates engine CLAUDE.md

The engine CLAUDE.md (engine/CLAUDE.md, rule 6a) explicitly prohibits this pattern:

"Never rely on byte-identical wire layout across versions. Every cross-version converter must reconstruct the target type field-by-field, even when versions appear identical today. No serde_bare::to_vec + from_slice shortcuts."

The PR adds new uses of convert_same_bytes / convert_same_bytes_ref for ToEnvoyConn, ToGateway, ToOutbound, and ActorCommandKeyData v4↔v5 conversions. These types are byte-identical across v4 and v5, but the rule is categorical: every converter must be field-by-field.

Affected sites:

  • versioned.rsToEnvoyConn::deserialize_version case 4, ToEnvoyConn::serialize_version case 4
  • Same for ToGateway, ToOutbound, ActorCommandKeyData

Each should reconstruct field-by-field (or at minimum document explicitly why byte-identity is stable here, if there's an exception path that isn't clear from the CLAUDE.md).


Missing exhaustive match arm in pegboard-envoy

engine/packages/pegboard-envoy/src/ws_to_tunnel_task.rs exhaustively matches on protocol::ToRivet. With v5 exporting ToRivetSqlitePersistPreloadHintsRequest, the existing match will fail to compile until a handler arm is added. A stub that returns SqlitePersistPreloadHintsResponse::SqliteErrorResponse("not yet wired") (consistent with the existing exec/execute stubs at lines 403–435) would unblock compilation while the real handler lands.


SqliteFenceMismatch carries no actionable metadata

type SqliteFenceMismatch struct {
    reason: str
}

The engine knows both the expected and actual generation at the point it returns a fence mismatch. Including expectedGeneration: SqliteGeneration and actualGeneration: SqliteGeneration would let callers log a useful diagnostic instead of parsing a freeform string. Compare how SQLite VFS correctness issues are surfaced elsewhere — a structured mismatch is easier to act on.


Minor

  • PR description is empty — the checklist template is not filled in. Even for a draft, a one-line summary of what the PR does and what's being deferred to follow-up PRs would help reviewers.
  • No serialization round-trip test for the new types — the existing versioned.rs tests cover ToEnvoyCommands and ActorCommandKeyData. A test that round-trips ToRivetSqlitePersistPreloadHintsRequest across version boundaries (v4→v5 incompatibility path, v5→v5 identity) would catch any codegen issues early.
  • PROTOCOL_VERSION version test duplicationversioned.rs test and stateless_sqlite_v3.rs both assert PROTOCOL_VERSION == 5. That's fine, just noting it is intentional.

What's Good

  • Clean protocol layering: the new request/response pair follows the exact same requestId: u32 + data: T envelope as all other SQLite messages.
  • SqlitePreloadHintRange { startPgno, pageCount } is a compact, cache-friendly representation for sequential page access hints.
  • Version converter chain is mechanically correct — v1→v5 deserialization and v5→v1 serialization paths are all wired correctly with appropriate incompatibility gates.
  • TypeScript and Rust codegen are properly aligned (union tag positions match, VERSION = 5 updated).

Summary: The protocol schema and versioning are structurally sound. The two blocking items before merging are the convert_same_bytes policy violation and the missing exhaustive match arm in pegboard-envoy. The SqliteFenceMismatch metadata gap is worth fixing before this ships to production for debuggability.

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