Skip to content

Commit b13cc3b

Browse files
committed
fix(chaos): exclude intentional teardown from drop metrics and redact JWT secret
Address PR review feedback on the WS portfolio feed load test: (1) mark connections intentionalClose before teardown so the close handler does not count them as unexpected drops/failures; (2) redact the JWT secret from persisted results via sanitizeConfig since results are uploaded as public CI artifacts; (3) stop hardcoding JWT_SECRET in performance-test.yml and generate an ephemeral runtime secret instead; (4) resolve ws/jsonwebtoken through a createRequire fallback because ESM import() ignores NODE_PATH; (5) run the CI load test with auth enabled (CHAOS_WS_AUTH_ENABLED=true and CHAOS_WS_JWT_SECRET=$JWT_SECRET) since /ws/portfolio/:id unconditionally requires a valid JWT.
1 parent f5ed7fb commit b13cc3b

3 files changed

Lines changed: 151 additions & 69 deletions

File tree

.github/workflows/performance-test.yml

Lines changed: 14 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -58,14 +58,17 @@ jobs:
5858
export NODE_ENV=test
5959
export PORT=3001
6060
export DATABASE_URL="sqlite::memory:"
61-
export JWT_SECRET="test-secret-key-for-ws-load-testing"
61+
# Runtime-generated (never hardcoded/committed): avoids serializing a
62+
# secret into the workflow or CI artifacts.
63+
export JWT_SECRET="$(openssl rand -hex 32)"
6264
export STELLAR_NETWORK_PASSPHRASE="Test SDF Network ; September 2015"
6365
export CORS_ORIGINS="http://localhost:3000,http://localhost:5173"
6466
export LOG_LEVEL="warn"
6567
export AUTH_ENABLED="false"
6668
export ENABLE_STARTUP_SELF_TEST="false"
67-
# Resolve ws package from backend's node_modules
68-
export NODE_PATH="backend/node_modules"
69+
# Resolve ws package from backend's node_modules (used via the createRequire
70+
# fallback in the load test, since ESM import() ignores NODE_PATH)
71+
export NODE_PATH="$GITHUB_WORKSPACE/backend/node_modules"
6972
7073
# Start backend in background (use working-directory approach for reliable PID capture)
7174
node backend/dist/index.js &
@@ -91,7 +94,10 @@ jobs:
9194
CHAOS_WS_BATCH_SIZE="10" \
9295
CHAOS_WS_BATCH_DELAY_MS="200" \
9396
CHAOS_WS_DURATION_MS="30000" \
94-
CHAOS_WS_AUTH_ENABLED="false" \
97+
# /ws/portfolio/:id unconditionally requires a valid JWT, so the load
98+
# test must sign tokens with the SAME runtime secret the backend uses.
99+
CHAOS_WS_AUTH_ENABLED="true" \
100+
CHAOS_WS_JWT_SECRET="$JWT_SECRET" \
95101
CHAOS_WS_RESULTS_DIR="ws-load-test-results" \
96102
node scripts/chaos/ws-portfolio-feed-load-test.mjs
97103
LOAD_TEST_EXIT=$?
@@ -121,7 +127,7 @@ jobs:
121127
echo "**Test Configuration:**" >> $GITHUB_STEP_SUMMARY
122128
echo "- Concurrent Connections: 50" >> $GITHUB_STEP_SUMMARY
123129
echo "- Sustain Duration: 30s" >> $GITHUB_STEP_SUMMARY
124-
echo "- Auth Enabled: false" >> $GITHUB_STEP_SUMMARY
130+
echo "- Auth Enabled: true (runtime-generated JWT secret)" >> $GITHUB_STEP_SUMMARY
125131
echo "" >> $GITHUB_STEP_SUMMARY
126132
echo "**Purpose:** Periodically verifies WebSocket portfolio feed capacity and documents connection ceiling for planning." >> $GITHUB_STEP_SUMMARY
127133
@@ -167,7 +173,9 @@ jobs:
167173
export NODE_ENV=test
168174
export DATABASE_URL="sqlite::memory:"
169175
export REDIS_URL="redis://localhost:6379"
170-
export JWT_SECRET="test-secret-key-for-performance-testing"
176+
# Runtime-generated (never hardcoded/committed): avoids serializing a
177+
# secret into the workflow or CI artifacts.
178+
export JWT_SECRET="$(openssl rand -hex 32)"
171179
export STELLAR_NETWORK_PASSPHRASE="Test SDF Network ; September 2015"
172180
export CORS_ORIGINS="http://localhost:3000,http://localhost:5173"
173181
export LOG_LEVEL="warn"

