Fix accessing bucket files in c2d jobs#1357
Conversation
|
/run-security-scan |
alexcos20
left a comment
There was a problem hiding this comment.
AI automated code review (Gemini 3).
Overall risk: low
Summary:
The PR introduces a targeted fix to ensure that the Docker image build restriction (env.free?.allowImageBuild) is exclusively applied to free compute jobs. Previously, the logic incorrectly applied this free-tier restriction to paid jobs, causing unintended build failures.
Comments:
• [INFO][bug] Good catch! By scoping the !env.free?.allowImageBuild check with isFree &&, you've correctly isolated the free-tier limitations so they no longer restrict paid workloads. This aligns the behavior with the configuration's intended namespace (env.free). The logic fix is sound.
LGTM!
|
/run-security-scan |
alexcos20
left a comment
There was a problem hiding this comment.
AI automated code review (Gemini 3).
Overall risk: low
Summary:
This PR fixes a bug related to Docker image build checks in free environments and introduces a solid Docker-in-Docker (sibling) volume mount mapping mechanism for persistent storage. The host mapping approach is well-designed but contains a bug related to missing imports and synchronous file reading.
Comments:
• [INFO][bug] Good logic fix. Ensuring the allowImageBuild rule only applies when the job isFree prevents unintended restrictions on paid compute jobs.
• [ERROR][bug] fs.readFileSync is used here, but fs is not natively imported at the top of the file (only fs/promises as fsp). This will likely throw a ReferenceError that gets silently swallowed by the catch block below, causing the host path detection to silently fail in all environments.
Additionally, synchronous file reads block the Node.js event loop, which is an anti-pattern inside async functions.
Suggested fix:
- const containerId = fs.readFileSync('/etc/hostname', 'utf8').trim()
+ const containerId = (await fsp.readFile('/etc/hostname', 'utf8')).trim()• [INFO][other] Excellent implementation of host path resolution using Docker's Mounts. This perfectly solves the classic "Docker sibling" bind-mount issue where in-container paths do not map to the host's actual filesystem paths.
• [INFO][style] Good job catching errors and falling back to the default baseFolder gracefully without crashing the node. This ensures safe behavior in non-Docker or custom deployments.
Fixes # .
Changes proposed in this PR: