# 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).