Conversation
…28) CRITICAL SECURITY FIX: The previous shallow copy using `new Map(this.graph)` only copied the Map structure - Set values remained as references to the original Sets. When checking for cycles, modifications to the temporary graph's Sets would mutate the original graph's Sets, corrupting state. Root cause: JavaScript's Map constructor creates shallow copies. For a Map<K, Set<V>>, the Set values are references, not copies. Fix: Deep copy using Array.from with mapped new Set() instances: `new Map(Array.from(this.graph.entries()).map(([k, v]) => [k, new Set(v)]))` Added 3 regression tests to verify immutability after cycle checks: - Graph unchanged after detecting would-create-cycle - Graph unchanged after detecting no-cycle - Graph unchanged after multiple sequential cycle checks 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Problem: Load average (os.loadavg()) is a 1-minute rolling average that doesn't reflect sudden resource spikes from recent spawns. The resource monitor could approve multiple spawns before load average caught up, causing fork-bomb scenarios during recovery or burst task submission. Solution: Track recently spawned workers in a "settling window" (15 seconds) and project their resource usage when deciding if more workers can spawn. Changes: - Add recentSpawnTimestamps array to track spawns within settling window - Project CPU and memory usage including settling workers - Add recordSpawn() method to ResourceMonitor interface (optional for backwards compatibility with existing implementations) - Call recordSpawn() from WorkerHandler after successful spawn - Increase default minSpawnDelayMs from 50ms to 1000ms for additional protection with the settling worker tracking This works in conjunction with the existing spawn delay to provide defense-in-depth against resource exhaustion. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
PR Compliance Guide 🔍Below is a summary of compliance checks for this PR:
Compliance status legend🟢 - Fully Compliant🟡 - Partial Compliant 🔴 - Not Compliant ⚪ - Requires Further Human Verification 🏷️ - Compliance label |
|||||||||||||||||||||||||||||
PR Code Suggestions ✨Explore these optional code suggestions:
|
||||||||||||
Fix 3 security issues identified by npm audit: - glob CLI command injection (HIGH) - GHSA-5j98-mcp5-4vw2 - body-parser denial of service (MODERATE) - GHSA-wqch-xfxh-vrr4 - vite path traversal on Windows (MODERATE) - GHSA-93m4-6634-74q7 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Previously, configuration validation failures silently fell back to defaults, making it difficult to discover environment variable typos or invalid values. Now logs a warning with specific errors. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Improves type safety by using the proper Worker domain type instead of any[] for the workers array return type. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
…kers tests - Implement recordSpawn() no-op in TestResourceMonitor for interface compatibility - Add 5 unit tests for settling workers tracking: - recordSpawn() does not throw - settling workers included in effective count - settling workers expire after 15 second window - settling workers persist within window - resource usage projection limits spawning 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
- Update wouldCreateCycle() line reference from 50 to 240 in TASK-DEPENDENCIES.md - Add deep copy pattern with explanation in TASK_ARCHITECTURE.md - Document why shallow copy (new Map(this.graph)) corrupts graph 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Add comprehensive changelog entries for recent work: - Security fixes (graph corruption, npm audit, config validation) - Performance improvements (settling workers tracking) - Technical improvements (type safety, test compatibility) - Test coverage (settling workers, graph immutability) - Documentation updates 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
| recordSpawn?(): void; | ||
| } | ||
|
|
||
| /** |
There was a problem hiding this comment.
[WARNING] Architecture: Optional Method Pattern Inconsistency
Making recordSpawn() optional violates the Interface Segregation Principle (ISP). This creates caller burden (requires optional chaining recordSpawn?.()) and implementation ambiguity.
Option 1: Make it required with no-op default (preferred)
export interface ResourceMonitor {
// ... existing methods ...
recordSpawn(): void; // Required, implementors can no-op
}Option 2: Use Interface Segregation
export interface ResourceMonitor { /* base methods */ }
export interface SettlingAwareResourceMonitor extends ResourceMonitor {
recordSpawn(): void;
}Comparison
| Approach | Pros | Cons |
|---|---|---|
| Required method | Simple, consistent contract | Breaks existing implementations |
| Interface segregation | Type-safe, backward compatible | More complex type hierarchy |
Recommended: Option 1 - Simpler and forces implementors to consciously handle spawn tracking.
From: architecture audit | Severity: MEDIUM
Generated by Claude Code via /code-review
| // SETTLING WORKERS TRACKING (Issue: load average is lagging indicator) | ||
| // Workers that were recently spawned but may not yet be reflected in system metrics | ||
| // This prevents spawning too many workers before load average catches up | ||
| private readonly SETTLING_WINDOW_MS = 15000; // 15 seconds for worker to "settle" |
There was a problem hiding this comment.
[WARNING] Complexity: Magic Number for Settling Window
The 15-second settling window is hardcoded. While documented with a comment, this value should ideally be configurable or derived from a configuration constant.
Suggested Fix:
// Option A: Make configurable via Configuration schema
private readonly SETTLING_WINDOW_MS: number;
constructor(config: Configuration) {
this.SETTLING_WINDOW_MS = config.workerSettlingWindowMs ?? 15000;
}
// Option B: Document relationship to other timings
// SETTLING_WINDOW_MS should be >= 3 * resourceMonitorIntervalMs (default 5000)
private readonly SETTLING_WINDOW_MS = 15000;Why: Hardcoded values reduce configurability and make tuning difficult for different deployment scenarios.
From: complexity audit | Severity: MEDIUM
Generated by Claude Code via /code-review
| // Workers that were recently spawned but may not yet be reflected in system metrics | ||
| // This prevents spawning too many workers before load average catches up | ||
| private readonly SETTLING_WINDOW_MS = 15000; // 15 seconds for worker to "settle" | ||
| private recentSpawnTimestamps: number[] = []; |
There was a problem hiding this comment.
[WARNING] Complexity: Mutable Array for Timestamp Tracking
The mutable array pattern works but could lead to subtle issues:
- No cleanup if
recordSpawn()is called butcanSpawnWorker()is never called - Array grows unbounded if cleanup only happens during spawn checks
Suggested Fix:
// Add cleanup to recordSpawn() as well to prevent unbounded growth
recordSpawn(): void {
this.cleanupSettlingWorkers(); // Prevent unbounded growth
this.recentSpawnTimestamps.push(Date.now());
this.logger?.debug('Recorded spawn for settling tracking', {
settlingWorkers: this.recentSpawnTimestamps.length,
settlingWindowMs: this.SETTLING_WINDOW_MS
});
}
private cleanupSettlingWorkers(): void {
const now = Date.now();
this.recentSpawnTimestamps = this.recentSpawnTimestamps.filter(
t => now - t < this.SETTLING_WINDOW_MS
);
}Why: Defensive programming - cleanup should happen on both read and write paths to handle edge cases.
From: complexity audit | Severity: MEDIUM
Generated by Claude Code via /code-review
|
|
||
| // Record spawn for settling worker tracking (accounts for lag in load average) | ||
| this.resourceMonitor.recordSpawn?.(); | ||
|
|
There was a problem hiding this comment.
[WARNING] Security: Optional Chaining Allows Silent Failure
The optional chaining (?.) means that if someone implements a custom ResourceMonitor without recordSpawn(), the settling workers tracking silently does nothing.
Suggested Fix:
// Add explicit logging when method is missing
if (this.resourceMonitor.recordSpawn) {
this.resourceMonitor.recordSpawn();
} else {
this.logger.debug('ResourceMonitor does not implement recordSpawn - settling tracking disabled');
}Why: Silent failures make debugging difficult. Explicit logging helps operators understand when spawn burst protection is inactive.
From: security audit | Severity: MEDIUM
Generated by Claude Code via /code-review
| this.logger?.debug('Cannot spawn: Max workers limit reached', { | ||
| // Clean up old spawn timestamps (outside settling window) | ||
| const now = Date.now(); | ||
| this.recentSpawnTimestamps = this.recentSpawnTimestamps.filter( |
There was a problem hiding this comment.
[WARNING] Performance: Array Filter Allocation
recentSpawnTimestamps.filter() creates a new array on every spawn check. While the array is typically small (0-5 elements), an in-place cleanup is more memory-efficient.
Suggested Fix (if optimization needed):
// In-place cleanup without allocation
private cleanupSettlingWorkers(): void {
const now = Date.now();
let writeIndex = 0;
for (let i = 0; i < this.recentSpawnTimestamps.length; i++) {
if (now - this.recentSpawnTimestamps[i] < this.SETTLING_WINDOW_MS) {
this.recentSpawnTimestamps[writeIndex++] = this.recentSpawnTimestamps[i];
}
}
this.recentSpawnTimestamps.length = writeIndex;
}Why: Current implementation is acceptable for expected usage (called every 5 seconds with <10 elements). Consider optimization only if this becomes a hot path.
From: performance audit | Severity: MEDIUM (acceptable trade-off)
Generated by Claude Code via /code-review
Code Review: Documentation IssuesThe following documentation issues were identified but are not directly addressable via line comments (files not in diff or cross-cutting concerns): 1. [WARNING] Stale Line Number in TASK-DEPENDENCIES.mdFile: Fix needed: - Dependency resolution: `src/services/handlers/dependency-handler.ts:344` (resolveDependencies)2. [WARNING] Missing Documentation for Settling Workers FeatureFiles affected:
Issue: The settling workers tracking feature is documented in CHANGELOG.md but missing from feature documentation. Suggested addition to FEATURES.md under "Autoscaling & Resource Management": ### Spawn Burst Protection (v0.3.1)
- **Settling Workers Tracking**: Tracks recently spawned workers for 15 seconds
- **Load Average Compensation**: Projects resource usage before system metrics update
- **recordSpawn() Interface**: ResourceMonitor method to record spawn events
- **minSpawnDelayMs**: Increased from 50ms to 1000ms for additional protection3. [WARNING] Tests: Missing Test CoverageIssue 1: Issue 2: No test for Suggested tests: // Test 1: Verify recordSpawn updates internal state
it('should update settling worker count after recordSpawn', () => {
monitor.recordSpawn();
// Use canSpawnWorker to verify settling workers are tracked
// ...
});
// Test 2: Verify WorkerHandler handles missing recordSpawn gracefully
it('should handle ResourceMonitor without recordSpawn method', () => {
const mockMonitor = {
getResources: vi.fn(),
canSpawnWorker: vi.fn(),
// Note: recordSpawn intentionally omitted
};
// Verify spawn works without throwing
});From: documentation and tests audits | Severity: MEDIUM Generated by Claude Code via |
…igurable Interface Segregation Principle compliance: recordSpawn() is now a required method on ResourceMonitor. Added settlingWindowMs configuration option for tunable settling window duration (default 15s). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
- Add cleanup of expired timestamps in recordSpawn() to prevent memory leak - Use configurable settlingWindowMs instead of hardcoded constant - Remove optional chaining on recordSpawn() call (now required method) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Add no-op recordSpawn() implementations to: - MockResourceMonitor (mock-resource-monitor.ts) - TestResourceMonitor (test-doubles.ts) - MockResourceMonitor (worker-handler.test.ts) Required after making recordSpawn() non-optional in interface. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
- Replace trivial not-throw test with actual state change verification - Test that recordSpawn() affects spawn eligibility at MAX_WORKERS=1 - Add all missing Configuration fields to ConfigFactory defaults 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
- Add Settling Workers Tracking section to FEATURES.md explaining the problem solved (lagging load average) and configuration options - Fix stale line reference in TASK-DEPENDENCIES.md (199->344) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
CI Feedback 🧐A test triggered by this PR failed. Here is an AI-generated analysis of the failure:
|
User description
Summary
DependencyGraph.wouldCreateCycle()where shallow copy allowed cycle checks to mutate the original graph state (Issue CRITICAL: Shallow copy in wouldCreateCycle() corrupts dependency graph #28)ResourceMonitorto prevent spawn burst overload when load average hasn't caught up with recent spawnsminSpawnDelayMsfrom 50ms to 1000ms for additional protectionChanges
Bug Fixes
Graph Corruption Prevention (Issue #28)
src/core/dependency-graph.ts: Changed from shallow copy (new Map(this.graph)) to deep copy that properly clones Set valuesMapconstructor only copies the Map structure;Setvalues remained as references to original SetswouldCreateCycle()could permanently add edges to the dependency graphPerformance Improvements
Settling Workers Tracking
src/implementations/resource-monitor.ts: AddedrecentSpawnTimestampsarray to track workers spawned within a 15-second settling windowsrc/implementations/resource-monitor.ts: AddedrecordSpawn()method to record spawn eventssrc/services/handlers/worker-handler.ts: CallsresourceMonitor.recordSpawn()after successful spawnsrc/core/interfaces.ts: Added optionalrecordSpawn()method toResourceMonitorinterfaceConfiguration Changes
src/core/configuration.ts: IncreasedminSpawnDelayMsdefault from 50ms to 1000msTests
tests/unit/core/dependency-graph.test.ts: Added 3 regression tests verifying graph immutability after cycle checksBreaking Changes
None. The
recordSpawn()method is optional on theResourceMonitorinterface for backwards compatibility.Testing
Test Coverage
dependency-graph.test.tswouldCreateCycle()Manual Testing Recommendations
Testing Gaps
Related Issues
Fixes #28
🤖 Generated with Claude Code
PR Type
Bug fix, Enhancement
Description
Fix critical graph corruption bug in
wouldCreateCycle()using deep copyAdd settling workers tracking to prevent spawn burst overload
recordSpawn()after successful worker spawnIncrease default
minSpawnDelayMsfrom 50ms to 1000ms for additional protectionDiagram Walkthrough
File Walkthrough
dependency-graph.ts
Deep copy implementation prevents graph mutationsrc/core/dependency-graph.ts
wouldCreateCycle()methodArray.from().map()to create new Set instances for each graphentry
interfaces.ts
Add recordSpawn method to ResourceMonitor interfacesrc/core/interfaces.ts
recordSpawn?()method toResourceMonitorinterfaceresource-monitor.ts
Implement settling workers tracking and resource projectionsrc/implementations/resource-monitor.ts
recentSpawnTimestampsarray andSETTLING_WINDOW_MSconstant (15seconds)
recordSpawn()method to track spawn eventscanSpawnWorker()to clean old timestamps and project resourceusage
settling workers
worker-handler.ts
Call recordSpawn after worker spawn eventsrc/services/handlers/worker-handler.ts
resourceMonitor.recordSpawn?.()after successful workerspawn
configuration.ts
Increase minSpawnDelayMs default to 1000mssrc/core/configuration.ts
minSpawnDelayMsdefault from 50ms to 1000ms in schemaminSpawnDelayMsdefault from 50ms to 1000ms in DEFAULT_CONFIGdependency-graph.test.ts
Add regression tests for graph immutabilitytests/unit/core/dependency-graph.test.ts
CRITICAL: Shallow copy in wouldCreateCycle() corrupts dependency graph #28)" suite
task
corruption
immutability