ADR-016: Ledger Write Idempotency (Replayable Triggers Must Have an Atomic Idempotency Gate)

Status: Proposed Date: 2026-06-16 Author: Code Review

---

Background

"Money" is spread across two gitignored stores: memory/portfolio.md (cash + holdings) + db/trades.db (trade ledger). Any write path that applies a logical event to the ledger, if that event can be replayed/re-triggered (HTTP retry, email polling, cron re-run, agent resend, double-click), and the code uses "additive" semantics (cur_units + units, cash -= amount) with no deduplication by event, will result in duplicate entries— real money is at stake.

This class of bug has bitten us twice:

#	Path	Trigger	Root Cause	Fix
1	PortfolioManager.record_external_trade (CommSec email import)	Email polling re-run / concurrent commsec_apply	processed_emails marked outside portfolio-tx, check→mark has TOCTOU	#62: Atomic state_claim(email_id) before touching ledger, state_unclaim on failure for retry
2	web_api patch_trade_status → _sync_trade_to_portfolio (web/GUI/agent manual PATCH executed)	Double-click / client timeout retry / agent resend	Endpoint only checks target status, not current status; _sync is additive, but docstring falsely claims "idempotent"	State machine guard: trade_before["status"] determines transition; re-PATCHing an already executed trade returns idempotently
3	Same as above (concurrent scenario, #109)	Two concurrent PATCH executed requests arrive simultaneously	The state machine guard's check (read trade_before.status) and set (patch_status) are lock-free, a three-step read-modify-write, not atomic	#127: TradesDB.claim_status_transition single SQL UPDATE ... WHERE status != ? atomic CAS, replacing the state machine guard as the current mechanism for this path

After #1 was fixed, a full audit of all ledger write paths on 2026-06-16 uncovered a third issue (payday TOCTOU). In #127, a fourth sub-issue was found on the same patch_trade_status path (see below). This ADR formally documents this class of invariant and includes the complete audit table to prevent a fifth occurrence.

Two Sub-Issues Fixed Together in #127 (Same Path, Not Counted as Separate Incidents, Recorded Here for Reference)

1. The rollback after a CAS winner's sync failure was previously an unconditional UPDATE—patch_status(trade_id, trade_before.status) did not check the current state. If, during the rollback, another concurrent request had changed the row to a different state (e.g., cancelled), the unconditional rollback would silently overwrite the other request's legitimate write. Changed to TradesDB.release_claim(trade_id, from_status="executed", to_status=trade_before.status)—only rolls back if the row is still in the executed state just claimed by this request; otherwise returns False + logs an error, never overwriting another's write.
2. _sync_trade_to_portfolio treated a "post-persist pm._reload() failure" as a "sync failure"— Upon exiting with_portfolio_tx, the data is already fsynced + atomic rename persisted. The subsequent pm._reload() only refreshes the in-memory view and is unrelated to the success of this write; if its exception is also treated as return False, None, the caller would misclassify an already-effective write as "not synced" → rollback state → allow retry → replay the same delta (stacking on top of an already-persisted write, creating a new double-spend window). Fix: pm._reload() is wrapped in its own try/except; failure only logs a warning, does not affect the (True, ...) return value.

Decision

Rule: Replayable ledger writes must have idempotency guarantees based on "logical event deduplication"

Any write path that satisfies "additive semantics + replayable trigger" must have one of the following:

1. Atomic Idempotency Gate—Before touching cash/holdings, use MemoryStore.state_claim(name, key) to atomically claim the deduplication key for that event (email_id / YYYY-MM / trade state transition); claim failure = already processed = early exit; on application failure, state_unclaim allows the next retry. (check+append under the same fcntl lock, eliminating TOCTOU.)
2. State Machine Guard—Read the current persistent state before applying; only apply once on a "real state transition" (patch_trade_status: sync only on non-executed → executed).
3. SET / reject-duplicate semantics—Written as idempotent overwrites (PUT holdings = SET) or reject duplicates (POST holdings symbol already exists → 409), inherently safe, no deduplication key needed.

docstring must not falsely claim "idempotent"—unless one of the three above actually exists in the code. Additive functions are never idempotent by themselves.

Residual Window (Accepted Trade-off)

If a SIGKILL/OOM occurs after the atomic claim is written to disk but before the portfolio-tx is committed → the key is claimed but the ledger is unchanged, the event is silently skipped, requiring manual state_unclaim for replay. This is better than "crash after commit = double-entry real money," so it is accepted. (See record_external_trade comments.)

2026-06-16 Audit Results (All Ledger Write Paths)

Path	Trigger	Semantics	Idempotency Guarantee	Conclusion
record_external_trade	Email polling / commsec_apply	Additive	state_claim(email_id)	✅ #62
commsec_apply endpoint	HTTP POST	—	Pass-through from above	✅
patch_trade_status → _sync_trade_to_portfolio	PATCH retry / double-click / concurrent	Additive	Atomic claim_status_transition CAS (#127, replacing original state machine guard) + rollback uses release_claim CAS (#127)	✅ #127
payday_check.run → add_income	cron + manual /payday concurrent	Additive	Atomic state_claim(YYYY-MM) (this PR)	✅ This PR
POST /api/holdings	HTTP POST	append	Symbol already exists → 409	✅
PUT /api/holdings/{symbol}	HTTP PUT	SET	Overwrite	✅
DELETE /api/holdings/{symbol}	HTTP DELETE	remove	Second time → 404	✅
napcat /deposit /withdraw /gold*	QQ private chat	Additive	Events within single WS connection processed serially, no replay	✅ (Direct user action)
deposit_cash / withdraw_cash / buy / sell / delete_holding	CLI / /api/skill/* / /api/deposit etc.	Additive	No deduplication key	⚠ See "Open Issues" below
db.trades_db.record_trade	/api/trades/record retry	INSERT	No deduplication key	⚠ Agent retries produce duplicate planned rows, but each row is executed once (apply-side idempotency is already guarded)

Open Issues (Systemic, Requires User Decision, Not in Scope of This PR)

All "additive POST write endpoints" lack an idempotency key (/api/deposit /api/withdraw /api/gold/buy|sell/api/skill/buy|sell). Under the local same-process GUI/CLI, a single click = a single intent, which is generally fine; but under the hub-and-spoke remote mode (INVEST_API_BASE), HTTP client timeout retries will cause double application—this is a common REST "POST non-idempotent" problem. A complete fix requires introducing an Idempotency-Key header scheme for write endpoints (client generates UUID, server deduplicates by key for N minutes). This is a design decision left for the user to decide if and when to implement.

Duplicate planned rows for record_trade could similarly be addressed with a (verdict_id, symbol, intended_date) unique constraint, but this is low priority (each row is executed once; real money is guarded by apply-side idempotency).

Non-Goals

• Do not merge the two sync paths: record_external_trade (CommSec = real settled, sold-no-holdings still records cash) and _sync_trade_to_portfolio (web manual, sold-no-holdings _SkipSync rollback) have intentionally different semantics; do not extract into a shared underlying function. See comments at both locations.
• This PR does not implement reverse reversal for executed → cancelled (rolling back cash/holdings when canceling an executed order)— a separate change, in a separate PR.
• This PR does not introduce the Idempotency-Key endpoint scheme (see "Open Issues").

References

• CLAUDE.md "Public Data Red Line #4" (Ledger Consistency)—already linked to this ADR
• #62 fix(portfolio): make CommSec record_external_trade idempotent
• core/memory_store.py:state_claim / state_unclaim (atomic deduplication primitives)
• tests/test_trades_portfolio_sync.py::TestPatchStatusIdempotency (#1 regression guard)