worker: run repo maintenance during idle time (Bug 2037216) by cgsheeh · Pull Request #1135 · mozilla-conduit/lando

cgsheeh · 2026-05-05T19:47:39Z

Lando's workers typically run repo-cleaning commands at
the beginning of each job processing time. In hg workers,
we run hg strip at the start of each job to remove
previously-created stale commits, despite those commits
not interfering with the job completion. In Git, we take
the opposite approach and simply ignore the temporary
work branch - it is never cleaned up.

Add a new "repo maintenance" step that runs while the
worker is being throttled due to no jobs remaining in
the queue. To avoid running excessive maintenance, the
runtime of the last maintenance run for each repo is
recorded in the worker, and maintenance is skipped if
it has been completed within the threshold.

For Mercurial, move the hg strip command into this maintenance
task, which should save us about 8s for each push to try.
For Git, add a cleanup of the stale working branches,
so we no longer have thousands of temp branches in our
worker repos.

After this change, each HgSCM.clean_repo call sites
always pass strip_non_public_commits=False, while
GitSCM.clean_repo call sites always pass True.
Remove the kwarg and make each behaviour the default.

Lando's workers typically run repo-cleaning commands at the beginning of each job processing time. In hg workers, we run `hg strip` at the start of each job to remove previously-created stale commits, despite those commits not interfering with the job completion. In Git, we take the opposite approach and simply ignore the temporary work branch - it is never cleaned up. Add a new "repo maintenance" step that runs while the worker is being throttled due to no jobs remaining in the queue. To avoid running excessive maintenance, the runtime of the last maintenance run for each repo is recorded in the worker, and maintenance is skipped if it has been completed within the threshold. For Mercurial, move the `hg strip` command into this maintenance task, which should save us about 8s for each push to try. For Git, add a cleanup of the stale working branches, so we no longer have thousands of temp branches in our worker repos. After this change, each `HgSCM.clean_repo` call sites always pass `strip_non_public_commits=False`, while `GitSCM.clean_repo` call sites always pass `True`. Remove the kwarg and make each behaviour the default.

github-actions · 2026-05-05T19:47:49Z

View this pull request in Lando to land it once approved.

zzzeid

Few small comments (and note failing tests).

zzzeid · 2026-05-05T19:56:20Z

+        """Delete leftover `lando-<timestamp>` work branches.
+
+        Each landing creates a fresh work branch in `update_repo`, and they
+        accumulate on disk indefinitely. Idle-time cleanup keeps the local
+        branch list small without affecting per-job latency.
+        """
+        branches = self._git_run(
+            "for-each-ref",
+            "--format=%(refname:short)",
+            "refs/heads/lando-*",
+            cwd=self.path,
+        ).splitlines()
+        if not branches:
+            return
+
+        # `git branch -D` refuses to delete the currently checked-out branch,
+        # so move off any `lando-*` branch first.
+        if self.get_current_branch().startswith("lando-"):
+            self._git_run("checkout", "--force", self.default_branch, cwd=self.path)
+
+        self._git_run("branch", "-D", *branches, cwd=self.path)


This should go in its own method that is called withinmaintenance, so that we can add other maintenance methods as needed, if needed.

I'd rather hold off on this until we have a second maintenance task that justifies the split. Extracting the body into a helper would leave maintenance() as a one-line wrapper, which isn't really useful. We're assuming we will know what the next maintenance step might look like, and if we're wrong it just creates more work for the next person who adds it.

Could hold off, but at that point I would suggest either modifying the docstring (to be more centered around what the maintenance concept is about, and adding more detail in the summary), or renaming the method to delete_job_branches.

would leave maintenance() as a one-line wrapper

We do that elsewhere, nothing wrong with that (see run_code_formatters).

zzzeid · 2026-05-05T19:59:41Z

+        except HgException:
+            pass
+        finally:
+            self.hg_repo.close()


Same comment here, this should go in a separate method that is called within maintenance.

See my comment on the Git version of maintenance.

See my reply as well in that thread.

shtrom · 2026-05-06T03:05:17Z

-        with pytest.raises(HgCommandError, match="no changes found"):
-            repo.run_hg_cmds([["outgoing"]])
-        assert not repo.run_hg_cmds([["status"]])
+    with repo.for_pull(), hg_clone.as_cwd():


What is the purpose of the hg_clone.as_cwd() here?

shtrom · 2026-05-06T03:06:11Z

