[Improvement-18034][alert] Add configurable timeout for script alert plugin by asadjan4611 · Pull Request #18050 · apache/dolphinscheduler

asadjan4611 · 2026-03-11T09:45:36Z

Purpose of the pull request

The Script Alert Plugin executes external scripts to send alerts. Currently, ProcessUtils.executeScript() calls process.waitFor() with no timeout. If a script hangs (network stall, deadlock, etc.), it permanently blocks a thread in the alert sender thread pool, degrading or blocking all alert delivery.

This PR adds a configurable timeout parameter (default: 60 seconds) to the Script Alert Plugin following the same pattern used by the HTTP Alert Plugin.

Brief change log

Add SCRIPT_TIMEOUT, NAME_SCRIPT_TIMEOUT, DEFAULT_SCRIPT_TIMEOUT constants to ScriptParamsConstants
Add InputNumberParam timeout parameter to ScriptAlertChannelFactory.params()
Replace blocking process.waitFor() with process.waitFor(timeout, TimeUnit.SECONDS) in ProcessUtils
Destroy process forcibly and interrupt stream gobblers on timeout
Parse timeout config in ScriptSender with fallback to default
Handle timeout exit code with clear error message in alert result

Verify this pull request

This change added tests and can be verified as follows:

Added ProcessUtilsTest.testExecuteScriptTimeout to verify process timeout and forced destruction (returns exit code -2)
Added ScriptSenderTest.testDefaultTimeout to verify default timeout works correctly
Added ScriptSenderTest.testCustomTimeout to verify custom timeout value is parsed and applied
Updated ScriptAlertChannelFactoryTest.testGetParams to verify new param count (3 → 4)
Compilation verified locally with mvn clean compile

Copilot

Pull request overview

Adds a configurable execution timeout to the Script Alert Plugin to prevent hung scripts from permanently blocking alert-sender threads, aligning behavior with the HTTP alert plugin’s timeout pattern.

Changes:

Introduce script timeout plugin parameter (default 60s) and expose it via the Script plugin channel factory params.
Update script execution to use waitFor(timeout, TimeUnit.SECONDS) and return a dedicated timeout exit code.
Add/adjust unit tests for the new timeout parameter and timeout behavior.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 7 comments.

Show a summary per file

File	Description
dolphinscheduler-alert/dolphinscheduler-alert-plugins/dolphinscheduler-alert-script/src/main/java/org/apache/dolphinscheduler/plugin/alert/script/ProcessUtils.java	Adds timed waiting for script processes and timeout handling.
dolphinscheduler-alert/dolphinscheduler-alert-plugins/dolphinscheduler-alert-script/src/main/java/org/apache/dolphinscheduler/plugin/alert/script/ScriptSender.java	Parses timeout config and maps timeout exit code to a clearer alert result message.
dolphinscheduler-alert/dolphinscheduler-alert-plugins/dolphinscheduler-alert-script/src/main/java/org/apache/dolphinscheduler/plugin/alert/script/ScriptParamsConstants.java	Adds timeout-related constants and default value.
dolphinscheduler-alert/dolphinscheduler-alert-plugins/dolphinscheduler-alert-script/src/main/java/org/apache/dolphinscheduler/plugin/alert/script/ScriptAlertChannelFactory.java	Exposes timeout as an `InputNumberParam` in the Script plugin UI params.
dolphinscheduler-alert/dolphinscheduler-alert-plugins/dolphinscheduler-alert-script/src/test/java/org/apache/dolphinscheduler/plugin/alert/script/ProcessUtilsTest.java	Updates tests to use the new timeout API and adds a timeout test case.
dolphinscheduler-alert/dolphinscheduler-alert-plugins/dolphinscheduler-alert-script/src/test/java/org/apache/dolphinscheduler/plugin/alert/script/ScriptSenderTest.java	Adds basic tests for default/custom timeout configuration.
dolphinscheduler-alert/dolphinscheduler-alert-plugins/dolphinscheduler-alert-script/src/test/java/org/apache/dolphinscheduler/plugin/alert/script/ScriptAlertChannelFactoryTest.java	Updates expected param count after adding timeout.

