Magic Folder: implement "Water Dragons" section of design doc

[davidsarah]

The prototype implementation of drop-upload in #1429 adds new or changed files in the local directory to the upload directory, but it does not unlink files in the upload directory when they are deleted locally.

Note that:

• some users might not want files to be immediately unlinked from the upload directory, in case the local deletion was accidental.
• the space for unlinked files would not be reclaimed unless and until garbage collection is performed. (This applies already to old versions of changed files.)

[davidsarah]

On 06/04/12 03:20, erpo4 wrote on the vg2 list:

davidsarah wrote:

erpo4 wrote:

I think the drop_upload feature is what I want. Files I put in the local directory seem to be uploaded to the grid. However, when I delete the files out of my local directory, the files are not automatically deleted from the grid. Is this the expected behavior?

Yes, it is. I've just filed <​https://tahoe-lafs.org/trac/tahoe-lafs/ticket/1710> to allow the automatic deletion behaviour. (This isn't likely to be a priority to support in the next couple of releases, though, unless someone volunteers to implement it.)

Just to be clear: I wasn't complaining. :) The only reason I asked was that it's not the behavior I'm used to from other cloud storage services (e.g. Ubuntu One). It may actually be preferable not to autodelete files that have been removed from the drop_upload directory. In the case that I accidentally rm -rf /drop_upload_dir/*, I wouldn't want that to wipe out my online backups when tahoe gets inotify'd. If this is to be a feature, perhaps it should default to off?

It would indeed default to off, since we don't want the behaviour to change surprisingly from the current behaviour on a Tahoe upgrade.

[daira]

Add magic-folder keyword to all drop-upload tickets.

[daira]

The intended behaviour for Magic Folder is described at docs/proposed/magic-folder/remote-to-local-sync.rst#deletion-of-a-file.

[dawuud]

I wrote some rough draft code that handles the local file deletion case:

this temporary dev branch: ​https://github.com/david415/tahoe-lafs/tree/scan-parent

this commit: ​https://github.com/david415/tahoe-lafs/commit/a256ddb50d384aa18ac241dcdb9d5757725ad9d2

but we should finish our rebase and cherry-pick this commit onto the new dev branch when it is available...

[dawuud]

i put those changes here: ​https://github.com/david415/tahoe-lafs/tree/2438.otf-objective-4.2-local-file-deletion.1 but we first need to get the tests to pass before working more on this...

[dawuud]

my latest using an added api call in backupdb.py specifically for checking if a given filename has an entry in the db:

​https://github.com/david415/tahoe-lafs/commit/594de028e81ce8afdc536fca4844b4c14fe9f6d0

[dawuud]

in preparation for actually completing this task i've merged in the magic-folder CLI tools dev branch... and thus have been able to prepare tahoe magic-folder test grids using the CLI. Here we've written rough draft versions of test_persistence and test_move_tree that use the magic-folder CLI.

​https://github.com/david415/tahoe-lafs/tree/1710.local-deletion-water-dragon.1 ​https://github.com/david415/tahoe-lafs/commit/0054798c9095466eea8446b66146156c9f2f4dc0 ​https://github.com/david415/tahoe-lafs/commit/ae9c492b79472fe71a30ee15dece87e29d18b7c1

we've introduced code duplication... next we should make sure all the magic-folder tests share this code instead of duplicating it.

we could benefit from a review but we are not currently blocked

[dawuud]

cleaned up duplicate code here... ​https://github.com/david415/tahoe-lafs/commit/6775c38f3eb40c494d7a06e5fa9dfee97f286507

[dawuud]

i'm building up Alice-Bob tests here so we can have multiple writers to a magic-folder in a test:

​https://github.com/david415/tahoe-lafs/tree/1710.local-deletion-water-dragon.1.test-alice-and-bob.1

[dawuud]

I had to change the magic-folder cli unit test api when i added the Alice + Bob test. in this commit i fixed the non-Alice+Bob magic-folder CLI tests: ​https://github.com/david415/tahoe-lafs/commit/e2ce372cbf02dc2ad56a87b505e157d576b7101b

[dawuud]

I've added a naive downloader and remote scan mechanism... The basic "sharing" between Alice and Bob is working in so far as the unit test passes now.

this dev branch: ​https://github.com/david415/tahoe-lafs/tree/1710.local-deletion-water-dragon.1.test-alice-and-bob.1

