feat(filters): add ttl in eth filters#7386
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughAdds configurable idle TTL expiry for Ethereum filters. Filter activity is tracked on polling, expired entries are removed from the store and type-specific managers, and filter registration/uninstallation paths share centralized lifecycle handling. ChangesEthereum filter TTL
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant EthEventHandler
participant MemFilterStore
participant TypeSpecificManagers
EthEventHandler->>MemFilterStore: sweep_expired()
MemFilterStore-->>EthEventHandler: expired filters
EthEventHandler->>TypeSpecificManagers: remove_from_manager(filter)
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/rpc/methods/eth/filter/mod.rs`:
- Around line 307-317: Update eth_uninstall_filter and uninstall_filter so
filter removal is atomic: remove the entry directly by its ID via
filter_store.remove(id), rather than first fetching a Filter and then removing
by filter.id(). Preserve the existing graceful Ok(false) behavior when no entry
exists, and continue removing the successfully removed filter from the manager
using the returned filter value.
🪄 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: 6c9011be-78b4-46fb-a760-97adc6357384
📒 Files selected for processing (3)
src/cli_shared/cli/config.rssrc/rpc/methods/eth/filter/mod.rssrc/rpc/methods/eth/filter/store.rs
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
filecoin-project/lotus(manual)
| Ok(()) | ||
| } | ||
|
|
||
| fn uninstall_filter(&self, filter: Arc<dyn Filter>) -> Result<(), Error> { | ||
| self.filter_store | ||
| .as_ref() | ||
| .context("Filter store is missing")? | ||
| .remove(id) | ||
| .remove(filter.id()) | ||
| .context("Failed to remove filter from store")?; | ||
|
|
||
| Ok(()) | ||
| self.remove_from_manager(filter.as_ref()) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
TOCTOU: concurrent uninstall/expiry can turn an idempotent uninstall into an error.
eth_uninstall_filter (lines 320-332, unchanged) does store.get(id) — which succeeds and bumps last_used — then calls uninstall_filter(filter), which does store.remove(filter.id()) again. If the entry is removed between those two calls (e.g. a second concurrent eth_uninstall_filter for the same id, or a future periodic sweep), remove returns None and .context("Failed to remove filter from store")? propagates an Err all the way to the RPC caller, instead of the graceful Ok(false) this function clearly wants to return for "no such filter."
Removing directly via store.remove(id) (fetch-and-remove atomic) avoids both the race and the redundant last_used bump on the entry about to be deleted.
🔒 Proposed fix to make uninstall atomic
- fn uninstall_filter(&self, filter: Arc<dyn Filter>) -> Result<(), Error> {
- self.filter_store
- .as_ref()
- .context("Filter store is missing")?
- .remove(filter.id())
- .context("Failed to remove filter from store")?;
-
- self.remove_from_manager(filter.as_ref())
- }
+ fn uninstall_filter(&self, filter: &dyn Filter) -> Result<(), Error> {
+ self.remove_from_manager(filter)
+ }
pub fn eth_uninstall_filter(&self, id: &FilterID) -> Result<bool, Error> {
let store = self
.filter_store
.as_ref()
.context("Filter store is not supported")?;
- if let Ok(filter) = store.get(id) {
- self.uninstall_filter(filter)?;
+ if let Some(filter) = store.remove(id) {
+ self.uninstall_filter(filter.as_ref())?;
Ok(true)
} else {
Ok(false)
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Ok(()) | |
| } | |
| fn uninstall_filter(&self, filter: Arc<dyn Filter>) -> Result<(), Error> { | |
| self.filter_store | |
| .as_ref() | |
| .context("Filter store is missing")? | |
| .remove(id) | |
| .remove(filter.id()) | |
| .context("Failed to remove filter from store")?; | |
| Ok(()) | |
| self.remove_from_manager(filter.as_ref()) | |
| Ok(()) | |
| } | |
| fn uninstall_filter(&self, filter: &dyn Filter) -> Result<(), Error> { | |
| self.remove_from_manager(filter) | |
| } | |
| pub fn eth_uninstall_filter(&self, id: &FilterID) -> Result<bool, Error> { | |
| let store = self | |
| .filter_store | |
| .as_ref() | |
| .context("Filter store is not supported")?; | |
| if let Some(filter) = store.remove(id) { | |
| self.uninstall_filter(filter.as_ref())?; | |
| Ok(true) | |
| } else { | |
| Ok(false) | |
| } | |
| } |
🤖 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/rpc/methods/eth/filter/mod.rs` around lines 307 - 317, Update
eth_uninstall_filter and uninstall_filter so filter removal is atomic: remove
the entry directly by its ID via filter_store.remove(id), rather than first
fetching a Filter and then removing by filter.id(). Preserve the existing
graceful Ok(false) behavior when no entry exists, and continue removing the
successfully removed filter from the manager using the returned filter value.
Codecov Report❌ Patch coverage is
Additional details and impacted files
... and 4 files with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
a5900e6 to
3073e4b
Compare
1 participant
Summary of changes
Changes introduced in this pull request:
FOREST_FILTER_TTL_SECSwhich make sure the filter expires if remained idle after thettltime limit (3600 seconds)Reference issue to close (if applicable)
Closes #7265
Other information and links
Change checklist
Outside contributions
Summary by CodeRabbit
New Features
Bug Fixes