[CanonicalTransaction] `assign_ledger_item` silently skips every later `after_create_commit` callback
- 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
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