fix validate_share panic after on_set_new_prev_hash in custom-work mode - #2156
Conversation
|
@plebhash would appreciate a review from you ! |
eef2244 to
7ad4da2
Compare
|
I fixed the failing semver check ! |
|
@plebhash shall i modify anything on this pr ? |
There was a problem hiding this comment.
@rx18-eng you re-requested my review while not only ignoring the main suggestion, but also resolving it.
all that in less than a minute after I left my comments.
honestly I would prefer that you took the time to read and understand what the suggestions mean, instead of blindly clicking buttons on Github UI
with this current workflow, you're just closing your eyes and doing whatever you can to rush your PR getting merged, despite correctness or sanity of the contribution.
that's not a good way to make FOSS contributions.
|
@plebhash you're right, I rushed. The first suggestion rendered as an empty code block in my view, so I read it as a no-op and clicked through the other two. I didn't go back to your "in other words: remove L561-L562" reply until after I'd already pushed and re-requested review. That's on me, I should have read every thread end to end first. I sincerely apologize for this ! |
|
Going back through it now, the point is clear: with the top-level clear at L561-L562 still in place, the arm-internal clears from suggestions 2 and 3 don't actually defend against the early-return on |
|
it seems you started your branch before #2151 had been merged, so please rebase against also please squash all commits into one |
e4bb180 to
3069d98
Compare
3069d98 to
aec69e6
Compare
Co-authored-by: plebhash <147345153+plebhash@users.noreply.github.com> Signed-off-by: rx18-eng <remopanda78@gmail.com>
aec69e6 to
cc3977e
Compare
Body
Closes #2155.
JobStore::deactivate_jobmethod that movesactive_jobintopast_jobsExtendedChannel::on_set_new_prev_hashfalse arm now callsdeactivate_job()beforemark_past_jobs_as_stale(), so the old active goes through past then stale and late shares for it returnStaleinstead of panickingStandardChannel::on_set_new_prev_hashreordered sojob_id_to_target.clear()only runs afteractivate_future_job(...)returns true, so the error path can't corrupt stateexpect("job target must exist")sites invalidate_share(extended.rs:675,standard.rs:597) replaced withok_or(ShareValidationError::InvalidJobId)?Two new tests
test_set_new_prev_hash_without_future_jobs_marks_active_as_staleinextended.rsreproduces the panic from the issue and assertsErr(Stale)after the fixtest_set_new_prev_hash_without_future_jobs_preserves_stateinstandard.rsconfirms state survives the error and the next share validates without panicBoth tests fail with the original panic on unpatched code; full crate suite passes (74/74) after the fix.