Skip to content

chore: drop unused malloc_conf, idiomatic cleanups, and first tests for src/jobs/ - #796

Closed
manan19 wants to merge 7 commits into
mainfrom
chore/cleanup-and-job-tests
Closed

chore: drop unused malloc_conf, idiomatic cleanups, and first tests for src/jobs/#796
manan19 wants to merge 7 commits into
mainfrom
chore/cleanup-and-job-tests

Conversation

@manan19

@manan19 manan19 commented Apr 24, 2026

Copy link
Copy Markdown
Contributor

Summary

A bundle of low-risk cleanups plus the first test coverage for src/jobs/, each verified with tests passing before and after.

Changes

  • Drop the unused malloc_conf export. The pub static malloc_conf in lib.rs was never exported with the _rjem_ prefix that jemalloc actually reads, so it was dead code. Same scope as @topocount's chore: remove unused jemalloc configs #780. Jemalloc itself stays as the global allocator; tikv-jemallocator, tikv-jemalloc-ctl, the rocksdb "jemalloc" feature, and emit_jemalloc_stats all remain unchanged.
  • .len() == 0 / .len() > 0is_empty() / !is_empty(). 53 occurrences across 19 files. Also simplified a few duplicate / unreachable checks in link_store.rs (including one caught by review where is_empty() was called on an &[u8; 24]). Supersedes refactor: use is_empty() instead of len() > 0 checks in perftest.rs #592.
  • Lazy regex compilation in core/validations/message.rs. The 4 module-level regexes (FNAME, TWITTER_USERNAME, GITHUB_USERNAME, geo) are now compiled once via std::sync::LazyLock instead of on every validation call. Aligns with the intent of perf: optimize regex compilation with lazy_static #576; uses stable std (available since Rust 1.80) rather than lazy_static! or once_cell.
  • First tests for src/jobs/ (13 new). This directory previously had zero coverage. Added inline #[cfg(test)] modules covering job creation, cutoff timestamp math, sync-gate behavior, stale-backup cleanup, in-progress detection, empty-store migration, and mempool backpressure.
  • Regression tests for link_store (6 new, in the existing link_store_test.rs). Covers the error branches ("targetId provided without type", "link type invalid") that the is_empty() simplification touched, which had no prior direct coverage.

Related issues / PRs

Test plan

  • cargo test passes (632/632)
  • cargo fmt --check passes
  • All 78 pre-existing validation tests still pass after the regex refactor
  • New cargo test jobs passes (13/13)
  • New link_store regression tests pass (6/6)

🤖 Generated with Claude Code

