Skip to content

fix(local-asr): 模型下载「已存在即跳过」改为校验大小,拒收截断/损坏文件 (#686)#687

Open
appergb wants to merge 2 commits into
betafrom
fix/download-integrity-size-check
Open

fix(local-asr): 模型下载「已存在即跳过」改为校验大小,拒收截断/损坏文件 (#686)#687
appergb wants to merge 2 commits into
betafrom
fix/download-integrity-size-check

Conversation

@appergb

@appergb appergb commented Jun 16, 2026

Copy link
Copy Markdown
Collaborator

User description

关联 issue

Closes #686

问题

asr/local/download.rs::run_download 在目标文件已存在时仅凭 dest.exists() 跳过,不校验大小:上次被中断 / 外部损坏而残留的截断文件被当成「已下完」跳过,最终在模型加载时以含糊错误失败,用户难自查。(多块路径 download_one 末尾已有 actual != total_size 校验,缺口在这个「已存在即跳过」分支。)

改动(单一职责)

  • 新增纯函数 existing_file_is_complete(actual_size, expected_size):大小一致 → 完整;不符 → 不完整;expected_size == 0(HF 未给大小)→ 退回旧行为「存在即信任」;元数据取不到 → 不完整。
  • run_downloaddest.exists() 时据此判定:完整才跳过;否则 log::warn + 删除残留文件,回到正常下载路径重下。

测试

cargo test --lib existing_file_complete
  existing_file_complete_only_when_size_matches_or_unknown ... ok

真值表覆盖:一致 / 截断 / 超大 / 无元数据 / 未知大小。

来源:2026-06-16 全仓多 Agent 审计(asr-local 专项)。


PR Type

Bug fix, Tests


Description

  • Added file size verification for existing downloads

  • Delete and redownload truncated/corrupted files instead of skipping

  • Fixed progress bar double-counting during redownloads

  • Unit tests for the integrity check function


Diagram Walkthrough

flowchart LR
  A[File exists?] -->|Yes| B{Check file size}
  B -->|Size matches or unknown| C[Skip, count as done]
  B -->|Size mismatch| D[Delete file, then download]
  A -->|No| D
  D --> E[Download normally]
  E --> F[Download complete]
  C --> F
Loading

File Walkthrough

Relevant files
Bug fix
download.rs
Add integrity check for existing files                                     

openless-all/app-tauri/src-tauri/src/asr/local/download.rs

  • New pure function existing_file_is_complete to verify file integrity
    via size comparison
  • Modified run_download to check file size before skipping; delete and
    redownload if corrupt
  • Updated already_done_bytes to only count truly complete files, fixing
    progress double-counting
  • Added unit tests for the integrity check with edge cases (truncated,
    oversized, missing metadata, unknown size)

run_download 此前仅凭 dest.exists() 跳过已存在文件,不比对 file.size——上次被
中断或外部损坏而残留的截断文件会被当成「已下完」跳过,最终在模型加载时以含糊
错误失败。

抽纯函数 existing_file_is_complete(actual, expected):大小一致才算完整;
expected==0(HF 未给大小)退回「存在即信任」;元数据取不到算不完整。run_download
据此判定,大小不符则删除残留文件重新下载。附真值表单测。
@appergb

appergb commented Jun 16, 2026

Copy link
Copy Markdown
Collaborator Author

@claude 请审核:本地模型下载「已存在即跳过」改为校验大小。重点看 (1) expected_size==0(HF 未给大小)退回「存在即信任」是否合理、会不会漏掉真实截断;(2) 删除残留文件后是否能正确回到下载路径重下(不会与多块续传/sentinel 逻辑冲突);(3) 是否还应顺带校验单 GET 退化路径(total_size 未知)的收到字节数。

@claude

claude Bot commented Jun 16, 2026

Copy link
Copy Markdown

Claude finished @appergb's task in 2m 13s —— View job


代码审查:本地模型下载完整性校验

  • 读取 PR diff 和改动文件
  • 分析三个重点问题
  • 发布审查意见

TL;DR

整体思路正确,逻辑严谨,单元测试覆盖充分。三个重点问题中,Q1/Q2 基本可接受(有小瑕疵),Q3 存在真实漏洞single_stream_download 遇到连接静默截断时会把截断文件重命名为 dest,而下一次运行因 expected_size==0 直接信任,缺口永远不会被补上。


Q1:expected_size == 0 退回「存在即信任」是否合理,会不会漏掉真实截断?

结论:大体安全,但存在一个组合漏洞。

安全性来自 single_stream_download.partial 写法(download.rs:967):

let partial = PathBuf::from(dest).with_extension("partial");
// ... 写 partial ...
tokio::fs::rename(&partial, dest).await?;   // 原子 rename 才写 dest

若下载中途被中断,dest 根本不存在,只有 .partial 存在,所以 dest.exists()false,新的 existing_file_is_complete 分支根本不会触发,可以正常重下。这意味着「信任存在」只对曾经原子 rename 完成过的文件生效,而不是任意残留,这一点设计是对的。

但有一个组合漏洞(Q3 详述):如果 single_stream_download 第一次下载时连接静默截断(详见 Q3),会把截断文件 rename 到 dest。之后每次启动,expected_size == 0 → existing_file_is_complete = true,截断文件永久被跳过、永远不会被重下。这比单独看 Q1 严重。

另外一个可接受的小瑕疵:already_done_bytes 在完整性检查循环之前就计算了(download.rs:317),遇到被判定为截断并删除的文件,already_done_bytes 仍然计入了 f.size,导致进度显示多算。不影响功能,但进度条可能在下载开始时突然后退。


Q2:删除残留文件后能否正确回到下载路径,不与多块续传/sentinel 冲突?

结论:功能上正确,无冲突。

原因:

  1. .partial / .partial.idx 不受影响:新代码只 remove_file(&dest),不碰 .partial。而 dest 是由 finalize().partial rename 而来的,finalize 同时清理了 .idx。因此 dest 存在 → .partial 已不存在,download_one 进去后是全新下载,没有旧续传状态可冲突。

  2. READY_SENTINEL:sentinel 写在 run_download 最末尾,所有 worker 完成后才写(download.rs:471),中途删文件不涉及 sentinel 逻辑。

  3. 单块 ≤ 32MB 路径chunk_with_retry):走 append 模式,try_download_range_append 里先 set_len(effective_start) 截断再写,不依赖已有文件状态,重下安全。


Q3:单 GET 退化路径(total_size == 0)是否应校验收到字节数?

结论:存在真实漏洞,建议补上。

single_stream_downloaddownload.rs:960)的实现:

