diff --git a/.github/workflows/agent-e2e.yml b/.github/workflows/agent-e2e.yml index 6202f76e..158a027a 100644 --- a/.github/workflows/agent-e2e.yml +++ b/.github/workflows/agent-e2e.yml @@ -18,8 +18,19 @@ concurrency: group: agent-e2e-${{ github.ref }} cancel-in-progress: true +# Deliberately NOT the same knob as the integration-tests-oss job in +# pull_request.yml, despite the similar name. That job pins a *Docker image +# tag* (conductoross/conductor on Docker Hub), defaulted in +# scripts/docker-compose-oss.yaml and overridable via the +# E2E_TEST_OSS_CONDUCTOR_VERSION org variable. This is a *Maven Central +# artifact version* for the conductor-server boot JAR, which is a different +# registry with a different set of published versions -- notably it has no +# `latest`, which is what that org variable currently holds. It also carries a +# constraint that job does not: the /agent/* control plane and the +# TaskDef.runtimeMetadata persistence these suites exercise only exist from +# 3.32.0-rc.8 onward. So it is pinned independently and bumped deliberately. env: - CONDUCTOR_OSS_VERSION: ${{ vars.CONDUCTOR_SERVER_VERSION || '3.32.1' }} + CONDUCTOR_OSS_VERSION: ${{ vars.CONDUCTOR_SERVER_VERSION || '3.32.1' }} # pinned conductor-oss release — see note above jobs: agent-e2e: diff --git a/.github/workflows/pull_request.yml b/.github/workflows/pull_request.yml index a102c49f..e1aca655 100644 --- a/.github/workflows/pull_request.yml +++ b/.github/workflows/pull_request.yml @@ -6,6 +6,11 @@ on: - main pull_request: workflow_dispatch: + inputs: + oss_conductor_version: + description: 'OSS Conductor image tag (falls back to E2E_TEST_OSS_CONDUCTOR_VERSION org var, then to a pinned default on fork PRs)' + required: false + type: string concurrency: group: ${{ github.workflow }}-${{ github.ref }} @@ -217,6 +222,33 @@ jobs: CONDUCTOR_RETRY_SERVER_ERRORS: "true" HTTPBIN_SERVICE_HOSTNAME: httpbin steps: + # OSS_CONDUCTOR_VERSION is resolved here rather than in the job `env` so + # that the two ways it can come back empty get different treatment: + # + # - Fork PR: GitHub withholds org/repo variables from pull_request runs + # on forks exactly as it withholds secrets, so vars.* is always "" for + # an outside contributor (observed in csharp-sdk#178). This job needs + # no secrets, only a tag, so leave the var unset and let the default + # baked into the `image:` line of scripts/docker-compose-oss.yaml + # apply. That is the same tag a plain local run of + # scripts/run-integration-oss.sh gets, and the one place it is + # written -- no second copy to drift out of sync here. + # - Anything else: the org variable is genuinely missing or its + # repository access policy no longer covers this repo. Fail loudly + # rather than silently drifting onto the default. + - name: Resolve OSS Conductor version + env: + REQUESTED_VERSION: ${{ inputs.oss_conductor_version || vars.E2E_TEST_OSS_CONDUCTOR_VERSION }} + IS_FORK_PR: ${{ github.event_name == 'pull_request' && github.event.pull_request.head.repo.full_name != github.repository }} + run: | + if [ -n "$REQUESTED_VERSION" ]; then + echo "OSS_CONDUCTOR_VERSION=${REQUESTED_VERSION}" >> "$GITHUB_ENV" + elif [ "$IS_FORK_PR" = "true" ]; then + echo "::notice::Fork PR: org variables are withheld, falling back to the default tag in scripts/docker-compose-oss.yaml" + else + echo "::error::No Conductor OSS image tag resolved. Set the E2E_TEST_OSS_CONDUCTOR_VERSION organization variable (and ensure its repository access policy includes this repo), or pass the oss_conductor_version input via workflow_dispatch." + exit 1 + fi - name: Checkout uses: actions/checkout@v4 - name: Set up Node @@ -235,6 +267,20 @@ jobs: - name: Install Dependencies if: steps.cache.outputs.cache-hit != 'true' run: npm ci + # `docker compose up` only pulls an image when it is missing locally. On a + # GitHub-hosted runner the VM is ephemeral and starts with no cached copy + # of this image, so `up` would pull anyway and this step is redundant + # today. It is here deliberately: it costs no extra network pull (`up` + # then finds the image locally), it separates "couldn't pull the image" + # from "the stack didn't come up" into two distinct red steps, and it is + # what keeps a mutable tag from going stale if this job ever moves to a + # self-hosted runner with a warm Docker daemon -- the same reason + # scripts/run-integration-oss.sh pulls. It also prints the tag actually in + # use, which for a fork PR comes from the compose file's default. + - name: Pull Conductor OSS image + run: | + echo "Using $(docker compose -f scripts/docker-compose-oss.yaml config --images conductor-server)" + docker compose -f scripts/docker-compose-oss.yaml pull conductor-server - name: Start Conductor OSS stack run: docker compose -f scripts/docker-compose-oss.yaml up -d - name: Wait for Conductor to be healthy diff --git a/scripts/docker-compose-oss.yaml b/scripts/docker-compose-oss.yaml index 012a750f..58613c56 100644 --- a/scripts/docker-compose-oss.yaml +++ b/scripts/docker-compose-oss.yaml @@ -4,9 +4,16 @@ # # The Conductor server reaches httpbin over the compose network at # http://httpbin:8081, which matches HTTPBIN_SERVICE_HOSTNAME=httpbin. +# +# The `image:` default below is the SINGLE place the Conductor OSS image tag is +# written. Everything that does not override OSS_CONDUCTOR_VERSION lands on it: +# a plain `scripts/run-integration-oss.sh` run, and the integration-tests-oss +# job on a fork PR (where GitHub withholds org variables). Overrides are the +# script's --version flag and, in CI, the E2E_TEST_OSS_CONDUCTOR_VERSION org +# variable or a workflow_dispatch input. Bump the tag here and both follow. services: conductor-server: - image: conductoross/conductor:latest + image: conductoross/conductor:${OSS_CONDUCTOR_VERSION:-3.32.3} environment: - CONFIG_PROP=config-postgres.properties ports: diff --git a/scripts/run-integration-oss.sh b/scripts/run-integration-oss.sh index 54945727..b5e4708e 100755 --- a/scripts/run-integration-oss.sh +++ b/scripts/run-integration-oss.sh @@ -12,12 +12,13 @@ # scripts/oss-test-run.log, override with -l|--log) so it can be shared later. # # Usage: -# scripts/run-integration-oss.sh [-t|--test ] [-l|--log ] [--keep-up] [-- jest args] +# scripts/run-integration-oss.sh [-t|--test ] [-l|--log ] [--keep-up] [--version ] [-- jest args] # Examples: -# scripts/run-integration-oss.sh # full OSS-gated suite +# scripts/run-integration-oss.sh # full OSS-gated suite, default image tag # scripts/run-integration-oss.sh --test WorkflowExecutor # scripts/run-integration-oss.sh --log /tmp/oss.log # custom log path # scripts/run-integration-oss.sh --keep-up # leave the stack running afterwards +# scripts/run-integration-oss.sh --version 3.33.0-rc1 # override the OSS image tag # scripts/run-integration-oss.sh -- --testPathPatterns="EventClient" set -euo pipefail @@ -35,12 +36,22 @@ while [[ $# -gt 0 ]]; do -t|--test) TEST_PATTERN="${2:?--test needs a path or pattern}"; shift 2 ;; -l|--log) LOG_FILE="${2:?--log needs a file path}"; shift 2 ;; --keep-up) KEEP_UP=1; shift ;; + --version) OSS_CONDUCTOR_VERSION="${2:?--version needs a tag}"; shift 2 ;; -h|--help) usage; exit 0 ;; --) shift; extra=("$@"); break ;; *) echo "Unknown argument: $1" >&2; usage; exit 1 ;; esac done +# No default is applied here on purpose. The default tag is written once, in the +# `image:` line of scripts/docker-compose-oss.yaml, so leaving OSS_CONDUCTOR_VERSION +# unset lets compose supply it -- the same path a fork PR takes in CI. Only export +# it when the caller actually asked for a specific tag, otherwise a value set but +# not exported in the caller's shell would never reach compose anyway. +if [[ -n "${OSS_CONDUCTOR_VERSION:-}" ]]; then + export OSS_CONDUCTOR_VERSION +fi + SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" REPO_ROOT="$(cd "${SCRIPT_DIR}/.." && pwd)" COMPOSE_FILE="${SCRIPT_DIR}/docker-compose-oss.yaml" @@ -76,6 +87,11 @@ HEALTH_URL="${CONDUCTOR_SERVER_URL%/api}/health" compose() { docker compose -f "${COMPOSE_FILE}" "$@"; } cleanup() { + local status=$? + if [[ "${status}" -ne 0 ]]; then + echo "Dumping conductor-server logs (exit ${status})..." >&2 + compose logs conductor-server || true + fi if [[ "${KEEP_UP}" == "1" ]]; then echo "--keep-up set: leaving the OSS stack running. Tear down with:" echo " docker compose -f ${COMPOSE_FILE} down -v" @@ -86,6 +102,18 @@ cleanup() { } trap cleanup EXIT +# Ask compose what it resolved rather than reconstructing the tag here, so this +# stays correct whether the tag came from --version or from the compose default. +SERVER_IMAGE="$(compose config --images conductor-server | head -1)" +echo "Using ${SERVER_IMAGE}" + +# `docker compose up` only pulls an image when it is missing locally, so a +# previously-cached mutable tag (a re-pushed rc, or `latest` if that is what was +# asked for) would silently be reused instead of getting the current version. +# Pull unconditionally so the stack always reflects the tag we just printed. +echo "Pulling ${SERVER_IMAGE} to ensure it's current..." +compose pull conductor-server + echo "Starting Conductor OSS stack (${COMPOSE_FILE})..." compose up -d @@ -96,7 +124,6 @@ deadline=$(( SECONDS + HEALTH_TIMEOUT )) until curl -sf "${HEALTH_URL}" >/dev/null 2>&1; do if (( SECONDS >= deadline )); then echo "Error: Conductor did not become healthy within ${HEALTH_TIMEOUT}s." >&2 - compose logs conductor-server || true exit 1 fi sleep 5 diff --git a/src/integration-tests/WorkflowExecutor.test.ts b/src/integration-tests/WorkflowExecutor.test.ts index d0e4f2ff..1f8bc8de 100644 --- a/src/integration-tests/WorkflowExecutor.test.ts +++ b/src/integration-tests/WorkflowExecutor.test.ts @@ -34,6 +34,7 @@ import { describeForOrkesV5, describeForOrkesOnlyV4, describeForOrkesOnlyV5, + testForRegionDurable, } from "./utils/customJestDescribe"; import { registerWorkflowDefWithRetry, registerWorkflowWithRetry } from "./utils/registerWorkflowWithRetry"; import { HTTPBIN_BASE_URL } from "./utils/testConstants"; @@ -427,7 +428,18 @@ describe("WorkflowExecutor", () => { describe("Execute Workflow with Return Strategies and Consistency Levels", () => { // Test data for combinations - const testCombinations = [ + interface ConsistencyTestCase { + name: string; + consistency: Consistency; + returnStrategy: ReturnStrategy; + shouldHaveWorkflowFields: boolean; + shouldHaveTaskFields: boolean; + // Needs a server with cross-region replication configured; see + // testForRegionDurable. + requiresRegionDurable?: boolean; + } + + const testCombinations: ConsistencyTestCase[] = [ // SYNCHRONOUS consistency tests { name: "SYNC + TARGET_WORKFLOW", @@ -457,34 +469,75 @@ describe("WorkflowExecutor", () => { shouldHaveWorkflowFields: false, shouldHaveTaskFields: true, }, - // REGION_DURABLE consistency tests + // DURABLE consistency tests. DURABLE is the default consistency level and + // takes a different executor path than SYNCHRONOUS (async decider queue via + // scheduleWorkflow, rather than an inline decide), so the return strategies + // are worth covering against it too. These cases are what the REGION_DURABLE + // block below was really exercising before orkes-conductor 5.5.0, when + // REGION_DURABLE was accepted and silently treated as a plain local start. { name: "DURABLE + TARGET_WORKFLOW", - consistency: Consistency.REGION_DURABLE, + consistency: Consistency.DURABLE, returnStrategy: ReturnStrategy.TARGET_WORKFLOW, shouldHaveWorkflowFields: true, shouldHaveTaskFields: false, }, { name: "DURABLE + BLOCKING_WORKFLOW", - consistency: Consistency.REGION_DURABLE, + consistency: Consistency.DURABLE, returnStrategy: ReturnStrategy.BLOCKING_WORKFLOW, shouldHaveWorkflowFields: true, shouldHaveTaskFields: false, }, { name: "DURABLE + BLOCKING_TASK", - consistency: Consistency.REGION_DURABLE, + consistency: Consistency.DURABLE, returnStrategy: ReturnStrategy.BLOCKING_TASK, shouldHaveWorkflowFields: false, shouldHaveTaskFields: true, }, { name: "DURABLE + BLOCKING_TASK_INPUT", + consistency: Consistency.DURABLE, + returnStrategy: ReturnStrategy.BLOCKING_TASK_INPUT, + shouldHaveWorkflowFields: false, + shouldHaveTaskFields: true, + }, + // REGION_DURABLE consistency tests. Gated on CONDUCTOR_REGION_DURABLE_ENABLED + // — see testForRegionDurable. These were previously named "DURABLE + *" while + // requesting REGION_DURABLE, which is why they only started failing when + // sdkdev moved to a server build that honours the flag. + { + name: "REGION_DURABLE + TARGET_WORKFLOW", + consistency: Consistency.REGION_DURABLE, + returnStrategy: ReturnStrategy.TARGET_WORKFLOW, + shouldHaveWorkflowFields: true, + shouldHaveTaskFields: false, + requiresRegionDurable: true, + }, + { + name: "REGION_DURABLE + BLOCKING_WORKFLOW", + consistency: Consistency.REGION_DURABLE, + returnStrategy: ReturnStrategy.BLOCKING_WORKFLOW, + shouldHaveWorkflowFields: true, + shouldHaveTaskFields: false, + requiresRegionDurable: true, + }, + { + name: "REGION_DURABLE + BLOCKING_TASK", + consistency: Consistency.REGION_DURABLE, + returnStrategy: ReturnStrategy.BLOCKING_TASK, + shouldHaveWorkflowFields: false, + shouldHaveTaskFields: true, + requiresRegionDurable: true, + }, + { + name: "REGION_DURABLE + BLOCKING_TASK_INPUT", consistency: Consistency.REGION_DURABLE, returnStrategy: ReturnStrategy.BLOCKING_TASK_INPUT, shouldHaveWorkflowFields: false, shouldHaveTaskFields: true, + requiresRegionDurable: true, }, ]; @@ -660,7 +713,10 @@ describe("WorkflowExecutor", () => { // Now replicate for all other combinations testCombinations.slice(1).forEach((testCase) => { - test(`Should execute complex workflow with ${testCase.name}`, async () => { + const testFn = testCase.requiresRegionDurable + ? testForRegionDurable + : test; + testFn(`Should execute complex workflow with ${testCase.name}`, async () => { console.log(`\n--- Testing ${testCase.name} ---`); // Execute workflow diff --git a/src/integration-tests/utils/customJestDescribe.ts b/src/integration-tests/utils/customJestDescribe.ts index 0cf353fc..7385fa5e 100644 --- a/src/integration-tests/utils/customJestDescribe.ts +++ b/src/integration-tests/utils/customJestDescribe.ts @@ -1,4 +1,4 @@ -import { describe } from "@jest/globals"; +import { describe, test } from "@jest/globals"; const orkesBackendVersion = Number(process.env.ORKES_BACKEND_VERSION); const isOss = (process.env.CONDUCTOR_SERVER_TYPE || "").toLowerCase() === "oss"; @@ -31,3 +31,25 @@ export const describeForOrkesOnlyV5 = // skip it there for now export const describeForOssSchedulerWip = !ossGated && orkesBackendVersion >= 4 ? describe : describe.skip; + +// Consistency.REGION_DURABLE requires the target server to have cross-region +// replication configured. Since orkes-conductor 5.5.0, a node without it rejects the +// start outright ("REGION_DURABLE consistency requested but region replication is not +// enabled/configured on this node") rather than silently downgrading to a plain local +// start — it refuses a guarantee it cannot provide instead of appearing to honour it. +// So these run only where the capability actually exists: set +// CONDUCTOR_REGION_DURABLE_ENABLED=true against such a target. +// +// Deliberately an explicit opt-in rather than catching the 500 and skipping: auto-skip +// would restore exactly the silent green that hid this for months (pre-5.5.0 the flag +// was accepted and ignored, so these cases were really exercising DURABLE), and would +// hide a genuine replication regression on a cluster where it is supposed to work. +// Mirrors the java-sdk gate of the same name (TaskClientTests, PR #167). +// +// Test-level rather than describe-level because the cases are generated from a +// combination table; it lives here so all suite gating stays in one module. +const regionDurableEnabled = ["1", "true"].includes( + (process.env.CONDUCTOR_REGION_DURABLE_ENABLED || "").toLowerCase() +); + +export const testForRegionDurable = regionDurableEnabled ? test : test.skip;