Skip to content

fix: 修复弱网验证码超时及重试等待(Bug872) - #788

Open
jia030928 wants to merge 6 commits into
MemTensor:v1.2.7from
jia030928:fix-bug872-login-code-network-v126
Open

jia030928 wants to merge 6 commits into
MemTensor:v1.2.7from
jia030928:fix-bug872-login-code-network-v126

Conversation

@jia030928

@jia030928 jia030928 commented Oct 29, 2026 •

Copy link
Copy Markdown

问题

Bug872:弱网下验证码请求沿用普通云请求的 5 秒超时,较慢响应被提前终止;失败后立即重试容易重复请求并触发限流。重新进入登录页以及改变邮箱大小写,还可能绕过发送等待。

修改

  • 手机、邮箱验证码发送及校验的超时下限改为 10 秒,保留更长的已配置超时;普通请求超时不变。
  • 保留云端限流分类,验证码超时返回 HTTP 504。界面按超时、连接失败、服务错误和限流显示提示,不再显示“发送结果暂未确认”。
  • 前端同步防重复调用;后端按规范化账号合并在途请求、共享失败等待,邮箱大小写变化不会重复发送或绕过等待。发送给云端的账号原文不变。
  • 失败后保留 60 秒重试等待,通过 Retry-After 返回剩余秒数;只有允许的跨域来源可读取该响应头。失败不写入成功发送记录,不自动重发。
  • 倒计时使用截止时间并在窗口恢复时校准。正常 1~60 秒剩余等待原样保留,非法/零值回退 60 秒,过大值限制为 60 秒;成功及失败响应统一处理。
  • 等待结束后清除对应提示,不覆盖后续登录错误;发送限流明确提示等待倒计时,登录限流使用“请稍后再试”。等待期间仍可输入验证码并登录。

验证

  • 已使用 merge 同步 upstream/v1.2.7 的 38bbafab,合并无冲突;最终提交 d1f155cf,相对目标分支为 Bug872 的 15 个文件。

  • 合并后 npm run lint、npm run typecheck、交付差异检查通过。

  • 完整 npm test 已重新执行:发布流程 129 项、打包保护 132 项、接口契约 8 项、OpenCode 24 项通过;Memory 两轮各 2692 项、后端 1313 项、前端 3368 项、桌面 Shell 1342 项通过。仓库原有跳过项保留。TUI 链路因前序失败未自动运行,已单独补跑 5 项通过。

  • 全量仍有 1 项失败:office-plugin-e2e.test.ts 启用插件时 fetch failed。该失败此前已在纯上游源码复现;本次同步中相关测试、插件及运行时实现均未变化。之前的三项前端基线失败已由上游修复,本轮前端全部通过。没有跳过或改写失败断言。

  • 初次回归因 Electron 43.8.1 首次下载未使用代理而中断;补齐锁定版本运行时后重新执行上述完整回归,环境失败日志单独保留。

  • 合并版本 macOS arm64 CN unsigned 重新打包成功(Electron 43.8.1);DMG 校验和构建/镜像/安装三份 ASAR 一致。DMG SHA256:1ca7976f9aee9a3dbd95f65d25e332e2b7fd94581cb1a5ef7c2cc27d58a9e6d0。

  • 新包内手机 7 组、邮箱 6 组隔离接口场景通过:6.5 秒慢响应成功、约 10 秒超时、断连/限流后的等待、同账号请求合并、邮箱大小写一致性、允许来源读取 Retry-After、拒绝未允许来源、真实等待后手动恢复且无自动重发。

  • 邮箱首次实时边界断言失败;临时夹具改为校验时钟确实到达 43 秒再检查,保留原断言与失败日志。复测在实际 43.002 秒时返回剩余 17 秒,60 秒后恢复;没有改动产品源码。

  • 新包原生首次启动和重启均进入登录页,主进程正常退出;退出后仍有一个 Computer Use 辅助进程残留,按测试路径清理后重启。DMG 可选背景替换仍报空间不足警告,镜像校验通过。

