From 2617b65f79b868a114bac55b113d1c6ab9ba9b02 Mon Sep 17 00:00:00 2001 From: VinPropane Date: Wed, 8 Apr 2026 21:58:18 -0400 Subject: [PATCH] NEON-15: add code review (2026-04-08) --- docs/reviews/2026-04-08-NEON-15.md | 47 ++++++++++++++++++++++++++++++ 1 file changed, 47 insertions(+) create mode 100644 docs/reviews/2026-04-08-NEON-15.md diff --git a/docs/reviews/2026-04-08-NEON-15.md b/docs/reviews/2026-04-08-NEON-15.md new file mode 100644 index 0000000..318dca7 --- /dev/null +++ b/docs/reviews/2026-04-08-NEON-15.md @@ -0,0 +1,47 @@ +# Code review: NEON-15 + +**Date:** 2026-04-08 +**Scope:** Branch `NEON-15-dispose-npgsql-datasource` vs `main` — [NEON-15](https://neon-sprawl.atlassian.net/browse/NEON-15) (dispose `NpgsqlDataSource` on shutdown), plus planning-doc rule cross-links. +**Base:** `main` (range `main...HEAD`, commits `abffd29` → `62ff210`). + +## Verdict + +**Approve with nits** + +## Summary + +The branch adds `NpgsqlDataSourceShutdownHostedService`, registers it **before** `PostgresDevPlayerSeedHostedService` so `StopAsync` runs **last** (correct use of reverse shutdown order), switches to `AddSingleton(…)`, 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/NEON-15-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` | **N/A** for merge blockers — Postgres path is already documented at prototype level; this change is **lifecycle / ops hygiene** under the same `Game/PositionState` area, not a new contract. | +| `docs/decomposition/modules/documentation_and_implementation_alignment.md` | **Partially matches** — E1.M1 snapshot lists many NEON plans; **NEON-15** is not linked yet. Optional follow-up to add a short pointer (suggestion below). | +| `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. | + +**Register / tracking table:** No update **required** for merge; optional E1.M1 snapshot pointer to NEON-15 for traceability. + +## Blocking issues + +_None._ + +## Suggestions + +1. **Traceability (optional):** Add `NEON-15` / `docs/plans/NEON-15-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 NEON-5. +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/`. + +## Nits + +- **Nit:** `NpgsqlDataSourceShutdownHostedService.StopAsync` ignores `cancellationToken`; acceptable for short dispose; if Npgsql ever exposes cancellation-aware teardown, consider threading it through. +- **Nit:** Plan still lists optional manual Ctrl+C run with a real connection string — worth doing once before merge if CI does not exercise long-lived `WebApplication` shutdown the same way as a local server. + +## 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.