diff --git a/internal/tools/releasepreflight/npm_absence_branch_test.go b/internal/tools/releasepreflight/npm_absence_branch_test.go index 23bad275..cb9ca221 100644 --- a/internal/tools/releasepreflight/npm_absence_branch_test.go +++ b/internal/tools/releasepreflight/npm_absence_branch_test.go @@ -71,6 +71,7 @@ type npmBranchCase struct { wantLimit string delayedChildStartup bool lateReadyObservation bool + pendingSignalAtBlock bool removeCleanupWait bool } @@ -145,25 +146,49 @@ func testNPMAbsenceBranchExecution(t *testing.T) { t.Fatalf("dispatch writer did not close after one permit: n=%d err=%v", n, err) } }) + t.Run("carrier/signal precedes its pipe close", func(t *testing.T) { + // The signal sender observes the pipe: a close before the signal is + // refused on every run, not only when Bash happens to read EOF first. + control := newNPMBranchControl(t) + sent := errors.New("fixture signal result") + atSignal := errors.New("signal sender not called") + err := control.signalCarrier(func() error { + atSignal = npmBranchSignalPipeOpen(control.signalReader) + return sent + }) + if atSignal != nil { + t.Fatalf("signal-sent pipe at the signal: %v", atSignal) + } + if !errors.Is(err, sent) { + t.Fatalf("cancellation result=%v, want the signal sender's result", err) + } + if n, err := control.signalReader.Read(make([]byte, 1)); n != 0 || err != io.EOF { + t.Fatalf("signal-sent pipe did not close after the signal: n=%d err=%v", n, err) + } + }) t.Run("carrier/stdio bound", func(t *testing.T) { item := cases[0] item.wantLimit = "stdio" executeNPMAbsenceBranch(t, header+"printf '%17000s' x\n", body, item) }) + pendingSignal := "signal pending when the carrier blocks" for _, startup := range []struct { name string delayedChildStartup bool lateReadyObservation bool + pendingSignalAtBlock bool }{ {name: "immediate"}, {name: "delayed child startup", delayedChildStartup: true}, {name: "delayed readiness observation", lateReadyObservation: true}, + {name: pendingSignal, pendingSignalAtBlock: true}, } { t.Run("carrier/blocked entrypoint cancellation/"+startup.name, func(t *testing.T) { item := cases[0] item.wantLimit = "context" item.delayedChildStartup = startup.delayedChildStartup item.lateReadyObservation = startup.lateReadyObservation + item.pendingSignalAtBlock = startup.pendingSignalAtBlock result := executeNPMAbsenceBranch(t, header, body, item) if result.orderingError != nil { t.Fatal(result.orderingError) @@ -173,18 +198,21 @@ func testNPMAbsenceBranchExecution(t *testing.T) { } }) } - t.Run("carrier/removed cleanup wait counterexample", func(t *testing.T) { - item := cases[0] - item.wantLimit = "context" - item.removeCleanupWait = true - result := executeNPMAbsenceBranch(t, header, body, item) - if result.orderingError == nil || result.orderingError.Error() != "carrier cleanup did not wait for classifier exit" { - t.Fatalf("removed-wait rejection=%v, want live-classifier cause", result.orderingError) - } - if result.trace != "view\nabsent\ncancelled\nreaped\n" || result.exitCode != 124 { - t.Fatalf("removed-wait mutant changed protected trace or exit: trace=%q exit=%d", result.trace, result.exitCode) - } - }) + for _, schedule := range []string{"immediate", pendingSignal} { + t.Run("carrier/removed cleanup wait counterexample/"+schedule, func(t *testing.T) { + item := cases[0] + item.wantLimit = "context" + item.removeCleanupWait = true + item.pendingSignalAtBlock = schedule == pendingSignal + result := executeNPMAbsenceBranch(t, header, body, item) + if result.orderingError == nil || result.orderingError.Error() != "carrier cleanup did not wait for classifier exit" { + t.Fatalf("removed-wait rejection=%v, want live-classifier cause", result.orderingError) + } + if result.trace != "view\nabsent\ncancelled\nreaped\n" || result.exitCode != 124 { + t.Fatalf("removed-wait mutant changed protected trace or exit: trace=%q exit=%d", result.trace, result.exitCode) + } + }) + } } for _, item := range cases { t.Run(job+"/"+item.name, func(t *testing.T) { @@ -342,6 +370,15 @@ func executeNPMAbsenceBranch(t *testing.T, header, body string, item npmBranchCa // after cancellation. The deadline bounds faults; it is not the oracle. control = newNPMBranchControl(t) } + // Cancellation is one SIGTERM after Bash reports P. Bash 3.2 handles a trapped + // signal that arrives after its last pending-trap check before wait only when the + // awaited child exits, and the held classifier exits only at its own + // -test.timeout; one that arrives while wait sets up its interruption crashes + // Bash 3.2. The controlled shim therefore blocks on descriptor 7 instead, + // which signalCarrier closes only after the signal was sent, so the trap runs + // while the classifier is still held, whenever the signal arrives. pendingSignalAtBlock + // sends the signal after Bash's last pending-trap check before that read. + // The uncontrolled shim keeps waiting for the classifier it started. script := `child= cleanup() { if [[ -n "$child" ]]; then @@ -381,7 +418,7 @@ go() { IFS= read -r permit <&4 [[ "$permit" == start ]] || return 97 fi - "$BRANCH_BINARY" -test.run='^TestNPMAbsenceBranchEntrypoint$' -test.timeout=` + childTimeout.String() + ` -- "$@" &- & + "$BRANCH_BINARY" -test.run='^TestNPMAbsenceBranchEntrypoint$' -test.timeout=` + childTimeout.String() + ` -- "$@" &- 7<&- & if [[ "$BRANCH_CONTROL" == 1 ]]; then exec 3>&- 6<&- printf S >&5 @@ -389,7 +426,15 @@ go() { [[ "$permit" == register ]] || return 97 fi child=$! - if [[ "$BRANCH_CONTROL" == 1 ]]; then printf P >&5; fi + if [[ "$BRANCH_CONTROL" == 1 ]]; then + printf P >&5 + if [[ "$BRANCH_PENDING_SIGNAL" == 1 ]]; then + IFS= read -r permit <&"$(printf E >&5; IFS= read -r permit <&7 || :; printf 7)" || : + else + IFS= read -r permit <&7 || : + fi + return 97 + fi local status=0 wait "$child" || status=$? child= @@ -419,16 +464,21 @@ report=report.json defer deadline.Stop() cmd := exec.CommandContext(ctx, "/bin/bash", "--noprofile", "--norc", "-c", script) cmd.Dir = dir - cmd.Env = childcoverage.Environment(t, []string{"PATH=" + dir, "HOME=" + dir, "TMPDIR=" + dir, "PROOFKIT_BRANCH_ENTRYPOINT=1", "BRANCH_BINARY=" + binary, "BRANCH_PAYLOAD=" + item.payload, fmt.Sprintf("BRANCH_VIEW_EXIT=%d", item.viewExit), fmt.Sprintf("BRANCH_EXISTING_EXIT=%d", item.existingExit), "BRANCH_CONTROL=0", "BRANCH_DELAY_START=0"}) - cmd.Cancel = func() error { return cmd.Process.Signal(syscall.SIGTERM) } + cmd.Env = childcoverage.Environment(t, []string{"PATH=" + dir, "HOME=" + dir, "TMPDIR=" + dir, "PROOFKIT_BRANCH_ENTRYPOINT=1", "BRANCH_BINARY=" + binary, "BRANCH_PAYLOAD=" + item.payload, fmt.Sprintf("BRANCH_VIEW_EXIT=%d", item.viewExit), fmt.Sprintf("BRANCH_EXISTING_EXIT=%d", item.existingExit), "BRANCH_CONTROL=0", "BRANCH_DELAY_START=0", "BRANCH_PENDING_SIGNAL=0"}) + cmd.Cancel = func() error { + return control.signalCarrier(func() error { return cmd.Process.Signal(syscall.SIGTERM) }) + } cmd.WaitDelay = childTimeout + time.Second cmd.SysProcAttr = &syscall.SysProcAttr{Setpgid: true} if control != nil { - cmd.ExtraFiles = []*os.File{control.readyWriter, control.commandReader, control.eventWriter, control.releaseReader} + cmd.ExtraFiles = []*os.File{control.readyWriter, control.commandReader, control.eventWriter, control.releaseReader, control.signalReader} cmd.Env = append(cmd.Env, "PROOFKIT_BRANCH_READY=1", "BRANCH_CONTROL=1") if item.delayedChildStartup { cmd.Env = append(cmd.Env, "BRANCH_DELAY_START=1") } + if item.pendingSignalAtBlock { + cmd.Env = append(cmd.Env, "BRANCH_PENDING_SIGNAL=1") + } } var stdout, stderr npmBranchOutput cmd.Stdout, cmd.Stderr = &stdout, &stderr diff --git a/internal/tools/releasepreflight/npm_absence_carrier_test.go b/internal/tools/releasepreflight/npm_absence_carrier_test.go index b9dfa1b9..7e5ba16c 100644 --- a/internal/tools/releasepreflight/npm_absence_carrier_test.go +++ b/internal/tools/releasepreflight/npm_absence_carrier_test.go @@ -11,12 +11,13 @@ import ( ) // This protocol owns only the cancellation carrier's registration, dispatch-release, -// and independent child-exit witnesses. Product branch events stay in trace. +// signal-sent, and independent child-exit witnesses. Product branch events stay in trace. type npmBranchControl struct { readyReader, readyWriter *os.File commandReader, commandWriter *os.File eventReader, eventWriter *os.File releaseReader, releaseWriter *os.File + signalReader, signalWriter *os.File finished bool } @@ -30,6 +31,7 @@ func newNPMBranchControl(t *testing.T) *npmBranchControl { {&control.commandReader, &control.commandWriter}, {&control.eventReader, &control.eventWriter}, {&control.releaseReader, &control.releaseWriter}, + {&control.signalReader, &control.signalWriter}, } { var err error *pair.reader, *pair.writer, err = os.Pipe() @@ -51,10 +53,11 @@ func (control *npmBranchControl) closeChildEnds() { _ = control.commandReader.Close() _ = control.eventWriter.Close() _ = control.releaseReader.Close() + _ = control.signalReader.Close() } func (control *npmBranchControl) close() { - for _, file := range []*os.File{control.readyReader, control.readyWriter, control.commandReader, control.commandWriter, control.eventReader, control.eventWriter, control.releaseReader, control.releaseWriter} { + for _, file := range []*os.File{control.readyReader, control.readyWriter, control.commandReader, control.commandWriter, control.eventReader, control.eventWriter, control.releaseReader, control.releaseWriter, control.signalReader, control.signalWriter} { if file != nil { _ = file.Close() } @@ -81,6 +84,42 @@ func (control *npmBranchControl) command(value string) error { return err } +// cmd.Cancel sends its SIGTERM only through signalCarrier: it calls send, and +// only after send has returned does it close the signal-sent pipe. The signal +// is then pending in Bash before Bash can read EOF from this pipe, so a read +// that ends at this EOF is followed by the trap, never by the next command. A +// nil control only sends. +func (control *npmBranchControl) signalCarrier(send func() error) error { + err := send() + if control != nil { + _ = control.signalWriter.Close() + } + return err +} + +// npmBranchSignalPipeOpen reads the signal-sent pipe without blocking: EAGAIN +// means its writer is still open, EOF means it was already closed. +func npmBranchSignalPipeOpen(reader *os.File) error { + connection, err := reader.SyscallConn() + if err != nil { + return err + } + var state error + err = connection.Read(func(fd uintptr) bool { + n, readError := syscall.Read(int(fd), make([]byte, 1)) + if errors.Is(readError, syscall.EAGAIN) { + return true + } + if readError == nil && n == 0 { + state = fmt.Errorf("signal-sent pipe closed before the signal") + } else { + state = fmt.Errorf("signal-sent pipe state n=%d err=%v", n, readError) + } + return true + }) + return errors.Join(state, err) +} + func (control *npmBranchControl) release() error { if control.releaseWriter == nil { return nil @@ -128,6 +167,13 @@ func (control *npmBranchControl) run(item npmBranchCase, cancel func() error) er return err } } + if item.pendingSignalAtBlock { + // Bash is expanding the redirection of its blocking read, past its last + // trap check before that read, so the signal arrives as a pending trap. + if err := control.event('E'); err != nil { + return err + } + } if err := cancel(); err != nil { return err }