From 45a96e24303122636c27b2cee6f720db009b008a Mon Sep 17 00:00:00 2001 From: Tim McCormack Date: Mon, 14 Apr 2025 13:01:09 -0400 Subject: [PATCH 1/4] feat: Run remote codejail even if unexpected exception in local safe_exec During dark launch of remote codejail, we want to ensure we always run both local and remote execution -- otherwise we're missing data for the remote side in an important situation. This will help answer the question of whether the unexpected exception happens on both sides, even though it may not look exactly the same due to differences in how unexpected errors are handled. An example input that provokes this in unsafe execution mode is `raise BaseException("hi")`; in safe execution mode, printing to `sys.__stdout__` should also produce an appropriate error. --- xmodule/capa/safe_exec/safe_exec.py | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/xmodule/capa/safe_exec/safe_exec.py b/xmodule/capa/safe_exec/safe_exec.py index 882db11fd1..828c3e160f 100644 --- a/xmodule/capa/safe_exec/safe_exec.py +++ b/xmodule/capa/safe_exec/safe_exec.py @@ -159,6 +159,8 @@ def safe_exec( raise SafeExecException(emsg) return + cacheable = True # unless we get an unexpected error + # Create the complete code we'll run. code_prolog = CODE_PROLOG % random_seed @@ -196,11 +198,17 @@ def safe_exec( limit_overrides_context=limit_overrides_context, slug=slug, ) - except SafeExecException as e: + except BaseException as e: # Saving SafeExecException e in exception to be used later. exception = e emsg = str(e) + if not isinstance(exception, SafeExecException): + # Something unexpected happened, so don't cache this evaluation. + # (We may decide to cache these in the future as well; this is just + # preserving existing behavior during a refactor of error handling.) + cacheable = False else: + exception = None emsg = None # Run the code in both the remote codejail service as well as the local codejail @@ -235,10 +243,10 @@ def safe_exec( # Put the result back in the cache. This is complicated by the fact that # the globals dict might not be entirely serializable. - if cache: + if cache and cacheable: cleaned_results = json_safe(globals_dict) cache.set(key, (emsg, cleaned_results)) # If an exception happened, raise it now. - if emsg: + if exception: raise exception From 3f2271ab6923d5e109634dc1dee02f8781791caf Mon Sep 17 00:00:00 2001 From: Tim McCormack Date: Mon, 14 Apr 2025 13:30:59 -0400 Subject: [PATCH 2/4] feat: Catch all exceptions from codejail dark launch - Catch all exceptions, not just Exception, to better prevent errors from interfering with mainline responses. - Introduce a separate try block around the monitoring code so that bugs there don't cause issues. - Print exception information as well for both sides (but only if not a SafeExecException, which is redundant with emsg). Some formatting changes to log messages as well. Example outputs: For `1/0`: ``` 2025-04-14 17:26:34,239 INFO 10232 [xmodule.capa.safe_exec.safe_exec] [user 3] [ip 172.18.0.1] safe_exec.py:240 - Remote execution in darklaunch mode produces globals={'expect': None, 'ans': '1/0'}, emsg=None, exception=None 2025-04-14 17:26:34,239 INFO 10232 [xmodule.capa.safe_exec.safe_exec] [user 3] [ip 172.18.0.1] safe_exec.py:245 - Local execution in darklaunch mode produces globals={'expect': None, 'ans': '1/0'}, emsg='ZeroDivisionError: division by zero', exception=None ``` For `raise BaseException("hi")`: ``` 2025-04-14 17:26:13,359 INFO 10232 [xmodule.capa.safe_exec.safe_exec] [user 3] [ip 172.18.0.1] safe_exec.py:240 - Remote execution in darklaunch mode produces globals={'expect': None, 'ans': 'raise BaseException("hi")'}, emsg=None, exception=None 2025-04-14 17:26:13,359 INFO 10232 [xmodule.capa.safe_exec.safe_exec] [user 3] [ip 172.18.0.1] safe_exec.py:245 - Local execution in darklaunch mode produces globals={'expect': None, 'ans': 'raise BaseException("hi")'}, emsg='hi', exception=BaseException('hi') ``` With codejail-service down, and `out = 1 + 2`: ``` 2025-04-14 17:30:28,597 INFO 12484 [xmodule.capa.safe_exec.safe_exec] [user 3] [ip 172.18.0.1] safe_exec.py:241 - Remote execution in darklaunch mode produces globals={'expect': None, 'ans': 'out = 1 + 2', 'out': 3, 'cfn_return': {'input_list': [{'ok': True, 'msg': 'Output:\n3', 'grade_decimal': 1}]}}, emsg=None, exception=CodejailServiceUnavailable('Codejail API Service is unavailable. Please try again in a few minutes.') 2025-04-14 17:30:28,597 INFO 12484 [xmodule.capa.safe_exec.safe_exec] [user 3] [ip 172.18.0.1] safe_exec.py:246 - Local execution in darklaunch mode produces globals={'expect': None, 'ans': 'out = 1 + 2', 'out': 3, 'cfn_return': {'input_list': [{'ok': True, 'msg': 'Output:\n3', 'grade_decimal': 1}]}}, emsg=None, exception=None ``` --- xmodule/capa/safe_exec/safe_exec.py | 26 +++++++++++++++++++------- 1 file changed, 19 insertions(+), 7 deletions(-) diff --git a/xmodule/capa/safe_exec/safe_exec.py b/xmodule/capa/safe_exec/safe_exec.py index 828c3e160f..29661f573b 100644 --- a/xmodule/capa/safe_exec/safe_exec.py +++ b/xmodule/capa/safe_exec/safe_exec.py @@ -227,18 +227,30 @@ def safe_exec( "extra_files": extra_files, } with function_trace('safe_exec.remote_exec_darklaunch'): - remote_emsg, _remote_exception = get_remote_exec(data) + remote_emsg, _ = get_remote_exec(data) + except BaseException as e: # pragma: no cover # pylint: disable=broad-except + # Swallow all exceptions and log it in monitoring so that dark launch doesn't cause issues during + # deploy. + remote_emsg = None + remote_exception = e + else: + remote_emsg = None + remote_exception = None + + try: log.info( - f"Remote execution in darklaunch mode produces: {darklaunch_globals} or exception: {remote_emsg}" + f"Remote execution in darklaunch mode produces globals={darklaunch_globals!r}, " + f"emsg={remote_emsg!r}, exception={remote_exception!r}" ) - log.info(f"Local execution in darklaunch mode produces: {globals_dict} or exception: {emsg}") + local_exc_unexpected = None if isinstance(exception, SafeExecException) else exception + log.info( + f"Local execution in darklaunch mode produces globals={globals_dict!r}, " + f"emsg={emsg!r}, exception={local_exc_unexpected!r}") set_custom_attribute('dark_launch_emsg_match', remote_emsg == emsg) set_custom_attribute('remote_emsg_exists', remote_emsg is not None) set_custom_attribute('local_emsg_exists', emsg is not None) - except Exception as e: # pragma: no cover # pylint: disable=broad-except - # Swallows all exceptions and logs it in monitoring so that dark launch doesn't cause issues during - # deploy. - log.exception("Error occurred while trying to remote exec in dark launch mode.") + except BaseException as e: # pragma: no cover # pylint: disable=broad-except + log.exception("Error occurred while trying to report codejail darklauch data.") record_exception() # Put the result back in the cache. This is complicated by the fact that From fd66048a491c24357f2851d6805d8e333b227190 Mon Sep 17 00:00:00 2001 From: Tim McCormack Date: Tue, 15 Apr 2025 13:57:34 -0400 Subject: [PATCH 3/4] test: Improve codejail darklaunch tests - Separate test for misconfiguration - Add helper method for generic dark launch testing - Test two darklaunch scenarios: Globals interference, and error that would previously have caused the remote side not to run - Rename mocks to have our usual `mock_` prefix --- .../capa/safe_exec/tests/test_safe_exec.py | 153 ++++++++++++++---- 1 file changed, 121 insertions(+), 32 deletions(-) diff --git a/xmodule/capa/safe_exec/tests/test_safe_exec.py b/xmodule/capa/safe_exec/tests/test_safe_exec.py index 7cb74c9402..7bba31a34d 100644 --- a/xmodule/capa/safe_exec/tests/test_safe_exec.py +++ b/xmodule/capa/safe_exec/tests/test_safe_exec.py @@ -6,7 +6,7 @@ import os import os.path import textwrap import unittest -from unittest.mock import patch +from unittest.mock import call, patch import pytest import random2 as random @@ -132,62 +132,151 @@ class TestCodeJailDarkLaunch(unittest.TestCase): """ @patch('xmodule.capa.safe_exec.safe_exec.get_remote_exec') @patch('xmodule.capa.safe_exec.safe_exec.codejail_safe_exec') - def test_default_code_execution(self, local_exec, remote_exec): + def test_default_code_execution(self, mock_local_exec, mock_remote_exec): # Test default only runs local exec. g = {} safe_exec('a=1', g) - assert local_exec.called - assert not remote_exec.called + assert mock_local_exec.called + assert not mock_remote_exec.called @override_settings(ENABLE_CODEJAIL_REST_SERVICE=True) @patch('xmodule.capa.safe_exec.safe_exec.get_remote_exec') @patch('xmodule.capa.safe_exec.safe_exec.codejail_safe_exec') - def test_code_execution_only_codejail_service(self, local_exec, remote_exec): + def test_code_execution_only_codejail_service(self, mock_local_exec, mock_remote_exec): # Set return values to empty values to indicate no error. - remote_exec.return_value = (None, None) + mock_remote_exec.return_value = (None, None) # Test with only the service enabled. g = {} safe_exec('a=1', g) - assert not local_exec.called - assert remote_exec.called + assert not mock_local_exec.called + assert mock_remote_exec.called @override_settings(ENABLE_CODEJAIL_DARKLAUNCH=True) @patch('xmodule.capa.safe_exec.safe_exec.get_remote_exec') @patch('xmodule.capa.safe_exec.safe_exec.codejail_safe_exec') - def test_code_execution_darklaunch(self, local_exec, remote_exec): - # Set return values to empty values to indicate no error. - remote_exec.return_value = (None, None) - g = {} + def test_code_execution_darklaunch_misconfig(self, mock_local_exec, mock_remote_exec): + """Test that darklaunch doesn't run when remote service is generally enabled.""" + mock_remote_exec.return_value = (None, None) - # Verify that incorrect config runs only remote and not both. with override_settings(ENABLE_CODEJAIL_REST_SERVICE=True): - safe_exec('a=1', g) - assert not local_exec.called - assert remote_exec.called + safe_exec('a=1', {}) - local_exec.reset_mock() - remote_exec.reset_mock() + assert not mock_local_exec.called + assert mock_remote_exec.called - # Set up side effects to mimic the real behavior of modifying the globals_dict. - def local_side_effect(*args, **kwargs): - test_globals = args[1] - test_globals['test'] = 'local_test' + @override_settings(ENABLE_CODEJAIL_DARKLAUNCH=True) + def run_dark_launch( + self, globals_dict, local, remote, + expect_attr_calls, expect_log_info_calls, expect_globals_contains, + ): + """ + Run a darklaunch scenario with mocked out local and remote execution. - def remote_side_effect(*args, **kwargs): - test_globals = args[0]['globals_dict'] - test_globals['test'] = 'remote_test' + Asserts set_custom_attribute and log.info calls and (partial) contents + of globals dict. - local_exec.side_effect = local_side_effect - remote_exec.side_effect = remote_side_effect + Return value is a dictionary of: + + - 'raised': Exception that safe_exec raised, or None. + """ assert is_codejail_in_darklaunch() - safe_exec('a=1', g) - assert local_exec.called - assert remote_exec.called - # Verify that the local/default behavior currently wins out. - assert g['test'] == 'local_test' + with ( + patch('xmodule.capa.safe_exec.safe_exec.codejail_safe_exec') as mock_local_exec, + patch('xmodule.capa.safe_exec.safe_exec.get_remote_exec') as mock_remote_exec, + patch('xmodule.capa.safe_exec.safe_exec.set_custom_attribute') as mock_set_custom_attribute, + patch('xmodule.capa.safe_exec.safe_exec.log.info') as mock_log_info, + ): + mock_local_exec.side_effect = local + mock_remote_exec.side_effect = remote + + try: + safe_exec("", globals_dict) + except BaseException as e: + safe_exec_e = e + else: + safe_exec_e = None + + # Always want both sides to be called + assert mock_local_exec.called + assert mock_remote_exec.called + + mock_set_custom_attribute.assert_has_calls(expect_attr_calls, any_order=True) + mock_log_info.assert_has_calls(expect_log_info_calls, any_order=True) + + for (k, v) in expect_globals_contains.items(): + assert globals_dict[k] == v + + return {'raised': safe_exec_e} + + def test_separate_globals(self): + """Test that local and remote globals are isolated from each other's side effects.""" + # Both will attempt to read and write the 'overwrite' key. + globals_dict = {'overwrite': 'original'} + + def local_exec(code, globals_dict, **kwargs): + globals_dict['overwrite'] = 'mock local' + + def remote_exec(data): + data['globals_dict']['overwrite'] = 'mock remote' + return (None, None) + + results = self.run_dark_launch( + globals_dict=globals_dict, local=local_exec, remote=remote_exec, + expect_attr_calls=[ + call('local_emsg_exists', False), + call('remote_emsg_exists', False), + call('dark_launch_emsg_match', True), + ], + expect_log_info_calls=[ + call( + "Local execution in darklaunch mode produces globals={'overwrite': 'mock local'}, " + "emsg=None, exception=None" + ), + call( + "Remote execution in darklaunch mode produces globals={'overwrite': 'mock remote'}, " + "emsg=None, exception=None" + ), + ], + # Should only see behavior of local exec + expect_globals_contains={'overwrite': 'mock local'}, + ) + assert results['raised'] is None + + def test_remote_runs_even_if_local_raises(self): + """Test that remote exec runs even if local raises.""" + def local_exec(code, globals_dict, **kwargs): + # Raise something other than a SafeExecException. + raise BaseException("unexpected") + + def remote_exec(data): + return (None, None) + + results = self.run_dark_launch( + globals_dict={}, local=local_exec, remote=remote_exec, + expect_attr_calls=[ + call('local_emsg_exists', True), + call('remote_emsg_exists', False), + call('dark_launch_emsg_match', False), + ], + expect_log_info_calls=[ + call( + "Remote execution in darklaunch mode produces globals={}, " + "emsg=None, exception=None" + ), + call( + "Local execution in darklaunch mode produces globals={}, " + "emsg='unexpected', exception=BaseException('unexpected')" + ), + ], + expect_globals_contains={}, + ) + + # Unexpected errors from local safe_exec propagate up. + assert isinstance(results['raised'], BaseException) + assert 'unexpected' in repr(results['raised']) class TestLimitConfiguration(unittest.TestCase): From bf2f8c3705e752c9f4d031268b8a6561fe674820 Mon Sep 17 00:00:00 2001 From: Tim McCormack Date: Tue, 15 Apr 2025 14:16:56 -0400 Subject: [PATCH 4/4] fix: Don't let local codejail exec pollute darklaunched remote globals We were running local exec before making the copy of globals_dict for remote_exec, so remote exec has been getting a polluted version of the globals. --- xmodule/capa/safe_exec/safe_exec.py | 7 +++++-- xmodule/capa/safe_exec/tests/test_safe_exec.py | 17 +++++++++++++++++ 2 files changed, 22 insertions(+), 2 deletions(-) diff --git a/xmodule/capa/safe_exec/safe_exec.py b/xmodule/capa/safe_exec/safe_exec.py index 29661f573b..b3a1b888f6 100644 --- a/xmodule/capa/safe_exec/safe_exec.py +++ b/xmodule/capa/safe_exec/safe_exec.py @@ -180,6 +180,11 @@ def safe_exec( else: + # Create a copy so the originals are not modified as part of this call. + # This has to happen before local exec is run, since globals are modified + # as a side effect. + darklaunch_globals = copy.deepcopy(globals_dict) + # Decide which code executor to use. if unsafely: exec_fn = codejail_not_safe_exec @@ -215,8 +220,6 @@ def safe_exec( # when in darklaunch mode. if is_codejail_in_darklaunch(): try: - # Create a copy so the originals are not modified as part of this call. - darklaunch_globals = copy.deepcopy(globals_dict) data = { "code": code_prolog + LAZY_IMPORTS + code, "globals_dict": darklaunch_globals, diff --git a/xmodule/capa/safe_exec/tests/test_safe_exec.py b/xmodule/capa/safe_exec/tests/test_safe_exec.py index 7bba31a34d..a45c5911a2 100644 --- a/xmodule/capa/safe_exec/tests/test_safe_exec.py +++ b/xmodule/capa/safe_exec/tests/test_safe_exec.py @@ -1,6 +1,7 @@ """Test safe_exec.py""" +import copy import hashlib import os import os.path @@ -216,10 +217,21 @@ class TestCodeJailDarkLaunch(unittest.TestCase): # Both will attempt to read and write the 'overwrite' key. globals_dict = {'overwrite': 'original'} + local_globals = None + remote_globals = None + def local_exec(code, globals_dict, **kwargs): + # Preserve what local exec saw + nonlocal local_globals + local_globals = copy.deepcopy(globals_dict) + globals_dict['overwrite'] = 'mock local' def remote_exec(data): + # Preserve what remote exec saw + nonlocal remote_globals + remote_globals = copy.deepcopy(data['globals_dict']) + data['globals_dict']['overwrite'] = 'mock remote' return (None, None) @@ -245,6 +257,11 @@ class TestCodeJailDarkLaunch(unittest.TestCase): ) assert results['raised'] is None + # Both arms should have only seen the original globals object, untouched + # by the other arm. + assert local_globals == {'overwrite': 'original'} + assert remote_globals == {'overwrite': 'original'} + def test_remote_runs_even_if_local_raises(self): """Test that remote exec runs even if local raises.""" def local_exec(code, globals_dict, **kwargs):