fix(process): reap entire process group on cmd.Cancel

[rremer]

I ran into an issue where many client disconnects during clone operations were leaving expensive git pack-objects --revs --thin --stdout --delta-base-offset processes in gitea. These processes would still run to completion, but the underlying http request which initiated them had long since been cancelled.

The bug is subtle, but the cmd.Cancel call would target the original PID, while git commands regularly fork several child processes which are not cancelled. Because their parent was cancelled, they'd get reparented to either the gitea process or pid 1 depending on the kernel.

This change uses the negative process group to cancel the entire process group together instead of just the parent.

[silverwind]

Please trim down on comments.

[wxiaoguang]

What do you think about this bc21fdd ?

[rremer]

@wxiaoguang while a much bigger refactor, your change will be safer because mine relied on callers to use setSysProcAttribute(), whereas now it would be the default behavior. The move to SIGTERM is also preferable, however I believe it introduced a new bug: if the child ignores SIGTERM it could hang forever, and grandchildren would never get a signal.

I've added a few test cases to prove this, and my proposed fix is just to introduce a default 10s (configurable) wait on Cancel() before escalating to SIGKILL: 9e336b1

[wxiaoguang]

however I believe it introduced a new bug: if the child ignores SIGTERM it could hang forever, and grandchildren would never get a signal.

I've added a few test cases to prove this, and my proposed fix is just to introduce a default 10s (configurable) wait on Cancel() before escalating to SIGKILL: 9e336b1

I intentionally didn't do that because it will just cause pid data-race problems. You can ask AI about the PID-race details.

So I strongly prefer to revert 9e336b1 (Actually, I mean, it needs to be reverted)

And, I don't see why the SIGTERM would be ignored in real world for our cases. If it would happen, I already prepared WithOnCancelForceKill

[rremer]

pid data-race problems.

Good point, although technically SIGINT suffers from this as well.

I don't see why the SIGTERM would be ignored in real world for our cases

Maybe not 'ignored' in real-world scenarios, but certainly blocked longer than what we might be willing to wait. How long is too-long is really only known by the caller, but I could see scenarios where that external renderer or ssh call sites hang on some connection. Is 5 minutes too-long? Is 30 seconds?

It looks like ProcessState can be trusted while we hold a reference, how about we just check it before doing any syscall? 3b7449f

[wxiaoguang]

I don't see why the SIGTERM would be ignored in real world for our cases

Maybe not 'ignored' in real-world scenarios, but certainly blocked longer than what we might be willing to wait. How long is too-long is really only known by the caller, but I could see scenarios where that external renderer or ssh call sites hang on some connection. Is 5 minutes too-long? Is 30 seconds?

It looks like ProcessState can be trusted while we hold a reference, how about we just check it before doing any syscall? 3b7449f

I don't see it would really affect real world use cases. And there is no easy solution for the PID reuse data-race.

I believe we can take the graceful kill (SIGTERM) as default behavior, if anything wrong, just switch to SIGKILL.

[wxiaoguang]

It looks like ProcessState can be trusted while we hold a reference, how about we just check it before doing any syscall? 3b7449f

No, it's not right. "there is no easy solution for the PID reuse data-race."

[rremer]

I don't see it would really affect real world use cases

I think the real-world use-case is pretty theoretical, for sure, but this would reduce the window of pid reuse from however long since the process was started, to probably nanoseconds.

[wxiaoguang]

I don't see it would really affect real world use cases

I think the real-world use-case is pretty theoretical, for sure, but this would reduce the window of pid reuse from however long since the process was started, to probably nanoseconds.

Let's just try to use SIGTERM as the default signal in production to see whether there would be real problems. If yes, just switch to SIGKILL.