Fastly chunked-config GC: operator-invoked config gc reclaims orphaned chunk entries (store-wide) - #314
Conversation
…te+scope prior_chunk_keys, offline cloud dry-run, local root inference, warning semantics, invert stale no-GC test
…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
…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
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
…dated, prefix-scoped) + unit tests
…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
changed the title
…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.
…ms_after_statements)
aram356
changed the title
- 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)
…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
changed the title
Address the current review round on the chunk-GC branch.
P1 — Future envelope AND pointer versions must not be overwritten. Add
`value_is_future_format` (raw-value predicate: a bumped envelope/pointer
version, or any unknown `edgezero_kind`) and check it FIRST in the CLI read
classifier, returning a hard error instead of a repairable Corrupt. Guard the
local-prune and dry-run predicates and the cloud GC candidate arm with it so a
v2 direct envelope under a chunk-shaped key is never deleted. This restores the
v1-reader fail-closed rule from the blob spec.
P1 — Ambiguous Fastly stderr ("not found"/"does not exist"/"404") for a CHUNK
now sets `fetch_failed` (Ok(None) => incomplete read) so a partial read becomes
a hard infrastructure error, not an overwriteable Corrupt.
P2 — Make the Corrupt repair contract adapter-agnostic. Centralise it in the
generic push layer (`classify_present_body` in cli/config): a Present body that
parses+verifies is Valid; one that fails to parse or mismatches its SHA is
Corrupt (push overwrites to repair); a bumped envelope version is a hard error.
Axum/Cloudflare/Spin now get repair without each implementing Corrupt.
P2 — Local locking safe across file aliases. Refuse a manifest with more than
one hard link (an atomic rename would break the link and path-based locks
cannot serialise the other names). Symlink resolution now fails closed on a
read-link error or hop-limit instead of falling back to a writable path.
P2 — Runtime remediation is correct for future formats: a value the running
build cannot parse as its own format asks the operator to redeploy an updated
build, not to re-push (re-pushing cannot help).
P3 — Threshold-free GC dry-run now prints a usable apply instruction
(`--yes --older-than <dur>`), matching the requirement that a non-zero window
is mandatory.
P3 — Provision docs corrected: provision writes only `[setup.*]`; the
`[local_server.*]` seeding is done by `config push --local`.
|
Thanks — all seven findings addressed in P1 — Future envelope AND pointer versions could be overwritten. Added P1 — Ambiguous Fastly stderr for a CHUNK. The chunk-fetch closure now treats an ambiguous not-found ( P2 — Corrupt repair contract only implemented by Fastly. Centralised the contract in the generic push layer ( P2 — Local locking unsafe across file aliases. P2 — Runtime remediation wrong for future formats. When the running build can't parse a stored value as its own format, P3 — Threshold-free dry-run apply instruction. The GC dry-run now prints P3 — Provision docs. CLI reference and walkthrough now state provision writes only Gates (all green): |
…store reads
Address the follow-up review round. Findings 1-3 are the P1 fail-closed gaps.
P1 -- Cloud GC no longer treats a FUTURE direct envelope as a zero-reference
foreign root. A direct envelope from a newer writer classifies as `Foreign` (no
`edgezero_kind`), so the ordinary-value fallback would wave it through and plan
its (unknown-scheme) chunks for deletion. Exclude `value_is_future_format` from
that fallback so it fails closed with nothing deleted.
P1 -- Future-format detection is now TYPED through pointer resolution. The
resolver returns `ResolveFailure::{FutureFormat,Corrupt}` instead of an untyped
string, and checks `value_is_future_format` on the REASSEMBLED envelope BEFORE
deserializing it as v1 -- so a v2 envelope wrapped in a valid v1 pointer (only
knowable after reassembly) can no longer erase into repairable corruption a
downgrade push would overwrite. The generic push path does the same version
pre-check before v1 deserialization, covering adapters whose Present body no
longer parses as the exact v1 schema.
P1 -- Root and store OPERATIONAL failures no longer read as absence. Store
resolution maps to `MissingStore` only on the resolver's own unambiguous "no
fastly config-store matches" signal (a bare "not found" also matches "`fastly`
not found on PATH" and list/auth/network errors). Entry `describe` maps to
`MissingKey` only when the stderr carries a clean absence marker AND no
operational marker (auth, network, 5xx, rate-limit), so two incomplete reads can
no longer pass the pre-write recheck and authorise an overwrite.
P2 -- Reconcile the `Corrupt` contract doc: only a PROVABLY unusable, fully-read
value is repairable; a value that could not be fully read (an absent/unfetchable
chunk, indistinguishable from an incomplete read) fails closed as a hard error,
never `Corrupt`.
P2 -- A DIRECT future envelope now gets the redeploy remediation at runtime. It
passes the resolver as a foreign `Ok`, so the future-format check is applied on
the SUCCESS path too -- it no longer reaches core as a generic integrity 500.
P2 -- The hard-link check works on Windows via `MetadataExt::number_of_links()`
(stable, no new deps), so Windows hard-link aliases fail closed too, not just
Unix ones.
P3 -- CI now runs the fastly runtime `--lib` unit tests under Viceroy; the WASM
job previously ran only `--test contract`, so the runtime remediation/fail-closed
tests compiled but never executed.
Tests: typed inner-future resolution, GC fail-closed on a future direct
envelope, operational-vs-absence stderr, direct-future redeploy remediation, and
a future envelope that fails v1 deserialize.
|
Thanks — all seven addressed in P1 — Cloud GC treated a future direct envelope as a zero-reference foreign root. A direct envelope from a newer writer classifies as P1 — A v2 envelope inside a v1 pointer could be overwritten by a downgrade push. Version detection is now typed through pointer resolution: P1 — Root/store operational failures were classified as absence. Store resolution maps to P2 — Missing-chunk contract. You're right that the fail-closed chunk behaviour contradicted the P2 — Direct future envelope runtime remediation. A direct future envelope passes the resolver as a foreign P2 — Non-Unix hard links. The check now reads the link count on Windows via P3 — CI runs the runtime tests. Added a fastly-only step that runs Gates (all green): fmt · workspace clippy |
…rns verbatim
Address the follow-up review round (four P1 data-loss/overwrite paths, three P2).
P1 -- Future-format detection is now SCHEMA-AGNOSTIC. `value_is_future_format`
and the generic `body_is_future_envelope` key on the `version` field alone (and,
generically, on the presence of `edgezero_kind`), not the four v1 fields. A
future shape like `{"version":2,"payload":...}` that drops v1 fields no longer
slips through as repairable corruption (overwriteable by a downgrade push, and
treated as an inert zero-reference root by GC).
P1 -- Operational errors can no longer become absence. A read maps to
MissingKey / an absent chunk ONLY on a CONFIRMED clean absence
(`stderr_is_confirmed_absence`: a not-found token with NO operational marker);
the marker list now includes 401/403/429 and an HTML "page not found". Store
resolution is now TYPED (`resolve_remote_config_store_id` returns
`Ok(None)` only when the list SUCCEEDS and no store matches), not a substring
match on an untyped error. Ambiguous/operational output stays a hard error.
P1 -- Destructive GC no longer accepts lossy listing input. All value-bearing
`fastly` stdout (the GC listing, root/chunk describes, the store-id list) is
converted with STRICT UTF-8 via `strict_stdout`; invalid bytes fail closed
instead of becoming U+FFFD and mutating a root value or chunk.
P1 -- The generic push now refuses an unknown `edgezero_kind`. A v1-shaped
envelope carrying `edgezero_kind: "new_format"` (which serde would otherwise
accept, ignoring the extra field) is a newer format non-Fastly adapters must not
overwrite.
P2 -- A CONFIRMED-missing Fastly chunk is now repairable. A clean not-found on a
chunk fetch resolves to `Corrupt` (a push overwrites to repair, as the blob spec
prescribes for persistent loss), while an operational/ambiguous fetch failure
stays a hard error. The confirmed-absence discrimination is what lets both hold:
persistent loss is fixable, an incomplete read still never authorises overwrite.
P2 -- The runtime store returns arbitrary DIRECT values VERBATIM again. The
store-layer inspection of direct values is reverted; the future-envelope
remediation moves to the typed app-config extractor, which now maps an
`UnknownVersion` (and a schema-changed future version) to an upgrade/redeploy
error instead of a generic integrity 500. The store still errors on a newer
value in OUR namespace (an unknown `edgezero_kind`, a future pointer/inner
version), detected before the exact-v1 struct deserialize.
P2 -- The minimal CLI feature graph lints warning-free:
`cargo clippy -p edgezero-cli --all-targets -- -D warnings` no longer trips on
unused `ResolveFailure` payloads / `into_message`, which the read classifier now
surfaces in its redacted infra diagnostic.
|
Thanks — all seven addressed in P1 — Schema-changing v2 envelopes failed open. P1 — Operational errors becoming absence. Introduced P1 — Lossy GC listing input. All value-bearing P1 — Generic push missed unknown P2 — Permanently missing Fastly chunk had no repair path. This is the crux, and it's what reconciles this round with the prior one. A confirmed clean absence on a chunk fetch ( P2 — Store layer stopped returning direct values verbatim. Reverted the store-layer inspection of direct values ( P2 — Minimal CLI feature graph lint. Gates (all green): fmt · workspace clippy |
Address the follow-up review round (three P1 data-loss paths, three P2, one P3). P1 -- GC no longer trusts a future inner format behind a v1 pointer. After reassembling a generation, GC deserialised straight into `BlobEnvelope`, which silently ignores a bumped version or an unknown `edgezero_kind`. A newer inner format can reference generations this build cannot see, so trusting only the outer pointer's chunks as the live set could delete them as orphans. GC now runs the same `value_is_future_format` check the runtime resolver does on the reassembled bytes and fails closed. P1 -- Absence is CONFIRMED against an authoritative complete listing, never a describe 404. A proxy/endpoint or auth 404 looks exactly like a genuine item-absence, so two such reads could pass the pre-write recheck and authorise an overwrite. A root/chunk describe failure is now confirmed against a completeness- strict `config-store-entry list` (fails closed on a paginated view or a duplicate key): only a listing that OMITS the key reads as absence, and store resolution returns a typed `Ok(None)`. The stderr-classification heuristics are removed. P1 -- The local writer re-checks future-format UNDER THE LOCK. The pre-push check ran before the write lock, so a newer writer could install a v2 value in the TOCTOU window; the locked reread now re-classifies each root and refuses to clobber a newer format before the upsert. P2 -- The typed app-config extractor refuses an unknown `edgezero_kind`, not just a bumped version. serde ignores the unknown field, so a v1-shaped envelope tagged `edgezero_kind: "new_format"` would otherwise deserialize and apply on non-Fastly runtimes; it now maps to the same upgrade/redeploy remediation the generic push gives. P2 -- Local push forces `format = "inline-toml"`. An existing `format = "json"` / `"file"` next to the inline `contents` this writer emits left a contradictory store the command still reported as written; it is now overwritten (with a warning) to match what is written. P2 -- `--no-default-features` lints clean: `chunked_config` is gated to the features that use it, so a default-feature build no longer trips 23 dead-code errors. P3 -- Reconcile the `ReadConfigEntry::Corrupt` contract with the implementation: a referenced chunk CONFIRMED absent by a complete listing is repairable `Corrupt` (the blob spec repairs persistent loss by re-pushing); an absence inferred from a bare 404 is NOT `Corrupt` and fails closed. One canonical contract.
|
Thanks — all seven addressed in P1 — GC trusted a future inner format behind a v1 pointer. After reassembling a generation, P1 — A bare operational 404 was treated as confirmed absence. Replaced the stderr heuristics entirely. Absence is now confirmed against an authoritative complete listing ( P1 — Local future-format protection had a TOCTOU window. The pre-push check ran before the write lock. The local writer now re-classifies each root under the lock ( P2 — Extractor future-format handling was inconsistent. P2 — Local push preserved an incompatible store format. P2 — Default feature graph wasn't lint-clean. P3 — Read-result contract vs implementation. Reconciled Gates (all green): fmt · workspace clippy |
…ine stores Address the follow-up review round (three P1 data-loss paths, six P2, one P3). P1 -- Future-format detection no longer fails open on a non-integer `version`. `value_is_future_format`, the generic `body_is_future_envelope`, and the runtime extractor all keyed on `as_u64()`, so `"2"`, `-1`, `2.5`, or `1.0` yielded `None` and slipped through as repairable corruption / an inert GC root. Any present `version` that is not EXACTLY the JSON integer 1 is now treated as future. P1 -- The locked local downgrade guard now catches a future INNER envelope behind a valid v1 pointer (only knowable after reconstruction). Each current root is RESOLVED against the locked `contents` table before writing; a typed `FutureFormat` is refused just like a raw future value. P1 -- A JSON/file-backed local store is REFUSED, not converted. Rewriting `format` in place left a stray `file` key (a manifest the local server rejects) or would have silently discarded the sibling entries that external file holds. The push now hard-errors and points the operator at explicit migration. P2 -- Generated chunk writes are preflighted (cloud and local): a content- addressed chunk key that would clobber an existing root-like sibling or a nested generation is refused before any write. P2 -- `find_config_store_id` fails closed on a malformed or duplicate row: a row lacking a non-empty `name`/`id` could BE the requested store, so a NotFound there would authorise an overwrite. Every row must have non-empty `name` and `id`, and names must be unique. P2 -- The destructive age gate rounds a fractional creation timestamp UP. Flooring both creation and now made a true age of 59.002s compute as 60s and pass a 60s `--older-than` almost a second early. P2 -- The hard-link refusal is re-checked AFTER acquiring the write lock and again IMMEDIATELY before the rename, closing the lock-wait race the single pre-lock check left open. P2 -- CI gates the `cli`-alone and no-default-features fastly builds, which the all-features unification could otherwise mask. P2 -- The spec's downgrade bullet is reconciled to the implemented REFUSE policy (a `version: 2` prior is refused, not overwritten), with the schema-agnostic version rule and the inner-envelope + non-inline-store cases spelled out. P3 -- Docs: the migration guide predicts chunking by the stable BYTE trigger (not characters); the CLI walkthrough lists `config diff` and `config gc`; the generated-project test now runs `config gc --help` to exercise its parser.
|
Thanks — all ten addressed in P1 — Future-format detection failed open on non-integer versions. P1 — The locked downgrade guard missed future inner envelopes. P1 — JSON/file-backed local stores were converted destructively. P2 — Generated chunk writes could clobber a root-like sibling. Both paths now preflight ( P2 — Malformed store-list rows became authoritative MissingStore. P2 — Fractional timestamps passed the age gate ~1s early. P2 — Hard-link lock-wait race. The link-count check now runs again AFTER acquiring the write lock and once more IMMEDIATELY before the rename, closing the window the single pre-lock check left open. P2 — CI feature-mode isolation. Added permanent gates for P2 — Spec vs implementation. Reconciled the spec's downgrade bullet to the implemented refuse policy (a P3 — Docs & coverage. Migration guide now predicts chunking by the stable byte trigger (not characters); the CLI walkthrough lists Gates (all green): fmt · workspace clippy |
The cloud push built a per-root `roots` vector (cloned keep-sets + root values) that no code reads — leftover from the removed push-time cloud reclamation. Cloud push never reclaims (GC is explicit and store-wide), so the loop now collects only the physical entries it actually commits.
|
Thanks — addressed the one P3 in P3 — Obsolete cloud-push GC bookkeeping. The cloud push built and populated a per-root No behavior change. Gates green: fmt · workspace clippy Appreciate the thorough review across the rounds. |
Point the edgezero dependency pins at the spec/fastly-chunk-gc branch (stackpop/edgezero#314) so `cargo update` follows the PR as it evolves; the lockfile currently resolves it to 0873ec5a.
`config gc` was safe-by-default (a run without `--yes` deletes nothing), but there was no explicit way to state "just preview" — you had to know that omitting `--yes` was the dry run. Add a `--dry-run` flag that documents that intent and matches `push`/`provision`. It conflicts with `--yes` (clap-enforced), so a single run can never both preview and delete; the dispatch already treats `--dry-run || !--yes` as a dry run. Docs (CLI reference, migration guide) and the spec updated; parse + conflict tests added.
`config gc` listed only what it would delete, which made a sweep hard to trust. Surface the protected/live ROOTS it is KEEPING too — each by key, plus the live-chunk total they hold — so a dry-run is reviewable at a glance. The plan already computes the protected set; it is now threaded through GcPlan and printed (in both dry-run and real runs, and even when there is nothing to reclaim). A root shown here is never a delete candidate.
prk-Jr
left a comment
prk-Jr
left a comment
There was a problem hiding this comment.
PR Review — approved
Summary
The design pivot since the last review round is the right call, and it is genuinely shipped rather than just claimed. Automatic cloud reclamation is gone: delete_config_store_entry now has exactly one call site (execute_gc_deletes, cli.rs:2526), reachable only through the operator-invoked config gc. That removes the eventual-consistency delete hazard and the read-back TOCTOU entirely, rather than narrowing them.
All three previously-blocking findings are resolved in code, verified independently rather than taken on the resolution comments:
- Eager cloud delete / read-back race — no delete path exists on push.
push_config_entrieswrites only, with an explicit comment recording why (updated_atis not bumped by an upsert, no CAS,created_atis not a supersession proxy). - Payload leak into push logs —
redact_describe_responsereports size + field count only (not names),redact_stderrsuppresses the body entirely, and the redaction now extends well past the original finding into core:BlobEnvelopeErrorhas a hand-writtenDebugso{err:?}cannot leak whatDisplayredacts,redact_serde_pathstrips string segments because a map key is indistinguishable from a struct field, andmap_secret_errorkeeps the unused parameters in its signature specifically so the redaction is visible at the one place they could have been formatted. That last detail is a nice touch. - Duplicate roots in one batch —
reject_duplicate_root_keyson both push paths, before any expansion or I/O.
The safety architecture of config gc is the strongest part. Deriving liveness from the store rather than from metadata, grouping candidates by generation, and requiring each group to reassemble to the content-address its own keys name and round-trip byte-identically through the writer, means every destructive decision rests on a hash rather than on what an inconsistent store claims about itself. Fail-closed is applied consistently: unparseable created_at, non-bare-array listing, duplicate keys, an unclassifiable root, a pointer whose chunks do not reconstruct, a referenced chunk absent from the listing — each aborts with nothing deleted.
Approving. The findings below are all non-blocking; none of them affect correctness of the delete path.
😃 Praise
prove_generation's round-trip against the writer, and the explicit "What this does NOT prove: authorship" paragraph — treating content-addressing as a consistency check rather than a signature, and documenting the residual instead of overclaiming. (inline)execute_gc_deletes'sdeleted/stranded/uncertaintaxonomy, with shell-escaped recovery commands and a POSIX-vs-Windows caveat. (inline)- The
CorruptvsFutureFormatsplit threaded from the resolver throughclassify_resolved_readtoReadConfigEntry, so operators get "re-push to repair" or "redeploy an updated build" rather than one undifferentiated 500 — andclassify_present_bodymakes the repair contract generic, so axum/Cloudflare/Spin get it too without each adapter reimplementing it. - Treating a missing chunk at runtime as
Unavailablerather thanInternal(config_store.rs), because the store is eventually consistent across keys and a flipped root pointer can outrun its chunks to a POP. A persistent 503 is strictly better operator guidance during that window than a spurious re-push-me 500. - The CI additions close a real blind spot:
--all-featuresunifies the fastly adapter'scliandfastlyfeatures and can mask a cfg mistake, so gatingcli-only,--no-default-features, andfastly clionwasm32-wasip1is well targeted — as is running the lib tests under Viceroy, since--test contractnever exercised the colocated resolver tests.
Findings (all non-blocking)
- 🤔 Empty
item_valueon any sibling entry permanently blocksconfig gcfor that store — cli.rs:2252. Fail-closed is defensible; the diagnostic blames the listing instead of naming the key. (inline) - 🤔
future_format_reasonadds a second full JSON parse of the blob to every request — extractor.rs:909. Two of its three cases are already caught by the v1 parse/verify below; a cheap guard keeps the happy path to one parse. (inline) - ♻️ Dry-run keep-predicate hand-mirrors the real prune — cli.rs:2140 vs cli.rs:1818-1836. A shared helper would enforce by construction what a comment currently asks a future edit to remember. (inline)
- ⛏ Lock/temp sidecars not ignored in this repo — the generated-project template gets
.*.edgezero-lock, but the root.gitignoredoes not, and neither covers the.edgezero-<pid>-<n>.tmpstaging file a hard kill can leave. (inline) - ⛏ Two remaining unredacted-stderr paths —
resolve_remote_config_store_id(cli.rs:3363) andcreate_fastly_store(cli.rs:1386) interpolate rawfastlystderr, while every value-bearing path routes throughredact_stderr. Safe as written:config-store listand<kind>-store createnever receive an entry value, so there is no payload to echo. Flagging only because they are the sole exceptions to an otherwise absolute invariant — a one-line comment on each saying "no entry value reaches this command, so stderr is safe to surface" would stop a future reader filing it as an oversight, or a future edit copying the pattern somewhere it is not safe.
📌 Out of scope / before merge
- The PR title and description still describe the design this branch removed. The title says "(last-writer-wins)", and the body describes a "cloud post-commit sweep with last-writer-wins read-back guard +
delete_config_store_entry" and a push that "reclaims prior chunks only while a post-commit read-back confirms it is still the last writer" — none of which ships. The body also predates most of what landed:config gcitself,Adapter::gc_config_entries,Adapter::preflight_config_write,ReadConfigEntry::Corrupt, the cross-processManifestLock+ atomic manifest replace, the core redaction work, and the CI matrix changes. Worth rewriting before merge, since on a squash-merge it becomes the permanent commit message. No code change involved — which is why this is not blocking the approval.
Verification
Run locally against 43cec87, not taken from CI:
cargo fmt --all -- --check— PASScargo clippy --workspace --all-targets --all-features -- -D warnings— PASScargo test --workspace --all-targets— PASS (exit 0, 1304 tests across 20 suites)- GitHub checks — 13/13 green (fmt, test, format-docs, CodeQL x4, and the fastly/cloudflare/spin wasm clippy + test jobs)
…ling Address the review-round nits on the recent --dry-run / kept_roots work. P2 -- app-demo's committed lockfile still pinned edgezero-macros to syn 2, while the workspace now requires syn 3, so `cargo run --locked` in app-demo failed. Regenerate the app-demo lock (syn 3.0.3, matching the root lock), and run the app-demo CI test step with `--locked` so this drift fails loudly next time instead of being silently regenerated. P3 -- the public `run_config_gc` now rejects `dry_run` + `yes` together up front, so a library caller bypassing clap gets an explicit "mutually exclusive" error rather than the wrong-threshold message or a silent preview. P3 -- the retained-root report says "retained root(s)", not "live": the set can include a root that is protected but NOT runtime-readable (warned separately), which "live" mislabels. P3 -- reattach the `gc_fastly_config_store` fail-closed doc block to that function; adding `append_kept_roots_report` had left the doc on the helper and `gc_fastly_config_store` undocumented.
Address the review-round P3 nits on the config gc reporting. - The dry_run+yes conflict error no longer contradicts the valid default: "Pass at most one (omit both for the default dry-run preview)", and the public `# Errors` doc now lists the conflict. The precedence test is a table proving the conflict wins over every `--older-than` shape (missing/zero/malformed/ overflow), so a library caller can never get the wrong-threshold error. - Retained-root reporting no longer calls protected-but-unreadable roots (or their chunks) "live": the heading/summary now say "referenced chunk(s)", since a root that fails the writer split check is conservatively protected, not runtime-live. Added a wording/count/empty-store unit test and extended the unreadable-root test to assert it is reported as retained, never "live". - Each proven generation is deleted in canonical chunk-INDEX order, not the remote listing order, so preview and failure-recovery (deletion stops at the first failure) are deterministic regardless of how Fastly lists the store. Adds `chunk_key_index` + its test. - Reattached the fail-closed Rustdoc to `gc_fastly_config_store`.
… extractor parse, gitignore Non-blocking follow-ups from the approved PR review. - `list_config_store_entries` now names the offending KEY (not just an index) and tells the operator what to do when a legitimate empty-valued sibling blocks a sweep, instead of blaming the "listing". - Extract `is_prunable_leaf` as the single source of truth for the local keep-predicate, shared by the real prune and the dry-run count, so the previewed orphan count can never drift from what `--yes` actually removes. - The typed app-config extractor no longer parses the whole blob as JSON twice on every request: a cheap `edgezero_kind` substring gate keeps the generic `Value` reparse (`future_format_reason`) off the happy path. The version case is still caught by `verify()`; same semantics and redaction, one parse on the hot path. - `.gitignore` (template + this repo's root) now also ignores the atomic-replace staging temp (`.*.edgezero-*.tmp`), which a SIGKILL/panic mid-push can strand, and the root globs cover `examples/app-demo`'s `fastly.toml` sidecars.
|
Thanks for the approve — addressed the four non-blocking follow-ups in Empty-value diagnostic (cli.rs:2252). Kept the fail-closed (belt-and-braces) but made it actionable: the error now names the offending key ( Shared prune predicate (cli.rs:2140). Extracted Double JSON parse (extractor.rs:909). The hot path no longer parses the blob as gitignore (gitignore.hbs:17). Both the template and this repo's root Gates green: fmt · workspace clippy |
aram356
changed the title
config gc reclaims orphaned chunk entries (store-wide)
3 participants
Summary
Oversized Fastly app-config (an envelope over Fastly's ~8 000-byte per-entry limit) is stored as content-addressed chunks + a root pointer. Because chunk keys are addressed by the envelope SHA, re-pushing a changed config rewrites every chunk key and orphans the entire previous generation. Left unreclaimed, a store accumulates dead chunk entries on every push (both the cloud Config Store and the local
fastly.toml[local_server.config_stores.<name>.contents]).This PR reclaims those orphans safely, split by path:
config push --local) prunes the prior generation in the same lockedfastly.tomlrewrite — it's a single file replaced atomically, so immediate reclamation is safe.config gc— the operator supplies the one fact the platform cannot.config gc(Fastly)A store-wide sweep that is safe by construction:
--older-thanis the operator's safety assertion, and it is about the whole store: "no root here changed within this window, and no writer is targeting it." An orphan's effective age ismin(its own age, how long the current config has been live), so a recently re-pushed sibling constrains the window. Required for--yes;0is rejected.strandedvsuncertain. Delete order is the canonical chunk index, so preview and recovery are deterministic.Safety properties (hardened over the review rounds)
versionthat is not exactly the JSON integer1, or an unknownedgezero_kind, fails closed everywhere destructive (GC, local prune, and the CLI overwrite path) and is never overwritten by this v1 CLI.json/file) store rather than corrupt it, and refuse hard-linked manifests.Key files
docs/superpowers/specs/2026-07-07-fastly-chunk-gc.md,.../plans/…crates/edgezero-adapter-fastly/src/chunked_config.rscrates/edgezero-adapter-fastly/src/cli.rsconfig gc(plan_gc_reclamation, generation proof, age gates,execute_gc_deletes); local prune in the lockedfastly.tomlrewrite; authoritative-listing reads; collision/downgrade guardscrates/edgezero-cli/src/{args,config}.rsconfig gcsubcommand + flags (--older-than,--dry-run,--yes), untyped runnercrates/edgezero-core/src/extractor.rsdocs/guide/*,.gitignore,.github/workflows/*--lockedgatesCloses
Closes #313
Test plan
cargo test -p edgezero-adapter-fastly --features cli— 205 passcargo test -p edgezero-adapter-fastly --features fastly --target wasm32-wasip1under Viceroy — 84 runtime + 6 contract passcargo test --workspace --all-targets— all pass;edgezero-cli— 176 pass; app-demo builds under--locked(CI now runscargo test --lockedthere)cargo clippy --workspace --all-targets --all-features -- -D warnings, pluscli-only and--no-default-featuresfastly-adapter clippy — cleancargo fmt --all -- --check, feature check (fastly cloudflare spin), and the wasm clippy matrix (wasip1fastly/fastly cli, wasip2 spin, wasm32-unknown cloudflare) — cleanCoverage highlights: value-based live-set incl. chunk-shaped root keys; generation proof (byte-identical writer reproduction); fail-closed on unknown/future/malformed/incomplete/paginated/ambiguous/duplicate state; both age gates;
--yesrequires a non-zero--older-thanat the adapter boundary;--dry-run/--yesconflict (conflict wins over every threshold); deterministic delete order; partial-deletestranded/uncertaintaxonomy; local prune / shrink-to-direct / sibling preservation / locked downgrade + collision refusal / non-inline-store refusal; redaction sentinels on every value-bearing path.Checklist