block for lock during transcript.inplace
#6075
Merged
+10
−2
Conversation
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
aryairani
force-pushed
the
arya/tweak-cache-key
branch
2 times, most recently
from
9046950 to
bf0caff
Compare
aryairani
commented
Dec 21, 2025
aryairani
commented
Dec 21, 2025
.github/workflows/ci.yaml
Outdated
|
|
||
| - name: verify stack ghci startup | ||
| if: runner.os == 'macOS' && steps.cache-ucm-binaries.outputs.cache-hit != 'true' | ||
| if: runner.arch == 'x64' && runner.os == 'Linux' && steps.cache-ucm-binaries.outputs.cache-hit != 'true' |
Contributor
Author
There was a problem hiding this comment.
move this to a different PR?
aryairani
commented
Dec 21, 2025
.github/workflows/ci.yaml
Outdated
| with: | ||
| path: ${{ env.runtime_tests_codebase }} | ||
| key: runtime-tests-codebase-${{env.runtime_tests_causalhash}} | ||
| key: runtime-tests-codebase-${{ matrix.os }}-${{env.runtime_tests_causalhash}} |
Contributor
Author
There was a problem hiding this comment.
not sure whether we need this
aryairani
commented
Dec 21, 2025
.github/workflows/ci.yaml
Outdated
| if: steps.cache-interpreter-test-results.outputs.cache-hit != 'true' | ||
| run: | | ||
| echo "passing=true" >> "${{env.interpreter_test_results}}" | ||
| - name: setup tmate session |
Contributor
Author
There was a problem hiding this comment.
could leave this in for convenience?
aryairani
commented
Dec 21, 2025
|
|
||
| if [ -z "$1" ]; then | ||
| stack build | ||
| stack build --fast |
Contributor
Author
There was a problem hiding this comment.
@dolio any objections to leaving this in? if incurring a rebuild with optimizations disabled is a concern, you can also pass in your already-built unison to this script as $1
aryairani
force-pushed
the
arya/tweak-cache-key
branch
from
4e0c5c2 to
f34fe5f
Compare
aryairani
changed the title
aryairani
changed the title
transcript.inplace
the transcript.inplace runner seems to open the codebase for migrations, then opens it a second time to tun the actual transcript. there seems to be a race condition in which releasing the lock the first time doesn't actually block for the lock to be released; and in fact it doesn't seem to be released in time to acquire the lock the second time, and we were seeing crashes in CI as a result. there were a few other places where it looked like it might be fine and good (though not necessary) to also block for the lock, but i left them out in order to have a more surgical PR.
aryairani
force-pushed
the
arya/tweak-cache-key
branch
from
141f0d1 to
d2ed60f
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Overview
The
transcript.inplacerunner seems to open the codebase for migrations, then opens it a second time to tun the actual transcript. There seems to be a race condition in which releasing the lock the first time doesn't actually block for the lock to be released; and in fact it doesn't seem to be released in time to acquire the lock the second time, and we were seeing crashes in CI as a result. cc @dolioNot sure why this locking issue just started manifesting recently.
What does this change accomplish and why?
Include "before and after" examples if appropriate. (You can copy/paste screenshots directly into this editor.)
List any Github issues that this PR closes, in closing-issues-using-keywords format.
n/a
Implementation approach and notes
Adds a new locking type which prints a message and blocks until it can acquire a lock, if it can't immediately acquire one. Then it uses that new locking type when trying to open the codebase the second time after running migrations before running the transcript.
Interesting/controversial decisions
There were a few other places where it looked like it might be fine and good (though not necessary) to also block for the lock, but I left them out in order to have a more surgical PR.
Test coverage
CI has started passing again
https://github.com/unisonweb/unison/actions/runs/20403439799/job/58629670481?pr=6075
vs
https://github.com/unisonweb/unison/actions/runs/20391436262/job/58601486802?pr=6075
not needed
Loose ends
Not exactly lose ends because they're not related to this issue, but see the remaining open conversations.
Also, like with any PR I'm reminded that CI is too slow, and could maybe use mOrE cAcHiNg
Final checklist
.cabalfiles, make sure thepackage.yamlfiles are up-to-date instead.