ra_log_meta: fix correctness bugs and harden shu integration - #655
Draft
lukebakken wants to merge 3 commits into
Draft
ra_log_meta: fix correctness bugs and harden shu integration#655lukebakken wants to merge 3 commits into
lukebakken wants to merge 3 commits into
Conversation
Review of the DETS-to-shu replacement surfaced several regressions and sharp edges. This addresses them and points the shu dependency at the fork carrying the matching shu fixes. Persistence correctness: - handle_batch rewrote every non-undefined field on every batch, so a voted_for cleared to undefined on a term change (rabbitmq#111) was never persisted and reappeared after a restart, and every last_applied update needlessly rewrote and fsynced the low-frequency fields. Track which fields actually changed per uid and write only those, so clears persist and a pure last_applied update stays a WAL-only write. - delete only removed the ETS row; the shu record survived, so deleted server metadata resurrected on restart and slots leaked. Persist the delete via shu:delete. - last_applied is fsynced periodically (like the old DETS auto_save) and store_sync/delete_sync force a batch sync, restoring the documented durability contract. Migration: - The DETS file was renamed inside an after clause that ran even when migration failed, discarding the source. Rename only after a successful, synced migration; a failure preserves meta.dets and retries on the next start. - A WAL too small for the batch falls back to incremental writes with compaction between them. A corrupt or unexpectedly shaped DETS fails loudly with the file preserved rather than silently starting empty. - Legacy bare-atom voted_for now round-trips back to the same atom. Compaction and shutdown: - A failed or crashed compaction worker is aborted via shu:abort_compact and the store continues, rather than crash-looping. A full in-memory WAL buffer during compaction applies backpressure by waiting for the compaction to complete. - terminate tolerates a shu:close during compaction, and the ra_log_meta child spec sets shutdown => 30_000 to match the compaction wait budget. Capacity and errors: - expected_count, atom_table_slots and wal_size are configurable via ra env, so the fixed-size store's ceilings can be raised for large deployments. shu:open and write errors surface with clear reasons instead of a badmatch. Tests: each case runs against its own ra system so a restart cannot mask a regression; new guards cover the voted_for clear, delete persistence, compaction, migration-failure preservation, tuple voted_for migration, and the atom-table-full controlled exit. Point the shu dependency at lukebakken/rmq-shu shu-fixes until the shu changes land upstream.
Follow-up to the shu-backed ra_log_meta work, closing hazards found by adversarial review and randomized testing, and pointing shu at the commit that makes slot reuse crash-safe with per-slot generations. Compaction runs asynchronously in a linked, monitored worker. Handle both the monitor DOWN carrying the result and the linked process EXIT, and route a compaction failure through apply_compaction_result, which aborts the compaction and returns the store to normal operation instead of taking ra_log_meta down. A failed compaction can no longer crash the metadata server. Make the DETS-to-shu migration crash atomic. The migration now writes into a temporary meta.shu.migrating and atomically renames it into place only after the data is fully written and synced. A crash midway can no longer leave a partial meta.shu that would suppress the retry, so the next start still re-runs the migration from the intact meta.dets. A failed temp cleanup fails loudly rather than proceeding over a stale temp file. Keep the oversized-batch fallback livelock free. When a coalesced batch exceeds the WAL, fall back to per-op incremental writes with a compaction between them; restructure the drain so a completed compaction actually lets the batch make progress rather than spinning on a WAL that never reports empty. Tighten durability. sync_shu now exits on a genuine fsync failure instead of replying ok, so store_sync and delete_sync never falsely report durability. sync_shu_durable finishes an in-flight compaction before syncing, so a synchronous caller cannot be told its write is durable while the value still sits in an unsynced compaction snapshot. Add tests for the crash atomicity of a failed migration (no partial meta.shu or leftover temp) and for an oversized batch completing rather than hanging.
Collaborator
Author
|
This comment also applies here! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Note
This PR was drafted by Claude (Anthropic's Claude Code) working unsupervised, and is being actively reviewed by @lukebakken before it lands. The two commits address findings from adversarial review passes and randomized testing on top of #624. Design choices (async compaction with linked worker, crash-atomic temp-file migration, exit-on-fsync-error durability) are Claude's; @lukebakken is validating each of them.
Companion PR: rabbitmq/shu#1 carries the paired shu storage-engine fixes. This PR pins that commit in
rebar.lockand must not merge before it.Summary
Stacked on #624 (base =
shu). The two commits address hazards found by adversarial review and randomized testing of the DETS-to-shu replacement, and point the shu dependency at a companion fix in rabbitmq/shu that makes slot reuse crash-safe with per-slot generations.52fbe73fixes correctness and robustness issues in the shu-backedra_log_meta.handle_batchrewrote every non-undefinedfield on every batch, so avoted_forcleared toundefinedon a term change (#111) was never persisted and reappeared after a restart, and everylast_appliedupdate needlessly rewrote and fsynced the low-frequency fields. It now tracks which fields actually changed per uid and writes only those.deleteonly removed the ETS row, so deleted server metadata resurrected on restart and slots leaked in the shu store; the delete is now persisted viashu:delete.last_appliedis fsynced periodically (like the old DETSauto_save) andstore_sync/delete_syncforce a batch sync, restoring the documented durability contract. Migration renaming the DETS file used to happen inside anafterclause that ran even when migration failed, discarding the source; the rename now happens only after a successful, synced migration. A WAL too small for a coalesced batch falls back to incremental writes with a compaction between them, so a large migration cannot deadlock the writer.93fb231closes the remaining hazards found by later adversarial review passes. Compaction runs asynchronously in a linked, monitored worker; both the monitorDOWNcarrying the result and the linked processEXITare handled, and a compaction failure is routed throughapply_compaction_result, which aborts the compaction and returns the store to normal operation instead of takingra_log_metadown. The DETS-to-shu migration is now crash atomic: it writes into a temporarymeta.shu.migratingand atomically renames it into place only after the data is fully written and synced, so a crash midway can no longer leave a partialmeta.shuthat would suppress the retry. The oversized-batch fallback is livelock free: a completed compaction actually lets the batch make progress rather than spinning on a WAL that never reports empty.sync_shunow exits on a genuine fsync failure instead of replyingok, sostore_syncanddelete_syncnever falsely report durability.sync_shu_durablefinishes an in-flight compaction before syncing, so a synchronous caller cannot be told its write is durable while the value still sits in an unsynced compaction snapshot.Both commits share a test suite that isolates each case into its own ra system + data directory so a crash or restart in one test cannot leave shared state masking a regression in another. New cases cover the term-change
voted_forclear (#111), delete persistence across restart, compaction cycles under a small WAL, DETS migration (bare-atom and{Name, Node}tuple forms), migration crash atomicity (no partialmeta.shuor leftover temp), skipping migration whenmeta.shualready exists, oversized-batch progress, and the atom-table-full path surfacing as a controlled process exit rather than an uncaught throw.Test plan
ra_log_meta_SUITE(8 tests, isolated per test)ra_2_SUITEfull integration (50 tests) against the paired shu commitrebar.lockpins the paired shu commitNotes
Kept as a draft while @lukebakken reviews the full pair. The paired shu changes are in rabbitmq/shu#1;
rebar.lockhere pins that commit (35175aa).