fix: internal trace calls should not compete the semaphore - #7421
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughExecution tracing now carries a ChangesSource-aware execution tracing
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Daemon
participant eth_trace_block
participant StateManager
participant ReplaySemaphore
Daemon->>eth_trace_block: warm current tipset with CallSource::Internal
eth_trace_block->>StateManager: execution_trace(tipset, Internal)
StateManager->>StateManager: populate execution trace cache
StateManager-->>eth_trace_block: trace results
eth_trace_block-->>Daemon: cache warmup result
Note over StateManager,ReplaySemaphore: Internal tracing skips replay semaphore acquisition
sequenceDiagram
participant ExternalRPC
participant eth_trace_block
participant StateManager
participant ReplaySemaphore
ExternalRPC->>eth_trace_block: request trace with CallSource::External
eth_trace_block->>StateManager: execution_trace(tipset, External)
StateManager->>ReplaySemaphore: acquire replay permit
ReplaySemaphore-->>StateManager: permit granted
StateManager-->>eth_trace_block: trace results
eth_trace_block-->>ExternalRPC: RPC response
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files
... and 7 files with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
4535dcc to
d09f192
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/state_manager/execution.rs (1)
25-33: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the
CallSourcescheduling contract.Explain that
Internalbypasses replay-permit acquisition whileExternalis throttled;execution_tracecurrently has no public API documentation. As per coding guidelines, document public functions and structs with doc comments.Also applies to: 208-222
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/state_manager/execution.rs` around lines 25 - 33, Document the public replay function and the execution_trace API with Rust doc comments, explicitly describing that CallSource::Internal bypasses replay-permit acquisition while CallSource::External is throttled. Cover the same scheduling contract wherever execution_trace is publicly exposed, including the referenced additional section, without changing behavior.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/daemon/mod.rs`:
- Around line 453-463: Bound cache-prefill execution separately from RPC replay
concurrency: add a dedicated semaphore or queue for the detached prefill tasks
created in the validated-tipset path, and acquire it before invoking the
internal eth_trace_block warmup shown here. Keep CallSource::Internal and the
RPC replay semaphore bypass unchanged, release the permit after completion, and
ensure task creation cannot allow unbounded concurrent replays.
In `@src/state_manager/execution.rs`:
- Around line 213-219: Make in-flight trace-cache admission source-aware so
CallSource-dependent replay-permit behavior is deterministic: update the
trace-cache flow around execution_trace_inner in
src/state_manager/execution.rs:213-219 to avoid coalescing incompatible sources
while preserving sharing of completed results. Apply the same source-scoped or
decoupled in-flight policy to the block-trace cache in
src/rpc/methods/eth.rs:3618-3637, ensuring warmup and external RPC requests
cannot join futures with the other source’s admission contract.
---
Nitpick comments:
In `@src/state_manager/execution.rs`:
- Around line 25-33: Document the public replay function and the execution_trace
API with Rust doc comments, explicitly describing that CallSource::Internal
bypasses replay-permit acquisition while CallSource::External is throttled.
Cover the same scheduling contract wherever execution_trace is publicly exposed,
including the referenced additional section, without changing behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: a2511d45-cb46-4ec5-af08-d03581a483ed
📒 Files selected for processing (5)
src/daemon/mod.rssrc/rpc/methods/eth.rssrc/rpc/methods/eth/types.rssrc/rpc/methods/state.rssrc/state_manager/execution.rs
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
filecoin-project/lotus(manual)
d09f192 to
919d04e
Compare
919d04e to
ef094e3
Compare
ef094e3 to
6485814
Compare
Summary of changes
Bug fix:
eth_trace_blockacquires a semaphore and can block the cache prefilling. Internal calls in cache prefilling should not compete the semaphore to avoid stuckness / deadlockChanges introduced in this pull request:
Reference issue to close (if applicable)
Closes
Other information and links
Change checklist
Outside contributions
Summary by CodeRabbit