Skip to content

Commit 3396438

Browse files
jkyberneeesclaude
andcommitted
fix(docker): address code-review findings in supercronic integration
Five findings from a post-merge review, all verified: C1 — graceful restart (/restart) re-execs the bare odek binary, bypassing cron-entrypoint.sh; supercronic was never restarted. Fix: cron-entrypoint.sh exports ODEK_ENTRYPOINT=$0 and ODEK_SUPERCRONIC_PID. spawnChild() uses ODEK_ENTRYPOINT when set so the wrapper is re-entered on restart. The wrapper kills the old supercronic PID before starting a new one, preventing duplicate scheduler instances. C2+C3 — no init process: supercronic zombies on exit; SIGTERM from docker stop not forwarded to supercronic (in-flight jobs killed abruptly at SIGKILL). Fix: init: true on both telegram compose services. Docker's built-in init becomes PID 1, reaping orphaned children and forwarding SIGTERM to the process group. C4 — arch="${TARGETARCH:-amd64}" silently installed the wrong-arch supercronic binary on arm64 hosts building without BuildKit; the SHA check still passed. Fix: change to ${TARGETARCH:?...} — a hard build failure with an actionable error message rather than a silent wrong-arch download. C5 — Docker creates a root-owned directory at ./crontab on the host when the source path is missing from a bind mount; [ -f ] returned false silently. Fix: add [ -d ] branch with an explicit warning explaining the cause and fix. C6 — supercronic backgrounded with &; set -e does not apply to background processes, so an immediate startup failure was silently swallowed. Fix: sleep 1 + kill -0 liveness check after launch; emits a clear WARNING if supercronic exits immediately, rather than proceeding as if cron is running. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent 5ebc394 commit 3396438

4 files changed

Lines changed: 56 additions & 7 deletions

File tree

cmd/odek/telegram.go

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -899,6 +899,14 @@ func spawnChild() error {
899899
if err != nil {
900900
return fmt.Errorf("executable: %w", err)
901901
}
902+
// When running inside the Docker container the entrypoint script exports
903+
// ODEK_ENTRYPOINT=$0. Re-exec through the wrapper so supercronic is
904+
// restarted alongside the new odek process. The wrapper reads
905+
// ODEK_SUPERCRONIC_PID (also in childEnv via os.Environ()) and kills the
906+
// previous supercronic before starting a new one — no duplicate instances.
907+
if ep := os.Getenv("ODEK_ENTRYPOINT"); ep != "" {
908+
exe = ep
909+
}
902910
// Copy args (same as current process).
903911
argv := make([]string, len(os.Args))
904912
copy(argv, os.Args)

docker/Dockerfile

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -76,7 +76,7 @@ RUN apk add --no-cache ca-certificates git github-cli bash coreutils curl jq
7676
ARG SUPERCRONIC_VERSION=v0.2.46
7777
ARG TARGETARCH
7878
RUN set -eu; \
79-
arch="${TARGETARCH:-amd64}"; \
79+
arch="${TARGETARCH:?TARGETARCH is empty — build with BuildKit (docker buildx build) or pass --build-arg TARGETARCH=amd64|arm64}"; \
8080
case "$arch" in \
8181
amd64) sha=5adff01c5a797663948e656d2b61d10932369ee437eb5cb54fa872b2960f222b ;; \
8282
arm64) sha=c0576a8eb092e3f79108ed0a2155a25c7766af78456e5a6070e54757ef513bfe ;; \

docker/cron-entrypoint.sh

Lines changed: 39 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -16,15 +16,48 @@ set -eu
1616
# Path to the crontab. Overridable so an operator can relocate the mount.
1717
CRONTAB="${ODEK_CRONTAB:-/home/odek/.crontabs/crontab}"
1818

