fix(worker): Avoid full index checkout on deletes

packages/openneuro-server/src/datalad/dataset.ts

@@ -377,6 +377,8 @@ export const deleteFiles = (datasetId, files, user) => {
     .del(filesUrl(datasetId))
     .set("Cookie", generateDataladCookie(config)(user))
     .set("Accept", "application/json")
+    // Fail with an error instead of hanging indefinitely if a worker stalls
+    .timeout({ response: 60000, deadline: 300000 })
     .send({ filenames })
     .then(() => filenames)
 }

services/datalad/datalad_service/tasks/files.py

@@ -1,5 +1,7 @@
 import json
 import logging
+import os
+import shutil
 import subprocess
 from urllib.parse import urlparse, parse_qs
 
@@ -13,16 +15,55 @@
 from datalad_service.config import AWS_S3_PUBLIC_BUCKET
 
 
+def target_path(dataset_root, path):
+    """Resolve a dataset relative path, rejecting anything outside of the dataset."""
+    # Normalize without resolving symlinks, annexed files are symlinks into .git/annex
+    target = os.path.normpath(os.path.join(dataset_root, path))
+    if target == dataset_root or not target.startswith(dataset_root + os.sep):
+        raise ValueError(f'"{path}" is not a path within this dataset')
+    if os.path.relpath(target, dataset_root).split(os.sep)[0] == '.git':
+        raise ValueError(f'"{path}" is a git internal path and cannot be removed')
+    return target
+
+
+def remove_from_worktree(dataset_root, target):
+    """Delete one file or directory and any parent directories it leaves empty."""
+    try:
+        if os.path.isdir(target) and not os.path.islink(target):
+            shutil.rmtree(target)
+        else:
+            os.remove(target)
+    except FileNotFoundError:
+        # Already gone, the index update below is all that is needed
+        pass
+    # Git does not track directories, so drop any that are now empty
+    parent = os.path.dirname(target)
+    while parent != dataset_root:
+        try:
+            os.rmdir(parent)
+        except OSError:
+            break
+        parent = os.path.dirname(parent)
+
+
 async def remove_files(store, dataset, paths, name=None, email=None, cookies=None):
     dataset_path = store.get_dataset_path(dataset)
     repo = pygit2.Repository(dataset_path)
     if name and email:
         author = pygit2.Signature(name, email)
     else:
         author = None
+    dataset_root = os.path.realpath(dataset_path)
+    # Validate every path before changing anything
+    targets = [target_path(dataset_root, path) for path in paths]
     repo.index.remove_all(paths)
     repo.index.write()
-    repo.checkout_index()
+    # Remove the requested paths from the working tree directly. repo.checkout_index()
+    # would do this too, but it diffs the entire index against the entire working tree,
+    # making any delete cost time proportional to the size of the dataset instead of to
+    # the number of paths being deleted.
+    for target in targets:
+        remove_from_worktree(dataset_root, target)
     await git_commit_index(repo, author, message='[OpenNeuro] Files removed')
 
 

services/datalad/tests/test_files.py

@@ -2,9 +2,11 @@
 import falcon
 from falcon import testing
 import json
+import pygit2
+import pytest
 from datalad.api import Dataset
 
-from datalad_service.tasks.files import parse_s3_annex_url
+from datalad_service.tasks.files import parse_s3_annex_url, target_path
 
 
 class FileWrapper:
@@ -372,6 +374,112 @@ def test_delete_nested_file(client, new_dataset):
     assert json.loads(response.content)['deleted'] == ['derivatives/LICENSE', 'CHANGES']
 
 
