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

Communicate process kill signals without being blocked by stdout/err reads #472

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

0div
Copy link
Contributor

@0div 0div commented Mar 19, 2025

Description

  • use go channels to communicate signals to the handler read loops in envd and broadcast shutdown w/o being blocked by blocked reads
  • make local envd more similar to what provision.sh does in template-manager

Test

I will commit an integration test in E2B js-sdk that goes like this:

test.skipIf(!isIntegrationTest)(
  'kill running nextjs app and make sure process is gone',
  async () => {
    const sandbox = await Sandbox.create(integrationTestTemplate, {
      timeoutMs: 120_000,
    })
    // const sandbox = await Sandbox.connect('inbpexoncdnuj9qld3pms-cf3f5359')

    const cmd = await sandbox.commands.run('cd /test-app && next dev', {
      background: true,
    })

    await wait(30_000)

    const pid = cmd.pid
    console.log('PID', pid)

    await sandbox.commands.kill(pid)
    await wait(10_000)

    const processes = await sandbox.commands.list()
    console.log('processes:', processes)
    assert.equal(processes.length, 0)
  }
)

@0div 0div added the bug Something isn't working label Mar 19, 2025
@0div 0div self-assigned this Mar 19, 2025
Copy link

linear bot commented Mar 19, 2025


RUN mkdir -p /home/user
RUN chmod 777 -R /home/user
RUN chmod 777 -R /usr/local
Copy link
Contributor

Choose a reason for hiding this comment

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

chmod 777 seems excessive, but i'm low context, you might be better of with chown -R user /home/user and chown -R user /user/local

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm just mirroring what is done in provision script used in template manager, doesn't invalidate your comment tho—here's it's solely for local dev, however in template manager it's used for templates

@dobrac
Copy link
Contributor

dobrac commented Mar 19, 2025

await sandbox.commands.kill(pid)
await wait(10_000)

is it necessary to wait after the kill? Shouldn’t the await be enough?

@mlejva
Copy link
Member

mlejva commented Mar 19, 2025

await sandbox.commands.kill(pid)
await wait(10_000)

Agree with @dobrac here. Why to have wait here if we're already await kill?

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

Successfully merging this pull request may close these issues.

4 participants