Skip to content

Fix controller hang when submission exits early #1695

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.

Sign up for GitHub

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

Open
simonlindholm wants to merge 2 commits into
base: main
Choose a base branch
from
+7 −4
Open

Fix controller hang when submission exits early #1695

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
184c031
Fix controller hang when submission exits early
simonlindholm May 13, 2026
e0af547
Improve error handling for hung interactive keeper
simonlindholm May 13, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion cms/grading/tasktypes/Interactive.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -252,7 +252,11 @@ def evaluate(self, job, file_cacher):
stdin=subprocess.DEVNULL,
stdout=subprocess.PIPE,
)
stdout, _ = p.communicate(timeout=self.controller_wall_limit * 2)
try:
stdout, _ = p.communicate(timeout=self.controller_wall_limit * 2)
except subprocess.TimeoutExpired:
p.kill()
stdout, _ = p.communicate()

KEEPER_ERROR_MESSAGE = "Internal error in interactive keeper"

Expand Down
5 changes: 2 additions & 3 deletions cms/grading/tasktypes/interactive_keeper.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,10 +87,8 @@ def main():
for i in range(process_limit):
c_to_u_r, c_to_u_w = os.pipe()
u_to_c_r, u_to_c_w = os.pipe()
os.set_inheritable(c_to_u_r, True)
os.set_inheritable(c_to_u_w, True)
os.set_inheritable(u_to_c_r, True)
os.set_inheritable(u_to_c_w, True)
pipes.append({"c_to_u": (c_to_u_r, c_to_u_w), "u_to_c": (u_to_c_r, u_to_c_w)})

controller_sandbox = Sandbox(0, shard, name="controller", temp_dir=temp_dir)
Expand Down Expand Up @@ -165,7 +163,8 @@ def main():
wait=False,
)

os.close(p["c_to_u"][0])
# We do not close p["c_to_u"][0] -- it only risks crashing the
# controller with SIGPIPE if the submission exits early.
Comment on lines +166 to +167
Copy link
Copy Markdown
Member

@prandla prandla May 13, 2026

Choose a reason for hiding this comment

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

i thought "manager gets sigpipe" is an issue that can happen anyways and that properly written managers always need to signal(SIGPIPE, SIG_IGN) anyways? (at least, looking at managers from old communication tasks, most of them do that.)

i admit this isn't very well documented, but imo leaving file descriptors open in two processes is not the right way to solve this.

i suppose this is at least not a resource leak, given that interactive_keeper should also exit shortly after the solution.

Copy link
Copy Markdown
Author

@simonlindholm simonlindholm May 14, 2026

Choose a reason for hiding this comment

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

SIGPIPE cannot happen if we leave the pipe open, so this fix should remove the need for signal(SIGPIPE, SIG_IGN). Is there any downside to leaving the pipe unclosed? IMO pipe closures are a big footgun which are worth eliminating on the judge side. Apart from the need for signal(SIGPIPE, SIG_IGN) which is very easy to forget, it's also easy for a judge to accidentally introduce excessive (well-intentioned) error checking, e.g.:

void write_message(const char* format, ...) {
    va_list args;
    va_start(args, format);
    if (vfprintf(fifo_out, format, args) <= 0) wrong_answer("Pipe to contestant closed unexpectedly.");
    if (fflush(fifo_out) == EOF) wrong_answer("Pipe to contestant closed unexpectedly.");
}

In a case where a submission prints a query and then exits without reading the response, this results in a race condition where depending on timing the submission may either get accepted or wrong answer. I'd argue strongly that the correct behavior here is to ignore errors, so such a submission would always get accepted. This is similar to how it's acceptable for non-interactive submission to not read the entire input.

(It would be good to also fix this for regular interactive problems, not of the Interactive task type, and it would also be good to properly look at the order in which manager/controller vs submission exits with various verdicts. See also discussion in Kattis/problemtools#77. I don't think I have the capacity to work on this, however.)

os.close(p["u_to_c"][1])

try:
Expand Down
Loading