this commit: ​https://github.com/david415/tahoe-lafs/commit/d214c7c2143e75a67a23b47d80220ff4f9837fbd

this test: ./bin/tahoe debug trial allmydata.test.test_magic_folder.RealTest?.test_alice_bob

[dawuud]

just now added "version" to the magic folder db schema... creating a version 3 of tahoe backupdb schema... which is unutilized by the existing tahoe backup command but is used by only magic folders thus far.

here we also roughly schetch out the addition and deletion behavior with the local version number being incremented by file modifications and deletions...

​https://github.com/david415/tahoe-lafs/commit/209c0285be35aaa587cd71aee6bcc1bac4d3e452

[dawuud]

added deletion/version propagation... and a basic test for that however i am having trouble getting the test_persistence test to pass... here's my latest:

​https://github.com/david415/tahoe-lafs/tree/1710.local-deletion-water-dragon.1.test-alice-and-bob.1 ​https://github.com/david415/tahoe-lafs/commit/ccdba44e8a51787ca39a9db41b975e9290727831

[dawuud]

Alice and Bob mock unit test fixed in my dev branch: ​https://github.com/david415/tahoe-lafs/tree/1710.local-deletion-water-dragon.1.test-alice-and-bob.1

[daira]

Rebased to ​https://github.com/tahoe-lafs/tahoe-lafs/commits/1710.test-alice-and-bob.3.

[dawuud]

slight progress with breaking work in progress commits on my dev branch here: ​https://github.com/david415/tahoe-lafs/tree/2477.refactor-all-the-things.0

here the alice + bob test checks for proper version numbers... and we can identitify that the current difficulty happens right after alice deletes the file... her magic-folder db entry for that file should be updated with the new version number but it is not... and so the test fails because version number mismatch.

[dawuud]

more progress in my dev branch... i got rid of the live-lock... however the test exits with a dirty reactor.

[daira]

Merged David's changes and mine (to use deferredutil.HookMixin) into ​https://github.com/tahoe-lafs/tahoe-lafs/tree/2477.refactor-all-the-things.1.

[dawuud]

found and fixed two more dropped deferreds but the alice bob unit test still errs: ​https://github.com/david415/tahoe-lafs/tree/2477.refactor-all-the-things.1

[daira]

After our Tuesday pairing, tests now pass on that branch.

[dawuud]

I do not believe we've implemented this Water Dragon feature: """When a Magic Folder client restarts, we can detect files that had been downloaded but were deleted while it was not running, because their paths will have last-downloaded records in the magic folder db without any corresponding local file."""

[daira]

Yes, I believe that's correct (that it is not implemented). As well as scanning for existing files/directories at startup, we should simulate change events for paths in the magic folder db at which no file/directory exists.

We can do that by obtaining the set of all entries in the local_files table before doing the scan, removing paths from that set as we process them, and then processing all those remaining in the set.

[dawuud]

i added a get_all_files as a magic-folder db util function... but this test_persistence unit test seems broken as the startup code path does not run twice like it should... ​https://github.com/david415/tahoe-lafs/tree/2489.write_downloaded_file.1

[dawuud]

I'm trying to fix the test_alice_bob test... and I've made some progress here... with this simple commit that teaches the uploader to ignore temp suffix files we create: ".tmp" and ".backup" :

​https://github.com/david415/tahoe-lafs/tree/1710.ignore-temp-files.0

[dawuud]

Daira pointed out yet another bug due to our Uploader / Downloader code refactor; i made a fix commit here: ​https://github.com/david415/tahoe-lafs/tree/2438.magic-folder-stable.1

""" Add RemoteScanMixin to share our _get_collective_latest_file method

Both the Uploader and Downloader need to know the latest collective remote file... """

​https://github.com/david415/tahoe-lafs/commit/6e8728ca0fd6a0186648bb64e187e999255f7bf3

[daira]

Replying to dawuud:

Daira pointed out yet another bug due to our Uploader / Downloader code refactor; i made a fix commit here: ​https://github.com/david415/tahoe-lafs/commit/6e8728ca0fd6a0186648bb64e187e999255f7bf3

The ​https://github.com/tahoe-lafs/tahoe-lafs/commits/2438.magic-folder-stable.3 branch fixes this in a different way; the 2438.magic-folder-stable.1 branches are now obsolete.

[dawuud]

I think we can close this ticket now that all of the deletion propagation features have been implemented.

Can we close it?

