← Zpět na zprávu

auctify — auction domain correctness review

Subject: sinfin/auctify, Rails engine, auction domain. Commit audited: a1b60f0c2b5089b8e5356e93de5dfd559fdbf611, 2024-11-15. Reviewed for: ACB (Aukční dům, www.acb.cz), running the closed "Auctea" application built on this engine. Scope of this document: correctness of the auction domain only — bid ordering, the increment ladder, proxy bids, closing, state machine, money representation, auditability, reserve price, and the tests behind all of those. Security of the wider app, folio, and deployment are out of scope except where they sit directly on the money path.

All line numbers refer to the audited commit. Paths are relative to the repo root.

---

Verdict

Auction-domain correctness score: 4 / 10.

The proxy-bidding engine at the centre of this gem is good. Auctify::BidsAppender implements a correct, terminating, deterministic English-auction proxy algorithm with a step ladder, and it is backed by an unusually thorough table-driven test (test/services/auctify/bid_appender_test.rb:379, the "ultimate bidding test", 12 interleaved manual and limit bids with hand-computed expectations at every step). Money is decimal(12,2) on every price column. Bids are soft-cancelled, never hard-deleted, on the operator path. The closing job takes a per-auction advisory lock. There is Prometheus instrumentation that measures exactly the risk the design carries.

What drags the score to 4 is that none of that correctness is protected against two bids arriving at the same time. Auctify::Sale::Auction#bid! opens a transaction and takes no lock of any kind — no lock!, no with_lock, no SELECT … FOR UPDATE, no advisory lock, no lock_version column, no unique index, no database CHECK constraint. Every invariant the appender computes (minimum next bid, who is winning, the current price, the count of applied bids, the extended end time) is computed in Ruby from a snapshot that a second request can invalidate before the first one commits. At this house's prices, one lost update is a six-figure CZK event.

Two further structural defects compound it: a failed bid is not rolled back, and an unauthenticated scaffold CRUD controller for Auctify::Bid is wired into the engine's routes.

---

1. Bid ordering and concurrency — CRITICAL

1.1 What serialises two simultaneous bids: nothing

PROVED. I searched the whole engine for every locking primitive. The complete result:

app/models/auctify/sale/auction.rb:158-170 is the entire concurrency control:

def bid!(bid)
  ensure_registration(bid)

  ActiveRecord::Base.transaction do
    bid.created_at ||= Time.current
    bap = Auctify::BidsAppender.call(auction: self, bid: bid)
    bap.success? ? after_bid_appended(bap) : after_bid_not_appended(bap)
    bap.success?
  end
end

A bare transaction under PostgreSQL's default READ COMMITTED isolation gives atomicity, not serialisability. Every SELECT inside it sees a fresh snapshot taken at statement start, and none of the reads take a lock. The row lock on auctify_sales is acquired only at the final UPDATE inside succesfull_bid! (app/models/auctify/sale/auction.rb:181) — after every decision has already been made.

The gem already depends on with_advisory_lock (auctify.gemspec:34) and already knows how to use it — app/jobs/auctify/bidding_closer_job.rb:19 wraps closing in with_advisory_lock("closing_auction_#{auction_id}") with the comment "can wait until other BCJob release lock and than continue!". The same three lines were never applied to the path where money is decided.

1.2 The exact interleaving that loses a bid

Take a lot at current_price = 1_000_000, ladder step 50 000 in that band, so current_minimal_bid = 1_050_000.

In this order the higher bid happens to land last and the outcome is accidentally right — except that applied_bids_count is now N+1 instead of N+2, because both requests computed it from a snapshot that excluded the other's row (auction.rb:178).

Reverse the commit order — R2 finishes its UPDATE first, R1's UPDATE lands second — and the database ends in this state:

• auctify_bids contains both rows: Adam 1 050 000 and Lucifer 1 100 000.
• auctify_sales.current_price = 1_050_000.
• auctify_sales.current_winner_id = Adam.
• auctify_sales.applied_bids_count is one short.
• auctify_sales.currently_ends_at is whatever R1's extend_end_time computed, which may be earlier than R2's, shortening the lot.

This is a classic lost update, and it has two distinct money consequences here:

(a) The lot is sold at the wrong price. When the auction closes, close_bidding sets self.winner = current_winner (auction.rb:71). current_winner is not the association reader — it is overridden at auction.rb:202-204 to recompute from the bid table, so it correctly returns Lucifer. But process_bidding_result! (auction.rb:329-338) then calls sold_in_auction!(buyer: current_winner, price: current_price, …) and current_price is the stale column value, 1 050 000. The house awards Lucifer's lot at Adam's price. On a 1.1M CZK hammer that is 50 000 CZK of hammer price plus the commission on it, gone, and the seller has a contractual claim for the difference.

(b) Or the lot never closes at all. sold_in_auction transitions if: :valid? (auction.rb:90), and buyer_vs_bidding_consistence (auction.rb:264-280) asserts sold_price == bidding_result.won_price. won_price is recomputed live from the bid table (bids_appender.rb:42) and equals 1 100 000, while sold_price was set to 1 050 000. The validation fails, the transition is refused, and the lot sits in bidding_ended forever with no buyer, no invoice, and no automatic recovery — EnsureAuctionsClosingJob only scans in_sale (ensure_auctions_closing_job.rb:8).

1.3 The interleaving that produces two bids at the same winning price

PROVED. Two different bidders each post exactly the current minimum in the same instant. Both pass check_price_minimum against the same stale snapshot (step 5 above), both rows are inserted, and both are applied. Which one wins is then decided by scope :ordered, -> { order(price: :desc, id: :desc) } (app/models/auctify/bid.rb:7): at equal price the higher primary key wins, i.e. whichever INSERT happened to draw the later sequence value. Sequence order is not bid order — it is decided by which Puma thread reached the INSERT first, which under load has no relationship to which HTTP request arrived first.

The standard auction rule, and the rule this gem applies elsewhere (test/services/auctify/bid_appender_test.rb:361, "two bids with same limit, first one wins"), is that the earlier bid wins a tie. Here it does not. There is no unique index and no ordering constraint that would prevent or arbitrate the tie.

Two further ordering inconsistencies sit next to this:

