Conversation
The scheduler held a single pending slot and let any newer record replace the one waiting. That is only valid for records of the same execution: a snapshot of execution B carries none of execution A's information, so when records of two executions interleaved, A's pending record was discarded and nothing else would deliver it. At invocation end this dropped A's terminal record while A's end hook was already waiting in drain(). Pending records are now keyed by execution ARN in insertion order. A newer record still supersedes an older one of the same execution, records of different executions never displace each other, and executions are served in the order they first became pending. Exactly one export remains in flight at a time.
16 tasks
Contributor
Author
|
/ai review |
This comment has been minimized.
This comment has been minimized.
Contributor
Codex AI reviewNo actionable findings. Residual risk is limited to concurrency timing because tests were not executed under the review constraints. Reviewed commit |
2 tasks
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
Addresses the item reported in #679 (comment) (part of #679; the issue stays open for the remaining follow-ups).
Description
ExportSchedulerheld one pending slot and let any newer record replace the one waiting. That rule is only valid for records of the same execution. A snapshot of execution B carries none of execution A's information, so when records of two executions interleaved, A's pending record was discarded and nothing else would deliver it. At invocation end this dropped A's terminal record while A's end hook was already waiting indrain().The pending slot is now a map keyed by execution ARN in insertion order. A newer record still supersedes an older one of the same execution. Records of different executions never displace each other. Executions are served in the order they first became pending, and a re-scheduled execution keeps its place, so a chatty execution cannot starve a quieter one. Exactly one export stays in flight at a time, so the exporter contract is unchanged.
Memory stays bounded at one record per concurrently active execution, and every pump drains the map to empty.
Demo/Screenshots
N/A. Behavior is covered by unit tests.
Checklist
Testing
Unit Tests
Yes. Three new
ExportSchedulerTestcases: records of different executions never displace each other; coalescing stays within one execution and executions are served in first-pending order; another execution's update while an export is in flight cannot drop a pending final record. One newWorkflowInsightHookTestcase reproduces the reported sequence end to end: execution A's end hook waits in the drain behind a blocked export while execution B's change hook arrives, and both executions'SUCCEEDEDrecords are delivered.All four tests were run against the previous scheduler and fail there with the reported symptom (A delivers only
RUNNING).mvn clean verifypasses across all modules; the two concurrency-sensitive test classes were repeated 10 times with no failures.Integration Tests
No new integration tests; the existing
LocalDurableTestRunner-based plugin tests still pass.Examples
N/A. No public API change.