From 5b4fee1060e7fba14d12e7a67477925d163389fb Mon Sep 17 00:00:00 2001 From: delchev Date: Thu, 10 Sep 2026 16:33:59 +0300 Subject: [PATCH] templates: type the correlateMessageEvent catch in Abort and Wait, and the no-subscription miss behind it (#7230) The #7145 fix typed the blanket `catch (RuntimeException)` in AbortOnDelete.java.template only; Abort.java.template and Wait.java.template still swallowed every RuntimeException around `Process.correlateMessageEvent`. A correlation that fails because the engine is down, the tenant scope is wrong or the message name was mis-generated was indistinguishable from the expected "the instance is not parked here" - and a wait that is never resumed is exactly the incident an operator has to explain later with nothing in the log. Typing the catch needed the platform to report the expected miss as a type first: BpmProviderFlowable.correlateMessageEvent dereferenced a null Execution when the instance carried no subscription for the message, so the "not parked" outcome arrived as a NullPointerException - not something a listener can honestly single out. It now throws the same IllegalArgumentException the validator raises for an already-ended instance. Both templates then follow the AbortOnDelete recipe: IllegalArgumentException is the quiet, expected miss, logged at debug with its throwable; anything else is logged at warn with its throwable, naming the consequence (a flow still running over a record whose status says it is over; an instance that stays parked forever). Fail-soft, but never silent. The two intent ITs that compile these templates assert the two catch shapes and that the old empty catch is gone. Fixes #7230 Co-Authored-By: Claude Opus 5 --- .../flowable/config/BpmProviderFlowable.java | 13 ++++++++++++ .../events/Abort.java.template | 18 +++++++++++++---- .../events/Wait.java.template | 17 ++++++++++++---- .../tests/api/IntentEmissionCoverageIT.java | 17 ++++++++++++++++ .../integration/tests/api/IntentEngineIT.java | 20 ++++++++++++++++--- 5 files changed, 74 insertions(+), 11 deletions(-) diff --git a/components/engine/engine-bpm-flowable/src/main/java/org/eclipse/dirigible/components/engine/bpm/flowable/config/BpmProviderFlowable.java b/components/engine/engine-bpm-flowable/src/main/java/org/eclipse/dirigible/components/engine/bpm/flowable/config/BpmProviderFlowable.java index 31687e63022..35b68c4ff3f 100644 --- a/components/engine/engine-bpm-flowable/src/main/java/org/eclipse/dirigible/components/engine/bpm/flowable/config/BpmProviderFlowable.java +++ b/components/engine/engine-bpm-flowable/src/main/java/org/eclipse/dirigible/components/engine/bpm/flowable/config/BpmProviderFlowable.java @@ -310,9 +310,17 @@ public void removeVariable(String executionId, String variableName) { /** * Correlates a message event to the process instance. * + * The instance not being subscribed to the message is an ordinary outcome for a caller that reacts + * to an event the instance may or may not be waiting for - it is reported with the same + * {@link IllegalArgumentException} the validator raises for an instance that has already ended, so + * that a caller can tell that expected miss apart from a real fault (a broken engine, a wrong + * tenant) instead of reading it out of a NullPointerException (#7230). + * * @param processInstanceId the process instance id * @param messageName the name of the event * @param variables the variables to be passed with the event + * @throws IllegalArgumentException if the instance is not running, is not visible to the current + * tenant, or is not waiting on a message event with this name */ public void correlateMessageEvent(String processInstanceId, String messageName, Map variables) { flowableArtefactsValidator.validateProcessInstanceId(processInstanceId); @@ -324,6 +332,11 @@ public void correlateMessageEvent(String processInstanceId, String messageName, .processInstanceId(processInstanceId) .executionTenantId(getTenantId()) .singleResult(); + if (execution == null) { + throw new IllegalArgumentException( + "Process instance with id [" + processInstanceId + "] is not waiting on a message event named [" + messageName + + "] - it has no such subscription, or it does not belong to current tenant."); + } runtimeService.messageEventReceived(messageName, execution.getId(), variables); } diff --git a/components/template/template-application-events-java/src/main/resources/META-INF/dirigible/template-application-events-java/events/Abort.java.template b/components/template/template-application-events-java/src/main/resources/META-INF/dirigible/template-application-events-java/events/Abort.java.template index 62223afed31..37ec4524f65 100644 --- a/components/template/template-application-events-java/src/main/resources/META-INF/dirigible/template-application-events-java/events/Abort.java.template +++ b/components/template/template-application-events-java/src/main/resources/META-INF/dirigible/template-application-events-java/events/Abort.java.template @@ -22,7 +22,9 @@ import gen.${javaGenFolderName}.data.${javaPerspective}.${entity}Entity; * ProcessIds stamp the trigger listener writes back for THIS process - not the record's single * ProcessId, which holds whichever process started last and would abort a stranger's instance once a * record is the subject of several flows. Fail-soft: no stamp, no matching parked instance, or an - * instance already past the abort scope is a no-op, never an error. + * instance already past the abort scope is a no-op, never an error - but an abort that fails for any + * other reason leaves a flow running over a record whose status says it is over, so it is logged with + * its cause rather than swallowed (dirigible #7230). * * Self-describing MessageHandler (strong-interface style): the destination/kind come from the * interface, not an annotation; @Component makes it a managed bean. @@ -63,9 +65,17 @@ public class ${process}Abort implements MessageHandler { } try { Process.correlateMessageEvent(instance, "${messageName}", java.util.Map.of()); - } catch (RuntimeException notAborting) { - // The instance is not in the abort scope (already ended or never reached it) - a no-op by - // the fail-soft glue convention, never an error. + } catch (IllegalArgumentException notAborting) { + // The one expected miss: the instance is not in the abort scope - already ended, or never + // reached the subscription - which the platform reports with exactly this type. A no-op by + // the fail-soft glue convention. + LOG.debug("${process} instance [{}] was not in the abort scope of \"${messageName}\" when its ${entity} transitioned", + instance, notAborting); + } catch (RuntimeException failed) { + // Anything else - the abort did not happen, so a flow the record's status says is over is + // still running, with its pending user tasks still in someone's inbox. Fail-soft, but never + // silent (dirigible #7230). + LOG.warn("Could not abort ${process} instance [{}] after its ${entity} transitioned - it is still running", instance, failed); } } diff --git a/components/template/template-application-events-java/src/main/resources/META-INF/dirigible/template-application-events-java/events/Wait.java.template b/components/template/template-application-events-java/src/main/resources/META-INF/dirigible/template-application-events-java/events/Wait.java.template index 4489d66f74f..06383a33f74 100644 --- a/components/template/template-application-events-java/src/main/resources/META-INF/dirigible/template-application-events-java/events/Wait.java.template +++ b/components/template/template-application-events-java/src/main/resources/META-INF/dirigible/template-application-events-java/events/Wait.java.template @@ -28,7 +28,9 @@ import gen.${javaGenFolderName}.data.${javaEventPerspective}.${eventEntity}Repos * holds whichever process started last: once a record can be the subject of several flows, correlating * on that one would resume a stranger's wait, or nothing at all. * Fail-soft by design: no stamp, no matching parked instance, or an instance already past the wait is - * a no-op, never an error - the event is simply not for this wait. + * a no-op, never an error - the event is simply not for this wait. A correlation that fails for any + * other reason leaves an instance that IS parked here parked forever, so it is logged with its cause + * rather than swallowed (dirigible #7230). * * Self-describing MessageHandler (strong-interface style): the destination/kind come from the * interface, not an annotation; @Component makes it a managed bean. @@ -84,9 +86,16 @@ public class ${className}Wait implements MessageHandler { } try { Process.correlateMessageEvent(instance, "${messageName}", java.util.Map.of()); - } catch (RuntimeException notParked) { - // The instance is not waiting on this message (already resumed, on another step, or - // ended) - a no-op by the fail-soft glue convention, never an error. + } catch (IllegalArgumentException notParked) { + // The one expected miss: the instance is not waiting on this message - already resumed, on + // another step, or ended - which the platform reports with exactly this type. A no-op by the + // fail-soft glue convention. + LOG.debug("${process} instance [{}] was not parked on \"${messageName}\" when the ${eventEntity} event arrived", instance, + notParked); + } catch (RuntimeException failed) { + // Anything else - the correlation did not happen, so an instance that IS parked on this wait + // stays parked and nothing will resume it. Fail-soft, but never silent (dirigible #7230). + LOG.warn("Could not resume ${process} instance [{}] on \"${messageName}\" - it stays parked", instance, failed); } } diff --git a/tests/tests-integrations/src/main/java/org/eclipse/dirigible/integration/tests/api/IntentEmissionCoverageIT.java b/tests/tests-integrations/src/main/java/org/eclipse/dirigible/integration/tests/api/IntentEmissionCoverageIT.java index 5e370029912..10452a01d63 100644 --- a/tests/tests-integrations/src/main/java/org/eclipse/dirigible/integration/tests/api/IntentEmissionCoverageIT.java +++ b/tests/tests-integrations/src/main/java/org/eclipse/dirigible/integration/tests/api/IntentEmissionCoverageIT.java @@ -2564,6 +2564,15 @@ private void assertEmission() { "the wait listener must correlate the message on its own process's stamped instance"); assertTrue(waitHandler.contains("new RfqRepository().findById(entity.Rfq)"), "the wait listener must resolve the parked record through the via back-reference"); + // ...and a correlation that does NOT happen is never silent (#7230). Only the not-parked miss - + // the platform's IllegalArgumentException - is the expected no-op; every other failure leaves an + // instance that IS parked on this wait parked forever, and is logged with its throwable. + assertTrue(waitHandler.contains("catch (IllegalArgumentException notParked)") && waitHandler.contains("LOG.debug("), + "the not-parked miss must be caught by its own type, not by a blanket RuntimeException"); + assertTrue(waitHandler.contains("catch (RuntimeException failed)") && waitHandler.contains("LOG.warn(\"Could not resume") + && waitHandler.contains("failed);"), "any other correlation failure must be logged with the throwable"); + assertFalse(waitHandler.contains("catch (RuntimeException notParked)"), + "the empty catch that treated every failure as not-parked must be gone"); String timerLoader = contentOf("gen/events/emission/LoadRfqFlowReviewExpire.java"); assertTrue(timerLoader.contains("execution.setVariable(\"__reviewExpireDate\", due)"), "the expire date loader must publish the variable the boundary timer arms from"); @@ -2655,6 +2664,14 @@ private void assertEmission() { && abortHandler.contains("ProcessStamps.idFor(entity.ProcessIds, \"ApprovalFlow\")") && abortHandler.contains("Process.correlateMessageEvent(instance, \"ApprovalFlowAbort\""), "the abort listener must match the status on -transitioned and abort ITS OWN instance, not whichever flow stamped last"); + // ...and an abort that does NOT happen is never silent either (#7230): the not-in-scope miss is + // typed and quiet, anything else leaves a flow running over a record whose status says it is over. + assertTrue(abortHandler.contains("catch (IllegalArgumentException notAborting)") && abortHandler.contains("LOG.debug("), + "the not-in-abort-scope miss must be caught by its own type, not by a blanket RuntimeException"); + assertTrue(abortHandler.contains("catch (RuntimeException failed)") && abortHandler.contains("LOG.warn(\"Could not abort") + && abortHandler.contains("failed);"), "any other correlation failure must be logged with the throwable"); + assertFalse(abortHandler.contains("catch (RuntimeException notAborting)"), + "the empty catch that treated every failure as not-aborting must be gone"); // ...and the row's DELETE retires the flow too (#7074): a listener on -deleted for every // entity-triggered process, cancelling ITS OWN still-running instance, whether or not abortOn is // declared - an Inbox task over a row that is gone opens an empty form and can still be completed. diff --git a/tests/tests-integrations/src/main/java/org/eclipse/dirigible/integration/tests/api/IntentEngineIT.java b/tests/tests-integrations/src/main/java/org/eclipse/dirigible/integration/tests/api/IntentEngineIT.java index b265aea5d72..80e5a6b27ff 100644 --- a/tests/tests-integrations/src/main/java/org/eclipse/dirigible/integration/tests/api/IntentEngineIT.java +++ b/tests/tests-integrations/src/main/java/org/eclipse/dirigible/integration/tests/api/IntentEngineIT.java @@ -1574,8 +1574,15 @@ void wait_step_and_boundary_timers_emit_catch_event_timers_and_correlating_glue( wait.contains("ProcessStamps.idFor(carrier.ProcessIds, \"CaseHandling\")") && wait.contains("Process.correlateMessageEvent(instance, \"CaseHandlingAwaitReply\""), "the listener should correlate the catch event's message on THIS process's stamped instance (#6862)"); - assertTrue(wait.contains("catch (RuntimeException"), - "correlation must be fail-soft - an instance not parked on the message is a no-op"); + // Fail-soft, but not blind (#7230): only the platform's IllegalArgumentException - not parked, + // or already ended - is the expected no-op; every other failure leaves an instance that IS + // parked here parked forever, and is logged with its throwable. + assertTrue(wait.contains("catch (IllegalArgumentException notParked)") && wait.contains("LOG.debug("), + "the not-parked miss should be caught by its own type, not by a blanket RuntimeException"); + assertTrue(wait.contains("catch (RuntimeException failed)") && wait.contains("LOG.warn(\"Could not resume") + && wait.contains("failed);"), "any other correlation failure should be logged with the throwable"); + assertFalse(wait.contains("catch (RuntimeException notParked)"), + "the empty catch that treated every failure as not-parked should be gone"); String loader = codeOf("gen/events/services/LoadCaseHandlingWorkExpire.java"); assertTrue(loader.contains("class LoadCaseHandlingWorkExpire implements JavaDelegate"), "the expire date loader should be a Flowable JavaDelegate"); @@ -1940,7 +1947,14 @@ void abort_on_emits_an_interrupting_event_subprocess_and_correlating_glue() { abort.contains("ProcessStamps.idFor(entity.ProcessIds, \"OrderApproval\")") && abort.contains("Process.correlateMessageEvent(instance, \"OrderApprovalAbort\""), "the abort listener should abort ITS OWN instance, not whichever flow stamped the record last (#6862)"); - assertTrue(abort.contains("catch (RuntimeException"), "correlation must be fail-soft"); + // Fail-soft, but not blind (#7230): the not-in-scope miss is typed and quiet, anything else is + // a flow still running over a record whose status says it is over - logged with its throwable. + assertTrue(abort.contains("catch (IllegalArgumentException notAborting)") && abort.contains("LOG.debug("), + "the not-in-abort-scope miss should be caught by its own type, not by a blanket RuntimeException"); + assertTrue(abort.contains("catch (RuntimeException failed)") && abort.contains("LOG.warn(\"Could not abort") + && abort.contains("failed);"), "any other correlation failure should be logged with the throwable"); + assertFalse(abort.contains("catch (RuntimeException notAborting)"), + "the empty catch that treated every failure as not-aborting should be gone"); } @Test