[Bug]: Filesystem storage reads require write access #3802

Activity

1. saikpr

   changed the title [-]Filesystem storage reads require write access[/-] [+][Bug]: Filesystem storage reads require write access[/+] on Nov 8, 2026
2. yjjnls commented on Nov 11, 2026

   

   yjjnls

   on Nov 11, 2026

   More actions

   Confirming the diagnosis against current main. All three getHandle call sites are read paths — WriteTo, openRangeReader (a SectionReader over the handle), and Size — and all pass checkExistence=true, so os.Stat has already verified a regular file exists before the open. Nothing creates through getHandle: the write paths go through replaceFrom -> replaceFile (MkdirAll + CreateTemp + rename), the compressed uploader writes its own parts, and the uncompressed-size sidecar is read with os.ReadFile. So O_CREATE in getHandle is unreachable-as-creation today and O_RDWR buys nothing on those paths — O_RDONLY is behavior-identical on writable mounts and fixes read-only mounts, where WriteTo/Size/range reads currently fail at open() with EROFS. A read-only-mounted template store is a real topology (template objects replicated read-only to consumer nodes), so this is worth fixing as described.

   @saikpr the scope you outline is exactly right — please go ahead with the PR; happy to review. One test note: a chmod-based "read-only file" unit test is silently defeated when tests run as root (O_RDWR succeeds on a 0444 file), so for CI-coverable proof either assert the flags passed to OpenFile (tiny refactor: inject the flag into getHandle) or gate an EROFS case on a read-only tmpfs mount and skip it elsewhere. A regression asserting that read paths succeed against a read-only-mounted directory is the most direct coverage for the reported topology.
3. ryan-sana commented on Nov 13, 2026

   

   ryan-sana

   on Nov 13, 2026

   Contributor

   More actions

   Hey @saikpr, thanks for digging into this! You're right. Those write flags are left over from when writes also went through getHandle. Writes now use temp file + rename, so only the reads use it, and they don't need write access.

   Yes please, go ahead with the PR. Here's what we'd love to see:

   • Open with plain os.Open (read-only) in getHandle
   • Do the directory check with Stat() on the opened file instead of os.Stat beforehand, and map "not exist" from the open to ErrObjectNotExist. That way, a file deleted between the check and the open can't come back as an empty file through O_CREATE.
   • Drop the checkExistence parameter, since every caller passes true
   • Tests:
     • WriteTo, Size and a range read all work on a read-only file in a read-only directory. Skip this one when running as root, since root ignores file modes.
     • A missing object still returns ErrObjectNotExist

   A small PR touching just the storage file and its test is perfect. Make sure make test and make lint pass. Happy to review once it's up 🙌
4. saikpr commented on Nov 13, 2026

   

   saikpr

   on Nov 13, 2026

   Author

   More actions

   #3830 opened :)