Feat: unified key-value store abstraction and adapters#165
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
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
Staff-Engineer 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 |
|
|
||
| // Only initialize KV store if configured in the manifest. | ||
| let kv_enabled = manifest.manifest().stores.kv.is_some(); | ||
|
|
There was a problem hiding this comment.
High: Axum KV defaults diverge from manifest contract and other adapters.
run_app only enables KV when [stores.kv] is explicitly present:
let kv_enabled = manifest.manifest().stores.kv.is_some();But manifest.kv_store_name() (manifest.rs:127-141) defines a default store name (EDGEZERO_KV) even when [stores.kv] is omitted. Fastly and Cloudflare both call kv_store_name() unconditionally and attempt to open the store regardless of whether [stores.kv] exists.
Impact: Handlers using Kv extractor work on Fastly/Cloudflare with zero manifest config but fail on Axum with a 500 unless users add a no-op [stores.kv] block. This breaks the "write once, deploy anywhere" contract.
Suggested fix: Either:
- Have Axum also attempt KV init unconditionally (matching Fastly/Cloudflare), or
- Have Fastly/Cloudflare also gate on
stores.kv.is_some()(making the contract explicit)
Option 1 is simpler and consistent with the existing default-name behavior.
|
|
||
| let kv_path = if kv_enabled { | ||
| Some(".edgezero/kv.redb") | ||
| } else { |
There was a problem hiding this comment.
Medium: Axum ignores configured KV store names/overrides entirely.
This always uses ".edgezero/kv.redb" regardless of what's configured in [stores.kv] name = ... or [stores.kv.adapters.axum].
Fastly and Cloudflare both resolve the name via manifest.kv_store_name("fastly") / manifest.kv_store_name("cloudflare"), but Axum never calls kv_store_name("axum").
Impact: [stores.kv] name = ... and per-adapter overrides are no-ops for Axum, so namespace parity/isolation is lost. If a user runs two apps locally expecting separate KV namespaces, they silently share the same file.
Suggested fix: Derive the redb path from the resolved store name:
let store_name = manifest.manifest().kv_store_name("axum");
let kv_path = format!(".edgezero/{}.redb", store_name.to_ascii_lowercase());|
|
||
| // Inject KV handle if the store exists — graceful fallback. | ||
| match FastlyKvStore::open(kv_store_name) { | ||
| Ok(store) => { |
There was a problem hiding this comment.
Medium: Missing KV bindings are swallowed, producing late opaque 500s.
Both Fastly (here) and Cloudflare (request.rs:72) swallow store-open failures and continue without injecting a KvHandle. Then the Kv extractor (extractor.rs:425) turns the missing handle into a generic EdgeError::internal("no kv store configured").
Impact: Misconfiguration (wrong store name, missing binding, typo in edgezero.toml) is only detected at request time and surfaced as an opaque 500 rather than an explicit platform/config error. Users see "no kv store configured" with no hint about why.
Suggested improvements:
- Include the store/binding name in the warning:
"KV store '{}' not available: {}"so users can correlate with their config. - Consider making the
Kvextractor error message more actionable:"no kv store configured -- check [stores.kv] in edgezero.toml and platform bindings". - Long-term: validate bindings at startup in the CLI layer.
| fn validate_value(value: &[u8]) -> Result<(), KvError> { | ||
| if value.len() > Self::MAX_VALUE_SIZE { | ||
| return Err(KvError::Validation(format!( | ||
| "value size {} exceeds limit of 25MB", |
There was a problem hiding this comment.
Medium: Hardcoded "25MB" will drift if MAX_VALUE_SIZE changes.
"value size {} exceeds limit of 25MB", value.len()Every other validation error references the constant (e.g., MAX_KEY_SIZE, MAX_LIST_PAGE_SIZE), but this one hardcodes the human-readable limit.
Suggested fix:
format!("value size {} exceeds limit of {} bytes", value.len(), Self::MAX_VALUE_SIZE)| assert!(matches!(err, KvError::Validation(_))); | ||
| assert!(format!("{}", err).contains("at least 60 seconds")); | ||
| }); | ||
| } |
There was a problem hiding this comment.
Medium: Missing test for MAX_TTL validation branch.
There's validation_rejects_short_ttl but no corresponding test for the ttl > MAX_TTL branch at line 304. That code path has zero test coverage.
#[test]
fn validation_rejects_long_ttl() {
let h = handle();
futures::executor::block_on(async {
let err = h
.put_with_ttl("k", &"v", Duration::from_secs(366 * 24 * 60 * 60))
.await
.unwrap_err();
assert!(matches!(err, KvError::Validation(_)));
assert!(format!("{}", err).contains("exceeds maximum"));
});
}| "dep:reqwest", | ||
| "dep:redb", | ||
| "dep:serde_json", | ||
| ] |
There was a problem hiding this comment.
Low: serde_json is added to the axum feature gate but never used in adapter source code.
grep -r serde_json crates/edgezero-adapter-axum/src/ returns zero matches. Same for Fastly adapter (Cargo.toml line 37). This adds unnecessary build graph churn.
If these were added speculatively for future use, remove them and add back when needed.
|
|
||
| - A value written at one edge location may not be immediately visible at another. | ||
| - `update()` is **not atomic**. Concurrent updates to the same key may result in lost writes. | ||
| - **TTL**: `put_with_ttl` enforces a minimum of **60 seconds** and a maximum of **1 year** across all adapters. |
There was a problem hiding this comment.
Low: Docs drift from API naming.
This says:
update()is not atomic.
But the actual API method is read_modify_write(), not update(). The correct name is used elsewhere in the doc (line 76), making this inconsistent.
Fix:
- `read_modify_write()` is **not atomic**. Concurrent updates to the same key may result in lost writes.| BODY=$(curl -s -X POST "$BASE/kv/counter") | ||
| COUNT=$(echo "$BODY" | grep -o '"count":[0-9]*' | head -1 | cut -d: -f2) | ||
| check "Counter returns a number" "true" "$([ -n "$COUNT" ] && echo true || echo false)" | ||
|
|
There was a problem hiding this comment.
Low: Counter smoke test can produce false positives.
This only checks that a number exists in the response JSON -- not that it's correct or that incrementing works. A malformed response with any digit would pass.
Suggested fix -- validate the actual value:
COUNT=$(echo "$BODY" | grep -o '"count":[0-9]*' | head -1 | cut -d: -f2)
check "Counter increments" "true" "$([ "$COUNT" -ge 1 ] 2>/dev/null && echo true || echo false)"| ctx.kv_handle() | ||
| .map(Kv) | ||
| .ok_or_else(|| EdgeError::internal(anyhow::anyhow!("no kv store configured"))) | ||
| } |
There was a problem hiding this comment.
Nit (related to issue #3): This error message gives no hint about why the store is missing. Consider:
.ok_or_else(|| EdgeError::internal(anyhow::anyhow!(
"no kv store configured -- check [stores.kv] in edgezero.toml and platform bindings"
)))This helps users self-diagnose misconfiguration instead of guessing.
- 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
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.