Prefix logs with remote inference connection ID#648
Merged
emranemran merged 2 commits intomainfrom Mar 10, 2026
Merged
Conversation
Contributor
🚀 fal.ai Preview Deployment
TestingConnect to this preview deployment by running this on your branch: 🧪 E2E tests will run automatically against this deployment. |
Contributor
✅ E2E Tests passed
Test ArtifactsCheck the workflow run for screenshots. |
Signed-off-by: Max Holland <max@livepeer.org>
mjh1
force-pushed
the
mh/connection-id-logs
branch
from
9998482 to
c831e8d
Compare
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds Fal connection ID propagation for log correlation: thread-safe storage and logging filter, server endpoints to set/clear the ID, and WebSocket/cloud components that PUT the ID on connect and DELETE it on disconnect. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as WebSocket Client
participant FalApp as fal_app.py
participant Server as app.py
participant LogsConfig as logs_config.py
participant Logger as Python Logger
Client->>FalApp: Establish WebSocket
FalApp->>Server: PUT /api/v1/internal/fal-connection-id (connection_id)
Server->>LogsConfig: set_fal_connection_id(connection_id)
LogsConfig->>LogsConfig: store connection_id (thread-safe)
FalApp->>Logger: emit logs
Logger->>LogsConfig: FalConnectionFilter.filter()
LogsConfig->>Logger: inject fal_conn attribute
Logger->>Logger: format using LOG_FORMAT (includes fal_conn)
Client->>FalApp: Disconnect
FalApp->>Server: DELETE /api/v1/internal/fal-connection-id
Server->>LogsConfig: set_fal_connection_id(None)
LogsConfig->>LogsConfig: clear connection_id
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/scope/server/logs_config.py (1)
34-36: Inconsistent lock usage for thread-safe read.
set_fal_connection_idacquires_fal_connection_id_lock, butget_fal_connection_idreads the global directly without the lock. The same applies toFalConnectionFilter.filter()at line 48.In CPython, simple reference reads/writes are atomic due to the GIL, so this is safe in practice. However, the inconsistency suggests either the lock in the setter is unnecessary, or the getter should also acquire it for consistency. Consider either removing the lock entirely (since atomic reference assignment is safe) or using it consistently.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/scope/server/logs_config.py` around lines 34 - 36, The getter and filter read _fal_connection_id without using _fal_connection_id_lock while set_fal_connection_id uses the lock; make access consistent by acquiring _fal_connection_id_lock when reading: update get_fal_connection_id() to acquire the lock before returning _fal_connection_id and update FalConnectionFilter.filter() to acquire the same lock when reading _fal_connection_id (or, if you prefer to remove synchronization entirely, remove _fal_connection_id_lock and all lock usage from set_fal_connection_id and FalConnectionFilter.filter so reads/writes are plain atomic assignments).src/scope/server/app.py (1)
463-474: Consider validatingconnection_idinput.The endpoint accepts any value from the JSON body without validation. If
connection_idis missing or not a string,set_fal_connection_idwill still be called withNoneor a non-string value. While theFalConnectionFilterhandlesNonegracefully, you might want to validate the input for consistency.🛡️ Optional input validation
`@app.put`("/api/v1/internal/fal-connection-id") async def put_fal_connection_id(request: Request): """Set the fal connection ID that is injected into every log line. Called by the fal_app proxy when a new WebSocket connection is established. This is an internal endpoint not intended for external consumers. """ body = await request.json() connection_id = body.get("connection_id") + if not isinstance(connection_id, str) or not connection_id: + raise HTTPException(status_code=400, detail="connection_id must be a non-empty string") set_fal_connection_id(connection_id) logger.info("Fal connection ID set") return {"status": "ok", "connection_id": connection_id}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/scope/server/app.py` around lines 463 - 474, The put_fal_connection_id handler currently accepts any JSON value; validate that body.get("connection_id") exists and is a string before calling set_fal_connection_id: if missing or not a str, return a 400 error JSON (and log a warning) and do not call set_fal_connection_id; if valid, proceed to call set_fal_connection_id(connection_id) and log success. Reference the put_fal_connection_id function and set_fal_connection_id (and FalConnectionFilter behavior) when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/scope/server/app.py`:
- Around line 463-474: The put_fal_connection_id handler currently accepts any
JSON value; validate that body.get("connection_id") exists and is a string
before calling set_fal_connection_id: if missing or not a str, return a 400
error JSON (and log a warning) and do not call set_fal_connection_id; if valid,
proceed to call set_fal_connection_id(connection_id) and log success. Reference
the put_fal_connection_id function and set_fal_connection_id (and
FalConnectionFilter behavior) when making the change.
In `@src/scope/server/logs_config.py`:
- Around line 34-36: The getter and filter read _fal_connection_id without using
_fal_connection_id_lock while set_fal_connection_id uses the lock; make access
consistent by acquiring _fal_connection_id_lock when reading: update
get_fal_connection_id() to acquire the lock before returning _fal_connection_id
and update FalConnectionFilter.filter() to acquire the same lock when reading
_fal_connection_id (or, if you prefer to remove synchronization entirely, remove
_fal_connection_id_lock and all lock usage from set_fal_connection_id and
FalConnectionFilter.filter so reads/writes are plain atomic assignments).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 126bf92f-84f5-4e42-96be-d0104216534a
📒 Files selected for processing (4)
src/scope/cloud/fal_app.pysrc/scope/server/app.pysrc/scope/server/cloud_connection.pysrc/scope/server/logs_config.py
Signed-off-by: Max Holland <max@livepeer.org>
Contributor
Author
|
Verified: |
emranemran
approved these changes
Mar 10, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes