From 2e14bc314168105131aa76bc3041adc32a52d8f9 Mon Sep 17 00:00:00 2001 From: kireto Date: Fri, 18 Sep 2026 11:19:15 +0000 Subject: [PATCH] GRM-170: fix: harden stall-detection enumeration and timestamp parsing --- .../templates/runner-healthcheck.sh.j2 | 19 +++-- docs/specs/GRM-170.md | 73 +++++++++++++++++++ 2 files changed, 87 insertions(+), 5 deletions(-) create mode 100644 docs/specs/GRM-170.md diff --git a/ansible/roles/gitea_runner/templates/runner-healthcheck.sh.j2 b/ansible/roles/gitea_runner/templates/runner-healthcheck.sh.j2 index f4f88b2..a8d728d 100644 --- a/ansible/roles/gitea_runner/templates/runner-healthcheck.sh.j2 +++ b/ansible/roles/gitea_runner/templates/runner-healthcheck.sh.j2 @@ -38,10 +38,15 @@ fi STALL_MINUTES={{ gitea_runner_stall_minutes }} DIAG_DIR="{{ gitea_runner_config_dir }}" now_epoch=$(date +%s) -timeout 15 docker ps --filter "name=GITEA-ACTIONS-TASK" \ - --format '{% raw %}{{.ID}} {{.Names}} {{.CreatedAt}}{% endraw %}' 2>/dev/null \ - | while read -r cid cname ccreated _rest; do - created_epoch=$(date -d "$ccreated" +%s 2>/dev/null || echo 0) +# Implements: REQ-1 — guard the enumeration: a slow/dead daemon must not +# abort the healthcheck under pipefail; an empty list just skips probing. +# Implements: REQ-2 — pipe-separate fields: CreatedAt contains spaces, so +# whitespace-splitting `read` only captured the date and broke the age gate. +{ timeout 15 docker ps --filter "name=GITEA-ACTIONS-TASK" \ + --format '{% raw %}{{.ID}}|{{.Names}}|{{.CreatedAt}}{% endraw %}' 2>/dev/null || true; } \ + | while IFS='|' read -r cid cname ccreated _rest; do + # GNU date rejects the redundant " +0000 UTC" suffix — drop it. + created_epoch=$(date -d "${ccreated% UTC}" +%s 2>/dev/null || echo 0) age_min=$(( (now_epoch - created_epoch) / 60 )) [[ "$age_min" -lt "$STALL_MINUTES" ]] && continue marker="$DIAG_DIR/.stall-diag-$cid" @@ -52,7 +57,11 @@ timeout 15 docker ps --filter "name=GITEA-ACTIONS-TASK" \ echo "=== stall diagnostics for $cname ($cid), age ${age_min}m ===" echo "--- exec probe: TIMEOUT (>10s) ---" echo "--- docker inspect ---" - timeout 15 docker inspect "$cid" 2>/dev/null | head -200 + # Implements: REQ-3 — full inspect, but redact the Env block: + # job containers carry CI tokens in env vars; the bundle must + # not become a secret-material artifact. + timeout 15 docker inspect "$cid" 2>/dev/null \ + | sed -E 's/("[^"]*(TOKEN|PASSWORD|SECRET|KEY)[^=]*=)[^",]*/\1/Ig' echo "--- docker top ---" timeout 15 docker top "$cid" 2>/dev/null echo "--- docker stats --no-stream ---" diff --git a/docs/specs/GRM-170.md b/docs/specs/GRM-170.md new file mode 100644 index 0000000..42b643f --- /dev/null +++ b/docs/specs/GRM-170.md @@ -0,0 +1,73 @@ +# GRM-170: Fix stall-detection robustness bugs in runner healthcheck + +## Problem + +Post-merge review of the GRM-168 stall-detection block in +`runner-healthcheck.sh.j2` found three defects: + +1. **Missing `|| true` on the container enumeration.** `timeout 15 + docker ps … | while …` runs under `set -euo pipefail`. If the daemon + is unresponsive — precisely the condition the section exists to + diagnose — `docker ps` exits nonzero, pipefail propagates it, and the + healthcheck dies mid-run before reaching the runner-service check. + Every other docker call in the script is guarded; this one is not. + +2. **CreatedAt split bug.** `docker ps --format '{{.ID}} {{.Names}} + {{.CreatedAt}}'` emits a timestamp containing spaces + (`2026-09-18 10:30:00 +0000 UTC`), but `read -r cid cname ccreated + _rest` only captures `2026-09-18` — the date part. `date -d` then + computes age from midnight: containers created today always appear + ≥N hours old, so the 15-minute gate effectively never filters. + +3. **`head -200` truncates `docker inspect`.** Inspect output is ~300+ + lines and the `State` block (OOMKilled, Pid, times) the spec requires + can be cut off. + +## Approach + +REQ-1: Wrap the enumeration so a failed `docker ps` yields empty input +instead of aborting the script: `{ timeout 15 docker ps … || true; } | +while …`. + +REQ-2: Emit fields separated by `|` (`{{.ID}}|{{.Names}}|{{.CreatedAt}}`) +and parse with `IFS='|' read -r cid cname ccreated _rest` so the full +timestamp reaches `date -d`; also strip the redundant ` UTC` suffix +because GNU date rejects `+0000 UTC` together. The age gate then +compares real minutes. + +REQ-3: Remove the `head -200` truncation on `docker inspect` output so +the full State block is captured — but pipe through a `sed` filter that +redacts the value of any env entry whose name contains TOKEN, PASSWORD, +SECRET, or KEY. Job containers carry CI tokens in their Env block; the +diagnostics bundle must not become a secret-material artifact +(OBL-INFRA-548 S02). + +REQ-4: Diagnostics-only constraint unchanged — no kills, no restarts. + +## Test Plan + +- Render the template and run `bash -n` on the output. +- Shell-simulate: feed a fake `docker ps` line with spaced CreatedAt and + verify `date -d` computes minutes correctly (manual check). +- `make lint-all` (ansible-lint, actionlint, ruff) passes. +- Molecule gitea_runner scenario converges with the template change. + +## Deploy Plan + +Merge via auto-merge → release (fix: commit bumps patch) → infra +dependency-bump PR picks up the new role version → runner role applied +on next infra run. This PR also carries the merged-but-unreleased +GRM-168 healthcheck into the release. + +## Rollback Plan + +Revert the three-line change set; the section degrades to the GRM-168 +behavior (still diagnostics-only, just less robust). + +## Acceptance Criteria + +- [x] `docker ps` enumeration guarded against nonzero exit. +- [x] Full CreatedAt timestamp parsed via `|` separator. +- [x] `docker inspect` captured without truncation. +- [x] Rendered script passes `bash -n`. +- [x] `make lint-all` passes.