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

bisync: session name should normalize to non-canonical version for overridden configs #7423

Closed
LivingUserError opened this issue Nov 15, 2023 · 7 comments · Fixed by #7497
Closed

Comments

@LivingUserError
Copy link

What is the problem you are having with rclone?

When calling bisync with a remote, but a leading / in the remote path like so

rclone bisync remote:/path/with/unneeded/slash /some/local/path --resync

the path will be normalized by Rclone and the command will be executed without throwing any error (or warning).

If you then try to run bisync without --resync however, it will not behave as expected. Running the exact same command without --resync will always yield:

ERROR : Bisync critical error: cannot find prior Path1 or Path2 listings, likely due to critical error on prior run

This suggests that bisync failed previously, when in fact it didn't. It seems to name the listings after the normalized path, while checking for existing listings with the non-normalized path, as provided by the user.

This seems likely, because running the command again, but without the leading slash and --resync, will work:

rclone bisync remote:path/with/unneeded/slash /some/local/path

What is your rclone version (output from rclone version)

rclone v1.64.2

  • os/version: arch Soaring (64 bit)
  • os/kernel: 6.6.1-zen1-1-zen (x86_64)
  • os/type: linux
  • os/arch: amd64
  • go/version: go1.21.3
  • go/linking: dynamic
  • go/tags: none

Which OS you are using and how many bits (e.g. Windows 7, 64 bit)

Arch Linux, 64 bit

Which cloud storage system are you using? (e.g. Google Drive)

Proton Drive

How to use GitHub

  • Please use the 👍 reaction to show that you are affected by the same issue.
  • Please don't comment if you have no relevant information to add. It's just extra noise for everyone subscribed to this issue.
  • Subscribe to receive notifications on status change and new comments.
@nielash
Copy link
Collaborator

nielash commented Nov 15, 2023

Thank you for reporting this! I am having trouble reproducing it, though. For me, the following paths:

remote:/path/with/unneeded/slash
remote:path/with/unneeded/slash

both result in:

remote_path_with_unneeded_slash

in the listing filename.

Are you using any backend-specific flags that could be overriding your config and adding a suffixed canonical name? (More info)

Another thing that would be helpful is if you could try going into your workdir to see what bisync is actually naming the listing files, and whether there is any difference when you --resync with and without the slash.

For example, try something like the following sequence:

rclone bisync remote:/path/with/unneeded/slash /some/local/path --resync
rclone lsl '~/.cache/rclone/bisync'
rclone bisync remote:path/with/unneeded/slash /some/local/path --resync
rclone lsl '~/.cache/rclone/bisync'

And let me know if there is any difference between the filenames output by the two lsls?

If that doesn't yield clues, I'm also curious if it could be something specific to the encoding settings on Proton Drive. Any chance you could try the same thing with Google Drive and see if you get a different result?

@nielash nielash self-assigned this Nov 15, 2023
@nielash nielash added the bisync label Nov 15, 2023
@LivingUserError
Copy link
Author

I am using one backend-specific option:

--protondrive-replace-existing-draft=true

My full command looks like this:

rclone bisync proton:sync /home/user/Proton --no-update-modtime --no-traverse --transfers 8 --protondrive-replace-existing-draft=true -v

ls '~/.cache/rclone/bisync' yields:

proton{kpshc}_sync..home_user_Proton.lck
proton{kpshc}_sync..home_user_Proton.path1.lst
proton{kpshc}_sync..home_user_Proton.path2.lst
proton_sync..home_user_Proton.path1.lst-new
proton_sync..home_user_Proton.path2.lst-new

This is after the command has been run with the unneeded slash and --resync first, and without both afterwards. If I find the time I can retry from zero, as you suggested.

I forgot to mention that I run rclone as root btw., maybe that also helps.

@nielash
Copy link
Collaborator

nielash commented Nov 17, 2023

Thanks! The {kpshc} in the remote name is the telltale sign of an overridden config. The two that don't have it are curious to me -- are you able to tell from their modtimes whether they were created around the same time as the others? Or are they possibly just leftover from an old run, before you started using --protondrive-replace-existing-draft=true?

Are you using --protondrive-replace-existing-draft=true for both --resync and non-resync?

@LivingUserError
Copy link
Author

