Skip to content

Commit e11ae37

Browse files
jstar0claude
andcommitted
fix: stop a uintptr round-trip and a racy ordering check failing CI
Both only appear where this machine cannot look: the checkptr crash is in a Linux-only file that does not compile on Darwin, and the rollback race needs CI's scheduling to lose. Local `go test ./...` on Darwin passes either way, which is why the first tag push reached CI before they were found. The Linux session-deletion ioctl seam took its argument as a uintptr, so the production caller converted a pointer to an integer and the test stub converted it back. A uintptr that crosses a function boundary is not a valid pointer and `checkptr`, which `-race` enables, rejects converting it back. The seam now carries an unsafe.Pointer and only the syscall expression itself holds the address as an integer, where the compiler keeps the object alive. The rollback test proved "rollback did not return early" with a non-blocking receive, which assumes the restoring goroutine's channel send happens before rollback returns. It does not: rollback waits on the acknowledgement file, and the send is the statement after writing it. The goroutine now marks the route restored before publishing that acknowledgement, so reading the flag after rollback returns is a real ordering check, and the channel read has a bounded wait for the goroutine's outcome. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent 6bddeea commit e11ae37

3 files changed

Lines changed: 27 additions & 10 deletions

File tree

‎internal/cli/fs_test.go‎

Lines changed: 13 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ import (
1313
"reflect"
1414
"runtime"
1515
"strings"
16+
"sync/atomic"
1617
"syscall"
1718
"testing"
1819
"time"
@@ -3729,6 +3730,7 @@ func TestFSRollbackCanonicalFailureWaitsForManagedRouteRestoration(t *testing.T)
37293730
retirementRequest := filepath.Join(stateDirectory, "retire.request.json")
37303731
canonicalRoute := "/sessions/2026/07/12/" + filename
37313732
restored := make(chan error, 1)
3733+
var routeRestored atomic.Bool
37323734
go func() {
37333735
deadline := time.Now().Add(5 * time.Second)
37343736
for time.Now().Before(deadline) {
@@ -3775,6 +3777,12 @@ func TestFSRollbackCanonicalFailureWaitsForManagedRouteRestoration(t *testing.T)
37753777
restored <- err
37763778
return
37773779
}
3780+
// Mark the route restored before publishing the acknowledgement
3781+
// that rollback is waiting on. Rollback cannot observe that
3782+
// acknowledgement, and so cannot return, until after this flag is
3783+
// set — which is what makes reading it below a real ordering check
3784+
// rather than a race against this goroutine's next statement.
3785+
routeRestored.Store(true)
37783786
restored <- writeMountAcknowledgement(storeDir, "session", state.Generation, canonicalRoute)
37793787
return
37803788
}
@@ -3795,13 +3803,16 @@ func TestFSRollbackCanonicalFailureWaitsForManagedRouteRestoration(t *testing.T)
37953803
if !errors.Is(err, errRetirementRejected) {
37963804
t.Fatalf("rollback error = %v, want retirement rejection", err)
37973805
}
3806+
if !routeRestored.Load() {
3807+
t.Fatal("rollback returned before the managed route became readable again")
3808+
}
37983809
select {
37993810
case restoreErr := <-restored:
38003811
if restoreErr != nil {
38013812
t.Fatal(restoreErr)
38023813
}
3803-
default:
3804-
t.Fatal("rollback returned before the managed route became readable again")
3814+
case <-time.After(10 * time.Second):
3815+
t.Fatal("the restoring goroutine never reported its outcome")
38053816
}
38063817
state, err := managedState(storeDir, "session")
38073818
if err != nil {

‎internal/vfs/session_deletion_object_linux.go‎

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -121,7 +121,7 @@ func readSessionDeletionGenerationLinux(file *os.File) (uint64, error) {
121121
errno := sessionDeletionIoctlLinux(
122122
file.Fd(),
123123
sessionDeletionFSIOCGetVersionRequestLinux(),
124-
uintptr(unsafe.Pointer(&version)),
124+
unsafe.Pointer(&version),
125125
)
126126
runtime.KeepAlive(file)
127127
if errno != 0 {
@@ -134,8 +134,14 @@ func readSessionDeletionGenerationLinux(file *os.File) (uint64, error) {
134134
return generation, nil
135135
}
136136

137-
func invokeSessionDeletionIoctlLinux(fileDescriptor uintptr, request uintptr, argument uintptr) unix.Errno {
138-
_, _, errno := unix.Syscall(unix.SYS_IOCTL, fileDescriptor, request, argument)
137+
// invokeSessionDeletionIoctlLinux takes the argument as a pointer rather than a
138+
// uintptr so no caller has to round-trip one back into a pointer. Only the
139+
// syscall itself may hold the address as an integer, and only inside the call
140+
// expression, where the compiler keeps the object alive; a uintptr that crosses
141+
// a function boundary is not a valid pointer and `checkptr` rejects converting
142+
// it back.
143+
func invokeSessionDeletionIoctlLinux(fileDescriptor uintptr, request uintptr, argument unsafe.Pointer) unix.Errno {
144+
_, _, errno := unix.Syscall(unix.SYS_IOCTL, fileDescriptor, request, uintptr(argument))
139145
return errno
140146
}
141147

‎internal/vfs/session_deletion_object_linux_test.go‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -145,11 +145,11 @@ func TestReadSessionDeletionGenerationLinux(t *testing.T) {
145145
t.Cleanup(func() { sessionDeletionIoctlLinux = oldIoctl })
146146

147147
t.Run("unsigned generation", func(t *testing.T) {
148-
sessionDeletionIoctlLinux = func(fd uintptr, request uintptr, argument uintptr) unix.Errno {
148+
sessionDeletionIoctlLinux = func(fd uintptr, request uintptr, argument unsafe.Pointer) unix.Errno {
149149
if fd != file.Fd() || request != sessionDeletionFSIOCGetVersionRequestLinux() {
150150
t.Fatalf("unexpected ioctl request: fd=%d request=%#x", fd, request)
151151
}
152-
*(*int32)(unsafe.Pointer(argument)) = -1
152+
*(*int32)(argument) = -1
153153
return 0
154154
}
155155
generation, err := readSessionDeletionGenerationLinux(file)
@@ -159,15 +159,15 @@ func TestReadSessionDeletionGenerationLinux(t *testing.T) {
159159
})
160160

161161
t.Run("unsupported", func(t *testing.T) {
162-
sessionDeletionIoctlLinux = func(uintptr, uintptr, uintptr) unix.Errno { return unix.ENOTTY }
162+
sessionDeletionIoctlLinux = func(uintptr, uintptr, unsafe.Pointer) unix.Errno { return unix.ENOTTY }
163163
if _, err := readSessionDeletionGenerationLinux(file); err == nil || !errors.Is(err, unix.ENOTTY) {
164164
t.Fatalf("readSessionDeletionGenerationLinux() error=%v, want ENOTTY", err)
165165
}
166166
})
167167

168168
t.Run("zero", func(t *testing.T) {
169-
sessionDeletionIoctlLinux = func(_ uintptr, _ uintptr, argument uintptr) unix.Errno {
170-
*(*int32)(unsafe.Pointer(argument)) = 0
169+
sessionDeletionIoctlLinux = func(_ uintptr, _ uintptr, argument unsafe.Pointer) unix.Errno {
170+
*(*int32)(argument) = 0
171171
return 0
172172
}
173173
if _, err := readSessionDeletionGenerationLinux(file); err == nil {

0 commit comments

Comments
 (0)