-
Notifications
You must be signed in to change notification settings - Fork 239
[blockchain] [ethereum] Avoid 409 Retries on Contract Deploys/Invokes and Mark Operations Pending #1715
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
… and Mark Operations Pending Signed-off-by: hfuss <hayden.fuss@kaleido.io>
Signed-off-by: hfuss <hayden.fuss@kaleido.io>
Signed-off-by: hfuss <hayden.fuss@kaleido.io>
… race conditions Signed-off-by: hfuss <hayden.fuss@kaleido.io>
Signed-off-by: hfuss <hayden.fuss@kaleido.io>
Signed-off-by: hfuss <hayden.fuss@kaleido.io>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1715 +/- ##
==========================================
- Coverage 99.96% 99.96% -0.01%
==========================================
Files 342 342
Lines 25022 25028 +6
==========================================
+ Hits 25014 25018 +4
- Misses 4 5 +1
- Partials 4 5 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! A few clean up comments and needs test to match coverage please
The detailed explanation was super helpful, just want to validate that this comment is in fact still true kaleido-io#136 (comment) as should be documented in FFTM incase the behaviour changes
and this worry:
do worry there's a race here, will the operation update indicating it succeeded, beat the retry that results in a 409 to initially mark it as pending ?
This could be a genuine race condition but there is a workaround of fetching the status of the operation to reevaluate it
| called <- true | ||
| }) | ||
| mom.On("SubmitOperationUpdate", mock.Anything).Return(nil) | ||
| //mom.On("SubmitOperationUpdate", mock.Anything).Return(nil) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this line?
| for { | ||
| suite.T().Logf("Waiting for invoke operation to succeed: %s", res.ID.String()) | ||
| op := suite.testState.client1.GetOperation(suite.T(), res.ID.String()) | ||
| if op.Status == core.OpStatusSucceeded { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems we are stuck here if it fails, until the timeout? Should we just check for failed status and make potential move this to a common test utils, will leave it up to you on this one
Proposed changes
Original downstream PR: kaleido-io#136
From @peterbroadhurst on why this fix is safe in relation to idempotency behavior: kaleido-io#136 (review)
Types of changes
Updates Ethereum blockchain package to:
Updates E2E for Ethereum to account for 409 vs 202 race on idempotent contract invokes
Fixes a flaky test in the IPFS downloader and avoids data races highlighted by
go test -raceBug fix
New feature added
Documentation Update
Please make sure to follow these points
Screenshots (If Applicable)
N/A - see logs below
Other Information
Original PR description and logs:
On contract invokes we observed an invoke submission being sent to an EVMConnect, but the connection was closed or otherwise disrupted resulting in a retry on
status=0:As a result, the EVMConnect did successful process the invoke operation submitted originally unbeknownst to FF, and so when FF then retried it:
Once the 5 retries were done, we finally then wrapped the error and returned the 409 back to the requestor:
And in the background, the txn listener sees the operation succeeded asynchronously:
Note
submissionReject=false. Really the operation ends up succeeding, but aside from the error message containing the operation ID, there's no clean way for the requestor to detect this, get the operation ID, and check the operation status for themselves. While idempotency keys might be helpful, this PR proposes:submissionRejected=false, if so we should not return an error so the operation can be properly marked as pending.a. do worry there's a race here, will the operation update indicating it succeeded, beat the retry that results in a 409 to initially mark it as pending ?