Add site setting to configure server-side script execution timeout#7862
Add site setting to configure server-side script execution timeout#7862labkey-martyp wants to merge 3 commits into
Conversation
Replace the hardcoded 60-second Rhino sandbox timeout with a site setting on the Admin Console Site Settings page. The limit is wall-clock time measured from script context creation, so it includes time spent in database operations and nested trigger scripts fired from the script; bulk imports that fire large trigger chains can exceed 60 seconds even when the scripts themselves do little work. The default remains 60 seconds and 0 disables the timeout, following the convention of the adjacent readOnlyHttpRequestTimeout setting. The value is resolved once per Rhino context since observeInstructionCount runs every 30k instructions, and it is also settable as a startup property via SiteSettings.scriptExecutionTimeout. Claude-Session: https://claude.ai/code/session_01CvVMhiSzPnaJtdn1w2Mef4
Follow-up to the scriptExecutionTimeout site setting: the SiteSettingsForm field is now a nullable Integer so a POST that omits the parameter leaves the stored setting unchanged instead of binding 0 and silently disabling the script watchdog. Saving is skipped when the value is absent, and the negative-value validation is null-guarded. Also adds RhinoService.TestCase.timeoutTest (BVT), which verifies that a 1-second timeout aborts a runaway script with the expected error message and that a value of 0 disables the watchdog entirely. Claude-Session: https://claude.ai/code/session_01LEjXWPFro12ZV6YcwtFMvi
labkey-jeckels
left a comment
labkey-jeckels
left a comment
There was a problem hiding this comment.
See questions about null handling but no need for additional review unless you want another pass.
| private int _sslPort; | ||
| private int _memoryUsageDumpInterval; | ||
| private int _readOnlyHttpRequestTimeout; | ||
| private Integer _scriptExecutionTimeout; |
There was a problem hiding this comment.
Why Integer vs int for other properties?
There was a problem hiding this comment.
this is set up where 0 means the timeout is disabled, so if the call submitted without the parameter an int would default to 0 and disable it. This makes disabling the timeout a more explicit change in the UI.
| props.setSSLPort(form.getSslPort()); | ||
| props.setMemoryUsageDumpInterval(form.getMemoryUsageDumpInterval()); | ||
| props.setReadOnlyHttpRequestTimeout(form.getReadOnlyHttpRequestTimeout()); | ||
| if (form.getScriptExecutionTimeout() != null) |
There was a problem hiding this comment.
If the admin blanks the field, we don't save the value? Other fields don't work this way. Is there a reason to have a different convention?
There was a problem hiding this comment.
yeah it's the same reason above, that 0 disables the timeout. So blanking it basically just leaves the value as is. You have to specifically set it to 0 if you want no timeout.
There was a problem hiding this comment.
That behavior makes more sense to me for an API that has optional properties than for a form-based UI. Consider rejecting the blank value instead of silently ignoring it.
There was a problem hiding this comment.
Good point — changed in b8282e0. A blank (or omitted) value is now rejected in validateCommand with "Script execution timeout is required; set to 0 to disable the timeout" instead of being silently ignored. The Integer binding stays, since null is what lets validation distinguish a blank from an explicit 0.
2 participants
Rationale
The Rhino sandbox terminates any server-side JavaScript (such as trigger scripts) after a hard-coded 60 seconds of wall-clock time, which includes database and other Java operations invoked by the script. Long-running but legitimate operations, such as bulk imports that fire per-row triggers, can exceed this budget with no recourse. This change makes the timeout a configurable site setting so admins can raise it, or disable it entirely, without a code change.
Related Pull Requests
None.
Changes
scriptExecutionTimeoutsite setting (default 60 seconds, 0 disables) exposed on the Site Settings admin page and as aSiteSettings.scriptExecutionTimeoutstartup property.longto avoid overflow with large values.Integer, so a POST that omits the parameter leaves the stored setting unchanged rather than silently disabling the timeout.RhinoService.TestCase.timeoutTest(BVT) covering both a short timeout aborting a runaway script and 0 disabling the watchdog.