✨(realtime) add SSE realtime relay + publisher#720
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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: 19
🤖 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 @.github/workflows/messages-ghcr.yml:
- Line 57: In the messages-ghcr.yml workflow file, replace the `secrets:
inherit` line with an explicit secrets mapping that only passes the GITHUB_TOKEN
secret to the reusable workflow docker-publish.yml. This is a security best
practice that restricts access to only the specific secret (GITHUB_TOKEN)
required for container registry authentication, rather than exposing all secrets
from the caller workflow to the reusable workflow.
- Around line 89-91: The BUILD_DATE variable assignment in the build_args
section directly accesses github.event.head_commit.timestamp without verifying
that head_commit exists, which causes blank values in push events like branch
deletions. Add a conditional guard using a GitHub Actions expression to check if
head_commit is populated before accessing its timestamp property, and provide a
fallback value (such as an empty string or the current date) if head_commit is
not available.
In @.github/workflows/messages.yml:
- Around line 46-47: The checkout action uses a mutable version tag
(actions/checkout@v6) without disabling credential persistence, creating
supply-chain and security risks. Replace the mutable `@v6` tag with a specific
commit SHA to pin the exact version, and add the persist-credentials option set
to false to prevent GitHub tokens from being persisted to disk during the
checkout step.
In `@bin/scalingo_run_web`:
- Line 36: The trap command on line 36 uses `${relay_child:-0}` which defaults
to 0 if relay_child is not set, and sending TERM to PID 0 signals the entire
process group instead of just the target child process, risking termination of
sibling processes. Modify the trap statement to add a conditional check that
only executes the kill command if relay_child is set to a non-zero value, such
as by wrapping the kill command in a test that verifies relay_child is not empty
or zero before attempting to terminate it.
In `@src/backend/core/api/viewsets/realtime.py`:
- Line 50: The post() method in the realtime viewset is missing a docstring,
which causes a missing-function-docstring lint error. Add a docstring to the
post() method that describes what the method does, its parameters, and its
return value. The docstring should follow the project's documentation standards
and be placed immediately after the method definition.
In `@src/backend/core/mda/inbound_tasks.py`:
- Around line 304-332: The notify_users call within the realtime notification
block can raise exceptions (Redis connection issues, misconfiguration) that
bubble up and cause the entire inbound task to fail even though the message was
already successfully delivered. Wrap the realtime notification logic (starting
from the import statement through the notify_users call) in a try-except block
that catches any exceptions, logs them appropriately using the existing logging
mechanism, and does not re-raise the exception. This ensures the fire-and-forget
behavior is preserved and realtime notification failures do not impact the
success of the main inbound message processing task.
In `@src/backend/core/services/realtime.py`:
- Around line 87-89: The json.dumps() call that serializes the event payload is
currently executing before the try block that guards _do_publish(), causing
non-serializable data to raise exceptions to the caller instead of being handled
as part of the best-effort publish behavior. Move the line containing
json.dumps({"event": event, "data": data or {}}, separators=(",", ":")) inside
the try block that wraps the _do_publish() call to ensure serialization errors
are caught and handled consistently with the documented error handling approach.
In `@src/backend/core/tests/api/test_realtime_token.py`:
- Line 25: Add docstrings to the three test functions that are failing the
missing-function-docstring lint check. For the test_requires_authentication
function (shown in the diff) and the other two test functions at lines 45 and
56, add appropriate docstrings immediately after each function definition that
describe what each test verifies. The docstrings should be enclosed in triple
quotes and provide a clear description of the test's purpose.
In `@src/backend/core/tests/test_realtime.py`:
- Line 21: Add docstrings to the four test functions that are failing the
missing-function-docstring lint check: test_mint_connection_token_roundtrip, and
the three other test functions at the locations mentioned (37, 44, and 57). Each
test function needs a docstring immediately following the function definition
that briefly describes what the test validates. This will satisfy the linting
requirement and unblock the build.
In `@src/frontend/caddy/Caddyfile`:
- Around line 58-62: The reverse_proxy matcher in the Caddyfile uses
`/realtime-relay/*` which only matches paths with at least one segment after the
slash (e.g., `/realtime-relay/something`), but does not match the bare path
`/realtime-relay` without a trailing slash. This causes requests to
`/realtime-relay` to bypass the proxy block and fall through to SPA handling.
Modify the reverse_proxy directive matcher to include both the bare path and the
wildcard pattern, ensuring all requests intended for the realtime-relay service
are properly proxied to the upstream server regardless of whether they include a
trailing slash or additional path segments.
In `@src/frontend/src/features/realtime/use-realtime.tsx`:
- Around line 55-57: In the useEffect hook where the early return occurs when
`enabled` is false, reset the `live` state to false before returning. This
ensures that if realtime was previously connected and `live` was true, it gets
properly cleared when `REALTIME_ENABLED` turns off, preventing stale state from
persisting after the connection is disabled.
In `@src/realtime/Dockerfile`:
- Around line 93-109: Add a HEALTHCHECK instruction to the runtime-prod stage to
enable failure detection for the relay container. The HEALTHCHECK should be
added after the ENV declarations and before the CMD instruction that starts the
realtime_relay service. The health check should probe the running service
(typically via an HTTP endpoint or similar mechanism) to verify the relay is
functioning properly and not hung or stuck, with appropriate intervals and
thresholds for failure detection.
In `@src/realtime/README.md`:
- Around line 8-10: Add language declarations to all fenced code blocks in the
README file to resolve markdownlint warnings. For the diagram block at line 8
and the fenced block at line 97, specify an appropriate language identifier
(such as "text" or "plaintext") after the opening triple backticks.
Additionally, ensure proper blank-line spacing around all headings and tables
throughout the file by adding blank lines before and after these elements where
markdownlint requires them, particularly in the sections mentioned at lines 12,
42, 47-48, 57, and 78.
In `@src/realtime/realtime_relay/__init__.py`:
- Line 7: The __version__ variable in realtime_relay/__init__.py is set to
"0.1.0" but does not match the version "0.7.0" declared in
src/realtime/pyproject.toml, causing version reporting inconsistencies. Update
the __version__ variable in realtime_relay/__init__.py to match the version
specified in pyproject.toml by changing it from "0.1.0" to "0.7.0" to ensure
consistent version reporting across all tooling and runtime.
In `@src/realtime/realtime_relay/config.py`:
- Around line 52-53: The jwt_secret assignment on line 52 uses os.environ.get()
with an empty string as a default, allowing the relay to start without valid
authentication credentials. Remove the default empty string from the
os.environ.get() call for REALTIME_JWT_SECRET and instead use direct dictionary
access (os.environ["REALTIME_JWT_SECRET"]) so that a KeyError is raised
immediately if the environment variable is missing, causing the application to
fail fast at startup rather than running with invalid auth state.
- Around line 55-56: The heartbeat_seconds and max_connections parameters on
lines 55-56 lack input validation, allowing negative or zero values that cause
runtime failures. Replace the simple int() conversions with explicit parsing and
bounds validation: ensure heartbeat_seconds is greater than 0 and
max_connections is greater than or equal to 0. When parsing the
REALTIME_HEARTBEAT and REALTIME_MAX_CONNECTIONS environment variables, catch any
ValueError or invalid bounds and raise a clear, descriptive error message that
indicates the valid ranges required, rather than letting opaque conversion
errors propagate.
In `@src/realtime/realtime_relay/hub.py`:
- Around line 53-55: The logger.warning call in the queue full condition is
logging every dropped event, which can cause unbounded log spam and consume
excessive CPU/I/O resources when a client stalls. Implement rate-limiting for
this warning by tracking the last time this warning was logged (using a
timestamp instance variable) and only emit the warning if a minimum time
interval (e.g., several seconds) has elapsed since the last warning. This way,
the same warning will not be repeated excessively during sustained backpressure
conditions.
In `@src/realtime/realtime_relay/tests/test_app.py`:
- Around line 157-160: In the try-except block where the task is awaited, change
the except clause from catching BaseException to catching only
asyncio.CancelledError. This will allow real ASGI/stream exceptions to propagate
while still suppressing the expected cancellation when the task is cancelled,
preventing test failures from being hidden by overly broad exception
suppression.
In `@src/realtime/realtime_relay/tests/test_hub.py`:
- Line 24: Replace the fixed `await asyncio.sleep(0.05)` call with a polling
mechanism that repeatedly checks `hub.reader_alive()` until it returns True or a
timeout is reached. Use a loop with short sleep intervals to poll the readiness
condition instead of relying on a fixed wait time, ensuring the test waits for
actual reader task readiness rather than an arbitrary delay.
🪄 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
Run ID: 4da9ac5a-ea35-4969-9766-5e7588329971
⛔ Files ignored due to path filters (8)
src/frontend/src/features/api/gen/index.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/config_retrieve200.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/index.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/realtime_token_create200.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/thread_event.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/thread_event_author.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/realtime/realtime.tsis excluded by!**/gen/**src/realtime/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (44)
.github/workflows/docker-publish.yml.github/workflows/messages-ghcr.yml.github/workflows/messages.ymlMakefilebin/scalingo_postcompilebin/scalingo_run_webcompose.yamlenv.d/development/frontend.defaultssrc/backend/Dockerfilesrc/backend/core/api/openapi.jsonsrc/backend/core/api/serializers.pysrc/backend/core/api/viewsets/config.pysrc/backend/core/api/viewsets/realtime.pysrc/backend/core/mda/dispatch_webhooks.pysrc/backend/core/mda/inbound_pipeline.pysrc/backend/core/mda/inbound_tasks.pysrc/backend/core/services/realtime.pysrc/backend/core/tests/api/test_config.pysrc/backend/core/tests/api/test_realtime_token.pysrc/backend/core/tests/test_realtime.pysrc/backend/core/urls.pysrc/backend/messages/settings.pysrc/frontend/caddy/Caddyfilesrc/frontend/src/features/api/utils.tssrc/frontend/src/features/layouts/components/main/index.tsxsrc/frontend/src/features/layouts/components/thread-view/components/thread-event/index.tsxsrc/frontend/src/features/providers/config.tsxsrc/frontend/src/features/providers/mailbox.tsxsrc/frontend/src/features/realtime/client.tssrc/frontend/src/features/realtime/use-realtime.tsxsrc/realtime/.dockerignoresrc/realtime/Dockerfilesrc/realtime/README.mdsrc/realtime/pyproject.tomlsrc/realtime/realtime_relay/__init__.pysrc/realtime/realtime_relay/__main__.pysrc/realtime/realtime_relay/app.pysrc/realtime/realtime_relay/auth.pysrc/realtime/realtime_relay/config.pysrc/realtime/realtime_relay/hub.pysrc/realtime/realtime_relay/tests/conftest.pysrc/realtime/realtime_relay/tests/test_app.pysrc/realtime/realtime_relay/tests/test_auth.pysrc/realtime/realtime_relay/tests/test_hub.py
| packages: write | ||
| attestations: write | ||
| id-token: write | ||
| secrets: inherit |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify what secrets are actually referenced by the reusable workflow.
rg -n '\bsecrets\.' .github/workflows/docker-publish.yml
sed -n '1,220p' .github/workflows/docker-publish.ymlRepository: suitenumerique/messages
Length of output: 5798
Use explicit secret mapping instead of blanket secrets: inherit.
The reusable workflow .github/workflows/docker-publish.yml only references secrets.GITHUB_TOKEN for container registry authentication. Passing all caller secrets via secrets: inherit unnecessarily exposes additional secrets to this workflow, increasing the blast radius.
Replace with:
secrets:
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
🤖 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 @.github/workflows/messages-ghcr.yml at line 57, In the messages-ghcr.yml
workflow file, replace the `secrets: inherit` line with an explicit secrets
mapping that only passes the GITHUB_TOKEN secret to the reusable workflow
docker-publish.yml. This is a security best practice that restricts access to
only the specific secret (GITHUB_TOKEN) required for container registry
authentication, rather than exposing all secrets from the caller workflow to the
reusable workflow.
Source: Linters/SAST tools
| build_args: | | ||
| BUILD_VERSION=${{ github.sha }} | ||
| BUILD_DATE=${{ github.event.head_commit.timestamp }} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect workflow triggers and confirm whether all supported events provide head_commit.
sed -n '1,180p' .github/workflows/messages-ghcr.yml
rg -n '^\s*on:|workflow_dispatch|push|pull_request|release|schedule' .github/workflows/messages-ghcr.ymlRepository: suitenumerique/messages
Length of output: 2508
🏁 Script executed:
# Get the full workflow file to check all triggers
wc -l .github/workflows/messages-ghcr.yml
cat -n .github/workflows/messages-ghcr.ymlRepository: suitenumerique/messages
Length of output: 3259
🏁 Script executed:
# Check the reusable workflow for any independent triggers
cat -n .github/workflows/docker-publish.yml | head -100Repository: suitenumerique/messages
Length of output: 4378
🌐 Web query:
GitHub Actions push event head_commit timestamp availability
💡 Result:
In GitHub Actions, the timestamp for the head commit is available within the push event payload [1][2]. You can access it using the github context via ${{ github.event.head_commit.timestamp }} [1][3]. The push event payload provided to GitHub Actions includes a head_commit object, which contains the details of the most recent commit [4][3]. This object includes a timestamp field in ISO 8601 format, representing the time the commit was created [1][2]. Note that: - The head_commit object may be null in some scenarios, such as when a branch is deleted [4]. - If you need information from earlier commits in the same push, they are available in the github.event.commits array, where each element also contains a timestamp field [4][1]. - The data in github.event corresponds directly to the webhook payload for the event that triggered the workflow [5]. If the head_commit data is insufficient for your needs, you can use the GitHub REST API or GraphQL API to retrieve complete commit details, including specific committer or author dates [4][1][6].
Citations:
- 1: https://github.com/docker/metadata-action/blob/master/src/context.ts
- 2: https://gist.github.com/EdgarOrtegaRamirez/0fe5532984481b093aadf6f517c135c8
- 3: https://stackoverflow.com/questions/63619329/how-to-get-the-commit-message-in-github-actions
- 4: https://docs.github.com/en/webhooks/webhook-events-and-payloads?actionType=submitted
- 5: https://github.com/github/docs/blob/main/content/actions/reference/workflows-and-actions/contexts.md
- 6: https://docs.github.com/en/actions/reference/workflows-and-actions/events-that-trigger-workflows
Guard BUILD_DATE for push events without head_commit.
Line 91 reads github.event.head_commit.timestamp, which is not populated in some push event scenarios (for example, branch deletion). This can silently produce blank build metadata.
🤖 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 @.github/workflows/messages-ghcr.yml around lines 89 - 91, The BUILD_DATE
variable assignment in the build_args section directly accesses
github.event.head_commit.timestamp without verifying that head_commit exists,
which causes blank values in push events like branch deletions. Add a
conditional guard using a GitHub Actions expression to check if head_commit is
populated before accessing its timestamp property, and provide a fallback value
(such as an empty string or the current date) if head_commit is not available.
| - name: Checkout repository | ||
| uses: actions/checkout@v6 |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify unpinned checkout usage and missing persist-credentials in this workflow.
rg -n 'uses:\s*actions/checkout@' .github/workflows/messages.yml
rg -n 'persist-credentials' .github/workflows/messages.ymlRepository: suitenumerique/messages
Length of output: 497
🏁 Script executed:
sed -n '40,55n' .github/workflows/messages.ymlRepository: suitenumerique/messages
Length of output: 161
🏁 Script executed:
cat -n .github/workflows/messages.yml | sed -n '40,55p'Repository: suitenumerique/messages
Length of output: 616
Pin checkout to commit SHA and disable credential persistence.
Line 47 uses a mutable tag (actions/checkout@v6) without disabling credential persistence, increasing supply-chain and token-exposure risk. Pin to a full commit SHA and set persist-credentials: false.
Suggested change
- name: Checkout repository
- uses: actions/checkout@v6
+ uses: actions/checkout@<full_commit_sha>
+ with:
+ persist-credentials: false📝 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.
| - name: Checkout repository | |
| uses: actions/checkout@v6 | |
| - name: Checkout repository | |
| uses: actions/checkout@692973e3563c7f3b3dba3e49cccf11b3cee42541 | |
| with: | |
| persist-credentials: false |
🧰 Tools
🪛 zizmor (1.26.1)
[warning] 46-47: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[error] 47-47: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 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 @.github/workflows/messages.yml around lines 46 - 47, The checkout action
uses a mutable version tag (actions/checkout@v6) without disabling credential
persistence, creating supply-chain and security risks. Replace the mutable `@v6`
tag with a specific commit SHA to pin the exact version, and add the
persist-credentials option set to false to prevent GitHub tokens from being
persisted to disk during the checkout step.
Source: Linters/SAST tools
| # Run uvicorn as a tracked child and forward SIGTERM to it, then stop the | ||
| # loop — otherwise the parent's `pkill -P $$` only reaches this subshell, | ||
| # leaving uvicorn (a grandchild) orphaned and ungracefully drained. | ||
| trap 'kill -TERM "${relay_child:-0}" 2>/dev/null; exit 0' TERM |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Avoid kill -TERM 0 in the relay shutdown trap.
On Line 36, ${relay_child:-0} can evaluate to 0 before the first child PID is assigned; kill 0 signals the whole process group, which can terminate sibling processes unexpectedly.
Proposed change
- trap 'kill -TERM "${relay_child:-0}" 2>/dev/null; exit 0' TERM
+ trap 'if [ -n "${relay_child:-}" ]; then kill -TERM "${relay_child}" 2>/dev/null; fi; exit 0' TERM📝 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.
| trap 'kill -TERM "${relay_child:-0}" 2>/dev/null; exit 0' TERM | |
| trap 'if [ -n "${relay_child:-}" ]; then kill -TERM "${relay_child}" 2>/dev/null; fi; exit 0' TERM |
🤖 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 `@bin/scalingo_run_web` at line 36, The trap command on line 36 uses
`${relay_child:-0}` which defaults to 0 if relay_child is not set, and sending
TERM to PID 0 signals the entire process group instead of just the target child
process, risking termination of sibling processes. Modify the trap statement to
add a conditional check that only executes the kill command if relay_child is
set to a non-zero value, such as by wrapping the kill command in a test that
verifies relay_child is not empty or zero before attempting to terminate it.
| ), | ||
| }, | ||
| ) | ||
| def post(self, request): |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add a post() docstring to fix lint failure.
Line 50 triggers missing-function-docstring in CI.
Suggested fix
)
def post(self, request):
+ """Return a short-lived realtime token for the authenticated user."""
# Realtime off is a normal answer, not an error: return a 200 with a null
# token so the client cleanly falls back to polling (and can re-check
# later) instead of treating it as a failure to retry. This is also the📝 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.
| def post(self, request): | |
| def post(self, request): | |
| """Return a short-lived realtime token for the authenticated user.""" |
🧰 Tools
🪛 GitHub Actions: Lint and tests / 8_lint-back.txt
[error] 50-50: pylint (missing-function-docstring) C0116: Missing function or method docstring
🪛 GitHub Actions: Lint and tests / lint-back
[error] 50-50: pylint: C0116 Missing function or method docstring (missing-function-docstring)
🤖 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/backend/core/api/viewsets/realtime.py` at line 50, The post() method in
the realtime viewset is missing a docstring, which causes a
missing-function-docstring lint error. Add a docstring to the post() method that
describes what the method does, its parameters, and its return value. The
docstring should follow the project's documentation standards and be placed
immediately after the method definition.
Source: Pipeline failures
| jwt_secret=os.environ.get("REALTIME_JWT_SECRET", ""), | ||
| jwt_algorithm=os.environ.get("REALTIME_JWT_ALGORITHM", "HS256"), |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Fail fast when REALTIME_JWT_SECRET is missing.
Line 52 defaults jwt_secret to an empty string, which allows the relay to boot in an invalid auth state. Treat this as required config and fail at startup.
Suggested fix
`@classmethod`
def from_env(cls) -> "Config":
raw_origins = os.environ.get("REALTIME_CORS_ORIGINS", "")
+ jwt_secret = os.environ.get("REALTIME_JWT_SECRET", "").strip()
+ if not jwt_secret:
+ raise RuntimeError("REALTIME_JWT_SECRET must be set")
return cls(
redis_url=os.environ.get("REDIS_URL", "redis://redis:6379"),
- jwt_secret=os.environ.get("REALTIME_JWT_SECRET", ""),
+ jwt_secret=jwt_secret,
jwt_algorithm=os.environ.get("REALTIME_JWT_ALGORITHM", "HS256"),🤖 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/realtime/realtime_relay/config.py` around lines 52 - 53, The jwt_secret
assignment on line 52 uses os.environ.get() with an empty string as a default,
allowing the relay to start without valid authentication credentials. Remove the
default empty string from the os.environ.get() call for REALTIME_JWT_SECRET and
instead use direct dictionary access (os.environ["REALTIME_JWT_SECRET"]) so that
a KeyError is raised immediately if the environment variable is missing, causing
the application to fail fast at startup rather than running with invalid auth
state.
| heartbeat_seconds=int(os.environ.get("REALTIME_HEARTBEAT", "15")), | ||
| max_connections=int(os.environ.get("REALTIME_MAX_CONNECTIONS", "0")), |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Validate numeric env bounds for heartbeat and connection cap.
Lines 55-56 accept any integer; invalid values crash with opaque ValueError, and negative caps can force permanent 503 behavior. Add explicit parsing + bounds (heartbeat > 0, max_connections >= 0).
Suggested fix
`@classmethod`
def from_env(cls) -> "Config":
raw_origins = os.environ.get("REALTIME_CORS_ORIGINS", "")
+ try:
+ heartbeat_seconds = int(os.environ.get("REALTIME_HEARTBEAT", "15"))
+ max_connections = int(os.environ.get("REALTIME_MAX_CONNECTIONS", "0"))
+ except ValueError as exc:
+ raise RuntimeError("REALTIME_HEARTBEAT and REALTIME_MAX_CONNECTIONS must be integers") from exc
+ if heartbeat_seconds <= 0:
+ raise RuntimeError("REALTIME_HEARTBEAT must be > 0")
+ if max_connections < 0:
+ raise RuntimeError("REALTIME_MAX_CONNECTIONS must be >= 0")
return cls(
@@
- heartbeat_seconds=int(os.environ.get("REALTIME_HEARTBEAT", "15")),
- max_connections=int(os.environ.get("REALTIME_MAX_CONNECTIONS", "0")),
+ heartbeat_seconds=heartbeat_seconds,
+ max_connections=max_connections,
cors_origins=tuple(o.strip() for o in raw_origins.split(",") if o.strip()),
)🤖 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/realtime/realtime_relay/config.py` around lines 55 - 56, The
heartbeat_seconds and max_connections parameters on lines 55-56 lack input
validation, allowing negative or zero values that cause runtime failures.
Replace the simple int() conversions with explicit parsing and bounds
validation: ensure heartbeat_seconds is greater than 0 and max_connections is
greater than or equal to 0. When parsing the REALTIME_HEARTBEAT and
REALTIME_MAX_CONNECTIONS environment variables, catch any ValueError or invalid
bounds and raise a clear, descriptive error message that indicates the valid
ranges required, rather than letting opaque conversion errors propagate.
| logger.warning( | ||
| "subscriber queue full; dropping event (rooms=%s)", self.rooms | ||
| ) |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Rate-limit drop logging to prevent log amplification under backpressure.
Line 53 logs every dropped event. If a client stalls, this path can emit unbounded warnings and add avoidable CPU/log I/O load to the relay process.
Suggested change
class Subscription:
@@
def __init__(self, hub: "Hub", rooms: tuple[str, ...]):
self._hub = hub
self.rooms = rooms
self.queue: asyncio.Queue[dict[str, Any]] = asyncio.Queue(_QUEUE_MAXSIZE)
self.dropped = 0
+ self._last_drop_log_at = 0.0
@@
def deliver(self, event: dict[str, Any]) -> None:
try:
self.queue.put_nowait(event)
except asyncio.QueueFull:
self.dropped += 1
- logger.warning(
- "subscriber queue full; dropping event (rooms=%s)", self.rooms
- )
+ now = time.monotonic()
+ if self.dropped == 1 or now - self._last_drop_log_at >= 30:
+ logger.warning(
+ "subscriber queue full; dropping events (rooms=%s, dropped=%s)",
+ self.rooms,
+ self.dropped,
+ )
+ self._last_drop_log_at = now🤖 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/realtime/realtime_relay/hub.py` around lines 53 - 55, The logger.warning
call in the queue full condition is logging every dropped event, which can cause
unbounded log spam and consume excessive CPU/I/O resources when a client stalls.
Implement rate-limiting for this warning by tracking the last time this warning
was logged (using a timestamp instance variable) and only emit the warning if a
minimum time interval (e.g., several seconds) has elapsed since the last
warning. This way, the same warning will not be repeated excessively during
sustained backpressure conditions.
| try: | ||
| await task | ||
| except BaseException: | ||
| pass |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Don’t suppress non-cancellation failures from the SSE task.
Lines 159-160 catch BaseException, which hides real ASGI/stream exceptions and can let tests pass while the app actually failed. Suppress only asyncio.CancelledError.
Suggested fix
finally:
release.set()
task.cancel()
try:
await task
- except BaseException:
+ except asyncio.CancelledError:
pass
return "".join(chunks)🤖 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/realtime/realtime_relay/tests/test_app.py` around lines 157 - 160, In the
try-except block where the task is awaited, change the except clause from
catching BaseException to catching only asyncio.CancelledError. This will allow
real ASGI/stream exceptions to propagate while still suppressing the expected
cancellation when the task is cancelled, preventing test failures from being
hidden by overly broad exception suppression.
| "redis://fake", channel_prefix=CHANNEL_PREFIX, redis_factory=redis_factory | ||
| ) | ||
| await hub.start() | ||
| await asyncio.sleep(0.05) # let the reader task spin up |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Replace fixed startup sleep with condition-based readiness wait.
Line 24 uses await asyncio.sleep(0.05), which can be flaky under slower CI or load. Poll hub.reader_alive() with a timeout instead.
Suggested change
`@pytest_asyncio.fixture`
async def running_hub(redis_factory):
@@
await hub.start()
- await asyncio.sleep(0.05) # let the reader task spin up
+ deadline = asyncio.get_running_loop().time() + 2.0
+ while not hub.reader_alive():
+ if asyncio.get_running_loop().time() >= deadline:
+ raise TimeoutError("Hub reader did not become ready")
+ await asyncio.sleep(0.01)
try:
yield hub📝 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.
| await asyncio.sleep(0.05) # let the reader task spin up | |
| await hub.start() | |
| deadline = asyncio.get_running_loop().time() + 2.0 | |
| while not hub.reader_alive(): | |
| if asyncio.get_running_loop().time() >= deadline: | |
| raise TimeoutError("Hub reader did not become ready") | |
| await asyncio.sleep(0.01) | |
| try: | |
| yield hub |
🤖 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/realtime/realtime_relay/tests/test_hub.py` at line 24, Replace the fixed
`await asyncio.sleep(0.05)` call with a polling mechanism that repeatedly checks
`hub.reader_alive()` until it returns True or a timeout is reached. Use a loop
with short sleep intervals to poll the readiness condition instead of relying on
a fixed wait time, ensuring the test waits for actual reader task readiness
rather than an arbitrary delay.
sylvinus
force-pushed
the
outbound_webhooks
branch
from
9331775 to
27222a7
Compare
1 participant
Fixes #215
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests