When I moved the code to posix_spawn, I tried to preserve the pre-posix_spawn behavior. It looks like the close() call is present on the lhs too: https://github.com/ninja-build/ninja/commit/89587196705f54afb904c8f4572e65de7274dd81
I agree with what Evan says: SetCloseOnExec() does look unnecessary, but carefully testing such a change is a good idea.
As far as I can tell, POSIX isn't super clear on what posix_spawn() should do in the presence of already-closed file descriptors that are CLOEXEC. https://pubs.opengroup.org/onlinepubs/007904975/functions/posix_spawn.html
mentions CLOEXEC as a fourth phase after process creation, attrp and file action processing. The "Errors" section says that the function may fail if process creation ("fork/exec"), attrp or file action processing fails. It doesn't mention cloexec processing, which happens after these 3 things. This weakly suggests that CLOEXEC on closed fds shouldn't make posix_spawn fail.
says "File descriptors open in the calling process image shall remain open in the new process image, except for those whose close-on- exec flag FD_CLOEXEC is set." So exec() is fairly explicit about cloexec on already-closed fds just being a no-op. As far as I known, the intent of the posix_spawn spec is that posix_spawn should be implementable by calling fork(), and then in the child processing attrp and file actions, followed by exec. Since exec() doesn't make this an error, that also suggests that this shouldn't make posix_spawn fail.
So while the SetCloseOnExec() calls is (probably) not needed, a pedantic reading of posix suggests it's not strictly wrong either, as far as I understand. But relying on this was certainly not intentional :)
(I admire your confidence that you having written something means that it's unlikely to have bugs! For me, it's usually the opposite.)