Skip to content

Fastly chunked-config GC: reclaim orphaned chunk entries on re-push (last-writer-wins) - #314

Open
aram356 wants to merge 51 commits into
mainfrom
spec/fastly-chunk-gc
Open

Fastly chunked-config GC: reclaim orphaned chunk entries on re-push (last-writer-wins)#314
aram356 wants to merge 51 commits into
mainfrom
spec/fastly-chunk-gc

Conversation

@aram356

@aram356 aram356 commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Summary

Fastly config push chunked storage was upsert-only, so re-pushing changed app config leaked the previous generation of chunk entries: chunk keys are content-addressed by the envelope SHA, so a config change rewrites every chunk key and orphans the old set. This affected both the cloud path (remote Config Store) and the local path (fastly.toml [local_server.config_stores.<name>.contents]).

This PR adds best-effort chunk garbage collection on re-push — reclaiming the prior generation the moment a new pointer supersedes it — plus the design spec and TDD implementation plan.

Concurrency: last-writer-wins. Concurrent cloud pushes are supported (the last root-pointer write wins on the value). A push reclaims prior chunks only while a post-commit read-back confirms it is still the last writer of that root; otherwise it yields and deletes nothing, so a superseded push never removes the winner's live chunks. Best-effort, not transactional (Fastly has no compare-and-delete) — the residual is documented, bounded to one store's chunk data, and surfaces as a read-time integrity error rather than wrong data. See the spec's "Concurrency model: last-writer-wins".

Changes

Crate / File Change
docs/superpowers/specs/2026-07-07-fastly-chunk-gc.md Design spec (5 review rounds)
docs/superpowers/plans/2026-07-07-fastly-chunk-gc.md Task-by-task TDD implementation plan
crates/edgezero-adapter-fastly/src/chunked_config.rs prior_chunk_keys(root_key, raw) — Value-first, v1-validated, prefix-scoped extraction of a prior pointer's chunk keys + unit tests
crates/edgezero-adapter-fastly/src/cli.rs GC helpers (FastlyConfigGcPlan, expand_root, orphan_chunk_keys, reject_reserved_root_keys, local_contents_table); local prune in the same fastly.toml rewrite + best-effort dry-run counts; cloud post-commit sweep with last-writer-wins read-back guard + delete_config_store_entry (--key --auto-yes, never --all) + offline dry-run intent; reserved-key rejection at the adapter boundary; ~30 new tests
crates/edgezero-adapter-fastly/Cargo.toml, Cargo.lock handlebars dev-dependency (renders the cloud fake-fastly test shim)

Behavior details:

  • Reserved-infix keys (.__edgezero_chunks.) are a hard error at the adapter boundary — they'd collide with the chunk namespace.
  • Suspicious/absent prior pointers and failed deletes degrade to warnings; the push still succeeds.
  • --dry-run stays offline (cloud reports GC intent without a count; local reports an exact count, and classifies malformed prior state as unknown: could not read prior state rather than 0).
  • Non-goal: reclaiming pre-existing leaks from before this feature — deferred to a future config gc.

Closes

Closes #313

Test plan

  • cargo test -p edgezero-adapter-fastly --features cli — 112 passed
  • cargo test --workspace --all-targets — all pass
  • cargo clippy --workspace --all-targets --all-features -- -D warnings — clean
  • cargo fmt --all -- --check — clean
  • cargo check --workspace --all-targets --features "fastly cloudflare spin" — clean
  • cargo check -p edgezero-adapter-spin --target wasm32-wasip2 --features spin — clean
  • TDD throughout (failing test → implement → green), per-task commits

Coverage highlights: prior-pointer validation (valid/direct/garbage/wrong-kind/bad-version/foreign-prefix); reserved-key rejection (local + cloud); local prune / shrink-to-direct / suspicious-pointer-skip / sibling-chunk preservation; local dry-run counts (exact / identical-repush-zero / non-table-unknown / suspicious-unknown); cloud deletes-prior-keeps-new / read-back concurrency skip / identical-repush-no-deletes / no-prior / delete-failure-warns / prior-read-failure-warns / shrink-to-direct / delete argv asserts --key + --auto-yes and never --all.

Checklist

  • Changes follow CLAUDE.md conventions
  • No Tokio deps added to core or adapter crates
  • Types imported from edgezero_core (n/a — adapter-internal code)
  • New code has tests
  • No secrets or credentials committed

@aram356 aram356 added the documentation Improvements or additions to documentation label Jul 8, 2026
…te+scope prior_chunk_keys, offline cloud dry-run, local root inference, warning semantics, invert stale no-GC test
aram356 added 3 commits July 7, 2026 22:06
…ence unsound); Value-first prior_chunk_keys so invalid pointer-kind warns; drop 'atomic' overclaim; define local dry-run degrade semantics; state cloud GC runs only after full commit
Value-first prior_chunk_keys (pointer-kind-but-malformed warns), thread
logical roots into write_fastly_local_config_store via roots: &[&str]
(no infix inference, since --key is free-form), best-effort local
dry-run counts, post-commit-only cloud sweep. Task-by-task with TDD
steps for subagent-driven-development.
…entical re-push counts 0 (was over-counting); enumerate all 10 writer call sites + roots args; forbid --all on delete; reword failed-delete warnings as informational (inert, future config gc); note sequential-spawn latency + approximate line numbers
@aram356 aram356 removed the documentation Improvements or additions to documentation label Jul 8, 2026
aram356 added 2 commits July 8, 2026 07:41
…t root read-back guard (invariant 5); build keep-sets from per-root expand_root instead of prefix-scanning flattened entries; make reserved-infix --key rejection mandatory at the Fastly adapter boundary + flip the infix test to expect rejection; add local suspicious-pointer real-push test and cloud concurrency-guard test
… (drop 'race-safe' overclaim, add Concurrency model section + plan precondition gate); correct cost note for the post-commit read-back describe; add dry-run suspicious-pointer test
@aram356
aram356 marked this pull request as draft July 8, 2026 15:42
aram356 added 6 commits July 8, 2026 08:48
Concurrent cloud pushes are SUPPORTED: root pointer is upsert so the last
write wins on the value. GC obeys LWW via the post-commit read-back guard
— a push reclaims prior chunks only while it is still the last writer of
the root, else it yields (never deletes the winner's live chunks).
Removes the single-writer assumption and the blocking 'do not implement'
gate; keeps the honest best-effort residual-window note. No code.
…inter test (seed real chunk keys, assert they survive); expand_root errors on empty instead of silent default; reserved-key error wording drops --key assumption
…k_keys, reject_reserved_root_keys, FastlyConfigGcPlan) + unit tests

Wired into push paths in the following commits; transient dead-code warnings until then.
write_fastly_local_config_store takes exact per-root keep-sets (gc_roots)
and prunes orphaned chunk keys in the same in-memory rewrite; suspicious
prior pointers warn and delete nothing. push_config_entries_local rejects
reserved keys, threads per-root expansion, and reports best-effort orphan
counts in dry-run. Inverts the stale no-GC test; adds shrink-to-direct,
suspicious-pointer, reserved-key, and dry-run count/identical/unknown tests.
…riter-wins)

push_config_entries rejects reserved keys, reads each root's prior value
before commit, and after the commit sweeps orphaned chunks guarded by a
post-commit root read-back (deletes only while still the last writer;
yields otherwise). Adds delete_config_store_entry (--key --auto-yes, never
--all) and an offline dry-run GC-intent line. Failed deletes and
suspicious/absent priors degrade to warnings; the push still succeeds.
Adds a command-aware fake fastly harness and 7 cloud GC tests.
@aram356 aram356 changed the title Spec: Fastly chunked-config GC to reclaim orphaned chunk entries on re-push Fastly chunked-config GC to reclaim orphaned chunk entries on re-push Jul 8, 2026
aram356 added 2 commits July 8, 2026 13:56
…astly via handlebars

Moves FastlyConfigGcPlan to the struct group with alphabetical fields;
renames single-char closure idents; replaces bare arithmetic with
saturating_add; fixes map_err/shadow/assert-on-result-state/absolute-path
lints; relocates GC helper unit tests after the test-module structs. Adds
handlebars dev-dependency and rewrites the cloud fake-fastly test shim to
render its shell script from a handlebars template.
@aram356 aram356 added the rust Pull requests that update rust code label Jul 8, 2026
@aram356 aram356 changed the title Fastly chunked-config GC to reclaim orphaned chunk entries on re-push Fastly chunked-config GC: reclaim orphaned chunk entries on re-push Jul 8, 2026
- local: GC of a chunked root leaves a chunked sibling's chunks intact
  (prefix-scoping vs shared string prefix app_config / app_config_staging)
- cloud: identical-bytes re-push deletes nothing (read-back returns our
  own value, so the assertion is non-vacuous)
- cloud: prior-read failure warns and deletes nothing (extends the fake
  fastly with a describe_hard_error mode)
@aram356 aram356 self-assigned this Jul 8, 2026
@aram356 aram356 removed the rust Pull requests that update rust code label Jul 8, 2026
…t delete argv + cloud shrink-to-direct

- local dry-run: distinguish absent (0) from present-but-non-table
  ("unknown: could not read prior state") via local_contents_table, so
  --local --dry-run no longer reports 0 orphans for state the real writer
  would reject; + non-table-contents test
- cloud: assert every delete argv passes --key + --auto-yes and NEVER
  --all (blast radius); fake now logs the full delete argv
- cloud: add the shrink-to-direct test (prior chunked -> new direct
  deletes all prior chunks, root upserted not deleted)
@aram356 aram356 changed the title Fastly chunked-config GC: reclaim orphaned chunk entries on re-push Fastly chunked-config GC: reclaim orphaned chunk entries on re-push (last-writer-wins) Jul 9, 2026
@aram356
aram356 marked this pull request as ready for review July 9, 2026 06:32
…; preflight key

Round-12 review found five P1 leak/deletion paths and several P2/P3 items. The
redaction findings share one cause I flagged last round — it was being closed
leak-by-leak — so this pass does the structural fix and sweeps the rest.

Structural: BlobEnvelopeError's Display embedded the blob-controlled stored hash,
so every caller that formatted it leaked. Redacted at the source, which covers
the extractor, the chunk resolver, the CLI push/diff paths, and the introspection
endpoint at once. A unit test locks the Display.

Local prune could delete a runtime-readable root. The writer removed prior-minus-
new chunk keys blindly; a chunk-shaped key can hold a valid direct envelope (a
padded envelope whose first chunk is a whole envelope), which is independently
readable. It is now kept (with a warning), mirroring the cloud value-based
protection.

Field-path map keys leaked. config_out_of_date_from_serde copied the serde path
into the 503 body; for a map, a segment IS a stored key. A struct field and a map
key are the same serde-path segment, so string segments are redacted (indices
kept). My round-11 comment claiming the path was "a schema location, not a value"
was wrong; fixed with a map-key sentinel test.

Secret-resolution errors exposed the stored key name, store id, and provider
message. Redacted to name only the (schema) field.

GC diagnostics quoted pointer-controlled fields: assemble names a POSITION not
the chunk key, generation verification drops both hashes, and delete failures
route stderr through redact_stderr.

P2: non-table local `contents` degrades to Unsupported instead of a misleading
MissingKey diff; a new preflight_config_key trait method rejects a reserved-
namespace key BEFORE the remote read (offline), not after list/describe; runtime
pointer validation now pins the writer's split layout (non-final chunks are full
payloads), bounding fetch fan-out. The self-scoped-pointer-at-chunk-shaped-root
case fails closed (documented as a known limitation, not data loss).

