The 'execProbe keeps the parent event loop available' case asserts nothing
about timing; the 1s spawn budget only exists so a wedged child cannot stall
the run. A cold Windows CI runner can take longer than 1s just to start the
node child, which failed the test for reasons unrelated to what it guards.
5s keeps the safety bound without the flake.
runPoolBackendStart reused assertPoolEntryStillOwned at every cancellation
checkpoint, and that helper releases the local backend slot before it throws.
That is right before spawn (no child, nothing to wait for) but wrong at the
post-spawn checkpoints (after the port announcement, waitForHermes, token
adoption, WS probe): the lease was handed back while the superseded child was
still running, so a successor could spawn into an occupied slot, and the
caller's teardownFailedLocalBackend -> releaseLocalBackendSlotAfterExit then
found nothing left to release and became a no-op. This breaks the
pool-spawn-coordinator invariant that a lease is held until the child exits
or the start fails.
Add a `releaseSlot` option to assertPoolEntryStillOwned and pass
`{ releaseSlot: false }` at every site where `entry.process` is set. The
pre-spawn sites keep releasing. The post-spawn release now happens only via
teardownFailedLocalBackend (after the child has provably exited) or the
child's own exit handler.
No vitest case: the helper and its callers live in main.ts alongside the
module-level backendPool / localBackendLifecycle state and are not importable
from a unit test without extracting them; the lease-after-exit ordering
itself is already pinned by pool-spawn-coordinator.test.ts ('a rejected wait
keeps the slot occupied').
The serve-support resolver caches the probe outcome per resolved runtime
for the process lifetime. That is right for a genuine "unknown
subcommand" exit, but a probe that died by timeout says nothing about the
runtime — only that this machine was slow right then (cold AV scan on
Windows, first Python import after boot). Caching that as `false` routed
a modern runtime through the legacy `dashboard` form until the app was
relaunched.
Export isTimeoutError from backend-probes and evict the cache entry on a
timeout so the next check re-probes; non-timeout failures stay cached.
One vitest case covers timeout → re-probe; the existing case still pins
non-timeout failure → cached.
Also replace the last synchronous read on the discovery path
(dashboard.py fast path) with fs.promises.readFile, matching the stack's
goal of keeping runtime discovery off the main event loop.
isActiveRuntimeUsable relied on async-return flattening for the trailing
canImportHermesCli promise: correct today, but appending any further `&&`
operand would have made the expression truthy regardless of the probe.
Await the probe explicitly so the intent survives future edits, and mark
unwrapWindowsVenvHermesCommand async like its siblings since it always
returns a promise.
connectRemote used two inline isCurrentAttempt/throw pairs with the same
message that backendConnectionState.assertCurrentAttempt already emits,
and the third guard in the same function already uses the helper. Use it
in all three places so the superseded-attempt message has one source.
The fast-path/probe/cache explanation for serve support lived above the
resolver call site in main.ts while the logic lives in
backend-serve-support.ts; move it next to the code and leave a pointer.
The stack inserted the errcode table directly after _strip_reply_fallback's
return with no blank lines, which reads as if the constant belongs to the
function body and trips E305. Two blank lines restore the module-level
boundary; no behaviour change.
Fold the transient/permanent sync-loop tests into one parametrized table
and add the two classifier branches that had no loop-level coverage: a
401 whose body was rewritten to HTML by a reverse proxy (errcode dropped,
so only http_status can stop the loop) and a 429 M_LIMIT_EXCEEDED that
must be retried because neither errcode nor status is an auth signal.
Deleting the http_status fallback in _is_permanent_matrix_auth_error now
fails the 401-html case.
Hoist _sync_error to module level so parametrize can call it directly
instead of the staticmethod.__func__ workaround. Trim the
test_ws_auth_retry docstring, which still described a Matrix test class
that moved to test_matrix.py.
The inherited fixture coordinate "40.4302" does not contain the substring
"403" (the dot splits it), so the old substring classifier also passed on
it and the case proved nothing. Use a coordinate that genuinely embeds the
digits so the test is red on the pre-fix classifier.
The pinned mautrix 0.21.1 raises MatrixRequestError (carrying errcode and
http_status) from HTTPAPI._send on every non-2xx and sync() returns only
the parsed JSON dict, so the result-object auth branch in _sync_loop was
unreachable; it dated from the nio client whose SyncError objects were
real. Drop it together with the nio-mock test that pinned it.
With structured attributes guaranteed, the leading-status regex and the
bounded keyword scan over the message text were the only remaining ways
for body digits or HTML words to leak into the verdict, so drop them too:
no errcode/http_status auth signal means retry. Trim the contributor's
17 tests to the two loop-level invariants: both production repros (502
HTML body embedding "403" via an SVG coordinate; timeout echoing a since
token embedding "401") keep looping, and a 401/M_UNKNOWN_TOKEN stops.
The comment above the result-object branch in _sync_loop claimed mautrix's
Client.sync() returns an object carrying a message string for auth failures.
That is wrong. In the pinned mautrix 0.21.0, HTTPAPI._send raises
make_request_error() for any non-2xx and otherwise returns parsed JSON, so a
real M_FORBIDDEN arrives as an exception and is handled by the except branch.
The claim was introduced by this PR, which rewrote an accurate comment about
the earlier matrix-nio client (whose SyncError result objects were genuine).
The branch itself is kept as defense in depth against a future client swap,
but it now classifies with the same errcode/http_status logic as the
exception path instead of a lone "unknown_token" substring test, which
silently missed M_MISSING_TOKEN and M_FORBIDDEN and resynced forever
against a credential that can never succeed.
A structured errcode/http_status is authoritative; the message text is only
consulted when the object exposes neither, since str(object) is an opaque
repr. The text scan deliberately cannot override a structured verdict, so a
transient 502 whose HTML body contains "Forbidden" is still retried.
Adds four tests. Three are discriminating RED/GREEN cases that fail against
the old substring branch (M_MISSING_TOKEN errcode, http_status=401 with no
keyword in the message, and an unstructured object whose only signal is
.message). The fourth pins the precedence rule and passes either way.
Verified: 136 passed / 1 failed in tests/gateway/test_matrix.py; the single
failure (test_password_login_uses_device_id) fails identically at the
pristine PR head and is unrelated.
(cherry picked from commit bc9e6a8dafcf349a4e6b20a261fb2449603c0239)
The independent-verifier caught that my first loop-level test did not
actually prove anything. The 502/SVG coordinate fixture I reused from
gmoranxyz's unit-level test does not contain the substring 403 once
case-folded, so the old naive substring classifier already treated it
as transient. A test that passes under both the buggy code and the
fix proves nothing about the fix.
I replaced the fixture with a plain connection timeout whose message
wraps the real Matrix sync pagination token, an arbitrary digit
string that happens to contain 401. I verified this directly: with
the pre-fix classifier restored, the retry test now fails (the old
code stops the loop on this fixture), and with the fix in place it
passes (the loop retries as it should). That is the RED/GREEN proof
the maintainer originally asked for.
I also documented in the stop test's docstring that it does not
discriminate old from new, since the word forbidden in its message
trips the old naive check too. It is still worth keeping as a
regression test proving genuine auth errors stop the loop, just not
as proof of this specific fix.
While I was in there I also fixed a stale comment above the
M_UNKNOWN_TOKEN sync-object pre-check. It said nio returns SyncError
objects, but the dependency here is mautrix, not matrix-nio, and
importing nio raises ModuleNotFoundError in this codebase. The
pre-check logic itself was already correct and untouched.
Co-authored-by: gmoranxyz <gmoranxyz@users.noreply.github.com>
(cherry picked from commit ad3aad579a675a5aae544a50f88a82717c0ac3b6)
I added two more classifier unit tests for the attribute narrowing:
a bare .code attribute that happens to be 401, and a bare .status
attribute that happens to be 403, both must stay classified as
transient since only .http_status is trustworthy. I also added a
parametrized test for the five transient exception types the sync
loop now short-circuits on.
On top of that I added two tests that exercise _sync_loop directly
instead of just the classifier function in isolation. One replays the
real 502 Umbrel repro string through a mocked client.sync and confirms
the loop retries with the 5s backoff. The other raises a genuine
M_FORBIDDEN error and confirms the loop stops on the first call with
no retry sleep. These catch a regression in how the loop wires the
classifier in, not just a regression in the classifier itself.
(cherry picked from commit f747bb4b5a6e6da1bb9136168f08d6e7af5ea64b)
I hit a bug where the Matrix sync loop treated a passing 502 from
Umbrel's app proxy as a permanent auth failure and stopped syncing for
good. The old check did a naive "403" in str(exc) substring match, and
the 502 HTML error body embedded an SVG path with the coordinate
40.4302, which contains the digit sequence 403.
I replaced the substring check with a layered classifier. Transport
exceptions like TimeoutError, ConnectionError, and OSError are always
treated as transient regardless of their message text. Structured
signals take priority next: the errcode attribute against a known set
of permanent Matrix error codes, then the http_status attribute
against 401/403 specifically (not status, status_code, or code, which
belong to unrelated exception shapes and risk coincidental integer
matches). Only when none of those are present does it fall back to a
bounded, word-boundary-safe text scan on the first 200 characters.
Added tests covering the attribute narrowing, the transient exception
types, and two loop-level tests exercising _sync_loop directly to
confirm it retries on a transient error and stops on a genuine 401/403.
(cherry picked from commit 96d3363e45a63e08d9f07518ed949df334a33b3c)
The 400-recovery reload installs a disk pair and only then re-runs issuer
binding on it. When that pair was minted by a different issuer the enforcer
strips its refresh token, and the reload must report "no recovery" so the
session is cleared like any other dead grant. No test pinned that verdict:
a reload that ignored the install result would keep a stripped pair in the
context and return True. The new invariant drives a real 400 through
_handle_refresh_response against a foreign-bound disk pair and asserts the
result is False, the context is cleared, and the foreign refresh token does
not survive on disk.
_hermes_live_ttl read expires_in off current_tokens without checking for
None; getattr(None, ...) happened to yield the default and report "live".
Both current callers install a pair first, but the helper is now explicit
that an empty context is never live, so a future call site cannot adopt
nothing.
Two defects in the peer-adoption path of the refresh fence:
1. `_hermes_install_disk_pair` raised `_RefreshCompletedByPeer` when issuer
binding stripped the candidate's refresh token. That is the right outcome
for `_refresh_token` (restart the flow so the SDK lands in 401 -> full
auth), but `_hermes_reload_tokens_after_refresh_failure` shares the helper
and must instead treat the candidate as rejected: restore the previous
pair and return False so the caller clears state and prompts. The helper
now returns whether a refresh token survived binding and each caller
decides; the one-shot adopt wrapper is inlined into `_refresh_token`.
2. `_hermes_live_ttl` treated `expires_in is None` as expired. RFC 6749 makes
`expires_in` optional, the SDK's `is_token_valid()` is True with no expiry,
and `_rebase_expires_in` preserves None on read, so a peer's rotated pair
without an expiry was never adopted and we POSTed its refresh token
anyway, burning a generation on single-use providers. None now counts as
live; the try/except around a pydantic `int | None` field is dropped.
One new test drives the real auth flow against a peer pair with no
`expires_in` and asserts the pair is adopted with zero POSTs.
flock is bound to an inode, not a path. Unlinking `<srv>.json.refresh.lock`
from `remove()` while a peer still holds the fence lets the next acquirer
open and lock a brand-new inode, so two processes hold "the" fence at once
and the single-use refresh token can be consumed twice. On Windows the unlink
of a locked file raises PermissionError straight out of `remove()`/`restore()`.
A 0-byte 0600 sidecar in a 0700 directory is harmless, so leave it in place.
It stays out of `_state_paths()` so `restore(only_if_absent=True)` still keys
off real token state only.
The SDK drives a refresh as a generator (request yielded from
_refresh_token, response consumed in _handle_refresh_response), so the
fence was a hand-driven @asynccontextmanager: __aenter__ in one method,
__aexit__ in another, generator object stashed on the provider. A plain
`acquire_refresh_fence(path, timeout) -> fd` / `release_refresh_fence(fd)`
pair says what actually happens and leaves nothing half-entered to leak.
The descriptor is opened with os.open at 0600 and closed on every
acquisition failure.
The `_refresh_token` release-on-exception stays: `_refresh_token` is also
reachable outside `async_auth_flow` (tests call it directly), and the
wrapper's finally only covers the generator-driven path.
HermesTokenStorage.remove() now unlinks the `.refresh.lock` sibling so
logout leaves no stray file. It is deliberately NOT added to
_state_paths(): snapshot()/restore(only_if_absent=True) treat any
existing state path as "newer state exists", and a lingering lock file
would silently veto a rollback.
tests/tools/test_mcp_oauth.py: drop the unused `Path` import (`time` is
still used by the socket poll helper).
The poll loop swallowed every OSError from the lock syscall as contention,
so a filesystem that cannot take advisory locks at all (ENOLCK on some
network mounts, EMFILE, ...) stalled for the full 60 s deadline and then
blamed a peer. Only EWOULDBLOCK/EAGAIN/EACCES/EDEADLK mean "held by
someone else"; anything else now raises RefreshFenceTimeout immediately
with the real errno. Still fails closed -- the refresh is never POSTed
without ownership -- but the failure is diagnosable and instant.
The errno set mirrors cron.scheduler._is_lock_contention_errno; it is
duplicated rather than imported because importing the scheduler pulls in
the whole cron module graph for a four-value tuple.
Both fence paths that pull a peer's pair off disk now go through
_hermes_rotated_candidate (different, non-empty refresh token + non-empty
access token) and _hermes_install_disk_pair, which runs
enforce_refresh_token_issuer on the installed pair. Before, the adopt path
skipped the issuer check entirely, so a pair minted by a different issuer
could be POSTed straight to the new one.
The adopt path installs the candidate even when its access token has
already expired: the POST we are about to build needs the new refresh
token, and skipping the POST (_RefreshCompletedByPeer) is only correct
when the peer's access token is live with a positive TTL, mirroring the
reload path's clamp-to-zero guard. When the issuer enforcer strips the
refresh token there is nothing to refresh with, so the flow restarts into
401 -> full authorization instead of failing with OAuthTokenError.
The reload helper drops its outer except-Exception: get_tokens already
returns None for absent or corrupt files, so the blanket catch only hid
programming errors. Comment updated: this path exists for writers outside
the fence (interactive login, pre-fence Hermes), not for a fenced peer.
`_token_store_lock` serialized a single get_tokens()/set_tokens() call and
then released. Its two justifications no longer hold:
- torn reads: `_write_json` goes through `atomic_json_write` (write to a
sibling, rename), so a reader can never observe a half-written token
file, locked or not;
- the read-modify-write of a single-use refresh token: a lock released
between the read and the POST cannot close that race. `_refresh_fence`
now spans read -> POST -> persist, and every store access on the refresh
path (adopt-from-disk read, post-failure reload, `_store_tokens` write)
runs inside it.
The remaining unfenced accesses are the cold `_initialize` read and the
authorization-code exchange's full overwrite -- neither is a
read-modify-write, so neither needs mutual exclusion. Keeping a second,
fail-open lock layer only adds a 10 s stall on a stale lock file with no
correctness gain. Tests that exercised the removed lock go with it.
The 433-line harness spawned three interpreters and an HTTPServer to show
that one refresh generation is consumed once. The same invariant holds
in-process: flock is per open file description, so two real provider
instances sharing one token store contend for the fence exactly like two
processes do, and the SDK auth flow can be pumped with asend() the way
httpx does. Two tests now bind the fix deterministically:
- two providers, one store, a single-use token endpoint: exactly one POST
carries R1 and both providers end holding the rotated pair (the loser
adopts from disk and never presents the burned grant);
- fence held by another holder past the deadline: the refresh fails
closed, no POST is sent and neither memory nor disk loses the tokens.
The fence is entered from the SDK's coroutine-driven auth flow, so its
acquire loop spun on time.sleep(0.05) and blocked the whole event loop
for up to 60 s while a peer finished its network round trip. The fence
is now an async context manager that polls a non-blocking flock with
asyncio.sleep, and it creates the token directory itself (the parent may
not exist yet on a first refresh; secure_parent_dir only chmods).
The in-process RLock layer is dropped: an advisory lock on a fresh
descriptor already excludes sibling tasks and threads of the same
process, and a thread RLock is reentrant across asyncio tasks on one
thread, so it excluded nothing there anyway.
After acquiring the fence the provider re-reads the store; when a peer
already rotated the pair and the adopted access token is valid, it
raises _RefreshCompletedByPeer instead of building the refresh request.
The auth-flow wrapper restarts the SDK flow so the original request goes
out with the winner's access token. Previously the loser adopted the
new pair but still presented its stale refresh token, burning a
generation on every single-use provider.
Design lifted from #71715.
Co-authored-by: Kevin Yin <182213728+yinkev@users.noreply.github.com>
The token-store lock is per-operation: get_tokens() and set_tokens() each
take it and release it. With a provider that issues single-use refresh
tokens, two processes can therefore both read R1, both POST it, and the
loser gets invalid_grant on a session that was healthy:
A: get_tokens() -> R1 (lock taken and RELEASED)
B: get_tokens() -> R1 (lock taken and RELEASED)
A: POST R1 -> 200, receives R2
B: POST R1 -> 400, credential already burned
Add _refresh_fence(), held across read -> POST -> persist so exactly one
process consumes a refresh generation. It fails CLOSED: unlike the
token-store lock it raises RefreshFenceTimeout instead of degrading to
unlocked, because proceeding without ownership is the race itself. It
locks a .refresh.lock sibling rather than the token file, since
flock/msvcrt locks are per-descriptor and nesting one path would
self-deadlock on Windows and silently no-op on POSIX.
The provider takes the fence before its final read, re-reads under it so
a peer rotation is adopted instead of overwritten, and releases in
_handle_refresh_response. async_auth_flow also releases on abandonment:
a cancelled generator never reaches the handler, which would strand the
fence and turn the race into a deadlock. That wrapper delegates send and
throw manually -- async generators have no yield from, and `async for`
would feed the SDK response to the inner generator as None.
tests/tools/test_mcp_oauth_refresh_fence.py is an acceptance test, not a
unit test: two real OS processes refresh against a single-use-token
authorization server that audits every redemption. It asserts R1 is
presented exactly once, neither process clears the session, and disk
converges on the newest token. Verified to FAIL without the fence
(audit=[rt-1, rt-1]); a threading-only lock cannot catch this.
(cherry picked from commit c1508a1d47e2cbd94e05fa507684f3716a1e988c)
The desktop app spawns 'serve' while the scheduled task runs 'gateway run',
so two backends routinely share one HERMES_HOME. With a provider that issues
single-use refresh tokens, both could POST the same token and the loser's
refresh was rejected, clearing an otherwise healthy session.
Guard the token file's read-modify-write with a bounded advisory file lock
(fcntl on Unix, msvcrt on Windows, in-process only where neither exists),
mirroring how cron/jobs.py guards jobs.json. Acquisition is non-blocking with
a 10s ceiling: a briefly-contended refresh beats a permanently stuck client.
(cherry picked from commit 16f0d811de446a66ed5fd061fd7594fca3d230da)
The redirect out of the archive used a literal '/app.asar/' replace, so the
staged get-windows specifier built with path.join on a Windows packaged build
(...\resources\app.asar\dist\...) was never rewritten and the helper spawn
would still land inside the archive. Mirror the app.asar(?=$|[\\/]) regex
main.ts already uses for the same job, and cover the backslash path in the
test. The dev-tree no-op case folds into the segment-boundary test so the
describe block stays at three cases.
Closes#88468 (with the preceding commit from PR #89633).
`read_window_below` answers "could not enumerate windows on this system" on
every packaged macOS build. get-windows does its real work by exec'ing a Swift
helper whose path it derives from its own module URL, and execFile cannot run a
file that lives inside app.asar: the kernel sees the archive as a file, so the
spawn fails ENOTDIR. Electron does rewrite asar paths for child_process, and
only once that module has been pulled through the CJS loader, which this ESM
main process never does (every import here is `from 'node:child_process'`).
Both specifiers now resolve outside the archive. The staged copy added in
1c75e05982 is redirected, and so is the node_modules fallback, which needs
`import.meta.resolve` first to have a path to redirect. electron-builder
unpacks the whole get-windows directory, so the redirected path exists on the
real filesystem and the derived helper path is executable.
`/app.asar/` only ever appears as a complete path segment in a packaged build,
so outside one the replace is a no-op. Tests cover the packaged redirect, the
dev-tree passthrough, and an `app.asar-tools` lookalike that must be left
alone.
Verified on a packaged build, macOS 26.6.2 arm64, ad-hoc signed: the same
machine that returns the enumeration failure on unpatched main answers with the
window underneath and its title.
(cherry picked from commit dce34f626a0796fcd945aa3885384a1ec86c819f)
`_CodexCompletionsAdapter._build_responses_kwargs` already asks
`classify_responses_route` for the GitHub flag but hand-rolled its own
x.ai host match for `is_xai`. Two classifiers for one route drift; use
`route.is_xai_responses` so the aux path agrees with the main transport.
`is_copilot` (used for `is_github_responses` replay stripping) is left
as-is.
`_classify_responses_issuer` reimplemented endpoint canonicalisation with
its own urlsplit/urlunsplit pass. The repo already owns that logic in
`hermes_cli/route_identity.py::normalize_route_base_url` (stdlib-only,
used by agent/backend_identity.py), so delegate to it.
Reasoning items persisted before canonicalisation were stamped with the
raw `other:<agent.base_url>` (trailing slash, host case). Comparing them
verbatim against the now-canonical `current_issuer_kind` marked them
foreign and dropped them on the very endpoint that minted them. Run the
persisted stamp through the same canonicaliser (`_canonical_issuer_kind`,
non-`other:` kinds untouched) before comparing.
Also fixes the `_chat_messages_to_responses_input` docstring, which still
stated the pre-stack rule that legacy endpoint-stamped items drop when the
current model is known; the stack replays them on a matching issuer.
The transport wiring (build_kwargs model -> _last_issuer_model -> normalize_response
stamp) and the new error_classifier string had no test that fails when either is
removed; these two do.
The openai SDK appends a trailing slash to `client.base_url`, so the aux
adapter stamped `other:https://h/v1/` while the main transport stamped
`other:https://h/v1`. On custom Responses endpoints every aux call
(compression, flush_memories) therefore dropped all main-minted reasoning
items as "foreign".
`_classify_responses_issuer` now strips whitespace and trailing slashes and
lowercases scheme+netloc before stamping. The aux adapter also derives its
route flags from `classify_responses_route` — the single owner of the
codex/xai/github predicates — instead of an inline chatgpt.com host check,
and reuses the same flags for the effort clamp.
Native compaction checkpoints and reasoning items persisted before model
stamping existed carry only `_issuer_kind`. Treating a missing `_issuer_model`
as foreign dropped every such item once the current model was known, which
wiped existing sessions' native-compaction context on upgrade (four consumer
tests in test_native_compaction / test_native_preflight_estimate /
test_413_compression went red on the stack).
Trust the endpoint stamp when no model stamp is present, as main does today.
Items minted after this change carry the model stamp and still drop on a
same-endpoint model switch; a wrong guess on a legacy item is caught by the
invalid_encrypted_content 400 classifier and the replay kill switch.
Encrypted reasoning blobs are sealed to the model that minted them, not
only to the endpoint. Switching models on the same custom Responses
endpoint therefore replayed blobs the new model cannot decrypt and the
turn failed with HTTP 400.
Stamp captured reasoning items with `_issuer_model` (the canonical wire
model) alongside `_issuer_kind`, and replay an item only when both the
issuer kind and the model match the current request. Endpoint-stamped
legacy items without model provenance are dropped once the current
model is known (fail closed); ordinary assistant text stays replayable.
The transport threads the effective wire model (request_overrides win)
into conversion and normalization; the auxiliary Codex adapter stamps
and filters against its own model rather than the main agent's. The
400 classifier also recognises the custom-endpoint wording
"encrypted content could not be decrypted or parsed" so recovery strips
the replay state instead of aborting.
Hand-grafted from #95849 (final head d9cf6bcc08) onto current main; the
middleware-model-rewrite half is intentionally left out.
Closes#95834
The zero-knob test only asserted that ack_stale still trips with the knob
at 0. Because _read_websocket_health evaluates ack age before the
event-silence dimension, the test stayed green even with the
`_event_max_silence_seconds > 0` guard deleted: it never observed the
stale stamp being ignored. Assert (True, "healthy") with knob=0, a stale
stamp and a green transport first, then make the ACK stale and keep the
ack_stale assertion. Dropping the guard now fails this test.
Also correct the on_socket_event_type comment: discord.py dispatches
socket_event_type before the op-code switch but only for a non-null `t`;
heartbeat ACK frames carry `t: null`, they do not "return before" it.
The #109521 explanation (socket_event_type fires for every parsed
DISPATCH frame and is not debug-gated unlike on_socket_raw_receive;
op-11 ACKs carry no event type) was written out four times: knob init,
stamp init, the on_socket_event_type handler, and _read_websocket_health.
Keep the authoritative paragraph on the handler that actually stamps, and
point the other three sites at it so a future edit has one place to go.
Drop the math.isfinite(event_silence) half of the silence guard. Both
operands are our own perf_counter floats, so their difference cannot be
non-finite; the ack_age guard is different because _last_ack comes from
discord.py. math stays imported for the remaining finiteness checks.
`_warn_liveness_config_disabled` told operators an unusable value turns
off "the websocket liveness probe". That is true for the interval,
threshold, ack-age and latency knobs, which sit in the probe's startup
guard, but `websocket_event_max_silence_seconds` is only checked inside
`_read_websocket_health`, so ack-age/latency keep guarding. Say so, or
an operator reading the log would believe the whole watchdog is down.
The event-silence tests duplicated `_make_adapter`/`_connect` from
test_discord_liveness.py and carried a bespoke handler-wait loop. Reuse
the sibling helpers instead: `_make_adapter` grows an optional
`max_event_silence` that is only written into `extra` when given, so the
sibling's own tests keep the adapter default and stay unchanged.
Two invariants now bind the fix:
- deaf socket: a `None` stamp (no DISPATCH yet) reads healthy across
several probe intervals, and once armed, transport-green + event
silence trips `event_silence` through `_liveness_loop`.
- `websocket_event_max_silence_seconds: 0` disables only that
dimension: with a stale stamp and a stale heartbeat ACK the probe must
still run and trip on `ack_stale`. This goes red if the knob is moved
into `_start_liveness_probe`'s all-or-nothing guard (#109782).
The YAML->extra passthrough test also asserts the new key, so dropping
its `_YAML_WEBSOCKET_LIVENESS_KEYS` entry fails.
Trim the new event-silence file from 8 tests to the 2 that bind the fix:
the deaf-socket e2e through `_liveness_loop` (transport green, no
DISPATCH → probe trips with the `event_silence` reason) and the
None-stamp window reading healthy (no false trip on quiet reconnects).
The removed cases re-checked knob parsing, defaults and the YAML seed
loop already covered by the sibling liveness knobs' tests, or restated
the two kept invariants from a different angle.
The adapter reads `websocket_event_max_silence_seconds` (14400 s) but the
key was never added to DEFAULT_CONFIG, so CONFIG_SCHEMA — generated from
it — did not expose the new liveness threshold. Manual YAML worked while
dashboard users could not discover or edit it; the four sibling
websocket_* discord liveness keys are all registered there. Register it
with the same default so both config surfaces stay consistent.
Incident 2 of #109521: a Gateway socket can stay ESTABLISHED and keep
ACKing heartbeats while zero DISPATCH events are parsed, so every
transport-side liveness sample (ready/open/ack-age/latency) reads
healthy for hours. The merged #109963 deliberately dropped the
event_silence dimension: a raw-frame stamp is debug-gated
(on_socket_raw_receive needs enable_debug_events) and, since heartbeat
ACKs are frames, ack_stale always fires first by construction.
This adds the dispatch-side signal that was requested instead:
- stamp on on_socket_event_type, which discord.py 2.7.1 dispatches for
every parsed DISPATCH frame with no debug gate (verified live against
the real received_message path: 4/4 frames fired with
enable_debug_events=False, on_socket_raw_receive 0/4)
- new knob websocket_event_max_silence_seconds (default 4h, the
incident report's field-proven operator bound); 0 opts out of this
dimension ONLY — the #109782 review failure put the knob in
_start_liveness_probe's all-or-nothing guard, killing the whole
watchdog; it is gated strictly inside _read_websocket_health here
- the stamp resets per connection (connect() clears it), and a None
stamp (no event parsed yet on this connection) is not silence
- docs (en + zh-Hans) cover the new knob and the per-dimension opt-out
Fixes#109521
(cherry picked from commit b4baa97fc45794209711a45e052111d7d44d5f90)
The timeout branch of _run_hook_callback_bounded unconditionally added
gate_key to _hook_abandoned. A worker that finishes between done.wait()
returning False and the caller taking the lock has already popped its
token via _release_token, so nothing would ever clear that entry: the
callback stayed blocked for every later call id until reload with no
thread behind it. Guard the insert on the worker still being registered.
The new test makes the race deterministic by swapping the module's
threading.Event for one whose wait() lets the worker finish and then
reports a timeout, and asserts a fresh call id still runs.
Also pass tool_call_id inline from terminal_tool_result instead of the
conditional dict plumbing: an empty id is already treated as "no
identity" by _hook_call_identity and unknown fields are withheld from
narrow-signature callbacks (same shape as _fire_approval_hook). Update
the stale "(hook_name, id(cb))" comment above _hook_running_callbacks.
Hook callbacks are gated per (hook, callback, call identity). Two hooks
on the tool-loop path fired without any identity, so concurrent terminal
calls in one turn, or overlapping turns, still collapsed onto a single
gate key and the second invocation was skipped as if a callback had hung.
transform_terminal_output now forwards the tool_call_id bound in the
approval context around dispatch (only when set); transform_llm_output
forwards the turn_id already in scope. Payloads are additive: the
dispatcher withholds unknown fields from narrow-signature callbacks.
The same-call negative control fired two sequential calls with a 0.1 s
timeout, so the first call timed out and the second was dropped by the
60 s suppression window; the identity gate itself was never exercised.
Mirror the positive test instead: a 5 s timeout, two threads with the
same tool_call_id while the callback is held on an Event.
Add one test for the abandoned-worker gate: a hung callback followed by
a call with a fresh tool_call_id, with suppression zeroed, must start
exactly one worker. Reverting the gate change makes it fail.
