refactor: simplify architecture with focused modules and test harness#236
Conversation
Add a single defaults module as the source of truth for tuning constants, replacing scattered definitions and inverted config→events imports. Event processor configs now convert directly from ServerConfig via from_server_config. Co-authored-by: Cursor <cursoragent@cursor.com>
Break the 4k-line events module into focused submodules while keeping the public API unchanged. Event processor tests move with the implementation. Co-authored-by: Cursor <cursoragent@cursor.com>
Introduce a library crate with app::run as the composition root so main.rs only handles CLI parsing, telemetry init, and logging setup. Co-authored-by: Cursor <cursoragent@cursor.com>
Introduce test_support with default_server_config and server_config_with to replace repeated 15-field literals in app and processor tests. Co-authored-by: Cursor <cursoragent@cursor.com>
Pass Metrics through all components instead of Option, gating collection with an enabled flag so call sites no longer branch on optional handles. Co-authored-by: Cursor <cursoragent@cursor.com>
Extract delivery health and in-flight task tracking from the monolithic dispatcher so the queue/worker logic stays easier to navigate and bisect. Co-authored-by: Cursor <cursoragent@cursor.com>
Collapse new, with_cache_size, and with_full_config into EventProcessorBuilder so production code only exposes with_replay_config. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 14 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (12)
WalkthroughThis PR introduces a new src/app.rs orchestration module (run, staged teardown, key/CLI utilities), centralizes default constants into src/defaults.rs, converts Metrics across relay/push/dispatcher/health components from optional to always-present, adds admission/dedup submodules for event replay protection, and adds dispatcher health/in-flight tracking. ChangesServer refactor: defaults, metrics, admission/dedup, orchestration
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
Ready to review this PR? Stage has broken it down into 7 individual chapters for you: 
Chapters generated by Stage for commit d41dc79 on Jul 5, 2026 8:05pm UTC. |
✅ Code Coverage Report
View detailed coverageDownload the Or run locally: cargo llvm-cov --html
open target/llvm-cov/html/index.html |
Remove stale test imports, add RateLimiter::is_empty for clippy, and replace intra-doc links to private items that break once the crate is a library. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/metrics.rs (1)
770-781: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAdd the
enabledguard to both histogram observers.observe_notification_parse_durationandobserve_push_dispatch_admission_durationbypassself.enabled, soMetrics::disabled()still records these histograms while the other recording methods stay no-op. Add the same early return here.🤖 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/metrics.rs` around lines 770 - 781, The notification parse and push dispatch admission histogram observers currently bypass the Metrics::enabled guard, so Metrics::disabled() still records them while the other metrics methods remain no-op. Update observe_notification_parse_duration and observe_push_dispatch_admission_duration in Metrics to match the existing enabled check pattern used by the other observe_* methods, adding an early return before calling observe_label_value.
🧹 Nitpick comments (1)
src/test_support.rs (1)
33-33: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueInconsistent import style for
DEFAULT_MAX_CONCURRENT_EVENT_PROCESSING.All other defaults are imported via the
use crate::defaults::{...}block at the top; this one is referenced with a fully-qualified path instead, without an obvious reason.♻️ Suggested fix
use crate::defaults::{ DEFAULT_DEDUP_RETENTION_SECS, DEFAULT_GLOBAL_UNWRAP_LIMIT_PER_HOUR, DEFAULT_GLOBAL_UNWRAP_LIMIT_PER_MINUTE, DEFAULT_MAX_DEDUP_CACHE_SIZE, DEFAULT_MAX_NOTIFICATION_AGE_SECS, DEFAULT_MAX_NOTIFICATION_FUTURE_SKEW_SECS, DEFAULT_MAX_SIZE, DEFAULT_MAX_TOKENS_PER_EVENT, DEFAULT_RATE_LIMIT_PER_HOUR, DEFAULT_RATE_LIMIT_PER_MINUTE, + DEFAULT_MAX_CONCURRENT_EVENT_PROCESSING, }; ... - max_concurrent_event_processing: crate::defaults::DEFAULT_MAX_CONCURRENT_EVENT_PROCESSING, + max_concurrent_event_processing: DEFAULT_MAX_CONCURRENT_EVENT_PROCESSING,🤖 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/test_support.rs` at line 33, The `DEFAULT_MAX_CONCURRENT_EVENT_PROCESSING` reference in `test_support` is inconsistent with the other defaults because it uses a fully qualified path instead of the top-level `use crate::defaults::{...}` import block. Update the `test_support` setup to import `DEFAULT_MAX_CONCURRENT_EVENT_PROCESSING` alongside the existing defaults, and then reference that imported symbol directly where the config is built.
🤖 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/app.rs`:
- Line 858: The test module has an unused std::path::PathBuf import that
triggers warnings-as-errors in CI. Remove the PathBuf import from the top of the
test module in src/app.rs, since the tests already rely on the existing Path
import and file.path()/display() usage and do not reference PathBuf anywhere.
In `@src/nostr/events/admission.rs`:
- Line 39: The intra-doc link in the `AdmissionOutcome` documentation is
unresolved because `EventProcessor` is not in scope, causing the docs build to
fail. Update the doc comment on `AdmissionOutcome` to either fully qualify
`EventProcessor::process_inner` with the correct path or remove the link if it
is not needed, keeping the reference valid for rustdoc.
In `@src/push/dispatcher/inflight.rs`:
- Line 11: The intra-doc link in the comment on inflight.rs is unresolved,
causing the docs build to fail under warnings-as-errors. Update the
documentation on the Semaphore reference to use a fully-qualified path or
replace the link with a plain code span so rustdoc no longer looks for an
in-scope item named Semaphore.
In `@src/test_support.rs`:
- Around line 1-47: Add tests in src/test_support.rs for default_server_config
and server_config_with. Verify that default_server_config() returns a
ServerConfig that satisfies ServerConfig::validate and that its zero-checked
limits stay non-zero, using the existing fields populated in
default_server_config. Also add a test that server_config_with correctly applies
mutations to a ServerConfig by overriding one or two fields and asserting the
changes are present.
---
Outside diff comments:
In `@src/metrics.rs`:
- Around line 770-781: The notification parse and push dispatch admission
histogram observers currently bypass the Metrics::enabled guard, so
Metrics::disabled() still records them while the other metrics methods remain
no-op. Update observe_notification_parse_duration and
observe_push_dispatch_admission_duration in Metrics to match the existing
enabled check pattern used by the other observe_* methods, adding an early
return before calling observe_label_value.
---
Nitpick comments:
In `@src/test_support.rs`:
- Line 33: The `DEFAULT_MAX_CONCURRENT_EVENT_PROCESSING` reference in
`test_support` is inconsistent with the other defaults because it uses a fully
qualified path instead of the top-level `use crate::defaults::{...}` import
block. Update the `test_support` setup to import
`DEFAULT_MAX_CONCURRENT_EVENT_PROCESSING` alongside the existing defaults, and
then reference that imported symbol directly where the config is built.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a5ef446f-036b-4a9e-92f8-5997d2cd3d66
📒 Files selected for processing (21)
src/app.rssrc/config.rssrc/crypto/nip59.rssrc/defaults.rssrc/lib.rssrc/main.rssrc/metrics.rssrc/nostr/client.rssrc/nostr/events/admission.rssrc/nostr/events/dedup.rssrc/nostr/events/mod.rssrc/nostr/events/processor.rssrc/push/apns.rssrc/push/dispatcher/health.rssrc/push/dispatcher/inflight.rssrc/push/dispatcher/mod.rssrc/push/fcm.rssrc/push/retry.rssrc/rate_limiter.rssrc/server/health.rssrc/test_support.rs
Add focused tests for defaults, admission refunds, RateLimiter::is_empty, and clearnet startup validation so CI coverage does not drop below master. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
erskingardner
left a comment
erskingardner
left a comment
There was a problem hiding this comment.
Adversarial pass found a couple of metrics-refactor issues. I verified the PR head locally with just ci, cargo doc --no-deps, and cargo test --features tor.
| /// Return a metrics handle that never records (for tests and fallback init). | ||
| #[must_use] | ||
| pub fn disabled() -> Self { | ||
| Self::new().expect("metrics registry").with_enabled(false) |
There was a problem hiding this comment.
Metrics::disabled() is now used as the fallback after Metrics::new() fails during startup, but it immediately calls Metrics::new().expect(...) again. If the original initialization error is deterministic (duplicate collector name, invalid descriptor, etc.), the fallback path panics instead of preserving the old behavior of logging the metrics init failure and continuing with metrics disabled. Either make this return a Result, make the startup fallback not call the fallible constructor again, or remove the fallback branch if metrics init should be fatal.
|
|
||
| /// Observe gift-wrap unwrap duration. | ||
| pub fn observe_gift_wrap_unwrap_duration(&self, outcome: OperationOutcome, duration_secs: f64) { | ||
| if !self.enabled { |
There was a problem hiding this comment.
This guard pattern is missing from observe_notification_parse_duration() immediately below, and the same omission appears in observe_push_dispatch_admission_duration(). Those two methods are called on the normal event path, so metrics.enabled = false still mutates histogram collectors for notification parsing and dispatcher admission while the surrounding metric methods return early. Please add the same if !self.enabled { return; } guard in both observers, ideally with a regression test for Metrics::disabled().
Propagate metrics registry init failures instead of retrying via Metrics::disabled(), gate notification-parse and dispatch-admission observers when disabled, and add tests for those paths and test_support. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
1 participant
Summary
Structural refactor to reduce complexity without changing runtime behavior. Split into seven bisect-friendly commits:
defaults.rsis the single source of tuning constants;TokenRateLimitConfig/ReplayProtectionConfigconvert fromServerConfigdirectly (no more inverted imports fromnostr::events/rate_limiterinmain).nostr/events—dedup,admission, andprocessorsubmodules replace the 3k-line monolith; public API unchanged.lib.rs+app.rs— server wiring and event loop move out ofmain.rs; GlitchTip guard stays inmain.test_support::default_server_config()/server_config_with()fixtures.Metrics—Option<Metrics>removed;enabledflag no-ops recording when metrics are off.push/dispatcher—healthandinflightextracted from the dispatcher module.EventProcessorBuilder— test-only fluent builder replacesnew/with_cache_size/with_full_config; production uses onlywith_replay_config.Test plan
just cipasses on each commit (fmt, clippy-D warnings, tests, audit)cargo run -- --config config/local.toml) — relays connect,/healthand/readyrespondmetrics.enabledin configMade with Cursor
Summary by CodeRabbit
New Features
Bug Fixes