refactor(perf-harness): address review findings

Close's error result is now cached in a cliProcess field so repeated
calls return the original outcome instead of nil; the hangup regression
test closes the client via the bounded escalation ladder (with a
defensive defer) instead of manually draining waitErr; and the two
Close escalation unit tests are merged into one table-driven test per
repo convention.

commit: dae912d26288c9c4aed32d1d83539df65c9eb612
author: brice · 3mo ago · 2026-07-18 16:40 UTC
parent: 681ca80
signature: GPG VerifiedCD88F9E564E0102646FB6077A1A07C7C7C3E349D · arch-gpg Verified for the committer email

3 files · +92 -55

M cmd/vev-perf-harness/hangup_test.go +12 -9


@@ -99,6 +99,14 @@ func TestClientTracePairsAfterPTYHangup(t *testing.T) {
99	99		t.Errorf("close daemon role: %v", err)
100	100		}
101	101		}()
	102	+	// Close the client on every exit path so a failed assertion cannot leak the
	103	+	// process. Close is idempotent (sync.Once): the explicit call below, once
	104	+	// reached, is the one whose result is asserted; this defer is a backstop.
	105	+	defer func() {
	106	+	if err := client.Close(); err != nil {
	107	+	t.Errorf("close client role: %v", err)
	108	+	}
	109	+	}()
102	110
103	111		// Wait until the client is attached with a receive pump blocked mid-frame:
104	112		// its trace then holds one more adapter_receive_start than end. That in-flight

@@ -112,15 +120,10 @@ func TestClientTracePairsAfterPTYHangup(t *testing.T) {
112	120		if err := client.pty.Close(); err != nil {
113	121		t.Fatalf("close client PTY master: %v", err)
114	122		}
115		-	select {
116		-	case <-client.waitErr:
117		-	case <-time.After(10 * time.Second):
118		-	t.Fatal("client did not exit after PTY hangup")
119		-	}
120		-	if client.output != nil {
121		-	if err := client.output.Close(); err != nil {
122		-	t.Errorf("close client terminal output: %v", err)
123		-	}
	123	+	// Close runs the bounded escalation ladder; the hangup above already severed
	124	+	// the PTY, so this should reap gracefully without reaching SIGTERM/SIGKILL.
	125	+	if err := client.Close(); err != nil {
	126	+	t.Fatalf("client did not exit cleanly after PTY hangup: %v", err)
124	127		}
125	128
126	129		// The severed client's trace must pair. On the unfixed client this fails with

 			t.Errorf("close daemon role: %v", err)
 		}
 	}()
+	// Close the client on every exit path so a failed assertion cannot leak the
+	// process. Close is idempotent (sync.Once): the explicit call below, once
+	// reached, is the one whose result is asserted; this defer is a backstop.
+	defer func() {
+		if err := client.Close(); err != nil {
+			t.Errorf("close client role: %v", err)
+		}
+	}()
 	// Wait until the client is attached with a receive pump blocked mid-frame:
 	// its trace then holds one more adapter_receive_start than end. That in-flight
 	if err := client.pty.Close(); err != nil {
 		t.Fatalf("close client PTY master: %v", err)
 	}
