Skip to content

Commit 643d767

Browse files
committed
fix: address Geoffrey's review comments
- Handle errors properly in isSingleLedger by returning (bool, error) - Record errors in trace span instead of silently ignoring them - Add explanatory comment for SystemStoreFactory provider - Apply conservative behavior (use filter) when counting fails Addresses review comments from PR #1090
1 parent 3e25109 commit 643d767

File tree

2 files changed

+22
-7
lines changed

2 files changed

+22
-7
lines changed

internal/storage/driver/module.go

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,8 @@ func NewFXModule() fx.Option {
2929
&ledger.Ledger{},
3030
)
3131
}),
32+
// SystemStoreFactory is provided separately to be used both by the Driver
33+
// and by the ledger store factory for counting ledgers in buckets
3234
fx.Provide(func(tracerProvider trace.TracerProvider) systemstore.StoreFactory {
3335
return systemstore.NewStoreFactory(systemstore.WithTracer(
3436
tracerProvider.Tracer("SystemStore"),

internal/storage/ledger/store.go

Lines changed: 20 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -273,31 +273,44 @@ func (store *Store) WithDB(db bun.IDB) *Store {
273273
// This allows queries to skip the WHERE ledger = ? clause when there's only one ledger in the bucket.
274274
// isSingleLedger checks in real-time if the bucket contains only one ledger.
275275
// This query is fast since the ledgers table has very few rows.
276-
func (store *Store) isSingleLedger(ctx context.Context) bool {
276+
func (store *Store) isSingleLedger(ctx context.Context) (bool, error) {
277277
if store.countLedgersInBucket == nil {
278-
return false
278+
return false, nil
279279
}
280280
count, err := store.countLedgersInBucket(ctx, store.ledger.Bucket)
281281
if err != nil {
282-
// On error, be conservative and assume multi-ledger
283-
return false
282+
return false, fmt.Errorf("failed to count ledgers in bucket: %w", err)
284283
}
285-
return count == 1
284+
return count == 1, nil
286285
}
287286

288287
// applyLedgerFilter conditionally applies the WHERE ledger = ? clause to a query.
289288
// If the bucket contains only one ledger, the filter is skipped for performance optimization.
289+
// On error, conservatively applies the filter.
290290
func (store *Store) applyLedgerFilter(ctx context.Context, query *bun.SelectQuery, tableAlias string) *bun.SelectQuery {
291-
if store.isSingleLedger(ctx) {
291+
singleLedger, err := store.isSingleLedger(ctx)
292+
if err != nil {
293+
// Log error but continue with conservative behavior (apply filter)
294+
trace.SpanFromContext(ctx).RecordError(err)
295+
return query.Where(tableAlias+".ledger = ?", store.ledger.Name)
296+
}
297+
if singleLedger {
292298
return query
293299
}
294300
return query.Where(tableAlias+".ledger = ?", store.ledger.Name)
295301
}
296302

297303
// getLedgerFilterSQL returns the SQL condition (without conjunction) and arguments for ledger filtering.
298304
// Returns empty string and nil args if single-ledger optimization is enabled.
305+
// On error, conservatively returns the filter.
299306
func (store *Store) getLedgerFilterSQL(ctx context.Context) (string, []any) {
300-
if store.isSingleLedger(ctx) {
307+
singleLedger, err := store.isSingleLedger(ctx)
308+
if err != nil {
309+
// Log error but continue with conservative behavior (return filter)
310+
trace.SpanFromContext(ctx).RecordError(err)
311+
return "ledger = ?", []any{store.ledger.Name}
312+
}
313+
if singleLedger {
301314
return "", nil
302315
}
303316
return "ledger = ?", []any{store.ledger.Name}

0 commit comments

Comments
 (0)