Repository navigation
fix(core): fall back to a direct write when link() is denied with EACCES - #7474
Conversation
miguel-heygen
left a comment
There was a problem hiding this comment.
Independent review at 174d183cd99bf694bdace253931615ed3236f5fc. This is a COMMENT review because this account opened the PR.
(1) Is EACCES safe to treat as "no hard links" at both sites? Yes.
- Both sites write their source into the same folder just before linking:
createFileAtomicallywrites its temp sibling (atomicFile.ts:135).claimwrites${file}-<pid>-<uuid>.tmpbeside the lock (ownerLock.ts:114-115).
- So an EACCES from
link()there can't be the usual "no write or search permission on the folder". On Linux that case has already passed, andprotected_hardlinksanswers EPERM, not EACCES. What's left is a policy denial, which is what SELinux on Termux does (issue #7473:ln a b→ Permission denied). - Exclusivity doesn't depend on the error code:
- Both fallbacks open with
wx(O_CREAT|O_EXCL):createDirectlyatatomicFile.ts:85, andcreateExclusivelyatownerLock.ts:133. - An existing file still raises EEXIST. In
claim, EEXIST fromlink()is still checked first, at:121. - A folder that really is unwritable fails at the
wxopen with its own EACCES, so the person still sees the error.
- Both fallbacks open with
- What the fallback gives up is the same as for EPERM and ENOTSUP today: a reader can briefly see a partial file.
ownerLockalready covers this withbeingTaken(a size-0 file younger thanTAKING_MS). - Windows:
WINDOWS_BUSYincludes EACCES, butbusyOnWindowsonly wraps the outer retry at:227. A Windows EACCES fromlink()now goes throughcreateExclusively, which either takes the lock, returns false on EEXIST, or throws back into the same busy retry. So that behaviour doesn't change. - Both of the issue's failures go through these two sites:
initwriteshyperframes.jsonviacreateFileAtomically;recordRenderSuccesswritesowner.pidviatakeHistoryOwnership(videoState.ts:113).
(2) The other hard-link sites
-
engine/src/services/videoFrameExtractor.ts:2010andproducer/src/services/distributed/plan.ts:1158: a barecatchfalls back tocopyFileSync. Fine on Termux. -
producer/src/services/distributed/planV2Publisher.ts:36-40:canFallbackToCopyalready includes EACCES. Fine. -
share/src/projectLock.mjs:64-70is the same contract as these two sites (link a candidate, on a no-hard-links codeopen(lock, "wx")), and its list lacks EACCES. Important for consistency. Nothing in this repo imports@hyperframes/shareon Android, so it doesn't reach the issue's path. But the lists have already drifted: core has EXDEV and EISDIR, ownerLock has ENOSYS, and share has EINVAL. One exportedNO_HARD_LINKSin core, used by all three, would stop the next code from landing in only one place. -
engine/src/utils/urlDownloader.ts:986: only EEXIST is handled, so on Termux any render with an http(s) media source fails at publish with EACCES. The same is already true on FAT and exFAT with EPERM. Not blocking for this PR, but worth an issue. It can't take this PR's one-line fix:- its publish has to be atomic and no-clobber, because the losing caller trusts any existing non-empty file (
race_reused,:999-1000); - a direct
wxwrite would let a concurrent caller reuse a half-written download.
A fallback there needs its own design, for example
wxto a sibling plus a completion marker, or a lock around a copy. - its publish has to be atomic and no-clobber, because the losing caller trusts any existing non-empty file (
(3) Do the tests pin the change? Yes.
- With EACCES removed from
atomicFile.ts, onlywrites directly on a volume without hard links (EACCES)fails (1 of 39). - With EACCES removed from
ownerLock.ts, onlytakes the lock when linking fails with EACCESfails (1 of 22). - Locally the full files pass:
atomicFile.test.ts38 passed / 1 skipped,ownerLock.test.ts22/22. - The EEXIST and live-owner refusal tests use other codes. The fallback path doesn't depend on the code, so that coverage carries over to EACCES.
CI: important.
- The Comments check fails:
comment-ratchet.mjsreports that the comment share rose in both files (atomicFile.tsfrom 17/205 to 18/206,ownerLock.tsfrom 12/244 to 13/245). It isn't in main's required list, but it's red and the PR body doesn't explain it. - A fix that adds no comment line:
- in
atomicFile.ts, fold Android into the existing line 10 ("…FAT, exFAT, some network shares, Android app data…"); - in
ownerLock.ts, drop the new line, since the commit message and the tests already carry the reason.
- in
- CodeQL is neutral (a missing configuration, not an alert).
- At 01:32 UTC: 78 success, 11 skipped, 3 still running, and Comments is the only failure. The required Build, Typecheck, Test, Test reachability, Test: runtime contract, Render on windows-latest, Studio and player captures, Studio: edit accuracy gate and Semantic PR title all pass. Skills: manifest in sync is skipped. regression and Tests on windows-latest were still running.
Verdict: COMMENT (would approve once the Comments check is green)
Reasoning: EACCES is safe to treat as "no hard links" at both sites, because the wx fallback keeps exclusivity and the tests pin both additions. The red comment-ratchet check is the only thing to fix here. The share lock list and urlDownloader are follow-ups.
miguel-heygen
left a comment
There was a problem hiding this comment.
Re-review at 05bd36e902ce5f5234aa5155052d8dbed00fd968 (one commit on top of 174d183cd, which I reviewed above). This is a COMMENT review because this account opened the PR. I'd approve this head.
What changed since my last review, and checks
packages/core/src/atomicFile.ts:10: the Android note is now part of the existing comment line instead of a new one.ownerLock.tsloses its added comment, and theEACCESentry stays atownerLock.ts:19. That was the cause of the red "Comments" check; it is green at this head.packages/share/src/projectLock.mjs:67:EACCESis added to the no-hard-links list. This was my consistency note. The fallback is still exclusive:open(lock, "wx")at:72, and anEEXISTfromlink()is still not in the list, so it is still thrown.packages/share/tests/kernel.test.mjs:338:EACCESis added to the fallback loop. I removedEACCESfromprojectLock.mjsand exactly the new case failed (exclusive-create fallback … with EACCES, 5/6 pass). With it restored,kernel.test.mjspasses 22/22.urlDownloader.ts:986and merging the three code lists into one are listed in the body as follow-ups. That works for me; neither blocks this PR.
CI
- The current run at this head is green: 86 success, 13 skipped, 0 failures. That includes Comments, Build, Typecheck, Lint, Format, CLI smoke (required), Test (cli 1/2, 2/2, runtime, studio, support), regression (all 10 shards), and Tests on windows-latest (all 5 lanes).
- The one red
Testat this head is in an earlier run that was cancelled when the new one started. It failed only because itsDetect changesstep was cancelled; it does not reflect the code.
Verdict: APPROVE
Reasoning: The new commit fixes the one red check and closes the sibling-site gap from my last review. The new share case is pinned by a test that fails without the change, and the remaining items are tracked as follow-ups.
On Android app data (Termux, SELinux enforcing), link(2) fails with EACCES in a folder the temp file was just written to. createFileAtomically() and the studio history owner lock treated that as fatal, so `hyperframes init` left a half-scaffolded project and every render logged a recordRenderSuccess failure. Both now take their existing exclusive direct-write path, as they already do for EPERM. Fixes #7473
Same Termux/SELinux link() denial as core and the studio history lock: the share lock's link-then-exclusive-create fallback now covers it too. Folds the Android note into the existing comment so no file's comment share rises.
05bd36e to
60d723d
Compare
There was a problem hiding this comment.
Approve at 60d723dd. Nothing blocks this.
- The
fails without the fix:line is present:atomicFile.test.ts›writes directly on a volume without hard links (EACCES). The studio-server side has its own case pinning theownerLock.tsentry. - It's safe to treat
EACCESas "no hard links" at both sites:- Each site writes its temp or draft file into the same folder just before
link(), so anEACCESthere can't be a plain directory-permission error. - Both fallbacks still open with
wx(atomicFile.ts:84,ownerLock.ts:132), so exclusivity is kept and an existing file still raisesEEXIST. - In
claim,EEXISTfromlink()is still checked before the list (ownerLock.ts:120). - A folder that really can't be written still fails at the
wxopen.
- Each site writes its temp or draft file into the same folder just before
- Windows behaviour is unchanged.
WINDOWS_BUSYonly wraps the outer retry, and thewxpath either takes the lock, returns false, or throws back into that retry. - The rebase drops the share change correctly.
projectLock.mjs:136on main already listsEACCES. The branch is 0 behind main. - Every check on this head passes, across both CI runs: CI, regression, Windows render, Comments, CodeQL, captures, perf.
- The
urlDownloader.ts:986gap and merging the three code lists into one remain tracked follow-ups.
What
createFileAtomically()(core) and the studio history owner lock (ownerLock.ts) now treatEACCESfromlink()the same way they treatEPERM: as "this folder doesn't allow hard links". Both then use their existing exclusive direct write (wx).Why
On Android app data (Termux, SELinux enforcing),
link(2)is denied withEACCESeven in a folder where the temp file was just written. Both sites treated it as fatal:hyperframes initaborted halfway.index.htmlandmeta.jsonwere written, buthyperframes.jsonandpackage.jsonwere not.Post-render step 'recordRenderSuccess' failed … link …/video-state-lock/owner.pid-…tmp -> owner.pid.Related work
Fixes #7473 (thanks @rdsdevcoorp for the exact diagnosis, including the second bundled copy).
How
Rebased on current
main. The change is now 4 files, +5 -5:packages/core/src/atomicFile.ts: addEACCEStoNO_HARD_LINKS, and name Android app data in the comment above it.packages/core/src/atomicFile.test.ts: addEACCESto the no-hard-links cases.packages/studio-server/src/history/ownerLock.ts: addEACCESto its ownNO_HARD_LINKS. This is the lock path from the report's second error.packages/studio-server/src/history/ownerLock.test.ts: addEACCESto its no-hard-links cases.The share lock (
packages/share/src/projectLock.mjs) and its test already treatEACCESas no hard links onmain, so this PR no longer touches them.Both callers have just written the temp file or draft into the same folder, so an
EACCESfromlink()means links are refused there, not that the folder is unwritable. The direct path stays exclusive: an existing file still raisesEEXIST, and a truly unwritable folder still raisesEACCESfromopen.I checked every other
linkSyncsite.videoFrameExtractoranddistributed/planalready fall back to a copy on any error.engine/src/utils/urlDownloader.ts:986has no fallback for any no-hard-link code. That is a separate, pre-existing gap and is left for a follow-up: losing callers there reuse any non-empty file, so a direct write needs its own design. The no-hard-link code lists have also drifted apart; merging them into one export from core is a separate follow-up.Size
Under 100 lines: one word (
EACCES) added to two code lists, plus the two tests that list the same codes.Test plan
EACCESis added to the no-hard-links cases in the core and studio-server suites.EACCESremoved from core'sNO_HARD_LINKS,packages/core/src/atomicFile.test.tsfailswrites directly on a volume without hard links (EACCES)(1 failed, 37 passed, 1 skipped). Restored, it passes.atomicFile.test.ts38 passed (1 skipped),ownerLock.test.ts22/22, and the share suite'skernel.test.mjs31/31.