fix(login): shut the key reader down gracefully so nothing races its fd

[Soph]

https://entire.io/gh/entireio/cli/trails/1058

The flake

test-core fails intermittently with a data race, and because the race detector fails the whole package, one landing takes ~20 unrelated parallel tests down with it — each reporting only race detected during execution of test, which hides the single real cause.

It has hit main twice: on the merge run of #2006 (31836107297, Aug 14) and again three days later on an unrelated PR (#1981), with green main runs in between.

One cause, two racing pairs

bubbletea's shutdown does this:

if p.cancelReader.Cancel() {
	if !kill {
		p.waitForReadLoop()
	}
}
_ = p.cancelReader.Close()

On a killed Run it skips the join, so Close lands while the input reader may still be inside wait(). That reader reads two descriptors on wake:

1. tty.Fd() — ours. This is the pair fix(login): don't close the TTY a killed Bubble Tea reader still holds #2006 designed around by leaking the descriptor, and the pair its regression test then re-created by closing that descriptor in teardown.
2. cancelSignalReader.Fd() — cancelreader's own cancel-signal pipe, which Close() closes (cancelreader_bsd.go:109, cancelreader_linux.go:118) while wait() reads it (:141, :150). Entirely upstream; nothing on our side can avoid it.

And Run treats a cancelled context as killed:

killed := p.externalCtx.Err() != nil || p.ctx.Err() != nil || err != nil

So passing ctx to tea.WithContext made every cancellation take the racy path. That matters more than it sounds: cancellation is the normal path here, not an edge case — waitForLoginURLResult cancels the reader as soon as authentication completes, so every sign-in where the user doesn't press a key went through it.

The fix: stop cancelling, start quitting

Quit delivers a QuitMsg, which eventLoop returns with a nil error, so killed stays false and shutdown joins the read loop before closing anything. Nothing holds either descriptor by the time they're closed — which kills both pairs, including the upstream one, without patching upstream.

It also means tty can simply be closed on the cancelled path rather than leaked, so the conditional close, the long comment, the teardown tripwire, and the leak all go away.

Not a bare swap. Quit sends on an unbuffered channel, so an event loop that never takes it would block Quit forever — and finishActionRead (login.go:615) joins this read once authentication completes, so a hang there would strand a completed sign-in. That's a worse failure than the leak being removed. bubbletea therefore keeps its own context, cancelled only if the graceful path doesn't land within loginURLQuitGrace, and the descriptor is left alone in exactly that case — keyed on tea.ErrProgramKilled, the upstream signal, rather than on a proxy for it.

Verification

On darwin under -race:

Load	Races before	Races after
2000× the cancelled test	reproduced the cancelreader pair	0
2×1500× every TTY test	—	0

Plus go test -race ./cmd/entire/cli/, mise run test:ci, and mise run lint all green.

The before/after comparison is the load-bearing evidence here: the same 2000-iteration run that reliably surfaced the upstream pair on the previous commit is now clean, which is what distinguishes this from the partial fix.

Test change

TestReadLoginURLActionFromTTY_CancelledLeavesDescriptorOpen becomes _CancelledClosesDescriptor. Its premise inverted — the cancelled path now owns the descriptor like every other path. It asserts on the descriptor number (Close sets Sysfd to -1 in internal/poll.(*FD).destroy, so it's platform-independent) rather than terminal state, which would report EIO on Linux merely because ptmx is still open.

🤖 Generated with Claude Code

[Copilot]

Pull request overview

This PR removes a teardown Close() in a Linux-only regression test that was reintroducing the same Bubble Tea cancel-reader data race that production code intentionally avoids by leaking the TTY descriptor on the cancelled path.

Changes:

• Stop closing the tty in TestReadLoginURLActionFromTTY_CancelledLeavesDescriptorOpen, aligning the test’s ownership behavior with readLoginURLActionFromTTY’s cancelled-path contract.
• Add an in-test explanation documenting why the descriptor must be left open (unjoinable reader goroutine + race detector failure mode).

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.