Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[turborepo] regression on handling SIGINT #3711

Open
zanona opened this issue Feb 9, 2023 · 19 comments · Fixed by #4313
Open

[turborepo] regression on handling SIGINT #3711

zanona opened this issue Feb 9, 2023 · 19 comments · Fixed by #4313
Assignees
Labels
kind: bug Something isn't working

Comments

@zanona
Copy link

zanona commented Feb 9, 2023

What version of Turborepo are you using?

1.7.4

What package manager are you using / does the bug impact?

npm

What operating system are you using?

Linux

Describe the Bug

Referring to #444 and its implementation on #663, it looks like there was a behavioural regression introduced in 1.7.0 where SIGINT is no longer handled properly. The last working version was 1.6.3.

The bug is related to the fact that when interrupting a program (^C), turbo doesn't wait for the program to handle the SIGINT command and immediately terminates the process.

It is worth noting that the SIGINT instruction continues to run in the background, and the terminal becomes interactive again. This poses a problem when running turbo from a Docker container, for example, while Docker waits for the SIGINT instruction to be handled and then kills the container process. Since turbo doesn't wait for SIGINT to complete, docker simply kills the process prematurely. The expected behaviour here would be that of 1.6.3.

Expected Behavior

Turbo should wait for the program to handle the SIGINT instruction and only then exit the running process.

To Reproduce

./apps/bar/package.json

{
   "name": "@foo/bar",
   "scripts": {
       "start": "trap 'echo wait…; sleep 3; echo done!; exit 0' SIGINT; sleep 10",
   }
}

turbo@1.6.3 (expected behaviour)

$ npx turbo run start --filter=@foo/bar --only
• Packages in scope: @foo/bar
• Running start in 1 packages
• Remote caching disabled
@foo/bar:start: cache miss, executing a0c8f8ecc5ad4cad
@foo/bar:start:
@foo/bar:start: > start
@foo/bar:start: > trap 'echo wait…; sleep 3; echo done!; exit 0' SIGINT; sleep 10
@foo/bar:start:
^C@foo/bar:start: wait…  # <<< SIGINT
@foo/bar:start: done!
$ echo interactive again....

turbo@1.7.0 (unexpected behaviour)

$ npx turbo run start --filter=@foo/bar --only
• Packages in scope: @foo/bar
• Running start in 1 packages
• Remote caching disabled
@foo/bar:start: cache miss, executing 52f6ea6626d6482f
@foo/bar:start:
@foo/bar:start: > start
@foo/bar:start: > trap 'echo wait…; sleep 3; echo done!; exit 0' SIGINT; sleep 10
@foo/bar:start:
^C@foo/bar:start: wait… # <<< SIGINT
$ echo interactive again
interactive again
$ @foo/bar:start: done! # <<< bg process

Reproduction Repo

No response

@zanona zanona added area: turborepo kind: bug Something isn't working labels Feb 9, 2023
@notaphplover
Copy link

I am actually facing the same issue with version 1.8.3. @mehulkar, I just uploaded a reproduction repo in case it's helpful.

@gajus
Copy link

gajus commented Mar 20, 2023

Tagging @jaredpalmer for visibility

@mehulkar
Copy link
Contributor

mehulkar commented Mar 21, 2023

May be fixed by #4276, could you verify against v1.8.5?

@notaphplover
Copy link

Hey @mehulkar, I might be wrong, but I would say the issue persist. I updated the reproduction repo to use turbo@1.8.5, but the issue persist

@mehulkar
Copy link
Contributor

Thanks for trying! cc @arlyon @chris-olszewski

chris-olszewski added a commit that referenced this issue Mar 31, 2023
### Description
I believe this fixes #3711

#4196 added graceful shutting down when calling `go-turbo`, this does
the same but for when we need to invoke the correct Rust binary in
`shim.rs`. I extracted the logic added in #4196 to a function that can
be used throughout the codebase.

### Testing Instructions

Currently only doing manual via the reproduction provided in #3711.
Edit: I originally wanted to add integration test to cover these cases,
this has proven to be a challenge. Will still look at trying to
orchestrate a test for this, but considering how much traction the
related issues have, I don't want to block on getting an integration
test.
@notaphplover
Copy link

@chris-olszewski I just saw a new turbo@1.9.0 version was released. I tried in the reproduction repo with that version and the issue persist:

image

Could you please reopen the issue?

@notaphplover
Copy link

Update: Same with turbo@1.9.1:

image

@chris-olszewski
Copy link
Member

@notaphplover, can you confirm the expected outcomes of sending a SIGINT to pnpm run foo:pnpm? On my machine it exits with a 0 same as pnpm run foo:pnpm:trap. I've also checked and it looks like turborepo has had this behavior (exit with code 1 regardless of trap) since 1.5.0, can you confirm this on your machine?

Also, if you need an exit code 0, could you change the trap command to trap 'exit 0' INT TERM; turbo run foo and that seems to work on my machine.

