fix bugs - #1443
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
|
/run-security-scan |
alexcos20
left a comment
alexcos20
left a comment
There was a problem hiding this comment.
AI automated code review (Gemini 3).
Overall risk: low
Summary:
This is an excellent PR that resolves multiple subtle bugs. It corrects negative runMaxWaitTime by correctly identifying unset ('0') timestamps, patches an unbound free-resource allocation flaw caused by NaN comparison evaluation, and correctly handles SQLite filtering and sorting semantics (0-index status drops, TEXT vs REAL comparison). The code is well-structured, carefully documented, and accompanied by exhaustive unit tests. LGTM!
Comments:
• [INFO][performance] Using CAST(dateCreated AS REAL) in the WHERE clause (and later in the ORDER BY clause) accurately resolves the string vs. numeric sorting issue. However, please note that applying a CAST (or any function) to a column in SQLite usually prevents the query planner from using an index on that column, resulting in a full table scan. This is acceptable for now to ensure correctness, but if this table grows significantly, consider migrating the column data type to REAL or INTEGER natively in a future schema migration to restore index usage.
• [INFO][security] Excellent catch using the nullish coalescing operator ?? 0 here. A sparse configuration resulting in NaN < amount silently yielding false and bypassing the free allocation constraint is a subtle but critical logical vulnerability. This reliably patches it.
• [INFO][style] Instantiating const nowSec = Date.now() / 1000 once per loop iteration is a great practice. It guarantees a consistent reference time for every job in the list and prevents minor calculation drift during the loop execution.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/components/database/sqliteCompute.ts (1)
775-778: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winApply the same numeric ordering to
getJobsByStatus.
getJobsnow sorts withCAST(dateCreated AS REAL) DESC.getJobsByStatusstill sorts the same TEXT column lexicographically, so a 9-digit second value sorts above a 10-digit one there. Both methods read the samecompute_jobstable, so the ordering contract differs between two paths.♻️ Proposed change outside the selected range (line 807)
- selectSQL += ` ORDER BY dateCreated DESC` + selectSQL += ` ORDER BY CAST(dateCreated AS REAL) DESC`🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/database/sqliteCompute.ts` around lines 775 - 778, Update the SQL ordering in getJobsByStatus to use CAST(dateCreated AS REAL) DESC, matching the ordering already applied in getJobs for the shared compute_jobs table. Preserve the existing status filtering and other query behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/c2d/compute_engine_base.ts`:
- Around line 801-806: Update the paid-resource gate immediately above the
free-resource check to default envResource.total to 0 before subtracting
envResource.inUse, matching the existing missing-value behavior in the free
gate. Preserve the existing request.amount comparison and other allocation
logic.
---
Nitpick comments:
In `@src/components/database/sqliteCompute.ts`:
- Around line 775-778: Update the SQL ordering in getJobsByStatus to use
CAST(dateCreated AS REAL) DESC, matching the ordering already applied in getJobs
for the shared compute_jobs table. Preserve the existing status filtering and
other query behavior.
🪄 Autofix
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 Plus
Run ID: c3105cbe-e5dc-4156-b8fa-0397a5a26b56
📒 Files selected for processing (10)
src/@types/C2D/C2D.tssrc/components/c2d/compute_engine_base.tssrc/components/c2d/compute_engine_docker.tssrc/components/core/handler/getJobs.tssrc/components/core/service/getServices.tssrc/components/core/utils/timestamps.tssrc/components/database/C2DDatabase.tssrc/components/database/sqliteCompute.tssrc/test/unit/c2d/usedResources.test.tssrc/test/unit/database/getJobsFilters.test.ts
| // `total` and `inUse` are both optional on ComputeResource: a sparse config (or an | ||
| // unresolved free-resource ref) makes the subtraction NaN, and `NaN < amount` is | ||
| // false, which would silently pass the gate and allow unlimited free allocation. | ||
| // Default both to 0 so a missing value denies instead of admitting — same shape as | ||
| // gate 1 above. | ||
| if ((envResource.total ?? 0) - (envResource.inUse ?? 0) < request.amount) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Harden gate 1 the same way; the comment claims parity that does not exist.
The comment states the free gate now has the "same shape as gate 1 above". Gate 1 at line 788 still reads envResource.total - (envResource.inUse ?? 0). If a fungible paid resource has no total, the subtraction is NaN, and NaN < request.amount is false, so the paid allocation passes unchecked. That is the same admit-on-missing-value hole this change closes for the free tier, and it applies to the paid path that guards shared CPU/RAM/disk.
🛡️ Proposed fix at line 788 (outside the selected range)
- if (isFungible && envResource.total - (envResource.inUse ?? 0) < request.amount)
+ if (isFungible && (envResource.total ?? 0) - (envResource.inUse ?? 0) < request.amount)
throw new Error(`Not enough available ${request.id} in this environment`)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/c2d/compute_engine_base.ts` around lines 801 - 806, Update the
paid-resource gate immediately above the free-resource check to default
envResource.total to 0 before subtracting envResource.inUse, matching the
existing missing-value behavior in the free gate. Preserve the existing
request.amount comparison and other allocation logic.
1 participant
Closes #1439
Fix negative
runMaxWaitTime, queued-job resource under-counting, and silentgetJobsfiltersWhy
A live node returned this from
GETcompute environments:{ "runningJobs": 15, "runningfreeJobs": 15, "queuedJobs": 0, "maxJobDuration": 3600, "runMaxWaitTime": -26786355914.46, "runMaxWaitTimeFree": -26786355914.46 }That value is exactly
15 × (3600 − now_in_seconds):Every one of the 15 jobs was measuring its elapsed runtime from the Unix epoch. Consumers
(ocean.js, the frontend env picker, schedulers) read these fields to estimate when capacity frees
up, so the environment reads as either permanently unavailable or instantly available depending on
which way the client compares. Worse, the same function feeds the strict resource-availability
gate, so a second defect in it lets the engine overcommit the host.
Three independent defects came out of that one payload, plus a fourth found in the neighbouring
job-listing path.
1.
'0'is truthy, so every pull-path job measures from 1970buildStartTimestampandalgoStartTimestampare initialized to the string'0', andbuildStartTimestampis only ever overwritten insidebuildImage. The old code branched ontruthiness:
'0'is truthy, so the first branch is taken for every job that did not build an image from aDockerfile — including jobs mid-algorithm holding a perfectly valid
algoStartTimestamp. Theelsebranch is effectively dead for those jobs. Only Dockerfile-build jobs ever reported acorrect remaining time. That matches the payload: all 15 jobs were free, and
env.free.allowImageBuildgates builds for free jobs, so all 15 were pull-path.A genuine overrun cannot reach this magnitude — it would need a job running since 1970. But
overruns do make the expression legitimately negative by seconds-to-minutes, between the moment
a job exceeds its budget and the next cron sweep that kills it, so the sentinel and the missing
clamp are two separate fixes.
2.
queueMaxWaitTimeis never reset, so released jobs stay "queued" foreverThe running/queued discriminator was
job.queueMaxWaitTime === 0. ButqueueMaxWaitTimeis thecaller's requested maximum queue wait — assigned once at job creation and never written again.
The queue-release path flips
statustoBuildImage/PullImageand starts the container, butleaves
queueMaxWaitTimeat its original positive value.So for the whole life of any job that was ever queued:
queuedJobs/queMaxWaitTimeforever, even while its container runsrunningJobs/runningfreeJobsusedResourcesThe last row is the real hazard.
checkIfResourcesAreAvailableandcheckGlobalResourceAvailabilityboth gate onenvResource.inUse, so under-counting a runningjob's resources lets the engine admit work the host cannot serve — including via the queue-release
path itself, which calls
checkIfResourcesAreAvailableand can therefore release several queuedjobs against the same phantom-free capacity. It also skews
runningJobs + 1 > maxJobs, so a nodecan exceed its own
maxJobs.The queued branch also added a runtime budget (
job.maxJobDuration) to a queue-wait metric.3. Free-tier availability gate silently passes on
NaNGate 1 defends itself (
envResource.total - (envResource.inUse ?? 0)), but the free-tier gate didnot:
envResource.total - envResource.inUse. Both fields are optional onComputeResource, and asparse config (or an unresolved free-resource ref) makes that
NaN.NaN < amountisfalse, sothe free gate passes — unlimited free allocation.
This is the same failure mode as #2 (under-counted usage → overcommit) in the same function, and
#2 changes exactly what feeds
inUse, so the two belong together.4.
getJobsfilters return the wrong rows, silentlySeparate subsystem, found while tracing the above.
dateCreated/dateFinishedareTEXTholdingdecimal seconds (
"1785760660.961"), andgetJobsbound the caller'sfromTimestampstraightthrough as a string — so SQLite did a memcmp, not a numeric comparison:
"1785760660.961""1785760660""1785760660961"'9'(0x39) >'.'(0x2E)"2026-08-04T…"'2'>'1'"abc"'a'(0x61) >'1'Every failure was
[](or a wrong subset) with HTTP 200 and no log line — indistinguishable from"no jobs in that window". The handler validated only
typeof === 'string'.ORDER BY dateCreated DESCwas lexicographic for the same reason. Andif (status)dropped the predicate entirely forC2DStatusNumber.JobStarted, which is0— filtering forJobStartedreturned every status.How
Capacity reporting —
compute_engine_base.tsOne shared numeric guard, not a truthiness fix. Changing the initializer from
'0'tonullwould fix this instance while leaving the identical trap for the next field or caller, andwould not repair rows already in the DB carrying
'0'. So:Callers only ever test
> 0. Two helpers build on it:getJobRemainingRuntimeSeconds(job, nowSec)—buildStartTimestampwins when both are set(build time counts against
maxJobDuration, consistent with the runtime-expiry check in thedocker engine); clamped to
[0, maxJobDuration];maxJobDurationitself guarded withNumber.isFinite. A job with no usable timestamp on either field reports its full budget,not
now— it is allocated but not yet ticking (PullImage/BuildImage/ConfiguringVolumes).getJobRemainingQueueWaitSeconds(job, nowSec)— mirrors the queue-expiry check(
queueMaxWaitTime - (now - dateCreated)), same clamp, and the same "no usabledateCreated⇒report the full requested wait" rule.
Discriminate on
status, do not mutatequeueMaxWaitTime. Zeroing the stored value on releasewould be wrong: it is a contract value, reused for the queue-expiry check and for the escrow lock
duration (
maxJobDuration + queueMaxWaitTime), and it would need a DB migration.statusisalready an exact, persisted, migration-free signal —
C2DStatusNumber.JobQueuedis set at creationiff
queueMaxWaitTime > 0and is replaced on release, and it is the only pre-allocationstate:
The two branches are inverted accordingly, and the resource tally is now guarded by
!isQueued, soevery post-queue state contributes its resources.
Date.now()is hoisted into a singlenowSecsoall jobs in one response are measured against the same instant.
Free gate hardened to
(envResource.total ?? 0) - (envResource.inUse ?? 0), matching gate 1 —a missing value now denies instead of admitting.
*MaxWaitTimesemantics: sum kept, documentedAll four accumulators still use
+=. Chosen deliberately overMath.maxto avoid any contractchange: the only difference now is that each term is non-negative and derived from a valid start
timestamp. Since the sum makes the field names misleading — 15 jobs with 1 s left each report the
same
15as one job with 15 s left — the four fields insrc/@types/C2D/C2D.tsnow carry doccomments stating the unit, the summing, and explicitly that a client cannot derive
time-to-free-capacity from these fields. A unit test pins the sum so a later refactor cannot
switch it silently.
Docker engine
getValidBuildDurationSecondsrefactored ontoparseJobTimestamp(it already had the correctstart <= 0check inline — now one shared rule instead of a second copy, across its three callsites). The runtime-expiry check no longer does
parseFloat(job.algoStartTimestamp)raw: a'0'sentinel there would place the start at the epoch and make a healthy container look instantly
expired, so it falls back to
timeNowand lets the next sweep see the real timestamp.getJobs— normalize at the boundary, compare numerically in SQLsrc/components/core/utils/timestamps.ts—parseFromTimestampmoved here fromgetServices.ts(which re-exports it, so the service handler and its tests are untouched), plusa
parseFromTimestampSeconds()wrapper. The seconds-vs-ms choice is explicit at the call siterather than implicit: the compute-jobs table stores seconds, the service path works in ms.
A header comment separates this from
parseJobTimestamp— that one reads stored timestamps andtreats
'0'as unset; this one normalizes query parameters and must distinguish "no filter"from "garbage".
GetJobsHandler.validaterejects an unparseablefromTimestampwith400and the samemessage shape as
GetServicesHandler, instead of200 []. Absent/empty still means "no filter",as before.
number(seconds), not a string — type-safety over convenience, so a rawstring can never reach the SQL again. Threaded through
SQLiteCompute.getJobsand theC2DDatabasepass-through, and the staleComputeDatabaseProvider.getJobsdeclaration(3 of 5 params; not constraining anything today because
C2DDatabase.provideris typed as theconcrete class) fixed to the real shape while here.
CAST(dateCreated AS REAL) >= ?/CAST(dateFinished AS REAL) >= ?, andORDER BY CAST(dateCreated AS REAL) DESC.if (status !== undefined && status !== null)sostatus = 0filters.No schema or migration change — only how existing
TEXTcolumns are compared.Summary by CodeRabbit