Implement the Dispatcher and its ddev cli command - #24935
Merged
Conversation
evalya-impact-summaryevalya impact analysis |
Contributor
|
HadhemiDD
changed the title
AAraKKe
requested changes
Aug 24, 2026
AAraKKe
left a comment
AAraKKe
left a comment
Collaborator
There was a problem hiding this comment.
@HadhemiDD I know this is in draft and probably will change quickly but I have a comment to extract the update/fix on the orchestrator to its own PR. Lets do that I will continue the review once it is extracted and you find it ready for review.
`asyncio.Queue` is not thread-safe, and a `SyncProcessor` runs in an executor thread. A `put_nowait` from there landing between `Queue.get`'s `empty()` check and its waiter being registered wakes nobody: the message stays in the deque, the loop awaits a waiter that will never resolve, and because the queue is not empty the bus never reaches its stop condition and spins until `max_timeout`. Submissions from off the loop thread now hop onto it with `call_soon_threadsafe`. A caller already on that thread, or with no bus running, still puts directly, so a message is queued by the time `submit_message` returns. The loop is captured in `initialize`, the first point at which one exists: processors are registered from `__init__`, on a thread that has none. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
3 tasks
HadhemiDD
force-pushed
the
hs/dispatcher-dispatch-tests-command
branch
from
August 24, 2026 12:07
24ce45d to
f45f8a1
Compare
HadhemiDD
changed the base branch from
master
to
hs/event-bus-thread-safe-submit
August 24, 2026 12:07
Contributor
Author
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f45f8a1e78
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
`register_processor` handed a processor its queue but not the loop, and `initialize` captured the loop before `on_initialize` runs. A `SyncProcessor` registered from that hook kept `loop is None` and submitted directly from its executor thread, which is the case this fix exists to remove. It now takes the loop at registration when the bus is already running. `asyncio.run` closes the loop when the bus stops, so a submission afterwards tried to schedule onto a dead loop and raised. A closed loop is queued into directly instead, as it was before there was a loop to hop onto. The regression test asserted which thread each put came from, which pinned it to `call_soon_threadsafe` rather than to the contract. It now asserts only that the message is delivered, against a queue that drops every off-loop put. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
HadhemiDD
force-pushed
the
hs/dispatcher-dispatch-tests-command
branch
from
August 24, 2026 13:36
f45f8a1 to
bd0cfb9
Compare
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
HadhemiDD
force-pushed
the
hs/dispatcher-dispatch-tests-command
branch
from
August 24, 2026 14:10
bd0cfb9 to
d7b581c
Compare
HadhemiDD
marked this pull request as ready for review
August 24, 2026 14:50
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d7b581cf5f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Restores the orchestrator's original design: a processor holds the bus it was registered in and delegates submit_message to it, so the thread-safe put lives in one place instead of being assembled from two injected variables. The loop is read at submit time rather than captured per processor, which drops running_loop(), processor.loop, processor.queue, the is_closed() check, the on-loop fast path, the RuntimeError guard, and the duplicate submit path. It also makes a mid-run registration correct without a branch. Every put now goes through the loop thread while the bus runs, so a worker's earlier submit can no longer land behind a later on-loop one. The bus captures the loop after on_initialize: that hook's submits have no task completion to be ordered behind, and a zero grace period stops the bus before a deferred put would run. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
HadhemiDD
force-pushed
the
hs/dispatcher-dispatch-tests-command
branch
from
August 25, 2026 10:44
d7b581c to
74a67ba
Compare
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Adds `Dispatcher`, an `EventBusOrchestrator` that owns a batching plan and registers the runner, gatherer and pull-request updater on one bus, and the `ddev ci dispatch-tests` command that builds and runs it. Every input can be passed explicitly, which is how a workflow calls it. Locally, `--pr` takes a number or a URL and reads the branch, commits and target branch from the GitHub API, and `--dry-run` shows the plan without touching GitHub. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`asyncio.Queue` is not thread-safe, and a `SyncProcessor` runs in an executor thread. A `put_nowait` from there landing between `Queue.get`'s `empty()` check and its waiter being registered wakes nobody: the message stays in the deque, the loop awaits a waiter that will never resolve, and because the queue is not empty the bus never reaches its stop condition and spins until `max_timeout`. The gatherer is the only cross-thread emitter, so this was reachable the moment the Dispatcher chained it to the pull-request updater. A single-batch run — a pull request touching one integration — is the widest window, and would have hung for the full three-hour timeout while still reporting success. Submissions from off the loop thread now hop onto it. A caller already there, or with no bus running, still puts directly, so a message is queued by the time `submit_message` returns. Also: a run only counts as successful once its final report has been published, so a stall after the last batch can no longer exit 0 with a stale comment; and the run-summary test reads its file as UTF-8, which Windows does not default to. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Reuse the PR-reference helpers `port-commit` already had rather than carrying a second copy: `resolve_owner_repo` and the pull-request URL pattern move to `ddev.utils.github`, and both commands read them there. Report a fatal bus failure as a message instead of a traceback, drop the two `DispatcherOutcome` fields nothing reads, and move the grace period into `DispatcherConfig` beside the other bus timings. The initial update's message id moves to the gatherer, which is the only thing that builds it. On the tests: use the fake client's own response wrapping, share one `TestBatch` builder, and assert the run summary at the layer that owns it rather than restating the renderer's contract. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Half the fields carried a comment and half carried nothing, which reads as an oversight rather than a choice. Every field is now documented the same way, below the field, where the documentation is attached to what it describes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`final_report_published` and `DispatcherOutcome.successful` claimed the final report reached a reader. On the no-pull-request path the flag only means there was no comment to lose it to, and `write_step_summary` is a documented no-op outside GitHub Actions. Both docstrings now say what holds. Not failing a run that has nowhere to report to is deliberate, so the behaviour is unchanged. The `--context` test read Click's `params` and compared the choices with the enum, which passes even if the option is wired to the wrong destination. It now parameterizes over `RunContext` and runs the command, so it still catches a member that was never wired up while asserting what a caller can observe. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
HadhemiDD
force-pushed
the
hs/dispatcher-dispatch-tests-command
branch
from
August 25, 2026 11:08
74a67ba to
138b6ed
Compare
AAraKKe
requested changes
Aug 25, 2026
AAraKKe
left a comment
AAraKKe
left a comment
Collaborator
There was a problem hiding this comment.
Thanks @HadhemiDD! Take a look at codex comments since I think some are uresolved. Then I have just some comments really and many of the things I was going to comment I decided to not add them at all. I believe the implementation is solid and what I think might not be (both things I commented and decided to withhold) will be visible 100% when we are running this in the workflows. For now we can keep them as is and we'll figure it out when running in the workflows if things need tweaking.
There is no point in trying to refine everything since we will see it clearly once running int he workflows.
| default=None, | ||
| help='Pull request to test, as a number or a URL. Its branch, commits and target branch are read from GitHub.', | ||
| ) | ||
| @click.option('--pr-number', type=int, default=None, help='Pull request number, when not using `--pr`.') |
Collaborator
There was a problem hiding this comment.
request: this is unnecessary, since --pr also accept the number having this option is redundant. Remove it.
Collaborator
There was a problem hiding this comment.
Same about this comment, ignore it for now. This will be better decided once we are in the workflow. I am not sure if what I am asking is better than what it is already in the PR so feel free to ignore it. I am keeping these messages for bookkeeping for alter.
Contributor
Author
There was a problem hiding this comment.
safe to ignore for now
Comment on lines
+33
to
+36
| @click.option('--checkout-sha', default=None, help='Ref the test workflow checks out. Defaults to the base commit.') | ||
| @click.option('--base-sha', default=None, help='Commit the run reports against. Defaults to the local HEAD.') | ||
| @click.option('--branch', default=None, help='Branch being tested. Defaults to the current branch.') | ||
| @click.option('--target-branch', default=None, help='Target branch of the pull request, used as the comparison base.') |
Collaborator
There was a problem hiding this comment.
request: I think this can be simplified. There are 2 modes dispatcher is going to run:
- In a PR: the comparison needs to be done between the commit it is running in and the merge base between the branches of the PR (if the PR is to merge branch A into B, the comparison needs to be done between A and the merge base between A and B).
- In a branch: the comparison is done between the current commit and the commit prior to it.
I would made the default mode the second one. That is the simplest case to handle, compare this commit with the prior commit. And change the run mode from that to PR when a PR is supplied. In the second case we don't care about branches at all, we are running comparing one commit with the previous one, whatever branch that is.
If a PR is supplied we don't need to supply anything else. We already know from the PR which commit we are running in (the merge commit) we also know the ahead and the base of the prs to get the merge base.
Collaborator
There was a problem hiding this comment.
Thinking about this a bit more... ignore this comment. I think it can be simplified but it is somethign we will know for sure once we have to run this in a workflow since we have information on the workflow we might be better off using. We can leave this as it is and decided once we put this into a worfklow.
Contributor
Author
There was a problem hiding this comment.
safe to ignore for now
Adds validate_options as the command's first call. It refuses the four options --pr reads from GitHub (--pr-number, --branch, --base-sha, --target-branch), parses the --pr reference, and requires a token for any run that reads a pull request or dispatches anything, leaving a dry run planning from local git as the one case that needs none. It returns the parsed number and the token, so fetch_pull_request takes both as arguments and holds no validation of its own: the token was previously looked up and checked in two places with two messages. Also drops --artifacts-dir. Artifacts already default to a subfolder of the output directory and nothing passed the option, so all it offered was a way to scatter one run's outputs across unrelated places. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Master renamed TaskPullRequestUpdater to TaskRunReporter, so the Dispatcher and its tests, which are new here, follow the new names. The conflict was the gatherer test's imports, where both sides had changed the same line. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
Validation ReportAll 21 validations passed. Show details
|
AAraKKe
approved these changes
Aug 25, 2026
AAraKKe
left a comment
AAraKKe
left a comment
Collaborator
There was a problem hiding this comment.
Thanks @HadhemiDD, there are a couple of issues with validations and when they fail but they are minimal changes I can fix tomorrow. No need to delay the full review on it.
github-actions Bot
pushed a commit
that referenced
this pull request
Aug 25, 2026
* Make a worker-thread submission reach the event bus `asyncio.Queue` is not thread-safe, and a `SyncProcessor` runs in an executor thread. A `put_nowait` from there landing between `Queue.get`'s `empty()` check and its waiter being registered wakes nobody: the message stays in the deque, the loop awaits a waiter that will never resolve, and because the queue is not empty the bus never reaches its stop condition and spins until `max_timeout`. Submissions from off the loop thread now hop onto it with `call_soon_threadsafe`. A caller already on that thread, or with no bus running, still puts directly, so a message is queued by the time `submit_message` returns. The loop is captured in `initialize`, the first point at which one exists: processors are registered from `__init__`, on a thread that has none. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Add changelog entry * Close the remaining loop-capture gaps and assert delivery `register_processor` handed a processor its queue but not the loop, and `initialize` captured the loop before `on_initialize` runs. A `SyncProcessor` registered from that hook kept `loop is None` and submitted directly from its executor thread, which is the case this fix exists to remove. It now takes the loop at registration when the bus is already running. `asyncio.run` closes the loop when the bus stops, so a submission afterwards tried to schedule onto a dead loop and raised. A closed loop is queued into directly instead, as it was before there was a loop to hop onto. The regression test asserted which thread each put came from, which pinned it to `call_soon_threadsafe` rather than to the contract. It now asserts only that the message is delivered, against a queue that drops every off-loop put. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Trim the worker-thread delivery test's docstring Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Give each processor a reference to its bus Restores the orchestrator's original design: a processor holds the bus it was registered in and delegates submit_message to it, so the thread-safe put lives in one place instead of being assembled from two injected variables. The loop is read at submit time rather than captured per processor, which drops running_loop(), processor.loop, processor.queue, the is_closed() check, the on-loop fast path, the RuntimeError guard, and the duplicate submit path. It also makes a mid-run registration correct without a branch. Every put now goes through the loop thread while the bus runs, so a worker's earlier submit can no longer land behind a later on-loop one. The bus captures the loop after on_initialize: that hook's submits have no task completion to be ordered behind, and a zero grace period stops the bus before a deferred put would run. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Trim the submit_message docstring Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Wire the Dispatcher together and give it an entry point Adds `Dispatcher`, an `EventBusOrchestrator` that owns a batching plan and registers the runner, gatherer and pull-request updater on one bus, and the `ddev ci dispatch-tests` command that builds and runs it. Every input can be passed explicitly, which is how a workflow calls it. Locally, `--pr` takes a number or a URL and reads the branch, commits and target branch from the GitHub API, and `--dry-run` shows the plan without touching GitHub. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Add changelog entry Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Make a worker-thread submission reach the event bus `asyncio.Queue` is not thread-safe, and a `SyncProcessor` runs in an executor thread. A `put_nowait` from there landing between `Queue.get`'s `empty()` check and its waiter being registered wakes nobody: the message stays in the deque, the loop awaits a waiter that will never resolve, and because the queue is not empty the bus never reaches its stop condition and spins until `max_timeout`. The gatherer is the only cross-thread emitter, so this was reachable the moment the Dispatcher chained it to the pull-request updater. A single-batch run — a pull request touching one integration — is the widest window, and would have hung for the full three-hour timeout while still reporting success. Submissions from off the loop thread now hop onto it. A caller already there, or with no bus running, still puts directly, so a message is queued by the time `submit_message` returns. Also: a run only counts as successful once its final report has been published, so a stall after the last batch can no longer exit 0 with a stale comment; and the run-summary test reads its file as UTF-8, which Windows does not default to. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Apply review findings to the Dispatcher entry point Reuse the PR-reference helpers `port-commit` already had rather than carrying a second copy: `resolve_owner_repo` and the pull-request URL pattern move to `ddev.utils.github`, and both commands read them there. Report a fatal bus failure as a message instead of a traceback, drop the two `DispatcherOutcome` fields nothing reads, and move the grace period into `DispatcherConfig` beside the other bus timings. The initial update's message id moves to the gatherer, which is the only thing that builds it. On the tests: use the fake client's own response wrapping, share one `TestBatch` builder, and assert the run summary at the layer that owns it rather than restating the renderer's contract. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Document DispatcherConfig fields as attribute docstrings Half the fields carried a comment and half carried nothing, which reads as an oversight rather than a choice. Every field is now documented the same way, below the field, where the documentation is attached to what it describes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Say what the final report guarantees, and test --context through the CLI `final_report_published` and `DispatcherOutcome.successful` claimed the final report reached a reader. On the no-pull-request path the flag only means there was no comment to lose it to, and `write_step_summary` is a documented no-op outside GitHub Actions. Both docstrings now say what holds. Not failing a run that has nowhere to report to is deliberate, so the behaviour is unchanged. The `--context` test read Click's `params` and compared the choices with the enum, which passes even if the option is wired to the wrong destination. It now parameterizes over `RunContext` and runs the command, so it still catches a member that was never wired up while asserting what a caller can observe. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Validate every option before the run does any work Adds validate_options as the command's first call. It refuses the four options --pr reads from GitHub (--pr-number, --branch, --base-sha, --target-branch), parses the --pr reference, and requires a token for any run that reads a pull request or dispatches anything, leaving a dry run planning from local git as the one case that needs none. It returns the parsed number and the token, so fetch_pull_request takes both as arguments and holds no validation of its own: the token was previously looked up and checked in two places with two messages. Also drops --artifacts-dir. Artifacts already default to a subfolder of the output directory and nothing passed the option, so all it offered was a way to scatter one run's outputs across unrelated places. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> 916e4f4
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
2 participants
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.
What does this PR do?
Connects the Dispatcher's pieces and gives them a way to be run.
Everything the Dispatcher needs has landed over the last few PRs — the event bus, the batching plan (#24687), the test runner (#23518), the gatherer (#23938, #24774), the pull-request updater (#24822) — and none of it was reachable. Nothing called
build_test_batchesat runtime, nothing registered the three processors on a bus, nothing publishedbuild_initial_update()orrender_run_summary(), and noRateLimiterFactorylimiter was ever handed to a client. #24822 said as much: "Wiring Dispatcher to actually create this updater is the entry-point task (AI-6474 / AI-6484)."Two things close that gap:
Dispatcher(ci/tests/dispatcher.py), anEventBusOrchestratorthat owns the plan. The whole plan is known before the bus starts, soon_initializeprimes the queue with revision0and every batch, and the tasks carry it from there:TestBatch→ runner →BatchFinished→ gatherer →UpdatePRComment→ updater.on_finalizepublishes the report to the run summary — the only report a run without a pull request has — closes the client, and exposes the outcome the caller exits on.build_dispatcherassembles the client, the three tasks and the bus, so one client and one rate limiter are shared by every task and the run's request rate is bounded as a whole rather than per task.ddev ci dispatch-tests, the command. Every input can be passed explicitly, which is how a workflow will call it. Locally,--prtakes a number or a URL and reads the branch, commits and target branch from the GitHub API — the affordanceddev release port-commitalready offers — and--dry-runshows the plan without touching GitHub at all.Also here:
AllTargetsRulebehind--all, for the runs where the change set is not what decides which targets run (a push tomaster, the nightly schedule, the Agent test workflow);[dispatcher]gainsworkflow,workflow_refandpoll_interval_seconds; and the gatherer gains aprogressproperty so the orchestrator can read the terminal aggregate without going through a message.Notes for reviewers
A pull request is tested at its merge commit but reported against its head.
checkout_shabecomesrefs/pull/<n>/merge, which is what the batch workflow checks out;base_shastays the head commit, which is what the check runs and the metrics belong to. Outside a pull request the two are the same.--workflow-refdefaults tomasterand should stay there. The workflow definition has to come from a reviewed branch even when the code under test does not.A run whose results are unknown is not green. The command exits non-zero when any batch failed, when the report never reached its comment, or when a batch never finished at all — the last being why
progress.doneis part of the success condition rather than just the per-batch statuses.The queue is primed in
on_initialize, not beforerun(). The design doc shows the command submitting revision0and every batch and then callingrun(). Doing it from the hook instead means the bus has attached its queue and captured its loop before anything is submitted, which the cross-thread fix below depends on. Same ordering, same messages.ddev/src/ddev/cli/release/port_commit_workflow.pyis in the diff on purpose. It already hadresolve_owner_repoand a byte-identical pull-request URL pattern, so both moved toddev/utils/github.pyand the two commands share them rather than this PR adding a second copy. The async PR fetchers stay separate:port-commitraises_PRNotFoundon 404 for its retry flow, this command aborts.Planning is local. A commit the clone has never fetched aborts with a message saying so, rather than a git stack trace. Verified against a merged pull request whose head branch is gone.
Not in this PR, and deliberately: retries (
BatchAttemptFinished,rerun-failed-jobs) and the RFC'sagent_integrations.test_dispatcher.*metrics. Both are described in the execution and PR reporting design and neither has any code yet. Thetest-batch.ymlworkflow is also still to come; the command names it through configuration so nothing here changes when it lands.Motivation
AI-6474 and AI-6484. The Dispatcher has been built for several months without ever having been run. This is the change that lets it run — and lets a human run it locally, against a real pull request, before it is pointed at CI.
Review checklist (to be filled by reviewers)
qa/requiredif this PR needs QA validation, orqa/skip-qaif it does not. Exactly one of the two is required.backport/<branch-name>label to the PR and it will automatically open a backport PR once this one is merged