[ChristianPavilonis]

Current review readiness

Ready for review. Required current-head CI passed at c52bdaf34bb8694a6f2ee5b584ef127379db4cb0: test run 42924132336.

Minimal fixes originate in #414 and are merged upward:

• Typed SDK connector timeouts map to Deadline; non-timeouts remain Unavailable. The previously failing hosted timeout test and all 78 provider cases now pass without assertion, retry or duration changes.
• Fresh consumers use the existing locked SDK cohort: optional native aws-types =1.6.0 and aws-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.
• The constructor-boundary test attempts the full upload while independently reading and requiring a complete, finite expected response. The existing startup-readiness poll treats a connection reset as not yet HTTP-ready within its unchanged deadline; success still requires a complete HTTP200 and the original payload checks. Production transport and limits are unchanged.

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 nine-path standalone prototype and its Core inputs are unchanged; its 10 tests pass. This is not production KV Geo. 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-Jr and dhruv8sh remain requested. Stack465 and PR bases are unchanged; #453 is excluded.

Summary

• Restack the existing standalone KV Geo prototype above the refreshed Add pluggable native Geo providers and protected CloudFront ingress #439 branch. Preserve the original importer, lookup algorithm, harness, manifest and historical measurement documents.
• Reconcile only the prototype lockfile with the current Core dependencies. No production crate, Geo dispatch or asynchronous production provider is added.
• Open for review as a standalone prototype. CloudFront remains the demo's serving provider; this prototype is not production KV Geo.

Stack and exact inputs

Base: #439, feature/pluggable-geo-providers at 414e52ae41ee95880fd11238a22ad2dc944f44c2.

Head: c52bdaf34bb8694a6f2ee5b584ef127379db4cb0.

The delta is the original nine prototype/document paths. Runtime source bytes remain identical to the original e4ede101 experiment. The Core source tree used by the earlier passing restack is also identical. #453 is excluded, with no MaxMind implementation merged.

Refs #438 and #391. This experiment does not close their production acceptance work.

Checks

Fresh checks on this restack:

• cargo test --offline --locked --manifest-path examples/geo-kv-prototype/Cargo.toml: 10 passed, zero ignored.
• Prototype all-target Clippy with -D warnings.
• Prototype formatting and whitespace checks.

Reusable earlier proof has unchanged prototype/lock/Core inputs: release build, workspace exclusion and Core dependency isolation, plus synthetic CLI repeat-build, 768 cache/oracle comparisons and eight fresh lookups. Historical real-data measurements remain attached to their original source and measurement record; they were not rerun for this restack.

No current combined-workspace or hosted runtime pass is claimed by these prototype checks. The standalone crate remains excluded from the root workspace, so ordinary workspace CI does not exercise it automatically.

Limits

The harness stores artifacts through synchronous local files behind KvHandle. This does not establish remote KV publication, latency, propagation, cancellation, reader-IP provenance, request integration, deployed cache behavior or fleet durability. Country-code compatibility and format/publication decisions remain as documented in the unchanged design and results files.

No licensed dataset, generated index, credentials or deployment inputs are added. No deployment or live infrastructure change is part of this PR.

[aram356]

PR Review

Summary

This PR adds a standalone KV Geo prototype: a CSV importer, the p1 binary index, digest-validated async lookup through KvHandle, the design spec and the results record. The lookup algorithm holds up under a randomized differential check: 872,448 lookups compared against naive range containment, covering both families, object budgets of 512–4096 bytes and cache budgets of 0, 2 KiB and 1 MiB, with zero mismatches. The blocking issues are about keeping the crate buildable and the evidence accurate. Nothing in CI builds it, its pins break on the next root bump, several tests can't fail, and the results doc's provenance doesn't match this head.

Findings

Blocking

• 🔧 No CI coverage, and exact pins coupled to the root: a simulated root serde_json bump makes this crate unresolvable while CI stays green (examples/geo-kv-prototype/Cargo.toml:8)
• 🔧 Object cache effectively untested: a cache that stores nothing passes all 10 tests (examples/geo-kv-prototype/src/index.rs:635)
• 🔧 Overlap-rejection test can't fail: no build succeeds at 140 bytes, and deleting the overlap check still passes (examples/geo-kv-prototype/src/index.rs:763)
• 🔧 Results doc provenance and coverage claims don't match this head (docs/superpowers/plans/2026-11-05-kv-geo-prototype-results.md:9)

Non-blocking