pr_body.md

Lines changed: 91 additions & 55 deletions
Original file line numberDiff line numberDiff line change
@@ -1,79 +1,115 @@
1-
Closes #995
1+
Closes #1530
22

33
## Summary
44

5-
Implements a paginated, filterable **rebalance history endpoint** at `GET /portfolio/:id/rebalance-history` that returns past rebalance outcomes for a given portfolio, including failed rebalances with error reasons.
5+
Implements a comprehensive chaos/load test script (`scripts/chaos/ws-portfolio-feed-load-test.mjs`) that simulates up to thousands of concurrent WebSocket subscriptions against the `portfolioFeed.ts` endpoint in order to measure connection acceptance latency, message delivery latency, and server resource usage under load. The test identifies and documents the practical concurrent-connection ceiling for current infrastructure sizing, enabling data-driven capacity planning.
66

77
---
88

99
## What was added
1010

11-
### New endpoint: `GET /portfolio/:id/rebalance-history`
11+
### New Script: `scripts/chaos/ws-portfolio-feed-load-test.mjs`
1212

13-
Returns a paginated list of past rebalances for a portfolio. Each record includes:
13+
A standalone Node.js ESM script that stress-tests the WebSocket portfolio feed endpoint (`/ws/portfolio/:id`) with configurable concurrency. Phase breakdown:
1414

