Skip to content

Add MAX_EXECUTIONS environment variable limit #5272

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
jardondiego wants to merge 6 commits into
base: master
Choose a base branch
from
+53 −0
Open

Add MAX_EXECUTIONS environment variable limit #5272

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
acb464c
Add MAX_EXECUTIONS environment variable limit
May 12, 2026
6d5eb78
Fix MAX_EXECUTIONS infinite loop and exception in task_loop
May 13, 2026
4b80b3b
Log error if MAX_EXECUTIONS is not an integer
May 13, 2026
da2f31d
Refactor MAX_EXECUTIONS validation for better readability
May 13, 2026
c6203fd
Extract _get_max_executions helper for cleaner task_loop logic
May 13, 2026
5326c42
Fix unused variables and whitespace in run_bot_test.py
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
32 changes: 32 additions & 0 deletions src/clusterfuzz/_internal/tests/core/bot/startup/run_bot_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,9 @@ def setUp(self):
'clusterfuzz._internal.bot.tasks.commands.process_command',
'clusterfuzz._internal.bot.tasks.update_task.run',
'clusterfuzz._internal.bot.tasks.update_task.track_revision',
'python.bot.startup.run_bot.update_task_enabled',
])
self.mock.update_task_enabled.return_value = True

self.task = mock.MagicMock()
self.task.payload.return_value = 'payload'
Expand All @@ -107,6 +109,36 @@ def test_exception(self):
self.assertFalse(clean_exit)
self.assertEqual('payload', payload)

def test_max_executions(self):
"""Test that the loop breaks after MAX_EXECUTIONS iterations."""
from clusterfuzz._internal.system import environment
environment._initial_environment = None
os.environ['MAX_EXECUTIONS'] = '3'
# Use side_effect to avoid raising an exception so it loops cleanly.
self.mock.process_command.side_effect = None
Copy link
Copy Markdown
Collaborator

@letitz letitz May 19, 2026

Choose a reason for hiding this comment

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

I guess this works, but do we even need it? And why not use return_value instead?


_, clean_exit, payload = run_bot.task_loop()

self.assertEqual(3, self.mock.process_command.call_count)
self.assertTrue(clean_exit)
self.assertEqual('payload', payload)

@mock.patch('clusterfuzz._internal.metrics.logs.error')
def test_max_executions_invalid(self, mock_error):
"""Test that an invalid MAX_EXECUTIONS logs an error and is removed."""
from clusterfuzz._internal.system import environment
environment._initial_environment = None
os.environ['MAX_EXECUTIONS'] = 'invalid'

# We need the loop to exit otherwise it will run indefinitely because we removed the limit.
# We can make process_command succeed once, then raise an exception so it exits.
self.mock.process_command.side_effect = [None, Exception('exit')]

_, _, _ = run_bot.task_loop()

mock_error.assert_any_call('Invalid value for MAX_EXECUTIONS: invalid')
self.assertEqual(2, self.mock.process_command.call_count)


class LeaseAllTasksTest(unittest.TestCase):
"""Tests for lease_all_tasks."""
Expand Down
21 changes: 21 additions & 0 deletions src/python/bot/startup/run_bot.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,12 +114,26 @@ def schedule_utask_mains():
task.pubsub_task.cancel_lease_ack()


def _get_max_executions():
"""Returns the MAX_EXECUTIONS limit as an int, or None if invalid/unset."""
val = environment.get_value('MAX_EXECUTIONS')
Copy link
Copy Markdown
Collaborator

@letitz letitz May 19, 2026

Choose a reason for hiding this comment

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

Ah, and one final comment: max_executions is a bit vague - clusterfuzz also runs fuzzers that have their own concept of executions, execution speed, etc.. Consider max_task_executions to disambiguate.

if not val:
return None
try:
return int(val)
except ValueError:
Copy link
Copy Markdown
Collaborator

@letitz letitz May 19, 2026

Choose a reason for hiding this comment

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

I think we should instead loudly fail and exit if the environment variable is wrong. It would surface configuration issues immediately, right before we start the task loop, whereas logging an error gets a bit lost in the noise.

logs.error(f'Invalid value for MAX_EXECUTIONS: {val}')
environment.remove_key('MAX_EXECUTIONS')
return None


def task_loop():
"""Executes tasks indefinitely."""
# Defer heavy task imports to prevent issues with multiprocessing.Process
from clusterfuzz._internal.bot.tasks import commands

clean_exit = False
execution_count = 0
while True:
stacktrace = ''
exception_occurred = False
Expand Down Expand Up @@ -191,6 +205,13 @@ def task_loop():
time.sleep(utils.random_number(1, failure_wait_interval))
break

execution_count += 1
max_executions = _get_max_executions()
Copy link
Copy Markdown
Collaborator

@letitz letitz May 19, 2026

Choose a reason for hiding this comment

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

This should move to above the while loop. There is no reason to re-check the environment variable for a new value on every iteration - it should have the same value for the lifetime of the whole process.

This also ties in with loudly failing on invalid values. If we check the value before we even enter the loop, we will immediately surface configuration errors instead of waiting until after we run a task to notice the error.

Finally, consider switching to a for loop instead. It looks like itertools.repeat() would work well here:

max_executions = _get_max_executions()
for _ in itertools.repeat(None, times=max_executions):
  ...

See https://docs.python.org/3/library/itertools.html#itertools.repeat

I guess the downside of the for loop is that it's harder to log the clean exit message.

if max_executions and execution_count >= max_executions:
logs.info(f'Reached MAX_EXECUTIONS limit ({max_executions}). Exiting.')
clean_exit = True
break

task_payload = task.payload() if task else None
return stacktrace, clean_exit, task_payload

Expand Down
Loading