From 0796cbc744a447ee00b0e6439449381f28a85e58 Mon Sep 17 00:00:00 2001 From: Jason Woltje Date: Thu, 5 Feb 2026 13:23:19 -0600 Subject: [PATCH] fix(#229): Remediate code review findings for performance tests - Fix CRITICAL: Increase single-spawn threshold from 10ms to 50ms (CI flakiness) - Fix CRITICAL: Replace no-op validation test with real backoff scale tests - Fix IMPORTANT: Add warmup iterations before all timed measurements - Fix IMPORTANT: Increase scan position ratio tolerance to 10x for sub-ms noise - Refactored queue perf tests to use actual service methods (calculateBackoffDelay) - Helper function to reduce spawn request duplication Co-Authored-By: Claude Opus 4.5 --- .../performance/queue-throughput.perf-spec.ts | 66 ++++++++--- .../secret-scanner-throughput.perf-spec.ts | 6 +- .../spawner-throughput.perf-spec.ts | 103 ++++++++---------- 3 files changed, 97 insertions(+), 78 deletions(-) diff --git a/apps/orchestrator/tests/performance/queue-throughput.perf-spec.ts b/apps/orchestrator/tests/performance/queue-throughput.perf-spec.ts index facd2e0..71cd346 100644 --- a/apps/orchestrator/tests/performance/queue-throughput.perf-spec.ts +++ b/apps/orchestrator/tests/performance/queue-throughput.perf-spec.ts @@ -1,8 +1,8 @@ /** * Performance Test: Queue Service Throughput * - * Benchmarks the queue service's pure functions and validation logic - * under load to verify performance characteristics. + * Benchmarks the queue service's pure functions under load + * to verify performance characteristics. * * Covers issue #229 (ORCH-128) */ @@ -44,6 +44,11 @@ describe("Performance: Queue Service", () => { describe("Backoff calculation performance", () => { it("should calculate 10,000 backoff delays in under 10ms", () => { + // Warmup + for (let i = 0; i < 100; i++) { + service.calculateBackoffDelay(i % 20, 1000, 60000); + } + const start = performance.now(); for (let i = 0; i < 10000; i++) { @@ -74,26 +79,55 @@ describe("Performance: Queue Service", () => { }); }); - describe("Validation performance", () => { - it("should validate 1000 task contexts rapidly", () => { - const contexts = Array.from({ length: 1000 }, (_, i) => ({ - repository: `https://git.example.com/repo-${String(i)}.git`, - branch: `feature/task-${String(i)}`, - workItems: [`US-${String(i).padStart(3, "0")}`], - skills: ["typescript", "nestjs"], - })); + describe("Backoff calculation at scale", () => { + it("should handle all retry levels from 0 to 100 consistently", () => { + // Warmup + for (let i = 0; i < 50; i++) { + service.calculateBackoffDelay(i, 1000, 60000); + } const start = performance.now(); + const results = new Map(); - for (const context of contexts) { - // Validate context fields (simulates what addTask validates) - expect(context.repository).toBeTruthy(); - expect(context.branch).toBeTruthy(); - expect(context.workItems.length).toBeGreaterThan(0); + for (let attempt = 0; attempt <= 100; attempt++) { + const delay = service.calculateBackoffDelay(attempt, 1000, 60000); + results.set(attempt, delay); } const duration = performance.now() - start; - expect(duration).toBeLessThan(100); + expect(duration).toBeLessThan(10); + + // Verify monotonic increase up to cap + for (let attempt = 1; attempt <= 100; attempt++) { + const current = results.get(attempt) ?? 0; + const previous = results.get(attempt - 1) ?? 0; + expect(current).toBeGreaterThanOrEqual(previous); + expect(current).toBeLessThanOrEqual(60000); + } + }); + + it("should calculate backoffs with varying base delays rapidly", () => { + const baseDelays = [100, 500, 1000, 2000, 5000]; + const maxDelays = [10000, 30000, 60000, 120000]; + + // Warmup + service.calculateBackoffDelay(0, 1000, 60000); + + const start = performance.now(); + + for (const base of baseDelays) { + for (const max of maxDelays) { + for (let attempt = 0; attempt < 20; attempt++) { + const delay = service.calculateBackoffDelay(attempt, base, max); + expect(delay).toBeLessThanOrEqual(max); + expect(delay).toBeGreaterThanOrEqual(base); + } + } + } + + const duration = performance.now() - start; + // 5 * 4 * 20 = 400 calculations should complete quickly + expect(duration).toBeLessThan(50); }); }); }); diff --git a/apps/orchestrator/tests/performance/secret-scanner-throughput.perf-spec.ts b/apps/orchestrator/tests/performance/secret-scanner-throughput.perf-spec.ts index c4663cd..f719c6a 100644 --- a/apps/orchestrator/tests/performance/secret-scanner-throughput.perf-spec.ts +++ b/apps/orchestrator/tests/performance/secret-scanner-throughput.perf-spec.ts @@ -115,9 +115,9 @@ describe("Performance: Secret Scanner", () => { expect(duration1).toBeLessThan(100); expect(duration2).toBeLessThan(100); - // And be within 5x of each other (no pathological behavior) - const ratio = Math.max(duration1, duration2) / Math.max(0.01, Math.min(duration1, duration2)); - expect(ratio).toBeLessThan(5); + // Both should complete within a reasonable ratio (allowing for sub-ms noise) + const ratio = Math.max(duration1, duration2) / Math.max(0.1, Math.min(duration1, duration2)); + expect(ratio).toBeLessThan(10); }); }); }); diff --git a/apps/orchestrator/tests/performance/spawner-throughput.perf-spec.ts b/apps/orchestrator/tests/performance/spawner-throughput.perf-spec.ts index 0e30f0e..f691a7f 100644 --- a/apps/orchestrator/tests/performance/spawner-throughput.perf-spec.ts +++ b/apps/orchestrator/tests/performance/spawner-throughput.perf-spec.ts @@ -14,6 +14,22 @@ import { AgentLifecycleService } from "../../src/spawner/agent-lifecycle.service import { KillswitchService } from "../../src/killswitch/killswitch.service"; import { ConfigService } from "@nestjs/config"; +function createSpawnRequest(taskId: string): { + taskId: string; + agentType: string; + context: { repository: string; branch: string; workItems: string[] }; +} { + return { + taskId, + agentType: "worker", + context: { + repository: "https://git.example.com/repo.git", + branch: "main", + workItems: [`US-${taskId}`], + }, + }; +} + describe("Performance: Agent Spawner Throughput", () => { let controller: AgentsController; let spawnerService: AgentSpawnerService; @@ -54,58 +70,48 @@ describe("Performance: Agent Spawner Throughput", () => { }); describe("Spawn latency", () => { - it("should spawn a single agent in under 10ms", async () => { + it("should spawn a single agent in under 50ms", async () => { + // Warmup + await controller.spawn(createSpawnRequest("warmup-1")); + const start = performance.now(); - await controller.spawn({ - taskId: "perf-single-001", - agentType: "worker", - context: { - repository: "https://git.example.com/repo.git", - branch: "main", - workItems: ["US-001"], - }, - }); + await controller.spawn(createSpawnRequest("perf-single-001")); const duration = performance.now() - start; - expect(duration).toBeLessThan(10); + expect(duration).toBeLessThan(50); }); it("should spawn 100 agents sequentially in under 500ms", async () => { + // Warmup + for (let i = 0; i < 5; i++) { + await controller.spawn(createSpawnRequest(`warmup-seq-${String(i)}`)); + } + const start = performance.now(); for (let i = 0; i < 100; i++) { - await controller.spawn({ - taskId: `perf-seq-${String(i)}`, - agentType: "worker", - context: { - repository: "https://git.example.com/repo.git", - branch: "main", - workItems: [`US-${String(i)}`], - }, - }); + await controller.spawn(createSpawnRequest(`perf-seq-${String(i)}`)); } const duration = performance.now() - start; expect(duration).toBeLessThan(500); + // 100 sequential + 5 warmup const agents = spawnerService.listAgentSessions(); - expect(agents).toHaveLength(100); + expect(agents.length).toBeGreaterThanOrEqual(100); }); it("should spawn 100 agents concurrently in under 200ms", async () => { + // Warmup + for (let i = 0; i < 5; i++) { + await controller.spawn(createSpawnRequest(`warmup-conc-${String(i)}`)); + } + const start = performance.now(); const promises = Array.from({ length: 100 }, (_, i) => - controller.spawn({ - taskId: `perf-concurrent-${String(i)}`, - agentType: "worker", - context: { - repository: "https://git.example.com/repo.git", - branch: `feature/task-${String(i)}`, - workItems: [`US-${String(i).padStart(3, "0")}`], - }, - }) + controller.spawn(createSpawnRequest(`perf-concurrent-${String(i)}`)) ); const results = await Promise.all(promises); @@ -121,19 +127,11 @@ describe("Performance: Agent Spawner Throughput", () => { }); describe("Session lookup performance", () => { - it("should look up agents by ID in under 1ms with 1000 sessions", async () => { + it("should look up agents by ID in under 10ms with 1000 sessions", async () => { // Pre-populate 1000 sessions const agentIds: string[] = []; for (let i = 0; i < 1000; i++) { - const result = await controller.spawn({ - taskId: `perf-lookup-${String(i)}`, - agentType: "worker", - context: { - repository: "https://git.example.com/repo.git", - branch: "main", - workItems: [`US-${String(i)}`], - }, - }); + const result = await controller.spawn(createSpawnRequest(`perf-lookup-${String(i)}`)); agentIds.push(result.agentId); } @@ -141,7 +139,7 @@ describe("Performance: Agent Spawner Throughput", () => { const lookupStart = performance.now(); for (let i = 0; i < 100; i++) { const randomIdx = Math.floor(Math.random() * agentIds.length); - const session = spawnerService.getAgentSession(agentIds[randomIdx]); + const session = spawnerService.getAgentSession(agentIds[randomIdx] ?? ""); expect(session).toBeDefined(); } const lookupDuration = performance.now() - lookupStart; @@ -153,15 +151,7 @@ describe("Performance: Agent Spawner Throughput", () => { it("should list all sessions in under 5ms with 1000 sessions", async () => { // Pre-populate 1000 sessions for (let i = 0; i < 1000; i++) { - await controller.spawn({ - taskId: `perf-list-${String(i)}`, - agentType: "worker", - context: { - repository: "https://git.example.com/repo.git", - branch: "main", - workItems: [`US-${String(i)}`], - }, - }); + await controller.spawn(createSpawnRequest(`perf-list-${String(i)}`)); } const listStart = performance.now(); @@ -175,18 +165,13 @@ describe("Performance: Agent Spawner Throughput", () => { describe("Memory efficiency", () => { it("should not have excessive memory growth after 1000 spawns", async () => { + // Force GC if available, then settle + if (global.gc) global.gc(); + const memBefore = process.memoryUsage().heapUsed; for (let i = 0; i < 1000; i++) { - await controller.spawn({ - taskId: `perf-mem-${String(i)}`, - agentType: "worker", - context: { - repository: "https://git.example.com/repo.git", - branch: "main", - workItems: [`US-${String(i)}`], - }, - }); + await controller.spawn(createSpawnRequest(`perf-mem-${String(i)}`)); } const memAfter = process.memoryUsage().heapUsed;