fix: wal replay recovery by chrollo-lucifer-12 · Pull Request #314 · danieldeer/seriousdb

chrollo-lucifer-12 · 2026-09-30T16:30:51Z

Description

This pr addresses the problems discussed in issue #306

Type of Change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Documentation update

Key Changes

close() now closes the file and sets self._file = None.
Added _ensure_open() so the WAL can reopen its file lazily after being closed.
Update replay and append to _ensure_open()
Wrapped write(), flush(), and fsync() in try/except.If any of them fails, the file is closed and the handle is cleared.The next append() then repairs any partial/torn data before reopening and writing.
Cache.load() close the previous wal before creating a new one.

Validation

Added a benchmark test for wal appends.
Running the test shows the following result after doing the said updates:

------------------------------------------------ benchmark 'wal': 1 tests ------------------------------------------------
Name (time in us) Min Max Mean StdDev Median IQR Outliers OPS (Kops/s) Rounds Iterations

test_wal_append 753.3000 946.1000 809.6280 40.9104 797.3000 57.8000 9;2 1.2351 50 1

Checklist

I reviewed the diff and kept it focused on the described change.
Python changes pass uv run ruff check . and uv run ruff format --check ., or this PR does not change Python.
Changed behavior is covered by appropriate tests, including a regression test for bug fixes; any limitations are explained under Validation.
Validation lists the checks or manual flows I ran and their results.
Relevant documentation is updated, or no documentation change is needed.

VihaanReddyM · 2026-10-05T05:08:31Z

@chrollo-lucifer-12 any updates?

mattsva · 2026-10-08T07:14:29Z

@chrollo-lucifer-12 please update it and fix conflicts

chrollo-lucifer-12 · 2026-10-08T08:46:55Z

@chrollo-lucifer-12 please update it and fix conflicts

Sorry for the delay I'll do it today, i was preoccupied with something

chrollo-lucifer-12 · 2026-10-08T10:13:23Z

@mattsva @VihaanReddyM i have resolved the conflicts

mattsva

Ive used AI (Claude Sonnet 5.5) to help me debug the CI errors. This is about what we got with your changes I guess.

Note this is just a help, first please make all the CI jobs run smoothly, maybe those AI review helps you, if not, its ok, dont take it too serious.

Please fix @chrollo-lucifer-12, afterwards Iam glade to first review the code another time, and then merge it, if it is fine.

chrollo-lucifer-12 · 2026-10-12T12:40:21Z

Ive used AI (Claude Sonnet 5.5) to help me debug the CI errors. This is about what we got with your changes I guess.

Note this is just a help, first please make all the CI jobs run smoothly, maybe those AI review helps you, if not, its ok, dont take it too serious.

Please fix @chrollo-lucifer-12, afterwards Iam glade to first review the code another time, and then merge it, if it is fine.

thanks for the review i will do the changes

…ensure_open should return file

chrollo-lucifer-12 · 2026-10-12T13:44:03Z

@mattsva i have done the requested changes, all tests now pass including the tests and ty typecheck. But in ruff checks i get "Use a context manager for opening files".

mattsva

Over all it is good from code quality. It is not mergeable yet, but it is pretty good.
Please fix the changes, that are overall mainly formatting things, i am not sure if you need to add something to the pyproject.toml to make the SIM115 be quite, but that would be okay too.
Also add the better benchmark, so we can really see how big of a change this is.

At all, really good work @chrollo-lucifer-12 .

Any more questions?

mattsva · 2026-10-12T21:54:18Z

    def __init__(self, filename: str):
        self.filename = filename
        self._offset = os.path.getsize(filename) if os.path.isfile(filename) else 0
+        self._file = open(filename, "a+b")


ruff SIM115 fails here. The handle is kept open on purpose and closed in close(), so please silence it with a reason:
self._file = open(filename, "a+b") # noqa: SIM115 - kept open, closed in close() or something like this.

mattsva · 2026-10-12T21:54:50Z

+    def _ensure_open(self):
+        """Return the open WAL file, reopening it if necessary."""
+        if self._file is None:
+            self._file = open(self.filename, "a+b")


Same as above: add # noqa: SIM115 - reopened lazily, closed in close().

mattsva · 2026-10-12T21:56:35Z

+    def close(self) -> None:
+         """Close the WAL file if it is open."""
+         f, self._file = self._file, None
+         if f is not None:


I guess indentation is off (9 spaces), so ruff format --check fails. Running uv run ruff format . fixes it.

(I guess it is somewhere around here, but uv run ruff format . should definitely catch it)

mattsva · 2026-10-12T21:57:05Z

+            if line:
+                try:
+                    entries.append(WalEntry.from_dict(json.loads(line.decode())))
+                except (json.JSONDecodeError, UnicodeDecodeError, TypeError, ValueError) as e:


This line is too long for ruff format. uv run ruff format . will wrap it.

mattsva · 2026-10-12T22:00:34Z

        self._offset = good_offset
        return entries

    def clear(self) -> None:


If self.close() raises, the temp file created above is left behind, because close() runs outside the try block. Please move it inside the try so the temp file is always cleaned up.
It may be good to have a test for those things.

