Skip to content

fix(tool): surface MessagePool service task errors in api_cmd test ctx#6966

Open
0xDevNinja wants to merge 1 commit intoChainSafe:mainfrom
0xDevNinja:0xdevninja/issue-6505-joinset-errors
Open

fix(tool): surface MessagePool service task errors in api_cmd test ctx#6966
0xDevNinja wants to merge 1 commit intoChainSafe:mainfrom
0xDevNinja:0xdevninja/issue-6505-joinset-errors

Conversation

@0xDevNinja
Copy link
Copy Markdown

@0xDevNinja 0xDevNinja commented Apr 24, 2026

Summary of changes

Changes introduced in this pull request:

  • Own the JoinSet in test_snapshot::ctx and generate_test_snapshot::ctx instead of passing &mut JoinSet::new() into MessagePool::new. The inline temporary was dropped on return, aborting every spawned mpool service task and silently swallowing any errors they emitted pre-cancellation.
  • Move the owned JoinSet into a detached tokio::spawn(drain_mpool_services(...)) that drains it with the canonical while let Some(result) = services.join_next().await loop from daemon/mod.rs::propagate_error, logging errors and non-cancel panics via tracing::warn!.
  • Add a shared drain_mpool_services helper in test_snapshot.rs (visibility pub(super)) and call it from both ctx() sites.

Detaching (vs. awaiting inline) is deliberate: the mpool services are long-lived, so an inline while let Some(...) would never return — that is precisely what caused codecov CI to time out and led to the earlier attempt being reverted from #6493. A detached drainer surfaces errors without blocking the caller.

Reference issue to close (if applicable)

Closes #6505

Other information and links

  • Canonical drain pattern: src/daemon/mod.rs:842-851 (propagate_error).
  • MessagePool::new signature confirmed at src/message_pool/msgpool/msg_pool.rs:~721 — takes services: &mut JoinSet<anyhow::Result<()>>, which matches the generic used here.
  • Prior related PR (different file, same pattern): test: unit test for start_rpc #6493.
  • No CHANGELOG.md entry — this is an internal test utility change and is not user-facing per the changelog guide at the top of that file.
  • No new test: the fix is observability-only (previously-silent warnings now surface via tracing); asserting on log output would be flaky. Existing rpc_regression_tests_print_uncovered passes locally; the heavier regression harness defers to CI.

Change checklist

  • I have performed a self-review of my own code,
  • I have made corresponding changes to the documentation. All new code adheres to the team's documentation standards,
  • I have added tests that prove my fix is effective or that my feature works (if possible),
  • I have made sure the CHANGELOG is up-to-date. All user-facing changes should be reflected in this document.

Outside contributions

  • I have read and agree to the CONTRIBUTING document.
  • I have read and agree to the AI Policy document. I understand that failure to comply with the guidelines will lead to rejection of the pull request.

AI Usage Disclosure

This PR was prepared with assistance from Claude Code (Anthropic). Extent:

  • Issue analysis, implementation, self-review, and PR drafting were AI-assisted.
  • All changes were compiled and tested locally by me before pushing — cargo fmt --all -- --check, cargo clippy --all-targets --no-deps -- -D warnings (with FOREST_F3_SIDECAR_FFI_BUILD_OPT_OUT=1), cargo check --lib --tests, and cargo test --lib rpc_regression_tests_print_uncovered (1/1 pass) all green.
  • I have reviewed every diff line and take full responsibility for correctness.

Summary by CodeRabbit

  • Tests
    • Improved message pool service task tracking and error logging during snapshot validation to prevent silent task failures and ensure reliable test execution.

Both `test_snapshot::ctx` and `generate_test_snapshot::ctx` passed
`&mut JoinSet::new()` to `MessagePool::new`. The temporary was dropped
on return, aborting every service task spawned by the pool and silently
swallowing any error they emitted pre-cancellation.

Own the `JoinSet` locally and move it into a detached `tokio::spawn`
that drains it with the canonical `while let Some(result) =
services.join_next().await` loop from `daemon/mod.rs::propagate_error`,
logging errors and non-cancel panics via `tracing::warn!`. Detaching
(vs. awaiting inline) avoids the codecov-timeout regression that caused
the earlier attempt to be reverted in ChainSafe#6493: the mpool services are
long-lived and would never complete the inline await.

Refs ChainSafe#6505
@0xDevNinja 0xDevNinja requested a review from a team as a code owner April 24, 2026 06:33
@0xDevNinja 0xDevNinja requested review from LesnyRumcajs and hanabi1224 and removed request for a team April 24, 2026 06:33
@CLAassistant
Copy link
Copy Markdown

CLAassistant commented Apr 24, 2026

CLA assistant check
All committers have signed the CLA.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 24, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: be91ec8e-fed5-41d0-b0ff-9c6b692dfdb1

📥 Commits

Reviewing files that changed from the base of the PR and between 65c39a0 and e005443.

📒 Files selected for processing (2)
  • src/tool/subcommands/api_cmd/generate_test_snapshot.rs
  • src/tool/subcommands/api_cmd/test_snapshot.rs

Walkthrough

These changes ensure that background JoinSet task results in test utility functions are properly handled instead of being silently dropped. A new drain_mpool_services function encapsulates the draining logic, logging warnings for task execution errors and panics while maintaining cancellation tolerance.

Changes

Cohort / File(s) Summary
JoinSet Error Handling in Test Utilities
src/tool/subcommands/api_cmd/test_snapshot.rs, src/tool/subcommands/api_cmd/generate_test_snapshot.rs
Added proper error handling for JoinSet task results instead of silent drops. Introduced new pub(super) async fn drain_mpool_services() that iterates through JoinSet results, logging warnings for task failures and panics while ignoring cancellations. Both files now create a shared JoinSet passed to MessagePool::new and spawn a background task to drain it.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The PR title clearly identifies the main change: fixing MessagePool service task error surfacing in api_cmd test utilities by properly handling JoinSet results.
Linked Issues check ✅ Passed The PR fully addresses issue #6505 requirements: adopts the canonical while let Some(res) pattern in both test files, creates drain_mpool_services helper for centralized error handling, and maintains detached spawning to avoid codecov timeouts.
Out of Scope Changes check ✅ Passed All changes are directly scoped to issue #6505: modifications limited to the two specified test files to fix JoinSet error handling, with no unrelated alterations.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
✨ Simplify code
  • Create PR with simplified code

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

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.

Handle JoinSet task errors in test utility functions

2 participants