fix(querysql): clarify database connection EOFs - #78
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4e71dd4fcd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| Rows: rows, | ||
| started: false, | ||
| Err: err, // important to return the error unadorned here, as some code e.g. casts it directly to mssql.Error | ||
| Err: err, // callers should inspect SQL errors with errors.As or errors.Is, never with direct type assertions |
There was a problem hiding this comment.
I confirmed in vaam and Postprocessing that no callers actually cast errors (which was probably the pre go 1.13 introduction in 2019 pattern of direct casting as errors.Is/As didn't exist yet). So this change won't have any impact on callers. Plus anyway, the actual case we're wrapping here is io.EOF which any caller casting to errors.Is wouldn't match as it's just a sentinel. And I found no current callers doing direct comparison err == io.EOF (which would be a bad practice anyway) on the query paths:
- Postprocessing: 5 direct comparisons, all traced to their producer:
- 4 × csv.Reader.Read()
- 1 × xml.Decoder.Token()
- VAAM: 95 direct comparisons across 48 files:
- 49 generated HTTP-response body decoders
- 6 other generated HTTP-response body decoders
- 28 generated request-body decoders
- 2 generated JSON model decoders
- 10 CSV, HTML, multipart, buffered-stream, or JSON-token readers
So none compare an error returned by go-querysql. So this is really a tiny change without expected consequences other than better error logging.
|
|
||
| - name: Start db | ||
| run: docker compose -f docker-compose.test.yml up -d | ||
| run: docker compose -f docker-compose.test.yml up -d --wait --wait-timeout 60 |
There was a problem hiding this comment.
Need to wait a bit or tests can be flaky when they start faster than the db is to get ready as observed here https://github.com/vippsas/go-querysql/actions/runs/30450189869 and also previous cases back in April https://github.com/vippsas/go-querysql/actions/runs/24980350884
Context
Microsoft's SQL driver can surface a failed database query as only
EOF, because its retryable connection error delegates its message to the underlyingio.EOF. During a production investigation, 2,350 of the 2,383 EOF errors reaching the dataflow run boundary over 30 days came from one SQL query path, but the error text did not identify the database connection as the source.querysqlhas historically left SQL errors unadorned. Callers should nevertheless inspect errors witherrors.Asorerrors.Is, never direct type assertions, because selected errors may gain useful context while preserving their error chain.Changes
QueryContextas unexpected database connection closureerrors.Iserrors.Asanderrors.Isas the supported inspection contract