fix: shutdown-noise in background services + seed defaults so the app starts out of the box#279
Merged
Merged
Conversation
A host that aborts startup cancels background services mid-wait. Three services awaited a delay outside any handler for that cancellation, so the TaskCanceledException escaped ExecuteAsync. .NET treats that as a crashed BackgroundService and, under the default StopHost behaviour, logs a critical "BackgroundService failed" on top of the error that actually stopped the host — burying the real cause. AuditRetentionService was the worst case: its one-minute startup delay had no try at all, so any shutdown in the first 60 seconds of process life tripped it. The interval delay at the foot of its loop sat outside the try as well; a single outer handler now covers both. AuditWriterService and ProgressFlushService each awaited a 5s error backoff from inside a catch block, where the enclosing try cannot observe it. Those are guarded locally and now break out of the loop on shutdown. Behaviour is unchanged on every non-shutdown path, and BackgroundServiceExceptionBehavior is deliberately left at StopHost so a genuinely crashed service still stops the host. Also document the seed-password guard, which is what surfaced this: the image sets no ASPNETCORE_ENVIRONMENT, so running it outside compose inherits Production and refuses to start until Seed__AdminPassword is set. That guard was undocumented outside docker-compose.yml. Adds a README section, a .env.example (compose points at a .env that had no template), and gitignores .env — it was tracked, so a compose-created file holding an admin password would have been committable.
The admin seed previously failed host startup outside Development when Seed:AdminPassword was unconfigured, so a plain `docker run` of the image (which inherits the base image's Production default) refused to boot. Both seeded accounts now fall back to their compiled-in defaults in every environment. This also makes the login page's quick-login buttons work as advertised: they hardcode [email protected]/Admin123! and [email protected]/User123!, and the Users:ShowTestAccounts setting that renders them defaults to on everywhere — so the demo account previously being skipped outside Development left a visible button that always failed. ResolveSeedPassword collapses to "configured value, else default", which makes SeedPasswordOutcome and SeedConfigurationException dead code; both are removed. docker-compose no longer requires SEED_ADMIN_PASSWORD (it used ${VAR:?...} on both api and worker, which blocked startup independently of the C# guard). These defaults are published in this repository. Startup now logs a warning outside Development naming the config key to set, and the README, .env.example and compose comments all call out that a deployment reachable by anyone else must set Seed:AdminPassword and disable Show Test Accounts. This trades fail-closed for boots-by-default; it is a deliberate product decision, not an oversight. Verified in a container: Production with no seed password now starts, seeds both accounts with warnings, and login succeeds for both (a wrong password is still rejected, and /admin/users still redirects anonymous users).
Dockerfile.worker built on mcr.microsoft.com/dotnet/runtime:10.0, but the worker hosts the same modules as the api and every module carries a <FrameworkReference Include="Microsoft.AspNetCore.App" />. The plain runtime image does not ship that shared framework, so the worker died on every boot with "No frameworks were found" and docker compose left it restarting forever. CI never caught this: it builds only ./Dockerfile, never Dockerfile.worker, and never starts the stack — and the image builds fine, it only fails at runtime. This is necessary but not sufficient. With the correct base image the worker gets further and then fails DI validation, because it registers ASP.NET Core authorization services while running as a non-web generic host: AuthorizationPolicyCache cannot resolve EndpointDataSource. That is a separate pre-existing bug and is not addressed here.
Review follow-ups on this branch: - AuditWriterService and ProgressFlushService drained their channel one last time AFTER the loop, outside any try. That final flush routinely throws during a failed startup (the provider is already disposed — observed as ObjectDisposedException in a real container), which escaped ExecuteAsync as the very "BackgroundService failed" noise this branch exists to remove, and skipped LogStopped as well. Both are now guarded and log the error instead. - AuditRetentionService gained a LogStopped, matching its two sibling services. - UserSeedService's XML doc referenced a non-existent 'Users:ShowTestAccounts' config key. The real identifier is the auth.show_test_accounts system setting, read via ISettingsContracts — it cannot be set by environment variable, so the old wording pointed operators at a no-op. - README documents that deployments which previously set only Seed__AdminPassword will gain a [email protected] account seeded with User123! on the next restart, and corrects "outside Development" to "outside Development or Testing" to match environment.IsLocalOrTest(). The shutdown regression tests are removed rather than shipped. Three successive designs — ExecuteTask status, awaiting ExecuteTask, and asserting the service's own completion log — were all unreliable under parallel load or on CI, and one of them turned the PR's build red. A test that fails intermittently is worse than no test: it trains people to ignore CI. The fixes remain verified end to end in a container, where stopping inside and after the 60s startup-delay window both exit 0 with zero critical lines.
The upgrade note said the demo user was skipped 'outside Development', repeating the imprecision corrected four lines above it — the gate is environment.IsLocalOrTest(), which is Development or Testing.
antosubash
marked this pull request as ready for review
July 21, 2026 21:30
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Started from a container that refused to start. The startup failure was a deliberate guard working correctly — but the logs buried it under a spurious critical error, and chasing that surfaced several more problems.
1. Background services reported normal shutdown as a crash
A host that aborts startup cancels background services mid-wait. Three services awaited a delay outside any handler for that cancellation, so
TaskCanceledExceptionescapedExecuteAsync— which .NET treats as a crashedBackgroundService, logging a critical error over the real cause:AuditRetentionServicetryat all; interval delay outside thetryAuditWriterServicecatch; and the post-loop shutdown drain, outside thetryentirelyProgressFlushServiceTwo distinct traps: a delay awaited inside a
catchcan't be seen by its own enclosingtry; and the final drain sat outside the loop'stryaltogether, so a failing last flush (the provider is routinely already disposed — observed asObjectDisposedExceptionin a real container) escaped and skippedLogStopped.BackgroundServiceExceptionBehavioris deliberately left atStopHost— a genuinely crashed service should still stop the host. The fix is to stop misreporting normal shutdown, not to suppress real crashes.AuditRetentionServicealso gained aLogStopped, matching its two siblings.2. The app now starts out of the box
The admin seed previously failed host startup outside Development when
Seed:AdminPasswordwas unset, so a plaindocker run(which inherits the base image'sProduction) refused to boot.Both accounts now fall back to their compiled-in defaults everywhere. This also fixes a latent inconsistency: the login page's quick-login buttons hardcode
[email protected]/Admin123!and[email protected]/User123!, andUsers:ShowTestAccountsdefaults to on in every environment — so the demo account being skipped outside Development left a visible button that always failed.ResolveSeedPasswordcollapses to configured value, else default, makingSeedPasswordOutcomeandSeedConfigurationExceptiondead code (removed).docker-compose.ymlalso had${SEED_ADMIN_PASSWORD:?...}on bothapiandworker, blocking startup independently of the C# guard; both relaxed.Security tradeoff — deliberate
This trades fail-closed for boots-by-default, and the defaults are published in this repository. An automated scan flags it as critical, correctly. It was chosen knowingly over two safer alternatives (random-password-plus-forced-change, and fixed-default-plus-forced-change).
Compensating visibility, all non-blocking: a startup warning outside Development naming the key to set, plus README /
.env.example/ compose notes stating that any reachable deployment must setSeed:AdminPasswordand disable Show Test Accounts. The smallest future lever to reduce exposure without losing boots-by-default is defaultingShowTestAccountsoff outside Development.3. Worker image was missing the ASP.NET Core shared framework
Dockerfile.workerbuilt ondotnet/runtime:10.0, but the worker hosts the same modules as the api and every module carries a<FrameworkReference Include="Microsoft.AspNetCore.App" />. The worker died withNo frameworks were foundon every boot, leavingdocker compose upwith a permanently restarting container.CI never caught it: it builds only
./Dockerfile, neverDockerfile.worker, and never starts the stack — the image builds fine and only fails at runtime.This is necessary but not sufficient. With the right base image the worker gets further and then fails DI validation: it registers ASP.NET Core authorization while running as a non-web generic host, so
AuthorizationPolicyCachecan't resolveEndpointDataSource. Not addressed here.Known-broken, not fixed here
docker compose upstill does not work end to end, for two pre-existing reasons outside this PR's scope:InitialCreatewas generated against SQLite with hardcoded store types —CreatedAt = table.Column<DateTime>(type: "TEXT"). On PostgreSQLTEXTis a real string type, so EF resolves it toStringTypeMappingand throwsInvalidCastExceptionpushing aDateTimethrough it. Both api and worker crash-loop. Realistically needs provider-specific migrations.The SQLite path — which the image itself defaults to — is solid.
Verification
Container (SQLite, the image's own default):
/admin/usersauthenticated / anonymous / non-adminAccessDenied/health/live,/health/ready, homepageBackgroundService failed/critlinesAuditWriterService stopped,ProgressFlushService stopped,AuditRetentionService stoppedBrowser (Playwright): login page renders both quick-login buttons; Admin quick-login → fills → submit → authenticated dashboard as
[email protected]; zero console errors.Local CI:
dotnet build -warnaserror0 errors ·biome checkclean ·validate-pages74 endpoints ·typecheck15/15 · fulldotnet testgreen.A note on test coverage
There are no unit tests for the shutdown fixes. Three designs were attempted —
ExecuteTaskstatus, awaitingExecuteTask, and asserting the service's own completion log — and all proved unreliable under parallel load or on CI; one turned this PR's build red. They were removed rather than shipped, since an intermittently failing test trains people to ignore CI. The evidence for these fixes is the container verification above, where all three services now log a clean stop. Reliable coverage here likely needs an approach that doesn't driveBackgroundService's lifecycle directly.Note
mainwas 13 commits behindorigin/mainwhen this started; this branches fromorigin/main(de00b59).