mattsva · 2026-10-12T22:00:58Z

        actual_size = os.path.getsize(self.filename)
        if actual_size > self._offset:
            with open(self.filename, "r+b") as f:
                f.truncate(self._offset)


Missing newline at end of file. ruff format adds it. (On all files)

mattsva · 2026-10-12T22:02:38Z

+                except OSError:
+                    pass
+                raise
+            old_wal = self.wal


Closing the old WAL after the swap is fine, and safer than closing it first, since a failed load keeps the old state. Please add a test that the old WAL's handle is closed after a second load().

This is optinal, but I guess/hope it helps

mattsva · 2026-10-12T22:04:16Z

@@ -0,0 +1,46 @@
+"""Benchmarks for write-ahead log persistence."""


This measures only the first append on a fresh file, so it mostly shows fsync cost. Since the change is about reusing the handle, please add a second benchmark that appends many entries to one open WAL, and post before/after numbers (command, OS, Python version, workload).

Make it more telling, please.

chrollo-lucifer-12 · 2026-10-15T16:09:09Z

Over all it is good from code quality. It is not mergeable yet, but it is pretty good. Please fix the changes, that are overall mainly formatting things, i am not sure if you need to add something to the pyproject.toml to make the SIM115 be quite, but that would be okay too. Also add the better benchmark, so we can really see how big of a change this is.

At all, really good work @chrollo-lucifer-12 .

Any more questions?

thanks, i dont ususally write python more into golang.

chrollo-lucifer-12 · 2026-10-15T16:30:39Z

Benchmarked WAL handle reuse on Linux (WSL2), Python 3.12.3, using pytest-benchmark.

Command:
uv run pytest benchmarks/test_wal.py --benchmark-only

Workload: one append to a fresh WAL, and 1,000 appends using a single open WAL.

Benchmark | Before | After -- | -- | -- Single append | 1.845 ms | 1.680 ms 1,000 appends | 1,444.807 ms | 1,353.096 ms Average per append (batch) | 1.445 ms | 1.353 ms

The repeated-append benchmark improved by approximately 6.4%.

these are the benchmarks

chrollo-lucifer-12 · 2026-10-15T16:33:13Z

the bottleneck is still fsync, maybe we can try fsync in batches ? @mattsva

chrollo-lucifer-12 added 3 commits September 28, 2026 12:45

fix(wal): improve replay and recovery

8abe56e

fix: manage WAL file handles during load, clear and unsuccessful writes

61d4328

add: simple benchmark test for wal append

c9b704b

chrollo-lucifer-12 changed the title ~~Fix/wal replay recovery~~ fix: wal replay recovery Sep 30, 2026

matheuzgomes reviewed Sep 30, 2026

View reviewed changes

Comment thread src/seriousdb/wal.py

chrollo-lucifer-12 added 2 commits October 8, 2026 15:16

resolve merge conflicts

d4953c1

Merge branch 'main' into fix/wal-replay-recovery

d60e09a

mattsva requested changes Oct 12, 2026

View reviewed changes

mattsva marked this pull request as draft October 12, 2026 12:17

chrollo-lucifer-12 added 4 commits October 12, 2026 18:53

resolve merge conflict, make close public method and idempotent and _…

8ea6fcd

…ensure_open should return file

fix Cache.load

99f9bae

fix: keep cache uninitialized when WAL replay fails

fedd60d

fix: add docstring for close and _ensure_open

de18445

mattsva requested changes Oct 12, 2026

View reviewed changes

fix the format checks and benchmark test

e583d10

		@@ -0,0 +1,46 @@
		"""Benchmarks for write-ahead log persistence."""

Conversation

chrollo-lucifer-12 commented Sep 30, 2026

Description

Type of Change

Key Changes

Validation

------------------------------------------------ benchmark 'wal': 1 tests ------------------------------------------------ Name (time in us) Min Max Mean StdDev Median IQR Outliers OPS (Kops/s) Rounds Iterations

test_wal_append 753.3000 946.1000 809.6280 40.9104 797.3000 57.8000 9;2 1.2351 50 1

Checklist

Uh oh!

Uh oh!

VihaanReddyM commented Oct 5, 2026

Uh oh!

mattsva commented Oct 8, 2026

Uh oh!

chrollo-lucifer-12 commented Oct 8, 2026

Uh oh!

chrollo-lucifer-12 commented Oct 8, 2026

Uh oh!

mattsva left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

chrollo-lucifer-12 commented Oct 12, 2026

Uh oh!

chrollo-lucifer-12 commented Oct 12, 2026

Uh oh!

mattsva left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrollo-lucifer-12 commented Oct 15, 2026

Uh oh!

chrollo-lucifer-12 commented Oct 15, 2026

Uh oh!

chrollo-lucifer-12 commented Oct 15, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

------------------------------------------------ benchmark 'wal': 1 tests ------------------------------------------------
Name (time in us) Min Max Mean StdDev Median IQR Outliers OPS (Kops/s) Rounds Iterations

mattsva left a comment •

edited

Loading

mattsva left a comment •

edited

Loading