refactor(sequencers): persist prepended batch

[julienrbrt]

Overview

ref: #2906

[gemini-code-assist]

Summary of Changes

Hello @julienrbrt, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request refactors the batch sequencing mechanism to enhance data durability. Specifically, it modifies the Prepend operation in the batch queue to ensure that transactions returned to the front of the queue are persisted to the database. This critical update addresses a potential data loss scenario where prepended batches were previously only held in memory, making the sequencer more robust against unexpected system restarts or failures.

Highlights

• Batch Persistence: The Prepend function in the batch queue now persists batches to the database, ensuring durability.
• Crash Recovery: This change prevents the loss of transactions that are prepended to the queue in case of system crashes.
• Test Coverage: A new test case has been added to verify the persistence of prepended batches across simulated restarts.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	/gemini help	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[github-actions]

The latest Buf updates on your PR. Results from workflow CI / buf-check (pull_request).

Build	Format	Lint	Breaking	Updated (UTC)
✅ passed	⏩ skipped	✅ passed	✅ passed	Dec 4, 2025, 4:17 PM

[claude]

Claude finished @julienrbrt's task —— View job

---

Comprehensive Code Review

I've performed a thorough review of PR #2907, which adds persistence for prepended batches. This is an important durability improvement for the force inclusion feature. Here's my detailed analysis:

---

1. Code Quality & Architecture ✅

Strengths:

• Clean extraction of persistBatch() helper method eliminates duplication between AddBatch and Prepend - good DRY adherence
• Clear documentation in the Prepend function explaining the intentional bypass of maxQueueSize limits
• Consistent use of mutex locking for thread safety
• Good separation of concerns: persistence happens before in-memory state changes

Observations:

• The implementation correctly persists before modifying in-memory state, ensuring durability
• The Prepend function has two code paths (reusing head space vs. expanding) which is optimal for memory efficiency

---

2. Error Handling & Resilience ⚠️

Issues:

1. Silent error handling in Next() (sequencers/single/queue.go:135-138)

   err = bq.db.Delete(ctx, ds.NewKey(key))
   if err != nil {
       // Log the error but continue
       fmt.Printf("Error deleting processed batch: %v\n", err)
   }

   • Using fmt.Printf for error logging is not production-ready
   • No structured logging with context (request IDs, batch details)
   • Should use the project's standard logger (check if there's a structured logger in use)

2. Silent error handling in Load() (sequencers/single/queue.go:162-163, 168-169)

   if result.Error != nil {
       fmt.Printf("Error reading entry from datastore: %v\n", result.Error)
       continue
   }

   • Same issue: fmt.Printf instead of proper logging
   • Errors during recovery could indicate data corruption but are silently skipped
   • Consider accumulating errors and returning them or at least logging with severity

Recommendation: Replace fmt.Printf with structured logging throughout. Check if there's a logger interface in core/ or use a standard logging approach used elsewhere in the codebase.

---

3. Security Considerations ✅

No security concerns identified:

• No exposure of sensitive data in logs
• Input validation is handled at appropriate layers
• Hash-based keys prevent collisions
• No SQL injection or command injection vectors
• Proper use of context.Context for cancellation

---

4. Performance & Resource Efficiency ⚠️

Concerns:

1. Hash computation overhead in persistBatch()

   • Every Prepend and AddBatch call computes a hash (sequencers/single/queue.go:187)
   • If batches contain many large transactions, this could be expensive
   • Consider: Could the hash be cached on the Batch struct if it's already computed?

2. Load ordering issue (critical architectural concern)

   • Load() function (sequencers/single/queue.go:144) reloads batches from datastore using query.Query{}
   • Datastore queries typically return results in key order (lexicographic hash order)
   • This breaks prepend semantics: prepended batches won't necessarily be at the front after restart
   • Impact: Force-included transactions might not maintain their priority order after a crash

   Example scenario:

   1. Add batch A (hash: abc123...)
   2. Add batch B (hash: def456...)  
   3. Prepend batch C (hash: 111222...)  // Should be first
   4. Crash and restart
   5. Load() reloads in hash order: 111222, abc123, def456
   6. Batch C is first (correct by luck)
   
   But if C's hash was 'zzz999...', it would be loaded last!
   

   Recommendation: Add an ordering mechanism:

   • Option A: Add a sequence number or timestamp to the key/value
   • Option B: Use a separate ordered index in the datastore
   • Option C: Document that ordering is NOT preserved across restarts (if acceptable for single sequencer)

3. Memory efficiency of compaction

   • The compaction logic (sequencers/single/queue.go:117-125) is well-designed with reasonable thresholds
   • Good balance between avoiding frequent compactions and memory leaks

---

5. Testing & Reliability ⚠️

Test Coverage Issues:

1. Test assumes ordering after Load() (sequencers/single/queue_test.go:758-767)

   // First should be prepended batch
   nextBatch, err := queue2.Next(ctx)
   require.NoError(t, err)
   assert.Equal(t, 1, len(nextBatch.Transactions))
   assert.Contains(t, nextBatch.Transactions, []byte("prepended"))

   • This test assumes prepended batch comes first after reload
   • As noted in review comment by gemini-code-assist, this only works if "prepended" hashes lower than "tx2"
   • Test should either:
     • Be updated to only verify presence, not order (if ordering isn't guaranteed)
     • Verify the ordering guarantee is implemented and documented

2. Missing test cases:

   • No test for concurrent Prepend() operations
   • No test for Prepend() with max queue size interactions (though documented as bypass)
   • No test for hash collision handling (unlikely but worth considering)
   • No test for persistBatch() failure scenarios

3. Positive test coverage:

   • Excellent test suite for basic prepend operations
   • Good coverage of edge cases (empty queue, after consuming items, multiple prepends)
   • Strong concurrency tests for general queue operations
   • Good test for persistence across restarts (but with ordering caveat above)

Recommendation: Add test coverage for:

func TestBatchQueue_PrependConcurrency(t *testing.T) { /* ... */ }
func TestBatchQueue_PrependPersistenceFailure(t *testing.T) { /* ... */ }

---

6. Documentation & Developer Experience ✅

Strengths:

• Excellent documentation on Prepend() function explaining the max queue size bypass (sequencers/single/queue.go:70-77)
• Clear comments explaining the persistence flow
• Good inline comments for complex logic (compaction threshold)

Minor suggestions:

• Consider adding a package-level comment explaining the persistence strategy
• Document the Load() ordering behavior (or fix it)

---

7. Additional Observations

Positive:

• The ForceIncludedMask field in the Batch struct (from core/sequencer/sequencing.go:42) is well-documented
• Good backward compatibility handling (mask can be nil)

Concern:

• persistBatch() doesn't persist the ForceIncludedMask field
• The protobuf Batch message (pb.Batch) only has Txs field, not the mask
• Question: Is this intentional? Will force-included transactions lose their mask after restart?

---

Summary & Recommendations

Critical Issues:

1. ⚠️ Load() ordering: The most critical issue is that batch ordering is not preserved across restarts due to hash-based key sorting. This needs to be addressed or explicitly documented as a known limitation.

Important Improvements:
2. 🔧 Replace fmt.Printf with structured logging throughout
3. 🧪 Add test cases for concurrent prepend and ordering guarantees
4. 📚 Document Load() behavior regarding ordering

Nice-to-haves:
5. Consider hash caching if performance profiling shows it's a bottleneck
6. Verify ForceIncludedMask persistence requirements

Verdict:
The core refactoring (adding persistBatch() helper) is excellent. However, the ordering issue with Load() is a significant concern that could impact the force inclusion feature's correctness after crashes. I recommend addressing this before merge, or at minimum, documenting it as a known limitation if ordering isn't critical for the single sequencer use case.

---

Related Review Comments

I've reviewed the existing feedback from other reviewers:

• gemini-code-assist: Correctly identified the DRY opportunity (now addressed) and the ordering concern (still valid and critical)
• alpe: Suggested documentation for max queue size behavior (now addressed) and DRY extraction (completed)
• julienrbrt's responses: Author initially chose not to extract for 2 occurrences (reasonable), but did extract after alpe's request. Test updated to use assert.Contains but ordering issue remains.

---

[gemini-code-assist]

Code Review

This pull request aims to make prepended batches persistent, which is a good improvement for durability. The implementation correctly persists the batch to the database before adding it to the in-memory queue, and a new test case verifies this persistence across restarts.

However, I've identified a couple of issues:

1. There is significant code duplication between the Prepend and AddBatch functions for the database persistence logic. This should be refactored into a helper method to improve maintainability.
2. A more critical issue is that the ordering of prepended batches is not guaranteed after a restart. The Load function reloads batches in an order determined by their hashes, not their insertion order. This breaks the expectation that a prepended item remains at the front of thequeue.
3. The new test case for persistence is currently making assumptions about this load order, which makes it brittle and hides the underlying conceptual issue.

I've left detailed comments on these points. The ordering issue is particularly important to address to ensure the correctness of the queue's behavior after a crash or restart.

[gemini-code-assist]

This part of the test assumes a specific order of batches after Load is called. However, Load retrieves batches from the datastore based on the lexicographical order of their keys (hashes), which doesn't correspond to the order they were added or prepended. The test might pass with the current data ("prepended" vs "tx2"), but it's brittle and could fail with different test data if the hash order changes.

This test's assumption of order masks a conceptual issue with Prepend's persistence where the prepended order is not guaranteed after a restart. If the ordering issue is addressed, this test will be valid. If not, the test should be changed to only verify the presence of batches, not their order, to avoid being flaky.

[julienrbrt]

Good one! This makes sense. I think we do want to preserve ordering, especially for force included transactions. The order must be deterministic between all based sequencers. However, this is the single sequencer, so it doesn't really matter. This is why the queue implementation only uses hash I suppose. I'll update the test.

[julienrbrt]

724c8d8

[codecov]

Codecov Report

❌ Patch coverage is 43.75000% with 9 lines in your changes missing coverage. Please review.
✅ Project coverage is 65.54%. Comparing base (20a5f9b) to head (b6895e5).
⚠️ Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
sequencers/single/queue.go	43.75%	4 Missing and 5 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2907      +/-   ##
==========================================
- Coverage   65.65%   65.54%   -0.12%     
==========================================
  Files          87       87              
  Lines        7926     7932       +6     
==========================================
- Hits         5204     5199       -5     
- Misses       2156     2165       +9     
- Partials      566      568       +2     

Flag	Coverage Δ
combined	65.54% <43.75%> (-0.12%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[alpe]

Nice!
Minor comment on doc an DRY.

[alpe]

Prepend does not check for max queue size. It makes sense for me to grow beyond the limit to get the high priority TX included. Nevertheless, some doc would be good to manage expectations

[julienrbrt]

91518df