Skip to content

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

Open
aram356 wants to merge 46 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 46 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
Round-7 review found two more destructive gaps, and they share one root cause:
every guard I had added validates METADATA, and metadata is exactly what an
inconsistent store gets wrong. Replaced with content verification.

The insight the previous rounds missed: a chunk key EMBEDS the SHA-256 of the
whole envelope it belongs to, so a chunk set is self-proving. Reassemble it and
the bytes either reproduce the content-address its own keys name, or they do
not. Nothing a store can claim about lengths, indexes or counts survives that,
and nothing outside our writer can forge it without a SHA-256 preimage.

[P1] A canonical-looking ordinary entry could be DELETED. Candidates were
classified by key shape, and `gc_reject_root_like_chunk` accepted invalid JSON /
unrelated JSON / integrity-invalid envelopes as "a plausible fragment". So an old
entry named `app.__edgezero_chunks.<64-hex>.0` holding plain text was scheduled
for deletion, violating "never delete a root, whatever its key". Candidates are
now grouped by (root, generation) and each group must PROVE it is writer-produced:
>= 2 chunks (an oversized envelope always splits into >= 2, so a lone chunk is
never ours), dense 0..n-1 indexes, and the values concatenated in index order
hash to the generation. Unprovable groups are left UNTOUCHED and reported --
skipped rather than fatal, so one foreign entry cannot block the store forever.
`gc_reject_root_like_chunk` is deleted; proof replaces heuristics.

[P1] Coordinated partial pointers still shrank the live set.
`validate_pointer_chunks` checks metadata only: drop the last chunk ref AND
restate `envelope_len` as the remaining sum, and the generation, indexes, lengths
and sum all agree while the omitted chunk silently leaves the live set and
becomes deletable -- though the live config still needs it. GC now reassembles
each live pointer's chunks from the listing, checking per-chunk len/sha256, and
hashes the result against `envelope_sha256`. This also subsumes the standalone
completeness guard: an incomplete listing cannot produce the bytes.

Deleting is now a per-GENERATION decision (aged by its youngest member), not
per-key: a partial delete would leave a corrupt generation behind.

[P2] Redaction extended to every stored-value diagnostic -- the direct-envelope
schema error and both integrity errors, not just the pointer parse.

[P2] `--no-env`: now names `EDGEZERO__STORES__CONFIG__<ID>__NAME` exactly and
says the logical id becomes the physical target. My round-6 fix for the guide
never landed -- the patch aborted mid-script and I reported it done without
checking. Verified on disk this time.

[P3] Spec/plan drift: the algorithm block, the `prior_chunk_keys` section (now
marked local-push-only, with the fail-direction asymmetry explained), the delete
semantics, and the stale absent-key claim in the plan.

