Skip to content

Fix controller hang when submission exits early#1695

Open
simonlindholm wants to merge 2 commits into
cms-dev:mainfrom
simonlindholm:fix-interactive-hang
Open

Fix controller hang when submission exits early#1695
simonlindholm wants to merge 2 commits into
cms-dev:mainfrom
simonlindholm:fix-interactive-hang

Conversation

@simonlindholm
Copy link
Copy Markdown

Having the u_to_c_w fd inherit to the controller means that when the submission exits, the controller will never read EOF, because it still itself has the write end of the pipe open. Easy fix is just to make that not inherit: nothing actually needs the inheritance, in fact it's rather a bit of a security issue that each spawned submission get access to extra fd's.

Small additional related fixes:

  • if the interactive keeper hits walltime limit (claude pointed out some obscure circumstances in which this can happen, e.g. if the submission causes lots of isolate warning spam which clogs an stderr pipe which is never read from), we currently crash and leave the child un-waited. Better to generate a proper error.
  • the change to make c_to_u_r not inherit means that now if the submission exits, controller writing will cause SIGPIPE unless the controller ignores that signal. We don't particularly want that to happen (it makes it impossible to detect which process exited first, which we really ought to do to give proper verdicts); better to leave the pipe unclosed.

@codecov
Copy link
Copy Markdown

codecov Bot commented May 13, 2026

Codecov Report

❌ Patch coverage is 0% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 53.89%. Comparing base (6a28840) to head (e0af547).
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
cms/grading/tasktypes/Interactive.py 0.00% 5 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1695   +/-   ##
=======================================
  Coverage   53.89%   53.89%           
=======================================
  Files         341      341           
  Lines       27977    27978    +1     
=======================================
+ Hits        15077    15078    +1     
  Misses      12900    12900           
Flag Coverage Δ
functionaltests 0.00% <0.00%> (ø)
unittests 53.89% <0.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

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

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants