Repository navigation
Add native celld config and KV store providers - #422
ChristianPavilonis wants to merge 14 commits into
Conversation
5426ea6 to
d475727
Compare
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
PR Review
Summary
This PR adds native celld config snapshots and live KV behind independent default-off features. The security and resource-bounding work is careful:
- Credentials are redacted everywhere.
- Redirects, proxies and retries are disabled.
- Body caps are enforced while streaming.
- Settings parsing is strict.
Changes are requested for one error-classification bug, two CI/test checks that can pass without proving anything, a test that exercises the wrong binary, and two open questions about the unreleased celld wire contract.
Findings
Blocking
- 🔧 An oversized KV value returns 503, not 400: the local 1 MiB cap maps to
LimitExceeded, which core turns intoservice_unavailable. Core and AWS returnValidationfor the same input (crates/edgezero-store-celld/src/kv.rs:88). - 🔧 The dependency contract check passes when
cargo treefails:set -edoes not apply inside command substitution (scripts/check_native_store_dependency_contract.sh:35,:41). - 🔧 The generated proof tests the wrong binary: the coexistence build overwrites the celld-only binary before the delete, malformed and missing phases (
crates/edgezero-cli/tests/celld_generated_proof.rs:550). - 🔧 The celld startup suites lack
run_test_nonzero.sh: the cfg-gated mixed-provider test could be compiled out with CI still green (.github/workflows/test.yml:206-207). - ❓ Unreleased celld dependency: the provider targets an unpublished celld revision, the real-service proof never runs in CI, and the guide ships publicly (
docs/guide/native-celld-stores.md:5). - ❓ Query-string key encoding: spaces are sent as
+, and the server's decoding is unverified (crates/edgezero-store-celld/src/kv.rs:54).
Non-blocking
- 🤔 Pool idle timeout: reqwest's 90 s default exceeds the server's 5 s header budget (
client.rs:90). - 🤔 Strict 204 Content-Type and 5xx classification (
client.rs:217). - 🤔 TLS trust is ambient and undocumented: there's no way to pin a CA, and a host without a CA bundle fails even for HTTP loopback (
client.rs:91). - 🤔 The final deadline check can turn a confirmed write into
Deadline(client.rs:237). - 🤔 The core KV contract suite isn't run against
CelldKvStore(kv.rs:191). - ♻️ Legacy AWS-only wrappers kept beside
*_with_policies(native_bindings.rs:221). - ⛏ Rename
capture_agent_tokens(run_options.rs:208). - ⛏ Process-label ignore reason and misleading module header (
celld_generated_proof.rs:501). - ⛏ Imports unused without the Axum feature (
config.rs:2255). - ⛏ The celld push refusal gives AWS-style advice (
config.rs:1667). - ⛏ Fixture threads have hard lifetimes (
native_celld_startup.rs:51,celld_generated_proof.rs:88). - ⛏ A double panic in fixture
Dropaborts the test binary (native_celld_startup.rs:137). - ⛏ Docs: the client "cache" is a hard limit, and authenticated mode requires HTTPS (
native-celld-stores.md:160). - ⛏ The TLS proof only works on Linux (
celld_generated_proof.rs:294). - 🌱 Key pages are fully deserialized before the limit check (
kv.rs:140). - 🌱 Plaintext token copy kept for cache identity (
preparation.rs:204). - 🌱 CI coverage gaps (no inline comment):
- CI lints celld only with
--all-features. Theconfig-only,kv-only,celld-config-only andcelld-kv-only builds, with theircfg_attr/expectarms, never run clippy with-D warnings. They pass locally today; a single-feature clippy step would keep it that way. - The dependency contract doesn't assert that
edgezero-store-celld, reqwest and tokio are absent from the wasm adapter graphs. It holds today only throughcfg(not(target_arch = "wasm32")).
- CI lints celld only with
📌 Out of Scope
connection::tests::pending_chunked_forward_preserves_original_deadlines_over_tcp(edgezero-adapter-axum) asserts a 5 ms timing tolerance. It fails about 2 runs in 12 even when run alone, on this PR's base (c66f93f1) as well as its head. It is not caused by this PR, but should be tracked separately.
CI Status
All runs below were on head f9b0f20c.
- fmt: PASS
- clippy (workspace,
--all-features,-D warnings): PASS - tests (
cargo test --workspace --all-targets): PASS, 2,879 passed, 0 failed, 6 ignored. Note: this gate runs only the 4settingstests ofedgezero-store-celld; celld coverage comes from the dedicatedtest.ymlstep. cargo check --workspace --all-targets --features "fastly cloudflare spin": PASS- Spin
wasm32-wasip2check: PASS - The PR's celld
test.ymlstep, run locally: PASS. Crate tests 4 / 27 / 30 / 38, axumnative-bindingslib 25,native_celld_startup4; the combined step's only failure was the pre-existing flaky test above. - Single-feature clippy for
config,kv,celld-configandcelld-kv, run locally: PASS
| if u64::try_from(value.len()).map_err(|_error| PreparationError::SizeLimit)? | ||
| > self.limits.max_value_bytes | ||
| { | ||
| return Err(PreparationError::SizeLimit); |
There was a problem hiding this comment.
🔧 An oversized value is reported as 503, not a validation error
Core accepts values up to KvHandle::MAX_VALUE_SIZE (25 MiB), so a 2 MiB put_bytes gets past core and then trips this 1 MiB max_value_bytes cap. SizeLimit becomes KvError::LimitExceeded (line 318), and core maps that to EdgeError::service_unavailable (key_value_store.rs:865). So a caller gets a retryable 503 for input that can never succeed.
Core's own value cap (validate_value) and the AWS sibling (dynamodb_kv.rs:252, invalid_input()) both return KvError::Validation (400) for the same input.
Fix: treat the local pre-send cap as invalid input. Keep SizeLimit for server 413s and oversized pages.
if u64::try_from(value.len()).map_err(|_error| PreparationError::InvalidResource)?
> self.limits.max_value_bytes
{
return Err(PreparationError::InvalidResource);
}A test that a 1 MiB + 1 byte value through KvHandle gives KvError::Validation would pin this.
| # Celld settings are present in bindings-only builds, but live transport is not. | ||
| celld_features() { | ||
| local graph | ||
| graph="$(cargo tree --offline --locked --edges normal --prefix none --format '{p}|{f}' "$@")" |
There was a problem hiding this comment.
🔧 This check passes when cargo tree fails
celld_features only runs inside selected="$(celld_features "$@")" (line 41). Bash turns off set -e inside command substitution unless inherit_errexit is set (it isn't, on either Linux or macOS bash). So when cargo tree fails, awk reads empty input and exits 0. selected is empty, and assert_celld_off reports "no leak".
I reproduced it with the same pattern on a bogus package: selected=[] rc=0, and the script carries on. Lines 47 and 48 are partly covered because assert_no_sdk runs the same args and fails closed. Line 49 (aws-stores,aws-dynamodb) has no such guard, so a renamed feature would pass silently.
Fix:
celld_features() {
local graph
graph="$(cargo tree --offline --locked --edges normal --prefix none --format '{p}|{f}' "$@")" || return 1
awk -F'|' '/^edgezero-store-celld v/ { sub(/ [(][*][)]$/, "", $2); print $2; exit }' <<< "$graph"
}
assert_celld_off() {
local selected
selected="$(celld_features "$@")" || exit 1
...The selected=... at line 63 needs the same || exit 1.
| "restart adoption and persistence: {restarted_response}" | ||
| ); | ||
| drop(second); | ||
| let all_binary = build( |
There was a problem hiding this comment.
🔧 The later phases run the AWS+celld binary, not the celld-only one
build() always returns the same path, target.join("debug/celld-proof-adapter-axum") (line 497). This build with celld-config,celld-kv,aws-stores,aws-dynamodb overwrites the file that binary (line 517) points to. Three later phases still run App::start(&binary, …):
deleting(line 562)- the malformed-config loop (line 571)
missing(line 579)
They all execute the AWS+celld build. The assertions probably still pass, but the test no longer proves what it says about the celld-only binary.
Fix: move the coexistence block to the end of the test, or copy each build to a path named after its feature set:
let built = target.join("debug/celld-proof-adapter-axum");
let named = target.join(format!("debug/celld-proof-{}", features.unwrap_or("none").replace(',', "+")));
fs::copy(&built, &named).expect("feature-specific binary");
named| cargo test --offline --locked -p edgezero-adapter-axum --features celld-config,celld-kv,test-utils --test native_celld_startup | ||
| cargo test --offline --locked -p edgezero-adapter-axum --features aws-stores,aws-dynamodb,celld-config,celld-kv,test-utils --lib --test native_aws_startup --test native_celld_startup |
There was a problem hiding this comment.
🔧 The new celld startup suites aren't protected against "0 passed"
The AWS step above (line 196) wraps its suite in scripts/run_test_nonzero.sh; these two lines don't. The mixed test production_runner_composes_celld_config_aws_secret_and_local_secret (native_aws_startup.rs:498) is behind #[cfg(feature = "celld-config")] and runs only from line 207. A typo in that cfg or in a feature name would compile it out, and the step would stay green. The plan lists this wrapper as an open item.
Fix:
scripts/run_test_nonzero.sh public_runner_gates_preparation_and_initializer_then_keeps_snapshot_and_live_kv cargo test --offline --locked -p edgezero-adapter-axum --features celld-config,celld-kv,test-utils --test native_celld_startup
scripts/run_test_nonzero.sh production_runner_composes_celld_config_aws_secret_and_local_secret cargo test --offline --locked -p edgezero-adapter-axum --features aws-stores,aws-dynamodb,celld-config,celld-kv,test-utils --lib --test native_aws_startup --test native_celld_startupThere was a problem hiding this comment.
🔧 The same gap on the five --lib steps just above (lines 201-205)
The celld lib runs (no features, config, kv, all features) and the axum native-bindings --lib run aren't wrapped in run_test_nonzero.sh either. The celld test modules are behind feature conditions (client.rs:331 #[cfg(any(feature = "config", feature = "kv"))], config.rs:52, kv.rs:334). An accidental change to one of those conditions would quietly reduce the config-only or kv-only run to the 4 settings tests, and the step would still exit 0.
Fix: wrap each line with run_test_nonzero.sh and a test name that must run for that feature mode.
|
|
||
| The native provider reads complete configuration documents at startup and exposes live KV operations through a scoped listener **inside celld**. There is no gateway, sidecar, Worker bridge or celld HTTP adapter. | ||
|
|
||
| This is an unreleased integration. It requires the modified celld revision `f72ae21a515859570622ac9d6ca9c19cf6870b81`. Upstream celld v0.6.2 does not have this API. Local tests do not establish fleet failover, deployment security or production readiness. |
There was a problem hiding this comment.
❓ Should this merge while it depends on an unreleased, unpublished celld API?
This provider targets a modified celld revision (f72ae21a…). The PR body says the companion branch "remains local and has not been published", and upstream v0.6.2 doesn't have this API. That means:
- The wire contract exists only in this repo's spec.
- The real-service proof (TLS, scoped isolation, the 61 s TTL, restart persistence) runs only on the author's machine. CI runs only the mocked
run_native_celld_integration.test.mjs.check_celld_scoped_isolation.shis never executed in CI, and its heredoc scripts are never linted. - Even so, this guide ships in the public docs navigation.
What's the plan here? Should the merge wait for a published or pinned celld artifact plus a CI job that runs the real-service proof? Or should the provider be clearly marked experimental and kept out of the docs nav until then? Either is workable, but it should be decided before this lands.
| self.stop.store(true, Ordering::Relaxed); | ||
| self.gate.store(true, Ordering::Relaxed); | ||
| if let Some(worker) = self.worker.take() { | ||
| worker.join().expect("fixture thread"); |
There was a problem hiding this comment.
⛏ If the worker thread panicked (for example on one of its assert!s), this expect panics again inside Drop while the test is already unwinding. That aborts the whole test binary and loses the other tests' results.
if let Some(worker) = self.worker.take() {
let joined = worker.join();
if !thread::panicking() {
joined.expect("fixture thread");
}
}| | Store alias | At most 64 ASCII letters, digits, underscores or hyphens | | ||
| | Preparation / operation / connect timeout | 30 / 5 / 1 seconds | | ||
|
|
||
| `[limits.celld]` or `with_celld_preparation_limits` selects one celld policy. File and explicit policies do not merge. AWS has an independent policy and accounting. Client caches hold at most 128 authorization-scoped entries. Accounting describes logical payload and metadata quotas, not process RSS. |
There was a problem hiding this comment.
⛏ Two doc corrections
- "Client caches hold at most 128 authorization-scoped entries" describes a hard limit, not a cache.
preparation.rs:214returnsSizeLimitfor the 129th distinct client, which fails startup; nothing is evicted. Suggested wording: "Startup fails if more than 128 distinct endpoint/credential clients are needed." - The guide never says that
mode = "authenticated"requireshttpseven on loopback (settings.rs:294). One sentence in the access section would save a confusinginvalid endpoint.
| if let Some((token, ca)) = tls { | ||
| command | ||
| .env("CELLD_APP_TOKEN", token) | ||
| .env("SSL_CERT_FILE", ca); |
There was a problem hiding this comment.
⛏ This TLS proof relies on SSL_CERT_FILE, which the production client's platform verifier honours only on Linux (through rustls-native-certs). On macOS it uses Security.framework and ignores the variable, so on a Mac the test fails at the TLS handshake. The file is gated #[cfg(unix)]. Gate this test to target_os = "linux", or document that limitation next to CELLD_BIN.
| .ok_or(PreparationError::Malformed)?; | ||
| let result = (|| { | ||
| let page: WirePage = | ||
| serde_json::from_slice(&body).map_err(|_error| PreparationError::Malformed)?; |
There was a problem hiding this comment.
🌱 serde_json::from_slice::<WirePage> builds the whole Vec<String> before the keys.len() > limit check. A buggy or hostile server can fill a 2 MiB page with empty strings ("", repeated), about 700k entries or roughly 17 MiB of transient allocation per list call. That's bounded, but about 8x the page cap. A bounded sequence visitor that fails at limit + 1 would keep it at the page cap.
| let identity = ClientIdentity { | ||
| endpoint: endpoint.as_str().to_owned(), | ||
| access: access.clone(), | ||
| token: token.map(|credential| credential.expose().to_owned()), |
There was a problem hiding this comment.
🌱 ClientIdentity.token keeps a plaintext copy of the bearer token for the whole preparation session, and it's used only for cache equality. Comparing a digest of the token (or the sensitive HeaderValue already built in the client) would remove the copy. It never leaks through Debug or errors, so this is hardening only. Zeroize-on-drop for CelldAccessToken could come later.
dhruv8sh
left a comment
dhruv8sh
left a comment
There was a problem hiding this comment.
Reviewed against the PR's real base (issue-410-native-store-providers): 2 commits, 43 files. Adds the edgezero-store-celld crate (config snapshot + live KV providers), wires it into Axum native bindings/preparation sessions, and makes config push refuse celld-bound stores.
Local gates (Rust 1.98.1): fmt, clippy (-D warnings) and cargo test --workspace --all-targets (2880 passed, 0 failed) all pass.
No blocking findings. Additional notes not tied to a diff line:
- 🤔 Depends on an unreleased celld fork (
docs/guide/native-celld-stores.md:5): the provider targets a wire API that only exists in modified celld revisionf72ae21a, not upstream v0.6.2. Please track upstreaming the API as a separate issue, or keep the feature clearly marked experimental until it lands. - 📝
tokio::testinstead ofblock_onincrates/edgezero-store-celld/src/{client,config,kv,preparation}.rs: acceptable here since the transport is native-only (cfg'd off on wasm32) and uses reqwest/tokio, matching the AWS store crate. No action needed.
| #[cfg(feature = "kv")] | ||
| if missing && matches!(kind, ResponseKind::Value) { | ||
| return Ok(None); | ||
| } |
There was a problem hiding this comment.
🤔 Missing store looks like a missing key. Any 404 on a value GET returns Ok(None) (and HEAD 404 at L193-203 makes exists() false). If a store alias is misconfigured or unknown and the service answers 404 rather than 403, the app sees "no such key" instead of an error, hiding deployment mistakes in live KV. Can the service ever return 404 for an unknown alias? If so, a distinct error category for store-not-found would help; otherwise documenting that an unknown alias returns 403 would be enough.
| fn kv_error(error: PreparationError) -> KvError { | ||
| match error { | ||
| PreparationError::InvalidBinding | ||
| | PreparationError::InvalidEndpoint | ||
| | PreparationError::InvalidLimits | ||
| | PreparationError::InvalidResource => KvError::Validation(error.to_string()), | ||
| PreparationError::SizeLimit => KvError::LimitExceeded { | ||
| message: "celld size limit".to_owned(), | ||
| }, | ||
| // Core has no typed auth/protocol/deadline variants. Do not smuggle | ||
| // remote JSON or transport source chains into its generic error variants. | ||
| PreparationError::Credentials | ||
| | PreparationError::Unauthenticated | ||
| | PreparationError::Forbidden | ||
| | PreparationError::Missing | ||
| | PreparationError::Malformed | ||
| | PreparationError::UnsupportedVersion | ||
| | PreparationError::Unavailable | ||
| | PreparationError::Deadline => KvError::Unavailable, | ||
| } |
There was a problem hiding this comment.
🤔 Deadlines reported as Unavailable. Deadline, Unauthenticated, Forbidden and Malformed all collapse to KvError::Unavailable, so callers can't tell an auth failure from a timeout. Understood that core's KvError has no matching variants today.
🌱 Consider adding typed Forbidden/Deadline variants to KvError in a follow-up.
| } | ||
| } | ||
| for (name, (aws, celld)) in names { | ||
| let subject = if celld { "celld token" } else { "Agent token" }; |
There was a problem hiding this comment.
⛏ When one env var is used by both AWS and celld bindings, the duplicate/absent failures are labelled "celld token", while type-validation failures use the token type that failed. Optional: keep if intended, or use one neutral subject such as "store token".
| let mut builder = Client::builder() | ||
| .no_proxy() | ||
| .redirect(Policy::none()) | ||
| .retry(never()) | ||
| .no_gzip() | ||
| .no_brotli() | ||
| .no_deflate() | ||
| .no_zstd() | ||
| .connection_verbose(false) | ||
| .http1_only() | ||
| .connect_timeout(Duration::from_millis(limits.connect_timeout_ms)) | ||
| .timeout(Duration::from_millis(limits.operation_timeout_ms)); | ||
| if let Some(certificate) = root { | ||
| builder = builder.tls_certs_only([certificate]); | ||
| } | ||
| let authorization = token | ||
| .map(|credential| { | ||
| let mut header = HeaderValue::from_str(&format!("Bearer {}", credential.expose())) | ||
| .map_err(|_error| PreparationError::Credentials)?; | ||
| header.set_sensitive(true); | ||
| Ok(header) | ||
| }) | ||
| .transpose()?; | ||
| Ok(Self { | ||
| authorization, | ||
| client: builder | ||
| .build() | ||
| .map_err(|_error| PreparationError::Unavailable)?, | ||
| clock, | ||
| endpoint, | ||
| limits, | ||
| }) | ||
| } |
There was a problem hiding this comment.
😃 Strong transport hardening: no proxy, redirects, retries or decompression; HTTP/1 only; sensitive auth header; exactly-one version-header and content-type checks (L270-296); capped streaming bodies; redacted Debug. The ambient-proxy child-process test and TLS-trust/no-plaintext-fallback matrix prove it nicely.
| #[cfg(not(any(feature = "celld-config", feature = "celld-kv")))] | ||
| pub(crate) struct NativeCelldSession; | ||
|
|
||
| pub(crate) struct NativePreparationSessions { |
There was a problem hiding this comment.
😃 Clean provider dispatch: capability checks go by actual binding variant, each provider gets its own limits, file-vs-explicit limit conflicts are rejected, and all 16 capability combinations are tested.
| async fn page( | ||
| &self, | ||
| prefix: &str, | ||
| cursor: Option<&str>, | ||
| limit: usize, | ||
| ) -> Result<KvPage, PreparationError> { | ||
| let deadline = self.client.deadline()?; | ||
| if !valid_key(prefix, true) || !(1..=1_000).contains(&limit) { | ||
| return Err(PreparationError::InvalidResource); | ||
| } | ||
| let fingerprint = self.fingerprint(prefix)?; | ||
| let prior = cursor | ||
| .map(|encoded| decode_cursor(encoded, &fingerprint, prefix)) | ||
| .transpose()?; | ||
| let mut url = self.client.url(&self.store, "keys")?; | ||
| { | ||
| let mut query = url.query_pairs_mut(); | ||
| query | ||
| .append_pair("prefix", prefix) | ||
| .append_pair("limit", &limit.to_string()); | ||
| if let Some(previous) = &prior { | ||
| query.append_pair("cursor", &previous.service); | ||
| } | ||
| } | ||
| let cap = usize::try_from(self.limits.max_page_bytes) | ||
| .map_err(|_error| PreparationError::InvalidLimits)?; | ||
| let body = self | ||
| .client | ||
| .execute(Method::GET, url, None, cap, deadline, ResponseKind::Page) | ||
| .await? | ||
| .ok_or(PreparationError::Malformed)?; | ||
| let result = (|| { | ||
| let page: WirePage = | ||
| serde_json::from_slice(&body).map_err(|_error| PreparationError::Malformed)?; | ||
| if page.keys.len() > limit { | ||
| return Err(PreparationError::SizeLimit); | ||
| } | ||
| let mut last = prior.as_ref().and_then(|previous| previous.last.as_deref()); | ||
| for key in &page.keys { | ||
| if !valid_key(key, false) | ||
| || !key.starts_with(prefix) | ||
| || last.is_some_and(|previous_key| key.as_str() <= previous_key) | ||
| { | ||
| return Err(PreparationError::Malformed); | ||
| } | ||
| last = Some(key); | ||
| } | ||
| let next = if let Some(service) = page.cursor { | ||
| if service.is_empty() | ||
| || service.len() > MAX_SERVICE_CURSOR | ||
| || prior | ||
| .as_ref() | ||
| .is_some_and(|previous| service == previous.service) | ||
| { | ||
| return Err(PreparationError::Malformed); | ||
| } | ||
| let envelope = Cursor { | ||
| version: 1, | ||
| fingerprint, | ||
| last: last.map(str::to_owned), | ||
| service, | ||
| }; | ||
| let bytes = | ||
| serde_json::to_vec(&envelope).map_err(|_error| PreparationError::Malformed)?; | ||
| let encoded = URL_SAFE_NO_PAD.encode(bytes); | ||
| if encoded.len() > MAX_PROVIDER_CURSOR { | ||
| return Err(PreparationError::SizeLimit); | ||
| } | ||
| Some(encoded) | ||
| } else { | ||
| None | ||
| }; | ||
| Ok(KvPage { | ||
| keys: page.keys, | ||
| cursor: next, | ||
| }) | ||
| })(); | ||
| // Includes parsing/validation time and gives deadline precedence. | ||
| self.client.check(deadline)?; | ||
| result | ||
| } |
There was a problem hiding this comment.
😃 Solid cursor/page validation: fingerprint-bound cursors, strictly ascending keys across pages, non-progressing service cursors rejected (L278-299), and TTL range checked before rounding.
prk-Jr
left a comment
prk-Jr
left a comment
There was a problem hiding this comment.
PR Review
Summary
Reviewed head bb56cfe1 against the PR base (issue-410-native-store-providers @ 3733482f). The commits since the previous reviews (on f9b0f20c) only merge base-stack changes and touch none of this PR's 43 files, so every earlier finding still applies unchanged. The blocking items from @aram356's review are still open and have no author replies. That is the reason for requesting changes here; this review adds no new blocking defect of its own.
😃 Praise
- Endpoint parsing is strict about origins: it rejects
127.1, decimal-integer IPs,localhost, trailing dots, credentials in the URL and path segments, and it requires HTTPS for authenticated access (settings.rs:231).
Findings
Blocking
- 🔧 More unguarded celld test steps (
test.yml:201-205): the samerun_test_nonzero.shgap as the existing startup-suite comment, on the five--libsteps. Posted as a reply on that thread.
Non-blocking
- ⛏ An empty
[limits]table is now silently accepted (native_bindings.rs:147) - 🤔 Two isolation probes can never fail (
check_celld_scoped_isolation.sh:95,:103) - ⛏ The timeout test doesn't prove the process group is killed (
run_native_celld_integration.test.mjs:101) - ⛏ Guide: setting limits in both places is a startup error (
native-celld-stores.md:160) - ⛏ Plan doc: wrong lockfile path and personal home-directory paths (plan
:493,:34)
CI Status (head bb56cfe1)
- Hosted: everything passes except
cache-new-revision-smokeandcache-restore-smoke(Deploy actions). Both fail on a cache miss (TARGET_CACHE_HIT: false). This PR doesn't touch that workflow, and the same jobs passed on #414 in the same batch, so this looks like a cache/infrastructure problem rather than this PR. - Local:
- axum lib tests across two feature sets pass.
- Node runner tests pass (9/9).
- Single-feature celld clippy with
-D warningson 1.98.1 is clean. --lockedmetadata passes for all three lockfiles.- The guide's TOML examples pass
config validateas documented.
- Not run locally: the real-celld TLS proof and the namespace isolation script (they need the modified celld binary and Linux).
| #[serde(deny_unknown_fields)] | ||
| struct LimitsTable { | ||
| aws: AwsPreparationLimits, | ||
| aws: Option<AwsPreparationLimits>, |
There was a problem hiding this comment.
⛏ An empty [limits] table is now silently accepted
On the base branch LimitsTable.aws was required. A bare [limits] header with no [limits.aws] was rejected as malformed, and a [limits] table combined with an explicit policy was rejected as Duplicate (self.limits.is_some()).
With both fields now Option, an empty [limits] (for example a typo, or keys placed under the wrong section) parses as LimitsTable { aws: None, celld: None }. Both providers then quietly use their default policies. The duplicate check (:351-357) now looks at each provider separately, so an empty table plus an explicit policy is also accepted.
Nothing becomes unsafe, but an operator mistake is ignored instead of reported.
Fix: in validate_with_policies, reject Some(LimitsTable { aws: None, celld: None }) as malformed.
| [[ $(request -X PUT --data-binary forbidden "$base/v1/stores/config/value?key=probe") == 403 ]] | ||
| [[ $(request "$base/v1/stores/undeclared/value?key=probe") == 403 ]] | ||
| [[ $(request "$base/shutdown") == 404 ]] | ||
| if curl --noproxy '*' --max-time 1 -sf "http://10.203.0.1:$internal/state" >/dev/null; then |
There was a problem hiding this comment.
🤔 This probe can't fail, whatever the isolation
celld starts its internal listener on --internal-listen 127.0.0.1:0 (line 38), and the port is parsed from celld internal listening on 127.0.0.1:... (line 53). Nothing ever listens on 10.203.0.1:$internal, so this curl fails whether or not the namespaces isolate anything. Only the loopback probe at line 98 and the private PID namespace actually test isolation.
The procfs loop (lines 103-107) has a similar problem: it matches only comm == celld, so a CELLD_BIN with any other basename (for example celld-scoped) passes automatically.
CI doesn't run this script, so this is non-blocking. The plan and guide do cite its result as isolation evidence, though.
Suggestion: add a control step that shows the probed address is reachable from a non-isolated namespace (or bind the internal listener to the veth address for that check), and match the server by PID or executable path rather than by name.
| "const fs=require('node:fs');", | ||
| "const child=spawn(process.execPath,['-e','setInterval(()=>{},1000)'],{stdio:'ignore'});", | ||
| "fs.writeFileSync(process.argv[1],String(child.pid));", | ||
| "process.on('SIGTERM',()=>{child.once('exit',()=>process.exit(0));child.kill('SIGTERM');});", |
There was a problem hiding this comment.
⛏ This test passes even if the runner signals only the direct child
The runner's guarantee is that it kills the whole process group (process.kill(-child.pid, signal)), so no descendant survives. Here, though, the fixture parent handles SIGTERM by forwarding it to its own child and waiting for it to exit. If kill regressed to child.kill(signal), the grandchild would still be reaped and the test would still pass.
Fix: have the fixture parent exit on SIGTERM without forwarding the signal (process.on('SIGTERM',()=>process.exit(0))). Then only the group kill can reap the grandchild.
| | Store alias | At most 64 ASCII letters, digits, underscores or hyphens | | ||
| | Preparation / operation / connect timeout | 30 / 5 / 1 seconds | | ||
|
|
||
| `[limits.celld]` or `with_celld_preparation_limits` selects one celld policy. File and explicit policies do not merge. AWS has an independent policy and accounting. Client caches hold at most 128 authorization-scoped entries. Accounting describes logical payload and metadata quotas, not process RSS. |
There was a problem hiding this comment.
⛏ "File and explicit policies do not merge" reads as though one of them simply wins. In fact, supplying both [limits.celld] and with_celld_preparation_limits fails startup with Duplicate (native_bindings.rs:351-357), and so does a second explicit call (run_options.rs:568-570). Please say that setting both is a startup error.
The guide also never names the [limits.celld] keys (max_value_bytes, operation_timeout_ms, ...). Listing them next to the limits table would help.
|
|
||
| The user subsequently authorized committing the EdgeZero changes and opening a draft PR. The PR is stacked on #414, targeting `issue-410-native-store-providers`, not main. The companion celld branch remains local and is a separate publication dependency. | ||
|
|
||
| Pre-publication locked metadata checks found stale `examples/app-demo/Cargo.lock` and `tests/fixtures/reusable-app/Cargo.lock`. Both were refreshed offline with only the new settings-only provider package and adapter edge added. No existing registry versions changed. Both locked graphs then passed, and `cargo check --offline --locked --manifest-path tests/fixtures/reusable-app/Cargo.toml -p fixture-axum` passed in 2.78 seconds using the shared native target directory. Full demo workspace tests and all three provider WASM target builds remain unexecuted for this publication step. These consumer lockfiles are outside the source fingerprint above. |
There was a problem hiding this comment.
⛏ This cites tests/fixtures/reusable-app/Cargo.lock and --manifest-path tests/fixtures/reusable-app/Cargo.toml. The real location is crates/edgezero-test-harness/fixtures/reusable-app/, so the recorded command can't be rerun as written.
Separately, lines 34, 105, 391 and 395 embed personal /home/pav/worktrees/... paths, which are worth dropping from a shared doc.
| } | ||
| } | ||
|
|
||
| pub(crate) fn endpoint(value: &str, access: &CelldAccess) -> Result<Url, PreparationError> { |
There was a problem hiding this comment.
😃 Strict origin parsing. Comparing the raw host with the parsed IP rejects 127.1 and decimal-integer forms. It also rejects localhost, trailing dots, userinfo and path segments, and it refuses plaintext HTTP for authenticated access. That closes off a whole class of SSRF and confusion tricks before any network I/O.
Current review readiness
Ready for review. Required current-head CI passed at
f9b0f20c6857d2e638bcfe7bf1edaa5af5425bfb: test run 42924132227.Minimal fixes originate in #414 and are merged upward:
Deadline; non-timeouts remainUnavailable. The previously failing hosted timeout test and all 78 provider cases now pass without assertion, retry or duration changes.aws-types =1.6.0andaws-smithy-runtime-api =1.19.0. This prevents the schema mismatch and preserves the explicit older behavior version instead of switching retry policy or suppressing deprecations. A fresh consumer passes with compiler and Clippy warnings denied throughout.Local final-top Axum checks pass 488 unit, 28 contract, six AWS startup, four celld startup and 36 hosting cases. Final-top workspace strict Clippy and formatting pass. The prior stable checkpoint passed all six mandatory commands; unchanged inputs retain their receipts, while fresh current-head CI checks the combined state. The current ready state supersedes historical draft-only prerequisites below; unexecuted deployment acceptance is not claimed complete. No TS work or deployment qualification is a review prerequisite. Integration #462 remains draft; reviewers
aram356,prk-Jranddhruv8shremain requested. Stack465 and PR bases are unchanged; #453 is excluded.Current review-stack checkpoint
Publication head:
f9b0f20c6857d2e638bcfe7bf1edaa5af5425bfb. This cleanup preserves the existing feature unit and PR identity. The additional change is the completed generated-native build correction: image-only code now follows the scaffold's existing Fastly-example selection, with no request-policy change. Its focused generator/native proof is retained; fresh per-branch hosted checks are not assumed green.Review order: #275 → #406 → #407 → #409 → #414 → #422 → #439 → #454. Draft integration checkpoint: #462. New SDK/group/codec/phase development and unrelated TS work are not prerequisites or additions to this PR. Earlier notes below retain their original tested revisions and limits; they are not a whole-tree pass for this new head.
Earlier implementation and proof notes
RC refresh checkpoint, 2026-11-10
Current head:
d475727691e2a80ca2dbb324568283f63e34a870.The five celld-only donor commits are replayed onto updated #414, without obsolete hosting or diagnostic ancestry. Config/KV source selection stays typed and feature-isolated; startup snapshots are bounded and KV preserves exact keys, prefixes, continuation and expiry rules.
Feature proof passes 38 celld cases, independent settings/config/KV suites, 478 Axum unit, 6 mixed AWS startup and 4 celld startup cases, CLI/isolation/runner checks and scoped strict Clippy. Independent provider source review found no confirmed defect. Lower namespace/CI fixes are merged and all five locked fetches pass.
Ordinary KV is not conditional create/CAS/atomic RMW or identity-allocation qualification. The separate record API follow-on is not included here. Prepared celld KV does not prove runtime connectivity/grants, and snapshots are not atomic across documents. Matching-service/generated runtime proofs remain separate gates.
Draft pending fresh hosted checks and the combined RC audit. Two unchanged native default-parallel deadline misses remain recorded and unclassified; serial passes and untouched-foundation parallel passes do not clear them. No timing bound was relaxed. No deployment, main merge, security-alert dismissal or review-thread resolution occurred. Historical design/review context follows.
Summary
Changes
edgezero-store-celldedgezero-adapter-axumedgezero-cliand generated Axum manifestCloses
Closes #437.
Related to #410 and stacked on #414. This is store-provider work, not the celld HTTP runtime adapter in #421.
Dependencies and draft status
issue-410-native-store-providers, currently draft Add native AWS store providers #414.f72ae21a515859570622ac9d6ca9c19cf6870b81. The companion celld branch remains local and has not been published by this PR.Test plan
cargo test --workspace --all-targets: 2,028 passed, 6 ignored. Ignored acceptance tests listed below were explicitly selected separately.cargo clippy --workspace --all-targets --all-features -- -D warnings, with focused Clippy follow-ups for the final test-only additions.cargo fmt --all -- --checkandgit diff --check.cargo check --workspace --all-targets --features "fastly cloudflare spin".wasm32-wasip2check passed; full Fastly and Cloudflare target builds were not executed.examples/app-demofull workspace tests. Both excluded consumer lockfiles passed offline locked metadata validation; reusablefixture-axumcheck passed.edgezero serve --adapter axum. Production-runner and generated native binaries were exercised instead.All Cargo verification used offline locked dependencies after reviewed lockfile updates. The workspace test runner needed stdin closed because existing consent tests assume noninteractive CI. No test was excluded to resolve that run.
The real-service command requires an explicit modified binary and uses only disposable state and dummy credentials:
The implementation plan records the tested source/binary fingerprints and proof limits. No real AWS/fleet credentials or production deployment were used.
Checklist
CLAUDE.mdconventions.edgezero_coretypes.