You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
A single dead upstream used to break depot downloads outright, because the one configured host was the only one ever tried. Fetches now walk the provider table starting from the active entry, fall back to the next entry on failure, and promote whichever provider answers to first choice, so a user on a fresh config recovers without editing anything.
Two bounds keep the fallback from costing more than it saves:
a provider that just failed is skipped for 60 s, so a dead host costs one timeout rather than one per depot;
one whole fetch, every attempt included, is capped at 11 s, so the net-packet handler's wait (kMaxWaitMs, now derived from kFetchBudgetMs) always outlasts the fetch instead of racing it.
Additionally, encoding issues were causing compilation errors. I added the following to the 'src/CMakeLists' file as a temporary workaround:
if(MSVC)
add_compile_options("$<$<COMPILE_LANGUAGE:C,CXX>:/utf-8>")
endif()
A single dead upstream used to break depot downloads outright, because
the one configured host was the only one ever tried. Fetches now walk
the provider table starting from the active entry, fall back to the
next entry on failure, and promote whichever provider answers to first
choice, so a user on a fresh config recovers without editing anything.
Two bounds keep the fallback from costing more than it saves:
- a provider that just failed is skipped for 60 s, so a dead host
costs one timeout rather than one per depot;
- one whole fetch, every attempt included, is capped at 11 s, so the
net-packet handler's wait (kMaxWaitMs, now derived from
kFetchBudgetMs) always outlasts the fetch instead of racing it.
Also pin the compiler to UTF-8 sources. MSVC otherwise guesses the
machine's ANSI code page, and the U+2550 banner comment in
Hook/Hooks_Manifest.cpp ends on byte 0x90 -- a valid cp936 double-byte
lead, so the newline after it is eaten and the next line of code is
swallowed. On a CJK locale that hides the file's opening "namespace {"
and the build fails with a syntax error far from the real cause.
The failover loop inherited a mutex that was held across the whole
fetch, on the premise (stated in the header) that it serialised a shared
WinHTTP connection. There is no such connection: Http::Execute opens and
closes its own session per request. What the lock actually protected was
the provider state, so every concurrent depot queued behind whichever
one was currently timing out -- and with up to three attempts per fetch
instead of one, the tail of that queue now reliably overran the hook's
kMaxWaitMs window and lost its injection even on a healthy provider.
Split it in two. g_stateMutex covers g_active and g_deadUntil through
four short accessors and never spans I/O. g_luaMutex separately
serialises the Lua attempts, which is load-bearing: LuaConfig exposes
one shared lua_State and does no locking of its own, so dropping the
lock outright would have let two fetch threads push onto the same stack.
TryLua exists to scope that lock so it is released before any network
I/O starts. A dead upstream now costs the waiting depots one timeout in
parallel rather than one each in sequence, and the old
ManifestClient -> Config lock nesting is gone with it.
Also stop an unrelated config edit from undoing what failover learned.
Any write to opensteamtool.toml reloads the whole config and calls
SetProvider, which wiped every cooldown; changing log.level was enough
to send the next fetch back into a host already known to be dead.
Re-selecting the active provider is now a no-op, and an explicit switch
clears the cooldown only on the provider it selects.
Finally, validate the timeouts where they are read. WinHTTP treats a
zero timeout as no timeout at all, and the old cast let a negative TOML
value wrap into a ~49-day uint32; either one pinned a worker thread past
the fetch budget. ReadTimeoutMs clamps each value to
[1, kFetchBudgetMs] and logs when it has to. The clamp inside cap() is
kept as defence in depth, rewritten as max/min because
std::clamp(v, 1, left) would be UB if the left <= 0 guard above it ever
moved.
Preserve promoted provider selection across config reloads
src/Utils/SteamMetadata/ManifestClient.cpp:123
When failover has promoted (for example) wudrm, an unrelated config hot-reload still calls this with the configured opensteamtool name and moves g_active back to the failed host, clearing its cooldown. The next depot then pays the dead-host timeout again, so the promoted provider is not actually the new first choice across config reloads. Track the configured selection separately and only reselect it when the manifest.url value changes.
Enforce the overall timeout deadline across HTTP phases
src/Utils/SteamMetadata/ManifestClient.cpp:200
The cap lambda is applied independently to each timeout phase, but Http::Execute installs these as per-operation WinHTTP limits and then runs resolve/connect/send/receive sequentially. If one phase consumes left, the later phases each get another left, so an attempt (and therefore the whole fallback walk) can run well past the 11-second deadline and the packet hook can time out first. Thread an absolute deadline through Http::Execute or recalculate the remaining budget before every phase/read.
Narrowing the fetch lock let two attempts against the same provider
overlap, which the cooldown bookkeeping was not written for. MarkFailed
wrote unconditionally while Promote cleared the cooldown only when it
actually switched providers, so the common case was also the worst one:
one thread gets a 200 from the active provider, Promote returns early
without clearing anything, and a concurrent failure then blacklists a
host that was just proved alive for the full 60 s. With the other
providers already cooling down, the next fetch takes the "every provider
is in cooldown" path and fails with no network I/O at all, losing the
injection for every depot in that window while a healthy upstream sits
idle. Steam opens many depots at once and an upstream under that burst
can easily 429 one request while answering another, so the interleaving
is not a corner case. Before the lock was narrowed the mutex serialised
the whole fetch and the two could never overlap.
Give each provider a success counter. An attempt samples it before the
request goes out and hands it back to MarkFailed, which now drops the
cooldown write if the counter moved -- a success landed in the meantime
and is the better evidence. RecordSuccess replaces Promote and clears
the cooldown unconditionally, including for the already-active provider,
so a spurious cooldown cannot outlive the next answer. SetProvider bumps
the counter for the same reason: an in-flight failure must not
immediately undo an explicit switch.
Note this only fixes the concurrent case. A single transient failure
still cools a provider for 60 s, which is the behaviour the cooldown was
introduced with; requiring consecutive failures instead would be a
change of intent and is left alone.
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
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.
A single dead upstream used to break depot downloads outright, because the one configured host was the only one ever tried. Fetches now walk the provider table starting from the active entry, fall back to the next entry on failure, and promote whichever provider answers to first choice, so a user on a fresh config recovers without editing anything.
Two bounds keep the fallback from costing more than it saves:
Additionally, encoding issues were causing compilation errors. I added the following to the 'src/CMakeLists' file as a temporary workaround:
if(MSVC)
add_compile_options("$<$<COMPILE_LANGUAGE:C,CXX>:/utf-8>")
endif()