Repository navigation
Conversation
…kground Agent, AsyncJob and AsyncJob status take most of the deployed e2e test time and spend it waiting on queues, Lambda and the model. Each now runs in its own child process against the same server or stack while the other suites run, and e2e.test.ts awaits them at the end. The single deploy and destroy are unchanged. A child process per suite keeps the client-side state the suites change apart: the cookie jar (the Agent suite signs users in and out), the realtime client's pooled WebSocket (the Agent suite force-closes it) and fetch spies. Server-side state they shared is split: - The Agent API-key resolver test gets its own secret AppSetting instead of writing the one the AppSetting suite writes and restores. - AuthBasic's "exactly one record" test counts the records holding its own user instead of every code key, and no longer purges all codes first; the purge test purges only its own user. A full purge could delete a code the Agent suite is waiting to read. Also drop the second databaseTests registration, which ran the Database suite twice (11 tests).
CI sets NODE_OPTIONS=--test-reporter=spec, which the child inherits; a second --test-reporter on the command line makes node:test exit on startup. Add the flag only when no reporter is set.
- AuthBasic "exactly one record": assert every code record is keyed by the username it holds, so a shared slot fails whoever delivered last, instead of filtering records by this test's username. - The start test sweeps all delivered codes once before the suites overlap (local .bb-data no longer grows run over run); the scoped purge deletes the user's keys directly instead of scanning. - done() starts its suite if the start test was filtered out; children are killed when the parent exits; inspector flags are not forwarded.
E2E: sandbox, production
|
The full (no-argument) purge of delivered codes was asserted by the AuthBasic purge test before that test moved to a per-user purge. Test it again, with the same assertions, before the background suites start, where a full purge cannot race their sign-ups.
…livery Sign a user up and wait for its code instead of writing a record by hand, and assert authGetLastCode is null after the purge, as the AuthBasic purge test on main did. E2E: sandbox, production
2ffcdd4 to
5b74d0b
Compare
osama-rizk
left a comment
osama-rizk
left a comment
There was a problem hiding this comment.
This PR runs the Agent, AsyncJob and AsyncJob status e2e suites in child processes alongside the rest of the comprehensive suite. Verdict: safe to merge once it is out of draft and the trailer is gone. Failures propagate, no test is lost or weakened, and the saving is real. I have one should-fix: a guard test for the exit-code path, which is now the only gate for 53 tests.
Speed / flakiness: The saving is real; I checked it against the job logs (test phase measured from Server ready to Destroying ... stack). Sandbox test phase went from 9.6 / 7.9 min (main runs 39812188339 / 39821845972) to 2.9 / 2.9 / 3.0 / 3.2. Production went from 9.0 / 6.9 to 4.1 / 2.8 / 3.3 / 3.4. E2E Production is the critical path for required checks, so the saving does land on required-green. Production job time minus deploy fell from 22.9 / 24.0 min to 19.1 / 17.8 / 17.6 / 16.0. However, Production deploy varies from 20 to 29 min, which hides the saving in any single run. Run start to last required check was 58.8 / 63.1 min here vs 62.6 / 83.2 on main; that comparison includes queueing and has too few samples to be more than indicative. The Agent child (about 173-180 s) is now the floor of the test phase. On flakiness, the change exposed the AuthOIDC stub IdP's dependence on instance routing (2 Sandbox failures). Running AuthOIDC first mitigates it, and both runs since then are green. The 8.1 / 8.8 min p50 baseline comes from your 25-run sample; I did not re-measure it.
E2E integrity: No test is removed or weakened. Test counts from the logs: main runs 395 parent tests, which is 384 unique plus the 11 duplicate Database tests. This PR runs 336 parent tests (331 plus the start test, the full-purge test and 3 wrappers) and 53 child tests (agent 39, async-job 9, async-job-status 5), which is the same 384. The per-user purge now asserts deleted === 1. The full purge keeps main's assertions and adds "no record left". The invariant form of the code-record test still fails on an unkeyed or shared slot.
I simulated the failure modes locally with this PR's background-suites.ts. A failing child test, an import crash, a leaked handle and a hang past the budget each fail the job (exit 1). Destroy waits for all three children, and a failed deploy never spawns them. The 20 min per-suite SIGKILL fits inside the 60 / 90 min job timeouts with destroy still running, which is better than main, where a hang ran to the job timeout and skipped destroy.
Child output is line-prefixed and nothing is lost. On shared state: the background suites use AuthBasic only (no Cognito), with unique usernames and per-user lockout. Their KV keys (agent-test, agent-whoami, job:, validated:, tracked:) are disjoint from the parent's, and every other kvScan / tableScan caller filters by prefix or pk. The AppSetting is split per suite, and the app sets no reserved concurrency. All 6 required checks ran and are green on the head.
What's good
- Child processes, not node:test
concurrency, and the header explains why (cookie jar, pooled WebSocket, fetch spies). It also states the rule a future suite must meet to move into the background. - Every failure class I tried reaches the job's exit code, and destroy is ordered after all children.
- Bounded hangs, children killed on parent exit, inspector flags stripped, no duplicate
--test-reporter. - The duplicate
databaseTestsregistration is fixed, the purge assertions are stricter, and the AuthOIDC root cause is written up honestly.
What needs work: 0 blocking · 1 should-fix · 1 minor · 1 nit (details inline).
Open questions
- There are now 2 fully green runs on the fixed tree (ae3df61, ce5fdb0). Do you plan more runs before merge?
- Can the stub IdP fix (self-contained refresh tokens in
@aws-blocks/bb-auth-oidc) be tracked as an issue, so the "AuthOIDC must run first" constraint can be dropped later?
Cross-cutting
- The head commit still carries
E2E: sandbox, production, which is why the non-required E2E jobs fail. Push a commit without the trailer and take the PR out of draft before merge. - #948 conflicts in
e2e.test.tsandaws-blocks/index.tsand addsmulti-compute.test.ts. Whichever lands second has to place that suite according to the rule in thebackground-suites.tsheader. - #939 changes Agent
stream()ownership checks, and those tests now run in the child. It merges clean, but run E2E Sandbox / Production on whichever lands second. No strict merge order is needed otherwise.
| child.on('close', (code, signal) => { | ||
| live.delete(child); | ||
| clearTimeout(timer); | ||
| if (code === 0) resolve(); |
There was a problem hiding this comment.
Non-blocking (should fix): this code === 0 check is now the only thing that turns 53 Agent/AsyncJob tests into a failing E2E Sandbox / E2E Production job, and nothing tests it.
I ran this file locally (only entry pointed at a fake child, and the budget made configurable) under tsx -C browser with CI's NODE_OPTIONS=--test-reporter=spec. Every failure class propagates correctly: a failing child test, an import-time crash, a leaked handle hitting the 15 s backstop, and a hang past the budget all fail the matching Background suite - <name> test and the job exits 1. test.after (the destroy) also starts only after all three settle. But when I mutated this line to always resolve(), a child reporting [async-job] ℹ fail 1 produced ✔ Background suite - async-job and exit 0. None of the CI runs exercised this path either: all 53 background tests passed in every run, so the red path was never observed in CI.
Since the regression mode is "required check green over a failing suite", a small guard is worth it.
Fix: let backgroundSuites() take an optional entry (e.g. backgroundSuites(names, { entry }), defaulting to run-background-suite.ts) and add a tsx --test unit test with a tiny fake child that passes, fails one test, and exits non-zero at import. Assert that done() resolves for the first and rejects for the other two. Run it next to test:vendorize / test:iam-boundary in test:e2e:local.
There was a problem hiding this comment.
Added in 031afe85: backgroundSuites() takes an entry, and test/background-suites.test.ts (run in the root test:e2e:local) uses a fake child that passes, fails one test, crashes at import or finishes late. The always-resolve() mutant now fails 4 of its tests.
| live.delete(child); | ||
| clearTimeout(timer); | ||
| if (code === 0) resolve(); | ||
| else reject(new Error(`Background suite "${name}" failed (exit ${code ?? signal}). Its results are in the ${prefix.trim()} lines above.`)); |
There was a problem hiding this comment.
Non-blocking (minor): when a background suite fails, the parent's ✖ failing tests: block only shows Background suite - agent ... failed (exit 1). Its results are in the [agent] lines above. The real failing test names are in the [agent] lines, interleaved with two other children and the parent's own spec output across ~3 minutes of log. The parent's ℹ tests count also drops from 395 to 336, because the 53 child tests are only counted in the children's own summaries. So nothing is hidden, but triage gets slower and any tooling that reads the parent summary sees fewer tests.
Fix: keep the last N lines of each child that start with ✖ (or the block after its failing tests: line) in the stdout line handler at L105, and append them to this error message. The parent's failing-tests summary then names the actual tests, e.g. Background suite "agent" failed (exit 1): ✖ resume after approval completes the agent turn ....
| result = run(name); | ||
| // Observed by done(); keep an early failure from surfacing as an | ||
| // unhandled rejection before the suite's own test awaits it. | ||
| result.catch(() => {}); |
There was a problem hiding this comment.
Non-blocking (nit): done() covers a name filter that skips the start test, but not the reverse. With a pattern that matches the start test and not the Background suite - * tests (for example --test-name-pattern "Background suites", note the plural), start() spawns all three children and nobody awaits them. Their failures are swallowed here, test.after destroys the sandbox/production stack while they are still calling it, and the exit handler then SIGKILLs them. This only affects filtered local runs, not CI.
Fix: expose something like settle(): Promise<void> (an allSettled over the started results) and await background.settle() at the top of test.after in e2e.test.ts, before the destroy. Alternatively, have start() do nothing unless at least one done() test is registered to run.
There was a problem hiding this comment.
Fixed in 031afe85: settle() is awaited at the top of test.after. Reproducing with --test-name-pattern="Background suites" showed a second problem: the children inherited the filter and ran 0 tests. Since b61d40b4, 73c28389 and c6c2290e they don't get --test-name-pattern/--test-only from the flags or NODE_OPTIONS.
…est the runner - backgroundSuites() gains settle(), and e2e.test.ts awaits it at the top of test.after. A --test-name-pattern that runs the start test but not the `Background suite - *` tests used to stop the server or destroy the stack while the children were still running. - A failed suite's error now lists the child's failing tests (from the spec reporter's `failing tests:` block), so the parent's own summary names them. - backgroundSuites() takes an optional `entry`, and test/background-suites.test.ts runs it against a fake child that passes, fails one test, crashes at import, or finishes late. It runs in the root test:e2e:local after test:iam-boundary. The children no longer inherit NODE_TEST_CONTEXT, so the runner works under `node --test` too. - The comments on the AuthOIDC-first rule link the stub IdP issue (#988).
…nk their output in tests - Children no longer inherit --test-name-pattern, --test-skip-pattern or --test-only, from the parent's flags or NODE_OPTIONS. With the parent's filter a child ran none of its tests and passed, so a filtered run reported a suite green that had not run. - The failing-test scrape strips ANSI codes, so colored output (FORCE_COLOR) still names the failing tests. - backgroundSuites() keeps the BackgroundSuiteName check for the real suites; other names need an explicit entry (the runner's tests). - One process exit handler for all children, not one per call. - An `output` option takes the children's prefixed lines. The runner's tests collect them, so a fake failing child's output stays out of the test runner's own stdout.
…kip-pattern on NODE_OPTIONS is split on whitespace outside double quotes, so a quoted test filter with a space is removed whole instead of leaving a broken fragment that stopped every child at startup. --test-skip-pattern now reaches the children: it only skips the matching tests there, as its author wants.
… spaces only
A filter wrapped whole in quotes ("--test-name-pattern=a b") is now
recognised and kept from the children, NODE_OPTIONS is split on spaces
(as node does, not on tabs), and the reporter check reads NODE_OPTIONS
the same way. The skip-pattern test also checks that the child's other
test still ran.
…ground children Like --inspect in the parent's own flags, one set in NODE_OPTIONS would make three children fight over the debug port.
📖 Glossary & notes — scoring, indicators, per-metric thresholds (click to expand)
Results — PR vs
|
| Task | Template | Tests | Judge | Cost | Turns | Score | Stop reason |
|---|---|---|---|---|---|---|---|
| async-word-counter | bare | — | — | ✨ $3.02 (-$3.98) | ✨ 19 (-17) | — | endTurn |
| auth-notes | demo | — | — | ✅ $6.32 (-$0.43) | ✅ 32 (0) | — | endTurn |
| cognito-profile | auth-cognito | — | — | ✨ $6.62 (-$7.61) | ✨ 27 (-22) | 🆕 0 (new) | error |
| collab-presence-board | default | — | — | ✨ $2.47 (-$0.74) | ✅ 18 (-3) | — | endTurn |
| email-digest | demo | — | — | ✅ $4.88 (-$0.52) | ✅ 29 (+3) | — | endTurn |
| file-gallery | bare | — | — | ✨ $2.64 (-$0.38) | ✅ 19 (+1) | — | endTurn |
| kb-chat-agent | demo | — | — | ✨ $2.03 (-$21.61) | ✨ 10 (-43) | 🆕 0 (new) | error |
| observability-api | backend | — | — | ✅ $5.42 (+$0.33) | ✅ 26 (-3) | — | endTurn |
| oidc-dsql-notes | react | — | — | ✅ $8.1 (-$0.83) | ✅ 34 (-1) | — | endTurn |
| sql-kb-catalog | nextjs | — | — | ✅ $6.13 (+$0.48) | ✅ 34 (+2) | — | endTurn |
Merge verdict
❌ This bench run BLOCKS the PR.
- ❌ Absolute floor — mean composite 0.0 is below the floor 65.
- ⚪ Regression vs
main— BENCH_MAX_REGRESSION=10 set, but no main baseline to diff — regression gate skipped.
The Δ vs base ball above is a fixed ±5-pt display band; the regression gate uses BENCH_MAX_REGRESSION. The ball is cosmetic — the gate threshold is what blocks.
Caveats & exclusions
- 🧰 Excluded (harness_error, not scored) — 8: 8× isolation_unavailable. Cells:
async-word-counter/bare,auth-notes/demo,collab-presence-board/default,email-digest/demo,file-gallery/bare,observability-api/backend,oidc-dsql-notes/react,sql-kb-catalog/nextjs
What
The E2E Sandbox / Production / Local suite (
test-apps/comprehensive/test/e2e.test.ts) ran every suite serially against its one deployed stack. Agent, AsyncJob and AsyncJob status take about 5 of the 7-8 minutes of deployed test time, nearly all of it polling queues, Lambda and the model.They now run each in its own child process (
test/run-background-suite.ts, spawned bytest/background-suites.tswith the parent's node binary and tsx loader flags) against the same server or stack, started right after the sanity check and awaited by one test per suite at the end of the file. Each suite still runs its own tests one at a time. The single deploy intest.beforeand destroy intest.afterare unchanged and untouched.Also removes the second
databaseTestsregistration: the Database suite was registered twice and ran its 11 tests twice (423 -> 412 tests locally). The same 11 tests still run once, so no assertion is lost.Coverage: no test or assertion is removed or weakened. Every suite still runs exactly once; the AuthBasic code-record assertions are stricter than before (see below).
Isolation audit
Why child processes rather than node:test
concurrency: the parent holds state the suites change. The Agent suite signs users in and out (shared cookie jar onglobal.fetch) and force-closes the realtime client's pooled WebSocket to test reconnects; the sanity check spies onfetch. A child gets its own copy of each.Server-side state the background suites shared with the parent, and the fix:
secret-setting: the Agent API-key resolver test wrote and read it while the AppSetting suite writes and restores it. The resolver test now has its own secret AppSetting (agent-api-key-setting, one more SSM parameter in the test stack).__last-code:*in the shared KV store): AuthBasic's "exactly one record" test purged every code first and then asserted the only code key was its own; the purge test purged every code. The Agent suite signs users up concurrently, so its records would fail the assertion and a purge could delete a code it is waiting to read. Now the full (no-argument) purge has its own test, run before the background suites start, with main's assertions (real sign-up and code delivery,deleted >= 1,authGetLastCodenull afterwards, bystander kept) plus "no code record left"; the "exactly one record" test asserts an invariant over every record (each keyed by the username it holds, so a shared slot still fails) plus its own key exactly once; the per-user purge test asserts exactly 1 record purged (the purge helper takes an optionalusername).Checked and clean: no other shared BB instance, fixed key or unfiltered scan between the three suites and the rest (agent KV keys are read only by the agent suite; other
kvScancallers filter by prefix); AsyncJob and AsyncJob status use disjointjobResultskeys; AuthBasic lockout is per username; mock persistence does synchronous read-modify-write in the one dev-server process; the realtime mock server keeps per-socket state and the agent uses its own channels; logging capture is synchronous; Lambda account concurrency is 1000 in all pool accounts.Process handling
--test-reporter=speconly when no reporter is set (CI sets one inNODE_OPTIONS; a second one makes node:test exit on startup).done()starts its suite if a name filter skipped the start test.[agent]/[async-job]/[async-job-status]prefix; each child's pass/fail reaches the job through itsBackground suite - <name>test.Evidence
npm run test:local, CI'sNODE_OPTIONS): 133 s -> 56-94 s. Local runs need their own network namespace when other dev servers share the host (the dev server reclaims :3000 from any other process).npm run build && npm run lint:deps && npm test && npm run test:e2e:localandnpm run lint: all exit 0.All 53 background tests passed in every job. The Agent suite (~170-185 s) is now the critical path of the test phase. E2E Production remains the job critical path; its wall clock is dominated by deploy (21-29 min) and destroy (~12 min), which vary by more than the ~5 min saved, so the test-phase saving is the stable signal. Time to both required E2E checks green: 58 min in run 3 vs 63 / 84 min in the two baseline runs (runner queueing included).
POST /aws-blocks/auth/extras/refresh returns new tokens for a valid refresh token(401TokenExpiredException). This is caused by this change exposing a latent defect, not by AWS: the AuthOIDC stub IdP keeps each issued refresh token in memory on the Lambda instance that served its/tokencall. That call is made over HTTP by the instance serving/extras/exchange, so it always runs on a different instance; a later refresh only succeeds if its own/tokencall is routed back to the issuing instance. Serial traffic on main happens to do that; the background suites' extra load breaks it. Fix in this PR: the AuthOIDC suite runs before the background suites start (as serial as on main; coverage unchanged). The stub IdP itself (self-contained refresh tokens, deterministic signing key) needs a separate fix in@aws-blocks/bb-auth-oidc, tracked in AuthOIDC stub IdP: make refresh tokens self-contained so they work across Lambda instances #988; until then the test stays as exposed as it is on main. Run 3, after the fix: both jobs green, 0 failures (336 parent + 53 background tests each). A run on the current head (comment-only change) is in progress.Test-only change (test-apps are not published): no changeset.
The timing runs above used
E2E: sandbox, production. The head no longer carries a trailer, so PR Checks runs the full E2E set.Review follow-ups (2026-10-23)
settle():test.afterwaits for every started background child before it stops the server or destroys the stack. A filtered local run (--test-name-pattern "Background suites") used to stop the server while the children were still running. The children also no longer inherit--test-name-pattern/--test-only(from the flags or NODE_OPTIONS), which made them run 0 tests and pass (031afe85, b61d40b4, 73c28389, c6c2290e).test/background-suites.test.ts(in the roottest:e2e:local) runs the runner against a fake child that passes, fails one test, crashes at import or finishes late. The always-resolve()mutant you described fails 4 of its tests (031afe85).npm run test:e2e:localpasses (363 tests, 0 failed, plus vendorize, iam-boundary and the new runner test 12/12), and build, lint and lint:deps pass. Unit tests fail only in@aws-blocks/core(telemetry tests that time out under host load, and one process-group test that fails only in a container); this PR changes no package.