71 lines
5.5 KiB
Markdown
71 lines
5.5 KiB
Markdown
# 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).
|