feat: add upstream lifecycle controls - #51
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
📝 WalkthroughWalkthroughThe PR adds configurable upstream startup, shutdown, idle, crash-recovery, restart-limit, and concurrency controls. It expands health reporting, coordinates profile restarts, tracks in-flight activity and secrets, and adds documentation, fixtures, and lifecycle tests. ChangesUpstream lifecycle controls
Estimated code review effort: 5 (Critical) | ~90+ minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant MiftahServer
participant UpstreamProcessManager
participant ProfileSessionLimiter
participant UpstreamSession
Client->>MiftahServer: request profile operation
MiftahServer->>UpstreamProcessManager: get or restart profile
UpstreamProcessManager->>ProfileSessionLimiter: acquire profile capacity
UpstreamProcessManager->>UpstreamSession: execute MCP request
UpstreamSession-->>MiftahServer: return operation result
UpstreamProcessManager->>UpstreamProcessManager: schedule idle shutdown or recovery
Possibly related issues
Possibly related PRs
Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@docs/architecture.md`:
- Line 15: Update the architecture description to state that each profile owns a
separate SDK Client and transport for each configured upstream, rather than one
client per profile. Clarify that local upstreams use StdioClientTransport, while
remote upstreams use the appropriate SSE/HTTP transport; preserve the existing
lifecycle and concurrency details.
In `@src/upstream/upstream-process-manager.ts`:
- Line 221: Fix the ESLint errors in the shutdown logic by avoiding the
intermediate array spread when mapping over profiles, and update both affected
stop-profile call sites to avoid compiling the same regular expression per
invocation. Use the relevant profile iteration and regex-related symbols in
src/upstream/upstream-process-manager.ts to apply the lint-compliant changes
consistently.
- Around line 381-391: Update the upstream process manager’s redaction flow so
manager-originated output uses a single redactProcessOutput helper backed by
both the dynamically populated secretValuesSet and options.secretValues. Replace
direct redactSecrets usage in attachStderr and apply the same helper when
normalizing capability and startup failures, ensuring resolved
environment/header credentials are removed from all output.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 83fbdaea-fc4a-465e-9b90-6318a471a604
📒 Files selected for processing (17)
README.mddocs/architecture.mddocs/config.mdsrc/cli/create-runtime.tssrc/config/schema.tssrc/config/types.tssrc/mcp/server/miftah-server.tssrc/upstream/multi-upstream-process-manager.tssrc/upstream/profile-session-limiter.tssrc/upstream/upstream-process-manager.tssrc/upstream/upstream-session.tssrc/utils/errors.tstests/config-preflight.test.tstests/config-runtime-parity.test.tstests/config-schema-contract.test.tstests/fixtures/fake-upstream.mjstests/upstream-manager.test.ts
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/upstream/upstream-process-manager.ts (1)
566-613: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winKeep shutdown best-effort on close failures
closeSessiononly special-casesUPSTREAM_SHUTDOWN_TIMEOUT; any other rejection fromentry.session.close()will short-circuitstopProfilebeforelimiter.release(...)and the"stopped"transition. On idle shutdown, that rejection also bubbles out ofvoid this.closeIdleSession(...). Treat close errors like the timeout path so finalization always runs.🤖 Prompt for 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. In `@src/upstream/upstream-process-manager.ts` around lines 566 - 613, Update closeSession to treat any rejection from entry.session.close() as a best-effort shutdown failure: force-terminate the transport, await the close promise’s settled result, and return true instead of rethrowing, while preserving the existing timeout handling. This ensures stopProfile always releases reservations and transitions to stopped, including callers such as closeIdleSession.
🤖 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/secrets/secret-resolver.ts`:
- Line 75: Hoist the inline environment-reference regex from
resolveValueWithSecretValues to module scope and reuse the shared constant when
matching values. Also hoist the global placeholder regex used by the
default-branch replace into a module-level constant, then update both call sites
to use these constants.
---
Outside diff comments:
In `@src/upstream/upstream-process-manager.ts`:
- Around line 566-613: Update closeSession to treat any rejection from
entry.session.close() as a best-effort shutdown failure: force-terminate the
transport, await the close promise’s settled result, and return true instead of
rethrowing, while preserving the existing timeout handling. This ensures
stopProfile always releases reservations and transitions to stopped, including
callers such as closeIdleSession.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 52fae6a2-db91-4d33-817b-83a508622322
📒 Files selected for processing (14)
docs/architecture.mdsrc/cli/create-runtime.tssrc/config/env-expand.tssrc/config/types.tssrc/mcp/server/miftah-server.tssrc/secrets/secret-resolver.tssrc/upstream/multi-upstream-process-manager.tssrc/upstream/profile-session-limiter.tssrc/upstream/upstream-process-manager.tssrc/upstream/upstream-session.tstests/config-preflight.test.tstests/config.test.tstests/fixtures/fake-upstream.mjstests/upstream-manager.test.ts
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
@coderabbitai review |
|
@coderabbitai run pre-merge checks |
✅ Actions performedRunning Pre-merge checks. Results will be posted in the summary comment once complete. |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/upstream/upstream-process-manager.ts`:
- Around line 609-610: Update the timeout handling in the upstream process
manager’s close/cleanup flow: after the deadline, do not await the original
close promise or unbounded terminateTransport call. Trigger forced transport
cleanup best-effort without blocking, then immediately return or finalize with
the timeout failure reason so close() and closeProfile() cannot hang. Add a
regression test using a never-resolving close promise to verify capacity and
health finalize promptly.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 6b32084c-6269-489f-a3bc-e3fd54af70e5
📒 Files selected for processing (4)
docs/config.mdsrc/secrets/secret-resolver.tssrc/upstream/upstream-process-manager.tstests/upstream-manager.test.ts
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
@coderabbitai review |
|
@coderabbitai run pre-merge checks |
✅ Action performedReview finished.
|
✅ Actions performedRunning Pre-merge checks. Results will be posted in the summary comment once complete. |
|
@coderabbitai generate docstrings |
✅ Action performedInitiated docstring generation; will generate only if new commits exist. |
|
Note Docstrings generation - SUCCESS |
Docstrings generation was requested by @mohanagy. * #51 (comment) The following files were modified: * `src/cli/create-runtime.ts` * `src/config/env-expand.ts` * `src/upstream/upstream-process-manager.ts`
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
@coderabbitai review |
|
@coderabbitai run pre-merge checks |
✅ Action performedReview finished.
|
✅ Actions performedRunning Pre-merge checks. Results will be posted in the summary comment once complete. |
|
@coderabbitai run pre-merge checks The four functions identified by the docstring generator now have complete TSDoc at current head |
✅ Actions performedRunning Pre-merge checks. Results will be posted in the summary comment once complete. |
1 participant
Summary
Validation
npm testnpm run lintnpm run typechecknpm run buildnpm run check:packRefs #12
Summary by CodeRabbit