Skip to content

Fix spurious failures when upgrading private inner nodes - #43

Open
Konstantin V Shvachko (shvachko) wants to merge 1 commit into
microsoft:mainfrom
shvachko:fix/inner-lock-spurious-cas
Open

Konstantin V Shvachko (shvachko) wants to merge 1 commit into
microsoft:mainfrom
shvachko:fix/inner-lock-spurious-cas

Conversation

@shvachko

Copy link
Copy Markdown

Summary

Use strong compare_exchange in ReadGuard::upgrade so a permitted spurious weak-CAS failure is not reported as TreeError::Locked on an unchanged private inner node. The production change preserves the expected version, the new version, both memory orderings, and genuine contention/stale-version rejection.

Root cause

Inner-root splitting (tree.rs, new-root publication) and snapshot recovery (snapshot.rs, inner-node reconstruction) upgrade newly allocated, unpublished inner nodes and unwrap the result. The old helper made a single compare_exchange_weak attempt. That operation is allowed to return Err(expected) even when no other thread changed the lock.

On Windows ARM64 this was observed during native loading. An isolated probe of the unchanged production helper recorded 989 unchanged-version failures in 100 million uncontended attempts; a strong-CAS control recorded none. This is a portable API-contract bug, not an ARM64-specific requirement. Returning a retry from the root-publication site is not a safe substitute: the old root has already been split/demoted there.

This is separate from the allocation-publication race in #42 and does not incorporate that PR or #41.

Regression coverage

  • Deterministic, architecture-independent integration fixture compiles the actual production helper via #[path] against a minimal test-only node/atomic shim. The shim injects one permitted unchanged-value weak-CAS failure; there is no fault-injection hook or additional abstraction in the library.
  • Verified the private-node regression fails on the original helper (Locked, version zero), then passes with strong CAS.
  • Controls cover the injector itself, held write locks, stale readers after a completed writer, and downgrade/drop version progression.
  • Native regression inserts 18,000 records over three rounds. It asserts at least three tree levels, proves additional inner-node splits in every round (including after recovery), checks every key/value and the complete ordered scan against a BTreeMap, snapshots/reopens, and verifies sealed snapshot bytes remain unchanged.
  • The native regression disables read/scan promotion to isolate structural growth/recovery from separate full-cache promotion defects. This is test isolation, not a proposed production configuration workaround.

Shuttle 0.7.1 does not model spurious integer weak-CAS failures, so its ordinary concurrency test cannot replace the deterministic fixture. CI Shuttle commands now explicitly select --lib: the existing Shuttle platform shims require cfg(test), while the new integration fixture is a separate crate. The native integration fixture still runs under ordinary cargo test and the sanitizer job.

Validation

Windows ARM64, Rust 1.93.0:

  • Original helper: deterministic negative control failed as expected (exit 101).
  • cargo test --release --test inner_lock_spurious: 5 passed.
  • Native Release inner-node/root-growth + existing tree tests: 12 passed.
  • Native Debug cargo test --lib inner_root_split: 1 passed.
  • cargo test --release --lib --features shuttle shuttle_bf_tree_concurrent_operations: passed, four PCT runners with 4,000 executions each.
  • cargo fmt --all -- --check: passed.
  • cargo clippy --release --tests: passed, existing upstream warnings; none reported in the added tests.

A broader native run is not claimed green: the existing snapshot::tests::cpr_snapshot_cache_only aborts at storage.rs:183 (allocation write-lock unwrap), followed by cleanup failure. Reproduced independently on untouched upstream ca61307 and on this branch. That allocation-publication defect is addressed separately by #42; it is not bundled here. Linux/x64 and hosted CI results remain pending.

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.

1 participant