Fix state isolation bug - eliminate shared singleton#3002
Closed
27Bslash6 wants to merge 2 commits intomodelcontextprotocol:mainfrom
Closed
Fix state isolation bug - eliminate shared singleton#300227Bslash6 wants to merge 2 commits intomodelcontextprotocol:mainfrom
27Bslash6 wants to merge 2 commits intomodelcontextprotocol:mainfrom
Conversation
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
Author
|
Closing - this was meant for my fork, not upstream. Apologies for the noise. |
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.
Summary
The Problem
Single
SequentialThinkingServerinstance atindex.ts:128was shared across ALL requests:This caused:
thoughtHistoryLengthvaluesThe Fix
Moved instance creation inside the request handler:
Each request now gets a fresh instance with isolated state.
Tests Added
Three new tests in
lib.test.ts:Test Results
All 40 tests pass:
Impact
Resolves #1