Skip to content

Serialize decision ledger appends through the store lock - #36

Open
HUAN2022A wants to merge 1 commit into
tigerless-labs:mainfrom
HUAN2022A:fix/atomic-ledger-append
Open

HUAN2022A wants to merge 1 commit into
tigerless-labs:mainfrom
HUAN2022A:fix/atomic-ledger-append

Conversation

@HUAN2022A

Copy link
Copy Markdown

Fixes #19

What was wrong

DecisionLedger.append was an unlocked read-modify-write: it read the whole
ledger, concatenated one decision, and wrote the file back. Two concurrent
appenders (a sleep and a decide, or two decide calls) could both read the
same content and each write back only their own line, so the last writer
silently dropped every decision appended in between. The ledger is truth, not
cache: losing a rejection verdict means the operator's refusal is forgotten and
the same proposal can come back on the next sleep.

The fix

  • DecisionLedger now takes the store layout and derives the ledger path from
    it; Manage._ledger was its only construction site.
  • append runs under the store lock, so every writer passes through the one
    advisory lock, and stages the new content into a sibling .pending file that
    atomically replaces the ledger — a crash can no longer leave the truth file
    truncated mid-write.

Reproduction

Three processes appending six decisions each (18 expected), on the code before
this change:

run survived
1 13
2 18
3 3

After the change the same reproduction keeps all 18 in every run, and
test_two_processes_appending_decisions_lose_nothing in
tests/system/test_concurrency.py pins that invariant.

Verification

On Linux, Python 3.12, from a clean checkout of this branch:

  • uv run pytest -q — 389 passed (388 before, plus the new test)
  • uv run ruff check . — clean
  • uv run mypy — clean

One scope note: decided() still reads without the lock, so a reader can
observe the ledger mid-replace; since the replace is atomic the worst case is
reading the pre-append state, never a torn file. Left out of this change since
the issue is about lost appends, but happy to take it further if you want
readers serialized too.

Concurrent appends each read the ledger, appended one decision, and wrote
the whole file back, so the last writer silently dropped every decision
appended in between; a sleep and a decide racing could forget an operator's
refusal. DecisionLedger now takes the store layout and appends under the
store lock, staging the new content and replacing the file in one step.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

DecisionLedger.append is a non-atomic read-modify-write

1 participant