bug:fix runc delete run before delete exec.fifo

[ningmingxiao]

if runc delete run before delete exec.fifo
runc delete will container root dir "delete /runc/nmx003"

func handleFifoResult(result openResult) error {
	if result.err != nil {
		return result.err
	}
	f := result.file
	defer f.Close()
	if err := readFromExecFifo(f); err != nil {
		return err
	}
	return os.Remove(f.Name())
}

os.remove will reurn a error
ERRO[0030] remove /run/runc/nmx003/exec.fifo: no such file or directory

[kolyshkin]

@ningmingxiao can you provide a reproducer?

[ningmingxiao]

I add some sleep at

func handleFifoResult(result openResult) error {
	if result.err != nil {
		return result.err
	}
	f := result.file
	defer f.Close()
	if err := readFromExecFifo(f); err != nil {
		return err
	}
        time.Sleep(time.Second*30)
	return os.Remove(f.Name())
}

runc create nmx001
runc start nmx001
when it is running at time.Sleep(time.Second*30)
runc delete nmx001 it will happen. @kolyshkin
It's not easy to reproduce unless a delay is added

[ningmingxiao]

I think RemoveAll is better

[kolyshkin]

I think RemoveAll is better

In its current implementation, and if path points to a non-directory (or an empty directory) -- yes, it's essentially does the same as I suggested above, i.e.

        err := os.Remove(path)
        if err == nil || os.IsNotExist(err) {
                  return nil
        }

but in a (theoretically possible) case when path is a non-empty directory, it will go ahead and remove it recursively, which is probably not what we really want here. I would prefer an error in such a case (even if it doesn't look like it's practically possible).

[ningmingxiao]

done, thanks @kolyshkin

[ningmingxiao]

ping @kolyshkin can this pr be merged ?

[kolyshkin]

lgtm

[Copilot]

Pull Request Overview

This PR fixes a bug related to the improper deletion of exec.fifo, which causes issues when runc delete is executed before deleting exec.fifo.

• Updated error handling in handleFifoResult to ignore os.IsNotExist errors during file removal.
• Prevents an error from being returned when exec.fifo is already removed, ensuring smooth deletion of the container's root directory.

[Copilot]

The updated error handling correctly ignores errors when exec.fifo does not exist, which fixes the bug. Consider adding a debug log when an unexpected error occurs to help diagnose issues in production environments.

[lifubang]

Pull Request Overview

This PR fixes a bug related to the improper deletion of exec.fifo, which causes issues when runc delete is executed before deleting exec.fifo.

• Updated error handling in handleFifoResult to ignore os.IsNotExist errors during file removal.
• Prevents an error from being returned when exec.fifo is already removed, ensuring smooth deletion of the container's root directory.

@Copilot Do you think this is really a bug.
If the container is deleted by other process, I think runc start / run should return the exact error and have a non-zero exit code, but not to ignore some errors. Or else the users may wrongly think that we start/run the container successfully, but the result is opposite.

[Copilot]

Pull Request Overview

This PR addresses a bug where deleting the container's exec.fifo file causes an error if the file no longer exists.

• Modified the error handling in handleFifoResult to ignore os.IsNotExist errors when removing the exec.fifo file.
• Ensures os.Remove is checked before returning any errors, preventing spurious failures.

[lifubang]

Do you think this is really a bug.
If the container is deleted by other process, I think runc start / run should return the exact error and have a non-zero exit code, but not to ignore some errors. Or else the users may wrongly think that we start/run the container successfully, but the result is opposite.

I think Copilot can't talk with us here.

@ningmingxiao @kolyshkin WDYT