fix(txn): make the SSI commit-time pivot check atomic with the commit decision (#136)

[gburd]

The defect

A write skew commits under DB_TXN_SNAPSHOT when the second transaction's
write lands while the first is inside DB_TXN->commit. Both commits return 0
and the stored state has no serial order. Reported in #136 with a complete
reproducer and a correct root-cause analysis.

Two halves of one race:

(a) The commit-side check was not atomic with the status transition.
__txn_commit read both pivot flags under TXN_SYSTEM_LOCK, then released it —
but td->status stays TXN_RUNNING until __txn_end publishes
TXN_COMMITTED, far later, past cursor close, lease checks and the log write.
Throughout that span T1 looked like a running transaction whose pivot check was
still ahead of it. The in-code comment claiming the test and the commit decision
are atomic was true only of the two flag reads, not of the decision.

(b) The writer-side "reader will abort itself" optimization trusted that
stale status. In __lock_get_internal, T2 met T1's SIREAD marker, saw
status == TXN_RUNNING and TXN_DTL_WCONF already set, and concluded T1
would abort at a later pivot check. So it skipped the branch that would have
rejected T2 for its own existing TXN_DTL_RCONF, did not set WCONF on itself,
and only added RCONF to T1 — a transaction that had already passed its one and
only pivot check. T1 ended up holding both pivot flags with nobody left to look
at them; T2 held only RCONF; both committed.

Design chosen: option 1 — publish a "checked/committing" state under the same mutex as the check

A passing pivot check now publishes TXN_DTL_SICHECKED on the detail in the
same TXN_SYSTEM_LOCK critical section as the check itself. The writer-side
fate tests ask "can the peer still resolve this edge?" through a new
TXN_SI_PAST_CHECK(td) predicate instead of status == TXN_COMMITTED, so a
committing-but-not-yet-committed peer is treated like a committed one and the
writer resolves the edge itself (DB_SNAPSHOT_UNSAFE).

Why the alternatives are worse:

• Option 2, re-check after the point of no return — not correct. The
  "last moment" before status = TXN_COMMITTED in __txn_end is after
  __txn_regop_log/log flush and after __lock_vec(DB_LOCK_PUT_READ) released
  the read locks. Aborting there is not available: __txn_end is documented as
  unable to return an error and panics on failure, and the commit record is
  already durable, so a replica or a recovery pass would already have accepted
  the commit. It also cannot use goto err (which calls __txn_abort) because
  the transaction's locks are gone. The task brief anticipated this, and reading
  the code confirms it: the window is not abortable, so the state must be
  published at the decision, not re-examined after it.
• Making the writer test status != TXN_RUNNING — wrong direction: it would
  treat an aborted peer as "past check", turning every edge into a doomed
  transaction into a spurious abort of the writer. An aborted transaction's reads
  never committed, so an edge into it is not a conflict at all. Hence
  TXN_SI_PAST_CHECK tests TXN_COMMITTED || (TXN_RUNNING && SICHECKED).
• Atomic flags word / wider locking — the existing analysis note already
  rejected converting TXN_DETAIL.flags to an atomic: it carries many non-SSI
  bits and atomics alone would not make the decision atomic. Both sides already
  serialize on TXN_SYSTEM_LOCK; the missing ingredient was state, not more
  mutual exclusion.
• Abort the pivot the moment the second edge forms (no commit-time check at
  all) — arguably the better long-term shape, but a much larger behavioral change
  than a bug fix should carry.

Also fixed the same staleness at the mirror site in mpool
(__memp_si_rwconflict), which had the identical wtd->status == TXN_COMMITTED
test. The reported reproducer does not reach it, but it is the same defect one
mechanism over — the lazy fix is the one predicate used by both callers.

Constraints honoured

• No layout/ABI/format change. TXN_DTL_SICHECKED is the spare bit 0x80
  in TXN_DETAIL's existing u_int32_t flags word (bits through 0x40 were
  taken). Verified by compiling the struct both ways:

  	master	this branch
  sizeof(TXN_DETAIL)	344	344
  offsetof(flags)	140	140
  offsetof(links)	144	144
  offsetof(slots)	312	312
  sizeof(DB_TXNREGION)	208	208

  No on-disk, log or region-layout change; no public ABI change. No new message
  IDs (no new user-visible strings), so s_message_id was not needed.

