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.
There was a problem hiding this comment.
Resolved in d13cbf664349.
The published revision retains the fixed-batch cleanup queue: each operation-lock lookup examines at most four queued candidates while holding the registry mutex. Live entries are rotated to the back, expired weak entries are removed, and concurrent requests for the same UUID still upgrade the same shared lock. It therefore avoids the full live-table scan identified in this review.
Regression coverage: operation_lock_pruning_is_bounded proves the fixed cleanup budget; the reclamation and 32-thread same-ID tests cover inactive-entry cleanup and serialization. The exact commit passed the Linux default/all-feature build, strict Clippy, workspace tests (52+206 / 52+216), strict rustdoc, and the focused operation_lock filter (4/4).
Remaining limitation: expired entries are reclaimed opportunistically by later lookups rather than immediately when the final holder exits; this is intentional and does not weaken the per-sandbox serialization guarantee.
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". |
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>
b33b23b to
d13cbf6
Compare
|
@codex review This PR reclaims inactive per-sandbox operation locks with bounded cleanup while preserving same-sandbox operation serialization. Please review the complete pull request: every commit, the cumulative diff against the base branch, and the implementation, tests, and documentation as one submitted change. |
|
Codex Review: Didn't find any major issues. What shall we delve into next? 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 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 and startup
preallocated permanent locks for persisted sandboxes. After this change, the
registry retains one shared lock only while a holder or waiter still owns it,
then reclaims expired entries through bounded work on later lookups.
What changed
Commit
d13cbf664349— reclaim inactive operation locks. Replaces permanentstrong registry ownership with weak references, adds fixed-batch cleanup,
removes startup preallocation, and defines the reclamation and same-ID
serialization invariants with focused tests.
These changes belong in one PR because the weak-reference representation,
bounded cleanup rule, startup behavior, and concurrent-sharing tests jointly
correct one in-memory ownership invariant. The PR does not change sandbox
state, runtime ownership, storage behavior, or request semantics.
Still to do
later lock lookups instead of requiring a background task or synchronous
cleanup when the final holder exits.
Related issue
closes #2289
User / Agent impact
No public API behavior changes. A long-running daemon can release inactive
per-sandbox lock objects while preserving serialization between concurrent
operations for the same sandbox.
Risk and compatibility
Low risk. The change is limited to the manager's in-memory operation-lock
registry.
Validation
Exact commit:
d13cbf6643497242d240b086d7dcf1553f9ef1ddParent:
717e63161d02967749c715a1ee6e7fc12c946ab5Tree:
1208e998fd3f59cccfd70824a0e67f3ddc9fb61eThe exact commit was exported with
git archive; the verified archive SHA-256is:
596cbdb39ee08a9c727b76749ee90f701aff08f63016ce739340b4203ce05d21Fresh Linux x86_64 source and separate initially empty default/all-feature
targets passed:
cargo fmt --all -- --checkcargo test --workspace --locked— 52 core + 206 daemon testscargo test --workspace --all-features --locked— 52 core + 216 daemontests
cargo test -p blazed --locked operation_lock -- --nocapture— 4/4git diff --check, andpublic-boundary scan
All listed tests completed with zero failures. Hosted Components, PR Lint, and CLA checks passed. Complete-PR review also passed without findings for
d13cbf6643: review request and review result.Documentation and rollback
No documentation changes are required because the public contract and
persisted data are unchanged. Reverting the single commit restores permanent
strong registry ownership without requiring data conversion.