Fix Socket.IO Uploads by Aligning Server/Client Paths #267
Conversation
Previously the Socket.IO server always bound to the default path (/socket.io), while the client was updated to construct a sub-path (e.g. /server/socket.io) based on mdvApiRoot. This mismatch caused upload handshake requests to fail with 500 errors. The server initialization in websocket.py now computes the socket path dynamically from MDV_API_ROOT: - Root deployments -> /socket.io - Sub-path deployments (e.g. /server/, /carroll/) -> <subpath>/socket.io This aligns the server with client behavior and restores upload functionality across both root and sub-path deployments.
WalkthroughAdds prefix-stripping WSGI middleware and earlier MDV_API_ROOT handling, changes Socket.IO server initialization to use a path derived from app.config["mdv_api_root"], and updates the client socket to allow websocket + polling transports with automatic reconnection. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Env as Environment
participant App as Flask App (wsgi_app)
participant Prefix as PrefixMiddleware
participant Config as app.config
participant WS as websocket.init (python/mdvtools/websocket.py)
participant SIO as SocketIO server
Env->>App: set MDV_API_ROOT (at import/startup)
App->>Config: app.config["mdv_api_root"]
Config-->>WS: mdv_api_root
WS->>WS: normalize mdv_api_root (ensure trailing slash unless "/")
WS->>SIO: SocketIO(app, cors_allowed_origins="*", path=mdv_api_root+"socket.io")
Note right of WS: logs computed Socket.IO path
App->>Prefix: wrap wsgi_app with PrefixMiddleware (if MDV_API_ROOT set)
Prefix->>App: strip prefix from PATH_INFO / adjust SCRIPT_NAME or return 404
sequenceDiagram
autonumber
participant Client as SocketIO client
participant Server as SocketIO server
Client->>Server: connect (transports:['websocket','polling'])
alt websocket available
Client->>Server: websocket handshake
Server-->>Client: upgrade to websocket
else websocket blocked / fails
Client->>Server: fallback to polling
end
Note right of Client: reconnection: true -> automatic retries on disconnect
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
python/mdvtools/websocket.py (2)
51-51: Return the SocketIO instance for easier testing and DI.Returning the instance enables simple assertions in tests and avoids reliance on the global.
Apply this diff:
- log(f"socketio initialized path={socketio_path} cors={allowed_origins}") + log(f"socketio initialized path={socketio_path} cors={allowed_origins}") + return socketio
44-49: Make SocketIO path resolution resilient to config casing and missing slashesFallback to app.config['MDV_API_ROOT'] and enforce both leading and trailing slashes; routes.py already normalizes MDV_API_ROOT for /api_root but defensive normalization here is safe.
File: python/mdvtools/websocket.py (lines 44–49)
- api_root = app.config.get('mdv_api_root', '/') - if api_root != '/' and not api_root.endswith('/'): - api_root += '/' - socketio_path = f"{api_root}socket.io" + api_root = app.config.get('mdv_api_root') or app.config.get('MDV_API_ROOT') or '/' + api_root = (api_root or '/').strip() + if api_root != '/': + if not api_root.startswith('/'): + api_root = '/' + api_root + if not api_root.endswith('/'): + api_root += '/' + socketio_path = f"{api_root}socket.io"
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
python/mdvtools/websocket.py(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: nawabfurquan
PR: Taylor-CCB-Group/MDV#263
File: src/charts/dialogs/SocketIOUploadClient.ts:213-219
Timestamp: 2025-09-17T13:35:04.309Z
Learning: The MDV_API_ROOT environment variable is now normalized server-side in routes.py (lines 69-72) to ensure it always starts and ends with "/", eliminating the need for client-side path sanitization in SocketIOUploadClient.
📚 Learning: 2025-09-17T13:35:04.309Z
Learnt from: nawabfurquan
PR: Taylor-CCB-Group/MDV#263
File: src/charts/dialogs/SocketIOUploadClient.ts:213-219
Timestamp: 2025-09-17T13:35:04.309Z
Learning: The MDV_API_ROOT environment variable is now normalized server-side in routes.py (lines 69-72) to ensure it always starts and ends with "/", eliminating the need for client-side path sanitization in SocketIOUploadClient.
Applied to files:
python/mdvtools/websocket.py
🧬 Code graph analysis (1)
python/mdvtools/websocket.py (2)
python/mdvtools/auth/tests/test_auth.py (1)
app(9-14)python/mdvtools/server.py (1)
log(40-46)
🔇 Additional comments (1)
python/mdvtools/websocket.py (1)
50-50: Confirm Flask‑SocketIOpathsemantics for pinned versionsSome flask-socketio / python-socketio releases differ on whether the server-side
pathaccepts a leading slash or a URL prefix (e.g.,/server/socket.io). Confirm against the exact versions pinned in this repo and cite the official docs.Location: python/mdvtools/websocket.py — line 50 (socketio = SocketIO(app, cors_allowed_origins="*", path=socketio_path))
Action: provide requirements.txt / pyproject.toml / poetry.lock or the flask-socketio and python-socketio version strings (or run
pip show flask-socketio python-socketio) so I can verify and cite the matching docs.
| socketio = SocketIO(app, cors_allowed_origins="*", path=socketio_path) | ||
| log(f"socketio initialized with cors_allowed_origins wildcard and path={socketio_path}") |
There was a problem hiding this comment.
Avoid wildcard CORS in production; make origins configurable.
Using cors_allowed_origins="" is a security posture gap. Read allowed origins from config and fall back to "" only in dev/test.
Apply this diff:
- socketio = SocketIO(app, cors_allowed_origins="*", path=socketio_path)
- log(f"socketio initialized with cors_allowed_origins wildcard and path={socketio_path}")
+ origins_cfg = app.config.get("MDV_CORS_ALLOWED_ORIGINS") or app.config.get("mdv_cors_allowed_origins")
+ if isinstance(origins_cfg, str):
+ allowed_origins = [o.strip() for o in origins_cfg.split(",") if o.strip()]
+ else:
+ allowed_origins = origins_cfg
+ if not allowed_origins:
+ allowed_origins = "*" if (app.debug or app.testing) else []
+ socketio = SocketIO(app, cors_allowed_origins=allowed_origins, path=socketio_path)
+ log(f"socketio initialized path={socketio_path} cors={allowed_origins}")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| socketio = SocketIO(app, cors_allowed_origins="*", path=socketio_path) | |
| log(f"socketio initialized with cors_allowed_origins wildcard and path={socketio_path}") | |
| origins_cfg = app.config.get("MDV_CORS_ALLOWED_ORIGINS") or app.config.get("mdv_cors_allowed_origins") | |
| if isinstance(origins_cfg, str): | |
| allowed_origins = [o.strip() for o in origins_cfg.split(",") if o.strip()] | |
| else: | |
| allowed_origins = origins_cfg | |
| if not allowed_origins: | |
| allowed_origins = "*" if (app.debug or app.testing) else [] | |
| socketio = SocketIO(app, cors_allowed_origins=allowed_origins, path=socketio_path) | |
| log(f"socketio initialized path={socketio_path} cors={allowed_origins}") |
|
PR update:
Forcing polling-only causes issues in dev environments due to CORS restrictions on HTTP requests. WebSockets are not affected by CORS in the same way and already fall back to polling if WebSocket fails. Removing WebSocket impacts dev uploads unnecessarily. Testing: confirmed uploads work under these environments with files up to 15 GB:
|
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
python/mdvtools/dbutils/safe_mdv_app.py(1 hunks)src/charts/dialogs/SocketIOUploadClient.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: nawabfurquan
PR: Taylor-CCB-Group/MDV#263
File: src/charts/dialogs/SocketIOUploadClient.ts:213-219
Timestamp: 2025-09-17T13:35:04.309Z
Learning: The MDV_API_ROOT environment variable is now normalized server-side in routes.py (lines 69-72) to ensure it always starts and ends with "/", eliminating the need for client-side path sanitization in SocketIOUploadClient.
📚 Learning: 2025-09-17T13:35:04.309Z
Learnt from: nawabfurquan
PR: Taylor-CCB-Group/MDV#263
File: src/charts/dialogs/SocketIOUploadClient.ts:213-219
Timestamp: 2025-09-17T13:35:04.309Z
Learning: The MDV_API_ROOT environment variable is now normalized server-side in routes.py (lines 69-72) to ensure it always starts and ends with "/", eliminating the need for client-side path sanitization in SocketIOUploadClient.
Applied to files:
python/mdvtools/dbutils/safe_mdv_app.py
🧬 Code graph analysis (1)
python/mdvtools/dbutils/safe_mdv_app.py (1)
python/mdvtools/socketio_upload.py (1)
initialize_socketio_upload(862-907)
⏰ 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). (1)
- GitHub Check: tsc
🔇 Additional comments (1)
src/charts/dialogs/SocketIOUploadClient.ts (1)
218-225: Websocket transport addition looks goodAdding
'websocket'ahead of'polling'with reconnection enabled gives us the desired fallback chain and matches the server-side path fix. LGTM.
| # Apply prefix middleware at module level - works for both direct run and WSGI deployment | ||
| prefix = os.environ.get('MDV_API_ROOT', '') | ||
| if prefix: | ||
| # Remove trailing slash from prefix if it exists | ||
| if prefix.endswith('/'): | ||
| prefix = prefix[:-1] | ||
|
|
||
| logger.info(f"Applying PrefixMiddleware with prefix: {prefix}") | ||
| app.wsgi_app = PrefixMiddleware(app.wsgi_app, prefix=prefix) |
There was a problem hiding this comment.
Reapply PrefixMiddleware after setting default MDV_API_ROOT
When the module loads, we capture MDV_API_ROOT and (optionally) wrap app.wsgi_app. Later, inside the __main__ block, we set the default /test/ value if the variable was absent. Because the middleware was already skipped earlier, we never re-wrap the app after this assignment. As a result, running python safe_mdv_app.py with no pre-set env var leaves the server listening on the root path while the rest of the stack believes it’s under /test/, recreating the socket path mismatch we’re trying to solve.
Please move the prefix application into a helper that can be invoked both at module load and after setting the default, e.g.:
+def apply_prefix_middleware() -> None:
+ prefix = os.environ.get('MDV_API_ROOT', '')
+ if prefix.endswith('/'):
+ prefix = prefix[:-1]
+ if prefix and not isinstance(app.wsgi_app, PrefixMiddleware):
+ logger.info(f"Applying PrefixMiddleware with prefix: {prefix}")
+ app.wsgi_app = PrefixMiddleware(app.wsgi_app, prefix=prefix)
+
+apply_prefix_middleware()
...
# Set default for local testing if not already set
if 'MDV_API_ROOT' not in os.environ:
os.environ['MDV_API_ROOT'] = '/test/'
+ apply_prefix_middleware()This keeps the import-time behavior for deployments while ensuring the local default actually takes effect.
Also applies to: 58-66
🤖 Prompt for AI Agents
In python/mdvtools/dbutils/safe_mdv_app.py around lines 37-45 and 58-66, the
PrefixMiddleware is applied at import time using MDV_API_ROOT but later a
default is set in __main__, so when MDV_API_ROOT is absent the app never gets
wrapped; refactor by extracting the prefix-detection-and-application logic into
a small helper function (e.g., apply_prefix_middleware(app)) that reads
MDV_API_ROOT, normalizes it (remove trailing slash), logs, and wraps
app.wsgi_app only when non-empty, then call that helper both at module load (to
preserve deployment behavior) and again after setting the default in the
__main__ block so the default `/test/` will be applied when running the module
directly.
|
So, now we're using the |
2 participants
This PR aims to fix the Socket.IO upload failures that were causing 500 errors when connecting. The root of the problem was a mismatch between the client and server socket paths. The client was attempting to connect using a sub-path such as /server/socket.io, while the server was only listening on the default /socket.io. This caused the upload handshake requests to hit an unhandled route, resulting in errors.
The issue was introduced in commit 78c48fa8 (“SocketIO Import and Create Project Fix (#263)”), when the serverUrl parameter was removed and replaced with a dynamically constructed socketPath based on mdvApiRoot. This change allowed the client to adapt to sub-path deployments, but the server-side initialization in websocket.py was not updated at the same time. The result was that the client and server were no longer aligned, and uploads stopped working.
The fix was to update the server configuration so that it also respects MDV_API_ROOT when setting up the Socket.IO instance. Now the server computes the socket path dynamically: it uses /socket.io when deployed at the root, and <MDV_API_ROOT>/socket.io when deployed under a sub-path such as /server/ or /carroll/. With this change, the client and server are once again in sync, and uploads succeed regardless of deployment structure.
Summary by CodeRabbit
Bug Fixes
New Features
Chores