Skip to content

Conversation

@yzang2019
Copy link
Collaborator

@yzang2019 yzang2019 commented Oct 9, 2025

Describe your changes and provide context

This PR will fix 3 issues:

  1. Flaky unit test is caused by async writes not committing latest version in time before db is closed, resulting in the recovery process being skipped since latestVersion = 0
  2. Async write is only enabled properly when useDedicatedChangelog=true, however, we want all async writes are going through the extra WAL file to avoid data loss so that we can recover during initilization.
  3. Fix closing order, we should close the channel first, wait for all pending changes to be flushed to DB and then closing the WAL

Testing performed to validate your change

@codecov
Copy link

codecov bot commented Oct 9, 2025

Codecov Report

❌ Patch coverage is 65.38462% with 18 lines in your changes missing coverage. Please review.
✅ Project coverage is 40.73%. Comparing base (c3ee918) to head (2f2afd7).
⚠️ Report is 5 commits behind head on main.

Files with missing lines Patch % Lines
ss/store.go 11.76% 12 Missing and 3 partials ⚠️
ss/pebbledb/db.go 85.71% 2 Missing and 1 partial ⚠️

❌ Your patch status has failed because the patch coverage (65.38%) is below the target coverage (70.00%). You can increase the patch coverage or adjust the target coverage.
❌ Your project status has failed because the head coverage (40.73%) is below the target coverage (60.00%). You can increase the head coverage or adjust the target coverage.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #122      +/-   ##
==========================================
+ Coverage   40.00%   40.73%   +0.73%     
==========================================
  Files          59       59              
  Lines        7510     7534      +24     
==========================================
+ Hits         3004     3069      +65     
+ Misses       4161     4120      -41     
  Partials      345      345              
Files with missing lines Coverage Δ
config/config.go 0.00% <ø> (ø)
ss/rocksdb/db.go 56.60% <100.00%> (+11.58%) ⬆️
ss/pebbledb/db.go 60.69% <85.71%> (+5.25%) ⬆️
ss/store.go 36.76% <11.76%> (-11.13%) ⬇️

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.


func (db *Database) Close() error {
// First, stop accepting new pending changes and drain the worker
close(db.pendingChanges)
Copy link
Collaborator

@masih masih Oct 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A small note that if Close is called twice for whatever reason close will panic when the channel is already closed. Ditto for rocksdb.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make sense, will add a check to make sure it's not called twice

},
)
database.streamHandler = streamHandler
go database.writeAsyncInBackground()

Check notice

Code scanning / CodeQL

Spawning a Go routine Note

Spawning a Go routine may be a possible source of non-determinism
@yzang2019 yzang2019 merged commit a309e97 into main Oct 9, 2025
7 of 9 checks passed
@yzang2019 yzang2019 deleted the yzang/fix-async-flush branch October 9, 2025 16:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants