feat(listener): API rate-limit tests and repeatable load-testing workflow (#852, #860) - #895
Open
sheyman546 wants to merge 5 commits into
Open
sheyman546 wants to merge 5 commits into
sheyman546 wants to merge 5 commits into
Conversation
The listener as merged in Core-Foundry#799 contains duplicated statements and imports that make several source files emit invalid JavaScript. As a result the API module graph could not be loaded at all, and `tsc` aborted before semantic analysis (32 syntax errors, zero real type checking). Collapse each duplicated declaration back to a single implementation: - utils/request-id.ts, middleware/security-headers.ts and services/discord-notification.ts: incomplete statements that terminated a function early and left a dangling block - index.ts: `subscriber` declared twice in the same scope - services/event-subscriber.ts: duplicated `processableEvents` and `request` declarations, plus a `backfillStartLedger` field that was used but never declared - api/events-server.ts and config.ts: duplicated imports / type imports Source files now emit valid JavaScript, so typecheck and the test suite can actually run against them again. 🤖 Generated with Codebuff Co-Authored-By: Codebuff <noreply@codebuff.com>
…d load (Core-Foundry#852) Adds the scenario suite the issue asks for, driving both the limiter directly and the real HTTP server: - normal load: every distinct client is admitted, remaining quota strictly decreases, and one client's usage never consumes another's - burst: a concurrent spike is capped at exactly `maxRequests`, every rejection returns the identical 429 envelope, and concurrent distinct clients are all admitted - repeated load across windows: with fake timers, clients are re-admitted once the window rolls over and no quota leaks between windows - overrides / disabled limiter / client isolation - end-to-end over `createEventsServer`: enforcement, no false positives, and /health + /api/rate-limit/metrics staying reachable while throttled Also updates one stale assertion in rate-limiter.test.ts that still expected a bare `Too Many Requests` body; the limiter returns the standard `{success:false,error:{code:'RATE_LIMITED'}}` envelope. 🤖 Generated with Codebuff Co-Authored-By: Codebuff <noreply@codebuff.com>
…y#860) Introduces a load-testing workflow for the critical read endpoints, with documented scenarios and results that are comparable across changes. - load-test.config.json declares the scenarios (status, events, analytics, rate-limit-metrics) and the pass/fail thresholds; the format is documented in load-test.config.schema.md - src/utils/load-test-runner.ts is the dependency-free measurement core: throughput, nearest-rank p50/p90/p95/p99/min/mean/max latency, error rate (4xx vs 5xx split), threshold gating, and report-to-report comparison with a regression tolerance. Reports are schema-versioned JSON. - `npm run load-test` runs the documented scenarios in-process through Jest (boots createEventsServer on an ephemeral port and measures over real HTTP), which is the supported in-process path because the listener's runtime dependencies are mapped to test doubles by jest.config.js - `npm run load-test:external -- --url <baseUrl>` runs the same scenarios against an already-running listener, with --baseline/--fail-on-regression for gating - `npm run test:load` unit-tests the measurement core (metric maths, threshold gating, comparison) with an injected probe, so it is fast and socket-free - LOAD_TESTING.md documents the scenarios, how RPS/latency are measured, the baseline workflow, CI usage and how to read the results 🤖 Generated with Codebuff Co-Authored-By: Codebuff <noreply@codebuff.com>
listener/package-lock.json had drifted from package.json (it still pinned jest 29, typescript 5.4 and @types/node 25, and was missing @types/cors, @types/express, @types/node-cron and @types/winston), so `npm ci` failed with EUSAGE before installing anything. Regenerate the lock file so a clean install is possible again. 🤖 Generated with Codebuff Co-Authored-By: Codebuff <noreply@codebuff.com>
|
@sheyman546 Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
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
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.
Closes #852
Closes #860
Summary
This adds the API rate-limit test coverage asked for in #852 and the repeatable load-testing workflow asked for in #860.
While doing so I hit a blocker that had to be fixed first: the listener does not transpile on
main. The#799merge left duplicated statements and imports in several source files, so they emit invalid JavaScript, the API module graph cannot be loaded at all, andtscaborts with 32 syntax errors before it ever gets to type checking. That is why #852's own test suite cannot even be loaded onmain— it dies withSyntaxError: Unexpected token '*'fromsrc/utils/request-id.ts. So the PR is three things: repair the merge damage (prerequisite), then the rate-limit tests, then the load-testing workflow.What this fixes
1. Prerequisite — the listener sources from the #799 merge do not transpile
Duplicated/garbled statements were left behind by the merge, in the runtime path:
src/utils/request-id.tsgenerateCorrelationIdbody never closed; a/**doc block for the next function was swallowedsrc/services/discord-notification.tssanitizeForDiscordnever closedsrc/middleware/security-headers.tsimport type { http.ServerResponse }(invalid) andres: http.ServerResponsesrc/index.tssubscriberdeclared twice in the same scope (letat line 48,constat line 169)src/services/event-subscriber.tsprocessableEventsdeclared twice,requestdeclared twice (constthenlet), andbackfillStartLedgerused but never declaredsrc/api/events-server.tsTemplateService/handleTemplateRoutesimported twicesrc/config.ts2. #852 — API rate-limit tests
src/api/rate-limit-scenarios.test.tscovers the three traffic conditions named in the issue, plus the cross-cutting guarantees:maxRequests; every rejection returns the identical 429 envelope (X-RateLimit-Limit/Remaining,Retry-After,{success:false,error:{code:'RATE_LIMITED'}}); concurrent requests from distinct clients are all admittedcreateEventsServeron a real socket: enforcement, no false positives for legitimate concurrent clients, and/health+/api/rate-limit/metricsstaying reachable while a client is throttledI also updated one stale assertion in
rate-limiter.test.tsthat still expected a bareToo Many Requestsbody.src/utils/response.tsstandardised error responses on{success:false,error:{code,message}}, so the old assertion was testing a shape the service no longer returns.3. #860 — API load-testing workflow
load-test.config.json— the documented scenarios (status,events,analytics,rate-limit-metrics) plus pass/fail thresholds. The format is specified inload-test.config.schema.md.src/utils/load-test-runner.ts— a dependency-free measurement core: throughput (RPS), nearest-rank p50/p90/p95/p99/min/mean/max latency, error rate with a 4xx/5xx split, threshold gating, and report-to-report comparison with a regression tolerance. Reports are schema-versioned JSON so they can be diffed.src/utils/load-test-{config,http-probe,reports}.ts— config loading, the monotonic-clock HTTP probe, and report persistence, shared by both entry points.src/scripts/load-test.ts—npm run load-test:external -- --url <baseUrl>for an already-running listener, with--baseline/--fail-on-regressionfor CI gating and exit codes 0/1/2.src/__tests__/load-test.workflow.test.ts—npm run load-testruns the documented scenarios in-process over real HTTP and writesreports/load/latest.json.src/__tests__/load-test-runner.test.ts—npm run test:loadunit-tests the measurement core with an injected probe (metric maths, threshold gating, comparison, formatting). Fast and socket-free.LOAD_TESTING.md— documents the scenarios, how RPS/latency are measured, the baseline workflow, CI usage and how to read the results.4. Housekeeping
listener/package-lock.jsonhad drifted frompackage.json(it still pinned jest 29 / typescript 5.4 /@types/node25 and was missing@types/cors,@types/express,@types/node-cron,@types/winston), sonpm cifailed withEUSAGEbefore installing anything. Regenerated.Root cause
Three separate causes, and it's worth keeping them separate because only two of them are addressed here:
Merge damage.
#799(Merge pull request #799 from …/feature/issue-479-482-653-654) resolved conflicts by keeping both sides in several files instead of collapsing them. The result is syntactically wrong in some files (unclosed function bodies) and merely duplicated in others. Because the broken files sit in the runtime path (request-id.tsis imported by the whole API layer), the failure was total rather than local: nothing downstream could load, andtscreported only 32 syntax errors with zero semantic errors — i.e. type checking was silently doing nothing.Evidence that this is pre-existing and not caused by this PR: the baseline is
upstream/mainat30b99fe, checked out in a cleangit worktree; 4 files emit invalid JS there and the typecheck output is 32 errors, allTS1xxx.Missing coverage, not a broken feature. The rate limiter itself works and
rate-limiter.tsalready had unit tests. What was missing for Add API Rate-Limit Tests #852 was coverage of the behavioural cases (burst capping, window rollover, cross-client isolation) and of the contract that callers actually depend on (the 429 envelope), end-to-end over a socket. Nothing here changes limiter behaviour — if a test had failed against the fixed source I would have fixed the source, not the test; the only test I changed was the one asserting a response shape the service stopped returning in an earlier refactor.No load-testing capability at all for Add API Load Testing Workflow #860, so there was no way to state or compare RPS/latency.
The fix and why
Repair, not bypass. For each corrupted file I collapsed the two merged variants into the single newer implementation, rather than deleting the failing tests or loosening
tsconfig. Where the merge had kept an old and a new version of the same logic, I kept the newer semantics and the field declarations that the newer code depends on — for examplegetContractEventskeeps the cursor/backfill-limit path (resolveBackfillStartLedger) and drops the older inlinestartLedger: 1variant;index.tskeeps the outersubscriberassignment (the health monitor reads it at line 73) while adopting the newer?? undefinedargument.Why the load-test measurement core is separate from I/O. Splitting
load-test-runner.ts(pure) from the HTTP probe means the metric maths, threshold gating and comparison logic can be unit tested deterministically without opening a socket, and both entry points share exactly one implementation.percentile()uses the nearest-rank definition that most HTTP load tools use; latency is sampled withprocess.hrtime.bigint()so it is monotonic. Scenarios run sequentially with a warm-up phase excluded from the numbers, because concurrent scenarios would contend for the same event loop and make runs non-comparable — which is the whole point of #860.Why
npm run load-testis a Jest spec. The listener's runtime dependencies (@stellar/stellar-sdk,node-cache,uuid) are not installed;jest.config.jsmaps them to test doubles. I verified this directly — a plaints-nodein-process run dies onCannot find module '@stellar/stellar-sdk', then'node-cache'. Jest is therefore the only supported way to execute the API in-process, so the in-process workflow drives a realcreateEventsServeron an ephemeral port through a real HTTP probe from inside Jest. It is gated behindLOAD_TEST=1sonpm teststays fast and side-effect free.npm run load-test:externalcovers the "already-running server" case, which is how you'd load test staging. I deliberately avoided adding@stellar/stellar-sdkas a dependency — that's a much bigger change than this issue warrants (see follow-ups).Why schema-versioned JSON reports. "Results are comparable across changes" needs a stable artifact. Reports carry a
schemaVersionand the environment (node version, platform, CPU count, memory) so a diff is self-describing.reports/load/*.jsonis git-ignored, withreports/load/baseline.jsonexplicitly allowed so a team can commit one shared reference point.How it was tested
Baseline =
upstream/main@30b99fein a cleangit worktree. All commands run inlistener/.main)npm ci --dry-runEUSAGE(lock file out of sync)npm run typecheck(tsc --noEmit)TS1xxx) — semantic analysis never rannpm run buildnpm testrate-limiter+rate-limit-scenariosSyntaxError)npm run test:loadnpm run load-testIn-scope suites together: 4 suites / 50 tests passing (
rate-limiter,rate-limit-scenarios,load-test-runner,load-test.workflow).On the test counts. The suite-level number improves (54 → 43 failing) and the test-level numbers move in both directions (137 → 316 failing, 929 → 1189 passing). That is expected and is the point: on
mainthose suites failed to load, so their tests never ran. Fixing the corruption lets them execute and surface their own pre-existing failures. The invariant that matters is that I checked the failure sets explicitly:main(i.e. regressions caused by this PR): nonemainand pass now: 11npm run format:checkstill reports pre-existing style issues (157 files onmain, 156 now); every file added or changed here is Prettier-clean under the repo's.prettierrc.Method notes (worth knowing when you reproduce this)
npx jest --forceExit. Jest completes the run and then hangs on open handles (pre-existing), which makes a barenpm testappear to time out.tsconfig.jsonexcludes**/*.test.ts, sotscnever type-checks test files. That is why merge corruption in test files can hide indefinitely. I scanned all 214 files undersrc/by transpiling each one and runningnode --checkon the output to find the ones that emit invalid JS.jest.config.jssetsdiagnostics: { warnOnly: true }, so ts-jest type errors surface as warnings and do not fail suites.Follow-ups worth filing separately
These are all pre-existing on
mainand deliberately out of scope here; I list them so they don't get lost.src/__tests__/notification-flow-e2e.test.ts(twoit(...)bodies were merged into each other around lines 324–330, which is why Jest reports the confusingit(, async) { }) andsrc/tests/notification-scheduler-refactored.test.ts(pastLockdeclared twice). I did not attempt these because reconstructing which test body is intended is a judgement call the owning author should make.src/__mocks__/@stellar/stellar-sdk.tsdoes not exportxdr. Any test touchingxdr.ScValdies withCannot read properties of undefined (reading 'ScVal'), which accounts for a large share of the remaining red (stress, load, integration, event-registry, event-utils, discord-notification…). Addingxdrto the double would likely turn ~10 suites green.@stellar/stellar-sdk,node-cacheanduuidare imported acrosssrc/but absent frompackage.json— 13 of the 27 remaining typecheck errors areTS2307for these. It also means the listener cannot boot outside Jest, sonpm run dev/npm startdon't work. Worth deciding whether to declare them or make the doubles first-class.logger.debugandsanitizeUrlare missing from the test double, breakingnotification-deduplicatorand the response-time middleware tests.tests/api-versioning.test.ts,api/archive-api.test.tsandservices/notification-api-webhook.test.ts(they expect the pre-envelope shape).template_usage_logtable is missing in the template integration suite (SQLITE_ERROR: no such table).npm ci && npm run typecheck && npm testwould have caught all of the above. Happy to file this if useful.--forceExitunnecessary (open handles) and consider excludingsrc/__tests__/stress.test.ts/load.test.tsfrom the defaultnpm testrun so the suite doesn't take minutes.Reviewer checklist
cd listener && npm ci(now succeeds)npm run test:load→ measurement core testsLOAD_TEST=1 npx jest src/api/rate-limiter.test.ts src/api/rate-limit-scenarios.test.ts src/__tests__/load-test-runner.test.ts src/__tests__/load-test.workflow.test.ts --forceExit→ 50 testsnpm run load-test→ prints a report andThresholds: PASSmain(failure sets were diffed; zero regressions)🤖 Generated with Codebuff