fix: guard the workout upload path against concurrent runs#912
Open
Anexus5919 wants to merge 1 commit into
Open
fix: guard the workout upload path against concurrent runs#912Anexus5919 wants to merge 1 commit into
Anexus5919 wants to merge 1 commit into
Conversation
|
@Anexus5919 is attempting to deploy a commit to the somiljain2024-4175's projects Team on Vercel. A member of the Team first needs to authorize it. |
Contributor
Author
|
@Somil450 @diksha78dev Kindly have a review on this pr. Thanks! |
…docs syncInProgress only wrapped the online-event handler, so a sync reached through fullSyncWorkouts (called unguarded on login) could run at the same time as an online-triggered sync. Both read the same unsynced workouts before either marked them synced, and uploadWorkoutToFirestore uses addDoc with no idempotency, so each concurrent run created a separate Firestore document for the same workout. Move the in-flight guard into syncWorkoutsToFirestore, the single upload choke point, so every entry point shares it, and drop the now-redundant toggling in the online handler.
461c6f2 to
f4c53dc
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
📌 Related Issue
Fixes #875
📝 Description
syncInProgressonly wrapped the online-event handler, so a sync reached throughfullSyncWorkouts(called unguarded on login inuseWorkoutSync) could run concurrently with an online-triggered sync. Both callgetUnsyncedWorkoutsand read the same unsynced set before either marks them synced, anduploadWorkoutToFirestoreusesaddDoc(no idempotency), so each concurrent run creates a separate Firestore document for the same workout.🔹 What has been changed?
src/services/workoutSyncService.ts: moved the in-flightsyncInProgressguard intosyncWorkoutsToFirestore(the single upload choke point), so every entry point (loginfullSyncWorkouts, the online handler, manual sync) shares it. Removed the now-redundant guard toggling from the online handler.🔹 Why are these changes needed?
await, so two concurrent callers can no longer both pass it and double-upload. This prevents duplicate workout documents in Firestore.🛠️ Type of Change
🧪 Testing
✅ Tests Performed
npx tsc --noEmit: changed file clean;npx eslint: 0 problems.if (syncInProgress) return/syncInProgress = truepair runs before anyawait, so concurrent callers are serialized to one upload run.🔹 Note on automated tests
No committed unit test: this path needs both IndexedDB and a Firestore mock, and the repo has no
fake-indexeddbharness. Verification is bytsc/eslintand the reasoning above.🌐 Browsers Tested
Not applicable (sync logic).
📷 Screenshots / Demo (if applicable)
Not applicable.
📋 Checklist
💬 Additional Notes
Branched from latest
upstream/main(5464425). A deeper follow-up (out of scope here) would give each workout a deterministic Firestore document id (e.g.setDockeyed bycrdtSessionId) so even a retry after a lost write acknowledgement cannot duplicate.