while let Some(chunk) = stream.next().await {
    let bytes = chunk?;          // 流错误会 bail
    file.write_all(&bytes).await?;
    total += bytes.len() as u64;
    on_progress(total);
}
file.flush().await.ok();
drop(file);
tokio::fs::rename(&partial, dest).await?;   // ← 无论写了多少字节都 rename!

reqwest 的 bytes_stream() 在 TCP 连接静默断开时会返回 None(非 Err),导致 while 循环正常结束,函数返回 Ok(()),截断文件被 rename 到 dest。这在多块路径的末尾(download.rs:704)有防御:

let actual = std::fs::metadata(&partial).map(|m| m.len()).unwrap_or(0);
if actual != total_size {
    anyhow::bail!("downloaded size {actual} != expected {total_size}");
}

single_stream_download 没有等价校验,且走这条路的文件 expected_size == 0,被 Q1 永久信任。两个缺口叠加,形成了一个静默损坏可以永久存活的场景。

建议的修复(可用 Content-Length 响应头做交叉验证):

async fn single_stream_download(...) -> Result<()> {
    let partial = PathBuf::from(dest).with_extension("partial");
    let resp = client.get(url).send().await?;
    let status = resp.status();
    if !status.is_success() {
        anyhow::bail!("HTTP {status} for {url}");
    }
    let content_length: Option<u64> = resp.content_length();  // 从 Content-Length 获取期望大小
    // ... 现有写入循环不变 ...
    file.flush().await.ok();
    drop(file);
    // 有 Content-Length 时做收尾校验,防止静默截断
    if let Some(expected) = content_length {
        let actual = std::fs::metadata(&partial).map(|m| m.len()).unwrap_or(0);
        if actual != expected {
            let _ = std::fs::remove_file(&partial);
            anyhow::bail!(
                "single-stream download truncated: got {actual} bytes, expected {expected}"
            );
        }
    }
    tokio::fs::rename(&partial, dest).await?;
    Ok(())
}

注意:若 HF 真的没给 Content-Lengthresp.content_length() 返回 None),这段代码退回原行为,不引入误拒。

建议修复 →


其他小问题

remove_file 的错误被静默丢弃download.rs:349):

let _ = std::fs::remove_file(&dest);

删除失败(权限不足、NFS、文件系统只读)时无日志,之后 download_one 会对已存在的 destset_len 尝试覆盖,行为不明确。建议改为:

if let Err(e) = std::fs::remove_file(&dest) {
    log::warn!("[download] 删除截断文件 {} 失败: {e:#},继续尝试覆盖", file.path);
}

总结

问题 严重度 结论
Q1: expected_size==0 信任已存在文件 低(独立看可接受) 与 Q3 组合后形成漏洞,见下
Q2: 删除后重下路径 无问题 功能正确,无冲突;进度多算是小瑕疵
Q3: single_stream_download 无字节数校验 (可能静默损坏永久化) 建议补 Content-Length 校验
remove_file 错误静默丢弃 建议加日志

建议合并前补上 Q3 的 Content-Length 校验,其余问题可在后续 PR 中处理。

@github-actions

github-actions Bot commented Jun 16, 2026

Copy link
Copy Markdown
Contributor

PR Reviewer Guide 🔍

(Review updated until commit 4023724)

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶

686 - Partially compliant

Compliant requirements:

  • 新增纯函数 existing_file_is_complete(actual_size, expected_size),行为符合规格
  • run_downloaddest.exists() 分支中使用该函数:完整跳过,否则删除重下
  • 添加了单元测试覆盖所有真值表情况
  • 同时修正了 already_done_bytes 的计算(只统计完整文件),避免进度条双倍计数

Non-compliant requirements:

(空)

Requires further human verification:

(空)

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 PR contains tests
🔒 No security concerns identified
⚡ No major issues detected

审核指出:大小不符的坏文件被删除重下时,already_done_bytes 已按 f.size 把它计入,
重下时 in_flight 又从 0 涨到 f.size,导致进度条短暂双计 / >100%(纯 UI,最终态正确)。
改为只把 existing_file_is_complete 为真的文件计入,与「完整才跳过、否则删重下」一致。
@github-actions

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 4023724

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.

[bug][local-asr] 模型下载「已存在即跳过」不校验大小,截断/损坏文件被当成完整

1 participant