From 3ab90ecd7979f28ad722d4d8908813f7ba4a3202 Mon Sep 17 00:00:00 2001 From: Marc Sturlese Date: Sun, 12 Jul 2026 04:41:18 +0200 Subject: [PATCH 1/2] fix(fetch): make the sidecar collision guard case-insensitive and centralize it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review of the first commit surfaced a residual of the same bug: the .json remap was an exact-string check, but ext_for returns the extension verbatim, so a file named e.g. CONFIG.JSON kept ext .JSON and its content landed at .JSON — the same path as the sidecar .json on a case-folding filesystem (macOS APFS, Windows, Docker Desktop bind mounts), re-triggering the clobber. Fold the disambiguation into a content_name() helper that compares against the sidecar case-insensitively, single-source the sidecar path behind sidecar_name(), and collapse the two sync_once guards into one _is_sidecar_path() neutralization of prev_local. Co-Authored-By: Claude Fable 5 --- pipeline/fetch/src/drive_fetch.py | 54 ++++++++++++++++-------- pipeline/fetch/tests/test_drive_fetch.py | 14 ++++++ 2 files changed, 51 insertions(+), 17 deletions(-) diff --git a/pipeline/fetch/src/drive_fetch.py b/pipeline/fetch/src/drive_fetch.py index b1b0dbb..d04a87d 100644 --- a/pipeline/fetch/src/drive_fetch.py +++ b/pipeline/fetch/src/drive_fetch.py @@ -240,9 +240,31 @@ def ext_for(cfg: Config, d: dict, mime: str) -> tuple[str | None, str]: return None, (os.path.splitext(name)[1] or ".bin") +def sidecar_name(fid: str) -> str: + """The metadata sidecar's filename — the on-disk contract clean reads. Single-sourced.""" + return f"{fid}.json" + + +def content_name(cfg: Config, d: dict, mime: str, fid: str) -> tuple[str | None, str]: + """(export_format|None, on-disk content filename), disambiguated from the sidecar so content + can never overwrite .json. Compared case-insensitively for case-folding filesystems + (macOS APFS, Windows, Docker Desktop bind mounts), where .JSON == .json.""" + fmt, ext = ext_for(cfg, d, mime) + name = f"{fid}{ext}" + if name.lower() == sidecar_name(fid).lower(): + name = f"{fid}.data{ext}" + return fmt, name + + +def _is_sidecar_path(fid: str, name: str | None) -> bool: + """True if `name` is (case-insensitively) the sidecar — i.e. not real content. A manifest + localPath equal to it is a pre-.data.json entry whose content the sidecar write clobbered.""" + return bool(name) and name.lower() == sidecar_name(fid).lower() + + def merge_sidecar_lineage(cfg: Config, fid: str, lineage: dict) -> None: """Adds lineage paths to an existing sidecar without touching the rest of its metadata.""" - sidecar = cfg.raw_dir / f"{fid}.json" + sidecar = cfg.raw_dir / sidecar_name(fid) meta: object = {} if sidecar.exists(): try: @@ -256,13 +278,11 @@ def merge_sidecar_lineage(cfg: Config, fid: str, lineage: dict) -> None: def download_file(cfg: Config, d: dict, fid: str, mime: str, lineage: dict) -> str: - """Downloads to raw/ (atomically) + a metadata sidecar. Returns the local name. - `.json` content is stored as .data.json — .json IS the sidecar.""" - fmt, ext = ext_for(cfg, d, mime) - if ext == ".json": - ext = ".data.json" - out = cfg.raw_dir / f"{fid}{ext}" - tmp = cfg.raw_dir / f".tmp-{fid}{ext}" + """Downloads to raw/ (atomically) + a metadata sidecar. Returns the local name. + The content name is disambiguated from the sidecar (.json) so the two never collide.""" + fmt, name = content_name(cfg, d, mime, fid) + out = cfg.raw_dir / name + tmp = cfg.raw_dir / f".tmp-{name}" args = ["drive", "download", fid, "--out", str(tmp)] if fmt: args += ["--format", fmt] @@ -274,12 +294,12 @@ def download_file(cfg: Config, d: dict, fid: str, mime: str, lineage: dict) -> s meta = meta["file"] if isinstance(meta, dict): meta.update(lineage) - write_atomic(cfg.raw_dir / f"{fid}.json", json.dumps(meta, indent=2, ensure_ascii=False).encode()) + write_atomic(cfg.raw_dir / sidecar_name(fid), json.dumps(meta, indent=2, ensure_ascii=False).encode()) return out.name def remove_local(cfg: Config, entry: dict, fid: str) -> None: - for p in (entry.get("localPath"), f"{fid}.json"): + for p in (entry.get("localPath"), sidecar_name(fid)): if p and (cfg.raw_dir / p).exists(): (cfg.raw_dir / p).unlink() @@ -349,10 +369,12 @@ def sync_once(cfg: Config, folder_id: str) -> dict: "parentPath": f"/{cfg.folder or folder_id}", "parentIds": parents(d), }) - # A localPath equal to the sidecar means the content was clobbered by the sidecar write - # (pre-.data.json state): never treat it as a valid mirror — re-download to heal. - if (prev and prev.get("fingerprint") == fp and prev_local - and prev_local != f"{fid}.json" and (cfg.raw_dir / prev_local).exists()): + # A localPath naming the sidecar (case-insensitive) is a pre-.data.json entry whose content + # the sidecar write clobbered: treat it as absent so the file re-downloads and heals, and so + # the rename-cleanup below never unlinks the freshly written sidecar. + if _is_sidecar_path(fid, prev_local): + prev_local = None + if prev and prev.get("fingerprint") == fp and prev_local and (cfg.raw_dir / prev_local).exists(): # Backfill/refresh path metadata without redownloading unchanged files. touched = False for k, v in lineage.items(): @@ -375,9 +397,7 @@ def sync_once(cfg: Config, folder_id: str) -> dict: continue # a rename or export-format change alters the local name: drop the previous file so its # stale content isn't orphaned in the mirror (deletion later only removes the current name). - # Never unlink .json — that is the sidecar (a healing entry's prev_local points at it). - if (prev_local and prev_local != local and prev_local != f"{fid}.json" - and (cfg.raw_dir / prev_local).exists()): + if prev_local and prev_local != local and (cfg.raw_dir / prev_local).exists(): (cfg.raw_dir / prev_local).unlink() manifest[fid] = { "name": _field(d, "name", "title", default=""), diff --git a/pipeline/fetch/tests/test_drive_fetch.py b/pipeline/fetch/tests/test_drive_fetch.py index 1359888..685535c 100644 --- a/pipeline/fetch/tests/test_drive_fetch.py +++ b/pipeline/fetch/tests/test_drive_fetch.py @@ -262,6 +262,20 @@ def test_sync_once_json_content_does_not_collide_with_sidecar(sync_env): assert manifest["C"]["localPath"] == "C.data.json" +def test_sync_once_uppercase_json_content_does_not_collide_with_sidecar(sync_env): + """The collision is case-insensitive: on a case-folding filesystem (macOS APFS, Windows, + Docker Desktop bind mounts) .JSON and the sidecar .json are the same file, so an + uppercase-extension JSON must also be routed to .data.JSON.""" + cfg, tmp_path, fake = sync_env + fake.items.append({"id": "C", "name": "CONFIG.JSON", "mimeType": "application/json", + "modifiedTime": "t1", "parents": ["folder1"]}) + df.sync_once(cfg, "root") + assert (tmp_path / "C.data.JSON").read_text() == "content-C" + assert json.loads((tmp_path / "C.json").read_text())["id"] == "C" # sidecar not clobbered + manifest = json.loads((tmp_path / "_state.json").read_text())["files"] + assert manifest["C"]["localPath"] == "C.data.JSON" + + def test_sync_once_heals_json_entry_clobbered_by_sidecar(sync_env): """A pre-fix manifest whose localPath points at the sidecar must re-download the content (even with an unchanged fingerprint) and must not delete the sidecar while healing.""" From 14739edd5695d406d75927032daa7bcf748f8829 Mon Sep 17 00:00:00 2001 From: Marc Sturlese Date: Sun, 12 Jul 2026 05:24:18 +0200 Subject: [PATCH 2/2] fix(fetch): decide the sidecar clobber by file identity, not name MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The name-only _is_sidecar_path regressed case-sensitive filesystems (Linux/ext4 in production): a legitimately distinct .JSON content file, which is NOT the sidecar there, was neutralized as clobbered — forcing a spurious re-download and orphaning the old file. Decide by samefile() instead: an exact sidecar-name localPath is always the clobber; a case-variant is the clobber only when it resolves to the same file as the sidecar (case-folding filesystems). Co-Authored-By: Claude Fable 5 --- pipeline/fetch/src/drive_fetch.py | 30 +++++++++++++++++------- pipeline/fetch/tests/test_drive_fetch.py | 17 ++++++++++++++ 2 files changed, 39 insertions(+), 8 deletions(-) diff --git a/pipeline/fetch/src/drive_fetch.py b/pipeline/fetch/src/drive_fetch.py index d04a87d..6ca3506 100644 --- a/pipeline/fetch/src/drive_fetch.py +++ b/pipeline/fetch/src/drive_fetch.py @@ -256,10 +256,24 @@ def content_name(cfg: Config, d: dict, mime: str, fid: str) -> tuple[str | None, return fmt, name -def _is_sidecar_path(fid: str, name: str | None) -> bool: - """True if `name` is (case-insensitively) the sidecar — i.e. not real content. A manifest - localPath equal to it is a pre-.data.json entry whose content the sidecar write clobbered.""" - return bool(name) and name.lower() == sidecar_name(fid).lower() +def _clobbered_by_sidecar(cfg: Config, fid: str, prev_local: str | None) -> bool: + """True if the manifest's content path IS the sidecar file — its bytes were overwritten by the + sidecar write (the pre-.data.json bug), so the entry must re-download to heal. Decided by file + identity, not name: an exact sidecar-name localPath is always the clobber; a case-variant (e.g. + .JSON) is the clobber only on a case-folding filesystem, where it resolves to the same file + as the sidecar — on a case-sensitive one it is a distinct, valid content file that must be kept + (else it would be needlessly re-downloaded and orphaned).""" + if not prev_local: + return False + sidecar = sidecar_name(fid) + if prev_local == sidecar: + return True + if prev_local.lower() != sidecar.lower(): + return False + try: + return (cfg.raw_dir / prev_local).samefile(cfg.raw_dir / sidecar) + except OSError: # content or sidecar missing -> not a live clobber; let the exists() skip re-fetch + return False def merge_sidecar_lineage(cfg: Config, fid: str, lineage: dict) -> None: @@ -369,10 +383,10 @@ def sync_once(cfg: Config, folder_id: str) -> dict: "parentPath": f"/{cfg.folder or folder_id}", "parentIds": parents(d), }) - # A localPath naming the sidecar (case-insensitive) is a pre-.data.json entry whose content - # the sidecar write clobbered: treat it as absent so the file re-downloads and heals, and so - # the rename-cleanup below never unlinks the freshly written sidecar. - if _is_sidecar_path(fid, prev_local): + # A localPath that is really the sidecar file is a pre-.data.json entry whose content the + # sidecar write clobbered: treat it as absent so the file re-downloads and heals, and so the + # rename-cleanup below never unlinks the freshly written sidecar. + if _clobbered_by_sidecar(cfg, fid, prev_local): prev_local = None if prev and prev.get("fingerprint") == fp and prev_local and (cfg.raw_dir / prev_local).exists(): # Backfill/refresh path metadata without redownloading unchanged files. diff --git a/pipeline/fetch/tests/test_drive_fetch.py b/pipeline/fetch/tests/test_drive_fetch.py index 685535c..84b3b45 100644 --- a/pipeline/fetch/tests/test_drive_fetch.py +++ b/pipeline/fetch/tests/test_drive_fetch.py @@ -106,6 +106,23 @@ def test_write_atomic_and_sidecar_merge(tmp_path): assert json.loads((tmp_path / "F.json").read_text()) == {"kept": True, "drivePath": "/X/a.pdf"} +def test_clobbered_by_sidecar_uses_file_identity_not_name(tmp_path, monkeypatch): + """The clobber is decided by file identity, not name: an exact sidecar-name localPath is always + the clobber, but a case-variant (.JSON) is the clobber ONLY on a case-folding filesystem + (where it resolves to the sidecar). On a case-sensitive filesystem it is a distinct valid file + that must be kept, else it is needlessly re-downloaded and orphaned.""" + cfg = _cfg(tmp_path) + (tmp_path / "C.json").write_text("{}") # the sidecar + assert df._clobbered_by_sidecar(cfg, "C", None) is False + assert df._clobbered_by_sidecar(cfg, "C", "C.json") is True # exact name -> always clobber + assert df._clobbered_by_sidecar(cfg, "C", "C.pdf") is False # unrelated content -> never + # case-variant name: outcome follows the filesystem, probed via samefile + monkeypatch.setattr(df.Path, "samefile", lambda self, other: False) # case-sensitive fs + assert df._clobbered_by_sidecar(cfg, "C", "C.JSON") is False # distinct file -> keep it + monkeypatch.setattr(df.Path, "samefile", lambda self, other: True) # case-folding fs + assert df._clobbered_by_sidecar(cfg, "C", "C.JSON") is True # same file -> heal + + # ── folder resolution ──────────────────────────────────────────────────────── def test_resolve_folder_id_explicit_wins(tmp_path): assert df.resolve_folder_id(_cfg(tmp_path, folder_id="explicit-id")) == "explicit-id"