fix(txn,lock,mp): publish the SSI pivot-check-passed state so a commit-window write skew cannot commit (#136)

docs_src/api/c/txnbegin.md

@@ -83,7 +83,7 @@ The **flags** parameter must be set to 0 or by bitwise inclusively **OR**'ing to
 
   > **Note:** In this fork, `DB_TXN_SNAPSHOT` provides *serializable* snapshot isolation. In stock Oracle Berkeley DB, `DB_TXN_SNAPSHOT` provided only plain (non-serializable) snapshot isolation, and the earlier `DB_TXN_SNAPSHOT_SAFE` flag has been removed — there is no separate non-serializable snapshot mode.
 
-  > **Known limitation (as of 5.3.34).** Outside review found that the serializable guarantee is **not yet absolute**. A conflicting write that lands while another transaction is *inside* <a href="txncommit.md" class="xref" title="DB_TXN-&gt;commit()">DB_TXN-&gt;commit()</a> can escape detection, allowing a write skew to commit; and two records on different pages of one B-tree may not be detected as conflicting. Separately, long-lived environments running many snapshot transactions can exhaust the mutex region (`ENOMEM`) because reader bookkeeping is not fully reclaimed. Applications that depend on serializability for a correctness invariant should not yet rely on it alone. Tracking: issues #136, #137, #138 (and #140 for a related replication-path defect).
+  > **Known limitation (as of 5.3.34).** Long-lived environments running many snapshot transactions can exhaust the mutex region (`ENOMEM`) because reader bookkeeping is not fully reclaimed. Tracking: issues #137, #138 (and #140 for a related replication-path defect).
 
 - `DB_TXN_SYNC`
 

rfc/0003-ssi-serializable-snapshot-isolation.md

@@ -60,19 +60,23 @@ Both of Cahill's rw-conflict detection paths are implemented:
 
 SIREAD markers are reclaimed incrementally (not only at checkpoint).
 
-> **Known limitations (2026-09, from external reports #136–#140).** The claims in
+The commit-time pivot check is atomic with respect to conflict recording. Both
+pivot flags are read under `TXN_SYSTEM_LOCK` — the mutex every recorder
+(`__lock_get_internal`, `__memp_si_rwconflict`) takes around its flag
+read-modify-write — and, in the same critical section, a passing check publishes
+`TXN_DTL_SICHECKED` on the detail. That flag is what makes the window between
+the check and `__txn_end`'s `TXN_COMMITTED` store safe: a recorder that arrives
+during it sees that the committing transaction will not re-examine its flags and
+resolves the edge itself (`DB_SNAPSHOT_UNSAFE`) instead of deferring to a check
+that has already happened. Deferring on `status == TXN_RUNNING` alone was
+issue #136 — a write skew where both transactions committed; `test/isolation`
+gates it.
+
+> **Known limitations (2026-09, from external reports #137–#140).** The claims in
 > this RFC describe the *intended* design; the delivered behavior is weaker in
 > ways confirmed by outside review. Until the fixes land with regression tests,
 > treat the serializability guarantee as **best-effort, not absolute**:
 >
-> - **#136 — write skew can commit.** The commit-time pivot check is **not**
->   atomic with respect to the `TXN_RUNNING` → `TXN_COMMITTED` transition. A
->   conflicting write that lands while the first transaction is *inside*
->   `DB_TXN->commit` can leave both transactions committing, producing a state
->   with no serial order. A separate observation from the same report: two
->   records on *different pages of one B-tree* may detect no conflict at all.
->   (An earlier working note in this directory claimed this race was resolved;
->   that claim was wrong and is retracted.)
 > - **#137 / #138 — marker reclamation is not fully bounded.** SIREAD cleanup
 >   does not reclaim the deferred committed-reader locker, and
 >   `__txn_reap_si_details` frees a transaction detail without releasing its MVCC

src/dbinc/txn.h

@@ -114,6 +114,7 @@ typedef struct __txn_detail {
 #define	TXN_DTL_NOWAIT		0x10	/* Don't block on locks. */
 #define	TXN_DTL_WCONF		0x20	/* SSI: write end of an rw-conflict. */
 #define	TXN_DTL_RCONF		0x40	/* SSI: read end of an rw-conflict. */
+#define	TXN_DTL_SICHECKED	0x80	/* SSI: past its commit pivot check. */
 	u_int32_t flags;
 
 	SH_TAILQ_ENTRY	links;		/* active/free/snapshot list */
@@ -129,6 +130,35 @@ typedef struct __txn_detail {
 	roff_t slots[TXN_NSLOTS];	/* Initial DB slot allocation. */
 } TXN_DETAIL;
 
+/*
+ * TXN_SI_PAST_CHECK --
+ *	SSI: TRUE when this transaction will not consult its own pivot flags
+ *	again, so a writer forming an rw-conflict edge into it now cannot defer
+ *	the conflict to it and must resolve the edge itself.  That is the case
+ *	once the transaction has committed and, crucially, also while it is
+ *	inside DB_TXN->commit past its one and only pivot check: __txn_commit
+ *	publishes TXN_DTL_SICHECKED under TXN_SYSTEM_LOCK, atomically with that
+ *	check.
+ *
+ *	Without the flag the whole span from the pivot check to __txn_end's
+ *	status store still reads TXN_RUNNING, so a writer deferred to a check
+ *	that had already happened and both transactions committed a write skew
+ *	(issue #136).  An aborted transaction is deliberately NOT "past check":
+ *	its reads never committed, so an edge into it is not a conflict at all
+ *	-- hence the status test rather than testing TXN_DTL_SICHECKED alone,
+ *	which survives on the detail if the commit fails after publishing it.
+ *	In that narrow error window (commit published the flag, then failed and
+ *	his on its way to TXN_ABORTED) a writer may abort itself needlessly.
+ *	That errs safe -- a spurious DB_SNAPSHOT_UNSAFE, never a missed one --
+ *	and only on a path where the peer is already failing.
+ *
+ *	Callers hold TXN_SYSTEM_LOCK, which is where every writer of the pivot
+ *	flags and of TXN_DTL_SICHECKED serializes.
+ */
+#define	TXN_SI_PAST_CHECK(td)						\
+	((td)->status == TXN_COMMITTED ||				\
+	    ((td)->status == TXN_RUNNING && F_ISSET(td, TXN_DTL_SICHECKED)))
+
 /*
  * DB_TXNMGR --
  *	The transaction manager encapsulates the transaction system.

src/lock/lock.c

@@ -1102,22 +1102,34 @@ again:	if (obj == NULL) {
 				 * Record R --rw--> W under the txn-region mutex so
 				 * the two-flag reads and writes are atomic against
 				 * a concurrent recorder and the commit-time check.
+				 *
+				 * Both fate tests below ask "can R still resolve
+				 * this edge itself?", which is FALSE not only once R
+				 * has committed but also while R is inside
+				 * DB_TXN->commit past its pivot check --
+				 * TXN_SI_PAST_CHECK covers both, because
+				 * __txn_commit publishes TXN_DTL_SICHECKED under
+				 * this same mutex.  Testing R's status alone read
+				 * TXN_RUNNING for the whole span between R's check
+				 * and __txn_end, so we deferred to a check that had
+				 * already happened and both transactions committed a
+				 * write skew (#136).
 				 */
 				if (TXN_ON(env))
 					TXN_SYSTEM_LOCK(env);
 				if (F_ISSET(LOCK_OWNER(env, sireadlp),
 				    TXN_DTL_WCONF) &&
-				    LOCK_OWNER(env, sireadlp)->status ==
-				    TXN_COMMITTED)
+				    TXN_SI_PAST_CHECK(LOCK_OWNER(env, sireadlp)))
 					ret = DB_SNAPSHOT_UNSAFE;
 				else {
 					rwconf = 1;
 					/*
 					 * Set our incoming-conflict flag unless
-					 * the reader will itself abort.
+					 * the reader will itself abort -- which it
+					 * only will if it has a pivot check left.
 					 */
-					if (LOCK_OWNER(env, sireadlp)->status ==
-					    TXN_COMMITTED ||
+					if (TXN_SI_PAST_CHECK(LOCK_OWNER(env,
+					    sireadlp)) ||
 					    !F_ISSET(LOCK_OWNER(env, sireadlp),
 					    TXN_DTL_WCONF)) {
 						if (F_ISSET(LOCKER_TD(env,

src/mp/mp_fget.c

@@ -156,12 +156,16 @@ __memp_si_rwconflict(env, txn, visible_bhp)
 			break;
 		}
 		/*
-		 * Set W's write-end flag unless W already committed with the
-		 * read end set (W was itself a pivot that slipped through);
-		 * in that case the incoming edge still makes R the pivot.
+		 * Set W's write-end flag unless W already reached the point
+		 * where it will not re-examine its own pivot flags -- it has
+		 * committed, or it is inside DB_TXN->commit past its pivot
+		 * check (TXN_DTL_SICHECKED, published under this same mutex).
+		 * If W is there and already carries the read end, W is a pivot
+		 * that nobody will stop, so the incoming edge makes R abort
+		 * instead.  Testing W's status alone missed the committing case
+		 * and let a write skew commit (issue #136).
 		 */
-		if (F_ISSET(wtd, TXN_DTL_RCONF) &&
-		    wtd->status == TXN_COMMITTED) {
+		if (F_ISSET(wtd, TXN_DTL_RCONF) && TXN_SI_PAST_CHECK(wtd)) {
 			ret = DB_SNAPSHOT_CONFLICT;
 			break;
 		}

src/txn/txn.c

@@ -746,19 +746,34 @@ __txn_commit(txn, flags)
 	 * could produce a non-serializable schedule.
 	 *
 	 * The pivot flags are set on td by concurrent writers in the lock
-	 * manager, which serialize their flag writes on the txn-region mutex
-	 * (see __lock_get_internal).  Read both flags under the same mutex so
-	 * the two-flag test and this commit decision are atomic with respect
-	 * to a writer recording our second conflict edge -- otherwise an edge
-	 * set between the two reads, or just after they pass, would let a real
-	 * pivot commit.
+	 * manager and in mpool, which serialize their flag writes on the
+	 * txn-region mutex (see __lock_get_internal, __memp_si_rwconflict).
+	 * Read both flags under that mutex so the two-flag test is atomic with
+	 * respect to a writer recording our second conflict edge.
+	 *
+	 * This is our ONLY pivot check: everything below (cursor close, lease
+	 * check, log write) is past the point where aborting on a late edge
+	 * would be correct, and __txn_end does not publish TXN_COMMITTED until
+	 * much later.  So, in the same critical section, publish
+	 * TXN_DTL_SICHECKED: it tells a writer that arrives during that whole
+	 * span that this transaction will not look at its pivot flags again, so
+	 * the writer must resolve the edge itself (abort with
+	 * DB_SNAPSHOT_UNSAFE) rather than defer to a check that has already
+	 * happened.  Without it a writer treats us as "still running, already
+	 * flagged, it will abort at its check" and both transactions commit a
+	 * write skew (issue #136).
+	 *
+	 * Only a snapshot-safe txn is examined by those writers, so nothing is
+	 * published for the ordinary path.
 	 */
 	if (F_ISSET(txn, TXN_SNAPSHOT_SAFE)) {
 		int is_pivot;
 		if (TXN_ON(env))
 			TXN_SYSTEM_LOCK(env);
 		is_pivot = F_ISSET(td, TXN_DTL_WCONF) &&
 		    F_ISSET(td, TXN_DTL_RCONF);
+		if (!is_pivot)
+			F_SET(td, TXN_DTL_SICHECKED);
 		if (TXN_ON(env))
 			TXN_SYSTEM_UNLOCK(env);
 		if (is_pivot) {

test/isolation/README.md

@@ -38,11 +38,11 @@ The state vector has two kinds of slot:
 
 | Scenario | Shape | Expectation on master |
 |---|---|---|
-| `write_skew_trigger` | two one-page DBs; T2's write lands while T1 is inside `commit` | **XFAIL — reproduces #136** |
+| `write_skew_trigger` | two one-page DBs; T2's write lands while T1 is inside `commit` | PASS (`DB_SNAPSHOT_UNSAFE` to T2) — regression gate for #136 |
 | `write_skew_control` | two one-page DBs; T2 writes and commits before T1 commits | PASS (`DB_SNAPSHOT_CONFLICT` to T1) |
 | `write_skew_late` | two one-page DBs; T2 writes after T1's commit returned | PASS (`DB_SNAPSHOT_UNSAFE` to T2) |
 | `write_skew_samebtree_control` | two records on **different pages of one** B-tree; control timing | PASS |
-| `write_skew_samebtree_trigger` | same, trigger timing | **XFAIL — reproduces #136** |
+| `write_skew_samebtree_trigger` | same, trigger timing | PASS — regression gate for #136 |
 | `g2_antidep` | G2-item: both txns scan for markers, both insert one | PASS |
 | `read_only_anomaly` | Fekete's 3-txn pattern; the read-only txn's observation is checked | PASS |
 | `lost_update` | both txns read the counter and write read+1 | PASS |
@@ -83,7 +83,8 @@ purpose: **one** violation in any attempt is a reproduction, while a pass
 requires **every** attempt to be clean. A serializability violation is a real
 counterexample; a single clean run of a racy schedule proves nothing.
 
-In practice both #136 shapes violate on the first attempt.
+In practice both #136 shapes violated on the first attempt before the fix; both
+now report `DB_SNAPSHOT_UNSAFE` to T2 in every attempt.
 
 ## Running it
 
@@ -113,5 +114,6 @@ Environment: `CC`, `LIBDB_BUILD` (default `../../build_unix`), `ISO_TIMEOUT`
   `test_iso_anomaly.c`. The message says which.
 - `2` — harness error.
 
-When #136 lands, clear `expect_fail` on `write_skew_trigger` and
-`write_skew_samebtree_trigger`; the tier then gates the fix against regression.
+#136 is fixed (`TXN_DTL_SICHECKED`, see `rfc/0003`), so `expect_fail` is clear
+on every scenario and this tier is a plain regression gate: any violation is a
+new bug.

test/isolation/test_iso_anomaly.c

@@ -1019,7 +1019,7 @@ read_your_writes(iso_scenario *sc, iso_state *st, iso_txn *t)
 static iso_scenario scenarios[] = {
     { "write_skew_trigger",
       "two one-page DBs; T2's write lands while T1 is inside commit",
-      2, 2, 2, 1, "#136", 40, sk_trigger },
+      2, 2, 2, 0, NULL, 40, sk_trigger },
     { "write_skew_control",
       "two one-page DBs; T2 writes and commits before T1 commits",
       2, 2, 2, 0, NULL, 5, sk_control },
@@ -1031,7 +1031,7 @@ static iso_scenario scenarios[] = {
       2, 2, 2, 0, NULL, 5, sk_samebtree_control },
     { "write_skew_samebtree_trigger",
       "two records on DIFFERENT pages of ONE btree; trigger timing",
-      2, 2, 2, 1, "#136", 40, sk_samebtree_trigger },
+      2, 2, 2, 0, NULL, 40, sk_samebtree_trigger },
     { "g2_antidep",
       "G2-item: both txns scan for markers, both insert one",
       1, 1, 2, 0, NULL, 1, g2_antidep },