Comments suppressed due to low confidence (1)

dolphinscheduler-alert/dolphinscheduler-alert-plugins/dolphinscheduler-alert-script/src/main/java/org/apache/dolphinscheduler/plugin/alert/script/ProcessUtils.java:64

The catch block interrupts the current thread for both InterruptedException and IOException. Interrupting the thread on an IOException can cause unrelated higher-level code to behave as if it was interrupted. Split the catch and only call Thread.currentThread().interrupt() when the exception is actually an InterruptedException.

        } catch (IOException | InterruptedException e) {
            log.error("execute alert script error {}", e.getMessage());
            Thread.currentThread().interrupt();
        }

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

Copilot · 2026-03-11T10:10:31Z

...alert-script/src/main/java/org/apache/dolphinscheduler/plugin/alert/script/ScriptSender.java

+        timeout = StringUtils.isNotBlank(config.get(ScriptParamsConstants.NAME_SCRIPT_TIMEOUT))
+                ? Long.parseLong(config.get(ScriptParamsConstants.NAME_SCRIPT_TIMEOUT))
+                : ScriptParamsConstants.DEFAULT_SCRIPT_TIMEOUT;


Long.parseLong(config.get(NAME_SCRIPT_TIMEOUT)) can throw NumberFormatException if the stored plugin config contains a non-numeric value. To prevent alert sending from failing with an uncaught exception, parse this defensively (e.g., try/catch with fallback to DEFAULT_SCRIPT_TIMEOUT, optionally logging a warning).

Suggested change

timeout = StringUtils.isNotBlank(config.get(ScriptParamsConstants.NAME_SCRIPT_TIMEOUT))

? Long.parseLong(config.get(ScriptParamsConstants.NAME_SCRIPT_TIMEOUT))

: ScriptParamsConstants.DEFAULT_SCRIPT_TIMEOUT;

String timeoutConfig = config.get(ScriptParamsConstants.NAME_SCRIPT_TIMEOUT);

if (StringUtils.isNotBlank(timeoutConfig)) {

long parsedTimeout = ScriptParamsConstants.DEFAULT_SCRIPT_TIMEOUT;

try {

parsedTimeout = Long.parseLong(timeoutConfig);

} catch (NumberFormatException ex) {

log.warn("Invalid script timeout config value: '{}', using default: {}",

timeoutConfig, ScriptParamsConstants.DEFAULT_SCRIPT_TIMEOUT, ex);

}

timeout = parsedTimeout;

} else {

timeout = ScriptParamsConstants.DEFAULT_SCRIPT_TIMEOUT;

}

Copilot · 2026-03-11T10:10:31Z

...src/main/java/org/apache/dolphinscheduler/plugin/alert/script/ScriptAlertChannelFactory.java

