Skip to content

Standardise the use of TimeProvider across the error instance - #5843

Open
warwickschroeder wants to merge 9 commits into
masterfrom
warwick/pattern-timeprovider
Open

Standardise the use of TimeProvider across the error instance#5843
warwickschroeder wants to merge 9 commits into
masterfrom
warwick/pattern-timeprovider

Conversation

@warwickschroeder

@warwickschroeder warwickschroeder commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Every time-sensitive decision now reads an injected clock

Timestamps across the error instance came from DateTime.UtcNow, so nothing time-dependent could be tested. They now come from a constructor-injected TimeProvider. Production gets TimeProvider.System and behaves identically; tests get a fake clock they can advance.

What is wrong today

  • The host never registered a clock. One arrived anyway, from telemetry, HTTP logging or YARP, whichever was switched on.
  • The EF persister overwrote the host's choice. BasePersistence used AddSingleton, and the container returns the last registration. A host-supplied clock was silently replaced, no error.
  • RavenDB tests faked nothing. AdvanceClock was an empty method, UtcNow returned the real one.
  • Three places read the clock twice for one instant, so two fields describing the same moment could differ by however long the code between them took: InMemoryArchive.Complete and InMemoryUnarchive.Complete (CompletionTime and Last), MessageRedirectsController.NewRedirects (the redirect's LastModified and the retry request's PeriodTo), and LicensingDataStore, where three reads built one date range so a call straddling midnight got a from and to from different days.
  • Maintenance mode dies on both EF persisters, and the clock's own test is what exposed it. Maintenance mode starts ServiceControl with storage and nothing else, so it is the one startup path that never calls AddServiceControl, which is why it is the right place to prove the persister supplies its own clock. It is also where NServiceBus's CriticalError goes missing, since that is registered only inside AddServiceControl while the EF persister's ExternalIntegrationRequestsDataStore asks for it in its constructor. So --maintenance on SQL Server or PostgreSQL exits with an unresolved-service error instead of starting. RavenDB returns early from its own registration in maintenance mode, so it never reaches the equivalent class. Not a regression: it fails the same way on master.

What it looks like afterwards

  • AddServiceControl registers TimeProvider.System with TryAddSingleton, so a host that supplies its own keeps it.
  • Both persisters do the same, so a host built from the persistence layer alone still resolves a clock, and neither persister can override the host's.
  • Maintenance mode is now RavenDB-only in the code, as it always was in the help text. It exists to start RavenDB's in-process database so RavenDB Studio can reach it, and a SQL Server or PostgreSQL instance has no such database to start; --maintenance has read Run RavenDB only - use for DB maintenance all along. IPersistenceConfiguration gained SupportsMaintenanceMode, true on RavenDB and false on the EF persisters, and PersistenceFactory turns the request down with a sentence naming the configured persister rather than letting the container throw.
  • The three double reads each read once now, tested with a clock that advances on every read.
  • RavenDB persistence tests get a real fake clock, level with SQL Server and PostgreSQL.
  • Raven: ExpirationManager computes @expires from it; ArchiveDocumentManager, UnarchiveDocumentManager, ArchivingManager and UnarchivingManager take it, with MessageArchiver passing its own down; LicensingDataStore throughput windows use it.
  • EF Core, so SQL Server and PostgreSQL: FailedMessageLifecycleDataStore stamps StatusChangedAt and LastModified from it across all five transitions; EFCoreArchivingManager and EFCoreUnarchivingManager pass it to the operation summaries.
  • API and retries: MessageRedirectsController stamps LastModified on create and update; FailureGroupsRetryController takes the operation start time from it, so the message sent and the operation opened agree; RetriesGateway bulk request types receive their start time instead of calling DateTime.UtcNow in a base constructor argument where nothing could reach it; RetryingManager, InMemoryRetry and RetryAllInGroupHandler take it.
  • Deliberately still on the machine clock: CheckRavenDBIndexLag subtracts LastIndexingTime, which the Raven server reports against its own clock, so an injected one would make the lag meaningless. Comment added so nobody "finishes the job".

Test coverage

  • Registration: a host-registered clock survives AddServiceControl, which is what catches the AddSingleton override. On RavenDB, StartupModeTests.CanRunMaintenanceMode now also asserts that the maintenance host resolves a clock of its own. On SQL Server and PostgreSQL, a new MaintenanceModeTests asserts the refusal instead, and is excluded from the RavenDB project the same way When_hosting_error_ingestion_only already is.
  • Lifecycle: five tests, one per transition, clock advanced a week, both stamps asserted.
  • Retention sweeps: seven tests. Cutoff both ways, statuses never swept, retention shrunk between runs, external body deletion limited to swept rows, group cascade, row kept when its body will not delete.
  • Archive progress: event start time and operation start time both from the clock, plus batch, finalize and complete for both directions.
  • Retry: start time from the clock, completion five fake minutes later reports exactly that.
  • Redirects: create and update both stamp it.
  • RavenDB fake clock registered before the persister, whose TryAddSingleton respects it. Caveat in the code: it moves only what the persister stamps, since the Raven server expires on its own clock.
  • 365 day retention on SQL Server and PostgreSQL. Advancing the clock wakes the live retention sweeper, which then deletes rows a test is still using. Retention now outruns any advance; tests that care set their own.
  • PersistenceTestBase no longer registers a clock, now that each backend context supplies one.

Comment thread src/ServiceControl.Persistence.EFCore/Abstractions/BasePersistence.cs Outdated
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant