Skip to content

Add reset/cleanup functions to defer in close nonce#803

Merged
guilherme-brandao merged 15 commits intodevfrom
guilherme/engn-3672-required-functions-not-being-called-not-deferd
Apr 16, 2025
Merged

Add reset/cleanup functions to defer in close nonce#803
guilherme-brandao merged 15 commits intodevfrom
guilherme/engn-3672-required-functions-not-being-called-not-deferd

Conversation

@guilherme-brandao
Copy link
Contributor

@guilherme-brandao guilherme-brandao commented Apr 14, 2025

Purpose of Changes and their Description

This PR enhances the robustness of the CloseReputerNonce function by implementing a defer block to ensure critical cleanup operations (fulfilling the nonce, resetting active reputers, resetting submissions) are always executed, even if errors occur during the processing of reputer bundles.

The primary goal is to prevent the system from getting stuck in an inconsistent state if CloseReputerNonce fails mid-execution. By guaranteeing cleanup actions via defer, we ensure the nonce is properly fulfilled and associated state (active reputers, submissions) is reset, improving the reliability of the nonce management and reward distribution process.

Link(s) to Ticket(s) or Issue(s) resolved by this PR

https://linear.app/alloralabs/issue/ENGN-3672/required-functions-not-being-called-not-deferd

Are these changes tested and documented?

  • If tested, please describe how. If not, why tests are not needed.
  • If documented, please describe where. If not, describe why docs are not needed.
  • Added to Unreleased section of CHANGELOG.md?

Still Left Todo

  • PR Review
  • Check if more tests need to be done

@xmariachi
Copy link
Contributor

xmariachi commented Apr 15, 2025

Please do similarly for the CloseWorkerNonce counterparts: ResetActiveWorkersForTopic , ResetWorkersIndividualSubmissionsForTopic, FulfillWorkerNonce

@xmariachi
Copy link
Contributor

Additionally.
I think we should move these into its own function and take them up to the point where we calculate the (topic, block) activity - so when we detect this, we can call defer these operations that we want to happen in any case.

For example, in this function we detect that and ultimately we CloseReputerNonce. However, if there is any error in between, CloseReputerNonce won't be called and thus the cleaning won't happen.

So I believe we should move both function cleanings (worker, reputer) closest to the point where the (topic, block) is detected (and thus we know we need to ultimately clean).

@guilherme-brandao guilherme-brandao marked this pull request as ready for review April 15, 2025 19:11
Copy link
Contributor

@xmariachi xmariachi left a comment

Choose a reason for hiding this comment

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

an additional comment, lgtm otherwise

@xmariachi xmariachi force-pushed the guilherme/engn-3672-required-functions-not-being-called-not-deferd branch from 1c27c5c to faadf6c Compare April 16, 2025 13:18
@guilherme-brandao guilherme-brandao changed the title Add reset/cleanup functions to defer in CloseReputerNonce Add reset/cleanup functions to defer in close nonce Apr 16, 2025
Copy link
Contributor

@xmariachi xmariachi left a comment

Choose a reason for hiding this comment

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

lgtm!

Copy link
Contributor

@amimart amimart left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@zale144 zale144 left a comment

Choose a reason for hiding this comment

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

lgtm

@guilherme-brandao guilherme-brandao merged commit 3906052 into dev Apr 16, 2025
8 checks passed
@guilherme-brandao guilherme-brandao deleted the guilherme/engn-3672-required-functions-not-being-called-not-deferd branch April 16, 2025 17:24
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.

4 participants