Skip to content

Commit 3606fd1

Browse files
frahlgclaude
andauthored
fix(state): three history writers hold their mutex until rollback ends (#1845)
* fix(state): three history writers hold their mutex until rollback ends The control loop's history write, the SQLite history import and the plain archive import held historyWriteMu around a transaction begun on the *sql.DB. When that context ended, database/sql could report the Tx done before SQLite had rolled it back, and the mutex went to the next writer early. They now begin the transaction on a connection of their own and close it before unlocking, as the batch and maintenance paths already do. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * chore(changeset): the control loop already wrote on its own connection Review note: the control loop writes through recordHistoryBatches, which used a Conn already. recordHistoryBatch serves RecordTick*, which only tests call, so the changeset no longer names the control loop. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
1 parent 722827f commit 3606fd1

5 files changed

Lines changed: 53 additions & 17 deletions

File tree

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
"ftw": patch
3+
---
4+
5+
When the import of older history or of plain archive files is stopped or runs out of time, other history writes now wait until SQLite has rolled it back. Before, the next write could start too early and then wait or fail as busy. No data was lost either way.

‎go/internal/state/history_migration.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -299,11 +299,11 @@ func (s *Store) mergeHistoricalSamples(ctx context.Context, samples []Sample, re
299299
}
300300
s.historyWriteMu.Lock()
301301
defer s.historyWriteMu.Unlock()
302-
tx, err := s.history.BeginTx(ctx, nil)
302+
tx, end, err := s.beginHistoryWrite(ctx)
303303
if err != nil {
304304
return err
305305
}
306-
defer tx.Rollback()
306+
defer end()
307307
if err := s.insertSamplesAndHours(ctx, tx, rs); err != nil {
308308
return err
309309
}

‎go/internal/state/history_plain.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -334,11 +334,11 @@ func (s *Store) writeHourBuckets(ctx context.Context, hours map[string]*namedHou
334334
return err
335335
}
336336
defer s.historyWriteMu.Unlock()
337-
tx, err := s.history.BeginTx(ctx, nil)
337+
tx, end, err := s.beginHistoryWrite(ctx)
338338
if err != nil {
339339
return err
340340
}
341-
defer tx.Rollback()
341+
defer end()
342342
var imported bool
343343
if err := tx.QueryRowContext(ctx, `SELECT EXISTS(SELECT 1 FROM history_migrations WHERE name=?)`, receipt).Scan(&imported); err != nil {
344344
return err

‎go/internal/state/history_rollback_test.go‎

Lines changed: 21 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -182,8 +182,10 @@ func isHistoryTestWrite(query string) bool {
182182
return strings.HasPrefix(q, "INSERT ") || strings.HasPrefix(q, "UPDATE ") || strings.HasPrefix(q, "DELETE ")
183183
}
184184

185-
func TestHistoryMaintenanceWaitsForCancelledRollback(t *testing.T) {
186-
for _, kind := range []string{"archive", "backfill", "completion"} {
185+
// Each writer that holds historyWriteMu around a transaction keeps it until
186+
// SQLite has rolled a cancelled transaction back.
187+
func TestHistoryWriterWaitsForCancelledRollback(t *testing.T) {
188+
for _, kind := range []string{"archive", "backfill", "completion", "tick", "cold-hours", "import"} {
187189
t.Run(kind, func(t *testing.T) {
188190
s := freshStore(t)
189191
if kind == "backfill" {
@@ -219,6 +221,14 @@ func TestHistoryMaintenanceWaitsForCancelledRollback(t *testing.T) {
219221
case "backfill":
220222
_, err := s.backfillSeriesHoursSnapshot(ctx, 64)
221223
return err
224+
case "tick":
225+
_, err := s.recordHistoryBatch(ctx, "", "", &HistoryPoint{TsMs: seriesHourMs}, nil, nil, 0)
226+
return err
227+
case "cold-hours":
228+
minute := BucketSummary{StartMS: seriesHourMs, ResolutionMS: ArchiveResolutionMS, FirstMS: seriesHourMs, LastMS: seriesHourMs, N: 1, Sum: 1200, Min: 1200, Max: 1200, Last: 1200}
229+
return s.writeHourBuckets(ctx, map[string]*namedHour{"meter": {driver: "meter", metric: "pv_w", hour: seriesHourMs, sources: []BucketSummary{minute}}}, "plain-archive:rollback-test")
230+
case "import":
231+
return s.mergeHistoricalSamples(ctx, []Sample{{Driver: "meter", Metric: "pv_w", TsMs: seriesHourMs + 1000, Value: 1300}}, nil)
222232
default:
223233
return s.ensureSeriesHours(ctx)
224234
}
@@ -228,23 +238,23 @@ func TestHistoryMaintenanceWaitsForCancelledRollback(t *testing.T) {
228238
d.armed.Store(true)
229239
done := make(chan error, 1)
230240
go func() { done <- run(ctx) }()
231-
// Cancel once the maintenance write is under way, so the rollback
232-
// held below is that write's. A deadline could land earlier, in a
241+
// Cancel once the write is under way, so the rollback held below
242+
// is that write's. A deadline could land earlier, in a
233243
// read that holds no writer mutex yet.
234244
select {
235245
case <-d.blocked:
236246
case err := <-done:
237-
t.Fatalf("maintenance ended before its write: %v", err)
247+
t.Fatalf("the write ended before it wrote: %v", err)
238248
case <-time.After(5 * time.Second):
239-
t.Fatal("maintenance did not reach its write")
249+
t.Fatal("the write did not start")
240250
}
241251
cancel()
242252
select {
243253
case <-d.started:
244254
case err := <-done:
245-
t.Fatalf("maintenance ended before rollback: %v", err)
255+
t.Fatalf("the write ended before rollback: %v", err)
246256
case <-time.After(5 * time.Second):
247-
t.Fatal("maintenance did not start rollback")
257+
t.Fatal("the write did not start rollback")
248258
}
249259
waitCtx, stop := context.WithTimeout(context.Background(), 50*time.Millisecond)
250260
lockErr := lockContext(waitCtx, s.historyWriteMu.TryLock)
@@ -256,15 +266,15 @@ func TestHistoryMaintenanceWaitsForCancelledRollback(t *testing.T) {
256266
select {
257267
case err = <-done:
258268
case <-time.After(5 * time.Second):
259-
t.Fatal("maintenance did not finish after rollback")
269+
t.Fatal("the write did not finish after rollback")
260270
}
261271
if lockErr == nil {
262-
t.Fatal("maintenance released the writer mutex before SQLite rollback finished")
272+
t.Fatal("the write released the writer mutex before SQLite rollback finished")
263273
}
264274
// The archive write also has its own 1 s limit, which a stalled
265275
// test goroutine can let expire before it cancels.
266276
if !errors.Is(err, context.Canceled) && !errors.Is(err, context.DeadlineExceeded) {
267-
t.Fatalf("cancelled maintenance: %v", err)
277+
t.Fatalf("cancelled write: %v", err)
268278
}
269279
if err := run(context.Background()); err != nil {
270280
t.Fatalf("retry after rollback: %v", err)

‎go/internal/state/store_ts.go‎

Lines changed: 23 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -393,11 +393,11 @@ func (s *Store) recordHistoryBatch(ctx context.Context, batchID, payloadHash str
393393

394394
s.historyWriteMu.Lock()
395395
defer s.historyWriteMu.Unlock()
396-
tx, err := s.history.BeginTx(ctx, nil)
396+
tx, end, err := s.beginHistoryWrite(ctx)
397397
if err != nil {
398398
return 0, err
399399
}
400-
defer tx.Rollback()
400+
defer end()
401401
seq, skipped, err := s.writeHistoryBatchTx(ctx, tx, batchID, payloadHash, p, rs, observations)
402402
if err != nil {
403403
return 0, err
@@ -413,6 +413,27 @@ func (s *Store) recordHistoryBatch(ctx context.Context, batchID, payloadHash str
413413
return seq, nil
414414
}
415415

416+
// beginHistoryWrite begins a transaction on a connection of its own, for a
417+
// caller that holds historyWriteMu and calls end before it unlocks. A
418+
// cancelled Tx may report ErrTxDone before its automatic rollback releases
419+
// SQLite's writer. Closing the connection waits for that rollback, so the
420+
// next writer cannot read a snapshot and then fail its upgrade with BUSY.
421+
func (s *Store) beginHistoryWrite(ctx context.Context) (tx *sql.Tx, end func(), err error) {
422+
conn, err := s.history.Conn(ctx)
423+
if err != nil {
424+
return nil, nil, err
425+
}
426+
tx, err = conn.BeginTx(ctx, nil)
427+
if err != nil {
428+
_ = conn.Close()
429+
return nil, nil, err
430+
}
431+
return tx, func() {
432+
_ = tx.Rollback()
433+
_ = conn.Close()
434+
}, nil
435+
}
436+
416437
func ackHistoryReceipts(ctx context.Context, tx *sql.Tx, seq, acknowledgedSequence int64) error {
417438
if acknowledgedSequence <= 0 || seq <= 0 {
418439
return nil

0 commit comments

Comments
 (0)