15-
| Field | Description |
15+
| Phase | Description |
1616
|-------|-------------|
17-
| `timestamp` | ISO-8601 datetime of the rebalance |
18-
| `trigger` | Raw trigger description |
19-
| `triggerType` | Normalized: `manual`, `auto`, or `circuit_breaker` |
20-
| `assetsTrades` | Number of asset trades executed |
21-
| `totalFeeXlm` | Total gas fee in XLM (null if unavailable) |
22-
| `totalFeeUsd` | Total gas fee in USD (null if unavailable) |
23-
| `totalSlippageBps` | Total slippage in basis points (null if unavailable) |
24-
| `status` | `success`, `partial`, or `failed` |
25-
| `errorReason` | Error description for failed rebalances (null otherwise) |
26-
27-
### Query Parameters (Filters)
28-
29-
| Param | Type | Description |
30-
|-------|------|-------------|
31-
| `from` | ISO-8601 string | Lower-bound timestamp filter (inclusive) |
32-
| `to` | ISO-8601 string | Upper-bound timestamp filter (inclusive) |
33-
| `trigger_type` | `manual` \| `auto` \| `circuit_breaker` | Filter by trigger type |
34-
| `status` | `success` \| `partial` \| `failed` | Filter by rebalance outcome |
35-
| `page` | integer (default: 1) | Page number |
36-
| `page_size` | integer (default: 50, max: 500) | Records per page |
37-
| `sort` | `asc` \| `desc` (default: desc) | Sort order by timestamp |
38-
39-
### Response Shape
17+
| **Phase 1: Health Check** | Verifies the backend is reachable via `GET /health` before starting |
18+
| **Phase 2: Ramp Up** | Opens WebSocket connections in configurable batches (supports `batch` and `linear` ramp strategies) |
19+
| **Phase 3: Sustain** | Maintains all open connections for a configurable duration while collecting message delivery metrics and server resource samples |
20+
| **Phase 4: Teardown** | Gracefully closes all connections with `1000` close code |
21+
| **Phase 5: Report** | Generates a comprehensive latency and resource report with capacity planning assessment |
22+
23+
### Metrics Collected
24+
25+
| Metric | Description |
26+
|--------|-------------|
27+
| **Connection Acceptance Latency** | Time from `new WebSocket()` to `open` event, with P50/P75/P95/P99/max/mean |
28+
| **First Message Delivery Latency** | Time from `CONNECTION_ACK` to first `PORTFOLIO_VALUE_UPDATE` |
29+
| **Message Delivery Latency** | Server timestamp vs. client receipt time for all `PORTFOLIO_VALUE_UPDATE` broadcasts |
30+
| **Message Throughput** | Total messages received and messages-per-second rate during sustain |
31+
| **Server Resource Usage** | Polls backend `/metrics` endpoint periodically during sustain phase |
32+
| **Client Overhead** | Tracks heap usage, RSS, and external memory of the load generator itself |
33+
| **Connection Drop Rate** | Connections that unexpectedly closed during the sustain phase |
34+
| **Connection Failure Rate** | Connections that failed to establish or receive `CONNECTION_ACK` |
35+
36+
### Ceiling Assessment
37+
38+
The script automatically assesses the practical connection ceiling based on:
39+
- Connection failure rate (>10% → degraded)
40+
- Connection drop rate during sustain (>5% → degraded)
41+
- P95 connection latency (>5s → degraded)
42+
- P95 message delivery latency (>2s → degraded)
43+
44+
When no issues are detected, it reports the tested concurrency level as the _minimum_ ceiling and recommends re-running with higher concurrency to find the true limit. When issues are detected, it estimates the practical ceiling using a penalty factor.
45+
46+
### Configuration (Environment Variables)
47+
48+
| Variable | Default | Description |
49+
|----------|---------|-------------|
50+
| `CHAOS_WS_BACKEND_URL` | `http://localhost:3001` | Backend base URL |
51+
| `CHAOS_WS_CONCURRENT_CONNECTIONS` | `100` | Total WebSocket connections to simulate |
52+
| `CHAOS_WS_BATCH_SIZE` | `10` | Connections to open per batch |
53+
| `CHAOS_WS_BATCH_DELAY_MS` | `200` | Delay between batches |
54+
| `CHAOS_WS_DURATION_MS` | `60000` | How long to sustain load (ms) |
55+
| `CHAOS_WS_PORTFOLIO_ID_PREFIX` | `load-test-pf` | Prefix for generated portfolio IDs |
56+
| `CHAOS_WS_JWT_SECRET` | auto-generated | JWT secret for token generation |
57+
| `CHAOS_WS_AUTH_ENABLED` | `false` | Whether to require JWT auth tokens |
58+
| `CHAOS_WS_VERBOSE` | unset | Enable debug-level logging |
59+
| `CHAOS_WS_TIMEOUT_MS` | `10000` | Individual connection timeout |
60+
| `CHAOS_WS_RAMP_STRATEGY` | `batch` | `batch` or `linear` ramp strategy |
61+
62+
### Usage
63+
64+
```bash
65+
# Run with defaults (100 connections, 60s sustain)
66+
npm run test:chaos:ws-load
67+
68+
# Run with custom concurrency
69+
CHAOS_WS_CONCURRENT_CONNECTIONS=500 CHAOS_WS_DURATION_MS=120000 \
70+
npm run test:chaos:ws-load
71+
72+
# Run with auth enabled (requires backend JWT secret)
73+
CHAOS_WS_AUTH_ENABLED=true CHAOS_WS_JWT_SECRET=my-secret-key \
74+
npm run test:chaos:ws-load
75+
```
76+
77+
### CI Integration
78+
79+
The script is added to the root `package.json` as `test:chaos:ws-load` alongside the existing `test:chaos` script. It can be integrated into CI for periodic capacity verification:
4080

4181
```json
42-
{
43-
"data": {
44-
"history": [ /* PortfolioRebalanceHistoryItem[] */ ],
45-
"pagination": {
46-
"page": 1,
47-
"pageSize": 50,
48-
"total": 127,
49-
"totalPages": 3
50-
},
51-
"filters": {
52-
"from": null,
53-
"to": null,
54-
"trigger_type": null,
55-
"status": null
56-
}
57-
}
58-
}
82+
"test:chaos:ws-load": "node scripts/chaos/ws-portfolio-feed-load-test.mjs"
5983
```
6084

61-
### Acceptance Criteria
85+
---
86+
87+
## Acceptance Criteria
88+
89+
- ✅ Load test can simulate a **configurable number** of concurrent WS subscriptions (via `CHAOS_WS_CONCURRENT_CONNECTIONS`)
90+
- ✅ Test produces a report on **latency** (connection, first-message, delivery with P50/P75/P95/P99) and **resource usage** (server `/metrics` + client overhead) under the simulated load
91+
-**Practical connection ceiling** is documented for capacity planning (automatic `assessCeiling()` function)
92+
- ✅ Script follows existing chaos test patterns (`kill-backend-mid-rebalance.mjs`) with consistent logging, env var configuration, and phased execution
93+
94+
---
95+
96+
## Review Fixes
97+
98+
Resolves the technical issues raised in the PR thread:
6299

63-
- ✅ All rebalance outcomes recorded and returned (success, partial, failed)
64-
- ✅ Failed rebalances include `errorReason` field with the failure description
65-
- ✅ Response time monitoring: queries exceeding 200ms are logged as warnings
66-
- ✅ Paginated with `page`, `page_size`, `total`, `totalPages`
67-
- ✅ Filterable by `from`, `to`, `trigger_type`, `status`
100+
- **Intentional disconnects no longer counted as failures.** The teardown phase closes every open connection with close code `1000`. Previously the `close` handler incremented `connectionsDropped` for any connection closed while in the `open` state, so the intentional teardown inflated the drop rate to ~100% and corrupted the ceiling assessment and reported metrics. Connections are now marked `intentionalClose` before teardown and their closes are excluded from the drop/failure counts (`dropRate` reflects only unexpected disconnects during the sustain phase).
101+
- **JWT secret no longer serialized into CI artifacts.** The results JSON previously persisted the full `CONFIG` object, including the JWT secret, and the results file is uploaded as a public workflow artifact (`ws-load-test-report`). The persisted `configuration` now redacts `jwtSecret` via `sanitizeConfig()`. The workflow also no longer hardcodes plaintext `JWT_SECRET` values; it generates an ephemeral random secret at runtime with `openssl rand -hex 32`.
102+
- **CI job now authenticates correctly.** `portfolioFeed.ts` unconditionally requires a valid JWT, so the load test job now runs with `CHAOS_WS_AUTH_ENABLED=true` and passes the same runtime secret as `CHAOS_WS_JWT_SECRET`, ensuring the tokens signed by the test validate against the backend.
103+
- **Rebased onto the base branch** (231 commits behind at the time of rebasing) to restore compatibility and keep the PR mergeable.
68104

69105
---
70106

71107
## Files Changed
72108

73109
### New files
74-
- `backend/src/test/rebalanceHistory.routes.test.ts`Unit tests covering: 404 for missing portfolio, default pagination, filter passthrough, failed event error reasons, and totalPages calculation.
110+
- `scripts/chaos/ws-portfolio-feed-load-test.mjs`Load test script
75111

76112
### Modified files
77-
- `backend/src/api/portfolios.routes.ts` — Added `GET /portfolio/:id/rebalance-history` route
78-
- `backend/src/api/validation.ts`Added `portfolioRebalanceHistoryQuerySchema` with Zod validation for all query parameters
79-
- `backend/src/db/rebalanceHistoryDb.ts`Added `dbGetPortfolioRebalanceHistory()` — parameterised SQL query with dynamic WHERE clause construction, COUNT for total, and status/trigger mapping
113+
- `package.json` — Added `"test:chaos:ws-load"` script entry
114+
- `.github/workflows/performance-test.yml`CI integration for the load test; no longer hardcodes `JWT_SECRET`
115+
- `pr_body.md`PR description

scripts/chaos/ws-portfolio-feed-load-test.mjs

Lines changed: 46 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -29,22 +29,39 @@
2929
import { randomBytes } from 'node:crypto'
3030
import { setTimeout as sleep } from 'node:timers/promises'
3131

32-
// ─── Dynamic import of ws to avoid issues when module not installed ──────────
33-
let WebSocketImpl
32+
// ─── Resolve optional runtime deps (ws, jsonwebtoken) ────────────────────────
33+
// In CI this script runs from the repo root while `ws`/`jsonwebtoken` live in
34+
// backend/node_modules. ESM `import()` ignores NODE_PATH, so fall back to
35+
// createRequire (CommonJS, honors NODE_PATH) and finally to the parent project
36+
// for local installs.
37+
import { createRequire } from 'node:module'
38+
39+
const requireFromScript = createRequire(import.meta.url)
40+
41+
let WebSocketImpl = null
3442
try {
3543
const wsModule = await import('ws')
3644
WebSocketImpl = wsModule.default || wsModule.WebSocket || wsModule
3745
} catch (_) {
38-
console.error('[CHAOS-WS] The "ws" package is required. Run: npm install ws')
39-
process.exit(1)
46+
try {
47+
const wsModule = requireFromScript('ws')
48+
WebSocketImpl = wsModule.WebSocket || wsModule.default || wsModule
49+
} catch (__) {
50+
console.error('[CHAOS-WS] The "ws" package is required. Run: npm install ws')
51+
process.exit(1)
52+
}
4053
}
4154

