Conversation
wyattwalter
requested review from
nidhi-nair,
pratapaprasanna and
sharat87
as code owners
WalkthroughUpdates supervisor logging: switches main logfile path, null-routes stdout/stderr files, and configures an event listener with a Python result handler for PROCESS_LOG events. Removes Caddy debug directive from the generated Caddyfile. No public API or exported entity changes. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Proc as Managed Process
participant Sup as supervisord
participant EL as stdout EventListener (Python handler)
note over Sup: stdout/stderr files -> /dev/null
Proc->>Sup: Emits PROCESS_LOG events
Sup->>EL: Dispatch PROCESS_LOG events
EL-->>Sup: result_handler responses (success/ack)
note right of EL: Handles log events per new listener command
Sup->>Sup: Writes main log to %(ENV_HOSTNAME)s-stdout.log
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
deploy/docker/fs/etc/supervisor/supervisord.conf(2 hunks)deploy/docker/fs/opt/appsmith/caddy-reconfigure.mjs(0 hunks)
💤 Files with no reviewable changes (1)
- deploy/docker/fs/opt/appsmith/caddy-reconfigure.mjs
🔇 Additional comments (2)
deploy/docker/fs/etc/supervisor/supervisord.conf (2)
13-13: No further action needed:%(ENV_HOSTNAME)sreferences the container’s HOSTNAME, which is always set in Docker, Kubernetes, Cloud Run, and Heroku.
38-40: Handler module exists as referenced
The filedeploy/docker/fs/usr/lib/python3/dist-packages/supervisor/appsmith_supervisor_stdout.pyis present and correctly referenced insupervisord.conf.
| stdout_logfile=/dev/null | ||
| stderr_logfile=/dev/null |
There was a problem hiding this comment.
🧩 Analysis chain
Ensure error handling in the Python handler to prevent log loss.
Routing stdout/stderr to /dev/null prevents double-logging as intended. However, if the Python handler in line 41 fails or crashes, logs would be permanently lost. Ensure the handler has robust error handling and consider adding monitoring/alerting for handler failures.
Run the following script to check for error handling in the Python handler:
🏁 Script executed:
#!/bin/bash
# Description: Check error handling in the appsmith_supervisor_stdout module
# Find and examine the Python handler for error handling patterns
fd -t f 'appsmith_supervisor_stdout.py' --exec cat {} \; | rg -C5 'try:|except:|logging.error|raise'
# Check for any supervisor event handler error logs or monitoring
rg -n 'event_handler|PROCESS_LOG.*error|supervisor.*error' --type=py -g 'deploy/**'Length of output: 400
🏁 Script executed:
#!/bin/bash
rg -n -C10 'def event_handler' deploy/docker/fs/usr/lib/python3/dist-packages/supervisor/appsmith_supervisor_stdout.pyLength of output: 768
Add error handling to event_handler
Wrap the logic in deploy/docker/fs/usr/lib/python3/dist-packages/supervisor/appsmith_supervisor_stdout.py lines 24–28 in a try/except and emit any exceptions to stderr or a fallback logger to prevent silent failures and lost logs.
🤖 Prompt for AI Agents
deploy/docker/fs/usr/lib/python3/dist-packages/supervisor/appsmith_supervisor_stdout.py
around lines 24–28: the event_handler logic must be wrapped in a try/except to
avoid silent failures; surround the existing body with a broad try block, catch
Exception as e, emit the exception details to stderr (or use a fallback logger
via logging.exception) and ensure the handler returns or continues gracefully so
logs are not lost.
There was a problem hiding this comment.
This is not completely wrong.. but this is also the case if we were to continue persisting these to disk in the way we were before this proposed change.
Whatever exception is thrown would go to stderr of the container, so it's not really lost. And we don't gain a lot from continuing to write it to disk either.
There was a problem hiding this comment.
@wyattwalter, you're absolutely right—this failure mode was already present before the change, and routing to /dev/null doesn't introduce new risk. Since exceptions from the handler would still appear in the container's stderr, observability is maintained without the double-logging overhead. Thank you for the clarification!
|
/build-deploy-preview skipTests=True |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/18222774846. |
|
Deploy-Preview-URL: https://ce-41275.dp.appsmith.com |
3 participants
Description
Tip
Add a TL;DR when the description is longer than 500 words or extremely technical (helps the content, marketing, and DevRel team).
Please also include relevant motivation and context. List any dependencies that are required for this change. Add links to Notion, Figma or any other documents that might be relevant to the PR.
Addresses at least a couple of issues in a number of support tickets about logs volume:
logs/editor/<hostname>-stderr.loglogs/supervisor/supervisord.logmaking trying to troubleshoot those deployments more difficult.Warning
If no issue exists, please create an issue first, and check with the maintainers if the issue is valid.
Automation
/ok-to-test tags="@tag.Sanity"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/18222964844
Commit: 54b5a1a
Cypress dashboard.
Tags:
@tag.SanitySpec:
Fri, 03 Oct 2025 13:38:52 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit