mirror of
https://github.com/4gray/iptvnator.git
synced 2026-10-13 20:56:19 -08:00
fix(portals): time out and fast-fail PWA proxy requests to dead hosts (#1424)
* refactor(portals): hoist the connectivity guard into libs/shared/host-health The breaker was written dependency-free so both processes that talk to portals could share it. Move the part that has no Electron in it — the state machine, the failure classification, the redirect-attribution helpers and the fast-fail error — into `@iptvnator/shared/host-health` (`scope:shared` / `domain:shared-runtime` / `type:util`). What stays in `apps/electron-backend` is the genuinely main-process part: one guard for the whole process, so both portal IPC handlers see each other's evidence, and the console warning that announces it. Every call site is unchanged; the wrapper re-exports the two types they import. The spec splits the same way — the state machine moves with the class, the singleton and its redirect attribution stay with the wrapper. Register the new project in the coverage policy, which every project with a test target must declare a tier for. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(portals): time out and fast-fail PWA proxy requests to dead hosts The web backend's proxy routes were bare `axios.get()` calls with no `timeout`, so a provider that accepted a connection and then went silent held the request until the OS gave up on the TCP connection — minutes, rather than the 15/30 s budget the Electron handlers use. Add the same per-route timeouts (Xtream 30 s, Stalker 15 s / 30 s for `create_link`, playlist and XMLTV 30 s). Those numbers are safe for large downloads: on axios' default transport `timeout` bounds the time to response headers and then continues as the socket's inactivity timeout, so a multi-megabyte XMLTV file that keeps delivering bytes is never cut off — only a stalled one is. With requests bounded, run `/xtream` and `/stalker` through the shared breaker, injected via `WebBackendAppOptions.hostGuard` so specs drive it with a clock they own. Playlist and XMLTV downloads keep the timeout but no breaker, matching Electron: a download is one request rather than a catalog fan-out, it is usually the direct result of the user asking for it, and it can outlive the half-open trial window. The breaker is checked before the Xtream URL revalidation, which resolves the hostname — a dead host is where DNS is slow too, and a request admitted and then abandoned by the URL policy hands its token back rather than holding the trial slot. `resetHostConnectivityGuard()` no longer no-ops in the PWA: the breaker lives in the backend process, so it travels to a new `POST /connectivity-guard/reset`, which reads only the origin and never logs the credential-bearing URL. `skipConnectionGuard` now survives the PWA transport too, so Stalker endpoint discovery keeps the exemption it has on the desktop instead of tripping the breaker with its own probes. A fast-fail keeps each route's HTTP 200 `{message, status}` envelope. The Stalker path needs one extra step: `forwardStalkerRequest` turns that envelope into `HTTP Error <code>: …` with a numeric `status`, and the renderer reads both as "the endpoint answered" — which would make discovery walk every candidate and fire lazy repair at a host just declared dead. A prior branch keyed on the shared `isHostConnectivityFastFailMessage` rethrows it bare instead. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(portals): report exempt Stalker probe responses to the PWA breaker Flagged by the author of #1421 as one of the twelve fixes that landed there after this branch cherry-picked the pre-review commit: an exempt discovery probe must still REPORT, it just must not COUNT. The web backend was skipping the report entirely for a probe, which loses the case that matters. A failure carrying an HTTP response proves the endpoint answered, and this route sets no `validateStatus`, so axios rejects every non-2xx with `error.response` attached — a probe answered with 404 or 500 was therefore dropped instead of clearing the record. Two counted failures either side of it then read as consecutive and opened the breaker in the middle of discovery, which is exactly what the exemption exists to prevent. `reportProviderRequestFailure` now takes `countFailures`, matching the Electron reporter, and both routes always report. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(portals): read the redirect hop from the transport that followed it `failedAfterRedirect` decided whether a failure belonged to a redirect destination by reading `error.config.url`. That is right for the Electron transport, which sets `maxRedirects: 0` and reissues every hop as its own request, so the hop IS the config URL. It is blind on the web backend, which uses axios' default transport: follow-redirects walks the chain inside one request and `config` is built once, so `config.url` stays the URL we asked for. Verified against the installed axios 1.19.0 with a live server that 302s to a dead port: asked for : http://127.0.0.1:63953/player_api.php config.url : http://127.0.0.1:63953/player_api.php request._currentUrl: http://127.0.0.1:1/dead So the comparison was original-vs-original, found no redirect, and charged two dead destinations to the provider that had answered both times with a 302 — then fast-failed it. Read `request._currentUrl` first and fall back to `config.url`, which covers both transports; Electron's native per-hop requests expose no `_currentUrl` and are unaffected. Also check redirect attribution BEFORE suppressing failure counting for an exempt probe. A 3xx from the guarded endpoint is an answer, so a probe that observed one must clear the record; otherwise a timeout, a probe redirected to a dead destination, and another timeout still read as two consecutive failures. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(portals): stop axios query params reading as a redirect Codex found that the Xtream breaker never opened at all, and it was right. The web backend passes credentials and the action through axios' `params`, so axios sends `…/player_api.php?username=…&action=…` while the baseline handed to `failedAfterRedirect` is the query-less URL the route built. Verified against axios 1.19.0 with a plain ECONNREFUSED and no redirect anywhere in sight: baseline : http://127.0.0.1:1/player_api.php request._currentUrl: http://127.0.0.1:1/player_api.php?username=demo&… The two normalized URLs differ, so every ordinary failure looked like a post-redirect failure, credited the endpoint, and the breaker could never trip. Compare origin and path, not the whole URL. That keeps what the check is for — an endpoint that answered and sent us elsewhere, including the same-origin `/player_api.php` → `/slow/player_api.php` case — and gives up only a redirect that changes nothing but the query, which is then counted as an ordinary failure. Erring towards counting is the safe direction here. The reason 57 tests passed over a dead feature is the real lesson: `StubHttpClient` threw bare `Error`s, so the guard's redirect check saw neither `config.url` nor `request._currentUrl` and quietly did nothing. The stub now shapes its rejections like axios does, including the query axios appends. With that alone, four existing tests fail against the old comparison. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(portals): count a hostname that stops resolving, and fix a stale docblock Two from review, one behavioural and one documentation. A name that will not resolve is the host failing to answer — the same evidence as the ENOTFOUND the transport would have raised a moment later. But the SSRF validation turns a lookup failure into a 400 "host could not be resolved", and the release path added earlier handed the token back as inconclusive, so the breaker could never open for a host whose DNS died and every request kept paying for the same dead lookup. `ProviderUrlError` now carries the underlying lookup error internally. A refusal that has one is counted; a genuine policy refusal — private address, bad scheme, credentials in the URL — still only releases the half-open slot, because that says nothing about reachability. The field is internal: `providerUrlErrorBody()` strips it at both call sites, so the client sees exactly the body it saw before, which the test asserts. The docblock on `resetHostConnectivityGuard` still said the PWA channel is unknown and the call no-ops. That stopped being true when this branch implemented `CONNECTIVITY_GUARD_RESET` over HTTP, and a stale contract there is how the next caller silently skips the PWA path. (The edit was in an earlier commit and was lost when the branch was rebuilt on master.) Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(portals): record the transport-specific redirect contract The redirect section still described one transport: hop-by-hop requests, `error.config.url`, whole-URL comparison. Two of those three are now wrong for the web backend, and this document is the canonical contract — leaving it stale is how the attribution bugs fixed in the last two commits get reintroduced. Says what is actually true: which field holds the failed hop on each transport and why the helper reads both, and that the comparison is origin + path because the web backend's credentials ride in axios' `params` and a whole-URL comparison therefore reported a redirect for every ordinary failure. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(portals): scope the guard summary to both processes The opening line still defined the breaker as an Electron main-process concern, which contradicted the ownership section below it and is the part a reader skims to decide whether the document applies to them. Names both processes, and records that the web backend had the worse version of the problem first — no request timeout at all — since that is why the timeouts and the breaker had to land there together. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(portals): declare the shared-interfaces dependency of host-health The new library's manifest listed only `tslib`, but its emitted JavaScript does `require('@iptvnator/shared/interfaces')` — the guard builds its fast-fail message with `buildHostConnectivityFastFailMessage`. Anything resolving the built artifact from its own manifest would have failed with MODULE_NOT_FOUND. The manifest was copied from `shared/logging`, which imports nothing across libraries and therefore needs nothing beyond `tslib`. `shared/m3u-utils` is the right precedent: it imports the same library and declares `"@iptvnator/shared/interfaces": "0.0.1"`. Verified against the build output rather than by inspection — the emitted `host-connectivity-guard.js` requires the module, and the generated `dist/libs/shared/host-health/package.json` now declares it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(portals): bound the PWA connectivity-guard reset The reset was a bare `fetch` with no timeout, which is the exact failure this change exists to remove, reintroduced one layer up. Every caller awaits the reset BEFORE issuing the request it is clearing the way for — `retryContentInitialization` awaits it first by design — so a backend or reverse proxy that accepts the POST and then goes quiet would leave Retry doing nothing at all, for as long as the socket stayed open. Bound it with an AbortController and a 5 s timer. The abort rejects, `resetHostConnectivityGuard` swallows it as it already does for any other failure, and the caller proceeds to its real request — which is what "best effort" was supposed to mean. The timer is cleared in a `finally`, and it covers the body read as well as the headers. Five seconds because this talks to the user's own backend rather than a provider: it should answer immediately, and a slow one must not hold up the retry that asked for it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
1 parent
3103eba083
commit
e94cc029eb
24 files changed
+2895
-1171
No files matched your search
@@ -1,31 +1,73 @@
|
||||
# Host Connectivity Guard
|
||||
|
||||
Per-host circuit breaker for portal requests in the Electron main process.
|
||||
Per-host circuit breaker for portal requests, in both processes that make them:
|
||||
the Electron main process and the self-hosted web backend.
|
||||
|
||||
## The problem
|
||||
|
||||
Every request to an unreachable portal costs its full axios timeout — 30 s for
|
||||
`XTREAM_REQUEST`, 15 s for `STALKER_REQUEST` (30 s for `create_link`). Browsing a
|
||||
`XTREAM_REQUEST`, 15 s for `STALKER_REQUEST` (30 s for `create_link`), and the
|
||||
same budgets on the web backend's `/xtream` and `/stalker` routes. Browsing a
|
||||
dead portal's catalog issues dozens of those back to back, which shows up as
|
||||
30-second spinners and a main-process log full of identical failures. Once a host
|
||||
has refused to answer twice in a row there is nothing left to learn from waiting
|
||||
again.
|
||||
30-second spinners and a log full of identical failures. Once a host has refused
|
||||
to answer twice in a row there is nothing left to learn from waiting again.
|
||||
|
||||
The web backend had a worse version of the same problem first: its proxy routes
|
||||
passed no `timeout` at all, so a provider that accepted a connection and then
|
||||
went silent held the request until the OS gave up on the TCP connection. A
|
||||
breaker is only useful once _not answering_ is bounded, which is why the timeouts
|
||||
and the breaker landed there together.
|
||||
|
||||
## Where it lives
|
||||
|
||||
`apps/electron-backend/src/app/util/host-connectivity-guard.ts` — a pure module
|
||||
(no Electron imports) wired into both IPC handlers.
|
||||
`libs/shared/host-health` (`@iptvnator/shared/host-health`, tagged
|
||||
`scope:shared` / `domain:shared-runtime` / `type:util`) — the breaker class, the
|
||||
failure classification and the redirect-attribution helpers, with no transport,
|
||||
logger or process singleton of its own. The owning app supplies the clock and
|
||||
decides how many guards exist.
|
||||
|
||||
The handlers are the choke point that sees _all_ traffic to a host. The
|
||||
renderer's `executeStalkerRequest` is not: it has four documented bypasses
|
||||
(auth, endpoint discovery, account info, the row-less stream resolver), and that
|
||||
is exactly the traffic that hits dead hosts.
|
||||
Each runtime owns its instance:
|
||||
|
||||
**The PWA is deliberately not covered yet.** `apps/web-backend` sets no
|
||||
per-request timeout at all, so a dead host there hangs on OS-level TCP timeouts
|
||||
rather than a 15/30 s budget — a timeout-driven breaker would rarely trip. The
|
||||
guard is written dependency-free so it can move into a `domain:shared-runtime`
|
||||
library and be shared with `web-backend` when that gap is closed.
|
||||
- **Electron** — `apps/electron-backend/src/app/util/host-connectivity-guard.ts`
|
||||
holds one guard for the whole process, wired into both IPC handlers. The
|
||||
handlers are the choke point that sees _all_ traffic to a host. The renderer's
|
||||
`executeStalkerRequest` is not: it has four documented bypasses (auth,
|
||||
endpoint discovery, account info, the row-less stream resolver), and that is
|
||||
exactly the traffic that hits dead hosts.
|
||||
- **PWA** — `apps/web-backend/src/app/host-guard.ts` guards the `/xtream` and
|
||||
`/stalker` proxy routes. The guard instance is injected through
|
||||
`WebBackendAppOptions.hostGuard`, alongside `now` and `guid`, so specs drive
|
||||
it with a clock they own.
|
||||
|
||||
### Request timeouts are the precondition
|
||||
|
||||
A breaker keyed on "the endpoint did not answer" is only useful once _not
|
||||
answering_ is bounded. `apps/web-backend` originally passed no `timeout` at all,
|
||||
so a silent host hung on OS-level TCP timeouts. Both runtimes now use the same
|
||||
budgets: Xtream 30 s, Stalker 15 s (30 s for `create_link`, which mints a stream
|
||||
URL before answering), playlist and XMLTV downloads 30 s.
|
||||
|
||||
Those numbers are safe for large downloads. On axios' default
|
||||
(follow-redirects) transport `timeout` is **not** a wall-clock deadline for the
|
||||
whole response: it bounds the time to response headers and then continues as the
|
||||
socket's inactivity timeout for the body. A multi-megabyte XMLTV file that keeps
|
||||
delivering bytes is never cut off mid-transfer — only a stalled one is.
|
||||
|
||||
### Scope: portal calls only
|
||||
|
||||
Both runtimes guard the portal API paths and nothing else. Playlist and XMLTV
|
||||
downloads (`/parse`, `/parse-xml`, and their Electron equivalents) get the
|
||||
timeouts but not the breaker, deliberately:
|
||||
|
||||
- The problem being solved is a catalog fan-out — dozens of requests to one
|
||||
endpoint back to back. A download is a single request.
|
||||
- A download is usually the direct result of the user asking for it (the
|
||||
add-playlist dialog sends `PLAYLIST_PARSE_BY_URL`). Refusing an immediate
|
||||
retry is a regression, not a protection, and there is no natural reset site on
|
||||
that path the way portal Retry has one.
|
||||
- A large XMLTV transfer can legitimately run for minutes — the timeout is
|
||||
idle-based, see above — which outlives the 45 s half-open trial expiry and
|
||||
would let a second trial in behind the first.
|
||||
|
||||
## Rules
|
||||
|
||||
@@ -48,25 +90,41 @@ mid-transfer happens on hosts that are very much alive. Cancelled requests
|
||||
(`ERR_CANCELED`) and SSRF-policy refusals are `inconclusive` — they say nothing
|
||||
about reachability and only release the half-open slot.
|
||||
|
||||
**A failure is only charged to the endpoint that produced it.** Redirects are
|
||||
followed hop by hop, each with its own config, so a failure on a later hop
|
||||
carries that hop's URL in `error.config.url`. Reaching any later hop _proves_ the
|
||||
guarded endpoint answered — the first hop is always the URL we asked for, and
|
||||
only a redirect status advances the chain — so it CLEARS the guarded endpoint's
|
||||
record, exactly like any other response. Merely declining to count it would leave
|
||||
an earlier direct failure standing, and a single later timeout would then
|
||||
fast-fail an endpoint that answered in between.
|
||||
**A failure is only charged to the endpoint that produced it.** Reaching any
|
||||
later hop _proves_ the guarded endpoint answered — the first hop is always the
|
||||
URL we asked for, and only a redirect status advances the chain — so a failure
|
||||
there CLEARS the guarded endpoint's record, exactly like any other response.
|
||||
Merely declining to count it would leave an earlier direct failure standing, and
|
||||
a single later timeout would then fast-fail an endpoint that answered in between.
|
||||
Every caller passes the URL it asked for as the baseline.
|
||||
|
||||
The comparison is against the whole request URL, not just its origin: a
|
||||
same-origin redirect (`/player_api.php` → `/slow/player_api.php`) proves the
|
||||
endpoint answered just as much as a cross-origin one, and charging it would
|
||||
fast-fail every OTHER call to a portal that answers. Both handlers therefore pass
|
||||
the URL they asked for. It requires positive evidence — anything unparseable or
|
||||
unknown counts the failure as usual, because guessing "redirect" here would stop
|
||||
the guard from ever tripping — and a failure that names no URL at all is still
|
||||
counted. Round-tripping through `URL` is identity for both handlers' URL shapes
|
||||
(including Stalker's hand-encoded `cmd`), and `requestWithValidatedRedirects`
|
||||
normalizes hop 1 the same way, so the comparison is exact.
|
||||
**Where the failed hop is found depends on the transport, and both are in play.**
|
||||
|
||||
| | Electron | Web backend |
|
||||
| --- | --- | --- |
|
||||
| Redirects | followed hop by hop (`maxRedirects: 0`), each its own request | followed inside one request by follow-redirects |
|
||||
| Failed hop is in | `error.config.url` | `error.request._currentUrl` |
|
||||
|
||||
`failedRequestUrlOf` reads `_currentUrl` first and falls back to `config.url`,
|
||||
which is correct for both: a per-hop request exposes no `_currentUrl`, and on the
|
||||
following transport `config` is built once and keeps the URL we asked for — so
|
||||
reading `config.url` there would compare a URL with itself, find no redirect, and
|
||||
charge a dead destination to the provider that answered. Anything added here must
|
||||
work on both, because the same helper serves both.
|
||||
|
||||
**The comparison is origin + path, not the whole URL.** A same-origin redirect
|
||||
(`/player_api.php` → `/slow/player_api.php`) proves the endpoint answered just as
|
||||
much as a cross-origin one, and charging it would fast-fail every OTHER call to a
|
||||
portal that answers — so the path has to be part of it. The query must NOT be:
|
||||
the web backend passes Xtream credentials through axios' `params`, so the sent
|
||||
URL always carries a query the baseline does not, and comparing whole URLs made
|
||||
every ordinary failure look like a redirect and stopped the breaker from ever
|
||||
opening. What that gives up is a redirect that changes nothing but the query,
|
||||
which is then counted as an ordinary failure — the safe direction.
|
||||
|
||||
It requires positive evidence — anything unparseable or unknown counts the
|
||||
failure as usual, because guessing "redirect" here would stop the guard from ever
|
||||
tripping — and a failure that names no URL at all is still counted.
|
||||
|
||||
Known gap: the failing hop is not guarded either (it has no token of its own),
|
||||
so a permanently broken redirect chain keeps costing a full timeout.
|
||||
@@ -160,6 +218,13 @@ body does. That is why the exempt path reports through
|
||||
`reportGuardedHostFailure(token, error, { countFailures: false })` rather than
|
||||
skipping the report.
|
||||
|
||||
The flag has to survive the PWA transport too, or discovery is exempt on the
|
||||
desktop and policed on the web. `PwaService.forwardStalkerRequest` forwards it
|
||||
as a `/stalker` control param and the route applies the same semantics — and,
|
||||
like `macAddress`/`token`/`serialNumber`, it is stripped from the query
|
||||
forwarded to the portal, because it is our control flag and not protocol
|
||||
content.
|
||||
|
||||
## Explicit reset
|
||||
|
||||
`CONNECTIVITY_GUARD_RESET` (`{ url }`) is handled by
|
||||
@@ -223,9 +288,14 @@ the next `nearEnd` event fires a second retry.
|
||||
They all go through `resetHostConnectivityGuard()`
|
||||
(`libs/services/src/lib/host-connectivity-reset.ts`), which holds the one rule
|
||||
they share: the reset is best effort, because the guard only ever _delays_ a
|
||||
request and a failed reset must not block the action that asked for it. In the
|
||||
PWA the channel is unknown and `sendIpcEvent` no-ops, which is correct —
|
||||
nothing there records per-host failures yet.
|
||||
request and a failed reset must not block the action that asked for it. Both
|
||||
runtimes honour it, so neither keeps fast-failing an endpoint the user just
|
||||
asked to retry — in the PWA `PwaService` forwards it to the web backend's
|
||||
`POST /connectivity-guard/reset`, since the breaker lives in the backend
|
||||
process and a local reset would clear nothing. That route takes the raw
|
||||
provider URL rather than a registered `targetId`, because callers reset
|
||||
precisely when the address may have changed, which is before any target exists
|
||||
for it; it fetches nothing, reads only the origin, and never logs the URL.
|
||||
|
||||
## Interaction with VOD multi-source
|
||||
|
||||
@@ -240,8 +310,20 @@ module's contract that unreachable ≠ contacted-and-refused. The
|
||||
|
||||
## Tests
|
||||
|
||||
- `libs/shared/host-health/src/lib/host-connectivity-guard.spec.ts` — the state
|
||||
machine, with an injected clock.
|
||||
- `apps/electron-backend/src/app/util/host-connectivity-guard.spec.ts` — the
|
||||
state machine, with an injected clock.
|
||||
main-process singleton and redirect attribution through it.
|
||||
- `apps/web-backend/src/app/web-backend-app.host-guard.spec.ts` — the proxy
|
||||
routes: the HTTP 200 refusal shape, that the outbound request really is
|
||||
skipped, that an open endpoint is refused before a DNS lookup is spent on it,
|
||||
redirect attribution, the discovery exemption, half-open, the reset endpoint,
|
||||
and that playlist/EPG downloads are never fast-failed. The per-route timeouts
|
||||
are asserted in `web-backend-app.spec.ts`, which pins the whole outbound
|
||||
request shape.
|
||||
- `apps/web/src/app/services/pwa.service.spec.ts` — that a fast-fail reaches the
|
||||
renderer with no numeric `status` and no `HTTP Error <code>` in its message,
|
||||
that the discovery bypass is forwarded, and that a reset calls the backend.
|
||||
- `apps/electron-backend/src/app/events/stalker.events.spec.ts` and
|
||||
`xtream.events.spec.ts` — trip, fast-fail without contacting axios, reset,
|
||||
exemption, and the absence of per-request log spam.
|
||||
|
||||
Reference in new issue
Block a user