Wait for task no longer #9352

kbrock · 2025-02-19T21:58:39Z

Overview

Depends upon

Issue

The wait_for_task mechanism relies upon the session.
Every http request changes the session. So when multiple requests come in, updates to the session can overwrite the changes done by another request.

Example:

Host's display of Cap&U was breaking because hits to notifications were overwriting the session variables. Note: VM's display of Cap&U was working fine because it did not have a hit to notifications (and therefore didn't have this race condition).

Solution

Move wait_for_task parameters from the session to the url.

Before

initiate_wait_for_task sets session[:async][:params] with action and params{}
browser_refresh_task puts shim into web page
browser polls wait_for_task waiting for task to complete
when task is complete, session[:async][:params] is used to complete action

After

initiate_wait_for_task gathers async_params
browser_refresh_task puts shim into web page with async_params
browser polls wait_for_task waiting for task to complete.
when task is complete, url parameters (async_params) is used to complete action

kbrock · 2025-02-19T22:05:05Z

@Fryguy This still stores the parameters in the session to be backwards compatible. Do you want to change 10 other controllers and remove this completely from the session?

Pro: eradicates race condition and completely removes session values
Con: This is an extra to 10 controllers (25 lines)

Fryguy · 2025-02-19T22:12:34Z

app/controllers/application_controller/wait_for_task.rb

    end

-    browser_refresh_task(task_id, !!options[:flash])
+    session[:async][:params] = async_params


I think you need the ||= {} up above to ensure this path exists.

good eye.
My question below is if we can drop session[:async] all together.
In the meantime, I'll add it back.

Fryguy · 2025-02-19T22:13:31Z

app/controllers/application_controller/wait_for_task.rb

-    session[:async][:interval] ||= 1000 # Default interval to 1 second
-    session[:async][:params] ||= {}
+    async_interval = params[:async_interval] || 1000 # Default interval to 1 second
+    async_params = params[:async_params] || session[:async][:params]


I don't think I like the async_params coming in as a regular param - not sure why that is needed.

that is the whole solution.
Otherwise it is a last one in. in our case, it gets blown away by notifications url hit.

moved over to Task

Fryguy · 2025-02-19T22:13:54Z

app/controllers/application_controller/wait_for_task.rb

    render :update do |page|
      page << javascript_prologue
-      ajax_call = remote_function(:url => {:action => 'wait_for_task', :task_id => task_id})
-      page << "setTimeout(\"#{ajax_call}\", #{session[:async][:interval]});"
+      ajax_call = remote_function(:url => {:action => 'wait_for_task', :task_id => task_id, :async_params => async_params, :async_interval => :async_interval})


Oh because of this ... hmmmm

async_parameters have been removed. now it only contains async_interval

kbrock · 2025-02-19T22:34:39Z

ok, so we have a security problem just passing the action. The send(param[:action]) is pretty wide open. Even if we change to a public_send.

The action typically comes from params[:action] which is the url but those are protected.
In other cases, the action is from the url, which keeps getting hit until the task_id is completed.

This new way passes action as a param (user editible) and that is not protected like it is when it comes in via the url.

I can probably change these urls to have a single url with a blocker on a completed task_id (vs having a #{action}_finished pattern.

Aside: Also, these all use rjs. Would have been nice to drop that as well, but this is starting to get serious scope creep.

kbrock · 2025-02-20T22:54:16Z

WIP: moving parameters into the Task.

It is unfortunate that the task is created elsewhere - so we need to find and then update it

kbrock · 2025-02-21T00:02:00Z

update:

moved async_params to MiqTask

WIP: This PR is complete. First commit is from #9358 - after merging, can up-WIP

kbrock · 2025-02-21T15:20:26Z

update:

fixed stub in spec to properly return a task_id

kbrock · 2025-02-24T16:03:20Z

@agrare Ideas how to test this on OpenStack (along with #9358)?

Fryguy · 2025-02-24T16:05:42Z

app/controllers/application_controller/wait_for_task.rb

    else # Task done
-      session[:async][:params].each { |k, v| @_params[k] = v } # Merge in the original params and
-      send(session.fetch_path(:async, :params, :action)) # call the orig. method
+      async_params = task.context_data[:async_params]


Should we delete the async_params out of the task? They weren't there in the first place and we stuffed them in. My concern is that the subsequent handler of the task might take the context data and send it wholesale, for example, to some provider operation.

From what I can tell, widgets and others store a bunch of data in context_data. It looks like the wild west.

The only provider I see that even mentions context_data is manageiq-providers-workflow that uses it to store the kubernetes id.

I don't see anything that uses context_data whole sale. e.g. grep -r 'context_data[^\[.]' is mostly assignments and nothing is accessed in a whole sale way.

I'm still open to removing it. Not sure if a glitch could cause issues here. We would delete it from the task when we view the task?

BottleneckEvent does allow the full context to come out - but I didn't look too much into that since it seems irrelevant.

Sorry - I misunderstood where this line of code fit in the flow. So, at this point the task is already completed, and it technically doesn't matter. I feel like it should be removed from a purity standpoint (i.e. whatever we add temporarily we shouild delete), but I won't hold up merge.

We would delete it from the task when we view the task?

No I was thinking just literally changing this line to

Suggested change

async_params = task.context_data[:async_params]

async_params = task.context_data.delete(:async_params).tap { task.save! }

ok. implemented but without the tap dance

I think I merged before this landed cause I don't see it in the code.

Fryguy · 2025-02-24T16:09:50Z

app/controllers/application_controller/wait_for_task.rb

    render :update do |page|
      page << javascript_prologue
-      ajax_call = remote_function(:url => {:action => 'wait_for_task', :task_id => task_id})
-      page << "setTimeout(\"#{ajax_call}\", #{session[:async][:interval]});"
+      ajax_call = remote_function(:url => {:action => 'wait_for_task', :task_id => task_id, :async_interval => async_interval})


Do we need this as a parameter? I think this can be stuffed in the task along with the params. My concern is this is user modifiable, and then we generate JavaScript with it immediately afterwards. I'm not sure how it's exploitable, but it's easy to stuff random JavaScript into this parameter.

It is updated on every poll. I was thinking that it is unnecessary task churn.

This is the sleep time. Increasing it just changes the client polling. I guess you could increase it too high and the task would not be collected on the client.
But just closing the window would basically do the same.

If they lower the value, then they could increase the polling - but on the same note, they could just keep hitting the url and not bother with our ui either.

So, my problem is they could change it to random javascript and it gets embedded on the next line:

page << "setTimeout(\"#{ajax_call}\", #{async_interval});"

While I can't come up with a practical hack that would use this, I figured why even allow the vector if it's a as simple as storing in the task alongside the a params.

ensured it is an integer

Conversations have asked if this would be better stored in the task itself. We are storing the parameters in there.

The ui process would be updating the task every second or so (1-5 seconds)
At the same time, the queue would hold this task open for a long time.

(As mentioned above I really don't like the churn)
But more importantly, this seems the recipe for a race condition - one process holding a stale copy of the object for minutes while another updates every second.

Oh that's a good point on the race condition, and I'm not sure if that table is set up for stale record detection. Ok this is fine then, especially if we can ensure it's an integer.

Fryguy · 2025-02-24T16:12:07Z

app/controllers/application_controller/wait_for_task.rb

+    task.context_data_will_change!
+    (task.context_data ||= {})[:async_params] = async_params
+    task.save!


Could this be more simply written as

Suggested change

task.context_data_will_change!

(task.context_data ||= {})[:async_params] = async_params

task.save!

task.context_data = task.context_data.merge(:async_params => async_params)

task.save!

Yes, task.context_data_will_change! is not necessary. Just have flashbacks from rails 3 (rails 4?) when it did not detect a change to a hash.

Also, task.context_data is often nil.

So,

task.context_data = (task.context_data || {}).merge(:async_params => async_params) # or asyc_hash = {:async_params => async_params} task.context_data = task.context_data ? context_data.merge(async_hash) : async_hash

I kinda like the original way but, as long as we handle the nil case, I'm good to change

Personally this one seems the cleanest. I thought context_data was automatically a Hash, but the initialization to en empty Hash is fine.

task.context_data = (task.context_data || {}).merge(:async_params => async_params)

kbrock · 2025-02-24T22:10:25Z

update:

changed how Task#context_data is assigned.

Fryguy · 2025-02-26T20:32:05Z

Merged #9358

kbrock · 2025-02-27T14:18:29Z

update:

rebased (dependent PR was merged)

We were storing in session[:async][:params], but that ended up with a race condition and didn't allow multiple tabs

kbrock · 2025-02-27T14:29:26Z

update:

ensured async_interval is an integer larger than 1 second

kbrock · 2025-02-27T15:00:37Z

update:

now deleting parameters from task
ensured to_i is always called on params[:async_interval]

Fryguy · 2025-02-28T19:13:50Z

Backported to spassky in commit fefd271.

commit fefd27182c9b69584c963456cbad79d52bcd2416
Author: Jason Frey <fryguy9@gmail.com>
Date:   Thu Feb 27 10:03:58 2025 -0500

    Merge pull request #9352 from kbrock/wait_for_task
    
    Wait for task no longer
    
    (cherry picked from commit f42d416d4eadd56aa4aea5b63ae04deed208bc39)

Wait for task no longer (cherry picked from commit f42d416)

kbrock requested a review from a team as a code owner February 19, 2025 21:58

Fryguy reviewed Feb 19, 2025

View reviewed changes

kbrock force-pushed the wait_for_task branch from da10933 to f1886c5 Compare February 19, 2025 22:25

kbrock mentioned this pull request Feb 20, 2025

Use params[:name] over session[:async][:params][:name] #9358

Merged

kbrock added bug wip labels Feb 20, 2025

kbrock changed the title ~~Wait for task no longer~~ [WIP] Wait for task no longer Feb 20, 2025

kbrock force-pushed the wait_for_task branch from f1886c5 to d79e60d Compare February 21, 2025 00:00

kbrock force-pushed the wait_for_task branch from d79e60d to 2241435 Compare February 21, 2025 15:20

kbrock assigned Fryguy Feb 24, 2025

kbrock removed the wip label Feb 24, 2025

kbrock changed the title ~~[WIP] Wait for task no longer~~ Wait for task no longer Feb 24, 2025

Fryguy reviewed Feb 24, 2025

View reviewed changes

kbrock force-pushed the wait_for_task branch from 2241435 to 45fbbea Compare February 24, 2025 21:34

Fryguy added the spassky/yes? label Feb 26, 2025

kbrock added 2 commits February 27, 2025 09:16

Drop duplicate line

11625d8

change browser_refres_task should_flash to a keyword parameter

def63ef

kbrock added 2 commits February 27, 2025 09:28

wait_for_task passes interval as a param (instead of the session)

6b17d66

wait_for_task stores async parameters in task.context_data

e7933d3

We were storing in session[:async][:params], but that ended up with a race condition and didn't allow multiple tabs

kbrock force-pushed the wait_for_task branch from 45fbbea to e7933d3 Compare February 27, 2025 14:28

kbrock mentioned this pull request Feb 27, 2025

update proper use of wait_for_task ManageIQ/guides#560

Merged

Fryguy approved these changes Feb 27, 2025

View reviewed changes

Fryguy merged commit f42d416 into ManageIQ:master Feb 27, 2025
15 checks passed

kbrock deleted the wait_for_task branch February 27, 2025 15:10

Fryguy added spassky/yes and removed spassky/yes? labels Feb 28, 2025

Fryguy removed the spassky/yes label Feb 28, 2025

Fryguy added a commit that referenced this pull request Feb 28, 2025

Merge pull request #9352 from kbrock/wait_for_task

fefd271

Wait for task no longer (cherry picked from commit f42d416)

Fryguy added the spassky/backported label Feb 28, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Wait for task no longer #9352

Wait for task no longer #9352

kbrock commented Feb 19, 2025 •

edited by Fryguy

Loading

kbrock commented Feb 19, 2025

Fryguy Feb 19, 2025

kbrock Feb 19, 2025

Fryguy Feb 19, 2025

kbrock Feb 19, 2025

kbrock Feb 21, 2025

Fryguy Feb 19, 2025

kbrock Feb 21, 2025

kbrock commented Feb 19, 2025

kbrock commented Feb 20, 2025

kbrock commented Feb 21, 2025 •

edited

Loading

kbrock commented Feb 21, 2025

kbrock commented Feb 24, 2025

Fryguy Feb 24, 2025

kbrock Feb 24, 2025

Fryguy Feb 26, 2025 •

edited

Loading

kbrock Feb 27, 2025

Fryguy Feb 27, 2025

Fryguy Feb 24, 2025

kbrock Feb 24, 2025 •

edited

Loading

Fryguy Feb 26, 2025 •

edited

Loading

kbrock Feb 27, 2025

kbrock Feb 27, 2025 •

edited

Loading

Fryguy Feb 27, 2025

Fryguy Feb 24, 2025

kbrock Feb 24, 2025

Fryguy Feb 24, 2025

kbrock commented Feb 24, 2025

Fryguy commented Feb 26, 2025

kbrock commented Feb 27, 2025

kbrock commented Feb 27, 2025

kbrock commented Feb 27, 2025

Fryguy commented Feb 28, 2025

	async_params = task.context_data[:async_params]
	async_params = task.context_data.delete(:async_params).tap { task.save! }

Wait for task no longer #9352

Wait for task no longer #9352

Conversation

kbrock commented Feb 19, 2025 • edited by Fryguy Loading

Overview

Depends upon

Issue

Solution

Before

After

kbrock commented Feb 19, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kbrock commented Feb 19, 2025

kbrock commented Feb 20, 2025

kbrock commented Feb 21, 2025 • edited Loading

kbrock commented Feb 21, 2025

kbrock commented Feb 24, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fryguy Feb 26, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kbrock Feb 24, 2025 • edited Loading

Choose a reason for hiding this comment

Fryguy Feb 26, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kbrock Feb 27, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kbrock commented Feb 24, 2025

Fryguy commented Feb 26, 2025

kbrock commented Feb 27, 2025

kbrock commented Feb 27, 2025

kbrock commented Feb 27, 2025

Fryguy commented Feb 28, 2025

kbrock commented Feb 19, 2025 •

edited by Fryguy

Loading

kbrock commented Feb 21, 2025 •

edited

Loading

Fryguy Feb 26, 2025 •

edited

Loading

kbrock Feb 24, 2025 •

edited

Loading

Fryguy Feb 26, 2025 •

edited

Loading

kbrock Feb 27, 2025 •

edited

Loading