Document (and test) FIX - #209
Merged
Jagadeeshftw merged 1 commit intoJul 27, 2026
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
================================================================================
PULL REQUEST
TITLE
docs(repositories): warn peekNextId() is synchronous-only and lock in behavior
BASE BRANCH
main
FEATURE BRANCH
fix/settlement-peeknextid-sync-warning
================================================================================
SUMMARY / MOTIVATION
SettlementRepository.peekNextId() returns the id that the next create() call
will assign, but it performs no locking. That guarantee is safe ONLY because
the store is an in-memory, single-process, synchronous implementation: Node's
event loop serializes all synchronous code, so nothing can interleave between a
caller reading peekNextId() and acting on it.
The method's name and existence invite misuse. If this repository is ever
swapped for an async-backed store (e.g. a real database), the previewed id
could be consumed by a concurrent caller between the peek and the create,
introducing a race that does not exist today.
This change makes that assumption explicit, locks in the current correct
behavior with a test, and flags the persistence-swap risk where the project
already documents "swappable for a persistent store later" intent.
This is a documentation + test-clarification change. No production behavior is
altered, so the risk of regression is effectively zero.
================================================================================
CHANGES
src/repositories/settlementRepository.ts
"SYNCHRONOUS-ONLY SAFETY GUARANTEE" warning. It now states that the method
MUST NOT be used to predict/reserve an id across an awaited boundary, is
safe only because create() is synchronous, and that any async-backed swap
would break the atomicity guarantee. The returned id is described as a
hint, not a reservation.
src/repositories/settlementRepository.test.ts
(locks in the synchronous-only guarantee: peek -> immediate create
returns the exact previewed id, and the counter advances by one)
anchor-change reindex branch in save(), lifting file coverage to 100%.
docs/ARCHITECTURE.md
documents the persistence-swap risk and lists required guardrails for any
async-backed repository (allocate ids atomically in a single operation;
never reserve across an await; keep the locking-in test green).
src/repositories/inMemoryRepository.ts
explaining that id allocation's atomicity depends on the synchronous,
single-threaded store and would break under an async swap. Points readers
to docs/ARCHITECTURE.md.
================================================================================
ACCEPTANCE CRITERIA MAPPING
[OK] peekNextId() carries an explicit doc-comment warning about its
synchronous-only safety guarantee.
-> settlementRepository.ts JSDoc on peekNextId().
[OK] A test locks in the current correct behavior.
-> settlementRepository.test.ts "previews the id that the immediately
following create() yields".
[OK] A note exists flagging the risk for any future async-backed repository
implementation.
-> docs/ARCHITECTURE.md "In-Memory Repositories & Future Persistence"
section, plus a source note in inMemoryRepository.ts.
================================================================================
TEST PLAN
Run the settlement repository tests:
npx jest src/repositories/settlementRepository --verbose
Expect the two new peekNextId tests and the new save reindex test to pass.
Run the full suite:
npm test
Expect all suites to pass (425 tests).
Build:
npm run build (tsc)
Expect a clean compile with no type errors.
Lint:
npm run lint (eslint "src/**/*.ts")
Expect no errors.
Coverage (touched files):
npx jest --coverage
src/repositories/settlementRepository.ts -> 100%
src/repositories/inMemoryRepository.ts -> 100%
Exceeds the 95% guideline.
================================================================================
VALIDATION RESULTS (run locally before opening this PR)
Test Suites : 40 passed, 40 total
Tests : 425 passed, 425 total
Build (tsc) : clean, no errors
ESLint : clean, no errors
Coverage : settlementRepository.ts 100%, inMemoryRepository.ts 100%
================================================================================
FILES CHANGED
docs/ARCHITECTURE.md | 37 ++++++++++++++++++++++++
src/repositories/inMemoryRepository.ts | 15 +++++++++-
src/repositories/settlementRepository.test.ts | 41 ++++++++++++++++++++++++++
src/repositories/settlementRepository.ts | 25 ++++++++++++++-
4 files changed, 116 insertions(+), 2 deletions(-)
(No new files were created; all four are modifications.)
================================================================================
NOTE ON SUBMITTING
This change was prepared on the local branch
fix/settlement-peeknextid-sync-warning and committed. It could not be pushed or
opened automatically because this environment has no GitHub credentials / gh
CLI / push access to the upstream repository. To open the PR:
then open a pull request from that branch into main on GitHub, using the TITLE
and SUMMARY above.
================================================================================
Closes #164