fix(selfhost,observability): close three durable-wrong-state gaps, label the latency histogram, probe the browser, and add memory to host pressure (#9544, #9545) - #9547
Merged
Conversation
Contributor
|
Tip ✅ LoopOver review result - approve/merge recommendedReview updated: 2026-07-28 07:54:44 UTC
Review summary Nits — 5 non-blocking
Decision drivers
Context & advisory signals — never blocks the verdict
Linked issue satisfactionAddressed Review context
Contributor next steps
Signal definitions
🧪 Chat with LoopOverAsk LoopOver a question about this PR directly in a comment — grounded only in the same cached, public-safe facts shown above, never a new claim.
Full command reference: https://loopover.ai/docs/loopover-commands 🧪 Experimental — new and may change. 🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed 💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →. Checked by LoopOver, a quiet PR intelligence layer for OSS maintainers.
|
Contributor
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
loopover-ui | 8ee87ad | Commit Preview URL Branch Preview URL |
Jul 28 2026, 07:37 AM |
Bundle ReportChanges will increase total bundle size by 77 bytes (0.0%) ⬆️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: loopover-uiAssets Changed:
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #9547 +/- ##
==========================================
- Coverage 89.58% 88.68% -0.90%
==========================================
Files 846 846
Lines 110332 110384 +52
Branches 26277 26295 +18
==========================================
- Hits 98837 97898 -939
- Misses 10231 11513 +1282
+ Partials 1264 973 -291
Flags with carried forward coverage won't be shown. Click here to find out more.
|
![loopover-orb[bot]](https://avatars.githubusercontent.com/in/4139483?s=60&v=4){kind=small}
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The repo-code half of the live
edge-nl-01inspection: three gaps that turn a transient event into a durable wrong state, and three observability gaps where a real signal was either unattributable or absent entirely.Closes #9544
Closes #9545
#9544 — durable wrong states
An empty secret file silently degraded to "unset". A missing or unreadable
<NAME>_FILEcorrectly throws (#6284), but a zero-byte or whitespace-only one setenv[NAME] = ""— which every downstreamnonBlank()reads as unconfigured, andpreflight.tsdeliberately skips absent values. A truncatedGITHUB_WEBHOOK_SECRETfile therefore booted an instance that rejected every webhook, healthily, indefinitely. Directly adjacent to the known rotation footgun on this host, where the file is rewritten in place: the window in which it is momentarily empty is exactly when a container restart reads it.The check sits outside the existing try/catch on purpose — throwing inside it would be caught and re-reported as "unreadable", collapsing a bad path and a truncated write into one misleading message and the wrong log event. They are different operator problems.
The crash handler could stall exit behind a telemetry flush. It awaited
flush()with no deadline beforeexit(1), so a wedged PostHog egress delayed — or entirely prevented — the very restart the module exists to guarantee, leaving a process that had already logged a fatal alive and serving from a state it declared unsound. NowPromise.raceagainst a 3s deadline (the timerunref()'d, so it can never itself extend the lifetime it bounds). Losing a few telemetry events is unambiguously the cheaper failure.Half-written blobs were served forever. Keys are input-hash-addressed, so a truncated PNG from a mid-write kill is never retried or overwritten — the next lookup for that input finds a file and serves it as a permanently "valid" cache hit. Now tmp+rename with a UUID temp name (so concurrent puts of one key cannot publish each other's partial file) and cleanup on the failure path.
#9545 — observability
The latency histogram had no route label, so
LoopoverRequestLatencySLOBreachfiring at p95 = 9.75s named nothing:sum by (le, route) (...)returned a single unlabelled series. The reason it had no label is the constraint the fix had to respect — a raw path is unbounded cardinality, which takes a Prometheus down rather than merely misinforming it.httpRouteGroupis therefore an allowlist, not a path sanitizer. A "replace ids with :param" normalizer still trusts the path's shape, and one unmatched pattern reintroduces unbounded cardinality silently; an allowlist can only ever be wrong in the safe direction. Every unknown path — including every 404 and every attacker probe — collapses to oneotherbucket, pinned by a 500-iteration fuzz sweep asserting no value outside the fixed set ever appears.BROWSER_WS_ENDPOINThad no readiness probe — the only dependency the review path needs that didn't. A browserless outage was invisible in/readywhile silently changing gate outcomes (#9464 stopped that closing PRs; this makes the outage itself visible rather than inferred afterwards). Registered only when configured, probes the cheap HTTP sibling rather than launching Chromium, drops any?token=so it can't reach a log, fails closed on a configured-but-unparseable endpoint, and caches/single-flights so/readypolling can't itself load a struggling backend.Host pressure watched CPU only. On this box the realistic killer is memory — Ollama at ~9.9 GiB beside browserless at ~1.5 GiB — and nothing observed it, so the OOM killer decided instead, taking the container and every in-flight job rather than deferring one maintenance job.
hostMemoryUsedFractionjoins load as a peer signal with the identical fail-opennullmeans "skip" contract, gates the drain escape too, and leaveshost_load_highoutranking it so existing alerts keep their reason string.Scope
The config-only items from the parent — the 8-day-stale
LoopoverBackupMissing, the latency alert's minimum-rate gate, and the 44% false-positivesecret_leakgate — are not here. They live in alertmanager rules and the private per-repo.loopover.ymlonedge-nl-01, not in this repo, and they are tracked in #9546 so neither half blocks the other. I have not touched production config.Validation
npx tsc --noEmit,selfhost:env-reference:check(166 vars, picked up the new knob),db:migrations:check,git diff --check— all cleanRegressions, each verified to fail against the unfixed code: an empty secret file throws and leaves the target unset; the process exits when
flush()never settles; the blob store writes via tmp+rename; an unknown path always groups toother; memory pressure defers with its own reason.Invariants: the empty and unreadable secret failures stay distinguishable; a whitespace-padded secret still loads trimmed and an explicit env value still wins without reading the file; a flush that settles first is still fully awaited and both fatal events are bounded identically; no temp leftovers after either outcome and concurrent puts yield an intact object; the browser probe is absent when unconfigured, maps
wss:→https:, and re-probes once its TTL lapses; memory is strictly-greater (not>=), skips onnull, and CPU still outranks it.Two arms are annotated rather than tested, with reasons: the blob cleanup's own failure (unreachable without mocking fs, and mocking it would test the mock), and a
?? ""thatString.splitmakes unreachable.The blob-store crash window — a SIGKILL between first and last byte — cannot be simulated in-process. Rather than dress up tests that pass either way, the tmp+rename mechanism is pinned by a source assertion, and the accompanying tests state honestly what they do cover.