@@ -31,7 +31,7 @@ def test_integrated_hgrepo_clean_repo(hg_clone):
    repo = HgSCM(hg_clone.strpath)


nit: Could we rename this variable to scm while we're at it?

repo = HgSCM() is the convention in this file, oddly enough. I updated this instance, but we should fix the others in a follow-up. :)

It pre-dates the SCM split (;

shtrom · 2026-05-06T03:07:42Z

+
+
+@pytest.fixture
+def mocked_enabled_repos(hg_landing_worker):


Could we use the git_landing_worker instead? This may exist for longer than the hg one.

I tried switching it over but hit test failures, filed https://bugzilla.mozilla.org/show_bug.cgi?id=2037553 to track the underlying issue.

shtrom · 2026-05-06T03:09:10Z

+    assert len(mocked_enabled_repos) >= 2, "Test requires at least two enabled repos."
+
+    failing_repo, *healthy_repos = mocked_enabled_repos
+    failing_repo._scm.maintenance.side_effect = SCMException("boom", "", "")


zzzeid

Few additional comments but looks good otherwise.

zzzeid · 2026-05-06T14:03:48Z

+        # `git branch -D` refuses to delete the currently checked-out branch,
+        # so move off any `lando-*` branch first.


Interesting bit here. In the future it might make more sense to ensure we are back on the default branch after a job is finished.

zzzeid · 2026-05-06T14:05:05Z

+
+        # `git branch -D` refuses to delete the currently checked-out branch,
+        # so move off any `lando-*` branch first.
+        if self.get_current_branch().startswith("lando-"):


nit: with our current setup, this is a little redundant. Seems that deterministically, this if statement will always be True with the exception of perhaps first start.

nit: self.get_current_branch().startswith("lando-") (or even "lando-") should probably be in their own property/method/variable. E.g., self.is_on_job_branch or something similar.

zzzeid · 2026-05-06T14:11:09Z

+        """Delete leftover `lando-<timestamp>` work branches.
+
+        Each landing creates a fresh work branch in `update_repo`, and they
+        accumulate on disk indefinitely. Idle-time cleanup keeps the local
+        branch list small without affecting per-job latency.
+        """
+        branches = self._git_run(
+            "for-each-ref",
+            "--format=%(refname:short)",
+            "refs/heads/lando-*",
+            cwd=self.path,
+        ).splitlines()
+        if not branches:
+            return
+
+        # `git branch -D` refuses to delete the currently checked-out branch,
+        # so move off any `lando-*` branch first.
+        if self.get_current_branch().startswith("lando-"):
+            self._git_run("checkout", "--force", self.default_branch, cwd=self.path)
+
+        self._git_run("branch", "-D", *branches, cwd=self.path)


Could hold off, but at that point I would suggest either modifying the docstring (to be more centered around what the maintenance concept is about, and adding more detail in the summary), or renaming the method to delete_job_branches.

would leave maintenance() as a one-line wrapper

We do that elsewhere, nothing wrong with that (see run_code_formatters).

zzzeid · 2026-05-06T14:12:34Z

+        except HgException:
+            pass
+        finally:
+            self.hg_repo.close()


See my reply as well in that thread.

cgsheeh requested a review from a team as a code owner May 5, 2026 19:47

zzzeid requested changes May 5, 2026

View reviewed changes

cgsheeh added 9 commits May 5, 2026 16:40

add treestatusdouble to landing worker fixtures

02c3eac

docstring

7d271d1

clarify docstring

9e9bafb

comment clarify

2ef9a61

less comment

d19a8ab

revert to old docstring state

3a62160

set maintenance interval via config on Worker class

6fa23f7

add db migration

da14d67

use -infinity instead of 0 as default last_maintenance

80563fe

cgsheeh requested review from shtrom and zzzeid May 6, 2026 03:10

shtrom approved these changes May 6, 2026

View reviewed changes

zzzeid approved these changes May 6, 2026

View reviewed changes

		@@ -31,7 +31,7 @@ def test_integrated_hgrepo_clean_repo(hg_clone):
		repo = HgSCM(hg_clone.strpath)

		# `git branch -D` refuses to delete the currently checked-out branch,
		# so move off any `lando-*` branch first.

Conversation

cgsheeh commented May 5, 2026

Uh oh!

github-actions Bot commented May 5, 2026

Uh oh!

zzzeid left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zzzeid left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

zzzeid left a comment •

edited

Loading