Harden the background compaction python tests against polling races

XMLWordPrintableJSON

    • Storage Engines, Storage Engines - Persistence
    • 297.326
    • SE Persistence backlog
    • None

      Issue Summary

      Auditing the ten python tests that drive background compaction, while investigating WT-18376, turned up three brittle patterns that remain after that fix. None has been observed failing yet, but the first can hang a test indefinitely and shares its root condition with WT-18376, which did fail. Each issue is listed below with its own path forward.

      Context

      WT-18376 was caused by test_compact11 sampling background_compact_success and background_compact_bytes_recovered as two separate reads and asserting on whichever recovered-bytes value it happened to catch. The whole run_once pass takes about a second, so a scheduling stall on a loaded host loses all of it. Fixed in PR #14476.

      The safe idiom in this test family, already used by test_compact06, test_compact07, test_compact09, test_compact13 and test_compact14, is to poll monotonic counters that survive the pass ending - background_compact_files_skipped, background_compact_skipped_exclude, or per-file btree_compact_pages_rewritten via get_files_compacted(). Such a loop cannot miss the event: if the pass already finished, the counter is past the threshold. The unsafe idiom is sampling transient state and drawing a conclusion from what was caught.


      Issue 1: turn_on_bg_compact() can spin forever when combined with run_once=true

      compact_util.turn_on_bg_compact() in test/suite/helpers/compact_util.py waits for the server to come up:

      def turn_on_bg_compact(self, config = ''):
          self.session.compact(None, f'background=true,{config}')
          while not self.get_bg_compaction_running():
              time.sleep(0.1)
      

      With run_once=true the server clears background_compact_running as soon as it completes its single pass over the metadata (conn_compact.c, top of __background_compact_server), and nothing ever sets it again. If the caller fails to observe the stat while it is set, the loop never terminates and the test hangs until the Evergreen task times out.

      Confirmed locally: starting a run_once pass on the test_compact11 workload and sleeping 3s before the first read gives

      background_compact_running = 0   success = 5   recovered = 259289088
      

      so the pass did all its work and the helper would still be waiting for a flag that will never be raised again. The pass takes ~0.93s, so this needs roughly nine consecutive missed 0.1s polls - the same class of stall behind WT-18376, hence rarer, and it surfaces as a task timeout rather than an assertion failure.

      Affects test_compact10 and test_compact11, the only two tests that pass run_once=true through the helper. test_compact06, test_compact07 and test_compact09 already sidestep it by calling session.compact() directly, each carrying the comment "Don't use the helper function as the server may go to sleep before we have the time to check it is actually running" - so the hazard is known, it just was not applied to the other two tests.

      Path forward: make the helper tolerate an already-finished pass rather than making every caller work around it. Stop waiting once background_compact_running is set or the server has visibly advanced, for example when the sum of background_compact_success, background_compact_fail and background_compact_skipped has moved from its value before the compact() call. Add a bounded timeout so a genuine failure to start reports an assertion instead of hanging. Then remove the three hand-rolled workarounds in test_compact06, test_compact07 and test_compact09 and have them call the helper again.


      Issue 2: test_compact06 asserts exact equality on a counter that grows once per iteration

      test/suite/test_compact06.py:80:

      while self.get_bg_compaction_files_skipped() == 0:
          time.sleep(1)
      self.turn_off_bg_compact()
      assert self.get_bg_compaction_files_skipped() == 1
      

      The server is enabled without run_once here, so it keeps iterating and skips the history store file once per full iteration. The assertion holds only because WT_BACKGROUND_COMPACT_WAIT_TIME is 10s and the test disables the server well inside that window. A stall longer than the wait time between the poll and turn_off_bg_compact() completing makes the counter 2 and fails the test. The margin is comfortable, so this is low risk, but it is the same brittle shape: an exact-equality assertion on a repeating counter.

      Path forward: assert a lower bound (assertGreaterEqual(..., 1)) since the test only cares that the history store was skipped, or set debug_mode=(background_compact) so the intent is explicit and then keep the exact check with the understanding that the iteration interval drops to 2s. Prefer the lower bound - the test's stated purpose is that the file gets skipped, not how many times.


      Issue 3: test_compact13 and test_compact14 pass background=true twice

      test/suite/test_compact13.py:62 and test/suite/test_compact14.py:53 both build

      bg_compact_config = 'background=true,free_space_target=1MB'
      self.turn_on_bg_compact(bg_compact_config)
      

      and the helper prepends background=true, itself, so the config string reaching session.compact() contains the key twice. WT's config parser takes the last occurrence, so the behaviour is correct today and this is not a race. It is worth cleaning up because it reads as though the caller is responsible for the background key, which invites a future caller to pass background=false and get silently overridden.

      Path forward: drop the background=true, prefix from both call sites so they pass only the options the helper does not supply, matching every other caller.


      Definition of done

      • turn_on_bg_compact() no longer hangs when the run_once pass completes before its first poll, and fails with a clear assertion after a bounded wait if the server genuinely never starts.
      • test_compact06, test_compact07 and test_compact09 use the helper again, with their workaround comments removed.
      • test_compact06 asserts a lower bound rather than exact equality on the skipped-files counter.
      • test_compact13 and test_compact14 no longer pass background=true to the helper.
      • test_compact06, test_compact07, test_compact09, test_compact10, test_compact11, test_compact13 and test_compact14 pass, including with an artificial multi-second stall injected immediately after background compaction is enabled, which is the condition that exposes issue 1.

            Assignee:
            Etienne Petrel
            Reporter:
            Etienne Petrel
            Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

              Created:
              Updated:
              Resolved: