fix(insight): schedule exports off the hook threads - #703
Open
wangyb-A wants to merge 2 commits into
Open
Conversation
Exporter I/O previously ran inline inside onInvocationStart, onOperationChange, and onInvocationEnd, so a slow exporter delayed the SDK's checkpoint callbacks in ON_CHANGE mode. Records are now handed to a background scheduler that exports at most one record at a time. Each record is a complete snapshot, so updates arriving while an export is in flight coalesce into a single pending slot and only the latest is exported next. Exporters for one record run concurrently and independently. onInvocationEnd drains the scheduler and then flushes every exporter once, so the final record is always delivered before the invocation returns; if no worker can be started, the drain exports the pending record itself at that boundary.
Contributor
Author
|
/ai review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
This comment has been minimized.
This comment has been minimized.
- Reject operation-change hooks once invocation end has begun. A checkpoint for an unawaited asynchronous operation can complete while the end record is draining; its RUNNING snapshot must not replace or follow the final record. The closed flag and the final schedule are set under one lock. - Complete the pump handle when worker submission fails, so a drain() that already observed the handle wakes up and exports the pending record inline instead of blocking forever. - Flush exporters concurrently and independently, matching how exports fan out, so one slow or failing flush never delays the others.
Contributor
Author
|
/ai review |
Contributor
Codex AI reviewFound one P2 correctness issue in the asynchronous record handoff. Review was static only, as required. Reviewed commit |
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
1 participant
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.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
Issue Link, if available
No linked issue. Supersedes the alternative approach in #699, which stays open only for comparison.
Description
Workflow Insight exporters ran inline inside the plugin hooks. In
ON_CHANGEmode that put exporter I/O on the SDK threads that deliver checkpoint callbacks, so a slow exporter slowed the execution.Records are now handed to a background scheduler. It exports at most one record at a time. Each record is a complete snapshot of the execution, so while an export is in flight newer updates coalesce into a single pending slot and only the latest one is exported next. Exporters for one record run concurrently and independently, so one failing or slow exporter never blocks the others.
onInvocationEndis the hook the SDK awaits. It schedules the final record, waits for the scheduler to drain, and then flushes every exporter once. The final record is therefore always delivered before the invocation returns. If no worker can be started, the drain exports the pending record itself at that boundary, so nothing is silently dropped.Behavior changes visible to exporters:
export()runs on a worker thread, never on a hook thread.flush()is called once per invocation end (including non-terminal suspends), instead of after everyexport().RUNNINGsnapshots may be superseded by a newer snapshot while an export is in flight.Demo/Screenshots
N/A. Behavior is covered by unit tests.
Checklist
Testing
Unit Tests
Yes.
ExportSchedulerTest(new, 11 tests) covers: exports run off the scheduling thread; updates arriving before or during an export collapse into the latest record; a record scheduled after a pump finishes starts a new pump;drain()is idempotent when idle and blocks until the in-flight and pending records are exported; a failing exporter never blocks the others for the same record; exporters for one record run concurrently; when no worker can be started the pending record is exported inline bydrain()and a laterschedule()retries the worker.WorkflowInsightHookTestadds plugin-level coverage: bursts coalesce with the final record always delivered last and never on the hook thread; exporters are flushed once per invocation end even when nothing was emitted. Existing tests that read records right afteronInvocationStartnow drain through a package-private seam.mvn clean verifypasses across all modules. The concurrency-sensitive tests were repeated 8 times with no failures.Integration Tests
No new integration tests. The existing
LocalDurableTestRunner-based tests inWorkflowInsightPluginTestandTransformContractTestexercise the plugin end to end and still pass.Examples
N/A. No public API change.