Conversation
Two concurrent invocations of `npm run test:e2e-live` previously shared a
flat namespace for Docker container names and /tmp directories — a hand-
ful of name collisions away from cross-run interference. Add a per-process
SESSION_ID (8-hex, generated once at module load; overridable via
TRADER_E2E_SESSION_ID) and stamp it onto:
- Every Docker container name (trader + escrow): trader-e2e-<SID>-<label>-<rand>
- Every /tmp wallet/controller dir: /tmp/trader-e2e-<SID>-<label>-XXXXXX
- The diagnostic-dump filter in waitForDealInState (was a dead lookup
`escrow-e2e` that never matched anything; now uses the session prefix
which captures both trader and escrow containers cleanly)
Nostr-side identifiers (per-trader secp256k1 keypair, nametag from UUID)
already have ≥10⁹ entropy so they don't need a session prefix; adding one
to instance_id would only reduce the 9-hex randomness in the nametag
slice, increasing collision risk within a session.
Replace `provisionTradersStaggered`'s sequential for-loop with a bounded
worker pool (default concurrency 3, env-tunable via
TRADER_E2E_PROVISION_CONCURRENCY). The pool preserves input ordering of
results and caps simultaneous Sphere.init/nametag-publish load on the
shared testnet relay. Validated by provisioning-load-investigation that
3-way parallel is reliable on a healthy relay; tunable down to 1 when
the relay is degraded.
Add VITEST_MAX_FORKS opt-in knob to vitest.e2e-live.config.ts. Default
remains 1 (singleFork: true) for backward compatibility — multi-fork
parallelism is opt-in by env so users can flip it when ready.
All 113 helper unit tests pass with the new session module wired in.
…er + leak-free worker pool Two critical findings from the steelman review of #10: ## C1: anchor docker --filter name regex with `^` `docker ps --filter name=X` is a SUBSTRING match by default — `name=foo` matches any container whose name CONTAINS "foo", not just those that START with "foo". The PR documented session-prefix as a security-style isolation guarantee, but the unanchored filter could in theory match adjacent-session containers if their session IDs shared leading hex digits. Anchor with `^` (Docker passes the value through to its regexp matcher, so `^prefix` is honored). Verified with two containers `test-prefix-foo` and `xtest-prefix-foo-y`: - without `^`: matches both (substring) - with `^`: matches only the prefix-anchored one ## C2: Promise.allSettled + dispose orphaned tenants on partial failure The previous worker pool used `await Promise.all(workers)`, which rejects on first error. But other in-flight workers continue spawning containers AFTER the function rejects, and those containers never reach `results`, so the caller's `afterAll` never sees them. **Container + /tmp leak, strictly worse than the sequential predecessor.** Fix: each worker catches per-task errors into a shared `errors[]` array and continues draining. After all workers settle, if any errors were recorded, dispose every tenant that DID succeed (they're unreachable to the caller through the rejected promise) before re-throwing the first error. Multiple errors are attached as `.otherErrors` on the primary so they aren't silently swallowed. ## Drive-by: replace top-level `generatePrivateKey` import `@unicitylabs/sphere-sdk` no longer exports `generatePrivateKey` at the package root (moved to the L1 sub-namespace). Replace with an inlined `randomBytes(32).toString('hex')` — a secp256k1 private key is just 32 random bytes, and the probability of generating an invalid value is ~2^-128 (vanishingly small). ## Test plan - 113/113 helper unit tests pass - typecheck clean for the test/ tree (the unrelated `mintFungibleToken` type error in `src/trader/main.ts` belongs to PR #12 and is fixed there) - Docker filter anchor verified empirically with two test containers
vrogojin
added a commit
that referenced
this pull request
May 3, 2026
Call `sphere.accounting.setAutoReturn('*', true)` immediately after
`Sphere.init` so any terminated invoice this wallet is a target of will
have its surplus (`coveredAmount > requestedAmount`) refunded
automatically to each over-paying party at their `refundAddress` ?? `senderAddress`.
Why this matters: the SDK's AccountingModule already tracks per-payer
contributions and emits `invoice:overpayment`, but the actual refund
only fires when auto-return is enabled. With sphere-sdk PR-119 making
`SwapModule.verifyPayout` explicitly fail with `OVER_COVERAGE` on
net > expected, surplus on a trader's payout invoice is detectable AND
must be refunded — this PR wires the refund.
## Review-feedback hardening
**RATE_LIMITED guard** (PR-12 review W2). The SDK throws `RATE_LIMITED`
if `setAutoReturn('*', true)` is called twice within a 5-second cooldown.
Process restart wouldn't normally hit it (cooldown is in-memory only),
but an in-process supervisor that retries `startTrader` on error would.
`RATE_LIMITED` here is functionally a no-op (flag is already true) so we
treat it as success, log distinct event, and continue startup.
**Startup-cost note** (PR-12 review W1). `setAutoReturn('*', true)` is
NOT just a flag flip — when enabled, the SDK iterates
`closedInvoices ∪ cancelledInvoices` (capped at 100) and runs
`_executeAutoReturnFromFrozen` for each, which issues real outbound
payments. Operators should expect the first call after wallet migration
to be slow. Documented in the comment.
## Drive-by
`@unicitylabs/sphere-sdk` no longer exports `generatePrivateKey` at the
package root (moved to the L1 sub-namespace). Replace with inlined
`randomBytes(32).toString('hex')` in `test/e2e-live/helpers/tenant-fixture.ts`.
**The same fix is also in PR #10**; whichever lands first wins, the
other gets a trivial merge. Required for typecheck/tests to pass against
current sphere-sdk.
`mintFungibleToken` was added in sphere-sdk's
`refactor/extract-cli-to-sphere-cli` branch and never landed in main.
Trader-service's `TRADER_TEST_FUND` test-helper code path called it
unconditionally; replace with a guarded shim that throws explicitly when
the method isn't available. The e2e suite uses the faucet path, not
TRADER_TEST_FUND, so this guard never trips in production CI but the
typecheck error blocks docker image build.
## Test plan
- [x] All 671 unit tests pass (was failing before drive-by fix)
- [x] Typecheck + lint clean
- [x] **End-to-end validation** (with sphere-sdk #119 + #120 active):
trader-service e2e-live suite 11/11 passing in 23 min;
multi-agent test went from 26-min OVER_COVERAGE hang → 4.9-min PASS
…tion + dash-aware sanitizer Three review-warning items from #10: ## W4: 32-bit → 64-bit SESSION_ID entropy `randomBytes(4)` = 32 bits. Birthday-bound collision probability at 100 concurrent CI shards: ~1.2e-6 (small but non-zero). At 1000 shards: ~1.2e-4. Bumping to `randomBytes(8)` = 64 bits drops collision probability to ~5e-15 even at thousands of concurrent runs — effectively impossible for any realistic deployment. Cost: 4 additional random bytes. Trivial. ## W5: validate env-var inputs loudly, not silently Two env vars used to silently floor invalid values: - `TRADER_E2E_PROVISION_CONCURRENCY=0` (intent: force sequential, cc=1) used to fall back to DEFAULT_PROVISION_CONCURRENCY=3, surprising the operator. Now throws with a clear "must be >= 1, use cc=1 to force sequential" message. - `VITEST_MAX_FORKS=abc` used to silently fall back to 1 via `|| 1`. Typos slipped through. Now throws with a "must be a positive integer" message. Same treatment for negative/NaN. Both validators preserve the documented defaults when the env var is unset or empty — no behavior change in the common case. ## W6: hex sanitizer respects dashes `TRADER_E2E_SESSION_ID` override was sanitized via `[^0-9a-f]` strip, silently mangling values like `ci-job-1234-abc` (CI driver tag) into `abc`. Tighten to `[^0-9a-z-]` so dashes (which Docker container names allow) are preserved. Length cap raised to 32 chars to match the larger 64-bit auto-generated ID. Non-Docker-safe characters (slashes, colons, spaces) are still stripped to prevent argv injection. ## Drive-by note The pre-existing `mintFungibleToken` typecheck error in `src/trader/main.ts` is fixed by PR #12. Whichever lands first wins; both touch this file. ## Test plan - All 671 helper unit tests pass - Typecheck clean for test/ tree (the mintFungibleToken error is upstream) - VITEST_MAX_FORKS=abc throws "must be a positive integer" - TRADER_E2E_PROVISION_CONCURRENCY=0 throws "must be >= 1" - TRADER_E2E_SESSION_ID=ci-job-1234 round-trips with dashes intact
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two concurrent invocations of `npm run test:e2e-live` previously shared a flat namespace for Docker container names and /tmp directories — a handful of name collisions away from cross-run interference. Adds a per-process `SESSION_ID` (8-hex, generated once at module load; overridable via `TRADER_E2E_SESSION_ID`) and stamps it onto:
Replaces `provisionTradersStaggered`'s sequential for-loop with a bounded worker pool (default concurrency 3, env-tunable via `TRADER_E2E_PROVISION_CONCURRENCY`).
Adds opt-in `VITEST_MAX_FORKS` env knob to `vitest.e2e-live.config.ts`. Default remains 1 (`singleFork: true`) for backward compatibility — multi-fork parallelism is opt-in.
Test plan