Skip to content

changelog: render CDN-hosted bundles in the {changelog} directive#3436

Merged
cotti merged 14 commits into
mainfrom
changelog_directive_s3
Jun 19, 2026
Merged

changelog: render CDN-hosted bundles in the {changelog} directive#3436
cotti merged 14 commits into
mainfrom
changelog_directive_s3

Conversation

@cotti

@cotti cotti commented Jun 1, 2026

Copy link
Copy Markdown
Contributor

Why

Scrubbed changelog bundles live in the public S3 / CloudFront bucket, but the {changelog} directive could previously only render bundles checked into the same repo. That meant a docset couldn't show another product's changelog without vendoring its bundle files, and there was no manifest to discover bundles without listing S3.

What

  • Producer: when a bundle is uploaded, publish/update a per-product registry.json manifest in the public bucket (carried through by the scrubber's pass-through copy). Manifest writes use S3 conditional PUTs (If-Match/If-None-Match) with bounded retries so parallel uploads merge safely instead of clobbering each other.
  • Consumer: a new :cdn: <product> option on the {changelog} directive fetches that product's registry.json plus the referenced bundles from CloudFront and renders them inline. A new :version: option filters to a single version (matched against bundle target or filename); in CDN mode it only downloads the matching bundle.
  • Naming: the per-product manifest is registry.json. "registry-index" is reserved for a possible future bucket-wide meta-registry, so the producer/consumer types and keys were renamed accordingly.

How

CDN fetching happens inside the synchronous Markdig finalize pass, so CdnChangelogFetcher does a synchronous HTTP fetch backed by a static in-memory cache (keyed by base URL + product + version) and the bundles it returns are self-contained. The base URL defaults to the production CloudFront domain and can be overridden via DOCS_BUILDER_CHANGELOG_CDN for testing.

Made with Cursor

cotti and others added 2 commits June 1, 2026 10:23
Bundle uploads now refresh a per-product registry-index.json so consumers
(the upcoming changelog directive cdn: mode) can enumerate bundles without
an S3 listing. The builder does a read-merge-conditional-PUT
(If-Match/If-None-Match) with bounded retries to stay safe under concurrent
uploads, writes the manifest to the private bucket, and relies on the
scrubber's existing pass-through to mirror it to the public CDN bucket.
Refresh failures are non-fatal: bundle objects are unaffected.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Lets the {changelog} directive load bundles published to the public
S3/CloudFront bucket via :cdn:, with optional :version: filtering, so a
docset can render another product's changelog without the bundle files
living in-repo. Also renames the per-product manifest from
registry-index.json to registry.json, reserving "registry-index" for a
possible future bucket-wide meta-registry.

Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
@cotti cotti requested review from a team as code owners June 1, 2026 14:39
@cotti cotti added the feature label Jun 1, 2026
@cotti cotti requested a review from reakaleek June 1, 2026 14:39
@cotti cotti temporarily deployed to integration-tests June 1, 2026 14:39 — with GitHub Actions Inactive
@coderabbitai

coderabbitai Bot commented Jun 1, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

This PR implements a complete end-to-end changelog bundle registry system enabling CDN-backed publishing and consumption. Producers upload bundles and trigger per-product registry.json manifest refresh with optimistic S3 concurrency control and configurable retry logic. Consumers declare products in docset.yml, which are validated and prefetched at build startup, then accessed via new :cdn: and :version: directive options with in-memory resolution. The system includes comprehensive documentation, shared data contracts for registries and bundles, consumer-side bundle loading APIs with YAML parsing and merge support, a dedicated CDN fetcher with HTTP pooling and per-bundle error resilience, build-time release notes prefetching through a resolver abstraction, directive rendering with product inference and version filtering, upload service integration triggering registry refresh, registry builder orchestration with optimistic concurrency and retry handling, and full test coverage across configuration, fetching, directive processing, and manifest publishing.

Possibly related PRs

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 18.06% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title 'changelog: render CDN-hosted bundles in the {changelog} directive' clearly and concisely describes the main feature being added—CDN support for the changelog directive.
Description check ✅ Passed The description comprehensively covers the pull request goals: why CDN support is needed, what changes are made (producer registry, consumer directive options, naming), and how it works (startup prefetching instead of sync fetching).
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
✨ Simplify code
  • Create PR with simplified code
  • Commit simplified code in branch changelog_directive_s3

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/Elastic.Documentation.Configuration/ReleaseNotes/CdnChangelogFetcher.cs`:
- Around line 65-67: The current code memoizes any result from FetchUncached
(var bundles = FetchUncached(...); _ = Cache.TryAdd(cacheKey, bundles);) which
can store empty or partial results; change this to only cache a clean, non-empty
fetch: update FetchUncached to return a result type (e.g., a struct/class or
tuple) that includes the bundles plus a success/hasWarnings flag (or throw on
registry failures), then in the caller check that result.Bundles is not empty
and result.HasWarnings is false (or result.Success is true) before calling
Cache.TryAdd(cacheKey, result.Bundles); otherwise skip caching to avoid
persisting transient CDN/404 failures.

In `@src/infra/docs-lambda-changelog-scrubber/README.md`:
- Around line 33-35: Update the README wording to clarify that the handler does
not copy arbitrary .json files: change the second bullet to state that only
registry manifests (keys matching RegistryKey.IsRegistry(...)) are passed
through by the handler; mention that other .json objects are skipped. Reference
the handler and the RegistryKey.IsRegistry(...) predicate to make the behavior
explicit.

In `@src/services/Elastic.Changelog/Uploading/RegistryBuilder.cs`:
- Around line 251-256: The catch-all that logs "Existing manifest ... could not
be parsed" is swallowing transient I/O/S3 errors and allowing a conditional
If-Match PUT (using the captured etag) to overwrite a valid manifest; change the
catch to only handle manifest parse-related exceptions (e.g., JsonException,
FormatException, InvalidDataException) or explicitly detect a parse failure, log
and return the rebuilt manifest as before, but rethrow or let other exceptions
(IOException, AmazonS3Exception/HttpRequestException, etc.) bubble up so
transient read errors are not treated as a corrupt manifest; locate the
try/catch around the response parsing in RegistryBuilder (the block that reads
response.ETag and parses the manifest, uses _logger.LogWarning and returns ([],
etag)) and update the exception handling accordingly.

In `@tests/Elastic.Markdown.Tests/Directives/ChangelogBasicTests.cs`:
- Around line 339-372: This test mutates the process-wide env var EnvVar via
Environment.SetEnvironmentVariable inside InitializeAsync(), causing flakiness;
instead either serialize the test or remove the global mutation by using an
injectable seam: change the test to call
CdnChangelogFetcher.PrimeCacheForTesting with a configurable base (add an
overload or setter on CdnChangelogFetcher to accept the CdnBase/Uri directly)
and stop setting Environment.SetEnvironmentVariable(EnvVar, CdnBase) (or restore
via a test-scoped lock); locate InitializeAsync(), the
Environment.SetEnvironmentVariable calls, and the PrimeCacheForTesting call and
update the test to pass the CDN base via the new injection point (or wrap the
env change in a process-wide test lock) so no global env var is mutated for
other tests.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 2c7ddd7d-5b9c-4326-b557-c37c8b7ac4b9

📥 Commits

Reviewing files that changed from the base of the PR and between bc1b00c and a5a8114.

📒 Files selected for processing (22)
  • docs/development/changelog-bundle-registry.md
  • docs/development/toc.yml
  • docs/syntax/changelog.md
  • src/Elastic.Documentation.Configuration/Elastic.Documentation.Configuration.csproj
  • src/Elastic.Documentation.Configuration/ReleaseNotes/BundleLoader.cs
  • src/Elastic.Documentation.Configuration/ReleaseNotes/CdnChangelogFetcher.cs
  • src/Elastic.Documentation.Configuration/ReleaseNotes/ChangelogRegistry.cs
  • src/Elastic.Documentation/ReleaseNotes/ChangelogVersionMatch.cs
  • src/Elastic.Markdown/Myst/Directives/Changelog/ChangelogBlock.cs
  • src/Elastic.Markdown/Myst/Directives/DirectiveHtmlRenderer.cs
  • src/infra/docs-lambda-changelog-scrubber/Program.cs
  • src/infra/docs-lambda-changelog-scrubber/README.md
  • src/services/Elastic.Changelog/Uploading/ChangelogUploadService.cs
  • src/services/Elastic.Changelog/Uploading/Registry.cs
  • src/services/Elastic.Changelog/Uploading/RegistryBuilder.cs
  • src/services/Elastic.Changelog/Uploading/RegistryKey.cs
  • tests/Elastic.Changelog.Tests/Uploading/ChangelogUploadServiceTests.cs
  • tests/Elastic.Changelog.Tests/Uploading/RegistryBuilderTests.cs
  • tests/Elastic.Changelog.Tests/Uploading/RegistryKeyTests.cs
  • tests/Elastic.Documentation.Configuration.Tests/ReleaseNotes/BundleLoaderFromContentTests.cs
  • tests/Elastic.Documentation.Configuration.Tests/ReleaseNotes/CdnChangelogFetcherTests.cs
  • tests/Elastic.Markdown.Tests/Directives/ChangelogBasicTests.cs

Comment thread src/Elastic.Documentation.Configuration/ReleaseNotes/CdnChangelogFetcher.cs Outdated
Comment thread src/infra/docs-lambda-changelog-scrubber/README.md
Comment thread src/services/Elastic.Changelog/Uploading/RegistryBuilder.cs Outdated
Comment thread tests/Elastic.Markdown.Tests/Directives/ChangelogBasicTests.cs Outdated
cotti and others added 2 commits June 1, 2026 11:53
Follows #3434, which moved bundle/changelog artifacts from {product}/bundles/
and {product}/changelogs/ to the singular {product}/bundle/ and
{product}/changelog/. The CDN consumer fetched bundles from the old plural
path, so it would 404 against the new layout; this points it at
{product}/bundle/{file} and updates the registry docs/comments/tests to match.
The directive's local-folder convention (changelog/bundles/) is unchanged.

Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
@cotti cotti had a problem deploying to integration-tests June 1, 2026 14:57 — with GitHub Actions Error
PhysicalDocsetNavigationIncludesNestedTocs snapshots the real docs/development
TOC. Adding changelog-bundle-registry.md introduced a fourth nav item (a file
leaf), so the expected counts are bumped from 3 to 4 (and file leaves 0 to 1).

Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
@cotti cotti temporarily deployed to integration-tests June 1, 2026 14:59 — with GitHub Actions Inactive
@Mpdreamz

Mpdreamz commented Jun 1, 2026

Copy link
Copy Markdown
Member

Quick ponder:

:cdn: <product>

Can that be optional? For most cases we can infer the product from the repository that holds the doc?

Review fixes:
- CdnChangelogFetcher only memoizes a clean, non-empty fetch so a transient
  CDN/replication failure no longer pins an empty/partial result for the
  process lifetime.
- RegistryBuilder narrows the existing-manifest catch to JsonException, so a
  transient S3/IO read error bubbles to the best-effort refresh handler instead
  of letting an If-Match PUT overwrite a valid manifest with partial data.
- Scrubber README clarifies only registry manifests (RegistryKey.IsRegistry)
  pass through; other .json keys are skipped.
- CDN render test primes the fetcher cache against the default base instead of
  mutating the process-wide DOCS_BUILDER_CHANGELOG_CDN env var.

Feature:
-: cdn: product is now optional and inferred from the repository name when
  omitted (common 1:1 repo/product case); multi-product repos still specify it
  explicitly, and a clear error is emitted when it cannot be inferred.
Co-authored-by: Cursor <cursoragent@cursor.com>
@cotti cotti had a problem deploying to integration-tests June 1, 2026 16:08 — with GitHub Actions Error
@cotti

cotti commented Jun 1, 2026

Copy link
Copy Markdown
Contributor Author

Quick ponder:

:cdn: <product>

Can that be optional? For most cases we can infer the product from the repository that holds the doc?

Ah, yes. Added inferring for the product as default.

@cotti cotti temporarily deployed to integration-tests June 1, 2026 16:12 — with GitHub Actions Inactive
Import Elastic.Documentation.Configuration and use the unqualified Paths
reference in the CDN inference test instead of the fully-qualified name.

Co-authored-by: Cursor <cursoragent@cursor.com>
@cotti cotti temporarily deployed to integration-tests June 2, 2026 11:48 — with GitHub Actions Inactive

@technige technige left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice feature overall — the optimistic-concurrency design and the security guards are solid. A few things I'd suggest looking at before merging.

Found = LoadedBundles.Count > 0;
}

private void ApplyLoadedBundles(BundleLoader loader, IReadOnlyList<LoadedBundle> loadedBundles)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: loader is declared but never used inside ApplyLoadedBundles — looks like it was left over from the refactoring. Would it be okay to drop the parameter? The CDN call site on line 520 constructs a fresh BundleLoader just to satisfy the signature right now.

return;
}

CdnProduct = product;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: CdnProduct is assigned here, before LoadCdnBundles has had a chance to validate the product name. That means an invalid product like "invalid$product" ends up stored on the block even when an error is emitted (the test CapturesCdnProductOption documents this). Would it be cleaner to move IsValidCdnProduct check up here and only assign CdnProduct after it passes?

private static readonly ConcurrentDictionary<string, IReadOnlyList<LoadedBundle>> Cache = new(StringComparer.Ordinal);

private readonly ILogger _logger = logFactory.CreateLogger<CdnChangelogFetcher>();
private readonly HttpClient _httpClient = handler is null ? new HttpClient() : new HttpClient(handler, disposeHandler: false);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: HttpClient is created per CdnChangelogFetcher instance but neither the fetcher nor ChangelogBlock implements IDisposable, so each instance leaks a socket handle. In practice the static cache means most fetcher instances never actually use _httpClient, but it's still a resource leak. Would it be worth either making CdnChangelogFetcher : IDisposable (and disposing after LoadCdnBundles returns), or sharing a static/injected HttpClient the way CrossLinkFetcher probably does?

}

var fetcher = new CdnChangelogFetcher(NullLoggerFactory.Instance, Build.ReadFileSystem);
var loadedBundles = fetcher.Fetch(baseUri, product, VersionFilter, msg => this.EmitError(msg), msg => this.EmitWarning(msg), Cancel.None);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Cancel.None here means a build cancellation can't interrupt an in-flight CDN fetch. I understand this is a constraint of the synchronous Markdig pipeline — would a short comment be useful for the next reader so the choice doesn't look accidental?

byFile[entry.File] = entry;

return byFile.Values
.OrderByDescending(b => b.Target ?? string.Empty, StringComparer.Ordinal)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question: StringComparer.Ordinal sorts by string bytes, so "9.10.0" sorts before "9.9.0" (lexicographic). The docs say the manifest is "sorted newest first" — is ordinal close enough for the expected range of version strings, or would using the same VersionOrDate.Parse comparator the consumer uses be worth it here? (The consumer re-sorts anyway, so this only affects how registry.json looks on disk.)

.MustHaveHappenedTwiceExactly();

// The successful (second) PUT used the re-read ETag and merged both the concurrent and local entries.
_puts.Should().ContainSingle();

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: _puts.Should().ContainSingle() here is checking the captured list, which only holds the PUT that didn't throw — the first (failed) call throws before _puts.Add runs. That's correct, but it's easy to misread as "only one PUT was attempted". A short comment like // first PUT threw, so only the successful retry is captured might save the next reader a double-take.

- "999"
""")
], _ => { });
CdnChangelogFetcher.PrimeCacheForTesting(new Uri(ChangelogBlock.DefaultCdnBaseUrl), Product, null, bundles);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: PrimeCacheForTesting takes new Uri(ChangelogBlock.DefaultCdnBaseUrl), which ties the test to the production constant being internal. That's a reasonable tradeoff given the synchronous-pipeline constraint, but would a brief comment here explaining why the production default URL has to match help future test authors avoid confusion?

""")
{
[Fact]
public void CapturesCdnProductOption() => Block!.CdnProduct.Should().Be("invalid$product");

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: This asserts CdnProduct == "invalid$product" even though the product fails validation. It accurately documents current behaviour, but it made me wonder whether CdnProduct should remain unset when the product is invalid (i.e., only assigned after IsValidCdnProduct passes). Leaving here as a question in case that's worth revisiting alongside the suggestion above.

@lcawl

lcawl commented Jun 15, 2026

Copy link
Copy Markdown
Contributor

I'm happy to test this out if a registry.json is added to the S3 bucket (e.g. for the cloud-serverless bundles). If that won't occur until after this PR is merged, just let me know and I'll happily start using it then (so I don't have to keep generating markdown output for serverless release docs).

Comment thread docs/syntax/changelog.md Outdated
:::
```

The value names the product (must match `[a-zA-Z0-9_-]+`) and maps to `{product}/registry.json` plus the bundles it lists on the CDN. The value is **optional**: leave it blank to infer the product from the repository that holds the doc (the common case where the repository name is the product id, for example the `elasticsearch` repo renders the `elasticsearch` product).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The value names the product (must match [a-zA-Z0-9_-]+)

My assumption is that it must also match a valid value from https://github.com/elastic/docs-builder/blob/main/config/products.yml Is that correct? If so, IMO that should be mentioned here too.

Comment thread docs/syntax/changelog.md Outdated
:::
```

Inference only works for repositories whose name matches the product id. Repositories that publish multiple products (for example `cloud`, which publishes `cloud-hosted`, `cloud-serverless`, and `cloud-enterprise`) must name the product explicitly; if a product cannot be inferred the block emits an error. When `:cdn:` is set, the local-folder argument is ignored. All other options (`:type:`, `:link-visibility:`, `:description-visibility:`, `:dropdowns:`, `:subsections:`) and `hide-features` apply identically to CDN-sourced bundles.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO we should use public repos in the example

Suggested change
Inference only works for repositories whose name matches the product id. Repositories that publish multiple products (for example `cloud`, which publishes `cloud-hosted`, `cloud-serverless`, and `cloud-enterprise`) must name the product explicitly; if a product cannot be inferred the block emits an error. When `:cdn:` is set, the local-folder argument is ignored. All other options (`:type:`, `:link-visibility:`, `:description-visibility:`, `:dropdowns:`, `:subsections:`) and `hide-features` apply identically to CDN-sourced bundles.
Inference only works for repositories whose name matches the product ID. For example, the `cloud-serverless` product release notes are published from the `docs-content` repo and therefore must specify the product explicitly. If a product cannot be inferred the block emits an error. When `:cdn:` is set, the local-folder argument is ignored. All other options (`:type:`, `:link-visibility:`, `:description-visibility:`, `:dropdowns:`, `:subsections:`) and `hide-features` apply identically to CDN-sourced bundles.

cotti and others added 2 commits June 16, 2026 08:28
- Validate :cdn: product before assigning CdnProduct so invalid names are
  never stored on the block (and update the test accordingly).
- Make BundleLoader.MergeBundlesByTarget static and drop the throwaway
  BundleLoader the CDN path constructed just to satisfy the signature.
- Use a process-shared static HttpClient (SocketsHttpHandler with
  PooledConnectionLifetime + a fetch timeout) in CdnChangelogFetcher to stop
  leaking a socket handle per directive; keep a per-instance client for
  injected test handlers and make the fetcher IDisposable.
- Sort registry.json by VersionOrDate (not Ordinal) so the on-disk manifest
  is genuinely newest-first for mixed-width semvers; add a regression test.
- Document the Cancel.None constraint, the single-PUT retry capture, and the
  cache-priming base URL.

Co-authored-by: Cursor <cursoragent@cursor.com>
@cotti cotti temporarily deployed to integration-tests June 16, 2026 14:25 — with GitHub Actions Inactive
…o example

Address review feedback on the {changelog} :cdn: option docs:
- State that the product value is a product id defined in products.yml,
  not just any [a-zA-Z0-9_-]+ string.
- Replace the multi-product `cloud` inference example with the public
  `cloud-serverless`-published-from-`docs-content` example.

Co-authored-by: Cursor <cursoragent@cursor.com>
@cotti cotti temporarily deployed to integration-tests June 16, 2026 14:39 — with GitHub Actions Inactive

@Mpdreamz Mpdreamz left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Design question — should CDN sourcing be declared in docset.yml?

The :cdn: option is per-directive inline configuration, meaning CDN dependencies are discovered at render time rather than at startup. This is the same concern raised on #3470 about changelog bundle's implicit CDN entry sourcing.

Compare how crosslinks work: repos are declared upfront in docset.yml under cross_links. docs-builder knows the external dependencies at startup, can fail fast if the index is unreachable, and fetches all link indexes concurrently before any page renders. The {changelog} directive gets none of that.

A docset-level declaration would look similar:

# docset.yml
release_notes:
  - product: elasticsearch
  - product: kibana

With that:

  • All registries and bundles are fetched once at startup, concurrently, before any page renders — rather than serially on first directive encounter.
  • A CDN failure is a startup error with a clear config pointer, not a silent empty render mid-build.
  • The {changelog} directive just references an already-loaded in-memory set, with no HttpClient calls at parse time.
  • :cdn: could still exist as a convenience override, but docset.yml would be the primary declaration path.

The process-wide ConcurrentDictionary cache means a product is only fetched once per build, so the per-directive overhead is bounded. But the fail-fast and concurrency gaps remain. Is this the direction you want, or is there a reason to keep it directive-inline?


Code issues

NullLoggerFactory.Instance for CDN fetches (ChangelogBlock.cs LoadCdnBundles): the fetcher is constructed with NullLoggerFactory.Instance, discarding all ILogger output (info-level fetch timing, debug-level cache hits). The block has access to build context — pass the real logger factory rather than null. CDN round-trip logs are exactly what you want when debugging a silent empty render.

repository == "unavailable" sentinel check (InferCdnProductFromRepository): this couples product inference to a specific sentinel string from the git information service. If that sentinel changes, inference silently fails with no diagnostic. A null/empty check or a typed signal from the git service would be safer.

Multi-line XML doc comments on private methods (LoadCdnBundles, FilterByVersion, ResolveCdnBaseUri, etc. in ChangelogBlock.cs): project style is one short line max on private methods, or none. Several new private methods have multi-sentence <summary> blocks.


Minor

  • ChangelogVersionMatch.Matchesnull/empty → match-all: the non-nullable string requested parameter silently returns true when empty/whitespace. A short comment explaining the match-all semantic would help callers.
  • Test gap: no test for :cdn: + :version: together in CDN mode (both are tested independently but not combined).

@cotti

cotti commented Jun 18, 2026

Copy link
Copy Markdown
Contributor Author

Design question — should CDN sourcing be declared in docset.yml?

The :cdn: option is per-directive inline configuration, meaning CDN dependencies are discovered at render time rather than at startup. This is the same concern raised on #3470 about changelog bundle's implicit CDN entry sourcing.

Compare how crosslinks work: repos are declared upfront in docset.yml under cross_links. docs-builder knows the external dependencies at startup, can fail fast if the index is unreachable, and fetches all link indexes concurrently before any page renders. The {changelog} directive gets none of that.

A docset-level declaration would look similar:

# docset.yml
release_notes:
  - product: elasticsearch
  - product: kibana

With that:

* All registries and bundles are fetched once at startup, concurrently, before any page renders — rather than serially on first directive encounter.

* A CDN failure is a startup error with a clear config pointer, not a silent empty render mid-build.

* The `{changelog}` directive just references an already-loaded in-memory set, with no `HttpClient` calls at parse time.

* `:cdn:` could still exist as a convenience override, but `docset.yml` would be the primary declaration path.

The process-wide ConcurrentDictionary cache means a product is only fetched once per build, so the per-directive overhead is bounded. But the fail-fast and concurrency gaps remain. Is this the direction you want, or is there a reason to keep it directive-inline?

Makes sense. I think adding it to docset also plays well with the intent of release notes being a product-centric entity in contrast with changelogs being a repo-level entity. The reasoning behind the initial approach was in allowing slighter quicker pacing, but with the benefit of hindsight we have consistently won more with tighter specs and RN already has a lot of input variety.

I'll make the changes needed around, and notify the current products involved.

…artup

Replace the directive's dynamic, per-page CDN fetching with a declarative model
that mirrors cross_links: products are listed under `release_notes` in docset.yml,
validated against products.yml, and prefetched once at build startup.

- Add product-keyed `release_notes` schema + validation in ConfigurationFile
- Refactor CdnChangelogFetcher into a stateless async fetch engine
- Add FetchedReleaseNotes, IReleaseNotesResolver/Noop, and ReleaseNotesFetcher
  (strict fail-fast: a declared product's registry failure fails the build)
- Thread IReleaseNotesResolver through DocumentationSet/ParserContext and all
  build entry points (isolated, serve, codex, assembler) with a Noop default
- Rework `{changelog}` `:cdn:` into a selector over the prefetched set; infer the
  product from the repository via products.yml (canonical id, not repo name)
- Add ChangelogCdn base-URL helper; update docs and tests

Co-authored-by: Cursor <cursoragent@cursor.com>
@cotti cotti requested a review from Mpdreamz June 19, 2026 03:15
@cotti cotti temporarily deployed to integration-tests June 19, 2026 03:17 — with GitHub Actions Inactive
@cotti cotti self-assigned this Jun 19, 2026
@cotti

cotti commented Jun 19, 2026

Copy link
Copy Markdown
Contributor Author

Updated: declarative, opt-in CDN sourcing via docset.yml (addresses @Mpdreamz's suggestion)

Reworked CDN changelog sourcing from dynamic, per-directive fetching into a declarative, startup-time prefetch model driven by release_notes in docset.yml, mirroring the existing cross_links mechanism.

Schema

  • New product-keyed release_notes: list in docset.yml. Each id is validated against products.yml (must exist and carry the release-notes feature; underscores normalized to hyphens).

New components (Elastic.Documentation.Configuration/ReleaseNotes)

  • ChangelogCdn — single source of truth for the CDN base URL (DOCS_BUILDER_CHANGELOG_CDN, default distribution).
  • IReleaseNotesResolver (+ NoopReleaseNotesResolver), FetchedReleaseNotes (immutable prefetch result), and ReleaseNotesFetcher (async, concurrent per-product fetch, strict fail-fast on registry errors including 404).
  • CdnChangelogFetcher refactored into a stateless async fetch engine (removed the static cache / shared HttpClient).

Wiring

  • IReleaseNotesResolver threaded through DocumentationSetParserContext, prefetched at every entry point (isolated build, serve, codex, assembler), with a Noop default everywhere else.

Directive

  • {changelog} :cdn: is now a selector over the prefetched set — no network at parse time.
  • Valueless :cdn: infers the product from the repo, mapped to its canonical id via products.yml (e.g. elastic-otel-javaedot-java); multi-product repos must name the product explicitly.
  • Removed the directive-local ResolveCdnBaseUri / env constants. An undeclared product now errors with guidance to add it under release_notes:.

Docs / tests — updated syntax/changelog.md and the registry dev doc; added directive CDN tests, async fetcher tests, and release_notes validation tests.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/Elastic.Documentation.Configuration/ReleaseNotes/CdnChangelogFetcher.cs (1)

103-106: ⚠️ Potential issue | 🟠 Major

Timeout exceptions bypass your error handling by being caught and suppressed.

Per .NET behavior, HttpClient.SendAsync with exceeded timeout throws TaskCanceledException (a subclass of OperationCanceledException). Your exception filter when (ex is not OperationCanceledException) excludes it entirely, so timeouts silently return empty or skip bundles instead of invoking emitError/emitWarning.

To distinguish timeouts from explicit cancellations, check the inner exception:

Suggested fix
-		catch (Exception ex) when (ex is not OperationCanceledException)
+		catch (Exception ex) when (ex is not OperationCanceledException || ex.InnerException is TimeoutException)
 		{
 			emitError($"Could not fetch changelog registry for product '{product}' from {registryUri}: {ex.Message}");
 			return [];
 		}
@@
-			catch (Exception ex) when (ex is not OperationCanceledException)
+			catch (Exception ex) when (ex is not OperationCanceledException || ex.InnerException is TimeoutException)
 			{
 				// The registry can reference a bundle whose scrubbed copy is not yet public; skip + warn.
 				emitWarning($"Could not fetch changelog bundle '{fileName}' for '{product}' from {bundleUri}: {ex.Message}");
 			}

Also applies to: 172-175

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/Elastic.Documentation.Configuration/ReleaseNotes/CdnChangelogFetcher.cs`
around lines 103 - 106, The exception filter in the catch block at the
CdnChangelogFetcher catch statement uses `when (ex is not
OperationCanceledException)` which silently suppresses TaskCanceledException
(raised on timeout) since it's a subclass of OperationCanceledException. To fix
this, modify the exception filter to differentiate between timeouts and
cancellations by checking the inner exception. Instead of filtering out all
OperationCanceledException types, allow TaskCanceledException through so
emitError is invoked for timeout scenarios. Apply the same fix to the second
occurrence also mentioned in the comment (lines 172-175).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@docs/development/changelog-bundle-registry.md`:
- Line 182: The markdown document has a forward-reference link that points to an
anchor defined later in the file, which violates markdown linting rules. In the
changelog-bundle-registry.md file, the link reference to [Declaration and
prefetch](`#declaration-and-prefetch`) appears before its corresponding anchor
definition. Resolve this by either reorganizing the document structure to place
the "Declaration and prefetch" section before the reference, or alternatively
use an inline anchor syntax (if your markdown flavor supports it) to define the
anchor at the point of reference before the link is used.

In `@docs/syntax/changelog.md`:
- Line 168: The description text for product bundles contains a forward
reference link to the anchor declaring-cdn-backed-products, which is defined
later in the document at line 196. To fix this markdown linting issue (MD051),
either move the section that defines the declaring-cdn-backed-products anchor to
appear before the line containing the forward reference, or reorder the content
so that the anchor is defined before it is referenced in the link.

In `@src/services/Elastic.Documentation.Assembler/AssembleSources.cs`:
- Around line 79-80: The await on releaseNotesFetcher.FetchAsync is missing
ConfigureAwait(false), which violates the library code async guideline. Add
ConfigureAwait(false) to the end of the FetchAsync call chain so that the
continuation does not capture the current synchronization context, which is
required for all library code async operations according to the repo's C# coding
standards.

---

Outside diff comments:
In `@src/Elastic.Documentation.Configuration/ReleaseNotes/CdnChangelogFetcher.cs`:
- Around line 103-106: The exception filter in the catch block at the
CdnChangelogFetcher catch statement uses `when (ex is not
OperationCanceledException)` which silently suppresses TaskCanceledException
(raised on timeout) since it's a subclass of OperationCanceledException. To fix
this, modify the exception filter to differentiate between timeouts and
cancellations by checking the inner exception. Instead of filtering out all
OperationCanceledException types, allow TaskCanceledException through so
emitError is invoked for timeout scenarios. Apply the same fix to the second
occurrence also mentioned in the comment (lines 172-175).
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 81cf8cce-c0f3-44a6-91d9-dfedec54dc63

📥 Commits

Reviewing files that changed from the base of the PR and between 36c8bff and c15b9f4.

📒 Files selected for processing (23)
  • docs/development/changelog-bundle-registry.md
  • docs/syntax/changelog.md
  • src/Elastic.Codex/Building/CodexBuildService.cs
  • src/Elastic.Documentation.Configuration/Builder/ConfigurationFile.cs
  • src/Elastic.Documentation.Configuration/ReleaseNotes/CdnChangelogFetcher.cs
  • src/Elastic.Documentation.Configuration/ReleaseNotes/ChangelogCdn.cs
  • src/Elastic.Documentation.Configuration/ReleaseNotes/FetchedReleaseNotes.cs
  • src/Elastic.Documentation.Configuration/ReleaseNotes/IReleaseNotesResolver.cs
  • src/Elastic.Documentation.Configuration/ReleaseNotes/ReleaseNotesFetcher.cs
  • src/Elastic.Documentation.Configuration/Serialization/YamlStaticContext.cs
  • src/Elastic.Documentation.Configuration/Toc/DocumentationSetFile.cs
  • src/Elastic.Markdown/IO/DocumentationSet.cs
  • src/Elastic.Markdown/Myst/Directives/Changelog/ChangelogBlock.cs
  • src/Elastic.Markdown/Myst/MarkdownParser.cs
  • src/Elastic.Markdown/Myst/ParserContext.cs
  • src/services/Elastic.Documentation.Assembler/AssembleSources.cs
  • src/services/Elastic.Documentation.Assembler/Navigation/AssemblerDocumentationSet.cs
  • src/services/Elastic.Documentation.Isolated/IsolatedBuildService.cs
  • src/tooling/docs-builder/Http/ReloadableGeneratorState.cs
  • tests/Elastic.Documentation.Configuration.Tests/ConfigurationFileReleaseNotesTests.cs
  • tests/Elastic.Documentation.Configuration.Tests/ReleaseNotes/CdnChangelogFetcherTests.cs
  • tests/Elastic.Markdown.Tests/Directives/ChangelogBasicTests.cs
  • tests/Elastic.Markdown.Tests/Directives/DirectiveBaseTests.cs
✅ Files skipped from review due to trivial changes (1)
  • src/Elastic.Documentation.Configuration/ReleaseNotes/ChangelogCdn.cs
🚧 Files skipped from review as they are similar to previous changes (1)
  • tests/Elastic.Markdown.Tests/Directives/ChangelogBasicTests.cs

Comment thread docs/development/changelog-bundle-registry.md
Comment thread docs/syntax/changelog.md
Comment thread src/services/Elastic.Documentation.Assembler/AssembleSources.cs Outdated
Addresses CodeRabbit review feedback: the FetchAsync call into the
ReleaseNotesFetcher library should not capture the synchronization context,
matching the repo's async guideline and the fetcher's own internals.

Co-authored-by: Cursor <cursoragent@cursor.com>
@cotti cotti temporarily deployed to integration-tests June 19, 2026 03:37 — with GitHub Actions Inactive

@Mpdreamz Mpdreamz left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the architectural rework — the startup prefetch via docset.yml addresses all the main concerns from the previous review. A few small items still need attention.

/// target version or its file stem equals the requested value. If nothing matches a warning is
/// emitted and the block renders empty (rather than silently falling back to all versions).
/// </summary>
private IReadOnlyList<LoadedBundle> FilterByVersion(IReadOnlyList<LoadedBundle> bundles)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Project style is one short comment line max on private methods (or none). This 4-line summary on a private method should be shortened to a single line or removed.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trimmed to a single-line summary in f0a79cb.

/// repo publishes the <c>edot-java</c> product), so the repository is mapped via products.yml. Returns
/// null when git information is unavailable.
/// </summary>
private string? InferCdnProductFromRepository()

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same style issue — 3-line summary on a private method. One line or none.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trimmed to a single-line summary in f0a79cb. Also swept the other private methods this PR adds (IsSafeBundleFileName, ResolveInlineEntries, and the RegistryBuilder helpers) down to a single line or none.

private string? InferCdnProductFromRepository()
{
var repository = Build.Git.RepositoryName;
if (string.IsNullOrWhiteSpace(repository) || repository == "unavailable")

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The "unavailable" sentinel is still here. IsNullOrWhiteSpace now guards the empty case, but this still couples product inference to an internal string from the git info service. If that sentinel ever changes, inference silently falls back to the repository name with no diagnostic. Consider exposing a typed property (e.g. Build.Git.IsAvailable) rather than matching the raw string.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in f0a79cb: added GitCheckoutInformation.IsAvailable (the sentinel now lives only in its owning type) and InferCdnProductFromRepository checks Build.Git.IsAvailable instead of matching the raw "unavailable" string.

/// <param name="file">The bundle file name or path (may be null/empty).</param>
public static bool Matches(string requested, string? target, string? file)
{
if (string.IsNullOrWhiteSpace(requested))

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The match-all-on-empty semantic is non-obvious to callers. Add a short inline comment: // empty/whitespace → match all versions.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added the inline comment // empty/whitespace → match all versions above the guard in f0a79cb.

/// instead of hitting the network. Regression guard: the HTML renderer previously gated on the
/// (CDN-null) local bundles folder path and silently emitted an empty body.
/// </summary>
public class ChangelogCdnRenderTests(ITestOutputHelper output) : DirectiveTest<ChangelogBlock>(output,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still missing a test that combines :cdn: + :version: together in CDN mode. Both options are tested independently but the interaction — filtering prefetched CDN bundles by version — has no coverage.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added ChangelogCdnVersionFilterTests in f0a79cb — injects two prefetched CDN bundles (9.4.0 + 9.3.0) with :cdn: + :version: 9.4.0 and asserts only the matching bundle renders.

…ability, version-filter test

- Shorten/remove verbose XML summaries on private methods (FilterByVersion,
  InferCdnProductFromRepository, IsSafeBundleFileName, ResolveInlineEntries,
  RegistryBuilder helpers) to a single line or none.
- Add GitCheckoutInformation.IsAvailable so product inference no longer matches
  the raw "unavailable" sentinel string.
- Document the match-all-on-empty semantic in ChangelogVersionMatch.Matches.
- Add a combined :cdn: + :version: directive test covering CDN-mode filtering.

Co-authored-by: Cursor <cursoragent@cursor.com>
@cotti cotti temporarily deployed to integration-tests June 19, 2026 13:05 — with GitHub Actions Inactive
@cotti cotti requested a review from Mpdreamz June 19, 2026 13:07
@cotti cotti merged commit 86a8ec4 into main Jun 19, 2026
33 of 34 checks passed
@cotti cotti deleted the changelog_directive_s3 branch June 19, 2026 13:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants