Repository navigation
fix: remove dead Stmt branch, fix prepare resource leak, fix errors.Is in execJob - #80
Merged
Merged
Conversation
Contributor
Author
|
Self-review caught two issues in the original submission — pushed a follow-up commit to fix both:
The core fix — moving |
deolreet21
changed the title
…execJob Two bugs together caused the prepared-statement feature to be broken: 1. In loadTasks (tasks.go), db.Prepare() returned a *sql.Stmt that was discarded with _. The stmt variable was never assigned, so task.Stmt was always nil. Fix: store the first returned *sql.Stmt on the task. 2. In execJob (core.go), the raw db.QueryContext() call was outside the else block, so it executed unconditionally after the if/else. When task.Stmt != nil, db is never set (nil pointer), which would panic. Fix: move the raw query call inside the else block.
…-specific Storing the first *sql.Stmt from db.Prepare() is unsafe when a task has multiple source DBs: the stmt is bound to one connection but execJob may route the job to any DB in the pool. The variable shadowing (s := db.Prepare(s.Query)) was also a bug that would have caused a compile-time-invisible runtime error. The core fix (moving db.QueryContext inside the else block in execJob) is sufficient to prevent the nil-pointer dereference. Validation of the SQL against all tagged DBs is retained; the stmt result is simply discarded as the original code intended.
Before: Task.Stmt was always nil — db.Prepare() returned a *sql.Stmt that was immediately discarded with _, so the if task.Stmt != nil branch in execJob could never execute. Every LoadTasks call also leaked a server-side statement handle per task per DB: the *sql.Stmt from db.Prepare() was never closed. Additionally, err == context.Canceled used value equality so wrapped errors from DB drivers would silently fall through to the generic error path instead of returning "job was canceled". A failed QueryContext on the Stmt path was also reported as "query preparation failed" — misleading, since preparation had already succeeded at load time. After: Task.Stmt field removed from the struct; the dead if task.Stmt != nil branch removed from execJob; the dead stmt variable removed from loadTasks; database/sql import removed from tasks.go. The *sql.Stmt returned by db.Prepare() is now closed immediately after validation, releasing the server-side handle. err == context.Canceled replaced with errors.Is(err, context.Canceled) to correctly match wrapped errors. Why: a prepared statement is connection-specific and cannot be shared across multiple source DBs. Storing it was always wrong for multi-DB tasks. The raw query path handles all execution correctly. The resource leak grew with every server restart proportional to tasks × DBs.
Before: GetRandom() on an empty DBs map returns ("", nil). The nil db
was passed straight to db.QueryContext, causing a nil pointer dereference
panic. This path is reached whenever dbName is empty (the common case)
and the task's source DB pool is empty — e.g. if FilterByTags produces
no results or srcDBs is empty at startup. Additionally, prepared.Close()
silently discarded its error; a Close failure on a network hiccup leaves
the server-side handle open and the connection unreturned to the pool,
which can exhaust the pool before any real query runs.
After: the else branch now checks whether db is nil after GetRandom() and
returns a descriptive error instead of panicking. prepared.Close() error
is now logged via co.lo.Error so failures are visible without aborting
the load.
Why: an empty DB pool is a misconfiguration that should produce a clear
error at query time, not a crash. Close errors during validation are
non-fatal but should be observable.
knadh
force-pushed
the
fix/execjob-prepared-stmt
branch
from
October 27, 2026 06:45
8048856 to
55ed711
Compare
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
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.
Fixes #77.
Before
Dead code —
Task.Stmtwas alwaysnil:db.Prepare()returned a*sql.Stmtthat was immediately discarded with_, sotask.Stmtwas alwaysnilat runtime. Theif task.Stmt != nilbranch inexecJobcould never execute.Resource leak: Every
LoadTaskscall opened a server-side prepared statement handle per task per DB and never closed it. The*sql.Stmtreturned bydb.Prepare()was never passed to.Close().err == context.Canceledvalue equality: Wrapped errors from DB drivers would not match, silently falling through to the generic error path instead of returning "the job was canceled".After
Task.Stmtfield removed from the structif task.Stmt != nilbranch removed fromexecJobstmtvariable removed fromloadTasksdatabase/sqlimport removed fromtasks.godb.Prepare()result is now closed immediately after validation, releasing the server-side handleerr == context.Canceledreplaced witherrors.Is(err, context.Canceled)Why
A prepared statement is connection-specific and cannot be shared across multiple source DBs — storing it was always wrong for multi-DB tasks. The raw query path handles all execution correctly. The resource leak grew with every server restart, proportional to tasks × source DBs.