After some extensive testing, you are probably right. I deleted my previous comment, because I noticed a mistake in the testing - didn't provide anything of value.

I must have added --protondrive-replace-existing-draft=true after the fact and that broke it. Might still want to add a hint that similar listings (i.e. listings form running the command with different options) exist. That was quite the debug session 😅

@nielash
Copy link
Collaborator

nielash commented Nov 20, 2023

Might still want to add a hint that similar listings (i.e. listings form running the command with different options) exist. That was quite the debug session 😅

Thanks, and I agree! This is one I have tripped over myself. I added a section to the docs about it in v1.64.0, but I don't think that goes far enough, as users are continuing to stumble over it, and it can actually have serious consequences in the reverse direction (if you remove a backend-specific flag and still have old listings using the alternate name.)

The ultimate fix for this is #5678, but in the meantime I think we should normalize to the non-canonical name for listings -- it should not be possible to have more than one session name for the same pair of bisync paths. I have an idea about how to implement this in a way that is backward-compatible, which I will try out as soon as I have a chance. Thanks again for your work on this!

@nielash nielash changed the title bisync: Fails with misleading error if path not correctly specified bisync: session name should normalize to non-canonical version for overridden configs Nov 20, 2023
@nielash nielash added this to the Soon milestone Nov 20, 2023
@nielash nielash added this to In progress in bisync Nov 20, 2023
@LivingUserError
Copy link
Author

Thank you for pointing me in the right direction! I could see why you would want to have multiple sessions for accessing the same remote tho - these backend-specific options could do pretty much anything, can't they? Maybe a warning/prompt is really the best one can do here, without limiting functionality.

@nielash
Copy link
Collaborator

nielash commented Nov 20, 2023

When I say "session" here I'm talking specifically about the SessionName.

In other words, if you do

rclone bisync a b

followed by

rclone bisync a b --protondrive-replace-existing-draft=true

bisync should understand that this is the same pair of paths, and that therefore the same listing file should be used. (Same as would happen today with a non-backend-specific flag.)

As opposed to:

rclone bisync a c

which is a different session from rclone bisync a b and should use a different listing file.

I don't think what I'm suggesting limits any functionality (it's more of a bug fix, really), but feel free to push back if you think I'm missing something!

nielash added a commit to nielash/rclone that referenced this issue Dec 9, 2023
Before this change, bisync used the "canonical" Fs name in the filename for its
listing files, including any {hexstring} suffix. An unintended consequence of
this was that if a user added a backend-specific flag from the command line
(thus "overriding" the config), bisync would fail to find the listing files it
created during the prior run without this flag, due to the path now having a
{hexstring} suffix that wasn't there before (or vice versa, if the flag was
present when the session was established, and later removed.) This would
sometimes cause bisync to fail with a critical error (if no listing existed
with the alternate name), or worse -- it would sometimes cause bisync to use an
old, incorrect listing (if old listings with the alternate name DID still
exist, from before the user changed their flags.)

After this change, the issue is fixed by always normalizing the SessionName to
the non-canonical version (no {hexstring} suffix), regardless of the flags. To
avoid a breaking change, we first check if a suffixed listing exists. If so, we
rename it (and overwrite the non-suffixed version, if any.) If not, we carry on
with the non-suffixed version. (We should only find a suffixed version if
created prior to this commit.)

The result for the user is that the same pair of paths will always use the same
.lst filenames, with or without backend-specific flags.
nielash added a commit to nielash/rclone that referenced this issue Jan 20, 2024
Before this change, bisync used the "canonical" Fs name in the filename for its
listing files, including any {hexstring} suffix. An unintended consequence of
this was that if a user added a backend-specific flag from the command line
(thus "overriding" the config), bisync would fail to find the listing files it
created during the prior run without this flag, due to the path now having a
{hexstring} suffix that wasn't there before (or vice versa, if the flag was
present when the session was established, and later removed.) This would
sometimes cause bisync to fail with a critical error (if no listing existed
with the alternate name), or worse -- it would sometimes cause bisync to use an
old, incorrect listing (if old listings with the alternate name DID still
exist, from before the user changed their flags.)

