From e336085816c1dcfe2f7b608c27a0eca3a15798dc Mon Sep 17 00:00:00 2001 From: Sipke Schoorstra Date: Fri, 9 Oct 2026 00:52:53 +0200 Subject: [PATCH 1/3] docs(speckit): plan Nuplane observer diagnostics --- .specify/feature.json | 2 +- AGENTS.md | 3 +- .../checklists/requirements.md | 35 ++++++++++ .../contracts/observer-diagnostic-contract.md | 25 +++++++ .../data-model.md | 12 ++++ specs/024-nuplane-failure-diagnostics/plan.md | 68 +++++++++++++++++++ .../quickstart.md | 29 ++++++++ .../research.md | 34 ++++++++++ specs/024-nuplane-failure-diagnostics/spec.md | 66 ++++++++++++++++++ .../024-nuplane-failure-diagnostics/tasks.md | 58 ++++++++++++++++ 10 files changed, 330 insertions(+), 2 deletions(-) create mode 100644 specs/024-nuplane-failure-diagnostics/checklists/requirements.md create mode 100644 specs/024-nuplane-failure-diagnostics/contracts/observer-diagnostic-contract.md create mode 100644 specs/024-nuplane-failure-diagnostics/data-model.md create mode 100644 specs/024-nuplane-failure-diagnostics/plan.md create mode 100644 specs/024-nuplane-failure-diagnostics/quickstart.md create mode 100644 specs/024-nuplane-failure-diagnostics/research.md create mode 100644 specs/024-nuplane-failure-diagnostics/spec.md create mode 100644 specs/024-nuplane-failure-diagnostics/tasks.md diff --git a/.specify/feature.json b/.specify/feature.json index 4baf4f0..af63770 100644 --- a/.specify/feature.json +++ b/.specify/feature.json @@ -1,3 +1,3 @@ { - "feature_directory": "specs/023-live-nuplane-options" + "feature_directory": "specs/024-nuplane-failure-diagnostics" } diff --git a/AGENTS.md b/AGENTS.md index 4b6a0c8..c24e2ee 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -162,10 +162,11 @@ When asked to review or address PR review comments, think critically about each - C# 14 / .NET 10; source projects multi-target `net8.0;net9.0;net10.0` per repository conventions + Existing `Microsoft.Extensions.Configuration`, `Microsoft.Extensions.DependencyInjection`, `System.Text.Json`; no new third-party packages (014-polymorphic-feature-config) - N/A; configuration provider inputs only (014-polymorphic-feature-config) - C# 14; source packages multi-target `net8.0;net9.0;net10.0` + Existing `Microsoft.Extensions.DependencyInjection`, `Microsoft.Extensions.Logging`, and `System.Collections.Immutable`; no new package dependencies (018-shell-generation-build-leases) +- C# 14 / .NET 10 SDK; source library targets `net8.0;net9.0;net10.0` + Existing Nuplane abstractions, Microsoft.Extensions.Options, and a direct `Microsoft.Extensions.Logging.Abstractions` dependency; test-only reference to Nuplane's real observer-dispatcher package (024-nuplane-failure-diagnostics) ## Recent Changes - 011-map-shell-config: Planned map-based shell configuration under `CShells:Shells` -For the active work unit, read `specs/023-live-nuplane-options/plan.md`. +For the active work unit, read `specs/024-nuplane-failure-diagnostics/plan.md`. diff --git a/specs/024-nuplane-failure-diagnostics/checklists/requirements.md b/specs/024-nuplane-failure-diagnostics/checklists/requirements.md new file mode 100644 index 0000000..3fd8261 --- /dev/null +++ b/specs/024-nuplane-failure-diagnostics/checklists/requirements.md @@ -0,0 +1,35 @@ +# Specification Quality Checklist: Nuplane Observer Failure Diagnostics + +**Purpose**: Validate specification completeness and quality before planning +**Created**: 2026-10-09 +**Feature**: [spec.md](../spec.md) + +## Content Quality + +- [x] No implementation details beyond observable logging and propagation behavior +- [x] Focused on host-maintainer value and operational diagnosis +- [x] User scenarios are written for package-integration maintainers +- [x] All mandatory sections are completed + +## Requirement Completeness + +- [x] No `[NEEDS CLARIFICATION]` markers remain +- [x] Requirements are testable and unambiguous +- [x] Success criteria are measurable +- [x] Success criteria describe observable outcomes +- [x] Acceptance scenarios are defined for the ordinary failure and exclusion paths +- [x] Edge cases cover eligibility, cancellation, fatal exceptions, and existing result semantics +- [x] Scope is bounded to Nuplane observer diagnostics +- [x] Dependencies and assumptions are identified + +## Feature Readiness + +- [x] Functional requirements have observable acceptance criteria +- [x] The primary user journey is covered by the P1 story +- [x] Success criteria map to injected failure and observer-dispatch evidence +- [x] No unsupported feature or release claim is made + +## Notes + +- The accepted issue body resolves the implementation and failure-classification choices; no owner clarification is needed. +- Build-time freshness and returned per-shell reload results are explicitly excluded from the new exception diagnostics. diff --git a/specs/024-nuplane-failure-diagnostics/contracts/observer-diagnostic-contract.md b/specs/024-nuplane-failure-diagnostics/contracts/observer-diagnostic-contract.md new file mode 100644 index 0000000..eec739c --- /dev/null +++ b/specs/024-nuplane-failure-diagnostics/contracts/observer-diagnostic-contract.md @@ -0,0 +1,25 @@ +# Observer Diagnostic Contract + +This document describes an observable behavior of the existing optional adapter. It introduces no public type, method, option, or Nuplane API. + +## Ordinary admitted-operation failure + +For a nonfatal exception thrown by catalog refresh, active-registry access, reload invocation, or the configured reload-results callback during an enabled, eligible reconciliation delivery, the adapter emits exactly one `Error` record before propagating the failure. + +The record carries: + +- the exact original exception object as the logging exception; +- structured `CorrelationId`, copied from `PackageChangeSet.CorrelationId`; +- structured `Operation`, identifying the observer callback as `OnPackagesReconciledAsync`. + +The adapter rethrows that same exception instance. It does not format and substitute the exception, wrap it, swallow it, or clear pending work. Nuplane's actual `ObserverEventDispatcher` remains responsible for its existing Warning and continuation to later observers. + +## Exclusions and unchanged behavior + +- Do not Error-log a thrown `OperationCanceledException` when the callback token is requested. An uncanceled `OperationCanceledException` is an ordinary operation failure. +- Do not Error-log `OutOfMemoryException`, `StackOverflowException`, `AccessViolationException`, `AppDomainUnloadedException`, or `BadImageFormatException`. +- Do not extend the new diagnostic boundary to build-time freshness in `BeginAsync`. +- A returned `ReloadResult.Error` is not a thrown failure and is not newly Error-logged; preserve the existing result callback and pending-reload behavior. +- If the host has no logging provider, construction and operation remain valid through the null logger fallback. + +No particular log message wording or event ID is a public compatibility promise. The exception object, structured correlation, observer-operation name, Error level, one-record count, and exception propagation are the contract. diff --git a/specs/024-nuplane-failure-diagnostics/data-model.md b/specs/024-nuplane-failure-diagnostics/data-model.md new file mode 100644 index 0000000..a4c5a8d --- /dev/null +++ b/specs/024-nuplane-failure-diagnostics/data-model.md @@ -0,0 +1,12 @@ +# Data Model: Nuplane Observer Failure Diagnostics + +This change adds no persisted data, public model, coordinator state, or new identifier. + +| Existing input | Use in diagnostics | Boundary | +|---|---|---| +| `PackageChangeSet.CorrelationId` | Structured `CorrelationId` value on the adapter's Error record | Copied from the current reconciliation delivery; Nuplane uses the same value on its observer warning. | +| Thrown `Exception` | The exception argument on one Error record and the exception rethrown from the observer | The object is not wrapped, translated, or replaced. | +| Observer operation | Fixed structured `Operation` value `OnPackagesReconciledAsync` | Identifies the event boundary; the original exception remains the evidence for the specific failing call. No stage tracker is added. | +| Callback cancellation token | Determines whether a thrown `OperationCanceledException` represents caller-requested cancellation | Suppression applies only when the token is requested and the thrown exception is `OperationCanceledException`. | + +Failure classification is limited to the existing observer callback. An `OperationCanceledException` with an uncanceled token is an ordinary failure. `OutOfMemoryException`, `StackOverflowException`, `AccessViolationException`, `AppDomainUnloadedException`, and `BadImageFormatException` remain unlogged fatal exceptions. A returned `ReloadResult.Error` is data for the configured result callback, not a thrown operation failure. diff --git a/specs/024-nuplane-failure-diagnostics/plan.md b/specs/024-nuplane-failure-diagnostics/plan.md new file mode 100644 index 0000000..d9af608 --- /dev/null +++ b/specs/024-nuplane-failure-diagnostics/plan.md @@ -0,0 +1,68 @@ +# Implementation Plan: Nuplane Observer Failure Diagnostics + +**Branch**: `024-nuplane-failure-diagnostics` | **Date**: 2026-10-09 | **Spec**: [spec.md](spec.md) +**Input**: Feature specification from `specs/024-nuplane-failure-diagnostics/spec.md` + +## Summary + +Make admitted Nuplane observer operation failures visible as one structured Error record carrying the original exception and reconciliation correlation, then preserve existing exception propagation and pending-work retry. Add an optional typed logger to the private coordinator, use `NullLogger` when no provider is registered, and resolve the logger in the existing explicit coordinator-holder factory. Leave build-time freshness, public APIs, coordinator state, and Nuplane's observer isolation unchanged. + +## Technical Context + +**Language/Version**: C# 14 / .NET 10 SDK; source library targets `net8.0;net9.0;net10.0` +**Primary Dependencies**: Existing Nuplane abstractions, Microsoft.Extensions.Options, and a direct `Microsoft.Extensions.Logging.Abstractions` dependency; test-only reference to Nuplane's real observer-dispatcher package +**Storage**: N/A +**Testing**: xUnit integration tests; source project Release builds for all three target frameworks; public PackageReference consumer qualification for all three runtime versions +**Target Platform**: .NET 8, .NET 9, and .NET 10 +**Project Type**: Multi-target .NET library and integration-test project +**Performance Goals**: No additional work on ineligible callbacks or `BeginAsync`; one Error logging call only on an admitted operation failure +**Constraints**: Preserve the original exception instance, existing epoch acknowledgements/retry, cancellation and fatal-exception exclusions, logger-optional composition, and observer continuation through Nuplane's dispatcher +**Scale/Scope**: The existing private coordinator and its package/test qualification only; no new public API, public options, timers, state machine, host policy, or package-store mutation + +## Constitution Check + +**Pre-design gate** + +- **I. Abstraction-First Architecture**: Pass. This adds no public consumer contract and no abstraction project. The logging dependency belongs only to the optional implementation package. +- **III. Modern C# Style**: Pass. Keep nullable annotations, file-scoped namespaces, idiomatic `var`, and existing source multi-targets. +- **IV. Explicit Error Handling**: Pass. Failures are Error-logged and rethrown unchanged; no coordinator swallowing or exception wrapping is introduced. +- **V. Test Coverage**: Pass. Add focused xUnit cases for the three failure sources, exclusions, DI, and the actual observer dispatcher. +- **VI. Simplicity & Minimalism**: Pass. Reuse the existing coordinator, holder, logging abstractions, and options; do not add a logger abstraction or helper layer. +- **VII. Lifecycle & Concurrency Contracts**: Pass. Keep the existing `finally` gate release and epoch acknowledgement paths; do not alter Nuplane fan-out isolation. Nuplane's dispatcher remains the boundary that catches the rethrown observer exception and continues to later observers. + +No constitution exception or unresolved gate remains. + +**Post-design gate**: The design adds only a private logger dependency and a catch/log/rethrow boundary around already-admitted observer work. It introduces no locks, async work, state transitions, public contract, or provider ownership. All gates remain satisfied. + +## Design and Research + +- [Research decisions](research.md) records source-backed choices and rejected alternatives. +- [Failure diagnostic contract](contracts/observer-diagnostic-contract.md) records the externally observable log/exception boundary without defining a new API. +- [Data model](data-model.md) describes the existing reconciliation correlation; no durable entities or coordinator state are added. +- [Quickstart and validation](quickstart.md) gives the composition shape and focused verification sequence. + +## Project Structure + +```text +Directory.Packages.props # central test-only Nuplane runtime pin at 0.0.11-preview.99 +src/CShells.Nuplane/ +├── CShells.Nuplane.csproj # direct logging abstractions dependency +├── CShellsNuplaneBuilderExtensions.cs # manual holder factory supplies optional logger +├── Internal/ +│ └── NuplaneRefreshCoordinator.cs # failure classification, one Error, unchanged rethrow +└── README.md # optional logging behavior and diagnostic boundary + +tests/CShells.Tests/CShells.Tests.csproj # test-only Nuplane dispatcher package reference +tests/CShells.Tests/Integration/Nuplane/ +├── NuplaneRefreshCoordinatorTests.cs # refresh failure, identity, exclusion, retry +├── NuplaneReloadResultsTests.cs # registry/reload and callback failures, identity, retry +├── NuplaneCompositionTests.cs # DI logger wiring and no-provider composition +├── NuplaneObserverDiagnosticsTests.cs # actual Nuplane dispatcher + following observer +└── NuplaneTestLoggerProvider.cs # shared structured log capture for integration tests +``` + +**Structure Decision**: Keep production changes in the existing optional adapter. Add a trailing optional `ILogger? logger = null` constructor parameter and use `NullLogger.Instance` when absent; pass `serviceProvider.GetService>()` through the existing holder factory. Add Nuplane `0.0.11-preview.99` only to the net10 integration-test project, with a central test pin, so acceptance can exercise the actual `ObserverEventDispatcher` and `ReconciliationLogger`; do not add a runtime dependency from the adapter package or use a test-only dispatch shim. + +## Complexity Tracking + +No constitution violations or additional architectural components are planned. diff --git a/specs/024-nuplane-failure-diagnostics/quickstart.md b/specs/024-nuplane-failure-diagnostics/quickstart.md new file mode 100644 index 0000000..b277c5d --- /dev/null +++ b/specs/024-nuplane-failure-diagnostics/quickstart.md @@ -0,0 +1,29 @@ +# Quickstart: Nuplane Observer Failure Diagnostics + +The diagnostic behavior is automatic when the optional Nuplane adapter is selected and the host has a logging provider. No new adapter option or public logging API is required. + +```csharp +using CShells.DependencyInjection; +using CShells.Nuplane; +using Microsoft.Extensions.DependencyInjection; + +var services = new ServiceCollection(); +services.AddLogging(); +services.AddCShells(shells => shells.WithNuplaneFeatureDiscovery()); +``` + +If the host does not register logging, the adapter uses its null logger fallback and retains the existing exception behavior. + +## Focused validation + +Run from the repository root through the standard `dotnet` build-slot wrapper: + +```sh +dotnet test tests/CShells.Tests/CShells.Tests.csproj -c Release --filter 'FullyQualifiedName~Nuplane' +dotnet build src/CShells.Nuplane/CShells.Nuplane.csproj -c Release -f net8.0 +dotnet build src/CShells.Nuplane/CShells.Nuplane.csproj -c Release -f net9.0 +dotnet build src/CShells.Nuplane/CShells.Nuplane.csproj -c Release -f net10.0 +dotnet test tests/CShells.Tests/CShells.Tests.csproj -c Release +``` + +Before issue closure, pass the required organization PR/review and main-branch CI gates, then use the separate PackageReference-only diagnostic consumer against the resulting owner-published corrective preview on actual .NET 8, .NET 9, and .NET 10 runtimes. Audit all ten CShells archives against successful owner workflow metadata and public-feed bytes first. The pre-correction preview counterexample is not correction proof. No manual package publication, stable-pin change, or stable-release qualification belongs to this work unit. diff --git a/specs/024-nuplane-failure-diagnostics/research.md b/specs/024-nuplane-failure-diagnostics/research.md new file mode 100644 index 0000000..8206516 --- /dev/null +++ b/specs/024-nuplane-failure-diagnostics/research.md @@ -0,0 +1,34 @@ +# Research: Nuplane Observer Failure Diagnostics + +## Decisions + +### 1. Use the existing typed-logger convention and optional fallback + +- **Decision**: Give the private `NuplaneRefreshCoordinator` an optional `ILogger` and use `NullLogger.Instance` when absent. Resolve the optional logger in the existing `CoordinatorHolder` service factory and pass it explicitly to the coordinator. Add a direct `Microsoft.Extensions.Logging.Abstractions` package reference to `CShells.Nuplane.csproj`. +- **Rationale**: Other CShells services use this optional typed-logger pattern. The adapter is optional and is also composed in service collections without logging. `Directory.Packages.props` already pins `Microsoft.Extensions.Logging.Abstractions` for all three target-framework groups (9.0.11 for net8/net9 and 10.0.0 for net10), so no new central version is required. +- **Alternatives considered**: A required logger would break no-provider composition; adding an adapter-specific logger abstraction would add a layer with no current consumer need. + +### 2. Keep the diagnostic boundary after existing admission and policy validation + +- **Decision**: Preserve argument guards, the caller-token precheck, ineligible-completion return, and option/trigger validation before the new Error boundary. Do not change `BeginAsync`. For admitted observer work, log one Error with the original exception and `changeSet.CorrelationId`, then use a bare rethrow so the same instance reaches Nuplane's dispatcher or a direct caller. Do not change gate `finally` blocks or acknowledge epochs on failure. +- **Rationale**: `PackageChangeSet.CorrelationId` is Nuplane's documented reconciliation-cycle identifier, and `ObserverEventDispatcher` already places that same value on its warning. The accepted Foundation contract requires the original exception and this correlation; the coordinator's internal freshness epoch is not the reconciliation ID. +- **Alternatives considered**: Logging inside `BeginAsync` would extend diagnostics to a distinct build-time operation. Wrapping or swallowing the exception would break upstream exception-identity tests and observer isolation semantics. + +### 3. Preserve the established cancellation and fatal-exception filter + +- **Decision**: Do not Error-log an `OperationCanceledException` only when the callback token is requested. Treat an uncanceled `OperationCanceledException` as an ordinary failure. Do not Error-log `OutOfMemoryException`, `StackOverflowException`, `AccessViolationException`, `AppDomainUnloadedException`, or `BadImageFormatException`. Other admitted nonfatal exceptions are logged and rethrown. +- **Rationale**: This is the concrete exclusion behavior used by Foundation's existing observer integration. It distinguishes caller cancellation from an operation that merely happens to throw an `OperationCanceledException`. +- **Alternatives considered**: Excluding every `OperationCanceledException` would hide non-cancellation failures; excluding every exception whenever the token is signaled would also hide unrelated faults. + +### 4. Verify both the coordinator boundary and Nuplane's real fan-out + +- **Decision**: Use focused coordinator tests for refresh, registry/reload, and callback failures, and a DI-backed test that resolves the public Nuplane `ObserverEventDispatcher` / `ReconciliationLogger` path with a following observer. Add `Nuplane` `0.0.11-preview.99` only to the net10 test project with a central pin; do not add it to the shipping adapter graph. Keep a no-logging-provider composition test. +- **Rationale**: The actual published dispatcher catches observer exceptions, records its existing Warning with correlation and message only, and continues through later observers. An independently reviewed package probe showed the current adapter has no Error entry and the dispatcher warning lacks the exception object. The fix must add the adapter Error without replacing this established fan-out behavior. +- **Alternatives considered**: Direct observer invocation alone cannot prove the actual dispatcher continues after the adapter's rethrow. A test-only dispatch shim cannot prove the public runtime path. + +## Source Findings + +- The coordinator currently owns refresh/reload epoch gates and preserves failed work for later eligible deliveries. Keep this state machine unchanged. +- `CShellsNuplaneBuilderExtensions` creates the private coordinator manually inside the `CoordinatorHolder` factory; this is the point to pass a logger resolved with optional `GetService`. +- The test project already references the adapter project. It currently sees only Nuplane abstractions transitively; the real `ObserverEventDispatcher` is in the Nuplane runtime package. The external public-package probe provides a concrete registration/dispatch pattern. No new test project or public contract is needed. +- Returned `ReloadResult.Error` values flow to the existing options callback and retain their current meaning; only thrown operation failures enter this diagnostics boundary. diff --git a/specs/024-nuplane-failure-diagnostics/spec.md b/specs/024-nuplane-failure-diagnostics/spec.md new file mode 100644 index 0000000..d497123 --- /dev/null +++ b/specs/024-nuplane-failure-diagnostics/spec.md @@ -0,0 +1,66 @@ +# Feature Specification: Nuplane Observer Failure Diagnostics + +**Feature Branch**: `024-nuplane-failure-diagnostics` +**Created**: 2026-10-09 +**Status**: Draft +**Input**: Add generic observer operation failure diagnostics to the existing CShells.Nuplane coordinator without changing its public API or swallowing failures. + +## User Scenarios & Testing + +### User Story 1 - Diagnose package reconciliation failures (Priority: P1) + +As a host maintainer using the optional Nuplane integration, I want an operational error record when an admitted package reconciliation operation fails, while the same failure still reaches the caller and remains retryable, so that I can diagnose it without changing Nuplane's observer-delivery behavior. + +**Why this priority**: Foundation's existing Nuplane adoption relies on an Error record containing the original exception. The published adapter currently allows the exception to reach Nuplane's observer dispatcher, which isolates it and continues delivery, but emits only a generic Warning without the exception payload. + +**Independent Test**: Register the adapter through dependency injection with a capturing logger, inject refresh, registry/reload, and result-callback failures, and verify one Error record containing the original exception and reconciliation correlation, unchanged exception propagation, and retry on a later eligible delivery. Then dispatch through Nuplane's real observer dispatcher and verify that a following observer still runs. + +**Acceptance Scenarios**: + +1. **Given** an enabled adapter and an eligible reconciliation delivery, **When** catalog refresh fails, **Then** one Error record contains that same exception and the reconciliation's correlation ID, the same exception instance propagates, and the pending freshness can be retried on a later eligible delivery. +2. **Given** an enabled adapter with automatic reload, **When** active-registry access or the reload call throws, **Then** one Error record contains the same exception and the reconciliation's correlation ID, the same exception instance propagates, and pending reload work remains retryable. +3. **Given** an enabled adapter with automatic reload and a configured result callback, **When** that callback throws, **Then** one Error record contains the same exception and the reconciliation's correlation ID, the same exception instance propagates, and pending reload work remains retryable. +4. **Given** a caller-requested cancellation during an admitted operation, **When** the operation propagates an `OperationCanceledException` while the callback's token is requested, **Then** it is not recorded as an Error and pending work is not acknowledged. +5. **Given** a fatal runtime exception during an admitted operation, **When** it propagates, **Then** it is not recorded as an Error. +6. **Given** a reconciliation callback delivered through Nuplane's observer dispatcher, **When** the adapter logs and rethrows an ordinary operation failure, **Then** the dispatcher retains its own warning behavior and continues to the next observer. +7. **Given** no logging provider is registered, **When** an ordinary operation failure occurs, **Then** the adapter remains usable and propagates the original exception unchanged. + +### Edge Cases + +- An eligible delivery is a completed reconciliation callback with one or more applied packages or a committed removal, and observer work is enabled. +- A failed-only or otherwise ineligible completion does no coordinator work and emits no new Error record. +- An options-read or refresh-trigger validation failure occurs before observer work is admitted; it retains its existing propagation and no-work behavior without an adapter Error record. +- An `OperationCanceledException` is an ordinary operation failure when the callback's cancellation token was not requested; it is Error-logged and rethrown unchanged. A requested token alone does not suppress logging for a different exception type. +- Caller-requested cancellation and these fatal exceptions are excluded from Error logging: `OutOfMemoryException`, `StackOverflowException`, `AccessViolationException`, `AppDomainUnloadedException`, and `BadImageFormatException`. +- A build-time freshness failure remains outside this observer-diagnostics boundary and retains its existing propagation behavior. +- Returned per-shell reload results with an `Error` remain result data for the configured callback; they are not themselves a thrown coordinator operation failure. +- A logger is optional. Its absence must not alter observer behavior. + +## Requirements + +### Functional Requirements + +- **FR-001**: Diagnostics MUST apply only after a completed reconciliation delivery with applied packages or a committed removal has passed existing option validation with observer work enabled and entered observer work. +- **FR-002**: An ordinary nonfatal exception thrown by catalog refresh, active-registry access, reload invocation, or the configured reload-results callback MUST produce exactly one Error record for that failed observer operation. +- **FR-003**: The Error record MUST carry the original exception object and the structured correlation ID supplied for the reconciliation; it MUST NOT replace the exception with a wrapper or formatted-message-only diagnostic. +- **FR-004**: The observer MUST rethrow the original exception instance unchanged after recording it. It MUST preserve existing gate release, epoch acknowledgement, and retry behavior. +- **FR-005**: An `OperationCanceledException` MUST propagate without an Error record only when the callback's cancellation token is requested. Fatal runtime exceptions MUST propagate without an Error record. An uncanceled `OperationCanceledException` MUST follow the ordinary failure behavior. +- **FR-006**: The integration MUST work when no logging provider is present; no new public configuration or logging API is introduced. +- **FR-007**: The adapter's Error record MUST supplement, not replace, Nuplane's existing observer-isolation warning. The failure MUST remain available to Nuplane's dispatcher so subsequent observers continue to run. +- **FR-008**: Build-time catalog freshness and returned per-shell `ReloadResult.Error` values MUST retain their existing semantics and remain outside the new exception-diagnostic behavior. + +### Key Entities + +- **Observer operation failure**: An exception thrown by admitted reconciliation work in catalog refresh, active-registry access, reload invocation, or result-callback execution. +- **Reconciliation correlation**: The `CorrelationId` supplied with the reconciliation delivery; it is emitted as a structured logging value without adding coordinator state. +- **Error record**: One Error-level logging event that carries the original exception object and reconciliation correlation. + +## Success Criteria + +### Measurable Outcomes + +- **SC-001**: Each injected ordinary failure from refresh, registry/reload, and result-callback execution yields exactly one Error record containing the exact thrown exception instance and `PackageChangeSet.CorrelationId` value. +- **SC-002**: Each injected ordinary failure still propagates as the exact original exception instance, and a subsequent eligible delivery can retry the retained work. +- **SC-003**: Requested cancellation and each listed fatal exception produce zero adapter Error records while retaining propagation. +- **SC-004**: Through the real Nuplane observer dispatcher, the adapter Error is observed, Nuplane's existing warning is preserved, and a following observer is invoked. +- **SC-005**: The optional adapter remains constructible and propagates failures when the host has no logging provider. diff --git a/specs/024-nuplane-failure-diagnostics/tasks.md b/specs/024-nuplane-failure-diagnostics/tasks.md new file mode 100644 index 0000000..cece1fc --- /dev/null +++ b/specs/024-nuplane-failure-diagnostics/tasks.md @@ -0,0 +1,58 @@ +# Tasks: Nuplane Observer Failure Diagnostics + +**Input**: Design documents from `specs/024-nuplane-failure-diagnostics/` +**Prerequisites**: `plan.md`, `spec.md`, `research.md`, `data-model.md`, `contracts/`, `quickstart.md` + +## Phase 1: Setup + +**Purpose**: Declare the implementation dependency and the actual-dispatcher test dependency. + +- [ ] T001 Add `Microsoft.Extensions.Logging.Abstractions` to `src/CShells.Nuplane/CShells.Nuplane.csproj`; keep using the existing target-framework-specific central pins in `Directory.Packages.props`. +- [ ] T002 Add the `Nuplane` runtime package to `tests/CShells.Tests/CShells.Tests.csproj` and add its test-only central version pin `0.0.11-preview.99` in `Directory.Packages.props`; do not add it to the shipping adapter project. + +## Phase 2: Foundational + +**Purpose**: Provide shared structured log capture for the focused test cases. + +- [ ] T003 [P] Add a reusable capturing `ILoggerProvider` in `tests/CShells.Tests/Integration/Nuplane/NuplaneTestLoggerProvider.cs` that retains level, exception instance, structured state, and category. + +## Phase 3: User Story 1 - Diagnose package reconciliation failures (Priority: P1) + +**Goal**: Operators can identify an admitted Nuplane reconciliation failure from one Error record while callers and Nuplane retain existing exception and retry behavior. + +**Independent Test**: Focused tests prove identity-preserving logging and retry for each thrown observer operation; direct cancellation/fatal exclusions; logger-optional DI; actual Nuplane dispatcher warning and continuation. + +### Tests for User Story 1 + +- [ ] T004 [US1] Add coordinator tests in `tests/CShells.Tests/Integration/Nuplane/NuplaneRefreshCoordinatorTests.cs` for one Error with `PackageChangeSet.CorrelationId`, original exception identity, and retained retry after refresh failure; also assert Begin-only refresh failure remains unlogged and unchanged. +- [ ] T005 [US1] Add logger classification tests in `tests/CShells.Tests/Integration/Nuplane/NuplaneRefreshCoordinatorTests.cs` and validation-boundary assertions in `tests/CShells.Tests/Integration/Nuplane/NuplaneLiveOptionsTests.cs` for requested-token cancellation, uncanceled `OperationCanceledException`, each of `OutOfMemoryException`, `StackOverflowException`, `AccessViolationException`, `AppDomainUnloadedException`, and `BadImageFormatException`, plus option-read/invalid-trigger failures that precede observer admission and emit no adapter Error. +- [ ] T006 [P] [US1] Add registry/reload and result-callback failure, exception-identity, and pending-retry assertions in `tests/CShells.Tests/Integration/Nuplane/NuplaneReloadResultsTests.cs`; assert returned `ReloadResult.Error` remains callback data and produces no new adapter Error. +- [ ] T007 [P] [US1] Add DI logger-factory wiring and no-`AddLogging` propagation coverage in `tests/CShells.Tests/Integration/Nuplane/NuplaneCompositionTests.cs`. +- [ ] T008 [P] [US1] Add `tests/CShells.Tests/Integration/Nuplane/NuplaneObserverDiagnosticsTests.cs` using the real `ObserverEventDispatcher` and `ReconciliationLogger`; assert the adapter Error carries the exception and correlation, Nuplane's Warning remains, and the following observer runs and can retry. + +### Implementation for User Story 1 + +- [ ] T009 [US1] Add optional `ILogger` injection with `NullLogger` fallback and a narrow admitted-observer catch/log/rethrow boundary in `src/CShells.Nuplane/Internal/NuplaneRefreshCoordinator.cs`; include fixed `OnPackagesReconciledAsync` operation and reconciliation correlation fields, exclude only requested-token cancellation and the five specified fatal types, and preserve gates, epochs, retries, `BeginAsync`, and returned-result semantics. +- [ ] T010 [US1] Pass the optional typed logger through the existing `CoordinatorHolder` manual factory in `src/CShells.Nuplane/CShellsNuplaneBuilderExtensions.cs`, using optional service resolution so hosts without logging remain valid. +- [ ] T011 [US1] Document the optional logger behavior and unchanged propagation boundary in `src/CShells.Nuplane/README.md` without adding public options or API. + +## Phase 4: Verification and package qualification + +**Purpose**: Prove the implementation against the focused source tests and supported package surfaces. + +- [ ] T012 Run focused Nuplane tests and the full `tests/CShells.Tests/CShells.Tests.csproj` Release suite; record exact commands and results in the issue's evidence before closure. +- [ ] T013 Build `src/CShells.Nuplane/CShells.Nuplane.csproj` in Release for `net8.0`, `net9.0`, and `net10.0`; retain exact command results. +- [ ] T014 In a disposable copy of `src/CShells.Nuplane/Internal/NuplaneRefreshCoordinator.cs`, remove the diagnostics path and compile/run the focused diagnostic tests to prove they detect the regression; restore the intended file and verify no mutation residue. +- [ ] T015 Open the normal organization PR after root and independent review, pass required PR CI/review gates, merge under repository policy, and verify the `.github/workflows/publish.yml` main-branch gate and owner-produced preview publication before treating package artifacts as acceptance evidence. +- [ ] T016 After the owner-published corrective preview is available, audit all ten public CShells package archives against successful workflow metadata and feed bytes, then run the PackageReference-only diagnostic consumer on actual .NET 8, .NET 9, and .NET 10 runtimes using `public-consumer-plan.md` in the modular-hosting evidence artifact workspace. Keep this proof distinct from the accepted pre-correction counterexample; do not manually publish or promote a stable release as part of this task. + +## Dependencies and Execution Order + +- Setup T001–T002 precedes the tests and implementation. +- T003 is shared test infrastructure. T004–T008 can be drafted independently by file after setup, but the verification sequence starts only after T009–T010 implement the expected behavior. +- T009 and T010 depend on the direct adapter package reference and logger contract. T011 follows the implementation. +- T012–T014 require the complete source change. T015 is the normal merge/main-preview gate. T016 requires owner-published corrective package artifacts after that gate and is not source-only acceptance. + +## Implementation Strategy + +Deliver the narrow adapter correction first: direct logging dependency, focused failure tests, one private logger boundary, and the explicit DI factory update. Verify tests and all three library target frameworks before repository review. Complete the required organization PR/main gate, then the actual-public archive and runtime consumer gate before issue closure. Do not manually publish packages, change stable pins, or conflate the old public counterexample with corrective-package proof. From edbf58283b562e9288fa5986683c0eef4b533dc8 Mon Sep 17 00:00:00 2001 From: Sipke Schoorstra Date: Fri, 9 Oct 2026 01:06:31 +0200 Subject: [PATCH 2/3] feat(nuplane): log observer failures --- Directory.Packages.props | 1 + .../contracts/observer-diagnostic-contract.md | 1 + .../024-nuplane-failure-diagnostics/tasks.md | 24 ++-- src/CShells.Nuplane/CShells.Nuplane.csproj | 1 + .../CShellsNuplaneBuilderExtensions.cs | 4 +- .../Internal/NuplaneRefreshCoordinator.cs | 72 ++++++---- src/CShells.Nuplane/README.md | 6 + tests/CShells.Tests/CShells.Tests.csproj | 1 + .../Nuplane/NuplaneCompositionTests.cs | 43 ++++++ .../Nuplane/NuplaneCoordinatorTestHarness.cs | 45 +++++- .../Nuplane/NuplaneLiveOptionsTests.cs | 4 +- .../NuplaneObserverDiagnosticsTests.cs | 83 +++++++++++ .../Nuplane/NuplaneRefreshCoordinatorTests.cs | 135 +++++++++++++++++- .../Nuplane/NuplaneReloadResultsTests.cs | 72 +++++++++- .../Nuplane/NuplaneTestLoggerProvider.cs | 73 ++++++++++ 15 files changed, 517 insertions(+), 48 deletions(-) create mode 100644 tests/CShells.Tests/Integration/Nuplane/NuplaneObserverDiagnosticsTests.cs create mode 100644 tests/CShells.Tests/Integration/Nuplane/NuplaneTestLoggerProvider.cs diff --git a/Directory.Packages.props b/Directory.Packages.props index bea38e8..9cd2b01 100644 --- a/Directory.Packages.props +++ b/Directory.Packages.props @@ -44,6 +44,7 @@ + diff --git a/specs/024-nuplane-failure-diagnostics/contracts/observer-diagnostic-contract.md b/specs/024-nuplane-failure-diagnostics/contracts/observer-diagnostic-contract.md index eec739c..e520040 100644 --- a/specs/024-nuplane-failure-diagnostics/contracts/observer-diagnostic-contract.md +++ b/specs/024-nuplane-failure-diagnostics/contracts/observer-diagnostic-contract.md @@ -21,5 +21,6 @@ The adapter rethrows that same exception instance. It does not format and substi - Do not extend the new diagnostic boundary to build-time freshness in `BeginAsync`. - A returned `ReloadResult.Error` is not a thrown failure and is not newly Error-logged; preserve the existing result callback and pending-reload behavior. - If the host has no logging provider, construction and operation remain valid through the null logger fallback. +- Exception-preserving diagnostics assume registered logging providers do not throw from `ILogger.Log`; logger-provider failures are outside this adapter contract. No particular log message wording or event ID is a public compatibility promise. The exception object, structured correlation, observer-operation name, Error level, one-record count, and exception propagation are the contract. diff --git a/specs/024-nuplane-failure-diagnostics/tasks.md b/specs/024-nuplane-failure-diagnostics/tasks.md index cece1fc..a7b00d9 100644 --- a/specs/024-nuplane-failure-diagnostics/tasks.md +++ b/specs/024-nuplane-failure-diagnostics/tasks.md @@ -7,14 +7,14 @@ **Purpose**: Declare the implementation dependency and the actual-dispatcher test dependency. -- [ ] T001 Add `Microsoft.Extensions.Logging.Abstractions` to `src/CShells.Nuplane/CShells.Nuplane.csproj`; keep using the existing target-framework-specific central pins in `Directory.Packages.props`. -- [ ] T002 Add the `Nuplane` runtime package to `tests/CShells.Tests/CShells.Tests.csproj` and add its test-only central version pin `0.0.11-preview.99` in `Directory.Packages.props`; do not add it to the shipping adapter project. +- [X] T001 Add `Microsoft.Extensions.Logging.Abstractions` to `src/CShells.Nuplane/CShells.Nuplane.csproj`; keep using the existing target-framework-specific central pins in `Directory.Packages.props`. +- [X] T002 Add the `Nuplane` runtime package to `tests/CShells.Tests/CShells.Tests.csproj` and add its test-only central version pin `0.0.11-preview.99` in `Directory.Packages.props`; do not add it to the shipping adapter project. ## Phase 2: Foundational **Purpose**: Provide shared structured log capture for the focused test cases. -- [ ] T003 [P] Add a reusable capturing `ILoggerProvider` in `tests/CShells.Tests/Integration/Nuplane/NuplaneTestLoggerProvider.cs` that retains level, exception instance, structured state, and category. +- [X] T003 [P] Add a reusable capturing `ILoggerProvider` in `tests/CShells.Tests/Integration/Nuplane/NuplaneTestLoggerProvider.cs` that retains level, exception instance, structured state, and category. ## Phase 3: User Story 1 - Diagnose package reconciliation failures (Priority: P1) @@ -24,24 +24,24 @@ ### Tests for User Story 1 -- [ ] T004 [US1] Add coordinator tests in `tests/CShells.Tests/Integration/Nuplane/NuplaneRefreshCoordinatorTests.cs` for one Error with `PackageChangeSet.CorrelationId`, original exception identity, and retained retry after refresh failure; also assert Begin-only refresh failure remains unlogged and unchanged. -- [ ] T005 [US1] Add logger classification tests in `tests/CShells.Tests/Integration/Nuplane/NuplaneRefreshCoordinatorTests.cs` and validation-boundary assertions in `tests/CShells.Tests/Integration/Nuplane/NuplaneLiveOptionsTests.cs` for requested-token cancellation, uncanceled `OperationCanceledException`, each of `OutOfMemoryException`, `StackOverflowException`, `AccessViolationException`, `AppDomainUnloadedException`, and `BadImageFormatException`, plus option-read/invalid-trigger failures that precede observer admission and emit no adapter Error. -- [ ] T006 [P] [US1] Add registry/reload and result-callback failure, exception-identity, and pending-retry assertions in `tests/CShells.Tests/Integration/Nuplane/NuplaneReloadResultsTests.cs`; assert returned `ReloadResult.Error` remains callback data and produces no new adapter Error. -- [ ] T007 [P] [US1] Add DI logger-factory wiring and no-`AddLogging` propagation coverage in `tests/CShells.Tests/Integration/Nuplane/NuplaneCompositionTests.cs`. -- [ ] T008 [P] [US1] Add `tests/CShells.Tests/Integration/Nuplane/NuplaneObserverDiagnosticsTests.cs` using the real `ObserverEventDispatcher` and `ReconciliationLogger`; assert the adapter Error carries the exception and correlation, Nuplane's Warning remains, and the following observer runs and can retry. +- [X] T004 [US1] Add coordinator tests in `tests/CShells.Tests/Integration/Nuplane/NuplaneRefreshCoordinatorTests.cs` for one Error with `PackageChangeSet.CorrelationId`, original exception identity, and retained retry after refresh failure; also assert Begin-only refresh failure remains unlogged and unchanged. +- [X] T005 [US1] Add logger classification tests in `tests/CShells.Tests/Integration/Nuplane/NuplaneRefreshCoordinatorTests.cs` and validation-boundary assertions in `tests/CShells.Tests/Integration/Nuplane/NuplaneLiveOptionsTests.cs` for requested-token cancellation, uncanceled `OperationCanceledException`, each of `OutOfMemoryException`, `StackOverflowException`, `AccessViolationException`, `AppDomainUnloadedException`, and `BadImageFormatException`, plus option-read/invalid-trigger failures that precede observer admission and emit no adapter Error. +- [X] T006 [P] [US1] Add registry/reload and result-callback failure, exception-identity, and pending-retry assertions in `tests/CShells.Tests/Integration/Nuplane/NuplaneReloadResultsTests.cs`; assert returned `ReloadResult.Error` remains callback data and produces no new adapter Error. +- [X] T007 [P] [US1] Add DI logger-factory wiring and no-`AddLogging` propagation coverage in `tests/CShells.Tests/Integration/Nuplane/NuplaneCompositionTests.cs`. +- [X] T008 [P] [US1] Add `tests/CShells.Tests/Integration/Nuplane/NuplaneObserverDiagnosticsTests.cs` using the real `ObserverEventDispatcher` and `ReconciliationLogger`; assert the adapter Error carries the exception and correlation, Nuplane's Warning remains, and the following observer runs and can retry. ### Implementation for User Story 1 -- [ ] T009 [US1] Add optional `ILogger` injection with `NullLogger` fallback and a narrow admitted-observer catch/log/rethrow boundary in `src/CShells.Nuplane/Internal/NuplaneRefreshCoordinator.cs`; include fixed `OnPackagesReconciledAsync` operation and reconciliation correlation fields, exclude only requested-token cancellation and the five specified fatal types, and preserve gates, epochs, retries, `BeginAsync`, and returned-result semantics. -- [ ] T010 [US1] Pass the optional typed logger through the existing `CoordinatorHolder` manual factory in `src/CShells.Nuplane/CShellsNuplaneBuilderExtensions.cs`, using optional service resolution so hosts without logging remain valid. -- [ ] T011 [US1] Document the optional logger behavior and unchanged propagation boundary in `src/CShells.Nuplane/README.md` without adding public options or API. +- [X] T009 [US1] Add optional `ILogger` injection with `NullLogger` fallback and a narrow admitted-observer catch/log/rethrow boundary in `src/CShells.Nuplane/Internal/NuplaneRefreshCoordinator.cs`; include fixed `OnPackagesReconciledAsync` operation and reconciliation correlation fields, exclude only requested-token cancellation and the five specified fatal types, and preserve gates, epochs, retries, `BeginAsync`, and returned-result semantics. +- [X] T010 [US1] Pass the optional typed logger through the existing `CoordinatorHolder` manual factory in `src/CShells.Nuplane/CShellsNuplaneBuilderExtensions.cs`, using optional service resolution so hosts without logging remain valid. +- [X] T011 [US1] Document the optional logger behavior and unchanged propagation boundary in `src/CShells.Nuplane/README.md` without adding public options or API. ## Phase 4: Verification and package qualification **Purpose**: Prove the implementation against the focused source tests and supported package surfaces. - [ ] T012 Run focused Nuplane tests and the full `tests/CShells.Tests/CShells.Tests.csproj` Release suite; record exact commands and results in the issue's evidence before closure. -- [ ] T013 Build `src/CShells.Nuplane/CShells.Nuplane.csproj` in Release for `net8.0`, `net9.0`, and `net10.0`; retain exact command results. +- [X] T013 Build `src/CShells.Nuplane/CShells.Nuplane.csproj` in Release for `net8.0`, `net9.0`, and `net10.0`; retain exact command results. - [ ] T014 In a disposable copy of `src/CShells.Nuplane/Internal/NuplaneRefreshCoordinator.cs`, remove the diagnostics path and compile/run the focused diagnostic tests to prove they detect the regression; restore the intended file and verify no mutation residue. - [ ] T015 Open the normal organization PR after root and independent review, pass required PR CI/review gates, merge under repository policy, and verify the `.github/workflows/publish.yml` main-branch gate and owner-produced preview publication before treating package artifacts as acceptance evidence. - [ ] T016 After the owner-published corrective preview is available, audit all ten public CShells package archives against successful workflow metadata and feed bytes, then run the PackageReference-only diagnostic consumer on actual .NET 8, .NET 9, and .NET 10 runtimes using `public-consumer-plan.md` in the modular-hosting evidence artifact workspace. Keep this proof distinct from the accepted pre-correction counterexample; do not manually publish or promote a stable release as part of this task. diff --git a/src/CShells.Nuplane/CShells.Nuplane.csproj b/src/CShells.Nuplane/CShells.Nuplane.csproj index f1595b8..b6e1da1 100644 --- a/src/CShells.Nuplane/CShells.Nuplane.csproj +++ b/src/CShells.Nuplane/CShells.Nuplane.csproj @@ -12,6 +12,7 @@ + diff --git a/src/CShells.Nuplane/CShellsNuplaneBuilderExtensions.cs b/src/CShells.Nuplane/CShellsNuplaneBuilderExtensions.cs index 0b66e5b..f6763d2 100644 --- a/src/CShells.Nuplane/CShellsNuplaneBuilderExtensions.cs +++ b/src/CShells.Nuplane/CShellsNuplaneBuilderExtensions.cs @@ -3,6 +3,7 @@ using CShells.Nuplane.Internal; using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.DependencyInjection.Extensions; +using Microsoft.Extensions.Logging; using Nuplane.Abstractions; namespace CShells.Nuplane; @@ -44,7 +45,8 @@ public static CShellsBuilder WithNuplaneFeatureDiscovery( new NuplaneRefreshCoordinator( serviceProvider.GetRequiredService(), serviceProvider.GetRequiredService>(), - () => serviceProvider.GetRequiredService()))); + () => serviceProvider.GetRequiredService(), + serviceProvider.GetService>()))); services.AddSingleton(serviceProvider => serviceProvider.GetRequiredService().Coordinator); services.AddSingleton(serviceProvider => serviceProvider.GetRequiredService().Coordinator); } diff --git a/src/CShells.Nuplane/Internal/NuplaneRefreshCoordinator.cs b/src/CShells.Nuplane/Internal/NuplaneRefreshCoordinator.cs index 444b647..89f389f 100644 --- a/src/CShells.Nuplane/Internal/NuplaneRefreshCoordinator.cs +++ b/src/CShells.Nuplane/Internal/NuplaneRefreshCoordinator.cs @@ -1,5 +1,7 @@ using CShells.Features; using CShells.Lifecycle; +using Microsoft.Extensions.Logging; +using Microsoft.Extensions.Logging.Abstractions; using Microsoft.Extensions.Options; using Nuplane.Abstractions; @@ -9,11 +11,13 @@ namespace CShells.Nuplane.Internal; internal sealed class NuplaneRefreshCoordinator( IRuntimeFeatureCatalog featureCatalog, IOptionsMonitor options, - Func shellRegistryFactory) : INuplaneObserver, IShellGenerationBuildParticipant + Func shellRegistryFactory, + ILogger? logger = null) : INuplaneObserver, IShellGenerationBuildParticipant { private readonly IRuntimeFeatureCatalog catalog = featureCatalog ?? throw new ArgumentNullException(nameof(featureCatalog)); private readonly IOptionsMonitor optionsMonitor = options ?? throw new ArgumentNullException(nameof(options)); private readonly Func registryFactory = shellRegistryFactory ?? throw new ArgumentNullException(nameof(shellRegistryFactory)); + private readonly ILogger loggerInstance = logger ?? NullLogger.Instance; private readonly object epochStateLock = new(); private readonly SemaphoreSlim refreshGate = new(1, 1); private readonly SemaphoreSlim reloadGate = new(1, 1); @@ -46,36 +50,48 @@ public async Task OnPackagesReconciledAsync( if (!policy.Enabled) return; - var hasSourceChanges = changeSet.Added.Count > 0 || changeSet.Updated.Count > 0 || changeSet.Removed.Count > 0; - var forceRefresh = policy.RefreshTrigger == NuplaneRefreshTrigger.EveryEligibleCompletion; - long catalogRequestAtDelivery; - bool callbackRequestsCatalogWork; - - lock (epochStateLock) + try { - if (forceRefresh || hasSourceChanges) - _requestedCatalogEpoch = checked(_requestedCatalogEpoch + 1); + var hasSourceChanges = changeSet.Added.Count > 0 || changeSet.Updated.Count > 0 || changeSet.Removed.Count > 0; + var forceRefresh = policy.RefreshTrigger == NuplaneRefreshTrigger.EveryEligibleCompletion; + long catalogRequestAtDelivery; + bool callbackRequestsCatalogWork; - catalogRequestAtDelivery = _requestedCatalogEpoch; - callbackRequestsCatalogWork = _requestedCatalogEpoch > _committedCatalogEpoch; - } + lock (epochStateLock) + { + if (forceRefresh || hasSourceChanges) + _requestedCatalogEpoch = checked(_requestedCatalogEpoch + 1); - cancellationToken.ThrowIfCancellationRequested(); - var registry = registryFactory(); - if (registry.GetActiveShells().Count == 0) - return; + catalogRequestAtDelivery = _requestedCatalogEpoch; + callbackRequestsCatalogWork = _requestedCatalogEpoch > _committedCatalogEpoch; + } - if (policy.AutoReload && callbackRequestsCatalogWork) - { - lock (epochStateLock) + cancellationToken.ThrowIfCancellationRequested(); + var registry = registryFactory(); + if (registry.GetActiveShells().Count == 0) + return; + + if (policy.AutoReload && callbackRequestsCatalogWork) { - _requestedReloadEpoch = checked(_requestedReloadEpoch + 1); - _reloadCatalogEpoch = catalogRequestAtDelivery; + lock (epochStateLock) + { + _requestedReloadEpoch = checked(_requestedReloadEpoch + 1); + _reloadCatalogEpoch = catalogRequestAtDelivery; + } } - } - await RefreshPendingCatalogAsync(cancellationToken).ConfigureAwait(false); - await TryReloadPendingAsync(registry, policy, cancellationToken).ConfigureAwait(false); + await RefreshPendingCatalogAsync(cancellationToken).ConfigureAwait(false); + await TryReloadPendingAsync(registry, policy, cancellationToken).ConfigureAwait(false); + } + catch (Exception exception) when (ShouldLog(exception, cancellationToken)) + { + loggerInstance.LogError( + exception, + "Nuplane reconciliation operation {Operation} failed for {CorrelationId}.", + nameof(INuplaneObserver.OnPackagesReconciledAsync), + changeSet.CorrelationId); + throw; + } } public async ValueTask BeginAsync( @@ -132,6 +148,14 @@ private IntegrationPolicy CapturePolicy() return policy; } + private static bool ShouldLog(Exception exception, CancellationToken cancellationToken) => + !(exception is OperationCanceledException && cancellationToken.IsCancellationRequested) && + exception is not OutOfMemoryException and + not StackOverflowException and + not AccessViolationException and + not AppDomainUnloadedException and + not BadImageFormatException; + private async ValueTask TryReloadPendingAsync( IShellRegistry registry, IntegrationPolicy policy, diff --git a/src/CShells.Nuplane/README.md b/src/CShells.Nuplane/README.md index 9667e50..0e3a7cb 100644 --- a/src/CShells.Nuplane/README.md +++ b/src/CShells.Nuplane/README.md @@ -67,6 +67,12 @@ An eligible completion is a delivered `OnPackagesReconciledAsync` callback with The adapter reads the current options once for each eligible reconciliation delivery and copies the four policy values before recording work. Standard `IOptionsMonitor` configuration therefore applies on the next eligible delivery, including values bound from a reloaded `IConfiguration`; changes do not start a background refresh or reload by themselves. The current delivery keeps the policy it captured even if configuration changes while its refresh is running. Build participation does not read options. Use `Configure`, `Bind`, or dependency-aware `AddOptions` configuration so the registered monitor owns the live values. A custom `IOptionsMonitor` registration is supported; replacing only `IOptions` is unsupported. +## Diagnose observer operation failures + +When logging is registered, an ordinary nonfatal failure from admitted reconciliation work produces one adapter `Error` record with the original exception, the `PackageChangeSet.CorrelationId`, and the `OnPackagesReconciledAsync` operation name. The adapter then rethrows the same exception so Nuplane retains its observer warning and continuation behavior. Requested-token cancellation, the documented fatal exceptions, build-time refresh failures, and returned per-shell `ReloadResult.Error` values remain outside this diagnostic boundary. With no logging provider, the adapter uses `NullLogger` and preserves the same failure propagation. + +As with the framework's other logging consumers, exception-preserving diagnostics assume registered logging providers do not throw from `ILogger.Log`; logger-provider failures are outside this adapter contract. + ## Refresh cold shells before construction When a package completion arrives with no active shell, the adapter records catalog freshness and returns without scanning assemblies, activating a shell, or reloading. The next requested shell build consumes pending freshness before CShells initializes or reads the feature catalog. A successful refresh is committed even if later shell initialization fails, so the next build can reuse that fresh snapshot. diff --git a/tests/CShells.Tests/CShells.Tests.csproj b/tests/CShells.Tests/CShells.Tests.csproj index 2ff74cf..d379391 100644 --- a/tests/CShells.Tests/CShells.Tests.csproj +++ b/tests/CShells.Tests/CShells.Tests.csproj @@ -16,6 +16,7 @@ + diff --git a/tests/CShells.Tests/Integration/Nuplane/NuplaneCompositionTests.cs b/tests/CShells.Tests/Integration/Nuplane/NuplaneCompositionTests.cs index b3bd7f3..629ee35 100644 --- a/tests/CShells.Tests/Integration/Nuplane/NuplaneCompositionTests.cs +++ b/tests/CShells.Tests/Integration/Nuplane/NuplaneCompositionTests.cs @@ -4,6 +4,8 @@ using CShells.Lifecycle; using CShells.Nuplane; using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.DependencyInjection.Extensions; +using Microsoft.Extensions.Logging; using Nuplane.Abstractions; using Nuplane.Loading; @@ -194,6 +196,47 @@ public void ResolvingTheCoordinatorDoesNotEagerlyResolveTheShellRegistry() Assert.Equal(0, registryResolutions); } + [Fact] + public async Task CoordinatorFactoryUsesTheOptionalLoggerFromTheLoggerFactory() + { + var loggerProvider = new NuplaneTestLoggerProvider(); + var refreshFailure = new InvalidOperationException("factory logger wiring failure"); + var catalog = new TestRuntimeFeatureCatalog + { + RefreshHandler = (_, _) => Task.FromException(refreshFailure) + }; + var registry = TestShellRegistry.Create(out _); + var services = NuplaneCoordinatorTestCases.CreateAdapterServices(catalog, registry, loggerProvider); + + await using var provider = services.BuildServiceProvider(); + var observer = provider.GetRequiredService(); + var thrown = await Assert.ThrowsAsync(() => NuplaneCoordinatorTestCases.NotifyAsync(observer)); + + Assert.Same(refreshFailure, thrown); + NuplaneCoordinatorTestCases.AssertAdapterError(loggerProvider, refreshFailure, "test"); + } + + [Fact] + public async Task MissingLoggingProviderUsesNullLoggerAndPreservesFailurePropagation() + { + var refreshFailure = new InvalidOperationException("logger is optional"); + var catalog = new TestRuntimeFeatureCatalog + { + RefreshHandler = (_, _) => Task.FromException(refreshFailure) + }; + var registry = TestShellRegistry.Create(out _); + var services = NuplaneCoordinatorTestCases.CreateAdapterServices(catalog, registry); + + await using var provider = services.BuildServiceProvider(); + Assert.Null(provider.GetService()); + var observer = provider.GetRequiredService(); + + var thrown = await Assert.ThrowsAsync(() => NuplaneCoordinatorTestCases.NotifyAsync(observer)); + + Assert.Same(refreshFailure, thrown); + Assert.Equal(1, catalog.RefreshCount); + } + [Fact] public async Task AdapterObserverIsAppendedAndUnrelatedObserversKeepTheirOwnShellLifetime() { diff --git a/tests/CShells.Tests/Integration/Nuplane/NuplaneCoordinatorTestHarness.cs b/tests/CShells.Tests/Integration/Nuplane/NuplaneCoordinatorTestHarness.cs index b1c7aa2..6b021ac 100644 --- a/tests/CShells.Tests/Integration/Nuplane/NuplaneCoordinatorTestHarness.cs +++ b/tests/CShells.Tests/Integration/Nuplane/NuplaneCoordinatorTestHarness.cs @@ -1,9 +1,14 @@ using System.Reflection; +using System.Runtime.ExceptionServices; +using CShells.DependencyInjection; using CShells.Features; using CShells.Lifecycle; +using CShells.Nuplane; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.DependencyInjection.Extensions; +using Microsoft.Extensions.Logging; using Microsoft.Extensions.Options; using Nuplane.Abstractions; -using System.Runtime.ExceptionServices; namespace CShells.Tests.Integration.Nuplane; @@ -88,6 +93,8 @@ public class TestShellRegistry : DispatchProxy public Action? ActiveShellsRead { get; set; } + public Func>? ActiveShellsHandler { get; set; } + public static IShellRegistry Create(out TestShellRegistry state) { var registry = DispatchProxy.Create(); @@ -101,7 +108,9 @@ public static IShellRegistry Create(out TestShellRegistry state) { var activeRead = Interlocked.Increment(ref _activeReadCount); ActiveShellsRead?.Invoke(activeRead); - return HasActiveShell ? new IShell[] { null! } : Array.Empty(); + return ActiveShellsHandler is { } activeShellsHandler + ? activeShellsHandler(activeRead) + : HasActiveShell ? new IShell[] { null! } : Array.Empty(); } if (targetMethod?.Name == nameof(IShellRegistry.ReloadActiveAsync)) @@ -119,6 +128,38 @@ public static IShellRegistry Create(out TestShellRegistry state) internal static class NuplaneCoordinatorTestCases { + public static ServiceCollection CreateAdapterServices( + IRuntimeFeatureCatalog catalog, + IShellRegistry registry, + NuplaneTestLoggerProvider? loggerProvider = null) + { + var services = new ServiceCollection(); + if (loggerProvider is not null) + { + services.AddLogging(logging => + { + logging.ClearProviders(); + logging.AddProvider(loggerProvider); + }); + } + + services.AddCShells(shells => shells.WithNuplaneFeatureDiscovery()); + services.RemoveAll(); + services.AddSingleton(catalog); + services.RemoveAll(); + services.AddSingleton(registry); + return services; + } + + public static void AssertAdapterError(NuplaneTestLoggerProvider loggerProvider, Exception exception, string correlationId) + { + var entry = Assert.Single(loggerProvider.Snapshot()); + Assert.Equal(LogLevel.Error, entry.Level); + Assert.Same(exception, entry.Exception); + Assert.Equal(nameof(INuplaneObserver.OnPackagesReconciledAsync), entry.Properties["Operation"]); + Assert.Equal(correlationId, entry.Properties["CorrelationId"]); + } + public static Task NotifyAsync( INuplaneObserver observer, bool sourceChanged = true, diff --git a/tests/CShells.Tests/Integration/Nuplane/NuplaneLiveOptionsTests.cs b/tests/CShells.Tests/Integration/Nuplane/NuplaneLiveOptionsTests.cs index dc572fc..a9a6ec1 100644 --- a/tests/CShells.Tests/Integration/Nuplane/NuplaneLiveOptionsTests.cs +++ b/tests/CShells.Tests/Integration/Nuplane/NuplaneLiveOptionsTests.cs @@ -186,9 +186,10 @@ public async Task OptionsValidationOrInvalidTrigger_FailsBeforeCatalogWorkIsReco typeof(NuplaneIntegrationOptions), ["The configured options were rejected."]); var monitor = new TestOptionsMonitor(new NuplaneIntegrationOptions()); + var loggerProvider = new NuplaneTestLoggerProvider(); var catalog = new TestRuntimeFeatureCatalog(); var registry = TestShellRegistry.Create(out var registryState); - var coordinator = new NuplaneRefreshCoordinator(catalog, monitor, () => registry); + var coordinator = new NuplaneRefreshCoordinator(catalog, monitor, () => registry, loggerProvider.CreateTypedLogger()); monitor.ThrowOnRead(monitorReadFailure); var monitorThrown = await Assert.ThrowsAsync(() => NuplaneCoordinatorTestCases.NotifyAsync(coordinator)); @@ -217,6 +218,7 @@ public async Task OptionsValidationOrInvalidTrigger_FailsBeforeCatalogWorkIsReco Assert.Equal(0, catalog.RefreshCount); Assert.Equal(3, registryState.ActiveShellReadCount); Assert.Equal(0, registryState.ReloadCount); + Assert.Empty(loggerProvider.Snapshot()); } [Fact(DisplayName = "Configuration reload changes the policy used by the registered Nuplane observer")] diff --git a/tests/CShells.Tests/Integration/Nuplane/NuplaneObserverDiagnosticsTests.cs b/tests/CShells.Tests/Integration/Nuplane/NuplaneObserverDiagnosticsTests.cs new file mode 100644 index 0000000..e9700ce --- /dev/null +++ b/tests/CShells.Tests/Integration/Nuplane/NuplaneObserverDiagnosticsTests.cs @@ -0,0 +1,83 @@ +using CShells.Lifecycle; +using CShells.Nuplane; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Logging; +using Nuplane.Abstractions; +using Nuplane.Events; +using Nuplane.Observability; + +namespace CShells.Tests.Integration.Nuplane; + +public sealed class NuplaneObserverDiagnosticsTests +{ + [Fact] + public async Task RealDispatcherLogsAdapterErrorThenWarningAndContinuesToFollowingObserver() + { + var loggerProvider = new NuplaneTestLoggerProvider(); + var refreshFailure = new InvalidOperationException("dispatcher refresh failure"); + var catalog = new TestRuntimeFeatureCatalog + { + RefreshHandler = (number, _) => number == 1 + ? Task.FromException(refreshFailure) + : Task.CompletedTask + }; + var registry = TestShellRegistry.Create(out _); + var followingObserver = new FollowingObserver(); + var services = NuplaneCoordinatorTestCases.CreateAdapterServices(catalog, registry, loggerProvider); + services.AddSingleton(followingObserver); + services.AddSingleton(); + services.AddSingleton(provider => provider.GetRequiredService()); + services.AddSingleton(); + services.AddSingleton(provider => provider.GetRequiredService()); + + await using var provider = services.BuildServiceProvider(); + var dispatcher = provider.GetRequiredService(); + var applied = FakePackageAssemblyCatalog.Resolved("feature-package"); + const string correlationId = "dispatcher-diagnostic-correlation"; + var changeSet = new PackageChangeSet([applied], [], [], correlationId, DateTimeOffset.UtcNow); + + await dispatcher.PublishReconciledAsync(changeSet, [applied], CancellationToken.None).WaitAsync(TimeSpan.FromSeconds(5)); + + Assert.Equal(1, catalog.RefreshCount); + Assert.Equal(1, followingObserver.ReconciledCalls); + var firstLogs = loggerProvider.Snapshot().ToArray(); + var error = Assert.Single(firstLogs, entry => entry.Level == LogLevel.Error); + var warning = Assert.Single(firstLogs, entry => entry.Level == LogLevel.Warning); + Assert.Same(refreshFailure, error.Exception); + Assert.Equal(nameof(INuplaneObserver.OnPackagesReconciledAsync), error.Properties["Operation"]); + Assert.Equal(correlationId, error.Properties["CorrelationId"]); + Assert.Equal(correlationId, warning.Properties["CorrelationId"]); + Assert.Contains(refreshFailure.Message, warning.Message, StringComparison.Ordinal); + Assert.True(Array.IndexOf(firstLogs, error) < Array.IndexOf(firstLogs, warning)); + + var retry = new PackageChangeSet([], [], [], "dispatcher-retry", DateTimeOffset.UtcNow); + await dispatcher.PublishReconciledAsync(retry, [applied], CancellationToken.None).WaitAsync(TimeSpan.FromSeconds(5)); + + Assert.Equal(2, catalog.RefreshCount); + Assert.Equal(2, followingObserver.ReconciledCalls); + Assert.Single(loggerProvider.Snapshot(), entry => entry.Level == LogLevel.Error); + Assert.Single(loggerProvider.Snapshot(), entry => entry.Level == LogLevel.Warning); + } + + private sealed class FollowingObserver : INuplaneObserver + { + private int _reconciledCalls; + + public int ReconciledCalls => _reconciledCalls; + + public Task OnPackagesChangingAsync(PackageChangeSet changeSet, CancellationToken cancellationToken) => Task.CompletedTask; + + public Task OnPackagesChangedAsync(PackageChangeSet changeSet, CancellationToken cancellationToken) => Task.CompletedTask; + + public Task OnPackageFailedAsync(string packageId, Exception exception, CancellationToken cancellationToken) => Task.CompletedTask; + + public Task OnPackagesReconciledAsync( + PackageChangeSet changeSet, + IReadOnlyList appliedPackages, + CancellationToken cancellationToken) + { + Interlocked.Increment(ref _reconciledCalls); + return Task.CompletedTask; + } + } +} diff --git a/tests/CShells.Tests/Integration/Nuplane/NuplaneRefreshCoordinatorTests.cs b/tests/CShells.Tests/Integration/Nuplane/NuplaneRefreshCoordinatorTests.cs index 8a3ea36..e98b249 100644 --- a/tests/CShells.Tests/Integration/Nuplane/NuplaneRefreshCoordinatorTests.cs +++ b/tests/CShells.Tests/Integration/Nuplane/NuplaneRefreshCoordinatorTests.cs @@ -168,23 +168,152 @@ await NuplaneCoordinatorTestCases.RunWithGateCleanupAsync( [Fact] public async Task FailedRefreshRetainsFreshnessForAnUnchangedEligibleRetry() { + var refreshFailure = new InvalidOperationException("refresh failed"); + var loggerProvider = new NuplaneTestLoggerProvider(); var catalog = new TestRuntimeFeatureCatalog { RefreshHandler = (number, _) => number == 1 - ? Task.FromException(new InvalidOperationException("refresh failed")) + ? Task.FromException(refreshFailure) : Task.CompletedTask }; var registry = TestShellRegistry.Create(out _); - var coordinator = new NuplaneRefreshCoordinator(catalog, new TestOptionsMonitor(new NuplaneIntegrationOptions()), () => registry); + var coordinator = new NuplaneRefreshCoordinator(catalog, new TestOptionsMonitor(new NuplaneIntegrationOptions()), () => registry, loggerProvider.CreateTypedLogger()); + var correlationId = "refresh-failure-correlation"; + var applied = FakePackageAssemblyCatalog.Resolved("feature-package"); + var changeSet = new PackageChangeSet([applied], [], [], correlationId, DateTimeOffset.UtcNow); - var exception = await Assert.ThrowsAsync(() => NuplaneCoordinatorTestCases.NotifyAsync(coordinator)); + var exception = await Assert.ThrowsAsync(() => coordinator.OnPackagesReconciledAsync(changeSet, [applied], CancellationToken.None)); + Assert.Same(refreshFailure, exception); Assert.Equal("refresh failed", exception.Message); + NuplaneCoordinatorTestCases.AssertAdapterError(loggerProvider, refreshFailure, correlationId); await NuplaneCoordinatorTestCases.NotifyAsync(coordinator, sourceChanged: false); Assert.Equal(2, catalog.RefreshCount); + Assert.Single(loggerProvider.Snapshot()); + } + + [Fact] + public async Task BuildOnlyRefreshFailureRemainsUnloggedAndUnchanged() + { + var refreshFailure = new InvalidOperationException("build refresh failed"); + var loggerProvider = new NuplaneTestLoggerProvider(); + var catalog = new TestRuntimeFeatureCatalog + { + RefreshHandler = (_, _) => Task.FromException(refreshFailure) + }; + var registry = TestShellRegistry.Create(out var registryState); + registryState.HasActiveShell = false; + var coordinator = new NuplaneRefreshCoordinator( + catalog, + new TestOptionsMonitor(new NuplaneIntegrationOptions()), + () => registry, + loggerProvider.CreateTypedLogger()); + await NuplaneCoordinatorTestCases.NotifyAsync(coordinator); + var context = new ShellGenerationBuildContext(ShellDescriptor.Create("build-only", 1), new ShellId("build-only")); + + var thrown = await Assert.ThrowsAsync(() => coordinator.BeginAsync(context).AsTask()); + + Assert.Same(refreshFailure, thrown); + Assert.Empty(loggerProvider.Snapshot()); } + [Fact] + public async Task RequestedCancellationDuringRefreshIsNotLoggedAndRemainsRetryable() + { + using var cancellation = new CancellationTokenSource(); + var cancellationFailure = new OperationCanceledException("caller canceled", cancellation.Token); + var loggerProvider = new NuplaneTestLoggerProvider(); + var catalog = new TestRuntimeFeatureCatalog + { + RefreshHandler = (number, _) => + { + if (number == 1) + { + cancellation.Cancel(); + return Task.FromException(cancellationFailure); + } + + return Task.CompletedTask; + } + }; + var registry = TestShellRegistry.Create(out _); + var coordinator = new NuplaneRefreshCoordinator( + catalog, + new TestOptionsMonitor(new NuplaneIntegrationOptions()), + () => registry, + loggerProvider.CreateTypedLogger()); + + var thrown = await Assert.ThrowsAnyAsync(() => NuplaneCoordinatorTestCases.NotifyAsync(coordinator, cancellationToken: cancellation.Token)); + + Assert.Same(cancellationFailure, thrown); + Assert.Empty(loggerProvider.Snapshot()); + await NuplaneCoordinatorTestCases.NotifyAsync(coordinator, sourceChanged: false); + Assert.Equal(2, catalog.RefreshCount); + } + + [Fact] + public async Task UnrequestedOperationCanceledExceptionIsLoggedAndRemainsRetryable() + { + var cancellationFailure = new OperationCanceledException("operation canceled without caller request"); + var loggerProvider = new NuplaneTestLoggerProvider(); + var catalog = new TestRuntimeFeatureCatalog + { + RefreshHandler = (number, _) => number == 1 + ? Task.FromException(cancellationFailure) + : Task.CompletedTask + }; + var registry = TestShellRegistry.Create(out _); + var coordinator = new NuplaneRefreshCoordinator( + catalog, + new TestOptionsMonitor(new NuplaneIntegrationOptions()), + () => registry, + loggerProvider.CreateTypedLogger()); + const string correlationId = "uncanceled-oce-correlation"; + var applied = FakePackageAssemblyCatalog.Resolved("feature-package"); + var changeSet = new PackageChangeSet([applied], [], [], correlationId, DateTimeOffset.UtcNow); + + var thrown = await Assert.ThrowsAnyAsync( + () => coordinator.OnPackagesReconciledAsync(changeSet, [applied], CancellationToken.None)); + + Assert.Same(cancellationFailure, thrown); + NuplaneCoordinatorTestCases.AssertAdapterError(loggerProvider, cancellationFailure, correlationId); + await NuplaneCoordinatorTestCases.NotifyAsync(coordinator, sourceChanged: false); + Assert.Equal(2, catalog.RefreshCount); + Assert.Single(loggerProvider.Snapshot()); + } + + [Theory] + [MemberData(nameof(FatalExceptionCases))] + public async Task FatalExceptionsAreNotLogged(Exception fatalException) + { + var loggerProvider = new NuplaneTestLoggerProvider(); + var catalog = new TestRuntimeFeatureCatalog + { + RefreshHandler = (_, _) => Task.FromException(fatalException) + }; + var registry = TestShellRegistry.Create(out _); + var coordinator = new NuplaneRefreshCoordinator( + catalog, + new TestOptionsMonitor(new NuplaneIntegrationOptions()), + () => registry, + loggerProvider.CreateTypedLogger()); + + var thrown = await Assert.ThrowsAnyAsync(() => NuplaneCoordinatorTestCases.NotifyAsync(coordinator)); + + Assert.Same(fatalException, thrown); + Assert.Empty(loggerProvider.Snapshot()); + } + + public static IEnumerable FatalExceptionCases => + [ + [new OutOfMemoryException("fatal")], + [new StackOverflowException("fatal")], + [new AccessViolationException("fatal")], + [new AppDomainUnloadedException("fatal")], + [new BadImageFormatException("fatal")] + ]; + [Fact] public async Task FailedBuildRefreshRetainsFreshnessForTheNextBuildAttempt() { diff --git a/tests/CShells.Tests/Integration/Nuplane/NuplaneReloadResultsTests.cs b/tests/CShells.Tests/Integration/Nuplane/NuplaneReloadResultsTests.cs index 8ea9ef7..2d6eb2e 100644 --- a/tests/CShells.Tests/Integration/Nuplane/NuplaneReloadResultsTests.cs +++ b/tests/CShells.Tests/Integration/Nuplane/NuplaneReloadResultsTests.cs @@ -12,6 +12,7 @@ public sealed class NuplaneReloadResultsTests [Fact] public async Task PartialErrorsReachCallbackUnchangedAndReloadRetriesWithoutAnotherCatalogScan() { + var loggerProvider = new NuplaneTestLoggerProvider(); var refusal = new InvalidOperationException("host refusal"); var nested = new ApplicationException("shell activation failed", refusal); IReadOnlyList? callbackResults = null; @@ -26,7 +27,7 @@ public async Task PartialErrorsReachCallbackUnchangedAndReloadRetriesWithoutAnot return ValueTask.CompletedTask; } }; - var (coordinator, catalog, registry, registryState) = CreateCoordinator(options); + var (coordinator, catalog, registry, registryState) = CreateCoordinator(options, loggerProvider); var success = new ReloadResult("successful-shell", null, null, null); var failed = new ReloadResult("refused-shell", null, null, nested); registryState.Results = [success, failed]; @@ -48,11 +49,13 @@ public async Task PartialErrorsReachCallbackUnchangedAndReloadRetriesWithoutAnot Assert.Equal(1, catalog.RefreshCount); Assert.Equal(2, registryState.ReloadCount); Assert.Equal(2, callbackCount); + Assert.Empty(loggerProvider.Snapshot()); } [Fact] public async Task CallbackFailureRetainsReloadWorkAndOriginalException() { + var loggerProvider = new NuplaneTestLoggerProvider(); var callbackFault = new InvalidOperationException("result adapter failed"); var callbackCount = 0; var options = new NuplaneIntegrationOptions @@ -66,10 +69,11 @@ public async Task CallbackFailureRetainsReloadWorkAndOriginalException() return ValueTask.CompletedTask; } }; - var (coordinator, catalog, registry, registryState) = CreateCoordinator(options); + var (coordinator, catalog, registry, registryState) = CreateCoordinator(options, loggerProvider); var thrown = await Assert.ThrowsAsync(() => NuplaneCoordinatorTestCases.NotifyAsync(coordinator)); Assert.Same(callbackFault, thrown); + NuplaneCoordinatorTestCases.AssertAdapterError(loggerProvider, callbackFault, "test"); await NuplaneCoordinatorTestCases.NotifyAsync(coordinator, sourceChanged: false); @@ -77,11 +81,13 @@ public async Task CallbackFailureRetainsReloadWorkAndOriginalException() Assert.Equal(2, registryState.ReloadCount); Assert.Equal(2, callbackCount); Assert.Single(registryState.Results); + Assert.Single(loggerProvider.Snapshot()); } [Fact] public async Task ThrownRegistryFailureEscapesUnchangedAndDoesNotInvokeResultCallback() { + var loggerProvider = new NuplaneTestLoggerProvider(); var registryFault = new InvalidOperationException("registry call failed"); var callbackCount = 0; var options = new NuplaneIntegrationOptions @@ -93,13 +99,14 @@ public async Task ThrownRegistryFailureEscapesUnchangedAndDoesNotInvokeResultCal return ValueTask.CompletedTask; } }; - var (coordinator, catalog, registry, registryState) = CreateCoordinator(options); + var (coordinator, catalog, registry, registryState) = CreateCoordinator(options, loggerProvider); registryState.ReloadHandler = (number, _) => number == 1 ? Task.FromException>(registryFault) : Task.FromResult>([new ReloadResult("test", null, null, null)]); var thrown = await Assert.ThrowsAsync(() => NuplaneCoordinatorTestCases.NotifyAsync(coordinator)); Assert.Same(registryFault, thrown); + NuplaneCoordinatorTestCases.AssertAdapterError(loggerProvider, registryFault, "test"); Assert.Equal(0, callbackCount); await NuplaneCoordinatorTestCases.NotifyAsync(coordinator, sourceChanged: false); @@ -107,6 +114,60 @@ public async Task ThrownRegistryFailureEscapesUnchangedAndDoesNotInvokeResultCal Assert.Equal(1, catalog.RefreshCount); Assert.Equal(2, registryState.ReloadCount); Assert.Equal(1, callbackCount); + Assert.Single(loggerProvider.Snapshot()); + } + + [Fact] + public async Task RegistryFactoryFailureIsLoggedAndLeavesCatalogWorkRetryable() + { + var registryFailure = new InvalidOperationException("registry factory failed"); + var loggerProvider = new NuplaneTestLoggerProvider(); + var catalog = new TestRuntimeFeatureCatalog(); + var registry = TestShellRegistry.Create(out _); + var factoryCalls = 0; + var coordinator = new NuplaneRefreshCoordinator( + catalog, + new TestOptionsMonitor(new NuplaneIntegrationOptions()), + () => Interlocked.Increment(ref factoryCalls) == 1 ? throw registryFailure : registry, + loggerProvider.CreateTypedLogger()); + var applied = FakePackageAssemblyCatalog.Resolved("feature-package"); + var changeSet = new PackageChangeSet([applied], [], [], "factory-failure-correlation", DateTimeOffset.UtcNow); + + var thrown = await Assert.ThrowsAsync( + () => coordinator.OnPackagesReconciledAsync(changeSet, [applied], CancellationToken.None)); + + Assert.Same(registryFailure, thrown); + NuplaneCoordinatorTestCases.AssertAdapterError(loggerProvider, registryFailure, "factory-failure-correlation"); + Assert.Equal(0, catalog.RefreshCount); + await NuplaneCoordinatorTestCases.NotifyAsync(coordinator, sourceChanged: false); + Assert.Equal(1, catalog.RefreshCount); + Assert.Single(loggerProvider.Snapshot()); + } + + [Fact] + public async Task ActiveShellRegistryFailureIsLoggedAndLeavesCatalogWorkRetryable() + { + var registryFailure = new InvalidOperationException("active shell lookup failed"); + var loggerProvider = new NuplaneTestLoggerProvider(); + var catalog = new TestRuntimeFeatureCatalog(); + var registry = TestShellRegistry.Create(out var registryState); + registryState.ActiveShellsHandler = number => number == 1 + ? throw registryFailure + : [null!]; + var coordinator = new NuplaneRefreshCoordinator( + catalog, + new TestOptionsMonitor(new NuplaneIntegrationOptions()), + () => registry, + loggerProvider.CreateTypedLogger()); + + var thrown = await Assert.ThrowsAsync(() => NuplaneCoordinatorTestCases.NotifyAsync(coordinator)); + + Assert.Same(registryFailure, thrown); + NuplaneCoordinatorTestCases.AssertAdapterError(loggerProvider, registryFailure, "test"); + Assert.Equal(0, catalog.RefreshCount); + await NuplaneCoordinatorTestCases.NotifyAsync(coordinator, sourceChanged: false); + Assert.Equal(1, catalog.RefreshCount); + Assert.Single(loggerProvider.Snapshot()); } [Fact] @@ -175,11 +236,12 @@ public async Task ResultCallbackCanRequestManualReloadAfterAutomaticReloadReleas } private static (NuplaneRefreshCoordinator Coordinator, TestRuntimeFeatureCatalog Catalog, IShellRegistry Registry, TestShellRegistry RegistryState) - CreateCoordinator(NuplaneIntegrationOptions options) + CreateCoordinator(NuplaneIntegrationOptions options, NuplaneTestLoggerProvider? loggerProvider = null) { var catalog = new TestRuntimeFeatureCatalog(); var registry = TestShellRegistry.Create(out var registryState); - var coordinator = new NuplaneRefreshCoordinator(catalog, new TestOptionsMonitor(options), () => registry); + var coordinator = new NuplaneRefreshCoordinator(catalog, new TestOptionsMonitor(options), () => registry, loggerProvider?.CreateTypedLogger()); return (coordinator, catalog, registry, registryState); } + } diff --git a/tests/CShells.Tests/Integration/Nuplane/NuplaneTestLoggerProvider.cs b/tests/CShells.Tests/Integration/Nuplane/NuplaneTestLoggerProvider.cs new file mode 100644 index 0000000..38745e1 --- /dev/null +++ b/tests/CShells.Tests/Integration/Nuplane/NuplaneTestLoggerProvider.cs @@ -0,0 +1,73 @@ +using System.Collections.Concurrent; +using Microsoft.Extensions.Logging; + +namespace CShells.Tests.Integration.Nuplane; + +internal sealed class NuplaneTestLoggerProvider : ILoggerProvider +{ + private readonly ConcurrentQueue _entries = new(); + + public IReadOnlyList Snapshot() => _entries.ToArray(); + + public ILogger CreateLogger(string categoryName) => new CapturingLogger(this, categoryName); + + public ILogger CreateTypedLogger() => new CapturingLogger(this); + + public void Dispose() + { + } + + private void Add(NuplaneTestLogEntry entry) => _entries.Enqueue(entry); + + private sealed class CapturingLogger(NuplaneTestLoggerProvider owner, string categoryName) : ILogger + { + public IDisposable? BeginScope(TState state) where TState : notnull => NullScope.Instance; + + public bool IsEnabled(LogLevel logLevel) => logLevel != LogLevel.None; + + public void Log( + LogLevel logLevel, + EventId eventId, + TState state, + Exception? exception, + Func formatter) + { + if (!IsEnabled(logLevel)) + return; + + var properties = state is IEnumerable> values + ? values.ToDictionary(pair => pair.Key, pair => pair.Value, StringComparer.Ordinal) + : new Dictionary(StringComparer.Ordinal); + owner.Add(new NuplaneTestLogEntry(categoryName, logLevel, eventId, exception, properties, formatter(state, exception))); + } + } + + private sealed class CapturingLogger(NuplaneTestLoggerProvider owner) : ILogger + { + private readonly CapturingLogger innerLogger = new(owner, typeof(T).FullName ?? typeof(T).Name); + + public IDisposable? BeginScope(TState state) where TState : notnull => innerLogger.BeginScope(state); + + public bool IsEnabled(LogLevel logLevel) => innerLogger.IsEnabled(logLevel); + + public void Log(LogLevel logLevel, EventId eventId, TState state, Exception? exception, Func formatter) => + innerLogger.Log(logLevel, eventId, state, exception, formatter); + } + + private sealed class NullScope : IDisposable + { + public static NullScope Instance { get; } = new(); + + public void Dispose() + { + } + } +} + +internal sealed record NuplaneTestLogEntry( + string Category, + LogLevel Level, + EventId EventId, + Exception? Exception, + IReadOnlyDictionary Properties, + string Message); From f4024e62855fa20ba5e8f35936764e2dd1f06d3c Mon Sep 17 00:00:00 2001 From: Sipke Schoorstra Date: Fri, 9 Oct 2026 01:11:27 +0200 Subject: [PATCH 3/3] docs: record adapter source verification --- specs/024-nuplane-failure-diagnostics/tasks.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/specs/024-nuplane-failure-diagnostics/tasks.md b/specs/024-nuplane-failure-diagnostics/tasks.md index a7b00d9..52e7cbc 100644 --- a/specs/024-nuplane-failure-diagnostics/tasks.md +++ b/specs/024-nuplane-failure-diagnostics/tasks.md @@ -40,9 +40,9 @@ **Purpose**: Prove the implementation against the focused source tests and supported package surfaces. -- [ ] T012 Run focused Nuplane tests and the full `tests/CShells.Tests/CShells.Tests.csproj` Release suite; record exact commands and results in the issue's evidence before closure. +- [X] T012 Run focused Nuplane tests and the full `tests/CShells.Tests/CShells.Tests.csproj` Release suite; record exact commands and results in the issue's evidence before closure. - [X] T013 Build `src/CShells.Nuplane/CShells.Nuplane.csproj` in Release for `net8.0`, `net9.0`, and `net10.0`; retain exact command results. -- [ ] T014 In a disposable copy of `src/CShells.Nuplane/Internal/NuplaneRefreshCoordinator.cs`, remove the diagnostics path and compile/run the focused diagnostic tests to prove they detect the regression; restore the intended file and verify no mutation residue. +- [X] T014 In a disposable copy of `src/CShells.Nuplane/Internal/NuplaneRefreshCoordinator.cs`, remove the diagnostics path and compile/run the focused diagnostic tests to prove they detect the regression; restore the intended file and verify no mutation residue. - [ ] T015 Open the normal organization PR after root and independent review, pass required PR CI/review gates, merge under repository policy, and verify the `.github/workflows/publish.yml` main-branch gate and owner-produced preview publication before treating package artifacts as acceptance evidence. - [ ] T016 After the owner-published corrective preview is available, audit all ten public CShells package archives against successful workflow metadata and feed bytes, then run the PackageReference-only diagnostic consumer on actual .NET 8, .NET 9, and .NET 10 runtimes using `public-consumer-plan.md` in the modular-hosting evidence artifact workspace. Keep this proof distinct from the accepted pre-correction counterexample; do not manually publish or promote a stable release as part of this task.