+                        .setValue(ScriptParamsConstants.DEFAULT_SCRIPT_TIMEOUT)
+                        .addValidate(Validate.newBuilder()
+                                .setType(DataType.NUMBER.getDataType())
+                                .setRequired(false)


The new timeout parameter allows any number, including negative values, which can lead to unexpected behavior in ProcessUtils (and may cause immediate timeout). Add a min validation (e.g., setMin(0D) or setMin(1D) depending on whether 0 should disable timeouts) to constrain input in the UI/config.

Suggested change

.setRequired(false)

.setRequired(false)

.setMin(0D)

Copilot · 2026-03-11T10:10:32Z

...t-script/src/test/java/org/apache/dolphinscheduler/plugin/alert/script/ProcessUtilsTest.java

    public void testExecuteScript() {
-        int code = ProcessUtils.executeScript(cmd);
+        int code = ProcessUtils.executeScript(60, cmd);
        assert code != -1;


This test uses the Java assert keyword, which is disabled by default in Maven/JUnit runs unless -ea is set, so the assertion may never execute. Use JUnit assertions (e.g., Assertions.assertNotEquals(-1, code) or a more specific expected exit code) so the test reliably validates behavior.

Suggested change

assert code != -1;

Assertions.assertNotEquals(-1, code);

Copilot · 2026-03-11T10:10:32Z

...t-script/src/test/java/org/apache/dolphinscheduler/plugin/alert/script/ScriptSenderTest.java

+    @Test
+    public void testCustomTimeout() {
+        scriptConfig.put(ScriptParamsConstants.NAME_SCRIPT_TIMEOUT, "30");
+        ScriptSender scriptSender = new ScriptSender(scriptConfig);
+        AlertResult alertResult = scriptSender.sendScriptAlert("test title Kris", "test content");
+        Assertions.assertTrue(alertResult.isSuccess());
+    }


scriptConfig is a static mutable map and @BeforeEach does not clear it. After testCustomTimeout sets NAME_SCRIPT_TIMEOUT, later tests may unintentionally reuse that value (test order is not guaranteed), making tests flaky. Clear/recreate the map in @BeforeEach or remove the timeout key at the end of this test.

Copilot · 2026-03-11T10:10:32Z

...t-script/src/test/java/org/apache/dolphinscheduler/plugin/alert/script/ScriptSenderTest.java

+    @Test
+    public void testDefaultTimeout() {
+        ScriptSender scriptSender = new ScriptSender(scriptConfig);
+        AlertResult alertResult = scriptSender.sendScriptAlert("test title Kris", "test content");
+        Assertions.assertTrue(alertResult.isSuccess());
+    }
+
+    @Test
+    public void testCustomTimeout() {
+        scriptConfig.put(ScriptParamsConstants.NAME_SCRIPT_TIMEOUT, "30");
+        ScriptSender scriptSender = new ScriptSender(scriptConfig);
+        AlertResult alertResult = scriptSender.sendScriptAlert("test title Kris", "test content");
+        Assertions.assertTrue(alertResult.isSuccess());
+    }


testDefaultTimeout/testCustomTimeout currently only assert that sending succeeds, but they don’t validate that the configured timeout value is actually parsed/applied. To meaningfully test the new behavior, consider using a script/command that sleeps longer than the configured timeout and assert a timeout failure, or mock/spying ProcessUtils to assert it’s called with the expected timeout.

Copilot · 2026-03-11T10:10:33Z

...alert-script/src/main/java/org/apache/dolphinscheduler/plugin/alert/script/ProcessUtils.java

+            boolean finished = process.waitFor(timeoutSeconds, TimeUnit.SECONDS);
+            if (!finished) {
+                log.error("script execution timed out after {} seconds, destroying process", timeoutSeconds);
+                process.destroyForcibly();
+                inputStreamGobbler.interrupt();
+                errorStreamGobbler.interrupt();
+                return -2;


timeoutSeconds is documented as "if <= 0 waits indefinitely", but the implementation always calls process.waitFor(timeoutSeconds, TimeUnit.SECONDS). For timeoutSeconds <= 0 this will not wait indefinitely and can immediately return -2. Handle timeoutSeconds <= 0 by calling process.waitFor() (no timeout) and only using timed wait when timeoutSeconds > 0.

Suggested change

boolean finished = process.waitFor(timeoutSeconds, TimeUnit.SECONDS);

if (!finished) {

log.error("script execution timed out after {} seconds, destroying process", timeoutSeconds);

process.destroyForcibly();

inputStreamGobbler.interrupt();

errorStreamGobbler.interrupt();

return -2;

if (timeoutSeconds > 0) {

boolean finished = process.waitFor(timeoutSeconds, TimeUnit.SECONDS);

if (!finished) {

log.error("script execution timed out after {} seconds, destroying process", timeoutSeconds);

process.destroyForcibly();

inputStreamGobbler.interrupt();

errorStreamGobbler.interrupt();

return -2;

}

} else {

// Wait indefinitely when timeoutSeconds <= 0, as documented

process.waitFor();

Copilot · 2026-03-11T10:10:33Z

...alert-script/src/main/java/org/apache/dolphinscheduler/plugin/alert/script/ProcessUtils.java

+                inputStreamGobbler.interrupt();
+                errorStreamGobbler.interrupt();
+                return -2;
+            }
+            return process.exitValue();


On timeout the code interrupts the StreamGobbler threads, but StreamGobbler blocks on BufferedReader.readLine() and doesn’t check the interrupt flag, so interrupt() alone may not stop these threads promptly. Consider closing the process streams and/or updating the gobblers to exit on interrupt, and then join() them (or mark them as daemon) to avoid leaking threads after a timeout.

Suggested change

inputStreamGobbler.interrupt();

errorStreamGobbler.interrupt();

return -2;

}

return process.exitValue();

try {

process.getInputStream().close();

} catch (IOException ioe) {

log.warn("Failed to close process input stream after timeout", ioe);

}

try {

process.getErrorStream().close();

} catch (IOException ioe) {

log.warn("Failed to close process error stream after timeout", ioe);

}

inputStreamGobbler.interrupt();

errorStreamGobbler.interrupt();

try {

inputStreamGobbler.join(TimeUnit.SECONDS.toMillis(1));

} catch (InterruptedException ie) {

Thread.currentThread().interrupt();

}

try {

errorStreamGobbler.join(TimeUnit.SECONDS.toMillis(1));

} catch (InterruptedException ie) {

Thread.currentThread().interrupt();

}

return -2;

}

int processExitCode = process.exitValue();

try {

inputStreamGobbler.join(TimeUnit.SECONDS.toMillis(1));

} catch (InterruptedException ie) {

Thread.currentThread().interrupt();

}

try {

errorStreamGobbler.join(TimeUnit.SECONDS.toMillis(1));

} catch (InterruptedException ie) {

Thread.currentThread().interrupt();

}

return processExitCode;

asadjan4611 · 2026-03-11T14:20:59Z

@SbloodyS I have resolved all the copilot issue's please review it again.

…apache#18034)

SbloodyS · 2026-03-16T09:29:43Z

...alert-script/src/main/java/org/apache/dolphinscheduler/plugin/alert/script/ProcessUtils.java

+                    closeProcessStreams(process);
+                    joinGobbler(inputStreamGobbler);
+                    joinGobbler(errorStreamGobbler);
+                    return -2;


It's better to use positive integer here.

SbloodyS · 2026-03-16T09:30:19Z

...src/main/java/org/apache/dolphinscheduler/plugin/alert/script/ScriptAlertChannelFactory.java

                .build();

-        return Arrays.asList(scriptUserParam, scriptPathParam, scriptTypeParams);
+        InputNumberParam scriptTimeoutParam =


It is best to tell the user the meaning of the exit code in the document.

SbloodyS · 2026-03-16T09:31:37Z

...alert-script/src/main/java/org/apache/dolphinscheduler/plugin/alert/script/ScriptSender.java

                ? config.get(ScriptParamsConstants.NAME_SCRIPT_USER_PARAMS)
                : "";
+        String timeoutConfig = config.get(ScriptParamsConstants.NAME_SCRIPT_TIMEOUT);
+        if (StringUtils.isNotBlank(timeoutConfig)) {


Suggested change

if (StringUtils.isNotBlank(timeoutConfig)) {

if (StringUtils.isNotEmpty(timeoutConfig)) {

SbloodyS · 2026-03-16T09:33:42Z

...alert-script/src/main/java/org/apache/dolphinscheduler/plugin/alert/script/ScriptSender.java

+            try {
+                parsedTimeout = Long.parseLong(timeoutConfig);
+            } catch (NumberFormatException ex) {
+                log.warn("Invalid script timeout config value: '{}', using default: {}",


We should return alertResult here.

SbloodyS · 2026-03-16T09:35:15Z

...t-script/src/test/java/org/apache/dolphinscheduler/plugin/alert/script/ProcessUtilsTest.java

    private String[] cmd = {"/bin/sh", "-c", shellFilPath + " -t 1"};

    @Test
+    @DisabledOnOs(OS.WINDOWS)


We don't need to do this since we've decleared in the document that windows is not supported.

SbloodyS · 2026-03-16T09:35:23Z

...t-script/src/test/java/org/apache/dolphinscheduler/plugin/alert/script/ProcessUtilsTest.java

+    }
+
+    @Test
+    @DisabledOnOs(OS.WINDOWS)


sonarqubecloud · 2026-03-16T10:05:37Z

Quality Gate passed

Issues
4 New issues
0 Accepted issues

Measures
0 Security Hotspots
76.1% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

asadjan4611 requested a review from SbloodyS as a code owner March 11, 2026 09:45

github-actions bot added backend test labels Mar 11, 2026

github-actions bot assigned asadjan4611 Mar 11, 2026

SbloodyS requested a review from Copilot March 11, 2026 10:05

Copilot started reviewing on behalf of SbloodyS March 11, 2026 10:06 View session

SbloodyS added this to the 3.4.2 milestone Mar 11, 2026

SbloodyS added the improvement make more easy to user or prompt friendly label Mar 11, 2026

Copilot AI reviewed Mar 11, 2026

View reviewed changes

[Improvement][alert] Add configurable timeout for script alert plugin (…

124bf56

…apache#18034)

asadjan4611 force-pushed the feature-18034-script-alert-timeout branch from e902f7e to 124bf56 Compare March 12, 2026 14:07

Merge branch 'dev' into feature-18034-script-alert-timeout

357592b

SbloodyS reviewed Mar 16, 2026

View reviewed changes

-                inputStreamGobbler.interrupt();
-                errorStreamGobbler.interrupt();
-                return -2;
-            }
-            return process.exitValue();
+                try {
+                    process.getInputStream().close();
+                } catch (IOException ioe) {
+                    log.warn("Failed to close process input stream after timeout", ioe);
+                }
+                try {
+                    process.getErrorStream().close();
+                } catch (IOException ioe) {
+                    log.warn("Failed to close process error stream after timeout", ioe);
+                }
+                inputStreamGobbler.interrupt();
+                errorStreamGobbler.interrupt();
+                try {
+                    inputStreamGobbler.join(TimeUnit.SECONDS.toMillis(1));
+                } catch (InterruptedException ie) {
+                    Thread.currentThread().interrupt();
+                }
+                try {
+                    errorStreamGobbler.join(TimeUnit.SECONDS.toMillis(1));
+                } catch (InterruptedException ie) {
+                    Thread.currentThread().interrupt();
+                }
+                return -2;
+            }
+            int processExitCode = process.exitValue();
+            try {
+                inputStreamGobbler.join(TimeUnit.SECONDS.toMillis(1));
+            } catch (InterruptedException ie) {
+                Thread.currentThread().interrupt();
+            }
+            try {
+                errorStreamGobbler.join(TimeUnit.SECONDS.toMillis(1));
+            } catch (InterruptedException ie) {
+                Thread.currentThread().interrupt();
+            }
+            return processExitCode;

	if (StringUtils.isNotBlank(timeoutConfig)) {
	if (StringUtils.isNotEmpty(timeoutConfig)) {

Conversation

asadjan4611 commented Mar 11, 2026

Purpose of the pull request

Brief change log

Verify this pull request

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

asadjan4611 commented Mar 11, 2026

Uh oh!

SbloodyS Mar 16, 2026

Choose a reason for hiding this comment

Uh oh!

SbloodyS Mar 16, 2026

Choose a reason for hiding this comment

Uh oh!

SbloodyS Mar 16, 2026

Choose a reason for hiding this comment

Uh oh!

SbloodyS Mar 16, 2026

Choose a reason for hiding this comment

Uh oh!

SbloodyS Mar 16, 2026

Choose a reason for hiding this comment

Uh oh!

SbloodyS Mar 16, 2026

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud bot commented Mar 16, 2026

Quality Gate passed

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants