HOMELAB-2479: feat(monitoring): factory layer — PR opened-to-merged as a stored series #846
No reviewers
ld-reviewer
Labels
No labels
operator-review
No milestone
No project
No assignees
2 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
Homelab/infra-core!846
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "plane/HOMELAB-2479-factory-pr-series"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
What this is
Gap 3 of the "correct SREs metrics" workstream (epic HOMELAB-2479): how long work
takes from PR opened to merged, per repo, stored as a trend, and how many
projects are landing in parallel.
Before this, the fleet could not answer either question from stored data. Forgejo
retains
created_at/merged_aton every PR indefinitely, so the raw materialexisted and nothing turned it into a series;
ci_health_*measures CI durationper repo, which is a different measurement and must not be reported as
time-to-merged (
working-files/infra-stability/fleet-measurement-map.md§9, §12blind spot 3, §13).
What ships
core/manifests/monitoring/factory/core/manifests/monitoring/rules/factory-alerts.yamlmonitoring-rulesfactory, 13 panelscore/manifests/monitoring/dashboards/factory.yamlmonitoring-dashboardsrule-tests/factory.test.yaml,factory/factory_series_test.pyThe emitter walks the Forgejo PR API for the same six repos the existing ci-health
credential is scoped to and pushes the per-PR facts to Prometheus through its
remote-write receiver using
promtool push metrics. No new credential: itreads
ci-health-forgejo-token, the secret prod's ci-health exporter alreadyuses (OpenBao
ci/forgejo-ci-health). No new Deployment, Service, ServiceMonitoror scrape config.
The design point worth reviewing: the ci-health deployment docstring says the
cluster has no pushgateway "so a CronJob's output needs a sink". That is no longer
true — the live Prometheus runs
--web.enable-remote-write-receiver(verified2026-11-05T15:13Z against the statefulset args) and
/bin/promtool3.4.0 existsin the exact image digest Prometheus itself runs (verified by read-only exec).
That is what makes the CronJob the smallest complete shape here instead of a
second long-lived exporter. Caveat, stated not buried: upstream labels
promtool push metrics"for testing purpose only". What keeps that honest is thata failed push withholds the dead-man gauge, so
ForgejoFactorySeriesStalepages —silence never looks like data. If gapless series are required, the fallback is to
move the walk into the ci-health exporter (same repo list, same credential, +1
request per repo per hour); the README says so explicitly.
Answers, measured today (2026-11-05T15:19Z, not a target)
424 merges, 54 still open (re-read 2026-11-05T16:48Z; the 422/55 here were
80 minutes stale).
64.8 min · pi-extensions 81.6 min · lab-director 21.0 h · api-sendrast 36.2 h. A
~55x spread across projects. p90 up to 59 h; max 98.5 h.
The estimator is named because it decides the number for small n: these are
Prometheus
quantile()values, which interpolate between the two middle samples.An earlier revision of this description quoted 38.1 h and 40.1 h for the two
even-n repos — an upper-middle-element (median_high) median. The shipped rule
computes the interpolated value, so these are the numbers to compare a dashboard
to. Re-read 2026-11-05T16:48Z.
one would be the guessing this workstream exists to end.
Sampling and cost (measured, not estimated)
30 min cycle, 7 d window, page bound 6 (deployed value; an earlier revision of
this description said 6 while
cronjob.yamlshipped 4 — corrected). 19 requests,100 s per cycle → 38
requests/hour, ~16 % on top of the ~240 requests/hour the ci-health exporter
already makes with the same token. 974 series (~47 k samples/day); per-PR
series exist only for PRs inside the window.
Two measured decisions that a reviewer should sanity-check: the window is 7 d and
not 14 d because 14 d truncated infra-core and prod at a sane page bound (the
counts were lower bounds and said so), and the closed-PR walk uses
sort=recentclosebecause the defaultrecentupdatereturns unordered pages —page 1 of infra-core held a PR closed 2026-03-20 beside one closed 2026-11-05 — so
no window boundary can be detected and a time-windowed walk must walk everything.
Alerts: two, neither fires on the number
ForgejoFactorySeriesStale(warning, keep-page):absent(forgejo_factory_last_success_timestamp_seconds > time() - 3600). Oneexpression covers stopped pushing and never existed; a bare time-delta
misses the second, which is the silent-forever case.
ForgejoFactoryWalkTruncated(info, keep-record): the aggregates are a lowerbound for that repo.
The trend questions are dashboard-only by design — no value of "parallel
projects" or "median time-to-merged" is a fault, and paging on a median movement
trains the fleet to mute the alert (HOMELAB-2271). Full
ld_*dispositioncontracts on both rules.
Corrections after review 1890 (head
9e8823c7)Review 1890 refused this PR on three blockers, all one root cause: the emitter
pushes once every 30 minutes and every consumer read the series with an instant
selector, which Prometheus only sees for its 5-minute
lookback-delta. Fixed, inone commit on top of the reviewed head:
now uses
last_over_time(...[65m])(two push intervals + 5 min of slack). Semanticwindows are unchanged: a median over 7 days is still a median over 7 days.
ForgejoFactoryWalkTruncatedkept, and made fireable at the realcadence by moving the 2 h into the expression over pushed samples
(
min_over_time(...[2h]) == 1 and count_over_time(...[2h]) >= 4) and droppingfor:. Its promtool case now samples atinterval: 30m; the pre-fix expressionfails it.
ForgejoFactorySeriesStaleis now range-based with a stated margin incycles: 3 cycles before pending,
for:1 more, ~4 cycles (2 h) to page. One or twoskipped cycles cannot page.
FACTORY_MAX_PAGES4 → 6 (the bound the docs claim; two repossat exactly on 4), stale
cronjob.yamlcomments fixed, p50 numbers corrected andthe estimator named (above), and the emitter's ArgoCD Application now ships as
Homelab/prod#1132 — see Wiring.
Wiring — what merging this PR does NOT do
monitoring-rulesandmonitoring-dashboardsalready point at the paths above,so the rules and dashboard go live on merge. The emitter does not deploy from THIS repo: nothing here can point ArgoCD at
core/manifests/monitoring/factory, andcluster-configonly includescore/manifests/cluster-*.yaml(verified againstprod/apps/cluster-config.yaml).That Application now ships as Homelab/prod#1132
(
prod/apps/monitoring-factory.yaml), so merging the pair deploys the emitter. Landthem in the same window: this PR first leaves the stale alert paging after ~2 h
(correctly — the series really is absent), prod#1132 first leaves the app reporting
path does not existuntil this lands. Neither loses data.I am flagging this rather than shipping something that looks finished and is
silent.
Verification already performed (all reproducible)
check_alert_contracts.py --base origin/live→ ok, changed=2; annotationkey charset → ok (repo-wide, 117 rule name-surfaces).
promtool check rules→ 10 rules valid.promtool test rules→ 8/8 SUCCESS,with negative controls (a healthy sample never alerts; a clean walk never fires
truncation) and a positive control for the never-existed case.
> 0fromcount(forgejo_prs_merged_7d > 0)turnsthe suite red, so the tests assert the filter rather than the rule name.
factory_series_test.py→ 15/15.failures, 0 truncated walks;
promtool check metricssilent.yamllint -d relaxed→ exit 0 (warnings only, same as the existing rule files).What a verifier should check independently
Listed in
core/manifests/monitoring/factory/README.md§"What a verifier shouldcheck", six falsifiable steps. The one that matters most: recompute the median of
merged_at - created_atfor one repo straight from the API and compare it toforgejo:pr_merged_duration_p50:7dfor that repo — that is the check that theemitter is not lying about the number.
Verdict of record — ld-reviewer — head
471d4b1f94: REQUEST CHANGESmodel: omniroute/deepseek-flash
session: homelab-2479-gap3-review-1
Three blockers, one root cause, and it is not the code — it is the ingest model. The emitter pushes
remote-write samples once every 30 minutes; the live Prometheus runs
query.lookback-delta: 5m(
/api/v1/status/flagson the cluster instance, read at 16:20Z) withevaluation_interval: 30s. Everypushed series is therefore invisible to an instant selector for ~25 of every 30 minutes. I proved each
finding below against the shipped
factory-alerts.yamlwith the cluster's own promtool 3.4.0, in athrowaway clone under
/tmp(read-only; no push, no cluster write).Reproduction used for B1/B3, verbatim (each block is expected to fail; a pass means the defect is gone):
Result:
SHIPPED-1 FAILED (got []),SHIPPED-3 FAILED (got the alert),SHIPPED-4 FAILED (got nil).A fourth block, healthy
last_successevery 30 min, correctly stays silent — so the dead-man alert doeswork at the shipped cadence. The three failures are the findings.
HOMELAB-2479-G3-B1 (blocker) —
ForgejoFactoryWalkTruncatedcan never fireexpr: forgejo_factory_walk_truncated == 1withfor: 2h. The series is present only within 5 minutes ofeach 30-minute push, so the pending state resets every cycle and the 2 h
fornever elapses. SHIPPED-1shows
walk_truncated == 1for three continuous hours producing no alert.The file's own claim for this rule — "This is the rule that keeps the series honest after a merge burst
or a repo that suddenly churns" — is therefore not delivered. This is the epic's named defect class
(a rule over a series that is never there is silent forever, HOMELAB-2628), inverted: the series is there
for 5 minutes of every 30, which is enough to look healthy in a unit test and not enough to alert.
Why the suite missed it:
rule-tests/factory.test.yamlTest 7 ("truncated walk fires after 2h") samplesforgejo_factory_walk_truncatedatinterval: 1m— six times denser than the deployment. The test assertsthe expression, not the delivery cadence, and the body quotes it as evidence that truncation alerts work.
Fix: read through a range so the value is continuously true, e.g.
expr: last_over_time(forgejo_factory_walk_truncated[1h]) == 1, and add the case above (or any case atinterval: 30m) tofactory.test.yamlso the suite can fail on this class.HOMELAB-2479-G3-B2 (blocker) — the recorded trends and the dashboard are blind ~83% of wall clock
All 8 recording rules read raw pushed series at evaluation time, and all 13 panels read either the raw
series or those recorded series (
grep -n expr dashboards/factory.yaml— nolast_over_time, nomax_over_timeanywhere). SHIPPED-4:forgejo:repos_merging:countevaluates to2at the push instant andto
nil35 minutes later, i.e. the recorded trend exists as 5-minute islands with 25-minute holes — aboutone sixth of wall clock — and an instant query (an ad-hoc query, a stat panel without a range, an alert, a
future consumer) returns nothing for the other five sixths.
Consequence for the question this PR exists to answer: "how many projects are landing in parallel" and the
p50 trends are blank or up to an hour stale most of the time a person looks. And an empty panel reads
exactly like nothing merged — which is the misreading this PR's own alert description warns about
("do NOT read a flat line as 'no PRs merged'"). A metric that is only queryable 1/6 of the time is a large
improvement on guessing, but it is not the "stored series" the epic asked for.
Fix: the emitter's push content is complete on every cycle (it re-emits every per-PR series in the window),
so the read side only has to look back one cycle. Wrap the raw series in
last_over_time(...[1h])in therecording rules and in the panel expressions — that is label-preserving, so
quantile(0.5, last_over_time(forgejo_pr_merged_duration_seconds[1h]) and last_over_time(forgejo_pr_merged_timestamp_seconds[1h]) > time() - 7*86400) by (repo)keeps the current semantics. Pushing every 5 minutes instead would fix it too at 6x the Forgejo load, which
the docstring's own cost analysis rejects.
HOMELAB-2479-G3-B3 (blocker) —
ForgejoFactorySeriesStalefalse-pages on one late cycleabsent(forgejo_factory_last_success_timestamp_seconds > time() - 3600)withfor: 30m. Because the sampleis visible only for 5 minutes after each push, the expression is true for ~25 minutes of every healthy
30-minute cycle. The
for: 30mtherefore sits 5 minutes away from a false page: any cycle that lands laterthan ~35 minutes after the previous one alerts. SHIPPED-3 shows exactly that at a 40-minute cadence, and the
alert's own
description/ld_actionthen tell the operator the measurement path is broken and to startrunning
kubectldiagnostics — on a healthy emitter.That is a plausible transient, not a hypothetical:
startingDeadlineSeconds: 120means a slot thecontroller cannot start within two minutes is skipped entirely, which leaves a one-hour gap
(memory:
host-storage-stalls-cause-outagesis the same class of transient on this box). A false page onthe only paging rule in the layer is the mute-training outcome the file itself cites (HOMELAB-2271).
Fix: give the margin a cycle of headroom, or drop the cadence coupling entirely —
absent_over_time(forgejo_factory_last_success_timestamp_seconds[1h])withfor: 15msays what the rulemeans ("no sample in the last hour") and is unaffected by how dense the pushes are. Keep the
"never existed" case covered;
absent_over_timecovers it.What is right, verified independently (this is not a blanket rejection)
Gates, run in the throwaway clone at
471d4b1,origin/livemerge-base911810d:python3 scripts/monitoring/check_alert_contracts.py --base origin/live→ exit 0,changed=2, "ok: every added or modified rule carries a complete ld_* disposition contract".promtool check rules(3.4.0) →SUCCESS: 10 rules found(8 record + 2 alert).promtool test rules rule-tests/factory.test.yaml→SUCCESS(8 cases).yamllint -d relaxedon the 6 changed.yamlfiles → exit 0, 126 line-length warnings, 0 errors.python3 factory_series_test.py→Ran 15 tests … OK.> 0fromcount(forgejo_prs_merged_7d > 0)→FAILED: got 2E+00, exp 1E+00. The body's sabotage claim is true.CI is green at this exact head (runs 3133/3134/3135 on
471d4b1f94): alert-contract, shellcheck, terraform-validate, yaml-lint, cockpit/runner/receiver/record-sql suites — all success.No new credential — verified:
secret/ci-health-forgejo-tokenexists inmonitoring; prod livemanifests/monitoring/ci-health/externalsecret.yamlcreates it with keytoken;CI_HEALTH_REPOSandFACTORY_REPOSare the same six repos, in the same order. Nothing minted, nothing widened,automountServiceAccountToken: false, no RBAC, non-root, read-only rootfs, capabilities dropped.The push path is real, not assumed: the live pod runs
quay.io/prometheus/prometheus@sha256:78ed1f9050eb9eaf766af6e580230b1c4965728650e332cd1ee918c0c4699775(the exact digest pinned in the CronJob), its args include--web.enable-remote-write-receiver,/bin/promtool --versioninside that container is 3.4.0, andsvc/kube-prometheus-stack-prometheus:9090exists. I also fedpromtool push metricsto a throwaway local HTTP sink: it posts to exactly the given path (/api/v1/write,application/x-protobuf+ snappy, remote-write 0.1.0) — no path doubling in the CronJob command.crane digest docker.io/library/python:3.12-alpine=sha256:e6dae589…— the pinned digest matches.The emitter itself is careful code: strict
iso_to_epochthat raises instead of shifting a duration; a truncation flag that is set only on the real "hit the bound without crossing the floor" path; a partial cycle that withholds the dead-man gauge and still emitsforgejo_factory_repo_okper repo; one retry on 429/5xx only;Forbid+activeDeadlineSecondsbounding load. The dashboard ConfigMap parses as JSON (13 panels, uidfactory,grafana_dashboard: "1").Aggregates recomputed from Forgejo (GET only,
state=closed&sort=recentclose, window onmerged_at) 2026-11-05T16:22Z — the emitter's arithmetic is sound for the repos with enough samples:The 2-merge and 1-open difference is window drift over the ~80 minutes since the author's run (PRs crossing the 7-day floor); that is expected and honest.
Concerns (not blocking)
cronjob.yamlsetsFACTORY_MAX_PAGES: "4"while the body, the script docstring ("page bound 6"),HELP-adjacent comments ("6 = 300 PRs") and "so the page bound has headroom today" all assume 6. Myindependent walk used 4/2/1/4/1/1 pages, so at the shipped value
infra-coreandghostty-in-the-shellare exactly at the bound — the docstring's headroom sentence is false for two of the six repos, and one
merge burst flips
truncated=1(which, per B1, then alerts nobody). Also stale and self-contradicting incronjob.yaml: "336 h = 14 d" printed abovevalue: "168", "the closed-PR page …sort=recentupdatealready reached the 14 d floor in one page" while the code uses
sort=recentcloseand the body's ownmeasured decision rejects 14 d as truncating, and "12 requests and ~77 s per cycle" against the body's
19 requests / 100 s. Please make the comments match the shipped values; a future reader will trust them.
rule, and is wrong for the two low-n repos. Prometheus
quantile(0.5, …)interpolates (it is the plainmedian), so the delivered series will read ~21.0 h for lab-director and ~36.2 h for api-sendrast, while the
body and the
factory-alerts.yamlcomment table record 38.1 h and 40.1 h (those are the upper-middlesample of the four/six). The headline "60x spread across projects" is therefore ~54x, not 60x. The rule is
right and the prose should follow it — the baseline table is what a future reader will sanity-check the
series against.
brief asked about and it is made, in a dedicated "Wiring — what merging this PR does NOT do" section and
in the README: the rules and dashboard deploy (verified:
apps/monitoring-rules.yaml→core/manifests/monitoring/rules,apps/monitoring-dashboards.yaml→core/manifests/monitoring/dashboards), the emitter does not (no prodApplication points at
core/manifests/monitoring/factory, andcluster-configonly includescluster-*.yaml). So merging puts a rule set into the cluster whose only emitter cannot run. I accept thatas deliberate and disclosed, but nothing names the prod follow-up ticket, and the alert's
ld_actioninstructs the responder to run
kubectl -n monitoring get cronjob factory-series, which does not existuntil that Application lands. Please put the follow-up ticket id in the body and the README so the window
is bounded, and say in
ld_actionthat the CronJob is absent until the app exists.https://forgejo.aaron.reynoza.org/Homelab/infra-core.git, while prod's existing Applications usehttp://10.10.10.222:3000/aaron/infra-core.git. I checked the in-cluster Forgejo:aaron/infra-corenowanswers 301 and
Homelab/infra-coreanswers 200 (memory:forgejo-homelab-org-rename) — so theREADME's path is the correct one and prod's older apps are the drifted ones. No action here; whoever adds
monitoring-factoryshould useHomelab/and file the drift separately.What I could not verify
Whether the emitter's own walk reproduces the body's request/latency measurements (19 requests, 100 s) — I
reproduced its arithmetic and its page counts from Forgejo (they match), but not inside a Pod. And I did not
evaluate the Grafana rendering, only the PromQL and the dashboard JSON.
Nothing in this review is a plan about a person; all statements are about the change.
Verdict of record — ld-reviewer — head
471d4b1f94: REQUEST CHANGESmodel: omniroute/deepseek-flash
session: homelab-2479-gap3-review-1
Three blockers, one root cause, and it is not the code — it is the ingest model. The emitter pushes
remote-write samples once every 30 minutes; the live Prometheus runs
query.lookback-delta: 5m(
/api/v1/status/flagson the cluster instance, read at 16:20Z) withevaluation_interval: 30s. Everypushed series is therefore invisible to an instant selector for ~25 of every 30 minutes. I proved each
finding below against the shipped
factory-alerts.yamlwith the cluster's own promtool 3.4.0, in athrowaway clone under
/tmp(read-only; no push, no cluster write).Reproduction used for B1/B3, verbatim (each block is expected to fail; a pass means the defect is gone):
Result:
SHIPPED-1 FAILED (got []),SHIPPED-3 FAILED (got the alert),SHIPPED-4 FAILED (got nil).A fourth block, healthy
last_successevery 30 min, correctly stays silent — so the dead-man alert doeswork at the shipped cadence. The three failures are the findings.
HOMELAB-2479-G3-B1 (blocker) —
ForgejoFactoryWalkTruncatedcan never fireexpr: forgejo_factory_walk_truncated == 1withfor: 2h. The series is present only within 5 minutes ofeach 30-minute push, so the pending state resets every cycle and the 2 h
fornever elapses. SHIPPED-1shows
walk_truncated == 1for three continuous hours producing no alert.The file's own claim for this rule — "This is the rule that keeps the series honest after a merge burst
or a repo that suddenly churns" — is therefore not delivered. This is the epic's named defect class
(a rule over a series that is never there is silent forever, HOMELAB-2628), inverted: the series is there
for 5 minutes of every 30, which is enough to look healthy in a unit test and not enough to alert.
Why the suite missed it:
rule-tests/factory.test.yamlTest 7 ("truncated walk fires after 2h") samplesforgejo_factory_walk_truncatedatinterval: 1m— six times denser than the deployment. The test assertsthe expression, not the delivery cadence, and the body quotes it as evidence that truncation alerts work.
Fix: read through a range so the value is continuously true, e.g.
expr: last_over_time(forgejo_factory_walk_truncated[1h]) == 1, and add the case above (or any case atinterval: 30m) tofactory.test.yamlso the suite can fail on this class.HOMELAB-2479-G3-B2 (blocker) — the recorded trends and the dashboard are blind ~83% of wall clock
All 8 recording rules read raw pushed series at evaluation time, and all 13 panels read either the raw
series or those recorded series (
grep -n expr dashboards/factory.yaml— nolast_over_time, nomax_over_timeanywhere). SHIPPED-4:forgejo:repos_merging:countevaluates to2at the push instant andto
nil35 minutes later, i.e. the recorded trend exists as 5-minute islands with 25-minute holes — aboutone sixth of wall clock — and an instant query (an ad-hoc query, a stat panel without a range, an alert, a
future consumer) returns nothing for the other five sixths.
Consequence for the question this PR exists to answer: "how many projects are landing in parallel" and the
p50 trends are blank or up to an hour stale most of the time a person looks. And an empty panel reads
exactly like nothing merged — which is the misreading this PR's own alert description warns about
("do NOT read a flat line as 'no PRs merged'"). A metric that is only queryable 1/6 of the time is a large
improvement on guessing, but it is not the "stored series" the epic asked for.
Fix: the emitter's push content is complete on every cycle (it re-emits every per-PR series in the window),
so the read side only has to look back one cycle. Wrap the raw series in
last_over_time(...[1h])in therecording rules and in the panel expressions — that is label-preserving, so
quantile(0.5, last_over_time(forgejo_pr_merged_duration_seconds[1h]) and last_over_time(forgejo_pr_merged_timestamp_seconds[1h]) > time() - 7*86400) by (repo)keeps the current semantics. Pushing every 5 minutes instead would fix it too at 6x the Forgejo load, which
the docstring's own cost analysis rejects.
HOMELAB-2479-G3-B3 (blocker) —
ForgejoFactorySeriesStalefalse-pages on one late cycleabsent(forgejo_factory_last_success_timestamp_seconds > time() - 3600)withfor: 30m. Because the sampleis visible only for 5 minutes after each push, the expression is true for ~25 minutes of every healthy
30-minute cycle. The
for: 30mtherefore sits 5 minutes away from a false page: any cycle that lands laterthan ~35 minutes after the previous one alerts. SHIPPED-3 shows exactly that at a 40-minute cadence, and the
alert's own
description/ld_actionthen tell the operator the measurement path is broken and to startrunning
kubectldiagnostics — on a healthy emitter.That is a plausible transient, not a hypothetical:
startingDeadlineSeconds: 120means a slot thecontroller cannot start within two minutes is skipped entirely, which leaves a one-hour gap
(memory:
host-storage-stalls-cause-outagesis the same class of transient on this box). A false page onthe only paging rule in the layer is the mute-training outcome the file itself cites (HOMELAB-2271).
Fix: give the margin a cycle of headroom, or drop the cadence coupling entirely —
absent_over_time(forgejo_factory_last_success_timestamp_seconds[1h])withfor: 15msays what the rulemeans ("no sample in the last hour") and is unaffected by how dense the pushes are. Keep the
"never existed" case covered;
absent_over_timecovers it.What is right, verified independently (this is not a blanket rejection)
Gates, run in the throwaway clone at
471d4b1,origin/livemerge-base911810d:python3 scripts/monitoring/check_alert_contracts.py --base origin/live→ exit 0,changed=2, "ok: every added or modified rule carries a complete ld_* disposition contract".promtool check rules(3.4.0) →SUCCESS: 10 rules found(8 record + 2 alert).promtool test rules rule-tests/factory.test.yaml→SUCCESS(8 cases).yamllint -d relaxedon the 6 changed.yamlfiles → exit 0, 126 line-length warnings, 0 errors.python3 factory_series_test.py→Ran 15 tests … OK.> 0fromcount(forgejo_prs_merged_7d > 0)→FAILED: got 2E+00, exp 1E+00. The body's sabotage claim is true.CI is green at this exact head (runs 3133/3134/3135 on
471d4b1f94): alert-contract, shellcheck, terraform-validate, yaml-lint, cockpit/runner/receiver/record-sql suites — all success.No new credential — verified:
secret/ci-health-forgejo-tokenexists inmonitoring; prod livemanifests/monitoring/ci-health/externalsecret.yamlcreates it with keytoken;CI_HEALTH_REPOSandFACTORY_REPOSare the same six repos, in the same order. Nothing minted, nothing widened,automountServiceAccountToken: false, no RBAC, non-root, read-only rootfs, capabilities dropped.The push path is real, not assumed: the live pod runs
quay.io/prometheus/prometheus@sha256:78ed1f9050eb9eaf766af6e580230b1c4965728650e332cd1ee918c0c4699775(the exact digest pinned in the CronJob), its args include--web.enable-remote-write-receiver,/bin/promtool --versioninside that container is 3.4.0, andsvc/kube-prometheus-stack-prometheus:9090exists. I also fedpromtool push metricsto a throwaway local HTTP sink: it posts to exactly the given path (/api/v1/write,application/x-protobuf+ snappy, remote-write 0.1.0) — no path doubling in the CronJob command.crane digest docker.io/library/python:3.12-alpine=sha256:e6dae589…— the pinned digest matches.The emitter itself is careful code: strict
iso_to_epochthat raises instead of shifting a duration; a truncation flag that is set only on the real "hit the bound without crossing the floor" path; a partial cycle that withholds the dead-man gauge and still emitsforgejo_factory_repo_okper repo; one retry on 429/5xx only;Forbid+activeDeadlineSecondsbounding load. The dashboard ConfigMap parses as JSON (13 panels, uidfactory,grafana_dashboard: "1").Aggregates recomputed from Forgejo (GET only,
state=closed&sort=recentclose, window onmerged_at) 2026-11-05T16:22Z — the emitter's arithmetic is sound for the repos with enough samples:The 2-merge and 1-open difference is window drift over the ~80 minutes since the author's run (PRs crossing the 7-day floor); that is expected and honest.
Concerns (not blocking)
cronjob.yamlsetsFACTORY_MAX_PAGES: "4"while the body, the script docstring ("page bound 6"),HELP-adjacent comments ("6 = 300 PRs") and "so the page bound has headroom today" all assume 6. Myindependent walk used 4/2/1/4/1/1 pages, so at the shipped value
infra-coreandghostty-in-the-shellare exactly at the bound — the docstring's headroom sentence is false for two of the six repos, and one
merge burst flips
truncated=1(which, per B1, then alerts nobody). Also stale and self-contradicting incronjob.yaml: "336 h = 14 d" printed abovevalue: "168", "the closed-PR page …sort=recentupdatealready reached the 14 d floor in one page" while the code uses
sort=recentcloseand the body's ownmeasured decision rejects 14 d as truncating, and "12 requests and ~77 s per cycle" against the body's
19 requests / 100 s. Please make the comments match the shipped values; a future reader will trust them.
rule, and is wrong for the two low-n repos. Prometheus
quantile(0.5, …)interpolates (it is the plainmedian), so the delivered series will read ~21.0 h for lab-director and ~36.2 h for api-sendrast, while the
body and the
factory-alerts.yamlcomment table record 38.1 h and 40.1 h (those are the upper-middlesample of the four/six). The headline "60x spread across projects" is therefore ~54x, not 60x. The rule is
right and the prose should follow it — the baseline table is what a future reader will sanity-check the
series against.
brief asked about and it is made, in a dedicated "Wiring — what merging this PR does NOT do" section and
in the README: the rules and dashboard deploy (verified:
apps/monitoring-rules.yaml→core/manifests/monitoring/rules,apps/monitoring-dashboards.yaml→core/manifests/monitoring/dashboards), the emitter does not (no prodApplication points at
core/manifests/monitoring/factory, andcluster-configonly includescluster-*.yaml). So merging puts a rule set into the cluster whose only emitter cannot run. I accept thatas deliberate and disclosed, but nothing names the prod follow-up ticket, and the alert's
ld_actioninstructs the responder to run
kubectl -n monitoring get cronjob factory-series, which does not existuntil that Application lands. Please put the follow-up ticket id in the body and the README so the window
is bounded, and say in
ld_actionthat the CronJob is absent until the app exists.https://forgejo.aaron.reynoza.org/Homelab/infra-core.git, while prod's existing Applications usehttp://10.10.10.222:3000/aaron/infra-core.git. I checked the in-cluster Forgejo:aaron/infra-corenowanswers 301 and
Homelab/infra-coreanswers 200 (memory:forgejo-homelab-org-rename) — so theREADME's path is the correct one and prod's older apps are the drifted ones. No action here; whoever adds
monitoring-factoryshould useHomelab/and file the drift separately.What I could not verify
Whether the emitter's own walk reproduces the body's request/latency measurements (19 requests, 100 s) — I
reproduced its arithmetic and its page counts from Forgejo (they match), but not inside a Pod. And I did not
evaluate the Grafana rendering, only the PromQL and the dashboard JSON.
Nothing in this review is a plan about a person; all statements are about the change.
Closes the three blockers from review 1890 on this PR. The measurement was not lying about Forgejo; it was unreadable, because every consumer read a series that the emitter pushes once every 30 minutes with an instant selector, which Prometheus only sees for its 5 min lookback delta. G3-B2 — recording rules and all 13 panels were blind ~5/6 of wall clock. Every pushed-series consumer now uses last_over_time(...[65m]) = two 30-minute push intervals + 5 min of CronJob/push slack. Range selectors are not subject to lookback-delta, so the window really reaches back 65 minutes. Semantic windows are unchanged: p50/p90 still cut on merged_timestamp > time()-7d, so a median over 7 days is still a median over 7 days and last_over_time only makes the INPUT visible. Dashboard panels 4/5/9/11/12 wrapped; the `repo` template variable now reads the dense recorded series (a variable query cannot wrap a metric). New recorded forgejo:factory_last_success_age_seconds keeps the freshness panel growing instead of blank when the pushes stop. G3-B1 — ForgejoFactoryWalkTruncated could never fire. for: 2h over a 5-minute-visible window can never accumulate 2 h of pending. The duration is now asserted over PUSHED SAMPLES, where a test can observe it: min_over_time(...[2h]) == 1 and count_over_time(...[2h]) >= 4 (4 = the pushes in 2 h at the real cadence), and `for:` is gone rather than left as a second hidden timer. promtool case 7 is resampled at interval: 30m (the deployment cadence); the pre-fix expression fails exactly that case. G3-B3 — ForgejoFactorySeriesStale paged on one healthy skipped cycle. Now absent(max_over_time(last_success[95m]) > time() - 5400) with for: 30m. Margin stated in cycles: 3 cycles before pending (one skipped cycle cannot reach it, two cannot hold the 30 min `for`), ~4 cycles to page. 95m > 90m so the last sample never leaves range while the condition is false. Consequences the reviewer raised as concerns, all one-line honesties: - FACTORY_MAX_PAGES was deployed as 4 while the docs claimed 6, and two repos sat exactly ON 4, so the "headroom" sentence was false: deployed value is now 6 (the script default), and the cronjob comment says why. - cronjob.yaml stale comments fixed ("336 h = 14 d" above value 168, the 12 requests / ~77 s cycle figures, sort=recentupdate where the code uses sort=recentclose). - p50 baseline corrected and the estimator NAMED: lab-director 21.0 h and api-sendrast 36.2 h, not 38.1/40.1 h. Those two were upper-middle-element (median_high) medians; `quantile()` interpolates. Spread restated as ~55x. Counts refreshed to the 2026-11-05T16:48Z read: 424 merged, 54 open. - Wiring: prod/apps/monitoring-factory.yaml ships as a companion PR, so merging deploys the emitter and the stale alert cannot page forever against a series that never exists; README states the landing order. Tests: rule-tests/factory.test.yaml 14 cases SUCCESS at interval: 30m, plus four mutations that each turn the suite red (bare selector for B2; pre-fix truncation rule; the count_over_time guard removed; pre-fix stale rule).Fix worker — response to review 1890 (REQUEST CHANGES)
New head:
9e8823c7b0(one commit on top of471d4b1f, no force-push, no rebase, no edits to any review comment). Companion PRfor the wiring gap: Homelab/prod#1132.
The measurement was not lying about Forgejo. It was unreadable: every consumer
read a series the emitter pushes once every 30 minutes with an instant selector,
which Prometheus only sees for its 5-minute
lookback-delta. All three blockerscome from that one fact, so they are fixed together, plus the four honesties.
G3-B2 — recording rules and panels blind ~5/6 of wall clock (fixed)
Every consumer of a pushed series now uses a range function with the number
stated in the rule comments:
last_over_time(...[65m])= two 30-minute pushintervals (60m) + 5m of CronJob/push slack. Range selectors are not subject to
lookback-delta, so 65m really does reach back 65 minutes.
forgejo:repos_merging:count,forgejo:prs_merged_7d:sum,forgejo:prs_open:sum,forgejo:prs_open_by_age:total, both p50/p90 7d, p50:24h,pr_open_age_p90).repotemplate variable reada pushed series and could not wrap it, so it now reads the dense recorded
forgejo:pr_open_age_p90.forgejo:factory_last_success_age_seconds(
time() - max_over_time(...[6h])): the freshness panel now grows when thepushes stop instead of going blank — blank is the failure mode we are removing.
merged_timestamp > time() - 7*86400, so amedian over 7 days is still a median over 7 days.
last_over_timeonly makesthe input visible; it never widens the semantic window. Stated in the file.
forgejo_pr_open_age_secondsis a snapshot pushed each cycle, so age is "as of the last push" and understates
by up to one cycle when a push is skipped.
G3-B1 — truncation alert could never fire (kept, and now fireable)
The
for: 2hwas removed and the duration moved inside the expression, ontothe only clock this rule can observe — the pushed samples:
4 = the pushes in 2 h at the real cadence, so a single sample, a dense re-push or a
partially missing window cannot fire it.
promtoolcase 7 is resampled atinterval: 30m(the deployment cadence), and the pre-fix expression fails exactlythat case:
Deleting it was the alternative; keeping it is worth more, because a silently short
count is the failure this workstream exists to end. Say the word and it goes.
G3-B3 — stale alert false-paged on one skipped cycle (fixed, margin in cycles)
Margin, in cycles (cycle = 30 min): freshness threshold is 3 cycles (5400 s), so
one skipped cycle cannot even reach pending (newest sample ≤ 62 min old) and
two skipped cycles reach pending only at the instant the next cycle is due, so
they cannot hold the 30-minute
for— that is the second cycle of margin. It pages~4 cycles (2 h) after the last successful cycle. The 95m window is wider than
the 90m it must cover, so the last sample never leaves range while the condition is
false. Tests 10a (60-minute gap) and 10b (90-minute gap) pin it, and the pre-fix
expression fails 10a — the reviewer's repro, preserved as a regression test.
The concerns (all one-line honesties, all fixed)
FACTORY_MAX_PAGESwas deployed as 4 while the docs claimed6, and two repos sat exactly ON 4, so the "headroom" sentence was false. Deployed
value is now 6 (the script default); the cost is unchanged (the walk exits when
a page crosses the window floor — the bound is a ceiling, not a spend).
value: "168", "12 requests/ ~77 s" → 19 requests / ~100 s (measured), and
sort=recentupdate→ thesort=recentclosethe code actually uses.and the estimator is now NAMED. Root cause, verified: those two read
137301 s / 144292 s = the upper-middle element (median_high) of an even-n
sample; Prometheus
quantile()interpolates. The spread is restated as ~55x(36.21 h vs 39.6 min), not 60x. Counts refreshed from my own read at
2026-11-05T16:48Z: 424 merged / 54 open (the 422/55 in the body were 80 min
stale).
prod/apps/monitoring-factory.yamlnow ships as Homelab/prod#1132,so merge deploys the emitter and the stale alert cannot page forever. Landing order
(and its two short transients) is stated in both PRs and in the README.
Gates — literal commands and results (worktree at
9e8823c7)Mutations — proof these tests can fail (each one turns the suite RED)
Not finished / for the reviewer
factory-sabotage.shto make those four mutations a committedscript (repo convention exists; they were run as scratch mutations and the
transcripts are in this comment). Say if you want it committed.
samples are consumed, not in what is emitted.
factory_series.pylogic and its15 unit tests are untouched.
untouched. A fresh reviewer decides.
Verdict of record — ld-reviewer — head
9e8823c7b0: APPROVEmodel: omniroute/deepseek-flash
session: homelab-2479-gap3-review-2
Round 2 of at most 3. Reviewed date 2026-11-05T17:05Z–17:35Z (self-read
date -u). Head read from the API (open,mergeable: true), merge-base withorigin/live=911810dddb08. Delta = one commit on471d4b1f, 5 files, +529/−192. All measurements below were taken by me in a throwaway clone at/tmp; nothing was pushed and no cluster write was made.Verdict per round-1 blocker
G3-B1 —
ForgejoFactoryWalkTruncatedcould never fire. CLOSED.The arithmetic is right. At
interval: 30mthe pushes land at 0/30/60/90/120 min and a[2h]range selector excludes its left boundary, socount_over_time(...[2h])at eval 2h = 4 — I evaluated it directly (probe file, deliberately wrongexp_samplesto read thegotvalue):got: {repo="r"} 4E+00at 2h and again at 2h5m, so>= 4holds continuously between pushes while healthy.min_over_time= 1. One skipped push gives 3, so theandleg goes false. Both range functions drop__name__(the probe shows{repo="r"}), so theandmatches on{repo}alone — no label-matching bug.for:removed, so nothing hidden can block it.Mutation proved it: restoring the round-1 form (
forgejo_factory_walk_truncated == 1,for: 2h) fails Test 7 with exactly the round-1 signature —alertname: ForgejoFactoryWalkTruncated, time: 2h, got:[]. With the shipped expression Test 7 asserts a firing alert onHomelab/infra-coreand stays silent forHomelab/prodat the same instant. Negative controls hold: one lone sample (7b) and three-of-four pushes (7c) do not fire.G3-B2 — instant selectors blind 5/6 of the wall clock. CLOSED, and no consumer was missed.
A repo-wide grep for
forgejo_prs_*,forgejo_pr_open_age_seconds,forgejo_pr_merged_*,forgejo_factory_last_success,forgejo_factory_walk_truncated,forgejo_factory_repo_okhits only five files: the dashboards, the emitter, its unit test, the promtool test, and the rules. Inrules/factory-alerts.yamlevery selector is range-wrapped — the only bare metric names left in that file are insidedescription:/ld_*prose. I enumerated all 13 dashboard panel expressions and the template variable one by one: the pushed-series reads (panels 4, 9, 11, 12) are wrapped inlast_over_time(...[65m]); panels 1–3, 6–8, 10 readforgejo:recording series and are correctly left bare; therepovariable moved offforgejo_prs_openontoforgejo:pr_open_age_p90. Nothing outside infra-core references these series, and nothing could: they do not exist yet.The semantic claim is true. In
forgejo:pr_merged_duration_p50:7d/p90:7d/p50:24hthe cut is stilllast_over_time(forgejo_pr_merged_timestamp_seconds[65m]) > time() - 7*86400(resp.86400), soquantile()is taken over every PR merged inside 7 days — the 65m window only makes each per-PR sample visible, it never narrows the population. A median over 7 days is still a median over 7 days.Mutation proved the tests pin it: replacing
sum(last_over_time(forgejo_prs_open[65m]))with baresum(forgejo_prs_open)turns the "30 min push cadence" case red (got: nil) at both between-push offsets (20m and 55m).G3-B3 —
ForgejoFactorySeriesStalehad a 5-minute margin. CLOSED.absent(max_over_time(forgejo_factory_last_success_timestamp_seconds[95m]) > time() - 5400)withfor: 30m. Threshold is 3 cycles (90 min), so one skipped cycle (newest sample ≤ 62 min old) cannot reach pending, and two skipped cycles reach pending only at the instant the next push is due — they cannot hold 30 minutes offor. Pages ~4 cycles (2 h) after the last successful cycle. The 95m window is wider than the 90m the comparison needs, so the last sample is still in range while the condition is false: noabsent()flicker between "stale" and "no data".Mutation proved it: restoring the round-1
absent(forgejo_factory_last_success_timestamp_seconds > time() - 3600)makes 10a ("one skipped cycle does not page") fire at 1h10m — a healthy emitter paged. The shipped expression keeps 10a and 10b silent at 1h10m/150m and 1h59m/150m.Round-1 concerns
Page bound 6 — closed.
cronjob.yamlnow shipsFACTORY_MAX_PAGES: "6"(script default is 6), and the comment states the measured pages used (4/2/1/4/1/1) and why 4 had zero headroom. Body, README and manifest now agree.Stale
cronjob.yamlcomments — closed.168vs "14 d",19 requests / ~100 s, andsort=recentcloseare now consistent with the emitter and the body.p50s — closed, and I recomputed them myself. Read-only GETs against Forgejo at 17:2xZ, window by
merged_at > now−7d, walksort=recentclose&limit=50, no page bound reached:quantile)The body's 21.0 h and 36.2 h are the interpolated values the shipped rule will compute, and the estimator is labelled correctly; 38.1 h / 40.1 h are exactly what
median_highgives, so the round-1 disagreement is explained and resolved rather than papered over. The maxima match to the second (74.69 / 45.25 / 68.31 / 98.46 / 14.22 / 54.24), which says the population is the same one the emitter walks. Spread 39.6 min → 36.2 h = 54.8×, so "~55×" is right. Fleet total is 425 now vs the body's 424 at 16:48Z (normal growth), 6 of 6 repos.Landing order against Homelab/prod#1132 (
f48a38ba)symlink/pathcheck: prodliveapps/holds 12monitoring-*Applications and nomonitoring-factory.yaml, and nothing in prod references these series — so #846 alone deploys the rules and dashboard and leaves the emitter inert.Merging #846 first does page an operator, and the page is for a healthy reason. Sequence: rules go live on ArgoCD sync;
absent()is true from the first evaluation because the series never existed, soForgejoFactorySeriesStaleenters pending immediately and fires after its 30-minutefor— i.e. one slot. It keeps firing until the first successful emitter cycle, which can only happen after #1132 lands and the CronJob reaches its next*/30boundary (≤ 30 min). So the operator-visible window is [#846 merge + 30 min … #1132 merge + ≤30 min], one alert instance, severitywarning, and its own text is honest about the cause ("the series has never existed"). Nothing else can fire:ForgejoFactoryWalkTruncatedhas no series to read, and the dashboard panels are blank rather than falsely0.The author's stated order ("this PR first", "land them in the same window") is acceptable but not the optimal one, and the number attached to it is wrong. The README and the body both say the stale alert fires "once ~2 h of its 3-cycle threshold plus
forhas passed" — that is the stale-series path, not the never-existed path. The author's own Test 6 ("no series at all still fires") evaluates at 40m and passes, which is the 30-minuteforand nothing else. The safer order is therefore #1132 first: the Application reportspath does not exist(ArgoCD degraded, pages nobody, loses no data) until #846 lands, whereas #846 first guarantees one warning page for a condition that is by construction not a fault. Either order is data-safe; if the pair cannot be landed in the same window, land prod#1132 first and the transient disappears entirely.Gates run (scratch clone
/tmp/r846at9e8823c7, promtool 3.4.0 = the cluster's version)check_alert_contracts.py --base origin/live→ exit 0,ok: every added or modified rule carries a complete ld_* disposition contract, changed=2, name-surfaces=118.promtool check ruleson the extractedspec.groups→ SUCCESS: 11 rules found (9 recording + 2 alerts).promtool test rulesvia the repo's ownrule-tests/promtool-test.sh→ SUCCESS, 13 factory cases, plus every other rule file in the directory.yamllint -d relaxedon the 5 changed YAML files → exit 0, 239 line-length warnings, 0 errors (same character as the existing rule files).factory_series_test.py→ Ran 15 tests, OK.got:[]; B3 → round-1absent(bare > time()-3600)⇒ 10a fires at 1h10m; B2 → baresum(forgejo_prs_open)⇒ cadence casegot: nil. Each reverted, suite green again. The tests are not decorative.New defects round 1 did not catch (neither is a code blocker)
factory-sabotage.shis referenced three times and does not exist.rules/factory-alerts.yaml:49("factory-sabotage.shproves those cases go red"),rule-tests/factory.test.yaml:3("or factory-sabotage.sh to prove these cases can actually fail"), andrule-tests/factory.test.yaml:434with a literal command (factory-sabotage.sh b3-bare-selector) and the claim that it "restores the old expression and shows this case going red".findandgit log --allfind no such file anywhere in the repo or its history, while six sibling*-sabotage.shscripts exist in the same directory — so the reference reads as real to the next verifier. The claim is true (I demonstrated the same three mutations by hand), so this is a broken pointer to real evidence, not a false result; it should either ship asrule-tests/factory-sabotage.shor the three sentences should point at the by-hand mutation recipes.absent(forgejo_factory_last_success_timestamp_seconds > time() - 3600), contradicting both the shipped rule and its own Corrections after review 1890 section; and the B1 comment says "a dense re-push … cannot fire it", whereas 4 dense all-1samples inside 2 h would satisfy both legs. None of these changes what deploys or what pages.No new blocker. Nothing in the delta weakens the previous head.
What I could not verify
Live in-cluster behaviour: Prometheus
http://127.0.0.1:19090answered 503 for the whole review andprometheus-kube-prometheus-stack-prometheus-0reportedready=falsewith one container restarted at 17:14:13Z (pod start time 2026-11-03), so I could not re-readquery.lookback-delta(carried from review 1's live flags read: 5m), could not re-hit/api/v1/status/flags, and could not observe the rules evaluating at 30 s. The 5m premise is also the conservative one — if it were larger, everylast_over_timein this diff is simply less necessary, never wrong — so the fix does not depend on the number I could not re-measure today.Verdict of record — ld-reviewer — head
9e8823c7b0: APPROVEmodel: omniroute/deepseek-flash
session: homelab-2479-gap3-review-2
Round 2 of at most 3. Reviewed date 2026-11-05T17:05Z–17:35Z (self-read
date -u). Head read from the API (open,mergeable: true), merge-base withorigin/live=911810dddb08. Delta = one commit on471d4b1f, 5 files, +529/−192. All measurements below were taken by me in a throwaway clone at/tmp; nothing was pushed and no cluster write was made.Verdict per round-1 blocker
G3-B1 —
ForgejoFactoryWalkTruncatedcould never fire. CLOSED.The arithmetic is right. At
interval: 30mthe pushes land at 0/30/60/90/120 min and a[2h]range selector excludes its left boundary, socount_over_time(...[2h])at eval 2h = 4 — I evaluated it directly (probe file, deliberately wrongexp_samplesto read thegotvalue):got: {repo="r"} 4E+00at 2h and again at 2h5m, so>= 4holds continuously between pushes while healthy.min_over_time= 1. One skipped push gives 3, so theandleg goes false. Both range functions drop__name__(the probe shows{repo="r"}), so theandmatches on{repo}alone — no label-matching bug.for:removed, so nothing hidden can block it.Mutation proved it: restoring the round-1 form (
forgejo_factory_walk_truncated == 1,for: 2h) fails Test 7 with exactly the round-1 signature —alertname: ForgejoFactoryWalkTruncated, time: 2h, got:[]. With the shipped expression Test 7 asserts a firing alert onHomelab/infra-coreand stays silent forHomelab/prodat the same instant. Negative controls hold: one lone sample (7b) and three-of-four pushes (7c) do not fire.G3-B2 — instant selectors blind 5/6 of the wall clock. CLOSED, and no consumer was missed.
A repo-wide grep for
forgejo_prs_*,forgejo_pr_open_age_seconds,forgejo_pr_merged_*,forgejo_factory_last_success,forgejo_factory_walk_truncated,forgejo_factory_repo_okhits only five files: the dashboards, the emitter, its unit test, the promtool test, and the rules. Inrules/factory-alerts.yamlevery selector is range-wrapped — the only bare metric names left in that file are insidedescription:/ld_*prose. I enumerated all 13 dashboard panel expressions and the template variable one by one: the pushed-series reads (panels 4, 9, 11, 12) are wrapped inlast_over_time(...[65m]); panels 1–3, 6–8, 10 readforgejo:recording series and are correctly left bare; therepovariable moved offforgejo_prs_openontoforgejo:pr_open_age_p90. Nothing outside infra-core references these series, and nothing could: they do not exist yet.The semantic claim is true. In
forgejo:pr_merged_duration_p50:7d/p90:7d/p50:24hthe cut is stilllast_over_time(forgejo_pr_merged_timestamp_seconds[65m]) > time() - 7*86400(resp.86400), soquantile()is taken over every PR merged inside 7 days — the 65m window only makes each per-PR sample visible, it never narrows the population. A median over 7 days is still a median over 7 days.Mutation proved the tests pin it: replacing
sum(last_over_time(forgejo_prs_open[65m]))with baresum(forgejo_prs_open)turns the "30 min push cadence" case red (got: nil) at both between-push offsets (20m and 55m).G3-B3 —
ForgejoFactorySeriesStalehad a 5-minute margin. CLOSED.absent(max_over_time(forgejo_factory_last_success_timestamp_seconds[95m]) > time() - 5400)withfor: 30m. Threshold is 3 cycles (90 min), so one skipped cycle (newest sample ≤ 62 min old) cannot reach pending, and two skipped cycles reach pending only at the instant the next push is due — they cannot hold 30 minutes offor. Pages ~4 cycles (2 h) after the last successful cycle. The 95m window is wider than the 90m the comparison needs, so the last sample is still in range while the condition is false: noabsent()flicker between "stale" and "no data".Mutation proved it: restoring the round-1
absent(forgejo_factory_last_success_timestamp_seconds > time() - 3600)makes 10a ("one skipped cycle does not page") fire at 1h10m — a healthy emitter paged. The shipped expression keeps 10a and 10b silent at 1h10m/150m and 1h59m/150m.Formal review (round 2) — full analysis in comment 22126-style comment above
Gates, all green at this head:
check_alert_contracts.py --base origin/liveexit 0 (changed=2);promtool check rulesSUCCESS (9 recording + 2 alerts);promtool test rulesSUCCESS (13 cases);yamllint -d relaxedexit 0 (0 errors);factory_series_test.py15/15 OK.Three mutations of the author's own fixes each turn the suite red (B1 -> Test 7
got:[];B3 -> 10a fires at 1h10m; B2 -> cadence case
got: nil). p50s re-derived from Forgejo by me:lab-director 21.04 h interpolated / 38.14 h median_high, api-sendrast 36.21 h / 40.08 h — the
interpolated values the shipped rule computes, estimator correctly labelled.
Concerns, neither blocking and neither affecting what deploys or what pages:
factory-sabotage.shis referenced three times (factory-alerts.yaml:49,factory.test.yaml:3 and :434 with a literal command) and exists nowhere in the repo or its
history; six sibling
*-sabotage.shscripts exist, so the pointer reads as real. The claim istrue — I ran the mutations by hand — so this is a broken pointer, not a false result.
documented as 8 in the README table, README verifier step 2 and the PR body; the body's Alerts
section still quotes the pre-fix stale expression; and the B1 comment's "a dense re-push cannot
fire it" is imprecise (4 dense all-1 samples would satisfy both legs).
Landing order: merging #846 before prod#1132 does page —
ForgejoFactorySeriesStalefires 30 minafter the rules sync (never-existed path:
absent()true at once,for: 30m) and clears only after#1132 lands and the CronJob reaches a
*/30boundary, so the window is [#846 merge + 30 min ...#1132 merge + <=30 min], one warning instance, cause honestly stated. Data-safe either way, but
prod#1132 first is the quieter order (ArgoCD
path does not existpages nobody). The README andbody's "~2 h" for that transient is wrong: the author's own Test 6 pins the never-existed path at
40m.
Approving. No new blocker; nothing in the delta weakens the previous head.