Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix edge case file upload bugs #2527

Closed
wants to merge 2 commits into from

Conversation

JC-comp
Copy link

@JC-comp JC-comp commented Oct 23, 2023

Changes

  • Wiat for download/upload threads to finish before closing databse: prevent invalid database access in threads causing segmentation fault
  • Upload the file even if it already exists online: prevent skipping changes in local first mode
  • Delete remote destination first for a local move if local destination not exists
  • Add support for shared logger

@abraunegg
Copy link
Owner

abraunegg commented Oct 24, 2023

@JC-comp
Could you please remove this commit: 7101210

Logging output is 100% broken at the moment for many things and is going to be 100% totally redone so it is better that this is not something you look at or add a commit for.

@abraunegg
Copy link
Owner

@JC-comp
Commit 0357f43 is now causing a conflict, please also pull this change from this PR

@JC-comp
Copy link
Author

JC-comp commented Oct 25, 2023

Not-yet-ready and patched commits have been removed

Copy link
Owner

@abraunegg abraunegg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JC-comp
Specifically for this commit (9426134), do you have an actual example of how and why this code is needed?

The whole point of the actual code, as is:

  • The file, we thought that was needed to be uploaded, already exists online for some reason
  • The hash + data size locally matches the online data

Why would the online file need to be replaced, when everything matches? The information only needs to be stored, thus negates a meaningless transfer.

Copy link
Owner

@abraunegg abraunegg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JC-comp
Specifically for this commit (38bc4a3), , do you have an actual example of hitting a 409 response when attempting a move - that required this code change?

I understand the edge use-case here, however, if something is already in the target location online, it most likely is already in the same location locally, so if you are moving the file|folder locally to an already existing location, the data is either going to be merged locally or replaced locally, thus is handled in the correct manner already to replace the content online.

@abraunegg
Copy link
Owner

@JC-comp
Additionally to the 2 comments above, as these 2 commits are 100% non-related, these should be separate PR items if they have the information to support their inclusion.

@JC-comp
Copy link
Author

JC-comp commented Jan 9, 2024

Both commits address issues related to cases where remote files are created at location A but are not reflected locally (either possible in local-first or monitor mode). Subsequently, uploading or moving files locally at/to location A can lead to inconsistencies (upload are not being handled and moves causing 409 conflicts).

commit 9426134 resolves upload problems when Integrity validation checks fail or are disabled.
commit 38bc4a3 resolves the issue of moving to a remotely created location by replacing it.

@abraunegg abraunegg changed the title Fix exit clean up and upload bugs Fix edge case file upload bugs Jan 13, 2024

// Save item to the database
saveItem(fileDetailsFromOneDrive);

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not think the updated logic here is correct.

If the response from fileDetailsFromOneDrive = checkFileOneDriveApiInstance.getPathDetailsByDriveId(parentItem.driveId, fileToUpload) generates a 200 we drop to the POSIX test.

In your code, you are then saving this data blindly to the DB, without validating that the details from online match what was intended to upload - so that DB save should not be happening until the validation of the data is done

Please can you review your thinking here

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My idea was that if we maintain the latest online version in the local database, it can help force triggering a reupload
later during the database inconsistency check in case the corresponding upload process fail. Otherwise, there might be a risk of losing this inconsistency.

abraunegg added a commit that referenced this pull request Jan 17, 2024
* Manually merge #2527 into 'alpha-5' as new PR
@abraunegg
Copy link
Owner

Closing in favour of #2595

@abraunegg
Copy link
Owner

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

Repository owner locked and limited conversation to collaborators Jan 28, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants