Performance by adamruzicka · Pull Request #1035 · theforeman/foreman_remote_execution

adamruzicka · 2026-05-26T12:36:49Z

Includes #1034

Opening as a draft until I actually read through it. Whether the individual fixes should be included is up to debate.

I'd expect these two to yield the biggest benefits:

The rest are sort of nice to haves

adamruzicka · 2026-05-26T14:09:13Z

As a non-admin user, looking at a job with 5000 hosts, this fix brings the time to first full draw from ~43 seconds to ~15 seconds

Lukshio

Hi, I've finished first round of the review, and found some things to discuss.

Lukshio · 2026-05-26T15:27:20Z

-  dispatch(fetchData);
+export const getJobInvocation = url => dispatch => {
+  stopJobInvocationPolling();
+  fetchJobInvocation(dispatch, url, { include_permissions: true });


I would be very careful about calling with inlcude_permissions only once. We can use it in each request, the difference between response with and without this information should not be more significant. After that we can simplify calls by removing this getJobInvocation. All informations should be already in the repeating calls.

One of the goals here was to reduce the number of times we re-load rarely changing information we already have.

We can use it in each request, the difference between response with and without this information should not be more significant.

The assumption was that the permissions shouldn't really change as the job runs, even though technically they can and recalculating those every second is a waste in majority of cases.

the difference between response with and without this information should not be more significant.

Yes, at the same time, it is yet another thing the backend has to compute.

With that being said, I don't feel strongly about this. We may find out that the savings coming from this are negligible and then we can very well favor the code simplicity over this.

On the other hand, I assume that when user privileges changes, or user logs out, server won't respond for this recurring api request, so there still will be permissions check on server side. So we can omit it on frontend.

server won't respond for this recurring api request, so there still will be permissions check on server side

Backend always checks permissions, no matter what.

So we can omit it on frontend.

I was under the impression we load these so that we know which action elements can be shown as active. Yes, we could skip that, make everything appear to be clickable and let the backend reject what the user can't do, but then the user experience wouldn't be all that great.

If we will check it in first request, it will render buttons correctly, so there won't be any issue, and if something changes (user log out, change permissions etc.) request will fail and user will be redirected or will have to reload, that will send new first request and receive updated privileges. So this should work fine.

Lukshio · 2026-05-26T15:31:04Z

+    statusLabel === STATUS.FAILED ||
+    statusLabel === STATUS.SUCCEEDED ||
+    statusLabel === STATUS.CANCELLED;
+  const autoRefresh = task?.state === STATUS.PENDING || false;


As this is bigger refactor. We have some inconsistency across the states like this one. Auto refresh is in status pending, but it should be also in status running which we are not checking. Same issue with the Create Report button, where user can create report when the job is not finished yet. Could we introduce new state RUNNING and adjust this check for it? Or convert pending state to array and then check it like that.

If you consider it as another task, I'm okay with that.

Create Report bug: https://redhat.atlassian.net/browse/SAT-44552

I would agree that the frontend should cover all the possible values that it might get from the backend one way or another. Filed https://projects.theforeman.org/issues/39360

I would prefer to solve the task after this refactor, so we don't change each others part, wdym?

…on details The extra data it gave doesn't seem to be used anywhere in the job invocation details page.

Replace setInterval-based polling (via withInterval) with a setTimeout chain that only schedules the next request after the previous one completes or fails, preventing request pileup when the server is slow. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

The include_permissions param causes the backend to instantiate an Authorizer and check 3 permissions on every request. These are user permissions that don't change during a page session, so fetch them once on the initial request and skip them on subsequent polls. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

updateJob fetched /api/job_invocations/:id into the UPDATE_JOB Redux key, but no selector ever read from that key. The polling loop already refreshes the job state via JOB_INVOCATION_KEY every second, so these extra requests after cancel/abort/recurring actions were wasted. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

The second useEffect called filterApiCall() unconditionally whenever any dependency changed, and again when statusLabel changed — causing 2-3 identical requests on initial load. Track previous values for both initialFilter and statusLabel so filterApiCall fires only once per actual change. Also move setStatus(RESOLVED) inside the JOB_INVOCATION_HOSTS branch of handleResponse so the LIST_TEMPLATE_INVOCATIONS fetch (which doesn't populate table data) doesn't prematurely flip the table out of its loading state. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

The detail page fetched /foreman_tasks/api/tasks/:id solely to read available_actions.cancellable. Add cancellable to the task child in the job invocation response instead, eliminating a separate API call. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

handleSuccess unconditionally rescheduled the next poll, so the useEffect's stopJobInvocationPolling call raced against in-flight requests — the cleared timeout was immediately replaced by the completing fetch. Check the response data inside handleSuccess instead, and simplify the effect to only handle unmount cleanup. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

status_label is derived from task.state: it reaches a terminal value (failed/succeeded/cancelled) only when task.state == 'stopped', so checking task.state separately adds nothing. Remove the autoRefresh variable and the unused finished/autoRefresh props passed to JobInvocationHostTable. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

fetchJobInvocation hardcoded params: { include_hosts: false } and ignored its params argument. Spread the argument so the initial load can pass include_permissions: true while subsequent polls omit it. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Covers: initial fetch params (include_permissions + include_hosts), poll scheduling while running, stopping on all terminal statuses (succeeded/failed/cancelled) and on error, stopJobInvocationPolling cancelling a pending timeout, and re-entrant getJobInvocation cancelling the previous poll. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

- Inline scheduleNextPoll into handleSuccess in TemplateInvocation — removes a one-use helper and makes the self-scheduling pattern direct - Update prevFilter/prevStatusLabel refs only when the guard passes, not unconditionally before it - Remove dead UPDATE_JOB and GET_TASK constants Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Add a cancelled flag inside the TemplateInvocation useEffect closure so that handleSuccess/handleError callbacks from in-flight fetches are ignored after the effect tears down. Without this, a handleError arriving after a fast isExpanded toggle could null out the new live timeoutRef, silently breaking polling. Also remove the now-unused jobId parameter from enableRecurringLogic and cancelRecurringLogic — it was only ever passed to the removed updateJob dispatch. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Adds a self-scheduling setTimeout chain in JobInvocationHostTable that refreshes the host list every 5s while the job is not finished. Uses a ref for statusLabel so the poll callback doesn't need to be recreated on every job status update. filterApiCall cancels any pending poll before issuing a manual fetch to avoid races. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

RowActions reads permissions from selectTemplateInvocationList which is keyed by LIST_TEMPLATE_INVOCATIONS. It was only fetched once on mount (page 1 hosts), so switching pages left it stale — RowActions returned null for any host not in the initial result, hiding the actions column. Now filterApiCall and pollHostTable both refresh LIST_TEMPLATE_INVOCATIONS alongside the main hosts fetch, keeping it in sync with whatever page is currently visible. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

The RowActions component needs per-host task and permissions data to render the actions column. Previously this came from a separate non-API endpoint (list_jobs_hosts) fetched once on mount, causing the actions column to disappear on page switches since the stale data had no entries for hosts on pages 2+. Now set_statuses_and_smart_proxies builds @task_by_host and @permissions_by_host from the already-loaded template invocations and exposes them in the hosts.json.rabl response. RowActions reads directly from the paginated JOB_INVOCATION_HOSTS Redux key instead of a separate LIST_TEMPLATE_INVOCATIONS key. Drops ALL_JOB_HOSTS, LIST_TEMPLATE_INVOCATIONS, handleResponse, selectTemplateInvocationList, and the non-API /job_invocations/:id/hosts fetch entirely. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Task and permissions data is now included in /api/job_invocations/:id/hosts, so the separate non-API list_jobs_hosts action is no longer needed. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

…s param The per-host authorization checks are expensive — skip them unless the caller explicitly requests them with include_permissions=true. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

The parent already knows whether the job is done — move isJobFinished out of the table and into JobInvocationActions where the STATUS constants live, export it, and let index.js pass a plain boolean prop down. The table no longer needs to know what constitutes "finished"; it just checks the prop via a ref (same pattern, cleaner responsibility split). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Inline scheduleNextPoll (single-line wrapper with no benefit) and extract updateHostsState to deduplicate the repeated state-update block shared between pollHostTable and makeApiCall. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

cancellable? is only defined on DynflowTask; the :some_task factory uses the base class, causing test failures. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

The frontend only needs task id (for links) and cancellable (to gate cancel actions). Shape the response in the rabl template rather than serializing all task attributes. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

adamruzicka · 2026-07-07T07:38:48Z

+        return unless @include_permissions
+        @task_by_host = template_invocations.to_h do |ti|
+          task = ti.run_host_job_task
+          [ti.host_id, task]
+        end
+        @permissions_by_host = hosts.to_h do |host|
+          template_invocation = template_invocations_by_host_id[host.id]
+          task = template_invocation.try(:run_host_job_task)
+          [host.id, {
+            :view_foreman_tasks => authorized_for(:permission => :view_foreman_tasks, :auth_object => task),
+            :cancel_job_invocations => authorized_for(:permission => :cancel_job_invocations, :auth_object => @job_invocation),
+            :execute_jobs => authorized_for(controller: :job_invocations, action: :create) && (!host.infrastructure_host? || User.current.can?(:execute_jobs_on_infrastructure_hosts)),
+          }]
+        end


Not happy about this at all.

Alternatives would be:

Having a ui-specific api-like controller

as a completely separate class

as a subclass of pi::V2::JobInvocationsController,only adding bits on top

graphql

Querying two different controllers on every poll is out of the question.

adamruzicka · 2026-07-07T07:39:09Z

-  Date.prototype.toLocaleString = function (locale, options) {
-    return originalToLocaleString.call(this, locale, { ...options, timeZone: 'UTC' });
+  Date.prototype.toLocaleString = function(locale, options) {
+    return originalToLocaleString.call(this, locale, {
+      ...options,
+      timeZone: 'UTC',
+    });


adamruzicka · 2026-07-07T07:40:12Z

+          if (!jobFinishedRef.current) {
+            pollTimeoutId.current = setTimeout(pollHostTable, 5000);
+          } else {
+            pollTimeoutId.current = null;
+          }


If we wanted to go the extra mile, we could stop refreshing the page once all the hosts on the current page reach a terminal state.

pollHostTable was a no-args wrapper that always called makeApiCall with currentPollParams.current, so it can be replaced by an inline lambda. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Extract a shared extractErrorMessage helper to deduplicate the error toast fallback chain across fetchJobInvocation, cancelJob, enableRecurringLogic, and cancelRecurringLogic. Also normalize the inconsistent final fallback ('Error' vs 'Unknown error.') to always use 'Unknown error.'. Replace the redundant run_host_job_task re-lookup in set_permissions_by_host with a direct lookup from @task_by_host, which was already built from the same association. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

pr-processor Bot added Not yet reviewed Waiting on contributor labels May 26, 2026

adamruzicka force-pushed the perf branch 2 times, most recently from 7adeb14 to 671573e Compare May 26, 2026 13:46

Lukshio reviewed May 26, 2026

View reviewed changes

pr-processor Bot removed the Not yet reviewed label May 26, 2026

Lukshio self-assigned this May 26, 2026

adamruzicka force-pushed the perf branch from 671573e to 613e793 Compare May 27, 2026 12:19

pr-processor Bot added the Needs re-review label May 27, 2026

adamruzicka force-pushed the perf branch from 613e793 to 25c50ae Compare May 27, 2026 12:32

pr-processor Bot removed the Needs re-review label May 27, 2026

sbernhard mentioned this pull request Jun 11, 2026

Fixes #39336 - Batch taxonomy subtree lookups to avoid N+1 queries theforeman/foreman#10990

Merged

adamruzicka force-pushed the perf branch from 25c50ae to 43583cf Compare June 25, 2026 12:39

pr-processor Bot added the Needs re-review label Jun 25, 2026

adamruzicka and others added 8 commits July 2, 2026 15:09

Fixes #39363 - Do not ask for host statuses when reading job invocati…

9ad6855

…on details The extra data it gave doesn't seem to be used anywhere in the job invocation details page.

Replace setInterval with setTimeout chain in TemplateInvocation polling

aace775

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

adamruzicka force-pushed the perf branch from 43583cf to 8061dce Compare July 2, 2026 13:11

adamruzicka and others added 5 commits July 2, 2026 15:35

adamruzicka marked this pull request as ready for review July 2, 2026 14:55

adamruzicka and others added 12 commits July 2, 2026 17:22

Make lints happy

8d13947

Drop list_jobs_hosts — replaced by API hosts endpoint

cd207a6

Task and permissions data is now included in /api/job_invocations/:id/hosts, so the separate non-API list_jobs_hosts action is no longer needed. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Gate task and permissions in hosts response behind include_permission…

9bf9f05

…s param The per-host authorization checks are expensive — skip them unless the caller explicitly requests them with include_permissions=true. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Fix polling cleanup inconsistencies and error toast fallbacks

cb43e25

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Make lints happy

f46dde2

Use try(:cancellable?) to handle base ForemanTasks::Task in tests

9677991

cancellable? is only defined on DynflowTask; the :some_task factory uses the base class, causing test failures. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Trim task data in hosts response to id and cancellable

2421ab3

The frontend only needs task id (for links) and cancellable (to gate cancel actions). Shape the response in the rabl template rather than serializing all task attributes. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

adamruzicka commented Jul 7, 2026

View reviewed changes

pr-processor Bot removed the Needs re-review label Jul 7, 2026

Unify pollHostTable into makeApiCall

96e0aae

pollHostTable was a no-args wrapper that always called makeApiCall with currentPollParams.current, so it can be replaced by an inline lambda. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

pr-processor Bot added the Needs re-review label Jul 7, 2026

Uh oh!

Conversation

adamruzicka commented May 26, 2026

Uh oh!

adamruzicka commented May 26, 2026

Uh oh!

Lukshio 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!

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!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants