diff --git a/src/MeshWeaver.Documentation/Data/Architecture/AccessControl.md b/src/MeshWeaver.Documentation/Data/Architecture/AccessControl.md index a87504326b..e983dba648 100644 --- a/src/MeshWeaver.Documentation/Data/Architecture/AccessControl.md +++ b/src/MeshWeaver.Documentation/Data/Architecture/AccessControl.md @@ -646,8 +646,9 @@ reports `Unavailable` rather than vanishing. gate — ends its chain in `TakeDecisionOutsideGate().Timeout(budget).Catch(…)`, which covers a value and a fault. `Take(1)` on an empty fold completes empty; `Timeout` forwards that completion unchanged (it bounds silence *before* a terminal, not an empty one); the `Catch` never fires; and the validator emits -**no** `NodeValidationResult`. `RunCreationValidatorsObs`'s `Concat` skips a validator that yields -nothing, so "no verdict" read as "nothing objected" — measured: a caller holding **no grant at all** +**no** `NodeValidationResult`. `RunCreationValidatorsObs`'s `Concat` skipped a validator that yielded +nothing (since #6391 the runner itself refuses such a validator, for every validator — see +[Write Verdict Totality](../WriteVerdictTotality)), so "no verdict" read as "nothing objected" — measured: a caller holding **no grant at all** created a node under a silent evaluator and got back `State = Active, CreatedBy = `. It now ends in `.DefaultIfEmpty(UnestablishedCheck(…, cause: null))`, whose message names the silent case apart from the stalled and the faulted one, because the three have different causes. diff --git a/src/MeshWeaver.Documentation/Data/Architecture/WriteVerdictTotality.md b/src/MeshWeaver.Documentation/Data/Architecture/WriteVerdictTotality.md index 71cf209bed..0e641291b2 100644 --- a/src/MeshWeaver.Documentation/Data/Architecture/WriteVerdictTotality.md +++ b/src/MeshWeaver.Documentation/Data/Architecture/WriteVerdictTotality.md @@ -731,6 +731,37 @@ stall on a named validator is that validator's read. `LifecycleRequestsAlwaysRea pins it: the silent validator's type name is in the verdict, and the assertion fails on the code before the change, which said only `validators`. +**Each validator gives EXACTLY ONE verdict, and the runner enforces it** (MeshWeaver#6391). The +`INodeValidator` contract is "emits exactly one `NodeValidationResult` and completes". +`RunCreationValidatorsObs` used to trust that, and two ways of breaking it went unnoticed: + +- **A validator that answered and stayed subscribed blocked the rest.** `Concat` subscribes to + validator *i+1* only when validator *i* COMPLETES. So a validator that answered `Valid` from a + live read (a hot fold that never completes) held every validator after it. As the last one it held + the create itself, until the create's deadline answered it as a stall at + `validators: `, although that validator had already answered. Now each validator's + first emission is its verdict (`Take(1)` per validator). +- **A validator that completed WITHOUT a verdict counted as a pass.** `Concat` skipped it, and the + create went ahead as though it had been approved. This is the #2742 fail-open, which + [Access Control](../AccessControl) closed inside `RlsNodeValidator` alone. It is now closed for + every validator: an empty completion refuses the create as `Unavailable` + (`activity.node.create.validatorGaveNoVerdict`). The refusal names the path and the validator, and + nothing is written. Unavailable, not `ValidationFailed`, because nothing objected to the request: + the check was never established. + +A validator that **never answers at all** is deliberately **not** given a bound of its own. The +create's deadline already answers it and names it (`validators: (i/n)`). A per-validator +timeout would only turn a stall into a refusal one rung earlier, and would leave the actual defect +in place: why that validator's read does not answer. The production stall that #6391 reports is +still open on that question. + +Pinned by `CreationValidatorGivesExactlyOneVerdictTest` (MeshWeaver.Hosting.Test). One test arms a +validator that answers `Valid` and then never completes, followed by one that refuses: the refusal +arrives, and without the refusing validator the create commits. Another arms a validator that +completes empty: the create is refused `Unavailable`, the validator is named, and nothing is stored. +An unarmed control covers both. On the code before the change, the first two were answered only at +the deadline, as a stall, and the empty validator's node was CREATED. + **The deadline must release the hub after a prompt create.** Rx's absolute `Timeout` schedules a `LocalScheduler.WorkItem` that can remain in its priority queue until the deadline even after the subscription completes and disposes. Its fallback observable therefore captures only a small diff --git a/src/MeshWeaver.Mesh.Contract/MeshExtensions.cs b/src/MeshWeaver.Mesh.Contract/MeshExtensions.cs index b5b0928005..362a1e8677 100644 --- a/src/MeshWeaver.Mesh.Contract/MeshExtensions.cs +++ b/src/MeshWeaver.Mesh.Contract/MeshExtensions.cs @@ -781,6 +781,12 @@ private static void AnswerAHeartbeatForAStreamThisOwnerDoesNotServe( /// private const string CreateStalledKey = "activity.node.create.stalled"; + /// + /// The refusal a create gives when one of its creation validators COMPLETED WITHOUT A VERDICT + /// (#6391) — no verdict is not consent, so the create is refused Unavailable. + /// + private const string ValidatorGaveNoVerdictKey = "activity.node.create.validatorGaveNoVerdict"; + /// /// The verdict a create gives when its row is written but a post-creation handler has not /// finished by the deadline — written, outcome unknown, nothing cancelled or rolled back. @@ -6518,18 +6524,44 @@ internal static bool GovernedActivityExecuting( // Each validator is named at the moment Concat SUBSCRIBES to it (Defer), i.e. when it is // the one the chain is waiting on — so a stalled verdict reads // "validators: RlsNodeValidator (i/n)", never a bare stage name (#6391). + // + // 🚨 EXACTLY ONE VERDICT PER VALIDATOR (#6391). The INodeValidator contract is "emits + // exactly one NodeValidationResult and completes"; the runner holds every validator to it + // instead of trusting it: + // • Take(1) — the FIRST emission is that validator's verdict. Concat subscribes to + // validator i+1 only when validator i COMPLETES, so a validator that answered from a + // live read and stayed subscribed (a hot fold that never completes) held every validator + // after it, and as the last one the whole create, until the create's own deadline. + // • An EMPTY completion is a refusal — a validator that completes without emitting gave + // no verdict, and "no verdict" is not consent (the #2742 rule, generalised from RLS to + // every validator). Concat used to skip it, and the create proceeded as if it had passed. + // A validator that never answers at all is deliberately NOT bounded here: the create's own + // budget answers it and names it (the stage above), and why it does not answer is that + // validator's defect to find — a per-validator timeout would only hide it. return validators - .Select((v, i) => onStage is null - ? v.Validate(context) - : Observable.Defer(() => + .Select((v, i) => Observable.Defer(() => { - onStage($"{v.GetType().Name} ({i + 1}/{validators.Count})"); + onStage?.Invoke($"{v.GetType().Name} ({i + 1}/{validators.Count})"); return v.Validate(context); - })) + }) + .Take(1) + .Select(result => (Result: (NodeValidationResult?)result, Validator: v.GetType().Name)) + .DefaultIfEmpty((Result: null, Validator: v.GetType().Name))) .Concat() - .Where(result => !result.IsValid) - .Select(result => + .Where(verdict => verdict.Result is not { IsValid: true }) + .Select(verdict => { + if (verdict.Result is not { } result) + return ((LocalizableText?, NodeCreationRejectionReason)?)( + LocalizableText.Keyed( + $"Creating '{node.Path}' was refused: creation validator {verdict.Validator} " + + "completed without a verdict. No verdict is not consent, so nothing was " + + "written; a validator must emit exactly one result.", + ValidatorGaveNoVerdictKey, + ("path", node.Path), ("validator", verdict.Validator)), + // Unavailable, not ValidationFailed: nothing objected to the request — the + // check was never established (#1446). Still fail-closed: nothing is written. + NodeCreationRejectionReason.Unavailable); var reason = result.Reason switch { NodeRejectionReason.NodeAlreadyExists => NodeCreationRejectionReason.NodeAlreadyExists, diff --git a/src/MeshWeaver.Messaging.Hub/Localization/strings.de.json b/src/MeshWeaver.Messaging.Hub/Localization/strings.de.json index f6302b1c8f..54360388d8 100644 --- a/src/MeshWeaver.Messaging.Hub/Localization/strings.de.json +++ b/src/MeshWeaver.Messaging.Hub/Localization/strings.de.json @@ -1537,6 +1537,7 @@ "activity.node.move.deleteStalled": "Das Verschieben von '{source}' nach '{target}' kam beim Entfernen der Quelle innerhalb von {seconds}s zu keinem Ergebnis. Die Kopie unter '{target}' war bereits angelegt; lesen Sie beide Pfade, bevor Sie es erneut versuchen.", "activity.node.create.postCreationOutcomeUnknown": "'{path}' wurde geschrieben, aber ein Schritt nach dem Anlegen war innerhalb von {seconds}s nicht abgeschlossen; das Ergebnis ist unbekannt — lesen Sie den Node, bevor Sie es erneut versuchen.", "activity.node.create.stalled": "Das Anlegen von '{path}' kam innerhalb von {seconds}s zu keinem Ergebnis: es wartete noch im Schritt '{stage}'. Nichts hat die Anfrage abgelehnt; ob die Arbeit dieses Schritts später noch ankommt, ist unbekannt — lesen Sie das Ziel, bevor Sie es erneut versuchen.", + "activity.node.create.validatorGaveNoVerdict": "Das Anlegen von '{path}' wurde abgelehnt: der Erstellungs-Validator {validator} wurde ohne Ergebnis beendet. Kein Ergebnis ist keine Zustimmung, daher wurde nichts geschrieben; ein Validator muss genau ein Ergebnis liefern.", "activity.node.create.unexpectedError": "Unerwarteter Fehler: {error}", "activity.node.create.chainTerminatedWithoutResult": "'{path}' konnte nicht angelegt werden: die Create-Pipeline wurde beendet, ohne einen Node zu erzeugen und ohne einen Grund zu melden. Das ist ein Defekt in der Create-Kette, keine Ablehnung der Anfrage — ein erneuter Versuch hilft voraussichtlich erst, wenn er behoben ist.", "activity.node.create.ownerDisposing": "die Aktivierung '{hub}', die dieses Anlegen bearbeitet hat, wurde verworfen, bevor die Create-Kette zu einem Ergebnis kam — das Anlegen wurde von dieser Aktivierung NICHT abgeschlossen; senden Sie es erneut gegen die frische Aktivierung (eine Zeile, die doch geschrieben wurde, antwortet mit NodeAlreadyExists). 🚨 Der Hub wird im Satz GENANNT, nicht nur in den strukturierten Daten: der Aufrufer läuft meist in einem anderen Prozess und sieht nichts vom Log dieses Hubs; ein ungenannter Teardown lässt das nächste Auftreten also ausgerechnet von der einzigen Seite aus unzuordenbar, die es sehen kann.", diff --git a/src/MeshWeaver.Messaging.Hub/Localization/strings.en.json b/src/MeshWeaver.Messaging.Hub/Localization/strings.en.json index bf9d9aea4c..91e8c40adc 100644 --- a/src/MeshWeaver.Messaging.Hub/Localization/strings.en.json +++ b/src/MeshWeaver.Messaging.Hub/Localization/strings.en.json @@ -1537,6 +1537,7 @@ "activity.node.move.deleteStalled": "Moving '{source}' to '{target}' reached no verdict within {seconds}s while removing the source. The copy at '{target}' had already landed; read both paths before retrying.", "activity.node.create.postCreationOutcomeUnknown": "'{path}' was written, but a post-creation step had not finished within {seconds}s; the outcome is unknown — read the node before retrying.", "activity.node.create.stalled": "Creating '{path}' reached no verdict within {seconds}s: it was still waiting at stage '{stage}'. Nothing refused the request; whether that stage's own work lands later is unknown, so read the target before retrying.", + "activity.node.create.validatorGaveNoVerdict": "Creating '{path}' was refused: creation validator {validator} completed without a verdict. No verdict is not consent, so nothing was written; a validator must emit exactly one result.", "activity.node.create.unexpectedError": "Unexpected error: {error}", "activity.node.create.chainTerminatedWithoutResult": "Could not create '{path}': the create pipeline terminated without producing a node and without reporting a reason. This is a defect in the create chain, not a rejection of the request — retrying is unlikely to help until it is fixed.", "activity.node.create.ownerDisposing": "the activation '{hub}' handling this create was disposed before the create chain reached a verdict — the create was NOT completed by that activation; re-drive it against the fresh one (a row that did land answers NodeAlreadyExists). 🚨 The hub is NAMED in the sentence, not only in the structured payload: the requester is usually in another process and sees none of this hub's log, so an unnamed teardown leaves the next occurrence unattributable from the only side that can see it.", diff --git a/test/MeshWeaver.Hosting.Test/CreationValidatorGivesExactlyOneVerdictTest.cs b/test/MeshWeaver.Hosting.Test/CreationValidatorGivesExactlyOneVerdictTest.cs new file mode 100644 index 0000000000..75b3165583 --- /dev/null +++ b/test/MeshWeaver.Hosting.Test/CreationValidatorGivesExactlyOneVerdictTest.cs @@ -0,0 +1,219 @@ +using System; +using System.Collections.Concurrent; +using System.Collections.Generic; +using System.Linq; +using System.Reactive.Linq; +using System.Threading; +using System.Threading.Tasks; +using MeshWeaver.Fixture; +using MeshWeaver.Hosting.Monolith.TestBase; +using MeshWeaver.Mesh; +using MeshWeaver.Mesh.Security; +using MeshWeaver.Mesh.Services; +using Microsoft.Extensions.DependencyInjection; +using Xunit; + +namespace MeshWeaver.Hosting.Test; + +/// +/// 🚨 The creation-validator runner takes EXACTLY ONE verdict from each validator (#6391) — the +/// contract is "emits exactly one result and completes", and the runner +/// holds every validator to it rather than trusting it. +/// +/// (a) The first emission IS the verdict. The runner chains validators with +/// Concat, which subscribes to the next validator only when the current one COMPLETES. A +/// validator that answered Valid from a live read and stayed subscribed therefore held every +/// validator after it — and the create itself — until the create's own deadline answered it as a +/// stall. Repro: a validator that emits Valid and then never completes, followed by a +/// validator that refuses. Pre-fix the create is answered only at the budget, as a stall; post-fix +/// the later validator's refusal arrives at once, and with no refusing validator the create +/// commits. +/// +/// (b) No verdict is not consent. A validator that COMPLETES WITHOUT EMITTING was +/// skipped by Concat, so the create proceeded as if it had passed — the #2742 fail-open, +/// generalised from RLS to every validator. Repro: a validator that returns +/// Observable.Empty. Pre-fix the node is CREATED; post-fix the create is refused +/// Unavailable, naming the validator, and nothing is stored. +/// +/// Nothing is raced and nothing is slept: the budget () is far below the +/// hub's request timeout, the assertions are on the verdict's CONTENT, and each repro has a negative +/// control on the same mesh (the same validators, unarmed for that path). +/// +public class CreationValidatorGivesExactlyOneVerdictTest(ITestOutputHelper output) : MonolithMeshTestBase(output) +{ + /// Rung 1 of the ladder for this mesh — the only configured value. + private static readonly TimeSpan Budget = TimeSpan.FromSeconds(6); + + /// How long past the budget a verdict may take to travel back: one hop, generous. + private static readonly TimeSpan Window = Budget + TimeSpan.FromSeconds(8); + + private readonly MisbehavingCreationValidator misbehaving = new(); + private readonly RefusingCreationValidator refusing = new(); + + private IStorageAdapter Storage => Mesh.ServiceProvider.GetRequiredService(); + + /// + /// Registration order is chain order: the misbehaving validator runs BEFORE the refusing one. + protected override MeshBuilder ConfigureMesh(MeshBuilder builder) + => base.ConfigureMesh(builder).ConfigureServices(services => services + .AddSingleton(misbehaving) + .AddSingleton(refusing) + .AddSingleton(new MeshOperationOptions { Timeout = Budget })); + + /// (a) A validator that answers and stays subscribed no longer blocks the validator after it. + [Fact(Timeout = 60000)] + public async Task AValidatorThatAnswersAndStaysSubscribed_DoesNotBlockTheNextValidator() + { + var ct = TestContext.Current.CancellationToken; + var path = NewPath("answers-then-stays-refused"); + misbehaving.AnswerThenStay(path); + refusing.Refuse(path); + + var response = await Create(path, ct); + + response.Success.Should().BeFalse(); + response.RejectionReason.Should().Be(NodeCreationRejectionReason.ValidationFailed, + "the validator AFTER the one that stayed subscribed must be asked and its refusal must " + + "arrive — pre-fix Concat never reached it and the create was answered as a stall"); + response.Error.Should().Contain(RefusingCreationValidator.Message); + refusing.Asked.Should().Contain(path, "the next validator really was asked"); + (await StoredAt(path, ct)).Should().BeNull("a refused create writes nothing"); + } + + /// (a) The same validator, with nothing after it refusing: the create commits rather than stalling. + [Fact(Timeout = 60000)] + public async Task AValidatorThatAnswersValidAndStaysSubscribed_LetsTheCreateCommit() + { + var ct = TestContext.Current.CancellationToken; + var path = NewPath("answers-then-stays"); + misbehaving.AnswerThenStay(path); + + var response = await Create(path, ct); + + response.Success.Should().BeTrue(response.Error + ?? "a Valid first emission is the verdict — staying subscribed must not hold the create"); + misbehaving.Asked.Should().Contain(path, "the armed validator really was asked"); + (await StoredAt(path, ct)).Should().NotBeNull(); + } + + /// (b) A validator that completes without a verdict refuses the create — it is never a pass. + [Fact(Timeout = 60000)] + public async Task AValidatorThatCompletesWithoutAVerdict_RefusesTheCreate_NamingIt() + { + var ct = TestContext.Current.CancellationToken; + var path = NewPath("completes-empty"); + misbehaving.CompleteEmpty(path); + + var response = await Create(path, ct); + + response.Success.Should().BeFalse("no verdict is not consent — pre-fix this node was CREATED"); + response.RejectionReason.Should().Be(NodeCreationRejectionReason.Unavailable, + "nothing objected to the request; the check was never established"); + response.Error.Should().Contain(path); + response.Error.Should().Contain(nameof(MisbehavingCreationValidator), + "the refusal names the validator that gave no verdict — that is where the defect lives"); + misbehaving.Asked.Should().Contain(path, "the armed validator really was asked"); + (await StoredAt(path, ct)).Should().BeNull("a refused create writes nothing"); + } + + /// Negative control: the same validators, armed for OTHER paths, let a create through. + [Fact(Timeout = 60000)] + public async Task ACreateNoValidatorMisbehavesFor_StillSucceeds() + { + var ct = TestContext.Current.CancellationToken; + misbehaving.CompleteEmpty(NewPath("someone-else")); + misbehaving.AnswerThenStay(NewPath("someone-else")); + refusing.Refuse(NewPath("someone-else")); + var path = NewPath("healthy-create"); + + var response = await Create(path, ct); + + response.Success.Should().BeTrue(response.Error ?? "no validator is armed for this path"); + (await StoredAt(path, ct)).Should().NotBeNull(); + } + + private Task Create(string path, CancellationToken ct) => + ObserveNodeOperation(new CreateNodeRequest(Markdown(path))) + .Select(d => d.Message) + .Do(r => Output.WriteLine($"CREATE {path}: success={r.Success} reason={r.RejectionReason} error={r.Error}")) + .Should().Within(Window).Emit("every create is answered inside the operation budget", + cancellationToken: ct); + + private static MeshNode Markdown(string path) => MeshNode.FromPath(path) with + { + Name = path.Split('/')[^1], + NodeType = "Markdown", + State = MeshNodeState.Active, + }; + + private Task StoredAt(string path, CancellationToken ct) => + Storage.Read(path, Mesh.JsonSerializerOptions) + .DefaultIfEmpty(null) + .Should().Within(TestTimeouts.Convergence).Emit(cancellationToken: ct); + + private static string NewPath(string prefix) => + $"{TestPartition}/{prefix}-{Guid.NewGuid().ToString("N")[..8]}"; +} + +/// +/// A creation validator that answers Valid and completes for every path except the ones it +/// has been armed for, where it breaks the one-verdict contract in one of two ways: it answers +/// Valid and then never completes (a hot fold), or it completes without ever emitting. +/// Instance state, never static. +/// +internal sealed class MisbehavingCreationValidator : INodeValidator +{ + private readonly ConcurrentDictionary armed = new(StringComparer.OrdinalIgnoreCase); + private readonly ConcurrentDictionary asked = new(StringComparer.OrdinalIgnoreCase); + + /// At , answer Valid and then stay subscribed forever. + public void AnswerThenStay(string path) => armed[path] = true; + + /// At , complete without emitting a verdict. + public void CompleteEmpty(string path) => armed[path] = false; + + /// Every armed path this validator was actually asked about. + public IReadOnlyCollection Asked => asked.Keys.ToArray(); + + /// + public IReadOnlyCollection SupportedOperations { get; } = [NodeOperation.Create]; + + /// + public IObservable Validate(NodeValidationContext context) + { + if (!armed.TryGetValue(context.Node.Path, out var answerThenStay)) + return Observable.Return(NodeValidationResult.Valid()); + asked.TryAdd(context.Node.Path, 0); + return answerThenStay + ? Observable.Return(NodeValidationResult.Valid()).Concat(Observable.Never()) + : Observable.Empty(); + } +} + +/// A well-behaved creation validator that refuses the paths it has been armed for. +internal sealed class RefusingCreationValidator : INodeValidator +{ + /// The refusal text, so a test can recognise it in the verdict. + public const string Message = "refused by RefusingCreationValidator"; + + private readonly ConcurrentDictionary refused = new(StringComparer.OrdinalIgnoreCase); + private readonly ConcurrentDictionary asked = new(StringComparer.OrdinalIgnoreCase); + + /// Refuse creates at . + public void Refuse(string path) => refused.TryAdd(path, 0); + + /// Every armed path this validator was actually asked about. + public IReadOnlyCollection Asked => asked.Keys.ToArray(); + + /// + public IReadOnlyCollection SupportedOperations { get; } = [NodeOperation.Create]; + + /// + public IObservable Validate(NodeValidationContext context) + { + if (!refused.ContainsKey(context.Node.Path)) + return Observable.Return(NodeValidationResult.Valid()); + asked.TryAdd(context.Node.Path, 0); + return Observable.Return(NodeValidationResult.Invalid(Message)); + } +}