Skip to content
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

Cancel ScriptExecution without calling CancelScript when the StartScript request has never been transferred to Tentacle #756

Merged
merged 3 commits into from
Jan 8, 2024

Conversation

nathanwoctopusdeploy
Copy link
Contributor

Background

Related to: OctopusDeploy/Issues#8535

Given ScriptExecution has started for a Polling Tentacle that has recently gone offline
And the GetCapabilities response is cached
And RPC retries have started for the StartScript Call
When the ScriptExecution is cancelled
Then TentacleClient will try and cancel script execution on Tentacle by calling CancelScript

In this scenario, we can guarantee that StartScript has never been transferred to Tentacle so the script is definitely not running.
Tentacle Client should be able to cancel any queued request and immediately return.

Before

Cancelling ScriptExecution would start trying to cancel the script on Tentacle by calling CancelScript

After

Cancelling ScriptExecution does not try and call CancelScript on Tentacle and instead cancels the queued request and stops ScriptExecution immediately.

How to review this PR

Quality ✔️

Pre-requisites

  • I have read How we use GitHub Issues for help deciding when and where it's appropriate to make an issue.
  • I have considered informing or consulting the right people, according to the ownership map.
  • I have considered appropriate testing for my change.

@nathanwoctopusdeploy nathanwoctopusdeploy force-pushed the sast/PollingCancellationFix branch from e6399d6 to b79a3c1 Compare January 7, 2024 22:52
@nathanwoctopusdeploy
Copy link
Contributor Author

[sc-67724]

Copy link

tentacleTypes.Add(TentacleType.Polling);
}

if (testListening)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Do you think it's worth ensuring we've added at least one of testPolling or testListening?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

100% - Update pushed

Copy link
Contributor

@sburmanoctopus sburmanoctopus left a comment

Choose a reason for hiding this comment

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

LGTM

@nathanwoctopusdeploy nathanwoctopusdeploy merged commit ca792e8 into main Jan 8, 2024
2 checks passed
@nathanwoctopusdeploy nathanwoctopusdeploy deleted the sast/PollingCancellationFix branch January 8, 2024 01:45
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.

2 participants