CLDSRV-973: keep serving when the delete path throws synchronously - #6255
CLDSRV-973: keep serving when the delete path throws synchronously#6255DarkIsDude wants to merge 1 commit into
Conversation
A synchronous throw in the data layer while deleting an object escaped into the promise machinery, where it crossed an async callback that had already fired. That produced "Callback was already called", raised as an unhandledRejection, which Node turns into a process exit. One request therefore killed the pod serving it, and client retries walked the failure across the deployment until the whole S3 endpoint was down. Contain it at the delete path: a throw becomes InternalError for that request and the process keeps serving traffic. The trigger seen in the field was an unguarded backend-client lookup for a location removed from the overlay (fixed separately in ARSN-623); this change stops the next such throw from being fatal. Issue: CLDSRV-973
Hello darkisdude,My role is to assist you with the merge of this Available options
Available commands
Status report is not available. |
Incorrect fix versionThe
Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:
Please check the |
Codecov Report❌ Patch coverage is
❌ Your patch check has failed because the patch coverage (57.14%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files
@@ Coverage Diff @@
## development/9.3 #6255 +/- ##
===================================================
- Coverage 85.17% 85.16% -0.02%
===================================================
Files 206 206
Lines 13424 13427 +3
===================================================
+ Hits 11434 11435 +1
- Misses 1990 1992 +2
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
Branches have divergedThis pull request's source branch To avoid any integration risks, please re-synchronize them using one of the
Note: If you choose to rebase, you may have to ask me to rebuild |
Issue: CLDSRV-973
Draft — the containment fix is complete, but the broader question in Open questions should be settled before this merges.
Problem
A synchronous throw in the data layer while deleting an object does not become a
500for that request. It escapes into the promise machinery, crosses anasynccallback that has already fired, and produces:Node's default for an unhandled rejection is to exit, so one request kills the pod serving it. Clients retry automatically, so the failure walks the deployment: in the field this took all 25 cloudserver pods into CrashLoopBackOff and the ARTESCA S3 endpoint fully offline, from a single
delete-objectscall. Found during the IronMountain XDM scale campaign (OS-1095) on ARTESCA 4.3.0-rc2 /cloudserver:9.3.9.Note the first log line: the request-scoped error was caught and logged with full context. The process still died on the second one.
Change
Contain it at the delete path in
services.deleteObject: wrap thedata.protectAzureBlocks(...)call so a synchronous throw is logged and returned asInternalErrorfor that request, instead of escaping. The pod keeps serving traffic.The specific trigger seen in the field (an unguarded backend-client lookup for a location removed from the overlay) is fixed separately in scality/Arsenal#2686 / ARSN-623. This PR is deliberately about the blast radius — so that the next throw on this path is a failed request rather than a fleet-wide outage.
Open questions for review
try/catchthe right depth of fix? It closes the path with a confirmed outage, but any other synchronous throw reachable from a request handler has the same consequence. A process-levelunhandledRejectionhandler that logs and attributes the rejection to its request — rather than exiting — would address the class instead of this one instance. That is a deliberate availability-vs-fail-fast policy decision, so I have not made it here.lib/server.js:89already installs anuncaughtExceptionhandler; there is nounhandledRejectionequivalent.Callback was already called.means the first callback already ran, i.e. the request had been answered. Arguably that case should always be logged and swallowed rather than escalated to fatal.development/9.3since 9.3.9 is the deployed version, so Bert-E forward-ports to 9.4.