#1710 closed enhancement (fixed)

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

Reported by: davidsarah Owned by: daira
Priority: normal Milestone: 1.12.0
Component: code-frontend-magic-folder Version: 1.9.1
Keywords: unlink space-efficiency gc otf-magic-folder-objective4 Cc:
Launchpad Bug:

Description (last modified by warner)

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.)

Change History (51)

comment:1 Changed at 2012-04-06T04:17:13Z by 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.

comment:2 Changed at 2014-12-02T19:49:06Z by warner

  • Component changed from code-frontend to code-frontend-drop-upload
  • Description modified (diff)

comment:3 Changed at 2015-06-01T16:11:09Z by daira

  • Keywords magic-folder added

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

comment:4 Changed at 2015-06-01T16:21:02Z by daira

  • Keywords otf-magic-folder-objective-4 added; drop-upload magic-folder removed
  • Milestone changed from undecided to 1.11.0
  • Owner set to dawuud
  • Summary changed from drop-upload: optionally unlink locally deleted files from upload directory to Magic Folder: implement local deletion of files as described in "Air Dragons" section of design doc

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

comment:5 Changed at 2015-06-01T16:29:39Z by daira

  • Summary changed from Magic Folder: implement local deletion of files as described in "Air Dragons" section of design doc to Magic Folder: implement local deletion of files/directories as described in "Air Dragons" section of design doc

comment:6 Changed at 2015-06-01T16:30:12Z by daira

  • Summary changed from Magic Folder: implement local deletion of files/directories as described in "Air Dragons" section of design doc to Magic Folder: implement local deletion of files/directories as described in "Water Dragons" section of design doc

comment:7 Changed at 2015-06-01T16:39:32Z by daira

  • Keywords otf-magic-folder-objective4 added; otf-magic-folder-objective-4 removed

comment:8 Changed at 2015-06-10T02:04:15Z by 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...

comment:9 Changed at 2015-06-10T21:49:32Z by 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...

comment:10 Changed at 2015-06-11T22:03:00Z by 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

comment:11 Changed at 2015-06-24T23:53:07Z by 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

comment:13 Changed at 2015-06-27T03:27:08Z by 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

comment:14 Changed at 2015-06-29T21:03:49Z by 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

comment:15 Changed at 2015-07-02T04:24:47Z by 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

comment:16 Changed at 2015-07-02T18:55:35Z by 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

comment:17 Changed at 2015-07-03T01:23:13Z by dawuud

added deletion/version propagation... and a basic test for that however i am having proble 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

Version 0, edited at 2015-07-03T01:23:13Z by dawuud (next)

comment:20 Changed at 2015-07-31T23:23:39Z by 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.

comment:21 Changed at 2015-08-20T11:11:46Z by dawuud

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

comment:22 Changed at 2015-08-20T16:10:25Z by 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.

comment:23 Changed at 2015-08-24T11:29:54Z by 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

comment:24 Changed at 2015-08-25T20:16:01Z by daira

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

comment:25 Changed at 2015-08-28T12:47:03Z by 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."""

comment:26 Changed at 2015-08-31T12:19:33Z by 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.

comment:27 Changed at 2015-08-31T21:41:01Z by 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

comment:28 Changed at 2015-09-02T18:15:43Z by 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

comment:29 Changed at 2015-09-16T14:18:24Z by daira

  • Summary changed from Magic Folder: implement local deletion of files/directories as described in "Water Dragons" section of design doc to Magic Folder: implement "Water Dragons" section of design doc

comment:30 Changed at 2015-09-21T08:52:46Z by 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

Last edited at 2015-10-02T21:22:02Z by daira (previous) (diff)

comment:31 Changed at 2015-10-02T21:15:06Z by 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.

Last edited at 2015-10-02T21:21:37Z by daira (previous) (diff)

comment:32 Changed at 2015-10-05T12:02:57Z by dawuud

  • Owner changed from dawuud to daira

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

Can we close it?

comment:33 Changed at 2015-10-06T00:11:07Z by daira

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

comment:34 Changed at 2015-10-15T10:58:19Z by 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.

comment:35 Changed at 2015-10-15T10:58:51Z by dawuud

  • Owner changed from daira to meejah

comment:36 Changed at 2015-10-15T13:08:33Z by daira

https://github.com/tahoe-lafs/tahoe-lafs/commits/2438.magic-folder-stable.4 is the current stable branch (on which we did the demos).

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

Last edited at 2015-10-15T13:08:52Z by daira (previous) (diff)

comment:37 Changed at 2015-10-16T18:10:26Z by 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?)

comment:38 Changed at 2015-10-19T13:39:10Z by daira

  • Owner changed from meejah to daira
  • Status changed from new to assigned

I'm reviewing that branch now.

comment:39 Changed at 2015-10-19T17:31:34Z by daira

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

comment:40 Changed at 2015-10-20T15:18:54Z by 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.

comment:41 Changed at 2015-10-20T15:19:26Z by 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 :)

comment:42 Changed at 2015-10-24T22:21:53Z by 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

comment:43 Changed at 2015-10-24T22:22:21Z by daira

(None is converted to an SQL NULL.)

comment:44 Changed at 2015-10-24T22:25:21Z by 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.

comment:45 follow-up: Changed at 2015-10-24T22:37:14Z by 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.

comment:46 in reply to: ↑ 45 Changed at 2015-10-24T23:04:46Z by 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.

comment:47 follow-up: Changed at 2015-10-24T23:23:34Z by daira

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

Last edited at 2015-10-24T23:28:30Z by daira (previous) (diff)

comment:48 in reply to: ↑ 47 Changed at 2015-10-25T13:23:05Z by 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).

comment:49 follow-up: Changed at 2015-10-25T13:33:33Z by 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

comment:50 in reply to: ↑ 49 Changed at 2015-12-08T15:13:56Z by daira

  • Resolution set to fixed
  • Status changed from assigned to closed

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.

comment:51 Changed at 2016-03-22T05:02:52Z by warner

  • Milestone changed from 1.11.0 to 1.12.0

Milestone renamed

Note: See TracTickets for help on using tickets.