Forward X-Scal-Request-Uids on Scuba quota requests#6220
Open
delthas wants to merge 5 commits into
Open
Conversation
Contributor
Hello delthas,My role is to assist you with the merge of this Available options
Available commands
Status report is not available. |
Contributor
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
|
❌ 2 Tests Failed:
View the top 3 failed test(s) by shortest run time
View the full list of 1 ❄️ flaky test(s)
To view more test analytics, go to the Test Analytics Dashboard |
delthas
force-pushed
the
improvement/CLDSRV-946/forward-requids-scuba
branch
from
July 6, 2026 14:28
d4798d7 to
22ff966
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Comment on lines
+269
to
+349
| (apiName, done) => { | ||
| // Object copy operations first check the target object, | ||
| // meaning the source object, containing the current bytes, | ||
| // is checked second. This logic handles these APIs calls by | ||
| // ensuring the bytes are positives (i.e., not an object | ||
| // replacement). | ||
| if (actionNeedQuotaCheckCopy(apiName, apiMethod)) { | ||
| // eslint-disable-next-line no-param-reassign | ||
| inflight = Math.abs(inflight); | ||
| } else if (!actionNeedQuotaCheck[apiName] && !actionWithDataDeletion[apiName]) { | ||
| return done(); | ||
| } | ||
| // When inflights are disabled, the sum of the current utilization metrics | ||
| // and the current bytes are compared with the quota. The current bytes | ||
| // are not sent to the utilization service. When inflights are enabled, | ||
| // the sum of the current utilization metrics only are compared with the | ||
| // quota. They include the current inflight bytes sent in the request. | ||
| let _inflights = shouldSendInflights ? inflight : undefined; | ||
| const inflightForCheck = shouldSendInflights ? 0 : inflight; | ||
| return _evaluateQuotas( | ||
| bucketQuota, | ||
| accountQuota, | ||
| bucket, | ||
| account, | ||
| _inflights, | ||
| inflightForCheck, | ||
| apiName, | ||
| log, | ||
| (err, _bucketQuotaExceeded, _accountQuotaExceeded) => { | ||
| if (err) { | ||
| return done(err); | ||
| } | ||
|
|
||
| bucketQuotaExceeded = _bucketQuotaExceeded; | ||
| accountQuotaExceeded = _accountQuotaExceeded; | ||
| bucketQuotaExceeded = _bucketQuotaExceeded; | ||
| accountQuotaExceeded = _accountQuotaExceeded; | ||
|
|
||
| // Inflights are inverted: in case of cleanup, we just re-issue | ||
| // the same API call. | ||
| if (_inflights) { | ||
| _inflights = -_inflights; | ||
| } | ||
| // Inflights are inverted: in case of cleanup, we just re-issue | ||
| // the same API call. | ||
| if (_inflights) { | ||
| _inflights = -_inflights; | ||
| } | ||
|
|
||
| request.finalizerHooks.push((errorFromAPI, _done) => { | ||
| const code = (bucketQuotaExceeded || accountQuotaExceeded) ? 429 : 200; | ||
| const quotaCleanUpStartTime = process.hrtime.bigint(); | ||
| // Quotas are cleaned only in case of error in the API | ||
| async.waterfall([ | ||
| cb => { | ||
| if (errorFromAPI) { | ||
| return _evaluateQuotas(bucketQuota, accountQuota, bucket, account, _inflights, | ||
| null, apiName, log, cb); | ||
| } | ||
| return cb(); | ||
| }, | ||
| ], () => { | ||
| monitorQuotaEvaluationDuration(apiMethod, type, code, quotaEvaluationDuration + | ||
| Number(process.hrtime.bigint() - quotaCleanUpStartTime)); | ||
| return _done(); | ||
| request.finalizerHooks.push((errorFromAPI, _done) => { | ||
| const code = bucketQuotaExceeded || accountQuotaExceeded ? 429 : 200; | ||
| const quotaCleanUpStartTime = process.hrtime.bigint(); | ||
| // Quotas are cleaned only in case of error in the API | ||
| async.waterfall( | ||
| [ | ||
| cb => { | ||
| if (errorFromAPI) { | ||
| return _evaluateQuotas( | ||
| bucketQuota, | ||
| accountQuota, | ||
| bucket, | ||
| account, | ||
| _inflights, | ||
| null, | ||
| apiName, | ||
| log, | ||
| cb, | ||
| ); | ||
| } | ||
| return cb(); | ||
| }, | ||
| ], | ||
| () => { | ||
| monitorQuotaEvaluationDuration( | ||
| apiMethod, | ||
| type, | ||
| code, | ||
| quotaEvaluationDuration + Number(process.hrtime.bigint() - quotaCleanUpStartTime), | ||
| ); | ||
| return _done(); | ||
| }, | ||
| ); | ||
| }); | ||
| }); | ||
|
|
||
| return done(); | ||
| }); | ||
| }, err => { | ||
| quotaEvaluationDuration = Number(process.hrtime.bigint() - requestStartTime); | ||
| if (err) { | ||
| log.warn('Error getting metrics from the quota service, allowing the request', { | ||
| error: err.name, | ||
| description: err.message, | ||
| }); | ||
| } | ||
| if (!actionWithDataDeletion[apiMethod] && | ||
| (bucketQuotaExceeded || accountQuotaExceeded)) { | ||
| return callback(errors.QuotaExceeded); | ||
| } | ||
| return callback(); | ||
| }); | ||
| return done(); | ||
| }, | ||
| ); | ||
| }, |
Comment on lines
+317
to
+332
| cb => { | ||
| if (errorFromAPI) { | ||
| return _evaluateQuotas( | ||
| bucketQuota, | ||
| accountQuota, | ||
| bucket, | ||
| account, | ||
| _inflights, | ||
| null, | ||
| apiName, | ||
| log, | ||
| cb, | ||
| ); | ||
| } | ||
| return cb(); | ||
| }, |
Formatting-only baseline: run Prettier on the files this PR modifies so the subsequent functional and async/await changes stay prettier-clean without mixing in unrelated reformatting. Also shortens one over-long test title (drops a redundant word) so a reflowed line stays within the eslint max-len of 120. No behavior change. Issue: CLDSRV-946
delthas
force-pushed
the
improvement/CLDSRV-946/forward-requids-scuba
branch
from
July 6, 2026 15:04
22ff966 to
e2084c3
Compare
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Thread the werelogs logger into ScubaClientImpl.getUtilizationMetrics and, when available, forward the serialized req_id chain (log.getSerializedUids()) as the X-Scal-Request-Uids header on Scuba utilization requests, mirroring the arsenal RESTClient convention. This lets Scuba correlate quota lookups with the originating cloudserver request. The header is only added when the logger exposes getSerializedUids(), so mock/absent loggers never send an undefined value. Quota (_evaluateQuotas) and Veeam capacity-metrics call sites now pass log; the background health check path is intentionally left untouched. Issue: CLDSRV-946
delthas
force-pushed
the
improvement/CLDSRV-946/forward-requids-scuba
branch
2 times, most recently
from
July 7, 2026 14:42
8b634e9 to
c0b8f6f
Compare
delthas
marked this pull request as ready for review
July 7, 2026 14:44
benzekrimaha
reviewed
Jul 8, 2026
benzekrimaha
reviewed
Jul 8, 2026
benzekrimaha
reviewed
Jul 8, 2026
benzekrimaha
reviewed
Jul 8, 2026
benzekrimaha
reviewed
Jul 8, 2026
benzekrimaha
reviewed
Jul 8, 2026
maeldonn
reviewed
Jul 8, 2026
Convert getUtilizationMetrics to an async function that awaits the parent ScubaClient.getLatestMetrics directly. It keeps a dual callback+async interface: callers can await the returned promise, and when a callback is supplied a thin trampoline delegates to the async version and adapts the result back via .then(onFulfilled, onRejected). This preempts the CodeQL "callback-style function" alert on the handler this PR modifies while leaving callers unchanged. The metrics duration is recorded in a finally block so both the success and error paths are covered. Drop the now-unused util.callbackify helper (and the util import), and have the Veeam capacity-metrics path await getUtilizationMetrics directly instead of re-promisifying it. getUtilizationMetrics is an internal Scuba client method, not a CORS-bearing S3 API handler, so the trampoline does not carry additionalResHeaders. Unit tests now stub the parent ScubaClient.prototype.getLatestMetrics. The quota metrics-failure test expects the account branch to also be queried: since the Scuba client resolves asynchronously (as in production), the parallel account lookup runs even when the bucket lookup errors. Issue: CLDSRV-946
delthas
force-pushed
the
improvement/CLDSRV-946/forward-requids-scuba
branch
from
July 13, 2026 14:42
c0b8f6f to
273de92
Compare
maeldonn
approved these changes
Jul 15, 2026
Drop the dual-mode callback trampoline from ScubaClientImpl.getUtilizationMetrics (the `.then()`-based callback branch and the trailing `callback` parameter), and migrate the only remaining callback caller — the bucketQuota/accountQuota tasks of _evaluateQuotas' async.parallel — to await the promise directly (async.parallel accepts async-function tasks). veeam already awaited it. Removes the `.then()` inside the async method and the awkward callback-as-last-arg signature raised in review; behaviour is unchanged (the quota tests stub getLatestMetrics directly and pass as-is). Issue: CLDSRV-946
benzekrimaha
approved these changes
Jul 21, 2026
benzekrimaha
left a comment
benzekrimaha
left a comment
Contributor
There was a problem hiding this comment.
LGTM , but I'd strongly advice converting _healthCheck to async/await too (CodeQL .then() still open)
Convert the _healthCheck().then().catch() chain to async/await (try/catch), removing the last .then() in the Scuba wrapper (clears the CodeQL prefer-async and eslint prefer-await-to-then flags on this file). Behaviour is unchanged; the _healthCheck unit tests already await it. Issue: CLDSRV-946
Contributor
Author
|
/approve |
Contributor
Build failedThe build for commit did not succeed in branch improvement/CLDSRV-946/forward-requids-scuba The following options are set: approve |
Contributor
In the queueThe changeset has received all authorizations and has been added to the The changeset will be merged in:
The following branches will NOT be impacted:
This pull request does not target the following hotfix branch(es) so they
There is no action required on your side. You will be notified here once IMPORTANT Please do not attempt to modify this pull request.
If you need this pull request to be removed from the queue, please contact a The following options are set: approve |
Contributor
Queue build failedThe corresponding build for the queue failed:
Remove the pull request from the queue
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Forward the werelogs req_id chain (
log.getSerializedUids()) as theX-Scal-Request-Uidsheader on cloudserver's Scuba utilization requests, so Scuba can correlate quota lookups back to the originating cloudserver request.traceparentalready flows; the req_id chain did not.ScubaClientImpl.getUtilizationMetricsnow takes alogparameter and, when the logger exposesgetSerializedUids(), merges the header into the per-call options (arsenalRESTClientconvention). A missing/mock logger never sends an undefined header. The quota (_evaluateQuotas) and Veeam capacity-metrics call sites passlog; the background health-check path is intentionally left untouched.Issue: CLDSRV-946