-	select {
+	// Close runs the bounded escalation ladder; the hangup above already severed
-	case <-client.waitErr:
+	// the PTY, so this should reap gracefully without reaching SIGTERM/SIGKILL.
-	case <-time.After(10 * time.Second):
+	if err := client.Close(); err != nil {
-		t.Fatal("client did not exit after PTY hangup")
+		t.Fatalf("client did not exit cleanly after PTY hangup: %v", err)
-	}
-	if client.output != nil {
-		if err := client.output.Close(); err != nil {
-			t.Errorf("close client terminal output: %v", err)
-		}
 	}
 	// The severed client's trace must pair. On the unfixed client this fails with

M cmd/vev-perf-harness/launcher.go +8 -8


@@ -131,6 +131,7 @@ type cliProcess struct {
131	131		cleanupRuntime func() error
132	132		terminalReady bool
133	133		readyPath string
	134	+	closeResult error
134	135		}
135	136
136	137		func (l *cliLauncher) Launch(m processMapping, role roleCommand) (launchedProcess, error) {

@@ -634,7 +635,6 @@ func (e *ptyLocalEcho) applicationOutput(chunk []byte) []byte {
634	635		// teardown. Every path closes the PTY exactly once and finalizes output and
635	636		// runtime state.
636	637		func (p *cliProcess) Close() error {
637		-	var result error
638	638		p.closed.Do(func() {
639	639		// 1. Drain terminal output for the whole teardown. Without an active
640	640		// consumer a full chunks channel stalls copyTerminal, which then stops

@@ -715,9 +715,9 @@ func (p *cliProcess) Close() error {
715	715		// downstream merge mystery. The graceful rungs never error on their own.
716	716		if killed {
717	717		if exited {
718		-	result = errors.New("public CLI process did not exit after hangup and SIGTERM; killed")
	718	+	p.closeResult = errors.New("public CLI process did not exit after hangup and SIGTERM; killed")
719	719		} else {
720		-	result = errors.New("timed out reaping public CLI process")
	720	+	p.closeResult = errors.New("timed out reaping public CLI process")
721	721		}
722	722		}
723	723

@@ -731,17 +731,17 @@ func (p *cliProcess) Close() error {
731	731		<-drainDone
732	732		}
733	733		if p.output != nil {
734		-	if err := p.output.Close(); result == nil {
735		-	result = err
	734	+	if err := p.output.Close(); p.closeResult == nil {
	735	+	p.closeResult = err
736	736		}
737	737		}
738	738		if p.cleanupRuntime != nil {
739		-	if err := p.cleanupRuntime(); result == nil {
740		-	result = err
	739	+	if err := p.cleanupRuntime(); p.closeResult == nil {
	740	+	p.closeResult = err
741	741		}
742	742		}
743	743		})
744		-	return result
	744	+	return p.closeResult
745	745		}
746	746
747	747		// drainTerminalOutput consumes copyTerminal's chunks for the duration of a

 	cleanupRuntime func() error
 	terminalReady  bool
 	readyPath      string
+	closeResult    error
 }
 func (l *cliLauncher) Launch(m processMapping, role roleCommand) (launchedProcess, error) {
 // teardown. Every path closes the PTY exactly once and finalizes output and
 // runtime state.
 func (p *cliProcess) Close() error {
-	var result error
 	p.closed.Do(func() {
 		// 1. Drain terminal output for the whole teardown. Without an active
 		// consumer a full chunks channel stalls copyTerminal, which then stops
 		// downstream merge mystery. The graceful rungs never error on their own.
 		if killed {
 			if exited {
-				result = errors.New("public CLI process did not exit after hangup and SIGTERM; killed")
+				p.closeResult = errors.New("public CLI process did not exit after hangup and SIGTERM; killed")
 			} else {
-				result = errors.New("timed out reaping public CLI process")
+				p.closeResult = errors.New("timed out reaping public CLI process")
 			}
 		}
 			<-drainDone
 		}
 		if p.output != nil {
-			if err := p.output.Close(); result == nil {
+			if err := p.output.Close(); p.closeResult == nil {
-				result = err
+				p.closeResult = err
 			}
 		}
 		if p.cleanupRuntime != nil {
-			if err := p.cleanupRuntime(); result == nil {
+			if err := p.cleanupRuntime(); p.closeResult == nil {
-				result = err
+				p.closeResult = err
 			}
 		}
 	})
-	return result
+	return p.closeResult
 }
 // drainTerminalOutput consumes copyTerminal's chunks for the duration of a

M cmd/vev-perf-harness/launcher_test.go +72 -38


@@ -134,45 +134,79 @@ func TestCLIProcessCloseIsBoundedWhenPublicRoleDoesNotExit(t *testing.T) {
134	134		}
135	135		}
136	136
137		-	func TestCLIProcessCloseSIGTERMStopIsGraceful(t *testing.T) {
138		-	// A role reaped during the SIGTERM wait stopped gracefully: no SIGKILL, and
139		-	// Close must not report an error for a stage that can still flush a trace.
140		-	waitErr := make(chan error, 1)
141		-	never := make(chan time.Time)
142		-	var term, kill int
143		-	p := &cliProcess{
144		-	waitErr: waitErr,
145		-	waitTimeout: func() <-chan time.Time { return never },
146		-	forceCleanup: func() { term++; waitErr <- nil },
147		-	forceKill: func() { kill++ },
148		-	}
149		-	if err := p.Close(); err != nil {
150		-	t.Fatalf("graceful SIGTERM stop returned error: %v", err)
151		-	}
152		-	if term != 1 \|\| kill != 0 {
153		-	t.Fatalf("TERM/KILL calls = %d/%d, want 1/0", term, kill)
154		-	}
155		-	}
156		-
157		-	func TestCLIProcessCloseErrorsAfterKillEscalation(t *testing.T) {
158		-	// A role that only dies to SIGKILL may have truncated its trace, so Close must
159		-	// surface a named escalation error rather than a silent success.
160		-	waitErr := make(chan error, 1)
161		-	deadline := make(chan time.Time, 1)
162		-	deadline <- time.Now() // one ready deadline releases the SIGTERM wait
163		-	var term, kill int
164		-	p := &cliProcess{
165		-	waitErr: waitErr,
166		-	waitTimeout: func() <-chan time.Time { return deadline },
167		-	forceCleanup: func() { term++ },
168		-	forceKill: func() { kill++; waitErr <- nil },
169		-	}
170		-	err := p.Close()
171		-	if err == nil \|\| !strings.Contains(err.Error(), "killed") {
172		-	t.Fatalf("kill escalation error = %v, want a named killed escalation", err)
	137	+	func TestCLIProcessCloseEscalationOutcome(t *testing.T) {
	138	+	tests := []struct {
	139	+	name string
	140	+	reapsAt string // "term" or "kill": which rung's callback reaps the process
	141	+	waitTimeout func() func() <-chan time.Time
	142	+	wantErrSub string // "" means Close must return nil
	143	+	wantTerm int
	144	+	wantKill int
	145	+	}{
	146	+	{
	147	+	// A role reaped during the SIGTERM wait stopped gracefully: no SIGKILL,
	148	+	// and Close must not report an error for a stage that can still flush a
	149	+	// trace. The deadline channel never fires, so only the SIGTERM callback
	150	+	// reaping the process can end the wait.
	151	+	name: "SIGTERM stop is graceful",
	152	+	reapsAt: "term",
	153	+	waitTimeout: func() func() <-chan time.Time {
	154	+	never := make(chan time.Time)
	155	+	return func() <-chan time.Time { return never }
	156	+	},
	157	+	wantErrSub: "",
	158	+	wantTerm: 1,
	159	+	wantKill: 0,
	160	+	},
	161	+	{
	162	+	// A role that only dies to SIGKILL may have truncated its trace, so
	163	+	// Close must surface a named escalation error rather than a silent
	164	+	// success. One ready deadline releases the SIGTERM wait so the ladder
	165	+	// escalates to SIGKILL, whose callback then reaps the process.
	166	+	name: "errors after kill escalation",
	167	+	reapsAt: "kill",
	168	+	waitTimeout: func() func() <-chan time.Time {
	169	+	deadline := make(chan time.Time, 1)
	170	+	deadline <- time.Now()
	171	+	return func() <-chan time.Time { return deadline }
	172	+	},
	173	+	wantErrSub: "killed",
	174	+	wantTerm: 1,
	175	+	wantKill: 1,
	176	+	},
173	177		}
174		-	if term != 1 \|\| kill != 1 {
175		-	t.Fatalf("TERM/KILL calls = %d/%d, want 1/1", term, kill)
	178	+	for _, tt := range tests {
	179	+	t.Run(tt.name, func(t *testing.T) {
	180	+	waitErr := make(chan error, 1)
	181	+	var term, kill int
	182	+	p := &cliProcess{
	183	+	waitErr: waitErr,
	184	+	waitTimeout: tt.waitTimeout(),
	185	+	forceCleanup: func() {
	186	+	term++
	187	+	if tt.reapsAt == "term" {
	188	+	waitErr <- nil
	189	+	}
	190	+	},
	191	+	forceKill: func() {
	192	+	kill++
	193	+	if tt.reapsAt == "kill" {
	194	+	waitErr <- nil
	195	+	}
	196	+	},
	197	+	}
	198	+	err := p.Close()
	199	+	if tt.wantErrSub == "" {
	200	+	if err != nil {
	201	+	t.Fatalf("Close() error = %v, want nil", err)
	202	+	}
	203	+	} else if err == nil \|\| !strings.Contains(err.Error(), tt.wantErrSub) {
	204	+	t.Fatalf("Close() error = %v, want an error containing %q", err, tt.wantErrSub)
	205	+	}
	206	+	if term != tt.wantTerm \|\| kill != tt.wantKill {
	207	+	t.Fatalf("TERM/KILL calls = %d/%d, want %d/%d", term, kill, tt.wantTerm, tt.wantKill)
	208	+	}
	209	+	})
176	210		}
177	211		}
178	212


@@ -134,45 +134,79 @@ func TestCLIProcessCloseIsBoundedWhenPublicRoleDoesNotExit(t *testing.T) {
134		}	134		}
135		}	135		}
136			136
137	-	func TestCLIProcessCloseSIGTERMStopIsGraceful(t *testing.T) {	137	+	func TestCLIProcessCloseEscalationOutcome(t *testing.T) {
138	-	// A role reaped during the SIGTERM wait stopped gracefully: no SIGKILL, and	138	+	tests := []struct {
139	-	// Close must not report an error for a stage that can still flush a trace.	139	+	name string
140	-	waitErr := make(chan error, 1)	140	+	reapsAt string // "term" or "kill": which rung's callback reaps the process
141	-	never := make(chan time.Time)	141	+	waitTimeout func() func() <-chan time.Time
142	-	var term, kill int	142	+	wantErrSub string // "" means Close must return nil
143	-	p := &cliProcess{	143	+	wantTerm int
144	-	waitErr: waitErr,	144	+	wantKill int
145	-	waitTimeout: func() <-chan time.Time { return never },	145	+	}{
146	-	forceCleanup: func() { term++; waitErr <- nil },	146	+	{
147	-	forceKill: func() { kill++ },	147	+	// A role reaped during the SIGTERM wait stopped gracefully: no SIGKILL,
148	-	}	148	+	// and Close must not report an error for a stage that can still flush a
149	-	if err := p.Close(); err != nil {	149	+	// trace. The deadline channel never fires, so only the SIGTERM callback
150	-	t.Fatalf("graceful SIGTERM stop returned error: %v", err)	150	+	// reaping the process can end the wait.
151	-	}	151	+	name: "SIGTERM stop is graceful",
152	-	if term != 1 \|\| kill != 0 {	152	+	reapsAt: "term",
153	-	t.Fatalf("TERM/KILL calls = %d/%d, want 1/0", term, kill)	153	+	waitTimeout: func() func() <-chan time.Time {
154	-	}	154	+	never := make(chan time.Time)
155	-	}	155	+	return func() <-chan time.Time { return never }
156	-		156	+	},
157	-	func TestCLIProcessCloseErrorsAfterKillEscalation(t *testing.T) {	157	+	wantErrSub: "",
158	-	// A role that only dies to SIGKILL may have truncated its trace, so Close must	158	+	wantTerm: 1,
159	-	// surface a named escalation error rather than a silent success.	159	+	wantKill: 0,
160	-	waitErr := make(chan error, 1)	160	+	},
161	-	deadline := make(chan time.Time, 1)	161	+	{
162	-	deadline <- time.Now() // one ready deadline releases the SIGTERM wait	162	+	// A role that only dies to SIGKILL may have truncated its trace, so
163	-	var term, kill int	163	+	// Close must surface a named escalation error rather than a silent
164	-	p := &cliProcess{	164	+	// success. One ready deadline releases the SIGTERM wait so the ladder
165	-	waitErr: waitErr,	165	+	// escalates to SIGKILL, whose callback then reaps the process.
166	-	waitTimeout: func() <-chan time.Time { return deadline },	166	+	name: "errors after kill escalation",
167	-	forceCleanup: func() { term++ },	167	+	reapsAt: "kill",
168	-	forceKill: func() { kill++; waitErr <- nil },	168	+	waitTimeout: func() func() <-chan time.Time {
169	-	}	169	+	deadline := make(chan time.Time, 1)
170	-	err := p.Close()	170	+	deadline <- time.Now()
171	-	if err == nil \|\| !strings.Contains(err.Error(), "killed") {	171	+	return func() <-chan time.Time { return deadline }
172	-	t.Fatalf("kill escalation error = %v, want a named killed escalation", err)	172	+	},
			173	+	wantErrSub: "killed",
			174	+	wantTerm: 1,
			175	+	wantKill: 1,
			176	+	},
173		}	177		}
174	-	if term != 1 \|\| kill != 1 {	178	+	for _, tt := range tests {
175	-	t.Fatalf("TERM/KILL calls = %d/%d, want 1/1", term, kill)	179	+	t.Run(tt.name, func(t *testing.T) {
			180	+	waitErr := make(chan error, 1)
			181	+	var term, kill int
			182	+	p := &cliProcess{
			183	+	waitErr: waitErr,
			184	+	waitTimeout: tt.waitTimeout(),
			185	+	forceCleanup: func() {
			186	+	term++
			187	+	if tt.reapsAt == "term" {
			188	+	waitErr <- nil
			189	+	}
			190	+	},
			191	+	forceKill: func() {
			192	+	kill++
			193	+	if tt.reapsAt == "kill" {
			194	+	waitErr <- nil
			195	+	}
			196	+	},
			197	+	}
			198	+	err := p.Close()
			199	+	if tt.wantErrSub == "" {
			200	+	if err != nil {
			201	+	t.Fatalf("Close() error = %v, want nil", err)
			202	+	}
			203	+	} else if err == nil \|\| !strings.Contains(err.Error(), tt.wantErrSub) {
			204	+	t.Fatalf("Close() error = %v, want an error containing %q", err, tt.wantErrSub)
			205	+	}
			206	+	if term != tt.wantTerm \|\| kill != tt.wantKill {
			207	+	t.Fatalf("TERM/KILL calls = %d/%d, want %d/%d", term, kill, tt.wantTerm, tt.wantKill)
			208	+	}
			209	+	})
176		}	210		}
177		}	211		}
178			212