Skip to content

fix: hold cascading child pages, preserve rollup attribution, document file storage - #296

Merged
fclairamb merged 46 commits into
mainfrom
batch/2026-08-31
Sep 1, 2026
Merged

fclairamb merged 46 commits into
mainfrom
batch/2026-08-31

Conversation

@fclairamb

@fclairamb fclairamb commented Sep 1, 2026 •

Copy link
Copy Markdown
Owner

Three fixes from the 2026-08-31 batch, all rooted in the RabbitMQ nonprod outage of 2026-08-30 and a silent upload-loss confirmed on the k8xp dev cluster.

1. A child no longer pages while a hard parent is still validating

The forward rollup was damage control: it suppressed the remaining pages after a late-confirming parent opened, but could not recall those already sent. During the 2026-08-30 outage that residue was 5 Slack reports instead of 1 — the dependents' health endpoints flipped to 503 within milliseconds, while the parent probe only observed its first failure period + timeout later (26 s observed). Both sides then waited the same 120 s confirmation, so the gap was preserved rather than closed.

The static fix — requiring parent.period <= child.period — is disproven by the outage itself: both inequalities already held and 5 pages still escaped. So the hold is dynamic runtime state instead:

A check whose confirmation period has elapsed does not open an incident while a hard ancestor is itself validating — it stays validating one more tick. When the ancestor confirms first, the child's eventual open is suppressed by the existing backward walk before anything is sent.

  • Zero cost when healthy — a check failing alone confirms at exactly its configured period. No permanent latency tax, which a config margin cannot avoid.
  • No new machinery — no timers, no recall, no schema change, no config knob.
  • Each ancestor gates only until a hold cap (FirstFailureAt + confirmation + period + timeoutOrDefault), so a paused or wedged parent frozen in validating can hold a child for at most one cap window.
  • Down and Up ancestors never gate; soft edges are never consulted.
  • The gate sits before createOrReopenIncident, so reopen-after-cooldown relapses are held too, and holdConfirmation is threaded into the status derivation so a held check displays validating rather than down-with-no-incident.

Also adds a soft, never-blocking warning on GET /checks/$check/dependencies (and an amber alert in dash0) for hard edges whose margin is too short to detect the same outage.

2. Rollup detach no longer erases the cascade's audit trail

markRollupDetached cleared both paging_suppressed and caused_by_incident_uid, and emitted nothing. After the outage, 10 of 11 dependent incidents read as though rollup had never run — the investigation had to be reconstructed from the events feed.

  • Detach now clears only paging_suppressed; the attribution survives as the post-mortem record. Safe because nothing downstream reads a non-nil caused_by as "currently suppressed" (ListSuppressedChildIncidents filters on paging_suppressed), and the reopen path resets attribution from scratch.
  • Adds the incident.rollup_detached event the wiki already documented but which never existed, routed to "never pages".
  • The preserved property — a detached child relapsing before its incident resolves still pages — is covered by a dedicated test.

3. File storage is documented, and the install guides no longer lose every upload

grep -ril filestorage web/docs/ returned nothing, while SP_FILESTORAGE_LOCAL_ROOT defaults to ./data/files relative to the container's working directory. Every install guide mounted a volume for Postgres and none for uploads, so org logos, status-page assets and incident screenshots landed on the ephemeral writable layer — silently. Nothing fails at write time; the loss surfaces later as 500 read file: file not found in storage.

  • SP_FILESTORAGE_LOCAL_ROOT was also not read at all: koanf collapses it to filestorage.local.root, missing the local_root tag, so a container told to store blobs on a mounted volume kept writing inside the image. Fixed in applyFileStorageEnv.
  • New /configuration/file-storage page: both backends, credential resolution, MinIO / OVHcloud / AWS worked examples, and the backend-switching guarantee — verified against GetStorageForURI, which resolves reads by the scheme recorded at write time — with the "old backend must stay reachable" caveat stated.
  • Volumes and SP_FILESTORAGE_LOCAL_ROOT added to the Kubernetes, Docker, Docker Compose and homepage quick-start guides, including the ReadWriteOnce single-replica caveat and the S3 block for multi-replica.

Testing

  • 9 table-driven cases for the confirmation hold and 4 for the detach contract, each on both dialects (SQLite + Postgres). Both suites include positive controls, and both were mutation-tested — reverting the fix makes them fail.
  • make build-backend lint-back test green; make build-dash0, bun run lint (no new errors), bun run test:unit green; make build-docs green.

Every spec passed an independent completeness audit before being archived.


4. Every ingest path now uses the configured check timeout

