From baa483a1d59bc736e32d6d5a72631882a834916b Mon Sep 17 00:00:00 2001 From: Lance Keay Date: Thu, 20 Aug 2026 12:14:53 +0100 Subject: [PATCH 1/2] Stop registering IStudentResultsClient in the worker's dependency bundle MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The rules-engine worker crash-looped at startup on main. StudentResultsBlobClient takes an IMemoryCache, and the worker has none: AddMemoryCache comes from AddPersistenceDependencies, which the worker deliberately opts out of so its manual DbContext registration stays the single source of truth. The container is validated on build, so the whole process died rather than one feature failing — the queue consumer, the dead-letter, metrics, search-analytics and content-staging retention jobs all stopped, over a service none of them uses. The registration served no host. AddInfrastructureDependencies has one production caller, the worker, which never resolves IStudentResultsClient. Every consumer — JourneyController, ResultSuggestionsController, the dev-data seeding orchestrator and SeedStudentResults — is in the web host, which does not call that bundle at all and registers the client itself in AddCpdBlobStorage. That web-side registration and its guarding test were added for the mirror image of this failure: the client registered only in Infrastructure, so the web could not boot. The web copy fixed that; this one was left behind, and it is what took the worker down. Added a guard for the class of failure rather than the instance. Constructing every registration in the bundle is what the host does at startup and what the worker died doing, so the test builds the bundle with validate-on-build and fails if anything in it cannot be constructed from what the worker provides. The Build workflow cannot catch this on its own — it compiles and runs tests, and never starts the worker host, so the failure reaches whoever runs the container next. Verified by restoring the registration: the new test fails with the same IMemoryCache error the container showed, and passes with it removed. The web host's blob-client assertions, including the one covering IStudentResultsClient, are unaffected. Refs #326 --- .../DependencyManager.cs | 10 ++- .../InfrastructureDependenciesTests.cs | 63 +++++++++++++++++++ 2 files changed, 70 insertions(+), 3 deletions(-) create mode 100644 tests/DfE.CheckPerformanceData.UnitTests/Infrastructure/InfrastructureDependenciesTests.cs diff --git a/src/DfE.CheckPerformanceData.Infrastructure/DependencyManager.cs b/src/DfE.CheckPerformanceData.Infrastructure/DependencyManager.cs index dee43a69b..874b48070 100644 --- a/src/DfE.CheckPerformanceData.Infrastructure/DependencyManager.cs +++ b/src/DfE.CheckPerformanceData.Infrastructure/DependencyManager.cs @@ -60,9 +60,13 @@ public static IServiceCollection AddInfrastructureDependencies(this IServiceColl // by every host that calls AddPersistenceDependencies — including the worker. services.AddScoped(); - // AB#296648: the 16-19 exam results a school can raise an enquiry against, held in the same - // per-window container under the results-enquiry checking-exercise prefix. - services.AddScoped(); + // IStudentResultsClient is deliberately NOT registered here. Its implementation takes an + // IMemoryCache, which this bundle's only caller — the worker — does not have: AddMemoryCache + // comes from AddPersistenceDependencies, and the worker opts out of that so its manual + // DbContext registration stays the single source of truth. Registering it here therefore + // failed validate-on-build and took the whole worker process down, consumers and retention + // jobs included, over a service the worker never resolves. Every consumer is in the web + // host, which assembles its own blob clients in AddCpdBlobStorage and registers it there. // Analytics sink: the real dfe-analytics adapter when DfeAnalytics:DatasetId is // configured (deployed envs wire it via Terraform), else a no-op so dev/review/ diff --git a/tests/DfE.CheckPerformanceData.UnitTests/Infrastructure/InfrastructureDependenciesTests.cs b/tests/DfE.CheckPerformanceData.UnitTests/Infrastructure/InfrastructureDependenciesTests.cs new file mode 100644 index 000000000..5c69867f3 --- /dev/null +++ b/tests/DfE.CheckPerformanceData.UnitTests/Infrastructure/InfrastructureDependenciesTests.cs @@ -0,0 +1,63 @@ +using DfE.CheckPerformanceData.Infrastructure; +using Microsoft.Extensions.Configuration; +using Microsoft.Extensions.DependencyInjection; + +namespace DfE.CheckPerformanceData.Application.UnitTests.Infrastructure; + +// Guards the registration bundle the rules-engine worker builds its container from. +// +// The worker is the only host that calls AddInfrastructureDependencies. The web host does not — +// it assembles its own set in AddCpdBlobStorage — so a service added to this bundle for the web's +// benefit is executed only by the worker. If its dependencies are not also in the worker's +// container, validate-on-build kills the host: the queue consumer, the dead-letter, metrics, +// search-analytics and content-staging retention jobs all stop, over a service none of them uses. +// +// That has now happened twice. The Build workflow cannot catch it, because it compiles and runs +// tests and never starts the worker, so the failure reaches whoever runs the container next. +public class InfrastructureDependenciesTests +{ + // Azurite's well-known development account. Nothing connects during registration or + // validation, so no storage emulator has to be running for this test. + private const string AzuriteConnection = + "DefaultEndpointsProtocol=http;AccountName=devstoreaccount1;" + + "AccountKey=Eby8vdM02xNOcqFlqUwJPLlmEtlCDXJ1OUzFT50uSRZ6IFsuFq2UVErCz4I6tq/K1SZFPTOtr/KBHBeksoGMGw==;" + + "BlobEndpoint=http://127.0.0.1:10000/devstoreaccount1;"; + + // Mirrors the registrations RulesEngineWorker/Program.cs makes before and after its call to + // AddInfrastructureDependencies — the collaborators the bundle is entitled to assume. Keep in + // step with that file: anything the worker stops registering has to come out of here too, or + // this test vouches for a container the worker does not actually have. + private static IServiceCollection WorkerServices() + { + var config = new ConfigurationBuilder().AddInMemoryCollection(new Dictionary + { + // The blob clients in this bundle take a BlobServiceClient, which the bundle only + // registers when a storage connection string is present. Compose and every deployed + // environment supply one, so a test without it would be checking a container shape + // no host ever has. + ["ConnectionStrings:AzureStorage"] = AzuriteConnection, + ["ZendeskSettings:Subdomain"] = "dfe", + ["ZendeskSettings:Domain"] = "zendesk", + ["ZendeskSettings:Email"] = "cypmd@education.gov.uk", + ["ZendeskSettings:ApiToken"] = "token", + }).Build(); + + var services = new ServiceCollection(); + services.AddLogging(); + services.AddInfrastructureDependencies(config); + return services; + } + + // Constructing every registration is exactly what the host does at startup, and exactly what + // the worker died doing. Asserting it here turns a crash-loop into a red test. + [Fact] + public void EveryRegistrationInTheBundle_CanBeConstructed() + { + var services = WorkerServices(); + + var exception = Record.Exception(() => services.BuildServiceProvider( + new ServiceProviderOptions { ValidateOnBuild = true, ValidateScopes = true })); + + Assert.Null(exception); + } +} From 52dc131fb83fdb9fb3949d6b0e33e038a3577bc4 Mon Sep 17 00:00:00 2001 From: Lance Keay Date: Thu, 20 Aug 2026 22:24:32 +0100 Subject: [PATCH 2/2] Make the turnaround-commitment prefill test independent of test order MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit EditPage_PrefillsCurrentValue read the value the window Summary displays and asserted the edit field held the same string. "Not set" is what the Summary prints when there is no commitment — a placeholder, not the value — so once EmptySubmission_IsAllowed_AndShowsNotSet had cleared the commitment, the test compared "Not set" against a legitimately empty input and failed: Locator expected to have value 'Not set' - unexpected value "" xUnit does not order tests within a class, so whichever order a run happened to pick decided whether this passed. It passes locally and failed the E2E job, which is the same test on the same commit. Mapping the placeholder to the empty string makes the assertion hold in both states, which is what its comment already claimed. Reproduced by running EmptySubmission first and then the prefill test — failing before the change with the error above, passing after — and confirmed the non-empty path still asserts by running Save_PersistsValue first and checking the prefill matches the value it saved. AB#296637 --- .../WindowAdmin/TurnaroundCommitmentTests.cs | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/tests/DfE.CheckPerformanceData.E2ETests/WindowAdmin/TurnaroundCommitmentTests.cs b/tests/DfE.CheckPerformanceData.E2ETests/WindowAdmin/TurnaroundCommitmentTests.cs index a7751dfe6..4c5bbb0bb 100644 --- a/tests/DfE.CheckPerformanceData.E2ETests/WindowAdmin/TurnaroundCommitmentTests.cs +++ b/tests/DfE.CheckPerformanceData.E2ETests/WindowAdmin/TurnaroundCommitmentTests.cs @@ -14,6 +14,9 @@ public sealed class TurnaroundCommitmentTests(PlaywrightFixture fixture) : Seedi // The seeded KS4 June window (see SeedCheckingWindows in DevDataSeeder). private static readonly Guid SeededWindowId = Guid.Parse("F34D285B-8660-4D12-9C30-787328DEAA0A"); + // What the Summary prints in place of an unset commitment. + private const string NotSetPlaceholder = "Not set"; + private string SummaryUrl => $"{Fixture.BaseUrl}/admin/windows/summary/{SeededWindowId}"; private string EditUrl => $"{Fixture.BaseUrl}/admin/windows/{SeededWindowId}/turnaround-commitment"; @@ -40,9 +43,16 @@ public async Task EditPage_PrefillsCurrentValue() await Page.GotoAsync(SummaryUrl); var value = await CurrentSummaryValueAsync(); + // "Not set" is what the Summary prints when there is no value — a placeholder, not the + // value itself, so the edit field is legitimately empty in that state. Comparing the two + // directly made this test depend on running before EmptySubmission_IsAllowed_AndShowsNotSet, + // which clears the commitment. xUnit does not order tests within a class, so whichever + // order a run happened to pick decided whether this passed. + var expected = value == NotSetPlaceholder ? string.Empty : value; + await Page.GotoAsync(EditUrl); var input = Page.Locator("#TurnaroundCommitment"); - await Expect(input).ToHaveValueAsync(value); + await Expect(input).ToHaveValueAsync(expected); } [Fact] @@ -68,7 +78,7 @@ public async Task EmptySubmission_IsAllowed_AndShowsNotSet() await Page.WaitForURLAsync("**/admin/windows/summary/**"); await Expect(Page.Locator(".govuk-error-summary")).ToHaveCountAsync(0); - Assert.Equal("Not set", await CurrentSummaryValueAsync()); + Assert.Equal(NotSetPlaceholder, await CurrentSummaryValueAsync()); } private async Task CurrentSummaryValueAsync()