Skip to content

Report Extended Code#6587

Merged
zachlefevre merged 1 commit into
mainfrom
zlefevre/report_extended_code
Apr 22, 2026
Merged

Report Extended Code#6587
zachlefevre merged 1 commit into
mainfrom
zlefevre/report_extended_code

Conversation

@zachlefevre

Copy link
Copy Markdown
Member

No description provided.

@zachlefevre zachlefevre requested review from a team as code owners April 16, 2026 01:05

@ask-bonk ask-bonk Bot left a comment

Copy link
Copy Markdown
Contributor

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 extended SQLite error code reporting to SqliteObserver::reportQueryEvent and QueryEvent.

Issues found (ranked by severity):

  1. [HIGH] setQueryExtendedCode assigns to the wrong member — writes queryResult instead of extendedErrorCode.
  2. [HIGH] The existing test in sqlite-test.c++ overrides reportQueryEvent with the old signature (missing the new extendedErrorCode parameter). Since the override uses override, this will be a compile error on the new base class signature. The test needs to be updated.

This review was written by an AI assistant and may not be perfect. Please verify all suggestions before applying.

Comment thread src/workerd/util/sqlite.h Outdated
Comment thread src/workerd/util/sqlite.h
@ask-bonk

ask-bonk Bot commented Apr 16, 2026

Copy link
Copy Markdown
Contributor

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

This PR adds extended SQLite error code reporting to SqliteObserver::reportQueryEvent and QueryEvent.

Issues found (ranked by severity):

  1. [HIGH] setQueryExtendedCode at src/workerd/util/sqlite.h:622 assigns to queryResult instead of extendedErrorCode — a copy-paste bug that silently drops the extended code and clobbers the query result.
  2. [HIGH] The existing test override of reportQueryEvent in src/workerd/util/sqlite-test.c++:923 still has the old signature (missing the new extendedErrorCode parameter). Since it uses override, this will be a compile error.

Both comments with a suggested fix have been posted to the PR.

github run

@codspeed-hq

codspeed-hq Bot commented Apr 16, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 72 untouched benchmarks
⏩ 129 skipped benchmarks1


Comparing zlefevre/report_extended_code (c793d88) with main (5fdf8aa)

Open in CodSpeed

Footnotes

  1. 129 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

@zachlefevre zachlefevre force-pushed the zlefevre/report_extended_code branch 2 times, most recently from a7312d1 to cbe2e9f Compare April 16, 2026 14:27

@ask-bonk ask-bonk Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Correction to my earlier review: both issues I flagged were false positives — apologies for the noise.

  1. setQueryExtendedCode correctly assigns to extendedErrorCode, not queryResult. I misread the diff.
  2. The test override in sqlite-test.c++ already includes the new int extendedErrorCode parameter.

The changes look correct. LGTM.

@ask-bonk

ask-bonk Bot commented Apr 16, 2026

Copy link
Copy Markdown
Contributor

LGTM

github run

@zachlefevre zachlefevre force-pushed the zlefevre/report_extended_code branch from cbe2e9f to ccf5da4 Compare April 16, 2026 21:12
@zachlefevre zachlefevre requested a review from justin-mp April 16, 2026 21:13
Comment thread src/workerd/util/sqlite.c++ Outdated
@zachlefevre zachlefevre force-pushed the zlefevre/report_extended_code branch from ccf5da4 to c7187aa Compare April 20, 2026 20:50
@zachlefevre zachlefevre force-pushed the zlefevre/report_extended_code branch 2 times, most recently from abb9acc to 1d13091 Compare April 22, 2026 15:41
@zachlefevre zachlefevre force-pushed the zlefevre/report_extended_code branch from 1d13091 to c793d88 Compare April 22, 2026 16:10
@zachlefevre zachlefevre merged commit 1c2520e into main Apr 22, 2026
22 checks passed
@zachlefevre zachlefevre deleted the zlefevre/report_extended_code branch April 22, 2026 16:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants