Skip to content

feat: Add an afterCommit context helper - #4110

Open
janbuchar wants to merge 1 commit into
masterfrom
storage-commit-hook
Open

feat: Add an afterCommit context helper#4110
janbuchar wants to merge 1 commit into
masterfrom
storage-commit-hook

Conversation

@janbuchar

Copy link
Copy Markdown
Contributor

@janbuchar janbuchar added the t-tooling Issues with this label are in the ownership of the tooling team. label Sep 7, 2026
@github-actions github-actions Bot added this to the 149th sprint - Tooling team milestone Sep 7, 2026
@github-actions github-actions Bot added the tested Temporary label used only programatically for some analytics. label Sep 7, 2026

@barjin barjin left a comment

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.

Thank you @janbuchar , just one thing ⬇️ (probably just the matter of documenting this better).

Comment on lines +205 to +207
* if (error) {
* throw new NonRetryableError('The page is too big to store', { cause: error });
* }

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.

If we throw a retriable error, does it mean we'll process the same request again? meaning

  1. run requestHandler
  2. commit dataset writes
  3. throw from afterStorageCommit
  4. (?) back to 1. and 2., creating duplicate dataset items?

Should we make every afterStorageCommit error non-retriable? Or maybe just document this better?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

t-tooling Issues with this label are in the ownership of the tooling team. tested Temporary label used only programatically for some analytics.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Storage write errors are no longer catchable, and there's no commit hook

3 participants