nodejs: implement 130/311 benchmarks#281
Conversation
📝 WalkthroughWalkthroughAdds new Node.js benchmarks (CRUD shopping-cart API, compression/zip utility), multiple package manifests, NoSQL wrapper implementations for AWS/Azure/GCP/OpenWhisk/local, migrates S3 adapter to AWS SDK v3, updates Node.js runtimes/deployment configs, and adjusts storage handlers and CI matrices. Changes
Sequence DiagramssequenceDiagram
participant Client
participant Handler as CRUD Handler
participant NoSQL as NoSQL Storage
Client->>Handler: event.requests [PUT/GET /cart]
loop per request
alt PUT /cart
Handler->>NoSQL: insert("shopping_cart", cartId, productId, data)
NoSQL-->>Handler: ack
else GET /cart/{id}
Handler->>NoSQL: get("shopping_cart", cartId, productId)
NoSQL-->>Handler: item or null
else GET /cart
Handler->>NoSQL: query("shopping_cart", cartId)
NoSQL-->>Handler: items[]
Handler->>Handler: aggregate(items) -> {products, total_cost, avg_price}
end
end
Handler-->>Client: [responses...]
sequenceDiagram
participant Client
participant Handler as Compression Handler
participant Storage as Object Storage
participant FS as Filesystem
Client->>Handler: event (bucket, prefix)
Handler->>Storage: downloadDirectory(bucket, prefix)
Storage-->>FS: write files to /tmp/unique
FS-->>Handler: files available
Handler->>Handler: parseDirectory -> sizes
Handler->>Handler: createZipArchive(/tmp/unique) -> archive.zip
Handler->>Storage: upload(bucket, archive.zip)
Storage-->>Handler: upload result
Handler-->>Client: { result: { bucket, key }, measurement: {...} }
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@benchmarks/100.webapps/130.crud-api/nodejs/function.js`:
- Around line 30-46: The code currently computes priceSum by adding
product.price per product, ignoring product.quantity; update the loop that
builds products, priceSum and quantitySum so that priceSum accumulates
product.price * product.quantity (i.e., total cost), keep quantitySum +=
product.quantity, and then compute avgPrice as (quantitySum > 0 ? priceSum /
quantitySum : 0.0) so avg_price is the correct per-item average; return
products, total_cost: priceSum (now total cost), and avg_price: avgPrice.
In `@benchmarks/300.utilities/311.compression/nodejs/function.js`:
- Around line 41-88: The function createTarGzArchive builds a custom
length-prefixed binary stream and then gzips it but still treats it as a .tar.gz
(outputPath), which is misleading and incompatible with tar tools; either (A)
rename the function and ensure outputPath uses a custom extension (e.g., change
createTarGzArchive to createCustomGzArchive and enforce outputPath
endsWith('.bin.gz') or '.custom.gz') so callers don't expect a real tar, or (B)
make it produce a real tar by replacing the custom packing logic with a proper
tar implementation (e.g., use the 'tar' module or node-tar to pack files from
sourceDir into a tar stream and then gzip it) and keep the .tar.gz extension;
update the function signature and any callers that reference createTarGzArchive
or outputPath accordingly.
- Around line 90-146: The handler creates a temporary directory at download_path
but never removes it; update exports.handler to wrap the
download/compress/upload logic in try/finally (or try/catch/finally) and in the
finally block synchronously or asynchronously remove the download_path (use
fs.rm or fs.rmdir with recursive:true or a safe recursive delete helper) so the
directory is always cleaned up even on errors; ensure you reference
download_path, createTarGzArchive, storage_handler.upload and await
uploadPromise before cleanup to avoid deleting files mid-upload and catch/remove
any deletion errors silently or log them.
🧹 Nitpick comments (1)
benchmarks/100.webapps/130.crud-api/nodejs/package.json (1)
2-8: Specify a valid license (and author if applicable) to avoid compliance ambiguity.
Leaving"license"empty can create distribution/compliance uncertainty. Please set a valid SPDX identifier (or"UNLICENSED"if intentional) consistent with the repository’s policy.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
benchmarks/100.webapps/130.crud-api/nodejs/function.jsbenchmarks/100.webapps/130.crud-api/nodejs/package.jsonbenchmarks/300.utilities/311.compression/nodejs/function.jsbenchmarks/300.utilities/311.compression/nodejs/package.json
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-19T16:15:01.811Z
Learnt from: mcopik
Repo: spcl/serverless-benchmarks PR: 99
File: benchmarks/000.microbenchmarks/010.sleep/cpp/main.cpp:11-0
Timestamp: 2025-06-19T16:15:01.811Z
Learning: In the serverless-benchmarks project, input validation for benchmark functions is not required since the benchmarks are invoked directly by the maintainers who control the input values. This applies to functions like the sleep benchmark in benchmarks/000.microbenchmarks/010.sleep/cpp/main.cpp.
Applied to files:
benchmarks/300.utilities/311.compression/nodejs/package.json
🔇 Additional comments (3)
benchmarks/300.utilities/311.compression/nodejs/function.js (2)
1-7: LGTM!The imports are appropriate, and module-scope
storage_handlerinstantiation enables connection reuse across warm invocations in serverless environments.
14-32: LGTM!The recursive directory size calculation is correct. Based on learnings, input validation is not required since benchmarks are invoked directly by maintainers.
benchmarks/300.utilities/311.compression/nodejs/package.json (1)
6-8: No action needed. The [email protected] package is a valid released version (published June 2024) and is stable for use.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Agent-Logs-Url: https://github.com/spcl/serverless-benchmarks/sessions/f21ff937-c93a-44c3-86bd-447a06015a92 Co-authored-by: mcopik <[email protected]>
…ejs-new-benchmarks3
mcopik
force-pushed
the
nodejs_new_benchmarks
branch
from
bfddbd4 to
913413c
Compare
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
configs/systems.json (1)
125-128:⚠️ Potential issue | 🟡 MinorUpdate
uuiddependency to a current version in all three Node.js runtime configurations.While the codebase correctly uses
uuid.v4()(compatible across all versions), the pinned version 3.4.0 is approximately 8 years old with no recent maintenance or security updates. Current versions (v9+) maintain backward compatibility with the named method API (uuid.v4()) while providing security and performance improvements. Update the uuid version in configs/systems.json at lines 126, 231, and 316 (AWS, Azure, and GCP runtime configurations respectively) to a current version like^9.0.0or higher.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@configs/systems.json` around lines 125 - 128, The uuid package version is pinned to the outdated "3.4.0" in the Node.js runtime package lists; update the "uuid" entry under the "packages" object in each Node.js runtime configuration (the three occurrences for AWS, Azure, and GCP) to a current, maintained semver like "^9.0.0" (or higher) so the runtime uses the up-to-date uuid release while preserving existing uuid.v4() usage.
🧹 Nitpick comments (9)
sebs/aws/s3.py (1)
233-239: Preferraise ... from errorto preserve exception context.Using
from Nonediscards the originalClientErrortraceback and response metadata (request-id, HTTP status, operation name), which is valuable for debugging intermittent AWS issues (throttling, 403 vs 404, expired credentials). Unless there's a specific reason to hide the cause, chain the exception explicitly.♻️ Proposed fix
try: self.client.download_file(Bucket=bucket_name, Key=key, Filename=filepath) except self.client.exceptions.ClientError as error: raise RuntimeError( f"Failed to download {key} from bucket {bucket_name}, " f"reason: {error.response['Error']['Message']}" - ) from None + ) from error🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sebs/aws/s3.py` around lines 233 - 239, The except block that catches self.client.exceptions.ClientError and raises a RuntimeError currently uses "from None", which discards the original ClientError context; change the exception chaining to "from error" so the RuntimeError preserves the original traceback and AWS response metadata (i.e., update the except handling around self.client.download_file to re-raise RuntimeError(... ) from error instead of from None).benchmarks/300.utilities/311.compression/nodejs/function.js (1)
109-116: Archive written inside the directory being archived.
archive_pathis placed insidedownload_path, which is alsosourceDirpassed tocreateTarGzArchive. It works today only becausecollectFilesruns to completion synchronously beforefs.writeFileSyncexecutes, but this is fragile — any future refactor that interleaves collection and writing (e.g., streaming the archive) will cause the archive to recursively include itself. Write the archive to a sibling path instead, e.g.path.join('/tmp', archive_name)(with a unique suffix), and upload from there.♻️ Proposed fix
- const archive_name = `${key}.tar.gz`; - const archive_path = path.join(download_path, archive_name); + const archive_name = `${key}.tar.gz`; + const archive_path = path.join('/tmp', `${uuidv4()}-${path.basename(archive_name)}`); await createTarGzArchive(download_path, archive_path);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@benchmarks/300.utilities/311.compression/nodejs/function.js` around lines 109 - 116, The archive file is being created inside download_path (archive_path = path.join(download_path, archive_name)), which is also passed as sourceDir to createTarGzArchive, risking the archive being included in itself; change the logic so archive_path is created in a sibling/temp directory (e.g., using os.tmpdir() with a unique suffix based on key/timestamp) instead of download_path, call createTarGzArchive(download_path, archive_path) to write to that temp location, then upload from the temp archive and cleanup the temp file; update references to archive_path (and related variables like archive_name, compress_begin/compress_end) accordingly.benchmarks/wrappers/gcp/nodejs/nosql.js (2)
47-58:query()assumes Datastore key name, not id.
item[KEY].nameis only populated when the entity was saved with a key that used anamepath element. Since_get_entity_keybuilds the key withsecondary_key[1](which is typically a string), this will usually work — but if a benchmark ever passes a numeric secondary key, Datastore auto-allocatesid(notname) anditem[secondary_key_name]becomesundefined. Consideritem[KEY].name ?? item[KEY].id.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@benchmarks/wrappers/gcp/nodejs/nosql.js` around lines 47 - 58, The query() function assumes Datastore keys always have a name and sets item[secondary_key_name] = item[KEY].name, which yields undefined for numeric-id keys; update the loop in query (and reference the KEY symbol and the query method) to set item[secondary_key_name] to item[KEY].name ?? item[KEY].id (or otherwise prefer name and fall back to id) so both named and auto‑allocated id keys are handled.
23-31: Non-atomic read-modify-write inupdate().
get()→Object.assign()→save()is a classic lost-update race: two concurrent handler invocations can both read the same baseline and the latersaveclobbers the earlier one's changes. Datastore offers transactions (runTransaction) or theif-match/updatemutation for CAS semantics. For a benchmark this may be acceptable, but the Python GCPnosql.updatelikely behaves the same — worth a note if you rely on update consistency across concurrent requests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@benchmarks/wrappers/gcp/nodejs/nosql.js` around lines 23 - 31, The update() method performs a non-atomic read-modify-write (get() → Object.assign() → save()) which can cause lost updates under concurrency; change it to use Datastore transactions or a conditional/CAS mutation: within update(), use this._client.runTransaction(...) (or the client's equivalent) to fetch the entity by this._get_entity_key(...), apply Object.assign(res, updates) inside the transaction, then save/commit the entity so the read-and-write are atomic, or alternatively use an if-match/CAS-style update API if available on this._client; ensure errors from transaction conflicts are retried or surfaced appropriately.benchmarks/wrappers/aws/nodejs/nosql.js (1)
30-35:insertmutates the caller'sdataargument.
data[primary_key[0]] = …adds key fields onto the object the caller passed in. The shared object infunction.jsis a fresh literal today, but this side effect is easy to trip over (e.g., retry loops re-passing the same object). Consider shallow-copying before injection. The same pattern appears in the local/openwhisk wrappers.♻️ Proposed fix
- async insert(table_name, primary_key, secondary_key, data) { - data[primary_key[0]] = primary_key[1]; - data[secondary_key[0]] = secondary_key[1]; - await this.client - .put({ TableName: this._get_table(table_name), Item: data }); - } + async insert(table_name, primary_key, secondary_key, data) { + const item = { + ...data, + [primary_key[0]]: primary_key[1], + [secondary_key[0]]: secondary_key[1], + }; + await this.client.put({ TableName: this._get_table(table_name), Item: item }); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@benchmarks/wrappers/aws/nodejs/nosql.js` around lines 30 - 35, The insert method currently mutates the caller's data object by assigning primary/secondary key fields directly; change insert to first create a shallow copy of the incoming data (e.g., newData = shallow copy of data) and then inject primary_key and secondary_key into that copy before calling this.client.put({ TableName: this._get_table(table_name), Item: newData }); apply the same non-mutating change to the corresponding local/openwhisk wrappers that use the same pattern so callers are not affected by side effects.benchmarks/wrappers/azure/nodejs/nosql.js (1)
78-88: Silent singleton reuse when config changes.If
get_instanceis first called with one(database, url, credential)triplet and later called with a different triplet, the second call silently returns the original client and ignores the new arguments. For benchmarks this is probably fine, but consider either ignoring args after the first call explicitly (documented) or throwing if args differ to avoid confusing behavior in multi-tenant tests. Same pattern applies to the GCP wrapper'sget_instance(database).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@benchmarks/wrappers/azure/nodejs/nosql.js` around lines 78 - 88, get_instance currently returns a previously-created nosql.instance even if later calls pass different (database, url, credential), which silently ignores new config; update get_instance (and mirror the change in the GCP wrapper's get_instance(database)) to detect argument mismatches: when nosql.instance already exists, compare the provided database/url/credential against stored values (store them on the instance, e.g., instance._database/_url/_credential) and throw an Error if they differ (or alternatively explicitly ignore args by documenting and returning instance) so callers aren't silently misconfigured; modify the constructor usage path that creates nosql.instance to set those stored fields and implement the comparison in get_instance.configs/systems.json (2)
384-392: OpenWhiskbase_imagesstill reference EOL Node.js runtimes (10/12/14).Node.js 10/12/14 are all out of support. The
openwhisk/action-nodejs-v10and-v12images haven't been rebuilt in years and the newly addedaws-sdkmay not even install there cleanly. Consider dropping them and adding 22 if available.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@configs/systems.json` around lines 384 - 392, Update the "nodejs" -> "base_images" -> "x64" map to remove EOL runtimes and add a current LTS: remove the "10", "12", and "14" keys (and their values "openwhisk/action-nodejs-v10", "-v12", "-v14"), keep supported entries (e.g., "18" and "20") and add a "22" key pointing to "openwhisk/action-nodejs-v22" (or the correct official image name if different); ensure any consumers of these keys handle the new set of versions.
404-407: OpenWhisk Node.js wrapper pins the maintenance-modeaws-sdkv2.
aws-sdk@^2.xentered maintenance mode in Sept 2024. Since this is a new dependency being introduced specifically forbenchmarks/wrappers/openwhisk/nodejs/nosql.js, please consider migrating the wrapper + this entry to@aws-sdk/client-dynamodb+@aws-sdk/lib-dynamodbto match the AWS wrapper. (Duplicate concern of thelocalwrapper comment.)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@configs/systems.json` around lines 404 - 407, Replace the pinned legacy aws-sdk v2 entry in the "packages" object with the modular AWS SDK v3 packages and update the OpenWhisk Node.js wrapper to use them: remove "aws-sdk" and add "@aws-sdk/client-dynamodb" and "@aws-sdk/lib-dynamodb" in configs/systems.json, then update benchmarks/wrappers/openwhisk/nodejs/nosql.js to import { DynamoDBClient } from "@aws-sdk/client-dynamodb" and { DynamoDBDocumentClient, PutCommand, GetCommand, QueryCommand, etc. } from "@aws-sdk/lib-dynamodb", instantiate DynamoDBClient and wrap it with DynamoDBDocumentClient (replacing any AWS.DynamoDB.DocumentClient usage), and refactor the wrapper functions (the code paths that call DocumentClient methods) to use the new client commands/respective method calls.dockerfiles/aws/nodejs/Dockerfile.build (1)
6-6: Adddnf clean allto trim the image layer.The
dnf swap/installleaves package metadata and downloaded RPMs in the layer. Matches Trivy DS-0019.🧹 Proposed fix
-RUN dnf swap -y curl-minimal curl && dnf install -y shadow-utils +RUN dnf swap -y curl-minimal curl \ + && dnf install -y shadow-utils \ + && dnf clean all \ + && rm -rf /var/cache/dnf🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dockerfiles/aws/nodejs/Dockerfile.build` at line 6, Update the Dockerfile.build RUN step that currently reads "RUN dnf swap -y curl-minimal curl && dnf install -y shadow-utils" to clean up package metadata and caches after installation; append commands such as "&& dnf clean all && rm -rf /var/cache/dnf /var/cache/yum" (or equivalent) to that RUN line so the layer does not retain downloaded RPMs and metadata.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@benchmarks/100.webapps/130.crud-api/nodejs/function.js`:
- Around line 1-4: The module-level call to nosql.nosql.get_instance() will
throw in GCP/Azure because required parameters are missing; remove the top-level
get_instance() invocation (do not call nosql.nosql.get_instance() during module
load), keep nosqlTableName, and instead lazily initialize nosqlClient inside the
request handler (e.g., in the exported handler function) after detecting
deployment context and passing the appropriate arguments (for GCP: database; for
Azure: database, url, credential; for AWS/local/OpenWhisk: no args or existing
defaults); alternatively, if you cannot determine environment here, update the
benchmark metadata to restrict to AWS/local/OpenWhisk.
In `@benchmarks/300.utilities/311.compression/nodejs/function.js`:
- Around line 90-146: Wrap the entire body of exports.handler in a
try/catch/finally: move current code into try, add a catch that logs or throws
an Error enriched with context (include phase name like "download", "compress",
or "upload" and the key variable) and rethrows so callers see the failure, and
add a finally block that removes the temporary download_path (use fs.rmSync or
fs.rmdirSync with { recursive: true } or an async fs.promises.rm) to ensure
cleanup on success or failure; ensure you reference the existing symbols
download_path, storage_handler.download_directory, createTarGzArchive,
storage_handler.upload, parseDirectory and uploadPromise so the error handling
wraps their awaited calls and captures which step failed and the key.
- Around line 66-88: The current routine reads every file into memory and builds
a single Buffer (fs.readFileSync, Buffer.concat) then calls zlib.gzipSync, which
will OOM and block the event loop; change the implementation to stream: create a
writable stream to outputPath (fs.createWriteStream) and a gzip transform via
zlib.createGzip(), pipe gzip into the file stream, and sequentially write each
file header and file body into the gzip stream without concatenating in-memory
(for file bodies use fs.createReadStream.pipe into the gzip stream or stream
each file's chunks into the gzip writable), then await stream completion with
stream.finished/promises.finished so the function returns after flush; replace
usages of fs.readFileSync, Buffer.concat, and zlib.gzipSync with the streaming
equivalents to avoid peak memory and event-loop blocking.
- Around line 100-103: The call to storage_handler.download_directory (in
function.js) is invalid because storage wrappers only implement upload,
download, uploadStream, downloadStream, and unique_name; replace the
download_directory usage by listing all objects under path.join(input_prefix,
key) (using the provider-specific listing utility or S3 list API) and loop over
each object calling storage_handler.download for each target path, or
alternatively add a download_directory method to every storage wrapper (wrappers
like benchmarks/wrappers/aws/nodejs/storage.js, azure, gcp, local, openwhisk)
implementing recursive listing+download so storage_handler.download_directory
exists; update references to storage_handler.download_directory or add the
method consistently across all wrappers so runtime TypeError is avoided.
In `@benchmarks/wrappers/aws/nodejs/nosql.js`:
- Around line 46-73: The update() method builds an UpdateExpression even when
updates is empty, which leaves update_expression as ["SET"] and then mangles it
into "SE" causing DynamoDB ValidationException; fix by short-circuiting when
updates is empty (e.g., if (!updates || Object.keys(updates).length === 0)
return) before constructing update_expression so this.client.update(...) is not
called with an invalid UpdateExpression; update the update() function (and any
relevant callers) to mirror the short-circuit behavior used in the sibling
local/openwhisk wrappers and return early without calling this.client.update or
_get_table when there are no updates to apply.
- Around line 7-14: The nosql constructor imports and instantiates AWS SDK v3
modules (`@aws-sdk/lib-dynamodb` and `@aws-sdk/client-dynamodb`) which are not
preinstalled in Lambda Node.js runtimes and will cause MODULE_NOT_FOUND at
runtime; add these packages to the deployment (declare them in package.json and
bundle with the function or include them in a Lambda layer) and update the
DynamoDB instantiation in the nosql constructor (the DynamoDBDocument.from(new
DynamoDB()) call) to supply a region fallback (use process.env.AWS_REGION || a
sensible default) so the client does not rely solely on Lambda-provided
environment variables.
In `@benchmarks/wrappers/azure/nodejs/nosql.js`:
- Around line 27-47: In the get method, assigning resource[secondary_key[0]] =
secondary_key[1] followed by unconditionally deleting resource.id can drop the
value when secondary_key[0] === "id"; update the cleanup logic inside async
get(...) so the deletion of the Azure-specific id field is conditional (only
delete resource.id when secondary_key[0] !== "id") and leave the assigned value
intact; reference the get function and the secondary_key variable when making
this change.
In `@benchmarks/wrappers/local/nodejs/nosql.js`:
- Around line 3-16: The nosql class constructor currently uses the deprecated
aws-sdk v2 and hardcodes "http://" into the endpoint; update the constructor to
use the v3 clients (`@aws-sdk/client-dynamodb` and `@aws-sdk/lib-dynamodb`) to
create a DynamoDBDocumentClient instead of aws.DynamoDB.DocumentClient and
remove any .promise() usage elsewhere, and change the endpoint handling in the
constructor to accept a full NOSQL_STORAGE_ENDPOINT URL (or validate/strip any
existing scheme) before passing it to the client so you don't prepend "http://"
blindly; locate the code in the nosql class constructor and replace the client
initialization and endpoint composition accordingly.
In `@configs/systems.json`:
- Around line 106-114: Replace the unpublished image tags (e.g.,
"amazon/aws-lambda-nodejs:24.2026.04.22.11-x86_64", "22.2026.04.22.11-arm64",
etc.) under the x86_64 and arm64 objects with published tags or stable
version-only tags; either use the published 2026-03-31 tags
("20.2026.03.31.11-<arch>", "22.2026.03.31.11-<arch>",
"24.2026.03.31.11-<arch>") or the simple version tags ("20", "22", "24") to
avoid 404s during pulls—update each value string in the x86_64 and arm64 entries
accordingly and prefer 22 or 24 (not 20) given Node.js 20 EOL.
---
Outside diff comments:
In `@configs/systems.json`:
- Around line 125-128: The uuid package version is pinned to the outdated
"3.4.0" in the Node.js runtime package lists; update the "uuid" entry under the
"packages" object in each Node.js runtime configuration (the three occurrences
for AWS, Azure, and GCP) to a current, maintained semver like "^9.0.0" (or
higher) so the runtime uses the up-to-date uuid release while preserving
existing uuid.v4() usage.
---
Nitpick comments:
In `@benchmarks/300.utilities/311.compression/nodejs/function.js`:
- Around line 109-116: The archive file is being created inside download_path
(archive_path = path.join(download_path, archive_name)), which is also passed as
sourceDir to createTarGzArchive, risking the archive being included in itself;
change the logic so archive_path is created in a sibling/temp directory (e.g.,
using os.tmpdir() with a unique suffix based on key/timestamp) instead of
download_path, call createTarGzArchive(download_path, archive_path) to write to
that temp location, then upload from the temp archive and cleanup the temp file;
update references to archive_path (and related variables like archive_name,
compress_begin/compress_end) accordingly.
In `@benchmarks/wrappers/aws/nodejs/nosql.js`:
- Around line 30-35: The insert method currently mutates the caller's data
object by assigning primary/secondary key fields directly; change insert to
first create a shallow copy of the incoming data (e.g., newData = shallow copy
of data) and then inject primary_key and secondary_key into that copy before
calling this.client.put({ TableName: this._get_table(table_name), Item: newData
}); apply the same non-mutating change to the corresponding local/openwhisk
wrappers that use the same pattern so callers are not affected by side effects.
In `@benchmarks/wrappers/azure/nodejs/nosql.js`:
- Around line 78-88: get_instance currently returns a previously-created
nosql.instance even if later calls pass different (database, url, credential),
which silently ignores new config; update get_instance (and mirror the change in
the GCP wrapper's get_instance(database)) to detect argument mismatches: when
nosql.instance already exists, compare the provided database/url/credential
against stored values (store them on the instance, e.g.,
instance._database/_url/_credential) and throw an Error if they differ (or
alternatively explicitly ignore args by documenting and returning instance) so
callers aren't silently misconfigured; modify the constructor usage path that
creates nosql.instance to set those stored fields and implement the comparison
in get_instance.
In `@benchmarks/wrappers/gcp/nodejs/nosql.js`:
- Around line 47-58: The query() function assumes Datastore keys always have a
name and sets item[secondary_key_name] = item[KEY].name, which yields undefined
for numeric-id keys; update the loop in query (and reference the KEY symbol and
the query method) to set item[secondary_key_name] to item[KEY].name ??
item[KEY].id (or otherwise prefer name and fall back to id) so both named and
auto‑allocated id keys are handled.
- Around line 23-31: The update() method performs a non-atomic read-modify-write
(get() → Object.assign() → save()) which can cause lost updates under
concurrency; change it to use Datastore transactions or a conditional/CAS
mutation: within update(), use this._client.runTransaction(...) (or the client's
equivalent) to fetch the entity by this._get_entity_key(...), apply
Object.assign(res, updates) inside the transaction, then save/commit the entity
so the read-and-write are atomic, or alternatively use an if-match/CAS-style
update API if available on this._client; ensure errors from transaction
conflicts are retried or surfaced appropriately.
In `@configs/systems.json`:
- Around line 384-392: Update the "nodejs" -> "base_images" -> "x64" map to
remove EOL runtimes and add a current LTS: remove the "10", "12", and "14" keys
(and their values "openwhisk/action-nodejs-v10", "-v12", "-v14"), keep supported
entries (e.g., "18" and "20") and add a "22" key pointing to
"openwhisk/action-nodejs-v22" (or the correct official image name if different);
ensure any consumers of these keys handle the new set of versions.
- Around line 404-407: Replace the pinned legacy aws-sdk v2 entry in the
"packages" object with the modular AWS SDK v3 packages and update the OpenWhisk
Node.js wrapper to use them: remove "aws-sdk" and add "@aws-sdk/client-dynamodb"
and "@aws-sdk/lib-dynamodb" in configs/systems.json, then update
benchmarks/wrappers/openwhisk/nodejs/nosql.js to import { DynamoDBClient } from
"@aws-sdk/client-dynamodb" and { DynamoDBDocumentClient, PutCommand, GetCommand,
QueryCommand, etc. } from "@aws-sdk/lib-dynamodb", instantiate DynamoDBClient
and wrap it with DynamoDBDocumentClient (replacing any
AWS.DynamoDB.DocumentClient usage), and refactor the wrapper functions (the code
paths that call DocumentClient methods) to use the new client
commands/respective method calls.
In `@dockerfiles/aws/nodejs/Dockerfile.build`:
- Line 6: Update the Dockerfile.build RUN step that currently reads "RUN dnf
swap -y curl-minimal curl && dnf install -y shadow-utils" to clean up package
metadata and caches after installation; append commands such as "&& dnf clean
all && rm -rf /var/cache/dnf /var/cache/yum" (or equivalent) to that RUN line so
the layer does not retain downloaded RPMs and metadata.
In `@sebs/aws/s3.py`:
- Around line 233-239: The except block that catches
self.client.exceptions.ClientError and raises a RuntimeError currently uses
"from None", which discards the original ClientError context; change the
exception chaining to "from error" so the RuntimeError preserves the original
traceback and AWS response metadata (i.e., update the except handling around
self.client.download_file to re-raise RuntimeError(... ) from error instead of
from None).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a673c626-fd5a-4321-8a27-1d098c17be01
📒 Files selected for processing (16)
benchmarks/100.webapps/130.crud-api/nodejs/function.jsbenchmarks/100.webapps/130.crud-api/nodejs/package.jsonbenchmarks/200.multimedia/210.thumbnailer/nodejs/package.json.22benchmarks/200.multimedia/210.thumbnailer/nodejs/package.json.24benchmarks/300.utilities/311.compression/nodejs/function.jsbenchmarks/300.utilities/311.compression/nodejs/package.jsonbenchmarks/wrappers/aws/nodejs/nosql.jsbenchmarks/wrappers/aws/nodejs/storage.jsbenchmarks/wrappers/azure/nodejs/nosql.jsbenchmarks/wrappers/gcp/nodejs/nosql.jsbenchmarks/wrappers/local/nodejs/nosql.jsbenchmarks/wrappers/openwhisk/nodejs/nosql.jsconfigs/nodejs.jsonconfigs/systems.jsondockerfiles/aws/nodejs/Dockerfile.buildsebs/aws/s3.py
✅ Files skipped from review due to trivial changes (5)
- configs/nodejs.json
- benchmarks/200.multimedia/210.thumbnailer/nodejs/package.json.22
- benchmarks/200.multimedia/210.thumbnailer/nodejs/package.json.24
- benchmarks/100.webapps/130.crud-api/nodejs/package.json
- benchmarks/300.utilities/311.compression/nodejs/package.json
| // Download directory from storage | ||
| const s3_download_begin = Date.now(); | ||
| await storage_handler.download_directory(bucket, path.join(input_prefix, key), download_path); | ||
| const s3_download_stop = Date.now(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether download_directory is defined in any Node.js storage wrapper
fd -e js storage.js benchmarks/wrappers | xargs -I{} sh -c 'echo "=== {} ==="; grep -nE "download_directory|downloadDirectory" "{}" || echo "NOT FOUND"'Repository: spcl/serverless-benchmarks
Length of output: 379
🏁 Script executed:
# Check what methods are actually defined in one storage wrapper (AWS as representative)
echo "=== AWS storage.js exports ===" && grep -nE "^(exports\.|module\.exports|async function|function )" benchmarks/wrappers/aws/nodejs/storage.js | head -20
echo -e "\n=== Check function.js around lines 100-103 ===" && sed -n '95,115p' benchmarks/300.utilities/311.compression/nodejs/function.js
echo -e "\n=== Search for download_directory usage in entire codebase ===" && rg "download_directory|downloadDirectory" --type jsRepository: spcl/serverless-benchmarks
Length of output: 1139
🏁 Script executed:
# Get the full AWS storage.js to see what methods are defined
echo "=== AWS storage.js full content ===" && cat benchmarks/wrappers/aws/nodejs/storage.js
echo -e "\n=== Verify exact line numbers in function.js ===" && cat -n benchmarks/300.utilities/311.compression/nodejs/function.js | sed -n '95,120p'Repository: spcl/serverless-benchmarks
Length of output: 3243
storage_handler.download_directory is not defined on any storage wrapper.
benchmarks/wrappers/aws/nodejs/storage.js (and all other providers: Azure, GCP, local, OpenWhisk) only expose upload, download, uploadStream, downloadStream, and unique_name. The call at line 102 will throw TypeError: storage_handler.download_directory is not a function at runtime.
Either add download_directory to each provider's storage wrapper or list all S3 objects under path.join(input_prefix, key) and download them individually via storage_handler.download.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@benchmarks/300.utilities/311.compression/nodejs/function.js` around lines 100
- 103, The call to storage_handler.download_directory (in function.js) is
invalid because storage wrappers only implement upload, download, uploadStream,
downloadStream, and unique_name; replace the download_directory usage by listing
all objects under path.join(input_prefix, key) (using the provider-specific
listing utility or S3 list API) and loop over each object calling
storage_handler.download for each target path, or alternatively add a
download_directory method to every storage wrapper (wrappers like
benchmarks/wrappers/aws/nodejs/storage.js, azure, gcp, local, openwhisk)
implementing recursive listing+download so storage_handler.download_directory
exists; update references to storage_handler.download_directory or add the
method consistently across all wrappers so runtime TypeError is avoided.
| const { DynamoDBDocument } = require("@aws-sdk/lib-dynamodb"); | ||
| const { DynamoDB } = require("@aws-sdk/client-dynamodb"); | ||
|
|
||
| class nosql { | ||
| constructor() { | ||
| this.client = DynamoDBDocument.from(new DynamoDB()); | ||
| this._tables = {}; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether the AWS SDK v3 packages are declared as deps anywhere in the benchmark tree
rg -nP '"@aws-sdk/(lib-dynamodb|client-dynamodb)"' --type=json
# Confirm no package.json near this wrapper adds them
fd -t f 'package.json' benchmarks/wrappers/aws/nodejs benchmarks/100.webappsRepository: spcl/serverless-benchmarks
Length of output: 241
🌐 Web query:
Are @aws-sdk/client-dynamodband@aws-sdk/lib-dynamodb preinstalled in the AWS Lambda Node.js 20/22/24 managed runtimes in 2026?
💡 Result:
No, @aws-sdk/client-dynamodb and @aws-sdk/lib-dynamodb are not preinstalled in the AWS Lambda Node.js 20/22/24 managed runtimes. The runtimes include select @aws-sdk/* packages in /var/runtime/node_modules (e.g., core packages and some client-* like client-s3, client-lambda), but AWS recommends bundling specific clients like client-dynamodb in your deployment package or layers for control and compatibility. Official docs confirm AWS SDK v3 is included but advise including needed modules yourself, as the runtime provides only a convenience subset, not all clients. StackOverflow inspection of Node.js 20 image shows many client-* but not explicitly client-dynamodb or lib-dynamodb; lib-dynamodb is a separate lib package typically bundled with client-dynamodb.
Citations:
- 1: https://stackoverflow.com/questions/53566478/which-npm-modules-are-preinstalled-in-aws-lambda-execution-environment
- 2: https://docs.aws.amazon.com/lambda/latest/dg/lambda-nodejs.html
- 3: https://github.com/aws/aws-sdk-js-v3/blob/v3.1022.0/supplemental-docs/AWS_LAMBDA.md
- 4: https://docs.aws.amazon.com/lambda/latest/operatorguide/sdks-functions.html
- 5: https://docs.aws.amazon.com/lambda/latest/dg/lambda-releases.html
These AWS SDK v3 packages are not preinstalled in Lambda Node.js 20/22/24 and must be explicitly bundled as dependencies.
The code imports @aws-sdk/lib-dynamodb and @aws-sdk/client-dynamodb, which are not included in the Lambda Node.js managed runtime (even as part of the convenience subset). These packages are not declared in any package.json in the benchmark tree, meaning the function will fail at runtime with a MODULE_NOT_FOUND error when attempting to require them. You must either:
- Add these as explicit dependencies in a package.json bundled with the deployment, or
- Ensure they're included in a Lambda layer
Additionally, new DynamoDB() with no region argument will rely on the AWS_REGION environment variable, which may not be available outside the Lambda execution environment.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@benchmarks/wrappers/aws/nodejs/nosql.js` around lines 7 - 14, The nosql
constructor imports and instantiates AWS SDK v3 modules (`@aws-sdk/lib-dynamodb`
and `@aws-sdk/client-dynamodb`) which are not preinstalled in Lambda Node.js
runtimes and will cause MODULE_NOT_FOUND at runtime; add these packages to the
deployment (declare them in package.json and bundle with the function or include
them in a Lambda layer) and update the DynamoDB instantiation in the nosql
constructor (the DynamoDBDocument.from(new DynamoDB()) call) to supply a region
fallback (use process.env.AWS_REGION || a sensible default) so the client does
not rely solely on Lambda-provided environment variables.
There was a problem hiding this comment.
@mcopik, you're right, and I apologize for the incorrect review comment. I've now confirmed that runtime versions nodejs18.x and later include version 3 of the SDK, which means @aws-sdk/client-dynamodb and @aws-sdk/lib-dynamodb are indeed available on disk in Lambda Node.js 20/22/24 managed runtimes without needing to be declared as explicit dependencies. The Node.js 22 runtime, for example, has version 3.806 of the AWS JavaScript SDK on disk. I'll note this for future reviews.
✏️ Learnings added
Learnt from: mcopik
Repo: spcl/serverless-benchmarks PR: 281
File: benchmarks/wrappers/aws/nodejs/nosql.js:7-14
Timestamp: 2026-04-23T13:21:16.277Z
Learning: In `spcl/serverless-benchmarks`, Lambda Node.js 18+ managed runtimes (nodejs18.x, nodejs20.x, nodejs22.x, nodejs24.x) include AWS SDK v3 pre-installed on disk (including `aws-sdk/client-dynamodb` and `aws-sdk/lib-dynamodb`). Do NOT flag these packages as missing dependencies in `benchmarks/wrappers/aws/nodejs/` code — they are available at runtime without needing to be declared in a `package.json`.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: mcopik
Repo: spcl/serverless-benchmarks PR: 281
File: benchmarks/100.webapps/130.crud-api/nodejs/function.js:1-4
Timestamp: 2026-04-23T13:20:33.872Z
Learning: In the `spcl/serverless-benchmarks` project, the `nosql` singleton is pre-initialized with required platform-specific parameters (database, url, credential) by the platform wrapper `handler.js` files (e.g., `benchmarks/wrappers/azure/nodejs/handler.js`, `benchmarks/wrappers/gcp/nodejs/handler.js`) at module load time before the benchmark function module is loaded. Therefore, a no-argument call to `nosql.nosql.get_instance()` inside a benchmark function (e.g., `benchmarks/100.webapps/130.crud-api/nodejs/function.js`) is intentional and safe — it returns the already-initialized singleton rather than creating a new one.
Learnt from: mcopik
Repo: spcl/serverless-benchmarks PR: 284
File: sebs/aws/config.py:349-351
Timestamp: 2026-04-15T15:34:29.897Z
Learning: In the `spcl/serverless-benchmarks` repository, `AWSResources._use_function_url` intentionally defaults to `True` in both the constructor and `initialize()` in `sebs/aws/config.py`. The maintainer (mcopik) wants Lambda Function URLs to be the new default trigger mechanism, replacing API Gateway. Do not flag this as a backward-compatibility issue.
| async update(table_name, primary_key, secondary_key, updates) { | ||
|
|
||
| const key = {}; | ||
| key[primary_key[0]] = primary_key[1]; | ||
| key[secondary_key[0]] = secondary_key[1]; | ||
|
|
||
| const update_names = {}; | ||
| const update_values = {}; | ||
| const update_expression = ["SET"]; | ||
| for (const [key_name, value] of Object.entries(updates)) { | ||
| update_expression.push(`#${key_name}_name = :${key_name}_value,`); | ||
| update_names[`#${key_name}_name`] = key_name; | ||
| update_values[`:${key_name}_value`] = value; | ||
| } | ||
| // remove trailing comma from the last assignment | ||
| update_expression[update_expression.length - 1] = update_expression[ | ||
| update_expression.length - 1 | ||
| ].slice(0, -1); | ||
|
|
||
| await this.client | ||
| .update({ | ||
| TableName: this._get_table(table_name), | ||
| Key: key, | ||
| UpdateExpression: update_expression.join(" "), | ||
| ExpressionAttributeNames: update_names, | ||
| ExpressionAttributeValues: update_values, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Missing empty-updates guard — update() will send malformed UpdateExpression.
When updates is {}, update_expression stays as ["SET"]; the trailing-comma trimmer then slices "SET" to "SE" and DynamoDB rejects the request with a ValidationException. The sibling local/nodejs/nosql.js and openwhisk/nodejs/nosql.js already short-circuit on empty updates — please mirror that here for consistency.
🐛 Proposed fix
async update(table_name, primary_key, secondary_key, updates) {
+ if (Object.keys(updates).length === 0) {
+ return;
+ }
const key = {};
key[primary_key[0]] = primary_key[1];
key[secondary_key[0]] = secondary_key[1];📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async update(table_name, primary_key, secondary_key, updates) { | |
| const key = {}; | |
| key[primary_key[0]] = primary_key[1]; | |
| key[secondary_key[0]] = secondary_key[1]; | |
| const update_names = {}; | |
| const update_values = {}; | |
| const update_expression = ["SET"]; | |
| for (const [key_name, value] of Object.entries(updates)) { | |
| update_expression.push(`#${key_name}_name = :${key_name}_value,`); | |
| update_names[`#${key_name}_name`] = key_name; | |
| update_values[`:${key_name}_value`] = value; | |
| } | |
| // remove trailing comma from the last assignment | |
| update_expression[update_expression.length - 1] = update_expression[ | |
| update_expression.length - 1 | |
| ].slice(0, -1); | |
| await this.client | |
| .update({ | |
| TableName: this._get_table(table_name), | |
| Key: key, | |
| UpdateExpression: update_expression.join(" "), | |
| ExpressionAttributeNames: update_names, | |
| ExpressionAttributeValues: update_values, | |
| }); | |
| } | |
| async update(table_name, primary_key, secondary_key, updates) { | |
| if (Object.keys(updates).length === 0) { | |
| return; | |
| } | |
| const key = {}; | |
| key[primary_key[0]] = primary_key[1]; | |
| key[secondary_key[0]] = secondary_key[1]; | |
| const update_names = {}; | |
| const update_values = {}; | |
| const update_expression = ["SET"]; | |
| for (const [key_name, value] of Object.entries(updates)) { | |
| update_expression.push(`#${key_name}_name = :${key_name}_value,`); | |
| update_names[`#${key_name}_name`] = key_name; | |
| update_values[`:${key_name}_value`] = value; | |
| } | |
| // remove trailing comma from the last assignment | |
| update_expression[update_expression.length - 1] = update_expression[ | |
| update_expression.length - 1 | |
| ].slice(0, -1); | |
| await this.client | |
| .update({ | |
| TableName: this._get_table(table_name), | |
| Key: key, | |
| UpdateExpression: update_expression.join(" "), | |
| ExpressionAttributeNames: update_names, | |
| ExpressionAttributeValues: update_values, | |
| }); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@benchmarks/wrappers/aws/nodejs/nosql.js` around lines 46 - 73, The update()
method builds an UpdateExpression even when updates is empty, which leaves
update_expression as ["SET"] and then mangles it into "SE" causing DynamoDB
ValidationException; fix by short-circuiting when updates is empty (e.g., if
(!updates || Object.keys(updates).length === 0) return) before constructing
update_expression so this.client.update(...) is not called with an invalid
UpdateExpression; update the update() function (and any relevant callers) to
mirror the short-circuit behavior used in the sibling local/openwhisk wrappers
and return early without calling this.client.update or _get_table when there are
no updates to apply.
| async get(table_name, primary_key, secondary_key) { | ||
| const { resource } = await this._get_table(table_name) | ||
| .item(secondary_key[1], primary_key[1]) | ||
| .read(); | ||
|
|
||
| if (!resource) { | ||
| return null; | ||
| } | ||
|
|
||
| resource[secondary_key[0]] = secondary_key[1]; | ||
|
|
||
| // remove Azure-specific fields | ||
| delete resource.id; | ||
| delete resource._etag; | ||
| delete resource._rid; | ||
| delete resource._self; | ||
| delete resource._ts; | ||
| delete resource._attachments; | ||
|
|
||
| return resource; | ||
| } |
There was a problem hiding this comment.
Edge case: get() loses the value when secondary_key[0] === "id".
Line 36 sets resource[secondary_key[0]] = secondary_key[1]; line 39 unconditionally runs delete resource.id. If a benchmark ever uses the literal string "id" as the secondary key field name (a reasonable choice when the Cosmos container's id happens to be the intended field), Line 39 will drop the value that was just written. Guard with a name check:
🛡️ Proposed fix
resource[secondary_key[0]] = secondary_key[1];
// remove Azure-specific fields
- delete resource.id;
+ if (secondary_key[0] !== "id") {
+ delete resource.id;
+ }
delete resource._etag;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async get(table_name, primary_key, secondary_key) { | |
| const { resource } = await this._get_table(table_name) | |
| .item(secondary_key[1], primary_key[1]) | |
| .read(); | |
| if (!resource) { | |
| return null; | |
| } | |
| resource[secondary_key[0]] = secondary_key[1]; | |
| // remove Azure-specific fields | |
| delete resource.id; | |
| delete resource._etag; | |
| delete resource._rid; | |
| delete resource._self; | |
| delete resource._ts; | |
| delete resource._attachments; | |
| return resource; | |
| } | |
| async get(table_name, primary_key, secondary_key) { | |
| const { resource } = await this._get_table(table_name) | |
| .item(secondary_key[1], primary_key[1]) | |
| .read(); | |
| if (!resource) { | |
| return null; | |
| } | |
| resource[secondary_key[0]] = secondary_key[1]; | |
| // remove Azure-specific fields | |
| if (secondary_key[0] !== "id") { | |
| delete resource.id; | |
| } | |
| delete resource._etag; | |
| delete resource._rid; | |
| delete resource._self; | |
| delete resource._ts; | |
| delete resource._attachments; | |
| return resource; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@benchmarks/wrappers/azure/nodejs/nosql.js` around lines 27 - 47, In the get
method, assigning resource[secondary_key[0]] = secondary_key[1] followed by
unconditionally deleting resource.id can drop the value when secondary_key[0]
=== "id"; update the cleanup logic inside async get(...) so the deletion of the
Azure-specific id field is conditional (only delete resource.id when
secondary_key[0] !== "id") and leave the assigned value intact; reference the
get function and the secondary_key variable when making this change.
| const aws = require("aws-sdk"); | ||
|
|
||
| class nosql { | ||
| constructor() { | ||
| if (process.env.NOSQL_STORAGE_TYPE !== "scylladb") { | ||
| throw new Error(`Unsupported NoSQL storage type: ${process.env.NOSQL_STORAGE_TYPE}!`); | ||
| } | ||
| this.client = new aws.DynamoDB.DocumentClient({ | ||
| region: "None", | ||
| accessKeyId: "None", | ||
| secretAccessKey: "None", | ||
| endpoint: `http://${process.env.NOSQL_STORAGE_ENDPOINT}`, | ||
| maxRetries: 0, | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🌐 Web query:
Is aws-sdk v2 for JavaScript in maintenance mode in 2026, and what is the recommended replacement?
💡 Result:
No, AWS SDK v2 for JavaScript is not in maintenance mode in 2026. It entered maintenance mode on September 8, 2024, and reached end-of-support on September 8, 2025. As of 2026-04-22, it is in end-of-support phase, receiving no updates or releases, though existing applications continue to function unless AWS services change fundamentally. The recommended replacement is AWS SDK for JavaScript v3, which offers improved features, performance, and ongoing support. Migration resources include automated codemod scripts and official guides.
Citations:
- 1: https://aws.amazon.com/blogs/developer/announcing-end-of-support-for-aws-sdk-for-javascript-v2/
- 2: https://docs.aws.amazon.com/sdk-for-javascript/v3/developer-guide/migrating.html
- 3: AWS SDK for JavaScript v2 is in Maintenance Mode aws/aws-sdk-js#4679
- 4: AWS SDK for JavaScript v2 Upcoming End-of-Support aws/aws-sdk-js#4601
- 5: https://github.com/aws/aws-sdk-js
🏁 Script executed:
# Verify the code snippet exists and check context around the hardcoded endpoint
if [ -f "benchmarks/wrappers/local/nodejs/nosql.js" ]; then
echo "=== File found, checking lines 3-16 and context ==="
sed -n '1,25p' benchmarks/wrappers/local/nodejs/nosql.js
else
echo "File not found at expected location"
find . -name "nosql.js" -type f 2>/dev/null | head -5
fiRepository: spcl/serverless-benchmarks
Length of output: 911
🏁 Script executed:
# Check the aws wrapper for comparison
if [ -f "benchmarks/wrappers/aws/nodejs/nosql.js" ]; then
echo "=== AWS wrapper for comparison ==="
sed -n '1,30p' benchmarks/wrappers/aws/nodejs/nosql.js
fiRepository: spcl/serverless-benchmarks
Length of output: 969
aws-sdk v2 reached end-of-support in September 2025 — migrate to v3.
aws-sdk v2 is no longer receiving any updates or support as of September 8, 2025. For this wrapper, migrate to @aws-sdk/client-dynamodb + @aws-sdk/lib-dynamodb (as benchmarks/wrappers/aws/nodejs/nosql.js already does), which eliminates the .promise() calls and keeps the local wrapper consistent.
Additionally, hardcoding http:// in the endpoint at line 14 forces callers to set NOSQL_STORAGE_ENDPOINT without a scheme and precludes HTTPS. If the env var accidentally includes a scheme, the URL becomes http://http://…. Either validate/strip the scheme or accept a full URL.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@benchmarks/wrappers/local/nodejs/nosql.js` around lines 3 - 16, The nosql
class constructor currently uses the deprecated aws-sdk v2 and hardcodes
"http://" into the endpoint; update the constructor to use the v3 clients
(`@aws-sdk/client-dynamodb` and `@aws-sdk/lib-dynamodb`) to create a
DynamoDBDocumentClient instead of aws.DynamoDB.DocumentClient and remove any
.promise() usage elsewhere, and change the endpoint handling in the constructor
to accept a full NOSQL_STORAGE_ENDPOINT URL (or validate/strip any existing
scheme) before passing it to the client so you don't prepend "http://" blindly;
locate the code in the nosql class constructor and replace the client
initialization and endpoint composition accordingly.
| "24": "amazon/aws-lambda-nodejs:24.2026.04.22.11-x86_64", | ||
| "22": "amazon/aws-lambda-nodejs:22.2026.04.22.11-x86_64", | ||
| "20": "amazon/aws-lambda-nodejs:20.2026.04.22.11-x86_64" | ||
| }, | ||
| "arm64": { | ||
| "16": "amazon/aws-lambda-nodejs:16.2024.09.06.13" | ||
| "24": "amazon/aws-lambda-nodejs:24.2026.04.22.11-arm64", | ||
| "22": "amazon/aws-lambda-nodejs:22.2026.04.22.11-arm64", | ||
| "20": "amazon/aws-lambda-nodejs:20.2026.04.22.11-arm64" | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm the tags exist on the public mirror
for tag in 20.2026.04.22.11-x86_64 22.2026.04.22.11-x86_64 24.2026.04.22.11-x86_64 \
20.2026.04.22.11-arm64 22.2026.04.22.11-arm64 24.2026.04.22.11-arm64; do
echo "=== $tag ==="
curl -sI "https://public.ecr.aws/v2/lambda/nodejs/manifests/${tag}" | head -n 5
doneRepository: spcl/serverless-benchmarks
Length of output: 1241
🌐 Web query:
AWS ECR public registry nodejs lambda image tags 2026.04.22
💡 Result:
The AWS ECR Public registry hosts official AWS Lambda base images for Node.js at public.ecr.aws/lambda/nodejs. These are used to build custom container images for AWS Lambda functions. As of 2026-04-22 (post recent updates around 2026-03-31), available tags include Node.js versions 18, 20, 22, 24 for x86_64 and arm64 architectures. Tags follow formats like: - MAJOR.MINOR[-x86_64 | -arm64] (e.g., 22-x86_64, 24-arm64) - MAJOR.YEAR.MM.DD.HH[-x86_64 | -arm64] (e.g., 18.2026.03.31.11-x86_64, 22.2026.03.31.11-arm64) - latest-x86_64, latest-arm64 Example Dockerfile usage: FROM public.ecr.aws/lambda/nodejs:22 These images include Amazon Linux, Node.js runtime, dependencies, AWS SDK, and Lambda Runtime Interface Client (RIC). They receive regular security updates. Full tag list and pulls available on Docker Hub mirror at amazon/aws-lambda-nodejs. Browse: https://gallery.ecr.aws/lambda/nodejs Tags: https://hub.docker.com/r/amazon/aws-lambda-nodejs/tags Source: https://github.com/aws/aws-lambda-base-images
Citations:
- 1: https://gallery.ecr.aws/lambda/nodejs
- 2: https://registry.hub.docker.com/r/amazon/aws-lambda-nodejs
- 3: https://github.com/aws/aws-lambda-base-images
- 4: https://hub.docker.com/r/amazon/aws-lambda-nodejs
- 5: https://hub.docker.com/r/amazon/aws-lambda-nodejs/tags
- 6: https://hub.docker.com/r/amazon/aws-lambda-nodejs/tags?name=14&page=1
🏁 Script executed:
#!/bin/bash
# Try alternative approaches to verify the image tags exist
# 1. Check if docker can see the image metadata (without pulling)
# 2. Try listing available tags via ECR API with proper endpoint
# Try querying the ECR public registry list endpoint
echo "=== Attempt 1: Query ECR list tags endpoint ==="
curl -sI "https://public.ecr.aws/v2/lambda/nodejs/tags/list" | head -n 5
# Try standard registry API without /v2 prefix
echo -e "\n=== Attempt 2: Check base registry endpoint ==="
curl -sI "https://public.ecr.aws/lambda/nodejs" | head -n 5
# Check what tags are actually available (from public registry info)
echo -e "\n=== Attempt 3: Check public ECR documentation ==="
curl -s "https://gallery.ecr.aws/lambda/nodejs" 2>&1 | head -n 50Repository: spcl/serverless-benchmarks
Length of output: 2023
🏁 Script executed:
#!/bin/bash
# Check Docker Hub registry for the specific tags
# Docker Hub API endpoint for listing tags
echo "=== Checking Docker Hub for amazon/aws-lambda-nodejs tags ==="
curl -s "https://registry.hub.docker.com/v2/repositories/amazon/aws-lambda-nodejs/tags/" | head -c 2000Repository: spcl/serverless-benchmarks
Length of output: 173
🌐 Web query:
amazon aws-lambda-nodejs 2026.04.22 tag release docker hub
💡 Result:
As of 2026-04-22, no Docker Hub tag for amazon/aws-lambda-nodejs matching exactly "2026.04.22" exists based on available search results. The most recent tags listed on Docker Hub (pushed within the last 1-6 days relative to search data) are from March 2026, such as: - 22.2026.03.31.11-x86_64 (latest-x86_64, Node.js 22, Amazon Linux 2023) - 24.2026.03.31.11-x86_64 (Node.js 24) - 18.2026.03.31.11-x86_64 (Node.js 18) - 22.2026.03.31.11-arm64 (latest-arm64) Older tags include 18.2026.03.27.21-x86_64 (pushed 1 day ago) and various from 2026.03.23 and earlier. A tag like 22.2026.04.12.14-x86_64 is referenced in layer details, indicating an April 2026 update exists but not shown in top pages. AWS regularly updates these base images for security patches (no formal release notes found; check GitHub aws/aws-lambda-base-images branches for Dockerfiles). Node.js 20 reaches EOL April 30, 2026; prefer 22.x or 24.x. Use public.ecr.aws/lambda/nodejs: for primary deploys. To pull latest: docker pull amazon/aws-lambda-nodejs:latest-x86_64 or :latest-arm64.
Citations:
- 1: https://docs.aws.amazon.com/lambda/latest/dg/lambda-runtimes.html
- 2: https://hub.docker.com/r/amazon/aws-lambda-nodejs/tags
- 3: https://hub.docker.com/r/amazon/aws-lambda-nodejs/tags?name=14&page=1
- 4: https://registry.hub.docker.com/r/amazon/aws-lambda-nodejs
- 5: https://hub.docker.com/r/amazon/aws-lambda-nodejs
- 6: https://github.com/aws/aws-lambda-base-images
- 7: https://docs.aws.amazon.com/lambda/latest/dg/nodejs-image.html
- 8: https://hub.docker.com/layers/amazon/aws-lambda-nodejs/22.2026.04.12.14-x86_64/images/sha256-1c882ee47e5a6512e282fa9c017441dfd5731ae0566321bffa356d1026ccb780
These image tags do not appear to be available yet—use published tags instead.
The tags amazon/aws-lambda-nodejs:*.2026.04.22.11-[x86_64|arm64] are not found in the public Docker Hub or ECR registry. The most recent available tags are from 2026-03-31 (e.g., 22.2026.03.31.11-x86_64). Using unpublished tags will cause 404 errors during container pulls in CI.
Replace these with either:
- Known published tags:
20.2026.03.31.11-x86_64|arm64,22.2026.03.31.11-x86_64|arm64,24.2026.03.31.11-x86_64|arm64 - Or version-only tags (always available):
20,22,24
Also note: Node.js 20 reaches EOL on 2026-04-30; migrate to 22.x or 24.x.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@configs/systems.json` around lines 106 - 114, Replace the unpublished image
tags (e.g., "amazon/aws-lambda-nodejs:24.2026.04.22.11-x86_64",
"22.2026.04.22.11-arm64", etc.) under the x86_64 and arm64 objects with
published tags or stable version-only tags; either use the published 2026-03-31
tags ("20.2026.03.31.11-<arch>", "22.2026.03.31.11-<arch>",
"24.2026.03.31.11-<arch>") or the simple version tags ("20", "22", "24") to
avoid 404s during pulls—update each value string in the x86_64 and arm64 entries
accordingly and prefer 22 or 24 (not 20) given Node.js 20 EOL.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
benchmarks/wrappers/gcp/nodejs/nosql.js (3)
38-49:updatehas a read-modify-write race.
getfollowed bysaveis not atomic — concurrent invocations can clobber each other's partial updates. Given this is a benchmark wrapper, that may be acceptable, but if the benchmark scenarios exercise concurrent updates against the same entity, consider wrapping inthis._client.runInTransaction(...)(or usingrunTransactionwithget+save) to match typical production semantics. Not blocking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@benchmarks/wrappers/gcp/nodejs/nosql.js` around lines 38 - 49, The current update method (update) does a non-atomic read-modify-write using this._client.get followed by this._client.save, which can cause races; change it to perform the read and write inside a datastore transaction (e.g., this._client.runInTransaction or runTransaction) so the fetch, Object.assign(res, updates) and save occur atomically and retry on conflicts; locate the update function and replace the standalone get/save sequence with a transactional callback that gets the entity, merges updates, and saves within the transaction.
3-3: UnusedKEYdestructured import.
KEYis imported but never referenced directly — line 71 usesDatastore.KEYinstead. Either drop the destructure or use the imported symbol for consistency.Proposed diff
-const { Datastore, KEY } = require("@google-cloud/datastore"); +const { Datastore } = require("@google-cloud/datastore");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@benchmarks/wrappers/gcp/nodejs/nosql.js` at line 3, The import destructures KEY but the code uses Datastore.KEY later; either remove KEY from the destructure or replace all usages of Datastore.KEY with the destructured KEY. Update the top-level import in the module (the line with const { Datastore, KEY } = require("@google-cloud/datastore")) to remove KEY if you opt to use Datastore.KEY, or change occurrences of Datastore.KEY in the code (e.g., where keys are created at line ~71) to use the destructured KEY for consistency.
64-75: Asymmetricquerysignature vs other methods.
insert/get/deletetake asecondary_keytuple[name, value], butquerytakes a baresecondary_key_namestring. Works, but it's easy to pass the wrong shape by mistake. Consider accepting the same tuple form (and just ignoring the value) for API consistency, or renaming parameters across the class to make the distinction obvious.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@benchmarks/wrappers/gcp/nodejs/nosql.js` around lines 64 - 75, The query method's signature is inconsistent with insert/get/delete: those expect a secondary_key tuple [name, value] while query currently accepts a secondary_key_name string, causing potential misuse; update the async query(table_name, primary_key, secondary_key) in the class to accept the same tuple shape, use secondary_key[0] as the field name (ignore the value), and set item[secondary_key[0]] = item[Datastore.KEY].name so the API is uniform, then update any internal call sites to pass the same tuple shape (or alternatively rename parameters consistently across the class if you prefer that style).benchmarks/wrappers/azure/nodejs/handler.js (1)
5-12: Startup-time NoSQL init looks correct.The call signature matches
get_instance(database, url, credential)inbenchmarks/wrappers/azure/nodejs/nosql.js. One small thought:'NOSQL_STORAGE_DATABASE' in process.envevaluates true even when the value is empty, in which caseget_instancewill then throw for missing URL/credentials. If you want a softer failure mode during cold starts, consider a truthiness check (process.env.NOSQL_STORAGE_DATABASE) so an empty value is simply ignored. Not blocking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@benchmarks/wrappers/azure/nodejs/handler.js` around lines 5 - 12, Change the environment check in the startup NoSQL init so an empty NOSQL_STORAGE_DATABASE does not trigger get_instance; replace the current "'NOSQL_STORAGE_DATABASE' in process.env" check with a truthiness test on process.env.NOSQL_STORAGE_DATABASE so the block (which calls nosql.nosql.get_instance(database, url, credential)) only runs when a non-empty database value is present and avoids throwing for missing URL/credentials during cold starts.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@sebs/regression.py`:
- Line 52: The Node.js regression list variable benchmarks_nodejs is missing the
311.compression benchmark; update the benchmarks_nodejs list in
sebs/regression.py (the variable named benchmarks_nodejs) to include
"311.compression" alongside the other entries so this benchmark is included in
all Node.js provider tests.
---
Nitpick comments:
In `@benchmarks/wrappers/azure/nodejs/handler.js`:
- Around line 5-12: Change the environment check in the startup NoSQL init so an
empty NOSQL_STORAGE_DATABASE does not trigger get_instance; replace the current
"'NOSQL_STORAGE_DATABASE' in process.env" check with a truthiness test on
process.env.NOSQL_STORAGE_DATABASE so the block (which calls
nosql.nosql.get_instance(database, url, credential)) only runs when a non-empty
database value is present and avoids throwing for missing URL/credentials during
cold starts.
In `@benchmarks/wrappers/gcp/nodejs/nosql.js`:
- Around line 38-49: The current update method (update) does a non-atomic
read-modify-write using this._client.get followed by this._client.save, which
can cause races; change it to perform the read and write inside a datastore
transaction (e.g., this._client.runInTransaction or runTransaction) so the
fetch, Object.assign(res, updates) and save occur atomically and retry on
conflicts; locate the update function and replace the standalone get/save
sequence with a transactional callback that gets the entity, merges updates, and
saves within the transaction.
- Line 3: The import destructures KEY but the code uses Datastore.KEY later;
either remove KEY from the destructure or replace all usages of Datastore.KEY
with the destructured KEY. Update the top-level import in the module (the line
with const { Datastore, KEY } = require("@google-cloud/datastore")) to remove
KEY if you opt to use Datastore.KEY, or change occurrences of Datastore.KEY in
the code (e.g., where keys are created at line ~71) to use the destructured KEY
for consistency.
- Around line 64-75: The query method's signature is inconsistent with
insert/get/delete: those expect a secondary_key tuple [name, value] while query
currently accepts a secondary_key_name string, causing potential misuse; update
the async query(table_name, primary_key, secondary_key) in the class to accept
the same tuple shape, use secondary_key[0] as the field name (ignore the value),
and set item[secondary_key[0]] = item[Datastore.KEY].name so the API is uniform,
then update any internal call sites to pass the same tuple shape (or
alternatively rename parameters consistently across the class if you prefer that
style).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b255500a-2eba-463f-ac02-e72470240285
📒 Files selected for processing (6)
.github/workflows/regression.ymlbenchmarks/wrappers/aws/nodejs/nosql.jsbenchmarks/wrappers/azure/nodejs/handler.jsbenchmarks/wrappers/gcp/nodejs/handler.jsbenchmarks/wrappers/gcp/nodejs/nosql.jssebs/regression.py
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (1)
benchmarks/300.utilities/311.compression/nodejs/function.js (1)
73-103:⚠️ Potential issue | 🟠 MajorClean up the
/tmpworkspace after upload or failure.The handler creates a unique temp directory but never removes it. Warm invocations can accumulate downloaded inputs and zip files until
/tmpfills up.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@benchmarks/300.utilities/311.compression/nodejs/function.js` around lines 73 - 103, The temp workspace created at download_path is never removed; wrap the workflow that starts after fs.mkdirSync (the calls to storage_handler.downloadDirectory, parseDirectory, createZipArchive, and storage_handler.upload that produce archive_path) in a try/finally and remove the download_path recursively in the finally block (e.g., fs.rmSync or fs.promises.rm with { recursive: true, force: true }); ensure any error is rethrown after cleanup so failures also free the /tmp directory.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@benchmarks/000.microbenchmarks/010.sleep/nodejs/function.js`:
- Line 7: The wrapper currently returns an object ({ result: sleep }) which
makes callers read result.result; change the function to return the numeric
benchmark value directly by returning the sleep variable instead of an object so
the regression path/validation receives the numeric sleep value (replace the
return in the function that currently returns {result: sleep} to return sleep).
In `@benchmarks/100.webapps/110.dynamic-html/nodejs/function.js`:
- Line 26: The Promise is resolving an object with a nested result ({ result:
Mustache.render(...) }) causing the runtime wrapper to double-wrap the response;
change the resolver in this handler so it resolves the rendered HTML directly
(use Mustache.render(data, input) as the resolved value instead of wrapping it
in { result: ... }) — locate the call to resolve(...) that contains
Mustache.render and return the rendered string directly.
In `@benchmarks/200.multimedia/210.thumbnailer/nodejs/function.js`:
- Line 28: The function currently returns an extra nested result object (return
{result: {bucket: output_prefix, key: uploadName}}), which makes provider
wrappers see result.result; change the return to return the thumbnail info
directly by returning an object with bucket and key (use output_prefix and
uploadName) so callers receive {bucket, key} instead of {result: {bucket, key}};
update any local references expecting the nested layer if present.
In `@benchmarks/300.utilities/311.compression/nodejs/function.js`:
- Around line 41-63: The Promise currently listens for archive errors but not
errors from the write stream created by fs.createWriteStream(outputPath)
(variable output), which can cause hangs/crashes; add an output.on('error',
(err) => reject(err)) handler alongside the existing archive.on('error', ...)
and ensure you don't double-resolve by keeping the existing output.on('close',
() => resolve()) behavior; update the block where output, archive,
archive.pipe(output), and archive.finalize() are used to attach the output error
listener for the write stream.
- Around line 73-89: The code uses the raw storage key when building local paths
(archive_name/archive_path), which breaks for keys containing slashes; change
usages so local filenames are derived from a safe/sanitized form of key (e.g.,
use path.basename(key) or replace path separators with an underscore) and ensure
archive_path is always created under download_path before calling
createZipArchive; update the archive_name and archive_path construction
(references: archive_name, archive_path, key, download_path) and keep
storage_handler.downloadDirectory and parseDirectory behavior the same so
downloads still land in download_path.
In `@benchmarks/wrappers/aws/nodejs/storage.js`:
- Around line 39-53: The downloadDirectory function currently calls
this.S3.send(new ListObjectsV2Command(...)) once and only processes
response.Contents; change it to loop and paginate using response.IsTruncated and
response.NextContinuationToken (call ListObjectsV2Command with ContinuationToken
until IsTruncated is false), accumulate all Contents into a single array, then
create directories and call this.download for each Key as before (keep using
downloadDirectory, ListObjectsV2Command, this.S3.send, response.IsTruncated,
response.NextContinuationToken, and this.download to locate the logic).
---
Duplicate comments:
In `@benchmarks/300.utilities/311.compression/nodejs/function.js`:
- Around line 73-103: The temp workspace created at download_path is never
removed; wrap the workflow that starts after fs.mkdirSync (the calls to
storage_handler.downloadDirectory, parseDirectory, createZipArchive, and
storage_handler.upload that produce archive_path) in a try/finally and remove
the download_path recursively in the finally block (e.g., fs.rmSync or
fs.promises.rm with { recursive: true, force: true }); ensure any error is
rethrown after cleanup so failures also free the /tmp directory.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a924f8c0-9912-4cdc-ae87-eafb0dc45f41
📒 Files selected for processing (12)
benchmarks/000.microbenchmarks/010.sleep/nodejs/function.jsbenchmarks/100.webapps/110.dynamic-html/nodejs/function.jsbenchmarks/200.multimedia/210.thumbnailer/nodejs/function.jsbenchmarks/300.utilities/311.compression/nodejs/function.jsbenchmarks/300.utilities/311.compression/nodejs/package.jsonbenchmarks/wrappers/aws/nodejs/handler.jsbenchmarks/wrappers/aws/nodejs/storage.jsbenchmarks/wrappers/azure/nodejs/handler.jsbenchmarks/wrappers/azure/nodejs/storage.jsbenchmarks/wrappers/gcp/nodejs/handler.jsbenchmarks/wrappers/gcp/nodejs/storage.jssebs/regression.py
✅ Files skipped from review due to trivial changes (1)
- benchmarks/wrappers/aws/nodejs/handler.js
🚧 Files skipped from review as they are similar to previous changes (4)
- benchmarks/wrappers/gcp/nodejs/handler.js
- benchmarks/300.utilities/311.compression/nodejs/package.json
- benchmarks/wrappers/azure/nodejs/handler.js
- sebs/regression.py
| var sleep = event.sleep; | ||
| timer(sleep*1000); | ||
| return sleep; | ||
| return {result: sleep}; |
There was a problem hiding this comment.
Return the benchmark value directly to avoid result.result.
Line 7 makes the wrapper response contain result: { result: sleep }. The regression path extracts the wrapper-level result, so validation receives an object instead of the numeric sleep value.
🐛 Proposed fix
- return {result: sleep};
+ return sleep;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return {result: sleep}; | |
| return sleep; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@benchmarks/000.microbenchmarks/010.sleep/nodejs/function.js` at line 7, The
wrapper currently returns an object ({ result: sleep }) which makes callers read
result.result; change the function to return the numeric benchmark value
directly by returning the sleep variable instead of an object so the regression
path/validation receives the numeric sleep value (replace the return in the
function that currently returns {result: sleep} to return sleep).
| function(err, data) { | ||
| if(err) reject(err); | ||
| resolve(Mustache.render(data, input)); | ||
| resolve({result: Mustache.render(data, input)}); |
There was a problem hiding this comment.
Avoid nesting result inside the wrapper-level result.
Line 26 returns { result: html }, but the Node.js wrappers already place the handler return value under result. This makes validators receive { result: html } instead of the rendered HTML string.
🐛 Proposed fix
- resolve({result: Mustache.render(data, input)});
+ resolve(Mustache.render(data, input));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@benchmarks/100.webapps/110.dynamic-html/nodejs/function.js` at line 26, The
Promise is resolving an object with a nested result ({ result:
Mustache.render(...) }) causing the runtime wrapper to double-wrap the response;
change the resolver in this handler so it resolves the rendered HTML directly
(use Mustache.render(data, input) as the resolved value instead of wrapping it
in { result: ... }) — locate the call to resolve(...) that contains
Mustache.render and return the rendered string directly.
| ); | ||
| await promise; | ||
| return {bucket: output_prefix, key: uploadName} | ||
| return {result: {bucket: output_prefix, key: uploadName}} |
There was a problem hiding this comment.
Keep the thumbnail result flat for the provider wrappers.
Line 28 causes wrapper output to become result: { result: { bucket, key } }. Return the object directly unless every validator was updated to unwrap the extra layer.
🐛 Proposed fix
- return {result: {bucket: output_prefix, key: uploadName}}
+ return {bucket: output_prefix, key: uploadName};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return {result: {bucket: output_prefix, key: uploadName}} | |
| return {bucket: output_prefix, key: uploadName}; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@benchmarks/200.multimedia/210.thumbnailer/nodejs/function.js` at line 28, The
function currently returns an extra nested result object (return {result:
{bucket: output_prefix, key: uploadName}}), which makes provider wrappers see
result.result; change the return to return the thumbnail info directly by
returning an object with bucket and key (use output_prefix and uploadName) so
callers receive {bucket, key} instead of {result: {bucket, key}}; update any
local references expecting the nested layer if present.
| return new Promise((resolve, reject) => { | ||
| const output = fs.createWriteStream(outputPath); | ||
| const archive = archiver('zip', { | ||
| zlib: { level: 9 } // Maximum compression | ||
| }); | ||
|
|
||
| output.on('close', () => { | ||
| resolve(); | ||
| }); | ||
|
|
||
| archive.on('error', (err) => { | ||
| reject(err); | ||
| }); | ||
|
|
||
| archive.pipe(output); | ||
|
|
||
| // Add all files from the directory, excluding the archive itself | ||
| archive.glob('**/*', { | ||
| cwd: sourceDir, | ||
| ignore: [path.basename(outputPath)] | ||
| }); | ||
|
|
||
| archive.finalize(); |
There was a problem hiding this comment.
Reject on output stream errors.
Line 42 creates a write stream, but only the archive emits are handled. If the output stream fails, the invocation can crash or hang instead of rejecting the archive promise.
🐛 Proposed fix
output.on('close', () => {
resolve();
});
+ output.on('error', (err) => {
+ reject(err);
+ });
+
archive.on('error', (err) => {
reject(err);
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@benchmarks/300.utilities/311.compression/nodejs/function.js` around lines 41
- 63, The Promise currently listens for archive errors but not errors from the
write stream created by fs.createWriteStream(outputPath) (variable output),
which can cause hangs/crashes; add an output.on('error', (err) => reject(err))
handler alongside the existing archive.on('error', ...) and ensure you don't
double-resolve by keeping the existing output.on('close', () => resolve())
behavior; update the block where output, archive, archive.pipe(output), and
archive.finalize() are used to attach the output error listener for the write
stream.
| // Create unique download path | ||
| const download_path = path.join('/tmp', `${key}-${uuidv4()}`); | ||
| fs.mkdirSync(download_path, { recursive: true }); | ||
|
|
||
| // Download directory from storage | ||
| const s3_download_begin = Date.now(); | ||
| await storage_handler.downloadDirectory(bucket, path.join(input_prefix, key), download_path); | ||
| const s3_download_stop = Date.now(); | ||
|
|
||
| // Calculate size of downloaded files | ||
| const size = parseDirectory(download_path); | ||
|
|
||
| // Compress directory | ||
| const compress_begin = Date.now(); | ||
| const archive_name = `${key}.zip`; | ||
| const archive_path = path.join(download_path, archive_name); | ||
| await createZipArchive(download_path, archive_path); |
There was a problem hiding this comment.
Do not use the storage key as a local filesystem path.
Lines 74 and 87 use key directly for local paths. A key like foo/bar makes archive_path point to a nested directory that may not exist, causing archive creation to fail.
🐛 Proposed fix
// Create unique download path
- const download_path = path.join('/tmp', `${key}-${uuidv4()}`);
+ const run_id = uuidv4();
+ const download_path = path.join('/tmp', `compression-${run_id}`);
fs.mkdirSync(download_path, { recursive: true });
@@
// Compress directory
const compress_begin = Date.now();
const archive_name = `${key}.zip`;
- const archive_path = path.join(download_path, archive_name);
+ const local_archive_name = `${path.basename(key) || 'archive'}.${run_id}.zip`;
+ const archive_path = path.join(download_path, local_archive_name);
await createZipArchive(download_path, archive_path);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@benchmarks/300.utilities/311.compression/nodejs/function.js` around lines 73
- 89, The code uses the raw storage key when building local paths
(archive_name/archive_path), which breaks for keys containing slashes; change
usages so local filenames are derived from a safe/sanitized form of key (e.g.,
use path.basename(key) or replace path separators with an underscore) and ensure
archive_path is always created under download_path before calling
createZipArchive; update the archive_name and archive_path construction
(references: archive_name, archive_path, key, download_path) and keep
storage_handler.downloadDirectory and parseDirectory behavior the same so
downloads still land in download_path.
| async downloadDirectory(bucket, prefix, downloadPath) { | ||
| const response = await this.S3.send(new ListObjectsV2Command({ Bucket: bucket, Prefix: prefix })); | ||
|
|
||
| if (!response.Contents) { | ||
| throw new Error(`No objects found in bucket '${bucket}' with prefix '${prefix}'`); | ||
| } | ||
|
|
||
| const downloadPromises = response.Contents.map(obj => { | ||
| const fileName = obj.Key; | ||
| const pathToFile = path.dirname(fileName); | ||
| fs.mkdirSync(path.join(downloadPath, pathToFile), { recursive: true }); | ||
| return this.download(bucket, fileName, path.join(downloadPath, fileName)); | ||
| }); | ||
|
|
||
| await Promise.all(downloadPromises); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
AWS SDK for JavaScript v3 ListObjectsV2Command pagination IsTruncated NextContinuationToken ContinuationToken documentation
💡 Result:
In AWS SDK for JavaScript v3, the ListObjectsV2Command from @aws-sdk/client-s3 handles pagination using IsTruncated, ContinuationToken (input), and NextContinuationToken (output). Key fields: Request (ListObjectsV2Command input): - ContinuationToken: STRING_VALUE - Obfuscated token to continue listing from a previous response. Response (ListObjectsV2Output): - IsTruncated: boolean - true if more results available (exceeds MaxKeys, default 1000). - NextContinuationToken: STRING_VALUE - Token for next request when IsTruncated is true. - ContinuationToken: STRING_VALUE - Echoes input token if provided. Manual pagination loop: Use ContinuationToken from previous NextContinuationToken while IsTruncated is true. Example: import { S3Client, ListObjectsV2Command } from "@aws-sdk/client-s3"; const client = new S3Client({ region: "us-east-1" }); let continuationToken; let allObjects = []; do { const command = new ListObjectsV2Command({ Bucket: "my-bucket", ContinuationToken: continuationToken, MaxKeys: 1000, // optional }); const response = await client.send(command); allObjects.push(...(response.Contents || [])); continuationToken = response.NextContinuationToken; } while (continuationToken && response.IsTruncated); Recommended: Use built-in paginator paginateListObjectsV2 for automatic handling. Example: import { paginateListObjectsV2, S3Client } from "@aws-sdk/client-s3"; const client = new S3Client({}); const paginator = paginateListObjectsV2({ client }, { Bucket: "my-bucket" }); for await (const page of paginator) { console.log(page.Contents); } Tokens are obfuscated, not real keys. S3 returns up to 1000 objects per call (MaxKeys). For full details, see official S3 ListObjectsV2 API docs.
Citations:
- 1: https://docs.aws.amazon.com/goto/SdkForJavaScriptV3/s3-2006-03-01/ListObjectsV2
- 2: https://docs.aws.amazon.com/code-library/latest/ug/s3_example_s3_ListObjectsV2_section.html
- 3: https://docs.aws.amazon.com/AmazonS3/latest/API/API_ListObjectsV2.html
- 4: https://docs.aws.amazon.com/AmazonS3/latest/API/s3_example_s3_ListObjectsV2_section.html
- 5: https://cdn.jsdelivr.net/npm/@aws-sdk/[email protected]/dist-types/commands/ListObjectsV2Command.d.ts
- 6: https://docs.aws.amazon.com/AWSJavaScriptSDK/v3/latest/Package/-aws-sdk-client-s3/Interface/ListObjectsV2Request/
🏁 Script executed:
# Check AWS SDK version in package.json
find . -name "package.json" -type f | head -5 | xargs grep -l "@aws-sdk" 2>/dev/null | head -1 | xargs cat | grep -A 5 "@aws-sdk"Repository: spcl/serverless-benchmarks
Length of output: 52
🏁 Script executed:
# Verify the file exists and read the relevant section
if [ -f "benchmarks/wrappers/aws/nodejs/storage.js" ]; then
sed -n '39,53p' benchmarks/wrappers/aws/nodejs/storage.js
fiRepository: spcl/serverless-benchmarks
Length of output: 707
🏁 Script executed:
# Find package.json files in the repository
find benchmarks -name "package.json" -type f 2>/dev/nullRepository: spcl/serverless-benchmarks
Length of output: 425
🏁 Script executed:
# Also check if there's a root package.json
if [ -f "package.json" ]; then
grep -A 2 -B 2 "@aws-sdk" package.json
fiRepository: spcl/serverless-benchmarks
Length of output: 52
🏁 Script executed:
# Check for package.json in wrappers directory
find benchmarks/wrappers -name "package.json" -type f 2>/dev/nullRepository: spcl/serverless-benchmarks
Length of output: 52
🏁 Script executed:
# Also check what files are in the nodejs wrapper directory
ls -la benchmarks/wrappers/aws/nodejs/ 2>/dev/null || echo "Directory not found"Repository: spcl/serverless-benchmarks
Length of output: 359
🏁 Script executed:
# Search for imports/requires of storage.js to understand how it's used
rg "storage\.js|from.*storage|require.*storage" benchmarks --type js | head -20Repository: spcl/serverless-benchmarks
Length of output: 752
🏁 Script executed:
# Check if storage.js is imported in any benchmark package
rg -A 3 "@aws-sdk" benchmarks --type json | head -40Repository: spcl/serverless-benchmarks
Length of output: 52
🏁 Script executed:
# Check the imports in storage.js to see all AWS SDK packages used
head -20 benchmarks/wrappers/aws/nodejs/storage.jsRepository: spcl/serverless-benchmarks
Length of output: 722
Paginate S3 listings before downloading the directory.
Line 40 only fetches the first page of ListObjectsV2 results. S3 returns up to 1000 objects per response (default MaxKeys). When a prefix contains more objects, the code silently omits them from download and compresses an incomplete directory, corrupting benchmark results.
Implement pagination using IsTruncated and NextContinuationToken in a loop until all pages are fetched:
🐛 Proposed fix
async downloadDirectory(bucket, prefix, downloadPath) {
- const response = await this.S3.send(new ListObjectsV2Command({ Bucket: bucket, Prefix: prefix }));
-
- if (!response.Contents) {
- throw new Error(`No objects found in bucket '${bucket}' with prefix '${prefix}'`);
- }
-
- const downloadPromises = response.Contents.map(obj => {
- const fileName = obj.Key;
- const pathToFile = path.dirname(fileName);
- fs.mkdirSync(path.join(downloadPath, pathToFile), { recursive: true });
- return this.download(bucket, fileName, path.join(downloadPath, fileName));
- });
+ const downloadPromises = [];
+ let continuationToken;
+
+ do {
+ const response = await this.S3.send(new ListObjectsV2Command({
+ Bucket: bucket,
+ Prefix: prefix,
+ ContinuationToken: continuationToken
+ }));
+
+ for (const obj of response.Contents || []) {
+ if (!obj.Key || obj.Key.endsWith('/')) continue;
+
+ const fileName = obj.Key;
+ const pathToFile = path.dirname(fileName);
+ fs.mkdirSync(path.join(downloadPath, pathToFile), { recursive: true });
+ downloadPromises.push(this.download(bucket, fileName, path.join(downloadPath, fileName)));
+ }
+
+ continuationToken = response.IsTruncated ? response.NextContinuationToken : undefined;
+ } while (continuationToken);
+
+ if (downloadPromises.length === 0) {
+ throw new Error(`No objects found in bucket '${bucket}' with prefix '${prefix}'`);
+ }
await Promise.all(downloadPromises);
};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@benchmarks/wrappers/aws/nodejs/storage.js` around lines 39 - 53, The
downloadDirectory function currently calls this.S3.send(new
ListObjectsV2Command(...)) once and only processes response.Contents; change it
to loop and paginate using response.IsTruncated and
response.NextContinuationToken (call ListObjectsV2Command with ContinuationToken
until IsTruncated is false), accumulate all Contents into a single array, then
create directories and call this.download for each Key as before (keep using
downloadDirectory, ListObjectsV2Command, this.S3.send, response.IsTruncated,
response.NextContinuationToken, and this.download to locate the logic).
|
Thank you @ldzgch! I extended the PR to cover two more things:
|
split out from the cloudflare PR, as requested
Summary by CodeRabbit
New Features
Chores
Bug Fixes