studio: open linked-folder sources in binary mode on windows

[mahiatlinux]

Fixes #8617.

Cause

folder_sync._snapshot opened each linked source with os.O_RDONLY and no O_BINARY, so on Windows the CRT opened it in text mode. Reads then collapse CRLF to LF and stop at the first Ctrl-Z (0x1A), but _copy_exact requires exactly st_size bytes. The short read raised Linked source changed while it was copied, every file landed in the failure list, and the folder surfaced N file(s) could not be indexed - the exact message in the issue.

Python documents the requirement: "on Windows adding O_BINARY is needed to open files in binary mode". backend/auth/storage.py:84 already carries the same guard for the same reason.

Bulk upload was unaffected because it writes the stored file through the upload route and never calls _snapshot. Both paths converge on ingestion.start_ingestion afterwards, so parsing was never the problem.

Scope

Every supported format was affected, not just text:

file	size	bytes readable in text mode
20-page PDF, Flate-compressed	225416	2020
docx (zip container)	36644	75
markdown, CRLF	42	39
txt, CRLF	33	31
html, CRLF	54	53

Any real PDF compresses its streams, so a 0x1A byte appears within the first few KB. A trivial uncompressed PDF happens to survive, which is why a minimal smoke test would miss this.

Fix

Add O_BINARY to the snapshot open flags, guarded with getattr so POSIX is unchanged.

Verification

Verified end to end through the real reconcile_folder path with a folder of PDF, docx, html, md and txt sources. Before the fix the job ends failed, 0 added, 5 failed, with 5 file(s) could not be indexed; after it the job completes, all five map to documents and every search term retrieves. Verification ran on Linux with the Windows CRT text-mode read behaviour reproduced at the file-descriptor layer, since the defect is unreachable natively on POSIX.

The added regression test asserts the flag is passed and that the snapshot is byte-identical to the source, and it fails without the fix. tests/test_rag_linked_folders.py is 80/80, and the RAG store and ingestion suites pass alongside it.

Existing folders left in the error state need one re-sync to clear.

[mahiatlinux]

@codex review

[chatgpt-codex-connector]

Codex Review: Didn't find any major issues. Keep it up!

Reviewed commit: 58edda08b7

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[danielhanchen]

Reviewed this one closely because the change looks obviously right, and I do not think it does anything.

The descriptor is already in binary mode before a single byte is read

_snapshot never reads through the raw fd. It wraps it:

with os.fdopen(fd, "rb", closefd = False) as src, open(target, "xb") as dst:
    _copy_exact(src, dst, before.st_size)

os.fdopen goes to io.open and then to FileIO, and CPython's FileIO constructor forces binary mode on the descriptor it was handed, whether that descriptor came from a path it opened itself or from an integer passed in. From Modules/_io/fileio.c (v3.13.12), after the fd branch and the path branch converge:

#if defined(MS_WINDOWS) || defined(__CYGWIN__)
    /* don't translate newlines (\r\n <=> \n) */
    _setmode(self->fd, O_BINARY);
#endif

Between the os.open on line 1006 and that os.fdopen on line 1025 the fd is only used by os.fstat, and translation mode does not affect fstat. So on Windows there is no CRLF collapse and no stop at Ctrl-Z in this function today, and the exact-size copy cannot short-read for that reason.

The general facts in the description are correct: os_open_impl adds only O_NOINHERIT on Windows and never O_BINARY, and the CRT default is _O_TEXT. They just stop applying the moment the fd goes through FileIO.

What that means for the report

If a user is seeing linked-folder files marked unindexable on Windows, this change will not fix it, and the cause is somewhere else. Worth chasing before this lands, otherwise the issue gets closed against a no-op.

The test asserts the flag, not the behaviour

test_snapshot_opens_the_source_in_binary_mode spies on os.open and then calls real_open(path, flags & ~0x8000), so the actual syscall is identical to pre-PR. The \r\n and \x1a payload is decorative: Path(snapshot).read_bytes() == document.read_bytes() passes with or without the change on Linux. It is a valid change detector for the flag, nothing more.

Small note on the monkeypatch: 0x8000 is unused by any os.O_* on x86-64 Linux, but asm-generic/fcntl.h defines O_LARGEFILE as 0x8000 on 32-bit arches, where the strip in the spy would clear it from every os.open in the process for the duration of the test.

Things I checked that are fine

• os.O_NOFOLLOW does not exist on Windows and os.O_BINARY does not exist on Linux, so both getattr calls fall to 0 on the other platform and POSIX flags are bit-identical to pre-PR.
• O_RDONLY | O_BINARY is a valid _wopen combination, so nothing breaks if this lands as defensive hygiene against a future rewrite to raw os.read.
• Scope: line 1006 is the only os.open in all of studio/backend/core/rag/, and the only read of a file inside a user's linked folder. Everything else reads the already-copied snapshot.