• Bid#<=> (app/models/auctify/bid.rb:16-21) breaks ties on created_at then id. The ordered scope that actually decides the winner breaks ties on id alone. The two orderings can disagree.
• created_at is assigned in Ruby, not by the database: bid.created_at ||= Time.current (auction.rb:162). With more than one app server, bid timestamps are subject to clock skew between web dynos, while id is not. The timestamp a dispute would be settled on and the value that actually decides the winner come from different clocks.

1.4 The bid/close race

PROVED. check_auction_state (bids_appender.rb:172-177) validates against the in-memory auction object and compares at whole-second granularity:

return if auction.in_sale? && bid.created_at.to_i <= auction.currently_ends_at.to_i

BiddingCloserJob holds an advisory lock while closing (bidding_closer_job.rb:19), but bid! never asks for that lock, so the two run concurrently:

1. The web request loads the auction at 19:59:59.9 — aasm_state = "in_sale", currently_ends_at = 20:00:00.
2. BiddingCloserJob fires, takes the advisory lock, reloads, closes: aasm_state → "bidding_ended", winner set, currently_ends_at pulled back to now (auction.rb:73-75). Commits.
3. The web request's check_auction_state reads its own stale copy → still in_sale → passes. The bid is inserted.
4. succesfull_bid! (auction.rb:173-182) calls save! on the stale object. Rails writes only dirty attributes, and aasm_state is not dirty on that object — so the row keeps bidding_ended, but current_price, current_winner_id and currently_ends_at are all overwritten, the last pushed two minutes into the future.

Final state: a closed lot with a live bid recorded after closing, a winner column that disagrees with what current_winner recomputes, and a currently_ends_at in the future that nothing will ever act on — EnsureAuctionsClosingJob filters on in_sale (ensure_auctions_closing_job.rb:8), so the lot is never reconsidered. create_jobs (auction.rb:365-377) does re-enqueue the notifier job, because it tests in_sale? on the same stale object, so bidders may also be emailed "bidding is about to end" for a lot that already ended.

Legal exposure: a bidder holds a timestamped confirmation of a bid that the house's own records say arrived after the hammer. Under Czech consumer-contract rules this is exactly the fact pattern a bidder litigates.

1.5 The soft-close extension is also unserialised

extend_end_time (auction.rb:324-327) is correct in isolation: currently_ends_at = [currently_ends_at, bid_time + limit].max, so an early bid never shortens the lot. But under concurrency, each request computes the max against its own stale currently_ends_at and then blindly writes the result. Bid A at 19:59:58 extends to 20:01:58; bid B at 19:59:59 extends to 20:01:59; if B commits first, A's write pulls the deadline back to 20:01:58. One second here, but the same mechanism scales with request latency: a request that stalls for ten seconds between read and write erases ten seconds of somebody else's extension.

1.6 A failed bid is not rolled back — HIGH

PROVED. BidsAppender signals failure by setting a flag, not by raising:

# app/services/auctify/bids_appender.rb:74-81
def append_bids!
  fail! unless bid.save
  if @updated_win_bid.present?
    fail! unless @updated_win_bid.save
  end
end

# :83-92
def update_auction!
  ...
  fail! unless auction.succesfull_bid!(price: new_current_price, winner: winner, time: bid.reload.created_at)
  ...
end

fail! (app/services/auctify/service_base.rb:65-67) only sets @failed = true. Nothing raises ActiveRecord::Rollback, and bid! (auction.rb:161-169) does not raise either — it returns bap.success? as the block value, which Rails ignores. The transaction commits regardless.

So there is a reachable state where bid.save succeeded at bids_appender.rb:75, a later step called fail!, update_auction! was skipped by the unless failed? guard at bids_appender.rb:23, the API returned HTTP 400 "your bid was not accepted" (api/v1/auctions_controller.rb:28) — and the bid row is committed and applied. It will be picked up by ordered_applied_bids and can win the lot. The bidder has been told in writing that they did not bid, and they win.

Two concrete triggers:

• auction.succesfull_bid! returns false when price < current_price or time.blank? (auction.rb:174). Under the §1.2 race, a request can reload a current_price that another transaction has already raised above its own bid.
• @updated_win_bid.save fails validation (bids_appender.rb:79). The defending autobid's price is [price, winning_bid.max_price].min (bids_appender.rb:221); if that lands on a value price_is_rounded (bid.rb:45-49) rejects, the rival's defending autobid is silently dropped, the challenger's bid is kept, and the auction row is never updated. SUSPECTED as to reachability: it depends on the ladder steps and require_bids_to_be_rounded_to that Auctea configures. To confirm, read Auctea's Auctify.configure block and check whether every bid_steps_ladder step is an exact multiple of require_bids_to_be_rounded_to; if not, this path is live.

1.7 What is good here

Credit where it is due:

• BiddingCloserJob does take a correct, correctly scoped advisory lock and re-checks state inside it: auction.close_bidding! if auction.reload.in_sale? (bidding_closer_job.rb:19-21). Two closer jobs for the same lot cannot double-close it. The lock key is per-auction, not global, so closings do not serialise against each other across lots.
• succesfull_bid! has a monotonicity guard, return false if price < current_price (auction.rb:174). It is defeated by stale reads, but the intent is right and it is the correct place for it.
• update_auction! deliberately discards its cached state and re-reads before deciding the new price: @winning_bid = nil; @bids = bids.reload (bids_appender.rb:84-85). Again the right instinct, applied without the lock that would make it sound.

---

2. The increment ladder — MEDIUM

How it is stored and computed

bid_steps_ladder was added as jsonb in db/migrate/20210225103227_add_bid_steps_ladder_to_auctify_sales.rb:5 and is deserialised by a custom ActiveRecord type, app/types/auctify/minimal_bids_ladder_type.rb. The type stores {"0" => 200, "1_000" => 500} in the database and reconstitutes Ruby Range keys on read (:63-75), with the last band always open-ended (Range.new(previous_step.first, nil, false), :72). This is neat and it is tested (test/types/minimal_bids_ladder_type_test.rb, four round-trip cases).

The minimum next bid is computed in two places:

# app/models/auctify/sale/auction.rb:222-228
def minimal_bid_increase_amount_at(price, respect_first_bid: true)
  return 0 if respect_first_bid && ordered_applied_bids.blank?   # first bid may equal the opening price
  return Auctify.configuration.require_bids_to_be_rounded_to if bid_steps_ladder.blank?
  _range, increase_step = bid_steps_ladder.detect { |range, step| range.cover?(price) }
  increase_step
end