范围与限制

  • 失败等待保存在当前后端进程内,可防止刷新/重新进入页面绕过,完全退出应用后不保留。
  • 相对目标分支没有依赖、版本号或数据库结构改动。包内版本沿用上游的 1.2.4,PR 目标为 v1.2.7。
  • 隔离验证使用本地模拟云,未覆盖真实短信/邮件送达、真实账号完整登录及已有数据升级保留。
  • 原生退出后 Computer Use 辅助进程残留、DMG 可选背景替换警告,详见本轮打包结果;未扩大 Bug872 范围处理。
  • 上轮外部 autotest 返回 NO TEST SCOPE,没有运行测试。平台 env.yaml 的位置及格式尚未提供,未在仓库添加猜测配置;该状态不能视为功能测试通过。

@Memtensor-AI

Memtensor-AI commented Oct 29, 2026 •

Copy link
Copy Markdown
Collaborator

🤖 Open Code Review

Target: PR #788
Task: 4ceb6004802df4bf
Base: v1.2.7
Head: fix-bug872-login-code-network-v126

🔍 OpenCodeReview found 12 issue(s) in this PR.

⚠️ Review warnings (3)

  • bounded analysis finalized with explicit task_done after reviewing all group files
  • bounded analysis finalized with explicit task_done after reviewing all group files
  • bounded analysis finalized with explicit task_done after reviewing all group files

1. App/backend/src/adapters/outbound/cloud-client/http-cloud-client.ts (L1006-L1009)

toCloudPhoneVerificationError only relies on classifyCloudError, which recognizes business throttling in the 40300–40399 range (plus HTTP 429/400/401/403). The sibling email mapper additionally treats the verification code family 40110–40120 (rate-limit codes 40112–40115) as throttling, because those business codes can arrive with HTTP 200.

If the phone verification endpoint shares that code family (both endpoints go through the same upstream verification service), a phone throttle returned as HTTP 200 + 40112 will be classified as internal → the local API answers 500 instead of 429, and the new Retry-After path never triggers, which defeats the purpose stated in the comment above. Please confirm the phone code range and, if it overlaps, extract a shared verification-code classifier instead of falling back to the generic classifyCloudError.


2. App/backend/src/adapters/outbound/cloud-client/http-cloud-client.ts (L131)

The timeout override is repeated three times (sendEmailCode, sendPhoneCode, login) while the file already follows the opposite pattern for social login (socialLoginTimeoutMs(timeoutMs)). Extracting a verificationTimeoutMs(timeoutMs) helper next to socialLoginTimeoutMs keeps the two timeout policies consistent and avoids a fourth copy in the future.

💡 Suggested Change

