getTableCount errors are discarded, so a failed query reports as a count of zero #96

New Issue

2026-08-09T10:23:50+02:00

clawbot commented

2026-08-09 10:23:50 +02:00

Surfaced during the #71 branch triage. Small, but it is the same failure
shape this repo has spent the day removing from its gate: a value that is
wrong reports as a plausible number rather than as an error.

The code

internal/vaultik/snapshot.go, seven call sites, every one discarding the
error:

snapshotCountBefore, _ := v.getTableCount("snapshots")   // :1544
fileCountBefore, _     := v.getTableCount("files")       // :1559
chunkCountBefore, _    := v.getTableCount("chunks")      // :1560
blobCountBefore, _     := v.getTableCount("blobs")       // :1561
fileCountAfter, _      := v.getTableCount("files")       // :1570
chunkCountAfter, _     := v.getTableCount("chunks")      // :1571
blobCountAfter, _      := v.getTableCount("blobs")       // :1572

getTableCount returns (int64, error). On error the count is the zero
value, and these are before/after pairs used to report what an operation
changed. So a failed query does not surface as a failure — it silently
becomes 0, and the delta computed from it is wrong in a direction
that looks like real work happened (or did not).

Why it is worth fixing rather than shrugging at

The #71 triage found this while establishing that
fix/sql-injection-whitelist would have broken main: that branch's
allow-list omits the snapshots table, so getTableCount("snapshots")
would have started failing — and because the error is discarded, the
count would have silently become 0 with no error anywhere. A latent
error-swallow turned a would-be loud failure into a silent wrong number.
That is precisely the class of defect that made a "security fix" look
harmless.

Exposure today is nil in the sense that every call site passes a literal
that the sanitizer accepts. The defect is that the code cannot tell you
when that stops being true.

Definition of done

The seven call sites handle the error rather than discarding it. These
are diagnostic counts, so aborting the operation is probably wrong —
the right shape is likely to surface the failure (log at warn, or
render the count as unknown rather than 0) so a broken query is
visible. Decide deliberately and state the choice; do not simply
propagate the error upward if that changes when the command fails.
A count that could not be read is never rendered as 0. Whatever
representation is chosen — unknown, -, omitted — a reader must be
able to distinguish "the table is empty" from "we could not ask".
--json output makes the same distinction, per the precedent set in
#64, where remote_present is null rather than false when the
answer is unknown.
A test covers the error path: force getTableCount to fail and assert
the output does not claim zero.
Check whether other , _ := discards in internal/vaultik have the
same shape — a discarded error feeding a user-visible number. Fix them
consistently or record why they are fine.
script/cibuild exits 0, verified per #93 (expected ok count, zero
(cached) markers, plausible wall time).

Surfaced during the #71 branch triage. Small, but it is the same failure shape this repo has spent the day removing from its gate: a value that is wrong reports as a plausible number rather than as an error. ## The code `internal/vaultik/snapshot.go`, seven call sites, every one discarding the error: ```go snapshotCountBefore, _ := v.getTableCount("snapshots") // :1544 fileCountBefore, _ := v.getTableCount("files") // :1559 chunkCountBefore, _ := v.getTableCount("chunks") // :1560 blobCountBefore, _ := v.getTableCount("blobs") // :1561 fileCountAfter, _ := v.getTableCount("files") // :1570 chunkCountAfter, _ := v.getTableCount("chunks") // :1571 blobCountAfter, _ := v.getTableCount("blobs") // :1572 ``` `getTableCount` returns `(int64, error)`. On error the count is the zero value, and these are before/after pairs used to report what an operation changed. So a failed query does not surface as a failure — it silently becomes `0`, and the *delta* computed from it is wrong in a direction that looks like real work happened (or did not). ## Why it is worth fixing rather than shrugging at The #71 triage found this while establishing that `fix/sql-injection-whitelist` would have **broken `main`**: that branch's allow-list omits the `snapshots` table, so `getTableCount("snapshots")` would have started failing — and because the error is discarded, the count would have silently become 0 with no error anywhere. A latent error-swallow turned a would-be loud failure into a silent wrong number. That is precisely the class of defect that made a "security fix" look harmless. Exposure today is nil in the sense that every call site passes a literal that the sanitizer accepts. The defect is that the code cannot tell you when that stops being true. ## Definition of done 1. The seven call sites handle the error rather than discarding it. These are diagnostic counts, so aborting the operation is probably wrong — the right shape is likely to surface the failure (log at warn, or render the count as unknown rather than `0`) so a broken query is visible. **Decide deliberately and state the choice**; do not simply propagate the error upward if that changes when the command fails. 2. A count that could not be read is **never rendered as `0`**. Whatever representation is chosen — `unknown`, `-`, omitted — a reader must be able to distinguish "the table is empty" from "we could not ask". 3. `--json` output makes the same distinction, per the precedent set in #64, where `remote_present` is `null` rather than `false` when the answer is unknown. 4. A test covers the error path: force `getTableCount` to fail and assert the output does not claim zero. 5. Check whether other `, _ :=` discards in `internal/vaultik` have the same shape — a discarded error feeding a user-visible number. Fix them consistently or record why they are fine. 6. `script/cibuild` exits 0, verified per #93 (expected `ok` count, zero `(cached)` markers, plausible wall time).

clawbot added this to the 1.0.0 milestone 2026-08-09 10:23:50 +02:00

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: sneak/vaultik#96