# app/services/auctify/bids_appender.rb:98-101
def new_current_minimal_bid
  return current_price if first_bid?
  increase_price(current_price)   # => current_price + minimal_bid_increase_amount_at(current_price, respect_first_bid: false)
end

Where it is enforced

PROVED — it is enforced server-side, on both fields, on the only path that can create a bid through the domain model. check_price_minimum (bids_appender.rb:141-149) runs inside approved_bid? (:58-72), which gates every write in build_result (:21-27):

minimum = changing_own_limit_when_winning? ? current_price : new_current_minimal_bid
if bid.price < minimum
  att = bid.with_limit? ? :max_price : :price
  bid.errors.add(att, :price_is_bellow_minimal_bid, …)
end

Can a crafted amount bypass it?

I worked through each shape a client can post. Auctify::Api::V1::AuctionsController#bid_params permits only :max_price and :price (api/v1/auctions_controller.rb:42), and the registration is looked up server-side from current_user (:49-54), so a bidder cannot bid as someone else.

I could not construct a bypass of the minimum through the domain model. This part is solid.

The operator console endpoint

PROVED. There is no operator bid-placing endpoint in this engine. config/routes.rb:18 exposes namespace :console { resources :bids, only: %i[destroy] } — cancel only. Auctify::Api::V1::Console::BidsController (api/v1/console/bids_controller.rb:11-19) implements destroy and nothing else. The ladder question therefore does not arise for the console. Whether Auctea adds its own operator bid endpoint is not visible from here; SUSPECTED risk — confirm by grepping Auctea for BidsAppender, \.bid! and Auctify::Bid.new outside the engine. Any operator path that constructs Auctify::Bid and calls save instead of auction.bid! bypasses the entire ladder.

The real ladder defects

MEDIUM — the ladder cap is not applied to limit bids. check_price_maximum (bids_appender.rb:151-162) enforces Auctify.configuration.maximal_increase_of_price_proc, the ceiling that stops a fat-finger bid of 10 000 000 where 1 000 000 was meant. Its first line is:

return if bid.with_limit?   # bids_appender.rb:152

So the sanity ceiling applies to direct bids and not to maximum/proxy bids. max_price is bounded only by the column, decimal(12,2) → 9 999 999 999.99 CZK. Git history shows this ceiling was added in response to a real incident: commit c8f50ad, "adding Auctify.configuration.maximal_increase_of_price_proc to set ceil for bid price; close #450". The fix covered one of the two entry points. A bidder who types 10 000 000 into the "maximum" field instead of 1 000 000 will have it accepted, and solve_limits_fight (bids_appender.rb:188-196) will spend it against any rival who pushes. Consequence: a disputed hammer at ten times the intended price, which the house must either enforce against an unwilling buyer or unwind against the seller.

LOW — an incomplete ladder crashes the bid. minimal_bid_increase_amount_at (auction.rb:226) does bid_steps_ladder.detect { … } and destructures the result. If no band covers price, detect returns nil, increase_step becomes nil, and the caller does price + nil (bids_appender.rb:226) → TypeError, a 500 on the bid. The type always generates an open-ended top band (minimal_bids_ladder_type.rb:72), so this needs a ladder that starts above 0 — e.g. {"1000" => 500} typed into the console for a lot with an opening price of 500. There is no validation on bid_steps_ladder anywhere in Sale::Base or Sale::Auction to prevent it.

LOW — increase_price_to walks the ladder one step at a time. bids_appender.rb:229-239:

running_price = current_price
while running_price <= overcome
  running_price = increase_price(running_price)
end

It starts from current_price, not from overcome, so it iterates once per ladder step across the whole gap. It terminates, because every configured step is positive — but a ladder containing a step of 0 in any band would spin forever inside a web request and pin a Puma thread. Nothing validates that steps are positive. SUSPECTED for Auctea: confirm by reading the bid_steps_ladder values actually stored (SELECT DISTINCT bid_steps_ladder FROM auctify_sales) and checking every value is > 0.

---

3. Autobids / proxy bids — the strongest part of the gem

AutobidFillerJob is not the proxy engine

PROVED, and worth stating plainly because the name misleads. app/jobs/auctify/autobid_filler_job.rb:7-12 is a one-off backfill, not part of live bidding:

def perform
  reg_ids = Auctify::Bid.pluck(:registration_id).uniq
  registrations_with_bids = Auctify::BidderRegistration.where(id: reg_ids)
  registrations_with_bids.find_each { |reg| reg.fillup_autobid_flags! }
end

It exists to populate the autobid boolean added in db/migrate/20211022133253_add_autobid_to_auctify_bids.rb, whose up method literally prints RUN \Auctify::AutobidFillerJob.perform_later\`. fillup_autobid_flags! (bidder_registration.rb:54-71) walks one registration's bids oldest-first and marks as autobid` any limit bid that did not raise that bidder's limit. It is a display/labelling flag. It places no bids and changes no price.

Two notes on it: it does Auctify::Bid.pluck(:registration_id) — loading every bid id in the table into Ruby memory — and it writes with bid.save!(validate: false) (bidder_registration.rb:69), bypassing the rounding and max-price validations. Both are acceptable for a one-shot backfill and unacceptable if anything schedules it repeatedly. SUSPECTED: confirm Auctea does not have it on a recurring schedule.

The actual proxy algorithm

Live proxy bidding is BidsAppender#solve_winner (bids_appender.rb:179-186), a clean four-way dispatch on which of the two bids carries a limit:

solve_limits_fight(winning_bid, new_bid)          if  new_bid.with_limit? &&  winning_bid.with_limit?
increase_bid_price(winning_bid, new_bid)          if  new_bid.with_limit? && !winning_bid.with_limit?
duplicate_increased_win_bid(winning_bid, new_bid) if !new_bid.with_limit? &&  winning_bid.with_limit?
# do nothing, all is solved already               if !new_bid.with_limit? && !winning_bid.with_limit?

Two overlapping maximums

solve_limits_fight (bids_appender.rb:188-196):

if winning_bid.max_price < new_bid.max_price
  update_winning_bid_to(winning_bid.max_price)
  new_bid.price = [new_bid.price, increase_price_to(overcome: winning_bid.max_price, ceil: new_bid.max_price)].max
else
  new_bid.price = new_bid.max_price
  update_winning_bid_to(increase_price_to(overcome: new_bid.max_price, ceil: winning_bid.max_price))
end

