Stop silently truncated SQL scans from passing as complete results
sql.Rows.Close reports the driver's close error, not the iteration error
(rs.lasterr). A scan that fails part-way through therefore returns a short
result set with a nil error — a wrong answer rather than a failure. errcheck
cannot see this: Close *is* checked at these sites; only Err is missing.
The streamed-apply fact spool was one such site. pageClass built a page,
closed the rows and returned; a mid-iteration failure produced an empty page,
which the driver loop reads as "class exhausted" and skips the rest of that
phase's facts. Its two sibling functions added alongside it both check Err.
Enabling rowserrcheck found twelve more of the same shape, and every one was
a silent truncation:
- Stats / RepoStats returned short kind and language histograms
- RepoPrefixes returned a short prefix list, which feeds repo scoping and
orphan purging
- the analysis generation seal read dropped components from `sealed`, where
the validation loop reads a missing component as a seal defect rather
than as the read failure it is
- the deferred LSP spool page ended early, abandoning the rest of the work
Sites that are best-effort by construction — vector refinement, error-message
decoration, orphan candidate collection — now say so at the call site with an
explicit ignore and the reason it is safe, instead of dropping the error by
omission. Orphan collection is the one destructive consumer: a truncated scan
there can only under-report, never promote a tracked repo into the purge list.
Test-side reads get the same treatment; a truncated EXPLAIN QUERY PLAN or
table_info scan makes the assertion built on it vacuous.
The linter is the regression guard. Reproducing an iteration failure mid-scan
requires driver-level fault injection that the store has no seam for, so a
test here would pin the fix without pinning the class.
This commit is contained in:
@@ -7,6 +7,16 @@ linters:
|
||||
# errcheck, govet, ineffassign, staticcheck, unused.
|
||||
default: standard
|
||||
|
||||
enable:
|
||||
# sql.Rows.Close reports the driver's close error, not the iteration
|
||||
# error (rs.lasterr), so a scan that fails part-way through looks like a
|
||||
# scan that ended. The result is an empty or short result set returned
|
||||
# with a nil error — a wrong answer, not a failure. errcheck cannot see
|
||||
# this because Close IS checked at these sites; only the Err call is
|
||||
# missing. Every such site found when this was enabled was a silent
|
||||
# truncation, so the class is worth a linter rather than review vigilance.
|
||||
- rowserrcheck
|
||||
|
||||
settings:
|
||||
errcheck:
|
||||
# Unchecked errors on the fmt print family are noise, not bugs: these
|
||||
|
||||
@@ -67,6 +67,10 @@ func (s *Store) PruneAnalysisGenerations(ctx context.Context, keep, batch int) e
|
||||
}
|
||||
candidates = append(candidates, generationID)
|
||||
}
|
||||
if err := rows.Err(); err != nil {
|
||||
rows.Close()
|
||||
return err
|
||||
}
|
||||
if err := rows.Close(); err != nil {
|
||||
return err
|
||||
}
|
||||
|
||||
@@ -276,6 +276,10 @@ func (s *Store) ListAnalysisCommunitySummaries(generationID int64, limit int, cu
|
||||
}
|
||||
items = append(items, item)
|
||||
}
|
||||
if err := rows.Err(); err != nil {
|
||||
rows.Close()
|
||||
return nil, "", err
|
||||
}
|
||||
if err := rows.Close(); err != nil {
|
||||
return nil, "", err
|
||||
}
|
||||
@@ -388,6 +392,10 @@ func (s *Store) ListAnalysisProcessSummaries(generationID int64, limit int, curs
|
||||
item.Truncated = truncated != 0
|
||||
items = append(items, item)
|
||||
}
|
||||
if err := rows.Err(); err != nil {
|
||||
rows.Close()
|
||||
return nil, "", err
|
||||
}
|
||||
if err := rows.Close(); err != nil {
|
||||
return nil, "", err
|
||||
}
|
||||
@@ -552,6 +560,10 @@ func (s *Store) analysisConceptsLocked(generationID int64, tokens []string, dire
|
||||
}
|
||||
result.Concepts[positions[token]].InVocabulary = vocabulary != 0
|
||||
}
|
||||
if err := rows.Err(); err != nil {
|
||||
rows.Close()
|
||||
return graph.AnalysisConceptQueryResult{}, err
|
||||
}
|
||||
if err := rows.Close(); err != nil {
|
||||
return graph.AnalysisConceptQueryResult{}, err
|
||||
}
|
||||
@@ -602,6 +614,10 @@ func (s *Store) ListAnalysisConcepts(generationID int64, limit int, cursorToken
|
||||
}
|
||||
tokens = append(tokens, token)
|
||||
}
|
||||
if err := rows.Err(); err != nil {
|
||||
rows.Close()
|
||||
return graph.AnalysisConceptQueryResult{}, "", err
|
||||
}
|
||||
if err := rows.Close(); err != nil {
|
||||
return graph.AnalysisConceptQueryResult{}, "", err
|
||||
}
|
||||
|
||||
@@ -624,6 +624,10 @@ func TestAnalysisGenerationQueryPlansUseBoundedIndexes(t *testing.T) {
|
||||
}
|
||||
details = append(details, detail)
|
||||
}
|
||||
if err := rows.Err(); err != nil {
|
||||
rows.Close()
|
||||
t.Fatal(err)
|
||||
}
|
||||
rows.Close()
|
||||
plan := strings.Join(details, " | ")
|
||||
if !strings.Contains(plan, index) {
|
||||
|
||||
@@ -681,6 +681,13 @@ func validateAnalysisGenerationTx(tx *sql.Tx, generationID int64) (graph.Analysi
|
||||
}
|
||||
sealed[component] = count
|
||||
}
|
||||
// A truncated read here would drop components from `sealed`, and the
|
||||
// loop below reads a missing component as a seal defect rather than as
|
||||
// the read failure it is.
|
||||
if err := rows.Err(); err != nil {
|
||||
rows.Close()
|
||||
return graph.AnalysisGenerationHeader{}, err
|
||||
}
|
||||
if err := rows.Close(); err != nil {
|
||||
return graph.AnalysisGenerationHeader{}, err
|
||||
}
|
||||
|
||||
@@ -210,6 +210,9 @@ ORDER BY id`, string(graph.EdgeCalls))
|
||||
}
|
||||
details = append(details, detail)
|
||||
}
|
||||
if err := rows.Err(); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
plan := strings.Join(details, "\n")
|
||||
if !strings.Contains(plan, "edges_by_kind") {
|
||||
t.Fatalf("plan does not use edges_by_kind:\n%s", plan)
|
||||
|
||||
@@ -50,6 +50,10 @@ ORDER BY file_path, line LIMIT 120000`)
|
||||
site.Kind = graph.EdgeKind(kind)
|
||||
fileOrdered = append(fileOrdered, site)
|
||||
}
|
||||
if err := rows.Err(); err != nil {
|
||||
_ = rows.Close()
|
||||
b.Fatalf("harvest probe sites: %v", err)
|
||||
}
|
||||
if err := rows.Close(); err != nil {
|
||||
b.Fatalf("close harvest: %v", err)
|
||||
}
|
||||
|
||||
@@ -281,6 +281,9 @@ func explainPlanTolerant(t *testing.T, s *Store, query string) []string {
|
||||
}
|
||||
plan = append(plan, detail)
|
||||
}
|
||||
if err := rows.Err(); err != nil {
|
||||
t.Fatalf("iterate plan rows: %v", err)
|
||||
}
|
||||
return plan
|
||||
}
|
||||
|
||||
@@ -311,6 +314,9 @@ func TestSweepPlanLockReceiverRebindBatch(t *testing.T) {
|
||||
}
|
||||
plan = append(plan, detail)
|
||||
}
|
||||
if err := rows.Err(); err != nil {
|
||||
t.Fatalf("iterate plan rows: %v", err)
|
||||
}
|
||||
joined := strings.Join(plan, "\n")
|
||||
if !strings.Contains(joined, "SCAN f") && !strings.Contains(joined, "go_receiver_rebind_files") {
|
||||
t.Fatalf("plan must drive from the temp file table:\n%s", joined)
|
||||
|
||||
@@ -237,6 +237,7 @@ func TestLongWALReaderDoesNotBlockReindexWriter(t *testing.T) {
|
||||
rows, err := readTx.QueryContext(ctx, `SELECT from_id FROM edges`)
|
||||
require.NoError(t, err)
|
||||
require.True(t, rows.Next())
|
||||
require.NoError(t, rows.Err())
|
||||
held = append(held, heldReader{conn: conn, tx: readTx, rows: rows})
|
||||
}
|
||||
|
||||
@@ -663,6 +664,7 @@ func TestEvictRepoLargeScopeUsesIndexedSetCandidates(t *testing.T) {
|
||||
plan.WriteString(detail)
|
||||
plan.WriteByte('\n')
|
||||
}
|
||||
require.NoError(t, planRows.Err())
|
||||
require.NoError(t, planRows.Close())
|
||||
require.Contains(t, plan.String(), "nodes_by_repo")
|
||||
|
||||
|
||||
@@ -2079,6 +2079,14 @@ func (s *Store) Stats() graph.GraphStats {
|
||||
}
|
||||
st.ByKind[kind] = n
|
||||
}
|
||||
// Same treatment as a Scan failure above: Close reports the driver's
|
||||
// close error, so without Err a scan that died mid-flight returns a
|
||||
// short histogram indistinguishable from a real one.
|
||||
if err := rows.Err(); err != nil {
|
||||
_ = rows.Close()
|
||||
panicOnFatal(err)
|
||||
return st
|
||||
}
|
||||
_ = rows.Close()
|
||||
|
||||
rows, err = s.stmtStatsByLanguage.Query()
|
||||
@@ -2096,6 +2104,11 @@ func (s *Store) Stats() graph.GraphStats {
|
||||
}
|
||||
st.ByLanguage[lang] = n
|
||||
}
|
||||
if err := rows.Err(); err != nil {
|
||||
_ = rows.Close()
|
||||
panicOnFatal(err)
|
||||
return st
|
||||
}
|
||||
_ = rows.Close()
|
||||
return st
|
||||
}
|
||||
@@ -2124,6 +2137,11 @@ func (s *Store) RepoStats() map[string]graph.GraphStats {
|
||||
st.ByLanguage[lang] += n
|
||||
out[repo] = st
|
||||
}
|
||||
if err := rows.Err(); err != nil {
|
||||
_ = rows.Close()
|
||||
panicOnFatal(err)
|
||||
return out
|
||||
}
|
||||
_ = rows.Close()
|
||||
|
||||
rows, err = s.stmtRepoStatsEdges.Query()
|
||||
@@ -2146,6 +2164,11 @@ func (s *Store) RepoStats() map[string]graph.GraphStats {
|
||||
st.TotalEdges = n
|
||||
out[repo] = st
|
||||
}
|
||||
if err := rows.Err(); err != nil {
|
||||
_ = rows.Close()
|
||||
panicOnFatal(err)
|
||||
return out
|
||||
}
|
||||
_ = rows.Close()
|
||||
return out
|
||||
}
|
||||
@@ -2166,6 +2189,9 @@ func (s *Store) RepoPrefixes() []string {
|
||||
}
|
||||
out = append(out, p)
|
||||
}
|
||||
// A short prefix list feeds repo scoping and orphan purging, so a
|
||||
// truncated scan must not pass for the whole set.
|
||||
panicOnFatal(rows.Err())
|
||||
return out
|
||||
}
|
||||
|
||||
|
||||
@@ -136,6 +136,12 @@ WHERE e.kind = ? AND n.kind = ? AND n.meta IS NOT NULL`
|
||||
IfaceMeta: meta,
|
||||
})
|
||||
}
|
||||
// Individual undecodable rows are skipped above by design; a failed
|
||||
// iteration is different in kind — it truncates the projection — so it
|
||||
// fails the whole call the way a failed Query does.
|
||||
if rows.Err() != nil {
|
||||
return nil
|
||||
}
|
||||
return out
|
||||
}
|
||||
|
||||
@@ -182,6 +188,9 @@ ORDER BY e.id`
|
||||
RepoPrefix: repoPrefix,
|
||||
})
|
||||
}
|
||||
if rows.Err() != nil {
|
||||
return nil
|
||||
}
|
||||
if len(out) == 0 {
|
||||
// Match the in-memory reference: empty graph returns nil.
|
||||
return nil
|
||||
@@ -228,6 +237,9 @@ ORDER BY e.id`
|
||||
Origin: origin,
|
||||
})
|
||||
}
|
||||
if rows.Err() != nil {
|
||||
return nil
|
||||
}
|
||||
return out
|
||||
}
|
||||
|
||||
@@ -558,6 +570,12 @@ func (s *Store) ThrowerErrorSurface(pathPrefix string) []graph.ThrowerErrorRow {
|
||||
acc.row.ErrorTargets = append(acc.row.ErrorTargets, to)
|
||||
}
|
||||
}
|
||||
// Pass 1 seeds every accumulator; a truncated read here silently drops
|
||||
// throwers from the result rather than reporting a failure.
|
||||
if trows.Err() != nil {
|
||||
_ = trows.Close()
|
||||
return nil
|
||||
}
|
||||
_ = trows.Close()
|
||||
if len(accums) == 0 {
|
||||
return nil
|
||||
@@ -598,6 +616,10 @@ ORDER BY e.id`
|
||||
acc.msgSeen[name] = struct{}{}
|
||||
acc.row.ErrorMsgs = append(acc.row.ErrorMsgs, name)
|
||||
}
|
||||
// Pass 2 only decorates rows pass 1 already produced, so a failed
|
||||
// read costs this thrower its message list and nothing else —
|
||||
// the same outcome as the Query error handled above.
|
||||
_ = mrows.Err()
|
||||
_ = mrows.Close()
|
||||
}
|
||||
|
||||
|
||||
@@ -202,6 +202,11 @@ func (s *Store) OrphanRepoPrefixes(known []string) []string {
|
||||
seen[p] = struct{}{}
|
||||
out = append(out, p)
|
||||
}
|
||||
// The result drives PurgeRepo, so a truncated scan can only
|
||||
// under-report orphans — it can never promote a tracked repo into
|
||||
// the purge list. Losing a candidate defers the purge to the next
|
||||
// warmup, which is the safe direction for a destructive caller.
|
||||
_ = rows.Err()
|
||||
_ = rows.Close()
|
||||
}
|
||||
return out
|
||||
|
||||
@@ -186,6 +186,9 @@ func (s *Store) GetEmbeddings(ids []string) map[string][]float32 {
|
||||
out[id] = vec
|
||||
}
|
||||
}
|
||||
// Best-effort by construction (see the Query error above): a short
|
||||
// batch costs recall in the refinement stage, never correctness.
|
||||
_ = rows.Err()
|
||||
_ = rows.Close()
|
||||
}
|
||||
|
||||
|
||||
@@ -52,6 +52,11 @@ func hasColumn(t *testing.T, db *sql.DB, table, col string) bool {
|
||||
return true
|
||||
}
|
||||
}
|
||||
// A truncated table_info scan reads as "column absent", which would let
|
||||
// a migration assertion pass without the migration having run.
|
||||
if err := rows.Err(); err != nil {
|
||||
t.Fatalf("iterate table_info(%s): %v", table, err)
|
||||
}
|
||||
return false
|
||||
}
|
||||
|
||||
|
||||
@@ -158,6 +158,12 @@ func (s *deferredLSPSpool) hasForScope(scope map[string]struct{}) bool {
|
||||
return true
|
||||
}
|
||||
}
|
||||
// A truncated scan reports "nothing in scope", matching the Query error
|
||||
// above. Failing open instead would keep re-arming the retry pass against
|
||||
// a spool that cannot be read; the caller also consults the in-memory
|
||||
// lspDeferredRetry list, so a read failure delays work rather than
|
||||
// dropping it.
|
||||
_ = rows.Err()
|
||||
return false
|
||||
}
|
||||
|
||||
@@ -214,6 +220,12 @@ ORDER BY file_path,line,from_id,kind,target LIMIT ?`, args...)
|
||||
record.payload.crossRepo = crossRepo != 0
|
||||
records = append(records, record)
|
||||
}
|
||||
// A short page would end the iteration early and look like a drained
|
||||
// spool, silently abandoning the remaining deferred work.
|
||||
if err := rows.Err(); err != nil {
|
||||
_ = rows.Close()
|
||||
return nil, false, err
|
||||
}
|
||||
if err := rows.Close(); err != nil {
|
||||
return nil, false, err
|
||||
}
|
||||
|
||||
@@ -367,6 +367,16 @@ WHERE class = ? AND file_path > ? ORDER BY file_path LIMIT ?`, int(class), after
|
||||
stats.Files++
|
||||
stats.Bytes += len(payload)
|
||||
}
|
||||
// Rows.Close reports the driver's close error, not the iteration error
|
||||
// (rs.lasterr), so a failure part-way through the scan is invisible
|
||||
// without Err. The empty page that produced would read as "class
|
||||
// exhausted" to the driver loop in provider_stream.go, silently dropping
|
||||
// the rest of this phase's facts. The break above is not an error, so
|
||||
// Err stays nil on the byte-budget path.
|
||||
if err := rows.Err(); err != nil {
|
||||
_ = rows.Close()
|
||||
return nil, last, stats, err
|
||||
}
|
||||
if err := rows.Close(); err != nil {
|
||||
return nil, last, stats, err
|
||||
}
|
||||
@@ -492,6 +502,10 @@ WHERE type_id IN (`+values+`) ORDER BY type_id,seq`, args...)
|
||||
}
|
||||
out = append(out, record)
|
||||
}
|
||||
if err := rows.Err(); err != nil {
|
||||
_ = rows.Close()
|
||||
return nil, err
|
||||
}
|
||||
if err := rows.Close(); err != nil {
|
||||
return nil, err
|
||||
}
|
||||
|
||||
@@ -78,6 +78,10 @@ func legacyFourPhaseWalk(b *testing.B, spool *factSpool) int {
|
||||
b.Fatal(err)
|
||||
}
|
||||
}
|
||||
if err := rows.Err(); err != nil {
|
||||
_ = rows.Close()
|
||||
b.Fatal(err)
|
||||
}
|
||||
_ = rows.Close()
|
||||
total += len(stub.calls) + len(stub.supers) + len(stub.metas) + len(stub.aliases)
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user