CCOR-13193 - run integration tests against Conductor OSS, and fix two client bugs it surfaced - #156
Open
chrishagglund-ship-it wants to merge 21 commits into
Open
chrishagglund-ship-it wants to merge 21 commits into
chrishagglund-ship-it wants to merge 21 commits into
Conversation
chrishagglund-ship-it
force-pushed
the
e2e-against-conductor-with-local-script
branch
from
August 18, 2026 15:44
05bc8d5 to
c49f547
Compare
chrishagglund-ship-it
marked this pull request as ready for review
August 18, 2026 16:08
chrishagglund-ship-it
marked this pull request as draft
August 28, 2026 17:43
chrishagglund-ship-it
changed the title
chrishagglund-ship-it
marked this pull request as ready for review
September 3, 2026 17:41
…erprise for a few endpoints, but the 404 vs the oss-way explicitly
… scheduleAtFixedRate'd
startPolling() builds a new TaskRunnerConfigurer, init()s it, and only then shuts the previous one down, so calling it twice leaves two runners polling the same task types. A task leased by the outgoing runner can be left in-progress until its response timeout expires -- the cause of the WorkflowSDKTests flakiness against OSS (that run logged three startPolling invocations). Removing the call from initWorkers() only fixed callers that don't also call startPolling() themselves, which the docs and examples/old/.../taskdomains/Main both do, and it forced scanWorkers() to rethrow so a scan failure wouldn't silently leave nothing polling. Guarding inside startPolling() instead fixes every caller shape and leaves initWorkers()'s contract alone, so scanWorkers() goes back to logging. startPolling() is now synchronized, matching the init* methods that call it, so the worker-set flag it reads is not raced. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
scheduleAtFixedRate cancels all future ticks on an uncaught exception, so the monitor must not let a failed getWorkflow escape. Catching unconditionally traded that for a workflow id that never resolves -- a purged workflow, expired credentials -- spinning at the 100ms poll interval forever, logging a stack trace each time while its caller blocks with no signal. Track when a run of consecutive failures started per workflow id: warn once, stay at DEBUG while it continues, and after a minute give up, drop the entry and completeExceptionally the future so the caller learns instead of hanging. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…eport why it fails initWorkers() followed by an explicit startPolling() is what the docs and examples do, so the integration test should exercise it; startPolling() is now idempotent, so the second call is a no-op rather than a runner restart. fail(e) rather than fail(e.getMessage()): a TimeoutException carries a null message, so this test's only CI failure surfaced as a bare AssertionFailedError with nothing to diagnose. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… reproduce it - distinct check_name per action-junit-report step; all of them defaulted to "JUnit Test Report", so the build job's report and the OSS job's landed on a single check run and overwrote each other - name the compose project, so the stack does not collide with the identically located compose file in the other SDK repos, on both project name and port - unset CONDUCTOR_AUTH_KEY/SECRET before the run: OSS has no /token endpoint, and ClientTestUtil builds its client with useEnvVariables(true), so a shell still holding Orkes credentials sent the whole run through an auth flow the local server cannot serve - point the script header at ci.yml, where the job actually lives Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
chrishagglund-ship-it
force-pushed
the
e2e-against-conductor-with-local-script
branch
from
September 8, 2026 18:03
0d55ef4 to
8104cee
Compare
…or fork repo PRs which won't get access to the variable.
Ports javascript-sdk#176's resolution of the same review feedback, so the OSS harness stays diffable across the SDKs. csharp-sdk#171 carries the identical change. One tag, one home. The tag was written twice: FORK_PR_FALLBACK_VERSION in the integration-tests-oss job, and `latest` as the local script's default. A local run therefore could not reproduce a CI failure, which is most of why the script exists. Rather than teach one of them to read the other, make the `image:` line of docker-compose-oss.yaml the only place it is written and let both fall through to it: the script applies no default of its own and exports OSS_CONDUCTOR_VERSION only when --version actually supplied one, and the fork-PR branch of the resolve step now leaves the variable unset instead of pinning its own copy. Fork CI and a plain local run reach the identical image by the identical path, with no YAML parsing on either side and one hardcode removed rather than a mechanism added. The non-fork empty case still fails loudly, and the workflow_run nuance is untouched -- that trigger runs in the base repo's context and still gets the org variable even for a fork PR. The script's "Using ..." and "Pulling ..." lines now come from `compose config --images conductor-server` instead of reconstructing the tag, so they stay honest whichever source supplied it. The compose header already noted that E2E_TEST_OSS_CONDUCTOR_VERSION is set to `latest` org-wide, so a normal CI run is not really pinned. That stays, with the addition that this default is now what actually pins fork PRs and local runs until someone sets the org variable to a real tag. Pull in CI. The script pulls before `up`, CI did not. On a GitHub-hosted runner the VM is ephemeral and starts with no cached copy, so `up` pulls anyway and the step is redundant today — kept regardless, because it costs no extra network pull (`up` then finds the image locally), it splits "couldn't pull the image" from "the stack didn't come up" into two distinct red steps, and it is what stops a mutable tag going stale the day this job moves to a self-hosted runner with a warm Docker daemon. It also prints the tag in use, which matters now that a fork PR's tag is not spelled out in the workflow. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
chrishagglund-ship-it
requested review from
ambiorix2099,
bradyyie and
mp-orkes
September 15, 2026 15:00
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.
Pull Request type
Changes in this PR
Added an integration test run against a Conductor OSS server, as a second job in
integration-tests.yml. The image tag comes from theE2E_TEST_OSS_CONDUCTOR_VERSIONorg variable (or aworkflow_dispatchinput); that variable is currently set tolatest, so the job tracks whateverlatestresolves to at run time rather than a fixed version. Set the org variable to a real tag if we want it deterministic.scripts/run-integration-oss.shreproduces the CI job locally (see CONTRIBUTING.md). Tests OSS can't serve are gated with@DisabledIfEnvironmentVariable(CONDUCTOR_SERVER_TYPE=oss), each carrying its specific reason — 53 of 106 skip today.push:trigger so the new job can run pre-merge. Remove before merging — it's fenced in a comment. Note that removing it also removes the only trigger this job has ever run under:workflow_runexecutes the default branch's copy of the workflow, so the merged configuration runs for the first time onmain.Getting that suite green turned up two real client bugs, both fixed here:
AnnotatedWorkerExecutor.startPolling()is now idempotent. It stood up a replacementTaskRunnerConfigurerand shut the old one down after starting the new one, so a double call left two runners polling the same task types and could strand a task the outgoing runner had already leased.initWorkers()called it twice on its own. It's now a no-op unless workers were added since the last start.shutdown()clears the runner so a later restart still works, andaddBean()is synchronized so the flag it sets is safely published.WorkflowExecutor's completion monitor no longer dies on a poll failure. It runs underscheduleAtFixedRate, which cancels every future tick on an uncaught exception, so a single failedgetWorkflowpermanently killed completion tracking for every workflow. The failure is now caught and logged, that one workflow stops being tracked and its future is completed exceptionally, and the monitor keeps polling everything else. Per-workflow behavior is deliberately unchanged from before: one failure ends tracking, no retry. No new public API.Six unit tests cover both fixes (four in
AnnotatedWorkerTests, two inWorkflowExecutorMonitorTests).