fix #2027 [Bot Engine] add coroutine-based checked pushNotification implementation#2029
Open
fix #2027 [Bot Engine] add coroutine-based checked pushNotification implementation#2029
Conversation
There was a problem hiding this comment.
Pull request overview
This PR resolves #2027 by introducing a coroutine-based push notification API intended to await completion and propagate failures, and by refactoring portions of the bot engine / Vert.x web stack to support coroutine-based execution end-to-end.
Changes:
- Add
BotDefinition.pushNotification(...)(suspending) built on a new suspendingBotRepository.notifyAsync(...), and makeConnectorController.notify/Connector.notifysuspend. - Move several engine and connector paths to coroutine-friendly patterns (
CoroutineVerticle,CoroutineRouterSupport,coHandler,handleIncomingEvent), updating related tests. - Add
vertx-lang-kotlin-coroutinesdependency to multiple modules and adjust supporting code (eg SSE endpoint, HTTP logging).
Reviewed changes
Copilot reviewed 28 out of 28 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| shared/src/main/kotlin/vertx/WebVerticle.kt | Switch base verticle to CoroutineVerticle and convert lifecycle hooks to suspending variants. |
| shared/pom.xml | Add vertx-lang-kotlin-coroutines dependency to shared. |
| pom.xml | Add vertx-lang-kotlin-coroutines version to dependency management. |
| nlp/entity-evaluator/duckling/service/pom.xml | Add coroutine dependency for Vert.x Kotlin coroutine APIs. |
| nlp/build-model-worker/pom.xml | Add coroutine dependency for Vert.x Kotlin coroutine APIs. |
| nlp/api/service/pom.xml | Add coroutine dependency for Vert.x Kotlin coroutine APIs. |
| nlp/admin/server/pom.xml | Add coroutine dependency for Vert.x Kotlin coroutine APIs. |
| nlp/api/client/src/main/kotlin/TockNlpClient.kt | Route OkHttp logging through project logger via HttpLoggingInterceptor lambda. |
| bot/toolkit-base/src/main/kotlin/BotInstaller.kt | Update bot installation API to accept coroutine router handlers while keeping vararg overloads. |
| bot/engine/src/test/kotlin/engine/TockConnectorControllerTest.kt | Update captured router installer type to coroutine router handler type. |
| bot/engine/src/test/kotlin/engine/BotVerticleTest.kt | Initialize verticle context explicitly for coroutine-based verticles. |
| bot/engine/src/test/kotlin/engine/BotRepositoryTest.kt | Update notify verification to coVerify and init verticle context during deploy. |
| bot/engine/src/main/kotlin/engine/nlp/NlpProxyBotService.kt | Change router handler factory to coroutine router handler type. |
| bot/engine/src/main/kotlin/engine/config/UploadedFilesService.kt | Change router handler factory to coroutine router handler type. |
| bot/engine/src/main/kotlin/engine/TockConnectorController.kt | Introduce handleIncomingEvent suspend flow and coroutine-based handling/service registration APIs. |
| bot/engine/src/main/kotlin/engine/ConnectorController.kt | Make notify suspend; add handleIncomingEvent and coroutine service registration API. |
| bot/engine/src/main/kotlin/engine/BotVerticle.kt | Install services via coroutine router support to enable coHandler usage. |
| bot/engine/src/main/kotlin/engine/BotRepository.kt | Add notifyAsync suspend flow and adapt notification state handling to coroutines. |
| bot/engine/src/main/kotlin/definition/AsyncDefinitionBuilders.kt | Add BotDefinition.pushNotification(...) suspending helper. |
| bot/engine/src/main/kotlin/connector/Connector.kt | Make connector notify suspend. |
| bot/engine/pom.xml | Add coroutine dependency for Vert.x Kotlin coroutine APIs. |
| bot/connector-web/src/main/kotlin/WebConnector.kt | Use coroutine route handlers (coHandler) and call handleIncomingEvent for synchronous handling where needed. |
| bot/connector-web/pom.xml | Add coroutine dependency for Vert.x Kotlin coroutine APIs. |
| bot/connector-web-sse/src/main/kotlin/ai/tock/bot/connector/web/sse/SseEndpoint.kt | Adjust SSE registration/send logic around channel lifecycle. |
| bot/connector-whatsapp-cloud/src/main/kotlin/WhatsAppConnectorCloudConnector.kt | Update connector notify signature to suspend. |
| bot/connector-twitter/src/main/kotlin/TwitterConnector.kt | Update connector notify signature to suspend. |
| bot/connector-open-ai/src/main/kotlin/OpenAIConnector.kt | Update connector notify signature to suspend. |
| bot/connector-messenger/src/main/kotlin/MessengerConnector.kt | Update connector notify signature to suspend. |
Comments suppressed due to low confidence (4)
bot/connector-twitter/src/main/kotlin/TwitterConnector.kt:261
- This connector’s
notifyis nowsuspend, but the implementation still callscontroller.handle(...), which is asynchronous and may return before processing completes. That means checked/push notification callers can resume early and miss failures. Prefer callingcontroller.handleIncomingEvent(...)fromnotifyso the suspend boundary reflects completion/error semantics.
override suspend fun notify(
controller: ConnectorController,
recipientId: PlayerId,
intent: IntentAware,
step: StoryStepDef?,
bot/connector-open-ai/src/main/kotlin/OpenAIConnector.kt:200
notifyis nowsuspend, but it ultimately callscontroller.handle(...)(viahandleEvent), which can return before processing completes. For the new checked/push notification flow,notifyshould await completion and propagate errors. Prefer usingcontroller.handleIncomingEvent(...)so this suspend function has true completion semantics.
override suspend fun notify(
controller: ConnectorController,
recipientId: PlayerId,
intent: IntentAware,
step: StoryStepDef?,
bot/connector-whatsapp-cloud/src/main/kotlin/WhatsAppConnectorCloudConnector.kt:262
- This connector’s
notifyis nowsuspend, but it still delegates tocontroller.handle(...), which is explicitly asynchronous and may return before processing completes. This defeats the checked/push notification goal (await completion + propagate errors). Prefer callingcontroller.handleIncomingEvent(...)fromnotifyso the suspension corresponds to completion.
override suspend fun notify(
controller: ConnectorController,
recipientId: PlayerId,
intent: IntentAware,
step: StoryStepDef?,
bot/connector-messenger/src/main/kotlin/MessengerConnector.kt:625
notifyis nowsuspend, but the implementation still callscontroller.handle(...), which can return before processing completes. This undermines the checked/push notification goal (await completion + propagate failures). Prefer usingcontroller.handleIncomingEvent(...)fromnotifyso the suspend boundary reflects completion/error semantics.
override suspend fun notify(
controller: ConnectorController,
recipientId: PlayerId,
intent: IntentAware,
step: StoryStepDef?,
Comment on lines
+97
to
+101
| Future.fromCompletionStage( | ||
| channelFuture.thenRun { | ||
| response.sendSseMessage(responseSerializer.writeValueAsString(msg)) | ||
| }, | ||
| ) |
Comment on lines
+261
to
+265
| notify(recipientId, intent, step, parameters, notificationType, errorListener) | ||
|
|
||
| if (stateModifier == NotifyBotStateModifier.ACTIVATE_ONLY_FOR_THIS_NOTIFICATION) { | ||
| val userTimelineAfterNotification = | ||
| userTimelineDAO.loadWithoutDialogs(botDefinition.namespace, recipientId) | ||
| userTimelineAfterNotification.userState.botDisabled = currentState | ||
| userTimelineDAO.save(userTimeline, botDefinition) | ||
| } | ||
| if (stateModifier == NotifyBotStateModifier.ACTIVATE_ONLY_FOR_THIS_NOTIFICATION) { | ||
| val userTimelineAfterNotification = | ||
| userTimelineDAO.loadWithoutDialogs(botDefinition.namespace, recipientId) |
Comment on lines
114
to
+118
| override fun handle( | ||
| event: Event, | ||
| data: ConnectorData, | ||
| ) { | ||
| verticle.launch { |
| <dependency> | ||
| <groupId>io.vertx</groupId> | ||
| <artifactId>vertx-lang-kotlin-coroutines</artifactId> | ||
| <scope>provided</scope> |
Comment on lines
+231
to
+235
| suspend fun BotDefinition.pushNotification( | ||
| connectorId: String, | ||
| recipientId: PlayerId, | ||
| intent: IntentAware, | ||
| step: StoryStepDef? = null, |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
resolves #2027 by adding a new
pushNotificationsuspending method, which is equivalent tonotify, but waits for the notification process to end and rethrows any error.This requires cross-cutting changes to the engine to run the whole chain on coroutines. To leverage this new suspending code, this PR adds a dependency on
vertx-lang-kotlin-coroutinesand updatesWebVerticleto extendCoroutineVerticle. This should not constitute a breaking change, especially considering these classes are pretty much for internal use only.Warning
The
Connector#notifymethod is now markedsuspend, which is a breaking change. The risk of third-party connector implementations breaking is however quite low, considering this method is optional to implement, and its contract is left rather vague for custom implementations.