fix(xcresult): read a copy of the bundle instead of migrating the caller's - #1184
Open
dfrankland wants to merge 1 commit into
Open
fix(xcresult): read a copy of the bundle instead of migrating the caller's#1184dfrankland wants to merge 1 commit into
dfrankland wants to merge 1 commit into
Conversation
…ler's
`xcresulttool` migrates a bundle that predates `database.sqlite3` in place the
first time it is read. Two things follow, neither of them ours to do:
- an upload writes into a build artifact it was only asked to read, and
- the read fails outright when that directory is not writable:
Error: "database.sqlite3" couldn't be moved because you don't have
permission to access "test4.xcresult".
which is `exit 64` and no JUnit at all, on the read-only artifact mounts CI
systems hand out.
It is also why two concurrent readers of one bundle race to create the same file.
`XCResult::new` now copies the bundle into a `TempDir` and reads that, so the
caller's directory is never written to and never needs to be writable. The copy
is unconditional rather than keyed on whether a migration would happen: sniffing
the format to save a copy trades a correctness guarantee for work we already do
in well under a second on a 64 MB bundle.
The temp directory is held behind an `Arc` because `XCResult` is `Clone` and
`TempDir` is not — every clone shares the copy, and it is removed when the last
one drops.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Merging to
After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Independent bug fix, no stack. Found while working on #1178 but unrelated to it — this is on the shared read path, so it fixes the default path too.
The bug
xcresulttoolmigrates a bundle that predatesdatabase.sqlite3in place the first time it is read:Data+Info.plist)database.sqlite3written into itexit 64, no JUnitSo today an upload either silently writes into a build artifact it was only asked to read, or fails outright when it cannot — and read-only artifact mounts are ordinary in CI. It is also why two concurrent readers of one bundle race to create the same file.
The fix
XCResult::newcopies the bundle into aTempDirand reads that. The caller's directory is never written to and never needs to be writable.The copy is unconditional rather than keyed on whether a migration would happen — sniffing the format to save a copy trades a correctness guarantee for work that takes well under a second on a 64 MB bundle.
The temp dir sits behind an
ArcbecauseXCResultisCloneandTempDiris not: every clone shares the copy, and it is removed when the last one drops.Testing
test_reading_a_bundle_neither_writes_to_it_nor_needs_it_writablemakes an un-migrated bundle read-only, reads it, restores permissions, and asserts both that the read succeeded and that the directory is byte-for-byte unchanged. Dropping the copy fails it witha read-only bundle must still be readable.Full suite: 37 unit, 22 integration.
🤖 Generated with Claude Code