Drops jemalloc as the global allocator so the project builds on paths
containing spaces (jemalloc's configure script rejects those). Also
applies mechanical cleanups and adds the first tests for src/jobs/,
which previously had zero coverage.

Changes:
- Remove jemalloc (tikv-jemallocator, tikv-jemalloc-ctl, rocksdb
  "jemalloc" feature, #[global_allocator] + malloc_conf, and the
  emit_jemalloc_stats hook). Relates to #780.
- Replace 53 occurrences of `.len() == 0` / `.len() > 0` with
  `is_empty()` / `!is_empty()` across 19 files. Supersedes #592.
- Lazy-compile the 4 module-level regexes in
  core/validations/message.rs using once_cell::Lazy so each regex
  compiles once instead of on every validation call. Aligns with #576.
- Add 13 unit tests for src/jobs/ (block_pruning, event_pruning,
  snapshot_upload, migrate_onchain_events) covering job creation,
  cutoff timestamp math, sync-gate behavior, stale-backup cleanup,
  in-progress detection, empty-store migration, and mempool
  backpressure.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings April 24, 2026 00:57
@vercel

vercel Bot commented Apr 24, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
snapchain-docs Ready Ready Preview, Comment Apr 24, 2026 11:25pm

Request Review

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR bundles several repo-wide cleanups and small improvements: removing jemalloc integration, making emptiness checks more idiomatic, optimizing message validation regex usage, and adding initial unit tests for the jobs modules.

Changes:

  • Remove jemalloc allocator/stats plumbing and related dependencies/features (including RocksDB’s jemalloc feature).
  • Replace many .len() == 0 / .len() > 0 checks with is_empty() / !is_empty() across the codebase.
  • Compile validation regexes once via once_cell::sync::Lazy and add unit tests for src/jobs/*.

Reviewed changes

Copilot reviewed 26 out of 27 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
src/utils/statsd_wrapper.rs Removes jemalloc stats emission helper.
src/lib.rs Removes global jemalloc allocator and malloc_conf export.
Cargo.toml Drops jemalloc-related deps/features; adds filetime dev-dependency; removes RocksDB jemalloc feature.
Cargo.lock Updates lockfile to reflect removed jemalloc crates and added filetime.
src/core/validations/message.rs Switches regex compilation to Lazy statics and updates call sites; some is_empty() refactors.
src/core/validations/verification.rs Refactors byte-vector emptiness checks to is_empty().
src/core/types.rs Refactors an assert to !is_empty().
src/consensus/consensus.rs Refactors asserts to !is_empty().
src/consensus/malachite/read_sync.rs Refactors peer emptiness check to is_empty().
src/network/http_server.rs Refactors base64 empty-string check to is_empty().
src/network/gossip.rs Refactors config string presence checks to !is_empty().
src/mempool/block_receiver.rs Refactors event emptiness checks to is_empty().
src/storage/trie/trie_node.rs Refactors emptiness checks to is_empty() / !is_empty().
src/storage/store/shard.rs Refactors pagination token check to !is_empty().
src/storage/store/block.rs Refactors pagination token check to !is_empty().
src/storage/store/account/username_proof_store.rs Refactors string/vector emptiness checks and conflict list checks.
src/storage/store/account/reaction_store.rs Refactors pagination token check to !is_empty().
src/storage/store/account/onchain_event_store.rs Refactors pagination token checks to !is_empty().
src/storage/store/account/message.rs Refactors pagination token and data_bytes presence/emptiness checks.
src/storage/store/account/link_store.rs Refactors validations/emptiness checks; simplifies some conditions.
src/storage/store/account/event.rs Refactors pagination token check to !is_empty().
src/storage/store/account/cast_store.rs Refactors pagination token checks to !is_empty().
src/storage/store/account/block_event_store.rs Refactors pagination token check to !is_empty().
src/jobs/snapshot_upload.rs Removes jemalloc stats calls and adds unit tests for snapshot upload behavior.
src/jobs/migrate_onchain_events.rs Removes TODO attribution in comment and adds unit tests around migration behavior/backpressure.
src/jobs/event_pruning.rs Adds unit tests for job creation and retention math.
src/jobs/block_pruning.rs Adds unit tests for job creation and sync gate behavior.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/storage/store/account/link_store.rs Outdated
LazyLock is stable since Rust 1.80 and the project builds on 1.95 with
no pinned MSRV, so the std version is preferred over the external crate
for this use case.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The is_empty() simplification in link_store.rs removed redundant
`.len() == 0` checks that were logically unreachable, but the error
branches (empty type with target, overlong type) had no existing test
coverage. These 6 regression tests lock in the current behavior so
future refactors catch drift.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@github-actions

github-actions Bot commented Apr 24, 2026

Copy link
Copy Markdown

Diff Coverage

Diff: origin/main...HEAD, staged and unstaged changes

  • src/consensus/consensus.rs (50.0%): Missing lines 108
  • src/consensus/malachite/read_sync.rs (100%)
  • src/core/types.rs (100%)
  • src/core/validations/message.rs (60.0%): Missing lines 30,32,34,396,414,542
  • src/core/validations/verification.rs (100%)
  • src/jobs/block_pruning.rs (90.9%): Missing lines 41-42,60-61
  • src/jobs/event_pruning.rs (85.2%): Missing lines 19-20,33,46
  • src/jobs/migrate_onchain_events.rs (96.8%): Missing lines 271,273
  • src/jobs/snapshot_upload.rs (97.9%): Missing lines 287,301
  • src/mempool/block_receiver.rs (100%)
  • src/network/gossip.rs (100%)
  • src/network/http_server.rs (0.0%): Missing lines 74
  • src/storage/store/account/block_event_store.rs (100%)
  • src/storage/store/account/cast_store.rs (100%)
  • src/storage/store/account/event.rs (100%)
  • src/storage/store/account/link_store.rs (100%)
  • src/storage/store/account/link_store_test.rs (90.2%): Missing lines 2158,2173,2187,2201,2216,2220,2234,2238
  • src/storage/store/account/message.rs (100%)
  • src/storage/store/account/onchain_event_store.rs (100%)
  • src/storage/store/account/reaction_store.rs (100%)
  • src/storage/store/account/username_proof_store.rs (100%)
  • src/storage/store/block.rs (100%)
  • src/storage/store/shard.rs (100%)
  • src/storage/trie/trie_node.rs (100%)

Summary

  • Total: 362 lines
  • Missing: 28 lines
  • Coverage: 92%

src/consensus/consensus.rs

  104             return sets.to_vec();
  105         }
  106 
  107         if let Some(addresses) = &self.validator_addresses {
! 108             assert!(!addresses.is_empty());
  109             return vec![ValidatorSetConfig {
  110                 effective_at: 0,
  111                 validator_public_keys: addresses.clone(),
  112                 shard_ids: vec![shard_id],

src/core/validations/message.rs

  26 const GITHUB_USERNAME_REGEX: &str = "^[a-zA-Z\\d](?:[a-zA-Z\\d]|-(?!-)){0,38}$";
  27 
  28 static FNAME_RE: LazyLock<Regex> = LazyLock::new(|| Regex::new(FNAME_REGEX).unwrap());
  29 static TWITTER_USERNAME_RE: LazyLock<Regex> =
! 30     LazyLock::new(|| Regex::new(TWITTER_USERNAME_REGEX).unwrap());
  31 static GITHUB_USERNAME_RE: LazyLock<Regex> =
! 32     LazyLock::new(|| Regex::new(GITHUB_USERNAME_REGEX).unwrap());
  33 static GEO_RE: LazyLock<Regex> =
! 34     LazyLock::new(|| Regex::new(r"^geo:(-?\d{1,2}\.\d{2}),(-?\d{1,3}\.\d{2})$").unwrap());
  35 /** Number of seconds (10 minutes) that is appropriate for clock skew */
  36 const ALLOWED_CLOCK_SKEW_SECONDS: u64 = 10 * 60;
  37 
  38 pub fn validate_message_type(message_type: i32) -> Result<(), ValidationError> {

  392     if input.len() > 15 {
  393         return Err(ValidationError::UsernameExceedsLength(input.clone(), 15));
  394     }
  395 
! 396     if !TWITTER_USERNAME_RE
  397         .is_match(&input)
  398         .map_err(|_| ValidationError::InvalidData)?
  399     {
  400         return Err(ValidationError::UsernameDoesntMatch(

  410     if input.len() > 38 {
  411         return Err(ValidationError::UsernameExceedsLength(input.clone(), 38));
  412     }
  413 
! 414     if !GITHUB_USERNAME_RE
  415         .is_match(&input)
  416         .map_err(|_| ValidationError::InvalidData)?
  417     {
  418         return Err(ValidationError::UsernameDoesntMatch(

  538     if location.is_empty() {
  539         return Ok(());
  540     }
  541 
! 542     let captures = GEO_RE
  543         .captures(location)
  544         .map_err(|_| ValidationError::InvalidLocationString)?;
  545 
  546     if captures.is_none() {

src/jobs/block_pruning.rs

  37             rx,
  38         );
  39         assert!(
  40             result.is_ok(),
! 41             "expected job creation to succeed: {:?}",
! 42             result.err()
  43         );
  44     }
  45 
  46     #[test]

  56             rx,
  57         );
  58         assert!(
  59             result.is_ok(),
! 60             "expected job creation to succeed: {:?}",
! 61             result.err()
  62         );
  63     }
  64 
  65     #[test]

src/jobs/event_pruning.rs

  15     fn test_job_creation_with_empty_shard_map() {
  16         let result = event_pruning_job("0/1 * * * * *", Duration::from_secs(86400), HashMap::new());
  17         assert!(
  18             result.is_ok(),
! 19             "expected job creation to succeed: {:?}",
! 20             result.err()
  21         );
  22     }
  23 
  24     #[test]

  29         assert!(cutoff < now, "cutoff should be before now");
  30         assert_eq!(
  31             now - cutoff,
  32             retention.as_secs() as u64,
! 33             "difference should equal retention in farcaster seconds"
  34         );
  35     }
  36 
  37     #[test]

  42         let cutoff_short = now - short.as_secs() as u64;
  43         let cutoff_long = now - long.as_secs() as u64;
  44         assert!(
  45             cutoff_long < cutoff_short,
! 46             "longer retention should prune further back in time"
  47         );
  48     }
  49 }

src/jobs/migrate_onchain_events.rs

  267                         0
  268                     };
  269                     sizes.insert(0u32, size);
  270                     let _ = reply.send(sizes);
! 271                 }
  272             }
