test_bank_forks_new_rw_arc_memory_leak should test hard fork count, rather than looping until OOM

[roryharr]

Problem

test_bank_forks_new_rw_arc_memory_leak is attempting to loop until OOM by creating bank_forks. This is indeterminate based on system settings. Tested the original commit (prior to the test introduction) and did not OOM on my system.

Summary of Changes

• Change test to count the hard forks of the data structure. This test failed on the original commit (IE It reproduced the issue this test is trying to catch).
• Test now runs in 100ms instead of 112s and is more effective at catching the intended issue

Note: this test + one other cause the runtime tests to take 120s instead of 11s, impeding local development

Fixes #

[codecov-commenter]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 83.1%. Comparing base (5565013) to head (1dfd751).
⚠️ Report is 21 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #8585   +/-   ##
=======================================
  Coverage    83.1%    83.1%           
=======================================
  Files         847      847           
  Lines      368907   368906    -1     
=======================================
+ Hits       306810   306825   +15     
+ Misses      62097    62081   -16     

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[roryharr]

I'm not sure if there is anyone else who should review this one. Test was originally added by a public contribution.

[brooksprumo]

Can you share the PR that added this test originally, please?

Edit: original PR is #1893

[brooksprumo]

• Change test to count the hard forks of the data structure. This test failed on the original commit (IE It reproduced the issue this test is trying to catch).

What did the failure on the original commit look like? Assert? OOM? Something else?

[brooksprumo]

For this PR, is the problem that the test is slow? Or that the test is flaky? Or the test isn't actually practically testing anything?

[roryharr]

• Change test to count the hard forks of the data structure. This test failed on the original commit (IE It reproduced the issue this test is trying to catch).

What did the failure on the original commit look like? Assert? OOM? Something else?

The test as written does not fail on the commit prior to the fix. Likely the system that I have is different than the system that the original test was run on (it was expecting OOM).
The modified version does fail on the commit prior to the fix for the assert on reference count

[roryharr]

For this PR, is the problem that the test is slow? Or that the test is flaky? Or the test isn't actually practically testing anything?

The test (prior to changes) doesn't actually fail prior to the original fix on my system. So the test is not catching anything.

The test as modified is significantly faster, and actually catches the issue that it was intended to test. So both.

[HaoranYi]

The change is correct and improves the test significantly. However, the test needs a comment explaining what it's testing for, since the memory leak issue is not immediately obvious.

Suggestion: Add a comment to clarify the test's purpose

  #[test]
  fn test_bank_forks_new_rw_arc_memory_leak() {
      // This test verifies that BankForks::new_rw_arc() doesn't create a reference cycle.
      //
      // Before PR #1893, there was a cycle:
      //   Arc<RwLock<BankForks>> → Bank → ProgramCache → Arc<RwLock<BankForks>>
      //
      // This happened because new_rw_arc() called:
      //   root_bank.set_fork_graph_in_program_cache(bank_forks.clone())
      //
      // The fix changed it to use a Weak reference:
      //   root_bank.set_fork_graph_in_program_cache(Arc::downgrade(&bank_forks))
      //
      // Breaking the cycle:
      //   Arc<RwLock<BankForks>> → Bank → ProgramCache → Weak<RwLock<BankForks>>
      //
      // Without the fix: strong_count == 2 (the clone creates an extra strong ref)
      // With the fix: strong_count == 1 (only the returned Arc holds a strong ref)
      let GenesisConfigInfo { genesis_config, .. } = create_genesis_config(10_000);
      let bank_forks = BankForks::new_rw_arc(Bank::new_for_tests(&genesis_config));
      assert_eq!(Arc::strong_count(&bank_forks), 1);
  }

This makes it clear to future readers what the test is checking and why it matters, without needing to dig through git history.

[roryharr]

The change is correct and improves the test significantly. However, the test needs a comment explaining what it's testing for, since the memory leak issue is not immediately obvious.

Suggestion: Add a comment to clarify the test's purpose

  #[test]
  fn test_bank_forks_new_rw_arc_memory_leak() {
      // This test verifies that BankForks::new_rw_arc() doesn't create a reference cycle.
      //
      // Before PR #1893, there was a cycle:
      //   Arc<RwLock<BankForks>> → Bank → ProgramCache → Arc<RwLock<BankForks>>
      //
      // This happened because new_rw_arc() called:
      //   root_bank.set_fork_graph_in_program_cache(bank_forks.clone())
      //
      // The fix changed it to use a Weak reference:
      //   root_bank.set_fork_graph_in_program_cache(Arc::downgrade(&bank_forks))
      //
      // Breaking the cycle:
      //   Arc<RwLock<BankForks>> → Bank → ProgramCache → Weak<RwLock<BankForks>>
      //
      // Without the fix: strong_count == 2 (the clone creates an extra strong ref)
      // With the fix: strong_count == 1 (only the returned Arc holds a strong ref)
      let GenesisConfigInfo { genesis_config, .. } = create_genesis_config(10_000);
      let bank_forks = BankForks::new_rw_arc(Bank::new_for_tests(&genesis_config));
      assert_eq!(Arc::strong_count(&bank_forks), 1);
  }

This makes it clear to future readers what the test is checking and why it matters, without needing to dig through git history.

Makes sense! Added.

[brooksprumo]