- Notifications
You must be signed in to change notification settings - Fork177
Retry PUT 409 errors - delete and re-create event#964
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
vdirsyncer/storage/dav.py Outdated
| try: | ||
| href,etag=awaitself._put(self._normalize_href(href),item,etag) | ||
| exceptaiohttp.ClientResponseErrorase: | ||
| ife.status==409: | ||
| dav_logger.debug("Conflict, will delete old event and recreate it.") | ||
| awaitself.delete(self._normalize_href(href),None) | ||
| dav_logger.debug("Now trying again") | ||
| href,etag=awaitself._put(self._normalize_href(href),item,None) | ||
| else: | ||
| raisee |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This is not correct, 409 indicates that the server's copy of the item has changed since we last read it. Deleting-and-then-updating is essentially overwriting the conflicting copy, effectively resulting in data loss.
Maybe this is happening due to race conditions with concurrent edits? In such cases, the correct thing to do is run the sync again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
In my case, I have the following~/.config/vdirsyncer/config:
[general]status_path = "~/.cache/vdirsyncer/status/"[storage caldav]type = "caldav"url = # redactedusername = # redactedpassword = # redactedread_only = trueitem_types = ["VEVENT"][storage gcal]type = "google_calendar"token_file = "~/.cache/vdirsyncer/gcal_robert_work.token"client_id = # redactedclient_secret = # redacteditem_types = ["VEVENT"][pair caldav_to_gcal]a = "caldav"b = "gcal"# Robert work (new)collections = [["caldav_to_gcal", "a_calendar_id", "b_calendar_id@group.calendar.google.com"]]conflict_resolution = "a wins"partial_sync = "ignore"So in my case, since my caldav server is set to read-only and I have conflict resolution set toa wins, I actually don't care about the old event, and I'm completely fine with deleting it.
vdirsyncer/storage/dav.py Outdated
| try: | ||
| rv=awaitself._put(href,item,None) | ||
| exceptaiohttp.ClientResponseErrorase: | ||
| ife.status==409: | ||
| dav_logger.debug("Conflict, will delete old event and recreate it.") | ||
| awaitself.delete(href,None) | ||
| dav_logger.debug("Now trying again") | ||
| rv=awaitself._put(href,item,None) | ||
| else: | ||
| raisee |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Same comment as above.
| else: | ||
| rv.append((href,Item(raw),etag)) | ||
| forhrefinhrefs_left: | ||
| raiseexceptions.NotFoundError(href) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
get_multi receives a set of ids and currently returns items with those ids. In this case, one of them is missing -- but callers of this function will expect it to be returned.
I understand the rationale here, but I've an impression that this change will break things for other places usingget_multi.
telotortium commentedMar 30, 2022
@WhyNotHugo Thanks for your comments. I agree that I need to figure out how to make these changes more correct before merging, but I don't understand CalDAV or the code well enough to know what I need to do. These hacks are just what I need to get the sync in my configuration working. |
WhyNotHugo commentedJun 9, 2022
A few tests are stil failing:https://builds.sr.ht/~whynothugo/job/776820#task-test-134 |
This comment was marked as off-topic.
This comment was marked as off-topic.
Attempt to addresspimutils#963I would appreciate a code review here - there might be a more elegant way to address this problem, but it works for my (very limited) use case.
Rather, just log them. This allows my calendar sync to continue to workeven if some of the hrefs can't be retrieved. In my case, one of thehrefs on my calendar was always returning a 404, so I want to skip it.
2b0af6b to3017744Comparefor more information, seehttps://pre-commit.ci
WhyNotHugo commentedMay 25, 2023
Basically each time that vdirsyncer reads an event, we also get an A 409 error means that we tried a write operation on an event assuming it had not changed, but ithas changed. If this keeps happening over and over, it likely means that there is an issue elsewhere in the logic. But in the general case, ignoring a 409 error means that you're overwriting or deleting files which have been modified by someone else, and will result in a data loss. I honestly haven't looked deeply into the cause of this bug because it sounds very non-trivial, and I'd rather focus energy on the rewrite which should either not have this bug, or be a lot easier to debug. |
marc1uk commentedJun 21, 2024
I'm also having this issue. As with@telotortium, I also have vdirsyncer configured for 'remote wins', and I don't understand why this is not being handled by that mechanism.
This Etag check seems redundant with an A/B wins configuration - it doesn't matter whether the event has been modified since last read, it's about to be overwritten anyway. Yes, this results in data loss, but that's been specifically requested by the user config. This may be a complex issue to resolve for a more general merge situation, but for the time being this completely breaks synchronization.Some workaround is needed to get the system back on its feet - even if that means data loss. |
marc1uk commentedJun 26, 2024
i added some comments on reproducing this issue over in#963 |
Attempt to address#963
I would appreciate a code review here - there might be a more elegant way to address this problem, but it works for my (very limited) use case.