Skip to content

Commit

Permalink
fix: close stdin for non-interactive tasks (#8838)
Browse files Browse the repository at this point in the history
### Description

Fixes #8281 

With this PR we now close `stdin` for any task that is not marked as
`"interactive": true` or `"persistent": true`.

**Explanation**

The `del-cli dbschema/edgeql-js dbschema/interfaces.*` part of
`dram:codegen` task waits for user input if `stdin` isn't closed even if
it isn't hooked up to a TTY:
```
dram:codegen: Checking the generated query builder into version control
dram:codegen: is not recommended. Would you like to create a .gitignore file to ignore
dram:codegen: the query builder directory? [y/n] (leave blank for "y")
```
This caused the task to never exit since there was no user to answer the
prompt.

The prompt was never surfaced as GH Action runs have logs grouped
together by default and they are not flushed if a run is interrupted
before it finishes.

**How We Got Here**

- We discovered that we were unintentionally letting starting child
processes with inherited `stdin`. We disabled this in
#7034 and shifted to never opening
`stdin` for some child processes.
- We started opening `stdin` for persistent tasks in
#7196 as starting without `stdin`
caused issues for some tools e.g. vite
- #7767 changed behavior so *all*
tasks started with `stdin`, but only some would receive input. We forgot
to update the closing logic to close `stdin` for the tasks that
shouldn't have it.


**Future Work**

We should invest time into fixing #6514 so that this prompt would have
been seen on the CI runs. Seeing the prompt would've made the issue
obvious.


### Testing Instructions

Verify that running `vite` server with streamed logs doesn't crash:
```
[0 olszewski@chriss-mbp] /tmp/vite-test $ TURBO_UI=0 turbo_dev dev
 WARNING  No locally installed `turbo` found. Using version: 2.0.10-canary.0.
• Packages in scope: @repo/eslint-config, @repo/typescript-config, @repo/ui, docs, web
• Running dev in 5 packages
• Remote caching disabled
web:dev: cache bypass, force executing d3eae62c93d9ce57
docs:dev: cache bypass, force executing c838417f19c48cfb
docs:dev: 
docs:dev: > [email protected] dev /private/tmp/vite-test/apps/docs
docs:dev: > vite --clearScreen false
docs:dev: 
web:dev: 
web:dev: > [email protected] dev /private/tmp/vite-test/apps/web
web:dev: > vite --clearScreen false
web:dev: 
web:dev: 
web:dev:   VITE v5.1.4  ready in 81 ms
web:dev: 
web:dev:   ➜  Local:   http://localhost:5173/
web:dev:   ➜  Network: use --host to expose
web:dev:   ➜  press h + enter to show help
docs:dev: Port 5173 is in use, trying another one...
docs:dev: 
docs:dev:   VITE v5.1.4  ready in 88 ms
docs:dev: 
docs:dev:   ➜  Local:   http://localhost:5174/
docs:dev:   ➜  Network: use --host to expose
docs:dev:   ➜  press h + enter to show help
```

Huge kudos to @jakubmazanec for remaining active on the issue while we
figure this out.
  • Loading branch information
chris-olszewski authored Jul 25, 2024
1 parent 4b34a31 commit 310b69b
Showing 1 changed file with 7 additions and 0 deletions.
7 changes: 7 additions & 0 deletions crates/turborepo-lib/src/task_graph/visitor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -952,6 +952,13 @@ impl ExecContext {
}
}

// Even if user does not have the TUI and cannot interact with a task, we keep
// stdin open for persistent tasks as some programs will shut down if stdin is
// closed.
if !self.takes_input {
process.stdin();
}

let mut stdout_writer = self
.task_cache
.output_writer(prefixed_ui.task_writer())
Expand Down

0 comments on commit 310b69b

Please sign in to comment.