Read queue depths with Find, not Scan (closes #234) #237
Reference in New Issue
Block a user
Delete Branch "issue-234-queue-depth-no-gorm-scan"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Closes #234.
nextfailsmake checkon its own. Reproduced on a clean checkout oforigin/nextataba02bcwith nothing applied:The guard is right and it stays as it is.
(*gorm.DB).ScanswapsGORM's own
traceRecorderin for the configured logger for theduration of the statement, and that recorder does not implement
gorm.ParamsFilter, so the statement reaches the log with its boundvalues interpolated. Both call sites move to
Find.The SQL does not change
callbacks.Query(Find) andcallbacks.RowQuery(Scan, viaRows)both start with the same
BuildQuerySQL, and both call sites setModelandSelectexplicitly, so the table, the column list, theWHERE, theGROUP BYand the soft-delete clause are built by thesame code either way. Result scanning is also the same path:
Scanreaches
gorm.ScanthroughScanRows,Findthrough the querycallback, and both re-parse the anonymous destination struct's schema
because its type differs from the model's.
Captured from the adapter, same test, before and after:
Identical apart from the placeholders, which is the whole point. Both
reported
rows=2on the same fixture.Metrics
Unchanged, and the existing assertions still cover the properties:
TestDeliveryMetrics_QueueDepthGauges— per-type counts, and adrained bucket reading 0 rather than holding its last value. PASS.
TestDeliveryMetrics_QueueDepthDeletedTarget— a backlog behinda deleted target still lands in
unknown. PASS. This is the case therewrite could have broken, since it depends on rows whose
target_idresolves to no target row; the query shape is untouched, so it does
not.
normalizeTargetTypewith summing, the full labeldomain written on every sample, and the
unknownseries materialisedat registration all live in
internal/metricsand are not touchedhere.
New test
TestQueueDepthSample_LogsNoBoundValue(internal/delivery) drives onesample through the real adapter and asserts the aggregate keeps its
placeholders and carries no status literal. Negative control: restoring
either
Scanfails it withso it is discriminating rather than vacuous, and it catches at runtime
what
scan_guard_test.gocatches statically.To capture that SQL the test needs the per-webhook databases to log
through a buffer, so
database.NewTestWebhookDBManagerWithLoggertakesthe logger;
NewTestWebhookDBManagerkeeps its signature and delegates.Gate
Host load average 29.69 / 40.07 / 51.49 on 48 cores at the start of the
build.
make checkon the branch: green, exit 0, 17 packages ok.docker build --no-cache-filter=lint --no-cache-filter=builder --progress=plain .exit 0:
Real durations on both cache-defeated stages, golangci-lint's own
summary line present, 17
oklines under#32and zero of them(cached). Inside the container:Neither #225 nor
#230 fired on any run here;
internal/handlersfinished in 20.3s in the container. The failurefixed by this branch was deterministic, not either of those flakes.
Rebased onto
origin/nextataba02bcimmediately before pushing. Thegate image was removed and no containers were left behind.
(*gorm.DB).Scan swaps GORM's own traceRecorder in for the configured logger for the duration of the statement, and that recorder does not implement gorm.ParamsFilter. The statement therefore reaches the log with its bound values interpolated, which is the one path (*gormlog.Logger).ParamsFilter cannot reach. internal/gormlog's scan_guard_test.go exists to keep that path out of production code; the queue-depth sampler landed with two calls on it, so next has been failing make check on its own. Both call sites now use Find, which goes through the normal query callback. The emitted SQL is otherwise unchanged -- callbacks.Query and callbacks.RowQuery share BuildQuerySQL, and both call sites set Model and Select explicitly, so the table, the column list and the soft-delete clause are built identically. Only the log line differs: Scan: ... WHERE status IN ("pending","retrying") AND ... Find: ... WHERE status IN (?,?) AND ... TestQueueDepthSample_LogsNoBoundValue drives one sample through the adapter and asserts the aggregate keeps its placeholders and carries no status literal. Restoring either Scan fails it as well as the static guard. database.NewTestWebhookDBManagerWithLogger lets that test capture the SQL the per-webhook databases emit; NewTestWebhookDBManager keeps its signature and delegates to it.PASS — independently verified at
354b271: metrics unchanged (negative control reproduced, guard untouched and green), merges cleanly intonext, gate fully green with everylint/builderstep cache-defeated.Disclosure, not attributed to this PR and not blocking it: my gate measured
make testat 72.1s, over the 60s hard cap inREPO_POLICIES.md. It is pre-existing onnextand load-dependent (host load average 50-57 on 48 cores during the run); this branch adds ~1.7s. Worth a separate improvement issue per the policy's own instruction, not a rework of #237.