Changelog CDN fetchers: skip unchanged folders via shallow registry maps - #3801
Conversation
8f92f56 to
7f4181c
Compare
d460310 to
08cd980
Compare
The draft predated the #3738 review rework: RegistryReconciler is now BundleRegistryReconciler and reconciles the bundle/{product}/ tree only, the changelog/ pool manifests remain legacy client-authored pass-through (changelog bundle still enumerates pools through them, RFC #698 replaces that), the registry reconcile/verify operator CLI was dropped with #3741, and the scrubber now also maintains the shallow per-tree folder-to-token maps (consumer side: #3801). Infra bullets match the applied docs-infra#360 IAM (no registry-operator grant, no private ListBucket) and observability as it exists (metric stream to docs-o11y; alerts and runbook tracked in docs-eng-team#692).
* Scrubber Lambda owns the public changelog registry via state reconcile
Phase 1 of elastic/docs-eng-team#688. The public registry.json was a log of
upload operations (client-written, pass-through copied); every known
consistency gap followed from that. The scrubber Lambda now derives it from
the public bucket's actual state: registry = f(state), never f(event).
- Extract the Lambda's top-level handler logic into testable classes in
Elastic.Changelog: ScrubberProcessor (batch coalescing by key and group,
object-level reconcile with post-write source validation) and
RegistryReconciler (delimited/paginated group listing, ETag reuse with
amends always recomputed, semantic idempotence, conditional PUT/DELETE
with bounded jittered retries on 412/409, newer-schema refusal).
Program.cs is now a thin adapter.
- Retire the registry pass-through in the same deploy: registry-key events
only schedule a group reconcile, so client-authored JSON no longer
reaches the public bucket uninspected.
- Add a producer (algorithm version) field to the manifest; a mismatch —
including legacy pass-through manifests — forces a full metadata
recompute and a write even when entries are identical.
- Emit per-invocation reconcile metrics as CloudWatch EMF (the Phase 0
observability item that could only land with the reconciler).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* Add changelog registry reconcile and verify commands
Phase 2 of elastic/docs-eng-team#688. The cutover/heal tooling for the
Lambda-owned public registry:
- `changelog registry reconcile` plans groups (one scope, or the union of
both buckets so orphan public groups are covered) and sends one versioned,
discriminated reconcile message per group to the scrubber queue —
{kind, version, scope, group, correlation_id}, validated through
ChangelogKeys on both ends. The CLI never mutates S3; the Lambda stays the
public bucket's single writer. --dry-run prints the plan; the non-dry-run
path asks for confirmation (--yes for CI). Each run stamps one correlation
id and prints a ledger line per group.
- On a reconcile message the Lambda performs a full group heal:
object-level reconcile over the union of both buckets' listings (copy
what's live, delete what isn't), then the group reconcile — recovering
lost/DLQ-expired scrub events. Requires the new optional
PRIVATE_BUCKET_NAME Lambda env var; malformed messages are rejected to
the DLQ where the Phase 0 alarm surfaces them.
- `changelog registry verify` is the read-only sibling and cutover gate:
compares each public manifest against what a reconcile would write (same
listing spec and entry rules by construction) and reports divergence as
missing/stale/corrupt/object-divergent, with unsupported schemas reported
distinctly.
- Fix the manifest ETag wire format: the snake_case policy serialized the
producer-side field as "e_tag" while consumers and the documented format
read "etag" — recorded ETags were invisible to every consumer.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* Suppress CA1001 on ChangelogRegistryServiceTests
Same suppression RegistryBuilderTests carries: xUnit owns the test class
lifetime and TestDiagnosticsCollector needs no disposal in these tests.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* Retire the client-side registry refresh from changelog upload
The scrubber Lambda is the sole producer of the public registry.json,
reconciled from public bucket state on the S3 events every upload already
emits (elastic/docs-eng-team#688 Phase 3). Uploads now write YAML objects
only; RegistryBuilder and the private-manifest write path are removed, and
the amend end-to-end test exercises RegistryReconciler instead.
* Rewrite changelog registry docs for scrubber Lambda ownership
The registry docs still described the retired model: client-side refresh,
registry pass-through, pre-scrub ETags, a 1 h CloudFront TTL (caching is
disabled), and a refresh "skipped for --artifact-type changelog". Documents
the reconciler as sole producer, the public-object ETag, convergence
semantics, absent-vs-empty manifests, the reconcile message contract, and
the registry reconcile/verify operator commands (docs-eng-team#688 Phase 4).
* Align registry docs with the merged #3738 rework and #3760
The draft predated the #3738 review rework: RegistryReconciler is now BundleRegistryReconciler and reconciles the bundle/{product}/ tree only, the changelog/ pool manifests remain legacy client-authored pass-through (changelog bundle still enumerates pools through them, RFC #698 replaces that), the registry reconcile/verify operator CLI was dropped with #3741, and the scrubber now also maintains the shallow per-tree folder-to-token maps (consumer side: #3801). Infra bullets match the applied docs-infra#360 IAM (no registry-operator grant, no private ListBucket) and observability as it exists (metric stream to docs-o11y; alerts and runbook tracked in docs-eng-team#692).
---------
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
* Scrubber Lambda owns the public changelog registry via state reconcile Phase 1 of elastic/docs-eng-team#688. The public registry.json was a log of upload operations (client-written, pass-through copied); every known consistency gap followed from that. The scrubber Lambda now derives it from the public bucket's actual state: registry = f(state), never f(event). - Extract the Lambda's top-level handler logic into testable classes in Elastic.Changelog: ScrubberProcessor (batch coalescing by key and group, object-level reconcile with post-write source validation) and RegistryReconciler (delimited/paginated group listing, ETag reuse with amends always recomputed, semantic idempotence, conditional PUT/DELETE with bounded jittered retries on 412/409, newer-schema refusal). Program.cs is now a thin adapter. - Retire the registry pass-through in the same deploy: registry-key events only schedule a group reconcile, so client-authored JSON no longer reaches the public bucket uninspected. - Add a producer (algorithm version) field to the manifest; a mismatch — including legacy pass-through manifests — forces a full metadata recompute and a write even when entries are identical. - Emit per-invocation reconcile metrics as CloudWatch EMF (the Phase 0 observability item that could only land with the reconciler). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Add changelog registry reconcile and verify commands Phase 2 of elastic/docs-eng-team#688. The cutover/heal tooling for the Lambda-owned public registry: - `changelog registry reconcile` plans groups (one scope, or the union of both buckets so orphan public groups are covered) and sends one versioned, discriminated reconcile message per group to the scrubber queue — {kind, version, scope, group, correlation_id}, validated through ChangelogKeys on both ends. The CLI never mutates S3; the Lambda stays the public bucket's single writer. --dry-run prints the plan; the non-dry-run path asks for confirmation (--yes for CI). Each run stamps one correlation id and prints a ledger line per group. - On a reconcile message the Lambda performs a full group heal: object-level reconcile over the union of both buckets' listings (copy what's live, delete what isn't), then the group reconcile — recovering lost/DLQ-expired scrub events. Requires the new optional PRIVATE_BUCKET_NAME Lambda env var; malformed messages are rejected to the DLQ where the Phase 0 alarm surfaces them. - `changelog registry verify` is the read-only sibling and cutover gate: compares each public manifest against what a reconcile would write (same listing spec and entry rules by construction) and reports divergence as missing/stale/corrupt/object-divergent, with unsupported schemas reported distinctly. - Fix the manifest ETag wire format: the snake_case policy serialized the producer-side field as "e_tag" while consumers and the documented format read "etag" — recorded ETags were invisible to every consumer. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Suppress CA1001 on ChangelogRegistryServiceTests Same suppression RegistryBuilderTests carries: xUnit owns the test class lifetime and TestDiagnosticsCollector needs no disposal in these tests. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Retire the client-side registry refresh from changelog upload The scrubber Lambda is the sole producer of the public registry.json, reconciled from public bucket state on the S3 events every upload already emits (elastic/docs-eng-team#688 Phase 3). Uploads now write YAML objects only; RegistryBuilder and the private-manifest write path are removed, and the amend end-to-end test exercises RegistryReconciler instead. * Rewrite changelog registry docs for scrubber Lambda ownership The registry docs still described the retired model: client-side refresh, registry pass-through, pre-scrub ETags, a 1 h CloudFront TTL (caching is disabled), and a refresh "skipped for --artifact-type changelog". Documents the reconciler as sole producer, the public-object ETag, convergence semantics, absent-vs-empty manifests, the reconcile message contract, and the registry reconcile/verify operator commands (docs-eng-team#688 Phase 4). * Align registry docs with the merged #3738 rework and #3760 The draft predated the #3738 review rework: RegistryReconciler is now BundleRegistryReconciler and reconciles the bundle/{product}/ tree only, the changelog/ pool manifests remain legacy client-authored pass-through (changelog bundle still enumerates pools through them, RFC #698 replaces that), the registry reconcile/verify operator CLI was dropped with #3741, and the scrubber now also maintains the shallow per-tree folder-to-token maps (consumer side: #3801). Infra bullets match the applied docs-infra#360 IAM (no registry-operator grant, no private ListBucket) and observability as it exists (metric stream to docs-o11y; alerts and runbook tracked in docs-eng-team#692). --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
08cd980 to
8cf2c34
Compare
There was a problem hiding this comment.
Requesting changes: the shallow-map cancellation/timeout path currently breaks graceful degradation and can fail all product fetches for a base URI after a single map timeout.
What is this? | From workflow: PR Review
Give us feedback! React with 🚀 if perfect, 👍 if helpful, 👎 if not.
| await using var stream = await response.Content.ReadAsStreamAsync(ctx).ConfigureAwait(false); | ||
| return await JsonSerializer.DeserializeAsync(stream, ChangelogRegistryJsonContext.Default.DictionaryStringString, ctx).ConfigureAwait(false); | ||
| } | ||
| catch (Exception ex) when (ex is not OperationCanceledException) |
There was a problem hiding this comment.
FetchShallowMapAsync currently treats every OperationCanceledException as fatal, but HttpClient timeout is surfaced as TaskCanceledException (which inherits OperationCanceledException) even when the caller token was not canceled. In that case the map fetch does not degrade to null and instead faults the shared Lazy<Task<...>> in _shallowMaps, so all subsequent product fetches for the same base URI fail immediately in this run.
That creates a real behavior regression versus the intended "map failures degrade to normal per-product registry fetch" path.
Please distinguish caller cancellation from transport timeout/transient cancellation here (for example, rethrow only when ctx.IsCancellationRequested, otherwise log+return null), and avoid keeping a failed/canceled lazy entry cached for the rest of the run.
… fetch HttpClient surfaces its own request timeout as a TaskCanceledException even when the caller's token was never signaled. Treating every OperationCanceledException as fatal faulted the shared per-base-URI Lazy<Task<...>> permanently, taking every subsequent product fetch down with it for the rest of the run. Only rethrow when ctx.IsCancellationRequested; otherwise degrade to null like any other transport failure. On a genuine cancellation, evict the faulted entry so a later call with a live token can retry instead of being stuck forever. Co-authored-by: Cursor <cursoragent@cursor.com>
Stale: this finding (shallow-map cancellation/timeout swallowing) was fixed in 97fef92 (pushed 2026-08-26T13:43:04Z, after this review was submitted at 13:01:51Z), verified by new regression tests.
Mpdreamz
left a comment
There was a problem hiding this comment.
Reviewed. Fixed a real bug flagged by the automated PR review: the shallow changelog-map fetch treated HttpClient's own request-timeout TaskCanceledException as a genuine cancellation, permanently faulting the shared per-base-URI Lazy<Task<...>> and taking every subsequent product fetch down with it for the rest of the run. Now only rethrows when the caller's token was actually signaled (ctx.IsCancellationRequested); a transport timeout degrades to null like any other transport failure, and a genuine cancellation evicts the faulted cache entry so a later call with a live token can retry. Added three regression tests (FetchAsync_ShallowMapTimesOut_DegradesInsteadOfFaultingLaterFetches, FetchAsync_ShallowMapTimesOut_DoesNotPoisonLaterProductsInTheSameRun, FetchAsync_CallerCancels_PropagatesCancellationRatherThanDegrading). Full test suite passes; CLI schema unchanged.
Implements the consumer half of the shallow per-tree registry maps introduced in #3738 (stacked on that PR's branch): the bundle CDN fetcher now consults
bundle/registry.jsonbefore fetching per-product registries, and skips folders whose content demonstrably hasn't changed.Refs: elastic/docs-eng-team#737, elastic/docs-eng-team#688, #3738.
What it does
CdnChangelogFetcherfetches the tree's shallow map (bundle/registry.json, shape{"<product>": "<token>"}) once per fetcher run, memoized per base URI. For each product:bundle/{product}/registry.jsonfetch is skipped and the locally cached registry is reused. Bundle content then resolves through the existing ETag-keyed cache, so an unchanged folder with a warm cache produces zero per-folder requests (asserted in tests).Graceful degradation (non-negotiable, preserved)
A shallow map that is absent (404), unparseable, or fails to fetch degrades to
nullinsideFetchShallowMapAsync— a single debug log, no errors, no warnings — and every per-product registry is fetched exactly as before the map existed. Pre-cutover CDNs and buckets without maps keep working unchanged. Tokens are treated as fully opaque: compared with ordinal string equality only, never parsed.Cache/token bookkeeping design
registry-{product}-{token}in the fetcher's existing memory + disk cache ({ApplicationData}/changelog-bundles/), the same store and conventions the ETag-keyed bundle cache (changelog-{product}-{file}-{etag}) already uses. Embedding the token in the key makes a token mismatch a plain cache miss under the new key — no separate "last-seen token" state to keep consistent.ShallowRegistryReconciler) excludes group manifests from the token digest precisely so rewriting a manifest cannot invalidate consumer caches: same token ⇒ same folder listing ⇒ same derived registry.Deliberately not covered:
CdnChangelogEntryFetcherThe changelog-pool entry fetcher (
changelog/{org}/{repo}/{branch}) has no local cache of any kind — it downloads entries and returns them directly to the bundle command. A matching pool token could therefore never skip anything: there is no cached state a skip could reuse, and honoring the issue's "no behavior change" constraint would require inventing a parallel content store for entry YAML. Per the review direction ("scope the opt-out to whatever cache genuinely exists; correctness over cleverness"), that fetcher is left unchanged; fetchingchangelog/registry.jsonthere would only add a wasted request per run. If a pool-level entry cache is ever introduced, the same token-keyed pattern applies directly.Test plan
All in
CdnChangelogFetcherTests(fakeHttpMessageHandler+MockFileSystem, per the existing pattern):Checks run:
dotnet format(clean),./build.sh build --skip-dirty-check(pass),dotnet publish src/tooling/docs-builder -c ReleaseAOT publish (zero trim/AOT warnings; the shallow map type is registered on the source-generatedChangelogRegistryJsonContext),dotnet testforElastic.Documentation.Configuration.Tests(608 passed) andElastic.Changelog.Tests(865 passed). No CLI surface changes, so nodocs/cli-schema.jsonregen.