+def test_delete_file_removes_from_working_tree(client, new_dataset):
+    ds_id = os.path.basename(new_dataset.path)
+    response = client.simulate_delete(
+        f'/datasets/{ds_id}/files', body='{ "filenames": ["CHANGES"] }'
+    )
+    assert response.status == falcon.HTTP_OK
+    # Removed from the working tree and the index, not just the commit
+    assert not os.path.exists(os.path.join(new_dataset.path, 'CHANGES'))
+    repo = pygit2.Repository(new_dataset.path)
+    index_paths = [entry.path for entry in repo.index]
+    assert 'CHANGES' not in index_paths
+    # Untouched files are still present in both
+    assert os.path.exists(os.path.join(new_dataset.path, 'dataset_description.json'))
+    assert 'dataset_description.json' in index_paths
+
+
+def test_delete_directory(client, new_dataset):
+    ds_id = os.path.basename(new_dataset.path)
+    for filename in ('derivatives:LICENSE', 'derivatives:sub-01:stats.tsv'):
+        response = client.simulate_post(
+            f'/datasets/{ds_id}/files/{filename}', body='test content'
+        )
+        assert response.status == falcon.HTTP_OK
+    response = client.simulate_post(
+        f'/datasets/{ds_id}/draft', params={'validate': 'false'}
+    )
+    assert response.status == falcon.HTTP_OK
+    response = client.simulate_delete(
+        f'/datasets/{ds_id}/files', body='{ "filenames": ["derivatives"] }'
+    )
+    assert response.status == falcon.HTTP_OK
+    assert json.loads(response.content)['deleted'] == ['derivatives']
+    assert not os.path.exists(os.path.join(new_dataset.path, 'derivatives'))
+    repo = pygit2.Repository(new_dataset.path)
+    assert not [
+        entry.path for entry in repo.index if entry.path.startswith('derivatives/')
+    ]
+
+
+def test_delete_prunes_emptied_directories(client, new_dataset):
+    """Deleting the last file in a directory should not leave the directory behind."""
+    ds_id = os.path.basename(new_dataset.path)
+    response = client.simulate_post(
+        f'/datasets/{ds_id}/files/sub-01:anat:sub-01_T1w.json', body='{}'
+    )
+    assert response.status == falcon.HTTP_OK
+    response = client.simulate_post(
+        f'/datasets/{ds_id}/draft', params={'validate': 'false'}
+    )
+    assert response.status == falcon.HTTP_OK
+    response = client.simulate_delete(
+        f'/datasets/{ds_id}/files',
+        body='{ "filenames": ["sub-01:anat:sub-01_T1w.json"] }',
+    )
+    assert response.status == falcon.HTTP_OK
+    assert not os.path.exists(os.path.join(new_dataset.path, 'sub-01'))
+
+
+def test_delete_leaves_untracked_files_alone(client, new_dataset):
+    """Only the requested paths are removed, unrelated untracked files survive."""
+    ds_id = os.path.basename(new_dataset.path)
+    untracked = os.path.join(new_dataset.path, 'untracked.txt')
+    with open(untracked, 'w') as f:
+        f.write('not committed')
+    response = client.simulate_delete(
+        f'/datasets/{ds_id}/files', body='{ "filenames": ["CHANGES"] }'
+    )
+    assert response.status == falcon.HTTP_OK
+    assert os.path.exists(untracked)
+
+
+def test_delete_does_not_rewrite_unrequested_paths(client, new_dataset):
+    """A delete only touches the paths it was asked to delete.
+
+    A full checkout of the index would also restore any other tracked file missing from
+    the working tree, so an unrelated delete could rewrite arbitrary amounts of a large
+    dataset as a side effect.
+    """
+    ds_id = os.path.basename(new_dataset.path)
+    missing = os.path.join(new_dataset.path, 'dataset_description.json')
+    os.remove(missing)
+    response = client.simulate_delete(
+        f'/datasets/{ds_id}/files', body='{ "filenames": ["CHANGES"] }'
+    )
+    assert response.status == falcon.HTTP_OK
+    assert not os.path.exists(os.path.join(new_dataset.path, 'CHANGES'))
+    assert not os.path.exists(missing)
+
+
+def test_target_path_rejects_paths_outside_dataset(tmp_path):
+    dataset_root = str(tmp_path)
+    with pytest.raises(ValueError):
+        target_path(dataset_root, '../outside.txt')
+    with pytest.raises(ValueError):
+        target_path(dataset_root, 'sub-01/../../outside.txt')
+    with pytest.raises(ValueError):
+        target_path(dataset_root, '/etc/passwd')
+    with pytest.raises(ValueError):
+        target_path(dataset_root, '.')
+    with pytest.raises(ValueError):
+        target_path(dataset_root, '.git/config')
+    assert target_path(dataset_root, 'sub-01/anat/sub-01_T1w.nii.gz') == os.path.join(
+        dataset_root, 'sub-01/anat/sub-01_T1w.nii.gz'
+    )
+
+
 def test_delete_non_existing_file(client, new_dataset):
     ds_id = os.path.basename(new_dataset.path)
     response = client.simulate_delete(