CCOR-13193 - run integration tests against Conductor OSS, and fix two client bugs it surfaced - #156
Draft
chrishagglund-ship-it wants to merge 17 commits into
Draft
CCOR-13193 - run integration tests against Conductor OSS, and fix two client bugs it surfaced#156chrishagglund-ship-it wants to merge 17 commits into
chrishagglund-ship-it wants to merge 17 commits into
Conversation
…erprise for a few endpoints, but the 404 vs the oss-way explicitly
… scheduleAtFixedRate'd
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
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>
chrishagglund-ship-it
marked this pull request as draft
August 28, 2026 17:43
… 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
changed the title
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).