Skip to content

Include extended error codes in error messages#6321

Merged
zachlefevre merged 1 commit intomainfrom
zlefevre/include_extended_error_codes_in_err_msgs
Mar 19, 2026
Merged

Include extended error codes in error messages#6321
zachlefevre merged 1 commit intomainfrom
zlefevre/include_extended_error_codes_in_err_msgs

Conversation

@zachlefevre
Copy link
Copy Markdown
Member

@zachlefevre zachlefevre commented Mar 13, 2026 •
edited
Loading

SQLITE error logs should include that error's extended error code.

@zachlefevre zachlefevre requested review from a team as code owners March 13, 2026 18:08
ask-bonk[bot]
ask-bonk bot reviewed Mar 13, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@ask-bonk ask-bonk bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm Bonk, and I've done a quick review of your PR.

This PR adds the SQLite extended error code to error messages in dbErrorMessage(). A few issues:

  1. [MEDIUM] Extended error code is appended as a raw integer. sqlite3_extended_errcode() returns an int — appending it directly produces something like SQLITE_CONSTRAINT- 19 (or SQLITE_CONSTRAINT- 2067 for SQLITE_CONSTRAINT_UNIQUE). The raw number isn't useful for debugging. Consider using sqlite3_errstr() which accepts extended error codes and returns a human-readable string (e.g., "UNIQUE constraint failed").

  2. [LOW] Missing space before dash. The format string "- " produces SQLITE_ERROR- 1 — there should be a space before the dash for readability.

  3. [LOW] Variable naming. extendedErrorMessage is misleading since sqlite3_extended_errcode() returns an int, not a message string. Something like extendedErrorCode would be more accurate.

These comments were generated by an AI code review assistant and may not be perfect.

@ask-bonk
Copy link
Copy Markdown
Contributor

ask-bonk bot commented Mar 13, 2026

Review posted successfully on PR #6321. Here's a summary of the findings:

  1. [MEDIUM] Raw integer not useful for debugging — sqlite3_extended_errcode() returns an int that gets appended as-is (e.g., 2067). Suggested using sqlite3_errstr() instead, which returns a human-readable description and is already used elsewhere in the file.

  2. [LOW] Missing space before dash — "- " should be " - " for readability.

  3. [LOW] Misleading variable name — extendedErrorMessage holds an int, not a message. Suggested extendedErrorCode.

I also included a concrete suggestion that only appends the extended code when it differs from the primary code (to avoid redundant output like SQLITE_ERROR - SQLITE_ERROR).

github run

ask-bonk bot added a commit that referenced this pull request Mar 13, 2026
@ask-bonk @zachlefevre
PR #6321: 3 issues found, review posted.
660d3c2 
Co-authored-by: zachlefevre <zachlefevre@users.noreply.github.com>
@ask-bonk ask-bonk bot requested a review from a team as a code owner March 13, 2026 18:09
zachlefevre added a commit that referenced this pull request Mar 16, 2026
@ask-bonk @zachlefevre
PR #6321: 3 issues found, review posted.
f49f49b 
Co-authored-by: zachlefevre <zachlefevre@users.noreply.github.com>
@zachlefevre zachlefevre force-pushed the zlefevre/include_extended_error_codes_in_err_msgs branch from 660d3c2 to f49f49b Compare March 16, 2026 17:37
@zachlefevre zachlefevre requested a review from jasnell March 16, 2026 17:40
@ask-bonk
Copy link
Copy Markdown
Contributor

ask-bonk bot commented Mar 16, 2026

error: Your local changes to the following files would be overwritten by checkout:
.opencode/package.json
Please commit your changes or stash them before you switch branches.
Aborting

github run

@ask-bonk
Copy link
Copy Markdown
Contributor

ask-bonk bot commented Mar 16, 2026

@zachlefevre Bonk workflow failed. Check the logs for details.

View workflow run · To retry, trigger Bonk again.

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Mar 16, 2026 •
edited
Loading

Codecov Report

❌ Patch coverage is 31.57895% with 78 lines in your changes missing coverage. Please review.
✅ Project coverage is 70.65%. Comparing base (b5dbc80) to head (fa43314).
⚠️ Report is 95 commits behind head on main.

Files with missing lines Patch % Lines
src/workerd/util/sqlite.c++ 17.77% 1 Missing and 73 partials ⚠️
src/workerd/util/sqlite-test.c++ 83.33% 0 Missing and 4 partials ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #6321      +/-   ##
==========================================
- Coverage   70.68%   70.65%   -0.03%     
==========================================
  Files         420      420              
  Lines      113010   113139     +129     
  Branches    18531    18615      +84     
==========================================
+ Hits        79881    79939      +58     
+ Misses      22086    22077       -9     
- Partials    11043    11123      +80

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

justin-mp
justin-mp requested changes Mar 17, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@justin-mp justin-mp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The branch includes weird changes from main. Please rebase on main rather than merging.

@harrishancock harrishancock mentioned this pull request Mar 19, 2026
Merged
@MellowYarker
Copy link
Copy Markdown
Contributor

MellowYarker commented Mar 19, 2026

This LGTM, and I'm happy to approve, but I think we need to rebase on main and drop the last 2 commits that update deps first.

@zachlefevre zachlefevre force-pushed the zlefevre/include_extended_error_codes_in_err_msgs branch from 877bb01 to dce6b65 Compare March 19, 2026 14:51
@zachlefevre zachlefevre requested a review from justin-mp March 19, 2026 14:52
justin-mp
justin-mp approved these changes Mar 19, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@justin-mp justin-mp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

non-blocking nit: It would be better if the test and the change were in the same commit.

MellowYarker
MellowYarker reviewed Mar 19, 2026
View reviewed changes
@zachlefevre zachlefevre force-pushed the zlefevre/include_extended_error_codes_in_err_msgs branch from dce6b65 to fa43314 Compare March 19, 2026 15:53
@zachlefevre zachlefevre merged commit c7ecef1 into main Mar 19, 2026
24 of 25 checks passed
@zachlefevre zachlefevre deleted the zlefevre/include_extended_error_codes_in_err_msgs branch March 19, 2026 17:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@MellowYarker MellowYarker MellowYarker left review comments

@ask-bonk ask-bonk[bot] ask-bonk[bot] left review comments

@jasnell jasnell jasnell approved these changes

@justin-mp justin-mp justin-mp approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@zachlefevre @codecov-commenter @MellowYarker @jasnell @justin-mp