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

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

Follow-up: Review suggestions below are done (strikethrough + Done.); nits remain optional.

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 Matches (post-review update) — Snapshot links NEON-15 for shared NpgsqlDataSource shutdown — 8c3e8c0.
docs/decomposition/modules/documentation_and_implementation_alignment.md Matches (post-review update) — E1.M1 row snapshot + plans column include NEON-15 — 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 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. 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/NEON-15-implementation-plan.md PR / review section lists server vs repo-process themes — 8c3e8c0.

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.