feat: Improve OAuth error handling and frontend sync behavior #10626
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughIntroduces MCP Composer orchestration with rollback safety, port management, and error handling across backend and frontend. Backend adds composer start/stop orchestration with auth settings rollback on failure, per-project error tracking, and robust process management with retry logic. Frontend adds OAuth auto-sync, per-project cache scoping, and loading state tracking for composer startup. Changes
Sequence DiagramsequenceDiagram
participant Client as Client/API
participant MCP_API as mcp_projects.py
participant Auth_DB as Auth Settings (DB)
participant Composer as MCP Composer Service
participant Process as OS Process
Client->>MCP_API: PATCH auth settings
MCP_API->>Auth_DB: Load original_auth_settings
Auth_DB-->>MCP_API: original_auth_settings
MCP_API->>Composer: start_project_composer(max_retries=3)
Note over Composer: Retry Loop (max_retries)
Composer->>Composer: _ensure_port_available()
alt Port Conflict
Composer->>Process: Kill existing process
Process-->>Composer: Killed
else Port Available
Composer->>Composer: Proceed
end
Composer->>Process: Start MCP process
alt Process Starts Successfully
Process-->>Composer: Process PID
Composer->>Composer: Register port/PID mapping
Composer-->>MCP_API: Success + URL
MCP_API->>Auth_DB: Commit auth settings
MCP_API-->>Client: 200 OK
else Process Fails
Process-->>Composer: Error
Composer->>Composer: Extract error message
Composer->>Composer: set_last_error(project_id, error_msg)
Composer-->>MCP_API: Error object
alt Max Retries Exceeded
MCP_API->>Auth_DB: Rollback to original_auth_settings
MCP_API-->>Client: 400 Error (with error details)
else Retry Available
Composer->>Process: Retry startup
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touchesImportant Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 3 warnings)
✅ Passed checks (3 passed)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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 |
Frontend Unit Test Coverage ReportCoverage Summary
Unit Test Results
|
Codecov Report❌ Patch coverage is ❌ Your project status has failed because the head coverage (39.97%) is below the target coverage (60.00%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #10626 +/- ##
==========================================
+ Coverage 31.51% 32.15% +0.64%
==========================================
Files 1364 1364
Lines 62085 62474 +389
Branches 9180 9249 +69
==========================================
+ Hits 19564 20090 +526
+ Misses 41604 41372 -232
- Partials 917 1012 +95
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
github-actions
bot
added
enhancement
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/lfx/src/lfx/services/mcp_composer/service.py (1)
74-98: Persistlast_errorfor all MCPComposer startup failures, not just port/startup retriesThe per-project
last_errortracking is a good addition, but currently it’s only updated in a few paths:
- When
_ensure_port_availableraisesMCPComposerPortError, you storeself._last_errors[project_id] = e.message.- When all retries in
_do_start_project_composerfail, you storeself._last_errors[project_id] = last_error.message.- On success you call
self.clear_last_error(project_id).However, several early failure paths in
_do_start_project_composerraiseMCPComposerConfigError(no auth settings, invalid/missing port, missing host, or_validate_oauth_settingsfailures) before any of those assignments, soget_last_errorwill remain unset or stale for those cases. That undermines the “persist MCP Composer service errors per project for UI display” goal.To make error reporting consistent, consider centralizing this in
start_project_composer:
- Wrap the
await self._do_start_project_composer(...)call in atry/except MCPComposerErrorblock.- In the
except, callself.set_last_error(project_id, e.message)and re-raise.- Inside
_do_start_project_composer, useset_last_errorrather than assigningself._last_errors[...]directly for the port/retry paths, and keepclear_last_erroron success.This way every
MCPComposerError, including configuration issues, reliably updateslast_errorfor theget_project_composer_urlendpoint to surface.Also applies to: 984-1015, 1050-1072, 1142-1157
src/frontend/src/controllers/API/queries/mcp/use-patch-flows-mcp.ts (1)
44-82: Fix options override so internal cache updates always run inusePatchFlowsMCPThe per-project mutation key and targeted invalidations look good, but the options wiring here is fragile:
const mutation = mutate(["usePatchFlowsMCP", params.project_id], patchFlowMCP, { onSuccess: (data, variables, context) => { /* internal updates */ }, onSettled: () => { /* invalidate per-project flows */ }, ...options, });Because
...optionscomes last, any caller that passesoptions.onSuccessoroptions.onSettledwill completely replace the internal handlers, so:
auth_settingswon't be updated in theuseGetFlowsMCPcache.project-composer-urland the per-projectuseGetFlowsMCPqueries won't be invalidated.
usePatchInstallMCPuses the safer pattern (...optionsfirst, then anonSuccessthat also callsoptions?.onSuccess), which is what we want here too.A minimal fix:
- const mutation: UseMutationResult< - PatchFlowMCPResponse, - any, - PatchFlowMCPRequest - > = mutate(["usePatchFlowsMCP", params.project_id], patchFlowMCP, { - onSuccess: (data, variables, context) => { + const mutation: UseMutationResult< + PatchFlowMCPResponse, + any, + PatchFlowMCPRequest + > = mutate(["usePatchFlowsMCP", params.project_id], patchFlowMCP, { + ...options, + onSuccess: (data, variables, context) => { const authSettings = (variables as PatchFlowMCPRequest).auth_settings; const currentMCPData = queryClient.getQueryData([ "useGetFlowsMCP", params.project_id, ]); if (currentMCPData && authSettings !== undefined) { queryClient.setQueryData(["useGetFlowsMCP", params.project_id], { ...currentMCPData, auth_settings: authSettings, }); } queryClient.invalidateQueries({ queryKey: ["project-composer-url", params.project_id], }); - if (options?.onSuccess) { - options.onSuccess(data, variables, context); - } + options?.onSuccess?.(data, variables, context); }, - onSettled: () => { - queryClient.invalidateQueries({ - queryKey: ["useGetFlowsMCP", params.project_id], - }); - }, - ...options, + onSettled: (data, error, variables, context) => { + queryClient.invalidateQueries({ + queryKey: ["useGetFlowsMCP", params.project_id], + }); + options?.onSettled?.(data, error, variables, context); + }, });This ensures the internal cache updates/invalidation always run while still honoring any callbacks provided via
options.
🧹 Nitpick comments (10)
src/lfx/tests/unit/custom/custom_component/test_component.py (2)
117-117: Consider moving the import to module level.While functionally correct, importing
AsyncMockinside the function is unconventional. Consider moving it to the top of the file alongside the otherunittest.mockimports for better code organization.Apply this diff to move the import:
from typing import Any -from unittest.mock import MagicMock +from unittest.mock import AsyncMock, MagicMock import pytestAnd remove the import from inside the function:
async def test_send_message_without_database(): - from unittest.mock import AsyncMock - component = Component()
132-140: Consider adding assertions to verify the mock behavior.The test could be strengthened by:
- Asserting that
result.idis set to verify the mock worked correctly- Using
assert_called_once_with(message)instead of justassert_called_once()to verify the correct argument was passedApply this diff to add the assertions:
result = await component.send_message(message) assert isinstance(result, Message) assert result.text == "Hello" assert result.sender == "User" assert result.sender_name == "Test" + assert result.id == "test-message-id" # Verify the message was stored (mock was called) - component._store_message.assert_called_once() + component._store_message.assert_called_once_with(message) # The focus is on testing the message handling logic, not the database persistence layer assert event_manager.on_message.calledsrc/frontend/src/modals/authModal/index.tsx (1)
81-101: OAuth auto-sync logic looks correct; consider clearing URLs when port is emptiedThe
handleAuthFieldChangeauto-sync foroauthHost/oauthPortcorrectly derivesoauthServerUrlandoauthCallbackPathand aligns with the new tests and desired re-sync behavior when host/port change.One minor UX edge case: when the user clears the Port field (empty string), the existing
oauthServerUrl/oauthCallbackPathare left pointing at the old port. Consider clearing or updating those fields whenportbecomes empty so the form doesn’t display a now-invalid URL.src/backend/base/langflow/api/v1/mcp_projects.py (2)
411-424: Auth rollback + composer orchestration mostly solid; consider clarifying behavior and rollback intentThe new flow around
original_auth_settings,should_handle_mcp_composer, and the commit-at-end pattern makes sense and aligns with the PR goals:
- Expected
MCPComposerErrorcases persist auth settings and surface an error payload while not rolling back, which lets the UI show composer failures without losing OAuth config.- Unexpected exceptions roll back via
project.auth_settings = original_auth_settingsand a raisedHTTPException, so the session context should abort without committing changes.A couple of tweaks worth considering:
Clarify commit semantics vs. comment
The comment
# Only commit if composer started successfully (or wasn't needed)is slightly misleading: in theMCPComposerErrorbranch you intentionally do commit auth changes even when composer fails, to satisfy “persist settings, show error” behavior.Consider updating the comment to reflect the actual intent, e.g.:
# Commit project/auth changes unless an unexpected error occurred # (composer startup failures are persisted so the UI can show errors)Make rollback intent explicit
You currently snapshot
original_auth_settings = project.auth_settingsand then on unexpected errors do:project.auth_settings = original_auth_settings raise HTTPException(...)Given the surrounding
session_scope, the transaction will be rolled back anyway, but this assignment may give a false sense of “guaranteed rollback” ifhandle_auth_settings_updatemutates the dict in place.If you genuinely need an in-process rollback before the session context rolls back, consider copying the structure:
import copy original_auth_settings = copy.deepcopy(project.auth_settings) if project.auth_settings else NoneOtherwise, you could drop the manual assignment and rely purely on the transaction rollback for clarity.
Also applies to: 447-524
777-790: Per-project composerlast_errorexposure works; confirm whether errors should be one-shot or persistentThe new behavior in
get_project_composer_urlto:
- Return a stored
last_errorwhenshould_use_mcp_composer(project)isFalse, and- Clear
last_erroronly after a successful composer URL retrievalis coherent with the idea of “remember the last OAuth failure even if composer is currently not usable”.
Two points to double-check:
Lifetime of
last_errorwhen composer is permanently disabledIf MCP Composer is turned off globally after a failure,
should_use_mcp_composerwill remainFalseandlast_errorwill never be cleared by this endpoint. That’s probably fine, but consider whether the UI expects this to behave like a one-shot error (cleared after displaying once) versus a persistent banner until the next successful OAuth/composer run.Symmetry with other paths that clear
last_errorYou clear
last_errorexplicitly on:
- successful start in the PATCH handler, and
- explicit OAuth disable (stop-composer branch).
Confirm that these three places (PATCH success, OAuth disable, composer-url success) cover all the scenarios you care about, and that you don’t need to clear
last_errorwhen auth type changes away from OAuth through other flows.If the current behavior is intentional, this is good; if not, adding an explicit clear (or documenting persistence) would avoid surprises later.
Also applies to: 804-808
src/frontend/src/pages/MainPage/pages/homePage/hooks/useMcpServer.ts (1)
1-2: Confirm thatuseQueryClientshares the same QueryClient used insideUseRequestProcessorYou now call
useQueryClient()directly and use it toremoveQueriesfor["project-composer-url", projectId], whileusePatchFlowsMCPinternally gets its ownqueryClientfromUseRequestProcessor.If
UseRequestProcessoris also usinguseQueryClientunder the hood, this is fine. If it creates its own QueryClient instance, you’d end up clearing queries on a different client than the one the mutation is invalidating, which would be confusing and could leave stale data around.Please double-check that:
UseRequestProcessorand this hook are wired to the same React Query client (i.e., they both operate within the sameQueryClientProviderand use the same underlying instance).- The query key you remove (
["project-composer-url", projectId]) exactly matches the one used inuseGetProjectComposerUrl.If they are aligned, this pattern is good; if not, consider exposing
queryClientfromUseRequestProcessorand reusing that instead of callinguseQueryClienthere.Also applies to: 48-48, 209-221
src/lfx/tests/unit/services/settings/test_mcp_composer.py (2)
21-38: Port-availability tests may be slightly flaky due to hard-coded portsUsing fixed high ports (59999, 59998) is usually fine but can still collide with other processes on CI, causing intermittent failures.
Consider making the “free port” test more robust by letting the OS allocate a port and then testing it after closing the socket, e.g.:
with socket.socket(socket.AF_INET, socket.SOCK_STREAM) as s: s.bind(("localhost", 0)) free_port = s.getsockname()[1] assert mcp_service._is_port_available(free_port) is TrueThis reduces the chance of port collisions across environments.
127-176: Auth-config change detection tests align with_has_auth_config_changedbehaviorThese tests cover the main branches of
_has_auth_config_changed(port changes, identical configs, auth_type change, both/oneNone) and match the documented implementation that compares OAuth-prefixed keys and relevant fields.You might later add a case for subtle normalization differences (e.g., whitespace or int vs. str for ports) to exercise
_normalize_config_value, but the current set already exercises the core logic well.src/lfx/tests/unit/services/settings/test_mcp_composer_windows.py (2)
182-374: Windows temp-file and non-Windows pipe handling tests are thorough and aligned with the service designThe temp-file tests cover:
- Windows path:
NamedTemporaryFileusage and ensuringsubprocess.Popenreceives real file handles instead ofPIPE.- Async reading and cleanup via
_read_process_output_and_extract_error, verifying both contents and that temp files are removed.- Successful-start path cleaning up temp files after
_start_project_composer_processreturns.- Non-Windows path: asserting that stdout/stderr use
subprocess.PIPEwhenplatform.system()is not"Windows".These tests exercise both success and failure/cleanup paths and match the Windows-specific design in the service snippets. The use of real temp files for the read/cleanup tests is appropriate and keeps behavior close to production.
473-523: Retry robustness test nicely covers zombie-cleanup failures
test_zombie_cleanup_failure_is_non_fatal_during_retrysets up:
- A first
_start_project_composer_processattempt that raisesMCPComposerStartupError._kill_zombie_mcp_processesraising an exception.- A second attempt that succeeds.
Asserting that
call_count == 2and thatproject_idis present inproject_composersconfirms that:
- Zombie cleanup errors do not abort retries.
- Success on a later attempt still leads to proper tracking.
The async side-effect function for
_start_project_composer_processis a good pattern here.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
src/backend/base/langflow/api/v1/mcp_projects.py(7 hunks)src/backend/base/langflow/main.py(1 hunks)src/frontend/src/controllers/API/queries/mcp/use-patch-flows-mcp.ts(2 hunks)src/frontend/src/controllers/API/queries/mcp/use-patch-install-mcp.ts(1 hunks)src/frontend/src/modals/authModal/__tests__/AuthModal.test.tsx(1 hunks)src/frontend/src/modals/authModal/index.tsx(1 hunks)src/frontend/src/pages/MainPage/pages/homePage/hooks/useMcpServer.ts(6 hunks)src/lfx/src/lfx/services/mcp_composer/service.py(13 hunks)src/lfx/tests/unit/custom/custom_component/test_component.py(1 hunks)src/lfx/tests/unit/services/settings/test_mcp_composer.py(1 hunks)src/lfx/tests/unit/services/settings/test_mcp_composer_windows.py(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-05T22:51:27.961Z
Learnt from: edwinjosechittilappilly
Repo: langflow-ai/langflow PR: 0
File: :0-0
Timestamp: 2025-08-05T22:51:27.961Z
Learning: The TestComposioComponentAuth test in src/backend/tests/unit/components/bundles/composio/test_base_composio.py demonstrates proper integration testing patterns for external API components, including real API calls with mocking for OAuth completion, comprehensive resource cleanup, and proper environment variable handling with pytest.skip() fallbacks.
Applied to files:
src/lfx/tests/unit/services/settings/test_mcp_composer.pysrc/frontend/src/modals/authModal/__tests__/AuthModal.test.tsxsrc/lfx/tests/unit/custom/custom_component/test_component.py
📚 Learning: 2025-06-23T12:46:42.048Z
Learnt from: CR
Repo: langflow-ai/langflow PR: 0
File: .cursor/rules/frontend_development.mdc:0-0
Timestamp: 2025-06-23T12:46:42.048Z
Learning: Custom React Flow node types should be implemented as memoized components, using Handle components for connection points and supporting optional icons and labels.
Applied to files:
src/frontend/src/pages/MainPage/pages/homePage/hooks/useMcpServer.ts
🧬 Code graph analysis (7)
src/lfx/tests/unit/services/settings/test_mcp_composer.py (1)
src/lfx/src/lfx/services/mcp_composer/service.py (5)
MCPComposerPortError(38-39)_is_port_available(99-115)_kill_process_on_port(117-238)_has_auth_config_changed(793-824)_do_start_project_composer(962-1156)
src/frontend/src/modals/authModal/__tests__/AuthModal.test.tsx (1)
src/frontend/src/components/common/genericIconComponent/index.tsx (1)
render(157-159)
src/frontend/src/pages/MainPage/pages/homePage/hooks/useMcpServer.ts (3)
src/frontend/src/controllers/API/queries/mcp/use-get-flows-mcp.ts (1)
useGetFlowsMCP(13-36)src/frontend/src/controllers/API/queries/mcp/use-patch-flows-mcp.ts (1)
usePatchFlowsMCP(27-85)src/frontend/src/customization/feature-flags.ts (1)
ENABLE_MCP_COMPOSER(20-21)
src/lfx/tests/unit/custom/custom_component/test_component.py (2)
src/lfx/src/lfx/custom/custom_component/component.py (2)
_store_message(1636-1646)send_message(1578-1634)src/lfx/src/lfx/schema/message.py (1)
Message(34-299)
src/lfx/tests/unit/services/settings/test_mcp_composer_windows.py (1)
src/lfx/src/lfx/services/mcp_composer/service.py (4)
_kill_zombie_mcp_processes(240-399)_read_process_output_and_extract_error(506-579)start_project_composer(907-960)_read_stream_non_blocking(581-620)
src/lfx/src/lfx/services/mcp_composer/service.py (1)
src/backend/base/langflow/services/deps.py (1)
get_settings_service(124-137)
src/backend/base/langflow/api/v1/mcp_projects.py (3)
src/lfx/src/lfx/services/mcp_composer/service.py (5)
MCPComposerService(69-1392)clear_last_error(95-97)MCPComposerError(27-35)set_last_error(91-93)get_last_error(87-89)src/backend/base/langflow/services/deps.py (1)
get_service(32-54)src/backend/base/langflow/services/schema.py (1)
ServiceType(4-22)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: Lint Backend / Run Mypy (3.10)
- GitHub Check: Run Backend Tests / Unit Tests - Python 3.10 - Group 4
- GitHub Check: Run Backend Tests / Unit Tests - Python 3.10 - Group 1
- GitHub Check: Run Backend Tests / Unit Tests - Python 3.10 - Group 2
- GitHub Check: Run Backend Tests / Unit Tests - Python 3.10 - Group 5
- GitHub Check: Run Frontend Tests / Determine Test Suites and Shard Distribution
- GitHub Check: Run Backend Tests / LFX Tests - Python 3.10
- GitHub Check: Run Backend Tests / Unit Tests - Python 3.10 - Group 3
- GitHub Check: Run Backend Tests / Integration Tests - Python 3.10
- GitHub Check: Run Frontend Unit Tests / Frontend Jest Unit Tests
- GitHub Check: Test Starter Templates
- GitHub Check: Update Component Index
- GitHub Check: Optimize new Python code in this PR
🔇 Additional comments (6)
src/frontend/src/modals/authModal/__tests__/AuthModal.test.tsx (1)
1-275: Comprehensive and aligned test coverage for OAuth auto-syncThe test suite thoroughly covers the new OAuth auto-sync behavior (host/port, callback, manual overrides, save payload, existing settings, and non-OAuth types) and matches the component’s logic. This should give good confidence in the new flow.
src/lfx/src/lfx/services/mcp_composer/service.py (1)
99-239: Port/process ownership and startup monitoring look robust and conservativeThe new helpers for port/process handling and startup monitoring (
_is_port_available,_kill_process_on_port,_kill_zombie_mcp_processes,_is_port_used_by_another_project,_ensure_port_available,_read_process_output_and_extract_error,_read_stream_non_blocking,_start_project_composer_process) form a coherent lifecycle:
- Only kill processes when they’re explicitly associated with the current project (
_port_to_project/_pid_to_project) or, on Windows, clearly identified as MCP Composer zombies.- Avoid killing unknown external processes on occupied ports, instead surfacing clear
MCPComposerPortErrormessages.- Use temp files for stdout/stderr on Windows to avoid pipe deadlocks and clean them up in all success/error paths.
- Monitor startup with repeated port checks and non-blocking log reads, producing structured error logs with obfuscated secrets.
This is well-structured for cross-platform safety and observability; no changes needed here.
Also applies to: 240-415, 622-699, 1158-1377
src/frontend/src/controllers/API/queries/mcp/use-patch-install-mcp.ts (1)
46-58: Per-project keying and options composition look good inusePatchInstallMCPThe updated mutation key
["usePatchInstallMCP", params.project_id]and per-project invalidation of["useGetInstalledMCP", params.project_id]are correct. Spreading...optionsbefore the customonSuccesswhile still callingoptions?.onSuccesskeeps internal cache behavior intact even when callers pass their own callbacks—this is the pattern worth mirroring inusePatchFlowsMCP.src/lfx/tests/unit/services/settings/test_mcp_composer.py (1)
178-321: Port-change handling and port-ownership tests look consistent with the service orchestrationThe restart and port-ownership tests here are well-structured:
test_port_change_triggers_restartcorrectly seedsproject_composersand asserts_do_stop_project_composeris invoked when auth config (port) changes.test_port_in_use_by_own_project_triggers_killvalidates the “own project owns port” path via_port_to_projectand ensures_kill_process_on_portis called after an initial “in use” check.test_port_in_use_by_unknown_process_raises_errormatches the documented_ensure_port_availablebehavior of raisingMCPComposerPortErrorfor untracked ports with a user-facing message.Mocks use
AsyncMockfor service coroutines, and the tests focus on control-flow effects, not implementation details, which is appropriate here.src/lfx/tests/unit/services/settings/test_mcp_composer_windows.py (2)
376-439: Startup timeout and retry-parameter tests correctly pin the intended behavior
test_startup_timeout_is_80_secondsasserts the defaults onstart_project_composer(max_startup_checks=40,startup_delay=2.0), matching the intended 80s timeout.test_retry_with_increased_timeoutpatches_start_project_composer_processto always fail and then checks that each retry passes through the expectedmax_startup_checksandstartup_delayvalues, accounting for both positional and keyword argument calls.The combination gives good protection against accidental changes to startup timing semantics.
441-471: Stream-reading avoidance tests look accurate for Windows vs Unix behaviorThe tests for
_read_stream_non_blockingcorrectly reflect the implementation:
- On Windows, it should immediately return
""and never touch the stream (peek()/readline()), which is whattest_read_stream_non_blocking_returns_empty_on_windowsasserts.- On Unix-like systems, patching
select.selectand verifying thatreadline()is used and the decoded content is returned matches the documented non-blocking behavior.This gives good coverage of a subtle, platform-specific code path.
| mcp_composer_service: MCPComposerService = cast( | ||
| MCPComposerService, get_service(ServiceType.MCP_COMPOSER_SERVICE) | ||
| ) | ||
| mcp_composer_service.clear_last_error(str(project_id)) |
There was a problem hiding this comment.
Do we also need to clear this if the auth settings changed to None or API Key?
There was a problem hiding this comment.
the logic already covers all cases where OAuth is disabled.
I don't think it's necessary to change it..
|
|
||
| if (port) { | ||
| newFields.oauthServerUrl = `http://${host}:${port}`; | ||
| newFields.oauthCallbackPath = `http://${host}:${port}/auth/idaas/callback`; |
There was a problem hiding this comment.
I don't believe this is the correct behavior (though, it is cool).
Callback can be any user-defined string, no?
There was a problem hiding this comment.
agree! fixed!
now only auto-populates callback path if empty (initial setup convenience), otherwise preserves user's custom value
| ] = {} # Track active start tasks to cancel them when new request arrives | ||
| self._port_to_project: dict[int, str] = {} # Track which project is using which port | ||
| self._pid_to_project: dict[int, str] = {} # Track which PID belongs to which project | ||
| self._last_errors: dict[str, str] = {} # Track last error message per project for UI display |
There was a problem hiding this comment.
I'm starting to think we should have a fourth auth settings state: Failed, which ensures no mcp server is started for this project, and can store the last error state.
There was a problem hiding this comment.
hey @jordanrfrazier ,
We've already implemented something similar with the instant error detection improvements in this PR.
Now we can see error feedback almost in runtime.
| # Also kill any process that might be using the old port | ||
| if existing_port: | ||
| try: | ||
| await asyncio.wait_for(self._kill_process_on_port(existing_port), timeout=5.0) |
There was a problem hiding this comment.
The kill_process method is worrisome to me - I can imagine a situation where the composer started on a port then user manually stopped MCP Composer. Then, user started up another application (could be anything) on the same port.
Then, Langflow gets here and kills user's process.
Very serious issue, imo, so we should brainstorm if we have any other options here. (We also may have a similar situation with _do_stop_project_composer?)
There was a problem hiding this comment.
Good catch, but this scenario is already protected against. The code only kills processes that Langflow itself started and is actively tracking via self._port_to_project.
For the scenario you described:
- User manually stops MCP Composer
- User starts another application on the same port
- Langflow tries to restart MCP Composer
What actually happens:
- At line 728-732 in _ensure_port_available(), we detect that the port is in use by an unknown process (not in our tracking)
- We explicitly refuse to kill it for security reasons
- We raise MCPComposerPortError asking the user to choose a different port
else:
# Port is in use by unknown process - don't kill it (security concern)
await logger.aerror(
f"Port {port} is in use by an unknown process (not owned by Langflow). "
f"Will not kill external application for security reasons."
)
raise MCPComposerPortError(...)
We only kill processes when:
- The port is owned by the same project trying to restart (line 710) - safe because it's our own stuck process
- The port is in existing_port (line 1083) - which comes from our internal tracking, meaning we started it
The tracking ensures we never kill external applications. If a process isn't in self._port_to_project or self.project_composers, we treat it as external and refuse to touch it.
|
Merged #10644 in here |
github-actions
bot
added
enhancement
github-actions
bot
added
enhancement
github-actions
bot
added
enhancement
github-actions
bot
added
enhancement
github-actions
bot
added
enhancement
github-actions
bot
added
enhancement
github-actions
bot
added
enhancement
github-actions
bot
added
enhancement
github-actions
bot
added
enhancement
github-actions
bot
added
enhancement
3 participants
This pull request introduces several improvements and bug fixes to both the backend and frontend of the project, focusing on more robust handling of MCP Composer errors, improved synchronization of OAuth settings in the authentication modal, and better frontend cache management. The changes enhance user feedback during authentication issues, ensure correct UI state synchronization, and improve test coverage for OAuth field auto-sync behavior.
Backend improvements to MCP Composer error handling and commit logic:
get_project_composer_urlendpoint now returns the last error if OAuth setup previously failed, even if OAuth is currently disabled for the project. [1] [2]ResourceWarninglogs from SSE connections in the backend to reduce log clutter.Frontend improvements to OAuth authentication modal and cache management:
AuthModalnow auto-synchronizes the Server URL and Callback Path fields whenever the OAuth Host or Port fields are changed, ensuring consistent and user-friendly configuration. Manual edits are preserved unless fields are changed again.Frontend cache and query management:
useMcpServerhook adds state for tracking slow composer responses and waiting status, laying the groundwork for better user feedback during long-running operations. [1] [2] [3]Summary by CodeRabbit
New Features
Bug Fixes
Tests