The higher maximum wins, at one ladder step above the loser's maximum, capped at the winner's own maximum. On an exact tie the else branch runs and the incumbent keeps the lot at exactly the tied price — the first-in-time rule, and it is tested directly (bid_appender_test.rb:361, "two bids with same limit, first one wins"; and step 8 of the ultimate test where both hold 7 000 and Adam, the incumbent, retains it at 7 000).

Determinism, termination, overshoot

• Deterministic: yes, for a fixed serial order of bids. The whole resolution is a pure function of (current_price, ladder, winning_bid, new_bid). It is not deterministic with respect to arrival order — but that is §1's defect, not this algorithm's.
• Terminates: yes. Each call to solve_winner produces at most one new bid and at most one duplicated defending bid (@updated_win_bid, bids_appender.rb:216-223) — there is no recursion and no loop over bidders. The only loop is increase_price_to (:234), bounded as long as every ladder step is positive (see §2).
• Overshoot: PROVED not to overshoot. Three independent guards: update_winning_bid_to clamps with [price, winning_bid.max_price].min (:221); increase_price_to returns [running_price, ceil].min (:238); and the model itself refuses a bid whose price exceeds its own limit, price_is_not_bigger_then_max_price (bid.rb:41-43). Tested end-to-end at bid_appender_test.rb:463, "do not set price above limit".
• No self-overbid: check_same_bidder (bids_appender.rb:164-170) blocks a bidder overbidding themselves unless they are raising their own limit, and check_max_price_increasing (:135-139) requires a raised limit to actually be higher. Both tested (bid_appender_test.rb:285, :302, :317).

This is the part of the gem I would keep.

---

4. Closing and soft close — HIGH

How an auction ends

PROVED. Closing is driven entirely by background jobs, and the gem ships no scheduler:

1. Something in the host app must run Auctify::EnsureAuctionsClosingJob periodically. README:124 — "This is up to You how you run it, but interval between runs must be shorter than Auctify.configuration.auction_prolonging_limit_in_seconds." CHANGELOG:24 says something different — "in periods equals at least half of" the same value. The two published instructions disagree, and the gem contains no cron entry, no sidekiq-cron schedule, no recurring.yml. I grepped for all of them; the only hits are in test/dummy/.
2. EnsureAuctionsClosingJob#perform (ensure_auctions_closing_job.rb:8-19) selects Sale::Auction.in_sale.where("currently_ends_at <= ?", Time.current + checking_period_to_future) and enqueues a BiddingCloserJob per lot — immediately if already past, otherwise set(wait_until: auction.currently_ends_at).
3. BiddingCloserJob#perform (bidding_closer_job.rb:7-24) re-checks the clock, takes the per-auction advisory lock, and calls close_bidding! if still in_sale.

Nothing else enqueues a closer. create_jobs (auction.rb:365-377) enqueues only the "close to end" notifier, never a closer — this was a deliberate change, CHANGELOG version 1.1.0, marked BREAKING.

If the job runner is down at the closing minute

PROVED: the auction closes late, and in the meantime it neither accepts bids nor produces a winner. It freezes.

• aasm_state stays in_sale, winner stays nil, sold_price stays nil. No invoice, no notification, no result page.
• Bidding stops anyway: open_for_bids? (auction.rb:238-240) is in_sale? && Time.current <= currently_ends_at, and check_auction_state (bids_appender.rb:174) rejects any bid past currently_ends_at. So the house does not sell late — it simply does not sell.
• When the runner comes back, the next EnsureAuctionsClosingJob run sweeps it up and closes it with the correct winner. close_bidding records how late it was: Yabeda.auctify.diff_in_closing_time_seconds.set({}, (now - self.currently_ends_at).round) (auction.rb:74) and then pins currently_ends_at back so the published end time is not falsified (auction.rb:75).

That recovery design is genuinely good, and the instrumentation around it is the best thing in the repo. lib/yabeda_config.rb:40-46 exposes current_max_delay_in_closing_auction_seconds — the delay on the oldest in_sale lot that should already have closed, sampled at /metrics scrape time rather than at trigger time so it survives multi-process deployment. If ACB is not alerting on that gauge, that is the single cheapest risk reduction available to them. One Prometheus alert rule covers the entire "job runner is down at the closing minute" scenario.

Soft close / anti-sniping

PROVED present, and correct in the single-threaded case. extend_end_time (auction.rb:324-327) is called from succesfull_bid! on every accepted bid:

new_end_time = bid_time + auction_prolonging_limit_in_seconds
self.currently_ends_at = [currently_ends_at, new_end_time].max

Because of the max, a bid early in the lot's life never shortens it; only bids inside the last limit seconds move the deadline. The limit resolves per sales pack with a global fallback: pack&.auction_prolonging_limit_in_seconds || Auctify.configuration.auction_prolonging_limit_in_seconds (auction.rb:259-261). Tested at auction_config_test.rb:59, :108 and :138.

It is not race-free — see §1.5. Two bids in the same window can each write a deadline computed from a stale read, and the later write wins regardless of which is larger.

MEDIUM — the closer job never reschedules itself

bidding_closer_job.rb:15-17:

if Time.current < auction.currently_ends_at
  Rails.logger.info("Too soon for closing of auction #{auction_label(auction)}.")
else
  ...

When a soft-close extension has moved the deadline, the pre-scheduled closer wakes, finds it is early, logs, and exits without rescheduling. That is by design — CHANGELOG 1.1.0 says the closer "no longer requeues itself", and EnsureAuctionsClosingJob is meant to cover it. But checking_period_to_future (ensure_auctions_closing_job.rb:22-24) reads the global config value, while the actual extension uses the per-pack value (auction.rb:260). If ACB sets a pack's auction_prolonging_limit_in_seconds shorter than the global default and sizes the cron interval against the global default as README:124 instructs, there is a window in which an extended lot has no pending closer and the next sweep is further away than the extension. The lot closes late by up to one cron interval. To confirm: SELECT id, title, auction_prolonging_limit_in_seconds FROM auctify_sales_packs against Auctea's Auctify.configuration.auction_prolonging_limit_in_seconds and the actual cron interval.

LOW — second-granularity comparison at the boundary

check_auction_state truncates both sides with .to_i (bids_appender.rb:174), with the honest comment "comparing time with seconds precision". A bid stamped 20:00:00.999 is accepted against a deadline of 20:00:00.000. That is one extra second of bidding, deliberately, and combined with the two-minute soft close it changes nothing in practice. Noted so it is not mistaken for a bug later.

