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]

📊 Comprehensive Completeness Verification Table

#	Issue / Topic	Severity	Core Risk	Key Technical Details Required	Related GitHub Issues
1	OAuth Refresh Token Race Condition	High	Logout, batch sync failure	Concurrent refresh explanation, rotating refresh token invalidation, LimiterFileSystem concurrency, singleton promise (mutex), refreshPromise, finally reset, retry handling	#1046
2	Sync Re-entrancy (syncOnce overlap)	High	Data corruption, duplicate uploads	cloudSync alarm trigger, manual trigger, installScript trigger, isSyncing lock, finally unlock	#1226
3	Google Drive Stale Cache (External Deletion)	Medium	Sync crash on 404	pathToIdCache, external folder deletion scenario steps, 404 detection, cache clear, retry wrapper	#1226
4	OneDrive 0-Byte Upload Bug	Medium	Upload crash (invalid Content-Range)	parseInt(size)-1 → -1, bytes 0--1/0, guard logic, suggestion to use PUT for small files	#1226, #1046
5	Dropbox exists Swallows Auth Errors	Medium	False file absence, duplicate creation	Catch-all error issue, detect path/not_found, rethrow auth/network errors	-
6	Sync Digest Eventual Consistency Problem	Medium	Re-upload loops, duplication	Promise.allSettled, re-listing cloud, eventual consistency explanation, incremental digest update recommendation	#1226
7	OneDrive createDir Non-Recursive Limitation	Medium	Directory creation failure	Parent path slicing issue, A/B/C example, mkdirAll dependency, recursive recommendation	#1226
8	Rate Limiter 429 Detection Incomplete	Low	Retry failure under rate limit	includes("429") weakness, add "too many requests", lowercase compare	-
9	Baidu Extension Context Safety	Low	Crash outside extension	Guard chrome.declarativeNetRequest existence	-
10	Legacy Typo Compatibility (baidu-netdsik)	Low	Backward compatibility break	Warning not to rename due to stored configs	-

---

[cyfung1031]

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

[CodFrm]

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