Conversation
added 15 commits
July 27, 2026 06:07
The generated lock acquisition did `lock_manager.get(&pk)` and, on miss, built a lock and `insert`ed it. Two tasks could both miss, both build, and both enter the row: the loser's insert returned the winner's lock and merged into it, but the winner had already registered its operation on a lock that was no longer in the map, so its wait set was computed before the loser existed. Neither waited for the other. Observed as lost updates: test_update_val_by_id_two_thread asserting 20_000 increments intermittently saw 19_999 (~8% of full-suite runs). Fix: LockMap::get_or_insert_with does the check and the insert under one guard, with a read-lock fast path for the common already-present case so unrelated rows' acquisitions stay concurrent, and a write-lock re-check slow path so two concurrent misses cannot both construct. Both generator lock paths (custom and full-row, in-memory and persist) collapse their two-branch acquire into this call. LockGuard cleanup re-opened the same window, so remove_with_lock_check now also requires Arc::strong_count == 2 (the map entry plus the local clone) before removing an unlocked entry: any higher count is a task that already took the Arc and is about to register on it. The read fast path clones under the read guard, so cleanup (which needs the write lock) either runs before the lookup or sees the extra reference. Documented trade-off: a task cancelled between taking the Arc and registering leaves one empty entry behind until the next operation on that key; mutual exclusion is unaffected. The generated custom-update methods are generic over Pk and open with `let pk = pk.into();` whose type was only pinned by the removed `get(&pk)` call; the binding is now explicitly typed `pk: #pk_ident` (the delete generators already did this). Measured: lost-update tests 0/150 full-suite runs after, 3/40 before.
EmptyDataVacuum::full_row_lock had a copy of the check-then-act pattern
fixed in the generated lock paths: get, and on miss build-and-insert,
letting a vacuum move and a row operation both believe they hold the
row. Collapse it into the same atomic LockMap::get_or_insert_with call.
The generic vacuum only knows `LockType: RowLock`, so the trait gains
`fn new()` (a lock with no columns held). This is a deliberate breaking
addition to a public trait, acceptable within the unreleased 0.9 line;
all real implementations are generated by this crate's own macro. The
three lock generators implement it by delegating to the generated
inherent `new`; FullRowLock uses the new explicit `Lock::new_released`
constructor for its placeholder state ('released' names the acquisition
flag, distinguishing it from `Lock::new`, which starts held) instead of constructing a held
lock and immediately releasing it.
Generated upsert did `pk_map.get(&pk)` then update() or insert() - a
check-then-act window. A concurrent delete of the same key between the
check and the update made upsert return NotFound, which the caller never
expects from an upsert. Observed via
tests/worktable/vacuum.rs::vacuum_parallel_with_upserts (~5% of
full-suite runs), whose delete task races 3000 upserts.
Fix: retry loop over three transient conflicts, each of which proves a
concurrent operation on the same key completed:
* update() hitting NotFound (concurrent delete) retries as insert;
* insert() hitting a primary-key conflict (concurrent insert) retries
as update, via the new typed WorkTableError::PrimaryAlreadyExists
(replacing a stringly AlreadyExists("Primary") comparison);
* update() hitting a row-absent PagesError retries: raw insert takes no
row lock and publishes the pk entry before unghosting the row data,
so a reader in that window sees Ghosted for a row that is about to
exist (the underlying publish-order issue is tracked in #169). The
classification comes from the new ExecutionError::is_row_absent
(Ghosted / Deleted / Vacuumed / PageNotFound), which the later
snapshot patches reuse.
Secondary unique-index conflicts propagate: retrying them can never
succeed.
Progress semantics, documented on the generated method: system-wide
lock-free, NOT wait-free per call. A retry is only taken when another
operation on the key completed in the window, so the system makes
progress on every iteration, but an individual call can retry
unboundedly under sustained adversarial same-key churn. There is
deliberately no retry limit - upsert is semantically infallible for
primary-key conflicts, and a limit would trade theoretical starvation
for real spurious errors. Each conflicting round yields to the
scheduler (tokio::task::yield_now - no timer dependency, so it cannot
panic on runtimes without enable_time) before retrying.
Tests: tests/worktable/upsert.rs drives sustained existence-flipping
churn on one key against four concurrent upserters under a timeout that
turns starvation into a failure. Both tiers are #[ignore]d for now:
they expose pre-existing engine races between the lock-free insert
path and locked mutations (panics and stalls that reproduce WITHOUT
upsert involvement; the moderate tier is 40/40 stable solo but still
hits the stall ~1/30 under full-suite parallel load). Repro recipes
and analysis in #169; un-ignore both tiers when it lands.
Measured: vacuum_parallel_with_upserts 0/100 full-suite runs after,
2-3/50 before.
Three problems in `update_X_by_<non-unique-index>`: 1. It scanned the index twice: once to acquire row locks and again to choose the rows to process. A row entering the matched range between the scans was processed without any held lock, and in the unsized branch `guards.remove(&pk).expect(..)` would panic on it. 2. A non-unique index iterates equal keys in random-discriminator order (indexset MultiPair uses fastrand), so the scanned order differed on every table load. When the update fails partway (unique-index conflict on a later row), which rows were already updated - in memory AND on disk - was a coin flip. This was the actual cause of test_update_unsized_larger_last_fail (~50% of full-suite runs); the persisted state faithfully recorded a legitimate alternate outcome, there was no persistence-layer rollback bug. 3. The per-query lock set only covered the updated columns, so nothing stopped a concurrent operation from rewriting the predicate column between a revalidation read and the update. Fix: snapshot the matching rows' primary keys once, sort them (one global lock-acquisition order for concurrent multi-row operations), acquire exactly those locks, then process exactly that set. The generated per-query lock now includes the predicate (`by`) column in addition to the updated columns, so the re-validation read under the held lock is authoritative: rows deleted or updated out of the matched range before their lock was taken are skipped, rows that joined after the snapshot are not touched (statement-start snapshot semantics, revalidated under lock). Snapshot resolution distinguishes "row is gone" from real storage errors via ExecutionError::is_row_absent (introduced with the upsert fix): absent rows silently leave the snapshot, anything else (corrupt page, invalid link) propagates instead of masquerading as an empty slot. Measured: 0/50 full-suite runs after, 19-28/50 before; 40/40 after the predicate-lock revision.
The previous shape collected storage links, sorted them, and later resolved each link to a row to delete. Storage links are not stable row identities: a concurrent delete can free a slot and an insert can reuse it for an unrelated row before the loop runs, so a stale link could resolve to - and delete - a row that never matched the query. Fix: snapshot the matching rows as primary keys, validated against the predicate at resolve time (a reused slot only stays in the set if the row now living there genuinely matches). Keys are sorted and deduped for a deterministic delete order (non-unique indexes iterate equal keys in random-discriminator order), and each delete goes through the ordinary single-row delete, which takes its own row lock and resolves by primary key, never through the snapshotted link. A row already deleted concurrently (NotFound) is skipped: the goal state for that row is reached. Row-absent resolution errors leave the snapshot; other storage errors propagate (ExecutionError::is_row_absent).
BatchOperation::validate accepted an event stream whose first id was 2
ahead of the last applied id if the first event was a SplitNode, or
unconditionally after 8 failed attempts ("TODO: for split sometimes
this happens"). Change events are positional (InsertAt/RemoveAt carry
node indices), so applying a stream with a missing event replays later
events against node state the missing event was supposed to produce,
silently scrambling on-disk nodes.
Analysis of indexset (put_cdc_checked, remove_cdc, Operation::commit)
shows it never consumes an event id without emitting the event, so a
gap is always transient - the op carrying the missing event just has
not been queued or batched yet - unless events were discarded after id
assignment, which only non-CDC index mutations do (vacuum's
update_index_after_move is the one reachable case on persisted tables,
tracked separately). Instrumented measurement over 50 full-suite runs:
neither escape hatch nor even a plain deferral ever fired.
Fix: strictly defer on any gap. A gap persisting past 8 attempts now
logs tracing::error naming the ids, so a genuine event leak becomes a
loud stall instead of silent corruption.
Measured: 50/50 full-suite runs clean afterwards, no stalls (the
suite's 4s wait_for_ops asserts would catch one).
DiskPersistenceEngine::apply_batch_operation drove its three concurrent
sub-futures (data write, primary index events, secondary index events)
with `while futs.next().await.is_some() {}`, dropping every Result. A
failed disk write or index apply vanished without a trace - not even
the task-level warn fired, because the error never left this function.
All futures are drained to completion before errors are surfaced:
bailing out on the first failure would drop the FuturesUnordered and
cancel the remaining sub-operations at arbitrary await points, and
these futures are not cancellation-safe (a data page could be left
half-written while its index events were abandoned).
Errors are tagged with their component ("batch data write" / "primary
index batch apply" / "secondary index batch apply") and ALL failures
are reported, not just the first in nondeterministic completion order:
a mixed outcome means the durable state may already be inconsistent,
and knowing which parts failed is what an operator needs.
`format!("{}By", &op.name)` borrowed needlessly in both type.rs
generators; clippy 1.97 flags this and the repo policy is a clean
`cargo clippy --all-targets`.
The persistence queue drops `lockfree::queue::Queue` for a
`parking_lot::Mutex<VecDeque<..>>` (the `lockfree` dependency is
removed; no new one is added). This is forced by soundness, not
preference:
lockfree 0.5.1 (unmaintained since 2019) constructs its internal
Removable slots with `ManuallyDrop::new(mem::uninitialized())`. That is
undefined behaviour for any element type whose layout has a niche, and
`Operation` is such a type (an enum full of Vecs, i.e. NonNull
pointers) - the code was always unsound. The next patch adds a
`primary_key_events` Vec to UpdateOperation, which changes the niche
layout for some monomorphisations and makes rustc's uninit validity
check abort deterministically (SIGABRT at Queue::new in
persistence::sync::many_strings). The options were an aborting unsound
dependency or a different queue.
Performance impact: none measurable. End-to-end on a persisted table
with three indexes (release build, 3 runs each, identical series with
only the queue implementation differing):
workload lockfree mutexed deque
20k inserts, single writer 657/671/657 ms 657/678/696 ms
4 writers x 5k inserts 410/394/416 ms 409/402/412 ms
The spread within each variant exceeds the difference between them,
which is expected mechanically:
* The queue carries one Operation per write on a persisted table. A
full insert costs ~33us (rkyv serialisation, index updates with CDC
event generation, a UUIDv7, analyzer bookkeeping, file IO
downstream). An uncontended parking_lot lock/unlock is 10-20ns,
~0.05% of that.
* lockfree's queue is a Michael-Scott linked queue: one heap-allocated
node plus a CAS on the shared tail per push, a CAS plus deferred
reclamation (its "incinerator" GC) per pop. All producers serialise
on the same tail cache line under either design - lock-freedom is a
progress guarantee, not a contention remover - while the contiguous
VecDeque does strictly less work per element: no per-node
allocation, no reclamation machinery, better cache locality.
* The queue has a single consumer (the engine task) and nanosecond
hold times, so the one scenario where lock-freedom genuinely helps
(a lock holder descheduled mid-critical-section) has effectively
zero exposure, and parking_lot parks instead of spinning.
Deliberate minor trade-offs: pop_iter() takes the lock once per
drained element (swap the deque out with mem::take if a profile ever
disagrees); len is incremented before the element is enqueued, a
window that errs only conservatively for wait_for_ops. If write
concurrency ever makes this queue visible in profiles, the upgrade is
a purpose-built MPSC channel, not a return to lockfree. data_bucket's
transitive lockfree dependency is dropped in its own 0.4.0 release
(only a SizeMeasurable impl, unsound path not instantiated).
…ragmentation Problem: EmptyDataVacuum's row-move path (update_index_after_move) mutated the indexes with the non-CDC calls `primary_index.insert(..)` and `secondary_indexes.reinsert_row(..)`. The generated `vacuum()` exists for persisted tables too, and there this had two consequences: 1. The moved links never reached the persistence stream, so the on-disk indexes went stale relative to memory. 2. indexset's non-CDC insert/remove still consume CDC event ids (fetch_add) while discarding the events, creating a permanent gap in the event-id stream. BatchOperation::validate strictly defers on gaps, so the first vacuum on a persisted table stalled persistence forever (tracing::error after 8 attempts). Fix: * UpdateOperation gains a `primary_key_events` field (it becomes UpdateOperation<PrimaryKey, SecondaryKeys>), and the disk engine's Update arm processes those events. A vacuum row move is exactly this operation: row bytes at the new link plus the primary- and secondary-index CDC events, applied atomically as one op. Audited every `Operation::primary_key_events()` caller: all three iterate the returned events, so Update returning Some(&[]) for ordinary updates is behaviourally identical to the previous None. * New `VacuumPersistence<PrimaryKey, SecondaryEvents>` trait; the persistence `Queue` implements it by pushing that Update operation, and `PersistenceTask::vacuum_sink()` hands an Arc<dyn ..> to the vacuum. The trait is #[doc(hidden)] macro-support API: it leaks low-level CDC event types and is not meant for downstream implementations; no semver stability is promised for it. * EmptyDataVacuum holds an optional sink (`with_persistence(..)`). With a sink, update_index_after_move uses reinsert_row_cdc / insert_cdc, captures the events, and emits the operation; without one (in-memory tables) the old non-CDC path is unchanged. Codegen for persisted tables wires `.with_persistence(self.1.vacuum_sink())` into the generated `vacuum()`. NOTE: any UpdateOperation construction site added later must also initialize `primary_key_events`. * New test tests/persistence/vacuum.rs: fills a persisted table across multiple pages, deletes, vacuums (asserting pages freed), inserts afterwards, asserts under a 30s timeout that persistence drains, then reloads from disk and verifies every surviving row by primary key, unique and non-unique index. Without the fix it fails with "persistence stalled after vacuum on persisted table" (re-confirmed by reverting only the wiring line). Known limitation: vacuum on a persisted table keeps the on-disk state consistent but does not reclaim on-disk space; pages freed in memory stay in the data file (tracked in issue #163).
… loads
Problem
-------
Independent of vacuum, an unthrottled bulk insert+delete on a persisted table
panics the background persistence engine task with "page should be available
in table of contents" in process_change_event_batch
(src/persistence/space/index/mod.rs for sized keys, unsized_.rs for String
keys). The engine task dies, so wait_for_ops() then hangs forever. The
failure is timing/batch-boundary dependent: on this machine a plain
1000-insert / 50-delete workload trips it in roughly 4 out of 5 runs, and it
reproduces identically on a tree without the vacuum patch (verified against
a clean checkout plus only the lock-race series). This is why
tests/persistence/vacuum.rs (patch 1/2) throttles its bulk phases with
wait_for_ops every 100 rows.
Change
------
Adds tests/persistence/bulk_load_stall.rs: bulk insert 1000 rows across a
unique i64 index, a non-unique u64 index, and a non-unique String (unsized)
index, delete the 50 lowest pks, then wait_for_ops under a 30s timeout so
the stall fails loudly instead of hanging; finally reload from disk and
verify the surviving rows. The test is #[ignore]d because it fails until the
underlying bug is fixed; run it with:
cargo test --test mod persistence::bulk_load_stall -- --ignored
Once the TOC bug is fixed: un-ignore this test and remove the throttling
workaround in tests/persistence/vacuum.rs.
Suspected area: BatchOperation::validate / QueueAnalyzer deferral
(src/persistence/operation/batch.rs, src/persistence/task.rs) interacting
with the table-of-contents key tracking across node max_value transitions
(SplitNode/CreateNode/RemoveNode) in process_change_event_batch.
Apply on top of patch 1/2 (vacuum CDC row moves).
An unthrottled bulk load on a persisted table panicked the engine task with "page should be available in table of contents" (process_change_event_batch, sized and unsized index spaces), after which wait_for_ops() hung forever. tests/persistence/bulk_load_stall.rs reproduced this on ~4 out of 5 runs. Batch collection groups operations by data page, so a batch routinely carries an interior event-id gap: in the reproduction, inserts on the first data page emit primary-index ids up to 407 while the deletes of that page's rows emit ids 1002..1051, and the ids in between belong to operations on other data pages that are not part of the batch. validate_events is what defers everything after such a gap, but its backward scan stopped at MAX_CHECK_DEPTH = 30 events: the 50 contiguous delete events at the tail hid the gap, the scan concluded the stream was gapless, and the batch was applied with the hole. The skipped ids carried the node max transitions (splits) of the in-memory index, so the on-disk table of contents still had the node at max 407 while the applied RemoveAt referenced max 312 - the expect in the sized index space (and its unsized twin) fired, killing the engine task. Event ids are only valid to apply as a gapless stream (the strict- deferral fix made that invariant explicit), so the scan must cover the whole batch: drop the depth bound. Cost is O(batch events) per validation attempt. Measured (debug profile, worst cases): a gapless 10_000-event batch scans in ~0.46ms and 100_000 events in ~4.2ms; peeling one interior gap with a long tail costs ~0.34ms at 10_000 and ~3.2ms at 100_000. Real batches are page-limited (MAX_PAGE_AMOUNT) to a few hundred events, i.e. microseconds, against disk I/O per batch. Unit tests cover the exact shape: a gap hidden behind a 50-event contiguous tail must defer the tail, and a gapless stream must stay untouched. With this fix the reproduction no longer panics (0/100 solo runs), but it stays #[ignore]d for now: unthrottled bulk loads still trip a separate pre-existing data-page bug in save_batch_data, fixed next.
With the event-gap fix in place the engine survives unthrottled bulk loads, which exposed a second, pre-existing bug: reloading after a bulk insert+delete returned zero-filled rows (id 0, empty strings) for pks whose index entries were intact. save_batch_data derives `ids_to_create` from `batch_data.keys()`, and HashMap iteration is unordered. `last_page_id` was then taken from `ids_to_create.last()` - an arbitrary element, not the max. A batch only creates several data pages at once when the engine is lagging behind a bulk load; when that happened and `.last()` was not the max, a page that had just been created and persisted stayed above `last_page_id`, so the next batch touching it classified it as new and re-created it zero-filled, wiping the rows persisted a batch earlier. Only the links of the wiping batch were applied on top of the fresh zero page, and the on-disk indexes still pointed at the wiped links. Track the max instead, and state the high-water-mark invariant directly (`last_page_id.max(*max)`) so a future refactor of the id-partitioning above cannot regress monotonicity. Un-ignore the bulk-load reproduction test now that both bugs it trips are fixed, and drop the insert throttling in the persisted-vacuum test that existed only to dodge them. Measured: reproduction 0/100 solo failures and the full suite 0/40 stressed runs after this fix; before it, the unthrottled tests read back zeroed rows in roughly 1-2 of 30 suite runs under load (and ~4/5 runs panicked before the event-gap fix).
PersistenceTask::wait_for_ops treats persistence as settled when three
triggers hold at once: the operation queue is empty, the analyzer's
inner table is empty, and analyzer_in_progress is false. The engine
loop's idle path broke the flag's covering guarantee:
task_analyzer_in_progress.store(false, Release);
let op = engine_queue.pop().await;
task_analyzer_in_progress.store(true, Release);
Queue::pop removes the element and runs len.fetch_sub before returning,
so between the fetch_sub and the store(true) all three triggers hold at
once -- queue.len() == 0, analyzer count == 0, analyzer_in_progress ==
false -- while the popped operation has not been fed to the analyzer,
let alone persisted. check_wait_triggers passes and wait_for_ops
returns with the operation still in flight. A caller that then drops
the table and reopens the space races the engine's write of that
operation.
Observed under full-suite parallel load (both rare):
- persistence::failure::insert::test_insert_primary_duplicate failed
1/30: the phase-3 row read data = 1000 instead of 100 -- the reload
saw a stale pk_gen_state and reused a primary key.
- persistence::sync::test_space_insert_many_sync failed with pk_gen
state 4917 vs 5000 while all 5000 rows were present -- the state
write of the tail of the queue was still in flight at reload.
Fix: make the busy-flag handoff atomic with the dequeue.
Queue::pop_marking_in_progress sets the flag back to true under the
queue mutex, before decrementing len. The store(true) is ordered before
len.fetch_sub(Release), and check_wait_triggers acquire-loads len
before it loads the flag, so a waiter that observes the post-pop length
of 0 is guaranteed to also observe in_progress == true. The empty+idle
state is no longer observable while an operation is in flight. The idle
path now also stores false before notify_waiters, so a woken waiter can
observe the idle state immediately (latency only, not correctness).
Also abort the engine task on PersistenceTask drop - but only when the
engine is provably idle. The AbortHandle was stored but never used, so
the detached engine task outlived the table: after a drop-and-reopen
within one runtime the old engine could still be applying a batch
while a new engine loaded the same files. An idle task is parked at
the queue pop, an await point where cancellation is clean. A BUSY
engine is deliberately not aborted: the batch sub-futures are not
cancellation-safe (a data page could be left half-written while its
index events were abandoned), so aborting mid-batch would trade one
corruption for another. A busy drop logs tracing::error and leaves the
task running; callers must drain with wait_for_ops() first. The proper
close()/Failed-state lifecycle is tracked in issue #168.
The queue's length counter also moves from AtomicU16 to AtomicUsize:
the queue is unbounded, and a 16-bit counter wraps at 65_536 queued
operations, at which point the wait triggers would see an "empty"
queue that still holds work.
Verified on top of the 0001-0013 series: cargo test --test mod
stress-ran 40 times (316 tests per run, 0 failures);
test_insert_primary_duplicate and test_space_insert_many_sync passed
40/40 under that load, against 1/30 failures before.
cargo fmt and cargo clippy --all-targets stay clean.
Atomic dependency migration plus the golden fixture that depends on it:
* indexset is consumed as `WorkTablesIndex 0.0.1` via a package alias
(`indexset = { package = "WorkTablesIndex", .. }`), so every
`use indexset::` path is unchanged. WorkTablesIndex is indexset
0.16.0 plus the halve() soundness fix - split_off(len/2) instead of
capacity/2, which panicked ("`at` split index (is N) should be <=
len") whenever a node's capacity exceeded its length after removals;
that panic was the last source of intermittent test failures
(~18/50 full-suite runs).
* data_bucket moves to 0.4.0, which consumes the same WorkTablesIndex
crate (two source crates providing the same Pair/ChangeEvent types
cannot coexist in one dependency tree) and drops its transitive
lockfree 0.5.1 dependency.
* tests/data/expected/space_index/indexset/
process_insert_at_big_amount.wt.idx is regenerated in the same
commit: the indexset_compatibility big-amount test replays live CDC
events and compares the space-index file byte-for-byte, so the
fixture encodes the split geometry and must change together with the
dependency (each side fails deterministically without the other).
The regenerated output was verified byte-identical across two runs
before adoption. File format is unchanged; existing table files load
fine under either version.
With this commit the series is self-contained against crates.io: no
[patch.crates-io], no path dependencies.
PR Summary by QodoConcurrency and persistence fix series: deterministic suite, sound queue, CDC-complete vacuum
AI Description
Diagram
High-Level Assessment
Files changed (34)
|
Code Review by Qodo
Context used✅ Compliance rules (platform):
4 rules 1. Detached engine after drop
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+380
to
+406
| impl<PrimaryKeyGenState, PrimaryKey, SecondaryKeys, AvailableIndexes> Drop | ||
| for PersistenceTask<PrimaryKeyGenState, PrimaryKey, SecondaryKeys, AvailableIndexes> | ||
| { | ||
| /// Aborts the engine task so it cannot outlive the table it persists. | ||
| /// Without this the detached task keeps running on the runtime after the | ||
| /// table is dropped, and a re-opened table can read the same files while | ||
| /// the old engine is still writing them. | ||
| /// | ||
| /// The abort only happens when the engine is provably idle (queue and | ||
| /// analyzer empty, no operation in flight) — that is the normal state | ||
| /// after `wait_for_ops`, and an idle task is parked at the queue pop, an | ||
| /// await point where cancellation is clean. Aborting a *busy* engine | ||
| /// would cancel persistence futures that are not cancellation-safe (a | ||
| /// data page could be left half-written while its index events are | ||
| /// abandoned), so a busy engine is left running and reported instead: | ||
| /// callers must drain with `wait_for_ops` before dropping. A proper | ||
| /// `close()` lifecycle (drain, join, surface terminal errors) is the | ||
| /// long-term replacement for this heuristic. | ||
| fn drop(&mut self) { | ||
| if self.check_wait_triggers() { | ||
| self.engine_task_handle.abort(); | ||
| } else { | ||
| tracing::error!( | ||
| "PersistenceTask dropped with work in flight; the engine task keeps running detached. Call wait_for_ops() before dropping to guarantee a clean shutdown." | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
1. Detached engine after drop 🐞 Bug ☼ Reliability
PersistenceTask::drop leaves the persistence engine task running detached when the task is dropped with work in flight, so it may keep writing the table’s files after the table handle is dropped and can race a subsequent re-open of the same table path. This is a real lifecycle hazard because it is allowed by the API (only logged) and can cause concurrent writers to the same persistence files.
Agent Prompt
### Issue description
`PersistenceTask::drop` only aborts the engine when it is idle; when it’s busy it logs an error and leaves the engine task running detached. This allows background writes to continue after the owning table/task handle is dropped, which can race a new table instance opened on the same files.
### Issue Context
The current implementation explicitly states that aborting busy work is unsafe (non-cancellation-safe futures), but leaving a detached writer is also unsafe for file-level exclusivity and predictable shutdown.
### Fix Focus Areas
- src/persistence/task.rs[380-523]
### Suggested fix direction
- Introduce an explicit async shutdown API (e.g., `close(self) -> Result<()>` or `shutdown(&self) -> impl Future`) that:
- stops accepting new operations,
- drains `queue`/`analyzer`,
- joins the engine task (store a `JoinHandle`, not just `AbortHandle`),
- surfaces terminal persistence errors.
- Make Drop behavior unambiguous and safe:
- either always abort to prevent post-drop writes (and document durability implications), or
- prevent the busy-drop footgun by making the owning table’s Drop call the shutdown path (if feasible in your runtime model), or
- gate table reopen on a per-path lock so two engines cannot write the same files concurrently.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+85
to
+98
| // Two strong references means this map entry and our own `lock` | ||
| // clone above, and nothing else. Any higher count is a task that | ||
| // has already taken this Arc out of `get_or_insert_with` and is | ||
| // about to register on it; removing the entry now would let the | ||
| // next caller build a *second* lock for the same row, and the two | ||
| // would not serialise against each other. | ||
| // | ||
| // Known trade-off: if that other task is cancelled between taking | ||
| // the Arc and registering its operation, nothing re-triggers this | ||
| // cleanup and the (unlocked, unused) entry stays in the map until | ||
| // the next operation on the same key drops its guard. That leaks at | ||
| // most one empty lock per abandoned key and never affects mutual | ||
| // exclusion. | ||
| && Arc::strong_count(&lock) == 2 |
There was a problem hiding this comment.
2. Lockmap cleanup can leak 🐞 Bug ☼ Reliability
LockMap::remove_with_lock_check now removes an unlocked entry only when Arc::strong_count()==2, so if a task is cancelled after obtaining the Arc from get_or_insert_with but before registering/creating a LockGuard, the unlocked entry may never be cleaned up. This can grow the lock map over time for high-cardinality keys with cancellations/timeouts.
Agent Prompt
### Issue description
`remove_with_lock_check` now requires `Arc::strong_count(&lock) == 2` to remove an unlocked entry, which prevents a correctness race but creates a cancellation window where an unlocked entry can remain forever if no later operation touches the same key.
### Issue Context
The code comments acknowledge this trade-off: cancellation between taking the Arc out of `get_or_insert_with` and registering the operation can leave an unused lock entry in the map.
### Fix Focus Areas
- src/lock/map.rs[42-102]
### Suggested fix direction
Implement a bounded or opportunistic cleanup path for abandoned unlocked entries, e.g.:
- Add a periodic sweep/eviction of unlocked entries (size cap / LRU / time-based), safe because entries are unlocked.
- Or store `Weak<RwLock<LockType>>` in the map and clean up dead entries opportunistically.
- Or add an explicit “registration” protocol: `get_or_insert_with` returns a guard/handle that atomically registers the operation under the map write lock, eliminating the cancellation gap.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fifteen-commit series fixing every intermittent failure in the test suite and the correctness bugs behind them. Each commit message carries the full analysis: mechanism, fix rationale, and measured before/after rates. Reviewed through two full expert review rounds plus follow-ups; every finding is either fixed here or deferred with a tracking issue.
What's fixed
get_or_insert_withwith read fast path; same fix in vacuum viaRowLock::newNotFound/pk-conflicts when racing deletes/inserts — typedPrimaryAlreadyExists, documented lock-free retry with scheduler yieldMultiPairrandom discriminators — the real cause of the ~50% "rollback loss" flake), lock-scan-A-process-scan-B hole, stale-link wrong-row delete risk — validated pk snapshots, predicate column in the lock setlockfreequeue replaced (mem::uninitializedUB, deterministic SIGABRT) withMutex<VecDeque>— measured zero throughput impact (benchmark in the commit message)wait_for_opsno longer returns with an operation in flight (1/30 repro under load); queue counter widened from u16; idle-only engine abort on dropWorkTablesIndex 0.0.1(indexset +halve()soundness fix) +data_bucket 0.4.0+ the regenerated golden fixtureVerification
cargo check --testsper commit — bisectable).cargo fmt --checkclean, zero clippy warnings, 428 tests passing (4 ignored: s3 feature, one pre-existing, two churn repros for Lock-free insert races locked mutations: ghost-visible rows, delete panic, stall under same-key churn #169).[patch.crates-io], no path dependencies.Breaking changes (0.9 line)
RowLock::new(new trait method),UpdateOperation<PrimaryKey, _>(new generic + field),WorkTableError::PrimaryAlreadyExists(new variant),lockfreedependency removed, indexset consumed asWorkTablesIndexvia package alias (alluse indexset::paths unchanged). Consumers must moveworktableanddata_buckettogether.Deferred with issues
#163 (vacuum disk-space reclamation), #164 (upsert single-lock linearization), #165 (lock-map retention on cancelled acquirer), #167 (non-CDC mutations burn CDC event ids), #168 (persistence lifecycle: close()/Failed state), #169 (lock-free insert vs locked mutations — found by the new churn tests, which ship
#[ignore]d until it lands).