fix(blaze): reclaim inactive operation locks - #2291
Conversation
|
@codex review Please review exact head |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 59bdb10c4d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| Ok(locks) => locks, | ||
| Err(poisoned) => poisoned.into_inner(), | ||
| }; | ||
| locks.retain(|_, lock| lock.strong_count() > 0); |
There was a problem hiding this comment.
When operations for many different sandbox UUIDs overlap, every acquisition scans all currently live entries while holding the global synchronous registry mutex. A burst reaching N active sandboxes therefore performs Θ(N²) weak-count checks and serializes async request workers behind progressively longer scans, regressing the previous O(1) lookup path; prune dead entries periodically or amortize cleanup instead of retaining the entire map on every request.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Resolved in the current PR #2291 head b33b23be97676307d4d9446b7e48f831a06c5e3e.
The registry now keeps a cleanup queue and examines at most four candidates per lock lookup. Live entries are rotated to the back and expired weak entries are removed, so acquisition no longer scans the complete live map while requests for the same UUID still upgrade the same lock.
The new operation_lock_pruning_is_bounded regression verifies the fixed work budget, alongside the reclamation and 32-thread same-ID sharing tests. The exact head passed the Linux default/all-feature build, strict Clippy, workspace tests (52+110 / 52+120), strict rustdoc, and all 3 focused operation-lock tests.
The per-sandbox lock registry keeps one Arc for every UUID it has seen. A long-running daemon therefore retains lock objects after operations finish. Store weak references and inspect a fixed batch of registry entries during each lookup. Inactive locks are reclaimed without scanning the complete live registry on every acquisition. Concurrent requests for one UUID still upgrade the same lock, preserving per-sandbox serialization. This only changes the in-memory operation-lock lifecycle. Sandbox state, runtime ownership, and request behavior remain unchanged. Fixes: b66e714 ("feat(blaze): manage recoverable sandboxes") Signed-off-by: Weisson Han <wenshu.hx@linux.alibaba.com>
59bdb10 to
b33b23b
Compare
|
@codex review Please review exact head |
|
Codex Review: Didn't find any major issues. Bravo. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Why
The sandbox manager kept a strong operation-lock reference for every UUID it
had ever seen. Even after all holders and waiters finished, the registry
retained the lock object, so a long-running daemon accumulated historical
entries.
Before this change, the table grew with every distinct sandbox ID. After this
change, it stores weak references, retains one shared lock while work is active,
and reclaims expired entries incrementally without scanning the complete live
registry on every acquisition.
What changed
Commit structure
Commit 1,
fix(blaze): reclaim inactive operation locks, updates the registryownership model and adds the tests that define its cleanup and concurrency
invariants.
These changes belong in one PR because the weak-reference representation,
bounded cleanup rule, and concurrent-sharing tests are one in-memory ownership
fix. There is no lifecycle, runtime, storage, or API behavior change to
separate.
Still to do
intentionally reclaimed by later lock lookups rather than at the instant the
last holder is dropped.
Related issue
closes #2289
User / Agent impact
No user-visible API behavior changes. The daemon can release inactive
per-sandbox lock objects while preserving operation serialization and bounded
cleanup work per acquisition.
Risk and compatibility
Low risk. The change is limited to the manager's in-memory lock registry.
Validation
Verified on Linux x86_64 for commit
b33b23be97676307d4d9446b7e48f831a06c5e3e(tree17885bb4bf7803d760a8950ea9fbc2e1f12e297d, archive SHA-256641ab42067257459fa2a9c2ca91624a3828ed785f63720a0f22ce2d41288bae8),from
src/blaze:cargo fmt --all -- --checkcargo test --workspace --locked(blaze-core 52, blazed 110)cargo test --workspace --all-features --locked(blaze-core 52, blazed 120)git diff --checkDocumentation and rollback
No documentation changes are required because public behavior is unchanged.
Reverting the single commit restores the strong-reference table; no persisted
data changes are involved.