trails-cool/trails

Watch

Fork 0

fix(poi): stop a successful import reporting itself as failed; wire the textfile collector #22

Merged

ullrich merged 1 commit from fix/poi-import-textfile-metrics into main

2026-07-27 12:02:49 +00:00

ullrich commented

2026-07-27 12:00:28 +00:00

Owner

Two defects found while installing the POI refresh timers for #19. Both only surface on the monthly run, which is why neither showed up in the July bootstrap import.

1. A successful import would have reported itself as failed

emit_metric is the last command a successful import runs, and its final act is:

mv "$tmp" "$NODE_EXPORTER_TEXTFILE_DIR/poi_import.prom"

poi-import.service sets NODE_EXPORTER_TEXTFILE_DIR=/var/lib/node_exporter/textfile_collector, and that directory did not exist on the flagship. Under set -euo pipefail the failing mv becomes the script's exit status — so systemd would have recorded poi-import.service as failed after an import that fetched, verified, classified and atomically swapped 8.4M rows successfully.

Verified against the pre-fix script, simulating the final line with the directory absent:

mv: rename /tmp/tmp.GKwF73l3YN to .../textfile_collector/poi_import.prom: No such file or directory
=== pre-fix exit=1 ===

The REACHED: script continued line never prints. The function's own comment already says "best-effort", so this makes it true: mkdir -p the target, guard every step, always return 0.

2. The metrics had nowhere to go anyway

node-exporter ran without --collector.textfile.directory and without that path mounted, so poi_import_last_status, _last_rows and _last_success_timestamp_seconds were written to disk and silently dropped. Confirmed against production Prometheus — the query returns an empty result while poi_index_age_seconds returns fine.

That mattered more than a missing gauge: the poi-index-stale alert annotation tells the operator to "Check … the Planner dashboard: Last Import Status", a panel fed by exactly these metrics. The alert's own remediation step led somewhere permanently blank.

3. Ownership, found while fixing 2

node-exporter runs as nobody (uid 65534) — confirmed with docker exec … id — but mktemp creates files mode 0600 root-owned. Wiring the collector alone would still have yielded nothing readable, so the file is now chmod 0644 before publishing.

Scope note

poi_index_age_seconds is published by the Planner from its own DB and is what poi-index-stale actually evaluates, so index staleness was always covered. What was missing is per-run outcome detail — "the import ran and failed" as distinct from "the data is getting old", which is the difference between catching a failure the next morning and catching it six weeks later.

Verification

Failure paths exercised in a harness extracting the real function:

Case	Result
Target dir absent	created, file written `0644`, exit 0
Unwritable parent	`WARN` logged, exit 0
`NODE_EXPORTER_TEXTFILE_DIR` unset	exit 0, no-op
Failure path (`status=0`)	omits `_last_success_timestamp_seconds`

docker compose config validates, and the resolved mount reports create_host_path: true — Docker creates the host directory itself, so the mkdir is belt-and-braces rather than a standing prerequisite.

Deploys via cd-infra (path infrastructure/); node-exporter is already in its up -d list, so the collector flag lands on merge. Worth a manual systemctl start poi-import.service afterwards to confirm the unit now exits 0 and the metric reaches Prometheus, rather than waiting for Aug 2.

Both timers are installed and enabled as of today: extract Aug 1 02:44 UTC on the BRouter host, import Aug 2 04:11 UTC on the flagship.

Refs #19

