Add a sqlite backend option for auto vacuum. - #476
Open
EricHorton wants to merge 21 commits into
Open
Conversation
Add a test that runs a workflow with an activity, removes it via RemoveWorkflowInstances, and then asserts that GetWorkflowInstanceHistory returns empty. The existing test only checks that the instance row is gone (GetWorkflowInstanceState returns ErrInstanceNotFound) but never verifies that history and attributes rows are also deleted. This test currently fails: RemoveWorkflowInstances uses 'id' as the column name in DELETE statements for the history and attributes tables, but 'id' in those tables is the event UUID, not the instance ID. The correct column is 'instance_id'. The DELETE silently matches zero rows, so instance rows are removed but history and attributes leak.
Three bugs fixed in RemoveWorkflowInstances: 1. Column-name mismatch: DELETE statements for the history and attributes tables used "id IN (...)" but the "id" column in those tables holds event UUIDs, not instance IDs. The correct column is "instance_id". The DELETE silently matched zero rows, so instance rows were removed but history and attributes rows leaked. 2. Unbounded SELECT: the initial query selected all matching expired instances with no LIMIT. With a large backlog this causes the activity to exceed ActivityLockTimeout (default 2m), resulting in "context deadline exceeded" on every attempt. The expiration workflow retries (MaxAttempts: 2), both fail, and the next attempt isn't until the next timer cycle — by which time the backlog is larger. Auto-expiration silently never completes. Add LIMIT bounded by the configured BatchSize (default 100). Each activity invocation processes at most one batch. The expiration workflow's built-in loop (10 iterations with ContinueAsNew) handles incremental draining across multiple invocations. 3. Defer in loop: "defer tx.Rollback()" inside the batch for-loop accumulated deferred calls that only executed at function exit. With a single bounded transaction there is no loop. Also adds "completed_at IS NOT NULL" to the WHERE clause to avoid matching incomplete instances, and adds missing "defer rows.Close()".
Problem:
Three functions had error handling that only checked for sql.ErrNoRows,
silently dropping all other scan errors:
if err := row.Scan(&state); err != nil {
if err == sql.ErrNoRows {
return backend.ErrInstanceNotFound
}
} // Other errors silently ignored, function continues!
This is dangerous because database errors (connection issues, type
mismatches, etc.) would be swallowed, and the function would continue
with zero-value data, potentially causing silent data corruption or
confusing downstream errors.
Affected functions:
- removeWorkflowInstance: state variable stays zero, leading to
incorrect "instance not finished" errors
- GetWorkflowInstanceState: returns zero state with nil error
- SignalWorkflow: continues with empty executionID
The fix wraps non-ErrNoRows errors and returns them immediately,
following standard Go error handling patterns.
Before: Non-ErrNoRows errors silently ignored, execution continues
After: All scan errors propagated to caller with context
Problem:
GetStats queries two result sets (workflowRows, activityRows) but
neither has explicit Close() calls. While the transaction's Rollback()
at function end will eventually release resources, this relies on
implicit behavior and delays cleanup.
In Go's database/sql, best practice is to defer Close() immediately
after obtaining rows:
rows, err := tx.QueryContext(...)
if err != nil {
return nil, err
}
defer rows.Close() // Clean up as soon as we're done iterating
This ensures resources are released promptly, even if we return early
due to scan errors during iteration. It also makes the cleanup path
explicit and self-documenting.
The existing code in getPendingEvents, getHistory, and GetFutureEvents
already follows this pattern - this change brings GetStats into
alignment.
Before: Rows closed implicitly when transaction rolls back at function end
After: Rows closed immediately after iteration completes
Two issues fixed in this commit: 1. GetFutureEvents: Remove premature rows.Err() check The original code checked futureEvents.Err() immediately after QueryContext(), but rows.Err() is only populated during iteration, not after the query. This check was always nil and thus useless. The fix moves the Err() check to after the for loop (where it belongs) and moves defer Close() before iteration for proper cleanup on early return from scan errors. Reference: https://pkg.go.dev/database/sql#Rows.Err "Err returns the error, if any, that was encountered during iteration." 2. removeFutureEvent: Return nil instead of stale err variable The function ended with 'return err' but 'err' was the result of the initial QueryContext call - always nil if we reached that point. This was a copy-paste artifact; the correct return is 'return nil'. Before: return err // Always nil, misleading After: return nil // Explicitly correct Both fixes align with patterns used elsewhere in the codebase (getPendingEvents, getHistory) and Go database/sql best practices.
Three bugs fixed in RemoveWorkflowInstances: 1. Column-name mismatch: DELETE statements for the history and attributes tables used "id IN (...)" but the "id" column in those tables holds event UUIDs, not instance IDs. The correct column is "instance_id". The DELETE silently matched zero rows, so instance rows were removed but history and attributes rows leaked. 2. Unbounded SELECT: the initial query selected all matching expired instances with no LIMIT. With a large backlog this causes the activity to exceed ActivityLockTimeout (default 2m), resulting in "context deadline exceeded" on every attempt. The expiration workflow retries (MaxAttempts: 2), both fail, and the next attempt isn't until the next timer cycle — by which time the backlog is larger. Auto-expiration silently never completes. Add LIMIT bounded by the configured BatchSize (default 100). Each activity invocation processes at most one batch. The expiration workflow's built-in loop (10 iterations with ContinueAsNew) handles incremental draining across multiple invocations. 3. Defer in loop: "defer tx.Rollback()" inside the batch for-loop accumulated deferred calls that only executed at function exit. With a single bounded transaction there is no loop. Also adds "completed_at IS NOT NULL" to the WHERE clause to avoid matching incomplete instances, and adds missing "defer rows.Close()".
The expiration workflow previously called RemoveWorkflowInstances once per 24h timer, removing at most BatchSize (100) instances. With 313k+ expired instances, this would take ~8.5 years to drain. Changes: - Backend.RemoveWorkflowInstances now returns (int, error) — the count is already computed (len of the SELECT result), zero extra DB calls - Expiration workflow loops the activity until removed == 0, capped at 100 iterations (10k instances per timer tick) - Each activity call gets its own 2m lock timeout window, avoiding the death-spiral where one call tries to process the entire backlog All backend implementations (sqlite, postgres, mysql, redis) and the mock updated. Existing tests pass with no flakiness.
Set auto_vacuum(full) in the DSN so new databases are created with auto_vacuum enabled from the start. For existing databases, check the current auto_vacuum setting and run VACUUM once to convert if needed. Without this, deleting workflow instances creates freelist pages but never returns disk space to the OS. On high-throughput deployments this causes workflows.db to grow monotonically even with working expiration. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Reduce batch size from 500 to 100 and add 1s sleep between batches. On large databases (100G+), continuous DELETE batches starve GetWorkflowTask/GetActivityTask by holding the SQLite write lock. The 1s yield between batches allows task processing to acquire the lock. At 100 rows/batch with 1s sleep, worst case drain rate is ~6000/min — still fast enough to drain backlogs while keeping tasks flowing. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The auto_vacuum=FULL + VACUUM on startup blocks VMC for hours on large (100G+) databases. VACUUM belongs in a later PR after draining.
sqlite: fix RemoveWorkflowInstances column names, add LIMIT
Ns/sqlite bug fixes scan errors
sqlite: add defer rows.Close() in GetStats
sqlite: fix error handling in events.go
fix: loop expiration activity to drain backlog incrementally
Handle done while in the CancelPending state.
frimpongbright841-sudo
approved these changes
Apr 9, 2026
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
4 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.
Builds on #475 to add a sqlite backend option for auto_vacuum. When enabled, the connection runs
PRAGMA auto_vacuum=fulland then runsVACUUM;once on startup to rebuild the database file and reclaim space.I can rebase once #475 is merged. Only the last two commits are relevant to this change.