• Lock ordering unchanged. No new mutex and no new nesting: both sides
  already took TXN_SYSTEM_LOCK around their flag access. The commit critical
  section grows by exactly one F_SET on a word already in cache.

• Existing correct behaviors preserved: control still yields
  DB_SNAPSHOT_CONFLICT, late still yields DB_SNAPSHOT_UNSAFE.

Proof the race is closed

The reporter's own reproducer, built against this branch, on both the debug
(--enable-debug --enable-diagnostic) and the release (CFLAGS=-O2) trees:

timing	master	this branch
trigger	both commit, alice=0 bob=0, no serial order	T2 put -> DB_SNAPSHOT_UNSAFE, alice=0 bob=1
control	T1 commit -> DB_SNAPSHOT_CONFLICT	unchanged
late	T2 put -> DB_SNAPSHOT_UNSAFE	unchanged

It is a race, so it was run repeatedly rather than once:

• debug build: 60 + 40 = 100 trigger runs, 100 control, 100 late -> 300/300
  serializable, 0 violations. Every trigger run gave DB_SNAPSHOT_UNSAFE;
  every control gave DB_SNAPSHOT_CONFLICT.
• release -O2 build: 25 runs per mode -> 75/75 serializable.
• On master the trigger reproduced on the first attempt, every attempt.

The isolation tier, expectations cleared

expect_fail is now clear on write_skew_trigger and
write_skew_samebtree_trigger (and the README table/exit-status section
updated), so the tier is a plain regression gate — any violation is a new bug.

9 scenario(s) run, 0 unexpected outcome(s)

Run 4x in full (each racy scenario doing 40 attempts) plus 5 extra runs of just
the two trigger scenarios: ~700 racy attempts, 0 non-serializable histories.

Both #136 shapes now show T2 read alice=1; put bob=0 -> DB_SNAPSHOT_UNSAFE.

On the reporter's "separate defect"

Confirmed not a second bug, with two independent constructions: the tier's
write_skew_samebtree_control (512-byte pages + filler keys, 33 leaf pages
verified via DB->stat→bt_leaf_pg, alice=min key, bob=max key) and the
reporter's own two-one-page-DB shape both return DB_SNAPSHOT_CONFLICT at
control timing and only fail at trigger timing. So the different-pages shape has
the same single root cause. The reporter never published their same-btree
variant, so their exact shape is unverified — with two records at the default
page size those sit on one page, a case they themselves report behaves
correctly. The samebtree_control PASS expectation is kept live so a real
page-granularity hole would surface there.

Regression matrix

Suite	Result
ssi001–ssi009	9/9 OK
txn001 / txn002 / txn003	OK
lock001 / lock002 / lock003	OK
recd001 / recd002 (btree)	OK
test001 btree / hash / queue / recno	4/4 OK
ssi009 + ssi001/002/004 under ASan	OK, 0 sanitizer reports
test/isolation/run.sh	9 scenarios, 0 unexpected (x4)
test/soak/run.sh	5 workloads, 0 unexpected, no slope exceeded
test/lockmatrix/run.sh	0 checks failed
test/fuzz/check-crashes.sh	9/9 PASS
test_sim_crash_recover (--enable-dst)	PASS, 64 committed txns survived, verifies clean

ASan runs used LD_PRELOAD=$(cc -print-file-name=libasan.so) ASAN_OPTIONS=detect_leaks=0 as required for the instrumented libdb_tcl.

Builds: --enable-debug --enable-diagnostic --enable-test,
--enable-diagnostic, release CFLAGS=-O2, --enable-debug --enable-dst,
ASan, and meson setup build && ninja -C build — all clean, 0 errors.

Measured commit-path cost

The fix adds one F_SET inside an already-held critical section, on the
SSI-only path (F_ISSET(txn, TXN_SNAPSHOT_SAFE)); non-SSI commits execute
identical code. test/bench/ssi_abort_bench, A/B against master built the same
way (-O2), alternating runs:

config	master	this branch
8 threads, hot=4096, 3s (n=7)	median 2532 txn/s	median 2356 txn/s
8 threads, hot=16, 3s (n=6)	median 2081 txn/s, abort 12.3–12.8%	median 2493 txn/s, abort 12.3–13.0%
4 threads, hot=4096, 10s (n=5)	median 1164 txn/s	median 1290 txn/s

Honest reading: this bench cannot resolve the cost. Run-to-run spread within
one config is ±2x (e.g. master alone ranged 1534–3066 txn/s at hot=4096), which
swamps any effect of a single store; the fix is ahead in two of three configs
and behind in one, which is noise, not signal. What is structural: the
critical section is not widened by any lock acquisition, loop or I/O — one bit
set on a word the same line already read — and the SSI abort rate is unchanged
(12.3–13.0% both sides at hot=16), so the fix is not converting benign
schedules into aborts at scale. It only aborts the writer in the specific
commit-window schedule that previously produced a non-serializable history.

Docs

• rfc/0003-ssi-serializable-snapshot-isolation.md — removed the libdb 5.3.34: DB_TXN_SNAPSHOT commits a write skew when the second write starts during the first commit #136 bullet
  from Known limitations (retitled to libdb 5.3.34: sequential read-only DB_TXN_SNAPSHOT transactions eventually cause DB_ENV->txn_begin to return ENOMEM #137–libdb 5.3.34: incomplete replication commit lock lists can violate client transaction isolation #140; the other items are left
  intact) and documented the delivered mechanism in Design.
• docs_src/api/c/txnbegin.md — removed the libdb 5.3.34: DB_TXN_SNAPSHOT commits a write skew when the second write starts during the first commit #136 half of the
  DB_TXN_SNAPSHOT known-limitation caveat, including the different-pages
  claim; the libdb 5.3.34: sequential read-only DB_TXN_SNAPSHOT transactions eventually cause DB_ENV->txn_begin to return ENOMEM #137/libdb 5.3.34: __txn_reap_si_details leaks MVCC mutex slots during snapshot cleanup #138 mutex-exhaustion caveat stays.
• test/isolation/README.md — expectation table, exit-status section.

Fixes #136

---

Supersedes #150, which was opened from a pre-rebase base. That branch is 3 commits stale and merging it would have deleted the 14 coverage-driver files added in #147 (-5304 lines); this branch is the same change rebased onto current master.

[github-actions]

Coccinelle convention checks

No new violations. ✅

Resolved since baseline (2) -- update dist/cocci/baseline.txt to lock these in.

rule_mutex_unbalanced|MUTEX_UNBALANCED|src/crypto/mersenne/mt19937db.c|return (ret);
rule_mutex_unbalanced|MUTEX_UNBALANCED|src/mp/mp_register.c|return (ret);

[github-actions]

ABI diff vs v5.3.34 (libabigail — authoritative)

Functions changes summary: 0 Removed, 0 Changed, 2 Added functions
Variables changes summary: 0 Removed, 0 Changed, 0 Added variable

2 Added functions:

  [A] 'function int __lock_sireap_lockers(ENV*)'    {__lock_sireap_lockers}
  [A] 'function int __os_csprng(ENV*, void*, size_t)'    {__os_csprng}

Removed exported symbols (nm -D, _NNNN version suffix normalized)

None.

---

Advisory: libabigail/nm is the authoritative binary-ABI check; Coccinelle is complementary source-level early warning. See dist/cocci/README.md.