Sync-related Coding Issue

[cyfung1031]

Comprehensive Code Review: Stability, Concurrency, Edge Cases, and Fixes

Based on a comprehensive review of all provided files, here is a detailed analysis of potential stability issues, race conditions, edge cases, external modifications (e.g., users deleting files via the cloud provider's web interface), along with specific code fixes. This report combines insights from multiple analyses to provide a unified view without duplication or loss of details.

1. Critical: OAuth Refresh Token Race Condition

Files: auth.ts
Severity: High (Causes logout/sync failure)
Scenario: When multiple files are uploading simultaneously (e.g., during syncOnce or mkdirAll), if the access token is expired, every concurrent request will attempt to refresh the token. For providers with rotating refresh tokens (like OneDrive/Google), the first request invalidates the old refresh token. Subsequent requests send the now-invalid refresh token and fail, causing the entire sync session to crash and potentially logging the user out. This is exacerbated in concurrent file operations via LimiterFileSystem (e.g., uploading 5 files at once).

Fix: Implement a "singleton promise" (Mutex) pattern for the refresh operation so concurrent requests wait for the first refresh to complete.

// auth.ts

let refreshPromise: Promise<string> | null = null; // Add global/module-level variable

export async function AuthVerify(netDiskType: NetDiskType, invalid?: boolean) {
  // ... existing setup ...
  
  // Logic for expiration check
  if (Date.now() >= token.createtime + 3600000 || invalid) {
    if (!refreshPromise) {
      refreshPromise = (async () => {
        try {
            const resp = await RefreshToken(netDiskType, token!.refreshToken); // Use non-null assertion or safe access
            if (resp.code !== 0) {
                await localStorageDAO.delete(key);
                if (invalid) {
                    // Logic to retry or throw
                    throw new WarpTokenError(new Error(resp.msg)); 
                }
                throw new WarpTokenError(new Error(resp.msg));
            }
            const newToken = {
                accessToken: resp.data.token.access_token,
                refreshToken: resp.data.token.refresh_token,
                createtime: Date.now(),
            };
            await localStorageDAO.saveValue(key, newToken);
            return newToken.accessToken;
        } finally {
            refreshPromise = null; // Clear promise when done
        }
      })();
    }
    
    try {
        return await refreshPromise;
    } catch (e) {
        // Handle failure, clear token, etc.
        if(invalid) return await AuthVerify(netDiskType);
        throw e;
    }
  }
  // ...
}

2. Major: Sync Re-entrancy (Overlap)

Files: synchronize.ts
Severity: High (Data corruption/Duplication)
Scenario: syncOnce is an async operation that can take time. It can be triggered by:

1. The cloudSync alarm (timer).
2. User manually clicking "Sync".
3. Script installation events (installScript).
   If these events happen close together, two syncOnce processes will run in parallel, fighting over the file system and state, leading to upload conflicts or data corruption.

Fix: Add a locking mechanism to SynchronizeService.

// synchronize.ts

export class SynchronizeService {
  private isSyncing = false; // Add state flag

  // ...

  async syncOnce(syncConfig: CloudSyncConfig, fs: FileSystem) {
    if (this.isSyncing) {
      this.logger.warn("Sync already in progress, skipping.");
      return;
    }
    this.isSyncing = true;

    try {
      this.logger.info("start sync once");
      // ... existing sync logic ...
      await this.updateFileDigest(fs);
      this.logger.info("sync complete");
    } finally {
      this.isSyncing = false; // Always release lock
    }
  }
}

3. Major: Google Drive Stale Cache (External Deletion)

Files: googledrive.ts, googledrive_rw.ts
Severity: Medium (Sync fails if user modifies cloud files)
Scenario: The class maintains a persistent pathToIdCache. If a user deletes a folder via the Google Drive website, ScriptCat retains the old ID. When ensureDirExists is called, it returns the old ID. The subsequent upload attempt fails with a 404/400 error, crashing the sync. For example:

1. ScriptCat tries to upload to FolderA (ID: 123).
2. ensureDirExists finds FolderA -> 123 in cache.
3. Upload attempts to use parent ID 123.
4. Google returns 404 Not Found.
5. Code throws error immediately, failing the sync.

Fix: Invalidate cache on 404 errors and retry automatically upon receiving a 404/400 regarding "Parent not found".

// googledrive.ts

// 1. Modify request() to handle 404 specifically
async request(url: string, config?: RequestInit, nothen?: boolean) {
    // ... setup ...
    return fetch(url, config)
      .then((data) => data.json())
      .then(async (data) => {
        if (data.error) {
          // If item not found, cache is likely invalid
          if (data.error.code === 404) {
             this.pathToIdCache.clear(); // Brutal but safe. Or implement targeted clearing if context available
          }
          // ... 401 handling ...
          throw new Error(JSON.stringify(data));
        }
        return data;
      });
}

// 2. Wrap ensureDirExists in googledrive_rw.ts or googledrive.ts to retry
// In googledrive.ts:
async ensureDirExists(dirPath: string): Promise<string> {
    try {
        return await this._ensureDirExistsInternal(dirPath); // Rename original logic to internal
    } catch (e) {
        // If error indicates not found, retry once (cache was cleared by request method)
        if (JSON.stringify(e).includes("404")) {
            return await this._ensureDirExistsInternal(dirPath);
        }
        throw e;
    }
}

4. Edge Case: OneDrive 0-Byte File Upload

Files: onedrive_rw.ts
Severity: Medium (Upload crash)
Scenario: If a user creates an empty script or resource (0 bytes), the Content-Range header calculation fails.
Code: parseInt(size, 10) - 1 becomes -1.
Result: Content-Range: bytes 0--1/0. OneDrive API will reject this.

Fix: Handle 0-byte files or skip them. Use simple PUT for small files (<4MB) and Upload Session only for large files. This avoids the complexity of range headers for tiny/empty files.

// onedrive_rw.ts

async write(content: string | Blob): Promise<void> {
    const sizeStr = this.size(content).toString();
    const size = parseInt(sizeStr, 10);
    
    // OneDrive upload session requires content. 
    // If empty, just create an empty file via PUT directly if possible, 
    // or strictly handle the range logic.
    if (size === 0) {
        // Simple PUT for empty file might be needed instead of UploadSession
        // Or ensure range is handled if API supports "bytes */0" (unlikely for upload)
        // Safest: Write a single space or handle via simple PUT if API allows.
        // For now, let's just guard the calculation:
         // Implementation dependent: You might need to use simple PUT for small files
         // instead of createUploadSession for <4MB files.
    }

    // ... existing uploadUrl logic ...

    myHeaders.append("Content-Range", `bytes 0-${Math.max(0, size - 1)}/${size}`);
    // ...
}

5. Major: Dropbox exists Swallows Auth Errors

Files: dropbox.ts
Severity: Medium (Logic error)
Scenario: The exists method catches all errors and returns false.
try { ... } catch (_) { return false; }
If the token is expired or network is down, request throws an error. exists catches it, returns false. The calling code thinks the file is missing and tries to create it, likely failing again or creating duplicates.

Fix: Only return false if the error specifically indicates "path not found". Rethrow network/auth errors.

// dropbox.ts
async exists(path: string): Promise<boolean> {
    try {
      // ... request ...
      return true;
    } catch (e: any) {
      const errStr = String(e);
      // Check for specific Dropbox "not found" error
      if (errStr.includes("path/not_found")) {
        return false;
      }
      throw e; // Rethrow network/auth errors
    }
}

6. Minor: Baidu Extension Context Safety

Files: baidu.ts
Severity: Low (Potential crash in non-extension contexts)
Scenario: The code uses chrome.declarativeNetRequest directly. If this code is ever run in a non-extension context (e.g., unit tests, Node.js, React Native), it will crash.

Fix: Guard the call.

if (typeof chrome !== "undefined" && chrome.declarativeNetRequest) {
    await chrome.declarativeNetRequest.updateDynamicRules(...);
}

7. Logic: Sync Digest Consistency

Files: synchronize.ts
Severity: Medium (Potential data duplication)
Scenario:

1. pushScript uploads a file.
2. Promise.allSettled waits for it.
3. updateFileDigest lists files from the cloud to update the local digest cache.
   Issue: Cloud storage (especially S3/Google Drive) can be eventually consistent. Listing files immediately after upload might return the old list (missing the new file). If updateFileDigest misses the file, the local digest map won't be updated. On the next sync, ScriptCat will think the file is missing or changed and try to upload it again.

Recommendation:
Instead of re-listing the entire file system in updateFileDigest, update the digest map incrementally in memory when pushScript succeeds.

// synchronize.ts

// 1. Pass the map to pushScript/pullScript or return the result
async pushScript(fs: FileSystem, script: PushScriptParam): Promise<{name: string, digest: string} | null> {
    // ...
    // After successful write:
    // You assume the write succeeded. 
    // ideally, return the new digest so the caller can update the map.
}

// 2. In syncOnce, update map based on results, THEN save to storage.
// This avoids the network round-trip and consistency issues of list().

8. Minor: Rate Limiter 429 Detection

Files: limiter.ts
Severity: Low (Incomplete error handling)
Issue: The retry logic checks errorStr.includes("429"). Some APIs return "Too Many Requests" in the body without the number "429" appearing in the generic Error string representation, or the fetch API might return a 429 status without throwing an error (depending on how the specific FileSystem handles fetch responses).

• googledrive.ts: Throws JSON string. Good.
• onedrive.ts: Throws resp.text(). Good.
• dropbox.ts: Throws status - text. Good.

Refinement: Expand the check to be safer, as some error messages spell it out.

// limiter.ts
const errorStr = String(error).toLowerCase();
if ((errorStr.includes("429") || errorStr.includes("too many requests")) && i < 10) {
    // ...
}

9. Logic: OneDrive createDir Recursion

Files: onedrive.ts
Severity: Medium (Potential failure in recursive directory creation)
Issue: The createDir implementation calculates parent by slicing the path array, but it only creates the last segment.

const data = await this.request(..., { body: JSON.stringify({ name: dirs[dirs.length - 1] ... }) });

If dirs is A/B/C and A/B does not exist, OneDrive will likely return an error (PathNotFound) because you are trying to create C inside .../A/B/children. While FileSystemFactory.mkdirAll handles recursive creation loop, calling fs.createDir("A/B") directly will fail, violating the implicit contract of a filesystem mkdir -p behavior if expected.

Recommendation: Since mkdirAll exists in Factory, ensure all calls verify parent existence or use mkdirAll. If strict mkdir -p (recursive) within the driver is required, createDir needs a loop similar to googledrive.ts.

10. Typo Compatibility

Files: auth.ts / factory.ts
Severity: Low (Compatibility issue)
Observation: Legacy typo baidu-netdsik (disk spelled dsik) is present.
Action: Do not fix this if active users have this string in their localStorage/config, as it will break their login. Ensure any new UI/Config code preserves this typo mapping.

[cyfung1031]

@CodFrm 用Grok花了6分钟帮你把 ScriptCat 现存所有 Open Issues 看了一次，抽了出来分门别类。

[CodFrm]

啊？这不是只是同步相关的么，而且就两个issue来回标记

[cyfung1031]

@CodFrm 还有这个

• [BUG] ScriptCat cloud sync fails on Cromite for Android #1046

[cyfung1031]

CodFrm 一直没提交修正 Sync 的代码。我用AI找了出来

唯有一步步提交PR吧

---

一、致命问题（优先级最高）

1. 重试机制错误：LimiterFileSystem 盲目重放所有 429 | PR #1376

文件：limiter.ts

问题

• 所有包含 429 的错误都会自动重试
• 不区分读/写操作（包括 write/create/delete）

影响

• 写操作可能被重复执行：

  • 已成功写入但响应丢失 → 重试导致覆盖/冲突
  • 删除操作被重放 → 状态混乱

• 在“网络抖动 + 限流”下会放大为数据错误或同步错乱

建议

• 仅自动重试幂等读操作：verify / list / read

• 写操作仅在“确认未落地”时重试

• 或引入：

  • 幂等键（idempotency key）
  • 条件请求（如 ETag / version）
  • provider-specific 重试策略

---

2. 多 provider 缺少“条件写 / 版本校验”

（结构性问题）

问题

• 多设备并发修改时直接覆盖（last write wins）
• 无版本控制 / 无冲突检测

影响

• 多端同步下：

  • 数据被静默覆盖
  • 回滚/误删难以恢复

• 属于数据正确性层面的致命缺陷

建议

• 引入至少一种机制：

  • ETag / revision 校验
  • If-Match / If-None-Match
  • 自定义版本号

• 或在上层实现冲突检测策略

---

3. 百度盘请求层：全局 DNR 规则竞态 + cookie 处理错误 | PR #1377

文件：baidu.ts

问题

• 使用固定 DNR rule id 100（全局共享）
• 每个请求动态 add/remove → 并发冲突
• 实际修改的是 responseHeaders，不是 requestHeaders
  → cookie 并未真正移除

影响

• 并发请求互相干扰：

  • A 请求未完成，B 已删除规则

• 行为不稳定：

  • 有时带 cookie，有时不带

• 导致：

  • 偶发认证失败
  • 难复现同步错误

建议

• 改为修改 requestHeaders 去除 Cookie

• 禁止使用全局 rule id

• 更优方案：

  • 使用 fetch(credentials: "omit")
  • 或在请求层统一处理，不依赖 DNR

---

二、高优先级问题

4. OneDrive request() 返回错误数据（确定性 bug）| PR #1375 & #1406

文件：onedrive.ts

问题

• token 过期后：

  • 刷新 token
  • 重发请求（成功）

• 但最终返回的是第一次失败结果 data

影响

• 实际请求已成功

• 上层仍认为失败

• 导致：

  • “授权已恢复但业务仍报错”

建议

• 成功重试后返回 retryData
• 同时统一检查 response.ok

---

5. AuthVerify() 返回过期 token（错误回退策略） | PR #1375

文件：auth.ts

问题

• token 已过期 + refresh 失败时：

  • 返回旧 token（当作成功）

影响

• 调用方继续使用无效 token
• 后续请求持续 401/403
• 掩盖真实问题（网络 or 认证）

建议

• token 过期 + refresh 失败 → 直接抛错

• 仅在 token 未过期时允许 fallback

• 区分：

  • 认证失败
  • 网络失败

---

6. 各 provider delete() 非幂等 | PR #1375

涉及：

• Google Drive / Dropbox / OneDrive / WebDAV / Baidu
  对照：S3（正确）

问题

• 删除不存在对象时直接报错

影响

• 多设备同步场景：

  • A 删除成功
  • B 再删 → 报错

• 上层误判为失败

建议
统一视为成功：

• WebDAV 404
• Google Drive not found
• Dropbox path_lookup/not_found
• OneDrive itemNotFound
• Baidu 对应 errno

→ 删除操作应默认幂等（与 S3 对齐）

---

7. 请求层错误处理不统一（Google Drive / OneDrive）| PR #1406

文件：googledrive.ts, onedrive.ts

问题

• 直接 response.json()，未检查 response.ok

影响

• 非 JSON 响应（HTML / 空 body）：

  • 抛 JSON parse error
  • 丢失真实 HTTP 信息

建议
统一模式：

1. 先检查 response.ok

2. 非 2xx：

   • 保留 status + body

3. 确认 JSON 再解析

---

8. WebDAV verify() 过弱 | PR #1395

问题

• 验证通过 ≠ 实际可写

影响

• 初始化成功
• 写入阶段才失败（晚失败）

建议

• verify 阶段增加：

  • 写权限测试
  • 目录访问验证

---

三、中等问题

9. Google Drive ensureDirExists() 语义不一致 | PR #1391

文件：googledrive.ts

问题

• 名字表示“确保存在”
• 实际只查找，不创建

影响

• 容易误用
• 后续维护风险高

建议

• 要么实现创建逻辑

• 要么改名：

  • getExistingDirIdOrThrow

---

10. S3 metadata 写入未使用 | PR #1408

文件：s3/rw.ts, s3.ts

问题

• 写入 x-amz-meta-createtime
• list 时未读取

影响

• 时间语义不完整
• 后续可能影响同步逻辑

建议

• 要么读取并使用
• 要么删除该字段

---

最初的 1-10

1. OAuth refresh token race | PR #1392
未修。AuthVerify() 仍会在过期时直接各自呼叫 RefreshToken(...)，没有 singleton promise / mutex。

2. Sync re-entrancy | PR #1374
已修，而且做法比单纯 isSyncing 更好。syncOnce()、安装后推送、删除后同步都统一走 stackAsyncTask("cloud_sync", ...)，同 key 任务会串行执行。

3. Google Drive stale cache / external deletion | PR #1407
未修。pathToIdCache 还在用，但 request() 没有在 404 时清 cache，ensureDirExists() 也只读 cache / getFileId()，没有自动 retry 重建目录。googledrive.ts

4. OneDrive 0-byte upload | PR #1405
未修。现在仍固定走 createUploadSession，而且 Content-Range 直接用 size - 1，0 bytes 会变成 0--1/0。rw.ts

5. Dropbox exists swallows auth errors | PR #1394
未修。exists() 还是 catch (_) { return false; }，会把 auth / network error 误判成不存在。dropbox.ts

6. Baidu extension context safety | PR #1377
这条目前算已不适用。现行 baidu.ts 里没有直接用 chrome.declarativeNetRequest，改成对请求显式 credentials = "omit"，避免依赖 DNR 规则。baidu.ts

7. Sync digest consistency / eventual consistency | PR #1393
未修。同步完成后仍是重新 fs.list() 生成 file_digest，不是在 pushScript() 成功后增量更新。synchronize.ts

8. Rate limiter 429 detection | PR #1376
已修。

9. OneDrive createDir recursion | PR #1390
部分修正，但 driver 本身没修。OneDriveFileSystem.createDir() 仍只建立最后一层；只是目前 buildFileSystem() 透过 FileSystemFactory.mkdirAll() 逐层建立 ScriptCat/sync，所以主要路径被绕过了。直接呼叫巢状 createDir("A/B") 风险仍在。onedrive.ts factory.ts

10. baidu-netdsik typo compatibility
不用修改

[cyfung1031]

@CodFrm https://bbs.tampermonkey.net.cn/thread-10167-1-1.html

[CodFrm]

不过同步没有一个日志显示 也确实不知道干了啥

[cyfung1031]

这次 PR 处理得这么快？

不过同步没有一个日志显示 也确实不知道干了啥

对呀。不论尝试（开始），成功，失败，都应该记录一下吧

[CodFrm]

主要都是fix，只用AI确定了影响面，和修复，就通过了

可以后续版本做一下这个优化

[cyfung1031]

已合并的 PR

顺序	PR 主题	对应未解决问题	对应 issue / 编号
1	Serialize cloud sync re-entrancy with stackAsyncTask	sync re-entrancy：syncOnce()、安装后推送、删除后同步应统一串行化，避免同步任务重入	#1374
2	AuthVerify expired-token fallback fix	AuthVerify() 在 token 过期且 refresh 失败时不应返回旧 token；应直接抛出认证 / 网络错误，避免调用方继续使用无效 token	#1375
3	Make provider delete operations idempotent	Google Drive / Dropbox / OneDrive / WebDAV / Baidu 删除不存在对象时应视为成功，与 S3 行为对齐，避免多端重复删除被误判失败	#1375
4	LimiterFileSystem retry policy: only auto-retry idempotent reads	LimiterFileSystem 不应盲目重放所有 429；写入 / 创建 / 删除操作自动 retry 可能造成重复写入、覆盖、状态错乱	#1376
5	Baidu request layer: remove DNR dependency and omit credentials	百度盘请求层不应使用全局 DNR rule id 动态 add/remove；cookie 处理应在请求层解决，避免并发竞态和认证不稳定	#1377
6	OneDrive recursive createDir support	OneDrive createDir("A/B") 不应只建立最后一层；需要逐层创建嵌套目录，避免直接调用嵌套路径失败	#1390
7	Clarify or fix Google Drive ensureDirExists semantics	ensureDirExists() 名称表示“确保存在”，但实际只查找不创建；容易误用，应改名或实现创建逻辑	#1391
8	OAuth refresh token race guard	多个请求在 token 过期时会同时调用 RefreshToken(...)；应使用 singleton promise / mutex 防止 refresh race	#1392
9	Sync digest consistency after push	同步完成后不应完全依赖重新 fs.list() 生成 file_digest；刚 push 成功的文件在 eventual consistency 下可能暂时 list 不到	#1393
10	Dropbox exists should not swallow auth/network errors	Dropbox exists() 不应 catch (_) { return false; }，否则会把 auth / network error 误判成路径不存在	#1394
11	Strengthen WebDAV verify with write/access checks	WebDAV verify() 过弱：验证通过不代表实际可写，初始化阶段应检查目录访问与写权限，避免写入阶段才失败	#1395
12	Keep legacy baidu-netdsik typo compatibility	历史 typo baidu-netdsik 需要保留兼容，不应修改成 breaking change	不修改

剩下的 PR内容

顺序	PR 主题	对应未解决问题	对应 issue / 编号
1	OneDrive zero-byte upload fix	OneDrive 0-byte upload：Content-Range 变成 bytes 0--1/0	#1405
2	Typed filesystem error base for Google Drive and OneDrive	请求层错误处理不统一；OneDrive request() refresh 后回传第一次失败结果；Google Drive / OneDrive 未统一检查 response.ok	#1406
3	Google Drive stale cache retry-on-404	Google Drive stale cache / external deletion：pathToIdCache 快取失效后未清除、未 retry	#1406 > #1407
4	S3 metadata usage in cloud sync	S3 metadata 写入未使用：cloud sync 未传 modifiedDate，list 也未读 x-amz-meta-createtime	#1408
5	Extend FileCreateOptions / FileInfo for conditional write metadata	多 provider 缺少条件写 / 版本校验的 API 基础；FileCreateOptions 只有 modifiedDate，FileInfo 缺 version/rev/etag	未提交
6	Provider conditional-write implementations	各 provider 实作条件写：GDrive If-Match、OneDrive eTag、Dropbox rev、S3/WebDAV ETag、Baidu unsupported/best-effort	#1406 > 未提交
7	Wire conditional writes into SynchronizeService.syncOnce()	sync 写入时带上 remote digest/version，避免多端并发时静默覆盖	#1406 > 未提交

5, 6, 7 是用来解决同一个问题。
或者会整个成一个大PR
先处理 #1405 ~ #1408 吧

@CodFrm