fix(blobs): throw when a conditional write fails - #756
Conversation
Conditional writes reported `{ etag: '', modified: true }` for every response
that was not a 412, including a 503 after retries were exhausted. They now throw
`BlobsInternalError`, the way unconditional writes already do. This changes
behavior: callers who never wrapped a conditional write in try/catch will get a
rejection where they used to get a false success.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughSummary by CodeRabbit
WalkthroughConditional Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to The change makes conditional writes report failed responses as errors while preserving successful 2xx writes and adds focused coverage; no actionable merge-blocking risk remains beyond normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed: dependency version conflict. Check your lock file or package.json. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
@netlify/ai
@netlify/aws-lambda-compat
@netlify/blobs
@netlify/cache
@netlify/dev
@netlify/dev-utils
@netlify/headers
@netlify/images
@netlify/otel
@netlify/redirects
@netlify/runtime
@netlify/runtime-utils
@netlify/static
@netlify/types
@netlify/database-dev
@netlify/database
@netlify/database-proxy
@netlify/edge-functions-dev
@netlify/edge-functions
@netlify/functions-dev
@netlify/functions
@netlify/identity
commit: |
1 participant
A conditional write reports success for every failure that isn't a 412.
setandsetJSONboth decide the result with the same two-armed ternary:412 means the precondition failed. Everything else counts as a successful write.
And since that returns before the
if (res.status === STATUS_OK)check below it,the
throw new BlobsInternalError(...)that ends both methods can never bereached on this path.
fetchAndRetryreturns the last failing response insteadof throwing, so a store that is down for all six attempts arrives here as an
ordinary 503 and gets reported as
{ etag: '', modified: true }.Drop
onlyIfMatch/onlyIfNewand the same call throws correctly. So only theconditional form swallows the failure, and that's the form whose return value is
the caller's only signal that anything landed.
Both blocks now check 412 first, treat any 2xx as success, and let everything
else fall through to the
BlobsInternalErrorthat was already there.I used
res.okrather thanres.status === STATUS_OKon purpose. Conditionalwrites succeed with 201, and four existing tests say so (
onlyIfNewsuccess, APIand edge, both methods). Narrowing to 200 turns those four red, which trades a
silent lost write for a spurious failure. The unconditional path is untouched, so
the two paths now accept different success sets. That asymmetry is intentional
here, though it might deserve its own look.
Fixes #741
Tests
Six added, spread across the four existing
Conditional writesblocks:setandsetJSONon both the API andedge paths, one of them via
onlyIfNewsoif-none-matchis covered toofor
setandsetJSONEach one asserts
BlobsInternalErrorand the message, so the status that reachedthe throw is pinned rather than assumed.
packages/blobsgoes from 140 to 146 passing with nothing regressed. The four 201tests above guard the
res.okchoice and stay green.Notes
no
try/catch. They get a rejection now where they used to get a falsesuccess. That is what the issue asks for and what unconditional writes already
do, but I titled it
fix, so relabel it if you would rather ship afeat!.{ modified: false }. On theAPI-credentials path the conditional PUT goes to a signed provider URL, so if
that provider answers a failed
if-matchwith a 404 instead of a 412, thisturns it into a throw. It isn't a regression, since that case returned a
phantom success before, but someone who knows the provider's status matrix
should probably check.
setandsetJSON. I left it duplicatedto keep the diff shaped like fix(blobs): send conditional write headers in setJSON #731. Happy to pull it into a private helper next
to
getConditionsif you would rather have that.