Concurrency-safe repository ownership and fair blocking primitives - #187
Draft
felipeblazing wants to merge 5 commits into
Draft
Concurrency-safe repository ownership and fair blocking primitives#187felipeblazing wants to merge 5 commits into
felipeblazing wants to merge 5 commits into
Conversation
…itory get_repository() was the only method in data_repository_manager that did not take _mutex, and it returned a reference INTO the map. A concurrent add_new_repository or clear_all_repositories therefore raced both the lookup and the returned reference — with concurrent queries creating and clearing repositories this is a routine use-after-free (silent in release builds). - Store repositories as shared_ptr (add_new_repository still accepts unique_ptr; the manager remains the one logical owner). - New locked accessor get_repository_shared() returns a shared_ptr copy under _mutex, so a repository obtained just before a concurrent clear_all_repositories remains valid for the caller's use. - get_repository() is kept temporarily for source compatibility (deprecated; forwards to get_repository_shared). - Regression tests: a deterministic obtained-before-clear lifetime test and a multi-thread get-vs-add/clear hammer. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…e aliases Register H10/H11 (sirius concurrency register), verified by grep across cucascade and the sirius tree before deletion: - data_repository_manager::get_repository() — the unlocked accessor's deprecated forwarding shim that B9 kept for source compatibility. Zero callers remain (sirius and cucascade both use get_repository_shared), so the migration window is over. The docs example that still showed it now shows get_repository_shared. - data_repository_manager::for_each_repository() — zero callers; every consumer uses the get_repositories() snapshot instead. - The <functional> include only for_each_repository needed. H10 judgement: shared_data_repository / shared_data_repository_manager stay, as compatibility aliases with comments saying exactly that. The names are no longer flatly wrong — data_repository stores shared_ptr<data_batch> (one batch sits in several repositories on fan-out) and since the shared_ptr map change the manager hands out lifetime-safe shared_ptr copies — and the aliases are load-bearing across the sirius tree (public signatures in batch_stream, gpu_pipeline_task, repository_wiring, batch_telemetry, the registry). The misleading part was that they read like distinct types with stronger sharing semantics; the new comments close that off and steer new code to the plain names. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…kout All BLOCK-policy waiters used to share one CV: a released stream went to whichever waiter won the wake-up race, and a caller that released and immediately re-acquired always beat a parked waiter (no CV round trip), so checkout could starve under contention (Sirius register F9). Each BLOCK caller now draws a ticket under the pool lock and a released stream is handed to the lowest ticket — strict arrival order, so checkout is starvation-free. GROW callers never wait and no longer take a pooled stream a parked waiter is owed; they mint a fresh stream instead. API shapes unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
memory_space::make_reservation parked on the space's notification channel with race-wins semantics: a release set one flag and notify_one'd an arbitrary waiter, and a fresh blocking caller's non-blocking fast path could claim the freed memory before any parked waiter retried. A heavy caller's release-and-re-request loop could therefore perpetually beat a light caller's single wait. notification_channel gains a FIFO wait list (scoped_waiter RAII ticket): NOTIFIED is only delivered to the head of the list, releases wake all waiters so the head definitely learns (non-heads re-sleep), a departing waiter passes the baton to the next in line, and shutdown() wakes every waiter instead of one. make_reservation keeps its fast path when nobody is parked, and otherwise joins the FIFO instead of barging. make_reservation_or_null / make_reservation_upto keep their try-semantics untouched. API shapes unchanged. Also removed: memory_reservation_manager's cross-space _wait_mutex / _wait_cv slow path. It was waited on but never notified anywhere, so any request with no candidate space (or with every candidate shutting down) hung forever; request_reservation returns nullptr for that case instead, mirroring make_reservation's shutdown contract. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…eak accounting
Step 6 groundwork for concurrent query teardown in Sirius:
- data_repository_manager::get_repositories() now returns
std::vector<std::shared_ptr<repository_type>> instead of raw pointers.
A memory-pressure sweep holds the snapshot across blocking work (thread
pool reserves, host/device copies); raw pointers dangled the moment a
concurrent teardown cleared the map. Shared elements make the snapshot
self-owning, so teardown no longer needs an external fence.
- data_repository gains a destructor-side leak callback
(set_leak_callback): under shared ownership a repository can outlive
its manager — a borrower may hold it past the owning query's erase —
so the destructor is the one place that reliably observes batches that
died un-consumed.
- data_repository_manager::set_leak_handler() attributes those reports
to the {operator_id, port_id} the repository was registered under,
applied to current and future repositories. clear_all_repositories()
keeps its snapshot-based report for callers that still clear eagerly;
shared-ownership teardown paths simply drop the manager instead.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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.
Five changes from Sirius's concurrent-query hardening effort (multiple queries sharing one engine instance). Each commit is independently reviewable and was validated downstream against Sirius's full 2562-case suite plus a dedicated adversarial concurrency grid.
Commits
data_repository_manager::get_repository→ locked +shared_ptr— the accessor was the only method in the class not taking_mutex, and returned a reference into the map; a concurrentadd_new_repository/clear_all_repositoriesraced it. The map now storesshared_ptr<data_repository>;get_repository_shared()returns a lifetime-safe copy under the lock.for_each_repository(); documents theshared_data_repositorycompatibility aliases.exclusive_stream_poolFIFO checkout — BLOCK callers draw tickets; the lowest unserved ticket takes a stream, so a release-and-reacquire caller queues at the tail instead of winning every wake race. GROW callers mint fresh streams instead of stealing a parked waiter's.memory_space::make_reservation's wait onnotification_channelhad race-wins semantics plus barging by fresh non-blocking fast-path callers, andshutdown()woke only one of N waiters. Ticketed RAII waiters with baton-passing fix all three;_or_null/_uptokeep try-semantics. Also removesmemory_reservation_manager::_wait_cv, which was waited on but never notified anywhere (a latent permanent hang on the no-candidate-space path).get_repositories()returnsshared_ptrsnapshots so borrowers (downgrade sweeps, pipeline wiring) co-own repositories across blocking work; erasing a repository from the manager no longer requires waiting out in-flight borrowers. Un-consumed batches are accounted per{operator, port}by a destructor-side leak callback.Validation
Downstream (Sirius
integration/concurrency-full): full suite 2562 cases / 32.9M assertions green; adversarial concurrency grid (spill storms under 1.5–2 GB pools, query-teardown races against in-flight downgrades, reservation-fairness and stream-pool FIFO unit tests, erase-while-sweep-holds lifetime tests) all green 5x. New cucascade-side unit coverage is included in the downstream test files and can be ported here on request.Draft while the downstream Sirius integration PR is reviewed in parallel.
🤖 Generated with Claude Code