Sync-related Coding Issue #1237
cyfung1031
started this conversation in
General
Replies: 1 comment 2 replies
-
📊 Comprehensive Completeness Verification Table
|
Beta Was this translation helpful? Give feedback.
2 replies
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
-
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.tsSeverity: High (Causes logout/sync failure)
Scenario: When multiple files are uploading simultaneously (e.g., during
syncOnceormkdirAll), 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 viaLimiterFileSystem(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.
2. Major: Sync Re-entrancy (Overlap)
Files:
synchronize.tsSeverity: High (Data corruption/Duplication)
Scenario:
syncOnceis an async operation that can take time. It can be triggered by:cloudSyncalarm (timer).installScript).If these events happen close together, two
syncOnceprocesses 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.3. Major: Google Drive Stale Cache (External Deletion)
Files:
googledrive.ts,googledrive_rw.tsSeverity: 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. WhenensureDirExistsis called, it returns the old ID. The subsequent upload attempt fails with a 404/400 error, crashing the sync. For example:FolderA(ID:123).ensureDirExistsfindsFolderA->123in cache.123.404 Not Found.Fix: Invalidate cache on 404 errors and retry automatically upon receiving a 404/400 regarding "Parent not found".
4. Edge Case: OneDrive 0-Byte File Upload
Files:
onedrive_rw.tsSeverity: Medium (Upload crash)
Scenario: If a user creates an empty script or resource (0 bytes), the
Content-Rangeheader calculation fails.Code:
parseInt(size, 10) - 1becomes-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.
5. Major: Dropbox
existsSwallows Auth ErrorsFiles:
dropbox.tsSeverity: Medium (Logic error)
Scenario: The
existsmethod catches all errors and returnsfalse.try { ... } catch (_) { return false; }If the token is expired or network is down,
requestthrows an error.existscatches it, returnsfalse. The calling code thinks the file is missing and tries to create it, likely failing again or creating duplicates.Fix: Only return
falseif the error specifically indicates "path not found". Rethrow network/auth errors.6. Minor: Baidu Extension Context Safety
Files:
baidu.tsSeverity: Low (Potential crash in non-extension contexts)
Scenario: The code uses
chrome.declarativeNetRequestdirectly. 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.
7. Logic: Sync Digest Consistency
Files:
synchronize.tsSeverity: Medium (Potential data duplication)
Scenario:
pushScriptuploads a file.Promise.allSettledwaits for it.updateFileDigestlists 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
updateFileDigestmisses 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 whenpushScriptsucceeds.8. Minor: Rate Limiter 429 Detection
Files:
limiter.tsSeverity: 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 thefetchAPI might return a 429 status without throwing an error (depending on how the specificFileSystemhandlesfetchresponses).googledrive.ts: Throws JSON string. Good.onedrive.ts: Throwsresp.text(). Good.dropbox.ts: Throwsstatus - text. Good.Refinement: Expand the check to be safer, as some error messages spell it out.
9. Logic: OneDrive
createDirRecursionFiles:
onedrive.tsSeverity: Medium (Potential failure in recursive directory creation)
Issue: The
createDirimplementation calculatesparentby slicing the path array, but it only creates the last segment.If
dirsisA/B/CandA/Bdoes not exist, OneDrive will likely return an error (PathNotFound) because you are trying to createCinside.../A/B/children. WhileFileSystemFactory.mkdirAllhandles recursive creation loop, callingfs.createDir("A/B")directly will fail, violating the implicit contract of a filesystemmkdir -pbehavior if expected.Recommendation: Since
mkdirAllexists inFactory, ensure all calls verify parent existence or usemkdirAll. If strictmkdir -p(recursive) within the driver is required,createDirneeds a loop similar togoogledrive.ts.10. Typo Compatibility
Files:
auth.ts/factory.tsSeverity: 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.
Beta Was this translation helpful? Give feedback.
All reactions