diff --git a/docs/reviews/2026-05-24-NEO-61.md b/docs/reviews/2026-05-24-NEO-61.md new file mode 100644 index 0000000..d8906e1 --- /dev/null +++ b/docs/reviews/2026-05-24-NEO-61.md @@ -0,0 +1,62 @@ +# Code review — NEO-61 world node instance depletion store + +**Date:** 2026-05-24 +**Scope:** Branch `NEO-61-e3m1-world-node-instance-depletion-store` · commits `869d411`–`6d2d3c0` vs `origin/main` +**Base:** `origin/main` + +## Verdict + +**Approve with nits** + +## Summary + +NEO-61 adds server-authoritative **remaining gathers** per world node instance: **`IResourceNodeInstanceStore`** (in-memory + Postgres **`V006`**) and **`ResourceNodeInstanceOperations.TryCommitSuccessfulGatherDecrement`** with registry-backed lazy init, atomic decrement, and stable **`node_depleted`** / **`unknown_node`** reason codes. The split-store pattern mirrors NEO-54; DI chains from **`AddResourceNodeCatalog`**, and **`InMemoryWebApplicationFactory`** forces the in-memory store in tests. Ten automated tests cover lazy init, depletion loop, unknown-node deny, store normalization/idempotency, and Postgres cross-factory persistence. Risk is low: no HTTP surface, no interact wiring, no inventory mutation — persistence + rules only, ready for NEO-62/NEO-63. + +## Documentation checked + +| Document | Result | +|----------|--------| +| [`docs/plans/NEO-61-implementation-plan.md`](../plans/NEO-61-implementation-plan.md) | **Matches** — split store + operations, lazy init, keep depleted rows at 0, reason codes, **`V006`**, DI chain, README, acceptance checklist complete. Minor: plan listed in-memory **concurrency tests** and an `out outcome` signature; implementation uses per-id locks (no concurrent test) and returns the outcome struct directly. | +| [`docs/plans/E3M1-prototype-backlog.md`](../plans/E3M1-prototype-backlog.md) | **Matches** — E3M1-05 acceptance items flipped with NEO-61 landed note. | +| [`docs/decomposition/modules/E3_M1_ResourceNodeAndGatherLoop.md`](../decomposition/modules/E3_M1_ResourceNodeAndGatherLoop.md) | **Matches** — NEO-61 depletion-store bullet under **Related implementation slices**; per-instance capacity, world-wide keying, **`node_depleted`** align with **Purpose** / **Responsibilities**. | +| [`docs/decomposition/modules/documentation_and_implementation_alignment.md`](../decomposition/modules/documentation_and_implementation_alignment.md) | **Matches** — E3.M1 row notes NEO-61 landed with README link; **Still planned** correctly lists gather engine / interact wiring. | +| [`docs/decomposition/modules/module_dependency_register.md`](../decomposition/modules/module_dependency_register.md) | **Partially matches** — E3.M1 row **In Progress** is correct, but **E3.M1 note** footnote still lists NEO-57–NEO-60 only and says “until depletion … slices land” without NEO-61 (alignment table already updated). | +| [`docs/decomposition/modules/client_server_authority.md`](../decomposition/modules/client_server_authority.md) | **Matches** — server-authoritative capacity; no client-trusted depletion state. | +| [`server/README.md`](../../server/README.md) | **Matches** — Resource node instance depletion subsection: lazy init, reason-code table, persistence split, NEO-63 deferral. | + +Register/tracking: alignment table update is done; **E3.M1 note** footnote should mention NEO-61 when touched (same pattern as NEO-60 register suggestion). + +## Blocking issues + +None. + +## Suggestions + +1. **AAA layout in stateful tests** — Two new test methods mix **Assert** into **Act**: + - `ResourceNodeInstanceOperationsTests.TryCommitSuccessfulGatherDecrement_RepeatedUntilCapacity_ShouldReturnNodeDepletedWithoutGoingNegative` — assertions inside the decrement loop. + - `ResourceNodeInstancePersistenceIntegrationTests.TryCommitSuccessfulGatherDecrement_WhenDepleted_ShouldPersistZeroRow` — assertions inside the final `using` Act block. + Move loop invariants / final expectations to a dedicated **Assert** section (collect last applied outcome during Act, assert after) per [csharp-style](../../.cursor/rules/csharp-style.md#unit-and-integration-tests-arrange-act-assert). + +2. **Dependency register footnote (optional)** — Extend **E3.M1 note** in [`module_dependency_register.md`](../decomposition/modules/module_dependency_register.md) to include **NEO-61** (E3M1-05 depletion store) so the register matches the alignment table — same follow-up as NEO-60 for NEO-57–NEO-60. + +3. **Concurrency test gap (optional)** — Plan §8 listed in-memory get/ensure/decrement **concurrency hygiene** tests; implementation uses per-id locks but has no parallel test. NEO-54 inventory store also omits concurrent tests, so non-blocking; add a lightweight `Parallel.For` smoke test if you want plan parity. + +## Nits + +- Nit: **`NormalizeInteractableId`** lives as `internal static` on **`InMemoryResourceNodeInstanceStore`** but is called from **`PostgresResourceNodeInstanceStore`** and **`ResourceNodeInstanceOperations`** — works, but a small shared helper (e.g. `ResourceNodeInstanceIds.Normalize`) would reduce cross-type coupling if this grows. + +- Nit: **`TryEnsureInitialized` failure → `unknown_node`** in `ResourceNodeInstanceOperations` (lines 26–31) is unreachable after a registry hit with non-negative **`maxGathers`**; a distinct internal reason or `Debug.Assert` would clarify intent vs catalog miss. + +- Nit: Postgres **`TryDecrementRemainingGathers`** opens a second connection via **`TryGetRemainingGathers`** on decrement miss to distinguish depleted-at-zero — correct behavior, slightly extra round-trip on deny path only. + +## Verification + +```bash +cd server +dotnet test NeonSprawl.Server.Tests/NeonSprawl.Server.Tests.csproj --filter "FullyQualifiedName~ResourceNodeInstance" +# Passed! Failed: 0, Passed: 10, Skipped: 0 + +dotnet test NeonSprawl.Server.Tests/NeonSprawl.Server.Tests.csproj +``` + +Postgres integration (when `ConnectionStrings__NeonSprawl` is set): `ResourceNodeInstancePersistenceIntegrationTests` — cross-factory read and depleted-row-at-zero persistence.