incidents.Service.defaultCheckTimeout is the last term of the confirmation-hold cap added in §1. The constructor seeds it to a 15 s fallback and expects callers to overwrite it with sp.check_timeout_ms — but the heartbeat, inbound-mail and MCP ingest paths never did. At shipped defaults the values coincide, so nothing was observable; the moment an operator raises or lowers the ceiling, the same check gets a different hold cap depending on which path processed the result.

The structural cause was the ms→Duration conversion being copy-pasted per call site, so a site that forgot to copy it inherited the fallback with no signal. SchedulingConfig.CheckTimeout() is now the single conversion, and heartbeat.NewService / emailcheck.NewHandler take the timeout as a required parameter — mirroring the existing publicationHook rationale — so a new call site has to make a deliberate choice rather than silently inherit the bug.

Each of the three wiring tests carries a positive control (NotEqual(fallback, configured), so it cannot pass vacuously) and a companion case pinning the other half of the contract: non-positive input keeps the fallback rather than collapsing to a zero-length hold that would release every child immediately.

5. The empty-org onboarding hero no longer outlives the first check

On production the dashboard kept showing "create your first check" for well over a minute after a check existed. The hero is gated on stats.total === 0 from GET /orgs/:org/checks/stats, and three caches stacked up: a 60 s server-side snapshot with deliberately no invalidation, no client mutation invalidating ["check-stats", org], and a poll timer stretched to 5 minutes while the live socket is connected. Worst case the hero sat over a non-empty org for several minutes, and a page reload only bypassed two of the three layers.

All three are fixed, the server-side bust being load-bearing:

  • Server — membership-changing writes call checkStatsCache.invalidate. The two chokepoints insertCheckResolvingSlugRace and Service.DeleteCheck cover create, upsert-create, import, apply (including its prune loop) and clone; s.db.CreateCheck has exactly one call site, so nothing in that set escapes. Worker status transitions deliberately keep riding the TTL — only total crossing zero flips the page mode.
  • Client — useCreateCheck / useDeleteCheck now invalidate ["check-stats", org].
  • Live map — check-stats added under the checks kind, and deliberately not under results: that firehose is what spec 2026-08-09-07 keeps away from org-wide refetches.

Both control tests that keep the fix honest are present: a server test proving a plain result write does not bust the cache (preserving the TTL trade-off), and a frontend test asserting a results-kind live hint does not invalidate the stats query.

The E2E onboarding assertion is authored but was not run locally — the shared dev server isn't in SP_RUNMODE=test. CI runs it.

The web/docs/.gitignore already whitelists /docs/api/_category_.json
(comment: "The category label is kept") but the file was never created,
so Docusaurus fell back to the raw directory name "api" in the sidebar.
gen-changelog.ts reads CHANGELOG.md via ../../../CHANGELOG.md, resolving to
/build/CHANGELOG.md inside the image build, but the docs-builder stage never
copied it in — so every container build silently fell back to the
"No changelog available yet." placeholder. The root .dockerignore's bare
*.md rule also excludes it from the build context, so negate that
specifically for CHANGELOG.md.
….example

koanf's env loader collapses SP_FILESTORAGE_LOCAL_ROOT to
filestorage.local.root, which misses the "local_root" koanf tag, so the
variable was silently dropped: a container told to store blobs on a mounted
volume kept writing to ./data/files inside the image and lost every upload
on restart. Read it in applyFileStorageEnv alongside the SP_FILESTORAGE_S3_*
vars, register it in the manual-reader list, and document both backends
(plus the container volume warning) in .env.example.
markRollupDetached no longer clears caused_by_incident_uid when a rollup
child's check recovers before its parent resolves — only paging_suppressed
flips. The attribution is the post-mortem record of the cascade; nothing
downstream reads a non-nil caused_by as "currently suppressed" (that's
paging_suppressed, gated by ListSuppressedChildIncidents), and the reopen
path already resets attribution from scratch on every reopen, so no stale
value can leak into a later relapse.

