The DID-matlab#191 guard poisons the file cache: after it fires, that member reads as empty bytes forever #73

Found while writing the read-side tests for #68. _open_series_member's guard against a handler that returns the manifest's own bytes causes, on the way to refusing, exactly the lasting silent corruption it exists to prevent — for the same member uid, permanently, with no error at any later read.

What happens

_fetch_remote_to_cache does not hand back the temporary download. It moves the file into the file cache under the member's uid and returns the path inside the cache:

cached = self._add_to_file_cache(dest_path, uid)   # cache.add_file(source_path, name)
if cached is not None:
    ...
    return cached, None

_open_series_member then applies the #191 comparison and, on a match, deletes that path with a raw os.remove:

if f_fetched.read() == f_manifest.read():
    with contextlib.suppress(OSError):
        os.remove(fetched_path)          # <- removes the cache's copy
    failures.append(... "handler returned the manifest's own bytes" ...)
    continue

The file is gone; the cache's index entry is not. So cache.is_file(member_uid) stays True forever, pointing at a path that no longer exists.

Every later open of that member then takes this route:

1. the local-first check, cached_path_for_uid, correctly returns None — it looks at the filesystem;
2. _fetch_remote_to_cache takes the fetch lock and re-checks the cache index — cache.is_file(uid) is True, so it returns the stale path without ever calling the handler;
3. back in _open_series_member, os.path.getsize(fetched_path) raises OSError, fetched_size becomes None, the #191 comparison is skipped as "comparison unavailable; accept the fetch";
4. ReadOnlyFileobj is handed back over a path that does not exist. Fileobj.fopen() swallows the OSError, so the caller reads b"" and sees no error at all.

That last step is the failure mode this file's own comment names, twenty lines above the guard: "Raise rather than hand back a Fileobj over a path that does not exist: Fileobj.fopen() swallows the OSError, so a caller that does not check it reads b'' and sees no error at all."

Reproduction

Store a demoSeries document whose manifest location is remote, evict the ingested members, then fetch one member with a handler that resolves the manifest location instead of the member uid — the exact mistake #191 anticipates. Then retry with a correct handler:

refused: DID:SQLITEDB:FileSeries:HandlerReturnedManifest
cache.is_file(member_uid): True
that path exists on disk: False
cached_path_for_uid: None
handler called on the retry: 0
bytes read back: b''

The correct handler is never called, and the member reads empty from then on. A cache clear is the only recovery, and nothing tells the caller one is needed.

Scope

Specific to _open_series_member. The ordinary-file read path also goes through _fetch_remote_to_cache, but nothing deletes its result afterwards, so the cache stays consistent there. An ordinary failed fetch (handler raises, or writes nothing) is also safe: _fetch_remote_to_cache deletes its partial before the cache add, so no index entry is ever created.

Suggested fix

Remove the rejected bytes through the cache rather than around it, so the index goes with the file:

cache = self._file_cache()
if cache is not None and cache.is_file(member_uid):
    cache.remove_file(member_uid)
else:
    with contextlib.suppress(OSError):
        os.remove(fetched_path)

Two things worth fixing alongside, both visible from the same trace:

• fetched_size is None should not mean "accept". When os.path.getsize on a file that was just fetched raises, the fetch did not produce a usable file; treating that as "comparison unavailable, accept the fetch" is what turns a stale cache entry into a silent empty read. Failing the fetch there would have surfaced this.
• _open_series_member should not return a ReadOnlyFileobj over a path it has not confirmed exists, for the reason its own neighbour comment gives.

Effect on the tests for #68

The read-side PR asserts that the guard fires with HandlerReturnedManifest and that the manifest stays readable as an ordinary file. The test that a later correct fetch still works is the one that found this, and it is left out of that PR rather than inverted to assert the broken behaviour — it should go in as the regression test when this is fixed.