Run the full-stack E2E harness in CI, and keep the process alive when a background integration fails - #4753
Run the full-stack E2E harness in CI, and keep the process alive when a background integration fails#4753TaprootFreak wants to merge 17 commits into
Conversation
The uncaughtException handler already survives Spark SDK failures identified by "Channel has been shut down", because SparkClient.call() treats that as an expected operating condition and reinitializes the wallet on it. The same failure also surfaces as an unhandled promise rejection (e.g. SparkClient boot without network connectivity), which had no matching handler and terminated the process. Extract the toleration rule into isToleratedProcessError() and apply it to both channels.
… process PolygonClient and ArbitrumClient both kick off their L2 network initialization in the constructor as a fire-and-forget call with no error handling. A rejection there becomes an unhandled promise rejection, which the process now correctly treats as fatal since the previous commit added a matching unhandledRejection handler - exposing that these two initializations could take the whole process down on a failure that should only affect the L2 bridge (e.g. an unsupported network/version combination, or a temporarily unreachable provider). Log and swallow the error instead, since these initializations are meant to run concurrently in the background.
|
Reviewed alongside the frontend counterpart, DFXswiss/services#1288. Two findings on this side, both fixed:
On the two behavioural changes: The E2E job itself was verified against the real harness by dispatching it manually with |
|
Ready for review with one check red on purpose.
The check clears itself once the frontend side lands — nothing here needs changing for it. Everything else is green, including the coverage ratchet and all three test shards. The job itself was verified against the real harness by dispatching it manually with No longer applies. The check is green: the job bootstraps the harness from the frontend pull request until it reaches that repository's default branch, so neither side has to merge first. |
|
An independent second-opinion review on a different model family turned up one thing three earlier passes had missed, now fixed in In the rejection handler, The consequence was the opposite of what the handler is for. A rejection the policy would have tolerated — one carrying a hostile prototype trap — never reached the policy at all, and the process exited. "A handler that throws is worse than none" is exactly the failure this change was meant to remove, and it had been reintroduced one line above it. The same review also pointed out that the PR description overstated the Arbitrum half: |
The harness lives in the companion frontend repository and is not on its default branch yet, so this job failed on ordering rather than on anything about the API. It now falls back to the pull request head that carries the harness, warns while it does so, and disables itself the moment the harness reaches the default branch. An explicitly supplied services_ref keeps failing hard when the harness is not on it: that is a caller mistake, not a bootstrap case.
initL2Network() handles its own failures today, so the catch never fires. Without a note, the next reader has to re-derive that and may well delete it.
refs/pull/1288/head stays fetchable forever, so a later rename of the harness scripts on the default branch would have quietly re-armed the fallback and tested against a years-old frontend. The step now refuses to bootstrap once that pull request is no longer open, and says which block to delete.
Once a rejection is judged tolerable, the process must survive it. The log call that follows reads properties off the rejection value, so a hostile getter could throw there and take the process down anyway — after the policy had already decided to keep it. Logging now falls back to the message alone. Also from the same review: read gh's answer without its stderr mixed in, so a warning on an open pull request cannot make it look closed.
The fallback log call was itself unguarded, so a logger that fails for its own reasons could still throw out of a handler that had already decided to keep the process alive.
up.sh records the values it resolved there; a compose run that resolves them differently recreates the API container in the middle of the suite.
The handlers decide whether the process survives, and nothing covered that decision: they sat in the module body of main.ts, where reaching them means starting the whole application. They now live behind two functions that take the logger and the exit action, so the guarantee itself is asserted — tolerated errors leave the process alone, everything else exits, a hostile rejection value throws nowhere, and a logger that always throws changes neither outcome.
The companion repository renamed the file its stack script generates, so that it stops overwriting the one a developer keeps.
The rejection path already wrapped values that are not Errors before logging them; the exception path cast instead, on the assumption that Node only ever delivers an Error. is legal, and the assumption cost the log line its content. Both paths now say which of the two they are reporting, and the tests read what was logged rather than only that nothing threw.
Both handlers share the value-to-text fallback now, so an exception whose value cannot be stringified was reported as an unstringifiable rejection. The wording is neutral, a test reads the message rather than only checking that nothing threw, and the rejection test moved into the block it belongs to.
|
Review continued after this was first marked ready, and the later passes changed the shape of the What came out of it:
One suggestion was not taken: collecting imports one per line, as the contributing guide describes. Current state: all checks green, including the full-stack job, which builds the API image from this |
|
@mara-steiner please check |
There was a problem hiding this comment.
Reviewed the full diff against the merge base and ran the API locally. The extraction of the two process-level handlers, the fallback chain in safeLogError, and the artifact-before-teardown ordering in the new workflow are all solid, and the local run is clean. Two findings are worth addressing before this merges. Most relevant first.
Findings
[Major] The two new .catch() callbacks are not protected the way the rest of the PR is — src/integration/blockchain/polygon/polygon-client.ts:36-38, src/integration/blockchain/arbitrum/arbitrum-client.ts:42
Both callbacks call this.logger.error(...) directly. DfxLogger.error() calls format(), which reads error?.stack unguarded (src/shared/services/dfx-logger.ts:87), and recordOnSpan(), which passes the value to span.recordException(error) (:74-82). If the rejection value has a throwing stack getter, the callback itself throws — the promise returned by .catch() then rejects, it is discarded via void, and the resulting unhandled rejection reaches the new global handler, which does not tolerate it and exits.
That is precisely the failure mode safe-log.ts was added to prevent, and these two call sites are the only places in the PR that do not use it. safeLogError() is already in the diff; using it here would close the gap.
[Major] A permanently failed bridge init is now indistinguishable from "not yet complete" — src/integration/blockchain/polygon/polygon-client.ts:36-38 with :103-108
With the .catch() in place, a failed posClient.init() leaves no readiness or error state on the client. checkL2BridgeCompletion() swallows every error from isDeposited() and returns false, so a client whose init never succeeded reports the same thing as a deposit that is simply still pending — and gets polled indefinitely instead of failing visibly. checkL1BridgeCompletion() (:112-134) has the same shape.
Crashing the process was never the better option, so this is not an argument against the .catch(). But the change converts a loud failure into a silent, permanent one, and that trade deserves an explicit readiness flag (or a rejected init promise the callers consult) rather than being left implicit.
[Minor] The tolerance policy is broader than the failure it documents — src/shared/utils/process-error-policy.ts:15
constructorName.includes('Spark') matches any class whose name contains "Spark", not just the transport failure the doc comment describes. For uncaughtException this was already the behaviour, so it is not a regression — but this PR applies the same policy to unhandledRejection as well, which widens its reach. A genuine boot-time failure such as a hypothetical SparkConfigError would now keep the process alive in a half-initialised state, which is exactly the outcome the "unrelated process errors are not tolerated" sentence promises to avoid. Consider narrowing to the message match alone, or to an allowlist of concrete constructor names.
[Minor] The stated reason for the new rejection handler does not hold — PR description
The description says the Spark error "also arrives as an unhandled promise rejection ... and since Node 15 that terminates the process". With an uncaughtException listener already registered — which main.ts had — that is not what happens. In the default throw mode, Node routes an unhandled rejection to that existing listener with origin === 'unhandledRejection', and the process survives if the handler does not exit. Verified on Node v24.19.0 with a handler that mirrors the previous main.ts shape: the handler is invoked and the process stays up. Nothing in the repo overrides this — Dockerfile:56 and start:prod set no --unhandled-rejections flag, and there is no NODE_OPTIONS.
The explicit handler is still an improvement (clearer intent, its own logger context, and it no longer depends on Node's implicit routing), so this is not a request to revert. But the description frames it as fixing a crash that the previous code would not actually have suffered, and that framing will mislead whoever reads this commit later.
[Minor] Action versions are behind the ones this repo already uses — .github/workflows/e2e-stack.yml:163
actions/upload-artifact@v4, while the only other use in the repo (api-pr.yaml:181) is on @v7. For actions/checkout@v4 (lines 41, 46, 110) the repo is genuinely mixed — four existing workflows are on @v4 and five on @v5 — so that one is consistent with part of the codebase; the artifact action is the real outlier.
[Minor] Logger mocking deviates from the established test pattern — src/shared/utils/__tests__/process-error-handlers.spec.ts:12, src/shared/utils/__tests__/safe-log.spec.ts:7,23,37
{ error: errorFn } as unknown as DfxLogger uses a double cast where the rest of the repo consistently uses jest.spyOn(DfxLogger.prototype, 'error').mockImplementation(...) (e.g. blockchain-config-check.service.spec.ts, composite-storage.service.spec.ts, crypto-input.consumer.spec.ts). Given that injecting the logger is the whole point of the new ProcessErrorHandlerDeps seam, the cast is defensible here — but worth a look for consistency.
[Nit] Duplicated test matrices — src/shared/utils/__tests__/process-error-handlers.spec.ts:45-177 vs 178-300
The two describe blocks carry structurally identical cases that differ only in which handler is called. it.each/describe.each is widely used in this repo for exactly this; parametrising the shared cases would leave only the exception-vs-rejection normalisation to be tested separately.
Not a finding, but worth recording
The description states that with NETWORK=testnet the Polygon bridge initialisation "rejects outright, so this was reachable in ordinary local development". Running locally with NETWORK=testnet from .env.local.example, it did not reject — no Polygon L2 network initialization failed line appeared and the API booted cleanly. That is consistent with the rest of the description (the failure needs a host with no route out), but the "ordinary local development" claim only holds without network access. The .catch() is right regardless.
Local verification
Checked out at the PR head, against the merge base with develop:
npm run lint— exit 0npm run type-check(tsc --noEmit) — 0 errors- Full suite: 426 suites passed, 8168 tests passed, 8 suites / 239 tests skipped, exit 0
- The three new specs: 45/45 passing
- API boots and serves:
Nest application successfully started/Application ready ...,GET /v1/asset→ HTTP 200, process stable under observation
Recommendation
Requesting changes, on the first finding only: the two client .catch() callbacks should route through safeLogError(). It is a small change, it uses a helper this PR already adds, and without it the hostile-value case the PR sets out to survive can still take the process down through the very callbacks that were added to prevent that.
Everything else above is a judgement call I am happy to leave to you — the bridge readiness state in particular may well be worth a separate change rather than growing this one. The direction of the PR is right and the local run is clean.
2 participants
What this adds
A CI job that runs the full-stack end-to-end harness against this pull request's API build, and the
process-level fixes without which that job cannot start at all.
Why the process kept dying
The harness runs the API on a Docker network with no route to the internet, so that a test can never
reach a real bank, KYC provider or exchange. Under those conditions the process was terminating
during boot, before
app.listen(), for two separate reasons. Both are real outside the harness too:they need an unreachable third party, not a test environment.
Spark errors were tolerated only half the time.
main.tsalready carried anuncaughtExceptionhandler that deliberately keeps the process alive for Spark SDK failures,matching
SparkClient.call(), which treatsChannel has been shut downas an ordinary operatingcondition and reinitialises the wallet. The same error also arrives as an unhandled promise
rejection — from inside the SDK's own retry handling, on no chain the caller can reach — and since
Node 15 that terminates the process. The tolerance policy now lives in one place,
isToleratedProcessError, and both handlers use it. The rejection handler is deliberately narrow: arejection that does not match the policy still exits, exactly as before.
An L2 bridge initialisation could take the process with it.
PolygonClientstarts its bridgesetup in the constructor as a fire-and-forget call with no
.catch(). A failure there should costthe bridge, not the application. With
NETWORK=testnet— the value both.env.exampleand.env.local.examplerecommend — that initialisation rejects outright, so this was reachable inordinary local development. It now logs and continues.
ArbitrumClientgets the same.catch()for symmetry, but to be accurate about it: itsinitL2Network()already swallows failures internally, so the added handler is defence in depthagainst a future change there rather than a fix for an observed crash. The comment above it says so.
The decision the handlers make is now tested
Both handlers decide whether the process survives, and nothing covered that decision: they sat in
the module body of
main.ts, where reaching them means starting the whole application. They nowlive behind
handleUncaughtExceptionandhandleUnhandledRejection, which take the logger and theexit action, so the guarantee itself is asserted — tolerated errors leave the process alone,
everything else exits with code 1, a hostile rejection value throws nowhere, and a logger that
always throws changes neither outcome. Log messages and logger context names are unchanged.
Two details worth calling out, both found in review:
getter could throw there and take the process down after the policy had decided to keep it. A
handler that throws is worse than none, so logging now falls back to the message alone, and the
fallback is itself guarded.
throw 'x'is legal, so an uncaught exception is not necessarily anErroreither. Both pathsnormalise the value for the logger and report which of the two they are describing.
Measured after all of this: the API boots completely, in a network with no internet route, and stays
up — with the tolerated Spark rejections showing up in the log exactly as intended.
Worth a follow-up, not fixed here: there are 28 fire-and-forget calls of this shape in
src/. Onlythese two run during boot and could prevent startup, so only these two are in scope; the rest are
unaudited.
The CI job
Full-stack E2Echecks out this pull request, builds the API image from it, checks out the frontendrepository for the harness, brings the stack up, runs the suite, collects the Playwright artefacts,
and tears everything down. Teardown runs on
if: always(), and artefacts are copied out before it,since teardown removes the volumes holding them.
The job carries no
if:skip. A skipped check counts as passing on GitHub, which would quietlydefeat the gate.
Until the harness reaches the frontend repository's default branch, the job bootstraps it from the
pull request that introduces it, warns while it does so, and refuses to bootstrap once that pull
request is no longer open — so the fallback cannot quietly re-arm years later and test against a
frozen copy. Either repository can merge first.