Repository navigation
Record upstream error bodies and auto-disable terminally rejected accounts - #93
Merged
Merged
Conversation
- Persist the full upstream error JSON (256k cap, redacted) on every non-200 as upstream_code/upstream_msg/upstream_body audit fields; raise the audit diagnostic budget to fit it. - Keep the last error body on the credential entry and expose it at /admin/credentials; the WebUI shows it on hover (desktop) or tap (mobile), click pins the popover open. - Upstream restriction 11140 (account limited for unusual activity) now disables the credential through the WebUI policy instead of relying on the 300s auth cooldown; plain 401/403 unchanged.
reset_cooldowns_for now also drops the recorded failure evidence (last_failure_at, last_error_body), so the WebUI re-enable toggle and the reset-cooldown endpoint both hand back a credential with no cooldown, no error label and no stale tooltip JSON.
An upstream 11140 restriction now marks the entry auto_disabled; the inventory exposes it, health reports auto_disabled, and the WebUI shows 自动禁用 on the badge instead of the manual disabled label. Re-enable and reset clear the flag along with the failure evidence.
Two new terminal signals feed the same auto-disable chain as the 11140 restriction: three consecutive refresh-token business rejections (network faults excluded, counted per credential, reset on success), and three 401s against a token refreshed within the last two minutes inside an hour. Refresh rejections now raise TokenRefreshRejected with the structured code/msg/body so the tooltip shows the upstream verdict instead of only a log line. Reset or re-enable clears both counters with the rest of the state.
The health badge no longer counts down 认证熔断 for disabled credentials: an auto-disabled or manually stopped account is in a steady state, so the badge shows 自动禁用 / 已停用 directly. Test expectations follow the widened audit diagnostic budget and the new upstream_error attempt records.
Reviewer's GuideThe PR adds a bounded, redacted upstream-error audit pipeline and credential-level error visibility, then uses terminal upstream verdicts and repeated fresh-token/refresh failures to auto-disable affected accounts. It also introduces a complete fresh-state reset contract and updates the WebUI/admin inventory to report automatic disabling and inspect the latest upstream response. Sequence diagram for terminal upstream rejection and credential auto-disablesequenceDiagram
participant Upstream
participant Gateway
participant CredentialManager
participant ControlStore
participant WebUI
Upstream-->>Gateway: HTTP 403 with JSON code 11140
Gateway->>Gateway: _upstream_diagnostic(raw)
Gateway->>CredentialManager: note_status(cred, 403, raw)
CredentialManager->>CredentialManager: _note_error_body()
CredentialManager->>CredentialManager: _auto_disable()
CredentialManager->>ControlStore: set_credential(identity, false)
CredentialManager->>WebUI: invalidate_model_table()
WebUI-->>WebUI: Display 自动禁用
Sequence diagram for upstream error audit and credential tooltipsequenceDiagram
participant Upstream
participant Gateway
participant AuditStore
participant AdminAPI
participant WebUI
Upstream-->>Gateway: Non-200 JSON response
Gateway->>Gateway: _upstream_diagnostic(raw)
Gateway->>AuditStore: observe_attempt(upstream_code, upstream_msg, upstream_body)
AuditStore->>AuditStore: safe_attempt()
AuditStore->>AuditStore: safe_diagnostic() and safe_body()
AdminAPI->>WebUI: last_error_body and last_error_code
WebUI-->>WebUI: LastErrorJson hover, tap, or click
Sequence diagram for refresh rejection counting and fresh-state resetsequenceDiagram
participant Upstream
participant Gateway
participant CredentialManager
participant ControlStore
participant AdminAPI
Gateway->>CredentialManager: refresh_due()
CredentialManager->>Upstream: Refresh token request
Upstream-->>CredentialManager: TokenRefreshRejected
CredentialManager->>CredentialManager: _note_refresh_reject()
alt Three consecutive refresh rejections
CredentialManager->>ControlStore: set_credential(identity, false)
end
AdminAPI->>CredentialManager: admin_set_credential_enabled(identity, true)
CredentialManager->>CredentialManager: reset_cooldowns_for(identity)
CredentialManager-->>AdminAPI: Fresh state with counters and failure evidence cleared
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 6 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="converter.py" line_range="1429" />
<code_context>
+ _, _, body = _upstream_diagnostic(raw or b"")
+ if not body:
+ return
+ with self._lock, (cm._lock if generation is not None else nullcontext()):
+ if not self._lease_matches(cm, generation):
+ return
</code_context>
<issue_to_address>
**Credential handling deadlocks**
When a third refresh rejection overlaps a path holding the pool lock while waiting for the same credential lock, `refresh_due` holds `cm._lock` while `_note_refresh_reject` reaches `_auto_disable`, which waits for `self._lock`; the concurrent pool operation holds `self._lock` while waiting for `cm._lock`, so both threads block and credential handling stalls.
Release `cm._lock` before calling `_auto_disable`, or otherwise ensure both paths acquire the locks in the same order.
Also at `converter.py:1471`, `converter.py:1653`.
</issue_to_address>
### Comment 2
<location path="converter.py" line_range="569" />
<code_context>
+ for key in ("code", "errCode", "error_code"):
+ value = node.get(key)
+ if value is not None and str(value).strip():
+ code = code or str(value).strip()
+ for key in ("msg", "message"):
+ value = node.get(key)
</code_context>
<issue_to_address>
**Nested rejection code is ignored**
When an upstream rejection has a non-terminal top-level code and a terminal code under `error`, `_diagnostic_from_payload` keeps the first non-empty code from the outer payload, so a generic outer code masks a terminal code in `error`. `_upstream_diagnostic` then records the wrong `upstream_code` and `note_status` misses the account restriction, leaving the credential enabled.
Prefer the nested error code when present, or otherwise define and apply explicit precedence for envelope versus business codes.
</issue_to_address>
### Comment 3
<location path="converter.py" line_range="1432" />
<code_context>
+ with self._lock, (cm._lock if generation is not None else nullcontext()):
+ if not self._lease_matches(cm, generation):
+ return
+ store.set_credential(identity, False)
+ self._sticky.clear()
+ entry["auto_disabled"] = True
</code_context>
<issue_to_address>
**Upstream rejection can become gateway failure**
When the control-store write fails while handling a terminal upstream rejection, `store.set_credential()` propagates SQLite and other control-store errors through `_auto_disable()` and `note_status()` before `_check_upstream_status()` raises the original `UpstreamHTTPError`. A control-store outage therefore replaces an upstream 403/401 response with a gateway error instead of forwarding the upstream rejection.
Catch and log control-store failures in the auto-disable path so they do not replace the original upstream response.
</issue_to_address>
### Comment 4
<location path="tests/test_credential_cooldowns.py" line_range="417" />
<code_context>
+ body=body.decode())
+ for _ in range(2):
+ pool._note_refresh_reject(entry, error)
+ self.assertEqual(entry["refresh_rejects"], 2)
+ pool._note_refresh_reject(entry, error)
+ self.assertTrue(entry["auto_disabled"])
</code_context>
<issue_to_address>
**Refresh rejection wiring goes untested**
When the `TokenRefreshRejected` handler in `refresh_due` is broken or missing, when `refresh_due` stops calling `_note_refresh_reject` for `TokenRefreshRejected`, `test_consecutive_refresh_rejections_auto_disable` still passes: lines 415–419 call the helper directly three times. Real refresh rejections then never trigger auto-disable.
Exercise `refresh_due` with a credential whose refresh raises `TokenRefreshRejected`, and verify its rejection count and auto-disable behavior.
</issue_to_address>
### Comment 5
<location path="tests/test_upstream_diagnostics.py" line_range="121" />
<code_context>
+ self.client.post(ROUTE, json={"model": "auto", "messages": [{"role": "user", "content": "hi"}]})
+ entry = next(a for a in self._attempts() if a.get("stage") == "upstream_error")
+ self.assertNotIn("synthetic-private-token", entry["upstream_body"])
+ raw = b"".join(f.read_bytes() for f in Path(self.directory).glob("logs.sqlite3*"))
+
+ def test_secrets_in_upstream_messages_are_dropped(self):
</code_context>
<issue_to_address>
**Stored body secret is not checked**
When a secret is present in the upstream body and database storage is unsafe while record reads redact it, when the audit database stores the original secret but strips it only from the returned record, `test_body_tokens_are_redacted_before_storage` passes because it checks only `entry["upstream_body"]`. The test reads the SQLite files at line 121 but never checks their contents, so a persisted token goes undetected.
Assert that the collected SQLite bytes do not contain `synthetic-private-token`.
</issue_to_address>
### Comment 6
<location path="web/src/pages/Credentials.tsx" line_range="144-145" />
<code_context>
+ className={s.errorJsonTrigger}
+ aria-expanded={open}
+ onClick={() => {
+ holdOpen();
+ setPinned((value) => !value);
+ }}
+ {...hoverProps}
</code_context>
<issue_to_address>
**Mobile trigger cannot toggle tooltip closed**
When a mobile user taps the error trigger to open the tooltip and then taps it again, on touch, `holdOpen()` sets `hovered` to true, but the pointer handlers ignore non-mouse events, so no pointer leave clears it. Tapping the trigger again only clears `pinned`; `open` remains true because `hovered` is still true, and the user must tap elsewhere or press Escape to dismiss the tooltip.
Only set `hovered` for mouse interaction, and let touch trigger taps toggle the open state without leaving `hovered` set.
</issue_to_address>Sourcery assessment
Needs a human reviewer. 5 findings to address first, and a matching restriction code, repeated fresh-token 401s, or repeated refresh rejection can persistently disable a credential through the control store; upstream error bodies are also added to retained audit records. Reverting the code would not re-enable credentials already disabled or remove bodies already stored.
Blocking findings: converter.py:1429, converter.py:569, converter.py:1432, tests/test_credential_cooldowns.py:417, tests/test_upstream_diagnostics.py:121
…ggle
- refresh_due defers _note_refresh_reject past the credential lock: the
third rejection reaches _auto_disable, which takes the pool lock, so
calling it under cm._lock could deadlock against pool paths that
acquire pool -> credential in the opposite order.
- _diagnostic_from_payload prefers the nested error envelope over a
generic outer code, so {"code":"error", "error":{"code":11140}}
no longer masks the terminal restriction code.
- _auto_disable catches control-store failures and logs them; a store
outage no longer replaces the upstream 401/403 being reported.
- Touch taps toggle the tooltip purely through the pin; holdOpen stays
mouse-only so a tap can never strand the hover state open.
- Tests: refresh_due wiring (patched refresh_if_due raising
TokenRefreshRejected), nested-code precedence, and the persisted-bytes
secret assertion.
- Refresh rejections are consecutive only now: a request-path refresh (get_headers -> _refresh) succeeding after the last rejection, or a gap beyond one hour, resets the counter — sporadic rejections over days no longer accumulate to the disable threshold. - TokenRefreshRejected during a keepalive pass now applies the same CRED_KEEPALIVE_RETRY_S backoff as every other keepalive failure. - The 401/403 body is parsed once in note_status and handed to the tooltip note, instead of re-parsing (and sanitizing) three times, once under the pool lock. - auto-disabled provenance persists in the control store (disable_credential_auto) and is adopted on pool rebuild, so a restart or cooldown reset keeps the 自动禁用 label and its tooltip body; any manual toggle clears the provenance, and the switch label is a plain 停用 while the badge distinguishes 人工停用/自动禁用. - Manual disable clears the flag on the live entry; entries() copies no longer swallow the write.
admin_credential_inventory resolved model_policy.snapshot() (a dict) into helpers expecting a PolicyView, so every credentials listing and toggle answered 500 管理操作失败. Reuse PolicyView.credential on one view per inventory call and drop the now-unused helper.
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
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
upstream_code/upstream_msg/upstream_bodyaudit fields (256k cap, redacted), and widen the audit diagnostic budget to fit it. The credentials tooltip in the WebUI now shows the last upstream verdict on hover (desktop) or tap (mobile); click pins it open.11140, three consecutive refresh-token business rejections (network faults excluded, counter resets on success), or three 401s against a token refreshed within two minutes inside an hour. The badge reports 自动禁用 instead of a meaningless cooldown countdown; plain 401/403 behavior is unchanged.Verification
for s in tests/test_*.py; do uv run python -B "$s"; done.vp check, 205 WebUI unit tests andvp buildare clean; deployed locally and verified end to end with live 403 traffic (11140 request illegalrecorded with fulldisplayMsgenvelopes).Summary by Sourcery
Record bounded, sanitized upstream rejection diagnostics and automatically disable credentials only when repeated evidence indicates a terminal account failure.
New Features:
Bug Fixes:
Enhancements:
Tests: