feat: ServerContext lifecycle — AsyncDisposable + stale-handler guards - #148
Conversation
📝 WalkthroughWalkthroughThis PR implements type-enforced lifecycle management for ServerContext via the AsyncDisposable protocol. ServerContext now tracks disposal state and implements Symbol.asyncDispose. A new ServerHandle type wraps McpServer with disposal semantics. All tool handlers check ctx?.disposed and return a LIFECYCLE error when invoked on disposed contexts, preventing stale reference bugs across setup cycles. ChangesServerContext Async Lifecycle and Tool Disposal Guards
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes The changes span tool handlers and core infrastructure with consistent patterns (disposal checks, error construction, and lifecycle tests) applied across multiple files. Logic is straightforward but requires verification that all tool entry points implement the guard consistently, that teardown properly sets disposed state, and that the async disposal protocol is correctly wired through setupCoreServer and ServerHandle. Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/core/server/server-context.lifecycle.test.ts`:
- Around line 1-59: This test file (the ServerContext lifecycle tests using
ServerContext, createServer, setupCoreServer and teardownServer) fails Prettier;
run the project's Prettier formatter against the file (or run the repo-wide
formatting command), apply the automatic fixes so whitespace/semicolons/spacing
match the project's config, then re-commit the updated file so CI formatting
checks pass.
In `@src/core/server/server-context.ts`:
- Around line 269-271: The async disposal currently calls this.teardown() but
leaves the singleton defaultContext pointing at the disposed instance; update
the async [Symbol.asyncDispose]() implementation to, after teardown, check if
defaultContext === this and if so clear/reset defaultContext (e.g., set to
undefined/null) so a future setupCoreServer path cannot reuse a disposed
context; reference the async disposal method ([Symbol.asyncDispose]),
teardown(), and defaultContext when making the change.
In `@src/core/setup.ts`:
- Around line 90-93: The disposal currently sets mcpServerInitialized = false
only after awaiting ctx[Symbol.asyncDispose](), so if that call throws the flag
stays true; update handle[Symbol.asyncDispose] to invoke
ctx[Symbol.asyncDispose]() inside a try block and move the mcpServerInitialized
= false into a finally block so the flag is always reset even on errors (refer
to handle[Symbol.asyncDispose], ctx[Symbol.asyncDispose] and the
mcpServerInitialized flag).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3acaf03c-f583-4bd3-97f7-160f8ec2b110
📒 Files selected for processing (18)
src/alliance/setup.tssrc/alliance/tools/guided-query-tool.tssrc/alliance/tools/suggest-query-params-tool.tssrc/core/index.tssrc/core/server/server-context.lifecycle.test.tssrc/core/server/server-context.tssrc/core/server/tool-error.test.tssrc/core/server/tool-error.tssrc/core/server/tools/count-tool.tssrc/core/server/tools/generate-urls-tool.tssrc/core/server/tools/keyword-search-tool.tssrc/core/server/tools/lifecycle.context.test.tssrc/core/server/tools/list-namespaces-tool.tssrc/core/server/tools/namespace-router-tool.tssrc/core/server/tools/query-documents-tool.tssrc/core/server/tools/query-tool.tssrc/core/setup.tstsconfig.json
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #148 +/- ##
=======================================
Coverage ? 85.79%
=======================================
Files ? 39
Lines ? 1478
Branches ? 495
=======================================
Hits ? 1268
Misses ? 208
Partials ? 2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Summary
week_plan_v1 Item 2:
ServerContextnow implementsAsyncDisposableso embedders can useawait usingfor automatic teardown, and all nine tool handlers reject invocations on a disposed context with a structuredLIFECYCLEerror instead of silently hitting nullified state.Issues addressed
Changes
ServerContextimplementsAsyncDisposable;teardown()setsdisposed = truebefore clearing owned stateServerHandle(McpServer & AsyncDisposable) returned fromsetupCoreServer/setupAllianceServer; exported from package rootawait usingon a server handle tears down context and resetsmcpServerInitializedso re-setup succeedsteardownServer()unchanged (sync backward-compat path; sameteardown()body)LIFECYCLEtool error code +lifecycleToolError()factoryctx?.disposed→LIFECYCLE)tsconfig.json:"lib": ["ES2022", "ESNext.Disposable"]Acceptance criteria
#134 (async-disposable)
ServerContexthas[Symbol.asyncDispose]()setupCoreServer()return type enablesawait usingteardownServer()remains for manual teardownawait usingtears down on scope exitesnext.disposablein tsconfig#135 (stale-handler guards)
disposedflag set by teardown / disposeLIFECYCLEerrorctx.disposedbefore executingTest plan
npm test— 215 tests passnpm run typecheck/npm run build— passserver-context.lifecycle.test.ts—await usingonServerContextandsetupCoreServerlifecycle.context.test.ts— stale closure, fresh cycle, setup → teardown → re-setuptool-error.test.ts—LIFECYCLEschema parseNotes for reviewers
ServerHandle[Symbol.asyncDispose]resetsmcpServerInitializedin addition to context teardown — required soawait usingallows a secondsetupCoreServer()in the same process.ctx?.disposed(optionalctxlegacy path unchanged). Setup always passesctx; stale-handler bug is closure-based.mcpServerInitializedscoping (multi-instance without teardown) is deferred to week_plan_v1 Item 3 / phase 4 setup API.Summary by CodeRabbit
Bug Fixes
New Features