[matboros]

Closes #2101

Reason for this change

The parallel integration-test scheduler assigned each test to a region from the --parallel-regions pool round-robin and ignored the per-test regions allowlist declared in integ.json. Region-constrained tests could be deployed in a disallowed region and fail (see #2101 for a real inf2 / ap-northeast-3 failure).

Description of changes

runIntegrationTestsInParallel (lib/workers/integ-test-worker.ts) now:

• reads each test's declared regions from its integ manifest (intersection across test cases when more than one is constrained);
• schedules a test only into pool regions within its allowlist;
• takes a region-constrained test before an unconstrained one when a worker picks up work, so a constrained test is not starved out of its only eligible region by an unconstrained one;
• reports a test whose allowlist does not intersect the pool as a TEST_ERROR with an actionable message, instead of running it in a disallowed region or silently skipping it.

Tests that declare no regions are unaffected and may run in any pool region.

Behavior change

A region-constrained test whose allowed regions are not in the active --parallel-regions pool now fails fast (with a message telling the user to add one of its regions to --parallel-regions) instead of running — incorrectly — in a region it excluded. This is the intended meaning of regions, but is a visible change for anyone who previously relied on the allowlist being ignored.

Description of how you validated changes

Added unit tests in test/workers/integ-worker.test.ts:

• a region-restricted test is scheduled only in its allowed pool region;
• a test whose allowed regions are all outside the pool is reported, not run.

Full package build (projen build) passes: compile, all unit tests, eslint, and package/bundle.

Checklist

• My code adheres to the CONTRIBUTING GUIDE

---

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

[codecov-commenter]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 92.08%. Comparing base (d08ccc1) to head (ae80728).
⚠️ Report is 5 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #2102   +/-   ##
=======================================
  Coverage   92.08%   92.08%           
=======================================
  Files          83       83           
  Lines       13246    13246           
  Branches     1886     1886           
=======================================
  Hits        12197    12197           
  Misses       1011     1011           
  Partials       38       38           

Flag	Coverage Δ
suite.unit	92.08% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[maintainer-for-aws]

Automated review

A maintainer will still review this — treat the notes below as a starting point.

📌 Commit ae80728

This PR fixes a scheduling bug in @aws-cdk/integ-runner: when running integration tests in parallel across a region pool (--parallel-regions), the parallel scheduler ignored each test's declared per-test regions allowlist, so a region-constrained test could be deployed into a region it explicitly excluded. The change adds a shared intersectTestCaseRegions helper, teaches the scheduler to assign each test only to the intersection of its allowlist with the pool (preferring region-eligible tests and pruning tests with no eligible environment), and adds a stale-snapshot safety net that re-validates the assigned region after synth and reschedules via a new WRONG_REGION diagnostic rather than deploying into a disallowed region. New unit tests, scheduler tests, and snapshot fixtures back the behavior.

The change is well-scoped, additive to the diagnostic model, and backward-compatible for tests that declare no regions. The one user-visible behavior change — a region-constrained test whose allowlist does not intersect the pool now fails fast with an actionable error instead of silently running in a disallowed region — is the intended fix, is called out in the PR body, and is confined to the internal dev tool. No issues found on this change.

🔴 0 blocking · 🟡 0 recommended · ⚪ 0 optional

---

Generated automatically. React 👍 or 👎 to tell us whether this review helped, so we can improve these reviews.

[maintainer-for-aws]

See the review summary comment for the overview; the notes below are inline.

[maintainer-for-aws]

🟡 Recommended — The try/catch around IntegManifestReader.fromPath(...) swallows every read/parse failure and returns undefined (unconstrained), so the test is then schedulable into any pool region. IntegManifestReader distinguishes two cases: a missing manifest (a legacy or brand-new test, the case the comment intends to preserve) and a present-but-invalid manifest. Collapsing them means a test whose integ.json is corrupt silently loses its regions restriction and can deploy to a region it meant to exclude — the exact failure this change exists to prevent, reintroduced for the malformed-manifest case (AGENTS.md#error-handling).

Suggested change: Catch only the missing-manifest error (e.g. NoManifestError) and return undefined for it, letting a manifest-load/parse error propagate — or report it as a TEST_ERROR for that test — so a broken manifest is not silently treated as "no region restriction."

[matboros]

Thanks for the review. Addressed in 90ba3c8:

• 🟡 Manifest read fails open for all errors — declaredRegions now catches only NoManifestError (a missing manifest → legacy/brand-new test, unconstrained as intended) and lets a present-but-invalid manifest propagate. The scheduler reports such a test as a TEST_ERROR for that test instead of silently dropping its regions restriction, so a corrupt integ.json can no longer deploy to an excluded region. Added a unit test + fixture (xxxxx.invalid-regions-manifest).
• Disjoint multi-test-case intersection — now reported with a dedicated message ("test cases declare disjoint regions allowlists, so no single region can run them all…") instead of an empty-bracket list. Added a unit test + fixture (xxxxx.disjoint-regions).

Full package build (compile + unit tests + eslint) passes.

[mrgrain]

what's wrong with Array.prototype.findLastIndex()?

[matboros]

Agreed it would read cleaner. The catch is the lib target: findLastIndex is an ES2023 array method, and this package compiles against es2022 (via the shared toolkitLibTsCompilerOptions), so it doesn't typecheck here — tsc returns TS2550 suggesting es2023. I kept a small local helper to avoid a repo-wide lib bump.

Happy to switch to the stdlib method if you'd prefer — that'd mean adding es2023.array to this package's lib in .projenrc.ts (it's currently shared with toolkit-lib, so I'd give integ-runner its own compiler options). Let me know which you'd rather have.

[mrgrain]

please make this error more human friendly. even I needed to think 3 times what that means.

[matboros]

Reworded in eee93b7 to:

This test's test cases each allow a different set of regions with no region in common, so there is no single region that can run all of them. Update the test so its test cases share at least one allowed region.

[mrgrain]

Nit: there's probably not much to improve here, but still feel this could be more human friendly and less technical

[matboros]

Reworded in eee93b7 to:

Could not read this test's snapshot (integ.json) to find out which regions it supports: <underlying error>

[mrgrain]

Nit: there's probably not much to improve here, but still feel this could be more human friendly and less technical

[matboros]

Reworded in eee93b7 to:

This test can only run in these regions: <allowed>. None of them are available in this run (<pool>). Add one of the test's regions to --parallel-regions to run it.

[mrgrain]

I think this has a correctness bug: If I understand this correctly, the code is reading the regions from the committed snapshot, not the latest test case code. By definition the snapshot might be stale and thus attempt to schedule the test in an incorrect region. Because the test case code isn't consulted at all, it will attempt this incorrect deployment and then probably fail with an obscure error.

I think this we might be better of piggy-backing on messages going back and forth from the scheduler and the worker:

• the scheduler schedules the test for a region
• the test checks the region it has been scheduled in is actually supported
  • if YES, proceed
  • if NO, send a message back to the scheduler with the supported regions
• the scheduler reacts to the message coming back and re-schedules the tests for a supported region (or fails the test if none available)

Of course this will introduce an extra round-trip for very region-limited test cases. We can decide if reading the snapshotted manifest is a good approximation for the regions likely to be allowed. The scheduler would read from the snapshot and speculatively pick one of those regions. Though the same can then happen in reverse: A snapshotted region is not actually allowed and then we get the extra round-trip. Not sure if this optimisation is worth it then.

[matboros]

Good catch — you're right that the snapshot can be stale relative to the test source, and the authoritative regions are only known after synth. I implemented the round-trip you described so you can see the shape and weigh in (ae80728):

• Scheduler still does a speculative first pick from the snapshot's integ.json — so for the common case (snapshot in sync) there's no extra round-trip.
• Worker validates after synth, before deploy: in runIntegTestCase, right after actual.synthForDeployment(), if the assigned region isn't in the freshly-synthesized app's regions, the runner throws WrongRegionError. Nothing is deployed in the wrong region.
• Report back + reschedule: the worker emits a WRONG_REGION diagnostic carrying the fresh supportedRegions; the scheduler updates that test's eligible regions from the authoritative data and reschedules it — reusing the exact machinery that already handles NOT_BOOTSTRAPPED (diagnostic → on handler → retry queue). It converges because the retry only goes to a now-known-supported region, so the worker won't re-throw.
• The per-test-case intersection is now a shared intersectTestCaseRegions helper used by both the snapshot read and the fresh-synth check, so the two paths agree on semantics.

So the extra round-trip happens only when the snapshot and the synthesized app disagree — which is exactly the stale case you flagged. This matches the "snapshot as a good approximation, reconcile on mismatch" option from your comment.

Two things I'd like your read on:

1. Is this the shape you had in mind, or would you rather the scheduler not speculate at all (e.g. a cheaper pre-pass)? I kept speculation because synthesizing every test up front to learn its regions would serialize what is meant to be parallel.
2. The worker throws before deploy, so there's no wasted deployment — but a region-limited test that the snapshot placed wrong does pay one synth + reschedule. Given how rarely snapshots are stale for regions, that seemed an acceptable cost. Agree?

Tests added: the intersection helper, the worker emitting WRONG_REGION (direct and wrapped-as-cause), and the scheduler rescheduling a stale-snapshot test (snapshot says us-east-1, fresh synth supports only us-east-2) into its supported region.

[mrgrain]

Nice, this is pretty much what I expected to happen.

1+2) That's the same issue to me. It's your call to make. I don't know what's more likely. Can you do some back-of-the-napkin maths on what the actual cost of each option is? I'm quite sure the proposed solution is better though. So in principle I agree, ideally we have more due diligence.

My Agent also found one other thing that seems like a legit find:

One thing still needs to change: the up-front checks in runIntegrationTestsInParallel still fail tests based on the committed snapshot. When the snapshot's regions don't overlap the pool, its test cases have no region in common, or integ.json can't be parsed, the test gets a final TEST_ERROR before anything is synthesized. That's the same stale-data problem in a different place. For example, if a PR changes regions: ['ap-northeast-3'] to ['us-east-1'] and we run with --parallel-regions us-east-1, the test fails with "None of them are available in this run", even though the new code would run fine.

Since the worker now has the authoritative answer, I'd treat the snapshot only as a scheduling hint. In those three cases, queue the test as unconstrained and let WRONG_REGION sort it out. If the fresh regions really don't fit the pool, the test is still reported, just one synth later. Because the runner check becomes the source of truth, it also needs a test of its own. Right now the worker tests mock runIntegTestCase and the scheduler test uses the mock worker, so nothing checks that IntegTestRunner throws WrongRegionError before it deploys.