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

Limit alarm handler walltime to 15minutes #1622

Merged
merged 1 commit into from
Feb 7, 2024

Conversation

jqmmes
Copy link
Contributor

@jqmmes jqmmes commented Feb 5, 2024

This PR adds a wall time timeout for alarm handlers.

When the timeout for an alarm it hit, we cancel deferred alarm deletion and abort the currently running handler.

The alarm timeouts are counted against limits, so they get deleted once the maximum retry limit is reached.

@jqmmes jqmmes requested review from a team as code owners February 5, 2024 19:27
@jqmmes jqmmes requested review from dom96 and kentonv February 5, 2024 19:27
@jqmmes jqmmes force-pushed the joaquim/alarm-handler-walltime-15m-limit branch from 83fa87a to 2d5ce68 Compare February 5, 2024 20:29
Copy link
Collaborator

@dom96 dom96 left a comment

Choose a reason for hiding this comment

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

Looks like this is tested upstream, nice.

How risky is this change? Would it make sense to put it behind an autogate to roll it out independent of a release?

Otherwise LGTM

src/workerd/api/global-scope.c++ Outdated Show resolved Hide resolved
src/workerd/io/worker-entrypoint.c++ Outdated Show resolved Hide resolved
@jqmmes jqmmes force-pushed the joaquim/alarm-handler-walltime-15m-limit branch 2 times, most recently from 1932198 to 8d3d366 Compare February 6, 2024 12:21
@jqmmes
Copy link
Contributor Author

jqmmes commented Feb 6, 2024

Updated PR with new expected behaviour where alarms will be deleted after 1 run + 6 retries.

  • Updated description
  • Added additional logging
  • Alarm retries will now count against limits.
  • Rebased

src/workerd/api/global-scope.c++ Outdated Show resolved Hide resolved
@jqmmes jqmmes force-pushed the joaquim/alarm-handler-walltime-15m-limit branch from 8d3d366 to 71ea6db Compare February 6, 2024 20:06
@jqmmes
Copy link
Contributor Author

jqmmes commented Feb 6, 2024

Updated PR with timeout as kj::Duration instead of uint32_t.

src/workerd/api/global-scope.c++ Outdated Show resolved Hide resolved
src/workerd/io/limit-enforcer.h Outdated Show resolved Hide resolved
src/workerd/io/worker-entrypoint.c++ Outdated Show resolved Hide resolved
src/workerd/io/worker-entrypoint.c++ Outdated Show resolved Hide resolved
@jqmmes jqmmes force-pushed the joaquim/alarm-handler-walltime-15m-limit branch 3 times, most recently from 2a7afc0 to dd52149 Compare February 6, 2024 22:44
@jqmmes
Copy link
Contributor Author

jqmmes commented Feb 6, 2024

Updated PR:

@jqmmes jqmmes force-pushed the joaquim/alarm-handler-walltime-15m-limit branch from dd52149 to 4e03280 Compare February 6, 2024 23:02
@jqmmes jqmmes force-pushed the joaquim/alarm-handler-walltime-15m-limit branch from 4e03280 to ddbde5c Compare February 7, 2024 12:34
@jqmmes jqmmes force-pushed the joaquim/alarm-handler-walltime-15m-limit branch 2 times, most recently from 6d13fe7 to 2f914a8 Compare February 7, 2024 17:55
@jqmmes jqmmes force-pushed the joaquim/alarm-handler-walltime-15m-limit branch from 2f914a8 to 3f6584b Compare February 7, 2024 20:21
@jqmmes jqmmes merged commit 83851df into main Feb 7, 2024
11 checks passed
@jqmmes jqmmes deleted the joaquim/alarm-handler-walltime-15m-limit branch February 26, 2024 12:54
@jqmmes jqmmes restored the joaquim/alarm-handler-walltime-15m-limit branch February 26, 2024 12:55
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.

5 participants