fix: memory leak in sidekiq#1940
Conversation
superagent-security
Bot
added
the
contributor:verified
|
Caution Review failedAn error occurred during the review process. Please try again later. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplace per-row AR model instantiation with lightweight Structs for balances and holdings, batch upserts using PERSIST_BATCH_SIZE to limit memory, and perform in-place FX conversions on entries instead of duplicating them. ChangesMemory optimization through struct-based data and batched persistence
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/models/holding/gapfillable.rb (1)
23-31:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCarry forward
cost_basison gap-filled holdings.At Line 23, synthetic
Holding::HoldingDataomitscost_basis, which can zero out/lose per-share basis continuity on non-trade dates when these rows are persisted.Based on learnings: store `cost_basis` on `Holding` as a per-share value and preserve that value through importer/materialization paths.💡 Proposed fix
filled_holdings << Holding::HoldingData.new( account_id: previous_holding.account_id, security_id: previous_holding.security_id, date: date, qty: previous_holding.qty, price: previous_holding.price, currency: previous_holding.currency, - amount: previous_holding.amount + amount: previous_holding.amount, + cost_basis: previous_holding.cost_basis )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/models/holding/gapfillable.rb` around lines 23 - 31, The synthetic Holding::HoldingData created when gap-filling (in the block that appends to filled_holdings) currently omits cost_basis and thus drops per-share basis continuity; update the initializer call for Holding::HoldingData to include cost_basis: previous_holding.cost_basis so the gap-filled record carries the per-share cost basis, and ensure downstream importer/materialization paths continue to persist and respect Holding.cost_basis as a per-share value (verify handling in any import/materialize methods that consume Holding::HoldingData).
🧹 Nitpick comments (1)
app/models/balance/sync_cache.rb (1)
40-52: ⚡ Quick winIn-place mutation of AR objects: verify no downstream consumers expect original values.
Mutating loaded Entry instances directly is effective for reducing allocations, but these are still ActiveRecord objects that could be referenced elsewhere via AR's identity map or association caches. If any downstream code during the sync cycle fetches these same entries (e.g., via
account.entrieswithout a fresh query), it would observe the convertedamountandcurrencyvalues.The comment correctly notes these aren't persisted, but consider adding
readonly!to make accidental saves fail-fast:💡 Optional defensive measure
`@converted_entries` ||= account.entries.excluding_split_parents.includes(:entryable).order(:date).to_a.map do |e| + e.readonly! custom_rate = e.entryable.exchange_rate if e.entryable.respond_to?(:exchange_rate)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/models/balance/sync_cache.rb` around lines 40 - 52, The code mutates Entry instances in place (see e.amount_money.exchange_to, e.amount, e.currency) which can leak converted values to other consumers; after setting e.amount and e.currency add a defensive e.readonly! (or otherwise freeze/mark as immutable) before returning e so accidental saves fail-fast and downstream code cannot persist the mutated state; ensure the call is placed in the same block that performs the conversion and keep the existing comment explaining non-persistence.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/models/holding/forward_calculator_test.rb`:
- Around line 134-141: The test helper assert_holdings finds calculated_entry
with calculated.find and then dereferences it without checking for nil; add an
assert_not_nil for calculated_entry (with a clear message including
expected_entry.security_id and expected_entry.date) immediately after the find
in assert_holdings to fail with a helpful assertion instead of raising
NoMethodError before asserting qty/price/amount.
In `@test/models/holding/reverse_calculator_test.rb`:
- Around line 240-248: The test helper assert_holdings is dereferencing
calculated_entry without checking it exists; add an assertion like
assert_not_nil calculated_entry (with a descriptive message referencing
expected_entry.security_id and expected_entry.date) immediately after finding
calculated_entry in the assert_holdings method so the subsequent assert_equal
calls don't raise NoMethodError, then keep the existing qty/price/amount
assertions unchanged.
- Around line 152-158: Add a nil-check for the lookup result before accessing
its fields: ensure you call assert_not_nil calculated_entry (with a helpful
message including expected_entry.security_id and expected_entry.date)
immediately after the calculated.find lookup so the subsequent assert_equal
checks for qty, price, and amount will not raise NoMethodError and will produce
a clear test failure if the calculator did not produce the expected holding.
- Around line 110-116: The test assumes calculated.find returns an entry; add an
explicit nil check: immediately after calculated_entry = calculated.find { |c|
c.security_id == expected_entry.security_id && c.date == expected_entry.date }
call add assert_not_nil calculated_entry, "No calculated entry for
security_id=#{expected_entry.security_id} on #{expected_entry.date}" so
subsequent assertions on calculated_entry.qty/price/amount don't raise
NoMethodError; keep the existing assert_equal lines unchanged.
---
Outside diff comments:
In `@app/models/holding/gapfillable.rb`:
- Around line 23-31: The synthetic Holding::HoldingData created when gap-filling
(in the block that appends to filled_holdings) currently omits cost_basis and
thus drops per-share basis continuity; update the initializer call for
Holding::HoldingData to include cost_basis: previous_holding.cost_basis so the
gap-filled record carries the per-share cost basis, and ensure downstream
importer/materialization paths continue to persist and respect
Holding.cost_basis as a per-share value (verify handling in any
import/materialize methods that consume Holding::HoldingData).
---
Nitpick comments:
In `@app/models/balance/sync_cache.rb`:
- Around line 40-52: The code mutates Entry instances in place (see
e.amount_money.exchange_to, e.amount, e.currency) which can leak converted
values to other consumers; after setting e.amount and e.currency add a defensive
e.readonly! (or otherwise freeze/mark as immutable) before returning e so
accidental saves fail-fast and downstream code cannot persist the mutated state;
ensure the call is placed in the same block that performs the conversion and
keep the existing comment explaining non-persistence.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: eef06f51-6f45-47d8-a82f-11259f8d16db
📒 Files selected for processing (11)
app/models/balance/base_calculator.rbapp/models/balance/materializer.rbapp/models/balance/sync_cache.rbapp/models/holding/forward_calculator.rbapp/models/holding/gapfillable.rbapp/models/holding/holding_data.rbapp/models/holding/materializer.rbapp/models/holding/reverse_calculator.rbtest/models/balance/materializer_test.rbtest/models/holding/forward_calculator_test.rbtest/models/holding/reverse_calculator_test.rb
| def assert_holdings(expected, calculated) | ||
| expected.each do |expected_entry| | ||
| calculated_entry = calculated.find { |c| c.security == expected_entry.security && c.date == expected_entry.date } | ||
| calculated_entry = calculated.find { |c| c.security_id == expected_entry.security_id && c.date == expected_entry.date } | ||
|
|
||
| assert_equal expected_entry.qty, calculated_entry.qty, "Qty mismatch for #{expected_entry.security.ticker} on #{expected_entry.date}" | ||
| assert_equal expected_entry.price, calculated_entry.price, "Price mismatch for #{expected_entry.security.ticker} on #{expected_entry.date}" | ||
| assert_equal expected_entry.amount, calculated_entry.amount, "Amount mismatch for #{expected_entry.security.ticker} on #{expected_entry.date}" | ||
| assert_equal expected_entry.qty, calculated_entry.qty, "Qty mismatch for security_id=#{expected_entry.security_id} on #{expected_entry.date}" | ||
| assert_equal expected_entry.price, calculated_entry.price, "Price mismatch for security_id=#{expected_entry.security_id} on #{expected_entry.date}" | ||
| assert_equal expected_entry.amount, calculated_entry.amount, "Amount mismatch for security_id=#{expected_entry.security_id} on #{expected_entry.date}" | ||
| end |
There was a problem hiding this comment.
Add nil check before accessing calculated_entry fields.
If calculated.find returns nil (e.g., calculator didn't produce expected holding), line 138 will raise NoMethodError instead of a clear assertion failure message. Add assert_not_nil before accessing fields to improve debugging experience.
🛡️ Proposed fix
def assert_holdings(expected, calculated)
expected.each do |expected_entry|
calculated_entry = calculated.find { |c| c.security_id == expected_entry.security_id && c.date == expected_entry.date }
+ assert_not_nil calculated_entry, "No calculated holding found for security_id=#{expected_entry.security_id} on #{expected_entry.date}"
assert_equal expected_entry.qty, calculated_entry.qty, "Qty mismatch for security_id=#{expected_entry.security_id} on #{expected_entry.date}"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/models/holding/forward_calculator_test.rb` around lines 134 - 141, The
test helper assert_holdings finds calculated_entry with calculated.find and then
dereferences it without checking for nil; add an assert_not_nil for
calculated_entry (with a clear message including expected_entry.security_id and
expected_entry.date) immediately after the find in assert_holdings to fail with
a helpful assertion instead of raising NoMethodError before asserting
qty/price/amount.
8e5b80e to
0e93137
Compare
superagent-security
Bot
added
pr:verified
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/models/holding/gapfillable.rb (1)
23-31:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve
cost_basisduring gap-filled holding generation.Gap-filled records copy position fields from
previous_holdingbut currently dropcost_basis. For days without trades, cost basis should carry forward unchanged.Proposed fix
filled_holdings << Holding::HoldingData.new( account_id: previous_holding.account_id, security_id: previous_holding.security_id, date: date, qty: previous_holding.qty, price: previous_holding.price, currency: previous_holding.currency, - amount: previous_holding.amount + amount: previous_holding.amount, + cost_basis: previous_holding.cost_basis )Based on learnings: "store cost_basis on Holding as a per-share value ... Implement this normalization in the provider/import/materialization path."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/models/holding/gapfillable.rb` around lines 23 - 31, The gap-fill creation of Holding::HoldingData is missing the cost_basis field, so update the constructor call in the gap-filling logic to preserve previous_holding.cost_basis (i.e., add cost_basis: previous_holding.cost_basis) so days without trades carry forward the per-share cost basis; ensure the symbol is added alongside account_id, security_id, date, qty, price, currency, and amount in the Holding::HoldingData instantiation that builds filled_holdings from previous_holding.
♻️ Duplicate comments (4)
test/models/holding/reverse_calculator_test.rb (3)
111-116:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd nil check before accessing calculated_entry fields.
The past review comment is still valid: if
calculated.findreturns nil, line 113 will raiseNoMethodErrorinstead of a clear assertion failure.🛡️ Proposed fix
expected.each do |expected_entry| calculated_entry = calculated.find { |c| c.security_id == expected_entry.security_id && c.date == expected_entry.date } + assert_not_nil calculated_entry, "No calculated holding found for security_id=#{expected_entry.security_id} on #{expected_entry.date}" assert_equal expected_entry.qty, calculated_entry.qty, "Qty mismatch for security_id=#{expected_entry.security_id} on #{expected_entry.date}"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/models/holding/reverse_calculator_test.rb` around lines 111 - 116, The test currently calls calculated.find and immediately accesses calculated_entry.qty/price/amount which will raise NoMethodError if calculated_entry is nil; update the test (around the calculated_entry = calculated.find { ... } line in reverse_calculator_test.rb) to assert the found entry is not nil (e.g., assert_not_nil or assert with a clear message using expected_entry.security_id and expected_entry.date) before asserting qty/price/amount, so failures produce a clear assertion message instead of a NoMethodError.
242-247:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd nil check before accessing calculated_entry fields.
This helper has the same issue - the past review comment is still valid. Add
assert_not_nilbefore field assertions to preventNoMethodErrorand provide clear failure messages.🛡️ Proposed fix
def assert_holdings(expected, calculated) expected.each do |expected_entry| calculated_entry = calculated.find { |c| c.security_id == expected_entry.security_id && c.date == expected_entry.date } + assert_not_nil calculated_entry, "No calculated holding found for security_id=#{expected_entry.security_id} on #{expected_entry.date}" assert_equal expected_entry.qty, calculated_entry.qty, "Qty mismatch for security_id=#{expected_entry.security_id} on #{expected_entry.date}"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/models/holding/reverse_calculator_test.rb` around lines 242 - 247, The test accesses calculated_entry fields without checking for nil; add an assertion before the field checks: insert assert_not_nil calculated_entry (with a clear message like "No calculated entry for security_id=... on ...") immediately after locating calculated_entry in the block that iterates expected_entry so the subsequent assert_equal lines for qty, price, and amount (which reference calculated_entry) cannot raise NoMethodError.
153-158:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd nil check before accessing calculated_entry fields.
Same issue as the previous assertion block - the past review comment is still valid. Add
assert_not_nilbefore field access to provide clear error messages if calculator doesn't produce expected holding.🛡️ Proposed fix
expected.each do |expected_entry| calculated_entry = calculated.find { |c| c.security_id == expected_entry.security_id && c.date == expected_entry.date } + assert_not_nil calculated_entry, "No calculated holding found for security_id=#{expected_entry.security_id} on #{expected_entry.date}" assert_equal expected_entry.qty, calculated_entry.qty, "Qty mismatch for security_id=#{expected_entry.security_id} on #{expected_entry.date}"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/models/holding/reverse_calculator_test.rb` around lines 153 - 158, The test accesses calculated_entry fields without verifying the lookup succeeded; add an assert_not_nil for calculated_entry before asserting qty/price/amount so failures show a clear message; specifically, in reverse_calculator_test where calculated_entry = calculated.find { |c| c.security_id == expected_entry.security_id && c.date == expected_entry.date }, insert assert_not_nil calculated_entry, "Missing calculated entry for security_id=#{expected_entry.security_id} on #{expected_entry.date}" (or similar) immediately before the three assert_equal lines to avoid nil method calls.test/models/holding/forward_calculator_test.rb (1)
136-141:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd nil check before accessing calculated_entry fields.
The past review comment on this helper is still valid: if
calculated.findreturns nil (when the calculator doesn't produce an expected holding), line 138 will raiseNoMethodErrorinstead of a clear assertion failure message.🛡️ Proposed fix
def assert_holdings(expected, calculated) expected.each do |expected_entry| calculated_entry = calculated.find { |c| c.security_id == expected_entry.security_id && c.date == expected_entry.date } + assert_not_nil calculated_entry, "No calculated holding found for security_id=#{expected_entry.security_id} on #{expected_entry.date}" assert_equal expected_entry.qty, calculated_entry.qty, "Qty mismatch for security_id=#{expected_entry.security_id} on #{expected_entry.date}"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/models/holding/forward_calculator_test.rb` around lines 136 - 141, The test currently calls calculated.find and immediately accesses calculated_entry.qty/price/amount which will raise NoMethodError if calculated_entry is nil; before the three assert_equal checks add an assertion that calculated_entry is not nil (e.g. assert_not_nil calculated_entry, "Missing calculated entry for security_id=#{expected_entry.security_id} on #{expected_entry.date}") so failures report a clear message, then proceed to assert_equal expected_entry.qty/price/amount against calculated_entry.qty/price/amount.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/models/holding/materializer.rb`:
- Around line 4-7: The current implementation builds the full
holdings_to_upsert_with_cost and holdings_to_upsert_without_cost arrays for all
holdings before calling each_slice, so peak memory still holds the entire upsert
payload; change the construction to stream/buffer upserts and flush every
PERSIST_BATCH_SIZE items instead: while iterating `@holdings` (or the method that
populates these arrays), append each transformed upsert hash to a temporary
buffer and when buffer.size >= PERSIST_BATCH_SIZE call the existing
persist/upsert logic (the code currently invoked inside each_slice) and clear
the buffer, then continue; ensure you update places referencing
holdings_to_upsert_with_cost and holdings_to_upsert_without_cost (the build
loops and the points that currently call each_slice at lines around where those
variables are used) so they no longer accumulate the full collection but instead
use the incremental buffer+flush approach using the PERSIST_BATCH_SIZE constant.
---
Outside diff comments:
In `@app/models/holding/gapfillable.rb`:
- Around line 23-31: The gap-fill creation of Holding::HoldingData is missing
the cost_basis field, so update the constructor call in the gap-filling logic to
preserve previous_holding.cost_basis (i.e., add cost_basis:
previous_holding.cost_basis) so days without trades carry forward the per-share
cost basis; ensure the symbol is added alongside account_id, security_id, date,
qty, price, currency, and amount in the Holding::HoldingData instantiation that
builds filled_holdings from previous_holding.
---
Duplicate comments:
In `@test/models/holding/forward_calculator_test.rb`:
- Around line 136-141: The test currently calls calculated.find and immediately
accesses calculated_entry.qty/price/amount which will raise NoMethodError if
calculated_entry is nil; before the three assert_equal checks add an assertion
that calculated_entry is not nil (e.g. assert_not_nil calculated_entry, "Missing
calculated entry for security_id=#{expected_entry.security_id} on
#{expected_entry.date}") so failures report a clear message, then proceed to
assert_equal expected_entry.qty/price/amount against
calculated_entry.qty/price/amount.
In `@test/models/holding/reverse_calculator_test.rb`:
- Around line 111-116: The test currently calls calculated.find and immediately
accesses calculated_entry.qty/price/amount which will raise NoMethodError if
calculated_entry is nil; update the test (around the calculated_entry =
calculated.find { ... } line in reverse_calculator_test.rb) to assert the found
entry is not nil (e.g., assert_not_nil or assert with a clear message using
expected_entry.security_id and expected_entry.date) before asserting
qty/price/amount, so failures produce a clear assertion message instead of a
NoMethodError.
- Around line 242-247: The test accesses calculated_entry fields without
checking for nil; add an assertion before the field checks: insert
assert_not_nil calculated_entry (with a clear message like "No calculated entry
for security_id=... on ...") immediately after locating calculated_entry in the
block that iterates expected_entry so the subsequent assert_equal lines for qty,
price, and amount (which reference calculated_entry) cannot raise NoMethodError.
- Around line 153-158: The test accesses calculated_entry fields without
verifying the lookup succeeded; add an assert_not_nil for calculated_entry
before asserting qty/price/amount so failures show a clear message;
specifically, in reverse_calculator_test where calculated_entry =
calculated.find { |c| c.security_id == expected_entry.security_id && c.date ==
expected_entry.date }, insert assert_not_nil calculated_entry, "Missing
calculated entry for security_id=#{expected_entry.security_id} on
#{expected_entry.date}" (or similar) immediately before the three assert_equal
lines to avoid nil method calls.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7fd89fb8-78c9-4da4-b2eb-0921557779b8
📒 Files selected for processing (11)
app/models/balance/base_calculator.rbapp/models/balance/materializer.rbapp/models/balance/sync_cache.rbapp/models/holding/forward_calculator.rbapp/models/holding/gapfillable.rbapp/models/holding/holding_data.rbapp/models/holding/materializer.rbapp/models/holding/reverse_calculator.rbtest/models/balance/materializer_test.rbtest/models/holding/forward_calculator_test.rbtest/models/holding/reverse_calculator_test.rb
|
Review: fix memory leak in Sidekiq The batched upsert approach and In-place mutation of ActiveRecord objects in The change from e.amount = new_amount
e.currency = account.currency
e…works safely today because The existing comment explains the intent, but the safety assumption is invisible to future readers. Consider adding a brief note that these objects come from # to_a creates independent instances; no AR identity map is active during sync.
The switch from
Generated by Claude Code |
|
Can you take a look at the PR review comments and address them, @ahnv? 🙏 |
|
Thanks for the thorough review — all three points addressed in In-place mutation / # to_a materialises independent instances; no AR identity map is active during sync,
# so callers holding a reference to the same association will never see these mutations.The broader risk you flagged (dirty attributes, accidental
|
8d76936 to
e2f82f1
Compare
superagent-security
Bot
removed
the
contributor:verified
|
Hey @jjmata, I have rebased main into this branch and resolved the conflicts |
|
Can you fix the broken unit tests @ahnv? |
Sure thing, sorry i was afk for a while |
…tion
Profiling of SyncJob (StackProf object mode + Sidekiq memory middleware)
showed peaks of 600k-1.1M live heap slots per job and ~196k retained
ActiveModel::Attribute::FromUser objects post-GC, driven by full-history
in-memory accumulation in the balance/holding sync pipeline.
Changes:
- Replace Holding.new / Balance.new in calculators with lightweight
Struct-based HoldingData / BalanceData. Skips AR attribute sets,
belongs_to proxies, dirty tracking, type casting, and callbacks
that were never used (upsert_all bypasses validations/callbacks
anyway). Eliminates ~30% of allocations and the bulk of retained
ActiveModel::Attribute::* instances.
- Build upsert payloads directly from struct fields instead of
Holding/Balance#attributes.slice(...).
- Batch upsert_all in PERSIST_BATCH_SIZE (2,000) slices in both
Balance::Materializer and Holding::Materializer so the intermediate
attribute-hash array is bounded instead of holding the full
multi-year history alongside the calculator output.
- Replace account.holdings.reload with account.holdings.reset in
Holding::Materializer. Same cache invalidation, no eager re-query;
the next consumer (Balance::SyncCache) loads on demand.
- Mutate entries in place in Balance::SyncCache#converted_entries
instead of Entry#dup. The instances are scoped to the throwaway
sync-cache and never persisted, so dup'ing was producing tens of
thousands of unused FromUser/FromDatabase attribute wrappers per
sync.
All persist paths run inside the existing Balance.transaction wrapper,
so batched upserts retain transactional atomicity. No production caller
of Balance::SyncCache or Holding::Materializer reuses the affected
instances outside the materializer's lifetime.
Test coverage: balance/{sync_cache,materializer,forward_calculator,
reverse_calculator} and holding/{materializer,forward_calculator,
reverse_calculator} plus account/syncer and sync (82 runs, 2,548
assertions, 0 failures).
Replace full-array accumulation + each_slice in Materializer#persist_holdings with two flush-on-fill buffers (holdings_buffer_to_upsert_with_cost / holdings_buffer_to_upsert_without_cost) that upsert and clear at PERSIST_BATCH_SIZE, keeping peak RSS bounded to ~2x batch size. Also add assert_not_nil guards in ReverseCalculatorTest before dereferencing calculated.find results to surface clear failures instead of NoMethodError.
…ent sync mutation safety - Extract Balance::BalanceData struct into its own file (app/models/balance/balance_data.rb) so it is discoverable without knowing it lived inside BaseCalculator - Remove inline Struct definition from Balance::BaseCalculator; update build_balance to reference Balance::BalanceData explicitly (required because class Foo::Bar syntax does not nest Foo in constant lookup) - Add comment to SyncCache#converted_entries clarifying that to_a materialises independent AR instances with no identity map active, making in-place mutation safe - Update all Balance::BaseCalculator::BalanceData references in materializer_test
e2f82f1 to
fd21371
Compare
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
|
@jjmata fixed |
…y and push account-entry join to SQL - Balance::Materializer#purge_stale_balances: replace sort_by(&:date) + first/last with minmax_by(&:date) to avoid full sort when only min/max are needed - MarketDataImporter: replace Entry.group(:account_id).minimum(:date) (which loads all account IDs into a Hash) with a LEFT JOIN subquery that computes MIN(date) per account in SQL and exposes it as first_entry_date on the Account relation
3 participants
Resolves #1935
This pull request introduces memory and performance optimizations for balance and holding materialization, and standardizes the data structures used for balances and holdings. The main changes include introducing lightweight Structs for in-memory calculations, batching upserts to reduce memory usage during sync, and updating code and tests to use the new data structures.
Memory and Performance Optimizations:
Balance::MaterializerandHolding::Materializernow batch database upserts in chunks (PERSIST_BATCH_SIZE = 2_000) to reduce peak memory usage during sync operations. This prevents large attribute arrays from being held in memory simultaneously with the full in-memory collections. [1] [2] [3] [4]converted_entriesmethod inBalance::SyncCachenow mutates entry instances in place instead of duplicating them, significantly reducing memory allocations during sync..resetinstead of.reloadafter materialization, avoiding unnecessary eager loading of large collections.Data Structure Standardization:
Balance::BaseCalculator::BalanceDataandHolding::HoldingDataas lightweight Structs for in-memory calculations, replacing previous uses ofBalanceandHoldingmodel instances. [1] [2]Test and Assertion Updates:
security_idinstead ofsecurityobjects, and error messages are updated for clarity. [1] [2] [3] [4]Attribute Handling Improvements:
.attributes, ensuring only the necessary fields are included and improving performance.These changes improve the efficiency and reliability of balance and holding calculations, especially for accounts with large historical data.
Summary by CodeRabbit
Performance
Refactor
Tests