• 🤔 Cache hit skips the family check, so warm and cold resolvers can disagree (src/index.rs:531)
• 🤔 IPv4-mapped input misses (src/index.rs:501)
• 🤔 Generation ID isn't derived from content (src/index.rs:274)
• ♻️ Validation runs after publication (src/index.rs:291, src/main.rs:55)
• ♻️ Second SHA-256 stack (Cargo.toml:20)
• 🤔 load and ArtifactStore have no tests (src/import.rs:149)
• ⛏ I/O errors lose the path (src/import.rs:132)
• ⛏ oracle_mismatches is a constant (src/main.rs:128)
• 🌱 An interrupted write blocks every rerun (src/store.rs:98)
• 🌱 Before p1 is frozen:
  • The manifest serializes u128 bounds as JSON numbers and each digest as a 32-element array. IPv6 bounds above 2^53 lose precision in JavaScript and jq, and each digest takes about 115 characters as an array versus 64 as hex. Hex strings fix both.
  • Tests match anyhow errors by substring, which doesn't give the bounded failure categories spec §9 asks for. A typed error enum would.
  • Cache::get calls order.retain on every hit, which costs O(entries). A production cache needs O(1) LRU bookkeeping.
  • Resolver::lookup(&mut self) can't be shared across concurrent requests as written.

📌 Out of Scope

• Dependabot's cargo entry covers only /. Standalone lockfiles such as this crate's get updated only when someone notices a root bump broke them. A tracking issue across the standalone crates would cover this.

CI Status

• PR checks: 18/18 pass at c52bdaf3
• Local workspace gates (fmt, clippy, test, feature check, spin wasm32): PASS
• Local prototype checks: fmt PASS, tests PASS (10/10), clippy -D warnings PASS, release build PASS, cargo check for wasm32-unknown-unknown and wasm32-wasip1 PASS
• None of the workspace gates or PR checks compile this crate.

How the findings were checked

• Mutation testing: cargo mutants on index.rs, import.rs and store.rs produced 243 mutants: 146 caught, 71 missed, 20 unviable, 6 timeouts.
• Probe tests in a scratch copy covered the cache/family divergence, IPv4-mapped input, the minimum budget and the randomized differential check.
• CLI run: end-to-end build/verify/lookup on synthetic Country-shaped CSV.

[aram356]

🔧 Nothing in CI builds this crate, and its exact pins are coupled to the root's

This crate's own [workspace] keeps it out of every root gate (cargo fmt --all, workspace clippy and test), and no workflow step uses --manifest-path examples/geo-kv-prototype/Cargo.toml. All 18 checks on this PR passed without compiling it.

The exact pins on bytes (L13) and serde_json (L19) mirror root [workspace.dependencies], which edgezero-core inherits, so the next root bump breaks resolution. I simulated one by changing only the root to serde_json = "=1.0.154":

error: failed to select a version for `serde_json`.
    ... required by package `edgezero-core v0.1.0 (.../crates/edgezero-core)`
    ... which satisfies path dependency `edgezero-core` (locked to 0.1.0) of package `geo-kv-prototype v0.0.0`
versions that meet the requirements `=1.0.154` are: 1.0.154
  previously selected package `serde_json v1.0.150`
    ... which satisfies dependency `serde_json = "=1.0.150"` of package `geo-kv-prototype v0.0.0`

The root workspace resolves the same bump fine, so CI stays green while this crate stops building.

Fix:

1. Loosen the mirrored pins. Cargo.lock still records the exact versions, and cargo test --locked passes with this change:

bytes = "1"
serde_json = "1"

2. Gate it the way edgezero-test-harness and app-demo already are. In test.yml, add to the Fetch dependencies (locked) step:

          cargo fetch --locked --manifest-path examples/geo-kv-prototype/Cargo.toml

and add a step to the workspace suite:

      - name: KV Geo prototype
        if: matrix.suite == 'workspace'
        run: |
          cargo fmt --manifest-path examples/geo-kv-prototype/Cargo.toml -- --check
          cargo clippy --offline --locked --manifest-path examples/geo-kv-prototype/Cargo.toml --all-targets -- -D warnings
          cargo test --offline --locked --manifest-path examples/geo-kv-prototype/Cargo.toml

[aram356]

🔧 The object cache is effectively untested

This assertion is the only cache check, and a cache that never stores anything satisfies it. With cargo mutants --file src/index.rs, each of these mutants passes all 10 tests:

• Cache::insert replaced with (), so nothing is ever cached
• != → == in the order.retain call in Cache::get, which breaks LRU order
• > → >= and + → * in the eviction loop bound
• Resolver::cache_stats returning a fresh default

