From eea976144f38ede9844b4c0059a7074c26180429 Mon Sep 17 00:00:00 2001 From: Harshit Chaudhary <55315065+Harry-kp@users.noreply.github.com> Date: Sat, 29 Aug 2026 19:49:03 +0530 Subject: [PATCH] fix: five issues found installing the gem into a real app MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Everything until now was verified inside the engine's own dummy app, where the gem is loaded from source. So I did what an adopter does: `rails new`, add the gem, run the generator, migrate, and run the README quickstart verbatim. It works — routed on create, both layers, `after_approved` firing through the outbox. That path is now known-good rather than assumed. Five things it surfaced: The outbox had no retention story. It is a queue, not a ledger — one row per activation, decision and outcome, and a relayed row has no reader — but nothing ever swept it, so the table grew without bound. `OutboxEvent.purge!` removes relayed events past a window, keeps dead letters whatever their age (an exhausted event is evidence, not litter), and never touches the audit trail. `:unprocessable_entity` is deprecated in Rack and will be removed. The replacement symbol does not exist on Rails 7.1, so the admin renders `422`, which is the same on every version we support. `shiny_json_logic` was pinned `~> 0.3`, which allows 0.4. It is pre-1.0, where a minor may break, and it evaluates every routing rule — so a routine `bundle update` in an adopter's app could change what matches. Pinned to the patch series. Ruby 3.1 was claimed and never tested, the same gap the Rails matrix just closed, and it is past end of life. The floor is 3.2, which CI proves. PostgreSQL had no stated version. The tables key on `gen_random_uuid()`, which is core Postgres only from 13; before that it needs pgcrypto and the migration simply fails. Documented as 13+. Co-Authored-By: Claude Opus 5 (1M context) --- .gitignore | 3 ++ CHANGELOG.md | 36 ++++++++----- README.md | 23 ++++++-- .../admin/template_steps_controller.rb | 4 +- .../admin/track_templates_controller.rb | 4 +- .../admin/trigger_rules_controller.rb | 4 +- app/models/approval_engine/outbox_event.rb | 18 +++++++ approval_engine.gemspec | 6 ++- .../approval_engine/outbox_retention_test.rb | 52 +++++++++++++++++++ 9 files changed, 127 insertions(+), 23 deletions(-) create mode 100644 test/models/approval_engine/outbox_retention_test.rb diff --git a/.gitignore b/.gitignore index fb27d6a..c4110e6 100644 --- a/.gitignore +++ b/.gitignore @@ -5,6 +5,9 @@ /tmp/ /test/tmp/ /test/dummy/log/*.log +# Rotated logs too: `test.log.0` doesn't match `*.log`, and a 100MB one is how +# it gets noticed — at the push. +/test/dummy/log/*.log.* /test/dummy/storage/ /test/dummy/tmp/ diff --git a/CHANGELOG.md b/CHANGELOG.md index f268e04..075e93c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,12 @@ behaviour, and every new surface is off until you switch it on. ### Added +- `ApprovalEngine::OutboxEvent.purge!` — the outbox is a queue, not a ledger, + and nothing swept it: one row per activation, decision and outcome, kept + forever. Purges relayed events past a window (30 days by default), keeps dead + letters whatever their age, and never touches the audit trail. + + - `ApprovalEngine.define_flow(name, tenant:, model:)` — a declarative DSL for a whole flow (the template, its ordered steps, and the rules that route to it) in one block, instead of three `create!` calls whose relationship you have to @@ -86,6 +92,14 @@ behaviour, and every new surface is off until you switch it on. `define_flow` may route to. `nil`, the default, means no vocabulary is declared and nothing is checked; set it and a `group:` typo stops the seed instead of resolving zero actors at build time. +- `ApprovalEngine::TestHelpers` — test support for host applications, so an + adopter is not hand-rolling fixtures to test their own flow. + `approve_approval!` walks a flow to a decision, `reject_approval!` stops at + whatever is waiting, `pending_approval_steps` is the inbox for one record, and + `drain_approval_outbox!` relays the side-effects — without which + `after_approved` has genuinely not run yet, and a test asserting on it fails + for the right reason while looking like the wrong one. Actions and queries + rather than assertions, so they read the same under Minitest and RSpec. ### Changed @@ -101,22 +115,20 @@ behaviour, and every new surface is off until you switch it on. `Alternatives` section naming the neighbours. `test/docs_test.rb` now fails the build if the README documents a config key, an `ApprovalEngine` method, a generator, or an image that does not exist. - -### Changed - +- **Minimum Ruby is now 3.2** and **PostgreSQL 13+ is stated explicitly.** 3.1 + was claimed but never tested and is past end of life; the tables key on + `gen_random_uuid()`, which is core Postgres only from 13. +- `shiny_json_logic` is pinned to `~> 0.3.6` rather than `~> 0.3`. It is pre-1.0 + and evaluates every routing rule, so a floating minor could change matching + behaviour under an adopter on a routine `bundle update`. +- The admin renders validation failures with `422` rather than + `:unprocessable_entity`, which Rack has deprecated and will remove — the + replacement symbol does not exist on Rails 7.1, so the number is the portable + spelling. - **Minimum Rails is now 7.1**, up from 7.0.8. The outbox relay's retry backoff uses `:polynomially_longer`, which 7.0 does not know, and 7.0 is past security support. CI now runs the whole supported range rather than one version of it. -### Added - -- `ApprovalEngine::TestHelpers` — test support for host applications. - `approve_approval!` walks a flow to a decision, `reject_approval!` stops at - whatever is waiting, `pending_approval_steps` is the inbox for one record, and - `drain_approval_outbox!` relays the side-effects, without which `after_approved` - has genuinely not run yet. Actions and queries rather than assertions, so they - read the same under Minitest and RSpec. - ### Fixed - Step notifications ignored delegations. A delegate can act on a step, so diff --git a/README.md b/README.md index da9a5c3..c7f8bd0 100644 --- a/README.md +++ b/README.md @@ -3,7 +3,7 @@ [![Gem Version](https://img.shields.io/gem/v/approval_engine?logo=rubygems&color=E9573F)](https://rubygems.org/gems/approval_engine) [![Downloads](https://img.shields.io/gem/dt/approval_engine?logo=rubygems&color=blue)](https://rubygems.org/gems/approval_engine) [![CI](https://github.com/Harry-kp/approval_engine/actions/workflows/ci.yml/badge.svg)](https://github.com/Harry-kp/approval_engine/actions/workflows/ci.yml) -![Ruby](https://img.shields.io/badge/ruby-%3E%3D%203.1-CC342D) +![Ruby](https://img.shields.io/badge/ruby-%3E%3D%203.2-CC342D) ![Rails](https://img.shields.io/badge/rails-%3E%3D%207.1-D30001) ![License: MIT](https://img.shields.io/badge/license-MIT-blue) @@ -35,7 +35,8 @@ approved, who approves, and what happens next. ## 60-second quickstart -Needs PostgreSQL (the routing engine uses `jsonb`), Rails 7.1+, Ruby 3.1+. +Needs PostgreSQL 13+ (the routing engine uses `jsonb` and `gin`; the tables key +on `gen_random_uuid()`), Rails 7.1+, Ruby 3.2+. ```sh bundle add approval_engine @@ -505,6 +506,22 @@ A rule too complex for the simple form — an `or`, a nested group, a substring into something it isn't. That fallback is the point: the form is a convenience over the storage format, never a lossy replacement for it. +## Housekeeping + +The audit trail is the point of the ledger and is never swept. The **outbox is +not** — it is a queue, one row per activation, decision and outcome, and a +relayed row has no reader. Sweep it on whatever schedule you already run: + +```ruby +ApprovalEngine::OutboxEvent.purge! # relayed, older than 30 days +ApprovalEngine::OutboxEvent.purge!(older_than: 7.days) +``` + +Dead letters are kept whatever their age — an event that exhausted its retries +is evidence of a callback that needs fixing, not litter. Find them with +`ApprovalEngine::OutboxEvent.failed`, and clear `failed_at` to let the relay +pick one up again. + ## Core concepts | Term | What it is | @@ -586,7 +603,7 @@ edge cases yet. ## Development -ApprovalEngine needs Ruby 3.1+ and PostgreSQL. +ApprovalEngine needs Ruby 3.2+ and PostgreSQL 13+. ```sh bin/setup diff --git a/app/controllers/approval_engine/admin/template_steps_controller.rb b/app/controllers/approval_engine/admin/template_steps_controller.rb index a807ab5..a325f08 100644 --- a/app/controllers/approval_engine/admin/template_steps_controller.rb +++ b/app/controllers/approval_engine/admin/template_steps_controller.rb @@ -21,7 +21,7 @@ def create if @template_step.save redirect_to admin_track_template_path(@track_template), notice: "Step added." else - render :new, status: :unprocessable_entity + render :new, status: 422 end end @@ -32,7 +32,7 @@ def update if @template_step.update(template_step_params) redirect_to admin_track_template_path(@track_template), notice: "Step updated." else - render :edit, status: :unprocessable_entity + render :edit, status: 422 end end diff --git a/app/controllers/approval_engine/admin/track_templates_controller.rb b/app/controllers/approval_engine/admin/track_templates_controller.rb index f527dee..85fa235 100644 --- a/app/controllers/approval_engine/admin/track_templates_controller.rb +++ b/app/controllers/approval_engine/admin/track_templates_controller.rb @@ -36,7 +36,7 @@ def create if @track_template.save redirect_to admin_track_template_path(@track_template), notice: "Template created." else - render :new, status: :unprocessable_entity + render :new, status: 422 end end @@ -47,7 +47,7 @@ def update if @track_template.update(track_template_params) redirect_to admin_track_template_path(@track_template), notice: "Template updated." else - render :edit, status: :unprocessable_entity + render :edit, status: 422 end end diff --git a/app/controllers/approval_engine/admin/trigger_rules_controller.rb b/app/controllers/approval_engine/admin/trigger_rules_controller.rb index d818508..27b03d0 100644 --- a/app/controllers/approval_engine/admin/trigger_rules_controller.rb +++ b/app/controllers/approval_engine/admin/trigger_rules_controller.rb @@ -34,7 +34,7 @@ def create else @trigger_rule.errors.add(:condition, @condition_error) if @condition_error prepare_condition_editor - render :new, status: :unprocessable_entity + render :new, status: 422 end end @@ -50,7 +50,7 @@ def update else @trigger_rule.errors.add(:condition, @condition_error) if @condition_error prepare_condition_editor - render :edit, status: :unprocessable_entity + render :edit, status: 422 end end diff --git a/app/models/approval_engine/outbox_event.rb b/app/models/approval_engine/outbox_event.rb index a2c4951..0cc5355 100644 --- a/app/models/approval_engine/outbox_event.rb +++ b/app/models/approval_engine/outbox_event.rb @@ -13,6 +13,7 @@ class OutboxEvent < ApplicationRecord validates :tenant_id, :event_name, presence: true scope :unprocessed, -> { where(processed: false) } + scope :processed, -> { where(processed: true) } # Dead letters: delivery retries were exhausted. Excluded from drain! so a # permanently-failing callback isn't resurrected forever; surfaced here for # ops to inspect and replay (clear failed_at to let drain! pick it up again). @@ -34,6 +35,23 @@ def self.drain!(older_than: 1.minute, limit: 1000) end end + # The outbox is a queue, not a ledger: once an event is relayed its row has + # no reader, and one is written per step activation, decision and outcome. + # Left alone the table grows without bound, so a host is expected to sweep + # it — the audit trail lives in AuditLog and is never touched by this. + # + # ApprovalEngine::OutboxEvent.purge! # processed, 30d+ + # ApprovalEngine::OutboxEvent.purge!(older_than: 7.days) + # + # Returns the number of rows deleted. Dead letters are kept regardless of + # age: an event that exhausted its retries is evidence, not litter. + def self.purge!(older_than: 30.days, limit: 10_000) + ids = processed.where(failed_at: nil) + .where(processed_at: ..older_than.ago) + .order(:processed_at).limit(limit).pluck(:id) + where(id: ids).delete_all + end + def mark_processed! update!(processed: true, processed_at: Time.current, error_payload: nil, delivery_error: nil) end diff --git a/approval_engine.gemspec b/approval_engine.gemspec index 6c4dbfb..ae4f39e 100644 --- a/approval_engine.gemspec +++ b/approval_engine.gemspec @@ -25,7 +25,7 @@ Gem::Specification.new do |spec| DESC spec.license = "MIT" - spec.required_ruby_version = ">= 3.1.0" + spec.required_ruby_version = ">= 3.2.0" # `source_code_uri` is pinned to the repository rather than derived from # `homepage`: moving the homepage some day must not silently repoint the @@ -51,5 +51,7 @@ Gem::Specification.new do |spec| end spec.add_dependency "rails", ">= 7.1", "< 9.0" - spec.add_dependency "shiny_json_logic", "~> 0.3" + # Pinned to the patch series on purpose: pre-1.0 gems may break across a + # minor, and this one evaluates every routing rule. Widen deliberately. + spec.add_dependency "shiny_json_logic", "~> 0.3.6" end diff --git a/test/models/approval_engine/outbox_retention_test.rb b/test/models/approval_engine/outbox_retention_test.rb new file mode 100644 index 0000000..c335b4a --- /dev/null +++ b/test/models/approval_engine/outbox_retention_test.rb @@ -0,0 +1,52 @@ +require "test_helper" + +module ApprovalEngine + # The outbox is a queue: one row per activation, decision and outcome, and + # nothing reads a relayed row again. Without a sweep the table grows forever. + class OutboxRetentionTest < ApprovalEngine::TestCase + setup do + invoice = Invoice.create!(tenant_id: TENANT, amount: 6000) + @approval = Approval.create!(tenant_id: TENANT, target: invoice, status: "approved") + end + + def event(processed:, processed_at: nil, failed_at: nil) + OutboxEvent.create!(tenant_id: TENANT, event_name: "approval.approved", record: @approval).tap do |e| + e.update_columns(processed: processed, processed_at: processed_at, failed_at: failed_at) + end + end + + test "purges relayed events past the window, and keeps recent ones" do + old = event(processed: true, processed_at: 60.days.ago) + recent = event(processed: true, processed_at: 1.day.ago) + + assert_equal 1, OutboxEvent.purge! + + assert_not OutboxEvent.exists?(old.id) + assert OutboxEvent.exists?(recent.id) + end + + test "never purges work that has not been relayed" do + pending = event(processed: false, processed_at: nil) + + OutboxEvent.purge!(older_than: 0.seconds) + + assert OutboxEvent.exists?(pending.id), "an unrelayed event is still owed to the host" + end + + # An event that exhausted its retries is evidence of a broken callback. + test "keeps dead letters however old they are" do + dead = event(processed: true, processed_at: 90.days.ago, failed_at: 90.days.ago) + + OutboxEvent.purge! + + assert OutboxEvent.exists?(dead.id) + end + + test "honours an explicit window and returns the count deleted" do + event(processed: true, processed_at: 10.days.ago) + event(processed: true, processed_at: 10.days.ago) + + assert_equal 2, OutboxEvent.purge!(older_than: 7.days) + end + end +end