Feat: unified key-value store abstraction and adapters#165
Conversation
…m, Fastly, and Cloudflare
Resolves in and in
… and error, and update documentation
…TL validation for expiration tests.
… instead of an in-memory map
ChristianPavilonis
left a comment
ChristianPavilonis
left a comment
There was a problem hiding this comment.
There's some opportunity to cleanup 🧹
There was a problem hiding this comment.
PR Review
The architecture is solid: KvStore trait → KvHandle typed wrapper → Kv extractor → manifest config. The contract test macro is a strong addition. The layering follows the established adapter pattern correctly.
However, there are several issues to address before merge, ranging from a WASM compat convention violation to a security concern in the demo handler.
Summary
| Priority | Count | Categories |
|---|---|---|
| Must fix | 4 | WASM compat, unbounded body, feature gating, committed artifacts |
| Should fix | 6 | Silent failures, dead code, test gaps, API inconsistency |
| Nits | 4 | Dependencies, defensive validation, unnecessary imports |
See inline comments for details on each.
What's well-designed
- Object-safe trait with
#[async_trait(?Send)]+Send + Sync— correct for both WASM single-threaded and native multi-threaded contexts - Validation can't be bypassed —
KvHandle.storeis private, all public methods validate before delegating - Graceful KV injection — Fastly/Cloudflare silently skip KV if the store doesn't exist, so handlers that don't use KV are unaffected
PersistentKvStorewith redb — ACID-compliant, persistent across restarts, lazy TTL eviction. Right tradeoff for local dev- Contract test macro — ensures all backends conform to the same behavioral contract
- Manifest-driven config with per-adapter overrides — follows existing patterns
Issues not attached inline
Must fix: Committed artifacts that don't belong
final_review.md— review document from a prior session. ReferencesMemoryKvStorewhich no longer exists. Should not be checked in.examples/app-demo/node_modules/.mf/cf.json— Miniflare runtime state containing client TLS fingerprints, IP geolocation, and bot detection scores. Auto-generated, should be gitignored.
Should fix: Contract tests don't cover TTL expiration
- The
kv_contract_tests!macro tests put/get/delete/list/exists but not TTL. TTL is tested only in axum adapter tests. No shared contract verifying expired entries disappear across backends.
Should fix: env::set_var in demo tests is unsound on Rust 1.91
handlers.rstests useenv::set_var/env::remove_varforAPI_BASE_URLin tests that run in parallel. Since Rust 1.83, these are unsafe in multi-threaded contexts. Consider extracting the base URL as a function parameter or using#[serial].
Nit: Transitive serde_json in Fastly/Cloudflare adapters
- Neither adapter lists
serde_jsonexplicitly. Works today via transitive dep through core, but fragile if core ever feature-gates it.
…or improved clarity and consistency
ChristianPavilonis
left a comment
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Looks pretty good.
there's still one outdated comment unresolved
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
PR Review — feat/kv-store
CI Status: fmt ✓ clippy ✓ tests ✓
Overall
Solid foundation — clean trait design, good contract test macro, proper WASM compat (SystemTime, futures::executor, no Tokio in core). The TOCTOU fix in the axum adapter is correctly implemented with re-check inside write transactions. Module rename from kv to key_value_store is clean and complete.
5 issues need fixing before merge (P1), 4 are lower-priority improvements (P2), and 1 is a nit (P3). See inline comments for details.
Summary
| Severity | Count | Description |
|---|---|---|
| P1 | 5 | Serialization error mapping, missing nested validation, body size bypass, broken doc examples, infinite loop risk |
| P2 | 4 | Missing DerefMut, unused dep, fragile TempDir, contract test gap |
| P3 | 1 | Smoke test brittleness |
Positive highlights
- TOCTOU race condition properly fixed (re-check inside write txn)
tokiodev-dep removed from core (previous review finding addressed)- Contract test macro design is excellent — shared behavioral tests across all backends
- MockStore correctly uses SystemTime for WASM compatibility
- Validation suite is comprehensive with good edge-case coverage
redbpersistence with lazy TTL eviction is well-implemented
…manifest configuration validation
…ore contract test setup, and remove dependency
There was a problem hiding this comment.
PR Review
Summary
Introduces a well-architected provider-neutral KV store abstraction with implementations for Axum (redb), Fastly, and Cloudflare. The layering is clean and the test coverage is thorough (405 tests, all passing). Three blocking issues need attention before merge — primarily cross-adapter API consistency and HTTP status semantics.
😃 Praise
- The
KvStoretrait →KvHandlewrapper →Kvextractor layering is excellent. Validation is centralized inKvHandle, the contract test macro ensures backend consistency, and the adapter pattern is followed faithfully across all three platforms. - The ASCII architecture diagram in the
key_value_store.rsmodule doc is a great touch for onboarding. - Thorough edge-case testing: unicode keys, type-overwrite, overlapping prefixes, clone-shares-state, and the full validation suite.
Findings
Blocking
- 🔧 Cloudflare
run_appignores manifest KV config: cross-adapter inconsistency — Fastly'srun_appreads[stores.kv]from manifest, Cloudflare's does not (crates/edgezero-adapter-cloudflare/src/lib.rs:70) - 🔧 Axum dev server unconditionally creates KV store: introduces failure modes for non-KV apps (
crates/edgezero-adapter-axum/src/dev_server.rs:101) - 🔧
KvError::Unavailablemaps to 500 instead of 503: loses retry semantics for transient backend outages (crates/edgezero-core/src/key_value_store.rs:89)
Non-blocking
- 🤔 Per-request KV store open + warn-on-failure: log noise under load with misconfigured stores (
crates/edgezero-adapter-fastly/src/request.rs:64) - ♻️
collect_bodyhelper should live in core: prevents every downstream app from reimplementing body draining (examples/app-demo/crates/app-demo-core/src/handlers.rs:159) - 🤔
update()name implies atomicity: considerread_modify_write()to make non-atomicity obvious (crates/edgezero-core/src/key_value_store.rs:341) - 🌱 Contract test DB path uniqueness: use
tempfile::tempdir()instead of thread-id-based paths (crates/edgezero-adapter-axum/src/key_value_store.rs:522) - ⛏
kv_counteruses GET for mutation: should be POST per HTTP semantics (examples/app-demo/edgezero.toml:60) - 🌱 Duplicated
MockKvtest double:MockStorein core andMockKvin demo are near-identical — consider exporting a reusableMemoryKvStorebehind atest-utilsfeature - 📝 Contract test TTL expiry bypasses validation: intentional but undocumented — add a comment explaining the bypass (
crates/edgezero-core/src/key_value_store.rs:554)
📌 Should fix
- CI does not compile WASM target code paths — all
#[cfg(target_arch = "wasm32")]gated code (including Cloudflare/Fastly KV implementations) is invisible to CI. Track as a separate issue to add--target wasm32-unknown-unknownand--target wasm32-wasip1CI jobs. - Cloudflare
list_keyspagination accumulates all keys unbounded — could OOM on large namespaces. Worth adding a safety limit in a follow-up.
CI Status
- fmt: PASS
- clippy: PASS
- tests: PASS (405 passed, 0 failed)
…re adapter entry point
There was a problem hiding this comment.
Review: KV Store Feature
Good architecture overall — clean trait boundaries, comprehensive validation, solid test coverage (51+ core tests). The 3-tier design (trait → handle → extractor) is well-layered and the contract test macro is an excellent pattern.
However, there are cross-adapter behavioral divergences and late-failure paths that should be addressed before merge. See inline comments, organized by severity.
Summary
| # | Severity | Issue |
|---|---|---|
| 1 | High | Axum KV defaults diverge from manifest contract — requires [stores.kv] block that Fastly/Cloudflare don't need |
| 2 | Medium | Axum ignores configured KV store names/overrides — namespace parity lost |
| 3 | Medium | Missing KV bindings swallowed silently, surfaced as opaque 500s at request time |
| 4 | Medium | Hardcoded "25MB" in error message drifts from MAX_VALUE_SIZE constant |
| 5 | Medium | Missing test for MAX_TTL validation branch |
| 6 | Low | Unused serde_json dependencies added to adapter crates |
| 7 | Low | Docs reference update() but API is read_modify_write |
| 8 | Low | Smoke test counter assertion too weak — checks digit presence, not correctness |
- fail fast for explicitly configured axum KV stores - derive bounded stable local KV filenames from store names - cap Fastly/Cloudflare missing-binding warning caches - improve KV diagnostics, docs, and smoke assertions - compile Fastly/Cloudflare wasm targets in CI
ChristianPavilonis
left a comment
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Re-review complete: I don’t have any new findings beyond the feedback already captured in existing review threads.
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
Review Summary
Solid implementation — the core trait design, validation layer, contract test macro, and Axum adapter are all high quality. Two items I'd want addressed before merge, plus a handful of improvements.
Blockers (2)
- KV init semantics differ across adapters — Axum errors when
[stores.kv]is configured and init fails; Fastly/CF silently degrade. Same manifest, different behavior. - Validation contract between
KvHandleandKvStoreis implicit — adapters rely onKvHandlepre-validating inputs but the trait doesn't document this. Unchecked casts in adapters are safe today but fragile.
Improvements (7)
read_modify_writenon-atomicity needs a stronger callout in user-facing docs- Warning cache bug in both Fastly and CF adapters — 65th+ unique store name logs on every request instead of being suppressed
- Cloudflare
ttl.as_secs()truncation should note dependency onKvHandlevalidation - Cloudflare setup docs missing
wrangler kv namespace createcommands - Cloudflare
limit as u64— unchecked cast, inconsistent with Fastly'stry_from - TTL eviction semantics underdocumented per-adapter (Axum: lazy, grows unbounded)
run_appin CF adapter is the natural place to enforceKvInitRequirementonce blocker #1 is addressed
3 participants
Summary
This PR implements the full Key-Value store specification for EdgeZero, providing a portable abstraction for Axum, Fastly, and Cloudflare.
Implements
KvStoretrait andKvHandlewrapper inkv.rs(closes KV Store Abstraction #43, As a developer I want a portable KV store interface #44).ctx.kv_handle()andKvextractor (closes As a developer I want to access KV stores from request handlers #48).[stores.kv]section toedgezero.toml(closes KV store manifest bindings #49).kv_contract_tests!test suite ensuring behavioral consistency (closes KV store contract tests #50).PersistentKvStore(Axum) (closes Axum KV adapter (dev) #47 )FastlyKvStoreincrates/edgezero-adapter-fastly/src/kv.rs(closes Fastly KV adapter #45 )CloudflareKvStoreincrates/edgezero-adapter-cloudflare/src/kv.rs(closes Cloudflare KV adapter #46 )Integration
scripts/smoke_test_kv.shfor cross-platform verification.examples/app-demoto use the new KV features inexamples/app-demo/crates/app-demo-core/src/handlers.rs.