---

5. State machine — HIGH

The states

Sale::Auction uses AASM with ten states (auction.rb:32-116): offered → accepted → in_sale → bidding_ended → auctioned_successfully → sold, with refused, cancelled, auctioned_unsuccessfully → not_sold as the off-ramps. The transition graph is sensible and the guards are real:

• sold_in_auction fires only from: :bidding_ended, and only if: :valid? (:90), which drags in buyer_vs_bidding_consistence (:264-280) — it refuses to record a sale whose buyer is not the winner or whose price does not match what the bid table says. That is exactly the right check in exactly the right place.
• not_sold_in_auction fires only if: :no_winner? (:98), which refuses if a winner exists (:282-288).
• cancel is only reachable from: [:offered, :accepted] (:114) — a lot in sale cannot be cancelled out from under its bidders. Tested at auction_states_test.rb:48.
• forbid_destroy_if_there_are_bids (:361-363, registered prepend: true at :123) blocks destroying an auction that has bids. Tested at auction_test.rb:153.

Enforced in Ruby only — PROVED

aasm_state is a plain string column with a default and NOT NULL and nothing else (db/migrate/20210217101246_add_state_to_auctify_sales.rb, schema at test/dummy/db/schema.rb:57). There is no PostgreSQL ENUM, no CHECK constraint restricting the value set, and no constraint on legal transitions. Every guard above lives in Ruby, in one class, and is bypassed by:

• update_column, update_columns, update_all — none of which run validations or callbacks;
• assigning the attribute directly, auction.aasm_state = "in_sale"; auction.save — AASM guards event! calls, not attribute writes;
• any SQL run by hand against the production database.

For a system of record in a regulated money business, "the state machine is correct as long as nobody ever writes the column directly" is a weak guarantee. A single CHECK (aasm_state IN (...)) would cost nothing and would at least bound the damage.

Can a sale be reopened after it closed?

Not through the state machine — PROVED. There is no event with to: :in_sale other than start_sale, and start_sale accepts only from: :accepted (auction.rb:60). From bidding_ended onwards there is no path back.

Yes through a direct column write, and a specific one is reachable from the CMS. published= is overridden at auction.rb:129-136:

def published=(value)
  super
  if published?
    accept_offer if offered?
    start_sale   if accepted?
  end
end

start_sale's before block (:53-58) resets current_price to offered_price, currently_ends_at to ends_at, and nulls current_winner and buyer. The state guard means this only fires from accepted, so a bidding_ended lot is safe. But an operator who un-publishes and re-publishes a lot that is in accepted — or who edits aasm_state back to accepted by any means — silently wipes the auction's current state while the bid rows survive. There is no confirmation and no audit trail on that path inside this engine.

Can a bid be placed on a closed sale?

Through Auction#bid!: no, except in the race at §1.4. check_auction_state (bids_appender.rb:172-177) requires in_sale? and a bid time within the deadline, and it is tested (auction_bidding_test.rb:109-115, a bid after close_bidding is rejected with no change in Bid.count; and bid_appender_test.rb:141).

Through Auctify::BidsController: yes, trivially — CRITICAL. See §7.1. That controller never consults the auction at all.

LOW — two scopes named not_sold collide

Sale::Base defines scope :not_sold, -> { where(sold_price: nil) } (base.rb:34). Sale::Auction declares an AASM state also called not_sold (auction.rb:41). AASM creates a scope per state only when the class does not already respond to that name, and Auction inherits the Base scope. So Auctify::Sale::Auction.not_sold almost certainly returns lots with a null sold_price — which includes every in_sale lot — rather than lots in the not_sold state. SUSPECTED, because I could not execute it. Confirm in a console: Auctify::Sale::Auction.not_sold.to_sql — if it prints WHERE sold_price IS NULL rather than WHERE aasm_state = 'not_sold', any reporting query in Auctea that uses it is wrong.

---

6. Money representation — no float in the price path; a float in the commission path

The price path is clean — PROVED

Every column that holds a bid or a hammer price is a fixed-point decimal. From the migrations:

I grepped the entire engine — app/, lib/, db/ — for float, Float and to_f. There is no float and no to_f anywhere on the bid or price path. decimal(12,2) caps a single price at 9 999 999 999.99 CZK, which is four orders of magnitude above this house's records. Comparisons, the ladder arithmetic and increase_price_to all operate on BigDecimal. This is correct and it is the right decision.

Two smaller notes on the schema:

• LOW. reserve_price is the only price column declared without precision/scale (20210412114319:5), so in PostgreSQL it is unconstrained numeric and can hold more than two decimal places while everything it is compared against is rounded to two. success? compares (reserve_price || 0) <= current_price (auction.rb:155); a reserve of 1 000 000.001 can never be met at exactly 1 000 000.00. Cheap to fix, worth fixing for consistency.
• bid_steps_ladder is jsonb with integer step values, not decimals — fine, steps are whole crowns.

The commission path uses a Float literal — MEDIUM

PROVED. app/models/auctify/sale/base.rb:95-107:

def auctioneer_commision_from_seller
  (seller_commission_in_percent || 0) * 0.01 * offered_price      # :96
end

def auctioneer_commision_from_buyer
  return nil if sold_price.nil?
  percent = buyer_commission_in_percent || (pack&.commission_in_percent) || Auctify.configuration.auctioneer_commission_in_percent
  sold_price * percent * 0.01                                     # :106
end

0.01 is a Float, and it is not exactly representable in binary. Line 96 is the worse of the two because of evaluation order: Integer Float is evaluated first, in floating point, before the BigDecimal is ever involved. For a 15% commission on a 1 000 000 CZK lot, 15 0.01 is 0.15000000000000002, and the product is 150000.00000000003. Line 106 is luckier — sold_price percent stays in BigDecimal and only the final 0.01 coerces — but it is the same anti-pattern and the same class of result.

I am rating this MEDIUM, not CRITICAL, and I want to be explicit about why, because the brief asked for CRITICAL on any float in a money path. This float cannot move a hammer price: it is not on the bid path, it does not touch current_price, sold_price or any Bid, and no bidding decision reads it. What it produces is a commission figure with binary noise in the last digits. If Auctea rounds for display, nobody ever sees it; if Auctea persists or sums these values, invoice totals stop being reproducible and will not tie out against a spreadsheet — which is an accounting and audit problem, not a lost-money problem. The fix is one character each: 0.01 → BigDecimal("0.01"), or better, percent / BigDecimal(100).