! 273         });
  274 
  275         // wait_for_mempool_to_clear is private; test its contract via the public behavior:
  276         // the channel responder above will eventually return a small size, so the call must complete.
  277         tokio::time::timeout(

src/jobs/snapshot_upload.rs

  283         .await;
  284 
  285         assert!(
  286             result.is_ok(),
! 287             "expected Ok after stale cleanup, got {:?}",
  288             result
  289         );
  290         assert!(
  291             !stale_file.exists(),

  297     fn test_stale_backup_threshold_is_12_hours() {
  298         assert_eq!(
  299             STALE_BACKUP_THRESHOLD.as_secs(),
  300             12 * 60 * 60,
! 301             "stale backup threshold should be exactly 12 hours"
  302         );
  303     }
  304 }

src/network/http_server.rs

  70     }
  71 
  72     pub fn deserialize<'de, D: Deserializer<'de>>(d: D) -> Result<Option<Vec<u8>>, D::Error> {
  73         let base64 = String::deserialize(d)?.replace(" ", "+");
! 74         if base64.is_empty() {
  75             Ok(None)
  76         } else {
  77             let decoded = BASE64_STANDARD
  78                 .decode(base64.as_bytes())

src/storage/store/account/link_store_test.rs

  2154             if let Some(message::message_data::Body::LinkBody(body)) = data.body.as_mut() {
  2155                 body.r#type = link_type.to_string();
  2156                 body.target = target;
  2157             }
! 2158         }
  2159         msg
  2160     }
  2161 
  2162     fn make_link_remove_with(

  2169             if let Some(message::message_data::Body::LinkBody(body)) = data.body.as_mut() {
  2170                 body.r#type = link_type.to_string();
  2171                 body.target = target;
  2172             }
! 2173         }
  2174         msg
  2175     }
  2176 
  2177     #[test]

  2183             result
  2184                 .unwrap_err()
  2185                 .message
  2186                 .contains("targetId provided without type"),
