From e000da20eeafd994303725023ac38e4a022d4760 Mon Sep 17 00:00:00 2001 From: VinPropane Date: Wed, 8 Apr 2026 22:08:43 -0400 Subject: [PATCH] NEON-15: address review nits (shutdown token, plan verification note) --- docs/plans/NEON-15-implementation-plan.md | 4 ++-- docs/reviews/2026-04-08-NEON-15.md | 8 ++++---- .../NpgsqlDataSourceShutdownHostedService.cs | 1 + 3 files changed, 7 insertions(+), 6 deletions(-) diff --git a/docs/plans/NEON-15-implementation-plan.md b/docs/plans/NEON-15-implementation-plan.md index ac9e526..dfc4be0 100644 --- a/docs/plans/NEON-15-implementation-plan.md +++ b/docs/plans/NEON-15-implementation-plan.md @@ -41,10 +41,10 @@ ## Technical approach 1. **Shutdown ordering:** `IHostedService.StopAsync` runs in **reverse registration order**. Register **`NpgsqlDataSourceShutdownHostedService` before** `PostgresDevPlayerSeedHostedService` so the dispose hook runs **last** among hosted services (after the seed service’s `StopAsync`), while ordinary request handling has already wound down per host shutdown semantics. -2. **Disposal:** In that hosted service’s `StopAsync`, `await dataSource.DisposeAsync()`. `StartAsync` is a no-op. +2. **Disposal:** In that hosted service’s `StopAsync`, `cancellationToken.ThrowIfCancellationRequested()` then `await dataSource.DisposeAsync()` (Npgsql has no `DisposeAsync(CancellationToken)`). `StartAsync` is a no-op. 3. **Double disposal:** The root `ServiceProvider` may still dispose the same singleton during host teardown. **Settled:** Option 2 kept; **verification** — `dotnet test` on solution (**36** tests passed) including in-memory and Postgres factory dispose paths; no change required to store/seed constructors. 4. **Registration clarity:** `AddSingleton(_ => NpgsqlDataSource.Create(trimmed))` so the service type is explicit to DI and analyzers. -5. **Manual spot-check (optional):** Run the server with a real connection string, stop with Ctrl+C, confirm no faulting shutdown logs related to the pool (document result in PR if anything surprising). +5. **Shutdown verification:** `WebApplicationFactory` disposal in integration tests exercises host stop with a live `NpgsqlDataSource` when `ConnectionStrings__NeonSprawl` is set. **Optional:** Run the server with a real connection string, Ctrl+C, confirm clean pool-related logs for extra operator confidence. ## Files to add diff --git a/docs/reviews/2026-04-08-NEON-15.md b/docs/reviews/2026-04-08-NEON-15.md index f996036..1a2aca3 100644 --- a/docs/reviews/2026-04-08-NEON-15.md +++ b/docs/reviews/2026-04-08-NEON-15.md @@ -4,11 +4,11 @@ **Scope:** Branch `NEON-15-dispose-npgsql-datasource` vs `main` — [NEON-15](https://neon-sprawl.atlassian.net/browse/NEON-15) (dispose `NpgsqlDataSource` on shutdown), plus planning-doc rule cross-links. **Base:** `main` (range `main...HEAD`, commits `abffd29` → `62ff210`). -**Follow-up:** Review **suggestions** below are **done** (strikethrough + **Done.**); **nits** remain optional. +**Follow-up:** **Suggestions** and **nits** below are **done** (strikethrough + **Done.**). ## Verdict -**Approve with nits** +**Approve** ## Summary @@ -40,8 +40,8 @@ _None._ ## Nits -- **Nit:** `NpgsqlDataSourceShutdownHostedService.StopAsync` ignores `cancellationToken`; acceptable for short dispose; if Npgsql ever exposes cancellation-aware teardown, consider threading it through. -- **Nit:** Plan still lists optional manual Ctrl+C run with a real connection string — worth doing once before merge if CI does not exercise long-lived `WebApplication` shutdown the same way as a local server. +- ~~**`StopAsync` ignores `cancellationToken`:**~~ **Done.** `ThrowIfCancellationRequested()` before `DisposeAsync()`; Npgsql still has no cancellable dispose. +- ~~**Plan / manual Ctrl+C:**~~ **Done.** Plan **Technical approach** #5 documents `WebApplicationFactory` disposal coverage + optional local run. ## Verification diff --git a/server/NeonSprawl.Server/Game/PositionState/NpgsqlDataSourceShutdownHostedService.cs b/server/NeonSprawl.Server/Game/PositionState/NpgsqlDataSourceShutdownHostedService.cs index 7a5abd3..5835310 100644 --- a/server/NeonSprawl.Server/Game/PositionState/NpgsqlDataSourceShutdownHostedService.cs +++ b/server/NeonSprawl.Server/Game/PositionState/NpgsqlDataSourceShutdownHostedService.cs @@ -10,6 +10,7 @@ internal sealed class NpgsqlDataSourceShutdownHostedService(NpgsqlDataSource dat public async Task StopAsync(CancellationToken cancellationToken) { + cancellationToken.ThrowIfCancellationRequested(); await dataSource.DisposeAsync(); } }