Would it make more sense to background this? If the parent agent ends up crashing and being restarted, we'll lose the sub-agents and have to re-inject them. We can keep track of the expected PID in e.g./.coder-agent/pid

We could probably background it either on the host or inside the container, but not doing so has some nice properties:

1. We immediately discover if a sub agent exits/crashes and we could restart immediately (we don't currently)
2. Job control is simpler (simply cancel the context vs looking up processes and verifying against pid)
3. With prebuilds, we can exit all sub-agents on claim and re-inject afterwards to ensure a clean slate

For the case where the parent agent crashes, keeping those sub-agents may be a bit hit-and-miss and those dev containers could be affected anyway on agent startup. I'm not aware of agents crashing though so this might not even be a concern we need to be mindful of now?

Fair enough!

This will probably fail unless the container is running as privileged or has the specific CAP_NET_ADMIN privilege set on the container?

As per the comment, this is an optional networking boost. (See regular agent bootstrap script, I'll update the comment to reference it.) Did you have some action in mind?

We could check for both of these things before trying? Not a blocker though.

Do we also need tochown the binary so that it's readable by the default container user?

Good callout. I didn't consider this butdocker cp seems to follow the permissions of the file on disk. So unless wechown it could be nonsense within the container (non-existent user, etc).

It's unlikely that the permissions will be bad for the user (typically 0755), but we could improve it for sure. It might make sense to turn this into a script rather than N amount ofdocker execs.

Do we try to execute this as a non-root user?

AFAIK this will get executed as the remote user configured bydevcontainer.json (or if unconfigured, container user), which seems like the correct behavior to me.

This could probably be amap[uuid.UUID]struct{} instead, and then below on line 888 just check for_, found := injected[agent.ID]

I don't foresee the memory savings being necessary here (will we have 1000s of sub agents?). The current form reads better and is simpler to use IMO (I always prefer this form for readability where applicable).

Should we set an upper bound on deletion attempts and raise if more than say 3 attempts fail?

Are you suggesting silently ignoring failures unless >= 3 fail? Or perhaps adding retry logic?

I'm mainly worried about spamming error logs into the void.

These will be part of the parent agent log 🤔

We can leave it as-is for now, but I think if this does start happening frequently (or all the time) it may be difficult to catch if it just goes into the parent agent log.