Skip to content

fix(storage): honest Promise<number | void> type for commit-latency recorder #1853

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
kriszyp merged 1 commit into from
Jul 23, 2026
Merged

fix(storage): honest Promise<number | void> type for commit-latency recorder #1853

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 6 additions & 4 deletions resources/DatabaseTransaction.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,8 +41,10 @@ export function setCommitLatencyRecorder(recorder: ((durationMs: number) => void
// queue time. The recorder is wrapped so it can never throw — a metrics failure must neither break the
// commit nor surface as an unhandled rejection on this floating `.then`. The thenable guard protects
// against a future caller passing a non-Promise `commitResolution` (today it is always the rocksdb-js
// async `Transaction.commit()` result, which is guaranteed to be a Promise).
function recordCommitLatency(commitResolution: Promise<unknown> | void, submittedAt: number) {
// async `Transaction.commit()` result, which is guaranteed to be a Promise). The parameter matches
// `commit()`'s honest `Promise<number | void>` result (the coordinated-retry sentinel); the resolved
// value is intentionally ignored — only the settle timing is recorded.
function recordCommitLatency(commitResolution: Promise<number | void>, submittedAt: number) {

@cb1kenobi cb1kenobi Jul 21, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Note: three open PRs fix this same recordCommitLatency type error — only one should land.

#1853 (this), #1887, and #1869 all edit these same lines in DatabaseTransaction.ts, so whichever merges first leaves the other two with a conflict. Of the three, this is the cleanest fix: it makes the parameter type honest (Promise<number | void>, matching commit()'s actual result), drops the ineffective as Promise<void> cast, and corrects the comment.

Suggest landing this dedicated fix and dropping the type change from the other two.

—
Generated by Barber AI

@kriszyp kriszyp Jul 22, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yeah ,we are really going to make sure we land this fix!

@cb1kenobi cb1kenobi Jul 22, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Update: the duplicate-fix note above is stale.

Since that comment, #1890 (fix(types): widen recordCommitLatency parameter so the build type-checks) closed and #1899 (Fix recordCommitLatency parameter type for the widened commitResolution union) merged — landing the same TS2345 fix with the looser Promise<unknown> | void type. Those were the actual competing PRs, not #1887/#1869 as previously stated (#1869 no longer touches this file; #1887's edit here is unrelated, further down in the same method).

#1853 is now a clean refinement on top of #1899's merged fix: narrows the parameter to the more precise Promise<number | void> (matching commit()'s actual resolved type, including the RETRY_NOW_VALUE sentinel) and updates the adjacent comments. mergeable: true against current main — no conflict.

—
Generated by Barber AI

if (!recordCommitLatencyMs) return;
const record = () => {
try {
Expand Down Expand Up @@ -400,8 +402,8 @@ export class DatabaseTransaction implements Transaction {
if (this.writes.length > 0) {
// The transaction was created with coordinatedRetry:true (see
// getReadTxn), so commit() can resolve to RETRY_NOW_VALUE. That
// sentinel is handled in the resolve callback below and never
// propagates past that branch.
// sentinel (a number) is why commitResolution is typed
// Promise<number | void>; it is handled in the resolve callback below.
commitResolution = transaction.commit();
// Record how long this commit stays outstanding (submit → settle) as a distribution
// metric. This is the same clock the overload check uses (outstandingCommitStart is
Expand Down
Loading