To confirm the exact behaviour on ACB's Ruby version: rails runner 'p Auctify::Sale::Base.find(<id>).auctioneer_commision_from_buyer.class' and check whether the printed value carries trailing digits beyond two.

---

7. Auditability — CRITICAL, because of one controller

The intended design is append-only, and it is good

PROVED. The operator-facing cancel path does not delete:

# app/models/auctify/bid.rb:23-27
def cancel!
  update!(cancelled: true)
  auction.recalculate_bidding!
end

Auctify::Api::V1::Console::BidsController#destroy (api/v1/console/bids_controller.rb:11-19) maps the HTTP DELETE onto that soft cancel. The bid row survives with cancelled = true; scope :applied and scope :canceled (bid.rb:8-9) let both views be reconstructed. The README states the intent explicitly: "auction stores all bids history even those cancelled by admin". The behaviour is tested — test/controllers/auctify/api/v1/console/bids_controller_test.rb:33 asserts assert_no_difference("auction.bids.count") around each cancel while ordered_applied_bids.count drops by one. Both associations are also declared dependent: :restrict_with_error (auction.rb:18,22; bidder_registration.rb:12,18), so cascading deletes cannot silently take bids with them.

That is the right design, and it means a dispute can in principle be reconstructed six months later from auctify_bids alone: every bid, its price, its limit, its registration, its timestamp, whether it was an autobid, whether it was cancelled.

Then there is Auctify::BidsController — CRITICAL

PROVED. config/routes.rb:5 mounts resources :bids — all seven scaffold actions — pointing at app/controllers/auctify/bids_controller.rb. That controller is the untouched rails generate scaffold output:

# app/controllers/auctify/bids_controller.rb
def index                          # :10-12
  @bids = Bid.all
end

def create                         # :28-36
  @bid = Bid.new(bid_params)
  if @bid.save ...
end

def update                         # :39-45
  if @bid.update(bid_params) ...
end

def destroy                        # :48-51
  @bid.destroy
  redirect_to auctify_bids_url, notice: "Bid was successfully destroyed."
end

def bid_params                     # :60-62
  params.require(:bid).permit(:registration_id, :price, :max_price)
end

Its superclass, Auctify::ApplicationController (app/controllers/auctify/application_controller.rb:1-7), is ActionController::Base plus protect_from_forgery. There is no authentication, no authorization, no CanCan ability check, and no reference to the auction anywhere in the file. Compare the console controller, which at least has before_action :api_authenticate_account! (api/v1/console/bids_controller.rb:8).

Consequences, each of which I can trace end to end:

1. Bid history is not append-only. DELETE /auctify/bids/:id calls @bid.destroy — a hard DELETE. No cancelled flag, no tombstone, no audit row. The deleted bid is gone from auctify_bids, and with it the evidence in any dispute. auction.applied_bids_count is not recalculated, so the auction row keeps a count that no longer matches the table.
2. Bids can be edited after the fact. PATCH /auctify/bids/:id with bid[price] rewrites a historical bid's price. The bid is not re-validated against the ladder or the auction state, because Bid's own validations (bid.rb:13-14) only check rounding and price-vs-limit — everything auction-related lives in BidsAppender, which this path never calls.
3. Bids can be created out of thin air on any lot, in any state. POST /auctify/bids with bid[registration_id], bid[price], bid[max_price] goes straight to Bid#save. No check_price_minimum, no check_auction_state, no check_same_bidder, no check_price_maximum, no registration ownership check. A bid can be inserted on a lot that closed last month, at any price, attributed to any registration whose integer id the attacker guesses. It will then be picked up by ordered_applied_bids and by bidding_result, and current_winner (auction.rb:202-204) recomputes live from the bid table — so the injected bid changes who is shown as winning.
4. Every bidder's secret maximum is published. GET /auctify/bids renders app/views/auctify/bids/index.html.erb:20, <td><%= bid.max_price %></td>, for every row of Bid.all — across all lots, all packs, all time. See §8.

The engine's own test suite confirms these actions work rather than flagging them: test/controllers/auctify/bids_controller_test.rb:13-52 asserts that index returns success, that create increments Bid.count, that update succeeds, and that destroy decrements Bid.count — all with no signed-in user.

SUSPECTED, for ACB specifically: whether these routes are reachable on www.acb.cz depends on whether Auctea mounts Auctify::Engine wholesale or cherry-picks routes, and on whatever before_action Auctea may inject into Auctify::ApplicationController via a decorator. I cannot see Auctea's code. Confirm in under a minute:

curl -s -o /dev/null -w '%{http_code}\n' https://www.acb.cz/auctify/bids
# then, from the Auctea repo:
grep -rn "mount Auctify" config/routes.rb
grep -rn "Auctify::ApplicationController\|Auctify::BidsController" app/ config/
bin/rails routes | grep auctify_bids

Any response other than 404 on the first command is a live CRITICAL. Even a 401 or 302 is worth verifying properly, because it may come from a global filter that a different route shape bypasses.

Other gaps in the audit trail

