NEON-15: address review nits (shutdown token, plan verification note)

pull/32/head
VinPropane 2026-04-08 22:08:43 -04:00
parent adde98e4a8
commit e000da20ee
3 changed files with 7 additions and 6 deletions

View File

@ -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 services `StopAsync`), while ordinary request handling has already wound down per host shutdown semantics.
2. **Disposal:** In that hosted services `StopAsync`, `await dataSource.DisposeAsync()`. `StartAsync` is a no-op.
2. **Disposal:** In that hosted services `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>(_ => 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

View File

@ -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

View File

@ -10,6 +10,7 @@ internal sealed class NpgsqlDataSourceShutdownHostedService(NpgsqlDataSource dat
public async Task StopAsync(CancellationToken cancellationToken)
{
cancellationToken.ThrowIfCancellationRequested();
await dataSource.DisposeAsync();
}
}