Skip to content

Commit a954bb1

Browse files
cardmagicclaude
andcommitted
fix: validate redrive filters and guard every close
Reviewing the TypeScript port surfaced two defects this branch shares. `limit` and `failed_after` were passed through unchecked. A limit of zero or a fraction reached the database as a filter nobody had agreed to, and a value that does not answer `utc` raised a NoMethodError from inside the manager rather than refusing the argument. `close` updated a task by id alone, so a cancel could overwrite a task the runner had already completed and write a second transition event for it. Both closes guard on the running status now and write their event only when the update changed a row. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent 5c7d4f0 commit a954bb1

6 files changed

Lines changed: 70 additions & 10 deletions

File tree

‎lib/solid_objects/dead_letter_scope.rb‎

Lines changed: 18 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -55,13 +55,29 @@ def redrive(actor_type: nil, failed_after: nil, limit: nil, authorization_contex
5555
scope: self,
5656
filters: {
5757
"actor_type" => actor_type,
58-
"failed_after" => failed_after&.utc&.iso8601(6),
59-
"limit" => limit
58+
"failed_after" => failed_after_filter(failed_after),
59+
"limit" => limit_filter(limit)
6060
},
6161
authorization_context:
6262
)
6363
end
6464

65+
# @rbs (untyped) -> String?
66+
def failed_after_filter(failed_after)
67+
return nil if failed_after.nil?
68+
raise ArgumentError, "failed_after must be a time" unless failed_after.respond_to?(:utc)
69+
70+
failed_after.utc.iso8601(6)
71+
end
72+
73+
# @rbs (untyped) -> Integer?
74+
def limit_filter(limit)
75+
return nil if limit.nil?
76+
return limit if limit.is_a?(Integer) && limit.positive?
77+
78+
raise ArgumentError, "limit must be a positive integer"
79+
end
80+
6581
# @rbs () -> ActiveRecord::Relation[untyped]
6682
def dead
6783
model.where(status: DEAD)

‎lib/solid_objects/redrive_manager.rb‎

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -40,18 +40,20 @@ def cancel(id, authorization_context: nil)
4040
record = Redrive.find(id)
4141
return task_for(record) unless record.status == RUNNING
4242

43-
close(record, status: CANCELLED)
44-
audit(record, action: "redrive.cancel")
43+
audit(record, action: "redrive.cancel") if close(record, status: CANCELLED)
4544
task_for(record)
4645
end
4746

48-
# @rbs (Redrive, status: String) -> void
47+
# @rbs (Redrive, status: String) -> bool
4948
def close(record, status:)
50-
record.update!(
49+
changed = Redrive.where(id: record.id, status: RUNNING).update_all(
5150
status:,
5251
active_scope: nil,
53-
finished_at: SolidObjects.database_adapter.database_now
52+
finished_at: SolidObjects.database_adapter.database_now,
53+
updated_at: SolidObjects.database_adapter.database_now
5454
)
55+
record.reload if changed.positive?
56+
changed.positive?
5557
end
5658

5759
# @rbs (Redrive, action: String) -> void

‎lib/solid_objects/redrive_runner.rb‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -59,7 +59,8 @@ def batch_size(record)
5959
# @rbs (Redrive) -> void
6060
def finish(record)
6161
manager = SolidObjects.redrives
62-
manager.close(record, status: RedriveManager::COMPLETED)
62+
return unless manager.close(record, status: RedriveManager::COMPLETED)
63+
6364
manager.audit(record, action: "redrive.finish")
6465
end
6566
end

‎sig/generated/lib/solid_objects/dead_letter_scope.rbs‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,12 @@ module SolidObjects
3131
# @rbs (?actor_type: String?, ?failed_after: untyped, ?limit: Integer?, ?authorization_context: untyped) -> RedriveTask
3232
def redrive: (?actor_type: String?, ?failed_after: untyped, ?limit: Integer?, ?authorization_context: untyped) -> RedriveTask
3333

34+
# @rbs (untyped) -> String?
35+
def failed_after_filter: (untyped) -> String?
36+
37+
# @rbs (untyped) -> Integer?
38+
def limit_filter: (untyped) -> Integer?
39+
3440
# @rbs () -> ActiveRecord::Relation[untyped]
3541
def dead: () -> ActiveRecord::Relation[untyped]
3642

‎sig/generated/lib/solid_objects/redrive_manager.rbs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -20,8 +20,8 @@ module SolidObjects
2020
# @rbs (String, ?authorization_context: untyped) -> RedriveTask
2121
def cancel: (String, ?authorization_context: untyped) -> RedriveTask
2222

23-
# @rbs (Redrive, status: String) -> void
24-
def close: (Redrive, status: String) -> void
23+
# @rbs (Redrive, status: String) -> bool
24+
def close: (Redrive, status: String) -> bool
2525

2626
# @rbs (Redrive, action: String) -> void
2727
def audit: (Redrive, action: String) -> void

‎test/integration/redrive_test.rb‎

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -212,6 +212,41 @@ def touch
212212
SolidObjects::AdministrationEvent.order(:id).map(&:action)
213213
end
214214

215+
test "refuses an invalid filter rather than redrive everything" do
216+
dead_effects(2)
217+
218+
assert_raises(ArgumentError) do
219+
SolidObjects.dead_letters.effects.redrive(limit: 0, authorization_context: "operator")
220+
end
221+
assert_raises(ArgumentError) do
222+
SolidObjects.dead_letters.effects.redrive(limit: 1.5, authorization_context: "operator")
223+
end
224+
225+
assert_equal 0, SolidObjects::Redrive.count
226+
assert_equal 0, SolidObjects::AdministrationEvent.count
227+
end
228+
229+
test "writes no audit row when a retry names a row that does not exist" do
230+
assert_raises(ActiveRecord::RecordNotFound) do
231+
SolidObjects.dead_letters.effects.retry("missing", authorization_context: "operator")
232+
end
233+
234+
assert_equal 0, SolidObjects::AdministrationEvent.count
235+
end
236+
237+
test "a cancel cannot overwrite a task the runner already finished" do
238+
dead_effects(1)
239+
task = SolidObjects.dead_letters.effects.redrive(authorization_context: "operator")
240+
drain
241+
242+
task.cancel(authorization_context: "operator")
243+
244+
assert_equal "completed",
245+
SolidObjects.redrives.find(task.id, authorization_context: "operator").status
246+
assert_equal [ "redrive.start", "redrive.finish" ],
247+
SolidObjects::AdministrationEvent.order(:id).map(&:action)
248+
end
249+
215250
test "refuses an unauthorized caller that reaches the manager directly" do
216251
SolidObjects.configuration.authorize_administration = ->(**) { false }
217252

0 commit comments

Comments
 (0)