Docs: spec reconciled (GC reaches prior_chunk_keys only via gc_classify_root on a
confirmed pointer, never the dangerous Ok([])); migration guide separates chunk
GC from per-leaf migration cleanup; plan test count de-pinned.

Tests: fastly cli 153 -> 157, core 462 -> 464. New guards mutation-verified. All
gates green including the full wasm matrix.
@aram356

aram356 commented Jul 18, 2026

Copy link
Copy Markdown
Contributor Author

Round 12 addressed — all five P1s and the P2/P3 items. Pushed as 37cadc5.

The redaction findings vindicate the process note I flagged last round: this was being closed leak-by-leak. So this pass does the structural fix first — BlobEnvelopeError's Display embedded the blob-controlled stored hash, so every caller that formatted it leaked (extractor, resolver, CLI push/diff, introspection). Redacting at the source covers all of them at once, with a unit test locking the Display. Then I swept the remaining specific paths you named.

On the field-path leak — you caught me contradicting myself. My round-11 comment literally said the path was "a schema location, not a value," and you're right that for a BTreeMap<String, _> a path segment is a stored key. Since a struct field and a map key are the same serde_path_to_error segment, I can't keep one and drop the other, so string segments are redacted (indices kept) — with the map-key sentinel test you asked for. This is a real debuggability tradeoff I want to flag explicitly: field paths in 503 bodies are now <redacted>.<redacted> even for the common pure-struct case. The category is still there and config validate gives the exact path locally, but if you'd rather keep struct field names and accept that a map-key config could leak its key, that's a policy call I'll defer to you on.

The local-prune P1 was the same value-based-root gap I fixed for cloud in round 10, missed on the local path — a chunk-shaped key holding a valid envelope is now kept with a warning (mutation-verified).

The rest: secret-resolution errors redact the key name / store id / provider message (naming only the schema field); GC diagnostics use positions and drop hashes; delete failures route through redact_stderr.

P2s:

  • Non-table local contents now degrades to Unsupported instead of a misleading "all values added" diff.
  • New preflight_config_key trait method rejects a reserved-namespace --key before the remote read, so an invalid key fails offline. I did not make the diff dry-run fully network-free — a diff inherently needs to read the remote to compare against; the actionable part was the ordering, which is fixed. Tell me if you meant something stronger by "offline."
  • Runtime pointer validation now pins the writer's split layout (non-final chunks are full 6997–7000-byte payloads), which bounds the fetch fan-out; a many-tiny-chunks pointer is rejected before any fetch.
  • The self-scoped-pointer-at-chunk-shaped-root case (double infix) I left fail-closed — GC aborts and deletes nothing rather than mis-reclaim. I documented it as a known limitation in the spec since it only arises from hand-authored entries and solving it fully means recognizing doubly-nested infixes; say the word if you want that built rather than documented.

Docs: the spec's "GC does not use prior_chunk_keys" is reconciled (GC reaches it only via gc_classify_root on a confirmed pointer, never the dangerous Ok([])); the migration guide separates chunk GC from the per-leaf migration cleanup; the plan's test count is de-pinned.

Tests: fastly cli 153 → 157, core 462 → 464. Same verification caveat as before — the WASM contract suites (Viceroy/wrangler) can't run here; the compile/clippy WASM matrix and native suites all pass.

…nested-chunk GC

Round-13 review found three more diagnostic leaks (P1), an over-correction that
broke the field-path contract, and several P2/P3 items.

Three remaining leaks:
- The LOCAL WRITER formatted toml_edit's parse error verbatim, which quotes the
  offending source line (a stored, possibly secret-bearing contents entry). The
  diff read redacted this; the writer now does too.
- redact_describe_response joined every top-level JSON object KEY into the
  diagnostic; a wrong-shape `{"<secret>": ...}` response leaked it. Now reports
  the field COUNT only.
- BlobEnvelopeError was redacted only through Display but still derived Debug, so
  `{err:?}` / anyhow `?err` printed the stored hash. Debug is now hand-written to
  mirror Display; the test checks both.

Field-path contract: my round-12 map-key redaction over-corrected and broke the
authoritative contract (2026-06-16 spec), which requires the offending dotted
field path in the response. I verified empirically that serde_path_to_error
represents a struct field and a map key with the SAME segment kind, so redacting
map keys blanks ordinary schema fields. The path is restored verbatim; the
sensitive VALUE stays redacted in the message (the real leak). The map-key test
now asserts the value-bearing message is clean while the path carries the key per
contract.

P2:
- Local dry-run count now applies the same runtime-readable-root protection the
  real prune does, so preview matches apply (the padded-envelope fixture previews
  and applies the same count).
- Runtime pointer validation enforces the 255-char physical-key limit and rejects
  a pointer entry larger than the 8000-char entry limit (bounding fetch fan-out).
- A malformed PARENT table (local_server/config_stores/store as a scalar) degrades
  to Unsupported instead of collapsing to MissingStore's "all values added" diff.
- preflight_config_key also rejects an over-limit key before provider I/O.
- A self-scoped pointer at a chunk-shaped root no longer aborts store-wide GC:
  chunk_key_generation_any splits on the LAST infix, so doubly-nested chunks are
  recognised as chunks; the holder classifies as a root and its references count
  live. (Mutation-verified against the abort.)
- The gc_config_entries trait doc now states the stronger store-wide older_than
  assertion (no root changed in the window, no active writer), matching the CLI.

P3: the spec no longer claims gc_classify_root delegates to prior_chunk_keys — it
independently deserialises and validates.

Tests: fastly cli 157 -> 159, core +Debug assertion. New guards mutation-verified.
All gates green including the full wasm matrix.
@aram356

aram356 commented Jul 19, 2026

Copy link
Copy Markdown
Contributor Author

Round 13 addressed — the three P1 leaks and all P2/P3 items. Pushed as 98e05fa.