@notaphplover
Copy link

notaphplover commented Apr 24, 2023

Hi @chris-olszewski 😃.

@notaphplover, can you confirm the expected outcomes of sending a SIGINT to pnpm run foo:pnpm? On my machine it exits with a 0 same as pnpm run foo:pnpm:trap. I've also checked and it looks like turborepo has had this behavior (exit with code 1 regardless of trap) since 1.5.0, can you confirm this on your machine?

On my machine pnpm run foo:pnpm exits with non zero code and pnpm run foo:pnpm:trap with zero code. If this is a problem I think I could make an effort and set a gh action reproducing the issue. I could even open a debugging port and allow you to connect through ssh with some magic tricks, but I was expecting any linux machine would behave similar in this case :(. Unlucky us I guess.

Regarding which version introduced this (I think) unexpected behavior, I tried with 1.5.6 and it had the error. It's being a little bit hard to test in other 1.5 and 1.4 versions, the daemon does not manage to start (probably related to #2034). I managed to recreate the right behavior on 1.4.6 so I would hazard to say you are right.

Also, if you need an exit code 0, could you change the trap command to trap 'exit 0' INT TERM; turbo run foo and that seems to work on my machine.

Yeah, I know, the thing is, that's not what I want. I want to exit with code zero if and only if the process is able to exit gracefully with no issues. For that reason I want to trap '' instead, propagating the exit code.

Hope all of this helps. I would prefer not to pass through the pain of setting the remote debugging session, but if you really need it I can go for it in a couple of days.

Edit: I just realized the debugging session wouldn't be of any help 😅, but the ssh connection would allow you to connect to the gh runner and recreate the issue. Probably an overkill since docker seems a much simpler way to go

@chris-olszewski chris-olszewski self-assigned this Apr 24, 2023
@chris-olszewski
Copy link
Member

If this is a problem I think I could make an effort and set a gh action reproducing the issue

No need to, I just wanted to make sure that the description in the repro was correct and my machine was doing something weird.

I managed to recreate the right behavior on 1.4.6 so I would hazard to say you are right.

Thanks for confirming, this narrows the code changes to check quite a lot.

I want to exit with code zero if and only if the process is able to exit gracefully with no issues.

Understand, just wanted to check if that would provide any intermediate relief.

Sorry again for the drop in communication.

@notaphplover
Copy link

Sorry again for the drop in communication.

All good. This is an open source project after all. Love the beautiful work you're doing. Sometimes these issues happen, I simply opened the other issue to avoid losing the tracking.

@chris-olszewski
Copy link
Member

@notaphplover I had some time to delve into this and this is a larger feature request. We currently always exit with exit code 1 if we receive a signal. In order to return the highest exit code we need to start gracefully handling signals where the first SIGINT we receive gets forwarded to the child processes and then if we receive another SIGINT we sent a SIGKILL.

I don't expect this work will get done until we finish porting the codebase to Rust. Hopefully as we port this signal code we can set the groundwork for being more graceful with our signal handling.

@EloB
Copy link

EloB commented Apr 11, 2024

Is this working or will be fixed anytime soon? I'm having some issues that docker doesn't close when I do Ctrl + C. I'm using pnpm and graceful teardown works using pnpm --filter=mypackage run dev but npx turbo run dev --filter=mypackage fails to teardown all child processes.

@giorgiogross
Copy link

giorgiogross commented May 22, 2024

Experiencing the same issue, turbo version 1.13.3; when I hit ctrl+c I get an ERROR run failed: command exited (1). That doesn't happen when I run a command with a single task from root directory and use interactive mode, but when I disable interactive or when there is more than one task I get that error. Oh, and I'm using npm! :)

@davilima6
Copy link

davilima6 commented Aug 12, 2024

Possibly related: tsconfig.vitest-temp.json is left behind in workspace packages after exiting TUI with Ctrl+C for runs of pnpm turbo run test, where packages have "test": "vitest" as npm script, and typecheck: enabled in vitest.config.ts.

Ctrl+C while running the internal test scripts (i.e. running vitest "directly") seems to exit gracefully and delete the temporary file.

@makeryi
Copy link

makeryi commented Aug 14, 2024

I found something that might be useful for addressing this issue:
crates/turborepo-lib/src/cli/mod.rs
crates/turborepo-lib/src/commands/run.rs
Not sure if changing Ok(1) to Ok(()) or Ok(0) would fix this issue.

@markkkkas
Copy link

Is this working or will be fixed anytime soon? I'm having some issues that docker doesn't close when I do Ctrl + C. I'm using pnpm and graceful teardown works using pnpm --filter=mypackage run dev but npx turbo run dev --filter=mypackage fails to teardown all child processes.

We are observing same issue :(

@GuskiS
Copy link

GuskiS commented Sep 21, 2024

Seeing same issue where docker fails to teardown when using turbo 2.1.2.

@kota65535
Copy link

Same here in turbo 2.2.3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.