Fix progress display for completed tasks with zero tick_total #1276
+26
−2
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
prevent infinite progress animation for completed zero-total tasks - Set progress value to 0 for completed tasks with tick_total = 0 - Fixes progress bar animation continuing after task completion - Update progress text handling for nil tick_count values
Contributor
adrianna-chang-shopify
left a comment
adrianna-chang-shopify
left a comment
There was a problem hiding this comment.
Thanks for contributing! Can you sign the CLA?
| # | ||
| # @return [String] the text for the Run progress. | ||
| def text | ||
| count = @run.tick_count |
Contributor
There was a problem hiding this comment.
Perhaps we could do this instead?
Suggested change
| count = @run.tick_count | |
| count = @run.tick_count || 0 |
And leave the if statement intact?
Author
|
I have signed the CLA! |
- Default tick_count to 0 when nil to prevent errors - Simplify progress text calculation by removing redundant nil checks Co-authored-by: adrianna-chang-shopify <[email protected]>
- Add test case for #value returning 0 when run is succeeded and tick_total is nil - Prevent infinite progress animation for zero-total completed tasks
Member
|
I can't reproduce the original issue. MaintenanceTasks::Run.last.slice :tick_count, :tick_total
# => {"tick_count" => 0, "tick_total" => 0}
MaintenanceTasks::Progress.new(MaintenanceTasks::Run.last).then { [it.max, it.value] }
# => [0, 0]and the progress bar markup is: and it appears like this: The code for the maintenance_tasks/app/models/maintenance_tasks/progress.rb Lines 29 to 30 in a5da6c1 If the run is completed, it is stopped, so even if it's not estimatable (nil or 0 total), the progress shows the count, which is 0 (and can't be nil because it's a |
Author
|
To reproduce the issue, please use the provided issue_1726.rb # frozen_string_literal: true
require 'bundler/inline'
gemfile(true) do
source 'https://rubygems.org'
gem 'rails', '~> 8.0'
gem 'sqlite3'
gem "rackup"
gem "puma"
gem "debug"
gem "maintenance_tasks", path: '.'
end
require 'rails/all'
require 'maintenance_tasks'
class App < Rails::Application
config.consider_all_requests_local = true
config.secret_key_base = 'i_am_a_secret'
config.active_storage.service_configurations = { 'local' => { 'service' => 'Disk', 'root' => './storage' } }
config.eager_load = false
config.public_file_server.enabled = true
routes.append do
mount MaintenanceTasks::Engine, at: "/maintenance_tasks"
root to: 'welcome#index'
end
config.paths["config/routes.rb"] = []
end
database = Pathname.new( __FILE__).basename.sub_ext('.sqlite3')
puts "Using database at #{database}"
ENV['DATABASE_URL'] = "sqlite3:#{database}"
ActiveRecord::Base.establish_connection(adapter: 'sqlite3', database: database)
Rails.logger = Logger.new(STDOUT)
App.initialize!
ActiveRecord::Schema.define do
create_table :old_posts, force: true do |t|
t.string :title
t.text :content
t.timestamps
end
create_table :posts, force: true do |t|
t.string :title
t.text :content
t.timestamps
end
create_table :comments, force: true do |t|
t.integer :post_id
t.text :content
t.timestamps
end
create_table :maintenance_tasks_runs, force: :cascade do |t|
t.string "task_name", null: false
t.datetime "started_at", precision: nil
t.datetime "ended_at", precision: nil
t.float "time_running", default: 0.0, null: false
t.bigint "tick_count"
t.bigint "tick_total"
t.string "job_id"
t.string "cursor"
t.string "status", default: "enqueued", null: false
t.string "error_class"
t.string "error_message"
t.text "backtrace"
t.datetime "created_at", null: false
t.datetime "updated_at", null: false
t.text "arguments"
t.integer "lock_version", default: 0, null: false
t.text "metadata"
t.index ["task_name", "status", "created_at"], name: "index_maintenance_tasks_runs", order: { created_at: :desc }
end
end
class OldPost < ActiveRecord::Base
end
class Post < ActiveRecord::Base
has_many :comments
end
class Comment < ActiveRecord::Base
belongs_to :post
end
class WelcomeController < ActionController::Base
def index
render json: {
message: "Rails Inline App is running!",
maintenance_tasks: "Mounted at /maintenance_tasks",
database: "Connected",
tables: ActiveRecord::Base.connection.tables.sort
}
end
end
class MigratePostsToOldPosts < MaintenanceTasks::Task
def collection
# The collection of elements to be processed by the task.
# This should return an ActiveRecord::Relation or an array.
Post.where('created_at < ?', 1.year.ago)
end
def process(element)
# The work to be done in a single iteration of the task.
# This should be idempotent, as the same element may be processed more
# than once if the task is interrupted and resumed.
OldPost.create!(title: element.title, content: element.content)
element.destroy!
end
def count
# Optionally, define the number of rows that will be iterated over
# This is used to track the task's progress
collection.count
end
end
Post.create!(
title: "Old Post from 2023",
content: "This is an old post that should be migrated",
created_at: 2.years.ago,
updated_at: 2.years.ago
)
Post.create!(
title: "New Post from 2025",
content: "This is a new post that should not be migrated",
created_at: Time.current,
updated_at: Time.current
)
Comment.create!(
post_id: 1,
content: "This comment should be deleted with the old post",
created_at: 2.years.ago,
updated_at: 2.years.ago
)
puts "🚀 Starting Rails Inline App with Maintenance Tasks Engine..."
puts "📍 Available endpoints:"
puts " GET http://localhost:3000/ - Welcome page"
puts " GET http://localhost:3000/maintenance_tasks - Maintenance Tasks Engine"
puts "📊 Tables: #{ActiveRecord::Base.connection.tables.sort}"
Rackup::Server.start(app: App, Port: 3000)Reproduction Stepsruby issue_1726.rb
Expected Behavior (with fix)Second execution should show: Actual Behavior (without fix)Second execution shows: 
This demonstrates the real-world scenario where tasks with mutable collections complete with empty results on subsequent runs. The issue is specific to completed tasks with |
Member
|
Your t.bigint "tick_count"When it should be maintenance_tasks/test/dummy/db/schema.rb Line 47 in 76a40ce The issue is that the migration doesn't create the column like that: To repro on the repo easily: $ rm test/dummy/db/schema.rb
$ bin/rails db:drop db:create db:migrateThen run ParamsTasks with The issue does not impact MySQL, because the migration runs like this: Whereas on SQLite we get: It seems the MySQL adapter preserves the You can fix this in your application by adding a migration that sets the default for the column to 0. Thanks again for your contribution and the detailed repro script! |
Author
|
@etiennebarrie Thank you for the very detailed analysis! I looked more into this and found it's a SQLite migration issue that began in Rails 7.x. For anyone with this problem: Here's a workaround to fix the constraints: # db/migrate/[timestamp]_restore_tick_count_constraints.rb
class RestoreTickCountConstraints < ActiveRecord::Migration[7.0]
def up
if ActiveRecord::Base.connection.adapter_name == 'SQLite'
change_column_default :maintenance_tasks_runs, :tick_count, from: nil, to: 0
change_column_null :maintenance_tasks_runs, :tick_count, false, 0
end
end
def down
if ActiveRecord::Base.connection.adapter_name == 'SQLite'
change_column_null :maintenance_tasks_runs, :tick_count, true
change_column_default :maintenance_tasks_runs, :tick_count, from: 0, to: nil
end
end
endThis is actually a bug in Rails itself, so it should be fixed in Rails. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Problem
When a maintenance task completes with an empty collection (
tick_total = 0), the progress bar shows infinite loading instead of completion. Thishappens because the
<progress>HTML element without avalueattribute continuously animates, even when the task has finished successfully.This is especially problematic for tasks with mutable collections. For example, when running a task multiple times where the first execution processes
all matching articles, subsequent runs will have
tick_total = 0but the progress bar still shows infinite loading.Example scenario:
When this task runs again after all matching articles have been processed, tick_total becomes 0, but the progress bar shows infinite loading instead of
completion.
Solution
Modified MaintenanceTasks::Progress#value to return 0 when:
This makes the HTML
<progress>element receive value="0", showing a completed state instead of the infinite animation.HTML output comparison:
Before:
<progress class="progress mt-4 is-primary is-light"></progress>(infinite progress animation)After:
<progress value="0" class="progress mt-4 is-success"></progress>(with value="0", shows completed state)Also updated Progress#text to handle cases where count is nil and task is completed, showing "Processed 0 items." for better user feedback.