Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -359,6 +359,16 @@ stays with the package's own policy (the reconciler applies `Auto`; a human's cl
rest — both restore absent declared nodes as they go, #4259); a package no source lists any more
cannot be re-fetched and is named as an orphan for the admin list.

**…and only from the source the record names, by provenance** (MeshWeaver#6512). Several sources can
list the same package id, and a directly read repository can AUTHOR any `source` in its
`package.json`. So where the install record names a source, the repair takes only a listing that is
*trusted* to be that source — a registry's stamp, or a source whose configured name is the recorded
one — never a listing that merely claims the name. When no such listing serves the package, it is
not re-asserted, and the line says so by name. Only a legacy record that names no source takes the
first listing. Pinned by `ARepairBindsToATrustedSourceTest`: a repository configured as another
source, listed first and authoring the recorded name, landed its own files over the installed
package before the change, and the recorded source's files come back after it.

The sweep's own line follows the same predicate: an `Incomplete` it is about to hand over is
logged at **Warning** (a detection followed by a repair — the shape the install gate already uses),
and one nothing here will repair stays at **Error** with the reason on the line. The Error for a
Expand Down
40 changes: 34 additions & 6 deletions src/MeshWeaver.PluginCatalog/InstanceAutoRegistrationService.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1952,6 +1952,20 @@ candidate.Source.Source is RegistryPackageSource

/// <summary>Whether two source names are the same non-empty source (case-insensitive, like
/// <see cref="PluginGrantEntry.Matches"/>). Pure.</summary>
/// <summary>
/// The listing the repair lane re-asserts an installed package from (#6512), or null when the
/// record names a source and no listing is TRUSTED to be that source. Trusted means a registry's
/// stamp, or the candidate's source being the name its source was configured under
/// (<see cref="ProvenanceIsTrusted"/>); a manifest that merely authors the recorded name is not
/// enough. A legacy record with no source keeps the first listing. Pure.
/// </summary>
/// <param name="listings">Every listing of one package id, in source order.</param>
/// <param name="recordedSource">The source the install record was stamped with, or null.</param>
private static InstallCandidate? RepairCandidate(IReadOnlyList<InstallCandidate> listings, string? recordedSource) =>
string.IsNullOrWhiteSpace(recordedSource)
? listings.FirstOrDefault()
: listings.FirstOrDefault(c => ProvenanceIsTrusted(c) && SameSource(c.Package.Source, recordedSource));

private static bool SameSource(string? a, string? b) =>
!string.IsNullOrWhiteSpace(a) && string.Equals(a.Trim(), b?.Trim(), StringComparison.OrdinalIgnoreCase);
Comment on lines 1953 to 1970

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.

Confirmed: the one-line SameSource summary (lines 1953-1954) now sits directly above RepairCandidate's own summary, so RepairCandidate carries two <summary> elements and SameSource carries none. It is a misplaced doc comment only: no behaviour, no compile warning (the CI build is -warnaserror and green on this head), and RepairCandidate's own summary, which states its actual contract, is intact.

Under this round's converge rule a comment-only correction does not earn a push (every push restarts the review and the dependent-suites run on a congested queue), so I am leaving the head as it is. The two lines move back above SameSource the next time this file is edited.


Expand Down Expand Up @@ -2581,22 +2595,36 @@ internal IObservable<DefaultInstallSummary> ReassertInstalled(IReadOnlyList<Reas
{
var listingIncomplete = answers.Any(a => a.Failed);
var sourceUnreachable = answers.Any(a => a.Failed && !a.Held);
// Bound by (source, id), not id alone: where several sources list the same id,
// the one the install record was stamped with is preferred, so a repair never
// re-asserts another source's files over the package that is installed.
// Bound by (source, id), not id alone, and by PROVENANCE, not by what a manifest
// says (#6512): a record that names its source is re-asserted only from a listing
// whose source is trusted to be that one — a registry's stamp, or the name the
// source was configured under. A directly read repository can AUTHOR any
// `source` in its package.json, so "the candidate says it is from X" would let
// another repository listing the same id land its files over the installed
// package. Only a legacy record that names no source takes the first listing.
var catalog = answers
.SelectMany(a => a.Candidates)
.GroupBy(c => c.Package.Id, StringComparer.Ordinal)
.ToImmutableDictionary(
g => g.Key,
g => g.FirstOrDefault(c => SameSource(c.Package.Source, recordedSources.GetValueOrDefault(g.Key)))
?? g.First(),
g => RepairCandidate(g.ToList(), recordedSources.GetValueOrDefault(g.Key)),
StringComparer.Ordinal);

var atRecordedHash = ImmutableList.CreateBuilder<InstallCandidate>();
foreach (var (id, recorded) in recordedModuleVersions.OrderBy(kv => kv.Key, StringComparer.Ordinal))
{
if (!catalog.TryGetValue(id, out var candidate))
if (catalog.TryGetValue(id, out var bound) && bound is null)
{
logger.LogWarning(
"[PackageRepair] {Id} is NOT re-asserted: its record names source "
+ "'{Source}', and no listing whose provenance is trusted to be that "
+ "source serves it — only listings that merely CLAIM it, or other "
+ "sources. A repair never lands another repository's files over the "
+ "installed package (MeshWeaver#6512).",
id, recordedSources.GetValueOrDefault(id));
continue;
}
if (bound is not { } candidate)
{
// Say WHICH of two things this is: the listing was partial (a held or
// failed source may well be the one that serves it), or every source
Expand Down
105 changes: 105 additions & 0 deletions test/Memex.Portal.Shared.Test/ARepairBindsToATrustedSourceTest.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,105 @@
using System;
using System.Collections.Generic;
using System.Linq;
using System.Reactive.Linq;
using System.Text.Json;
using System.Threading.Tasks;
using MeshWeaver.Fixture;
using MeshWeaver.Hosting.Monolith.TestBase;
using MeshWeaver.Mesh;
using MeshWeaver.Mesh.Services;
using MeshWeaver.PluginCatalog;
using Microsoft.Extensions.DependencyInjection;
using Microsoft.Extensions.Logging;
using Xunit;

namespace Memex.Portal.Shared.Test;

/// <summary>
/// 🚨 <b>The repair lane re-asserts an installed package only from a source TRUSTED to be the one
/// its record names — never from a repository that merely AUTHORS that name</b> (MeshWeaver#6512).
///
/// <para>Two directly read sources list the same id. The honest one (configured as
/// <c>registered-1</c>) served the install, so the record names <c>registered-1</c>. The impostor
/// (configured as <c>registered-0</c>, listed FIRST) authors <c>"source": "registered-1"</c> in its
/// manifest and serves different files at the same module version. Before #6512 the lane preferred
/// the first listing whose <c>Source</c> equals the record's — the impostor — and its files landed
/// over the installed package.</para>
///
/// <para><b>SHOULD-FAIL-IF</b> the lane trusts an authored source again: the lost node comes back
/// with the impostor's body.</para>
/// </summary>
public class ARepairBindsToATrustedSourceTest(ITestOutputHelper output) : MonolithMeshTestBase(output)
{
private const string HonestSource = "registered-1";
private const string ImpostorMarker = "IMPOSTOR body";

private readonly ImpostorSource impostor = new();

/// <summary>A direct source whose manifest AUTHORS the honest source's name — what a package.json
/// may carry — and serves its own files. Lists nothing until the install has landed, so the
/// install itself is unambiguous and only the repair meets the impostor.</summary>
private sealed class ImpostorSource : IPackageSource
{
private volatile bool listing;

public void StartListing() => listing = true;

public IObservable<IReadOnlyList<PackageManifest>> ListPackages(string gitRef) =>
Observable.Return<IReadOnlyList<PackageManifest>>(listing
? [ReassertFixture.Candidate() with { Source = HonestSource }]
: []);

public IObservable<IReadOnlyList<PackageFile>> FetchPackageFiles(PackageManifest package, string gitRef) =>
Observable.Return<IReadOnlyList<PackageFile>>(ReassertFixture.Files()
.Select(f => f.RelativePath.EndsWith("/Guide.md", StringComparison.Ordinal)
? new PackageFile(f.RelativePath, $"# Guide\n\n{ImpostorMarker}.")
: f)
.ToList());
}

protected override MeshBuilder ConfigureMesh(MeshBuilder builder)
=> base.ConfigureMesh(builder)
.AddPluginCatalog()
.ConfigureServices(services => services
// Order is the configured name: the impostor is registered-0 and listed FIRST.
.AddSingleton<IPackageSource>(_ => impostor)
.AddSingleton<IPackageSource>(_ => new ReassertFixture.Source(ReassertFixture.Files()))
.AddSingleton(new PluginCatalogOptions { InstallPreInstalledPackages = false }));

[Fact(Timeout = 300_000)]
public async Task ARecordNamingItsSource_IsRepairedFromThatSource_NotFromOneThatAuthorsItsName()
{
var ct = TestContext.Current.CancellationToken;
var logger = Mesh.ServiceProvider.GetRequiredService<ILoggerFactory>()
.CreateLogger<ARepairBindsToATrustedSourceTest>();
await Mesh.ServiceProvider.GetRequiredService<InstanceAutoRegistrationService>().Completed
.FirstAsync().Timeout(180.Seconds()).Await(ct);

// Installed from the honest source, the record stamped with its configured name.
await CatalogLayoutAreas
.InstallOrUpdate(Mesh, new ReassertFixture.Source(ReassertFixture.Files()), "HEAD",
ReassertFixture.Candidate() with { Source = HonestSource }, logger)
.Should().Within(180.Seconds()).Emit("the install must land first", cancellationToken: ct);
(await ReassertFixture.WaitForNode(Mesh, ReassertFixture.GuidePath, present: true)).Should().BeTrue();
(await ReassertFixture.ReadRecord(Mesh))!.Source.Should().Be(HonestSource,
"CONTROL — the record names the source the repair must bind to");

impostor.StartListing();
await Mesh.ServiceProvider.GetRequiredService<IMeshService>().DeleteNode(ReassertFixture.GuidePath)
.Should().Within(TestTimeouts.Convergence).Emit("the loss is the precondition", cancellationToken: ct);
(await ReassertFixture.WaitForNode(Mesh, ReassertFixture.GuidePath, present: false)).Should().BeTrue();

await Mesh.ServiceProvider.GetRequiredService<InstalledPackageRepairService>()
.RunRepair().Timeout(240.Seconds()).Await(ct);

(await ReassertFixture.WaitForNode(Mesh, ReassertFixture.GuidePath, present: true)).Should().BeTrue(
"the honest source still lists the package, so the repair re-asserts it");
var guide = await ReassertFixture.Read(Mesh, ReassertFixture.GuidePath);
var body = JsonSerializer.Serialize(guide!.Content, Mesh.JsonSerializerOptions);
body.Should().NotContain(ImpostorMarker,
"a repair must never land the files of a repository that merely AUTHORS the recorded source");
body.Should().Contain("The node the tests lose",
"the node comes back from the source the record names");
}
}