Also add the incident.rollup_detached event type (previously documented in
the wiki but never implemented) and emit it from the recovered branch with
the parent incident/check uids, mirroring incident.rolled_up's payload
shape. Routed to "never pages" in queueLifecycleNotifications.
Add the translated label to all four dash0 locales (en/fr/de/es — the
spec's own text said "six locales" but dash0 only ships four) and mark the
new event type intentionally unmapped in EVENT_TYPE_REGISTRY, mirroring
incident.rolled_up: amber incident-family fallback is enough, it is a
timeline record, not a page.
Three corrections: the recovered branch now actually emits
incident.rollup_detached and keeps causedByIncidentUid (previously
documented behavior that didn't exist in code); "skips the fan-out for
everything except resolved" was backwards — pagingSuppressed gates
resolved paging too; and a new paragraph explains why keeping the
attribution on detach is safe across both the relapse-before-resolve and
reopen-from-scratch cases.
Both dialects (SQLite + embedded Postgres), all 4 cases from the spec:
detach keeps caused_by_incident_uid and emits incident.rollup_detached
without paging; a relapse before the child's own incident resolves still
pages, proving the kept attribution is never misread as "still
suppressed"; the still-down branch is unchanged (reopens and pages); and a
reopen with no live parent is a fresh rollup decision that clears
attribution from scratch.

Verified case 1/2/4 genuinely fail if markRollupDetached regresses to
clearing caused_by_incident_uid (case 3 correctly stays green, since it
exercises the untouched branch).

Also fixes the one pre-existing test this changed the contract for:
TestParentResolveStillDetachesRecoveredChild asserted the old (buggy)
"attribution cleared on detach" behavior; it now asserts attribution is
kept, matching TestParentResolveUnsuppressesStillFailingChild's sibling
assertion for the still-down branch.
…nnel switch

applyActivationEvent's EventType switch is exhaustive-linted and missed
the new event type added for the rollup-detach fix. Unreachable for
activation-funnel purposes, like its incident.rolled_up neighbor.
make build-backend, lint-back and test all green; make build-dash0,
bun run lint (baseline-only, no new errors) and bun run test:unit
(1235/1235) all green.
incidents.Service.defaultCheckTimeout is the last term of the rollup
confirmation-hold cap in ancestorHoldRemaining. The constructor seeds it to
DefaultCheckTimeoutFallback (15s) and expects callers to overwrite it with
the operator-configured scheduling.check_timeout_ms, but the heartbeat,
inbound-mail and MCP ingest paths never did — so the moment an operator
raised or lowered the ceiling, the same check got a different hold cap
depending on which path processed the result.

The structural cause was the ms-to-Duration conversion being copy-pasted per
call site, letting a site that forgot to copy it inherit the fallback with no
signal. SchedulingConfig.CheckTimeout() is now the single conversion, and
heartbeat.NewService / emailcheck.NewHandler take the timeout as a REQUIRED
parameter so a new call site has to make a deliberate choice rather than
silently inherit the bug.

Implemented by a concurrent session; committed here after an independent
completeness audit against the spec.
Layer 1 of spec 2026-09-01-01: create, delete and clone each bust the
per-org check-stats cache immediately, and a plain result write does not
(the control preserving the TTL trade-off).
Layer 2 of spec 2026-09-01-01: useCreateCheck and useDeleteCheck now
invalidate ["check-stats", org] alongside the checks list queries, so
the dashboard's empty-org hero gate does not wait out its own poll/TTL
window after a check is created or deleted.
Layer 3 of spec 2026-09-01-01: add check-stats under the checks entity's
checks kind in DEFAULT_QUERY_ROOTS so a real membership/status transition
refreshes the dashboard's empty-org gate immediately instead of waiting
for the 5-minute lazy poll. Deliberately NOT attached to the results
kind (spec 2026-08-09-07's firehose guard) — covered by a negative test.
Layer 1's regression proof end to end: the quick-create-first-check test
no longer stubs the post-create stats response — it now asserts the real,
unstubbed dashboard reload sees the new check immediately (KPI tiles
render, hero gone) instead of racing the old minute-long server cache.
Not run locally: the :4000 devloop backend is not in SP_RUNMODE=test
right now (test-user seed endpoint 405s) and is shared with concurrent
work, so this is authored-but-not-run per the spec's guidance.
@fclairamb
fclairamb enabled auto-merge (squash) September 1, 2026 00:58
@fclairamb
fclairamb merged commit 0318192 into main Sep 1, 2026
12 checks passed
@fclairamb
fclairamb deleted the batch/2026-08-31 branch September 1, 2026 02:04
@fclairamb fclairamb mentioned this pull request Sep 1, 2026
fclairamb added a commit that referenced this pull request Sep 1, 2026
🤖 I have created a release *beep* *boop*
---


##
[0.21.2](v0.21.1...v0.21.2)
(2026-09-01)


### Bug Fixes

* **deps:** update go dependencies (non-major)
([#293](#293))
([5e962ac](5e962ac))
* **deps:** update go dependencies (non-major)
([#297](#297))
([2c2fbdb](2c2fbdb))
* **deps:** update module github.com/prometheus/client_model to v0.6.3
([#295](#295))
([d2d936b](d2d936b))
* hold cascading child pages, preserve rollup attribution, document file
storage ([#296](#296))
([0318192](0318192))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).
This was referenced Sep 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant