From d6552b1eb176692334e3015e15ff2847cbe13019 Mon Sep 17 00:00:00 2001 From: lcawl Date: Wed, 16 Sep 2026 14:40:26 -0500 Subject: [PATCH 1/5] [Release notes] Product scoped amend and rebuilt registry --- docs/cli/changelog/cmd-note.md | 2 +- docs/development/changelog-bundle-registry.md | 13 +- .../Reconciliation/NoteAmendReconciler.cs | 219 ++++++++++++------ .../Reconciliation/NotesIndexReconciler.cs | 30 ++- .../Scrubbing/ScrubberProcessor.cs | 14 +- .../NoteAmendReconcilerTests.cs | 150 ++++++++++-- .../Scrubbing/ScrubberProcessorTests.cs | 81 +++++++ 7 files changed, 390 insertions(+), 119 deletions(-) diff --git a/docs/cli/changelog/cmd-note.md b/docs/cli/changelog/cmd-note.md index 2463549d89..e473acc4be 100644 --- a/docs/cli/changelog/cmd-note.md +++ b/docs/cli/changelog/cmd-note.md @@ -66,7 +66,7 @@ The scrubber writes two indexes per note: `{changelog}` does not read these indexes. It loads published bundle YAML (and amend sidecars listed in `bundle/{product}/registry.json`). -If the release bundle for that product and version or date has already shipped when you upload, the scrubber generates a `{parent}.amend-notes.yaml` sidecar so the changelog reaches published docs without a manual rerun. Don't run `changelog bundle-amend --add` for that file. Refer to [](/data/release-notes/bundle.md#changelog-bundle-notes-after-ship). +If the release bundle for that product and version or date has already shipped when you upload, the scrubber generates an amend sidecar for **that product**, then rebuilds that product's `bundle/{product}/registry.json` so `{changelog}` `:cdn:` pages pick it up. Other products that share the version are left alone. Don't run `changelog bundle-amend --add` for that file. Refer to [](/data/release-notes/bundle.md#changelog-bundle-notes-after-ship). If there is no existing or planned bundle for that product and version or date, you can create a bundle from a path list that contains all the relevant changelogs. Refer to [Bundle by file paths](/cli/changelog/bundle.md#changelog-bundle-files). diff --git a/docs/development/changelog-bundle-registry.md b/docs/development/changelog-bundle-registry.md index 3affe8e2a7..c99e71847e 100644 --- a/docs/development/changelog-bundle-registry.md +++ b/docs/development/changelog-bundle-registry.md @@ -71,11 +71,14 @@ narrowed reconciliation to the bundle tree): - **Amend-notes sidecars** — `bundle/{product}/{parent}.amend-notes.yaml`, also **public bucket only**, authored by the scrubber Lambda's `NoteAmendReconciler`. When a note is uploaded after its release bundle has already shipped, the reconciler generates one aggregate sidecar per - published bundle that lists all such late notes. The Lambda rebuilds it from current state on - every reconcile, so redelivered events never produce duplicate amends. `{changelog}` `:cdn:` and - `changelog render` merge this sidecar into the parent the same way as numbered `.amend-{N}` - files, after those numbered amends. The `.amend-notes` suffix - is **reserved** — do not create files with that suffix manually; see + published bundle that lists all such late notes **for that product**. The Lambda rebuilds it + from current state on every reconcile, so redelivered events never produce duplicate amends. + After a write, skip-unchanged, or delete of that sidecar, the same pass rebuilds + `bundle/{product}/registry.json` and the bundle shallow map so `{changelog}` `:cdn:` can + discover it. Other products at the same version are not walked and their sidecars are not + deleted. `{changelog}` `:cdn:` and `changelog render` merge this sidecar into the parent the + same way as numbered `.amend-{N}` files, after those numbered amends. The `.amend-notes` + suffix is **reserved** — do not create files with that suffix manually; see [](/cli/changelog/bundle-amend.md). Public copies track private-bucket create and delete events. Authors cannot issue those deletes through docs-builder today: `changelog upload` does not delete objects, and `changelog remove` is local-only. For the author-facing diff --git a/src/services/Elastic.Changelog/Reconciliation/NoteAmendReconciler.cs b/src/services/Elastic.Changelog/Reconciliation/NoteAmendReconciler.cs index 313387d953..914885b1e2 100644 --- a/src/services/Elastic.Changelog/Reconciliation/NoteAmendReconciler.cs +++ b/src/services/Elastic.Changelog/Reconciliation/NoteAmendReconciler.cs @@ -14,11 +14,12 @@ namespace Elastic.Changelog.Reconciliation; /// -/// Compares each note in the per-version notes indexes against published bundles for the same -/// version, and either creates or deletes a reconciler-owned amend sidecar +/// Compares each note in the product-scoped notes indexes against that product's published +/// bundles for the same version, and either creates or deletes a reconciler-owned amend sidecar /// ({parent}.amend-notes.yaml) that carries notes that arrived after the release shipped. -/// Also updates each bundle_seq in the notes index: 0 = no bundle yet, 1 = shipped in the -/// original bundle or a human amend, 2 = carried by the reconciler amend sidecar. +/// Also updates each bundle_seq on the product-scoped notes index: 0 = no bundle yet, +/// 1 = shipped in the original bundle or a human amend, 2 = carried by the reconciler amend sidecar. +/// The dual-written legacy notes-{version}.json path list is refreshed in the same pass. /// /// /// @@ -49,87 +50,136 @@ public sealed class NoteAmendReconciler( private readonly TimeSpan _retryBaseDelay = retryBaseDelay ?? TimeSpan.FromMilliseconds(200); /// - /// For the given repository scope, scans every product's bundle registry to determine which - /// notes have shipped and which are late, writes or deletes the reconciler-owned amend sidecars, - /// and re-writes the notes indexes with correct bundle_seq values. + /// For the given repository scope, scans each product that appears in + /// (not every bundle/{product}/ prefix), writes or + /// deletes that product's reconciler-owned amend sidecars, and re-writes product-scoped and + /// legacy notes indexes with correct bundle_seq values. /// - /// The notes scope for this repo. - /// Output of . - /// Cancellation token. - public async Task ReconcileAsync( + /// + /// Product ids that had an amend-notes write, skip-unchanged, or delete. Callers rebuild + /// those products' registry.json and the bundle shallow map. Products that were not + /// in the notes map are not returned and are not swept. + /// + public async Task> ReconcileAsync( ChangelogScope notesScope, - IReadOnlyDictionary> notesByVersion, + IReadOnlyDictionary>> notesByProduct, Cancel ctx ) { - if (notesByVersion.Count == 0) - return; + if (notesByProduct.Count == 0) + return []; var groupParts = notesScope.Group.Split('/'); var (org, repo) = (groupParts[0], groupParts[1]); - // Track bundle_seq for each (version → path → seq). Default 0 = unreleased. var seqMap = new Dictionary>(StringComparer.Ordinal); - foreach (var (version, notes) in notesByVersion) - seqMap[version] = notes.ToDictionary(n => n.Path, _ => 0, StringComparer.Ordinal); + foreach (var (product, byVersion) in notesByProduct) + { + foreach (var (version, notes) in byVersion) + seqMap[ProductVersionKey(product, version)] = notes.ToDictionary(n => n.Path, _ => 0, StringComparer.Ordinal); + } - // List all product names from the bundle tree. - var products = await ListBundleProductsAsync(ctx); - _logger.LogDebug("NoteAmendReconciler: scanning {Count} bundle product(s) for repo {Org}/{Repo}", products.Count, org, repo); + _logger.LogDebug( + "NoteAmendReconciler: scanning {Count} product(s) from the notes index for repo {Org}/{Repo}", + notesByProduct.Count, + org, + repo + ); - foreach (var product in products) + var touched = new HashSet(StringComparer.Ordinal); + foreach (var (product, byVersion) in notesByProduct) { ctx.ThrowIfCancellationRequested(); - await ProcessProductAsync(org, repo, product, notesByVersion, seqMap, ctx); + if (await ProcessProductAsync(org, repo, product, byVersion, seqMap, ctx)) + _ = touched.Add(product); } - // Re-write notes indexes with the updated bundle_seq values. - await Parallel.ForEachAsync(notesByVersion, new ParallelOptions + await RewriteNotesIndexesAsync(org, repo, notesByProduct, seqMap, ctx); + return [.. touched]; + } + + private async Task RewriteNotesIndexesAsync( + string org, + string repo, + IReadOnlyDictionary>> notesByProduct, + IReadOnlyDictionary> seqMap, + Cancel ctx + ) + { + var productWrites = notesByProduct.SelectMany(p => p.Value.Select(v => (Product: p.Key, Version: v.Key, Notes: v.Value))).ToList(); + + await Parallel.ForEachAsync(productWrites, new ParallelOptions { MaxDegreeOfParallelism = MaxParallelWrites, CancellationToken = ctx - }, async (kvp, ct) => + }, async (write, ct) => { - var (version, notes) = kvp; - var seqs = seqMap[version]; - var updatedEntries = notes - .Select(n => n with { BundleSeq = seqs.TryGetValue(n.Path, out var s) ? s : 0 }) - .OrderBy(n => n.Path, StringComparer.Ordinal) - .ToList(); - var indexKey = ChangelogKeys.NotesIndexKey(org, repo, version); - await notesIndexReconciler.WriteIndexAsync(indexKey, updatedEntries, ct); + var seqs = seqMap[ProductVersionKey(write.Product, write.Version)]; + var updatedEntries = WithSeqs(write.Notes, seqs); + var indexKey = ChangelogKeys.NotesIndexKey(org, repo, write.Product, write.Version); + await notesIndexReconciler.WriteIndexAsync(indexKey, updatedEntries, ct, new NotesIndexMetadata(write.Product, write.Version)); }); + + foreach (var (version, unionNotes) in UnionByVersion(notesByProduct)) + { + ctx.ThrowIfCancellationRequested(); + var seqs = MaxSeqsForVersion(notesByProduct.Keys, version, seqMap); + var updatedEntries = WithSeqs(unionNotes, seqs); + var indexKey = ChangelogKeys.NotesIndexKey(org, repo, version); + await notesIndexReconciler.WriteIndexAsync(indexKey, updatedEntries, ctx); + } } - // ----------------------------------------------------------------------------------------- - // Product scanning - // ----------------------------------------------------------------------------------------- + private static List WithSeqs(IReadOnlyList notes, IReadOnlyDictionary seqs) => + notes + .Select(n => n with { BundleSeq = seqs.TryGetValue(n.Path, out var s) ? s : 0 }) + .OrderBy(n => n.Path, StringComparer.Ordinal) + .ToList(); - private async Task> ListBundleProductsAsync(Cancel ctx) + private static Dictionary> UnionByVersion( + IReadOnlyDictionary>> notesByProduct + ) { - var products = new List(); - var request = new ListObjectsV2Request { BucketName = publicBucketName, Prefix = ChangelogKeys.BundlePrefix, Delimiter = "/" }; - - ListObjectsV2Response response; - do + var byVersion = new Dictionary>(StringComparer.Ordinal); + foreach (var byProductVersion in notesByProduct.Values) { - response = await s3Client.ListObjectsV2Async(request, ctx); - foreach (var prefix in response.CommonPrefixes ?? []) + foreach (var (version, notes) in byProductVersion) { - // CommonPrefix is like "bundle/elasticsearch/" — strip the outer segments. - var inner = prefix[ChangelogKeys.BundlePrefix.Length..]; - var product = inner.TrimEnd('/'); - if (!string.IsNullOrEmpty(product)) - products.Add(product); + if (!byVersion.TryGetValue(version, out var union)) + byVersion[version] = union = []; + foreach (var note in notes) + { + if (!union.Any(e => e.Path == note.Path)) + union.Add(note); + } } - request.ContinuationToken = response.NextContinuationToken; } - while (response.IsTruncated == true); + return byVersion; + } - return products; + private static Dictionary MaxSeqsForVersion( + IEnumerable products, + string version, + IReadOnlyDictionary> seqMap + ) + { + var max = new Dictionary(StringComparer.Ordinal); + foreach (var product in products) + { + if (!seqMap.TryGetValue(ProductVersionKey(product, version), out var seqs)) + continue; + foreach (var (path, seq) in seqs) + { + if (!max.TryGetValue(path, out var current) || seq > current) + max[path] = seq; + } + } + return max; } - private async Task ProcessProductAsync( + private static string ProductVersionKey(string product, string version) => $"{product}/{version}"; + + private async Task ProcessProductAsync( string org, string repo, string product, @@ -152,23 +202,22 @@ Cancel ctx } catch (AmazonS3Exception ex) when (ex.StatusCode == HttpStatusCode.NotFound) { - return; + return false; } catch (Exception ex) when (ex is not OperationCanceledException) { _logger.LogWarning(ex, "Could not read bundle registry for product {Product}; skipping", product); - return; + return false; } if (registry is null || registry.Bundles.Count == 0) - return; + return false; - // For each version that has notes, look for a matching parent bundle. + var touched = false; foreach (var (version, notes) in notesByVersion) { ctx.ThrowIfCancellationRequested(); - // Parent bundle: not an amend file, target matches the version. var parentBundle = registry.Bundles.FirstOrDefault( b => !string.IsNullOrEmpty(b.File) && !BundleAmendMerger.IsAmendFile(b.File) && ChangelogVersionMatch.Matches( version, @@ -178,13 +227,27 @@ Cancel ctx ); if (parentBundle is null) - continue; // No bundle yet → every note stays at bundle_seq 0. - - await ProcessVersionBundleAsync(org, repo, product, parentBundle, registry, version, notes, seqMap[version], ctx); + continue; + + if ( + await ProcessVersionBundleAsync( + org, + repo, + product, + parentBundle, + registry, + version, + notes, + seqMap[ProductVersionKey(product, version)], + ctx + ) + ) + touched = true; } + return touched; } - private async Task ProcessVersionBundleAsync( + private async Task ProcessVersionBundleAsync( string org, string repo, string product, @@ -201,7 +264,7 @@ Cancel ctx var parent = await TryReadBundleAsync(parentKey, ctx); if (parent is null) - return; + return false; // A bundle with no file-annotated entries (hand-authored / legacy) has no reliable // shipped set — skip to avoid false positives. @@ -213,7 +276,7 @@ Cancel ctx parentKey, version ); - return; + return false; } // Read existing numeric amend bundles (in order) to compute the full merged set. @@ -271,17 +334,16 @@ Cancel ctx { var amendBundle = AmendDocumentBuilder.Build(parent.Products, lateEntries, []); var newJson = ReleaseNotesSerialization.SerializeBundle(amendBundle); - await WriteAmendNotesAsync(amendNotesKey, newJson, ctx); + _ = await WriteAmendNotesAsync(amendNotesKey, newJson, ctx); foreach (var note in lateNotes) - seqByPath[note.Path] = 2; // carried by the reconciler amend + seqByPath[note.Path] = 2; + return true; } + return false; } - else - { - // All notes are shipped — delete the sidecar if it exists. - await DeleteAmendNotesIfExistsAsync(amendNotesKey, ctx); - } + + return await DeleteAmendNotesIfExistsAsync(amendNotesKey, ctx); } // ----------------------------------------------------------------------------------------- @@ -331,7 +393,7 @@ Cancel ctx // Conditional S3 write / delete // ----------------------------------------------------------------------------------------- - private async Task WriteAmendNotesAsync(string key, string newJson, Cancel ctx) + private async Task WriteAmendNotesAsync(string key, string newJson, Cancel ctx) { const int maxAttempts = 5; for (var attempt = 1; attempt <= maxAttempts; attempt++) @@ -366,7 +428,7 @@ private async Task WriteAmendNotesAsync(string key, string newJson, Cancel ctx) if (existingJson == newJson) { _logger.LogDebug("Amend-notes {Key} is unchanged; skipping write", key); - return; + return true; } } catch (AmazonS3Exception ex) when (ex.StatusCode == HttpStatusCode.NotFound) @@ -390,7 +452,7 @@ private async Task WriteAmendNotesAsync(string key, string newJson, Cancel ctx) _ = await s3Client.PutObjectAsync(putRequest, ctx); _metrics.IncrementRegistryWrites(); _logger.LogInformation("Wrote amend-notes sidecar {Key}", key); - return; + return true; } catch (AmazonS3Exception ex) when (ex.StatusCode is HttpStatusCode.PreconditionFailed || (int)ex.StatusCode == 409) { @@ -411,9 +473,10 @@ private async Task WriteAmendNotesAsync(string key, string newJson, Cancel ctx) _logger.LogDebug(ex, "Amend-notes write {Key} failed (attempt {A}/{Max}); retrying", key, attempt, maxAttempts); } } + return false; } - private async Task DeleteAmendNotesIfExistsAsync(string key, Cancel ctx) + private async Task DeleteAmendNotesIfExistsAsync(string key, Cancel ctx) { try { @@ -425,19 +488,21 @@ private async Task DeleteAmendNotesIfExistsAsync(string key, Cancel ctx) _ = await s3Client.DeleteObjectAsync(new DeleteObjectRequest { BucketName = publicBucketName, Key = key, IfMatch = etag }, ctx); _logger.LogInformation("Deleted stale amend-notes sidecar {Key}", key); + return true; } catch (AmazonS3Exception ex) when (ex.StatusCode == HttpStatusCode.NotFound) { - // Nothing to delete — this is the expected steady state when all notes are shipped. + return false; } catch (AmazonS3Exception ex) when (ex.StatusCode == HttpStatusCode.PreconditionFailed) { - // Another reconciler deleted or replaced it concurrently — safe to ignore. _logger.LogDebug("Amend-notes {Key} was updated concurrently; delete skipped", key); + return true; } catch (Exception ex) when (ex is not OperationCanceledException) { _logger.LogWarning(ex, "Could not delete stale amend-notes sidecar {Key}; will retry on next reconcile", key); + return false; } } diff --git a/src/services/Elastic.Changelog/Reconciliation/NotesIndexReconciler.cs b/src/services/Elastic.Changelog/Reconciliation/NotesIndexReconciler.cs index 1e8f2d1f52..2a973e870c 100644 --- a/src/services/Elastic.Changelog/Reconciliation/NotesIndexReconciler.cs +++ b/src/services/Elastic.Changelog/Reconciliation/NotesIndexReconciler.cs @@ -48,10 +48,14 @@ public sealed class NotesIndexReconciler( /// read to derive the grouping; every affected index is then (re)written. /// /// - /// A map of version → list of NoteIndexEntry (the version-union, same shape NoteAmend - /// consumes today). Returns an empty dictionary when no notes exist. + /// Notes grouped by product, then version. Legacy version-union indexes are still written + /// in this pass but are not part of the return value; uses + /// the product map only. /// - public async Task>> ReconcileRepoAsync(ChangelogScope notesScope, Cancel ctx) + public async Task>>> ReconcileRepoAsync( + ChangelogScope notesScope, + Cancel ctx + ) { if (notesScope.Kind != ChangelogScopeKind.Notes) throw new ArgumentException($"Notes reconcile requires a Notes scope; got '{notesScope}'.", nameof(notesScope)); @@ -86,11 +90,11 @@ public async Task>> Re { _logger.LogDebug("No versions found for repo {Repo}; removing any stale indexes", notesScope.Group); await DeleteStaleIndexes(existingIndexKeys, intendedKeys, ctx); - return new Dictionary>(); + return new Dictionary>>(StringComparer.Ordinal); } var writes = BuildIndexWrites(org, repo, byProductVersion, byVersion); - var written = new Dictionary>(StringComparer.Ordinal); + var writtenProducts = new Dictionary>>(StringComparer.Ordinal); try { await Parallel.ForEachAsync(writes, new ParallelOptions @@ -105,10 +109,14 @@ await WriteIndexAsync( ct, write.Product is null ? null : new NotesIndexMetadata(write.Product, write.Version!) ); - if (write.Product is not null) + if (write.Product is null) return; - lock (written) - written[write.Version!] = write.Entries; + lock (writtenProducts) + { + if (!writtenProducts.TryGetValue(write.Product, out var byVer)) + writtenProducts[write.Product] = byVer = [with(StringComparer.Ordinal)]; + byVer[write.Version!] = write.Entries; + } }); } finally @@ -116,7 +124,11 @@ await WriteIndexAsync( await DeleteStaleIndexes(existingIndexKeys, intendedKeys, ctx); } - return written; + return writtenProducts.ToDictionary( + kv => kv.Key, + kv => (IReadOnlyDictionary>)kv.Value, + StringComparer.Ordinal + ); } private static void AddIndexEntry(Dictionary> map, string groupKey, string poolRelativePath) diff --git a/src/services/Elastic.Changelog/Scrubbing/ScrubberProcessor.cs b/src/services/Elastic.Changelog/Scrubbing/ScrubberProcessor.cs index fb6f5bd138..51f25a1360 100644 --- a/src/services/Elastic.Changelog/Scrubbing/ScrubberProcessor.cs +++ b/src/services/Elastic.Changelog/Scrubbing/ScrubberProcessor.cs @@ -140,8 +140,16 @@ public async Task> ProcessAsync(IReadOnlyList shallowWork return; } - // Notes indexes (notes-{target}.json) are reconciler-owned; a client that uploads one is + // Notes indexes are reconciler-owned; a client that uploads one is // rejected here — the reconciler writes directly to the public bucket, so no copy is needed. if (ChangelogKeys.IsNotesIndex(key)) { diff --git a/tests/Elastic.Changelog.Tests/Reconciliation/NoteAmendReconcilerTests.cs b/tests/Elastic.Changelog.Tests/Reconciliation/NoteAmendReconcilerTests.cs index d2c6c29a91..45b7012278 100644 --- a/tests/Elastic.Changelog.Tests/Reconciliation/NoteAmendReconcilerTests.cs +++ b/tests/Elastic.Changelog.Tests/Reconciliation/NoteAmendReconcilerTests.cs @@ -104,12 +104,21 @@ private static string RegistryJsonWithTarget(string target, params string[] file private static NotesIndex ReadNotesIndex(string json) => JsonSerializer.Deserialize(json, NotesIndexJsonContext.Default.NotesIndex)!; - private static IReadOnlyDictionary> NotesByVersion(string version, params string[] paths) => - new Dictionary> + private static IReadOnlyDictionary>> NotesByProduct( + string product, + string version, + params string[] paths + ) => + new Dictionary>>(StringComparer.Ordinal) { - [version] = [.. paths.Select(p => new NoteIndexEntry { Path = p, BundleSeq = 0 })] + [product] = new Dictionary>(StringComparer.Ordinal) + { + [version] = [.. paths.Select(p => new NoteIndexEntry { Path = p, BundleSeq = 0 })] + } }; + private static string ProductIndexKey(string? product = null) => ChangelogKeys.NotesIndexKey(Org, Repo, product ?? Product, Version); + // ----------------------------------------------------------------------------------------- [Fact] @@ -120,8 +129,8 @@ public async Task LateNote_NoBundleAmendYet_WritesAmendNotesSidecar() _s3.Seed(PublicBucket, BundleKey(parent), ParentBundleYaml("main/pr-100.yaml")); _s3.Seed(PublicBucket, NoteKey("main", "note-cve.yml"), NoteYaml); - var notesByVersion = NotesByVersion(Version, "main/note-cve.yml"); - await _reconciler.ReconcileAsync(NotesScope(), notesByVersion, TestContext.Current.CancellationToken); + var notesByProduct = NotesByProduct(Product, Version, "main/note-cve.yml"); + await _reconciler.ReconcileAsync(NotesScope(), notesByProduct, TestContext.Current.CancellationToken); // Amend sidecar must have been written. _s3.Exists(PublicBucket, AmendNotesKey(parent)).Should().BeTrue("late note must produce an amend sidecar"); @@ -131,6 +140,11 @@ public async Task LateNote_NoBundleAmendYet_WritesAmendNotesSidecar() _s3.Exists(PublicBucket, indexKey).Should().BeTrue("notes index must be re-written with bundle_seq values"); var index = ReadNotesIndex(_s3.ContentOf(PublicBucket, indexKey)); index.Notes.Should().ContainSingle().Which.BundleSeq.Should().Be(2); + + var productIndex = ReadNotesIndex(_s3.ContentOf(PublicBucket, ProductIndexKey())); + productIndex.Notes.Should().ContainSingle().Which.BundleSeq.Should().Be(2); + productIndex.Product.Should().Be(Product); + productIndex.Version.Should().Be(Version); } [Fact] @@ -142,8 +156,8 @@ public async Task NoteShippedInParent_NoAmendWritten_SeqIsOne() _s3.Seed(PublicBucket, BundleKey(parent), ParentBundleYaml("main/note-cve.yml", "main/pr-100.yaml")); _s3.Seed(PublicBucket, NoteKey("main", "note-cve.yml"), NoteYaml); - var notesByVersion = NotesByVersion(Version, "main/note-cve.yml"); - await _reconciler.ReconcileAsync(NotesScope(), notesByVersion, TestContext.Current.CancellationToken); + var notesByProduct = NotesByProduct(Product, Version, "main/note-cve.yml"); + await _reconciler.ReconcileAsync(NotesScope(), notesByProduct, TestContext.Current.CancellationToken); // No amend sidecar should be written. _s3.Exists(PublicBucket, AmendNotesKey(parent)).Should().BeFalse("note already in parent → no amend needed"); @@ -180,8 +194,8 @@ public async Task NoteShippedInHumanAmend_NoAmendNotesWritten_SeqIsOne() _s3.Seed(PublicBucket, BundleKey(humanAmend), ReleaseNotesSerialization.SerializeBundle(amendBundle)); _s3.Seed(PublicBucket, NoteKey("main", "note-cve.yml"), NoteYaml); - var notesByVersion = NotesByVersion(Version, "main/note-cve.yml"); - await _reconciler.ReconcileAsync(NotesScope(), notesByVersion, TestContext.Current.CancellationToken); + var notesByProduct = NotesByProduct(Product, Version, "main/note-cve.yml"); + await _reconciler.ReconcileAsync(NotesScope(), notesByProduct, TestContext.Current.CancellationToken); _s3 .Exists(PublicBucket, AmendNotesKey(parent)) @@ -208,8 +222,8 @@ public async Task ParentBundleHasNoFileAnnotations_Skipped_SeqRemainsZero() _s3.Seed(PublicBucket, BundleKey(parent), ReleaseNotesSerialization.SerializeBundle(handAuthored)); _s3.Seed(PublicBucket, NoteKey("main", "note-cve.yml"), NoteYaml); - var notesByVersion = NotesByVersion(Version, "main/note-cve.yml"); - await _reconciler.ReconcileAsync(NotesScope(), notesByVersion, TestContext.Current.CancellationToken); + var notesByProduct = NotesByProduct(Product, Version, "main/note-cve.yml"); + await _reconciler.ReconcileAsync(NotesScope(), notesByProduct, TestContext.Current.CancellationToken); // No amend written; shipped state is unknown. _s3.Exists(PublicBucket, AmendNotesKey(parent)).Should().BeFalse("unknown shipped state → skip, no amend"); @@ -244,8 +258,8 @@ public async Task NoteRemovedFromIndex_ExistingAmendSidecarDeleted() _s3.Seed(PublicBucket, AmendNotesKey(parent), ReleaseNotesSerialization.SerializeBundle(staleAmend)); // No notes for this version (note was deleted from the pool). - var notesByVersion = NotesByVersion(Version); - await _reconciler.ReconcileAsync(NotesScope(), notesByVersion, TestContext.Current.CancellationToken); + var notesByProduct = NotesByProduct(Product, Version); + await _reconciler.ReconcileAsync(NotesScope(), notesByProduct, TestContext.Current.CancellationToken); _s3.Exists(PublicBucket, AmendNotesKey(parent)).Should().BeFalse("stale amend sidecar must be deleted when no notes remain"); _s3.Deletes.Should().ContainSingle().Which.Key.Should().Be(AmendNotesKey(parent)); @@ -259,15 +273,15 @@ public async Task Idempotent_SameStateRedelivered_NoSecondPut() _s3.Seed(PublicBucket, BundleKey(parent), ParentBundleYaml("main/pr-100.yaml")); _s3.Seed(PublicBucket, NoteKey("main", "note-cve.yml"), NoteYaml); - var notesByVersion = NotesByVersion(Version, "main/note-cve.yml"); + var notesByProduct = NotesByProduct(Product, Version, "main/note-cve.yml"); // First reconcile → amend sidecar written. - await _reconciler.ReconcileAsync(NotesScope(), notesByVersion, TestContext.Current.CancellationToken); + await _reconciler.ReconcileAsync(NotesScope(), notesByProduct, TestContext.Current.CancellationToken); var putsAfterFirst = _s3.Puts.Count; putsAfterFirst.Should().BeGreaterThan(0, "first pass must write the amend sidecar and the notes index"); // Second reconcile with the same state → content is identical → no additional PUTs. - await _reconciler.ReconcileAsync(NotesScope(), notesByVersion, TestContext.Current.CancellationToken); + await _reconciler.ReconcileAsync(NotesScope(), notesByProduct, TestContext.Current.CancellationToken); var putsAfterSecond = _s3.Puts.Count; // The notes index re-write is idempotent too (same content, conditional PUT is a no-op). @@ -284,8 +298,8 @@ public async Task NoBundleForVersion_NoAmend_SeqRemainsZero() _s3.Seed(PublicBucket, BundleKey("elasticsearch-8.0.0.yaml"), ParentBundleYaml("main/pr-100.yaml")); _s3.Seed(PublicBucket, NoteKey("main", "note-cve.yml"), NoteYaml); - var notesByVersion = NotesByVersion(Version, "main/note-cve.yml"); - await _reconciler.ReconcileAsync(NotesScope(), notesByVersion, TestContext.Current.CancellationToken); + var notesByProduct = NotesByProduct(Product, Version, "main/note-cve.yml"); + await _reconciler.ReconcileAsync(NotesScope(), notesByProduct, TestContext.Current.CancellationToken); // No amend sidecar: no matching bundle. _s3.Puts.Should().NotContain(p => p.Key.Contains("amend-notes"), "no matching bundle → no amend possible"); @@ -297,13 +311,101 @@ public async Task NoBundleForVersion_NoAmend_SeqRemainsZero() } [Fact] - public async Task NoProductsInBundleTree_NoAmend() + public async Task NoPublishedBundle_NoAmend() + { + // Product is in the notes map but has no bundle registry — do not walk other products. + var notesByProduct = NotesByProduct(Product, Version, "main/note-cve.yml"); + await _reconciler.ReconcileAsync(NotesScope(), notesByProduct, TestContext.Current.CancellationToken); + + _s3.Puts.Should().NotContain(p => p.Key.Contains("amend-notes", StringComparison.Ordinal)); + } + + [Fact] + public async Task EceNote_DoesNotWriteOrDeleteHostedAmendNotes() { - // Bundle tree is empty (no products listed under bundle/). - // No registry.json objects exist, so ListObjectsV2 returns no common prefixes. - var notesByVersion = NotesByVersion(Version, "main/note-cve.yml"); - await _reconciler.ReconcileAsync(NotesScope(), notesByVersion, TestContext.Current.CancellationToken); + const string ece = "cloud-enterprise"; + const string hosted = "cloud-hosted"; + const string version = "4.2.0"; + const string eceParent = "cloud-4.2.0.yaml"; + const string hostedParent = "cloud-4.2.0.yaml"; + const string hostedSidecar = "cloud-4.2.0.amend-notes.yaml"; + + var eceNoteYaml = "title: ECE note\n" + + "type: known-issue\n" + + "products:\n" + + " - product: cloud-enterprise\n" + + " versions: [4.2.0]\n"; + + _s3.Seed( + PublicBucket, + $"bundle/{ece}/registry.json", + JsonSerializer.Serialize( + new ChangelogRegistry { Product = ece, Bundles = [new ChangelogRegistryBundle { File = eceParent, Target = version }] }, + ChangelogRegistryJsonContext.Default.ChangelogRegistry + ) + ); + _s3.Seed( + PublicBucket, + $"bundle/{hosted}/registry.json", + JsonSerializer.Serialize( + new ChangelogRegistry + { + Product = hosted, + Bundles = + [ + new ChangelogRegistryBundle { File = hostedParent, Target = version }, + new ChangelogRegistryBundle { File = hostedSidecar, Target = version } + ] + }, + ChangelogRegistryJsonContext.Default.ChangelogRegistry + ) + ); + _s3.Seed(PublicBucket, $"bundle/{ece}/{eceParent}", ParentBundleFor(ece, version, "main/pr-100.yaml")); + _s3.Seed(PublicBucket, $"bundle/{hosted}/{hostedParent}", ParentBundleFor(hosted, version, "main/pr-hosted.yaml")); + + var hostedAmend = new Bundle + { + Products = [new BundledProduct(hosted, target: version, lifecycle: Lifecycle.Ga)], + Entries = + [ + new BundledEntry + { + File = new BundledFile { Name = "main/note-hosted.yml", Checksum = "old" }, + Title = "Hosted note", + Type = ChangelogEntryType.KnownIssue + } + ] + }; + var hostedSidecarYaml = ReleaseNotesSerialization.SerializeBundle(hostedAmend); + _s3.Seed(PublicBucket, $"bundle/{hosted}/{hostedSidecar}", hostedSidecarYaml); + _s3.Seed(PublicBucket, NoteKey("main", "note-ece.yml"), eceNoteYaml); + + var notesByProduct = NotesByProduct(ece, version, "main/note-ece.yml"); + var touched = await _reconciler.ReconcileAsync(NotesScope(), notesByProduct, TestContext.Current.CancellationToken); - _s3.Puts.Should().NotContain(p => p.Key.Contains("amend-notes")); + touched.Should().Equal(ece); + _s3.Exists(PublicBucket, $"bundle/{ece}/cloud-4.2.0.amend-notes.yaml").Should().BeTrue(); + _s3.ContentOf(PublicBucket, $"bundle/{hosted}/{hostedSidecar}").Should().Be(hostedSidecarYaml); + _s3.Deletes.Should().NotContain(d => d.Key.Contains($"bundle/{hosted}/", StringComparison.Ordinal)); + } + + private static string ParentBundleFor(string product, string version, params string[] entryFileNames) + { + var bundle = new Bundle + { + Products = [new BundledProduct(product, target: version, lifecycle: Lifecycle.Ga)], + Entries = + [ + .. entryFileNames.Select( + n => new BundledEntry + { + File = new BundledFile { Name = n, Checksum = "abc123" }, + Title = $"Entry for {n}", + Type = ChangelogEntryType.BugFix + } + ) + ] + }; + return ReleaseNotesSerialization.SerializeBundle(bundle); } } diff --git a/tests/Elastic.Changelog.Tests/Scrubbing/ScrubberProcessorTests.cs b/tests/Elastic.Changelog.Tests/Scrubbing/ScrubberProcessorTests.cs index 7dcc42fa01..915c3d9319 100644 --- a/tests/Elastic.Changelog.Tests/Scrubbing/ScrubberProcessorTests.cs +++ b/tests/Elastic.Changelog.Tests/Scrubbing/ScrubberProcessorTests.cs @@ -517,6 +517,87 @@ public async Task Process_NoteFile_ScrubbedAndNotesReconcileTriggered() _s3.Exists(PublicBucket, "changelog/elastic/elasticsearch/notes-elasticsearch-9.0.0.json").Should().BeTrue(); } + [Fact] + public async Task Process_NoteFile_ListsAmendNotesOnThatProductOnly() + { + _ = A.CallTo(() => _scrubber.ScrubAsync(A._, A._, A._)).ReturnsLazily( + (string _, string content, Cancel _) => Task.FromResult(new ScrubResult { Content = content }) + ); + + const string ece = "cloud-enterprise"; + const string hosted = "cloud-hosted"; + const string version = "4.2.0"; + const string parent = "cloud-4.2.0.yaml"; + const string hostedSidecar = "cloud-4.2.0.amend-notes.yaml"; + + _s3.Seed(PublicBucket, $"bundle/{ece}/{parent}", ProductParentBundle(ece, version, "main/pr-100.yaml")); + _s3.Seed(PublicBucket, $"bundle/{hosted}/{parent}", ProductParentBundle(hosted, version, "main/pr-hosted.yaml")); + _s3.Seed(PublicBucket, $"bundle/{ece}/registry.json", ProductRegistryJson(ece, version, parent)); + _s3.Seed(PublicBucket, $"bundle/{hosted}/registry.json", ProductRegistryJson(hosted, version, parent, hostedSidecar)); + var hostedSidecarYaml = ProductParentBundle(hosted, version, "main/note-hosted.yml"); + _s3.Seed(PublicBucket, $"bundle/{hosted}/{hostedSidecar}", hostedSidecarYaml); + + const string noteYaml = + """ + title: ECE known issue + type: known-issue + products: + - product: cloud-enterprise + versions: [4.2.0] + """; + _s3.Seed(PrivateBucket, "changelog/elastic/cloud/main/note-ece.yml", noteYaml); + + var failed = await _processor.ProcessAsync([Message("ObjectCreated:Put", "changelog/elastic/cloud/main/note-ece.yml")], Ctx); + + failed.Should().BeEmpty(); + _s3.Exists(PublicBucket, $"bundle/{ece}/cloud-4.2.0.amend-notes.yaml").Should().BeTrue(); + _s3.ContentOf(PublicBucket, $"bundle/{hosted}/{hostedSidecar}").Should().Be(hostedSidecarYaml); + + var eceRegistry = + JsonSerializer.Deserialize( + _s3.ContentOf(PublicBucket, $"bundle/{ece}/registry.json"), + ChangelogRegistryJsonContext.Default.ChangelogRegistry + )!; + eceRegistry.Bundles.Select(b => b.File).Should().BeEquivalentTo([parent, "cloud-4.2.0.amend-notes.yaml"]); + + var hostedRegistry = + JsonSerializer.Deserialize( + _s3.ContentOf(PublicBucket, $"bundle/{hosted}/registry.json"), + ChangelogRegistryJsonContext.Default.ChangelogRegistry + )!; + hostedRegistry.Bundles.Select(b => b.File).Should().BeEquivalentTo([parent, hostedSidecar]); + _metrics.GroupReconciles.Should().Be(1); + } + + private static string ProductRegistryJson(string product, string version, params string[] files) + { + var bundles = files.Select(f => new ChangelogRegistryBundle { File = f, Target = version }).ToList(); + return JsonSerializer.Serialize( + new ChangelogRegistry { Product = product, Bundles = bundles }, + ChangelogRegistryJsonContext.Default.ChangelogRegistry + ); + } + + private static string ProductParentBundle(string product, string version, params string[] entryFileNames) + { + var bundle = new Bundle + { + Products = [new BundledProduct(product, target: version, lifecycle: Lifecycle.Ga)], + Entries = + [ + .. entryFileNames.Select( + n => new BundledEntry + { + File = new BundledFile { Name = n, Checksum = "abc123" }, + Title = $"Entry for {n}", + Type = ChangelogEntryType.BugFix + } + ) + ] + }; + return ReleaseNotesSerialization.SerializeBundle(bundle); + } + [Fact] public async Task Process_PassThroughMarker_DoesNotOverwriteExistingCanonicalContent() { From 79035a8b2709ff8bfa85e7bd157ab458c0a722d1 Mon Sep 17 00:00:00 2001 From: lcawl Date: Wed, 16 Sep 2026 16:05:19 -0500 Subject: [PATCH 2/5] Address review comments --- .../Reconciliation/NoteAmendReconciler.cs | 18 ++- .../Reconciliation/NotesIndexReconciler.cs | 122 +++++++++++++----- .../NoteAmendReconcilerTests.cs | 75 ++++++++++- .../NotesIndexReconcilerTests.cs | 27 +++- 4 files changed, 200 insertions(+), 42 deletions(-) diff --git a/src/services/Elastic.Changelog/Reconciliation/NoteAmendReconciler.cs b/src/services/Elastic.Changelog/Reconciliation/NoteAmendReconciler.cs index 914885b1e2..a7c6648a68 100644 --- a/src/services/Elastic.Changelog/Reconciliation/NoteAmendReconciler.cs +++ b/src/services/Elastic.Changelog/Reconciliation/NoteAmendReconciler.cs @@ -53,12 +53,13 @@ public sealed class NoteAmendReconciler( /// For the given repository scope, scans each product that appears in /// (not every bundle/{product}/ prefix), writes or /// deletes that product's reconciler-owned amend sidecars, and re-writes product-scoped and - /// legacy notes indexes with correct bundle_seq values. + /// legacy notes indexes with correct bundle_seq values. Empty version lists mean that + /// product×version just vanished from the notes index; amend still runs so sidecars can drop. /// /// - /// Product ids that had an amend-notes write, skip-unchanged, or delete. Callers rebuild - /// those products' registry.json and the bundle shallow map. Products that were not - /// in the notes map are not returned and are not swept. + /// Product ids that had an amend-notes write, skip-unchanged, or delete (including when the + /// sidecar was already absent). Callers rebuild those products' registry.json and the + /// bundle shallow map. Products that were not in the notes map are not returned and are not swept. /// public async Task> ReconcileAsync( ChangelogScope notesScope, @@ -114,6 +115,9 @@ Cancel ctx CancellationToken = ctx }, async (write, ct) => { + if (write.Notes.Count == 0) + return; + var seqs = seqMap[ProductVersionKey(write.Product, write.Version)]; var updatedEntries = WithSeqs(write.Notes, seqs); var indexKey = ChangelogKeys.NotesIndexKey(org, repo, write.Product, write.Version); @@ -123,6 +127,9 @@ Cancel ctx foreach (var (version, unionNotes) in UnionByVersion(notesByProduct)) { ctx.ThrowIfCancellationRequested(); + if (unionNotes.Count == 0) + continue; + var seqs = MaxSeqsForVersion(notesByProduct.Keys, version, seqMap); var updatedEntries = WithSeqs(unionNotes, seqs); var indexKey = ChangelogKeys.NotesIndexKey(org, repo, version); @@ -492,7 +499,8 @@ private async Task DeleteAmendNotesIfExistsAsync(string key, Cancel ctx) } catch (AmazonS3Exception ex) when (ex.StatusCode == HttpStatusCode.NotFound) { - return false; + _logger.LogDebug("Amend-notes {Key} already absent; treating delete as done so registry rebuild can retry", key); + return true; } catch (AmazonS3Exception ex) when (ex.StatusCode == HttpStatusCode.PreconditionFailed) { diff --git a/src/services/Elastic.Changelog/Reconciliation/NotesIndexReconciler.cs b/src/services/Elastic.Changelog/Reconciliation/NotesIndexReconciler.cs index 2a973e870c..a6710ac736 100644 --- a/src/services/Elastic.Changelog/Reconciliation/NotesIndexReconciler.cs +++ b/src/services/Elastic.Changelog/Reconciliation/NotesIndexReconciler.cs @@ -50,7 +50,8 @@ public sealed class NotesIndexReconciler( /// /// Notes grouped by product, then version. Legacy version-union indexes are still written /// in this pass but are not part of the return value; uses - /// the product map only. + /// the product map only. A product×version whose notes-index was just deleted as stale is + /// included with an empty note list so amend can drop that product's sidecar. /// public async Task>>> ReconcileRepoAsync( ChangelogScope notesScope, @@ -86,51 +87,67 @@ Cancel ctx var existingIndexKeys = await ListExistingNotesIndexes(notesScope, ctx); var intendedKeys = IntendedIndexKeys(org, repo, byProductVersion.Keys, byVersion.Keys); - if (byVersion.Count == 0) - { - _logger.LogDebug("No versions found for repo {Repo}; removing any stale indexes", notesScope.Group); - await DeleteStaleIndexes(existingIndexKeys, intendedKeys, ctx); - return new Dictionary>>(StringComparer.Ordinal); - } - - var writes = BuildIndexWrites(org, repo, byProductVersion, byVersion); var writtenProducts = new Dictionary>>(StringComparer.Ordinal); + IReadOnlyList<(string Product, string Version)> vanished = []; try { - await Parallel.ForEachAsync(writes, new ParallelOptions + if (byVersion.Count == 0) { - MaxDegreeOfParallelism = MaxParallelReads, - CancellationToken = ctx - }, async (write, ct) => + _logger.LogDebug("No versions found for repo {Repo}; removing any stale indexes", notesScope.Group); + } + else { - await WriteIndexAsync( - write.Key, - write.Entries, - ct, - write.Product is null ? null : new NotesIndexMetadata(write.Product, write.Version!) - ); - if (write.Product is null) - return; - lock (writtenProducts) + var writes = BuildIndexWrites(org, repo, byProductVersion, byVersion); + await Parallel.ForEachAsync(writes, new ParallelOptions { - if (!writtenProducts.TryGetValue(write.Product, out var byVer)) - writtenProducts[write.Product] = byVer = [with(StringComparer.Ordinal)]; - byVer[write.Version!] = write.Entries; - } - }); + MaxDegreeOfParallelism = MaxParallelReads, + CancellationToken = ctx + }, async (write, ct) => + { + await WriteIndexAsync( + write.Key, + write.Entries, + ct, + write.Product is null ? null : new NotesIndexMetadata(write.Product, write.Version!) + ); + if (write.Product is null) + return; + lock (writtenProducts) + { + if (!writtenProducts.TryGetValue(write.Product, out var byVer)) + writtenProducts[write.Product] = byVer = [with(StringComparer.Ordinal)]; + byVer[write.Version!] = write.Entries; + } + }); + } } finally { - await DeleteStaleIndexes(existingIndexKeys, intendedKeys, ctx); + vanished = await DeleteStaleIndexes(existingIndexKeys, intendedKeys, ctx); } - return writtenProducts.ToDictionary( - kv => kv.Key, - kv => (IReadOnlyDictionary>)kv.Value, - StringComparer.Ordinal - ); + MergeVanishedProductVersions(writtenProducts, vanished); + return ToProductMap(writtenProducts); } + private static void MergeVanishedProductVersions( + Dictionary>> map, + IReadOnlyList<(string Product, string Version)> vanished + ) + { + foreach (var (product, version) in vanished) + { + if (!map.TryGetValue(product, out var byVer)) + map[product] = byVer = [with(StringComparer.Ordinal)]; + if (!byVer.ContainsKey(version)) + byVer[version] = []; + } + } + + private static IReadOnlyDictionary>> ToProductMap( + Dictionary>> map + ) => map.ToDictionary(kv => kv.Key, kv => (IReadOnlyDictionary>)kv.Value, StringComparer.Ordinal); + private static void AddIndexEntry(Dictionary> map, string groupKey, string poolRelativePath) { if (!map.TryGetValue(groupKey, out var entries)) @@ -208,13 +225,19 @@ private async Task> ListExistingNotesIndexes(ChangelogScop return keys; } - private async Task DeleteStaleIndexes(IReadOnlyList existingKeys, HashSet intendedKeys, Cancel ctx) + private async Task> DeleteStaleIndexes( + IReadOnlyList existingKeys, + HashSet intendedKeys, + Cancel ctx + ) { + var vanished = new List<(string Product, string Version)>(); foreach (var key in existingKeys) { if (intendedKeys.Contains(key)) continue; + var identity = await TryReadProductScopedIndexIdentity(key, ctx); try { _ = await s3Client.DeleteObjectAsync(new DeleteObjectRequest { BucketName = publicBucketName, Key = key }, ctx); @@ -223,7 +246,38 @@ private async Task DeleteStaleIndexes(IReadOnlyList existingKeys, HashSe catch (Exception ex) when (ex is not OperationCanceledException) { _logger.LogWarning(ex, "Failed to delete stale notes index {Key}", key); + continue; } + + if (identity is not null) + vanished.Add(identity.Value); + } + + return vanished; + } + + /// + /// Reads and from a + /// product-scoped body. Legacy version-union indexes omit those fields and are not vanished + /// products — callers must not parse the filename slug. + /// + private async Task<(string Product, string Version)?> TryReadProductScopedIndexIdentity(string key, Cancel ctx) + { + try + { + using var response = await s3Client.GetObjectAsync(new GetObjectRequest { BucketName = publicBucketName, Key = key }, ctx); + await using var stream = response.ResponseStream; + var index = await JsonSerializer.DeserializeAsync(stream, NotesIndexJsonContext.Default.NotesIndex, ctx); + if (index?.Product is not { Length: > 0 } product || index.Version is not { Length: > 0 } version) + return null; + if (!ChangelogKeys.IsValidProduct(product) || !ChangelogKeys.IsValidRepo(version)) + return null; + return (product, version); + } + catch (Exception ex) when (ex is not OperationCanceledException) + { + _logger.LogDebug(ex, "Could not read identity from stale notes index {Key}", key); + return null; } } diff --git a/tests/Elastic.Changelog.Tests/Reconciliation/NoteAmendReconcilerTests.cs b/tests/Elastic.Changelog.Tests/Reconciliation/NoteAmendReconcilerTests.cs index 45b7012278..ec4adbd068 100644 --- a/tests/Elastic.Changelog.Tests/Reconciliation/NoteAmendReconcilerTests.cs +++ b/tests/Elastic.Changelog.Tests/Reconciliation/NoteAmendReconcilerTests.cs @@ -259,10 +259,83 @@ public async Task NoteRemovedFromIndex_ExistingAmendSidecarDeleted() // No notes for this version (note was deleted from the pool). var notesByProduct = NotesByProduct(Product, Version); - await _reconciler.ReconcileAsync(NotesScope(), notesByProduct, TestContext.Current.CancellationToken); + var touched = await _reconciler.ReconcileAsync(NotesScope(), notesByProduct, TestContext.Current.CancellationToken); _s3.Exists(PublicBucket, AmendNotesKey(parent)).Should().BeFalse("stale amend sidecar must be deleted when no notes remain"); _s3.Deletes.Should().ContainSingle().Which.Key.Should().Be(AmendNotesKey(parent)); + touched.Should().Equal(Product); + } + + [Fact] + public async Task SidecarAlreadyAbsent_NoLateNotes_ProductStillTouched() + { + const string parent = "elasticsearch-9.3.0.yaml"; + _s3.Seed(PublicBucket, RegistryKey(), RegistryJson(parent)); + _s3.Seed(PublicBucket, BundleKey(parent), ParentBundleYaml("main/pr-100.yaml")); + + var notesByProduct = NotesByProduct(Product, Version); + var touched = await _reconciler.ReconcileAsync(NotesScope(), notesByProduct, TestContext.Current.CancellationToken); + + _s3.Exists(PublicBucket, AmendNotesKey(parent)).Should().BeFalse(); + touched.Should().Equal(Product); + } + + [Fact] + public async Task ReconcileRepoOmitsProduct_ExistingAmendSidecarDeleted() + { + const string parent = "elasticsearch-9.3.0.yaml"; + _s3.Seed(PublicBucket, RegistryKey(), RegistryJson(parent)); + _s3.Seed(PublicBucket, BundleKey(parent), ParentBundleYaml("main/pr-100.yaml")); + var staleAmend = new Bundle + { + Products = [new BundledProduct(Product, target: Version, lifecycle: Lifecycle.Ga)], + Entries = + [ + new BundledEntry + { + File = new BundledFile { Name = "main/note-cve.yml", Checksum = "old" }, + Title = "CVE", + Type = ChangelogEntryType.Security + } + ] + }; + _s3.Seed(PublicBucket, AmendNotesKey(parent), ReleaseNotesSerialization.SerializeBundle(staleAmend)); + _s3.Seed( + PublicBucket, + ChangelogKeys.NotesIndexKey(Org, Repo, Product, Version), + /*lang=json,strict*/ + """{"schema_version":1,"product":"elasticsearch","version":"9.3.0","notes":[{"path":"main/note-cve.yml","bundle_seq":2}]}""" + ); + _s3.Seed( + PublicBucket, + $"bundle/kibana/registry.json", + JsonSerializer.Serialize( + new ChangelogRegistry + { + Product = "kibana", + Bundles = [new ChangelogRegistryBundle { File = "kibana-9.3.0.yaml", Target = Version }] + }, + ChangelogRegistryJsonContext.Default.ChangelogRegistry + ) + ); + _s3.Seed(PublicBucket, "bundle/kibana/kibana-9.3.0.yaml", ParentBundleFor("kibana", Version, "main/note-kibana.yml")); + _s3.Seed( + PublicBucket, + NoteKey("main", "note-kibana.yml"), + "title: Kibana known issue\n" + "type: known-issue\n" + "products:\n" + " - product: kibana\n" + " versions: [9.3.0]\n" + ); + + var notesByProduct = await _notesReconciler.ReconcileRepoAsync(NotesScope(), TestContext.Current.CancellationToken); + notesByProduct.Should().ContainKey(Product); + notesByProduct[Product][Version].Should().BeEmpty(); + notesByProduct.Should().ContainKey("kibana"); + + var touched = await _reconciler.ReconcileAsync(NotesScope(), notesByProduct, TestContext.Current.CancellationToken); + + _s3.Exists(PublicBucket, AmendNotesKey(parent)).Should().BeFalse(); + touched.Should().Contain(Product); + _s3.Exists(PublicBucket, ChangelogKeys.NotesIndexKey(Org, Repo, Product, Version)).Should().BeFalse(); + _s3.Exists(PublicBucket, "bundle/kibana/kibana-9.3.0.amend-notes.yaml").Should().BeFalse(); } [Fact] diff --git a/tests/Elastic.Changelog.Tests/Reconciliation/NotesIndexReconcilerTests.cs b/tests/Elastic.Changelog.Tests/Reconciliation/NotesIndexReconcilerTests.cs index e2147b80fa..90354b4b83 100644 --- a/tests/Elastic.Changelog.Tests/Reconciliation/NotesIndexReconcilerTests.cs +++ b/tests/Elastic.Changelog.Tests/Reconciliation/NotesIndexReconcilerTests.cs @@ -294,7 +294,7 @@ public async Task ReconcileRepo_ProductScopedStale_DeletedWithoutDroppingLegacyV ); SeedNote("main", "note-slow-rollover.yml", NoteYaml); - await _reconciler.ReconcileRepoAsync(NotesScope(), TestContext.Current.CancellationToken); + var map = await _reconciler.ReconcileRepoAsync(NotesScope(), TestContext.Current.CancellationToken); _s3.Exists(PublicBucket, ChangelogKeys.NotesIndexKey("elastic", "elasticsearch", "9.0.0")).Should().BeTrue(); _s3.Exists(PublicBucket, ChangelogKeys.NotesIndexKey("elastic", "elasticsearch", "elasticsearch", "9.0.0")).Should().BeTrue(); @@ -306,6 +306,26 @@ public async Task ReconcileRepo_ProductScopedStale_DeletedWithoutDroppingLegacyV .Key .Should() .Be(ChangelogKeys.NotesIndexKey("elastic", "elasticsearch", "kibana", "9.0.0")); + map.Should().ContainKey("kibana"); + map["kibana"].Should().ContainKey("9.0.0"); + map["kibana"]["9.0.0"].Should().BeEmpty(); + map["elasticsearch"]["9.0.0"].Should().ContainSingle(e => e.Path == "main/note-slow-rollover.yml"); + } + + [Fact] + public async Task ReconcileRepo_NoNotes_LegacyIndexWithoutProduct_DoesNotInventVanishedProduct() + { + _s3.Seed( + PublicBucket, + ChangelogKeys.NotesIndexKey("elastic", "elasticsearch", "9.0.0"), + /*lang=json,strict*/ + """{"schema_version":1,"notes":[]}""" + ); + + var map = await _reconciler.ReconcileRepoAsync(NotesScope(), TestContext.Current.CancellationToken); + + map.Should().BeEmpty(); + _s3.Deletes.Should().ContainSingle().Which.Key.Should().Be(ChangelogKeys.NotesIndexKey("elastic", "elasticsearch", "9.0.0")); } [Fact] @@ -325,7 +345,7 @@ public async Task ReconcileRepo_NoNotes_DeletesProductScopedAndLegacyIndexes() ); _s3.Seed(PublicBucket, "changelog/elastic/elasticsearch/main/12345.yaml", "title: PR entry"); - await _reconciler.ReconcileRepoAsync(NotesScope(), TestContext.Current.CancellationToken); + var map = await _reconciler.ReconcileRepoAsync(NotesScope(), TestContext.Current.CancellationToken); _s3.Puts.Should().BeEmpty(); _s3 @@ -336,5 +356,8 @@ public async Task ReconcileRepo_NoNotes_DeletesProductScopedAndLegacyIndexes() ChangelogKeys.NotesIndexKey("elastic", "elasticsearch", "9.0.0"), ChangelogKeys.NotesIndexKey("elastic", "elasticsearch", "elasticsearch", "9.0.0") ]); + map.Should().ContainKey("elasticsearch"); + map["elasticsearch"]["9.0.0"].Should().BeEmpty(); + map.Should().NotContainKey("9.0.0"); } } From 81fedf1522ab7baca3d57c1f99faadf4cfeeeb12 Mon Sep 17 00:00:00 2001 From: lcawl Date: Wed, 16 Sep 2026 16:39:56 -0500 Subject: [PATCH 3/5] Fix review comment re swallowing stale-index identity read failures --- .../Reconciliation/NotesIndexReconciler.cs | 41 +++++++++++++------ .../NotesIndexReconcilerTests.cs | 41 +++++++++++++++++++ 2 files changed, 70 insertions(+), 12 deletions(-) diff --git a/src/services/Elastic.Changelog/Reconciliation/NotesIndexReconciler.cs b/src/services/Elastic.Changelog/Reconciliation/NotesIndexReconciler.cs index a6710ac736..8c93b039e3 100644 --- a/src/services/Elastic.Changelog/Reconciliation/NotesIndexReconciler.cs +++ b/src/services/Elastic.Changelog/Reconciliation/NotesIndexReconciler.cs @@ -237,7 +237,10 @@ Cancel ctx if (intendedKeys.Contains(key)) continue; - var identity = await TryReadProductScopedIndexIdentity(key, ctx); + var read = await ReadStaleIndexIdentity(key, ctx); + if (read.Kind is StaleIndexReadKind.ReadFailed or StaleIndexReadKind.AlreadyGone) + continue; + try { _ = await s3Client.DeleteObjectAsync(new DeleteObjectRequest { BucketName = publicBucketName, Key = key }, ctx); @@ -249,19 +252,19 @@ Cancel ctx continue; } - if (identity is not null) - vanished.Add(identity.Value); + if (read.Kind == StaleIndexReadKind.ProductScoped) + vanished.Add((read.Product!, read.Version!)); } return vanished; } /// - /// Reads and from a - /// product-scoped body. Legacy version-union indexes omit those fields and are not vanished - /// products — callers must not parse the filename slug. + /// Reads identity from a stale notes-index body. Does not parse the filename slug. + /// A failed GET/deserialize must not delete the object; a legacy body (no product/version) + /// may be deleted without a vanished product. /// - private async Task<(string Product, string Version)?> TryReadProductScopedIndexIdentity(string key, Cancel ctx) + private async Task ReadStaleIndexIdentity(string key, Cancel ctx) { try { @@ -269,15 +272,19 @@ Cancel ctx await using var stream = response.ResponseStream; var index = await JsonSerializer.DeserializeAsync(stream, NotesIndexJsonContext.Default.NotesIndex, ctx); if (index?.Product is not { Length: > 0 } product || index.Version is not { Length: > 0 } version) - return null; + return new StaleIndexRead(StaleIndexReadKind.Legacy); if (!ChangelogKeys.IsValidProduct(product) || !ChangelogKeys.IsValidRepo(version)) - return null; - return (product, version); + return new StaleIndexRead(StaleIndexReadKind.Legacy); + return new StaleIndexRead(StaleIndexReadKind.ProductScoped, product, version); + } + catch (AmazonS3Exception ex) when (ex.StatusCode == HttpStatusCode.NotFound) + { + return new StaleIndexRead(StaleIndexReadKind.AlreadyGone); } catch (Exception ex) when (ex is not OperationCanceledException) { - _logger.LogDebug(ex, "Could not read identity from stale notes index {Key}", key); - return null; + _logger.LogWarning(ex, "Could not read identity from stale notes index {Key}; leaving it for the next reconcile", key); + return new StaleIndexRead(StaleIndexReadKind.ReadFailed); } } @@ -520,6 +527,16 @@ public async Task WriteIndexAsync(string key, IReadOnlyList entr } private readonly record struct NotesIndexWrite(string Key, IReadOnlyList Entries, string? Product, string? Version); + + private enum StaleIndexReadKind + { + ReadFailed, + AlreadyGone, + Legacy, + ProductScoped + } + + private readonly record struct StaleIndexRead(StaleIndexReadKind Kind, string? Product = null, string? Version = null); } /// Optional product and version written onto a product-scoped notes index body. diff --git a/tests/Elastic.Changelog.Tests/Reconciliation/NotesIndexReconcilerTests.cs b/tests/Elastic.Changelog.Tests/Reconciliation/NotesIndexReconcilerTests.cs index 90354b4b83..975a66e00b 100644 --- a/tests/Elastic.Changelog.Tests/Reconciliation/NotesIndexReconcilerTests.cs +++ b/tests/Elastic.Changelog.Tests/Reconciliation/NotesIndexReconcilerTests.cs @@ -2,7 +2,9 @@ // Elasticsearch B.V licenses this file to you under the Apache 2.0 License. // See the LICENSE file in the project root for more information +using System.Net; using System.Text.Json; +using Amazon.S3; using AwesomeAssertions; using Elastic.Changelog.Reconciliation; using Elastic.Documentation.Configuration.ReleaseNotes; @@ -360,4 +362,43 @@ public async Task ReconcileRepo_NoNotes_DeletesProductScopedAndLegacyIndexes() map["elasticsearch"]["9.0.0"].Should().BeEmpty(); map.Should().NotContainKey("9.0.0"); } + + [Fact] + public async Task ReconcileRepo_UnparseableProductScopedIndex_IsLeftInPlace() + { + var kibanaKey = ChangelogKeys.NotesIndexKey("elastic", "elasticsearch", "kibana", "9.0.0"); + _s3.Seed(PublicBucket, kibanaKey, "{not-json"); + SeedNote("main", "note-slow-rollover.yml", NoteYaml); + + var map = await _reconciler.ReconcileRepoAsync(NotesScope(), TestContext.Current.CancellationToken); + + _s3.Exists(PublicBucket, kibanaKey).Should().BeTrue(); + _s3.Deletes.Should().NotContain(d => d.Key == kibanaKey); + map.Should().NotContainKey("kibana"); + map["elasticsearch"]["9.0.0"].Should().ContainSingle(e => e.Path == "main/note-slow-rollover.yml"); + } + + [Fact] + public async Task ReconcileRepo_ProductScopedIndexGetFailure_IsLeftInPlace() + { + var kibanaKey = ChangelogKeys.NotesIndexKey("elastic", "elasticsearch", "kibana", "9.0.0"); + _s3.Seed( + PublicBucket, + kibanaKey, + /*lang=json,strict*/ + """{"schema_version":1,"product":"kibana","version":"9.0.0","notes":[]}""" + ); + SeedNote("main", "note-slow-rollover.yml", NoteYaml); + _s3.AfterGet = (key, _) => + { + if (key == kibanaKey) + throw new AmazonS3Exception("unavailable") { StatusCode = HttpStatusCode.InternalServerError }; + }; + + var map = await _reconciler.ReconcileRepoAsync(NotesScope(), TestContext.Current.CancellationToken); + + _s3.Exists(PublicBucket, kibanaKey).Should().BeTrue(); + _s3.Deletes.Should().NotContain(d => d.Key == kibanaKey); + map.Should().NotContainKey("kibana"); + } } From b7c4dacc0c20fb1536162d57ad891a817a55ac25 Mon Sep 17 00:00:00 2001 From: lcawl Date: Wed, 16 Sep 2026 17:09:38 -0500 Subject: [PATCH 4/5] Fix review comment re transient delete failures --- .../Reconciliation/NoteAmendReconciler.cs | 14 +++--- .../Reconciliation/NotesIndexReconciler.cs | 32 ++++++++++--- .../Reconciliation/FakeS3.cs | 6 +++ .../NoteAmendReconcilerTests.cs | 46 ++++++++++++++++++- .../NotesIndexReconcilerTests.cs | 30 ++++-------- 5 files changed, 94 insertions(+), 34 deletions(-) diff --git a/src/services/Elastic.Changelog/Reconciliation/NoteAmendReconciler.cs b/src/services/Elastic.Changelog/Reconciliation/NoteAmendReconciler.cs index a7c6648a68..3211bea69a 100644 --- a/src/services/Elastic.Changelog/Reconciliation/NoteAmendReconciler.cs +++ b/src/services/Elastic.Changelog/Reconciliation/NoteAmendReconciler.cs @@ -95,7 +95,7 @@ Cancel ctx _ = touched.Add(product); } - await RewriteNotesIndexesAsync(org, repo, notesByProduct, seqMap, ctx); + await RewriteNotesIndexesAsync(org, repo, notesByProduct, seqMap, touched, ctx); return [.. touched]; } @@ -104,6 +104,7 @@ private async Task RewriteNotesIndexesAsync( string repo, IReadOnlyDictionary>> notesByProduct, IReadOnlyDictionary> seqMap, + HashSet touchedProducts, Cancel ctx ) { @@ -115,12 +116,16 @@ Cancel ctx CancellationToken = ctx }, async (write, ct) => { + var indexKey = ChangelogKeys.NotesIndexKey(org, repo, write.Product, write.Version); if (write.Notes.Count == 0) + { + if (touchedProducts.Contains(write.Product)) + await notesIndexReconciler.DeleteIndexAsync(indexKey, ct); return; + } var seqs = seqMap[ProductVersionKey(write.Product, write.Version)]; var updatedEntries = WithSeqs(write.Notes, seqs); - var indexKey = ChangelogKeys.NotesIndexKey(org, repo, write.Product, write.Version); await notesIndexReconciler.WriteIndexAsync(indexKey, updatedEntries, ct, new NotesIndexMetadata(write.Product, write.Version)); }); @@ -507,11 +512,6 @@ private async Task DeleteAmendNotesIfExistsAsync(string key, Cancel ctx) _logger.LogDebug("Amend-notes {Key} was updated concurrently; delete skipped", key); return true; } - catch (Exception ex) when (ex is not OperationCanceledException) - { - _logger.LogWarning(ex, "Could not delete stale amend-notes sidecar {Key}; will retry on next reconcile", key); - return false; - } } // ----------------------------------------------------------------------------------------- diff --git a/src/services/Elastic.Changelog/Reconciliation/NotesIndexReconciler.cs b/src/services/Elastic.Changelog/Reconciliation/NotesIndexReconciler.cs index 8c93b039e3..60aa4cb32a 100644 --- a/src/services/Elastic.Changelog/Reconciliation/NotesIndexReconciler.cs +++ b/src/services/Elastic.Changelog/Reconciliation/NotesIndexReconciler.cs @@ -50,8 +50,9 @@ public sealed class NotesIndexReconciler( /// /// Notes grouped by product, then version. Legacy version-union indexes are still written /// in this pass but are not part of the return value; uses - /// the product map only. A product×version whose notes-index was just deleted as stale is - /// included with an empty note list so amend can drop that product's sidecar. + /// the product map only. A product×version whose product-scoped index is stale is included + /// with an empty note list so amend can drop that product's sidecar. Those keys stay in S3 + /// until amend succeeds; leftover version-union indexes are deleted in this pass. /// public async Task>>> ReconcileRepoAsync( ChangelogScope notesScope, @@ -241,6 +242,12 @@ Cancel ctx if (read.Kind is StaleIndexReadKind.ReadFailed or StaleIndexReadKind.AlreadyGone) continue; + if (read.Kind == StaleIndexReadKind.ProductScoped) + { + vanished.Add((read.Product!, read.Version!)); + continue; + } + try { _ = await s3Client.DeleteObjectAsync(new DeleteObjectRequest { BucketName = publicBucketName, Key = key }, ctx); @@ -249,11 +256,7 @@ Cancel ctx catch (Exception ex) when (ex is not OperationCanceledException) { _logger.LogWarning(ex, "Failed to delete stale notes index {Key}", key); - continue; } - - if (read.Kind == StaleIndexReadKind.ProductScoped) - vanished.Add((read.Product!, read.Version!)); } return vanished; @@ -403,6 +406,23 @@ private static bool IsNoteFileName(string fileName) => } } + /// + /// Deletes a notes-index object. A missing key is success so amend can drop a vanished + /// product's index after the sidecar work without racing a prior delete. + /// + public async Task DeleteIndexAsync(string key, Cancel ctx) + { + try + { + _ = await s3Client.DeleteObjectAsync(new DeleteObjectRequest { BucketName = publicBucketName, Key = key }, ctx); + _logger.LogInformation("Removed notes index {Key}", key); + } + catch (AmazonS3Exception ex) when (ex.StatusCode == HttpStatusCode.NotFound) + { + _logger.LogDebug("Notes index {Key} already absent", key); + } + } + /// /// Writes the notes index with conditional S3 writes (If-Match / If-None-Match) to guard against /// concurrent reconcile races, mirroring the pattern used by . diff --git a/tests/Elastic.Changelog.Tests/Reconciliation/FakeS3.cs b/tests/Elastic.Changelog.Tests/Reconciliation/FakeS3.cs index eb473b305b..e2bc6f725e 100644 --- a/tests/Elastic.Changelog.Tests/Reconciliation/FakeS3.cs +++ b/tests/Elastic.Changelog.Tests/Reconciliation/FakeS3.cs @@ -49,6 +49,9 @@ internal sealed class FakeS3 /// Runs before each DeleteObject is evaluated, with the 1-based call number. public Action? BeforeDelete { get; set; } + /// When set, thrown from DeleteObject for that key before the store is mutated. + public Func? DeleteFault { get; set; } + /// Runs after a GetObject resolved its content (which is returned unchanged), with the key and 1-based call number — simulates the source changing right after a read. public Action? AfterGet { get; set; } @@ -227,6 +230,9 @@ private DeleteObjectResponse Delete(DeleteObjectRequest request) lock (_lock) n = ++_deletes; BeforeDelete?.Invoke(n); + var fault = DeleteFault?.Invoke(request.Key); + if (fault is not null) + throw fault; lock (_lock) { diff --git a/tests/Elastic.Changelog.Tests/Reconciliation/NoteAmendReconcilerTests.cs b/tests/Elastic.Changelog.Tests/Reconciliation/NoteAmendReconcilerTests.cs index ec4adbd068..0de2c2e960 100644 --- a/tests/Elastic.Changelog.Tests/Reconciliation/NoteAmendReconcilerTests.cs +++ b/tests/Elastic.Changelog.Tests/Reconciliation/NoteAmendReconcilerTests.cs @@ -2,7 +2,9 @@ // Elasticsearch B.V licenses this file to you under the Apache 2.0 License. // See the LICENSE file in the project root for more information +using System.Net; using System.Text.Json; +using Amazon.S3; using AwesomeAssertions; using Elastic.Changelog.Reconciliation; using Elastic.Documentation; @@ -262,7 +264,8 @@ public async Task NoteRemovedFromIndex_ExistingAmendSidecarDeleted() var touched = await _reconciler.ReconcileAsync(NotesScope(), notesByProduct, TestContext.Current.CancellationToken); _s3.Exists(PublicBucket, AmendNotesKey(parent)).Should().BeFalse("stale amend sidecar must be deleted when no notes remain"); - _s3.Deletes.Should().ContainSingle().Which.Key.Should().Be(AmendNotesKey(parent)); + _s3.Deletes.Select(d => d.Key).Should().Contain(AmendNotesKey(parent)); + _s3.Deletes.Select(d => d.Key).Should().Contain(ChangelogKeys.NotesIndexKey(Org, Repo, Product, Version)); touched.Should().Equal(Product); } @@ -329,6 +332,7 @@ public async Task ReconcileRepoOmitsProduct_ExistingAmendSidecarDeleted() notesByProduct.Should().ContainKey(Product); notesByProduct[Product][Version].Should().BeEmpty(); notesByProduct.Should().ContainKey("kibana"); + _s3.Exists(PublicBucket, ChangelogKeys.NotesIndexKey(Org, Repo, Product, Version)).Should().BeTrue(); var touched = await _reconciler.ReconcileAsync(NotesScope(), notesByProduct, TestContext.Current.CancellationToken); @@ -338,6 +342,46 @@ public async Task ReconcileRepoOmitsProduct_ExistingAmendSidecarDeleted() _s3.Exists(PublicBucket, "bundle/kibana/kibana-9.3.0.amend-notes.yaml").Should().BeFalse(); } + [Fact] + public async Task SidecarDeleteIoFailure_Throws_AndLeavesProductScopedIndex() + { + const string parent = "elasticsearch-9.3.0.yaml"; + _s3.Seed(PublicBucket, RegistryKey(), RegistryJson(parent)); + _s3.Seed(PublicBucket, BundleKey(parent), ParentBundleYaml("main/pr-100.yaml")); + var staleAmend = new Bundle + { + Products = [new BundledProduct(Product, target: Version, lifecycle: Lifecycle.Ga)], + Entries = + [ + new BundledEntry + { + File = new BundledFile { Name = "main/note-cve.yml", Checksum = "old" }, + Title = "CVE", + Type = ChangelogEntryType.Security + } + ] + }; + _s3.Seed(PublicBucket, AmendNotesKey(parent), ReleaseNotesSerialization.SerializeBundle(staleAmend)); + _s3.Seed( + PublicBucket, + ChangelogKeys.NotesIndexKey(Org, Repo, Product, Version), + /*lang=json,strict*/ + """{"schema_version":1,"product":"elasticsearch","version":"9.3.0","notes":[]}""" + ); + + var notesByProduct = await _notesReconciler.ReconcileRepoAsync(NotesScope(), TestContext.Current.CancellationToken); + _s3.Exists(PublicBucket, ChangelogKeys.NotesIndexKey(Org, Repo, Product, Version)).Should().BeTrue(); + + var sidecarKey = AmendNotesKey(parent); + _s3.DeleteFault = + key => key == sidecarKey ? new AmazonS3Exception("unavailable") { StatusCode = HttpStatusCode.InternalServerError } : null; + + var act = async () => await _reconciler.ReconcileAsync(NotesScope(), notesByProduct, TestContext.Current.CancellationToken); + await act.Should().ThrowAsync(); + _s3.Exists(PublicBucket, sidecarKey).Should().BeTrue(); + _s3.Exists(PublicBucket, ChangelogKeys.NotesIndexKey(Org, Repo, Product, Version)).Should().BeTrue(); + } + [Fact] public async Task Idempotent_SameStateRedelivered_NoSecondPut() { diff --git a/tests/Elastic.Changelog.Tests/Reconciliation/NotesIndexReconcilerTests.cs b/tests/Elastic.Changelog.Tests/Reconciliation/NotesIndexReconcilerTests.cs index 975a66e00b..06d2f68e31 100644 --- a/tests/Elastic.Changelog.Tests/Reconciliation/NotesIndexReconcilerTests.cs +++ b/tests/Elastic.Changelog.Tests/Reconciliation/NotesIndexReconcilerTests.cs @@ -286,11 +286,12 @@ public async Task ReconcileRepo_TwoProductsSameVersion_ProductIndexesAreIsolated } [Fact] - public async Task ReconcileRepo_ProductScopedStale_DeletedWithoutDroppingLegacyVersion() + public async Task ReconcileRepo_ProductScopedStale_ReturnedAsEmptyBucketWithoutDeleting() { + var kibanaKey = ChangelogKeys.NotesIndexKey("elastic", "elasticsearch", "kibana", "9.0.0"); _s3.Seed( PublicBucket, - ChangelogKeys.NotesIndexKey("elastic", "elasticsearch", "kibana", "9.0.0"), + kibanaKey, /*lang=json,strict*/ """{"schema_version":1,"product":"kibana","version":"9.0.0","notes":[]}""" ); @@ -300,14 +301,8 @@ public async Task ReconcileRepo_ProductScopedStale_DeletedWithoutDroppingLegacyV _s3.Exists(PublicBucket, ChangelogKeys.NotesIndexKey("elastic", "elasticsearch", "9.0.0")).Should().BeTrue(); _s3.Exists(PublicBucket, ChangelogKeys.NotesIndexKey("elastic", "elasticsearch", "elasticsearch", "9.0.0")).Should().BeTrue(); - _s3 - .Deletes - .Should() - .ContainSingle() - .Which - .Key - .Should() - .Be(ChangelogKeys.NotesIndexKey("elastic", "elasticsearch", "kibana", "9.0.0")); + _s3.Exists(PublicBucket, kibanaKey).Should().BeTrue(); + _s3.Deletes.Should().NotContain(d => d.Key == kibanaKey); map.Should().ContainKey("kibana"); map["kibana"].Should().ContainKey("9.0.0"); map["kibana"]["9.0.0"].Should().BeEmpty(); @@ -331,8 +326,9 @@ public async Task ReconcileRepo_NoNotes_LegacyIndexWithoutProduct_DoesNotInventV } [Fact] - public async Task ReconcileRepo_NoNotes_DeletesProductScopedAndLegacyIndexes() + public async Task ReconcileRepo_NoNotes_DeletesLegacyOnly_KeepsProductScopedForAmend() { + var productKey = ChangelogKeys.NotesIndexKey("elastic", "elasticsearch", "elasticsearch", "9.0.0"); _s3.Seed( PublicBucket, ChangelogKeys.NotesIndexKey("elastic", "elasticsearch", "9.0.0"), @@ -341,7 +337,7 @@ public async Task ReconcileRepo_NoNotes_DeletesProductScopedAndLegacyIndexes() ); _s3.Seed( PublicBucket, - ChangelogKeys.NotesIndexKey("elastic", "elasticsearch", "elasticsearch", "9.0.0"), + productKey, /*lang=json,strict*/ """{"schema_version":1,"product":"elasticsearch","version":"9.0.0","notes":[]}""" ); @@ -350,14 +346,8 @@ public async Task ReconcileRepo_NoNotes_DeletesProductScopedAndLegacyIndexes() var map = await _reconciler.ReconcileRepoAsync(NotesScope(), TestContext.Current.CancellationToken); _s3.Puts.Should().BeEmpty(); - _s3 - .Deletes - .Select(d => d.Key) - .Should() - .BeEquivalentTo([ - ChangelogKeys.NotesIndexKey("elastic", "elasticsearch", "9.0.0"), - ChangelogKeys.NotesIndexKey("elastic", "elasticsearch", "elasticsearch", "9.0.0") - ]); + _s3.Deletes.Should().ContainSingle().Which.Key.Should().Be(ChangelogKeys.NotesIndexKey("elastic", "elasticsearch", "9.0.0")); + _s3.Exists(PublicBucket, productKey).Should().BeTrue(); map.Should().ContainKey("elasticsearch"); map["elasticsearch"]["9.0.0"].Should().BeEmpty(); map.Should().NotContainKey("9.0.0"); From 97dbbf2ce3eb57f1e41cb359d6702410342ebd25 Mon Sep 17 00:00:00 2001 From: lcawl Date: Tue, 22 Sep 2026 14:25:05 -0500 Subject: [PATCH 5/5] Address review comment about deleting vanished product index --- docs/cli/changelog/cmd-note.md | 2 +- docs/development/changelog-bundle-registry.md | 8 ++-- .../Reconciliation/NoteAmendReconciler.cs | 32 +++++++++---- .../Reconciliation/NotesIndexReconciler.cs | 4 +- .../Scrubbing/ScrubberProcessor.cs | 10 +++- .../Reconciliation/FakeS3.cs | 6 +++ .../NoteAmendReconcilerTests.cs | 24 ++++++---- .../Scrubbing/ScrubberProcessorTests.cs | 48 +++++++++++++++++++ 8 files changed, 107 insertions(+), 27 deletions(-) diff --git a/docs/cli/changelog/cmd-note.md b/docs/cli/changelog/cmd-note.md index e473acc4be..7b5efc3838 100644 --- a/docs/cli/changelog/cmd-note.md +++ b/docs/cli/changelog/cmd-note.md @@ -66,7 +66,7 @@ The scrubber writes two indexes per note: `{changelog}` does not read these indexes. It loads published bundle YAML (and amend sidecars listed in `bundle/{product}/registry.json`). -If the release bundle for that product and version or date has already shipped when you upload, the scrubber generates an amend sidecar for **that product**, then rebuilds that product's `bundle/{product}/registry.json` so `{changelog}` `:cdn:` pages pick it up. Other products that share the version are left alone. Don't run `changelog bundle-amend --add` for that file. Refer to [](/data/release-notes/bundle.md#changelog-bundle-notes-after-ship). +If the release bundle for that product and version or date has already shipped when you upload, the scrubber generates an amend sidecar for **that product**, then rebuilds that product's `bundle/{product}/registry.json` so `{changelog}` `:cdn:` pages pick it up. Other products that share the version are left alone. Don't run `changelog bundle-amend --add` for that file. Refer to [](/data/release-notes/bundle.md#changelog-bundle-notes-after-ship). An empty product-scoped notes index stays until that registry rebuild succeeds, so a failed registry write can retry with the vanished product still in the notes map. If there is no existing or planned bundle for that product and version or date, you can create a bundle from a path list that contains all the relevant changelogs. Refer to [Bundle by file paths](/cli/changelog/bundle.md#changelog-bundle-files). diff --git a/docs/development/changelog-bundle-registry.md b/docs/development/changelog-bundle-registry.md index c99e71847e..daee6651b0 100644 --- a/docs/development/changelog-bundle-registry.md +++ b/docs/development/changelog-bundle-registry.md @@ -75,7 +75,8 @@ narrowed reconciliation to the bundle tree): from current state on every reconcile, so redelivered events never produce duplicate amends. After a write, skip-unchanged, or delete of that sidecar, the same pass rebuilds `bundle/{product}/registry.json` and the bundle shallow map so `{changelog}` `:cdn:` can - discover it. Other products at the same version are not walked and their sidecars are not + discover it. An empty product-scoped notes index is removed only after that registry + rebuild succeeds. Other products at the same version are not walked and their sidecars are not deleted. `{changelog}` `:cdn:` and `changelog render` merge this sidecar into the parent the same way as numbered `.amend-{N}` files, after those numbered amends. The `.amend-notes` suffix is **reserved** — do not create files with that suffix manually; see @@ -172,8 +173,9 @@ amend sidecars. Do not hand-edit `notes-{version}.json` or `.amend-notes` sideca delete pool objects through docs-builder today. A 404 on both the product-scoped index and the legacy version-union index means "no notes -published for this product and version". An empty `notes` array never appears on a successfully -reconciled index — the index is deleted rather than emptied, following the same +published for this product and version". An empty `notes` array is not the durable form of a +successfully reconciled index: after sidecar work and a successful product registry rebuild, +the empty product-scoped index is deleted rather than rewritten empty, following the same [absent ≠ empty](#absent-empty) rule as the bundle registry. Until older clients stop reading `notes-{version}.json`, the Lambda keeps that key while any note still declares the version. diff --git a/src/services/Elastic.Changelog/Reconciliation/NoteAmendReconciler.cs b/src/services/Elastic.Changelog/Reconciliation/NoteAmendReconciler.cs index 3211bea69a..cc65f9f154 100644 --- a/src/services/Elastic.Changelog/Reconciliation/NoteAmendReconciler.cs +++ b/src/services/Elastic.Changelog/Reconciliation/NoteAmendReconciler.cs @@ -2,6 +2,7 @@ // Elasticsearch B.V licenses this file to you under the Apache 2.0 License. // See the LICENSE file in the project root for more information +using System.Collections.Concurrent; using System.Net; using System.Text.Json; using Amazon.S3; @@ -57,18 +58,19 @@ public sealed class NoteAmendReconciler( /// product×version just vanished from the notes index; amend still runs so sidecars can drop. /// /// - /// Product ids that had an amend-notes write, skip-unchanged, or delete (including when the - /// sidecar was already absent). Callers rebuild those products' registry.json and the - /// bundle shallow map. Products that were not in the notes map are not returned and are not swept. + /// Products that had an amend-notes write, skip-unchanged, or delete (including when the + /// sidecar was already absent), plus empty product-scoped notes-index keys to delete after + /// those products' registry rebuild succeeds. Products that were not in the notes map are + /// not returned and are not swept. /// - public async Task> ReconcileAsync( + public async Task ReconcileAsync( ChangelogScope notesScope, IReadOnlyDictionary>> notesByProduct, Cancel ctx ) { if (notesByProduct.Count == 0) - return []; + return new NoteAmendOutcome([], []); var groupParts = notesScope.Group.Split('/'); var (org, repo) = (groupParts[0], groupParts[1]); @@ -95,11 +97,11 @@ Cancel ctx _ = touched.Add(product); } - await RewriteNotesIndexesAsync(org, repo, notesByProduct, seqMap, touched, ctx); - return [.. touched]; + var emptyIndexes = await RewriteNotesIndexesAsync(org, repo, notesByProduct, seqMap, touched, ctx); + return new NoteAmendOutcome([.. touched], emptyIndexes); } - private async Task RewriteNotesIndexesAsync( + private async Task> RewriteNotesIndexesAsync( string org, string repo, IReadOnlyDictionary>> notesByProduct, @@ -109,6 +111,7 @@ Cancel ctx ) { var productWrites = notesByProduct.SelectMany(p => p.Value.Select(v => (Product: p.Key, Version: v.Key, Notes: v.Value))).ToList(); + var emptyIndexes = new ConcurrentBag(); await Parallel.ForEachAsync(productWrites, new ParallelOptions { @@ -120,7 +123,7 @@ Cancel ctx if (write.Notes.Count == 0) { if (touchedProducts.Contains(write.Product)) - await notesIndexReconciler.DeleteIndexAsync(indexKey, ct); + emptyIndexes.Add(new NoteAmendEmptyIndex(write.Product, indexKey)); return; } @@ -140,6 +143,8 @@ Cancel ctx var indexKey = ChangelogKeys.NotesIndexKey(org, repo, version); await notesIndexReconciler.WriteIndexAsync(indexKey, updatedEntries, ctx); } + + return [.. emptyIndexes]; } private static List WithSeqs(IReadOnlyList notes, IReadOnlyDictionary seqs) => @@ -553,3 +558,12 @@ private async Task DeleteAmendNotesIfExistsAsync(string key, Cancel ctx) return slash >= 0 ? normalized[(slash + 1)..] : normalized; } } + +/// +/// Products whose amend sidecar was written, skipped as unchanged, or deleted, plus product-scoped +/// notes-index keys that are empty and must be deleted after those products' registry rebuild succeeds. +/// +public sealed record NoteAmendOutcome(IReadOnlyList TouchedProducts, IReadOnlyList EmptyProductIndexes); + +/// A product-scoped notes index with no remaining notes. +public sealed record NoteAmendEmptyIndex(string Product, string Key); diff --git a/src/services/Elastic.Changelog/Reconciliation/NotesIndexReconciler.cs b/src/services/Elastic.Changelog/Reconciliation/NotesIndexReconciler.cs index 60aa4cb32a..d3c4707865 100644 --- a/src/services/Elastic.Changelog/Reconciliation/NotesIndexReconciler.cs +++ b/src/services/Elastic.Changelog/Reconciliation/NotesIndexReconciler.cs @@ -407,8 +407,8 @@ private static bool IsNoteFileName(string fileName) => } /// - /// Deletes a notes-index object. A missing key is success so amend can drop a vanished - /// product's index after the sidecar work without racing a prior delete. + /// Deletes a notes-index object. A missing key is success so a retry can drop a vanished + /// product's index after registry rebuild without racing a prior delete. /// public async Task DeleteIndexAsync(string key, Cancel ctx) { diff --git a/src/services/Elastic.Changelog/Scrubbing/ScrubberProcessor.cs b/src/services/Elastic.Changelog/Scrubbing/ScrubberProcessor.cs index 51f25a1360..b79e89ef02 100644 --- a/src/services/Elastic.Changelog/Scrubbing/ScrubberProcessor.cs +++ b/src/services/Elastic.Changelog/Scrubbing/ScrubberProcessor.cs @@ -141,14 +141,20 @@ public async Task> ProcessAsync(IReadOnlyListWhen set, thrown from DeleteObject for that key before the store is mutated. public Func? DeleteFault { get; set; } + /// When set, thrown from PutObject for that key before the store is mutated. + public Func? PutFault { get; set; } + /// Runs after a GetObject resolved its content (which is returned unchanged), with the key and 1-based call number — simulates the source changing right after a read. public Action? AfterGet { get; set; } @@ -207,6 +210,9 @@ private PutObjectResponse Put(PutObjectRequest request) lock (_lock) n = ++_puts; BeforePut?.Invoke(n); + var fault = PutFault?.Invoke(request.Key); + if (fault is not null) + throw fault; lock (_lock) { diff --git a/tests/Elastic.Changelog.Tests/Reconciliation/NoteAmendReconcilerTests.cs b/tests/Elastic.Changelog.Tests/Reconciliation/NoteAmendReconcilerTests.cs index 0de2c2e960..7f7b31e614 100644 --- a/tests/Elastic.Changelog.Tests/Reconciliation/NoteAmendReconcilerTests.cs +++ b/tests/Elastic.Changelog.Tests/Reconciliation/NoteAmendReconcilerTests.cs @@ -261,12 +261,13 @@ public async Task NoteRemovedFromIndex_ExistingAmendSidecarDeleted() // No notes for this version (note was deleted from the pool). var notesByProduct = NotesByProduct(Product, Version); - var touched = await _reconciler.ReconcileAsync(NotesScope(), notesByProduct, TestContext.Current.CancellationToken); + var outcome = await _reconciler.ReconcileAsync(NotesScope(), notesByProduct, TestContext.Current.CancellationToken); _s3.Exists(PublicBucket, AmendNotesKey(parent)).Should().BeFalse("stale amend sidecar must be deleted when no notes remain"); _s3.Deletes.Select(d => d.Key).Should().Contain(AmendNotesKey(parent)); - _s3.Deletes.Select(d => d.Key).Should().Contain(ChangelogKeys.NotesIndexKey(Org, Repo, Product, Version)); - touched.Should().Equal(Product); + _s3.Deletes.Select(d => d.Key).Should().NotContain(ProductIndexKey()); + outcome.TouchedProducts.Should().Equal(Product); + outcome.EmptyProductIndexes.Should().Equal(new NoteAmendEmptyIndex(Product, ProductIndexKey())); } [Fact] @@ -277,10 +278,11 @@ public async Task SidecarAlreadyAbsent_NoLateNotes_ProductStillTouched() _s3.Seed(PublicBucket, BundleKey(parent), ParentBundleYaml("main/pr-100.yaml")); var notesByProduct = NotesByProduct(Product, Version); - var touched = await _reconciler.ReconcileAsync(NotesScope(), notesByProduct, TestContext.Current.CancellationToken); + var outcome = await _reconciler.ReconcileAsync(NotesScope(), notesByProduct, TestContext.Current.CancellationToken); _s3.Exists(PublicBucket, AmendNotesKey(parent)).Should().BeFalse(); - touched.Should().Equal(Product); + outcome.TouchedProducts.Should().Equal(Product); + outcome.EmptyProductIndexes.Should().Equal(new NoteAmendEmptyIndex(Product, ProductIndexKey())); } [Fact] @@ -334,11 +336,12 @@ public async Task ReconcileRepoOmitsProduct_ExistingAmendSidecarDeleted() notesByProduct.Should().ContainKey("kibana"); _s3.Exists(PublicBucket, ChangelogKeys.NotesIndexKey(Org, Repo, Product, Version)).Should().BeTrue(); - var touched = await _reconciler.ReconcileAsync(NotesScope(), notesByProduct, TestContext.Current.CancellationToken); + var outcome = await _reconciler.ReconcileAsync(NotesScope(), notesByProduct, TestContext.Current.CancellationToken); _s3.Exists(PublicBucket, AmendNotesKey(parent)).Should().BeFalse(); - touched.Should().Contain(Product); - _s3.Exists(PublicBucket, ChangelogKeys.NotesIndexKey(Org, Repo, Product, Version)).Should().BeFalse(); + outcome.TouchedProducts.Should().Contain(Product); + _s3.Exists(PublicBucket, ChangelogKeys.NotesIndexKey(Org, Repo, Product, Version)).Should().BeTrue(); + outcome.EmptyProductIndexes.Should().Contain(new NoteAmendEmptyIndex(Product, ProductIndexKey())); _s3.Exists(PublicBucket, "bundle/kibana/kibana-9.3.0.amend-notes.yaml").Should().BeFalse(); } @@ -498,9 +501,10 @@ public async Task EceNote_DoesNotWriteOrDeleteHostedAmendNotes() _s3.Seed(PublicBucket, NoteKey("main", "note-ece.yml"), eceNoteYaml); var notesByProduct = NotesByProduct(ece, version, "main/note-ece.yml"); - var touched = await _reconciler.ReconcileAsync(NotesScope(), notesByProduct, TestContext.Current.CancellationToken); + var outcome = await _reconciler.ReconcileAsync(NotesScope(), notesByProduct, TestContext.Current.CancellationToken); - touched.Should().Equal(ece); + outcome.TouchedProducts.Should().Equal(ece); + outcome.EmptyProductIndexes.Should().BeEmpty(); _s3.Exists(PublicBucket, $"bundle/{ece}/cloud-4.2.0.amend-notes.yaml").Should().BeTrue(); _s3.ContentOf(PublicBucket, $"bundle/{hosted}/{hostedSidecar}").Should().Be(hostedSidecarYaml); _s3.Deletes.Should().NotContain(d => d.Key.Contains($"bundle/{hosted}/", StringComparison.Ordinal)); diff --git a/tests/Elastic.Changelog.Tests/Scrubbing/ScrubberProcessorTests.cs b/tests/Elastic.Changelog.Tests/Scrubbing/ScrubberProcessorTests.cs index 915c3d9319..f6e8ecf4da 100644 --- a/tests/Elastic.Changelog.Tests/Scrubbing/ScrubberProcessorTests.cs +++ b/tests/Elastic.Changelog.Tests/Scrubbing/ScrubberProcessorTests.cs @@ -2,7 +2,9 @@ // Elasticsearch B.V licenses this file to you under the Apache 2.0 License. // See the LICENSE file in the project root for more information +using System.Net; using System.Text.Json; +using Amazon.S3; using AwesomeAssertions; using Elastic.Changelog.Reconciliation; using Elastic.Changelog.Scrubbing; @@ -569,6 +571,52 @@ public async Task Process_NoteFile_ListsAmendNotesOnThatProductOnly() _metrics.GroupReconciles.Should().Be(1); } + [Fact] + public async Task Process_LastNoteRemoved_RegistryWriteFailure_LeavesProductScopedIndex_ThenRetryDeletes() + { + _ = A.CallTo(() => _scrubber.ScrubAsync(A._, A._, A._)).ReturnsLazily( + (string _, string content, Cancel _) => Task.FromResult(new ScrubResult { Content = content }) + ); + + const string product = "elasticsearch"; + const string version = "9.0.0"; + const string parent = "elasticsearch-9.0.0.yaml"; + const string sidecar = "elasticsearch-9.0.0.amend-notes.yaml"; + const string noteKey = "changelog/elastic/elasticsearch/main/note-late.yml"; + var productIndex = ChangelogKeys.NotesIndexKey("elastic", "elasticsearch", product, version); + var registryKey = $"bundle/{product}/registry.json"; + + _s3.Seed(PublicBucket, $"bundle/{product}/{parent}", ProductParentBundle(product, version, "main/pr-100.yaml")); + _s3.Seed(PublicBucket, registryKey, ProductRegistryJson(product, version, parent, sidecar)); + _s3.Seed(PublicBucket, $"bundle/{product}/{sidecar}", ProductParentBundle(product, version, "main/note-late.yml")); + _s3.Seed( + PublicBucket, + productIndex, + /*lang=json,strict*/ + """{"schema_version":1,"product":"elasticsearch","version":"9.0.0","notes":[{"path":"main/note-late.yml","bundle_seq":2}]}""" + ); + _s3.Seed( + PublicBucket, + "changelog/elastic/elasticsearch/notes-9.0.0.json", + /*lang=json,strict*/ + """{"schema_version":1,"notes":[{"path":"main/note-late.yml","bundle_seq":2}]}""" + ); + + _s3.PutFault = + key => key == registryKey ? new AmazonS3Exception("unavailable") { StatusCode = HttpStatusCode.InternalServerError } : null; + + var failed = await _processor.ProcessAsync([Message("ObjectRemoved:Delete", noteKey)], Ctx); + failed.Should().NotBeEmpty(); + _s3.Exists(PublicBucket, $"bundle/{product}/{sidecar}").Should().BeFalse(); + _s3.Exists(PublicBucket, productIndex).Should().BeTrue(); + + _s3.PutFault = null; + var failedRetry = await _processor.ProcessAsync([Message("ObjectRemoved:Delete", noteKey)], Ctx); + failedRetry.Should().BeEmpty(); + _s3.Exists(PublicBucket, productIndex).Should().BeFalse(); + PublicManifest(registryKey).Bundles.Select(b => b.File).Should().Equal(parent); + } + private static string ProductRegistryJson(string product, string version, params string[] files) { var bundles = files.Select(f => new ChangelogRegistryBundle { File = f, Target = version }).ToList();