The results doc (L88) lists "cache eviction" as covered, and its cache-enabled timings depend on this code.

Fix: these tests pass on this head and catch every mutant above. The third one covers core_country_supported and Location::json, which also had surviving mutants: an always-false flag passed, because no test asserts a supported country returns true.

#[test]
fn cache_evicts_least_recently_used_within_budget() {
    let object = |n: u8, len: usize| Object {
        bytes: Bytes::from(vec![n; len]),
        width: 4,
        record_bytes: 14,
        count: 0,
    };
    let mut cache = Cache::new(20);
    cache.insert([1; 32], object(1, 10));
    cache.insert([2; 32], object(2, 10));
    assert!(cache.get(&[1; 32]).is_some());
    cache.insert([3; 32], object(3, 10));
    assert!(cache.get(&[2; 32]).is_none(), "least recently used entry is evicted");
    assert!(cache.get(&[1; 32]).is_some());
    assert!(cache.get(&[3; 32]).is_some());
    assert_eq!(cache.stats.peak_payload_bytes, 20);
    cache.insert([4; 32], object(4, 21));
    assert!(cache.get(&[4; 32]).is_none(), "oversized object is never cached");
    let mut exact = Cache::new(10);
    exact.insert([5; 32], object(5, 10));
    assert!(exact.get(&[5; 32]).is_some(), "object equal to budget fits");
}

#[test]
fn repeated_lookup_is_served_from_cache() {
    block_on(async {
        let kv = KvHandle::new(Arc::new(MemoryStore::default()));
        let (receipt, manifest, _) = build(&kv, source(), &ranges(), &[], 1024).await.unwrap();
        let path = u64::from(manifest.ipv4.as_ref().unwrap().level) + 1;
        let ip: IpAddr = "0.0.0.0".parse().unwrap();
        let mut warm = Resolver::open(kv.clone(), &receipt, 1 << 20).await.unwrap();
        warm.lookup(ip).await.unwrap();
        assert_eq!(warm.cache_stats().hits, 0);
        warm.lookup(ip).await.unwrap();
        assert_eq!(warm.cache_stats().hits, path);
        let mut cold = Resolver::open(kv, &receipt, 0).await.unwrap();
        cold.lookup(ip).await.unwrap();
        cold.lookup(ip).await.unwrap();
        assert_eq!(cold.cache_stats().hits, 0);
    });
}

#[test]
fn core_country_flag_and_json_shape() {
    let us = Location::new("US", "CA").unwrap();
    assert!(us.core_country_supported());
    assert_eq!(
        us.json(),
        serde_json::json!({"country": "US", "subdivision": "CA", "core_country_supported": true})
    );
    assert_eq!(Location::new("XK", "").unwrap().json()["subdivision"], serde_json::Value::Null);
}

With these three tests and the overlap fix at L763, missed mutants in index.rs drop from 19 to 6. Of the six left, three are leaf-capacity arithmetic that only changes packing, two are equivalent mutants, and one is an eviction-accounting edge.

[aram356]

🔧 This assertion passes whether or not overlaps are rejected

No build can succeed at 140 bytes. After all node objects are written, build rejects any manifest larger than object_bytes (L291-294), and the manifest is several hundred bytes: a 64-character generation plus each 32-byte digest serialized as a JSON array. With this test's empty source() it is 400–500 bytes, and 877 bytes with three real source-file entries. Probing valid, non-overlapping input:

budget 140: Err("manifest exceeds target value budget")
budget 200: Err("manifest exceeds target value budget")
budget 300: Err("manifest exceeds target value budget")
budget 400: Err("manifest exceeds target value budget")
budget 500: Ok

If I delete the overlap ensure! in build_family (L211-214), all 10 tests still pass.

Fix (verified to fail once the overlap check is removed):

let err = build(&kv, source(), &ranges, &[], 1024).await.unwrap_err();
assert!(err.to_string().contains("overlapping"), "{err}");

The 139 assertion on the next line is still meaningful. malformed_records_with_matching_digest_are_rejected also asserts only is_err(); checking the message there too would stop it from passing for the wrong reason.

[aram356]

🔧 The provenance and coverage claims don't match the tree this PR merges

1. This fingerprint reproduces at 6e8bb0e1 but not at the PR head:

6e8bb0e1  70227e734eee4709c7072a45a50dcc00ea15bd1c947c82596025a309e9c2ddbf
c52bdaf3  e9c6bbc0c3e1323e195e1d1f44ebfa3832d24998d9a0b17c834bb3af37018088

The only difference is the refreshed Cargo.lock, which this recipe hashes. src/*.rs and Cargo.toml are byte-identical. Please state that measurements ran at 6e8bb0e1, and record the head fingerprint as a lockfile-only change. The release-binary hash on L10 was also built from the old lockfile.

2. L88 lists cache eviction as covered by the synthetic tests. It isn't; see the comment at index.rs:635.

3. L100 says the workflow base-branch filters exclude feature/pluggable-geo-providers. test.yml and format.yml use an unfiltered pull_request:, and this PR ran 18 checks. The limitation that still holds is the second half: no workflow builds this standalone crate.

4. The Base: line of the spec (2026-11-05-kv-geo-design.md L5) still names 72ceed7e…, but this PR's base is 414e52ae….

[aram356]

🤔 On a cache hit, the address family is never checked

The cache is keyed by digest alone, and this re-check covers envelope and level but not family. An uncached read goes through node_key(generation, family, hash), and Object::decode checks bytes[4] == family. A cache hit skips both, so warm and cold resolvers can disagree.

Probe: a manifest that is valid by digest, whose IPv6 root directory has a single child pointing at the IPv4 leaf's hash:

cache disabled  ::1 => Err(missing required index object)
warm cache      ::1 => Ok(Some(Location([85, 83, 0, 0, 0, 0])))   // "US", from the IPv4 leaf

Spec §7 requires warm, fresh and cache-disabled resolvers to produce the same outcomes. The builder never emits this shape, so triggering it needs crafted data.

Fix:

ensure!(
    object.bytes[4] == family
        && object.bounds(0).0 == reference.start
        && object.bounds(object.count - 1).1 == reference.end
        && u16::from_be_bytes(object.bytes[6..8].try_into()?) == reference.level,
    "cached child descriptor mismatch"
);

Keying Cache by (family, hash) would also work.

[aram356]

♻️ Second SHA-256 stack

Core uses sha2 = "0.11" (root Cargo.toml:113), so this crate compiles two copies each of sha2, digest, block-buffer, crypto-common and cpufeatures. Switching is a drop-in change: with sha2 = "0.11", all 10 tests pass, clippy -D warnings is clean, and the lockfile has a single sha2.

sha2 = "0.11"

[aram356]

🤔 load and ArtifactStore have no tests

The tests call locations() and blocks() on in-memory bytes, so load() and HashedReader never run. These mutants pass every test:

• HashedReader::read → Ok(0), so every file reads as empty
• HashedReader::finish → String::new(), so every source digest is identical

Those digests feed the generation ID and the manifest.source == imported.source check in verify. A regression here would silently merge different releases into one generation.

store.rs has no tests either. Flipping != → == on the immutable-overwrite check (store.rs:108) survives, and so does replacing ArtifactStore::path with Ok(Default::default()), which removes the path-traversal guard.

Suggestion:

• A load() test over a temp directory holding the three CSV files, asserting each recorded digest equals the file's SHA-256. std::env::temp_dir() is enough; no new dependency.
• A store.rs test covering: traversal key rejected, identical rewrite accepted, different rewrite rejected, missing key returns Ok(None).

[aram356]

⛏ I/O errors lose the path

A missing source directory prints only Error: No such file or directory (os error 2). If GeoLite2-City-Locations-en.csv is missing, load also silently switches to the Country filenames, so the error names neither the expected file nor the edition.

file: File::open(path).with_context(|| format!("open {}", path.display()))?,

[aram356]

⛏ oracle_mismatches is a constant

measure returns an error on the first mismatch (L118), so any report that gets written always shows 0 here. Either drop the field, or count mismatches and fail after the pass.

[aram356]

🌱 An interrupted write blocks every rerun

create_new followed by write_all leaves a partial file if the process stops mid-write. Every rerun then hits AlreadyExists, compares bytes and fails with "immutable artifact differs" until someone deletes the file by hand. Spec §8 expects rerunning the same inventory to converge. The README already disclaims crash safety, which is why this is only a seedling. To fix it, write to a sibling temp file and fs::hard_link it into place. hard_link fails if the target already exists, so the no-overwrite rule still holds, and reruns converge.

[dhruv8sh]

PR Review

Summary

Standalone, opt-in geo-KV prototype (own [workspace], no production impact) plus design/results docs. Lookup algorithm looks correct and is well validated; only non-blocking notes.

😃 Praise

• Lookups are verified against a plain containment search over the original, unmerged source networks (import.rs:224-236). XK is kept with a core_country_supported flag rather than widening or dropping it, and the lookup path uses only KvHandle with block_on tests.

Findings

Non-blocking

• 🤔 Generation check after write (src/main.rs:55) — inline
• 🤔 O(n) cache hit bookkeeping (src/index.rs:427) — inline
• ⛏ Silent edition fallback (src/import.rs:150) — inline
• ⛏ Manifest size unbounded on read (src/index.rs:466) — inline
• 🌱 Prototype not exercised by CI: its own [workspace] means root CI never builds or tests it, so it can rot as edgezero-core changes. Consider a cargo test --locked --manifest-path examples/geo-kv-prototype/Cargo.toml step or a follow-up issue.
• 📝 Results doc predates the toolchain bump: it records Rust 1.95.0 and base 72ceed7e; the repo is now on 1.98.1 and the measurements weren't rerun. Keep that caveat visible.

CI Status

• fmt: PASS
• clippy (prototype, -D warnings): PASS
• tests (prototype): PASS (10 passed, 0 failed)

[dhruv8sh]

🤔 Generation check runs after objects are written: index::build writes every object for the new generation before this check. On a mismatch the command fails but those objects stay in the output directory.

Fix: read and compare any existing receipt.json before calling index::build, or refuse up front when the generation differs.

[dhruv8sh]

🤔 O(n) cache bookkeeping on every hit: order.retain walks the whole queue. With 16 KiB objects and a 4 MiB cache (~256 entries) this cost lands inside the published repeated-cache p95.

Fix: use a recency counter / LRU structure, or note it in the results doc.

[dhruv8sh]

⛏ Silent edition fallback: if the City locations file is missing this assumes the Country edition, and the user then sees a file-not-found for a Country file, hiding the real cause.

Fix: check for both editions explicitly and fail clearly when neither is present.

[dhruv8sh]

⛏ Manifest size not bounded on read: the build side enforces bytes.len() <= object_bytes (line 291) but Resolver::open doesn't. Low impact since the digest is pinned.

Fix: ensure!(bytes.len() <= manifest.object_bytes) for symmetry.

[dhruv8sh]

😃 Objects are checked against a pinned digest and validated before use, cached objects are re-checked against the parent on every hit, and missing/corrupt objects are errors rather than misses — with tests covering it.

[prk-Jr]

PR Review

Summary

Re-review at e043f93e. The nine prototype files are byte-identical to c52bdaf3, where the previous CHANGES_REQUESTED review was posted, and none of its threads have replies. I re-checked the four blocking findings independently at this head, and all four still apply. No new code defects turned up.

😃 Praise

• A missing or corrupt index object is an error, never a silent miss. Objects are digest-pinned and fully validated before use or caching. The CSV oracle runs over the original, unmerged networks, so it is genuinely independent of the index.

Findings

Blocking (still open from earlier threads, re-checked at e043f93e)

• 🔧 Nothing in CI compiles this crate (thread): no file under .github/ or scripts/ references geo-kv-prototype, and all 18 checks on e043f93e pass without building it. The root pins still match today, so it builds with --locked, but the coupling remains unguarded. The existing app-demo tests job is a precedent for adding a job here.
• 🔧 Object cache effectively untested (thread): with Cache::insert changed to return immediately, all 10 tests still pass.
• 🔧 Overlap-rejection test can't fail (thread): with the build_family ordering/overlap check replaced by true, all 10 tests still pass. The 140-byte budget fails at the manifest-size check regardless.
• 🔧 Results doc provenance and coverage claims (thread): the doc files are unchanged since that comment, so its points still apply.

Non-blocking

• 📝 PR description names old revisions: it says the head is c52bdaf3, the base is 414e52ae, and "required current-head CI passed at c52bdaf3". The actual head is e043f93e and the actual base is 3a0e5eb4. CI does pass on the real head (18/18), so only the stated SHAs need updating.

The earlier non-blocking threads (cache-hit family check, IPv4-mapped input, generation ID, validation after publication, the second sha2 copy, and so on) also still apply. They are not repeated here.

CI Status

• PR checks: 18/18 success at e043f93e. None of them compile this crate.
• Prototype, run locally at e043f93e: cargo test --locked 10/10 on Rust 1.98.1 and 1.99.1. clippy --all-targets -D warnings and fmt --check both pass (1.99.1).
• Workspace gates were not re-run locally, since the PR touches only the standalone crate and docs.