Fix hang in source <(lets completion -s zsh) under job control#383
Merged
Conversation
Every invocation queried the terminal background color (OSC 11) via lipgloss.HasDarkBackground(os.Stdin, stderr) when stderr is a TTY -- in logging.InitLogging and in progress bar setup. Inside process substitution under an interactive (job-control) shell, lets runs in a background process group; reading/reconfiguring the controlling terminal suspends it with SIGTTIN/SIGTTOU, the fifo never reaches EOF, and `source` blocks forever. Skip background-color detection when the process is not in the terminal's foreground process group (TIOCGPGRP vs getpgrp), defaulting to a dark background, lipgloss's own fallback. Windows has no terminal process groups, so it always queries. Regression test runs zsh with job control under a PTY (script) in bats; it failed with a timeout before the fix.
Applied by golangci-lint --fix (errorlint); lets errors.Is/As unwrap the read error alongside the download error.
Contributor
Reviewer's GuideIntroduces a guarded terminal-background detection helper used by both theming and logging so that color detection is skipped when lets is not in the terminal’s foreground process group, preventing hangs in shells using job control and process substitution, and adds a regression test plus minor supporting tweaks. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The new Bats test relies on
script -qecandtimeout, which are GNU-specific flags/utilities and may behave differently or be missing on BSD/macOS systems; consider guarding the test, using more portable flags, or documenting the requirement so local runs don’t fail unexpectedly. - IsForegroundProcess returns false on any ioctl error, which silently forces a dark theme even for non-TTY or unexpected fds; if that fallback is intentional, it might be worth constraining where the function is used or adding a short comment about this implicit behavior so future callers don’t assume an error means “not foreground” in a stronger sense.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new Bats test relies on `script -qec` and `timeout`, which are GNU-specific flags/utilities and may behave differently or be missing on BSD/macOS systems; consider guarding the test, using more portable flags, or documenting the requirement so local runs don’t fail unexpectedly.
- IsForegroundProcess returns false on any ioctl error, which silently forces a dark theme even for non-TTY or unexpected fds; if that fallback is intentional, it might be worth constraining where the function is used or adding a short comment about this implicit behavior so future callers don’t assume an error means “not foreground” in a stronger sense.
## Individual Comments
### Comment 1
<location path="internal/util/foreground_unix.go" line_range="14-16" />
<code_context>
+// terminal from a background process group suspends the process with
+// SIGTTIN/SIGTTOU — e.g. inside process substitution:
+// `source <(lets completion -s zsh)`.
+func IsForegroundProcess(fd uintptr) bool {
+ pgrp, err := unix.IoctlGetInt(int(fd), unix.TIOCGPGRP)
+ if err != nil {
+ return false
+ }
</code_context>
<issue_to_address>
**issue (bug_risk):** Differentiate ioctl failures from true background status to avoid disabling detection on non-tty fds.
Because any `unix.IoctlGetInt` error is treated as "not foreground", non-tty fds (e.g., when stdout is a file/pipe or `EBADF`) are incorrectly handled as background, forcing callers to skip detection. Since `SIGTTIN/SIGTTOU` only apply to ttys, it would be better to treat `ENOTTY` (and possibly other ioctl failures) as "safe" and either return `true` or at least distinguish them from a real background process-group indication, so non-tty outputs retain normal detection behavior.
</issue_to_address>
### Comment 2
<location path="tests/completion.bats" line_range="59-62" />
<code_context>
+# group suspends the process with SIGTTOU, so `source <(lets completion -s zsh)`
+# in an interactive (job-control) shell hung forever. `script` allocates a PTY,
+# `set -m` enables job control like an interactive shell.
+@test "completion: source <(lets completion -s zsh) must not hang under job control" {
+ run timeout 10 script -qec "zsh -c 'set -m; source <(lets completion -s zsh); echo SOURCED_OK'" /dev/null
+ assert_success
+ assert_output --partial "SOURCED_OK"
+}
</code_context>
<issue_to_address>
**suggestion (testing):** Guard the regression test with skips when `zsh` or `script` binaries are unavailable
This test will fail on systems without `zsh` or `script` (or with a different `script` implementation), which is common on minimal CI images. Please guard it with availability checks (e.g., `command -v zsh >/dev/null || skip "zsh not available"` and similarly for `script`) so the suite stays portable while still exercising the regression where possible.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+14
to
+16
| func IsForegroundProcess(fd uintptr) bool { | ||
| pgrp, err := unix.IoctlGetInt(int(fd), unix.TIOCGPGRP) | ||
| if err != nil { |
Contributor
There was a problem hiding this comment.
issue (bug_risk): Differentiate ioctl failures from true background status to avoid disabling detection on non-tty fds.
Because any unix.IoctlGetInt error is treated as "not foreground", non-tty fds (e.g., when stdout is a file/pipe or EBADF) are incorrectly handled as background, forcing callers to skip detection. Since SIGTTIN/SIGTTOU only apply to ttys, it would be better to treat ENOTTY (and possibly other ioctl failures) as "safe" and either return true or at least distinguish them from a real background process-group indication, so non-tty outputs retain normal detection behavior.
Comment on lines
+59
to
+62
| @test "completion: source <(lets completion -s zsh) must not hang under job control" { | ||
| run timeout 10 script -qec "zsh -c 'set -m; source <(lets completion -s zsh); echo SOURCED_OK'" /dev/null | ||
| assert_success | ||
| assert_output --partial "SOURCED_OK" |
Contributor
There was a problem hiding this comment.
suggestion (testing): Guard the regression test with skips when zsh or script binaries are unavailable
This test will fail on systems without zsh or script (or with a different script implementation), which is common on minimal CI images. Please guard it with availability checks (e.g., command -v zsh >/dev/null || skip "zsh not available" and similarly for script) so the suite stays portable while still exercising the regression where possible.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Problem
source <(lets completion -s zsh)hangs indefinitely in an interactive shell (regression from the recent theming work).Root cause
Every invocation eagerly queries the terminal background color (OSC 11) via
lipgloss.HasDarkBackground(os.Stdin, stderr)when stderr is a TTY — once inlogging.InitLoggingand once in progress-bar setup. Inside process substitution under an interactive (job-control) shell, stdout is a pipe but stdin/stderr are still the terminal, and lets runs in a background process group. Reading/reconfiguring the controlling terminal from a background process group suspends the process with SIGTTIN/SIGTTOU, so the fifo never reaches EOF andsourceblocks forever.Fix
Add
util.IsForegroundProcess(comparesTIOCGPGRPon the tty with our process group) and route both call sites through a newtheme.IsDarkBackground, which skips the query and assumes a dark background (lipgloss's own fallback) when the process is not in the foreground. Foreground behavior is unchanged — the query still runs and themes resolve as before.This also fixes the same hang for any lets command run inside
<(...)or&, not just completion.Testing
zsh -c 'set -m; source <(lets completion -s zsh)'under a PTY (script) with a timeout: fails with status 124 before the fix, passes after (verified both directions in Docker).go test ./...,lets test-bats completion.bats, andlets lintall pass.Second commit is an unrelated
%v→%winreadMixinthatlets lint --fix(errorlint) applies automatically; committed separately to keep the fix focused.Note: the
lets-cli/fangfork has the same unguarded query inmustColorscheme(gated on stdout being a TTY, so reachable via e.g.lets --help &) — worth porting the same guard upstream.Summary by Sourcery
Guard terminal background-color detection behind a foreground-process check to prevent hangs when lets runs in background contexts, and tidy up related configuration and documentation.
Bug Fixes:
%wverb for nested errors.Enhancements:
Build:
golang.org/x/systo a direct dependency for terminal process-group inspection.Documentation:
Tests:
source <(lets completion -s zsh)under zsh job control completes successfully rather than hanging.