harden proto storage — recovery, atomicity, persistence - #90
Merged
Conversation
schemas were lost on restart because ProtoRegister records were skipped during AOF replay. now recovery collects them (last-wins dedup) and the shard restores them into the shared registry on startup. changes: - RecoveryResult gains a `schemas` vec (behind protobuf feature) - replay_aof collects ProtoRegister records into a schema map - spawn_shard/run_shard accept an optional SharedSchemaRegistry - after recovery, schemas are restored via registry.restore() - engine passes its schema_registry clone to each shard
the previous read-modify-write pattern for PROTO.SETFIELD and PROTO.DELFIELD spanned two shard roundtrips, leaving a window where another client could overwrite the key between the read and write. moving the mutation into the shard's single-threaded dispatch makes it inherently atomic. changes: - add ProtoSetField/ProtoDelField ShardRequest variants - add ProtoFieldUpdated ShardResponse variant - dispatch_proto_field_op helper reads, mutates, and writes back within a single dispatch call - to_aof_record maps field ops to full ProtoSet records - connection.rs and concurrent_handler.rs simplified to just route the request and match on the response
after a BGREWRITEAOF, the snapshot captures proto values but the AOF is truncated — losing any ProtoRegister records. on the next restart, recovery loads the values but has no schemas, breaking all field-level operations. now handle_rewrite writes ProtoRegister records for all registered schemas into the AOF immediately after truncation.
- log a warning when broadcasting ProtoRegisterAof fails instead of silently discarding the error with `let _ =` - add a startup warning when both concurrent mode and protobuf are enabled, since generic commands (DEL, TTL, EXPIRE) don't affect proto keys in concurrent mode
- add MAX_PROTO_VALUE_BYTES (64MB) check in validate() to prevent a single PROTO.SET from exhausting shard memory - add MAX_SCHEMAS (1024) check in register() to cap the number of registered schemas - fix proto memory overhead from 24 to 48 bytes to properly account for both String (24 bytes) and Bytes (24 bytes) structs
add unit tests for value size limit rejection, at-limit acceptance, and schema count cap. add integration tests for schema recovery after restart, schema survival through AOF rewrite, DEL on proto keys, and concurrent mode schema recovery. fix: sync AOF after re-persisting schemas during rewrite — without this, schema records could remain in the BufWriter buffer and be lost if the server is killed before the next fsync cycle.
kacy
added a commit
that referenced
this pull request
Feb 11, 2026
* thread schema registry into shards and recover schemas from AOF schemas were lost on restart because ProtoRegister records were skipped during AOF replay. now recovery collects them (last-wins dedup) and the shard restores them into the shared registry on startup. changes: - RecoveryResult gains a `schemas` vec (behind protobuf feature) - replay_aof collects ProtoRegister records into a schema map - spawn_shard/run_shard accept an optional SharedSchemaRegistry - after recovery, schemas are restored via registry.restore() - engine passes its schema_registry clone to each shard * atomic SETFIELD/DELFIELD via shard-local mutation the previous read-modify-write pattern for PROTO.SETFIELD and PROTO.DELFIELD spanned two shard roundtrips, leaving a window where another client could overwrite the key between the read and write. moving the mutation into the shard's single-threaded dispatch makes it inherently atomic. changes: - add ProtoSetField/ProtoDelField ShardRequest variants - add ProtoFieldUpdated ShardResponse variant - dispatch_proto_field_op helper reads, mutates, and writes back within a single dispatch call - to_aof_record maps field ops to full ProtoSet records - connection.rs and concurrent_handler.rs simplified to just route the request and match on the response * persist schemas through AOF rewrite after a BGREWRITEAOF, the snapshot captures proto values but the AOF is truncated — losing any ProtoRegister records. on the next restart, recovery loads the values but has no schemas, breaking all field-level operations. now handle_rewrite writes ProtoRegister records for all registered schemas into the AOF immediately after truncation. * broadcast error logging and concurrent mode warning - log a warning when broadcasting ProtoRegisterAof fails instead of silently discarding the error with `let _ =` - add a startup warning when both concurrent mode and protobuf are enabled, since generic commands (DEL, TTL, EXPIRE) don't affect proto keys in concurrent mode * proto value size limit, memory correction, schema count cap - add MAX_PROTO_VALUE_BYTES (64MB) check in validate() to prevent a single PROTO.SET from exhausting shard memory - add MAX_SCHEMAS (1024) check in register() to cap the number of registered schemas - fix proto memory overhead from 24 to 48 bytes to properly account for both String (24 bytes) and Bytes (24 bytes) structs * tests and sync fix for recovery, rewrite, and limits add unit tests for value size limit rejection, at-limit acceptance, and schema count cap. add integration tests for schema recovery after restart, schema survival through AOF rewrite, DEL on proto keys, and concurrent mode schema recovery. fix: sync AOF after re-persisting schemas during rewrite — without this, schema records could remain in the BufWriter buffer and be lost if the server is killed before the next fsync cycle.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
second phase of proto storage hardening, addressing 9 issues found in a deep audit:
ProtoRegisterrecords were skipped during AOF replay — schemas are now collected during recovery and restored into the shared registry on shard startuphandle_rewrite()truncated the AOF (which containedProtoRegisterrecords) without re-persisting schemas. schemas are now written after truncation and synced before the response is sentlet _ = engine.broadcast(...)in ProtoRegister now logs a warning on failure--concurrentand--protobufare set, since DEL/TTL/EXPIRE don't affect proto keys in concurrent modevalidate()What was tested
cargo test -p emberkv-core --features protobuf— 335 unit testscargo test -p ember-protocol— 294 testscargo test -p ember-integration-tests— 113 integration tests (44 proto-specific)cargo clippy --workspace --features protobuf -- -D warnings— cleancargo checkwithout protobuf feature — no regressionsnew tests added:
value_too_large_rejected,value_at_limit_allowed,schema_count_limitschema_recovery_after_restart,schema_survives_aof_rewrite,del_removes_proto_key,concurrent_schema_recovery_after_restartDesign considerations
restore()is idempotent, and it guarantees recovery works regardless of which shard's AOF is read firstwriter.sync()) ensures schema records are durable before the rewrite response is sent. without this, a kill between rewrite and next fsync cycle would lose schemasProtoFieldUpdatedwith the full serialized data, which maps cleanly toAofRecord::ProtoSetfor persistence — no new AOF record type needed