[daira]

We're close, but I need to review whether all of the Water Dragons features have been implemented.

[dawuud]

I think we did not finish implementing deletion i did part of it; and meejah mentioned he added that feature. Please post the dev branch location so we can rebase it into our new dev/stable branches.

[daira]

​https://github.com/tahoe-lafs/tahoe-lafs/commits/2438.magic-folder-stable.4 is the current stable branch. ​https://github.com/tahoe-lafs/tahoe-lafs/commits/2521.magic-db-schema.1 is the current, broken, development branch.

[meejah]

There's also ​https://github.com/meejah/tahoe-lafs/commits/1710.actually-unlink-files.0 which I will do a version of rebased onto magic-db-schema.1 (I guess?)

[daira]

I'm reviewing that branch now.

[daira]

My current work-in-progress is at 2521.magic-db-schema.1.wip .

[meejah]

I have a ​https://github.com/meejah/tahoe-lafs/commits/1710.actually-unlink-files.1 branch now also, and another unit-test and further smoke test logic.

There's still a case which doesn't work:

If alice creates a file, then bob deletes it, and then alice re-creates it bob never sees it. (It *does* however, work, if bob is the one to re-create it). I'm still investigating this.

[meejah]

I mean: I have more things to push to the .1 branch, including some as-yet-unwritten code to fix the issue I mention above :)

[daira]

daira: dawuud meejah: the deletion bug is an omission in the design doc. in the case of existent files, the event when the file is replaced by the downloader will be ignored because the entry for that path in the magic folder db has the same size/mtime/ctime as the new file daira: the same *should* be the case for a deleted file. so the (size, mtime, ctime) in the db needs to be the same as returned by fileutil.get_pathinfo. the latter is (None, now, now) in the current implementation, which I think is probably wrong... daira: because the current time at the point of the get_pathinfo call won't/can't match the time in the database daira: the right fix is probably to change get_pathinfo to return PathInfo(isdir=False, isfile=False, islink=False, exists=False, size=None, mtime=None, ctime=None) for the ENOENT case daira: and make sure that's also what is written to the db when the downloader detects a deleted file

[daira]

(None is converted to an SQL NULL.)

[daira]

We also need to think about what happens when a file is replaced by a directory or vice versa. Those are treated as different paths (the directory path ends in /), so it's a bit different from the deleted file case.

[meejah]

the case I have a fix for is: on say "bob's" side if "alice" deletes: the downloader downloads a "delete", so moves the file to "whatever.tmp" but immediately bob's uploader gets an inotify for the moved file, and when that gets processed it uploads another version (with delete=True) -- but so then if "alice" now replaces the file, bob doesn't download it because the versions match.

[daira]

Replying to meejah:

the case I have a fix for is: on say "bob's" side if "alice" deletes: the downloader downloads a "delete", so moves the file to "whatever.tmp"

Do you mean "whatever.backup"? Nothing should ever be moved to "whatever.tmp".

In any case, the IN_MOVED_FROM event for "whatever" should be ignored as described in comment:42. (The IN_MOVED_TO event should also be ignored because it's for an ignorable filename pattern.)

but immediately bob's uploader gets an inotify for the moved file, and when that gets processed it uploads another version (with delete=True) -- but so then if "alice" now replaces the file, bob doesn't download it because the versions match.

Yes, this sounds like the same case that I was talking about. It isn't a problem that Bob's uploader gets an inotify event, but that event should be ignored because Bob's db should already have been updated with metadata showing that the file has been deleted.

[daira]

I will update the design doc tomorrow to clarify what should happen.

[daira]

Replying to daira:

I will update the design doc tomorrow to clarify what should happen.

See ​https://github.com/tahoe-lafs/tahoe-lafs/commit/5d2365f6c4cc388c2d74e43b2c1b7e8fc4c4ab86 (the previous three commits to the design doc are also worth reading).

[daira]

So, I believe the fix is just to add this before the last line of MagicFolderDB.is_new_file:

if not pathinfo.exists and row[0] is None:
    return False

[daira]

Replying to daira:

So, I believe the fix is just to add this before the last line of MagicFolderDB.is_new_file:

if not pathinfo.exists and row[0] is None:
    return False

This fix is on the 2438.magic-folder-stable.5 branch, and there are tests for deletion. So I believe this is fixed, at least until we have a new design that handles the >2-client case.

[warner]

Milestone renamed