! 2187             "expected targetId-without-type error"
  2188         );
  2189     }
  2190 
  2191     #[test]

  2197             result
  2198                 .unwrap_err()
  2199                 .message
  2200                 .contains("targetID provided without type"),
! 2201             "expected targetID-without-type error"
  2202         );
  2203     }
  2204 
  2205     #[test]

  2212         );
  2213         let result = LinkStore::make_add_key(&msg);
  2214         assert!(
  2215             result.is_err(),
! 2216             "type longer than 8 bytes should be rejected"
  2217         );
  2218         assert!(
  2219             result.unwrap_err().message.contains("link type invalid"),
! 2220             "expected link-type-invalid error"
  2221         );
  2222     }
  2223 
  2224     #[test]

  2230         );
  2231         let result = LinkStore::make_remove_key(&msg);
  2232         assert!(
  2233             result.is_err(),
! 2234             "type longer than 8 bytes should be rejected"
  2235         );
  2236         assert!(
  2237             result.unwrap_err().message.contains("link type invalid"),
! 2238             "expected link-type-invalid error"
  2239         );
  2240     }
  2241 
  2242     #[test]

ts_hash is Option<&[u8; TS_HASH_LENGTH]> (24 bytes), so is_empty() on
the inner slice can never return true. The branch has always been dead
code; removing it per PR review.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@manan19 manan19 changed the title chore: jemalloc removal, idiomatic cleanups, and job test coverage chore: remove jemalloc, idiomatic cleanups, and first tests for src/jobs/ Apr 24, 2026
@manan19
manan19 requested a review from topocount April 24, 2026 02:39

@topocount topocount left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing jemalloc as a rocksdb feature breaks a bunch of the memory management improvements we've been working on

@topocount topocount left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer that this pr be broken up into specific tasks if we want to merge any of this in