Tests

On the head, Python 3.13, Linux:

studio/backend $ python -m pytest tests/test_rag_linked_folders.py -q
80 passed in 40s

(21 passed / 59 skipped on the first run; the skips were requires_sqlite_vec, and they run once sqlite-vec is installed.)

What I could not check

I have no Windows host. The experiment that would settle it is copying a CRLF plus Ctrl-Z file through the pre-PR _snapshot on Windows and checking the byte count. My reading of fileio.c says it already works, so if you have a machine that reproduces the original failure, that is the thing to run before changing the flags.

Happy to be wrong here if the repro says otherwise, but as it stands the comment on line 1004 describes a defect this code path does not have.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 4e692fff25

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Preserve file identity for Windows reconciliation

When Windows os.scandir supplies st_dev/st_ino as 0, this fallback allows the initial snapshot but the same zero identity is persisted in linked_folder_files. Every later scan therefore compares (0, 0) with (0, 0), so replacing a document with different same-length content while preserving its mtime is never added to work and the index remains stale indefinitely—contradicting the existing test_same_size_same_mtime_inode_replacement_is_reconciled invariant. Obtain a usable identity during _scan (for example through a handle/stat path that supplies the Windows file ID) rather than discarding identity checks here.

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 8e351b7e50

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Fix binary mode at the actual read boundary

On Windows this flag cannot fix the reported short reads: as the new comment notes, os.fdopen(fd, "rb") already switches the CRT descriptor to binary mode before _copy_exact performs any read, so the pre-change implementation reads the same bytes as this one. The added byte-for-byte test likewise exercises that existing fdopen path without asserting the os.open flags and therefore would pass against the parent commit. The linked-folder failure will remain if it is reproducible, so its actual cause/read path needs to be identified and covered by a regression that fails before the fix.

Useful? React with 👍 / 👎.

[danielhanchen]

I took this one over and pushed a commit to the branch. Short version: the original diagnosis was wrong, but the bug is real, I found the actual cause, and it is worse than reported.

Before the PR

On Windows, linking a folder for RAG succeeded and then every file in it came back as "could not be indexed". Bulk upload of the same files worked.

What the PR assumed

That os.open without O_BINARY left the descriptor in CRT text mode, so the copy collapsed CRLF and stopped at Ctrl-Z, short-reading the source.

That is a no-op here, on every interpreter Studio supports. _snapshot never reads through the raw fd, it wraps it:

with os.fdopen(fd, "rb", closefd = False) as src, open(target, "xb") as dst:

os.fdopen goes to io.open then FileIO, and CPython's FileIO constructor forces the descriptor binary on the merged fall-through path, whichever branch produced the fd:

#if defined(MS_WINDOWS) || defined(__CYGWIN__)
    /* don't translate newlines (\r\n <=> \n) */
    _setmode(self->fd, O_BINARY);
#endif

Confirmed at v3.10.14 L475, v3.11.9 L473, v3.12.7 L478, v3.13.1 L489, v3.14.0 L505. Every control transfer between the fd/path fork and that line is a goto error; the only goto done comes after it. CI runs 3.10 to 3.13, so the whole supported range is covered. Between the os.open and the os.fdopen the fd is touched only by os.fstat, which on Windows bypasses the CRT entirely through GetFileInformationByHandle.

The general facts in the description are right, and worth keeping: CPython never sets the CRT _fmode, and os_open_impl adds only O_NOINHERIT on Windows, so a raw os.open fd genuinely does start in text mode. It just stops mattering the moment it goes through FileIO.

The actual cause

os.scandir's DirEntry.stat() returns st_ino == st_dev == 0 on Windows. os.fstat() does not.

_scan records identity from entry.stat(follow_symlinks = False). _snapshot compares that against os.fstat. The tuple can never match, so every file raises "Linked source changed during reconciliation".

• Python docs, os.DirEntry.stat(), unchanged across 3.10 to 3.14: "On Windows, the st_ino, st_dev and st_nlink attributes of the stat_result are always set to zero. Call os.stat() to get these attributes."
• Modules/posixmodule.c, find_data_to_file_info(), byte-identical across those tags: memset(info, 0, sizeof(*info)) then copies only attributes, three timestamps and size. It cannot do better, WIN32_FIND_DATAW has no volume serial or file index.
• CPython's own test suite skips st_dev / st_ino / st_nlink when comparing DirEntry.stat() to os.stat().
• Live repro on Windows 11 24H2 NTFS in gh-126253: DirEntry gives st_ino=0, st_dev=0, os.stat gives real values.
• It is deliberate and will not change. gh-72228: "scandir() is designed for effiency, not for portability or correctness." gh-85278: "I won't accept having to make a second set of system calls on every file."

