fix(judge): remove misleading module-level concurrency stats - #198
fix(judge): remove misleading module-level concurrency stats#198sreevats30 wants to merge 2 commits into
Conversation
|
@sreevats30 is attempting to deploy a commit to the barunaniket-9400's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
Reference: Issue #159 |
There was a problem hiding this comment.
LGTM — the in-process limiter was a no-op on Workers and judgeQueueStats() was reporting zeros in production. Clean removal.
Two small nits:
Both judge.ts and health/route.ts are missing a trailing newline (git shows \ No newline at end of file). Can you add one?
Thanks for fixing the � → — mojibake in the health route comment 👍
Also — please tick the checklist box confirming you ran tsc --noEmit, lint, test, build locally on Node 22.
@barunaniket just give it a look though
|
@hagemaruwu Added the trailing newlines in src/lib/judge.ts and src/app/api/health/route.ts |
|
@hagemaruwu is it a green light? should we merge? |
|
Yep @barunaniket , nits addressed and logic is clean. Green light — merge it |
|
@barunaniket merge it pls |
|
@hagemaruwu will need to review then only will be merging |
barunaniket
left a comment
barunaniket
left a comment
There was a problem hiding this comment.
Good change — this is a tight, single-purpose PR and the reasoning holds up. I verified the refactor is genuinely behaviour-preserving rather than taking it on faith: stripping brace-only lines and the four limiter statements, all 69 statements in judgeExecute are byte-identical between main and this branch. The retry loop, the 503 retry_after_secs handling, the backoff+jitter, AbortSignal.timeout(30_000) and the result mapping are all untouched — only nesting changed, since dropping try/finally de-indents the body. Branch is based on current main HEAD, and judgeQueueStats had exactly one consumer (the health route updated here), so the removal is clean.
One thing to fold in before merge, plus a few notes.
Please also remove JUDGE_CONCURRENCY from .env.example
This PR deletes the only reader of process.env.JUDGE_CONCURRENCY, but .env.example:20 still documents it:
# Max concurrent sandbox jobs (bounded so a burst of submissions can't saturate
# the box and cause false TLEs). Defaults to CPU cores - 1.
# JUDGE_CONCURRENCY=4
After this merges that becomes dead config that will quietly mislead anyone setting up an environment — someone will set it and wonder why nothing changes. It's a few lines in the same PR; leaving it for later means it never happens. (The comment is doubly stale, incidentally: it claims the default is "CPU cores - 1" but the code being deleted used a flat 4.)
Notes — not blocking
The rationale is slightly overstated. Worker isolates do persist module-level state across requests, so this was a per-isolate cap of 4 rather than a no-op — with N isolates you'd get up to 4N concurrent jobs. Your conclusion still stands (it was never a meaningful global limit, and judgeQueueStats reported one isolate's view as though it were fleet-wide, which is exactly the "misleading" part), so the change is right — just worth phrasing as "not a global limit" rather than "ineffective", since the existing code comment made the same overstatement.
There's also an upside you didn't claim: a promise parked in jobQueue only resolves when another request calls releaseSlot(). If the isolate is recycled while requests are queued, those promises never resolve and the requests hang until timeout. Deleting the queue removes that failure mode outright.
There is a real behaviour change for local/self-hosted Node, which the description says nothing about. Node-side concurrency goes from capped-at-4 to unbounded, so a submission burst now reaches the judge all at once and comes back as 503s → retries → a user-visible "Judge server is currently busy" message, where previously it queued silently in-process. Downstream protection exists (JUDGE_MAX_QUEUE=128 in docker-compose.yml, the retry/backoff you kept, per-user submit rate limits), so I think this is the right trade — holding a request open inside a Worker to wait for a slot burns wall-clock and CPU budget for no benefit. But it's a trade-off worth a sentence in the description rather than being presented as pure dead-code removal.
/api/health response shape changes. I grepped and nothing in the repo reads judgeQueue (docker-compose.yml:7 only curls the endpoint), so this is safe in-tree. Flagging in case an external uptime check or dashboard parses that field — it would lose it silently rather than erroring.
PR hygiene: "How was this tested?" is empty and most checklist boxes are unticked. For a deletion this size that's largely fine — there are no tests over judge.ts either way — but a line on how you exercised a submission end-to-end would help.
Nice incidental fixes: the mojibake \uFFFD → — in the health route comment, and the missing trailing newlines on both files.
One unrelated heads-up: the Vercel check on this PR is failing with Authorization required to deploy. That's a deploy-auth/config problem rather than anything wrong with your code (it passes on other PRs) — ignore it, we'll sort it out separately.
Happy to merge once .env.example is cleaned up.
| // This limiter only helps during local/Node.js dev; on Workers it's effectively a no-op. | ||
| const MAX_CONCURRENT_JOBS = Math.max( | ||
| 1, | ||
| Number(process.env.JUDGE_CONCURRENCY) || 4, |
There was a problem hiding this comment.
This is the only place JUDGE_CONCURRENCY is read anywhere in the repo — I checked. Removing it here leaves .env.example:20 documenting an environment variable that no longer does anything.
Please drop that block from .env.example in this PR so the two stay in sync.
|
|
||
| const ok = checks.db && checks.judge; | ||
| return NextResponse.json( | ||
| { ok, ...checks, judgeQueue: judgeQueueStats(), at: new Date().toISOString() }, |
There was a problem hiding this comment.
Worth calling out in the PR description as a response-shape change to a public endpoint: /api/health no longer returns the judgeQueue object.
Nothing in the repo consumes it (docker-compose.yml:7 just curls the endpoint for a liveness check), so this is safe in-tree — but any external uptime monitor or dashboard reading that field will silently lose it rather than fail loudly.
|
@sreevats30 any updates? |
3 participants
Summary
Removes the dead module-level concurrency limiter (
activeJobs/jobQueue) andjudgeQueueStatsfromsrc/lib/judge.tsandsrc/app/api/health/route.ts. Since Worker isolates do not share state, this Node-side queue was ineffective and reported misleading stats; real concurrency limiting is handled downstream by the Rust/Akiro judge.Type of change
Related issues
Closes #
How was this tested?
Checklist
npx tsc --noEmit,npm run lint,npm run test,npm run buildpackage-lock.jsonchanges (see CONTRIBUTING.md - the native-binding gotcha)npm run challenges:validatepasses and hiddentestsare strongNotes for reviewers