Skip to content

filememoize cache collides across rerooted trees in parallel recursive validation #391

Description

@yarikoptic-gitmate

Summary

filememoize in src/utils/memoize.ts keys its cache by
(file.parent.path, file.path). After rerootTree (in src/files/filetree.ts),
every nested tree's root dataset_description.json ends up with the
same key — parent.path = "/", file.path = "/dataset_description.json".

When two or more nested BIDS datasets are validated in parallel (via
Promise.allSettled in validate()), whichever fires its loadJSON
first populates the cache entry; the others await the same cached
promise and receive that first file's contents. Their own
dataset_description.json is never read, and subsequent rules that
depend on context.json (e.g. the DatasetType enum check, any
dataset-description-scoped field validation) apply to the wrong file.

This is independent of — and pre-exists — the recent -r expansion
that discovers nested datasets under rawbids/ and sourcedata/
(#390, which
surfaced it). It also affects multi-derivative recursion (e.g. a
dataset with both derivatives/fmriprep/ and derivatives/mriqc/
whose dataset_description.json files differ) in exactly the same way.

Reproducer

# Root (DatasetType=study), with two nested raw BIDS datasets;
# rawbids/ has an intentionally invalid DatasetType ("Raw"), sourcedata
# is valid ("raw").
mkdir -p /tmp/fmcollide/rawbids /tmp/fmcollide/sourcedata/ds00003

cat > /tmp/fmcollide/dataset_description.json <<EOF
{ "Name": "root", "BIDSVersion": "1.10.0", "DatasetType": "study" }
EOF
cat > /tmp/fmcollide/rawbids/dataset_description.json <<EOF
{ "Name": "rawbids", "BIDSVersion": "1.10.0", "DatasetType": "Raw" }
EOF
cat > /tmp/fmcollide/sourcedata/ds00003/dataset_description.json <<EOF
{ "Name": "ds00003", "BIDSVersion": "1.10.0", "DatasetType": "raw" }
EOF

for i in 1 2 3 4 5; do
  ./local-run --ignoreNiftiHeaders -r --json \
    --schema https://bids-specification.readthedocs.io/en/latest/schema.json \
    /tmp/fmcollide 2>&1 | python3 -c "
import json, sys
r = json.load(sys.stdin)
for name, sub in (r.get('derivativesSummary') or {}).items():
    errs = sum(1 for i in sub.get('issues', {}).get('issues', [])
               if i.get('code') == 'JSON_SCHEMA_VALIDATION_ERROR')
    print(f'{name}: {errs}', end='  ')
print()
"
done

Expected

rawbids reports exactly 1 JSON_SCHEMA_VALIDATION_ERROR on
DatasetType every run; sourcedata/ds00003 reports 0 every run.

Observed (5 back-to-back runs, unchanged inputs)

/rawbids/: 1         /sourcedata/ds00003/: 0
/sourcedata/ds00003/: 0   /rawbids/: 0
/rawbids/: 1         /sourcedata/ds00003/: 1     # false positive on sourcedata
/sourcedata/ds00003/: 0   /rawbids/: 0
/sourcedata/ds00003/: 0   /rawbids/: 0

Three distinct outcomes depending on which parallel loadJSON wins the
cache race:

  • rawbids wins → its own "Raw" content used; real error on rawbids only.
  • sourcedata wins → its "raw" content used for both; errors suppressed
    on rawbids.
  • partial → both report errors because both see rawbids's "Raw",
    producing a false positive on sourcedata/ds00003.

Root cause

src/utils/memoize.ts, filememoize:

const subcache = cache.get(file.parent.path)     // e.g. "/"
const key = `${file.path}:${args.join(',')}`     // e.g. "/dataset_description.json:"

rerootTree strips the newRoot prefix from paths, so files at the root of
any rerooted tree share the same (parent.path, file.path) pair. The
cache cannot distinguish them.

Suggested fix

Key the cache by the underlying BIDSFile.opener reference (or a
unique, opener-provided identifier), which is preserved across
rerooting:

  • FsFileOpener has an absolute this.path = join(datasetPath, path)
    unique per physical file.
  • HTTPOpener.url — unique.
  • BrowserFileOpener.file — unique File reference.

A WeakMap<FileOpener, ...> keyed by file.opener would give correct
per-file memoization without path collisions.

Alternatively, add an abstract uniqueId() to FileOpener and key on
that string.

Scope

  • Confirmed with a synthetic reproducer on main (and on
    bf-recurse). Existing tests do not exercise the parallel
    multi-nested case.
  • Fix is non-trivial because it changes a widely-used memoization helper
    (loadJSON, loadTSV, loadTSVGZ, readBytes, readText are all
    filememoize-wrapped). Callers that clear the cache by
    fileTree.path (src/schema/walk.ts) would need to adapt.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions