Skip to content

dev: live-harness follow-ups (node-rejection constants, orchestrator tests, env-down pkill) #716

Description

@0xisk

Follow-ups found while building the integration-live harness (branch
test/integration-live-harness). None of these block that branch; they were
deliberately left out to keep it scoped. Design notes live in
docs/design/integration-live-harness*.md.

Filed from the Codebase improvement template.

1. Centralize the node-rejection strings

Node error strings are hardcoded in 8 files, in three different roles:

Role Where
Live assertion contracts/test/integration/specs/confidentialFungibleToken.spec.ts (exact phrase)
Live assertion contracts/src/token/test/ConfidentialFungibleToken.test.ts:1188 (loose /block limits|exhaust the block/i)
Retry logic contracts/test-utils/harness/LiveSimulatorBackend.ts:170DETERMINISTIC_REJECTION = /1010: Invalid Transaction/
Prose only 6 doc comments in live.globalSetup.ts, scripts/live/LiveOrchestrator.ts, test-utils/fixtures/nativeShieldedToken.ts

The retry logic and the canary now encode different notions of the same
rejection: the canary demands the full phrase, isDeterministicRejection matches
only the 1010: Invalid Transaction prefix. That is how these drift apart.

Proposed contracts/test-utils/harness/nodeRejections.ts:

/** Verified against ledger-v8 8.1.0 / midnight-js 4.1.1. These are the NODE's
 *  words, not ours — a ledger bump can reword them. */
export const VERIFIED_AGAINST = { '@midnight-ntwrk/ledger-v8': '8.1.0' } as const;

export const RPC_INVALID_TRANSACTION = '1010: Invalid Transaction';

export const NODE_REJECTION = {
  /** Tx IR exceeds the per-tx block byte budget. */
  blockLimitExceeded: `${RPC_INVALID_TRANSACTION}: Transaction would exhaust the block limits`,
  /** Coin spent against stale node state (re-spend / stale UTXO). */
  staleCoinState: `${RPC_INVALID_TRANSACTION}: Custom error: 103`,
} as const;

On tying this to the ledger version: a Record<ledgerVersion, ...> map would be
speculative, since only one ledger version is pinned at a time and the unused
branches would rot. Prefer a VERIFIED_AGAINST constant plus a harness unit test
asserting the installed version still matches, so a ledger bump fails loudly and
forces someone to re-verify the strings rather than discovering it later as a
mystery red canary.

2. errorChainText — the shallow-walk bug

Both canaries collect the error text from message, one level of cause, and
String(error). The node's 1010 text can also sit:

  • more than one level deep in a cause chain,
  • in AggregateError.errors,
  • only in toString(), for an effect FiberFailure that keeps its cause behind a
    Symbol.

isDeterministicRejection already walks all of these, and
test-utils/harness/test/LiveSimulatorBackend.test.ts:177-199 documents the
two-level and FiberFailure shapes explicitly. The shallow version would miss
those and fail on the wrong assertion.

It works today for the block-limit rejection (verified across three live runs), so
this is latent rather than broken. Fix it once, in the shared module, and have both
canaries plus isDeterministicRejection use it.

3. Tighten the base spec's matcher

contracts/src/token/test/ConfidentialFungibleToken.test.ts:1188 still uses
/block limits|exhaust the block/i. A broad matcher can be satisfied by an
unrelated deploy failure (funding, proving, a submission bounce) and report a false
green, which silently voids the scope claim the canary exists to prove. The
integration canary was tightened to the exact phrase; this one should match.

4. make env-down kills its own recipe shell

Every env-up and env-down prints:

make: [Makefile:21: env-down] Terminated (ignored)

Makefile:21 is @-pkill -f "docker compose -f $(COMPOSE_FILE) logs". pkill
excludes its own PID but not its parent — and the recipe's sh -c command line
contains the pattern, so pkill matches and kills the shell running it. The @-
prefix hides the failure.

Effect is mild: the line meant to kill the backgrounded docker compose logs -f
streamers is unreliable, but the following docker compose down stops the
containers, so the streamers exit anyway. Still, the line does not do what it was
written to do.

Fix is the standard bracket trick, so the pattern cannot match the shell that
contains it:

@-pkill -f "docker compose -f $(COMPOSE_FILE) [l]ogs" 2>/dev/null || true

5. Missing tests for the live orchestrator

The scripts vitest project added on the harness branch covers resolvePlan,
classify, and report naming. Still untested:

  • assertSoleLiveProject (live.globalSetup.ts) — belongs to the harness
    project, not scripts, so the new project does not reach it. Already exported as
    a pure function for this purpose.
  • RunLock staleness — inject pidAlive the way lockHolderState already does.
  • The teardown policy — exactly-once, opt-out honoured, and teardown ordered before
    the lock release.

6. No CI job runs the scripts project

It is green locally in ~200 ms and gates the orchestrator's only unit coverage.

7. Confirm CI gating parity for the integration-live job

scripts/test-live.ts --list now emits integration, and the workflow on
ci/live-test-workflow (draft #681) feeds --list into
fromJSON(needs.plan.outputs.categories), so the job is spawned automatically.
Unconfirmed: whether that job inherits the same node-up / 6-hour-budget /
live-tests-label gating as the per-category unit jobs.

8. Compile integration artifacts into their own directory

Split out into its own issue: #758. Raised by @andrew-fleming on #717.

Noted separately — not part of this issue

The dry integration project is red on main as installed: 5 failures in
contracts/test/integration/specs/initStateIsolation.spec.ts, because
test/integration/fixtures/* are written against an older
@openzeppelin/compact-simulator API. The installed 0.2.0 constructor takes
(deps), so new ComposedTokensSimulator(args, {}) hits deps.backend.kind on
undefined. Verified by running the suite with all harness-branch changes stashed.
This may already be tracked; if not it deserves its own issue, since fixing it
means updating three fixtures rather than the harness.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    2-lowNice-to-have / deferredCIChanges relating to CI/CD processesenhancementNew feature or request

    Type

    No type

    Projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions