neon-sprawl/docs/reviews/2026-05-24-NEO-61.md

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	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	Matches — E3M1-05 acceptance items flipped with NEO-61 landed note.
docs/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	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	Matches — E3.M1 row In Progress is correct; E3.M1 note footnote updated for NEO-61 (E3M1-05).
docs/decomposition/modules/client_server_authority.md	Matches — server-authoritative capacity; no client-trusted depletion state.
server/README.md	Matches — Resource node instance depletion subsection: lazy init, reason-code table, persistence split, NEO-63 deferral.

Register/tracking: alignment table and E3.M1 note footnote updated for NEO-61.

Blocking issues

None.

Suggestions

1. AAA layout in stateful tests — Two new test methods mix Assert into Act: Done.

   • 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.

2. Dependency register footnote (optional) — Extend E3.M1 note in 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. Done.

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. Done.

• 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. Done — throws InvalidOperationException (programmer error after validated catalog maxGathers).

• 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

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.