fix: dockerfile logs - #13
Conversation
Audit Report: PR
|
| Property | Value |
|---|---|
| Source Branch | fix/dockfile_log |
| Target Branch | main |
| Commits | 1 — c1aaf59 fix: dockerfile logs |
| Files Changed | 1 (Dockerfile) |
| Insertions | 5 |
| Deletions | 5 |
| Net Change | 0 (pure restructure + CMD change) |
Diff Summary
-# Set environment variables
-ENV PYTHONUNBUFFERED=1
-ENV PATH="/app/.venv/bin:$PATH"
-
-CMD ["/app/.venv/bin/python", "/app/server/main.py"]
+# Set environment variables
+ENV PYTHONUNBUFFERED=1
+ENV PATH="/app/.venv/bin:$PATH"
+
+CMD ["bash", "-c", "python /app/server/main.py 2>&1 | tee -a /app/logs/server.log"]The two ENV declarations were moved earlier in the Dockerfile (before COPY server/), and the CMD was changed from a direct exec-form invocation to a shell pipeline that tees stdout+stderr into /app/logs/server.log.
2. Change Summary
2.1 — ENV Instruction Reordering
The ENV PYTHONUNBUFFERED=1 and ENV PATH="/app/.venv/bin:$PATH" declarations were moved from after the EXPOSE 8000 line to immediately after the pip install step, placing them before the COPY server/ instruction.
Effect: Cosmetically cleaner (env vars are now near the step that creates the venv), but the net Docker layer ordering and resulting image are identical. Both ENV instructions create separate read-only layers regardless of position. There is no caching benefit or penalty.
2.2 — CMD Rewrite: exec-form → shell pipeline for log tee
The entrypoint was changed from:
CMD ["/app/.venv/bin/python", "/app/server/main.py"]to:
CMD ["bash", "-c", "python /app/server/main.py 2>&1 | tee -a /app/logs/server.log"]Stated intent: Persist server logs to /app/logs/server.log inside the container so they survive across docker logs rotation and are available via the volume-mounted ./logs/ directory (see start_docker.sh line 39).
3. Detailed Findings
[MAJOR-01] PID 1 is now bash, not the Python process — signal handling broken
| Property | Value |
|---|---|
| Severity | Major |
| Category | Correctness |
| File | Dockerfile : Line 32 |
Description:
With the exec-form CMD the Python process ran as PID 1 inside the container and received OS signals (SIGTERM, SIGINT) directly, allowing graceful shutdown (uvicorn handles these). With the new shell pipeline CMD, bash becomes PID 1. When Docker sends SIGTERM on docker stop, bash receives it and exits immediately — the Python/uvicorn process receives SIGKILL via the OOM reaper rather than a graceful SIGTERM, bypassing shutdown hooks (open connections, in-flight payments, etc.).
Code:
CMD ["bash", "-c", "python /app/server/main.py 2>&1 | tee -a /app/logs/server.log"]Recommendation:
Use exec to replace the shell with tee, or better, forward signals explicitly:
CMD ["bash", "-c", "exec python /app/server/main.py 2>&1 | tee -a /app/logs/server.log"]Note: even with exec, in a pipeline the shell itself is still PID 1 and bash does not forward signals to pipeline children by default. The most robust fix is to handle file logging within the Python application (uvicorn supports --log-config) or use a process supervisor (e.g. tini):
RUN apt-get install -y tini
ENTRYPOINT ["/usr/bin/tini", "--"]
CMD ["bash", "-c", "python /app/server/main.py 2>&1 | tee -a /app/logs/server.log"][MAJOR-02] tee -a appends indefinitely — no log rotation, unbounded disk growth
| Property | Value |
|---|---|
| Severity | Major |
| Category | Performance / Correctness |
| File | Dockerfile : Line 32 |
Description:
tee -a /app/logs/server.log opens the log file in append mode on every container start and grows it without bound. Long-running deployments or containers that crash-restart will accumulate the full history in a single file with no size cap, no rotation, and no cleanup. If the container is restarted frequently (e.g. in a crash loop) the log file can fill the volume mount or the container's overlay filesystem.
Code:
tee -a /app/logs/server.logRecommendation:
Either implement log rotation inside the application (Python logging.handlers.RotatingFileHandler), or use a logrotate sidecar/cron job on the host, or remove -a and accept that each restart truncates the log file. For a demo context, simply omitting -a is a pragmatic fix.
[MINOR-01] python in CMD relies on PATH being set at container runtime — fragile dependency
| Property | Value |
|---|---|
| Severity | Minor |
| Category | Correctness / Quality |
| File | Dockerfile : Line 32 |
Description:
The original CMD used an absolute path /app/.venv/bin/python. The new CMD uses the bare python command, relying on ENV PATH="/app/.venv/bin:$PATH" being in effect at runtime. While this works correctly in this Dockerfile because the ENV is set, it introduces an implicit dependency on PATH that could silently break if the image is built FROM this image and the derived image resets PATH or if someone runs the container with --env PATH=....
Code:
CMD ["bash", "-c", "python /app/server/main.py 2>&1 | tee -a /app/logs/server.log"]Recommendation:
Use the absolute path for robustness:
CMD ["bash", "-c", "/app/.venv/bin/python /app/server/main.py 2>&1 | tee -a /app/logs/server.log"][MINOR-02] Typo in branch name: fix/dockfile_log (missing 'e' in Dockerfile)
| Property | Value |
|---|---|
| Severity | Minor |
| Category | Quality / Docs |
| File | Branch name |
Description:
The branch is named fix/dockfile_log instead of fix/dockerfile_log. While this has no functional impact, it is a permanent typo in the repository's branch history and commit messages.
Recommendation:
No action needed post-merge, but future branch names should be checked before creation.
[SUGGESTION-01] Log tee destination should be configurable via environment variable
| Property | Value |
|---|---|
| Severity | Suggestion |
| Category | Quality |
| File | Dockerfile : Line 32 |
Description:
The log file path /app/logs/server.log is hardcoded in the CMD. In environments where the volume is not mounted or where the path should differ, this cannot be changed without rebuilding the image.
Recommendation:
Parameterize via an environment variable with a sensible default:
ENV LOG_FILE=/app/logs/server.log
CMD ["bash", "-c", "python /app/server/main.py 2>&1 | tee -a \"$LOG_FILE\""][SUGGESTION-02] Consider consolidating log output — stdout AND file logging may duplicate effort
| Property | Value |
|---|---|
| Severity | Suggestion |
| Category | Quality |
| File | Dockerfile : Line 32 |
Description:
tee writes to both stdout (visible via docker logs) and the file. The server application (server/main.py) already uses Python's logging module with PYTHONUNBUFFERED=1. Docker log drivers also support log persistence. Tee-based file logging is an additional layer that may create redundancy. The existing start_docker.sh already advises users to use docker logs -f x402-tron-demo for log viewing.
Recommendation:
Evaluate whether the use case genuinely requires file-backed log persistence (e.g. post-mortem across container restarts) or if docker logs and Docker log driver configuration (e.g. --log-driver json-file --log-opt max-size=10m) suffice for the demo scenario.
4. Positive Observations
PYTHONUNBUFFERED=1is correctly retained. Without this, Python buffers stdout/stderr andteewould not see real-time output. The existing ENV ensures log lines appear immediately.- Volume mount in
start_docker.shis already in place. The host-side./logs:/app/logsbind mount ensures logs are accessible outside the container withoutdocker exec, which is the right approach for a file-based logging strategy. - The
/app/logsdirectory is created during image build (RUN mkdir -p /app/logs). This prevents the container from failing at startup if the volume is not mounted. - Single-commit, focused PR. The change is small, self-contained, and easy to reason about.
5. Review Verdict
Verdict: Request Changes
Rationale:
The core intent — making server logs observable via a file — is sound and the approach is reasonable for a demo project. However, the change introduces a meaningful regression in signal handling (MAJOR-01): the Python/uvicorn process no longer receives SIGTERM on docker stop, which can cause abrupt container termination and loss of in-flight state. For a payment-handling service this is a correctness concern even in a demo context.
The unbounded log growth (MAJOR-02) is a practical operational concern that should be addressed before merge to avoid silent disk exhaustion in persistent deployments.
Minimum required fixes before merge:
- Fix signal propagation (MAJOR-01) — use
tinior restructure CMD so the Python process receives signals. - Address unbounded log growth (MAJOR-02) — either remove
-a, add log rotation, or document the limitation explicitly. - Consider using absolute Python path (MINOR-01) for robustness.
2 participants
No description provided.