Before:

      await requestBoolean(fetchImpl, baseUrl, Math.max(timeoutMs, CLOUD_VERIFICATION_TIMEOUT_MS), "/api/agentUser/sendPhoneVerification", {

After:

      await requestBoolean(fetchImpl, baseUrl, verificationTimeoutMs(timeoutMs), "/api/agentUser/sendPhoneVerification", {

3. App/backend/src/adapters/outbound/cloud-client/http-cloud-client.ts (L545-L547)

Timeout detection here (and in readCloudEnvelope's new error.name check) depends on the rejection carrying name === "AbortError" | "TimeoutError". When the abort happens while reading the body, undici does not always surface the abort reason: e.g. a torn-down socket rejects response.text() with TypeError: terminated. In that case readCloudEnvelope swallows the error and synthesizes { code: response.ok ? 0 : response.status, data: null }, so the timeout is silently converted into a bogus success (sendPhoneCode would then fail with "Cloud verification response was not true" → internal 500 instead of 504).

Basing the decision on the signal state rather than the error shape is more robust: pass signal into readCloudEnvelope and rethrow signal.reason whenever signal.aborted, regardless of the underlying error.


4. App/frontend/desktop/src/components/use-verification-code-auth.ts (L266-L267)

Failure→message mapping now exists in two places: this path uses codeRequestFailureKeys (countdown wording), while resolveAuthErrorMessage independently re-classifies and maps rateLimited/timeout (non-countdown wording). Consequences:

  1. The new rateLimited/timeout branches in resolveAuthErrorMessage are unreachable for the send path (failure is always non-null here), so the two tables can drift silently.
  2. For the login path, a classified server/connection failure is not handled by resolveAuthErrorMessage and falls through to login.loginFailed ("check your code"), which misleads the user when the request actually failed on the network/server side.
    Suggestion: give resolveAuthErrorMessage a suffix/key-map parameter (e.g. the failure→MessageKey table) so a single function handles all four buckets for both call sites.

5. App/frontend/desktop/src/components/use-verification-code-auth.ts (L189-L191)

The same constant fallbackResendSeconds doubles as "default window" and "maximum window", so a server Retry-After larger than 60s is silently truncated and the resend button re-enables before the server allows it (repeated 429s). Conversely error.retryAfterSec ?? fallbackResendSeconds passes a server-sent 0 down here only to be replaced by the fallback, so an explicit server instruction is ignored either way. Today the backend caps at 60s (RESEND_WINDOW_MS / 1000), but this is an implicit coupling that will break silently if that window grows. Consider separate defaultResendSeconds/maxResendSeconds constants and honouring a server value above the default (bounded only by a generous sanity cap).


6. App/frontend/desktop/src/api/http.ts (L61)

toApiRequestError is the shared HTTP layer, but the Retry-After handling only accepts bare integer seconds (RFC 7231 also allows an HTTP-date) and applies no sanity bound, so consumers must re-clamp whatever value comes out (startCountdown caps it again). Parsing is also coupled to one endpoint's formatting via the comment. Suggest extracting a small parseRetryAfter(value): number | null helper that trims/normalizes the value and clamps to a sane maximum (e.g. 15 min), keeping the retry semantics in one place instead of split across this file and the hook.


7. App/backend/src/services/verification-request-error.ts (L8)

code is derived only from status === 429, so a timeout (504) is emitted as code: "internal". error-envelope.ts's HTTP_STATUS_BY_CODE maps internal to 500, so the envelope now returns HTTP 504 with a body code that means 500. Clients that branch on code (e.g. to pick a localized message) cannot distinguish a timeout from a genuine server fault, and the code→status contract used everywhere else in toErrorEnvelope no longer holds. Consider either deriving the status from the code (both 429 and 500) or representing timeouts explicitly so code and status stay consistent.


8. App/backend/src/services/account-service.ts (L82-L83)

The destructured address here actually holds the full channel:address key (the map is keyed by key), which is misleading, and the whole map is swept on every request. Prefer a clear name and check/expire only the current key (const retryAt = failedCodeRetryAt.get(key); if (retryAt !== undefined && retryAt <= time) failedCodeRetryAt.delete(key);), which also keeps the hot path O(1).

💡 Suggested Change

Before:

    for (const [address, retryAt] of failedCodeRetryAt) {
      if (retryAt <= time) failedCodeRetryAt.delete(address);

After:

    const existingRetryAt = failedCodeRetryAt.get(key);
    if (existingRetryAt !== undefined && existingRetryAt <= time) failedCodeRetryAt.delete(key);

9. App/backend/src/services/account-service.ts (L147)

The "preserve definitive validation/auth errors, map the rest" idiom now appears twice with different shapes (block-body .catch(... => { throw x ?? error }) here, and const failure = toVerificationRequestError(error, ...); if (!failure) throw error; inside sendCodeRequest). Please extract a small helper (e.g. mapVerificationFailure(error, retryAfterSec)) or use a plain try/catch in both places so the pass-through rule is single-sourced and the login path reads consistently.


10. App/frontend/desktop/src/components/use-verification-code-auth.ts (L36)

TypeError is used as the "network failure" signal, but in this renderer it is also thrown for genuine programming errors (e.g. "Cannot read properties of undefined"). Such an error would now be reported as "Check your network" and, via the failure !== null branch in sendCode, immediately start a 60s resend lockout — hiding real bugs behind a misleading message and blocking the user. Consider narrowing the detection to the actual fetch rejection shapes (error instanceof TypeError on sendCode/verifyCode only after the request was issued, or checking navigator.onLine / the loader-specific abort error) and let unexpected errors fall through to the generic fallback without starting a cooldown.

💡 Suggested Change

Before:

  if (error instanceof TypeError) return "connection";

After:

  if (error instanceof TypeError && isNetworkFailureMessage(error.message)) return "connection";

11. App/backend/src/services/verification-request-error.ts (L15)

Only three codes bypass the wrapping. Any other error that already carries a valid ApiErrorCode (infrastructure codes such as not_found, conflict, memory_layer_unavailable, plugin_timeout, ...) or a ZodError coming out of a CloudClient implementation is folded into a VerificationRequestError with status 500 / code internal. That both drops the original code and bypasses error-envelope's existing mappings (e.g. ZodError -> 400 invalid_argument with issues[0].message), so a malformed cloud payload would surface as a 500 whose message is the raw Zod issue JSON. Suggest leaving any error that already looks envelope-compatible untouched, e.g. if (error instanceof ZodError) return null; plus if (typeof code === "string" && API_ERROR_CODES.includes(code)) return null; (or only wrap when code/status is absent or clearly transient).


12. App/backend/src/services/error-envelope.ts (L96-L98)

Retry-After is emitted for whatever status the envelope carries, including 500 and 504. RFC 9110 defines Retry-After for 3xx/429/503 only, and sendCodeRequest passes retryAfterSec for plain internal failures too, so internal 500s will advertise Retry-After: 60 and generic HTTP clients/proxies may auto-retry a failed send-code call. Consider restricting the header to 429/503 (or explicitly documenting why a delay is advertised on 5xx).

💡 Suggested Change

Before:

      if (envelope.retryAfterSec !== undefined) {
        reply.header("Retry-After", String(envelope.retryAfterSec));
      }

After:

      if (envelope.retryAfterSec !== undefined && (envelope.status === 429 || envelope.status === 503)) {
        reply.header("Retry-After", String(envelope.retryAfterSec));
      }

Generated by cloud-assistant via Open Code Review.

@Memtensor-AI

Copy link
Copy Markdown
Collaborator

⚠️ Automated Test Results: NO TEST SCOPE

Automated tests were not run because the changed files do not map to an executable test scope.

Details: No executable test scope maps to the changed files. Automated tests were not run; add env.yaml source_mapping + execution for this path family, then rerun. Changed files: App/backend/src/adapters/outbound/cloud-client/http-cloud-client.test.ts, App/backend/src/adapters/outbound/cloud-client/http-cloud-client.ts, App/backend/src/services/account-service.ts, App/backend/src/services/error-envelope.ts, App/backend/src/services/tests/account-service.test.ts, App/backend/src/services/tests/verification-retry-flow.test.ts, App/backend/src/services/verification-request-error.ts, App/frontend/desktop/src/api/http.ts, App/frontend/desktop/src/api/tests/http.test.ts, App/frontend/desktop/src/components/tests/use-verification-code-auth.test.ts
Manual review or env.yaml source_mapping/execution coverage is required before merge.

Branch: fix-bug872-login-code-network-v126

@Memtensor-AI

Copy link
Copy Markdown
Collaborator

⚠️ Automated Test Results: NO TEST SCOPE

Automated tests were not run because the changed files do not map to an executable test scope.

Details: No executable test scope maps to the changed files. Automated tests were not run; add env.yaml source_mapping + execution for this path family, then rerun. Changed files: App/backend/src/adapters/inbound/local-api/server.ts, App/backend/src/adapters/inbound/local-api/tests/server-cors.test.ts, App/backend/src/adapters/outbound/cloud-client/http-cloud-client.test.ts, App/backend/src/adapters/outbound/cloud-client/http-cloud-client.ts, App/backend/src/services/account-service.ts, App/backend/src/services/error-envelope.ts, App/backend/src/services/tests/account-service.test.ts, App/backend/src/services/tests/verification-retry-flow.test.ts, App/backend/src/services/verification-request-error.ts, App/frontend/desktop/src/api/http.ts
Manual review or env.yaml source_mapping/execution coverage is required before merge.

Branch: fix-bug872-login-code-network-v126

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants