Skip to content

Conversation

@27Bslash6
Copy link

Summary

  • Fixes race conditions caused by shared singleton SequentialThinkingServer instance
  • Creates new instance per request for complete state isolation
  • Adds comprehensive concurrency tests

The Problem

Single SequentialThinkingServer instance at index.ts:128 was shared across ALL requests:

const thinkingServer = new SequentialThinkingServer();

This caused:

  • Race conditions in concurrent requests
  • Session pollution (different conversations mixing thoughts)
  • Incorrect thoughtHistoryLength values
  • Non-deterministic behavior

The Fix

Moved instance creation inside the request handler:

server.setRequestHandler(CallToolRequestSchema, async (request) => {
  if (request.params.name === "sequentialthinking") {
    const thinkingServer = new SequentialThinkingServer();
    return thinkingServer.processThought(request.params.arguments);
  }
  // ...
});

Each request now gets a fresh instance with isolated state.

Tests Added

Three new tests in lib.test.ts:

  1. State isolation between different server instances
  2. Branch isolation (no cross-contamination)
  3. Interleaved concurrent request handling

Test Results

All 40 tests pass:

  • 37 existing tests (unchanged)
  • 3 new concurrency tests

Impact

  • Zero breaking changes
  • No API modifications
  • Stateless architecture (correct for MCP servers)
  • No memory leaks (instances are GC'd after request)

Resolves #1

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
@27Bslash6
Copy link
Author

Closing - this was meant for my fork, not upstream. Apologies for the noise.

@27Bslash6 27Bslash6 closed this Nov 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant