refactor(jd-client): robust error handling for initialization and retry logic by nulllpc · Pull Request #456 · stratum-mining/sv2-apps

nulllpc · 2026-04-25T12:42:36Z

#168

This PR attempts to improve error handling for JDC by addressing some low-hanging improvements:

Replaces several expect() and unwrap() calls in JobDeclaratorClient::start with proper error propagation using Result.
Refactors the JDC error handling by making the upstream initialization logic "action-aware."

Changes:

Updated start() signature to return JDCError<JobDeclarationClient>.
Introduced InitializationError(String) variant to JDCErrorKind to preserve context from formerly panicking calls.
Updated main.rs and integration tests to handle the new Result type
try_initialize_single is updated to inspect the Action from sub-component errors and return a full JDCError with either a Shutdown or Fallback action.
The retry loop in initialize_jd now checks e.action. It immediately propagates fatal Shutdown errors up to start() while continuing to retry on Fallback errors.
The Action enum now derives PartialEq and Eq to enable this logic.
Function signatures now consistently use the JDCResult type alias.

Shourya742

Overall approach looks good.

nulllpc · 2026-05-01T16:50:50Z

Hi @Shourya742 I updated my PR accordingly, I reviewed the changes made in #467 and I think my changes still align well with changes there and the comment you made here

#467 (comment)

Shourya742

Changes look okay to me. As a rule of thumb, avoid adding side effects in combinators. Once we fix this, I will approve it.

nulllpc · 2026-05-01T19:24:12Z

I addressed all comments, please review again @Shourya742

Shourya742

Few nits

Shourya742 · 2026-05-02T13:03:51Z

@nulllpc once you fix the clippy and fmt nits, I’ll approve. Nice work.

nulllpc · 2026-05-02T15:17:15Z

All done! Let's get this merged, this is my first PR for sv2. Thanks for your reviews @Shourya742

Shourya742 · 2026-05-02T15:18:18Z

@nulllpc can you squash the commits.

nulllpc · 2026-05-02T15:31:00Z

Done! @Shourya742 I double-checked commit message as well

Shourya742

Thanks! ACK

nulllpc · 2026-05-04T14:24:04Z

Hi @Shourya742 can we merge this PR, are we waiting for sth?

GitGab19 · 2026-05-11T08:48:34Z

@nulllpc CI is failing here, I guess you need to rebase it and fix issues.

nulllpc · 2026-05-11T17:08:45Z

Hi @GitGab19, I updated the PR, it's ready for review now

GitGab19 · 2026-05-12T08:33:24Z

                            _ = tokio::time::sleep(Duration::from_secs(1)) => {}
                        }