• MEDIUM — cancelling a bid on an already-sold lot leaves a half-applied change and a 500. Bid#cancel! (bid.rb:23-27) runs two writes with no transaction around them. The first, update!(cancelled: true), commits. The second, auction.recalculate_bidding! (auction.rb:184-195), ends in save! — which for an auctioned_successfully lot runs buyer_vs_bidding_consistence (:264-280) and finds that sold_price no longer equals the recomputed bidding_result.won_price. save! raises ActiveRecord::RecordInvalid, uncaught, and the console endpoint returns a 500. The bid is left cancelled, the auction is left claiming a sale at a price no live bid supports, and the operator sees a stack trace rather than a refusal. This is exactly the operation an operator performs when a fraudulent winning bid is discovered after the hammer — the worst possible moment for a partial write.
• MEDIUM — recalculate_bidding! never updates the winner columns. It rewrites applied_bids_count and (downward only) current_price (auction.rb:185-194) and touches neither current_winner_id nor winner_id. Reads via the overridden current_winner method (:202) recompute and are right; reads via the scope where_current_winner_is (:30) and via winner hit the stale columns and are wrong. Two sources of truth for "who is winning" that diverge the moment a bid is cancelled. The scope is tested only during ordinary bidding (auction_test.rb:168), never after a cancel.
• LOW — previous_winning_bid counts cancelled bids. auction.rb:210-216 walks bids.ordered, not ordered_applied_bids, so it can return a cancelled bid. It is documented as the way a host app finds who to notify about being outbid (auction_callbacks.rb:28), so the wrong person can be notified after a cancel.
• MEDIUM — bid timestamps come from app-server clocks. bid.created_at ||= Time.current (auction.rb:162) is Ruby, not now(). Across multiple web processes the recorded bid times are only as good as NTP on those hosts, while the id sequence that actually decides ties is monotonic. In a dispute, the house's own timestamps may contradict its own ordering.
• No audited integration in this engine. The audits table exists in the test schema (test/dummy/db/schema.rb:113) because folio brings the audited gem, but nothing in auctify declares audited on Auctify::Bid or Auctify::Sale::Auction. SUSPECTED whether Auctea adds it — confirm with grep -rn "audited" app/models/ in Auctea, and SELECT DISTINCT auditable_type FROM audits in production. If bids are not audited, the only record of an operator cancelling a bid is the cancelled flag, with no record of who cancelled it or when.

Can a dispute be reconstructed six months later?

Partly. auctify_bids retains price, limit, registration, autobid flag, cancelled flag and timestamps, and soft cancels preserve the row — that is enough to rebuild the bidding sequence. What is missing: who cancelled a bid and when; any record of a bid deleted via BidsController#destroy; a reliable tie-break where timestamps and ids disagree; and a trustworthy applied_bids_count, which drifts under concurrency (§1.2) and is not repaired by the scaffold controller paths.

---

8. Reserve price — MEDIUM (leak), plus a crash

The reserve is not sent directly — PROVED

app/serializers/auctify/sale/auction_serializer.rb exposes exactly six attributes (:6-11): id, current_price, current_minimal_bid, currently_ends_at, ends_at, open_for_bids?, plus a current_winner block (:13-20). reserve_price is not among them, and neither is any bidder's max_price. BidsAppender's result struct (bids_appender.rb:36-44) likewise carries current_price, current_minimal_bid, winning_bid, winner and won_price — no reserve, and won_price is deliberately masked to nil until the reserve is cleared (:42).