Two defects found while installing the POI refresh timers for #19. Both only surface on the monthly run, which is why neither showed up in the July bootstrap import. ## 1. A successful import would have reported itself as failed `emit_metric` is the **last command** a successful import runs, and its final act is: ```bash mv "$tmp" "$NODE_EXPORTER_TEXTFILE_DIR/poi_import.prom" ``` `poi-import.service` sets `NODE_EXPORTER_TEXTFILE_DIR=/var/lib/node_exporter/textfile_collector`, and that directory did not exist on the flagship. Under `set -euo pipefail` the failing `mv` becomes the script's exit status — so systemd would have recorded `poi-import.service` as **failed** after an import that fetched, verified, classified and atomically swapped 8.4M rows successfully. Verified against the pre-fix script, simulating the final line with the directory absent: ``` mv: rename /tmp/tmp.GKwF73l3YN to .../textfile_collector/poi_import.prom: No such file or directory === pre-fix exit=1 === ``` The `REACHED: script continued` line never prints. The function's own comment already says "best-effort", so this makes it true: `mkdir -p` the target, guard every step, always `return 0`. ## 2. The metrics had nowhere to go anyway `node-exporter` ran without `--collector.textfile.directory` and without that path mounted, so `poi_import_last_status`, `_last_rows` and `_last_success_timestamp_seconds` were written to disk and silently dropped. Confirmed against production Prometheus — the query returns an empty result while `poi_index_age_seconds` returns fine. That mattered more than a missing gauge: the `poi-index-stale` alert annotation tells the operator to *"Check … the Planner dashboard: Last Import Status"*, a panel fed by exactly these metrics. The alert's own remediation step led somewhere permanently blank. ## 3. Ownership, found while fixing 2 `node-exporter` runs as `nobody` (uid 65534) — confirmed with `docker exec … id` — but `mktemp` creates files mode `0600` root-owned. Wiring the collector alone would still have yielded nothing readable, so the file is now `chmod 0644` before publishing. ## Scope note `poi_index_age_seconds` is published by the Planner from its own DB and is what `poi-index-stale` actually evaluates, so **index staleness was always covered**. What was missing is per-run outcome detail — "the import ran and failed" as distinct from "the data is getting old", which is the difference between catching a failure the next morning and catching it six weeks later. ## Verification Failure paths exercised in a harness extracting the real function: | Case | Result | |---|---| | Target dir absent | created, file written `0644`, exit 0 | | Unwritable parent | `WARN` logged, exit 0 | | `NODE_EXPORTER_TEXTFILE_DIR` unset | exit 0, no-op | | Failure path (`status=0`) | omits `_last_success_timestamp_seconds` | `docker compose config` validates, and the resolved mount reports `create_host_path: true` — Docker creates the host directory itself, so the `mkdir` is belt-and-braces rather than a standing prerequisite. Deploys via `cd-infra` (path `infrastructure/`); `node-exporter` is already in its `up -d` list, so the collector flag lands on merge. Worth a manual `systemctl start poi-import.service` afterwards to confirm the unit now exits 0 and the metric reaches Prometheus, rather than waiting for Aug 2. Both timers are installed and enabled as of today: extract Aug 1 02:44 UTC on the BRouter host, import Aug 2 04:11 UTC on the flagship. Refs #19

ullrich added 1 commit

2026-07-27 12:00:28 +00:00

fix(poi): stop a successful import reporting itself as failed; wire the textfile collector

CI / Dockerfile Package Check (pull_request) Successful in 24s

Details

CI / Security Scan (pull_request) Successful in 41s

Details

CI / Checks (pull_request) Successful in 2m58s

Details

CI / Visual Tests (pull_request) Successful in 3m0s

Details

CI / Journal Image Smoke Test (pull_request) Successful in 2m48s

Details

CI / E2E Tests (pull_request) Successful in 7m46s

Details

1aa3de4eba

Two defects found while installing the POI refresh timers (#19), both of
which only surface on the monthly run.

1. `emit_metric` is the last command a successful import executes. It
   `mv`s a .prom file into `$NODE_EXPORTER_TEXTFILE_DIR`, which the unit
   sets to /var/lib/node_exporter/textfile_collector — a path that did
   not exist on the flagship. Under `set -euo pipefail` that failing
   `mv` became the script's exit status, so a fully successful import
   would have been recorded by systemd as a failed unit. Verified
   against the pre-fix script: exit 1, with the work already done.

   The function is documented as "best-effort", so make it so — mkdir
   the target, guard every step, always return 0.

2. The metrics had nowhere to go regardless: node-exporter ran without
   `--collector.textfile.directory` and without that path mounted, so
   `poi_import_last_status` / `_last_rows` /
   `_last_success_timestamp_seconds` were written and silently dropped.
   The `poi-index-stale` alert annotation points operators at a Planner
   dashboard panel fed by exactly those metrics, so the alert's own
   remediation step led to an empty panel.

Also chmod 0644 the file before publishing it: node-exporter runs as
`nobody` (uid 65534) but mktemp creates 0600 root-owned files, so the
collector could not have read it even once wired up.

`poi_index_age_seconds` (published by the Planner from its own DB) is
unaffected and is what the stale-index alert evaluates, so index
staleness was always covered — it is the per-run outcome detail that was
missing.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01X8hNxgYp777FRqYtVmQNaU

ullrich merged commit 7527143401 into main

2026-07-27 12:02:49 +00:00

ullrich deleted branch fix/poi-import-textfile-metrics

2026-07-27 12:02:49 +00:00

ullrich referenced this pull request from a commit

2026-07-27 12:02:50 +00:00

Merge pull request 'fix(poi): stop a successful import reporting itself as failed; wire the textfile collector' (#22) from fix/poi-import-textfile-metrics into main

ullrich referenced this pull request

2026-07-27 12:16:47 +00:00

cd-infra kills Prometheus when it enters the deploy stopped (SIGHUP branch misreads stopped-container ID as 'was running') #23

ullrich referenced this pull request

2026-07-27 12:18:57 +00:00