A new version of the Alternate FTP backend has been contributed to Duplicati.
I have done a quick test of this change and it seems to be more stable in this particular setup than current Canary (106)
Before including it in next Canary, it would be useful if concerned users could give it a try, installers for main platforms can be found here if you have a Github login:
if you don’t have a Github account, you can get the Win 64 installer from here:
Note that the first link will expire in 3 days, the second will not and if needed I can upload other installers to the second link.
Note that next Canary will certainly not appear before one month, probably 2 so there is no rush to test.
Thanks in advance if you contribute your experience.
On Windows 10 with old FileZilla Server, Test connection with a missing folder got worse.
“Failed to connect: One or more errors occurred.” popup instead of
“Failed to connect: CWD failed. “/aftp”: directory not found.”
Both of these got the same server response to CWD
(000021)5/18/2023 14:40:26 PM - duplicati (::1)> CWD /aftp
(000021)5/18/2023 14:40:26 PM - duplicati (::1)> 550 CWD failed. “/aftp”: directory not found.
EDIT:
About → Show log → Stored got
May 18, 2023 2:40 PM: Request for http://127.0.0.1:8300/api/v1/remoteoperation/test gave error
System.AggregateException: One or more errors occurred. ---> FluentFTP.Exceptions.FtpCommandException: Code: 550 Message: CWD failed. "/aftp": directory not found.
at FluentFTP.AsyncFtpClient.<SetWorkingDirectory>d__89.MoveNext()
--- End of inner exception stack trace ---
at System.Threading.Tasks.Task.ThrowIfExceptional(Boolean includeTaskCanceledExceptions)
at System.Threading.Tasks.Task.Wait(Int32 millisecondsTimeout, CancellationToken cancellationToken)
at Duplicati.Library.Backend.AlternativeFTP.AlternativeFtpBackend.CreateClient()
at Duplicati.Library.Backend.AlternativeFTP.AlternativeFtpBackend.List(String filename, Boolean stripFile)
at Duplicati.Library.Backend.AlternativeFTP.AlternativeFtpBackend.Test()
at Duplicati.Server.WebServer.RESTMethods.RemoteOperation.TestConnection(String url, RequestInfo info)
at Duplicati.Server.WebServer.RESTHandler.DoProcess(RequestInfo info, String method, String module, String key)
---> (Inner Exception #0) FluentFTP.Exceptions.FtpCommandException: Code: 550 Message: CWD failed. "/aftp": directory not found.
at FluentFTP.AsyncFtpClient.<SetWorkingDirectory>d__89.MoveNext()<---
EDIT 2:
Seems to pass a smoke test aside from that, including some playing with passive versus active.
No encryption though, and no over-the-Internet-with-potential-errors though. I’ll test a little more.
Thanks, repro with Filezillaserver 1.7. It was a quick test. I was so happy to have mastered Ftp, Filezilla and all that that I selected at first standard Ftp backend, created the directory, then switched to Alternate Ftp without realizing that this feature would be not tested. After creating a new backup, I see that this feature has never worked correctly and it’s worse with the new version. Looking at the code I see that the backend rely on the directory already existing. Maybe it has been written with the idea that all the files were put directly in the default directory ? Yes, if no directory is selected, Duplicati displays a warning but backup works (test does not).
Strange that you can’t make Ftps work, it has worked for me (I setup the server to enforce encryption).
Oh wait, there is a problem in this new code, it has left the init of Client with
// We do not support parallel uploads, and the feature is buggy
EnableThreadSafeDataConnections = false,
I was trying to reproduce an earlier Test connection error which I think was delete on access test file. Because I couldn’t, I just tested the access test (took away write permissions from folder). Old and new:
“Failed to connect: Error writing file: One or more errors occurred.” error popup, with server log saying:
May 19, 2023 7:36 AM: Request for http://localhost:8200/api/v1/remoteoperation/test gave error
System.Exception: Error writing file: One or more errors occurred. ---> System.AggregateException: One or more errors occurred. ---> Duplicati.Library.Interface.UserInformationException: Error writing file: duplicati-access-privileges-test.tmp
at Duplicati.Library.Backend.AlternativeFTP.AlternativeFtpBackend.<PutAsync>d__40.MoveNext()
--- End of inner exception stack trace ---
at System.Threading.Tasks.Task.ThrowIfExceptional(Boolean includeTaskCanceledExceptions)
at System.Threading.Tasks.Task.Wait(Int32 millisecondsTimeout, CancellationToken cancellationToken)
at Duplicati.Library.Backend.AlternativeFTP.AlternativeFtpBackend.Test()
--- End of inner exception stack trace ---
at Duplicati.Library.Backend.AlternativeFTP.AlternativeFtpBackend.Test()
at Duplicati.Server.WebServer.RESTMethods.RemoteOperation.TestConnection(String url, RequestInfo info)
at Duplicati.Server.WebServer.RESTHandler.DoProcess(RequestInfo info, String method, String module, String key)
I don’t know if your fix for CWD error handling deterioration improved what might be old bug with STOR:
(000073)5/19/2023 7:36:39 AM - duplicati (::1)> STOR duplicati-access-privileges-test.tmp
(000073)5/19/2023 7:36:39 AM - duplicati (::1)> 150 Opening data channel for file upload to server of "/aftp test/duplicati-access-privileges-test.tmp"
(000073)5/19/2023 7:36:39 AM - duplicati (::1)> 550 Could not open file for writing.
and actually I’m wondering if new code does what some others do on missing folder – offer to create it.
The main goal IMO is to not get worse, so if some old bugs remain for another fix another day, so be it.
I didn’t quite follow the EnableThreadSafeDataConnections worry. That feature isn’t used these days.
@gpatel-fr
First off - thanks a lot for lifting up the maintainer gauntlet, and specifically for merging my PR. Highly appreciated
I think you might be looking at a wrong branch / commit? (or maybe I misunderstood the concern?)
As far as I can tell, the new code I submitted (see L#548 of original) no longer references EnableThreadSafeDataConnections - If it had, it would’ve resulted in a build error.
Could you please include a link or more specific reference?
Actually, I looked at a place that is wrong for me, in the Github interface - I should always look at code in my local editor instead because I miss too much stuff
Yes if the old code does something bad, it’s not necessary for the upgrade to fix it.
If I had time I’d try to add a test in the CreateClient function ‘if we are called from Test, catch the error in SetWorkingDirectory and ignore it’. Maybe it would fix this piece of nastiness.
@ts678
Thanks for all your time and effort spent on Duplicati, I see that for a long time you’re one of the most active folks here and on Github Issues - basically keeping Duplicati flame alive.
I would like to ask for clarification re the two issues you found:
Issue 1:
Issue2:
Q’s:
I didn’t quite get whether the deterioration you mentioned is A. between the current (old) Alternate FTP and the new Alternate FTP (i.e. between the old and new FluentFTP versions) - or - B. between the “regular” FTP and Alternate FTP.
I’m not in front of the code now, but as far as I recall “Test Connection” indeed works differently between “regular” FTP (better) and Alternate FTP (worse).
If it is B, I’m happy to try and fix, but was mostly focused on just upgrading the FluentFTP library version without introducing new changes - do folks think I should improve Alternative FTP’s Test Connection to match FTP Test Connection? (or is this just adding risk)
If it is A, I’ll definitely take a stab at fixing (the new code shouldn’t in my mind introduce any deterioration)
Are Issue 1 and Issue 2 the same or different? I couldn’t quite understand.
Could you please provide specific steps to help reproduce the issue(s)?
Between latest Canary 2.0.6.106 and the test build (i.e. as requested).
FTP (Alternative) introduced a more ambitious test. In a way it’s better.
There are, though, more things that could go wrong, due to complexity.
Typically the test is TestList which does a list. Exact listing is ignored.
AlternativeFTP tests permissions too by doing a put, get, and delete.
Issue 1 is just have no pre-existing folder of that name. Test connection.
Issue 2 depends on FTP server, but on Windows I finally used the folder
Right-click → Properties → Security → Advanced to make permissions
explicit rather than inherited from C:\ and took away all write permission.
As Test connection tries to test permissions, that should be noticed…
If macOS had cooperated with 2.0.6.105 Canary (but it still has limitations even in 2.0.6.106 retry), there would possibly be a Beta by now, so worries about late-stage risk would be lowered. That didn’t happen.
There’s an ongoing question of how to relax such precautions without adding unwanted branching pains. Simplistic approach of variable tightening of commits seems to be the historical plan (lacking better one). There are some mostly Developer category topics on this where you can suggest, if you have a solution.
a. It unpacks the AggregateException, and throws the correct missing folder exception
b. Moved this try/catch to the SetWorkingDirecotry in CreateClient (instead of List and PutAsync)
c. Fixed the condition that detects the missing folder (was highly dependent on specific error string from the server, changed to depend on error code)
d. Fixed CreateFolder (by having CreateClient not try to SetWorkingDirectory when called from CreateFolder)
Diff seems ugly, due to two large try blocks that were removed in (b). The changed indentation confuses Github text diff? The actual diff is less than what it seems in List and PutAsync.
@ts678 regarding Issue 2:
I’m still not sure I fully understand what the issue is.
If I understand correctly, we get the following prompt, when write permissions were intentionally removed:
Isn’t this behavior working as intended? (given write permissions were removed).
I’m not trying to be thick, just literally having a hard time understanding what the intended behavior would have been, if not this popup.
Good point, and thanks for noticing what I missed.
“Failed to connect: One or more errors occurred.” was the first message, and this one looked similar, except for the extra three words that gave the issue. The start, though, seems a little bit inaccurate if connection was actually established, and it was. I suppose if I was super ambitious and creative, the other specific tests such as the read and the delete could possibly be induced to see their messages.
Worked a bit more on the PR, now ready for review / merging
(and ideally a new test binary here?)
Issue 1 resolved, Issue 2 improved (the “Failed to connect:” prefix is added elsewhere, to many/all errors; I don’t want to bloat this change)
No regression left in either case compared to latest Canary 2.0.6.106, furthermore, some things that were broken even in latest Canary are now fixed in this PR (e.g. offering the user to create a missing base folder).
