80 lines
6.1 KiB
Markdown
80 lines
6.1 KiB
Markdown
# NEO-13 — Implementation plan
|
||
|
||
## Story reference
|
||
|
||
| Field | Value |
|
||
|--------|--------|
|
||
| **Key** | NEO-13 |
|
||
| **Title** | Dispose NpgsqlDataSource on application shutdown |
|
||
| **Linear** | [NEO-13](https://linear.app/neon-sprawl/issue/NEO-13) |
|
||
| **Parent** | [Tech Debt](https://linear.app/neon-sprawl/project/tech-debt-d2148715e875) |
|
||
| **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.
|
||
|
||
## Decisions (planning + implementation)
|
||
|
||
| Decision | Choice |
|
||
|----------|--------|
|
||
| **Lifecycle / double dispose** | **Option 2** — internal shutdown `IHostedService` calls `DisposeAsync` on the shared `NpgsqlDataSource`; no non-disposable holder. Rationale: smallest change; **Npgsql 10** + full `dotnet test` showed clean teardown (no failures from a second dispose path). |
|
||
| **ConfigureAwait** | Not used in `StopAsync` — matches existing server hosted-service style (`PostgresDevPlayerSeedHostedService`). |
|
||
|
||
## Acceptance criteria checklist
|
||
|
||
- [x] Graceful shutdown disposes the shared `NpgsqlDataSource` (or a documented alternative if upstream guidance prefers another pattern).
|
||
- [x] `dotnet test` still passes (in-memory and Postgres integration paths).
|
||
- [x] 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 **`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`, `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. **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
|
||
|
||
| 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` | **N/A (shipped)** — holder pattern not needed after Option 2 verification. |
|
||
| `server/NeonSprawl.Server/Game/PositionState/PostgresDevPlayerSeedHostedService.cs` | **N/A (shipped)** — same. |
|
||
|
||
## 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
|
||
|
||
**Resolved:** Double-dispose risk accepted under **Option 2**; **Npgsql 10** + full test run showed no shutdown regressions. **None** outstanding for this story.
|
||
|
||
## PR / review
|
||
|
||
**Merge / PR description:** Call out **two themes** so reviewers do not miss non-server files: (1) **Server** — `NpgsqlDataSourceShutdownHostedService`, explicit `AddSingleton<NpgsqlDataSource>`, and `InMemoryWebApplicationFactory` stripping; (2) **Repo process** — [planning-implementation-docs](../../.cursor/rules/planning-implementation-docs.md) and cross-links in `AGENTS.md`, `story-kickoff`, code-review, and docs-review agent rules.
|
||
|
||
Cross-check [decomposition — server / persistence](../decomposition/README.md) if referenced for lifecycle expectations; keep **NEO-13** in commit subjects per [linear-git-naming](../../.cursor/rules/linear-git-naming.md).
|