👷(CLDSRV-860) Monitor async await migration by DarkIsDude · Pull Request #6088 · scality/cloudserver

DarkIsDude · 2026-02-25T15:47:58Z

https://scality.atlassian.net/wiki/spaces/OS/pages/3523346468/2025-10-30+-+Async+Await+migration

.github/scripts/check-diff-async.mjs

.github/workflows/tests.yaml

.github/scripts/count-async-functions.mjs

.github/scripts/check-diff-async.mjs

.github/workflows/tests.yaml

francoisferrand · 2026-03-11T08:10:19Z

.github/scripts/count-async-functions.mjs

+
+if (process.env.GITHUB_STEP_SUMMARY) {
+    const { writeFileSync, appendFileSync } = await import('node:fs');
+    appendFileSync(process.env.GITHUB_STEP_SUMMARY, [


do we really want a summary in every PR?
for monitoring the progress, this may be done in a separate job (eg. nightly on on dev branches) which builds a report giving the status on the "latest branch" (as a badge in the README, as a GitHub page, ....)

IMO we can keep it simple as compute the report in each PR. Not a big deal, it's only some seconds so no finance impact.

I was not thinking about build time or finance, but really about relevance:

a summary is "inside" the build, so people will not go look at it - not will it show them how much progress they made (in their PR)

for "monitoring", i.e. if someone want to (periodically...) check the status : not sure it is very practical to just go and look inside the last finished build

the summary does not hurt I agree, but I'd rather have something more dedicated to their purpose, like

a report in a "static" place : GitHub page, badge in the Readme, periodic reporting job...

a comment on the PR indicating how this specific PR affected the async migration (like "Great, you removed 10 async calls! Migration is now 53% complete, keep pushing!")

That's a good point 🙏. Let's start simple again and I'll add your comment. We don't know how this will be use and what is needed from the team/dev. So I suggest to not waste time on that and iterate when we have feedback ?

We don't know how this will be use and what is needed from the team/dev

That is not really true : this work was started because you wanted to solve a problem, i.e. gaining visibility on progress of the migration I think.
And my point here is really simple : does the summary approach give you that visibility?

.github/workflows/tests.yaml

.github/scripts/check-diff-async.mjs

francoisferrand · 2026-03-11T08:19:37Z

.github/scripts/check-diff-async.mjs

should these scripts not be in the Guidelines repo, so they can be reused?

(and if needed, it may include a github action as well)

Right now as it's the first repo, I would keep them here. When will be migrated to an another repo, can be. Maybe with a claude skills also 🤷

Right now as it's the first repo, I would keep them here

why do you say that?
Do we expect to have to iterate on these scripts, and need "closeness" with the code? Or do you think the script are kind of tailored to cloudserver, and would not work as such in other code base?

.github/scripts/check-diff-async.mjs

.github/scripts/count-async-functions.mjs

Issue: CLDSRV-860

.github/scripts/count-async-functions.mjs

.github/scripts/check-diff-async.mjs

.github/scripts/count-async-functions.mjs

Issue: CLDSRV-860

.github/scripts/check-diff-async.mjs

.github/scripts/count-async-functions.mjs

.github/scripts/check-diff-async.mjs

Issue: CLDSRV-860

.github/workflows/tests.yaml

Issue: CLDSRV-860

package.json

francoisferrand · 2026-03-20T08:04:28Z

.github/scripts/check-diff-async.mjs

+        if (fn.isAsync()) continue;
+
+        const startLine = fn.getStartLineNumber();
+        if (!addedLines.has(startLine)) continue;


is this the right filter?
This would consider only functions whose "header" is changed : and skip if only an update is made inside the existing function's body...

should we not search that there are no addedLines between fn.getStartLineNumber() and fn.getEndLineNumber() (or however it is called)

That's a good comment that I didn't notice. Anyway I suggest to keep current behaviour. We can start "slowly" and not too aggressive. This will allow dev to be familiar with async/await and introduce not too much change at the start. I'll add a comment in the guideline to setup your suggestion in some months ?

https://scality.atlassian.net/wiki/spaces/OS/pages/3523346468/2025-10-30+-+Async+Await+migration#Next-steps

I understand the iterative approach, but I think it would be really too limited to only require await migrations on signature changes -- they change very rarely, not sure signature only is significant enough for a first iteration.

delthas

LGTM on the whole (agree we can iterate later)

francoisferrand · 2026-03-25T07:40:19Z

.github/scripts/count-async-functions.mjs

+console.log(`Total functions:      ${totalFunctions}`);
+console.log(`Async functions:      ${asyncFunctions} (${asyncFunctionPercent}%)`);
+console.log(`Callback functions:   ${callbackFunctions}`);
+console.log(`Remaining .then():    ${thenChains}`);


should we also count "dual-style" functions (async + cb)?

francoisferrand · 2026-03-25T07:43:20Z

.github/scripts/count-async-functions.mjs

+
+if (process.env.GITHUB_STEP_SUMMARY) {
+    const { writeFileSync, appendFileSync } = await import('node:fs');
+    appendFileSync(process.env.GITHUB_STEP_SUMMARY, [


We don't know how this will be use and what is needed from the team/dev

That is not really true : this work was started because you wanted to solve a problem, i.e. gaining visibility on progress of the migration I think.
And my point here is really simple : does the summary approach give you that visibility?

francoisferrand · 2026-03-25T07:52:55Z

.github/workflows/tests.yaml

-      run: yarn run --silent lint -- --max-warnings 0
+    - name: Lint Javascript (strict, excluding async migration rules)
+      run: |
+        yarn run --silent lint -- --max-warnings 0 --rule "promise/prefer-await-to-then: off" --rule "n/callback-return: off"


should these rule exceptions not be added in the package.json instead?
so that developer can run yarn lint locally as well...

or do we expect developers to run lint with this rule, and be smart about ignoring them?

francoisferrand · 2026-03-25T08:11:04Z

eslint.config.mjs

+        "promise/prefer-await-to-then": "warn",
+        "n/callback-return": "warn",


kind of redundant with the dedicated linter you added

in CI : disabled

in dev : since we migrate "function by function", there will probably be lots of warnings there... which may make the lint practically useless (too hard to use, too many warnings to ignore) for devs?

i.e. should we really add these now?

Issue: CLDSRV-860

claude · 2026-03-27T13:25:35Z

.github/scripts/count-async-functions.mjs

+
+    const propertyAccesses = sourceFile.getDescendantsOfKind(SyntaxKind.PropertyAccessExpression);
+    for (const access of propertyAccesses) {
+        if (access.getName() === 'then') {


The .then() counting via PropertyAccessExpression name check will match any property access named then, not just Promise chains. Objects with a then property that are not Promises will inflate the metric. Consider using CallExpression with a then method name instead, to narrow to .then() calls rather than .then property reads.

— Claude Code

claude · 2026-03-27T13:25:37Z

.github/workflows/tests.yaml

+    - name: Check async/await compliance in diff
+      run: yarn run check-diff-async
+
+  async-migration-report:


The async-migration-report job runs on every push to every branch (the workflow triggers on push), and auto-push: true pushes benchmark data to gh-pages each time. Feature branch pushes will pollute the benchmark history. Consider restricting this job to main-only with if: github.ref == 'refs/heads/main'.

— Claude Code

claude · 2026-03-27T13:25:38Z

.github/scripts/count-async-functions.mjs

+console.log(`Migration (trend):    ${asyncFunctions}/${asyncFunctions + callbackFunctions} (${migrationPercent}%)`);
+
+if (process.env.GITHUB_STEP_SUMMARY) {
+    const { appendFileSync, writeFileSync } = await import('node:fs');


readFileSync is already imported statically at line 7. The dynamic await import('node:fs') here is redundant. appendFileSync and writeFileSync can be added to the static import at the top of the file instead.

— Claude Code

claude · 2026-03-27T13:25:47Z

.then() counting in count-async-functions.mjs matches any PropertyAccessExpression named then, not just Promise chains — could inflate the metric
- Use CallExpression check instead to narrow to actual .then() calls
- async-migration-report job runs on every branch push with auto-push: true, polluting benchmark history
- Add if: github.ref == 'refs/heads/main' to restrict to main merges only
- Redundant dynamic await import('node:fs') in count-async-functions.mjs:87 when readFileSync is already statically imported
- Add appendFileSync, writeFileSync to the static import at line 7

Review by Claude Code

DarkIsDude self-assigned this Feb 25, 2026

This comment was marked as resolved.

Sign in to view

DarkIsDude changed the base branch from development/9.2 to development/9.3 February 25, 2026 15:48

This comment was marked as resolved.

Sign in to view

DarkIsDude commented Mar 10, 2026

View reviewed changes

.github/scripts/check-diff-async.mjs Outdated Show resolved Hide resolved

DarkIsDude marked this pull request as ready for review March 10, 2026 16:09

DarkIsDude requested review from a team, benzekrimaha and delthas March 10, 2026 16:09

DarkIsDude force-pushed the feature/CLDSRV-860/monitor-async-await-migration branch from 93020af to 8981239 Compare March 10, 2026 16:10

claude bot reviewed Mar 10, 2026

View reviewed changes

.github/scripts/check-diff-async.mjs Outdated Show resolved Hide resolved

claude bot reviewed Mar 10, 2026

View reviewed changes

.github/workflows/tests.yaml Outdated Show resolved Hide resolved

claude bot reviewed Mar 10, 2026

View reviewed changes

.github/scripts/count-async-functions.mjs Outdated Show resolved Hide resolved

claude bot reviewed Mar 10, 2026

View reviewed changes

.github/scripts/check-diff-async.mjs Outdated Show resolved Hide resolved

claude bot reviewed Mar 10, 2026

View reviewed changes

.github/workflows/tests.yaml Show resolved Hide resolved

This comment was marked as resolved.

Sign in to view

francoisferrand requested changes Mar 11, 2026

View reviewed changes

This comment was marked as resolved.

Sign in to view

DarkIsDude added 3 commits March 19, 2026 14:23

🚨 add eslint configuration to monitor async/await

d3e6d78

Issue: CLDSRV-860

👷 update CI to add new check on async/await file

155ad19

Issue: CLDSRV-860

👷 remove then check from the code

869e8b2

Issue: CLDSRV-860

DarkIsDude force-pushed the feature/CLDSRV-860/monitor-async-await-migration branch from a64c535 to f1e7018 Compare March 19, 2026 13:23

claude bot reviewed Mar 19, 2026

View reviewed changes

.github/scripts/count-async-functions.mjs Outdated Show resolved Hide resolved

claude bot reviewed Mar 19, 2026

View reviewed changes

.github/scripts/count-async-functions.mjs Outdated Show resolved Hide resolved

claude bot reviewed Mar 19, 2026

View reviewed changes

.github/scripts/count-async-functions.mjs Show resolved Hide resolved

claude bot reviewed Mar 19, 2026

View reviewed changes

.github/scripts/check-diff-async.mjs Outdated Show resolved Hide resolved

claude bot reviewed Mar 19, 2026

View reviewed changes

.github/scripts/check-diff-async.mjs Outdated Show resolved Hide resolved

claude bot reviewed Mar 19, 2026

View reviewed changes

.github/scripts/count-async-functions.mjs Outdated Show resolved Hide resolved

This comment was marked as resolved.

Sign in to view

👷 remove async/await rule in the CI

c4d81e1

Issue: CLDSRV-860

DarkIsDude force-pushed the feature/CLDSRV-860/monitor-async-await-migration branch from f1e7018 to c4d81e1 Compare March 19, 2026 13:26

claude bot reviewed Mar 19, 2026

View reviewed changes

.github/scripts/check-diff-async.mjs Show resolved Hide resolved

claude bot reviewed Mar 19, 2026

View reviewed changes

.github/scripts/count-async-functions.mjs Outdated Show resolved Hide resolved

claude bot reviewed Mar 19, 2026

View reviewed changes

.github/scripts/check-diff-async.mjs Outdated Show resolved Hide resolved

This comment was marked as resolved.

Sign in to view

DarkIsDude added 3 commits March 19, 2026 16:35

✨ add count array source path from package.json

8e5ad67

Issue: CLDSRV-860

✅ create test for new scripts

b15c6e8

Issue: CLDSRV-860

🔒️ use array argument to avoid injection

68ce4e6

Issue: CLDSRV-860

DarkIsDude requested a review from francoisferrand March 19, 2026 16:31

claude bot reviewed Mar 19, 2026

View reviewed changes

.github/workflows/tests.yaml Outdated Show resolved Hide resolved

This comment was marked as resolved.

Sign in to view

👷 change how we compute the migration progress

8612e12

Issue: CLDSRV-860

DarkIsDude force-pushed the feature/CLDSRV-860/monitor-async-await-migration branch from f3ff7ea to 8612e12 Compare March 19, 2026 16:35

claude bot reviewed Mar 19, 2026

View reviewed changes

package.json Show resolved Hide resolved

This comment was marked as resolved.

Sign in to view

francoisferrand reviewed Mar 20, 2026

View reviewed changes

delthas approved these changes Mar 23, 2026

View reviewed changes

francoisferrand reviewed Mar 25, 2026

View reviewed changes

👷 init the CodeQL for async/await

c057495

Issue: CLDSRV-860

claude bot reviewed Mar 27, 2026

View reviewed changes

		"promise/prefer-await-to-then": "warn",
		"n/callback-return": "warn",

Conversation

DarkIsDude commented Feb 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment was marked as resolved.

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment was marked as resolved.

This comment was marked as resolved.

Uh oh!

Uh oh!

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

delthas left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

francoisferrand Mar 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

claude bot Mar 27, 2026

Choose a reason for hiding this comment

DarkIsDude commented Feb 25, 2026 •

edited

Loading

francoisferrand Mar 25, 2026 •

edited

Loading