4255
// Optional: jwt for token generation when auth is enabled
4356
let jwtModule = null
4457
try {
4558
jwtModule = await import('jsonwebtoken')
4659
} catch (_) {
47-
// jwt is optional; only needed when CHAOS_WS_AUTH_ENABLED=true
60+
try {
61+
jwtModule = requireFromScript('jsonwebtoken')
62+
} catch (__) {
63+
// jwt is optional; only needed when CHAOS_WS_AUTH_ENABLED=true
64+
}
4865
}
4966

5067
// ─── Configuration ───────────────────────────────────────────────────────────
@@ -67,6 +84,16 @@ const CONFIG = {
6784
const httpHost = CONFIG.backendUrl.replace(/^https?:\/\//, '')
6885
const wsBaseUrl = `ws://${httpHost}`
6986

87+
/**
88+
* Configuration copy that is safe to persist/upload: the JWT secret must
89+
* never be serialized into results files or CI artifacts.
90+
*/
91+
function sanitizeConfig(config) {
92+
const safeConfig = { ...config }
93+
safeConfig.jwtSecret = '[REDACTED]'
94+
return safeConfig
95+
}
96+
7097
// ─── Logging ─────────────────────────────────────────────────────────────────
7198

7299
function ts() {
@@ -200,7 +227,9 @@ function generateTestToken(portfolioId, userId) {
200227
if (!jwtModule) {
201228
throw new Error('jsonwebtoken module not available. Install: npm install jsonwebtoken')
202229
}
203-
return jwtModule.default.sign(
230+
// Works for both ESM namespace (jwtModule.default) and CJS exports (jwtModule)
231+
const jwtApi = jwtModule.default || jwtModule
232+
return jwtApi.sign(
204233
{ sub: userId, type: 'access' },
205234
CONFIG.jwtSecret,
206235
{ expiresIn: '1h' },
@@ -241,6 +270,8 @@ function connectPortfolioFeed(portfolioId, userId, token) {
241270
firstMessageTime: null,
242271
messagesReceived: [],
243272
state: 'connecting',
273+
/** Set to true before an intentional teardown close so it is not counted as a drop/failure */
274+
intentionalClose: false,
244275
}
245276

246277
ws.on('open', () => {
@@ -300,7 +331,11 @@ function connectPortfolioFeed(portfolioId, userId, token) {
300331

301332
ws.on('close', (code, reason) => {
302333
clearTimeout(connectTimeout)
303-
if (record.state === 'connecting') {
334+
// Intentional teardown (closeAllConnections) must never count as a drop
335+
// or failure — those metrics are for unexpected disconnects only.
336+
if (record.intentionalClose) {
337+
record.state = 'closed-intentional'
338+
} else if (record.state === 'connecting') {
304339
latencyMetrics.connectionsFailed++
305340
record.state = 'closed-early'
306341
} else if (record.state === 'open') {
@@ -346,6 +381,9 @@ function closeAllConnections(records) {
346381
log.info('Closing ' + openRecords.length + ' open connections...')
347382

348383
for (const record of openRecords) {
384+
// Mark as intentional so the 'close' handler does not count this teardown
385+
// as an unexpected drop or a connection failure.
386+
record.intentionalClose = true
349387
try {
350388
record.ws.close(1000, 'Load test complete')
351389
} catch (_) {
@@ -571,7 +609,7 @@ async function runLoadTest() {
571609
const resultsPath = path.default.join(dir, 'ws-load-test-results.json')
572610
const resultsData = {
573611
timestamp: new Date().toISOString(),
574-
configuration: CONFIG,
612+
configuration: sanitizeConfig(CONFIG),
575613
connectionSummary: {
576614
totalAttempted: CONFIG.concurrentConnections,
577615
succeeded: latencyMetrics.connectionsSucceeded,

0 commit comments

Comments
 (0)