After this change, the issue is fixed by always normalizing the SessionName to
the non-canonical version (no {hexstring} suffix), regardless of the flags. To
avoid a breaking change, we first check if a suffixed listing exists. If so, we
rename it (and overwrite the non-suffixed version, if any.) If not, we carry on
with the non-suffixed version. (We should only find a suffixed version if
created prior to this commit.)

The result for the user is that the same pair of paths will always use the same
.lst filenames, with or without backend-specific flags.
nielash added a commit that referenced this issue Jan 20, 2024
Before this change, bisync used the "canonical" Fs name in the filename for its
listing files, including any {hexstring} suffix. An unintended consequence of
this was that if a user added a backend-specific flag from the command line
(thus "overriding" the config), bisync would fail to find the listing files it
created during the prior run without this flag, due to the path now having a
{hexstring} suffix that wasn't there before (or vice versa, if the flag was
present when the session was established, and later removed.) This would
sometimes cause bisync to fail with a critical error (if no listing existed
with the alternate name), or worse -- it would sometimes cause bisync to use an
old, incorrect listing (if old listings with the alternate name DID still
exist, from before the user changed their flags.)

After this change, the issue is fixed by always normalizing the SessionName to
the non-canonical version (no {hexstring} suffix), regardless of the flags. To
avoid a breaking change, we first check if a suffixed listing exists. If so, we
rename it (and overwrite the non-suffixed version, if any.) If not, we carry on
with the non-suffixed version. (We should only find a suffixed version if
created prior to this commit.)

The result for the user is that the same pair of paths will always use the same
.lst filenames, with or without backend-specific flags.
@nielash nielash moved this from In progress to Done in bisync Jan 20, 2024
miku added a commit to internetarchive/rclone that referenced this issue Jan 23, 2024
* master: (86 commits)
  fs: add more detailed logging for file includes/excludes
  bisync: add --resync-mode for customizing --resync - fixes rclone#5681
  bisync: fix --colors flag
  bisync: factor resync to separate file
  bisync: skip empty test case dirs
  bisync: add options to auto-resolve conflicts - fixes rclone#7471
  bisync: check for syntax errors in path args - fixes rclone#7511
  bisync: add overlapping paths check
  bisync: allow lock file expiration/renewal with --max-lock - rclone#7470
  bisync: Graceful Shutdown, --recover from interruptions without --resync - fixes rclone#7470
  bisync: full support for comparing checksum, size, modtime - fixes rclone#5679 fixes rclone#5683 fixes rclone#5684 fixes rclone#5675
  bisync: document beta status more clearly - fixes rclone#6082
  bisync: normalize session name to non-canonical - fixes rclone#7423
  bisync: update version number in docs
  bisync: account for differences in backend features on integration tests - see rclone#5679
  operations: fix renaming a file on macOS
  bisync: fallback to cryptcheck or --download when can't check hash
  local: fix cleanRootPath on Windows after go1.21.4 stdlib update
  bisync: support two --backup-dir paths on different remotes
  bisync: support files with unknown length, including Google Docs - fixes rclone#5696
  ...
WuTofu pushed a commit to WuTofu/rclone that referenced this issue Feb 24, 2024
Before this change, bisync used the "canonical" Fs name in the filename for its
listing files, including any {hexstring} suffix. An unintended consequence of
this was that if a user added a backend-specific flag from the command line
(thus "overriding" the config), bisync would fail to find the listing files it
created during the prior run without this flag, due to the path now having a
{hexstring} suffix that wasn't there before (or vice versa, if the flag was
present when the session was established, and later removed.) This would
sometimes cause bisync to fail with a critical error (if no listing existed
with the alternate name), or worse -- it would sometimes cause bisync to use an
old, incorrect listing (if old listings with the alternate name DID still
exist, from before the user changed their flags.)

After this change, the issue is fixed by always normalizing the SessionName to
the non-canonical version (no {hexstring} suffix), regardless of the flags. To
avoid a breaking change, we first check if a suffixed listing exists. If so, we
rename it (and overwrite the non-suffixed version, if any.) If not, we carry on
with the non-suffixed version. (We should only find a suffixed version if
created prior to this commit.)

The result for the user is that the same pair of paths will always use the same
.lst filenames, with or without backend-specific flags.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging a pull request may close this issue.

2 participants