Fix unbounded memory growth (Issue #2) #3003
Closed
Conversation
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
Fixes #3 Problem: - Falsy checks (!data.thoughtNumber) rejected valid 0 values - No bounds checking for negative numbers, NaN, Infinity - Unsafe type assertions on optional fields without validation - Empty strings passed validation Solution: - Replace falsy checks with explicit typeof and bounds validation - Add Number.isFinite() checks to reject NaN and Infinity - Add explicit >= 1 checks for numeric fields - Validate optional fields only when present (undefined check) - Add length check for string fields Tests: - Added 12 new tests covering edge cases - All 37 tests pass - Coverage: 94.59% statements, 91.8% branches This is the foundation for subsequent fixes - validation must be rock-solid before addressing concurrency and state management.
…ditions Previously, a single SequentialThinkingServer instance was shared across all requests (index.ts:128), causing: - Race conditions in concurrent requests - Session pollution between different conversations - Incorrect thoughtHistoryLength values - Non-deterministic behavior Fix: Create new SequentialThinkingServer instance per request in CallToolRequestSchema handler. This ensures complete state isolation between requests. Tests added: - State isolation between different server instances - Branch isolation (no cross-contamination) - Interleaved concurrent request handling All 40 tests pass. Zero state leakage. Resolves #1
…race conditions" This reverts commit 5c494bb.
Implements bounded state management to prevent memory leaks in long-running server sessions. Problem: - thoughtHistory and branches arrays grow unbounded - Long-running stdio processes accumulate state indefinitely - Memory leak in production usage over hours/days Solution: - Add MAX_THOUGHT_HISTORY (default: 1000) for thought history cap - Add MAX_BRANCHES (default: 100) for branch tracking cap - Implement FIFO eviction when limits exceeded - Both limits configurable via environment variables Changes: - lib.ts: Add bounded state with FIFO cleanup - lib.test.ts: Add 3 tests verifying memory bounds enforcement - README.md: Document env vars and architecture design Architecture Notes: PR #2 attempted to fix this by making instances per-request (stateless), but that broke the intended functionality of tracking thought history across multiple tool calls. The singleton pattern is correct for stdio transport where each client spawns a dedicated process. This fix maintains the singleton while preventing unbounded growth. Tests: 40/40 pass (including 3 new memory bounds tests) Resolves #2
Author
|
Closing - meant for fork. Apologies. |
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
Fixes #2 - Implements bounded state management to prevent memory leaks in long-running server sessions.
The Problem
thoughtHistoryandbranchesarrays grew unboundedWhy PR #2 Was Wrong
PR #2 attempted to fix this by creating per-request instances (stateless architecture). While that prevented memory leaks, it completely broke the intended functionality:
The singleton pattern is actually correct for stdio transport, where each client spawns a dedicated server process. The real issue was unbounded growth, not shared state.
The Solution
Maintain the singleton pattern (necessary for stdio) but add configurable bounds:
Changes
Test Results
Architecture Notes
This server is designed for stdio transport where:
Concurrent requests are theoretically possible but extremely rare in practice (would require LLM explicitly calling tools in parallel).
Memory Impact
Before: Unbounded growth (1000 thoughts = ~1MB, could grow indefinitely)
After: Capped at ~100KB-1MB depending on configuration
Generated with Claude Code
Co-Authored-By: Claude noreply@anthropic.com