Isolate infinite loop bug, add timeout to actions #583

2024-03-29T23:56:31+08:00

zachgoll commented

2024-03-29 23:56:31 +08:00

(Migrated from github.com)

The following causes an infinite loop on cascade deletes:

Account gets deleted
Dependent valuation/transaction gets deleted, triggering sync_account
sync_account interacts with Account, causes deadlock

# account.rb

has_many :valuations, dependent: :destroy
has_many :transactions, dependent: :destroy

# valuation.rb

after_commit :sync_account

# transaction.rb

after_commit :sync_account

Solution

Add timeout to Github Actions (to protect against this on future PRs)
Trigger sync process explicitly at the controller (rather than model) layer. The sync process will continue to increase in complexity and this gives more flexibility and explicitness for triggering syncs.

The following causes an infinite loop on cascade deletes: 1. Account gets deleted 2. Dependent valuation/transaction gets deleted, triggering `sync_account` 3. `sync_account` interacts with `Account`, causes deadlock ``` # account.rb has_many :valuations, dependent: :destroy has_many :transactions, dependent: :destroy ``` ``` # valuation.rb after_commit :sync_account ``` ``` # transaction.rb after_commit :sync_account ``` ## Solution - Add timeout to Github Actions (to protect against this on future PRs) - Trigger sync process explicitly at the controller (rather than model) layer. The sync process will continue to increase in complexity and this gives more flexibility and explicitness for triggering syncs.

zachgoll (Migrated from github.com) reviewed 2024-03-29 23:59:29 +08:00

app/models/account.rb

						
				@@ -12,3 +12,4 @@

				  has_many :transactions, dependent: :destroy

				  monetize :balance

zachgoll (Migrated from github.com) commented

2024-03-29 23:59:29 +08:00

This is correct behavior and matches the behavior at the database level. The previous version was an oversight.

zachgoll commented

2024-03-30 00:06:29 +08:00

(Migrated from github.com)

Here is the intentionally forced loop error, cancelled after 5 minutes due to GH Action timeout:

See:

https://github.com/maybe-finance/maybe/actions/runs/8482867745/job/23242934822?pr=583

Here is the intentionally forced loop error, cancelled after 5 minutes due to GH Action timeout: ![CleanShot 2024-03-29 at 12 05 06](https://github.com/maybe-finance/maybe/assets/16676157/f81fd4a0-07df-4605-8269-84a8d7db79e9) See: https://github.com/maybe-finance/maybe/actions/runs/8482867745/job/23242934822?pr=583

zachgoll commented

2024-03-30 00:52:18 +08:00

(Migrated from github.com)

This PR is blocking several others, so merging it now.

That said, the strategy for triggering syncs will be an important concept for this app, so feel free to comment here on this PR after it's closed, open a discussion, or separate issues if there's a better way to do this.

This PR is blocking several others, so merging it now. That said, the strategy for triggering syncs will be an important concept for this app, so feel free to comment here on this PR after it's closed, open a discussion, or separate issues if there's a better way to do this.

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: gavin/maybe#583