don
/
neon-sprawl
mirror of https://github.com/ViPro-Technologies/neon-sprawl
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

NEON-15: add implementation plan for NpgsqlDataSource shutdown

pull/32/head
VinPropane 2026-04-08 21:16:50 -04:00
parent d77462012c
commit abffd29299
1 changed files with 70 additions and 0 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

70
docs/plans/NEON-15-implementation-plan.md 100644
View File

@ -0,0 +1,70 @@
# NEON-15 — Implementation plan
## Story reference
| Field | Value |
|--------|--------|
| **Key** | NEON-15 |
| **Title** | Dispose NpgsqlDataSource on application shutdown |
| **Jira** | [NEON-15](https://neon-sprawl.atlassian.net/browse/NEON-15) |
| **Parent** | [NEON-13 — Tech Debt](https://neon-sprawl.atlassian.net/browse/NEON-13) |
| **Related** | NS-17 code review — [docs/reviews/2026-03-30-NS-17.md](../reviews/2026-03-30-NS-17.md) (suggestion #2) |
## Goal, scope, and out-of-scope
**Goal:** Ensure the shared `NpgsqlDataSource` registered when `ConnectionStrings:NeonSprawl` is set is **released cleanly on host shutdown**, without breaking **in-memory** overrides or **Postgres** integration tests / `WebApplicationFactory`.
**In scope**
- Explicit, observable shutdown cleanup for the PostgreSQL path (align with Npgsql 10 + ASP.NET Core hosting patterns; no new analyzer warnings).
- Keep **single** shared data source for `PostgresPositionStateStore` and `PostgresDevPlayerSeedHostedService`.
- Update test host overrides that strip Postgres registrations so they remain consistent.
**Out of scope**
- Connection pooling tuning, migration tooling, or schema changes.
- Client / Godot code.
## Acceptance criteria checklist
- [ ] Graceful shutdown disposes the shared `NpgsqlDataSource` (or a documented alternative if upstream guidance prefers another pattern).
- [ ] `dotnet test` still passes (in-memory and Postgres integration paths).
- [ ] No new analyzer warnings; align with Npgsql / ASP.NET Core guidance for the installed package version.
## Technical approach
1. **Shutdown ordering:** `IHostedService.StopAsync` runs in **reverse registration order**. Register a small internal **`IHostedService` 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()` (with `ConfigureAwait(false)` if the file’s style uses it elsewhere). `StartAsync` is a no-op.
3. **Double disposal:** The root `ServiceProvider` may also dispose `IDisposable`/`IAsyncDisposable` singletons when the host tears down. **Verify** whether Npgsql 10’s `NpgsqlDataSource` tolerates a second dispose; if not, fall back to a **non-disposable holder** singleton that owns the `NpgsqlDataSource` and is the **only** component that disposes it (inject the holder or an abstraction into `PostgresPositionStateStore` and `PostgresDevPlayerSeedHostedService`). Prefer the minimal hosted-service-only change if tests and a local run show clean shutdown.
4. **Registration clarity:** Use an explicit `AddSingleton<NpgsqlDataSource>(…)` (or `NpgsqlDataSourceBuilder` if that matches Npgsql 10 samples) so the service type is obvious 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).
## Files to add
| Path | Purpose |
|------|---------|
| `server/NeonSprawl.Server/Game/PositionState/NpgsqlDataSourceShutdownHostedService.cs` | Internal `IHostedService` that disposes the shared `NpgsqlDataSource` on shutdown (registered before the seed hosted service so it stops last). |
## Files to modify
| Path | Rationale |
|------|-----------|
| `server/NeonSprawl.Server/Game/PositionState/PositionStateServiceCollectionExtensions.cs` | Register `NpgsqlDataSource` with an explicit service type if adjusted; register the shutdown hosted service **before** `PostgresDevPlayerSeedHostedService`; keep the existing branch for in-memory vs Postgres. |
| `server/NeonSprawl.Server.Tests/InMemoryWebApplicationFactory.cs` | Extend the descriptor removal loop to drop the new shutdown hosted service when forcing the in-memory store (mirror the `PostgresDevPlayerSeedHostedService` pattern). |
| `server/NeonSprawl.Server/Game/PositionState/PostgresPositionStateStore.cs` | **Only if** the holder / non-auto-dispose pattern is required — inject the new type instead of raw `NpgsqlDataSource`. |
| `server/NeonSprawl.Server/Game/PositionState/PostgresDevPlayerSeedHostedService.cs` | **Only if** the holder pattern is required — same as store. |
## Tests
| Path / action | What to cover |
|---------------|----------------|
| `dotnet test` (full server test project) | Regression: **InMemoryWebApplicationFactory** suites unchanged; **PostgresWebApplicationFactory** / `PostgresIntegrationHarness` / `SecondProcess_ShouldReadLastPosition_AfterFirstFactoryDisposed` still pass (factory dispose + multi-factory scenario). |
| **No new test file required** for kickoff | If shutdown-only logic is covered indirectly by existing factories and `await using` tests; if implementation adds a holder or non-obvious lifecycle, add a focused test in `NeonSprawl.Server.Tests` that builds a Postgres factory, disposes it, and asserts clean teardown (optional follow-up). |
## Open questions / risks
**Double disposal** when both a shutdown hosted service and the root `ServiceProvider` dispose the same `NpgsqlDataSource` — resolve by confirming Npgsql behavior or switching to the holder pattern. **None** other if that is settled in implementation.
## PR / review
Cross-check [decomposition — server / persistence](../decomposition/README.md) if referenced for lifecycle expectations; keep **NEON-15** in commit subjects per [jira-git-naming](../../.cursor/rules/jira-git-naming.md).