Enhancement: Implement customizable post-task cleanup policies (#1611) by AviJxn · Pull Request #1641 · aces/cbrain

AviJxn · 2026-07-07T15:57:46Z

Closes #1611 this implements the customizable data preservation and cleanup policies discussed, mapping the state action matrix into two intuitive dropdowns in the task control panel.

Changes

Schema: Added success_cleanup_policy and failure_cleanup_policy string columns (defaulting to 'keep_all' so legacy tasks do not break).
Controller: Added the selection dropdowns to _control.html.erb and whitelisted the new attributes in TasksController.
Model Logic: Added should_cleanup_component? to CbrainTask to handle the matrix mapping with erase_workdir_and_outputs as the default on success.
Background Process: Created RemoveTaskInputs and RemoveTaskOutputs background activities both include real time checks against active tasks to safely skip erasing any shared or downstream assets.
Worker Lifecycle: Injected an isolated enqueue_post_task_cleanup_policies helper hook into BourreauWorker to prevent making the monolithic loop any longer.

…ies for success and failure states

AviJxn · 2026-07-07T16:31:22Z

Hey @prioux it looks like the CI pipeline is dropping on a strict framework sequence error. The workflow runs rake db:seed before executing pending migrations, which triggers a crash on my new cleanup policy migration (BrainPortal/db/migrate/20260629232300_add_cleanup_policies_to_cbrain_tasks.rb). The core implementation files are entirely isolated and clean.

prioux · 2026-07-08T18:25:26Z

@AviJxn That's because your pull request includes a migration, but it doesn't include the effect of that migration on the full schema. In your work environment, when you apply your migration, it will also update the file db/schema.rb which describe the final effect of the DB. That file is what is loaded in CI. During a CI check, no migrations are applied, because the DB is initialized right from the start with the final schema.

prioux

There are many issues here. See comments.

Also, there are missing elements that I would ideally have liked to have in this PR:

provide an abstract CbrainTask method that returns the IDs of outputs; the default would be [] and subclasses would override this, thus giving the framework an easy access to the list of outputs of a task. BoutiquesTasks would fetch from the params array and return the _cbrain_output_XXX arrays for instance.
the decision about which input file's cache to clean or not would benefit from using the kind of logic we use in the special module BoutiquesInputCacheCleaner ; that module use the timestamps of the synchronization subsystem to make some decisions
we could entirely skip cleaning any input or output that are less than 1mb; we don't really care about leaving those in the caches, in the end. The point is to provide cleanup of LARGE pieces of data that are only used once (either as inputs or as produced outputs)

prioux · 2026-07-09T14:23:25Z

+      # Safety check: skip if another active task relies on this exact input
+      file_in_use = CbrainTask.active
+                              .where.not(id: cbrain_task.id)
+                              .any? { |t| t.params[:interface_userfile_ids]&.include?(userfile_id) }


You can't be sure that t.params[:interface_userfile_ids] will contain an array of IDs that are numeric or string. This is a known inconsistent part of CBRAIN. So your comparison with .include?() would fail. Generally, when I need to write such code, I turn everything into strings. So I'd end up with something ugly like this:

.any? { |t| (t.params[:interface_userfile_ids] || []).map(&:to_s) .include?(userfile_id.to_s) }

prioux · 2026-07-09T14:24:06Z

+      next unless userfile
+
+      # Safety check: skip if another active task relies on this exact input
+      file_in_use = CbrainTask.active


You can restrict the search to only the active tasks on the current server, we don't really care if a file is being used on a different one.

prioux · 2026-07-09T14:24:35Z

+    [true, nil]
+  end
+
+  def prepare_dynamic_items


This is leftover from another BAC, it shouldn't be here.

prioux · 2026-07-09T14:27:02Z

+    cbrain_task = CbrainTask.where(:bourreau_id => CBRAIN::SelfRemoteResourceId).find_by(id: item)
+    return [false, "Task not found"] unless cbrain_task
+
+    output_userfiles = Userfile.where(task_id: cbrain_task.id).to_a


This is non-sense; there is no such relation between userfiles and tasks. You can't find the output of tasks using a relation, you need to query the params array of a task.

Task models needs to be extended with a method that returns their output IDs. For BoutiquesTasks, these would be recognized by looking up the keys _cbrain_output_IDHERE in params, where IDHERE is the ID of the output-files object in the Boutiques descriptor for the task.

The fact that this code is submitted and would crash (because Userfile.where(:task_id => x) is illegal) tells me you never actually ran your code, did you?

prioux · 2026-07-09T14:28:31Z

+    [true, nil]
+  end
+
+  def prepare_dynamic_items


Also spurious.

prioux · 2026-07-09T14:30:26Z

    end # case 'status' is 'New', 'Data Ready', 'Recover*' and 'Restart*'


+    enqueue_post_task_cleanup_policies(task)


All the code including in "bourreau_worker.rb" is misplaced. The role of the BourreauWorker is to manage the life cycle of the task, e.g. how each state is turned into another, but not the actual work being performed during each state.

So the code for cleaning up inputs and outputs should instead be integrated into the class ClusterTask.

prioux · 2026-07-09T14:31:13Z

+
+# Removes the cached outputs of a CBRAIN task safely from the local cache.
+# Must be run on a Bourreau only.
+class BackgroundActivity::RemoveTaskOutputs < BackgroundActivity::TerminateTask


Again, this is a bad name for that class, we are not removing outputs.

prioux · 2026-07-09T14:31:46Z

+
+# Removes the cached inputs of a CBRAIN task safely from the local cache.
+# Must be run on a Bourreau only.
+class BackgroundActivity::RemoveTaskInputs < BackgroundActivity::TerminateTask


This is a bad name for that class, we are not removing inputs ! We are removing the cached copies of inputs.

prioux · 2026-07-09T14:32:59Z

+    add_column :cbrain_tasks, :success_cleanup_policy, :string, default: 'keep_all'
+    add_column :cbrain_tasks, :failure_cleanup_policy, :string, default: 'keep_all'
+  end
+end


Missing final NL

prioux · 2026-07-09T14:37:34Z

+    populate_items_from_task_custom_filter
+  end
+
+end


Missing final NL

AviJxn added 2 commits July 7, 2026 21:05

Enhancement aces#1611: Implement customizable post-task cleanup polic…

9b8c9fa

…ies for success and failure states

Chore: trigger fresh CI run to clear environment seed crash

2c78aa9

prioux requested changes Jul 9, 2026

View reviewed changes

		end # case 'status' is 'New', 'Data Ready', 'Recover' and 'Restart'


		enqueue_post_task_cleanup_policies(task)

Uh oh!

Conversation

AviJxn commented Jul 7, 2026

Changes

Uh oh!

AviJxn commented Jul 7, 2026

Uh oh!

prioux commented Jul 8, 2026

Uh oh!

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

prioux Jul 9, 2026 • 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!

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

prioux Jul 9, 2026 •

edited

Loading