Skip to content

Conversation

Lagoja
Copy link
Contributor

@Lagoja Lagoja commented Sep 13, 2024

Summary

This fixes two previously reported issues:

  1. If process-compose is not terminated gracefully (e.g., it's parent shell crashes, or is closed on accident), then process-compose may terminate without also terminating it's services.
  2. If process-compose is started in the background, a user can now run the attach command to re-attach the TUI to the backgrounded process compose

A few current limitations:

  1. This only applies to devbox services up -b, but could be extended to devbox services up as well by starting process-compose in the background, and then attaching the TUI
  2. You will now need to explicitly run devbox services stop to stop process-compose for your project

Todos:

  1. Should we apply the backgrounding to devbox services up as well?
  2. Should we allow users to specify a port or specific process-compose instance to attach to?
  3. If so, should we list the process-compose instances somewhere?

How was it tested?

Tested on the Apache example:

Attach:

  1. Run devbox services up -b in the apache folder
  2. Run devbox services attach in the apache folder, verify that it launches the TUI
  3. Hit Ctrl-C to exit the TUI

Backgrounding:
4. Run devbox services list, verify that process-compose is still running
5. Terminate your shell or editor
6. Launch a new shell or editor, navigate to the apache example
7. Run devbox services list to verify that process-compose is still running.

Copy link
Contributor

@mikeland73 mikeland73 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, though as discussed offline this doesn't completely address

If process-compose is not terminated gracefully (e.g., it's parent shell crashes, or is closed on accident), then process-compose may terminate without also terminating it's services.

specifically if pc is started in the foreground.

Should we apply the backgrounding to devbox services up as well?
I don't think so. Usually when people terminate a foreground process they don't expect it to continue.

@Lagoja Lagoja marked this pull request as ready for review September 13, 2024 18:54
@Lagoja
Copy link
Contributor Author

Lagoja commented Sep 13, 2024

@mikeland73 I think I have foreground handled without needing to start up a background process/daemon in the latest commit. I'm not sure exactly what signal is being sent by the terminal, but this seemed to work in my testing.

Note that we cannot handle a SIGKILL signal gracefully, so this won't work in those scenarios

@Lagoja Lagoja merged commit 3ac5262 into main Sep 13, 2024
24 checks passed
@Lagoja Lagoja deleted the jl/attach-pc branch September 13, 2024 22:25
Copy link

sentry-io bot commented Sep 18, 2024

Suspect Issues

This pull request was deployed and Sentry observed the following issues:

  • ‼️ **Generic Error: <redacted errors.withStack>: <redacted usererr.combined> go.jetpack.io/devbox/internal/boxcli/usererr in... View Issue
  • ‼️ **syscall.Errno: <redacted *errors.withStack>: <redacted errors.withStack>: <redacted errors.withStack>: <redact... go.jetpack.io/devbox/internal/plugin in (*Local... View Issue
  • ‼️ **Generic Error: <redacted *errors.withStack>: <redacted errors.withStack>: <redacted errors.withStack>: <redact... go.jetpack.io/devbox/internal/devconfig in (*Co... View Issue
  • ‼️ **Generic Error: <redacted *errors.withStack>: <redacted errors.withStack>: <redacted usererr.combined> go.jetpack.io/devbox/internal/boxcli/usererr in... View Issue

Did you find this useful? React with a 👍 or 👎

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants