Recover from driver panic on GEOGRAPHY/GEOMETRY column types#687
Recover from driver panic on GEOGRAPHY/GEOMETRY column types#687
Conversation
Co-authored-by: dlevy-msft-sql <194277063+dlevy-msft-sql@users.noreply.github.com>
Co-authored-by: dlevy-msft-sql <194277063+dlevy-msft-sql@users.noreply.github.com>
Co-authored-by: dlevy-msft-sql <194277063+dlevy-msft-sql@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds a panic-recovery wrapper around rows.ColumnTypes() to prevent sqlcmd from crashing when the SQL Server driver panics on GEOGRAPHY/GEOMETRY (type 240) metadata retrieval.
Changes:
- Introduces
safeColumnTypes()torecover()from panics inrows.ColumnTypes()and convert them into errors. - Switches
runQueryto callsafeColumnTypes()instead ofrows.ColumnTypes()directly. - Adds a unit test to verify panics are caught and converted to errors.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| pkg/sqlcmd/sqlcmd.go | Adds safeColumnTypes() and uses it in runQuery to prevent crashes from driver panics during column type inspection. |
| pkg/sqlcmd/sqlcmd_test.go | Adds a unit test validating the panic recovery behavior of safeColumnTypes(). |
Comments suppressed due to low confidence (1)
pkg/sqlcmd/sqlcmd.go:521
- If safeColumnTypes returns an error, runQuery still continues with rows.Next()/AddRow() even though BeginResultSet() was not called. This can both emit additional scan errors (Scan called with 0 destinations) and, more importantly, can overwrite the earlier -100 retcode with 0 when col1 == "" at end-of-resultset, masking the failure. Consider short-circuiting result-set processing when ColumnTypes fails (e.g., stop the loop / mark results=false / skip Next+AddRow) so retcode remains -100 and the query terminates cleanly after reporting the error.
if first {
first = false
cols, err = safeColumnTypes(rows)
if err != nil {
retcode = -100
qe = s.handleError(&retcode, err)
s.Format.AddError(err)
} else {
s.Format.BeginResultSet(cols)
}
}
inresult := rows.Next()
for inresult {
col1 := s.Format.AddRow(rows)
inresult = rows.Next()
if !inresult {
if col1 == "" {
retcode = 0
} else if _, cerr := fmt.Sscanf(col1, "%d", &retcode); cerr != nil {
retcode = -102
}
}
}
if retcode != -102 {
if err = rows.Err(); err != nil {
retcode = -100
qe = s.handleError(&retcode, err)
s.Format.AddError(err)
}
}
s.Format.EndResultSet()
Co-authored-by: dlevy-msft-sql <194277063+dlevy-msft-sql@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
pkg/sqlcmd/sqlcmd.go:500
- If safeColumnTypes returns an error (e.g., recovered driver panic), runQuery continues into rows.Next()/Format.AddRow without calling BeginResultSet. That leads to scan errors (columnDetails is empty) and can also overwrite the earlier -100 retcode with 0, incorrectly reporting success. Consider aborting processing of the current result set when column type retrieval fails (e.g., set qe to the error / break out before scanning rows) so the batch reliably fails and doesn’t attempt to format rows without column metadata.
cols, err = safeColumnTypes(rows)
if err != nil {
retcode = -100
qe = s.handleError(&retcode, err)
s.Format.AddError(err)
} else {
s.Format.BeginResultSet(cols)
}
|
@copilot fix the issue from the review |
Co-authored-by: dlevy-msft-sql <194277063+dlevy-msft-sql@users.noreply.github.com>
This was the comment: pkg/sqlcmd/sqlcmd.go:501
|
Co-authored-by: dlevy-msft-sql <194277063+dlevy-msft-sql@users.noreply.github.com>
Fixed in commit 50e47a4. Added |
|
@copilot fix the review items |
Co-authored-by: dlevy-msft-sql <194277063+dlevy-msft-sql@users.noreply.github.com>
|
@copilot fix that review comment |
Co-authored-by: dlevy-msft-sql <194277063+dlevy-msft-sql@users.noreply.github.com>
Summary of Changes
Latest Change:
pkg/sqlcmd/sqlcmd.go: Changed error handling to return the error directly (qe = err) instead of usinghandleErrorsafeColumnTypesfails, setqe = errdirectly instead ofqe = s.handleError(&retcode, err)Problem Addressed:
The
handleErrorfunction only returns non-nil errors formssql.Errortypes that meet certain severity criteria. For ourlocalizer.Errorffrom panic recovery, it returnsnil, causingrunQueryto return(retcode, nil). Callers likegoCommandonly treat the batch as failed whenerr != nil, so this would silently ignore the fatal column type error.Solution:
Return the error directly as
qe = errso callers properly detect the failure and can set appropriate exit codes and stop execution as needed.Verification:
✅ Build succeeds
✅ All unit tests pass
✅ go vet passes
Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.