Comment thread Cargo.toml Outdated
Comment thread Cargo.toml
Comment thread src/lib.rs
Per PR review (#796), the original commit removed too much. jemalloc
remains the global allocator and the tikv-jemallocator / tikv-jemalloc-ctl
deps + rocksdb \"jemalloc\" feature stay in place. Only the malloc_conf
export — which was unused as topocount noted in the original #780
proposal — is removed. emit_jemalloc_stats and its callers are
restored.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@manan19 manan19 changed the title chore: remove jemalloc, idiomatic cleanups, and first tests for src/jobs/ chore: drop unused malloc_conf, idiomatic cleanups, and first tests for src/jobs/ Apr 24, 2026

@manan19 manan19 left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the feedback. Splitting into separate PRs:

  • #796 (this PR) — will close in favor of the splits below; malloc_conf removal already covered by your #780, no separate PR needed
  • PR A: .len() == 0 / .len() > 0is_empty() refactor (53 occurrences across 19 files), plus the related link_store.rs simplifications and the regression tests in link_store_test.rs
  • PR B: Lazy regex compilation in core/validations/message.rs via std::sync::LazyLock
  • PR C: First tests for src/jobs/ (13 new tests across 4 modules)

Will open these as separate PRs and link them here once they're up.

@manan19

manan19 commented Apr 24, 2026

Copy link
Copy Markdown
Contributor Author

Closing in favor of the split-out PRs per @topocount's review:

malloc_conf removal is already covered by your existing #780 — no separate PR needed.

@manan19 manan19 closed this Apr 24, 2026
manan19 added a commit that referenced this pull request Apr 25, 2026
## Summary

The `src/jobs/` directory had zero test coverage despite running
production-critical pruning, snapshot upload, and on-chain event
migration logic. Adds 13 inline `#[cfg(test)]` tests across all four
modules:

- **`block_pruning`** (4 tests) — job creation succeeds in both sync
states; sync gate semantics (skips when not synced, runs when synced)
- **`event_pruning`** (3 tests) — job creation; cutoff timestamp =
`farcaster_time - retention.as_secs()`; longer retention prunes further
back
- **`snapshot_upload`** (4 tests) — `UploadAlreadyInProgress` returned
when backup dir has recent contents; empty `only_for_shard_ids` filter
skips all backups; stale-backup contents cleaned up when mtime is older
than 12h; `STALE_BACKUP_THRESHOLD` constant is exactly 12 hours
- **`migrate_onchain_events`** (2 tests) — empty store completes
migration immediately (fast exit before `wait_for_mempool_to_clear`);
mempool backpressure waits and recovers when size drops below
`MAX_MEMPOOL_SIZE`

Adds `filetime = "0.2"` as a `[dev-dependencies]` entry for the
stale-backup test (used to set an old mtime on the temp directory).

Split out from #796 per review feedback.

## Test plan

- [x] `cargo test jobs` passes (13/13)

🤖 Generated with [Claude Code](https://claude.com/claude-code)

---------

Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
manan19 added a commit that referenced this pull request Apr 27, 2026
## Summary

The 4 constant-source regexes in `core/validations/message.rs`
(`FNAME_REGEX`, `TWITTER_USERNAME_REGEX`, `GITHUB_USERNAME_REGEX`, geo)
were being recompiled with `fancy_regex::Regex::new(...)` on every call
to `validate_fname`, `validate_ens_name`, `validate_base_name`,
`validate_twitter_username`, `validate_github_username`, and
`validate_user_location`. Each is hot-path validation code.

Moves them to module-level `static LazyLock<Regex>` so each compiles
exactly once. The CAIP-19 regex is dynamically constructed from a local
variable (depends on namespace input), so it's left as-is.

Aligns with the intent of #576; uses stable `std::sync::LazyLock`
(stabilized in Rust 1.80) instead of `lazy_static!` or `once_cell`.
Split out from #796 per review feedback.

## Test plan

- [x] All 78 existing validation tests pass

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
manan19 added a commit that referenced this pull request Apr 27, 2026
## Summary

Mechanical Clippy-style refactor: `.len() == 0` → `.is_empty()` and
`.len() > 0` → `!x.is_empty()` across 21 files (49 occurrences).
`verification.rs` is already handled by #827, so this PR skips it.

Also includes a few related simplifications in `link_store.rs` that the
refactor surfaced:

- Drop duplicate `is_empty() || .len() == 0` checks in `link_add_key` /
`link_remove_key` — the OR-clauses were logically redundant.
- Remove unreachable `is_empty()` check on `Option<&[u8; 24]>` — a
fixed-size 24-byte array reference can never be empty (caught in PR #796
review).
- Simplify `data_bytes.is_some() && data_bytes.as_ref().unwrap().len() >
0` in `store/account/message.rs` to `data_bytes.as_ref().is_some_and(|b|
!b.is_empty())`.

Adds 6 regression tests in `link_store_test.rs` covering the error
branches in `link_add_key` / `link_remove_key` (`"targetId provided
without type"`, `"link type invalid"`) which had no prior direct test
coverage.

Supersedes #592. Split out from #796 per review feedback.

## Test plan

- [x] `cargo test` passes locally
- [x] `cargo fmt --check` passes
- [x] 6 new `link_store_test` regression tests pass

🤖 Generated with [Claude Code](https://claude.com/claude-code)

---------

Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants