fix: concurrency correctness and release hardening - #46
Conversation
The generated upsert retries a locked update/insert whenever a racing unlocked insert/delete moves the row out from under its locked decision (NotFound / row-absent). Raw insert and delete do not join the row lock, so the previous hot yield_now spin could livelock the upsert against sustained same-key churn, starving upserter tasks past their 60s test timeout (raw_insert_delete_churn_never_panics_or_stalls failed ~28% of integration runs). Escalate the retry to bounded exponential backoff (yield for the first 8 spins, then capped micro-sleeps) so the racing mutation's publication settles and the upsert makes forward progress. Full integration matrix: 50/50 clean (was 36/50). Addresses issue #37.
|
Added commit The generated upsert retry loop used a hot Validation on this branch:
Note for reviewers: verified against the branch that several catalog items are already resolved here — #15 (tuple-key persisted reload test passes) and #36 (Running/Closing/Failed/Closed lifecycle + |
|
Added commit These benchmarks are retained past CIDR-2027 for ongoing v1+ release validation, so they belong on the v1 branch. The move also finishes latent breakages the original crate carried (it never compiled against current Verified: all three binaries ( |
1113e9a to
7eab7c1
Compare
|
Correction: commit |
Updating a String-bearing row to a value that fits its slot must be an in-place mutation, not a delete+reinsert. Observed via the row's physical Link: reinsert moves it to a new slot. All three cases currently FAIL — same-length, shorter, and repeated same-length updates all reinsert, which is the overwrite perf bug (WorkTable 9x slower than sqlite on update while leading insert/read).
… cause) The overwrite perf bug is root-caused: custom-update gen_size_check inits need_to_reinsert = true then ORs the size-changed check, so EVERY update to a String-bearing table reinserts (fresh slot + re-serialize + full re-index) even when nothing grew — WorkTable leads insert/point_read but is dead last on overwrite. Flipping the initializer to false lets same-length updates skip reinsert, but the in-place archived write of a String field then corrupts rows in existing unsized tests (update_parallel_more_strings, update_many_times, in_place multithread). So a real fix must make the in-place write of an equal-length archived String safe — a storage-path change, not a one-liner. Codegen reverted to master (no behavior change / no breakage); the regression test is committed #[ignore]d with the full analysis so the fix has a proof to turn green.
|
Stacked two more commits onto this PR (single WT review PR):
PR now contains 6 commits: #40 row-publication, #23 integration, concurrent-mutation stabilization, #37 upsert livelock, + the overwrite-bug regression. |
WTI PR #6 gained the review-response commit that gates the positional delete additions behind feature=multimap. Repin from 6f40801 to 3929082 so this PR consumes the CI-green WTI. Builds clean.
… debug_assert Adds tests/worktable/vacuum_no_row_loss.rs: 400 large rows, half deleted across many pages, concurrent grace-period reader, full survivor audit by primary key + unique index after vacuum quiesces. PASSES — no row loss or resurrection; the #46 grace-period deferral holds. Adds a debug_assert_ne!(page_from, page_to) in the compaction loop making the invariant explicit (F2): the source being reclaimed post-loop must never be the move destination. The assert does not fire under the audit, confirming the invariant holds today while guarding future regressions. Updated docs/pr46-review-findings.md accordingly.
…d (not reproduced) Fresh-eyes review of the v1-blocker PR. Adds docs/pr46-review-findings.md tracking all six findings, and a standing guard test for the highest-rated one (mutation gate held across .await). F1 (gate-across-await livelock): reproduced the exact scenario — colliding same-stripe keys, single-worker + 2-worker runtimes, updates awaiting while holding the gate — and it PASSES. tokio's cooperative scheduling plus the thread::yield_now() fallback keep the parked holder pollable, so no livelock. Finding downgraded; test kept (timeout-wrapped, can't hang the harness) to catch a future regression. Remaining findings (F2 vacuum data-loss, F3-F6) tracked in the doc.
… debug_assert Adds tests/worktable/vacuum_no_row_loss.rs: 400 large rows, half deleted across many pages, concurrent grace-period reader, full survivor audit by primary key + unique index after vacuum quiesces. PASSES — no row loss or resurrection; the #46 grace-period deferral holds. Adds a debug_assert_ne!(page_from, page_to) in the compaction loop making the invariant explicit (F2): the source being reclaimed post-loop must never be the move destination. The assert does not fire under the audit, confirming the invariant holds today while guarding future regressions. Updated docs/pr46-review-findings.md accordingly.
…d (not reproduced) Fresh-eyes review of the v1-blocker PR. Adds docs/pr46-review-findings.md tracking all six findings, and a standing guard test for the highest-rated one (mutation gate held across .await). F1 (gate-across-await livelock): reproduced the exact scenario — colliding same-stripe keys, single-worker + 2-worker runtimes, updates awaiting while holding the gate — and it PASSES. tokio's cooperative scheduling plus the thread::yield_now() fallback keep the parked holder pollable, so no livelock. Finding downgraded; test kept (timeout-wrapped, can't hang the harness) to catch a future regression. Remaining findings (F2 vacuum data-loss, F3-F6) tracked in the doc.
Summary
Single rolling review PR for WorkTable's concurrency-correctness and beta hardening work. Supersedes #44 and #45 without claiming the separate unsized-update performance issue is fixed.
Closes #37.
Review findings resolved
WorkTablesIndex =0.0.5directly and throughdata_bucket =0.5.2; removes the ineffective downstream[patch.crates-io]override.saturating_add, preventing debug/test overflow.update_in_place_unsized.rs.Dependency status
WorkTablesIndex 0.0.5is published and contains the exact multimap-removal fix.data_bucket 0.5.2is published and depends onWorkTablesIndex =0.0.5.Performance boundary (explicit)
page_accessis currently oneRwLock<()>per table. Archived-page insert, update, delete, hydration, reset/reuse, and vacuum mutations that take its exclusive side therefore form a table-wide writer barrier, including for disjoint rows/pages. README and protocol documentation now say this explicitly.The same-key correctness fix adds one primary-key hash and an uncontended FIFO-ticket acquire/release to mutation operations. Its throughput and p99 impact has not yet been measured.
This PR does not claim HFT contention validation is complete. A fresh throughput and tail-latency campaign for the mandatory publication and mutation-gate paths remains a release/deployment gate.
The ignored
same_length_update_stays_in_placefixture also remains an open performance issue: equal-lengthStringupdates currently reinsert because the attempted in-place archived-string write is unsafe/corrupting. The fixture documents the blocker; this PR does not claim to resolve it.Commits (15 on top of
master)e39364cfix: make row publication concurrency-safe by default6515ba6fix: avoid stale multimap removal lookup3c248fdfix: stabilize concurrent mutation tests7eab7c1fix: bound upsert retry backoff to prevent same-key churn livelock8d4e9f2test: regression for unsized-update in-place (currently failing)4f3f790test: document unsized-update reinsert bug (ignored regression + root cause)34fcad4build: bump WorkTablesIndex pin to reviewed rev (multimap-gated)111f61etest: bound vacuum soak to prevent orphaned harnesses87f3dfbfix: cap upsert retry shift before backoffb569492build: consume published index dependency chainfdc780bfix: defer vacuum page reuse through read gracefeba51dtest: audit index invariants after mutation churn274da81docs: disclose table-wide publication writer barriera21cb07fix: avoid overlapping page and link reclamatione730460fix: serialize synchronous insert with row mutationsLocal verification
cargo fmt --all -- --checkcargo clippy --workspace --all-targets -- -D warningscargo clippy --workspace --all-targets --all-features -- -D warningscargo test --workspace --all-targets— passedcargo test --workspace --all-targets --features versioned-row-publication— passedcargo test --workspace --all-targets --all-features— WorkTable: 146 unit + 342 integration passed, 5 integration tests ignoredCI is the final authority for the pushed branch.