hackclub / hackclub/hcb

[CanonicalTransaction] `assign_ledger_item` silently skips every later `after_create_commit` callback

Open
#14,967 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
Ruby
Stars
870
Forks
138
Avg merge
1d 14h
Merged PRs (30d)
208

Description

### Summary

`CanonicalTransaction#assign_ledger_item` runs in an `after_create_commit` and calls `update!` on **itself**. That nested write commits, which clears the record's "newly created" state, so **every `after_create_commit` declared after it is silently skipped**.

Two callbacks are lost on every CT that goes through `assign_ledger_item`:

- `after_create_commit :write_system_event` — https://github.com/hackclub/hcb/blob/main/app/models/canonical_transaction.rb#L150
- the Stripe block that runs `PendingEventMappingEngine::Settle::Single::Stripe` and `EventMappingEngine::Map::Single::Stripe` — https://github.com/hackclub/hcb/blob/main/app/models/canonical_transaction.rb#L151-L156

### Evidence

A/B test, instrumenting `write_system_event` and `assign_ledger_item` with a prepended module:

```
[A: ledger_item not pre-set => assign_ledger_item runs] {assign_ledger_item: 1}
[B: ledger_item pre-set => assign_ledger_item skipped] {write_system_event: 1}

=> write_system_event ran 0x when assign_ledger_item ran, 1x when it did not
```

When `assign_ledger_item` is skipped, `write_system_event` fires normally. When it runs, `write_system_event` never fires at all.

### Why it happens

`assign_ledger_item` is declared at [canonical_transaction.rb:138](https://github.com/hackclub/hcb/blob/main/app/models/canonical_transaction.rb#L138), *before* the two callbacks above. Inside it:

```ruby
ActiveRecord::Base.transaction do
li = calculated_ledger_item || create_ledger_item!(...)
update!(ledger_item: li) # <-- nested commit on self, resets the created-state flag
li.map!
end
```

Because this already runs in `after_create_commit`, the outer transaction has committed, so `ActiveRecord::Base.transaction` opens a **new real transaction**. Committing it runs commit callbacks for the record and leaves it no longer flagged as newly created, so the remaining `after_create_commit` entries are skipped when control returns to the callback chain.

### Scope

**`CanonicalPendingTransaction` is not affected.** It declares `after_create_commit :write_system_event` *before* `after_create_commit :assign_ledger_item` ([canonical_pending_transaction.rb:171-178](https://github.com/hackclub/hcb/blob/main/app/models/canonical_pending_transaction.rb#L171-L178)), so its system event has already fired by the time the nested write happens. Only `CanonicalTransaction` has the damaging order.

### Contributing factor

For Stripe card settlements this path is reached more often than it looks. The `before_create` ledger-item inference at [canonical_transaction.rb:127-135](https://github.com/hackclub/hcb/blob/main/app/models/canonical_transaction.rb#L127-L135) never reaches its `raw_stripe_transaction` branch, because `linked_object` returns a `CardCharge` and short-circuits the `elsif` chain one branch earlier:

```ruby
elsif linked_object.present?
linked_object.try(:canonical_pending_transaction).try(:ledger_item_id) # CardCharge => nil
elsif raw_stripe_transaction&.stripe_authorization_id # never reached
```

`CardCharge` does not respond to `canonical_pending_transaction`, so `try` returns `nil`, `ledger_item_id` stays unset, and `assign_ledger_item` runs — triggering the bug.

### Suggested fix

Move `assign_ledger_item` to be declared last among the `after_create_commit` callbacks, or take the self-`update!` out of the commit callback (e.g. `update_column` / assigning `ledger_item_id` in `before_create`) so it does not open a nested transaction on itself. The `before_create` `elsif` ordering is worth fixing at the same time.

### How this was found

Surfaced while auditing redundant `Ledger::Item#map!`/`#refresh!` calls on transaction creation, and verified independently with the A/B test above.

Contributor guide

Open the contributing guide

Research direction

Start with the after_create_commit declarations and assign_ledger_item in app/models/canonical_transaction.rb, then compare the callback order in app/models/canonical_pending_transaction.rb. Reproduce the described A/B instrumentation around assign_ledger_item and write_system_event. Done means later after_create_commit callbacks consistently run for CanonicalTransaction records, without changing the unaffected pending-transaction behavior.

Written by the indexing model from the issue text.

Assessment

Tech stack
ruby
Domain
backend, payments
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
52/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.