Skip to content

fix: reap after persisting#1739

Open
RodrigoVillar wants to merge 1 commit intomainfrom
rodrigo/reap-after-persisting
Open

fix: reap after persisting#1739
RodrigoVillar wants to merge 1 commit intomainfrom
rodrigo/reap-after-persisting

Conversation

@RodrigoVillar
Copy link
Contributor

@RodrigoVillar RodrigoVillar commented Mar 4, 2026

Why this should be merged

Fixes the bug described in #1737 by first persisting revisions before reaping nodestores in deferred persistence.

How this works

  • reap() no longer wakes up the background thread
  • when pop returns in the event_loop, we first persist and then reap

How this was tested

CI + added regression test

Breaking Changes

  • firewood
  • firewood-storage
  • firewood-ffi (C api)
  • firewood-go (Go api)
  • fwdctl

@RodrigoVillar RodrigoVillar self-assigned this Mar 4, 2026
@RodrigoVillar RodrigoVillar force-pushed the rodrigo/clarify-deferred-persistence-terminology branch 3 times, most recently from 90c9fd8 to 9f29c24 Compare March 5, 2026 19:16
@RodrigoVillar RodrigoVillar force-pushed the rodrigo/reap-after-persisting branch from 1ca6d10 to 3bd3742 Compare March 5, 2026 19:38
@RodrigoVillar RodrigoVillar changed the title Rodrigo/reap after persisting fix: reap after persisting Mar 5, 2026
@RodrigoVillar RodrigoVillar force-pushed the rodrigo/reap-after-persisting branch 3 times, most recently from a6eb2df to e9234fe Compare March 5, 2026 20:23
Base automatically changed from rodrigo/clarify-deferred-persistence-terminology to main March 6, 2026 11:55
@RodrigoVillar RodrigoVillar force-pushed the rodrigo/reap-after-persisting branch from e9234fe to 3998344 Compare March 6, 2026 14:55
@RodrigoVillar RodrigoVillar force-pushed the rodrigo/reap-after-persisting branch from 3998344 to 51843c1 Compare March 6, 2026 15:01
@RodrigoVillar RodrigoVillar marked this pull request as ready for review March 6, 2026 15:09
Copy link
Member

@rkuris rkuris left a comment

Choose a reason for hiding this comment

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

I would much rather prevent reaping of unpersisted revisions. We already have a mechanism for deferring putting them on the queue for reaping if they are actively being used. We should add a test that verifies that it's been fully persisted as well, which I think is much simpler than this restructuring.

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.

Chaos test unable to reopen database when DeferredPersistenceCommitCount > 1

4 participants