refactor(server): Split lifespan into composable browser + auth lifespans#199
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
There was a problem hiding this comment.
Pull request overview
Refactors the MCP server startup/shutdown handling by splitting the prior single lifespan context into composable auth + browser lifespans, aiming to validate auth at startup and ensure browser cleanup on shutdown.
Changes:
- Replace the local
asynccontextmanagerlifespan withfastmcp.server.lifespan.lifespan-decorated lifespans. - Add
auth_lifespanto validate an authentication profile exists at startup. - Compose
auth_lifespanandbrowser_lifespanviaauth_lifespan | browser_lifespanwhen constructingFastMCP.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Use lowercase dict instead of Dict, add auth validation log line
|
@greptileai review |
| @lifespan | ||
| async def auth_lifespan(app: FastMCP) -> AsyncIterator[dict[str, Any]]: | ||
| """Validate authentication profile exists at startup.""" | ||
| logger.info("Validating LinkedIn authentication...") | ||
| get_authentication_source() | ||
| yield {} |
There was a problem hiding this comment.
No error logging on auth failure
If get_authentication_source() raises a CredentialsNotFoundError (or any other unexpected exception), the exception propagates out of the lifespan silently with no log line at the auth_lifespan level. Depending on how fastmcp surfaces lifespan errors, a user may see only the raw exception text with no surrounding context. Consider wrapping the call to log the failure before re-raising:
| @lifespan | |
| async def auth_lifespan(app: FastMCP) -> AsyncIterator[dict[str, Any]]: | |
| """Validate authentication profile exists at startup.""" | |
| logger.info("Validating LinkedIn authentication...") | |
| get_authentication_source() | |
| yield {} | |
| @lifespan | |
| async def auth_lifespan(app: FastMCP) -> AsyncIterator[dict[str, Any]]: | |
| """Validate authentication profile exists at startup.""" | |
| logger.info("Validating LinkedIn authentication...") | |
| try: | |
| get_authentication_source() | |
| except Exception: | |
| logger.error("LinkedIn authentication validation failed", exc_info=True) | |
| raise | |
| yield {} |
Prompt To Fix With AI
This is a comment left during a code review.
Path: linkedin_mcp_server/server.py
Line: 33-38
Comment:
**No error logging on auth failure**
If `get_authentication_source()` raises a `CredentialsNotFoundError` (or any other unexpected exception), the exception propagates out of the lifespan silently with no log line at the `auth_lifespan` level. Depending on how fastmcp surfaces lifespan errors, a user may see only the raw exception text with no surrounding context. Consider wrapping the call to log the failure before re-raising:
```suggestion
@lifespan
async def auth_lifespan(app: FastMCP) -> AsyncIterator[dict[str, Any]]:
"""Validate authentication profile exists at startup."""
logger.info("Validating LinkedIn authentication...")
try:
get_authentication_source()
except Exception:
logger.error("LinkedIn authentication validation failed", exc_info=True)
raise
yield {}
```
How can I resolve this? If you propose a fix, please make it concise.| async def auth_lifespan(app: FastMCP) -> AsyncIterator[dict[str, Any]]: | ||
| """Validate authentication profile exists at startup.""" | ||
| logger.info("Validating LinkedIn authentication...") | ||
| get_authentication_source() |
There was a problem hiding this comment.
Synchronous blocking I/O inside async lifespan
get_authentication_source() is a synchronous function that performs filesystem I/O (checking profile directory existence via os.path.exists). Calling it directly inside an async generator blocks the event loop for the duration of the filesystem call. While this is unlikely to cause practical problems at startup (one-time, fast path), it is a best-practice violation in an async context.
Consider offloading it to a thread pool:
| get_authentication_source() | |
| await asyncio.get_event_loop().run_in_executor(None, get_authentication_source) |
(Don't forget to add import asyncio at the top of the file.)
Prompt To Fix With AI
This is a comment left during a code review.
Path: linkedin_mcp_server/server.py
Line: 37
Comment:
**Synchronous blocking I/O inside async lifespan**
`get_authentication_source()` is a synchronous function that performs filesystem I/O (checking profile directory existence via `os.path.exists`). Calling it directly inside an async generator blocks the event loop for the duration of the filesystem call. While this is unlikely to cause practical problems at startup (one-time, fast path), it is a best-practice violation in an `async` context.
Consider offloading it to a thread pool:
```suggestion
await asyncio.get_event_loop().run_in_executor(None, get_authentication_source)
```
(Don't forget to add `import asyncio` at the top of the file.)
How can I resolve this? If you propose a fix, please make it concise.
Greptile Summary
This PR refactors the single monolithic
lifespancontext manager into two focused, composable lifespans —auth_lifespan(validates authentication at startup) andbrowser_lifespan(manages browser setup and teardown) — composed via fastmcp's|operator asauth_lifespan | browser_lifespan. It also cleans up the legacyDictimport in favour of the built-indict.Key changes:
@asynccontextmanagerin favour of fastmcp's@lifespandecorator, aligning with the framework's native composition API.auth_lifespanprovides a fail-fast authentication check before the browser is ever started; if credentials are missing, startup aborts without initialising any browser resources.browser_lifespanretains the original startup/shutdown log messages and callsclose_browser()on teardown.|composition), and sinceauth_lifespanhas no cleanup code this is benign.auth_lifespandoes not log whenget_authentication_source()raises an exception (making failure diagnosis harder), and the synchronous filesystem call inside the async generator could briefly block the event loop on startup.Confidence Score: 4/5
auth_lifespancorrectly fails fast before browser resources are allocated, andbrowser_lifespanpreserves existing teardown behaviour. The two style concerns (no error log on auth failure, sync I/O in async context) do not affect correctness or safety in practice, only observability and strict async hygiene.Important Files Changed
auth_lifespanandbrowser_lifespan) using fastmcp's `Sequence Diagram
sequenceDiagram participant F as FastMCP Framework participant AL as auth_lifespan participant BL as browser_lifespan participant AS as get_authentication_source() participant CB as close_browser() Note over F,CB: Startup Phase F->>AL: enter lifespan AL->>AL: log "Validating LinkedIn authentication..." AL->>AS: get_authentication_source() alt credentials missing AS-->>AL: raise CredentialsNotFoundError AL-->>F: propagate exception (server aborts) else credentials found AS-->>AL: return True AL->>AL: yield {} (startup complete) F->>BL: enter lifespan BL->>BL: log "LinkedIn MCP Server starting..." BL->>BL: yield {} (startup complete) end Note over F,CB: Server Running... Note over F,CB: Shutdown Phase F->>BL: resume after yield (shutdown) BL->>BL: log "LinkedIn MCP Server shutting down..." BL->>CB: close_browser() CB-->>BL: done F->>AL: resume after yield (shutdown) Note over AL: no cleanup neededLast reviewed commit: 64ac267