don
/
neon-sprawl
mirror of https://github.com/ViPro-Technologies/neon-sprawl
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity
neon-sprawl/docs/reviews/2026-04-08-NEON-15.md

4.2 KiB
Raw Blame History

Code review: NEON-15

Date: 2026-04-08
Scope: Branch NEON-15-dispose-npgsql-datasource vs main — 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<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/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.
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.

Follow-up (suggestions implemented)

  • Traceability: documentation_and_implementation_alignment.md (E1.M1 row) and E1_M1_InputAndMovementRuntime.md now link NEON-15 for Postgres pool shutdown.
  • PR narrative: docs/plans/NEON-15-implementation-plan.md PR / review section now lists the two merge-description themes (server vs repo process).