19-
if [ -f "$CRONTAB" ]; then
20-
echo "cron-entrypoint: starting supercronic for $CRONTAB" >&2
21-
# -passthrough-logs keeps each job's own stdout/stderr intact in the
22-
# container log alongside supercronic's scheduling lines.
23-
supercronic -passthrough-logs "$CRONTAB" &
19+
# Graceful-restart support: odek's /restart command re-execs via this script
20+
# (see ODEK_ENTRYPOINT below). Kill any supercronic from the previous run so we
21+
# never end up with two instances scheduling the same crontab concurrently.
22+
if [ -n "${ODEK_SUPERCRONIC_PID:-}" ]; then
23+
kill "$ODEK_SUPERCRONIC_PID" 2>/dev/null || true
24+
unset ODEK_SUPERCRONIC_PID
25+
fi
26+
27+
if [ -d "$CRONTAB" ]; then
28+
# Docker creates a directory when the bind-mount source doesn't exist on the
29+
# host. This is almost always a misconfiguration — warn loudly rather than
30+
# silently skipping so the operator knows why reminders aren't firing.
31+
echo "cron-entrypoint: WARNING: $CRONTAB is a directory, not a file" >&2
32+
echo "cron-entrypoint: Docker created it because the host path was missing." >&2
33+
echo "cron-entrypoint: Fix: remove the directory on the host and create the file." >&2
34+
echo "cron-entrypoint: Skipping supercronic — cron jobs will NOT run." >&2
35+
elif [ -f "$CRONTAB" ]; then
36+
echo "cron-entrypoint: starting supercronic for $CRONTAB" >&2
37+
# -passthrough-logs keeps each job's own stdout/stderr intact in the
38+
# container log alongside supercronic's scheduling lines.
39+
supercronic -passthrough-logs "$CRONTAB" &
40+
ODEK_SUPERCRONIC_PID=$!
41+
export ODEK_SUPERCRONIC_PID
42+
# Brief liveness check: supercronic parses the crontab at startup and exits
43+
# immediately on a syntax error or missing binary. Neither is recoverable at
44+
# runtime, so catching it here produces a clear warning rather than silent
45+
# non-delivery. set -e does not cover backgrounded processes, so we check
46+
# explicitly after a short window.
47+
sleep 1
48+
if ! kill -0 "$ODEK_SUPERCRONIC_PID" 2>/dev/null; then
49+
echo "cron-entrypoint: WARNING: supercronic exited immediately — cron jobs will NOT run" >&2
50+
unset ODEK_SUPERCRONIC_PID
51+
fi
2452
else
25-
echo "cron-entrypoint: no crontab at $CRONTAB — skipping supercronic" >&2
53+
echo "cron-entrypoint: no crontab at $CRONTAB — skipping supercronic" >&2
2654
fi
2755

56+
# Advertise this script's own path so spawnChild (odek's /restart handler) can
57+
# re-exec through the wrapper instead of the bare binary. Without this, a
58+
# graceful restart would skip supercronic entirely.
59+
export ODEK_ENTRYPOINT="$0"
60+
2861
# Default to printing usage if no command was provided (matches the previous
2962
# `ENTRYPOINT ["odek"]` behaviour for a bare `docker run`).
3063
exec odek "$@"

docker/docker-compose.yml

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -62,6 +62,13 @@ services:
6262
image: odek:local
6363
env_file: .env
6464
command: ["telegram"]
65+
# init: true adds Docker's built-in init (tini) as PID 1. This gives us:
66+
# - Zombie reaping: supercronic child processes are reaped when they exit.
67+
# - Signal forwarding: SIGTERM from `docker stop` reaches all children,
68+
# giving in-flight cron jobs a clean shutdown window.
69+
# - Graceful restart safety: when odek exits during /restart, the spawned
70+
# child is reparented to the init rather than dying with odek.
71+
init: true
6572
volumes:
6673
- ./workspace:/workspace
6774
- ./.odek:/home/odek/.odek
@@ -82,6 +89,7 @@ services:
8289
image: odek:local
8390
env_file: .env
8491
command: ["telegram"]
92+
init: true # zombie reaping + SIGTERM forwarding (see telegram-restricted)
8593
volumes:
8694
- ./workspace:/workspace
8795
- ./.odek:/home/odek/.odek

0 commit comments

Comments
 (0)