Four things make this fit #8617 exactly: _root_identity uses os.lstat, not DirEntry, so linking and scanning succeed and only per-file snapshots fail; bulk upload never calls _snapshot; the failure is content-independent, matching the reporter retrying with only .md and .txt; and _scan already guards identity[1] not in (None, 0) in two places, so the zero-inode case was known, it just was not applied in _snapshot.

After the commit I pushed

• O_BINARY stays. It is harmless and it guards a future rewrite to raw os.read. The comment now says that instead of asserting a defect this path does not have.
• The identity check compares 4 fields when the scan recorded a real inode and 2 when it did not, matching the convention already at lines 932 and 954. That also covers os.stat's own zeroing fallback on ERROR_ACCESS_DENIED and ERROR_SHARING_VIOLATION.
• The post-copy check now compares fstat to fstat rather than fstat to the scan metadata. Identical on POSIX, strictly stronger on Windows, and it keeps mid-copy swap detection alive even with no scan identity.
• The test no longer spies on a flag. It asserts byte-for-byte copy behaviour, plus four identity tests, two of which fail without the production fix (verified by disabling only the fix).

Does it break anything

No POSIX behaviour changes. os.O_BINARY does not exist on Linux or macOS so the getattr is 0 and the flags are bit-identical to before. The identity check keeps all four fields wherever the scan produced a real inode, which is every POSIX platform, so the only relaxation happens exactly where the check was returning zeros on both sides anyway.

Simulation

_snapshot driven directly against the awkward cases, Linux, after the fix:

control: POSIX happy path                               OK      copied
1. CRLF file                                            OK      37 bytes byte-identical, CRLF preserved
2. file containing 0x1A (Ctrl-Z)                        OK      20 bytes, nothing truncated at 0x1A
3. zero byte file                                       OK      empty file snapshotted without error
4a. file shrinks mid-copy                               RAISED  Linked source changed while it was copied
4b. file grows after the copy                           RAISED  Linked source changed while it was copied
5a. symlink escaping the root                           RAISED  File escaped the linked folder
5b. symlink within root, POSIX O_NOFOLLOW               RAISED  OSError [Errno 40] Too many levels of symbolic links
5c. symlink within root, Windows-like (no O_NOFOLLOW)   RAISED  Linked source changed during reconciliation
6. coarse (2 second) mtime granularity                  RAISED  Linked source changed during reconciliation
7a. Windows-like scandir identity (0,0) vs real fstat   OK      copied despite the scan recording no identity
7b. Windows-like identity (0,0) on BOTH sides           OK      identity check PASSED with (0,0) both sides
7c. (0,0) identity with the file swapped underneath     OK      snapshot contains BBBB (expected AAAA)

Before the fix, 7a raised "Linked source changed during reconciliation", which is the user-visible failure. Cases 1, 2 and 3 pass identically with the O_BINARY change fully reverted, which is the no-op shown directly.

7b and 7c are the uncomfortable part: with zeros on both sides the old check passed vacuously, and 7c swaps a different file with matching size and mtime underneath and the check still passes. So on Windows that identity check has never carried information. Before this it converted that into rejecting everything; now it degrades honestly to size plus mtime plus the fstat-to-fstat mid-copy check.

Tests: 84 passed in test_rag_linked_folders.py (was 80), 184 passed across that file plus test_warm_window_review_fixes.py.

Found but deliberately not fixed here

• ntpath.realpath can return a \\?\ prefix, and ntpath.splitroot(r"\\?\C:\x") gives ('\\\\?\\C:', ...) versus ('C:', ...), so commonpath raises and _is_within returns False, giving "File escaped the linked folder". Needs long-path analysis on a real Windows box.
• DirEntry reads WIN32_FIND_DATAW.ftLastWriteTime while os.fstat reads FILE_BASIC_INFO.LastWriteTime. Microsoft's File Times notes the last write time is not fully updated until every writing handle is closed, and on FAT the two disagree by an hour across DST. Still-open gh-85278. Loosening the mtime equality is a design call, not mine to make unilaterally.
• Sharing violations arrive as PermissionError with .winerror is None, not WinError 32, which matters if retry logic gets added.
• Diagnosability: the bare except Exception logs the cause server-side but stores only "N file(s) could not be indexed", and every candidate above collapses to that same string. Recording the per-file error text would have made this a five minute diagnosis instead of a day.

What I could not verify

I have no Windows host. The chain is docs plus CPython source across five tags plus a maintainer's own repro in the tracker plus a Linux simulation of the metadata shape. Strong, but not the same as running it.

@aardvarkpaul, if you still have the install: the Studio backend log lines matching linked-folder ingestion failed would settle this completely. That one line separates the identity mismatch from the \\?\ path, and both look identical in the UI.