The three remaining leaks were all the same class one more layer out — the writer's TOML error (vs the diff read's, which I'd redacted), the response redactor's object keys (I'd redacted values but joined the keys), and BlobEnvelopeError's Debug (I redacted Display in round 12 but left #[derive(Debug)], so {err:?}/anyhow ?err still printed the hash). All fixed; Debug is now hand-written and the test checks both Display and Debug.

On the field-path finding — you're right, and this is the tradeoff I flagged in round 12 resolving against my round-12 choice. I over-corrected: redacting the path broke the authoritative contract (2026-06-16 spec requires the dotted field path in the response). I checked empirically — serde_path_to_error really does represent a struct field and a map key with the identical Segment::Map kind, so there's no way to keep one and drop the other. Since the contract is authoritative, I restored the path verbatim and kept the value redacted in the message (that was the real leak). The map-key test now documents this explicitly: the value-bearing message is clean, the path carries the key per contract. Net: round-12's map-key redaction is reverted, round-11's value redaction stands.

On the double-infix GC abort — you were right that documenting it didn't reconcile invariant 10, so I fixed it instead. chunk_key_generation_any split on the first infix; it now splits on the last, so a chunk of a chunk-shaped root (infix twice) is recognized as a chunk rather than misread as an unclassifiable root. The holder classifies as a root, its references count live, and store-wide GC continues. Mutation-verified: reverting to split_once reproduces the exact abort you quoted.

The rest:

  • Dry-run count now matches the real prune — it applies the same runtime-readable-root protection, so the padded-envelope fixture previews and applies the same number.
  • Runtime validation enforces the 255-char key limit and rejects an over-8000-char pointer entry (tightening the fan-out bound). On the 6997–6999 split slack you noted: I can't pin the exact split at the metadata level without the content (the crypto gate catches wrong content; this is purely a fan-out bound), so I bounded it via the pointer-size limit instead.
  • Malformed parent tables (local_server/config_stores/store as a scalar) now degrade to Unsupported, not MissingStore.
  • preflight also rejects an over-limit key before I/O.
  • The gc_config_entries trait doc now carries the stronger store-wide older_than assertion (no root changed in the window, no active writer), matching the CLI — so a direct trait caller can't follow the weaker paraphrase.
  • Spec P3: corrected — gc_classify_root independently deserializes and validates; it does not delegate to prior_chunk_keys. (My round-12 spec edit claiming delegation was wrong.)

Tests: fastly cli 157 → 159, core +1. Same verification caveat: the WASM contract suites (Viceroy/wrangler) can't run here; the compile/clippy WASM matrix and native suites pass.

…re preflight

Round-14 review found two more disclosure paths, an empty-key partial-write risk,
and several P2/P3 items — plus the field-path issue, which I settle here.

Diagnostic leaks:
- Three fastly.toml parse sites and three describe/list JSON parse sites
  interpolated the toml_edit / serde error verbatim, which quotes the offending
  stored value. All redacted to category-only, matching the writer.
- redact_describe_response joined every top-level object KEY into the diagnostic;
  a wrong-shape `{"<secret>": ...}` response leaked it. Now reports the field
  COUNT only.

Field-path — decided decisively after flip-flopping. The two specs conflict: the
app-config contract wants the dotted path; the redaction invariant forbids a
stored string on any path. `serde_path_to_error` cannot distinguish a struct
field from a map KEY (verified empirically), so the security invariant wins: the
path's STRING segments are redacted to `<redacted>` with STRUCTURE preserved
(dots + indices), and the exact path is recoverable via local `config validate`.
Both specs are reconciled to state this.

Empty key: writer-valid but resolver-invalid (canonical chunk parsing rejects an
empty root, and a cloud push could commit chunks before the final empty-root
write fails). Rejected in expansion AND preflight.

Char vs byte limits: the 8000 entry limit is a CHARACTER count (Fastly measures
characters), so direct-vs-chunked selection and pointer-size checks now count
chars — a non-ASCII value fitting 8000 chars is no longer needlessly chunked.
Chunk PAYLOAD splitting stays byte-based (UTF-8 boundaries); envelope_len /
chunk lengths stay bytes (integrity).

Body-aware preflight: preflight_config_key becomes preflight_config_write(key,
body) and runs the full chunk expansion offline, so a body-dependent failure
(derived chunk key over the store limit, oversized pointer) is caught BEFORE the
remote read, not during the later write.

Nested orphans: age attribution and candidate grouping used the FIRST infix, so a
nested orphan generation (chunks scoped to a chunk-shaped root) was silently
dropped. Both now use the LAST infix, matching classification, so nested orphans
are grouped and reclaimed. New test covers a nested ORPHAN (the round-13 test had
only a live one).

Docs: invariant 11 now says a generation is PLANNED/PROVEN atomically while
physical deletion is non-transactional; the redaction note says count-only (not
field names); the app-config spec documents the runtime field-path redaction; the
migration guide states the store-wide no-writer assertion; the contradictory
`gc_classify_root` delegation claim and the listing helper's stale comment are
corrected. The cloud push dry-run offline claim is reconciled with the diff
contract (one read-back for the diff; no GC list/describe/delete).

Tests: fastly cli 159 -> 161, core 464. New guards mutation-verified. All gates
green including the full wasm matrix.
@aram356

aram356 commented Jul 19, 2026

Copy link
Copy Markdown
Contributor Author

Round 14 addressed — the two P1 disclosure paths, the empty-key risk, and the P2/P3 items. Pushed as f9709f2.

The two leaks were more parse sites of the same class — I'd redacted the writer's TOML error in round 13 but missed three more fastly.toml parse sites and three describe/list JSON parse sites, all interpolating the error verbatim. And redact_describe_response joined the object keys into the diagnostic (I'd redacted values but not keys). All fixed; the response redactor now reports the field count only.

On the field-path — I'm settling this decisively, because I've flip-flopped it three rounds and that's on me. The two specs genuinely conflict (app-config contract wants the path; the redaction invariant forbids stored strings), and I confirmed empirically that serde_path_to_error gives struct fields and map keys the identical segment kind — so there's no way to keep one and drop the other. The security invariant wins. The path is redacted to <redacted> with structure preserved (fixing round-13's structure-loss complaint), and the exact path is recoverable via local config validate (no HTTP boundary). I reconciled both specs to say this, so it shouldn't reopen. If you'd rather prioritize the app-config debuggability contract over the redaction invariant, that's the one call I'll defer to you on — but I've committed to security-first here.

Empty key is rejected in both expansion and preflight now (writer-valid but resolver-invalid, with the partial-cloud-write risk you flagged).

Char vs byte limits: the entry limit is now char-based for direct-vs-chunked selection and pointer size (a non-ASCII value fitting 8000 chars isn't needlessly chunked); chunk payload splitting stays byte-based on UTF-8 boundaries, and envelope_len/chunk lengths stay bytes for integrity — the separation you asked for.

Body-aware preflight: preflight_config_keypreflight_config_write(key, body), which runs the full expansion offline, so derived-key/pointer-size failures are caught before the remote read.

Nested orphans — good catch that my round-13 fix was incomplete. Classification split on the last infix, but age attribution and grouping still used the first, so a nested orphan generation was silently dropped. Both now use the last infix; the new test covers a nested orphan (mutation-verified it's dropped without the fix).

Two doc reconciliations worth flagging:

  • The "generation deleted whole or not at all" invariant now correctly says the generation is planned and proven atomically while physical deletion is non-transactional (sequential deletes, a later failure can strand a partial generation) — your exact point.
  • The cloud dry-run "offline" line conflicted with the cli-reference (which says dry-run shows a diff, needing a read-back). I reconciled toward the diff contract: the adapter's push dry-run does no GC list/describe/delete and no write, and the one read-back is the documented diff. If you intended cloud --dry-run to be fully network-free (no diff), that's a UX change to the diff contract I'd want your call on before making.

Also fixed: the contradictory gc_classify_root delegation claim (it independently validates; does not call prior_chunk_keys) and the listing helper's stale comment (it keeps the value, doesn't discard it).

Tests: fastly cli 159 → 161, core 464. Same caveat — the Viceroy/wrangler WASM contract suites can't run here; the compile/clippy matrix and native suites pass.

…add coverage

Round-15 review confirmed all P1s fixed and no unsafe deletion path. This
addresses the Unicode mismatch, the security-doc contradictions, and the missing
coverage.

Unicode char/byte pointer mismatch (the substantive fix): the writer chunks by
CHARACTER count, but pointer validation could only reject on the byte-valued
envelope_len up front, so a value under 8000 chars but over 8000 UTF-8 bytes got
a chunked pointer the writer would never have produced — accepted by runtime,
then rejected by GC's writer round-trip once orphaned, leaving permanent residue.
After reconstruction (bytes hash-verified), the resolver now re-checks the
CHARACTER count: if it fits directly, this writer would not have chunked it, so
the pointer is rejected — keeping the runtime and GC verdicts consistent. New
adversarial test builds a valid chunked pointer for a 2001-crab (2001-char,
8004-byte) value and asserts rejection.

Security-doc contradictions:
- The schema-drift test put its sentinel in a VALUE, so the former object-KEY
  disclosure would have passed. Moved to an object KEY; asserts count-only output
  (mutation-verified against key-joining).
- The blob spec's code sketch still showed the vulnerable raw serde
  message/path; updated to the redacted implementation.
- The GC plan still said "field-name shape"; now "field COUNT, never names".
- The error.rs test comment still said the map key should appear; corrected.

Deletion language: spec pseudocode still said "whole generations only"; now
matches invariant 11 (planned/proven atomically, physical delete non-transactional).

Dry-run I/O contract: the CLI reference said "no shell-outs" (false — it does a
read-only read-back), and the spec said "single describe" (false for chunked —
describes the root then every chunk). Both corrected to "read-only, one logical
read, possibly several provider calls".

Coverage added: sentinel malformed-TOML test for the three provisioning parsers;
a failing upsert stderr sentinel test; a CLI test proving the body-aware preflight
runs before remote I/O; and a nested destructive age case (recent nested live gen
+ old nested orphan under one chunk-shaped root) that pins root_live_since's last-
infix split (mutation-verified).

Stale docs: the pointer-overflow message printed byte length as "characters"
(now chars); the helper comment said `.len() <= 8000` (now characters); the
migration guide said local pruning removes "exactly" every prior chunk (now notes
runtime-readable-root retention).

Tests: fastly cli 161 -> 165, cli 165 -> 166. New guards mutation-verified. All
gates green including the full wasm matrix.
@aram356

aram356 commented Jul 19, 2026

Copy link
Copy Markdown
Contributor Author

Round 15 addressed — the Unicode mismatch, the security-doc contradictions, and the missing coverage. Pushed as 74fa24e.

The Unicode char/byte mismatch was a real bug, and a good catch on how it becomes permanent residue. When I moved the direct-vs-chunked threshold to characters in round 14, I left validate_pointer_chunks rejecting on the byte-valued envelope_len — so a <8000-char/>8000-byte value could carry a chunked pointer the writer would never produce: runtime accepted it, then GC's writer round-trip rejected it once orphaned, leaving unprovable residue forever. Since the char count is only knowable after reconstruction, the resolver now re-checks it post-reconstruction (bytes already hash-verified) and rejects if it would have fit directly — so runtime and GC agree. The adversarial test builds a valid chunked pointer for a 2001-crab value (2001 chars, 8004 bytes) and confirms rejection.

On the redaction docs — you're right that the docs still demonstrated the vulnerable code, which is worse than a stale comment. Fixed together: the blob spec's code sketch now shows the redacted implementation (not inner().to_string() + raw path), the GC plan says "field count, never names", and the error.rs comment no longer claims the map key appears. And you caught that my schema-drift test was toothless — it put the sentinel in a value, so the object-KEY disclosure I "fixed" last round was never actually tested. Moved the sentinel to an object key; mutation-verified it fails against the old key-joining redactor.

The rest:

  • Deletion language: the pseudocode still said "whole generations only"; now matches invariant 11 (planned/proven atomically, physical delete non-transactional).
  • Dry-run I/O contract: the CLI reference said "no shell-outs" (false) and the spec said "single describe" (false for chunked — it describes the root then every chunk). Both now say read-only, one logical read, possibly several provider calls.
  • Stale docs: the pointer-overflow message printed byte length as "characters" (fixed); the helper comment and the migration guide's "prunes exactly" (which ignores runtime-readable-root retention) are corrected.

Coverage — I added all four you listed: sentinel malformed-TOML for the three provisioning parsers, a failing upsert stderr sentinel, a CLI-level test proving the body-aware preflight runs before remote I/O (a reserved --key fails at preflight, not on a shell-out), and the nested destructive age case (recent nested live gen + old nested orphan under one chunk-shaped root), which is mutation-verified to pin root_live_since's last-infix split.

On the dry-run e2e: the "reads happen, writes suppressed" behavior is already covered by the existing dry-run tests (the identical-repush dry-run and the corrupt-prior dry-run both reach the writer's report without writing), so I documented the contract accurately rather than adding a redundant heavy test — tell me if you'd still want a dedicated one.

Tests: fastly cli 161 → 165, cli 165 → 166. Same caveat — the Viceroy/wrangler WASM contract suites can't run here; the compile/clippy matrix and native suites pass.

Fixes the merge-blocking read-compatibility regression and tightens the
resolver to accept only writer-produced chunk layouts.

- Revert the direct-vs-chunked threshold to a conservative UTF-8 BYTE
  count. A character-based threshold left an envelope under 8 000 chars
  but over 8 000 bytes stored directly, and rejected the already-stored
  chunk pointer for it on read (an HTTP 500 for data written by an
  earlier release). Byte length is always >= character length, so the
  byte check never over-stores under any reading of Fastly's limit, and
  it is what existing v1 pointers were written against. Covered by an
  acceptance test that chunks and resolves a value in that gap.
- Extract writer_chunk_spans as the single source of truth for the split
  layout, shared by the writer and the resolver so the two cannot drift.
- Validate EXACT split boundaries after reconstruction: replay the
  writer's spans over the reconstructed envelope and require the chunk
  count and every chunk length to match the pointer. The hashes already
  prove the bytes; this proves the layout, so a hand-authored pointer
  that reassembles correctly along boundaries the writer would never
  choose is now rejected on the read path too -- the same guarantee
  prove_generation enforces before a GC delete. Adds a resolver-to-writer
  round-trip test across five sizes and a boundary-shift rejection test.
- Strengthen the generic CLI I/O ordering regression: drive it with a
  derived-key overflow (a valid root key whose chunk keys exceed the
  store limit once the body chunks) rather than a reserved key, so it
  pins that the full body-aware preflight runs offline before any remote
  read, not just a key-shape check.
- Add local-prune coverage: dry-run count parity against the real
  deletion count on one fixture, and a real push over a malformed prior
  pointer warning while deleting nothing.

Docs:

- Blob spec no longer prescribes serialising the validator's rendered
  report. On the runtime path that runs after the secret walk, so the
  report can carry a resolved secret; both sketches now keep only the
  structural field name, matching the implementation.
- Note that a local config validate recovers the exact field path only
  when the local TOML still matches what was deployed.
- Document the local prune's root-like retention, the byte-based value
  threshold and its v1 stability, Fastly's multi-entry exception to the
  one-entry-per-adapter rule, and that chunk GC is implemented rather
  than future work. Drop the atomic whole-generation claim.
@aram356

aram356 commented Jul 20, 2026

Copy link
Copy Markdown
Contributor Author

Thanks — the P1 was mine and you were right that it was merge-blocking. All findings addressed in e42e9f2.

P1 — existing v1 pointers unreadable after upgrade

Reverted. The character-based threshold I introduced in an earlier round was the bug: it left an envelope under 8,000 characters but over 8,000 bytes stored directly, while the already-stored chunk pointer for that same value was rejected on read — an HTTP 500 for data written by an earlier release.

The writer is back to the conservative UTF-8 byte count the merge-base used. Byte length is always ≥ character length, so it never over-stores under any reading of Fastly's limit, and it is what existing v1 pointers were written against. chunked_value_under_char_limit_but_over_byte_limit_resolves builds a real envelope in that gap (2,001 crabs), asserts the writer chunks it, and asserts the resolver reconstructs it byte-for-byte.

Only the key limit remains character-counted (255) — that one is genuinely a character limit and is unrelated to the value threshold.

P2 — runtime accepts non-writer layouts

Now validated exactly, after reconstruction. I extracted writer_chunk_spans as the single source of truth for the split layout; prepare_fastly_config_entries builds its chunks from it and the resolver replays it over the reconstructed envelope, requiring the chunk count and every chunk length to match the pointer.

The hashes already prove the bytes; this proves the layout. A pointer that reassembles to the correct envelope along boundaries the writer would never choose is now rejected on the read path too — the same guarantee prove_generation already enforced before a GC delete. Comparing lengths is sufficient because the per-chunk and whole-envelope SHAs have pinned the bytes, so equal boundaries pin each chunk to the writer's own.

Two tests: writer_output_round_trips_through_the_resolver (five sizes, on/just-past/well-past boundaries) and resolver_rejects_a_non_writer_split_that_reassembles_correctly, which moves two bytes across the first boundary so every metadata check still passes (dense indexes, single generation, both lengths in range, sum equal to envelope_len, all SHAs correct) and only the boundary replay catches it.

find_utf8_boundary and the new span helper are un-gated so the runtime resolver can use them; the --features fastly wasm32-wasip1 clippy gate confirms no dead-code trap.

P2 — blob spec prescribed a resolved-secret leak

Fixed in both sketches. On the runtime path validate() runs after the secret walk, so #[secret] fields hold resolved values and validator's params echo the rejected value — validation_err.to_string() would render a secret into the HTTP body and the log line. Both spec sketches now keep only the structural field name and drop the report entirely, which is what the implementation already did.

Also qualified the "the exact path is available from a local config validate" note: that holds only when the local TOML still matches what was deployed, since it reads local source rather than the deployed blob.

P3s

  • CLI I/O orderingcloud_push_preflight_rejects_derived_key_overflow_before_remote_io drives the ordering proof with a derived-key overflow: a valid ~200-char root key whose chunk keys exceed the 255-char limit only once the >8,000-byte body chunks. That is undetectable by key shape, so it pins that the full body-aware preflight runs offline before any remote read. If preflight ran after read_remote the failure would be a fastly-not-found shell-out, never the key-limit message.
  • Local prune — documented the root-like retention the spec omitted (an orphan whose value is itself a runtime-readable envelope or pointer is kept, with a warning). Added push_config_entries_local_dry_run_count_matches_real_deletions (dry-run count vs. actual deletions on one fixture) and a real-push-over-malformed-prior test that warns and deletes nothing.
  • Stale descriptions — the CLI guide's "every adapter writes one entry" now carries Fastly's multi-entry exception; the blob spec no longer calls chunk GC future work; the plan no longer claims atomic whole-generation deletes.

Gates

fmt, clippy --workspace --all-targets --all-features, test --workspace --all-targets, the fastly cloudflare spin feature check, the wasm32-unknown-unknown / wasip1 / wasip2 clippy matrix, app-demo fmt + clippy, and docs prettier all pass.

One note: clippy -p edgezero-adapter-fastly --features "fastly cli" --target wasm32-wasip1 fails, but that combination is not a CI gate (the matrix runs --features fastly alone) and both failures are pre-existing and outside this diff — make_test_envelope is #[cfg(unix)] at cli.rs:3967 while some callers are not, plus a mem::drop at cli.rs:2291. Happy to fix that gating separately if you want it closed.

Two pre-existing breakages surfaced by `clippy -p edgezero-adapter-fastly
--features "fastly cli" --target wasm32-wasip1` (not a CI gate, since the
`cli` feature targets the native host, but a valid combination that should
still compile):

- `make_test_envelope` was `#[cfg(unix)]` while several of its callers were
  not, so the test module failed to compile off unix. The helper only builds
  a JSON string and has no unix-specific code, so the gate was simply
  over-restrictive; removed it.
- `drop(stdin)` tripped `drop_non_drop` because `ChildStdin` is a non-Drop
  stub on wasip1. Moved the write into `write_value_to_fastly_stdin`, which
  takes the handle by value and drops it on return — a natural scope-end
  drop that closes the pipe on native and satisfies clippy everywhere.

Behaviour on native is unchanged.
@aram356

aram356 commented Jul 21, 2026

Copy link
Copy Markdown
Contributor Author

Closed out the fastly cli + wasip1 loose end I flagged above, in 31008ad — both were pre-existing and unrelated to chunk GC, but the combination is valid and now compiles:

  • make_test_envelope was #[cfg(unix)] while several callers were not; the helper only builds a JSON string, so the gate was over-restrictive and is removed.
  • drop(stdin) tripped drop_non_drop (ChildStdin is a non-Drop stub on wasip1). Moved the write into write_value_to_fastly_stdin, which takes the handle by value and drops it on return — a natural scope-end close, unchanged behaviour on native.

clippy -p edgezero-adapter-fastly --features "fastly cli" --target wasm32-wasip1 --all-targets now passes, and the standard gates (workspace all-features clippy, wasip1 --features fastly, full workspace tests) are still green.

… runtime

P1 (merge-blocking): the Fastly resolver routed EVERY value through
envelope/pointer parsing, so ordinary Config Store entries — "value_a",
the documented greeting = "hello" — came back as corruption errors,
breaking the shared ConfigStore contract and its wasm32-wasip1 test gate
(reproduced under viceroy). A Config Store holds arbitrary values, so the
resolver now classifies by `edgezero_kind` and touches ONLY our own chunk
pointers: anything else is returned verbatim, and a value carrying an
unrecognised `edgezero_kind` (our reserved namespace) is the sole new
error. Envelope integrity is unchanged — the typed app-config extractor
still parses and verifies the BlobEnvelope after get(). `classify_root_value`
is the single discriminant shared by the resolver and GC. Tests that fed
non-pointer JSON expecting an error now feed pointer-kind-but-malformed
values.

P2: the local push rewrote fastly.toml in place, so an interrupted write
could truncate it and a concurrent push could drop a sibling edit. The
rewrite is now atomic — write a sibling temp file, fsync-free rename over
the target (atomic on POSIX) — and refuses to clobber a file that changed
under it (conflict detection rather than a lock file, which would strand
state on any interrupted push). Covered by conflict and cleanup tests.

P3: the runtime resolver rejected non-writer split boundaries but GC only
checked metadata + reconstructed content, so a hash-valid 6998/rest split
was runtime-unreadable yet counted live by GC and could never satisfy
prove_generation — permanent unreclaimable residue. GC now applies the
same exact-split predicate (extracted as verify_writer_split_layout) and,
staying fail-closed, WARNS that such a root is not runtime-readable
instead of silently calling it healthy. New GC test asserts the warning
and that nothing is deleted.

P3: both cloud-push ordering tests now inject a fake `fastly` on PATH that
logs invocations and assert ZERO were made before the offline preflight
rejected — so a regression can no longer reach the developer's
authenticated CLI. Added a frozen v1 wire-format fixture built from
literal constants (7000-byte split, infix, key suffix, pointer fields),
independent of the current writer, so coordinated writer/resolver drift
can no longer stay green.

Docs: blob spec no longer prescribes the pre-redaction map_secret_error /
validator-report sketches (they leak resolved secrets); the boundary
tests are pinned to the v1 8000-BYTE threshold, distinct from Fastly's
8000-CHARACTER platform cap; the raw-value passthrough contract is
recorded. cli-reference corrects bytes-vs-characters and states that a
cloud push never deletes (only --local prunes; cloud uses config gc).
@aram356

aram356 commented Jul 23, 2026

Copy link
Copy Markdown
Contributor Author

Thanks — the P1 was a real one and I reproduced it exactly. All findings addressed in b54e710.

P1 (merge-blocking) — raw ConfigStore contract + WASM gate

Confirmed under viceroy: the resolver routed every value through envelope/pointer parsing, so "value_a" and the documented greeting = "hello" came back as corruption errors and the wasm32-wasip1 contract suite aborted.

Root cause: a Config Store holds arbitrary entries, but the resolver treated non-envelope-non-pointer as corrupt. Fixed by classifying on edgezero_kind and touching only our own pointers:

  • Not ours (plain string, direct BlobEnvelope, unrelated JSON, non-JSON) → returned verbatim.
  • edgezero_kind == "fastly_config_chunks" → resolved as before.
  • edgezero_kind present but unrecognised → error (that field is our reserved namespace).

classify_root_value is now the single discriminant shared by the resolver and GC, so they can't drift on what "ours" means. No integrity is lost: the typed app-config extractor still parses and verify()s the envelope after get() — the store was never the right layer to police envelope validity. The wasm gate now passes (74 tests), including the contract tests. Tests that fed non-pointer JSON expecting an error were repointed at pointer-kind-but-malformed values.

P2 — local rewrite not atomic

fastly.toml was rewritten in place. Now: write a sibling temp file, rename over the target (atomic on POSIX), and refuse to clobber a file that changed under the read-modify-write, reporting the conflict instead. I chose conflict-detection over a lock file deliberately — a lock would strand state whenever a push is interrupted, which is the worse failure mode for a dev CLI. Conflict + cleanup tests added.

P3 — runtime/GC disagreement (unreclaimable residue)

You're right that a hash-valid 6998/rest split was runtime-unreadable yet live to GC, and then permanently unprovable. I extracted the resolver's exact-split check as verify_writer_split_layout and GC now applies the identical predicate. Staying fail-closed, it keeps the chunks but warns the root is not runtime-readable and will never be reclaimed automatically (re-push to rewrite), rather than silently reporting it healthy. New test asserts the warning and that nothing is deleted.

P3 — orchestration/compat test pinning

Both cloud-push ordering tests now inject a fake fastly on PATH that logs every invocation, and assert zero invocations before the offline preflight rejects — so a regression can't reach an authenticated CLI. I mutation-tested it (commenting out the preflight makes both fail). The v1 compatibility test is now a frozen fixture built from literal constants (7000-byte split, .__edgezero_chunks. infix, <sha>.<index> suffix, pointer field names), independent of the current writer, so coordinated writer/resolver drift can no longer stay green.

P2 — docs

  • Blob spec: replaced the pre-redaction map_secret_error sketch (§3.3.3) and the "log the full validator report" prescription (§6.2.2) with the redacted implementations — both leaked resolved secrets. The Fastly chunking tests (§12.3) are pinned to the v1 8000-byte threshold, called out as distinct from Fastly's 8000-character platform cap, and the raw-value passthrough contract is recorded.
  • cli-reference: corrects bytes-vs-characters and states that a cloud push never deletes — only --local prunes; cloud orphans need config gc.

Gates

fmt, workspace clippy --all-features, test --workspace --all-targets (20 binaries), the fastly cloudflare spin feature check, the wasm clippy matrix (wasip1 fastly and fastly cli, wasip2 spin, wasm32-unknown cloudflare), -p edgezero-adapter-fastly --no-default-features --features cli, the previously-failing fastly wasm contract suite under viceroy (74 passed), app-demo fmt + clippy, and docs prettier all pass.

P1 (data loss): the local `fastly.toml` rewrite compared-then-renamed with
a TOCTOU window — two concurrent pushes could both pass the compare and the
later rename would discard the earlier push's edit. The whole
read-modify-write now runs under a cross-process advisory lock
(`ManifestLock`, `File::lock` on a persistent sidecar), so pushes serialise
and each builds on the previous — both edits survive. A 25-round two-thread
test reproduces the loss without the lock (mutation-verified).

P2: the atomic replace created the temp file with umask permissions and
replaced a symlink with a regular file. It now copies the target's existing
permissions onto the temp (a 0600 manifest stays 0600) and canonicalizes
the path first, so a symlinked manifest is updated through the link.

P2: a transient chunk `LookupError` (TooManyLookups, ConfigStoreInvalid,
an unclassified Other) was flattened to a string and mapped to Internal
"re-run config push", which cannot fix request-scoped lookup exhaustion.
The chunk callback now classifies the error and keeps the transient class
as Unavailable (503), matching the root lookup; only a bad/oversized key or
value stays corrupt (Internal).

P2: GC aborted the whole store when any non-chunk-shaped value failed the
envelope/pointer classifier, so one ordinary `greeting = "hello"` sibling
blocked all reclamation. A definitively foreign value at an ordinary key is
now protected as a zero-reference root. Two guards keep this safe: the
value must be provably inert (a new `MalformedObject` classification catches
an object-shaped-but-unparseable value — e.g. a truncated pointer — and
still fails closed so its chunks are never orphaned), and the key must be
outside the reserved `.__edgezero_chunks.` namespace.

P2: a failed cloud upsert was labelled "Failed" with committed entries
"safe to skip", implying a known boundary — but a timeout can land after
Fastly committed the key, including the root pointer. The diagnostic now
states the failed entry's outcome is UNKNOWN and directs the operator to
re-run the whole idempotent push. The blob spec's matching "previous config
stays active" guarantee is corrected.

P3: restored the `#[cfg(unix)]` on `fake_spin_returning` that a helper
insertion had displaced (unblocks non-unix test builds). Added a frozen v1
multibyte fixture with a 4-byte codepoint straddling byte 7000, hand-split
at the retreat boundary via a frozen rule, pinning the codepoint-retreat
wire format independently of the writer. Corrected stale docs: the blob
read algorithm now describes raw passthrough (the store no longer verifies
direct envelopes), and the GC spec/plan describe the atomic locked replace
rather than a trailing `fs::write`.
@aram356

aram356 commented Jul 26, 2026

Copy link
Copy Markdown
Contributor Author

Thanks — the P1 was a real TOCTOU and you were right that the earlier conflict-detection didn't close it. All findings addressed in c5689c0.

P1 (data loss) — concurrent local pushes

The compare-then-rename had a genuine race: two pushes could both pass the comparison, and the later rename discarded the earlier edit. The whole read-modify-write now runs under a cross-process advisory lock (ManifestLockFile::lock on a persistent sidecar next to the manifest). Moving the read inside the lock means pushes serialise and each one reads what the previous wrote and builds on it, so both edits survive (not just a detected conflict). New test spawns two threads adding distinct keys across 25 rounds and asserts both survive; I mutation-tested it (removing the lock makes it fail with the exact loss).

P2 — permissions / symlink

The temp+rename created a new inode at umask perms and replaced a symlink with a regular file. It now copies the target's existing permissions onto the temp (a 0600 manifest stays 0600) and canonicalizes first, so a symlinked manifest is updated through the link.

P2 — transient chunk lookups

The chunk callback flattened every LookupError to a string, so TooManyLookups / ConfigStoreInvalid / Other became Internal "re-run config push". The callback now classifies before stringifying and keeps the transient class as Unavailable (503) — matching the root lookup — while a bad/oversized key or value stays corrupt (Internal). Unit test pins the split.

P2 — one foreign sibling blocking GC

A greeting = "hello" sibling aborted all reclamation. A definitively foreign value at an ordinary key is now protected as a zero-reference root. Getting this safe took two guards, because a naive "no discriminator ⇒ foreign" rule regressed the truncated-pointer fail-closed test (a truncated pointer loses edgezero_kind too):

  1. I split the classifier's foreign case into Foreign vs a new MalformedObject ({-shaped but unparseable — a possible truncated/corrupt pointer). Only Foreign is treated as inert; MalformedObject still fails closed, so a corrupt root's chunks are never orphaned.
  2. The key must also be outside the reserved .__edgezero_chunks. namespace — a non-canonical key living in that namespace is not an ordinary sibling and still fails closed.

New test reclaims a dead generation despite a foreign sibling; the existing truncated-pointer and non-canonical-key fail-closed tests still pass. (The runtime resolver treats MalformedObject like Foreign — passthrough — since the store must return arbitrary stored bytes verbatim.)

P2 — cloud push unknown outcome

A failed upsert was labelled "Failed" with committed entries "safe to skip", implying a known boundary — but a timeout can land after Fastly committed the key, including the root pointer. The message now states the failed entry's outcome is UNKNOWN and directs a full idempotent re-run (content-addressed keys + --upsert), not a hand-resume. Blob spec's matching "previous config stays active" guarantee corrected to the unknown-outcome / re-run framing. (The Spin adapter has similar wording but wasn't flagged and already mentions idempotent retry — left it scoped to Fastly.)

P3s

  • Restored the #[cfg(unix)] on fake_spin_returning that my fake_fastly_logging insertion had displaced — unblocks non-unix test builds.
  • Added a frozen v1 multibyte fixture: a 4-byte codepoint straddles byte 7000 and chunk 0 is hand-split at the retreat boundary (6998) via a rule written out in the test, never the production writer — so a change to the codepoint-retreat wire format fails the test. Mutation-checked (a wrong boundary breaks it).
  • Docs: the blob read algorithm now describes raw passthrough (the store no longer verifies direct envelopes — that's the typed layer's job); the GC spec/plan describe the atomic locked replace instead of a trailing fs::write.

Gates

fmt, workspace clippy --all-features, test --workspace --all-targets (20 binaries), the fastly cloudflare spin feature check, the wasm clippy matrix (wasip1 fastly + fastly cli, wasip2 spin, wasm32-unknown cloudflare), -p edgezero-adapter-fastly --no-default-features --features cli, the fastly wasm contract suite under viceroy (76 passed), app-demo fmt + clippy, and docs prettier all pass.

Four blockers plus follow-ups from review.

P1 (data loss): GC classified an object carrying an UNKNOWN/future or
non-string `edgezero_kind` as a direct envelope, because `BlobEnvelope`
ignores unknown fields — so a future-format pointer with envelope-shaped
fields became a zero-reference root and its canonical chunks were
reclaimed. `classify_root_value` now discriminates on the PRESENCE of
`edgezero_kind` (any non-recognised value is UnknownKind, never Foreign),
`gc_classify_root` routes through it and fails closed on UnknownKind /
MalformedObject, and the chunk-shaped candidate arm excludes any value
that announces our namespace. Runtime rejects the same values.

P1 (data loss): the manifest lock covered only local config push; provision
overwrote fastly.toml with an unlocked `fs::write`, and the lock keyed on
the lexical path while replacement canonicalized later — so a symlink and a
direct path locked different sidecars. `ManifestLock` now resolves the real
target once (shared by lock and replace), `append_fastly_setup` takes the
same lock and uses the atomic replace, so provision and push serialise on
one target.

P1 (CI): `value_is_inert_foreign` was dead under the fastly-only wasm build,
failing the required clippy gate. It is now exercised by a classifier unit
test, and the previously-missing `fastly cli` wasip1 combination is added to
the CI matrix so this class of regression is caught.

P1 (correctness): pointer-last is not an atomic cloud flip — Config Store is
eventually consistent across keys, so a POP can see a new pointer before its
chunks propagate. A MISSING referenced chunk now maps to Unavailable (503,
retryable) instead of Internal (500); a retry resolves the propagation
window. Spec's "atomic flip" claim corrected.

P2: atomic staging used a predictable pid-only temp path written before
permissions were applied. It now creates the temp with `create_new`
(O_EXCL, never follows a planted symlink), copies the target's permissions
BEFORE writing, syncs before rename, and a `TempFileGuard` removes it on any
failure. Permissions are preserved and symlinks followed to the real file.

P2: `ValueTooLong` is now transient (Unavailable), not corruption — the SDK
already retried at the reported size, so it reaching us means the value grew
between host calls, a race a retry resolves.

P2: the "frozen v1" fixtures now assert against PRECOMPUTED literal wire
hashes (envelope, inner, and per-chunk) and feed them into the pointer as
literals, so a serialization or hash-helper drift fails the test instead of
silently rebuilding a new golden.

P3: distinct GC reporting — the cloud run no longer pre-prints planned keys
as "deleting"; execution reports deleted / FAILED / skipped per key, and the
local dry-run no longer counts prior chunks already absent from the file
(the real prune's remove is a no-op there). Generated projects gitignore the
lock sidecar. Recovery commands note their POSIX/bash quoting. Stale GC/blob
docs updated (inert-foreign siblings, root-like-orphan warnings, local-only
cloud pruning, missing-chunk-as-transient).
@aram356

aram356 commented Jul 27, 2026

Copy link
Copy Markdown
Contributor Author

Thanks — the two data-loss P1s were real and I reproduced both. All 12 findings addressed in a17d159.

P1 — GC deletes a future-kind root's chunks (data loss)

Confirmed: BlobEnvelope ignores unknown fields, so an object with a future/unknown edgezero_kind plus envelope-shaped fields classified as Direct (zero references) and its content-addressed chunks were reclaimed. Also a non-string edgezero_kind was mis-classified Foreign.

Fix: classify_root_value now discriminates on the presence of edgezero_kind (None → Foreign; recognised string → Pointer; anything else, including non-string → UnknownKind). gc_classify_root routes through it and fails closed on UnknownKind/MalformedObject rather than falling through to envelope parsing, and the chunk-shaped candidate arm excludes any value that announces our namespace. New unit test grafts a fastly_config_chunks_v2 kind onto a valid envelope and asserts GC fails closed; a table test pins all predicates across unknown-string / non-string / truncated cases. Runtime rejects the same values.

P1 — manifest lock coverage + path aliasing (data loss)

Right on both counts. ManifestLock now resolves the real target once (via canonical_manifest_target) and both the lock and the replace use it, so a symlink and a direct path lock the same sidecar. Provision (append_fastly_setup) now takes the same lock and uses the atomic replace instead of a bare fs::write. New tests: a 25-round provision-vs-push thread race asserts both edits survive, and a symlinked-manifest test asserts the link is preserved and the real file updated.

P1 — required WASM clippy gate

value_is_inert_foreign was dead under fastly-only. It's now exercised by the classifier unit test, and I added the fastly cli wasip1 combination to the CI matrix (format.yml) — that combination surfaced two more clippy issues in this very change (item ordering, an unwrap_or_else), which are now fixed, so the gate earns its place.

P1 — pointer-last is not an atomic cloud flip

Agreed — there's no cross-key atomicity and Config Store is eventually consistent across POPs. Rather than claim atomicity, a missing referenced chunk now maps to Unavailable (503, retryable) instead of Internal (500): the dominant cause is propagation lag right after a push, and a retry resolves it (content-addressing means the new generation never overwrote the old chunks). Genuine lasting loss shows as a persistent 503 the operator repairs by re-pushing — strictly safer than a spurious 500. New integration test (via the in-memory backend) asserts a pointer whose chunks aren't present → Unavailable. Spec's "atomic flip" wording corrected.

P2s

  • Temp staging: now create_new (O_EXCL — never follows a planted symlink), permissions copied from the target before writing any bytes, sync_all before rename, and a TempFileGuard removes the temp on every early return. Perms-preservation and symlink-follow have tests.
  • ValueTooLong → transient (the SDK already retried at the reported size, so it means the value grew between host calls).
  • Frozen fixtures: both now assert live bytes against precomputed literal hashes (envelope + inner + per-chunk) and feed those literals into the pointer, so a serialization or SHA-helper drift fails the test rather than rebuilding a fresh golden.
  • CI: fastly cli wasip1 added (above).

P3s

  • GC reporting: the cloud run no longer pre-prints planned keys as "deleting"; execute_gc_deletes reports deleted / FAILED / skipped per key as it happens. The local dry-run no longer counts prior chunks already absent from the file (the real prune's remove is a no-op there) — new regression test.
  • Generated projects gitignore the lock sidecar.
  • Recovery commands carry a POSIX/bash note (cmd/PowerShell quote differently).
  • Stale docs fixed: inert-foreign siblings don't abort GC, root-like-orphan warnings exist, cloud pruning is --local-only, and missing-chunk-as-transient.

Gates

fmt, workspace clippy --all-features, test --workspace --all-targets (20 binaries), the fastly cloudflare spin feature check, the wasm clippy matrix including the new fastly cli wasip1, the fastly WASM contract suite under viceroy (79 passed), app-demo fmt + clippy, and docs prettier all pass.

Three data-recovery/loss blockers plus follow-ups.

P1 (broken recovery): a cloud `config push` first resolved the existing
remote pointer, so a missing chunk / hash mismatch / malformed pointer
aborted the push BEFORE the repairing write — recovery required manual
Fastly edits, contradicting the runtime's "re-run config push to repair"
contract. A new `ReadConfigEntry::Corrupt` distinguishes an EXISTING entry
whose value will not resolve (the describe succeeded) from a real IO error.
The push treats Corrupt like an absent remote: it warns and overwrites (the
in-band repair), with the same behaviour now uniform across cloud and local.

P1 (data loss): local pruning deleted a prior chunk key whose value carries
an unknown/future/non-string `edgezero_kind` — a value the cloud GC path
already fails closed on. The prune's protection predicate now keeps anything
that ANNOUNCES our namespace (via `value_announces_our_kind`) or classifies
as a root, and the dry-run count mirrors it. An older CLI no longer destroys
a newer-format entry.

P1 (data loss): a malformed pointer at a chunk-shaped root could lose its
nested generation — the truncated value looks like a chunk fragment, so it
became a candidate while its independently-provable nested chunks were
deleted with no readable root to name them. A chunk-shaped candidate that
has any canonical chunk nested beneath it now fails closed. A real leaf
payload has no nested chunks, so normal GC is unaffected.

P2: `canonical_manifest_target` now follows a DANGLING manifest symlink to
its intended target (creating that file, preserving the link) instead of
replacing the symlink with a regular file. `atomically_replace_file`
propagates `sync_all` errors (an ENOSPC/EIO during writeback fails the
command before the known-good manifest is replaced, not after) and syncs the
containing directory after the rename.

P2: added command-level GC coverage through `run_config_gc` — manifest load,
store resolution, adapter-registry dispatch, listing, classification, and
reporting together — plus an integrated unknown-kind fail-closed regression.

P3: recovery commands already carry a POSIX/bash note; the generated
gitignore now globs `.*.edgezero-lock` (covering a non-default manifest
name); and the GC spec's fail-closed list and `prior_chunk_keys` note are
reconciled with the implemented inert-foreign-sibling and gc_classify_root
behaviour.
@aram356

aram356 commented Jul 28, 2026

Copy link
Copy Markdown
Contributor Author

Thanks — findings 1–3 were all real, and they were the mirror image of last round's hardening (I fixed cloud/runtime but left the sibling paths). All 8 addressed in 210416b.

P1 — cloud push can't repair a broken generation

You're right that --yes/--no-diff couldn't get past the read. The push resolved the existing pointer first, and a corrupt one aborted before the repairing write. I added a ReadConfigEntry::Corrupt variant that distinguishes an existing entry whose value won't resolve (the describe succeeded — so the store is reachable and the key present) from a genuine IO error (which still errors). The push treats Corrupt like an absent remote: it warns and overwrites — the in-band repair the runtime and spec promise. Both the cloud read and the local read now map corruption to Corrupt uniformly (the local path previously overloaded Unsupported). Tests: adapter-level (malformed pointer / missing chunk / hash mismatch → Corrupt) and flow-level (render_first_read_diff → proceed; handle_consent with --yes → proceed).

P1 — local prune deletes unknown/future-kind roots

Fixed the asymmetry: the prune's protection predicate now keeps anything that announces our namespace (value_announces_our_kind — pointer, unknown, or non-string kind) or classifies as a root, exactly what cloud GC fails closed on. The dry-run count mirrors it so the preview still matches deletions. Test seeds a real generation, overwrites one chunk with a fastly_config_chunks_v2 value, and asserts the re-push keeps it and warns.

P1 — malformed pointer at a chunk-shaped root loses its nested generation

This was the subtle one. A truncated pointer at a chunk-shaped key can't announce its discriminator, so it looked like a leaf fragment and became a candidate — while its independently-provable nested chunks got deleted with no readable root to name them. The candidate arm now additionally requires that nothing is nested beneath the key: if any canonical chunk of this key exists in the listing, it's treated as an unreadable nested root and fails closed. A real leaf payload never has nested chunks, so normal GC is untouched. New test builds exactly that shape (malformed pointer + aged provable nested generation) and asserts the whole run refuses and deletes nothing.

P2s

  • Dangling symlink: canonical_manifest_target now read_links a dangling manifest symlink and targets the intended (missing) file, creating it and preserving the link, instead of clobbering the symlink. Both symlink cases have tests.
  • Durability: sync_all errors now propagate (an ENOSPC/EIO fails the command before the known-good manifest is replaced, with the temp cleaned up), and the containing directory is synced after the rename (best-effort, since opening a dir as a file isn't portable).
  • Command-level GC test: two integrated tests through run_config_gc — a clean store dispatches end-to-end and reports nothing-to-reclaim (deleting nothing), and an unknown-kind root fails the whole command closed. (Concurrent symlink/direct-path locking is covered by the provision-vs-push and symlink tests added last round.)

P3s

  • Recovery commands already carry the POSIX/bash note (the full cross-shell escaping engine felt out of scope for a Fastly-CLI recovery hint; the note scopes it honestly — happy to reconsider if you'd prefer per-shell output).
  • Generated gitignore now globs .*.edgezero-lock (covers a manifest named something other than fastly.toml).
  • GC spec reconciled: the fail-closed list now distinguishes namespace-claiming/malformed-at-reserved-key (abort) from an inert foreign sibling at an ordinary key (protected, GC continues), and the prior_chunk_keys note no longer implies GC calls it.

Gates

fmt, workspace clippy --all-features, test --workspace --all-targets (20 binaries), the fastly cloudflare spin feature check, the wasm clippy matrix (wasip1 fastly + fastly cli, wasip2 spin, wasm32-unknown cloudflare), the fastly WASM contract suite under viceroy (79 passed), app-demo fmt + clippy, and docs prettier all pass.

…ymmetry

Corrects the `Corrupt` read outcome, which was both too narrow and too
broad, and closes remaining local/cloud asymmetries.

Corrupt was too NARROW: a malformed direct value, invalid JSON, or a
direct-envelope SHA mismatch passed through as `Present`, and the push then
aborted parsing it — recovery needed manual deletion. `read_config_entry`
(cloud and local) now VERIFIES the resolved value as an envelope; a
non-verifying value is `Corrupt` (repairable by overwrite).

Corrupt was too BROAD in two ways, both of which could destroy healthy or
newer state:
  - An infrastructure chunk-fetch failure (fastly can't spawn, auth fails,
    schema drift) became `Corrupt`, letting `push --yes` overwrite healthy
    remote after an INCOMPLETE read. The fetch callback now records
    infrastructure failures, and such a read stays a hard error.
  - An unknown/future `edgezero_kind` became `Corrupt` and was offered for
    ordinary overwrite. An older CLI must not clobber a newer format, so it
    is now a hard read error (upgrade the CLI). All three cases route
    through one `classify_resolved_read`.

Local prune was not symmetric with cloud GC: it deleted a truncated pointer
at a chunk-shaped key even when a canonical chunk was nested beneath it. The
protection predicate (and the dry-run count) now also keep a key that has a
nested generation, matching cloud GC's fail-closed rule.

`config diff` / push dry-run / pre-write recheck no longer model a corrupt
remote as absence or a clean comparison: diff reports a distinct
`CorruptRemote` outcome that exits "could not compare" (2) regardless of
`--exit-code`; the dry-run stops fabricating a local-vs-empty diff; and the
recheck reports a Present→Corrupt transition precisely instead of as a
removal.

`canonical_manifest_target` follows the WHOLE symlink chain (bounded), so a
multi-hop dangling manifest link writes at the final target and every
intermediate link is preserved — and the same target keys one lock.

The atomic replace no longer silently skips permission preservation: a
metadata error other than NotFound fails rather than widen access.

Tests: `classify_resolved_read` taxonomy; a malformed nested-root holder is
kept by local prune; a multi-hop dangling symlink chain; a command-level GC
test that the ENV-derived platform name drives store selection; the corrupt
diff exit code. Docs: blob spec adds `Corrupt` to `ReadConfigEntry`; the GC
spec's local-preservation predicate now matches the implementation.
@aram356

aram356 commented Jul 29, 2026

Copy link
Copy Markdown
Contributor Author

Thanks — this was a sharp review. The three Highs were all about the Corrupt outcome I added last round being mis-scoped, and you were right in both directions. All 8 findings addressed in a2c2eac.

High #1 — Corrupt too narrow (malformed direct values un-repairable)

read_config_entry now VERIFIES the resolved value as a BlobEnvelope. A value that resolves but isn't a verifying envelope — invalid JSON, missing fields, a SHA mismatch, a foreign non-envelope — is now Corrupt (repairable by overwrite) instead of Present-then-abort. Applies to both the cloud and local read paths.

High #2 — infra fetch failures must not be Corrupt

Right — an incomplete read must never license overwriting healthy remote state. The chunk-fetch callback now records infrastructure failures (fetch_remote_config_store_entry returning Err: spawn/auth/schema), and a resolve error caused by one stays a hard error, not Corrupt. A genuinely absent chunk (Ok(None)) is still repairable corruption.

Medium #4 — unknown/future kind must stay a read error

Also right, and it's the mirror of #2: an unknown/future edgezero_kind is now a hard read error ("upgrade the CLI"), never offered for overwrite — an older CLI clobbering a newer format would lose it. This matches GC's protection and the blob protocol.

All three now route through one classify_resolved_read(resolved, raw_value, fetch_failed), unit-tested across every branch.

High #3 — local prune asymmetry on a malformed nested root

The local prune's protection predicate (and its dry-run count) now also keep a key that has a canonical chunk nested beneath it — a truncated/unreadable nested root — matching the cloud GC fail-closed rule I added last round. New test builds that exact shape and asserts the holder is kept.

Medium #5 — diff/recheck modeling corruption as absence

  • config diff now reports a distinct DiffOutcome::CorruptRemote that exits 2 ("could not compare") regardless of --exit-code — never a clean/absent success a script could misread.
  • The push dry-run no longer fabricates a local-vs-empty diff for a corrupt remote; it reports the corruption and proceeds.
  • recheck_before_write reports a Present→Corrupt transition precisely ("unusable at write time; overwriting to repair") instead of as a removal.

Medium #6 — multi-hop dangling symlink

canonical_manifest_target now follows the whole symlink chain (bounded against cycles), so fastly.toml → middle.toml → missing.toml writes at the final target, preserves every intermediate link, and resolves to the same lock as a direct write. New multi-hop test.

Medium #7 — command-level GC destructive routing

Added a command-level test that the ENV-derived platform name (EDGEZERO__STORES__CONFIG__APP_CONFIG__NAME) drives store selection: the gc resolves it via config-store list to a specific id and lists entries under that id (the fake only knows the env name, so ignoring the overlay fails resolution), and I assert the resolved --store-id flows through. I did not add a command-level successful---yes-deletion test: a reclaimable generation must be byte-identical to the Fastly writer's output (prepare_fastly_config_entries, pub(crate)), which isn't reachable from the edgezero-cli crate. The deletion mechanics and delete arguments are covered exhaustively by the fastly-internal run_gc tests (fake_fastly_gc + delete-argv logging); this test closes the routing gap the wrapper uniquely owns. Happy to expose a test fixture if you'd prefer the full end-to-end delete at the command level.

Low #8

  • The atomic replace no longer silently skips permission preservation: a metadata error other than NotFound fails rather than widen access.
  • Blob spec adds Corrupt to the ReadConfigEntry enumeration; the GC spec's local-preservation predicate now matches the implementation (namespace-claiming values + nested-root protection).

Gates

fmt, workspace clippy --all-features, test --workspace --all-targets (20 binaries), the fastly cloudflare spin feature check, the wasm clippy matrix (wasip1 fastly + fastly cli, wasip2 spin, wasm32-unknown cloudflare), the fastly WASM contract suite under viceroy (79 passed), app-demo fmt + clippy, and docs prettier all pass.

Address the current review round on the chunk-GC branch.

P1 — Future envelope AND pointer versions must not be overwritten. Add
`value_is_future_format` (raw-value predicate: a bumped envelope/pointer
version, or any unknown `edgezero_kind`) and check it FIRST in the CLI read
classifier, returning a hard error instead of a repairable Corrupt. Guard the
local-prune and dry-run predicates and the cloud GC candidate arm with it so a
v2 direct envelope under a chunk-shaped key is never deleted. This restores the
v1-reader fail-closed rule from the blob spec.

P1 — Ambiguous Fastly stderr ("not found"/"does not exist"/"404") for a CHUNK
now sets `fetch_failed` (Ok(None) => incomplete read) so a partial read becomes
a hard infrastructure error, not an overwriteable Corrupt.

P2 — Make the Corrupt repair contract adapter-agnostic. Centralise it in the
generic push layer (`classify_present_body` in cli/config): a Present body that
parses+verifies is Valid; one that fails to parse or mismatches its SHA is
Corrupt (push overwrites to repair); a bumped envelope version is a hard error.
Axum/Cloudflare/Spin now get repair without each implementing Corrupt.

P2 — Local locking safe across file aliases. Refuse a manifest with more than
one hard link (an atomic rename would break the link and path-based locks
cannot serialise the other names). Symlink resolution now fails closed on a
read-link error or hop-limit instead of falling back to a writable path.

P2 — Runtime remediation is correct for future formats: a value the running
build cannot parse as its own format asks the operator to redeploy an updated
build, not to re-push (re-pushing cannot help).

P3 — Threshold-free GC dry-run now prints a usable apply instruction
(`--yes --older-than <dur>`), matching the requirement that a non-zero window
is mandatory.

P3 — Provision docs corrected: provision writes only `[setup.*]`; the
`[local_server.*]` seeding is done by `config push --local`.
@aram356

aram356 commented Jul 30, 2026

Copy link
Copy Markdown
Contributor Author

Thanks — all seven findings addressed in 2518670. Both P1s (the destructive-write paths) are the core of this round.

P1 — Future envelope AND pointer versions could be overwritten. Added value_is_future_format, a raw-value predicate that flags a bumped envelope/pointer version or any unknown edgezero_kind. The CLI read classifier now checks it first and returns a hard error rather than a repairable Corrupt, so a newer format is never overwritten by a v1 push. The same predicate guards the local-prune and dry-run keep-predicates and the cloud GC candidate arm, so a v2 direct envelope sitting under a chunk-shaped key can't be deleted either. This restores the v1-reader fail-closed rule (blob spec:3501). A v2 direct envelope passes resolve as a Foreign passthrough and is caught on the raw value; a v2 pointer fails resolve on its version check and is also caught on the raw value — both land on the hard-error path.

P1 — Ambiguous Fastly stderr for a CHUNK. The chunk-fetch closure now treats an ambiguous not-found (Ok(None)) as an incomplete read: it sets fetch_failed, so the read resolves to a hard infrastructure error ("a chunk fetch failed … the remote was not fully read, so nothing was changed") instead of an overwriteable Corrupt.

P2 — Corrupt repair contract only implemented by Fastly. Centralised the contract in the generic push layer (classify_present_body in config.rs), so it holds for every adapter: a Present body that parses and integrity-verifies is Valid (diff against it); one that fails to parse or fails its SHA is Corrupt (the push overwrites to repair); a bumped envelope version (BlobEnvelopeError::UnknownVersion) is a hard error. Axum/Cloudflare/Spin now get repair without each implementing Corrupt, and the generic push no longer aborts on a malformed envelope.

P2 — Local locking unsafe across file aliases. reject_hard_linked_manifest now refuses a manifest with nlink > 1 (an atomic rename would break the link, and a path-based lock can't serialise writers arriving via the other names) — fail closed with a fix hint. canonical_manifest_target now fails closed on a read-link error and on the hop limit (40) instead of falling back to a writable path.

P2 — Runtime remediation wrong for future formats. When the running build can't parse a stored value as its own format, config_store now asks the operator to redeploy an updated build (re-pushing can't help) rather than to re-run config push.

P3 — Threshold-free dry-run apply instruction. The GC dry-run now prints --yes --older-than <dur> (a non-zero window is required), matching what --yes actually accepts.

P3 — Provision docs. CLI reference and walkthrough now state provision writes only [setup.*]; the [local_server.*] seeding is done by config push --local.

Gates (all green): cargo fmt --all -- --check; cargo clippy --workspace --all-targets --all-features -D warnings; cargo test --workspace --all-targets (no failures); feature check fastly cloudflare spin; wasm clippy matrix (wasip1 fastly + fastly cli, wasip2 spin, wasm32-unknown cloudflare); fastly WASM contract suite under Viceroy (6/6); app-demo fmt+clippy; docs prettier.

…store reads

Address the follow-up review round. Findings 1-3 are the P1 fail-closed gaps.

P1 -- Cloud GC no longer treats a FUTURE direct envelope as a zero-reference
foreign root. A direct envelope from a newer writer classifies as `Foreign` (no
`edgezero_kind`), so the ordinary-value fallback would wave it through and plan
its (unknown-scheme) chunks for deletion. Exclude `value_is_future_format` from
that fallback so it fails closed with nothing deleted.

P1 -- Future-format detection is now TYPED through pointer resolution. The
resolver returns `ResolveFailure::{FutureFormat,Corrupt}` instead of an untyped
string, and checks `value_is_future_format` on the REASSEMBLED envelope BEFORE
deserializing it as v1 -- so a v2 envelope wrapped in a valid v1 pointer (only
knowable after reassembly) can no longer erase into repairable corruption a
downgrade push would overwrite. The generic push path does the same version
pre-check before v1 deserialization, covering adapters whose Present body no
longer parses as the exact v1 schema.

P1 -- Root and store OPERATIONAL failures no longer read as absence. Store
resolution maps to `MissingStore` only on the resolver's own unambiguous "no
fastly config-store matches" signal (a bare "not found" also matches "`fastly`
not found on PATH" and list/auth/network errors). Entry `describe` maps to
`MissingKey` only when the stderr carries a clean absence marker AND no
operational marker (auth, network, 5xx, rate-limit), so two incomplete reads can
no longer pass the pre-write recheck and authorise an overwrite.

P2 -- Reconcile the `Corrupt` contract doc: only a PROVABLY unusable, fully-read
value is repairable; a value that could not be fully read (an absent/unfetchable
chunk, indistinguishable from an incomplete read) fails closed as a hard error,
never `Corrupt`.

P2 -- A DIRECT future envelope now gets the redeploy remediation at runtime. It
passes the resolver as a foreign `Ok`, so the future-format check is applied on
the SUCCESS path too -- it no longer reaches core as a generic integrity 500.

P2 -- The hard-link check works on Windows via `MetadataExt::number_of_links()`
(stable, no new deps), so Windows hard-link aliases fail closed too, not just
Unix ones.

P3 -- CI now runs the fastly runtime `--lib` unit tests under Viceroy; the WASM
job previously ran only `--test contract`, so the runtime remediation/fail-closed
tests compiled but never executed.

Tests: typed inner-future resolution, GC fail-closed on a future direct
envelope, operational-vs-absence stderr, direct-future redeploy remediation, and
a future envelope that fails v1 deserialize.
@aram356

aram356 commented Aug 1, 2026

Copy link
Copy Markdown
Contributor Author

Thanks — all seven addressed in c2b8585. Findings 1–3 (the P1 fail-closed gaps) are the focus.

P1 — Cloud GC treated a future direct envelope as a zero-reference foreign root. A direct envelope from a newer writer classifies as Foreign (no edgezero_kind), so the ordinary-value fallback waved it through and planned its chunks for deletion. The fallback now also requires !value_is_future_format(...), so a future value falls to the "could not classify root" arm and GC fails closed with nothing deleted. Updated the spec's foreign-at-ordinary-key paragraph to call out the exception.

P1 — A v2 envelope inside a v1 pointer could be overwritten by a downgrade push. Version detection is now typed through pointer resolution: resolve_fastly_config_value_typed returns ResolveFailure::{FutureFormat, Corrupt} instead of an untyped string, and checks value_is_future_format on the reassembled envelope before deserializing it as v1 (extracted into finalize_reconstructed_envelope). A newer inner envelope — only knowable after the chunks are fetched — now surfaces as FutureFormat, which classify_resolved_read turns into a hard error rather than repairable Corrupt. The generic path (config.rs) does the same via a schema-agnostic body_is_future_envelope pre-check, so a v2 body that no longer deserializes as the exact v1 schema is still refused, not overwritten.

P1 — Root/store operational failures were classified as absence. Store resolution maps to MissingStore only on the resolver's own unambiguous no fastly config-store matches signal — a bare "not found" also matches "fastly not found on PATH" and list/auth/network errors, so those now fail closed. Entry describe maps to MissingKey only when the stderr carries a clean absence marker and no operational marker (auth / network / 5xx / rate-limit), via stderr_signals_operational_failure. Two incomplete reads can no longer pass the pre-write recheck and authorise an overwrite. The existing read_remote_returns_missing_store_on_appropriate_stderr test encoded the old behaviour (a failed list call → MissingStore); it's split into a fail-closed test for the operational case and a genuine-absence test (a successful list that omits the store → MissingStore).

P2 — Missing-chunk contract. You're right that the fail-closed chunk behaviour contradicted the Corrupt doc. Reconciled the doc rather than loosening the safety: Corrupt is now scoped to a provably unusable, fully-read value; a value that could not be fully read (an absent/unfetchable chunk, indistinguishable from an incomplete read) fails closed as a hard error the operator retries, never Corrupt. There's no trustworthy in-band absence signal from the CLI, so an explicit repair operation would be the way to offer in-band recovery — I've left that as a follow-up rather than widen the overwrite path in this round.

P2 — Direct future envelope runtime remediation. A direct future envelope passes the resolver as a foreign Ok, so the redeploy remediation is now applied on the success path too (future = future_format || outcome.err().is_future_format()) — it no longer reaches core and surfaces as a generic integrity 500. New test direct_future_envelope_asks_to_redeploy_not_repush.

P2 — Non-Unix hard links. The check now reads the link count on Windows via std::os::windows::fs::MetadataExt::number_of_links() (stable in 1.95, no new deps), alongside nlink() on Unix — so Windows hard-link aliases fail closed too. Any other target (where no count is available) still leaves the file alone.

P3 — CI runs the runtime tests. Added a fastly-only step that runs cargo test -p edgezero-adapter-fastly --features fastly --target wasm32-wasip1 --lib under Viceroy. That job previously ran only --test contract, so the runtime remediation/fail-closed unit tests compiled but never executed — the lib target under Viceroy now runs 83 of them.

Gates (all green): fmt · workspace clippy --all-features -D warnings · workspace tests · feature check fastly cloudflare spin · wasm clippy matrix (wasip1 fastly + fastly cli, wasip2 spin, wasm32-unknown cloudflare) · fastly contract + --lib runtime tests under Viceroy (6/6, 83/83) · app-demo fmt+clippy · docs prettier.

…rns verbatim

Address the follow-up review round (four P1 data-loss/overwrite paths, three P2).

P1 -- Future-format detection is now SCHEMA-AGNOSTIC. `value_is_future_format`
and the generic `body_is_future_envelope` key on the `version` field alone (and,
generically, on the presence of `edgezero_kind`), not the four v1 fields. A
future shape like `{"version":2,"payload":...}` that drops v1 fields no longer
slips through as repairable corruption (overwriteable by a downgrade push, and
treated as an inert zero-reference root by GC).

P1 -- Operational errors can no longer become absence. A read maps to
MissingKey / an absent chunk ONLY on a CONFIRMED clean absence
(`stderr_is_confirmed_absence`: a not-found token with NO operational marker);
the marker list now includes 401/403/429 and an HTML "page not found". Store
resolution is now TYPED (`resolve_remote_config_store_id` returns
`Ok(None)` only when the list SUCCEEDS and no store matches), not a substring
match on an untyped error. Ambiguous/operational output stays a hard error.

P1 -- Destructive GC no longer accepts lossy listing input. All value-bearing
`fastly` stdout (the GC listing, root/chunk describes, the store-id list) is
converted with STRICT UTF-8 via `strict_stdout`; invalid bytes fail closed
instead of becoming U+FFFD and mutating a root value or chunk.

P1 -- The generic push now refuses an unknown `edgezero_kind`. A v1-shaped
envelope carrying `edgezero_kind: "new_format"` (which serde would otherwise
accept, ignoring the extra field) is a newer format non-Fastly adapters must not
overwrite.

P2 -- A CONFIRMED-missing Fastly chunk is now repairable. A clean not-found on a
chunk fetch resolves to `Corrupt` (a push overwrites to repair, as the blob spec
prescribes for persistent loss), while an operational/ambiguous fetch failure
stays a hard error. The confirmed-absence discrimination is what lets both hold:
persistent loss is fixable, an incomplete read still never authorises overwrite.

P2 -- The runtime store returns arbitrary DIRECT values VERBATIM again. The
store-layer inspection of direct values is reverted; the future-envelope
remediation moves to the typed app-config extractor, which now maps an
`UnknownVersion` (and a schema-changed future version) to an upgrade/redeploy
error instead of a generic integrity 500. The store still errors on a newer
value in OUR namespace (an unknown `edgezero_kind`, a future pointer/inner
version), detected before the exact-v1 struct deserialize.

P2 -- The minimal CLI feature graph lints warning-free:
`cargo clippy -p edgezero-cli --all-targets -- -D warnings` no longer trips on
unused `ResolveFailure` payloads / `into_message`, which the read classifier now
surfaces in its redacted infra diagnostic.
@aram356

aram356 commented Aug 2, 2026

Copy link
Copy Markdown
Contributor Author

Thanks — all seven addressed in 477429c.

P1 — Schema-changing v2 envelopes failed open. value_is_future_format (and the generic body_is_future_envelope) now key on the version field alone, not the presence of the four v1 fields, so {"version":2,"payload":...} is detected as future everywhere — refused by push, refused after v1-pointer reconstruction, and failed-closed by GC — rather than being read as repairable corruption / an inert zero-reference root.

P1 — Operational errors becoming absence. Introduced stderr_is_confirmed_absence (a not-found token with no operational marker) as the only path to MissingKey/absent-chunk; the operational marker list now includes numeric 401/403/429 and an HTML page not found. Store resolution is now typed: resolve_remote_config_store_id returns Ok(None) only when the list call succeeds and no store matches — no more substring reclassification of an untyped error. Ambiguous/operational output stays Err.

P1 — Lossy GC listing input. All value-bearing fastly stdout (the GC listing, root/chunk describes, and the store-id list) now converts with strict UTF-8 via strict_stdout; invalid bytes fail closed instead of becoming U+FFFD and mutating a root value or chunk before deletion planning.

P1 — Generic push missed unknown edgezero_kind. body_is_future_envelope now also returns true when an edgezero_kind field is present — a v1-shaped envelope with edgezero_kind: "new_format" (which serde silently ignores) is refused on Axum/Cloudflare/Spin, not overwritten.

P2 — Permanently missing Fastly chunk had no repair path. This is the crux, and it's what reconciles this round with the prior one. A confirmed clean absence on a chunk fetch (stderr_is_confirmed_absence) now resolves to Corrupt → a push overwrites to repair, exactly as the blob spec prescribes for persistent loss (blob-app-config.md:6374). An operational/ambiguous fetch failure still sets fetch_failed and stays a hard error. So persistent loss is fixable and an incomplete read never authorises an overwrite — the confirmed-absence discrimination is the single mechanism that lets both invariants hold. read_config_entry_hard_errors_on_a_not_found_chunk is retargeted accordingly.

P2 — Store layer stopped returning direct values verbatim. Reverted the store-layer inspection of direct values (config_store.rs). The store again returns arbitrary direct values verbatim per blob-app-config.md:6336; the future-envelope remediation moved to the typed app-config extractor, which now maps an UnknownVersion (and a schema-changed future version, via a pre-deserialize version check) to an upgrade/redeploy error instead of a generic integrity 500. The store still errors on a newer value in our namespace (an unknown edgezero_kind, or a future pointer/inner-envelope version — the latter detected before the exact-v1 struct deserialize so an incomplete future pointer can't slip through as corruption).

P2 — Minimal CLI feature graph lint. cargo clippy -p edgezero-cli --all-targets -- -D warnings is now clean: the read classifier surfaces the resolver's (already-redacted) message in its infra diagnostic, so ResolveFailure's payload and into_message are used under the cli-only feature set, not just under all-features unification.

Gates (all green): fmt · workspace clippy --all-features · clippy -p edgezero-cli --all-targets (minimal graph) · workspace tests · feature check fastly cloudflare spin · wasm clippy matrix (wasip1 fastly+fastly cli, wasip2 spin, wasm32-unknown cloudflare) · Viceroy contract (6/6) + runtime --lib (83/83) · app-demo fmt+clippy · docs prettier.

Address the follow-up review round (three P1 data-loss paths, three P2, one P3).

P1 -- GC no longer trusts a future inner format behind a v1 pointer. After
reassembling a generation, GC deserialised straight into `BlobEnvelope`, which
silently ignores a bumped version or an unknown `edgezero_kind`. A newer inner
format can reference generations this build cannot see, so trusting only the
outer pointer's chunks as the live set could delete them as orphans. GC now runs
the same `value_is_future_format` check the runtime resolver does on the
reassembled bytes and fails closed.

P1 -- Absence is CONFIRMED against an authoritative complete listing, never a
describe 404. A proxy/endpoint or auth 404 looks exactly like a genuine
item-absence, so two such reads could pass the pre-write recheck and authorise an
overwrite. A root/chunk describe failure is now confirmed against a completeness-
strict `config-store-entry list` (fails closed on a paginated view or a duplicate
key): only a listing that OMITS the key reads as absence, and store resolution
returns a typed `Ok(None)`. The stderr-classification heuristics are removed.

P1 -- The local writer re-checks future-format UNDER THE LOCK. The pre-push
check ran before the write lock, so a newer writer could install a v2 value in
the TOCTOU window; the locked reread now re-classifies each root and refuses to
clobber a newer format before the upsert.

P2 -- The typed app-config extractor refuses an unknown `edgezero_kind`, not just
a bumped version. serde ignores the unknown field, so a v1-shaped envelope tagged
`edgezero_kind: "new_format"` would otherwise deserialize and apply on non-Fastly
runtimes; it now maps to the same upgrade/redeploy remediation the generic push
gives.

P2 -- Local push forces `format = "inline-toml"`. An existing `format = "json"` /
`"file"` next to the inline `contents` this writer emits left a contradictory
store the command still reported as written; it is now overwritten (with a
warning) to match what is written.

P2 -- `--no-default-features` lints clean: `chunked_config` is gated to the
features that use it, so a default-feature build no longer trips 23 dead-code
errors.

P3 -- Reconcile the `ReadConfigEntry::Corrupt` contract with the implementation:
a referenced chunk CONFIRMED absent by a complete listing is repairable `Corrupt`
(the blob spec repairs persistent loss by re-pushing); an absence inferred from a
bare 404 is NOT `Corrupt` and fails closed. One canonical contract.
@aram356

aram356 commented Aug 3, 2026

Copy link
Copy Markdown
Contributor Author

Thanks — all seven addressed in bb82733.

P1 — GC trusted a future inner format behind a v1 pointer. After reassembling a generation, gc_verify_generation deserialized straight into BlobEnvelope, which ignores a bumped version / unknown edgezero_kind. GC now runs the same value_is_future_format check the runtime resolver does on the reassembled bytes (before trusting its references) and fails closed with a newer-format error. New test gc_fails_closed_on_a_future_inner_generation (a v1 pointer whose chunks reassemble to a v2 envelope → GC aborts, nothing deleted).

P1 — A bare operational 404 was treated as confirmed absence. Replaced the stderr heuristics entirely. Absence is now confirmed against an authoritative complete listing (list_config_store_keys, completeness-strict: fails closed on a paginated/non-bare-array view or a duplicate key). A root/chunk describe failure is confirmed against it — only a listing that omits the key reads as absence; a present key (or a listing that itself fails) is a hard error. Store resolution already returns a typed Ok(None). So a proxy/endpoint/auth 404 can no longer pass the pre-write recheck and authorise an overwrite. stderr_signals_operational_failure / stderr_is_confirmed_absence and their test are gone.

P1 — Local future-format protection had a TOCTOU window. The pre-push check ran before the write lock. The local writer now re-classifies each root under the lock (reject_future_local_roots) and refuses to overwrite a newer format before the upsert. New test push_config_entries_local_refuses_to_overwrite_a_future_prior.

P2 — Extractor future-format handling was inconsistent. future_format_reason (renamed from the version-only helper) now also flags a present edgezero_kind, so a v1-shaped envelope tagged edgezero_kind: "new_format" gets the upgrade/redeploy remediation on non-Fastly runtimes instead of being silently applied — matching the generic push's refusal. New test app_config_extractor_asks_to_redeploy_on_an_unknown_edgezero_kind.

P2 — Local push preserved an incompatible store format. ensure_inline_toml_format now overwrites an existing format = "json" / "file" to inline-toml (with a warning) rather than leaving a contradictory store the command reported as written. New test write_fastly_local_config_store_replaces_incompatible_format.

P2 — Default feature graph wasn't lint-clean. chunked_config is now gated to the features that use it, so cargo clippy -p edgezero-adapter-fastly --no-default-features --lib -- -D warnings passes (was 23 dead-code errors).

P3 — Read-result contract vs implementation. Reconciled ReadConfigEntry::Corrupt's docs to the canonical rule: a referenced chunk CONFIRMED absent by a complete listing is repairable Corrupt (the blob spec repairs persistent loss by re-pushing); an absence inferred from a bare 404 is not Corrupt and fails closed. The critical qualifier is confirmed, and the Fastly impl now matches it.

Gates (all green): fmt · workspace clippy --all-features · clippy -p edgezero-cli --all-targets · clippy -p edgezero-adapter-fastly --no-default-features --lib · workspace tests · feature check fastly cloudflare spin · wasm clippy matrix (wasip1 fastly+fastly cli, wasip2 spin, wasm32-unknown cloudflare) · Viceroy contract (6/6) + runtime --lib (83/83) · app-demo fmt+clippy · docs prettier.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Fastly chunked-config GC: reclaim orphaned chunk entries on re-push

3 participants