Implement idempotent, concurrency-safe wallet transfers - #134
Open
km2411 wants to merge 45 commits into
Open
Conversation
Replace the unconditional Go/golangci-lint/gcc setup with Python setup and dependency install. The var-driven LINT_CMD/FORMAT_CHECK_CMD/TEST_CMD structure and runs-on stay untouched (see ADR-0001 for why, once that lands).
ruff/mypy config, pytest settings, pre-commit hooks (gitleaks, ruff, mypy), a Makefile wrapping lint/format-check/fmt/test/ci, and a smoke test proving the pytest gate is wired. Gates guard gracefully around src/wallet_transfer not existing yet.
Process only, no decisions recorded yet - filled ADRs come once the deeper problem/requirements/approach review is done.
/new-adr scaffolds the next adr-NNN from the template. /assignment-review self-checks the diff against evaluation_guide.md and .github/copilot-instructions.md's rubric before opening the PR. Canonical copies live at .agents/skills/ (vendor-neutral SKILL.md standard); .claude/skills symlinks to it.
Python + PostgreSQL over the CI-hinted Go default, reasoned explicitly against the evaluation criteria (schema, locking, idempotency) rather than the unstated toolchain signal.
Durable idempotency_records with a unique constraint + request fingerprint, written in the same transaction as the transfer it guards. Includes the validation-boundary split (pure input validation vs. business-rule failure) found and fixed during review.
Ordered pessimistic row locks, explicit lock_timeout, bounded retry with jittered backoff. Includes the enumerated CT1-CT6 required concurrency test scenarios and scripts/simulate.py as empirical validation beyond the automated suite.
DB-level invariants (FK/CHECK constraints making invalid states unrepresentable), versioned via Flyway per explicit direction. UUID primary keys throughout, decided explicitly rather than left open.
Hand-authored openapi/spec.yaml as source of truth, generated Pydantic models, hand-written FastAPI routes, Swagger UI free from FastAPI, CI drift check keeping the contract and implementation honest.
Protocol-first contracts per layer, Red-Blue-Green against them. Fake (not mock) repository for fast service-layer cycles; real Postgres via testcontainers for repository/e2e tiers. Includes fixture design, parameterization guidance, and the full DT/ST/LT/E required test scenario matrix.
Reverses the earlier sync-routes framing: asyncpg has no sync API, so committing to it for mock-free test setup means the whole stack (routes, service, repository) is async, not just the tests.
Makes an implicit assumption explicit: PENDING is never durably observable outside its own transaction, no background worker or queue. Found during review as a real decision never previously recorded on its own.
Explicit considered-options treatment of the choice ASSIGNMENT.md calls out directly (stored vs. derived balance), previously only asserted via ADR-0004's schema with no justification of its own.
Project agent-context and hard rules (AGENTS.md, CLAUDE.md symlink), architecture doc skeleton with Known Limitations, and the .importlinter layers contract enforcing handler -> service -> repository -> domain.
Dev-only convenience, decoupled from CI/tests, which use testcontainers against their own throwaway Postgres instead.
Same targets (install, lint, format-check, fmt, test, test-cov, pre-commit-install, ci), translated to just syntax. Updates every `make X` reference in AGENTS.md and the assignment-review skill to `just X`. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…2 through ADR-0010 An independent cold review of ADR-0001 through ADR-0009 surfaced two contradictions that would have broken on first implementation, plus several gaps. All are resolved here: - Idempotency write order vs. schema (critical): ADR-0002 inserts the idempotency record before the transfer row exists, but ADR-0004's original transfer_id FK was NOT NULL and non-deferred, so that insert would fail every time. Fixed by generating every table's primary key application-side as UUIDv7 (ADR-0004) instead of gen_random_uuid(), and declaring the FK DEFERRABLE INITIALLY DEFERRED. - Wallet-existence check timing (critical): ADR-0002's validation- boundary list, ADR-0003, and ADR-0006's ST6 disagreed on whether a non-existent wallet is rejected before any transaction or discovered inside one via the locking SELECT FOR UPDATE. Reconciled on ADR-0003's (correct) telling; ADR-0006's ST6 split into ST6/ST7 to test both paths with their actual mechanics. - No ADR described how a wallet comes to exist for scripts/simulate.py or manual testing. Added ADR-0010: a flag-gated (SEED_DEMO_DATA=true), always-last (reserved V9000+ Flyway version band) seed migration, no new endpoint. - No ADR specified the HTTP status for a FAILED (insufficient-funds) transfer. Decided in ADR-0008: 200, since it's a correctly handled business outcome, not a transport-level failure. - ADR-0003's bounded retry didn't account for connection-pool exhaustion under its own CT6 stress scenario. Added as a third retryable class alongside lock_timeout/deadlock_detected, with an explicit pool-sizing note. - Added CT7 (ADR-0003) and a generalization note (ADR-0002) proving the idempotency-key uniqueness guarantee holds under any interleaving of client retries and the server's own internal bounded-retry attempts, not just two top-level concurrent requests. - Named single-currency as an explicit, deliberate limitation in ARCHITECTURE.md, consistent with how the project's other scope cuts are already documented there. - Minor: corrected a cross-reference that misattributed the testcontainers Docker-access risk to ADR-0001, added a failure_reason CHECK constraint, acknowledged the ledger-row/transfer-match invariant as test- not schema-enforced, and assigned validation-boundary checks to the domain layer explicitly. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Kicks off implementation against the ten reviewed ADRs — read this before writing any code; the ADRs are the source of truth if anything here is ambiguous. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Pre-existing scaffold file didn't match ruff format's output — fixed to get a clean just ci baseline before real implementation starts. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
pyproject.toml, CI, and ADR-0004's uuid6-vs-stdlib reasoning all target 3.12; this makes pyenv select it automatically in this directory instead of silently falling back to whatever's active globally. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Four Flyway migrations implementing ADR-0004's schema: wallets with a non-negative balance CHECK, transfers with self-transfer and FAILED-needs-a-reason CHECKs, ledger_entries with a NOT NULL transfer_id, and idempotency_records with the DEFERRABLE INITIALLY DEFERRED transfer_id FK that ADR-0002's write order depends on (idempotency record inserted before the transfer row it references, same transaction). Wires `just migrate` to run the official flyway/flyway Docker image against the docker-compose Postgres, gated behind SEED_DEMO_DATA for db/seed/ per ADR-0010 (not added yet). Gives the Postgres service an explicit compose network name so the standalone Flyway container can reach it by hostname regardless of the compose project name. Verified: `just migrate` applies all four cleanly, and a manual psql transaction confirms the deferred-FK write order — an idempotency record inserted before its transfer row commits fine, while one that never gets a transfer row is correctly rejected at COMMIT. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Wallet, Transfer, LedgerEntry per ADR-0006 — pure, no I/O. Transfer owns its own state-transition validation: only PENDING -> PROCESSED and PENDING -> FAILED are reachable (DT2), any transition on an already-terminal transfer is rejected (DT1), and self-transfer / non-positive amount are rejected at construction time, before a transaction ever opens (ADR-0002's validation boundary). Transfer is immutable — mark_processed()/mark_failed() return a new instance rather than mutating in place. Adds packaging metadata (pyproject.toml) so the src/ layout installs editable, and empty stub packages for repositories/services/handlers so .importlinter's layering contract is checkable from this commit onward instead of only once every layer is fully built. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
WalletRepository, TransferRepository, LedgerRepository, and IdempotencyRepository, per ADR-0006/0007 — async typing.Protocol, no implementation yet. A UnitOfWork Protocol binds all four to one transaction and is what the retry loop (ADR-0003) will open fresh per attempt, giving concrete shape to "same transaction" without threading a raw connection through every repository call. WalletRepository.get_two_for_update() encapsulates the ascending-wallet_id lock ordering (ADR-0003) as a single call rather than leaving callers responsible for sorting two ids correctly — locking mechanics belong in the repository, not repeated in the service. Retryable errors (lock_timeout, deadlock_detected, a pool-acquire timeout) and IdempotencyKeyConflictError are defined here since repositories are what raise them; the service layer will be what catches them. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
A second, real implementation of the four repository Protocols plus UnitOfWork (ADR-0006) — not a mock. FakeUnitOfWork gives real commit/rollback semantics without a database: entering stages a snapshot of a shared FakeDatabase, a clean exit merges it back, and an exception discards the snapshot entirely, so a rolled-back attempt leaves no trace, idempotency record included — the same guarantee the deferred FK gives for real (ADR-0002). FakeUnitOfWorkFactory can be configured to raise a given error on the first N calls before returning a working UnitOfWork, so the upcoming service-layer tests (ST3: fails once then succeeds; ST4: fails every attempt) can force a deterministic retryable-error outcome without real lock contention. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…tration Implements create_transfer() against the repository Protocols/fakes (ADR-0006), following ADR-0002/0003's write order exactly: pre-generate the transfer's UUIDv7 id, insert the idempotency record, insert the PENDING transfer, lock both wallets via get_two_for_update() (ascending order is the repository's job, not the service's), balance check, ledger writes, status update — all inside one UnitOfWork per attempt. - Idempotency conflict: matching fingerprint returns the cached transfer without touching wallets; mismatched fingerprint raises IdempotencyKeyReusedError (409). - Missing wallet(s) raise WalletNotFoundError (404) after the idempotency record was already inserted — the UnitOfWork's exception exit rolls back the whole attempt, record included. - Insufficient funds resolves FAILED and returns normally — not an exception, never retried. - RetryableRepositoryError triggers bounded retry (default 3 attempts) with jittered exponential backoff via an injectable async sleep; exhausting it raises RetryExhaustedError (503). - Self-transfer / non-positive amount raise Transfer.create()'s InvalidTransferError before any UnitOfWork is even opened. Adds uuid6 as the runtime id-generation dependency (ADR-0004) and pytest-asyncio for the async test suite. Verified: all of ST1-ST7 pass against the in-memory fakes, including the behavioral (not call-count) proofs — ST1 clears the fake's wallets after seeding the cached result, so a replay that incorrectly re-executed instead of using the cache would visibly 404; ST6/ST7 assert directly on the fake's committed state to prove no repository call happened / everything rolled back, respectively. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Real Postgres implementations of the four repository Protocols plus AsyncpgUnitOfWork, wired to a connection pool sized with headroom above CT6's 100-attempt ceiling (min=10, max=120 — ADR-0003). AsyncpgUnitOfWork.__aenter__ acquires a pooled connection, starts a transaction, and runs `SET LOCAL lock_timeout` once per transaction so it bounds every lock wait inside it, not just the wallet locks — including the idempotency-insert's unique-constraint conflict wait (ADR-0003). __aexit__ commits on a clean exit, rolls back on exception, and translates asyncpg's LockNotAvailableError / DeadlockDetectedError into our own LockTimeoutError / DeadlockDetectedError so the service's retry loop only ever sees the three documented retryable classes; a pool-acquire TimeoutError is translated the same way at __aenter__. AsyncpgWalletRepository.get_two_for_update() deliberately issues two sequential single-row `SELECT ... FOR UPDATE` statements in ascending id order, not one combined `WHERE id = ANY(...) ORDER BY ... FOR UPDATE` query — Postgres's LockRows plan node locks in scan order, not ORDER BY order, so the combined form would silently not guarantee the lock ordering the deadlock-avoidance mechanism actually depends on. AsyncpgIdempotencyRepository.insert() catches UniqueViolationError directly and raises IdempotencyKeyConflictError — a business-level conflict, not a retryable one, so it's translated at the repository call site rather than by the UnitOfWork's lock-error translation. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Real-Postgres testing (developing CT1/CT7) found that the write order ADR-0002/ADR-0003 originally specified — insert the transfer row, then explicitly lock both wallets ascending by wallet_id — deadlocks under real concurrency. INSERT INTO transfers takes an implicit FOR KEY SHARE lock on each referenced wallet as part of its FK check, acquired in whatever order the columns happen to be checked, not in the ascending-wallet_id order the explicit lock uses. Two concurrent attempts sharing a wallet could each hold that implicit lock from their own INSERT while blocked on the explicit FOR UPDATE the other already held via its own implicit lock — a real, repeatable deadlock_detected, defeating the entire point of the ascending lock order. Fix: acquire the explicit wallet locks first, then insert the transfer row. By the time the transfer INSERT's own FK check runs, our own FOR UPDATE is already the strongest lock held on both rows, so that check has nothing to contend with. Wallet-not-found (404) is now discovered by the explicit lock query returning zero rows before the transfer row is ever inserted, rather than via the FK rejecting the insert as originally reasoned — same outcome, just found earlier and by a different mechanism. The FK constraint remains as defense-in-depth. This is exactly the failure mode ADR-0006 designed the real-Postgres repository tier to catch: the in-memory fake used for ST1-ST7 cannot reproduce Postgres's implicit FK locking, so this bug was invisible until CT1/CT7 ran against a real database. ADR-0002 and ADR-0003 updated to document the corrected order and the reasoning; both remain Proposed pending the rest of §6's test matrix. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…e transaction Postgres aborts an entire transaction after any statement error, including a UniqueViolationError from the idempotency-key unique constraint — every later query on that connection fails with InFailedSQLTransactionError until the transaction ends. Since create_transfer()'s conflict path needs to keep querying the *same* transaction after catching that error (get_by_key(), then transfers.get_by_id(), to return the cached result), the insert needs to fail without taking the rest of the transaction down with it. Wraps the INSERT in a nested connection.transaction() — asyncpg turns this into a SAVEPOINT/RELEASE SAVEPOINT/ROLLBACK TO SAVEPOINT automatically since it's called inside the UnitOfWork's already-open outer transaction. A conflict now rolls back only the savepoint, not the whole attempt, so the fingerprint-comparison and cached-result lookup that follow it work as ADR-0002 describes. Found the same way as the wallet-locking-order bug: real-Postgres testing (CT4/CT5/CT7) hit this immediately; the in-memory fake has no equivalent failure mode to have caught it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The repository/integration tier ADR-0006 calls for: a session-scoped testcontainers Postgres, Flyway migrations applied through the same flyway/flyway image just migrate uses (not a bypass mechanism), function-scoped TRUNCATE-per-test isolation. Flyway runs as its own container on a shared Docker network alongside the Postgres testcontainer, reachable by network alias — this is what "the same migrations applied the same way" actually requires; a plain-SQL schema-creation shortcut inside the fixture would reopen the drift risk ADR-0004 exists to close. The asyncpg pool is function-scoped, not session-scoped: it's bound to the event loop it was created on, and pytest-asyncio gives each test its own loop by default, so a shared pool across tests threw cross-loop errors. Recreating it per test is cheap and sidesteps the issue entirely — the container is the expensive part to keep alive, not the pool. Covers CT1 (concurrent debits exceeding balance, parametrized [2, 5, 20] — 50-100 is scripts/simulate.py's job per ADR-0003, not this tier), CT2 (opposite-direction pair, bounded by a timeout well under Postgres's deadlock_timeout so the timeout itself is part of the no-deadlock proof), CT3 (forced lock contention via a manually-held transaction, released after the contender's first attempt should have timed out), CT4/CT5 (concurrent identical/conflicting idempotency keys), and CT7 (a client-initiated retry landed deterministically in the gap after the original request's first internal attempt rolls back, via TransferService's new on_retry hook — not a wall-clock sleep, per ADR-0003's explicit requirement). Both of the previous two commits' bugs (the wallet-lock ordering deadlock, the idempotency-conflict transaction poisoning) were found by writing this suite against real Postgres — the in-memory fake used for ST1-ST7 has no equivalent failure mode for either. Also fixes two pre-commit gaps surfaced by this change: mypy's explicit_package_bases/mypy_path so it can type-check the flat (no __init__.py) tests/ tree without colliding on two files both named conftest.py, and switches the mypy hook itself from mirrors-mypy's isolated environment (which had pytest/asyncpg unresolvable, flagging every fixture decorator as untyped) to a system-language hook using the project's own venv, matching how import-linter is already wired. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
LT1/LT2 check a single PROCESSED and a single FAILED transfer directly. LT3/LT4 run a mixed concurrent batch (CT1-style fan-in on one wallet plus a CT2-style opposite-direction pair, all at once) and then check both invariants together: sum(CREDIT) == sum(DEBIT) globally, and each wallet's balance equals its initial balance plus its own credits minus its own debits — proving the stored-balance column (ADR-0009) and the ledger stay honest with each other after a real batch of concurrency, not just a single transfer. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
openapi/spec.yaml is the hand-authored source of truth for POST /transfers (ADR-0005): CreateTransferRequest, TransferResponse, and a shared ErrorResponse. amount is deliberately left unconstrained (no minimum) and there's no cross-field wallet-inequality rule — self-transfer and non-positive amount stay domain-level invariants (ADR-0002/0006), not request-shape constraints, so they must reach Transfer.create() to be rejected rather than being caught earlier by a generated model. status only allows PROCESSED/FAILED, never PENDING (ADR-0008). `just generate-models` runs datamodel-code-generator against the spec into handlers/generated_models.py — never hand-edited, pure build output. Uses --use-annotated so constrained fields come out as Annotated[...] rather than Pydantic v1-style constr()/conint(), which mypy --strict rejects as an invalid type annotation. Adds a per-file ruff ignore for the generated file's line length, since its embedded OpenAPI description text isn't meant to be hand-wrapped. fastapi/pydantic/uvicorn added as runtime dependencies for the handler layer this unblocks. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Fulfills the observability surface ARCHITECTURE.md's Known Limitations already commits to: attempt started (debug), a retryable error triggering a retry (warning), the terminal outcome — resolved, or rejected with a 404/409 (info) — and retries exhausted (error). Every line carries the idempotency key so a specific request's attempts can be traced through the logs. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Thin route handler: validates nothing itself beyond what the
generated Pydantic model already enforces, converts the request body
into the service's own CreateTransferRequest, and maps each service
exception to the HTTP status ADR-0008 specifies — InvalidTransferError
422, WalletNotFoundError 404, IdempotencyKeyReusedError 409,
RetryExhaustedError 503. No business logic and no direct repository
access, per the layering contract.
create_app() wires a FastAPI app with a lifespan that creates the
asyncpg pool and TransferService once at startup (DATABASE_URL read
lazily inside the lifespan, not at import time, so building the app
for schema introspection — the OpenAPI drift check — doesn't need a
reachable database). A custom RequestValidationError handler squashes
FastAPI's default structured validation-error list into the same
{"detail": string} envelope every other error response uses, so a
malformed-body 422 doesn't carry an undocumented shape the drift
check would otherwise catch as real divergence.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
ADR-0005/ADR-0006 (E2) call for this as an explicit CI check, not a pytest case: compares openapi/spec.yaml against the FastAPI app's own runtime-introspected OpenAPI document (app.openapi(), no server or database needed since the DSN is only read inside the lifespan). Diffs paths/methods, each operation's response status codes, and each named schema's required fields and property names — the parts that actually describe the contract — while deliberately ignoring incidental metadata (auto-generated titles, description wording) that would otherwise produce false positives on every regeneration. Verified both directions: passes against the current spec/app pair, and correctly fails when a field is renamed in only one of the two (manually confirmed, not committed as a test — this is a CI script, not a pytest case, per ADR-0006). Wired into `just ci` as a fourth gate alongside lint/format-check/test. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
FastAPI's TestClient against the real running app (lifespan-managed pool included) and the same testcontainers Postgres the repository tier uses, via a new postgres_dsn fixture factored out of the pool fixture so both can build their own connections to the same container. Covers a normal transfer, a sequential idempotent replay returning byte-identical bodies, every validation-boundary rejection (malformed body, self-transfer, non-positive amount → 422; nonexistent wallet → 404; reused key with a different payload → 409), and insufficient funds resolving 200/FAILED rather than an error status — closing the loop on what the fake and the direct-repository tests already proved, now through the real HTTP layer. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
db/seed/V9001__seed_demo_wallets.sql, applied only via SEED_DEMO_DATA=true just migrate (ADR-0010) — four wallets with round balances and stable, hardcoded ids, since there's no wallet-creation endpoint for scripts/simulate.py or manual exploration to work with otherwise. Verified both directions against the docker-compose Postgres: a plain `just migrate` applies only the four schema migrations and leaves wallets empty; SEED_DEMO_DATA=true additionally applies V9001 after them, in the reserved V9000+ band Flyway sorts to the end regardless of how many more schema migrations exist by then. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Dockerfile + a new `app` service in docker-compose.yml, built on the runtime requirements.txt only (PYTHONPATH=/app/src, no editable install needed for a container). Deliberately does not run migrations from the container's entrypoint — ADR-0004 is explicit that migrations are a deploy/setup-time concern via `just migrate`, never part of the service's runtime path — so a fresh database needs `just migrate` (optionally SEED_DEMO_DATA=true) before the app can serve requests. New just targets: `up` (Postgres only, waits for pg_isready), `down`, `dev` (local uvicorn against .venv for fast iteration), `demo` (the full containerized stack with demo wallets seeded — what the interview demo runs), and `simulate`. scripts/simulate.py is ADR-0003's demo tooling for CT6 — drives the real running HTTP API (not a direct DB/service call) with concurrent load against the seeded demo wallets, then verifies balance reconciliation directly against Postgres. Covers same-wallet fan-in, an opposite-direction pair, and duplicate idempotency-key replay. Verified live end-to-end: `just demo` brings up a working stack (confirmed via curl against /transfers and /docs), and `just simulate --fan-in 100` passes all three scenarios against it, satisfying CT6's 50-100-concurrent-attempt requirement — the one scenario ADR-0003 deliberately scopes to this manual tool rather than the testcontainers-backed automated suite. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Covers both run modes (just demo for the full containerized stack, just up + just dev for fast local iteration against .venv), how to verify the app is actually serving requests, how to drive scripts/simulate.py's concurrency demo, and the common pitfalls (migrations not yet applied, Docker not running, port conflicts). Symlinked automatically into .claude/skills/ alongside the existing new-adr and assignment-review skills. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…mponents) Completes the Definition of Done item deferred since the schema migration commit: a filled-in Executive Summary, a System Overview diagram, and a ## section for each of the four top-level modules (handlers, services, repositories, domain) — purpose, public interface, dependencies, and a small diagram for the one place the internal flow genuinely isn't obvious from the name (get_two_for_update()'s two sequential locking statements). The Executive Summary leads with the wallet-locking-order correction (ADR-0002/0003) since it's the one design decision a new reader would otherwise get wrong by reasoning from ADR-0002's original prose alone — exactly the kind of thing this doc exists to surface. Known Limitations left unchanged; all four bullets are still accurate. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Each one's own governing test scenarios are green: idempotency (ST1/ST2/ST7, CT4/CT5/CT7), concurrency/locking (CT1-CT7, ST3/ST4, scripts/simulate.py for CT6's 100-concurrent case), schema (LT1-LT4), OpenAPI contract-first (E2, the drift check), interface-first TDD (all four test tiers exist and pass), async/asyncpg (the whole implementation), single-request resolution (E1's status-code assertions), stored balance (LT4), and demo wallet seeding (the seed migration, verified both with and without SEED_DEMO_DATA). Per HANDOVER.md's Definition of Done — flipped individually because each one's tests are actually green now, not as a bulk status change. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Both were written pre-implementation and had drifted from reality: - AGENTS.md's project structure table was missing openapi/, scripts/, db/migrations/ + db/seed/, and Dockerfile; its docker-compose.yml description still said "Postgres only" after the app service was added. The concurrency-strategy bullet said "see the concurrency ADR once written" — ADR-0003 has existed and been Accepted for a while. "How to work here" didn't mention the containerized demo/run commands or that just install needs requirements.txt too. - HANDOVER.md is a point-in-time kickoff document, not a living one, but three of its claims were now flatly false rather than just dated: "No code exists yet", ARCHITECTURE.md described as "skeletal", and ADR-0002-0010 described as still Proposed. Added a status note at the top rather than rewriting the historical content, checked off the Definition of Done items that are actually true, and left the AI-disclosure and PR-opened items unchecked with a note on why (both are deliberately still pending). Also noted in §5 that the suggested parallel agent decomposition wasn't what actually happened — implementation ran sequentially. Also fixes a real bug surfaced while checking this: `just install` only installed requirements-dev.txt, never requirements.txt (uuid6, asyncpg, fastapi, uvicorn, pydantic) — a fresh clone following that exact documented step would end up missing every runtime dependency. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The branch is pushed now, so the drift check will run in GitHub Actions once the PR opens — the earlier note assumed it wasn't pushed yet.
The rest of the file describes the assignment template itself (how an interviewer sets up a candidate repo), not this solution — a reviewer landing on README.md had no way to find the actual design docs, how to run it, or how to test it without already knowing to look at ARCHITECTURE.md/HANDOVER.md. Adds a short "This submission" section up top with exactly those pointers, leaving the template content below it untouched. Satisfies the PR template checklist's "README or notes updated" item directly rather than relying on notes elsewhere to cover for it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Raw Claude Code JSONL logs, committed as one of the two methods
ASSIGNMENT.md's AI usage section explicitly sanctions ("add this to
the repo or email it"): 01-setup.jsonl (repo/ADLC-friendly project
scaffolding), 02-design-and-adr-review.jsonl (design phase plus the
independent adversarial architecture review that found and fixed the
contradictions documented throughout docs/decisions/adrs/, ending
with HANDOVER.md), and 03-implementation-and-tests.jsonl (this
session — implementation, the full test suite, the two real
concurrency bugs found via real-Postgres testing, Docker/demo
tooling, and PR preparation).
Scanned all three for common secret patterns (AWS/GitHub/Slack
tokens, private key headers) before committing — none found.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…emplate Replaces the template-facing content (Included/Intended use/Notes/How to Submit Assignment — interviewer-facing instructions for setting up a candidate repo) with a README describing the actual solution: what it is, an ADR table linking each of the ten (all Accepted) decision records, how to run it (`just demo`), how to test it (`just ci`), the API contract, and where the AI usage disclosure and session transcripts live. The previous version kept both; this one is a clean rewrite scoped to just this implementation. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Implements an idempotent, concurrency-safe POST /transfers API for wallet-to-wallet transfers with PostgreSQL-backed durability, pessimistic locking, and a double-entry ledger, following a strict handlers → services → repositories → domain architecture and ADR-driven design record.
Changes:
- Adds core transfer orchestration (idempotency record first, ordered wallet row locks, balance updates, ledger writes, bounded retry) and the FastAPI handler layer.
- Introduces PostgreSQL schema + migrations (Flyway) and supporting repository implementations (asyncpg UnitOfWork + repositories).
- Adds multi-tier tests (domain, service w/ in-memory fakes, repository/integration w/ testcontainers Postgres, E2E API tests) plus demo tooling and OpenAPI drift checking.
Reviewed changes
Copilot reviewed 76 out of 82 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_smoke.py | Placeholder smoke test |
| tests/service/test_transfer_service.py | Service-layer behavioral tests |
| tests/repository/test_ledger_correctness.py | Ledger invariants integration tests |
| tests/repository/test_concurrency.py | Concurrency/idempotency integration tests |
| tests/repository/helpers.py | DB helper utilities for tests |
| tests/repository/conftest.py | Repository-tier fixture wiring |
| tests/fakes/in_memory_repositories.py | In-memory repositories + UoW fake |
| tests/e2e/test_transfers_api.py | HTTP API end-to-end tests |
| tests/e2e/conftest.py | E2E FastAPI TestClient fixture |
| tests/domain/test_transfer.py | Domain transfer state machine tests |
| tests/conftest.py | Testcontainers Postgres + migrations fixtures |
| src/wallet_transfer/services/transfer_service.py | Transfer orchestration + retry |
| src/wallet_transfer/services/fingerprint.py | Request fingerprinting (SHA-256) |
| src/wallet_transfer/services/errors.py | Service-layer error types |
| src/wallet_transfer/services/init.py | Services package exports |
| src/wallet_transfer/repositories/wallet_repository.py | Wallet repository protocol |
| src/wallet_transfer/repositories/wallet_repository_asyncpg.py | asyncpg wallet repository |
| src/wallet_transfer/repositories/unit_of_work.py | UnitOfWork protocols |
| src/wallet_transfer/repositories/unit_of_work_asyncpg.py | asyncpg UnitOfWork + translation |
| src/wallet_transfer/repositories/transfer_repository.py | Transfer repository protocol |
| src/wallet_transfer/repositories/transfer_repository_asyncpg.py | asyncpg transfer repository |
| src/wallet_transfer/repositories/pool.py | asyncpg pool factory defaults |
| src/wallet_transfer/repositories/ledger_repository.py | Ledger repository protocol |
| src/wallet_transfer/repositories/ledger_repository_asyncpg.py | asyncpg ledger repository |
| src/wallet_transfer/repositories/idempotency_repository.py | Idempotency repository protocol |
| src/wallet_transfer/repositories/idempotency_repository_asyncpg.py | asyncpg idempotency repository |
| src/wallet_transfer/repositories/errors.py | Repository error types |
| src/wallet_transfer/repositories/init.py | Repositories package exports |
| src/wallet_transfer/handlers/transfers.py | POST /transfers handler |
| src/wallet_transfer/handlers/generated_models.py | OpenAPI-generated Pydantic models |
| src/wallet_transfer/handlers/app.py | FastAPI app factory + lifespan |
| src/wallet_transfer/handlers/init.py | Handlers package exports |
| src/wallet_transfer/domain/wallet.py | Wallet domain entity |
| src/wallet_transfer/domain/transfer.py | Transfer entity + transitions |
| src/wallet_transfer/domain/ledger_entry.py | LedgerEntry entity |
| src/wallet_transfer/domain/init.py | Domain package exports |
| scripts/simulate.py | Manual concurrency demo driver |
| scripts/check_openapi_drift.py | OpenAPI drift gate script |
| requirements.txt | Runtime dependencies |
| requirements-dev.txt | Dev/test dependencies |
| README.md | Updated project overview/run/test docs |
| pyproject.toml | Tooling config (ruff/mypy/pytest) |
| openapi/spec.yaml | Hand-authored API contract |
| justfile | Dev/CI task runner commands |
| HANDOVER.md | Historical implementation briefing |
| docs/decisions/adrs/adr-0010-demo-wallet-seeding.md | ADR: seed strategy |
| docs/decisions/adrs/adr-0009-stored-balance-not-derived.md | ADR: stored balance decision |
| docs/decisions/adrs/adr-0008-single-request-transfer-resolution.md | ADR: sync resolution |
| docs/decisions/adrs/adr-0007-async-io-asyncpg.md | ADR: asyncpg end-to-end |
| docs/decisions/adrs/adr-0006-interface-first-tdd.md | ADR: interface-first TDD |
| docs/decisions/adrs/adr-0005-api-contract-first-openapi.md | ADR: OpenAPI contract-first |
| docs/decisions/adrs/adr-0004-schema-and-migrations.md | ADR: schema + Flyway |
| docs/decisions/adrs/adr-0003-concurrency-locking-strategy.md | ADR: ordered locks + retry |
| docs/decisions/adrs/adr-0002-idempotency-strategy.md | ADR: idempotency records |
| docs/decisions/adrs/adr-0001-language-and-persistence-choice.md | ADR: Python + Postgres |
| docs/decisions/adrs/adr-000-madr-template.md | ADR template |
| docs/ai-transcripts/README.md | AI transcript index |
| Dockerfile | App container build |
| docker-compose.yml | Local Postgres + app stack |
| db/seed/V9001__seed_demo_wallets.sql | Flag-gated demo wallet seed |
| db/migrations/V4__create_idempotency_records.sql | Migration: idempotency_records |
| db/migrations/V3__create_ledger_entries.sql | Migration: ledger_entries |
| db/migrations/V2__create_transfers.sql | Migration: transfers |
| db/migrations/V1__create_wallets.sql | Migration: wallets |
| ARCHITECTURE.md | Architecture documentation |
| AGENTS.md | Agent/project hard rules |
| .python-version | Python version pin |
| .pre-commit-config.yaml | Pre-commit hooks |
| .importlinter | Layering enforcement rules |
| .gitignore | Python + env ignores |
| .github/workflows/ci.yml | CI workflow updated for Python |
| .env.example | Example env config |
| .dockerignore | Docker build ignores |
| .agents/skills/run-app/SKILL.md | Run/demo instructions skill |
| .agents/skills/new-adr/SKILL.md | ADR scaffolding skill |
| .agents/skills/assignment-review/SKILL.md | Self-review skill |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+10
to
+13
| async def insert(self, transfer: Transfer) -> None: | ||
| """Insert a new transfer row. Called with a PENDING transfer, before the wallet locks | ||
| are acquired (ADR-0002/ADR-0003 write order).""" | ||
| ... |
| -- Idempotency records. transfer_id's FK is DEFERRABLE INITIALLY DEFERRED because the write | ||
| -- order (ADR-0002) inserts this row *before* the transfer row exists, referencing a | ||
| -- pre-generated UUIDv7 transfer id (ADR-0004). A plain FK would reject the insert immediately; | ||
| -- deferring the check to commit lets the transfer row (inserted next, same transaction) land |
| @@ -0,0 +1,5 @@ | |||
| # Scaffold smoke test — proves the pytest gate is wired before real code exists. | |||
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.
Summary
Implements idempotent
POST /transferswallet-to-wallet transfers with a double-entry ledger and concurrency-safe balance updates, in Python/FastAPI/PostgreSQL against ten reviewed ADRs (docs/decisions/adrs/, all now Accepted). Layered handler → service → repository → domain architecture, enforced mechanically via.importlinter.AI disclosure
1. What tool you used: Claude Code (Anthropic), Sonnet 5, run interactively from the terminal against this repository.
2. How it's generally used: I've moved from standard SDLC to what I think of as ADLC — an AI-native, closed-loop development process, not just "AI-assisted coding" bolted onto an unchanged workflow. This repo is set up that way deliberately:
AGENTS.mdas the canonical, tool-agnostic agent context (CLAUDE.mdsymlinks to it), a versioned ADR trail as the actual design record, and a.agents/skills/directory of owned, reusable skills (/new-adr,/assignment-review,/run-app) rather than one-off prompts re-typed each session. The structure is inspired by 12-factor-agent principles and by my own OSS work on this (kaiverse-io/chassis) — setting a project up so it's legible to an agent from a cold start, and so multiple agents (or the same agent across sessions) can pick up the same context and collaborate on it consistently, not just one person's chat history.The practical consequence for how I review AI-generated work: it's no longer realistic to review thousands of lines of generated code line-by-line and call that the review. What I actually review is the ADRs and the implementation plan (
HANDOVER.mdhere) — the decisions, not just the diff. That's where I push back, ask "why," and force a rewrite before a single line of implementation exists, which is cheaper and more effective than catching the same issue in code review after the fact. Getting the agent to deliver quality code against that plan, not just propose it, is what the guardrails are for: pre-commit hooks (gitleaksfor secret scanning,ruff/mypy --strict/import-linterfor lint, types, and the layering contract), a CI gate (just ci) that has to pass before anything merges, and a real-Postgres test tier specifically because an in-memory fake can't prove concurrency correctness — all enforced automatically, not left to the agent's self-report of "looks good."3. Transcript: Committed to this PR under
docs/ai-transcripts/— full raw session logs (JSONL), not a summarized excerpt:01-setup.jsonl— repository/project setup02-design-and-adr-review.jsonl— design phase and the independent adversarial architecture review03-implementation-and-tests.jsonl— implementation, tests, and PR preparationSee
docs/ai-transcripts/README.mdfor what each file covers and how to read the format.Schema Design
Four tables (
db/migrations/), each constraint chosen to make an invalid state unrepresentable rather than relying on application discipline:wallets(id UUID PK, balance BIGINT CHECK >= 0, ...)transfers(id UUID PK, from_wallet_id/to_wallet_id FK, amount BIGINT CHECK > 0, status CHECK IN (...), failure_reason, CHECK from≠to, CHECK status≠'FAILED' OR failure_reason IS NOT NULL)ledger_entries(id UUID PK, transfer_id UUID NOT NULL FK, wallet_id FK, type CHECK IN ('DEBIT','CREDIT'), amount BIGINT CHECK > 0)— indexed onwallet_id,transfer_id;transfer_id NOT NULLmakes a ledger row with no transfer unrepresentableidempotency_records(idempotency_key TEXT PK, request_fingerprint, transfer_id UUID UNIQUE REFERENCES transfers DEFERRABLE INITIALLY DEFERRED)Every
idis a UUIDv7 generated application-side (not a DB default) — required because the idempotency record must reference a transfer's id before that transfer row exists. Migrations via Flyway, applied identically in local dev, CI (testcontainers), and prod — one schema source of truth.Idempotency Strategy
idempotencyKeyis required on every request. A durableidempotency_recordstable with aUNIQUEprimary key on the key, plus a SHA-256 fingerprint of(fromWalletId, toWalletId, amount), is inserted first, inside the same transaction as the transfer it guards — so the uniqueness guarantee is DB-enforced, not an application check-then-act race. Same fingerprint on conflict → return the cached terminal result; different fingerprint →409. Safe across process restarts (nothing tracked outside Postgres) and generalizes to any number of concurrent attempts — a client retry, a client double-fire, and the server's own internal bounded retries are all just transactions racing the same unique-constraint insert (proven under an explicit synchronization point intests/repository/test_concurrency.py::test_ct7_*, not timing luck).Concurrency Strategy
Pessimistic row locks (
SELECT ... FOR UPDATE) on both wallets, always acquired in ascendingwallet_idorder — what actually prevents deadlock between opposite-direction transfers on the same pair.SET LOCAL lock_timeoutper transaction bounds every lock wait; a bounded retry (3 attempts, exponential backoff+jitter) covers three retryable error classes. Insufficient funds is never retried — it's a terminal business outcome (200/FAILED), not a system error.One correction worth calling out explicitly: real-Postgres testing found that the wallet locks must be acquired before the transfer row is inserted, not after —
INSERT INTO transferstakes an implicit, unordered lock on each referenced wallet as part of its own FK check, which silently defeated the ascending-lock-order guarantee and caused genuine deadlocks under concurrency. This was invisible to the in-memory fake used for service-layer tests and only surfaced once the concurrency tests ran against real Postgres — documented as a correction in ADR-0002/ADR-0003, not silently patched.Verified two ways:
tests/repository/test_concurrency.py(CT1–CT7, real Postgres viatestcontainers, realasyncio.gatherconcurrency) andscripts/simulate.py(manual, against the real running HTTP API +docker-composePostgres — 100 concurrent same-wallet attempts, an opposite-direction pair, duplicate-key replay).How to Run
just demo— full containerized stack (Postgres + app + seeded demo wallets), orjust up && just migrate && just dev— Postgres only + local.venv+uvicorn --reloadSee
.agents/skills/run-app/SKILL.mdfor both modes and the concurrency demo.How to Test
just ci— lint, format-check, the full suite (domain, service-layer against an in-memory fake, repository/end-to-end against real Postgres viatestcontainers), and the OpenAPI drift check.just simulatefor the manual concurrency demo against a running instance.Tradeoffs / Assumptions
currencycolumn, conversion, or cross-currency check; deliberately out of scope.PENDINGis never durably observable.SEED_DEMO_DATA=true), sinceASSIGNMENT.mddoesn't ask for one.idempotencyKeytreated as required, not optional — a stricter reading than the spec strictly demands, flagged as a reviewed interpretation.just check-openapi-drift) is wired intojust cilocally, but this repo's CI workflow (.github/workflows/ci.yml) only maps three named steps to theLINT_CMD/FORMAT_CHECK_CMD/TEST_CMDrepo variables. Whoever configures those variables on this repo should make sureTEST_CMDincludes the drift check (e.g.just test && just check-openapi-drift, orjust ci) — otherwise it's silently skipped in CI even though it passes locally.Checklist