I grepped the whole engine for reserve_price and max_price outside the model and service layers. Outside the appender and the models, max_price appears in exactly three places: the permitted-params list (api/v1/auctions_controller.rb:42 and bids_controller.rb:61 — the bidder's own input) and the scaffold views. reserve_price appears nowhere in any view, serializer or API response.

But the reserve leaks by inference — MEDIUM

PROVED. winner is masked by the reserve (bids_appender.rb:107-116):

def winner
  (overcame_reserve_price? ? winning_bid.bidder : nil)
end

def overcame_reserve_price?
  return false if winning_bid.blank?
  return false if auction.reserve_price.present? && auction.current_price < auction.reserve_price
  true
end

Auction#current_winner (auction.rb:202-204) returns that masked value, and the serializer publishes it to every client (auction_serializer.rb:13-20). So the public JSON for a lot with an unmet reserve reports no current winner even though bids exist, and the instant current_price crosses reserve_price it starts reporting one.

Any client polling that endpoint — and the live site polls it continuously over message_bus long-polling — can therefore read the reserve off the transition: it lies strictly between the last current_price that showed no winner and the first that showed one, i.e. within one ladder step. On a lot with a 50 000 CZK step at the million mark, a bidder learns the confidential reserve to within 50 000 CZK by watching a public feed. Consequence: a bidder stops exactly at the reserve instead of bidding past it, and the seller's confidential floor becomes a public ceiling. On a house that hammers in the millions, a systematic few percent off realised prices is the plausible cost, and the seller has a legitimate complaint about confidentiality.

The fix is small and does not change the auction logic: either always publish current_winner regardless of the reserve, or never publish it until the lot closes. It is the asymmetry that leaks.

And the serializer crashes when there is no winner — MEDIUM

PROVED by reading. auction_serializer.rb:13-20:

attribute :current_winner do |auction|
  winner = auction.current_winner
  {
    id: winner.id,            # :16
    auctify_id: winner.auctify_id,
    to_label: winner.to_label
  }
end

auction.current_winner returns nil whenever the lot has no bids or the reserve is not yet met. nil.id raises NoMethodError, inside a FastJsonapi attribute block, during render — a 500 on the auction detail endpoint. Every test that exercises this serializer places bids first and uses a fixture with no reserve_price (test/controllers/auctify/api/v1/auctions_controller_test.rb:22-24; fixture auction_in_progress at test/fixtures/auctify/sales.yml:41-54), so the nil path is never executed by the suite.

SUSPECTED for ACB: this serializer is referenced in the engine only by the dummy test cell (test/dummy/app/cells/dummy/auctify/auctions/form_cell.rb:19). The real API renders cell("#{global_namespace_path}/auctify/auctions/form", …) (api/v1/auctions_controller.rb:58), i.e. a cell that lives in Auctea. Confirm by checking whether Auctea's cell calls Auctify::Sale::AuctionSerializer — grep -rn "AuctionSerializer" app/ in Auctea. If it does, then every lot with a reserve is serving 500s until its first bid clears the reserve, and the leak in the previous finding is realised as an error rather than a null.

MEDIUM — any CMS account can cancel any bid

Adjacent to the reserve question and on the same money path. api/v1/console/bids_controller.rb:26-28:

def api_authenticate_account!
  fail CanCan::AccessDenied if current_account.blank?
end

The only check is that some Folio::Account is signed in. There is no role check, no CanCan ability, no scoping to the pack or the lot. Any account with any role — an editor who only maintains catalogue pages — can DELETE /auctify/api/v1/console/bids/:id and cancel the winning bid on any lot. The engine's own test is named "all actions are allowed for admin account only" (test/controllers/auctify/api/v1/console/bids_controller_test.rb:73) but only proves that a signed-in bidder (Folio::User, not Folio::Account) gets a 401 — it never tests a non-admin account. The test's name asserts a guarantee the code does not provide.

---

9. Test coverage of 1–8

The suite is 7 355 lines against 8 946 lines of source, and where it covers something it covers it well — the appender tests in particular are table-driven with hand-computed expectations at every step, which is the right way to test this domain. The gaps are not in breadth; they are concentrated in exactly the places where money is at risk.

The one-sentence summary of the coverage picture: everything that can be tested with one thread and one connection is tested well; everything that requires two is not tested at all; and the two most destructive code paths in the engine (BidsController#destroy and #create) have tests that assert they work.

---

What is good — specifically

I want this list to be as concrete as the defect list, because the decision in front of ACB is whether to keep this engine, and the answer is not "throw it away".

---

The three findings most likely to cost ACB money

1. No lock on the bid path — app/models/auctify/sale/auction.rb:161

ActiveRecord::Base.transaction with no lock!, no advisory lock, no lock_version and no unique index. Every bid decision is made from a snapshot another request can invalidate before commit.

Scenario. A lot at 1 000 000 CZK with a 50 000 step. In the final minute two bidders post 1 050 000 and 1 100 000 within the same few hundred milliseconds. Both pass the minimum check against the same stale current_price. Both rows commit. Depending on which UPDATE lands second, auctify_sales.current_price ends at 1 050 000 while a 1 100 000 bid sits in auctify_bids. At close, process_bidding_result! (auction.rb:332) sells at the stale current_price to the recomputed winner — 50 000 CZK of hammer plus commission, lost, with a seller claim for the difference — or, if the consistency validation fires first (auction.rb:274), the transition is refused and the lot sticks in bidding_ended with no buyer and no invoice, requiring manual repair. Neither outcome is detectable from the UI. The fix is three lines: wrap the body of bid! in with_advisory_lock("bidding_auction_#{id}"), exactly as bidding_closer_job.rb:19 already does for closing.

2. Unauthenticated scaffold CRUD on bids — app/controllers/auctify/bids_controller.rb:28-51, routed at config/routes.rb:5

create, update and destroy on Auctify::Bid with no authentication, no authorization, and no reference to the auction. POST /auctify/bids bypasses the ladder, the auction-state check, the self-overbid check and the price ceiling. DELETE /auctify/bids/:id is a hard row delete with no tombstone.

Scenario. Anyone who can reach the route posts bid[registration_id]=<guessed id>&bid[price]=9999999 on a live lot. The bid is inserted with no checks; current_winner recomputes live from the bid table (auction.rb:202-204) and the injected bid wins. Or, after the fact, they DELETE the winning bid — the row is gone, applied_bids_count is not repaired, and there is no record that it ever existed. The exposure is the full hammer price of any lot, plus the loss of the evidence needed to contest it. It also publishes every bidder's secret maximum at GET /auctify/bids (app/views/auctify/bids/index.html.erb:20), which turns proxy bidding against the bidders who use it.

Confirm reachability on ACB in one command: curl -s -o /dev/null -w '%{http_code}\n' https://www.acb.cz/auctify/bids. Anything but 404 is live. Regardless of the answer, these five actions should be deleted from the engine — they are scaffold leftovers with no legitimate use.

3. A rejected bid is still committed — app/services/auctify/bids_appender.rb:75,79,88 with app/models/auctify/sale/auction.rb:161-169

fail! sets a boolean; it never raises, and bid! never rolls back. A bid can be saved, then fail a later step, then have update_auction! skipped by the unless failed? guard at bids_appender.rb:23 — and the transaction commits anyway while the API returns HTTP 400 to the bidder (api/v1/auctions_controller.rb:28).

Scenario. Under the concurrency in finding 1, a bidder's request reloads a current_price that another transaction has already raised past their bid, succesfull_bid! returns false at auction.rb:174, and fail! fires. The bidder is shown "your bid was not accepted" and walks away. The row is committed and applied — so ordered_applied_bids counts it, bidding_result ranks it, and if no higher bid follows, it wins the lot. The house then has a buyer who holds a written rejection of the bid they are being invoiced for, and a losing bidder who was outbid by a bid the system said did not exist. The legal position is indefensible in either direction. The fix is one line: raise ActiveRecord::Rollback if bap.failure? inside the transaction block.

---

Recommended order of work

Ordered by money at risk per hour of effort, not by severity label.

1. Delete Auctify::BidsController and its five routes (config/routes.rb:5, app/controllers/auctify/bids_controller.rb, app/views/auctify/bids/). Half an hour. Removes the largest exposure in the engine. Verify against Auctea first that nothing links to auctify_bids_path.
2. Wrap bid! in the advisory lock the gem already depends on (auction.rb:161). Three lines, copied from bidding_closer_job.rb:19. Then write the missing concurrency test — two threads, two connections, one lot — and let it fail first.
3. Roll back on failure (auction.rb:161-169). One line.
4. Alert on auctify_current_max_delay_in_closing_auction_seconds (lib/yabeda_config.rb:40-46). The metric already exists and is already correct. A Prometheus rule firing above, say, 120 seconds covers the entire "job runner is down" scenario for the cost of a config change.
5. Stop the reserve leaking through current_winner (auction_serializer.rb:13-20 together with bids_appender.rb:107-116) and fix the nil.id crash on the same lines.
6. Apply maximal_increase_of_price_proc to limit bids by deleting bids_appender.rb:152, after checking with ACB what a sane ceiling on a maximum bid is.
7. Wrap Bid#cancel! in a transaction (bid.rb:23-27) and decide explicitly what cancelling a bid on a sold lot should do.
8. Add a CHECK constraint on aasm_state and give reserve_price the same precision: 12, scale: 2 as every other price column.

Items 1–3 are the ones that stop money leaving. Everything after that is hardening.

---

Boundaries of this review

• Auctea is closed source. Every statement about what is reachable on www.acb.cz — route mounting, added before_action filters, whether Auctea overrides Auctify::ApplicationController, which cell renders the auction JSON, whether audited is applied to Auctify::Bid, what Auctify.configure actually sets — is marked SUSPECTED above, with the specific command that settles it. Everything marked PROVED is from reading the code at a1b60f0.
• I did not execute the test suite, and I did not send any request to www.acb.cz. Claims about runtime behaviour are derived from reading, and the two places where that matters (AASM scope collision in §5, BigDecimal/Float coercion in §6) are marked SUSPECTED with the command to confirm.
• The engine pins rails ~> 6.1.4 (auctify.gemspec:30), which is past end of security support, and the repo has had 7 commits since mid-2022. Dependency and maintenance risk is another reviewer's scope; I note it only because it determines how a fix to any of the above would actually be shipped — ACB will be patching a fork, not waiting for upstream.