+                        if e.action == Action::Shutdown {


Can you add a short comment here explaining why we need this if statement?

So basically try_initialize_single can return either a fallback or shutdown signal. The rationale here is to catch the shutdown signal and then fail fast, instead of retrying again. Before, it was passing only JDCErrorKind so we wouldn't be able to differentiate between a shutdown and a fallback signal. This is the same as @Shourya742 explained here: https://github.com/stratum-mining/sv2-apps/pull/456/changes#r3224976497

Ok, can you write a short comment explaining that though?

GitGab19 · 2026-05-12T11:01:35Z

                Err(e) => {
                    tracing::error!("Failed to initialize upstream: {:?}", e);
                    mode.set_solo_mining();
                }


Probably the same check explained in https://github.com/stratum-mining/sv2-apps/pull/456/changes#r3225801560 should be done here?

What do you think @Shourya742 ?

I realized that when we fallback to solo mining here, we only set the mode but we do not set the upstream_state. Is this intentional or a bug? @Shourya742

It is already set in the fallback branch for the channel manager, so we don't need an assignment here

Ok, I see ChannelManager::new() initiates with upstream state as solo mining so technically it still works, but I think it's better to be explicit here. What do you think @Shourya742 @GitGab19

Not really. I’m going to refactor the startup and fallback code soon anyway to make it more streamlined, so it’s not really needed right now.

Cool, thanks for the explanation

panicking Replaces several `expect()` and `unwrap()` calls in `JobDeclaratorClient::start` with proper error propagation using `Result`. - Updated `start()` signature to return `JDCError<JobDeclarationClient>`. - Introduced `InitializationError(String)` variant to `JDCErrorKind` to preserve context from formerly panicking calls. - Updated `main.rs` and integration tests to handle the new `Result` type - Avoid blindly falling back to solo mining - Return proper error when template receiver fail to start - Fix clippy and fmt checks

nulllpc · 2026-05-12T17:32:46Z

I resolved all comments @Shourya742 @GitGab19 Please check again when you have time

nulllpc · 2026-05-12T18:25:08Z

Tests are failing because they still expect setup to fallback to solo mining, I'll adjust the tests accordingly

nulllpc force-pushed the npc/refactor-non-critical-jdc-errors branch from 90ed53b to a541334 Compare April 25, 2026 12:52

nulllpc changed the title ~~refactor(jd-client): propagate initialization errors instead of panicking~~ refactor(jd-client): robust error handling for initialization and retry logic Apr 25, 2026

nulllpc marked this pull request as ready for review April 25, 2026 14:41

Shourya742 reviewed Apr 26, 2026

View reviewed changes

Comment thread miner-apps/jd-client/src/lib/error.rs Outdated

nulllpc force-pushed the npc/refactor-non-critical-jdc-errors branch 4 times, most recently from 3d9b16b to aaece78 Compare May 1, 2026 16:48

Shourya742 requested changes May 1, 2026

View reviewed changes

Comment thread miner-apps/jd-client/src/lib/mod.rs Outdated

Comment thread miner-apps/jd-client/src/lib/mod.rs Outdated

Comment thread miner-apps/jd-client/src/lib/mod.rs Outdated

Comment thread miner-apps/jd-client/src/lib/mod.rs Outdated

Shourya742 reviewed May 2, 2026

View reviewed changes

Comment thread miner-apps/jd-client/src/main.rs Outdated

Shourya742 reviewed May 2, 2026

View reviewed changes

Comment thread integration-tests/lib/mod.rs Outdated

nulllpc force-pushed the npc/refactor-non-critical-jdc-errors branch from 7979589 to f3c949c Compare May 2, 2026 15:29

Shourya742 approved these changes May 2, 2026

View reviewed changes

nulllpc force-pushed the npc/refactor-non-critical-jdc-errors branch from f3c949c to 42587c2 Compare May 4, 2026 14:23

nulllpc force-pushed the npc/refactor-non-critical-jdc-errors branch 3 times, most recently from 105844a to 7af9c03 Compare May 6, 2026 16:40

nulllpc force-pushed the npc/refactor-non-critical-jdc-errors branch 2 times, most recently from aa97b02 to 7779c3f Compare May 11, 2026 17:08

GitGab19 reviewed May 12, 2026

View reviewed changes

Shourya742 reviewed May 12, 2026

View reviewed changes

Comment thread miner-apps/jd-client/src/lib/mod.rs

GitGab19 reviewed May 12, 2026

View reviewed changes

nulllpc force-pushed the npc/refactor-non-critical-jdc-errors branch from 7779c3f to 3287d6c Compare May 12, 2026 17:31

Conversation

nulllpc commented Apr 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Shourya742 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nulllpc commented May 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Shourya742 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nulllpc commented May 1, 2026

Uh oh!

Uh oh!

Shourya742 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Shourya742 commented May 2, 2026

Uh oh!

nulllpc commented May 2, 2026

Uh oh!

Shourya742 commented May 2, 2026

Uh oh!

nulllpc commented May 2, 2026

Uh oh!

Shourya742 left a comment

Choose a reason for hiding this comment

Uh oh!

nulllpc commented May 4, 2026

Uh oh!

GitGab19 commented May 11, 2026

Uh oh!

nulllpc commented May 11, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nulllpc commented May 12, 2026

Uh oh!

nulllpc commented May 12, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

nulllpc commented Apr 25, 2026 •

edited

Loading

nulllpc commented May 1, 2026 •

edited

Loading