Tests (132 -> 135, all gates green incl. the full wasm matrix):
- coordinated partial pointer rejected at BOTH the unit and the GC level
- unprovable chunk-shaped entries (plain text, someone's real config) left
  untouched and reported, while a genuine orphan generation is still reclaimed
- a lone self-consistent "chunk" is never reclaimed (the >= 2 rule)
- classifier carries pointer refs verbatim, and they describe the real chunks

Mutation-tested: disabling the planner's content verification, the group proof,
or the >= 2 rule each fails a test. The first pass at this found two guards whose
tests did NOT fail when mutated (a unit test on the verifier does not prove the
planner calls it) -- both now covered at the GC level.

Test fixtures now carry REAL chunk bytes; the old "CHUNK-PAYLOAD" placeholder
encoded the very assumption this round disproves ("reclamation only uses keys and
timestamps").
@aram356

aram356 commented Jul 17, 2026

Copy link
Copy Markdown
Contributor Author

Round 7 addressed — all five findings. Pushed as c8ff0c1.

Both P1s were the same mistake, and it's one you've now had to point out twice in different clothes: every guard I'd added validates metadata, and metadata is exactly what an inconsistent store gets wrong. Your two counterexamples are the proof — plain text at a chunk-shaped key passes a "looks like a fragment" check, and a coordinated envelope_len edit passes every arithmetic check. Patching either individually would have been the third round of the same error.

So I took your suggested fix as the design: a chunk key embeds the SHA-256 of the whole envelope it belongs to, so a chunk set is self-proving. Reassemble it and the bytes either reproduce the content-address the keys name, or they don't. Nothing the store claims about lengths, indexes or counts survives that, and nothing outside the writer forges it without a SHA-256 preimage.

  • Candidates are grouped by (root, generation) and each group must prove itself: ≥2 chunks (an oversized envelope always splits into ≥2, so a lone chunk is never ours — this is the one case a pure hash check can't catch), dense 0..n-1, and the concatenation hashes to the generation. Unprovable groups are left untouched and reported — skipped, not fatal, so one foreign entry can't block the store forever, per your "unknown groups must remain untouched".
  • Live pointers are reassembled from the listing with per-chunk len/sha256 checks and hashed against envelope_sha256. This also subsumes the standalone completeness guard: an incomplete listing can't produce the bytes.
  • gc_reject_root_like_chunk is deleted. Proof replaces the heuristic.
  • Deleting is now a per-generation decision (aged by its youngest member); a partial delete would leave a corrupt generation.

Two process notes, both to my discredit:

  1. My round-6 --no-env guide fix never landed. The patch script hit an assertion and aborted before writing, and I reported it done without checking the file. That's why you found line 334 unchanged. Verified on disk this time.
  2. My first pass at this round's tests was partly vacuous — and my own mutation testing caught it, not me. Disabling the planner's content verification and the ≥2 rule initially failed nothing: a unit test on gc_verify_generation does not prove the planner calls it. Both are now covered at the GC level, and all three guards fail under mutation.

Also worth flagging: the fixtures used a "CHUNK-PAYLOAD" placeholder value, justified by a comment reading "reclamation must only ever use keys and timestamps" — the exact assumption this round demolishes. They now carry real chunk bytes.

Redaction extended to every stored-value diagnostic (direct-envelope schema + both integrity errors). Spec/plan drift fixed, including marking prior_chunk_keys local-push-only and explaining why the asymmetry is safe there (local prunes prior − new, so an under-reporting pointer leaks rather than deletes; GC computes the live set, where the same lie deletes something live — which is why only GC pays for full verification).

Tests 132 → 135; all gates green including the full wasm matrix.

Round-8 review demolished the round-7 provenance claim, and it was right.

I wrote that "nothing outside our writer can pass without a preimage attack on
SHA-256". That is FALSE, and backwards: a forger does not need a preimage, they
pick envelope E FIRST and compute H = sha256(E). Content-addressing proves
self-consistency, never authorship -- anyone can content-address their own data.
So a foreign writer could store E's pieces under our reserved namespace as
`r.__edgezero_chunks.H.0`/`.1` and, once aged, GC deleted both. Verified: with
the round-trip removed, the new test shows the foreign group being deleted.

[P1] Provenance. `prove_generation` now re-runs `prepare_fastly_config_entries`
over the reassembled bytes and requires EXACTLY these keys and values. That pins
the split boundaries, the chunked-vs-direct threshold and the count (the >= 2
rule falls out: a small envelope round-trips to one ROOT-keyed entry, a large one
to >= 2 chunks). What it proves is "byte-identical to this writer's own output",
which is what the spec now claims -- not authorship. Telling a faithful
reproduction from the real thing needs trusted metadata or an authenticated
marker, and this store offers neither (any writer with store access could forge
either, and there is nowhere to keep a key). Documented as an accepted
limitation, in the spec and the operator guide, instead of an overclaim.

[P1] Diagnostics. `validate_pointer_chunks` embedded raw `chunks[].key`, which is
pointer-controlled and NOT yet validated there -- so a malformed pointer smuggles
`prod-db-password=hunter2` into a log line. Now reports POSITION only. The
runtime read path leaked too (serde Display, `edgezero_kind`, chunk keys, both
SHAs) and is redacted the same way; `redact_json_err` is now unconditional
because the guest needs it on every request. Two resolver tests asserted the
error must NAME the failing key -- the direct opposite -- and are inverted.

[P2] Partial deletes were not retryable, and saying so was false: once a sibling
is gone the survivors are a fragment `prove_generation` can never verify, so GC
would never reclaim them. Deletion is now per-generation and STOPS at a
generation's first failure, which leaves the common case whole and genuinely
retryable. A mid-generation failure strands survivors; the command now names them
and prints the manual `fastly config-store-entry delete` commands rather than
claiming a re-run helps.

[P2] Untrusted `envelope_len` could abort the process (including the edge guest,
on the read path) via `String::with_capacity` before any check rejected it.
Lengths are bounded to what the writer emits, summed with `checked_add`, and no
buffer is reserved from store-supplied numbers on either path.

[P2] Contract contradictions: the spec's abort-vs-skip conflict, invariant 7's
"all deletes are attempted", the plan's dead `gc_reject_root_like_chunk`, and the
guide's abort promise. The summary line also claimed every unprovable group
failed its hash check, which is wrong for the count rule -- reworded. Fixed a
real bug this round introduced on the way: with `doomed` grouped, the summary
counted GENERATIONS where the operator reads "orphan(s)".

Tests 135 -> 140. Mutation-verified: reverting the round-trip deletes the foreign
group; reverting the redaction leaks the sentinel.

Gates: all green including the full wasm matrix -- which caught `redact_json_err`
being cli-gated while the runtime path now needs it. Same cfg trap as round 6.
@aram356

aram356 commented Jul 17, 2026

Copy link
Copy Markdown
Contributor Author

Round 8 addressed — all five findings. Pushed as 4aae23c.

On the provenance P1: you're right, and my claim was not just wrong but backwards. I wrote that "nothing outside our writer can pass without a preimage attack on SHA-256." A forger needs no preimage — they pick envelope E first, then compute H = sha256(E). Content-addressing proves self-consistency, never authorship; anyone can content-address their own data. I confirmed your exploit end-to-end: with the round-trip removed, the new test shows the foreign group being deleted.

I took your suggested fix. prove_generation now re-runs prepare_fastly_config_entries over the reassembled bytes and requires exactly those keys and values — pinning the split boundaries, the chunked-vs-direct threshold and the count (the ≥2 rule now falls out for free: a small envelope round-trips to one ROOT-keyed entry, a large one to ≥2 chunks).

And I took your second point — that even this proves format compatibility, not authorship — as a documentation fix rather than pretending otherwise. There's no way to close it: an authenticated marker needs a key, and there's nowhere to keep one that a writer with store access couldn't also forge. So the spec now states the property we actually deliver — "we never delete an entry that is not a faithful reproduction of our own writer's output for the bytes it holds" — and documents the residual explicitly, in both the spec and the operator guide ("do not store unrelated data under that reserved namespace"). That's a weaker claim than "provenance", and it's the true one.

The diagnostics leak was worse than reported. You flagged validate_pointer_chunks and the read path; the read path also leaked edgezero_kind, the chunk keys, and both SHAs, and String::with_capacity(pointer.envelope_len) there means your P2 overflow can abort the edge guest at runtime, not just the CLI. All redacted to position + category; lengths bounded to what the writer emits, summed with checked_add, and nothing reserved from store-supplied numbers on either path. Two existing resolver tests asserted the error must name the failing key — the exact opposite of the requirement — so I inverted them.

On partial deletes: you're right that "re-run to retry" was false. Once a sibling is gone the survivors are a fragment prove_generation can never verify again. Deletion is now per-generation and stops at a generation's first failure, which leaves the common case whole and genuinely retryable; a mid-generation failure strands survivors, and the command now names them and prints the manual fastly config-store-entry delete commands instead of claiming a re-run helps.

You were also right that the summary over-claimed: one-entry groups fail the count rule, not the hash check. Reworded. While regrouping doomed I introduced and caught a related bug — the summary counted generations where the operator reads "orphan(s)".

Tests 135 → 140, mutation-verified. All gates green including the wasm matrix, which caught redact_json_err being cli-gated while the runtime path now needs it — the same cfg trap as round 6.

Thanks for the pagination note; I'd reached the same conclusion and left the bare-array assumption as-is.

…est deletes

Round-9 review found a P1 I had argued against myself, plus real leaks and an
unsound retry claim.

[P1] GC deleted chunks a live pointer referenced. I spent rounds 6-8 arguing key
shape is not authoritative for DELETE candidates -- then trusted key shape to
decide what is a ROOT: the live-set scan skipped every chunk-shaped key. A valid
pointer parked at `shadow.__edgezero_chunks.<sha>.0` is followed by the runtime
resolver, so its references are live, but GC never saw them and deleted the
generation. Now root-vs-chunk is decided by VALUE (`value_is_pointer_kind`): any
entry whose value is a pointer is a root, wherever it lives. A chunk payload is a
raw envelope fragment and carries no `edgezero_kind`, so it is never a false root.

[P1] Runtime diagnostics leaked pointer-controlled data. The full-envelope
mismatch printed `pointer.envelope_sha256` verbatim, and a fetch failure carried
the pointer-controlled chunk key from `get_sync` all the way to a `log::warn`.
Both are stored, attacker-settable strings on the read path. Redacted to a
position/category; `resolve_fastly_config_value` now attaches a position locator
instead of the callback's raw error. Sentinel non-leak tests for both.

[P2] "Re-run to retry" was unsound. A failed remote delete has UNKNOWN outcome
-- Fastly may commit it before returning an error -- so a "first delete failed,
generation left whole" claim is false: if it did commit, the survivors are a
fragment. The outcome now distinguishes CONFIRMED-stranded (a sibling's delete
succeeded first) from UNCERTAIN (unconfirmed), and reports each honestly rather
than promising idempotent retry. Added the stateful test the review asked for: a
committed-but-reported-failed delete surfaces the survivor as an unprovable
fragment on the next run, never silently.

[P2] Manual-recovery commands were not shell-safe. Root keys are free-form, so a
chunk key can hold `$(...)`, spaces or `;`. Commands are now single-quoted with
embedded quotes escaped; a hostile-key test round-trips the argument through
`sh` and asserts it parses back to the exact key.

[P2] Local fail-soft was unreachable through the CLI. `run_config_push_typed`
resolves the prior value before the write, so a corrupt local pointer aborted the
diff read before the writer's "overwrite, warn, prune nothing" could repair it --
the recovery path a `config push --local` is FOR. `read_config_entry_local` now
degrades an unresolvable prior to `Unsupported` (local only; the cloud read still
errors), so the write proceeds. Tested at that seam.

[P3] Doc/comment drift: spec/plan "every delete attempted / continue-then-fail /
inert-retryable", the root-classification step (value not shape), and two code
comments still claiming content-addressing proves authorship.

Tests 145 -> 146 (fastly cli 146; +3 sentinel/shell/degrade, +2 delete-outcome,
+ root-by-value). Three new guards mutation-verified. Gates all green including
the wasm matrix -- which caught `value_is_pointer_kind` cli-gated while a
test-without-cli build needs it. Third instance of that cfg trap; unit-tested in
its own module like the others.
@aram356

aram356 commented Jul 17, 2026

Copy link
Copy Markdown
Contributor Author

Round 9 addressed — all six findings. Pushed as 0126e1c.

The first P1 is one I should have caught myself, because I'd already argued the opposite for three rounds. I kept insisting key shape isn't authoritative for delete candidates — then the live-set scan trusted key shape to decide what's a root, excluding every chunk-shaped key up front. A valid pointer at shadow.__edgezero_chunks.<sha>.0 is followed by the runtime resolver, so its references are live, but GC never saw them and deleted the generation. Confirmed with the adversarial test you asked for (it deletes the referenced chunks before the fix). Root-vs-chunk is now decided by value: any entry whose value is a pointer is a root, wherever it lives. A chunk payload is a raw fragment with no edgezero_kind, so it's never a false root.

The runtime leak was broader than the two lines cited. The full-envelope mismatch printed envelope_sha256 verbatim and a fetch failure carried the pointer-controlled chunk key from get_sync through the resolver to log::warn. Both redacted (position/category only), with sentinel tests on both paths. The String::with_capacity on that same read path is also why the round-8 overflow could abort the edge guest, which is now fixed on both paths.

On "re-run to retry" — you're right it was unsound, and my round-8 fix was too confident. A failed remote delete has unknown commit state, so "first delete failed, generation left whole" is a claim I can't make. The outcome now distinguishes confirmed-stranded (a sibling delete succeeded first — definitely a fragment, manual only) from uncertain (unconfirmed — a re-run may reclaim it or surface it as an unprovable fragment), and reports each honestly. I added exactly the stateful test you described: a committed-but-reported-failed delete surfaces the survivor as unprovable on the next run rather than silently leaving it.

Shell-safety: recovery commands are single-quoted with embedded quotes escaped, and the hostile-key test round-trips the argument through sh to prove it parses back to the exact key.

On the local fail-soft being unreachable — good catch, that was dead behaviour. run_config_push_typed resolved the prior value before the write, so a corrupt local pointer aborted the diff read before the writer's fail-soft could repair it — defeating the whole point of config push --local as a recovery path. read_config_entry_local now degrades an unresolvable prior to Unsupported (local only; the cloud read still errors, since we must not overwrite remote state we couldn't read). I tested this at the read seam rather than building a full CLI-level Fastly fixture — happy to add the end-to-end version if you'd prefer it, but the seam is where the bug was and the write-path fail-soft is already covered.

Docs/comments: spec and plan no longer say "every delete attempted / continue-then-fail / inert-retryable", the root-classification step now says value-not-shape, and I removed two code comments still claiming content-addressing proves authorship.

Tests 145 → 146, three new guards mutation-verified. All gates green including the wasm matrix — which caught value_is_pointer_kind being cli-gated while a test-without-cli build needs it. That's the third time this exact cfg trap has surfaced; each new cli-only helper needs a unit test in its own module, and I've been adding them, but it's worth me building the habit rather than relying on the gate.

…it + fail-soft

Round-10 review found two security/destructive P1s and three lower issues.

Chunked read path leaked stored hashes via HTTP 500. Unlike the direct path, the
runtime chunked resolver returned reconstructed bytes after checking only the
OUTER pointer hashes, never parsing/verifying the inner BlobEnvelope. A
reconstructed value with an attacker-set embedded sha256 (a secret) reached
core, whose verify() formatted that stored hash into the 500 body. The resolver
now parses and verifies the inner envelope on both paths, redacted to a
category; core's extractor also redacts its parse/verify errors (the all-path
fix, covering every adapter). Sentinel tests at the resolver and the extractor.

A runtime-readable root at a chunk-shaped key could be deleted. Root-vs-chunk was
decided partly by key shape: a valid DIRECT envelope at a chunk-shaped key was
skipped as a chunk payload. A small envelope padded with trailing whitespace
chunks so that chunk 0 is itself a complete, verifying envelope -- a root the
runtime could read -- yet the generation round-trips and passes every proof, so
GC deleted chunk 0. Classification is now purely by VALUE: any entry whose value
is a valid envelope or a pointer is a protected root, at any key. Excluding it
also drops its generation to an incomplete group, which is left unprovable --
safe (leak rather than delete a possible root). Also fixes the root count to
include chunk-shaped roots, and restores fail-closed for a pointer-kind value at
a chunk-shaped key that cannot be classified (e.g. a cross-root pointer).

Local dry-run over corrupt prior state was still blocked. The corrupt-prior read
degrades to Unsupported, but the shared consent handler treated every Unsupported
as Spin Cloud and rejected dry-run. A LOCAL Unsupported now takes the normal
consent path, so a dry-run reaches the writer's report and a real push just needs
--yes. End-to-end test drives config push --local --dry-run over a corrupt Fastly
prior and asserts it reaches the writer, not the Spin-Cloud rejection.

Derived chunk keys were unchecked against Fastly's 256-char key limit. A chunk
key adds ~85 chars to the root, so a near-limit root produced invalid physical
keys that failed only mid-write. prepare_fastly_config_entries now rejects any
over-limit key (root and derived) before any I/O.

Docs realigned: the stale "whole, genuinely retryable" delete claims in the spec
invariants, contract, plan, guide, and the execute_gc_deletes comment now match
the unknown-outcome model; new invariants for inner-envelope verify, value-based
root protection, and the key limit.

Also removed review-round / PR / task / phase labels from code comments and
renamed the phase-prefixed tests (per reviewer feedback that comments should
explain the code, not track the process).

Tests: fastly cli 148 -> 149, cli 163 -> 164, plus a core extractor sentinel.
Two new P1 guards and the consent-routing fix are mutation-verified. All gates
green including the full wasm matrix.
@aram356

aram356 commented Jul 18, 2026

Copy link
Copy Markdown
Contributor Author

Round 10 addressed — all six findings. Pushed as b288ee9.

The leak P1 was a real gap I'd left asymmetric. The direct read path verified the inner envelope; the chunked path checked only the outer pointer hashes and returned the reconstructed bytes unverified, so a bad embedded sha256 reached core's verify(), which formatted that stored hash into the HTTP 500. Fixed at two levels: the resolver now verifies the inner envelope on both paths (redacted), and core's extractor redacts its own parse/verify errors — the latter is the real all-path fix, since it covers every adapter, not just Fastly. Both have sentinel tests (the extractor one asserts the HTTP-facing message drops the sentinel).

The chunk-shaped-root deletion was the sharper find, and your construction is exact. A small envelope padded with trailing whitespace chunks so that chunk 0 is the whole envelope plus spaces — which still parses and verifies. So chunk 0's key holds a valid direct envelope (a readable root), yet the generation round-trips through the writer and passes every proof, so GC deleted it. I'd protected pointer values at chunk-shaped keys in round 9 but not direct envelope values. Classification is now purely by value: any entry whose value is a valid envelope or pointer is a protected root at any key. Protecting chunk 0 drops its generation to an incomplete group, which prove_generation then leaves untouched — so we leak rather than delete a possible root, which is the safe direction.

That refactor also surfaced a regression I caught in test: a cross-root pointer at a chunk-shaped key fails this root's scope check, and my first cut treated the classify-error as "it's a candidate." A pointer-kind value is always root-like, so an unclassifiable one now fails closed rather than becoming deletable. And the root count now includes chunk-shaped roots (your P3 #2).

On the local dry-run — you're right that my round-9 fix was incomplete. Degrading to Unsupported stopped the read erroring but collided with the Spin-Cloud meaning, so the handler still rejected the dry-run. A local Unsupported now takes the normal consent path (which already lets dry-run through and requires --yes otherwise). The end-to-end test drives config push --local --dry-run over a corrupt Fastly prior and asserts it reaches the writer; mutating the fix reproduces the exact Spin-Cloud rejection you quoted. One scope note: I left malformed TOML as a hard error — that's a corrupt file, not a corrupt prior value, and the writer needs a parseable doc to write into. The fail-soft is specifically for a bad prior value.

Key-limit: prepare_fastly_config_entries now rejects any physical key over 256 chars (root and derived) before I/O, so a near-limit root fails up front instead of mid-write.

Docs: the stale "whole, genuinely retryable" claims in the spec invariants, contract, plan, guide, and the execute_gc_deletes comment now all match the unknown-outcome model from round 9; added invariants for the inner-envelope verify, value-based root protection, and the key limit.

Separately: I also stripped the review-round / PR / task labels out of the code comments and renamed the phase-prefixed tests, on feedback that comments should explain the code rather than track the process. No behavior change, but it touches many comment lines in the diff.

Tests: fastly cli 148 → 149, cli 163 → 164, plus the core extractor sentinel. Two new P1 guards and the consent-routing fix are mutation-verified. All gates green including the full wasm matrix.

…rs at runtime

Round-11 review found a second-layer response leak (the blocker), an incomplete
local dry-run degradation, and defense-in-depth gaps.

Typed deserialization leaked stored values into the 503 body. After envelope
verify, a serde type error (e.g. a string where u32 is expected) named the
offending value, and config_out_of_date_from_serde put that whole message into
the HTTP response. A field holding "SECRET" produced `invalid type: string
"SECRET", expected u32` in the body. The message is now a redacted category; the
field PATH (a schema location, not a value) is preserved so the operator still
knows which field is out of date. Sentinel test at the extractor asserts the
HTTP-facing message drops the value.

Local dry-run over corrupt state was only partly reachable. Round 10 degraded a
corrupt-but-parsing pointer to Unsupported, but malformed TOML, an unreadable
file, and a non-string root still hard-errored in the diff read — so the writer's
promised "unknown count" degradation was unreachable for them. The read now
degrades all of these to Unsupported; the real push still fails fatally at the
writer on malformed TOML (a file it cannot parse to write into), which a new test
pins alongside the dry-run cases.

Runtime pointer metadata was unvalidated. validate_pointer_chunks (canonical
keys, one generation, dense indexes, per-chunk length bound, checked sum) was
CLI/GC-only; the runtime resolver checked only kind/version before fetching. It
is now unconditional and runs on the read path too, so a shape the writer could
never emit is rejected before any chunk fetch. The GC-specific "skipping chunk
GC" wording moved to the GC caller so the validator's message is context-neutral.

Key-limit counted bytes while claiming characters, and 256 vs 255 was ambiguous.
Now counts chars() (a non-ASCII --key is measured as Fastly measures it) against
the stricter 255 (Fastly's guide; the API ref says 256).

Coverage the review asked for: a valid pointer rooted at a chunk-shaped holder
classifies successfully (unit test on gc_classify_root, not just the cross-root
fail-closed case); a protected direct-envelope root's siblings also survive;
generation aging by youngest member; a failure in one generation does not stop an
independent one; and the fake's delete failure is now a 404 so every
delete-failure test proves a 404 is still a failure.

Docs: spec now covers direct-envelope roots in classification, the runtime
validation, the char-based 255 key limit, and the local-read degradation
requirement; plan no longer claims first-failure stopping prevents stranding.

Tests: fastly cli 149 -> 153, cli 164 -> 165, core +1 sentinel. All gates green
including the full wasm matrix.
@aram356

aram356 commented Jul 18, 2026

Copy link
Copy Markdown
Contributor Author

Round 11 addressed — all findings, both blockers first. Pushed as 0bde0e9.

The P1 was the same leak class one layer deeper, and I'd missed it. Round 10 redacted the envelope parse/verify errors; this one is the typed deserialize error — config_out_of_date_from_serde put the raw serde message (invalid type: string "SECRET", expected u32) straight into the 503 body. Now the message is a redacted category and the field path is preserved (a schema location, not a value), so the operator still learns which field is stale. I wrote the HTTP-response sentinel test you asked for — it drives a valid envelope whose timeout_ms is a sentinel string and asserts the client-facing message drops it while keeping the timeout_ms path.

On the P2 acceptance failure — you're right, and it exposes that my round-10 fix was half a fix. I degraded only the corrupt-but-parsing pointer; malformed TOML, an unreadable file, and a non-string root still hard-errored in the diff read, so the writer's "unknown count" degradation was unreachable for exactly the cases spec:530 enumerates. The read now degrades all of them to Unsupported. I kept the real push failing at the writer on malformed TOML (a file it can't parse to write into — the spec says so too), and pinned both directions: three dry-run cases reach the writer, and a real push over malformed TOML still fails.

Runtime pointer validation (P3) — done properly, not narrowed. validate_pointer_chunks was cli/test-only; it's now unconditional and runs in the resolver before any fetch, so a shape the writer can't emit is rejected up front rather than driving a fan-out of chunk fetches. Invariant 14 now holds on the read path, not just in GC. The GC-flavored "skipping chunk GC" wording moved to the GC caller so the validator's message is context-neutral. Adding this did change two resolver tests' error paths (validation now catches the tampered pointers earlier) — I reworked them to still assert the redaction property, which is their real point.

Key-limit (P3): now counts chars() against the stricter 255 (Fastly's guide; the API ref says 256 — I took the lower bound so an emitted key is valid under either), with a non-ASCII test proving a 255-char multi-byte key is accepted despite exceeding 255 bytes.

Coverage — I added every case you listed:

  • a valid pointer rooted at a chunk-shaped holder classifies successfully (a gc_classify_root unit test — the existing cross-root test only showed fail-closed);
  • a protected direct-envelope root's siblings also survive (not just the holder);
  • generation aging by youngest member;
  • a failure in one generation doesn't stop an independent one;
  • the fake's delete failure is now a 404, so every delete-failure test also proves a 404 stays a failure.

Docs: spec covers direct-envelope roots, the runtime validation, the char-based 255 limit, and the local-read degradation requirement; plan no longer claims first-failure stopping prevents stranding (a committed-but-reported-failed delete's outcome is unknown — that's the round-9 model).

One scope call I'll flag: the two WASM contract suites you couldn't run (Fastly blocked by keychain, Cloudflare runner unavailable) — I ran the Fastly and Cloudflare clippy/compile WASM gates locally and they pass, but I can't run the Viceroy/wrangler contract suites here either, so those remain unverified on my side too.

Tests: fastly cli 149 → 153, cli 164 → 165, core +1 sentinel. All gates green including the full wasm compile/clippy matrix.

…; 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.

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