# Code review: NEO-13

**Date:** 2026-04-08  
**Scope:** Branch `NEO-13-dispose-npgsql-datasource` vs `main` — [NEO-13](https://linear.app/neon-sprawl/issue/NEO-13) (dispose `NpgsqlDataSource` on shutdown), plus planning-doc rule cross-links.  
**Base:** `main` (range `main...HEAD`, commits `abffd29` → `62ff210`).

**Follow-up:** **Suggestions** and **nits** below are **done** (strikethrough + **Done.**).

## Verdict

**Approve**

## Summary

The branch adds `NpgsqlDataSourceShutdownHostedService`, registers it **before** `PostgresDevPlayerSeedHostedService` so `StopAsync` runs **last** (correct use of reverse shutdown order), switches to `AddSingleton<NpgsqlDataSource>(…)`, and extends `InMemoryWebApplicationFactory` so the new hosted service is stripped with the rest of the Postgres path. The implementation matches the written plan and acceptance criteria. A separate commit adds `.cursor/rules/planning-implementation-docs.md` and wires it into kickoff, code-review, docs-review, and `AGENTS.md`; that is coherent and self-consistent.

Residual risk is low and already acknowledged in the plan: the root `ServiceProvider` may still dispose the same `NpgsqlDataSource` after hosted services stop; the team accepted **Option 2** after green tests. No API or authority contract changes.

## Documentation checked

| Document | Result |
|----------|--------|
| `docs/plans/NEO-13-implementation-plan.md` | **Matches** — Decisions (Option 2, ConfigureAwait), AC checked, technical approach and file lists align with the diff; open questions resolved per [planning-implementation-docs](../../.cursor/rules/planning-implementation-docs.md). |
| `docs/decomposition/modules/E1_M1_InputAndMovementRuntime.md` | **Matches (post-review update)** — Snapshot links NEO-13 for shared `NpgsqlDataSource` shutdown — `8c3e8c0`. |
| `docs/decomposition/modules/documentation_and_implementation_alignment.md` | **Matches (post-review update)** — E1.M1 row snapshot + plans column include NEO-13 — `8c3e8c0`. |
| `docs/decomposition/modules/module_dependency_register.md` | **N/A** — no status or dependency change required for a shutdown-only fix. |
| `docs/decomposition/modules/client_server_authority.md`, `contracts.md` | **N/A** — no authority or wire-shape change. |
| `.cursor/rules/planning-implementation-docs.md`, `story-kickoff.md`, `code-review-agent.md`, `docs-review-agent.md`, `AGENTS.md` | **Matches** — rule text is clear; relative links in agent rules remain valid. |

## Blocking issues

_None._

## Suggestions

**All done.**

1. ~~**Traceability (optional):** Add `NEO-13` / `docs/plans/NEO-13-implementation-plan.md` to the E1.M1 **Plans / pointers** column in `documentation_and_implementation_alignment.md` (or a one-line note under Postgres in `E1_M1_InputAndMovementRuntime.md`) so future readers find shutdown lifecycle work next to NEO-8.~~ **Done.** E1.M1 snapshot + plans list + module implementation snapshot updated — `8c3e8c0`.
2. ~~**PR narrative:** Call out the **two themes** in the branch (server disposal + planning-doc rule) so reviewers do not miss the `.cursor/` and `AGENTS.md` changes if they only skim `server/`.~~ **Done.** `docs/plans/NEO-13-implementation-plan.md` **PR / review** section lists server vs repo-process themes — `8c3e8c0`.

## Nits

- ~~**`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

- `dotnet test NeonSprawl.sln` — **Passed** (36 tests) at review time.
- Optional: run `NeonSprawl.Server` with `ConnectionStrings:NeonSprawl` set, stop the process, confirm clean shutdown logs.
