Switch to Waitress for production server and improve event handling - #9
Merged
Conversation
Production mode uses waitress (8 threads, 300s channel timeout) instead of Flask's dev server
Falls back to app.run(debug=True) if FLASK_DEBUG=1 is set (for development)
backend/requirements.txt
Added waitress==3.0.2
Why this fixes the error
Werkzeug's dev server (with debug=True) wraps the WSGI app in DebuggedApplication, which buffers entire streaming responses in memory to catch and display errors. For SSE streams that stay open indefinitely, this caused a Content-Length to be calculated, which then mismatched the actual body when the upstream connection dropped. Waitress does not buffer streaming responses — it properly uses Transfer-Encoding: chunked for stream_with_context generators, so no Content-Length is ever set on SSE responses.
…n for granular message and session updates
|
Warning Review limit reached
More reviews will be available in 50 minutes and 57 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThis PR implements GlobalEvent envelope parsing across backend and frontend SSE streams, transitions the backend to production-grade Waitress server with proper timeouts, and adds systemd integration to deployment scripts for flexible service management. ChangesGlobalEvent Envelope Parsing and Inline SSE Reconciliation
Production Server Configuration and Proxy Setup
Systemd Deployment Integration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/app/routes/helpers.py (1)
1001-1020:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUnwrap the envelope before all event-specific parsing, not just session matching.
_event_matches_session()now understandspayload.payload, but_parse_permission_event()and_parse_question_event()still inspect the top-leveltype/properties. Wrappedpermission.*andquestion.*frames will therefore match the session and still never update the persisted pending state, so approvals/questions disappear after refresh or reconnect.🤖 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 `@backend/app/routes/helpers.py` around lines 1001 - 1020, _event_matches_session currently unwraps the GlobalEvent envelope into local variables parsed→inner→target but _parse_permission_event and _parse_question_event still read the original top-level payload/type, so wrapped frames (payload.payload) match session but are not parsed into persisted pending state; fix by unwrapping the envelope before any event-specific parsing (move the parsed→inner→target logic to the common entry point or have _parse_permission_event and _parse_question_event accept/use the already-unwrapped target), and update those functions to inspect target.get("type") and target.get("properties") (or accept the unwrapped dict argument) so permission.* and question.* frames nested under payload.payload are parsed and persisted correctly.
🤖 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 `@backend/run.py`:
- Around line 14-15: When FLASK_DEBUG is enabled the current app.run call
exposes the debugger on 0.0.0.0; change the run logic so that when
os.environ.get("FLASK_DEBUG") is truthy you run app.run(host="127.0.0.1",
port=BACKEND_PORT, debug=True, threaded=True) to bind debug to loopback, and
only allow host="0.0.0.0" when an explicit opt-in (e.g. a separate env var like
FLASK_DEBUG_BIND_ALL) is set; update the conditional around app.run (the call to
app.run in backend/run.py) to check FLASK_DEBUG_BIND_ALL before using 0.0.0.0
while keeping BACKEND_PORT and threaded=True unchanged.
- Around line 18-23: The serve(...) call currently hard-codes threads=16 and
channel_timeout=300; update it to read THREADS and CHANNEL_TIMEOUT (or similarly
named) environment variables with sensible defaults (e.g., 16 and 300), parse
them to integers, validate/fallback on invalid values, and pass those variables
into serve(...) instead of the literals; modify where serve(app, host="0.0.0.0",
port=BACKEND_PORT, threads=16, channel_timeout=300, ...) is invoked to use the
parsed env values so operators can configure concurrency and idle timeouts at
runtime.
In `@frontend/src/App.tsx`:
- Around line 2497-2531: The branch keyed by classification.hasMessageUpdate
currently reads msgPayload.properties.info directly (via
extractPayloadFromDataLines) and misses normalized direct-shape message
payloads; replace that direct access with the stream helper that normalizes both
properties.info and direct properties (e.g., call the normalized message
extractor provided in the stream helpers before using msgPayload), then use the
returned normalized message object when calling setMessages and when
building/updating ChatMessage (preserve existing checks for id/role/text/parts
and the existing update/new-message logic inside setMessages).
- Around line 1727-1755: The merge currently preserves client-side
parts/messages when the server returns fewer parts or omits a message (in the
loop over freshMessages and the follow-up loop over current), which prevents
server-side deletions (message.removed/compaction) from taking effect; change
the logic in the freshMessages merge to always treat the server (fm) as
authoritative: remove the branches that reuse existing.parts when fm.parts is
missing or shorter and always push fm (or { ...fm, parts: fm.parts }) so server
parts replace client parts; likewise remove the second loop’s behavior that
re-adds cm when freshById lacks it — do not push current messages that aren’t
present in freshById/seenIds so omitted messages are dropped, ensuring
message.removed and compaction work as intended (refer to variables
freshMessages, current, existing, mergedParts, seenIds, and freshById to locate
changes).
- Around line 2538-2555: The identifier setInvocationHeaders used in the App.tsx
update block is not declared; add a matching state setter (or use the correct
prop) so the updater works. Specifically, declare a React state hook like const
[invocationHeaders, setInvocationHeaders] = useState<YourType>(initialValue) in
the same component (and import useState from React if missing), or replace
setInvocationHeaders with the actual setter name if the state already exists
under a different identifier; ensure the state type matches the object shape
used in the updater so TypeScript compiles.
In `@frontend/src/utils/streamUtils.ts`:
- Around line 252-257: The event type "message.part.removed" is being lumped
with part updates so extractPartFromEvent() never yields data for removals;
update the classification logic (the branch handling eventType in
streamUtils.ts) to mark removals separately (e.g., set a distinct flag like
result.hasPartRemoved) and then update extractPartFromEvent() or the caller that
processes these flags to return or act on a removal payload (such as the part
id) so the UI removes the part instead of no-op; also apply the same change to
the other occurrence noted (lines ~356-381) so removed parts are handled
consistently.
In `@scripts/start-app.sh`:
- Around line 125-128: The script advertises FRONTEND_BASE_URL as
"http://127.0.0.1:${FRONTEND_PORT}" which differs from the default
FRONTEND_ORIGINS/FRONTEND_ALLOWED_HOSTS ("localhost"), causing CORS mismatches;
change FRONTEND_BASE_URL to use the same default origin as FRONTEND_ORIGINS
(e.g., "http://localhost:${FRONTEND_PORT}") or derive FRONTEND_BASE_URL from
FRONTEND_ORIGINS so the printed URL and the allowed origin match; update any use
of FRONTEND_BASE_URL accordingly and ensure OPENCODE_CORS_ORIGINS remains
derived from FRONTEND_ORIGINS.
- Around line 109-116: In wait_for_opencode(), the URL is being constructed
using ${port} before port is assigned which breaks under set -u; change the
local declarations so port is assigned first (e.g., local port="$1") and then
build url using that value (e.g., local
url="http://127.0.0.1:${port}/global/health"), or use a safe default like local
port="${1:-<default>}" before composing url; update the wait_for_opencode
function accordingly and ensure curl uses the constructed url; also note the
localhost vs 127.0.0.1 origin mismatch still needs separate CORS/code-path
validation.
In `@scripts/stop-app.sh`:
- Around line 45-60: The stop_by_port function currently falls back to the
hard-coded default port when the .port file is missing; change it so the
function requires a recorded runtime port file and does not use the default port
for termination: in stop_by_port, if the port_file does not exist, log a warning
and return without attempting to discover/kill by default_port, and apply the
same behavior to the analogous logic around lines 93-96 (the other port-based
stopper) so no process is killed unless a valid .port file is present; keep the
existing kill logic when a port is read from the file.
- Around line 68-70: The script currently returns early when systemd_active
"$systemd_unit" is true but later unconditionally removes runtime metadata
(.port/.url) — preserve metadata for services you did not stop. Change
stop_service so that cleanup of .port/.url files only runs when this script
actually stopped the service (e.g., set and check a local flag like
"stopped_by_script" or move the metadata removal into the branch that performs
the stop), keep the systemd_active check and early return in place, and
reference the existing systemd_active and stop_service logic to ensure
.port/.url files are not deleted for systemd-managed services.
---
Outside diff comments:
In `@backend/app/routes/helpers.py`:
- Around line 1001-1020: _event_matches_session currently unwraps the
GlobalEvent envelope into local variables parsed→inner→target but
_parse_permission_event and _parse_question_event still read the original
top-level payload/type, so wrapped frames (payload.payload) match session but
are not parsed into persisted pending state; fix by unwrapping the envelope
before any event-specific parsing (move the parsed→inner→target logic to the
common entry point or have _parse_permission_event and _parse_question_event
accept/use the already-unwrapped target), and update those functions to inspect
target.get("type") and target.get("properties") (or accept the unwrapped dict
argument) so permission.* and question.* frames nested under payload.payload are
parsed and persisted correctly.
🪄 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: CHILL
Plan: Pro
Run ID: 80af3a87-8d48-4514-8dfa-443fbf46fe16
📒 Files selected for processing (10)
backend/app/routes/helpers.pybackend/app/routes/messages.pybackend/requirements.txtbackend/run.pyfrontend/src/App.tsxfrontend/src/utils/streamUtils.tsfrontend/vite.config.tsscripts/run-backend-service.shscripts/start-app.shscripts/stop-app.sh
📜 Review details
🧰 Additional context used
🪛 ast-grep (0.42.3)
backend/run.py
[warning] 14-14: Running flask app with host 0.0.0.0 could expose the server publicly.
Context: app.run(host="0.0.0.0", port=BACKEND_PORT, debug=True, threaded=True)
Note: [CWE-668]: Exposure of Resource to Wrong Sphere [OWASP A01:2021]: Broken Access Control [REFERENCES]
https://owasp.org/Top10/A01_2021-Broken_Access_Control
(avoid_app_run_with_bad_host-python)
🪛 Ruff (0.15.14)
backend/run.py
[error] 15-15: Possible binding to all interfaces
(S104)
[error] 20-20: Possible binding to all interfaces
(S104)
🪛 Shellcheck (0.11.0)
scripts/start-app.sh
[warning] 110-110: This assignment is used again in this 'local', but won't have taken effect. Use two 'local's.
(SC2318)
Comment on lines
+45
to
60
| stop_by_port() { | ||
| local name="$1" port_file="$2" default_port="$3" | ||
| local port | ||
| if [[ -f "$port_file" ]]; then | ||
| port="$(cat "$port_file")" | ||
| else | ||
| port="$default_port" | ||
| fi | ||
|
|
||
| # kill only the process listening on the specific port | ||
| local pid | ||
| pid="$(cat "$pid_file")" | ||
| if [[ -z "$pid" ]]; then | ||
| rm -f "$pid_file" | ||
| return 1 | ||
| fi | ||
|
|
||
| if ! kill -0 "$pid" >/dev/null 2>&1; then | ||
| rm -f "$pid_file" | ||
| return 1 | ||
| pid="$(ss -tlnp "sport = :${port}" 2>/dev/null | grep -oP 'pid=\K[0-9]+' | head -1)" | ||
| if [[ -n "$pid" ]]; then | ||
| kill "$pid" 2>/dev/null || true | ||
| sleep 0.5 | ||
| kill -0 "$pid" 2>/dev/null && kill -9 "$pid" 2>/dev/null || true | ||
| fi |
There was a problem hiding this comment.
Require recorded runtime state before doing port-based termination.
If the .port file is missing, this falls back to killing whatever is listening on the hard-coded default port. On a fresh machine, after cleanup, or when only a systemd unit exists, that can terminate an unrelated process on 5173/38473/40961.
Suggested fix
stop_by_port() {
local name="$1" port_file="$2" default_port="$3"
local port
- if [[ -f "$port_file" ]]; then
- port="$(cat "$port_file")"
- else
- port="$default_port"
- fi
+ [[ -f "$port_file" ]] || return 1
+ port="$(cat "$port_file")"
# kill only the process listening on the specific port
local pid
pid="$(ss -tlnp "sport = :${port}" 2>/dev/null | grep -oP 'pid=\K[0-9]+' | head -1)"
if [[ -n "$pid" ]]; then
kill "$pid" 2>/dev/null || true
sleep 0.5
kill -0 "$pid" 2>/dev/null && kill -9 "$pid" 2>/dev/null || true
+ return 0
fi
+ return 1
}
@@
- if ! $stopped; then
- stop_by_port "$name" "$port_file" "$default_port"
- stopped=true
+ if ! $stopped && stop_by_port "$name" "$port_file" "$default_port"; then
+ stopped=true
fiAlso applies to: 93-96
🤖 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 `@scripts/stop-app.sh` around lines 45 - 60, The stop_by_port function
currently falls back to the hard-coded default port when the .port file is
missing; change it so the function requires a recorded runtime port file and
does not use the default port for termination: in stop_by_port, if the port_file
does not exist, log a warning and return without attempting to discover/kill by
default_port, and apply the same behavior to the analogous logic around lines
93-96 (the other port-based stopper) so no process is killed unless a valid
.port file is present; keep the existing kill logic when a port is read from the
file.
Comment on lines
+68
to
+70
| if systemd_active "$systemd_unit"; then | ||
| echo "$name: managed by systemd ($systemd_unit) — use 'sudo systemctl stop $systemd_unit'" | ||
| return 0 |
There was a problem hiding this comment.
Don't delete runtime metadata for services you didn't stop.
When a service is active under systemd, stop_service returns early and leaves it running, but Line 110 still removes every .port/.url file. scripts/start-app.sh relies on those files to recover active systemd ports, so the next startup can fall back to the wrong defaults and wire the stack to the wrong endpoints.
Also applies to: 110-110
🤖 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 `@scripts/stop-app.sh` around lines 68 - 70, The script currently returns early
when systemd_active "$systemd_unit" is true but later unconditionally removes
runtime metadata (.port/.url) — preserve metadata for services you did not stop.
Change stop_service so that cleanup of .port/.url files only runs when this
script actually stopped the service (e.g., set and check a local flag like
"stopped_by_script" or move the metadata removal into the branch that performs
the stop), keep the systemd_active check and early return in place, and
reference the existing systemd_active and stop_service logic to ensure
.port/.url files are not deleted for systemd-managed services.
Collaborator
Author
|
@copilot Please perform a PR review, look at the comments, and fix all issues. |
…appending new results
Contributor
Fixed the outstanding review issues and checked the newer Validated with:
Code review passed on the final branch state. CodeQL passed on the earlier rerun after the security fix; the last full validation rerun hit a timeout. |
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
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
Release Notes
New Features
Bug Fixes
message.updatedevent handling during inline reconciliationset -uChores