Skip to content

fix: cancel per-attempt gRPC contexts promptly - #1798

Merged
Dosik13 merged 2 commits into
hiero-ledger:mainfrom
zjuzhongwen:main
Aug 31, 2026
Merged

fix: cancel per-attempt gRPC contexts promptly#1798
Dosik13 merged 2 commits into
hiero-ledger:mainfrom
zjuzhongwen:main

Conversation

@zjuzhongwen

@zjuzhongwen zjuzhongwen commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Description:

_Execute previously called defer cancel() directly inside its retry loop. Since the defer belonged to the outer function scope, cancellation for completed attempts was delayed until _Execute returned.

This could keep timeout-related resources alive longer than necessary when an RPC completed before its configured deadline.

The RPC invocation is now scoped to a small closure, allowing defer cancel() to run at the end of each attempt.

Related issue(s):

Fixes #1797

Notes for reviewer:

Checklist

  • Documented (Code comments, README, etc.)
  • Tested (unit, integration, etc.)

Signed-off-by: zjuzhongwen <zjuzhongwen@outlook.com>
@zjuzhongwen
zjuzhongwen requested review from a team as code owners August 5, 2026 17:10
@zjuzhongwen
zjuzhongwen requested review from Dosik13 and radkomih August 5, 2026 17:10
@lfdt-bot

lfdt-bot commented Aug 5, 2026

Copy link
Copy Markdown

Snyk checks have passed. No issues have been found so far.

Status Scan Engine Critical High Medium Low Total (0)
Open Source Security 0 0 0 0 0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

@codecov

codecov Bot commented Aug 6, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

Files with missing lines Coverage Δ
sdk/executable.go 77.18% <100.00%> (+1.20%) ⬆️

... and 13 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@Dosik13 Dosik13 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thank you for the contribution — the fix is correct, and extracting the attempt into its own function is a clean way to scope the defer cancel(). Summing up my comments:

  1. Add a one-line doc comment on the helper so the per-attempt scope doesn't get inlined away later.
  2. Deduplicate the two branches so the marshal + return appears once.
  3. Please move the test into the existing mock_test.go and drive it through _Execute (BUSYOK) — that checks the real retry behaviour and also covers the transaction path.

One more small thing: please change the issue reference to Fixes #1797 so it auto-closes. Happy to approve after that!

Comment thread sdk/executable.go
Comment thread sdk/executable.go
Comment thread sdk/executable_unit_test.go Outdated
Signed-off-by: zjuzhongwen <zjuzhongwen@outlook.com>
@zjuzhongwen

Copy link
Copy Markdown
Contributor Author

Thank you for the contribution — the fix is correct, and extracting the attempt into its own function is a clean way to scope the defer cancel(). Summing up my comments:

  1. Add a one-line doc comment on the helper so the per-attempt scope doesn't get inlined away later.
  2. Deduplicate the two branches so the marshal + return appears once.
  3. Please move the test into the existing mock_test.go and drive it through _Execute (BUSYOK) — that checks the real retry behaviour and also covers the transaction path.

One more small thing: please change the issue reference to Fixes #1797 so it auto-closes. Happy to approve after that!

@Dosik13 Thanks for the review!

I’ve addressed all the comments:

  • Added a doc comment explaining the per-attempt scope of _ExecuteRequest.
  • Deduplicated the query/transaction branches so marshaling and returning happen once.
  • Moved the regression test into mock_test.go and now drive it through _Execute with a transaction retry (BUSY → OK).

The test verifies that the first attempt’s context is already canceled when the second attempt begins.

Thanks again!

@Dosik13 Dosik13 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thank you very much!

@Dosik13
Dosik13 merged commit e8fb353 into hiero-ledger:main Aug 31, 2026
13 checks passed
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.

Per-attempt gRPC contexts are not canceled promptly in _Execute

3 participants