Retry PUT 409 errors - delete and re-create event #964
Conversation
vdirsyncer/storage/dav.py
Outdated
| try: | ||
| href, etag = await self._put(self._normalize_href(href), item, etag) | ||
| except aiohttp.ClientResponseError as e: | ||
| if e.status == 409: | ||
| dav_logger.debug("Conflict, will delete old event and recreate it.") | ||
| await self.delete(self._normalize_href(href), None) | ||
| dav_logger.debug("Now trying again") | ||
| href, etag = await self._put(self._normalize_href(href), item, None) | ||
| else: | ||
| raise e |
There was a problem hiding this comment.
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.
In my case, I have the following ~/.config/vdirsyncer/config:
[general]
status_path = "~/.cache/vdirsyncer/status/"
[storage caldav]
type = "caldav"
url = # redacted
username = # redacted
password = # redacted
read_only = true
item_types = ["VEVENT"]
[storage gcal]
type = "google_calendar"
token_file = "~/.cache/vdirsyncer/gcal_robert_work.token"
client_id = # redacted
client_secret = # redacted
item_types = ["VEVENT"]
[pair caldav_to_gcal]
a = "caldav"
b = "gcal"
# Robert work (new)
collections = [["caldav_to_gcal", "a_calendar_id", "[email protected]"]]
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 to a 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 = await self._put(href, item, None) | ||
| except aiohttp.ClientResponseError as e: | ||
| if e.status == 409: | ||
| dav_logger.debug("Conflict, will delete old event and recreate it.") | ||
| await self.delete(href, None) | ||
| dav_logger.debug("Now trying again") | ||
| rv = await self._put(href, item, None) | ||
| else: | ||
| raise e |
| @@ -547,7 +547,7 @@ async def get_multi(self, hrefs): | |||
| else: | |||
| rv.append((href, Item(raw), etag)) | |||
| for href in hrefs_left: | |||
| raise exceptions.NotFoundError(href) | |||
There was a problem hiding this comment.
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 using get_multi.
|
@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. |
|
A few tests are stil failing: https://builds.sr.ht/~whynothugo/job/776820#task-test-134 |
telotortium
force-pushed
the
patch-1
branch
from
e94ac1f to
7a625da
Compare
This comment was marked as off-topic.
This comment was marked as off-topic.
Attempt to address pimutils#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.
Rather, just log them. This allows my calendar sync to continue to work even if some of the hrefs can't be retrieved. In my case, one of the hrefs on my calendar was always returning a 404, so I want to skip it.
telotortium
force-pushed
the
patch-1
branch
3 times, most recently
from
2b0af6b to
3017744
Compare
for more information, see https://pre-commit.ci
|
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 it has 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. |
|
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. |
|
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.