Repository navigation
Add native AWS store providers - #414
ChristianPavilonis wants to merge 17 commits into
Conversation
ChristianPavilonis
changed the title
ChristianPavilonis
changed the title
5dc18fe to
63892b0
Compare
There was a problem hiding this comment.
PR Review
Summary
This PR adds opt-in native AWS store providers (AppConfig Agent and Secrets Manager startup snapshots, live DynamoDB KV) and per-ID source selection for the Axum runner. The fail-closed provider design is solid. However, an existing run_app entrypoint silently ignores the new bindings locator, the new test fixtures fail on macOS, DynamoDB can accumulate HTTP connectors without bound under valid tight limits, and DynamoDB startup rejects tables that are mid-scaling. Requesting changes for those, plus doc and CI-coverage gaps.
Findings
Blocking
- 🔧
run_appignoresEDGEZERO__STORE_BINDINGS_FILE: pre-existing projects, the demo and the documented entrypoint get local stores with no warning (dev_server.rs:1017, inline at:990) - 🔧 AWS fixtures fail on macOS: accepted sockets inherit
O_NONBLOCK; 31 provider-test failures and 4 of 5 startup runs failing locally, all fixed by oneset_nonblocking(false)per fixture (dynamodb_tests.rs:148and 4 sibling sites) - 🔧 Connectors accumulate under tight limits: the per-operation connect timeout changes every call, so the SDK connector cache grows (
dynamodb_kv.rs:634) - 🔧 DescribeTable rejects
UPDATING: restarts during autoscaling fail startup (dynamodb_kv.rs:687) - 🔧 Docs contradict the code:
single_store_kindscontradiction,aws-dynamodbmissing from "Optional AWS features", draft/process wording in user guides,[limits.aws]undocumented (docs/guide/adapters/axum.md:463/478,docs/guide/native-aws-stores.md:3,10,119,212) - 🔧 A new feature-gated test never runs in CI: no job runs the axum lib tests with an
aws-*feature, so theaws-dynamodbtest innative_stores.rs:521runs nowhere (.github/workflows/test.yml:189) - ❓ Local secret IDs read the same variables: removing the one-secrets-ID limit lets them alias (
cli.rs:420) - ❓ A required CI step pulls three container images from the network (
.github/workflows/test.yml:259)
Non-blocking
- 🤔 Reads enforce the configurable write cap: lowering
max_value_bytesmakes existing rows return 503 permanently; probe-verified (dynamodb_kv.rs:357) - 🤔 Runtime DynamoDB failures all become
Unavailablewith nothing logged (dynamodb_kv.rs:286) - 🤔 Credential-chain failures are reported as
Unavailable(secrets_manager.rs:312) - 🤔 Endpoint settings from the environment or profile can redirect signed requests (
secrets_manager.rs:85) - 🤔 List cursors expose region, table and namespace (
dynamodb_kv.rs:206) - 🤔 Manifest build features are silently dropped for custom commands (
adapter.rs:650) - 🤔 Startup tests check less than they claim (
native_aws_startup.rs:348) - 🤔 The DynamoDB test client duplicates the production retry/timeout policy (
dynamodb_tests.rs:399) - 🤔 The DynamoDB proof fixture shuts down after 3 minutes, with child stdio discarded (
dynamodb_generated_proof.rs:65) - 🤔 Exact SDK pins in a library crate constrain consumers (
Cargo.toml:49) - ♻️
--target*validate flags do nothing;--strictis ignored in bindings mode (args.rs:640) - ♻️ The CLI and startup errors throw away specific, safe categories (
config.rs:1566) - ♻️ Public API wider than callers can use (
native_bindings.rs:356) - ♻️ Per-kind code repeated: the
{config, kv, secrets}triple repeats acrossNativeSuppliedIds,NativeStoreDeclarations,NativeSourceSelection,NativeStoreOverrides,NativeSourcePlanandNativeProviderCapabilities. There are threeprepare_aws_*functions, andbuild_production_stores(dev_server.rs:1357-1482) repeats the sameBuiltIn => prepare_aws_* / local => prepare(...)dispatch three times.dev_server.rsis now 4,789 lines (about 1,650 production), and theBuiltInarm ofnative_stores::prepare(native_stores.rs:415) can't be reached from its callers. AStoreKindenum plus a per-kind struct, with store building moved intonative_stores.rs, would remove most of the repetition. NAME/KEY overlay validation also exists three times (run_options.rs:376-382,native_bindings.rs:589-597,EnvConfig::store_selector_checked).native_bindings.rs:557-568re-implements a weaker copy of the manifest ID rules, with no__or case-insensitive duplicate check (comparemanifest.rs:1577-1592). - ♻️ CLI argument handling duplicated: the "normalize, reject run args for build, append
--for run" policy exists three times with three different error strings (adapter.rs:363-389,adapter.rs:637-660, axumcli.rs:536-546), and the manifest feature lookup exists four times (adapter_build_features, inline atadapter.rs:373-378,config.rs:1621-1626, axumlocate_project). One helper onNativeCargoSelectionwould cover them. Thebuild_featurescomputed inexecute()(adapter.rs:124) anddeploy()(:213) is never used by those actions. - ♻️ Test cost: the agent-only, secrets-only and combined generated builds in
native_store_generated_proof.rs:848-850are compiled and never run (the combined one is immediately superseded at:855). That costs minutes of cold build for coveragecargo check -p edgezero-adapter-axum --no-default-features --features aws-appconfig-agentgives in seconds.dynamodb_generated_proof.rsre-proves CRUD, TTL and restart, whichnative_aws_emulator_proof.rscovers against DynamoDB Local; its unique parts are the default-off refusal and the--featurespassthrough. The emulator proof's ~75-line hand-written HTTP/chunked parser could usehttparse, which is already a dev-dependency. - ⛏ Isolated-partition region passes as
aws(settings.rs:601) - ⛏ Bearer token length counted in
stats()/Debug(preparation.rs:165) - ⛏ AppConfig timeouts map to
Unavailable, SDK timeouts toDeadline(preparation.rs:597) - ⛏
std::time::Instant::now()in settings validation (settings.rs:189) - ⛏ Leftover test-only
eprintln!(dynamodb_kv.rs:486,secrets_manager.rs:204) - ⛏ Cleanup:
- The
build_shell_commanddoc comment now sits aboveadapter_build_features(adapter.rs:558), andbuild_shell_commandlost its doc. - Stale comments:
secret_env.rs:10-11("single secret namespace"),config.rs:5049-5055("both Single-capable for secrets"),native_store_generated_proof.rs:819-820("Port 1 …", but the code usesempty_endpoint()). - Dead conditional: both branches
return response;(dynamodb_generated_proof.rs:351-356). tempfileis an unused dev-dependency ofedgezero-store-aws.- Ignore reasons contain process wording ("coordinator-owned Cargo check", "coordinator owns Cargo integration"); say what the test needs instead.
- The bind-
:0-then-drop port helper was copied intonative_aws_startup.rs:370,dev_server.rs:4222and the three CLI proofs. The trailingStdTcpListener::bind(address).unwrap()atdev_server.rs:4731/4787proves nothing, because the runner never bound in those tests; it can only fail spuriously. - A relative
EDGEZERO_GENERATED_TEST_TARGET_DIRis not resolved against the project indynamodb_generated_proof.rs:462-465, unlike the other two proofs.
- The
- 🌱 Single partition key per namespace: every key of a store shares
pk = namespace, so one store's throughput is bounded by what DynamoDB will serve for a single partition-key value. It works well fordynamodb:LeadingKeysIAM scoping, but it is worth documenting as a capacity limit, with key sharding as a later option for hot stores. - 📝 PR description: at about 16,000 lines across 50 files, with DynamoDB added to the same PR, this is hard to review in one pass; a separate DynamoDB PR would have helped. Several verification receipts point to the author's local
/tmp/...paths, which reviewers can't inspect; the CI job links are what we can actually check.
📌 Out of Scope
crates/edgezero-adapter-axum/tests/production_hosting.rsfails on macOS independently of this PR. Four proxy/TLS tests bind127.0.0.2, which macOS doesn't configure by default (native_selected_families_…,native_source_bound_single_proxy_…,native_two_proxy_chain_…,native_verified_tls_ingress_…). The parallel run also races on bind-:0-then-drop ports (startup bind: listener: address unavailable). Both reproduce identically on base5bbff2bf(28 passed / 4 failed single-threaded), so they predate this PR; worth tracking separately.
CI Status
Local, macOS, head c66f93f1:
- fmt: PASS
- clippy (
--workspace --all-targets --all-features -D warnings): PASS - tests (
--workspace --all-targets): PASS (2,860 passed, 0 failed, 4 ignored) - check (
--features "fastly cloudflare spin"): PASS - spin wasm32-wasip2 check: PASS
scripts/check_native_store_dependency_contract.sh: PASS- feature-gated provider/startup suites: FAIL on macOS (non-blocking socket fixtures, see inline); PASS with the one-line fixture fix applied
dynamodb_generated_proofwith a cold target: PASS (82 s)
GitHub Actions on head c66f93f1: all checks green, including the emulator step that pulls container images.
| /// # Errors | ||
| /// Returns startup or listener errors from the selected runner. | ||
| #[inline] | ||
| pub fn run_generated_app<A>() -> anyhow::Result<()> |
There was a problem hiding this comment.
🔧 run_app silently ignores EDGEZERO__STORE_BINDINGS_FILE
Only run_generated_app reads the locator. run_app (line 1017) goes through capture_dev_environment, which never looks at it. Every project generated before this PR, the in-repo demo (examples/app-demo/crates/app-demo-adapter-axum/src/main.rs), and the entrypoint documented in docs/guide/adapters/axum.md:25-35 all call run_app.
Scenario: an operator follows docs/guide/dynamodb-kv.md:36 ("Set EDGEZERO__STORE_BINDINGS_FILE … There is no fallback to redb"), enables aws-dynamodb, and deploys an existing project. KV writes land in .edgezero/kv-*.redb on each replica's local disk, and nothing fails or warns. AppConfig and Secrets Manager bindings are skipped the same way. This contradicts the PR's "an unavailable selected provider fails startup; it never falls back to local data".
Fix: no existing deployment sets this new variable, so failing closed in the dev runner breaks nobody:
// capture_dev_environment, after `let Some(key) = os_key.to_str()`
if is_store_bindings_locator_key(key) {
return Err(failure(
"settings",
"STORE_BINDINGS_FILE",
"not supported by run_app; use run_generated_app or run_production_app",
));
}Alternatively, route run_app through the same dispatch and drop the extra entrypoint. Either way, add a migration note: switch main.rs and add the [features] block from Cargo.toml.hbs. Update app-demo too.
| let worker = thread::spawn(move || { | ||
| while !worker_stopped.load(Ordering::Acquire) { | ||
| match listener.accept() { | ||
| Ok((mut stream, _)) => { |
There was a problem hiding this comment.
🔧 The AWS test fixtures fail on macOS: accepted sockets stay non-blocking
The listener is set_nonblocking(true). On macOS, std uses plain accept(), so the accepted stream inherits O_NONBLOCK, and set_read_timeout has no effect on it. When the accept poll runs before the request bytes arrive, read fails with WouldBlock (os error 35) and the fixture thread panics.
Measured locally (macOS, head c66f93f):
--features secrets-manager --lib: 4 of 28 failed (read request headers: Os { code: 35, kind: WouldBlock })--features dynamodb-kv --lib: 15 of 46 failed--all-features --lib: 12 of 78 failed--features aws-stores,test-utils --test native_aws_startup: 4 of 5 runs failed
After adding one line after each accept, every one of those passed, including 5 of 5 startup runs. Linux CI doesn't hit this because Linux sockets don't inherit the flag.
Fix:
Ok((mut stream, _)) => {
stream.set_nonblocking(false).expect("blocking fixture stream");The same pattern needs the same fix at secrets_manager.rs:452, adapter-axum/tests/native_aws_startup.rs:87, edgezero-cli/tests/dynamodb_generated_proof.rs:68 and edgezero-cli/tests/native_store_generated_proof.rs:308.
|
|
||
| fn timeout_policy(limits: DynamoDbKvLimits, budget: Duration) -> TimeoutConfig { | ||
| TimeoutConfig::builder() | ||
| .connect_timeout(budget.min(Duration::from_millis(limits.connect_timeout_ms))) |
There was a problem hiding this comment.
🔧 Under tight limits, every operation adds a new HTTP connector that is never freed
budget is the remaining deadline at nanosecond precision. The SDK caches one connector (a hyper client with its own pool) per exact (connect_timeout, read_timeout), in a HashMap with no eviction (aws-smithy-http-client-1.5.0/src/client.rs:755-822, looked up per attempt from aws-smithy-runtime-1.16.0/src/client/orchestrator.rs:491-498). Whenever budget < connect_timeout_ms, each operation gets a new cache key.
Probe (two consecutive operation_policy calls on the same store):
- default limits:
Some(1s) Some(1s), stable, nothing leaks operation_timeout_ms = attempt_timeout_ms = 1000(accepted byDynamoDbKvLimits::validate):Some(999.835583ms) Some(999.595166ms)
With such limits, every KV operation adds a connector that is never freed and pays a fresh TCP/TLS handshake. Memory grows with traffic. Values that are close but not equal leak whenever the time between runtime_deadline() and the call exceeds the gap.
Fix: keep the connect timeout constant. The attempt timeout, operation timeout and outer deadline already bound the budget.
.connect_timeout(Duration::from_millis(limits.connect_timeout_ms))Please add a regression test that asserts the connect timeout is identical across two operations.
| if table | ||
| .table_status | ||
| .as_ref() | ||
| .is_none_or(|status| status.as_str() != "ACTIVE") |
There was a problem hiding this comment.
🔧 The startup check rejects UPDATING tables, so a restart during autoscaling fails
An UPDATING table still serves reads and writes. Application Auto Scaling, capacity-mode switches and stream changes all move a table into UPDATING through UpdateTable, sometimes for minutes.
Scenario: a rolling deploy lands during a scale event. New instances fail preparation with PreparationError::InvalidResource and never open a listener. The category also sends operators looking for a schema problem.
Fix: accept both states, and add a fixture case with "TableStatus":"UPDATING":
.is_none_or(|status| !matches!(status.as_str(), "ACTIVE" | "UPDATING"))If you'd rather keep the strict check, map non-ACTIVE status to Unavailable rather than InvalidResource.
| pre-rewrite `[stores.config.defaults]` table is gone (see | ||
| [the migration guide](../manifest-store-migration.md)). | ||
| A declared `[stores.secrets]` ID still defaults to an environment-backed store when | ||
| not explicitly bound. It is no longer limited to one ID by Axum. Existing |
There was a problem hiding this comment.
🔧 The guide contradicts itself and the code
- This line says secrets are "no longer limited to one ID by Axum", but line 463, a few lines up, still says "Axum lists
secretsin itssingle_store_kinds, so only one secrets id may be declared". The code now returns&[](cli.rs:419-423). - The
## Secret Storeand## Config Storeheadings were removed. The config-store intro went with them: missing files create empty stores, malformed files fail bindings, push takes effect after restart, and the migration-guide link. - "Optional AWS features" (around line 576) lists
aws-appconfig-agent,aws-secrets-managerandaws-stores, but notaws-dynamodb. - Line 590, "This limitation must be disclosed in release and PR review notes.", is process wording in a user guide. So are
native-aws-stores.md:3("This guide is a draft"),:119("Before publishing this guide, the operator must…") and:212("must be called out in reviews of the first version").native-aws-stores.md:10callsaws-stores"the existing alias", but this PR introduces it. [limits.aws](field names and defaults: 1 MiB document, 64 KiB secret, 1024 mappings, 8 MiB snapshot, 30 s / 5 s / 1 s / 2 s) is documented only indocs/superpowers/plans/…. The guide says the cap is configurable but never says how.- Nothing says that once a bindings file is selected, the remaining local IDs need absolute
EDGEZERO__ADAPTER__CONFIG_DIR/DATA_DIR, and that a missing local snapshot becomes fatal (dev_server.rs:1376-1395,native_stores.rs:310-317). Line 452 implies that unbound IDs keep using.edgezero/.
| "aws-us-gov" => family.starts_with("us-gov-"), | ||
| "aws" => { | ||
| [ | ||
| "af-", "ap-", "ca-", "eu-", "il-", "me-", "mx-", "sa-", "us-", |
There was a problem hiding this comment.
⛏ An isolated-partition region passes as aws
valid_region("aws", "eu-isoe-west-1") returns true: eu-isoe-west starts with eu-, and only us-iso* is excluded. So arn:aws:secretsmanager:eu-isoe-west-1:… validates (for DynamoDB settings too), although the comment says isolated partitions are not supported in v1.
Fix: reject any segment starting with iso, and add eu-isoe-west-1 to the rejection matrix:
&& !family.split('-').any(|part| part.starts_with("iso"))| } => env_name | ||
| .as_ref() | ||
| .map_or(Some(0), |name| Some(name.len())) | ||
| .and_then(|bytes| bytes.checked_add(bearer_token.len())) |
There was a problem hiding this comment.
⛏ The bearer token's length leaks into stats() and Debug
metadata_bytes adds bearer_token.len(), and stats() is printed by AwsPreparation's Debug (:68-74). That contradicts :195 ("no … auth data") and settings.rs:123 ("Debug hides both the token and its length"). The URL and variable name are known configuration, so the token length can be worked out. Metadata has no cap, so counting the token buys nothing; drop it from the sum.
| } | ||
|
|
||
| #[cfg(feature = "appconfig-agent")] | ||
| fn map_http_error(_error: reqwest::Error) -> PreparationError { |
There was a problem hiding this comment.
⛏ AppConfig timeouts map to Unavailable, but SDK timeouts map to Deadline
A reqwest connect, request or body timeout becomes Unavailable, while commit 652e324 makes the equivalent SDK connector timeout Deadline. The two providers now report the same condition differently.
fn map_http_error(error: reqwest::Error) -> PreparationError {
if error.is_timeout() { PreparationError::Deadline } else { PreparationError::Unavailable }
}| || self.connect_timeout_ms == 0 | ||
| || self.connect_timeout_ms > self.attempt_timeout_ms | ||
| || self.attempt_timeout_ms > self.operation_timeout_ms | ||
| || Instant::now() |
There was a problem hiding this comment.
⛏ std::time::Instant::now() in settings validation
The settings module is compiled unconditionally, including for wasm32, where std::time::Instant::now() panics on wasm32-unknown-unknown, and the project rule is web-time. The result also depends on host uptime, so validation isn't deterministic. AwsPreparation::new already checks that the deadline is representable (preparation.rs:98-102). A fixed maximum would do here and at :466, for example self.operation_timeout_ms <= 86_400_000.
| clippy::print_stderr, | ||
| reason = "test-only closed SDK error kind excludes source details and payloads" | ||
| )] | ||
| fn report_sdk_error_kind<E, R>(error: &SdkError<E, R>) { |
There was a problem hiding this comment.
⛏ Leftover #[cfg(test)] debug output
report_sdk_error_kind prints sdk-error-kind=… to stderr during tests, and nothing reads it. The same leftover is at secrets_manager.rs:204-219. It looks like a debugging aid from the timeout-classification change; please remove both, along with their call sites.
dhruv8sh
left a comment
dhruv8sh
left a comment
There was a problem hiding this comment.
Approving. Neither review pass found a correctness, security or CI-blocking defect; all findings are non-blocking. CI is green at c66f93f1, and locally fmt, clippy (-D warnings) and the full test suite pass (2861 passed, 0 failed).
Inline comments cover the 🤔 / ♻️ / ⛏ findings. Remaining notes:
🏕 campsite: No CHANGELOG entry. The base branch keeps a per-feature ## Unreleased section in CHANGELOG.md, and this PR adds a whole new crate.
📝 memo: In secrets_manager.rs:164-178 the size cap applies after the full read. The code comment already notes this, and risk is low since AWS caps secrets at 64 KiB.
😃 praise:
- DynamoDB list cursors are bound to region, table, namespace and prefix, reject unknown fields, and are capped at 16 KiB.
- A secret response with both
SecretStringandSecretBinary, or neither, is rejected asMalformed. - Error messages and
Debugoutput report a category only, never values. - SDK tests run in a child process with a cleared environment, loopback-only endpoints and dummy credentials.
dev_server.rsbinds the listener only after store preparation succeeds, so a failed preparation never opens a port.
| .config_override(self.operation_policy(deadline)?) | ||
| .send() | ||
| .await | ||
| .map_err(|_error| KvError::Unavailable)?; |
There was a problem hiding this comment.
🤔 thinking (non-blocking): Put, delete, get and query (also lines 318, 347, 422) map every SDK error to KvError::Unavailable: throttling, access denied, expired token, missing table and timeout all look the same. Startup preparation already distinguishes these in classify_preparation_error (lines 517-536). Could runtime paths reuse that classification where KvError has a matching variant, or document that the collapse is intentional?
| options = options.with_data_dir(root)?; | ||
| } | ||
| options.env = env; | ||
| for (key, value) in pairs { |
There was a problem hiding this comment.
🤔 thinking (non-blocking): from_vars copies every input variable, including EDGEZERO__SECRETS__* values, into bootstrap_values on a Clone struct, but only agent_token() reads that map. Nothing leaks today (no Debug), but secrets stay in memory longer than needed and are copied on every clone. Suggest retaining only the names listed in access_token_env, as capture_agent_tokens already does.
| /// `[stores.config].ids` has length 1). | ||
| #[arg(long)] | ||
| pub store: Option<String>, | ||
| /// Explicit native bindings file. Used only to reject unsupported AWS-bound publication. |
There was a problem hiding this comment.
🤔 thinking (non-blocking): The rejection of AWS-backed bindings only runs when --store-bindings is passed. Without it, config push pushes local entries with no warning. Consider also checking EDGEZERO__STORE_BINDINGS_FILE, or documenting the limit.
| }; | ||
| // Expiry is checked after validation and cursor encoding, not at request start. | ||
| let completed = epoch_ms()?; | ||
| let keys = rows |
There was a problem hiding this comment.
🤔 thinking (non-blocking): DynamoDB applies Limit (line 398) before expired rows are filtered here, so a page can come back short or empty while still returning a cursor. Callers that stop on empty keys will stop early. Suggest documenting this in the KvPage contract and adding a test for a page where every row has expired.
| [features] | ||
| default = ["axum"] | ||
| axum = [ | ||
| "native-bindings", |
There was a problem hiding this comment.
🤔 thinking (non-blocking question): The axum feature now always enables native-bindings, which pulls toml and edgezero-store-aws (settings only, no SDK) into every native build. The dependency-contract script confirms no AWS SDK leaks in. Is the larger default dependency graph intended, or should native-bindings stay opt-in?
| } | ||
|
|
||
| #[inline] | ||
| async fn exists(&self, key: &str) -> Result<bool, KvError> { |
There was a problem hiding this comment.
♻️ refactor: exists calls get_bytes, downloading up to ~384 KiB just to return a bool. A get_item with a projection of only the key, expiry and TTL attributes would avoid that.
| _ => {} | ||
| } | ||
| } | ||
| if status == Some(403) { |
There was a problem hiding this comment.
♻️ refactor: This classifier and the DynamoDB one (dynamodb_kv.rs:531) disagree: DynamoDB maps HTTP 401 to AccessDenied, Secrets Manager does not. Neither maps signature errors (InvalidSignatureException, MissingAuthenticationTokenException, IncompleteSignature) to Credentials. Suggest sharing one code table between them.
| AgentAccessToken::new(value.clone()).map_err(|_category| { | ||
| failure("settings", "Agent token", "invalid configured value") | ||
| })?; | ||
| self.bootstrap_values |
There was a problem hiding this comment.
⛏ nitpick: capture_agent_tokens uses a plain insert, which could overwrite a BootstrapValue::Ambiguous entry. Probably unreachable in practice, but an entry-based match like from_vars would make it robust.
| let remaining = self.operation_budget()?; | ||
| let connect = remaining.min(Duration::from_millis(self.limits.connect_timeout_ms)); | ||
| let attempt = remaining.min(Duration::from_millis(self.limits.attempt_timeout_ms)); | ||
| let client = timeout(remaining, async { |
There was a problem hiding this comment.
⛏ nitpick: The cached client is built with a timeout taken from the first call's remaining budget. Each call overrides it today, but building the client without a timeout would be safer if that ever changes.
| branches: [main] | ||
| pull_request: | ||
|
|
||
| permissions: |
There was a problem hiding this comment.
⛏ nitpick: Unrelated whitespace change: the blank line before permissions: was deleted.
# Conflicts: # crates/edgezero-cli/Cargo.toml
prk-Jr
left a comment
prk-Jr
left a comment
There was a problem hiding this comment.
PR Review
Summary
Reviewed at 3733482f. The commits since the previous changes-requested review (at c66f93f) only merge the stack and repair fixtures, so the blocking findings from that review still apply at this head. This pass adds one documentation blocker and several smaller items. Requesting changes because the earlier blockers are still open.
😃 Praise
- Every level of the bindings TOML is
deny_unknown_fields, including the internally tagged provider enums and the nested limit tables. The file-type check uses the opened handle, so there is no type race between checking and reading. - Every Axum feature combination compiles: none,
cli,native-bindings,axum, eachaws-*alone, andaws-stores. With no AWS feature, a bindings file fails closed withUnsupportedProviderbefore any local store opens. - The listener binds only after source validation, store preparation and the initializer complete.
Findings
Earlier blockers still open at 3733482f
- 🔧
run_appstill ignoresEDGEZERO__STORE_BINDINGS_FILE. Onlyrun_generated_appreads it (dev_server.rs:990vs:1017). The demo and theaxum.mdentrypoint still callrun_app. - 🔧 The AWS fixtures still fail on macOS.
cargo test -p edgezero-store-aws --all-features --libfailed in three runs at this head (2, 3 and 1 of 79 failed) withread request headers: Os { code: 35, kind: WouldBlock }. Commite9181716keeps the listener alive longer but does not make accepted streams blocking. - 🔧 Unchanged at this head:
- The
UPDATINGtable status is still rejected (dynamodb_kv.rs:687). - SDK connectors still accumulate under tight limits (
dynamodb_kv.rs:634). - The docs contradictions are still present.
- CI still runs no axum lib tests with an
aws-*feature. - The two open questions (local secret IDs sharing env variables, container pulls in the required CLI job) have no answer yet.
- The
New, blocking
- 🔧 The guide's
config pushcommand cannot reach the guard it describes (docs/guide/native-aws-stores.md:281, inline). This belongs with the existing docs blocker.
New, non-blocking
- 🤔 Secret name length is validated on a trimmed value, but runtime uses the untrimmed one (
native_bindings.rs:600, inline). - 🤔 The env-selected bindings file cannot use non-default trusted limits (
run_options.rs:293, inline). - 🤔 A compiled-out AppConfig provider is reported as a missing token (
run_options.rs:183, inline). - ⛏ Setting one timeout limit alone can be rejected because of another field's default (
settings.rs:187, inline). - ⛏ The CLI reference does not list the new flags.
docs/guide/cli-reference.mdhas no--adapter,--store-bindings,--target,--target-features,--target-all-featuresor--target-no-default-featuresforconfig validate, and no--store-bindingsforconfig push. It also does not mention that--adapter axumonvalidateswitches to bindings-only mode. This file is not in the diff, so the finding goes here. - ⛏ "Existing" wording in
dynamodb-kv.md(docs/guide/dynamodb-kv.md:18, inline). This is a sibling of the earliernative-aws-stores.mdwording comment. - 📝 Addition to the earlier read-cap comment (
dynamodb_kv.rs:357).get_byteschecksmax_value_bytesbefore the expiry filter (:354-367). A row that is over a lowered cap and has also expired therefore returnsUnavailableinstead of reading as absent, until the DynamoDB TTL sweep removes it. Capping reads at the fixed 393,216-byte maximum, as already suggested, fixes both cases. - 📝 The CodeQL clear-text logging alert on
scripts/native_aws_test_admin.mjslooks like a false positive. The script only accepts a literalhttp://127.0.0.1:<port>endpoint with a dummy SigV4 header. The logged result holds hard-coded fixture values, and stdout is the IPC channel to the Rust test. Dismissing it as test fixture data seems reasonable.
CI Status
- GitHub Actions at
3733482f: all checks pass. - Local, macOS, at
3733482f:edgezero-store-aws --all-features --libfails intermittently, as described above. cargo checkacross the Axum feature matrix: passes.- Axum lib
native_/run_optionstests withaws-stores,aws-dynamodb,test-utils: 70 passed. - CLI lib bindings/push tests: 10/10.
- Node
native_aws_*tests: 16/16. - Not run: the Docker emulator suite, the ignored generated-build proofs, and anything against real AWS.
|
|
||
| ## Push remains local | ||
|
|
||
| `edgezero config push --adapter axum` writes local config snapshot files. It does not publish |
There was a problem hiding this comment.
🔧 This command cannot reach the push guard described here
The bundled edgezero binary parses config push as ConfigCmd::Push(ConfigCmdStubArgs) (args.rs:100). That stub accepts any flags, --store-bindings included, and run_config_push always returns the "requires the typed downstream CLI" error (config.rs:314-322). cli-reference.md says the same.
Scenario: an operator copies edgezero config push --adapter axum --store-bindings /abs/bindings.toml. The command exits with the stub error. They never see the AWS-binding refusal, and nothing is written locally either.
Fix: use the typed CLI form here, as blob-app-config-migration.md does:
<your-app>-cli config push --adapter axum --store-bindings /absolute/path/bindings.toml| } | ||
| if kind == "secrets" && source != NativeBindingSource::Supplied { | ||
| let namespace = if source == NativeBindingSource::Local { | ||
| env.store_setting(kind, id, "name").map_or(id, str::trim) |
There was a problem hiding this comment.
🤔 The length check measures a trimmed name, but runtime uses the untrimmed name
For local secret IDs this checks store_setting(...).map_or(id, str::trim) against MAX_NAME_LEN. The runtime store is built with options.env_config().store_name("secrets", id) (dev_server.rs:1467). EnvConfig::store_name (env_config.rs:170-174) filters out only blank and control values; it does not trim.
Scenario: EDGEZERO__STORES__SECRETS__SIGNING__NAME holds 512 bytes plus a trailing space. Startup accepts it and reports Ready. Then every SecretHandle::get_bytes call fails validate_name with 513 bytes. The fail-fast this check exists for (secret_namespace_limits_are_checked_before_preparation) becomes a failure on every request.
Fix: measure what runtime uses, env.store_name(kind, id).len(), or trim in both places.
| } | ||
| } | ||
| } | ||
| if let Some(path) = options.env.get(&["store_bindings_file"]).map(str::to_owned) { |
There was a problem hiding this comment.
🤔 With an env-selected bindings file, the trusted file limits cannot be changed
from_vars selects the bindings file as soon as the locator is present, using the default NativeBindingsLimits (256 KiB, 128 IDs). Any later with_native_bindings_limits call is rejected with "limits must precede selection" (:470-487). The NativeBindingsLimits doc presents these as host-controlled trusted bounds. On the env-located path, which from_env, run_production_app and run_generated_app all use, the host has no way to set them. A larger deployment has to drop from_env and rebuild every hosting setting by hand.
Fix: record the locator in from_vars and read the file lazily, either at plan validation or the first bindings() call. Alternatively, add a from_env_with_limits(limits) constructor.
| } | ||
| let mut options = Self::from_vars(pairs)?; | ||
| options.non_unicode_overrides = non_unicode; | ||
| // Only this env-loading boundary may resolve named token variables. |
There was a problem hiding this comment.
🤔 A compiled-out AppConfig provider is reported as a missing token
capture_agent_tokens runs on the env-loading path whatever features are compiled. Suppose a binary built without aws-appconfig-agent loads a bindings file with [config.x] provider = "aws-appconfig-agent" and access_token_env = "T", and T is unset. Startup fails with the Agent-token-absent error before the capability check (native_stores.rs:258-271) can report "provider not compiled". That sends the operator after a token, when the actual fix is a Cargo feature.
Fix: skip token capture when !cfg!(feature = "aws-appconfig-agent"), or run the provider-capability check before capturing tokens.
| if self.max_value_bytes == 0 | ||
| || self.max_value_bytes > 393_216 | ||
| || self.connect_timeout_ms == 0 | ||
| || self.connect_timeout_ms > self.attempt_timeout_ms |
There was a problem hiding this comment.
⛏ Setting one limit alone can fail because of another field's default
Unset fields are filled in by #[serde(default)], then connect <= attempt <= operation is enforced. Setting only operation_timeout_ms = 1500 keeps the default attempt_timeout_ms = 2000, so validation returns InvalidLimits without naming any field. AwsPreparationLimits::validate has the same behaviour (:475-477).
Fix: document the ordering rule next to the limits, or clamp unset attempt and connect defaults to the configured operation timeout.
| features = ["aws-dynamodb"] | ||
| ``` | ||
|
|
||
| `aws-stores` retains its existing AppConfig Agent and Secrets Manager membership. |
There was a problem hiding this comment.
⛏ "aws-stores retains its existing AppConfig Agent and Secrets Manager membership". aws-stores is introduced in this PR, so it has no earlier membership for readers to rely on. This is a sibling of the wording comment on native-aws-stores.md. Suggest: "aws-stores enables AppConfig Agent and Secrets Manager only."
Current review readiness
Ready for review. Required current-head CI passed at
c66f93f118141a2ed7469a8ab96b12902f3854e6: test run 42924131967.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. An additional Fastly WASM runtime job was still running at transition and is not counted as passed. 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:
c66f93f118141a2ed7469a8ab96b12902f3854e6. 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:
48761e3e528eae0481e9f6e2eb30c0002f00d239.Only the AWS provider delta is replayed onto the updated hosting/diagnostic stack. Duplicate historical foundation/hosting commits are omitted. Explicit production options and initializer startup have a TCP no-ambient-merge regression. Ordinary test PR coverage remains unfiltered; an old donor filter was removed again.
Feature proof passes 417 Axum unit, 5 AWS startup, 33 hosting, 74 AWS provider, 256 CLI unit and 15 Node runner cases, scoped strict Clippy and SDK-isolation checks. Parent namespace/CI composition reruns pass all 5 AWS startup, 33 hosting and 35 Node CI cases. All five tracked locked fetches pass.
Generated native consumer/DynamoDB and pinned disposable-service runtime proofs are still pending integration execution. Bounded retained SDK values are not an RSS cap; timeout cancellation does not prove blocking credential work stopped. AWS clients intentionally retain their normal credential chain, separate from explicit hosting-source selection. No cloud acceptance is claimed.
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
Sendsetup futures.Design and boundaries
This adds native store providers, not a new HTTP runtime.
edgezero-store-awsimplements the existing core store traits and does not depend on Axum. Axum owns provider selection and startup composition; core owns portable handles, per-ID registries and typed configuration resolution.The portable
edgezero.tomldeclares logical store IDs and defaults. AWS resource references belong in a separate versioned native bindings document, selected through an absolute file path inEDGEZERO__STORE_BINDINGS_FILEor through explicitAxumRunOptions. Applications can also supply ready handles or non-Senddeferred setup, without a plugin registry.Each declared ID selects one supplied, explicitly bound or implicit local source. Supplied/file collisions and undeclared IDs are rejected. The runner validates all sources before opening runner-owned local stores or polling deferred setup.
CONFIG_DIR/DATA_DIRare required only for remaining local bindings. An unavailable selected provider fails startup; it never falls back to local data.ConfigStorereadsSecretStorereadsKvStoreoperationsflowchart TD Bindings[Bindings and supplied inputs] --> Validate[Validate sources and compiled providers] Validate --> Prepare[Assemble app and prepare stores in the native runtime] Prepare --> Registries[Complete per-ID registries] Registries --> Initialize[Run app-owned initializer when supplied] Initialize --> Serve[Bind listener and serve requests]One shared AWS preparation session owns startup budgets, client reuse and snapshot deduplication. The initializer receives the same registries used by requests. The application chooses its required config keys, schemas, secret checks and retained startup state; the framework does not infer those requirements or promise an atomic revision across remote resources.
AWS features are default-off and native-only. Existing entrypoints retain their local defaults; newly generated entrypoints select strict startup only when the bindings-file locator is present. Trusted Server adoption is separate work, and this does not claim a source-compatible upgrade from older EdgeZero versions across the #275/#406 prerequisites.
Stack and scope
Existing draft stack member based on
docs/outbound-http-spec/ #275 atea66eff045512cccc60646d6ec339e83ff44fed7, with the compatible #406 production lifecycle bridge. Neither prerequisite PR was rewritten or merged. The user approved adding DynamoDB on this same branch/PR and committing/pushing it after checks.Real AWS/IAM/capacity/deployment/image acceptance remains explicitly excluded. No real AWS service calls, infrastructure changes, image pulls or deployments were performed. SDK fixtures use dummy credentials, isolated or bypassed profile discovery, and literal-loopback endpoints.
DynamoDB KV
dynamodb-kvprovider / generatedaws-dynamodbfeature; existingaws-storesremains AppConfig Agent + Secrets Manager only.[kv.<id>]binding with explicit region/table/physical namespace, no credential or endpoint fields. Normal operator-owned SDK credential/endpoint policy remains supported.pk/sktable; bounded DescribeTable preflight, exact table-name/schema checking, no provisioning or local fallback. All-remote KV needs noDATA_DIR.DynamoDB is live storage, not a retained startup snapshot. The existing default 8 MiB retained-payload cap is per shared config/secret preparation session, not process/RSS or live database contents. Config/secrets still adopt remote changes only on restart; no live config/secret refresh or mutation is added.
Bindings validation remains structural-only / provider availability unknown. No offline Cargo resolver or guessed availability verdict is shipped. Custom shell actions remain untouched.
Verification
Original snapshot phase: all 24 post-rebase gates passed at its recorded source checkpoint; historical evidence remains in the original spec/plan and
/tmp/native-store-integration/final-checks/. This does not claim those old results prove the later DynamoDB edits.DynamoDB extension: 20 code gates plus docs lint/full-format/build passed, against an archived exact staged source tree independent of eight pre-existing dirty files. SHA-256 input manifests matched; later publication-record edits are Markdown-only.
aws-storesmembership, Cloudflare/Fastly/Spin WASM checks.Unavailable, one attempt and the retained/removed row. SDK model Debug disclosure is directly asserted.Senddeferred source validation without polling/opening another local store; existing generic native staging atomicity/drop tests preserved.edgezero serveinvocation; shared argument/action selection has unit/source proof, generated build and startup are executed.An inherited Secrets timeout test transiently returned
Unavailableinstead ofDeadlineunder the combined parallel run. It passed alone, then all 74 provider tests passed serially. The initial failure log is retained; no production/unrelated test was changed to hide it.Commands/fingerprints:
/tmp/dynamodb-baseline/final-checks.sh,/tmp/dynamodb-baseline/final/,snapshot-{before,after}.sha256; checked source tree2fa76fb26377cf41203c255f924d988d6b657002. Final read-only provider review found no confirmed defects and is separate from executed checks.Closes
Closes #410
Checklist
Sendsetup preservedFixture lifetime follow-up
Commit
e9181716975c068cd6894e046eec1ee3e194ad69keeps the loopback endpoint alive until its SDK child completes. A local reproduction of the hosted timeout failure identified a genuine ConnectionRefused after the one-response worker dropped its listener while the SDK still owned retries. The deterministic endpoint-lifetime regression fails before this fix. All 79 AWS tests, strict all-feature/all-target AWS Clippy, and 30 repetitions of the original timeout test pass afterward. This changes only the test fixture. Production timeout limits, retry policy and typed error classification are unchanged. Current-head hosted qualification is running; no release tag is claimed.