feat(nuq): concurrency tracking #1
Conversation
|
@cubic-dev-ai review this |
@sanxroz I've started the AI code review. It'll take a few minutes to complete. |
1 similar comment
@sanxroz I've started the AI code review. It'll take a few minutes to complete. |
cubic is running your AI reviewPipeline
|
cubic is running your AI reviewPipeline
|
cubic is running your AI reviewPipeline
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 2 files
Prompt for AI agents (all 1 issues)
Understand the root cause of the following 1 issues and fix them.
<file name="apps/api/src/services/worker/nuq.ts">
<violation number="1" location="apps/api/src/services/worker/nuq.ts:713">
The `max_concurrency` limit is hardcoded to `100` with a `TODO` comment when a new owner's concurrency record is created. A key architectural parameter like a concurrency limit should not be a magic number inside a query string. This makes the system difficult to configure and maintain, as changing limits requires a code change and deployment.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: The code intentionally seeds new owner rows with a default max_concurrency of 100 when concurrency tracking is enabled. While a TODO notes future configurability, having a hard-coded default value is not inherently incorrect or harmful, and there is no evidence of a functional defect caused by this choice. This is a design preference rather than a clear bug.</violation>
</file>
React with 👍 or 👎 to teach cubic. Mention @cubic-dev-ai to give feedback, ask questions, or re-run the review.
| ), | ||
| owner_increment AS ( | ||
| INSERT INTO ${this.queueName}_owner_concurrency (id, current_concurrency, max_concurrency) | ||
| SELECT owner_id, job_count, ${/* TODO max concurrency */ 100} |
There was a problem hiding this comment.
The max_concurrency limit is hardcoded to 100 with a TODO comment when a new owner's concurrency record is created. A key architectural parameter like a concurrency limit should not be a magic number inside a query string. This makes the system difficult to configure and maintain, as changing limits requires a code change and deployment.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• GPT-5: The code intentionally seeds new owner rows with a default max_concurrency of 100 when concurrency tracking is enabled. While a TODO notes future configurability, having a hard-coded default value is not inherently incorrect or harmful, and there is no evidence of a functional defect caused by this choice. This is a design preference rather than a clear bug.
Prompt for AI agents
Address the following comment on apps/api/src/services/worker/nuq.ts at line 713:
<comment>The `max_concurrency` limit is hardcoded to `100` with a `TODO` comment when a new owner's concurrency record is created. A key architectural parameter like a concurrency limit should not be a magic number inside a query string. This makes the system difficult to configure and maintain, as changing limits requires a code change and deployment.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: The code intentionally seeds new owner rows with a default max_concurrency of 100 when concurrency tracking is enabled. While a TODO notes future configurability, having a hard-coded default value is not inherently incorrect or harmful, and there is no evidence of a functional defect caused by this choice. This is a design preference rather than a clear bug.</comment>
<file context>
@@ -677,14 +677,59 @@ class NuQ<JobData = any, JobReturnValue = any> {
+ ),
+ owner_increment AS (
+ INSERT INTO ${this.queueName}_owner_concurrency (id, current_concurrency, max_concurrency)
+ SELECT owner_id, job_count, ${/* TODO max concurrency */ 100}
+ FROM owner_counts
+ ON CONFLICT (id)
</file context>
[internal] Confidence score: 10/10
[internal] Posted by: System Design Agent
|
@cubic-dev-ai review this |
@sanxroz I've started the AI code review. It'll take a few minutes to complete. |
1 similar comment
@sanxroz I've started the AI code review. It'll take a few minutes to complete. |
![cubic-dev-local[bot]](https://avatars.githubusercontent.com/u/189440881?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 2 files
Prompt for AI agents (all 3 issues)
Understand the root cause of the following 3 issues and fix them.
<file name="apps/api/src/services/worker/nuq.ts">
<violation number="1" location="apps/api/src/services/worker/nuq.ts:699">
The logic to acquire new jobs does not enforce the `max_concurrency` limit. The query fetching jobs for activation should be constrained to only select jobs for owners where `current_concurrency < max_concurrency`.</violation>
<violation number="2" location="apps/api/src/services/worker/nuq.ts:713">
The `max_concurrency` limit is hardcoded to 100. This value should be configurable per-queue or per-owner rather than being a fixed constant inside a generic class, which hinders reusability and proper configuration.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: Using a constant value of 100 for `max_concurrency` is an intentional default (noted by the TODO) and does not create a functional bug; it merely limits configurability. Without evidence that configurability is required here, this is a low-severity preference issue.</violation>
</file>
<file name="apps/nuq-postgres/nuq.sql">
<violation number="1" location="apps/nuq-postgres/nuq.sql:40">
Stuck jobs that are cleaned up by the `nuq_queue_scrape_lock_reaper` cron job do not have their owner's concurrency count decremented. This will cause an owner's concurrency count to permanently increase over time, eventually preventing any new jobs from running.</violation>
</file>
React with 👍 or 👎 to teach cubic. Mention @cubic-dev-ai to give feedback, ask questions, or re-run the review.
| ), | ||
| owner_increment AS ( | ||
| INSERT INTO ${this.queueName}_owner_concurrency (id, current_concurrency, max_concurrency) | ||
| SELECT owner_id, job_count, ${/* TODO max concurrency */ 100} |
There was a problem hiding this comment.
The max_concurrency limit is hardcoded to 100. This value should be configurable per-queue or per-owner rather than being a fixed constant inside a generic class, which hinders reusability and proper configuration.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• GPT-5: Using a constant value of 100 for max_concurrency is an intentional default (noted by the TODO) and does not create a functional bug; it merely limits configurability. Without evidence that configurability is required here, this is a low-severity preference issue.
Prompt for AI agents
Address the following comment on apps/api/src/services/worker/nuq.ts at line 713:
<comment>The `max_concurrency` limit is hardcoded to 100. This value should be configurable per-queue or per-owner rather than being a fixed constant inside a generic class, which hinders reusability and proper configuration.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: Using a constant value of 100 for `max_concurrency` is an intentional default (noted by the TODO) and does not create a functional bug; it merely limits configurability. Without evidence that configurability is required here, this is a low-severity preference issue.</comment>
<file context>
@@ -677,14 +677,59 @@ class NuQ<JobData = any, JobReturnValue = any> {
+ ),
+ owner_increment AS (
+ INSERT INTO ${this.queueName}_owner_concurrency (id, current_concurrency, max_concurrency)
+ SELECT owner_id, job_count, ${/* TODO max concurrency */ 100}
+ FROM owner_counts
+ ON CONFLICT (id)
</file context>
[internal] Confidence score: 10/10
[internal] Posted by: System Design Agent
| if (this.options.concurrencyLimit === "per-owner") { | ||
| // Single query: select, update jobs, and update owner concurrency | ||
| updateQuery = ` | ||
| WITH next AS ( |
There was a problem hiding this comment.
The logic to acquire new jobs does not enforce the max_concurrency limit. The query fetching jobs for activation should be constrained to only select jobs for owners where current_concurrency < max_concurrency.
Prompt for AI agents
Address the following comment on apps/api/src/services/worker/nuq.ts at line 699:
<comment>The logic to acquire new jobs does not enforce the `max_concurrency` limit. The query fetching jobs for activation should be constrained to only select jobs for owners where `current_concurrency < max_concurrency`.</comment>
<file context>
@@ -677,14 +677,59 @@ class NuQ<JobData = any, JobReturnValue = any> {
+ if (this.options.concurrencyLimit === "per-owner") {
+ // Single query: select, update jobs, and update owner concurrency
+ updateQuery = `
+ WITH next AS (
+ ${queryGetNext}
+ ),
</file context>
[internal] Confidence score: 10/10
[internal] Posted by: System Design Agent
| CREATE INDEX IF NOT EXISTS nuq_queue_scrape_failed_created_at_idx ON nuq.queue_scrape USING btree (created_at) WHERE (status = 'failed'::nuq.job_status); | ||
| CREATE INDEX IF NOT EXISTS nuq_queue_scrape_completed_created_at_idx ON nuq.queue_scrape USING btree (created_at) WHERE (status = 'completed'::nuq.job_status); | ||
|
|
||
| CREATE TABLE IF NOT EXISTS nuq.queue_scrape_owner_concurrency ( |
There was a problem hiding this comment.
Stuck jobs that are cleaned up by the nuq_queue_scrape_lock_reaper cron job do not have their owner's concurrency count decremented. This will cause an owner's concurrency count to permanently increase over time, eventually preventing any new jobs from running.
Prompt for AI agents
Address the following comment on apps/nuq-postgres/nuq.sql at line 40:
<comment>Stuck jobs that are cleaned up by the `nuq_queue_scrape_lock_reaper` cron job do not have their owner's concurrency count decremented. This will cause an owner's concurrency count to permanently increase over time, eventually preventing any new jobs from running.</comment>
<file context>
@@ -37,6 +37,13 @@ CREATE INDEX IF NOT EXISTS nuq_queue_scrape_queued_optimal_2_idx ON nuq.queue_sc
CREATE INDEX IF NOT EXISTS nuq_queue_scrape_failed_created_at_idx ON nuq.queue_scrape USING btree (created_at) WHERE (status = 'failed'::nuq.job_status);
CREATE INDEX IF NOT EXISTS nuq_queue_scrape_completed_created_at_idx ON nuq.queue_scrape USING btree (created_at) WHERE (status = 'completed'::nuq.job_status);
+CREATE TABLE IF NOT EXISTS nuq.queue_scrape_owner_concurrency (
+ id uuid NOT NULL,
+ current_concurrency int8 NOT NULL,
</file context>
[internal] Confidence score: 10/10
[internal] Posted by: System Design Agent
|
@cubic-dev-local review this |
@sanxroz I've started the AI code review. It'll take a few minutes to complete. |
There was a problem hiding this comment.
2 issues found across 2 files
Prompt for AI agents (all 2 issues)
Understand the root cause of the following 2 issues and fix them.
<file name="apps/api/src/services/worker/nuq.ts">
<violation number="1" location="apps/api/src/services/worker/nuq.ts:699">
The per-owner concurrency limit is tracked but not enforced. The logic activates new jobs for an owner even if they are already at their `max_concurrency` limit, which defeats the purpose of the `concurrencyLimit` feature.</violation>
<violation number="2" location="apps/api/src/services/worker/nuq.ts:713">
The `max_concurrency` limit for an owner is hardcoded to 100, with a `TODO` comment indicating an incomplete implementation. This prevents per-owner configuration of concurrency limits, limiting the feature's usefulness.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: Although the insert defaults `max_concurrency` to 100, the PR description explicitly states that the new table “inserts default max_concurrency to 100,” implying this default is intentional for now. The code also allows the value to be adjusted later without overwriting it on conflict. This is an acknowledged TODO rather than a demonstrable bug, so it is too speculative to flag.</violation>
</file>
React with 👍 or 👎 to teach cubic. Mention @cubic-dev-ai to give feedback, ask questions, or re-run the review.
| ), | ||
| owner_increment AS ( | ||
| INSERT INTO ${this.queueName}_owner_concurrency (id, current_concurrency, max_concurrency) | ||
| SELECT owner_id, job_count, ${/* TODO max concurrency */ 100} |
There was a problem hiding this comment.
The max_concurrency limit for an owner is hardcoded to 100, with a TODO comment indicating an incomplete implementation. This prevents per-owner configuration of concurrency limits, limiting the feature's usefulness.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• GPT-5: Although the insert defaults max_concurrency to 100, the PR description explicitly states that the new table “inserts default max_concurrency to 100,” implying this default is intentional for now. The code also allows the value to be adjusted later without overwriting it on conflict. This is an acknowledged TODO rather than a demonstrable bug, so it is too speculative to flag.
Prompt for AI agents
Address the following comment on apps/api/src/services/worker/nuq.ts at line 713:
<comment>The `max_concurrency` limit for an owner is hardcoded to 100, with a `TODO` comment indicating an incomplete implementation. This prevents per-owner configuration of concurrency limits, limiting the feature's usefulness.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: Although the insert defaults `max_concurrency` to 100, the PR description explicitly states that the new table “inserts default max_concurrency to 100,” implying this default is intentional for now. The code also allows the value to be adjusted later without overwriting it on conflict. This is an acknowledged TODO rather than a demonstrable bug, so it is too speculative to flag.</comment>
<file context>
@@ -677,14 +677,59 @@ class NuQ<JobData = any, JobReturnValue = any> {
+ ),
+ owner_increment AS (
+ INSERT INTO ${this.queueName}_owner_concurrency (id, current_concurrency, max_concurrency)
+ SELECT owner_id, job_count, ${/* TODO max concurrency */ 100}
+ FROM owner_counts
+ ON CONFLICT (id)
</file context>
[internal] Confidence score: 10/10
[internal] Posted by: System Design Agent
| if (this.options.concurrencyLimit === "per-owner") { | ||
| // Single query: select, update jobs, and update owner concurrency | ||
| updateQuery = ` | ||
| WITH next AS ( |
There was a problem hiding this comment.
The per-owner concurrency limit is tracked but not enforced. The logic activates new jobs for an owner even if they are already at their max_concurrency limit, which defeats the purpose of the concurrencyLimit feature.
Prompt for AI agents
Address the following comment on apps/api/src/services/worker/nuq.ts at line 699:
<comment>The per-owner concurrency limit is tracked but not enforced. The logic activates new jobs for an owner even if they are already at their `max_concurrency` limit, which defeats the purpose of the `concurrencyLimit` feature.</comment>
<file context>
@@ -677,14 +677,59 @@ class NuQ<JobData = any, JobReturnValue = any> {
+ if (this.options.concurrencyLimit === "per-owner") {
+ // Single query: select, update jobs, and update owner concurrency
+ updateQuery = `
+ WITH next AS (
+ ${queryGetNext}
+ ),
</file context>
[internal] Confidence score: 10/10
[internal] Posted by: System Design Agent
|
@cubic-dev-local review this |
@sanxroz I've started the AI code review. It'll take a few minutes to complete. |
There was a problem hiding this comment.
3 issues found across 2 files
Prompt for AI agents (all 3 issues)
Understand the root cause of the following 3 issues and fix them.
<file name="apps/api/src/services/worker/nuq.ts">
<violation number="1" location="apps/api/src/services/worker/nuq.ts:699">
The `concurrencyLimit: 'per-owner'` feature does not actually enforce any limit. The SQL queries in `prefetchJobs` and `getJobToProcess` fetch jobs without checking the owner's current concurrency against `max_concurrency`. The feature only tracks concurrency but fails to limit it, which contradicts its name and purpose.</violation>
<violation number="2" location="apps/api/src/services/worker/nuq.ts:713">
The `max_concurrency` value for new owners is hardcoded to 100, with a `/* TODO max concurrency */` comment. This prevents per-owner or per-queue configuration of limits without code changes and undermines the flexibility of the feature, as the `max_concurrency` column in the database schema implies it should be configurable.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: The hard-coded `100` simply provides a default `max_concurrency` when inserting new owners. Callers can still adjust limits in the database afterwards, and the code does not yet consume `max_concurrency`, so this placeholder does not introduce a concrete defect worth flagging.</violation>
<violation number="3" location="apps/api/src/services/worker/nuq.ts:1083">
The `NuQ` class is designed as a generic queue, but the `concurrencyLimit: "per-owner"` feature introduces a tight coupling to a specific database schema that will cause runtime errors for any other queue using this option. The migration in `nuq.sql` only creates the concurrency table for `nuq.queue_scrape`, breaking the generic design of the `NuQ` class.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: Using `concurrencyLimit: "per-owner"` requires a per-queue owner concurrency table, just as every queue already requires its own backing table. The migration currently creates the table for the only queue that enables this option, so there is no demonstrated runtime bug or unexpected coupling beyond the existing design.</violation>
</file>
React with 👍 or 👎 to teach cubic. Mention @cubic-dev-ai to give feedback, ask questions, or re-run the review.
| ), | ||
| owner_increment AS ( | ||
| INSERT INTO ${this.queueName}_owner_concurrency (id, current_concurrency, max_concurrency) | ||
| SELECT owner_id, job_count, ${/* TODO max concurrency */ 100} |
There was a problem hiding this comment.
The max_concurrency value for new owners is hardcoded to 100, with a /* TODO max concurrency */ comment. This prevents per-owner or per-queue configuration of limits without code changes and undermines the flexibility of the feature, as the max_concurrency column in the database schema implies it should be configurable.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• GPT-5: The hard-coded 100 simply provides a default max_concurrency when inserting new owners. Callers can still adjust limits in the database afterwards, and the code does not yet consume max_concurrency, so this placeholder does not introduce a concrete defect worth flagging.
Prompt for AI agents
Address the following comment on apps/api/src/services/worker/nuq.ts at line 713:
<comment>The `max_concurrency` value for new owners is hardcoded to 100, with a `/* TODO max concurrency */` comment. This prevents per-owner or per-queue configuration of limits without code changes and undermines the flexibility of the feature, as the `max_concurrency` column in the database schema implies it should be configurable.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: The hard-coded `100` simply provides a default `max_concurrency` when inserting new owners. Callers can still adjust limits in the database afterwards, and the code does not yet consume `max_concurrency`, so this placeholder does not introduce a concrete defect worth flagging.</comment>
<file context>
@@ -677,14 +677,59 @@ class NuQ<JobData = any, JobReturnValue = any> {
+ ),
+ owner_increment AS (
+ INSERT INTO ${this.queueName}_owner_concurrency (id, current_concurrency, max_concurrency)
+ SELECT owner_id, job_count, ${/* TODO max concurrency */ 100}
+ FROM owner_counts
+ ON CONFLICT (id)
</file context>
[internal] Confidence score: 10/10
[internal] Posted by: System Design Agent
| if (this.options.concurrencyLimit === "per-owner") { | ||
| // Single query: select, update jobs, and update owner concurrency | ||
| updateQuery = ` | ||
| WITH next AS ( |
There was a problem hiding this comment.
The concurrencyLimit: 'per-owner' feature does not actually enforce any limit. The SQL queries in prefetchJobs and getJobToProcess fetch jobs without checking the owner's current concurrency against max_concurrency. The feature only tracks concurrency but fails to limit it, which contradicts its name and purpose.
Prompt for AI agents
Address the following comment on apps/api/src/services/worker/nuq.ts at line 699:
<comment>The `concurrencyLimit: 'per-owner'` feature does not actually enforce any limit. The SQL queries in `prefetchJobs` and `getJobToProcess` fetch jobs without checking the owner's current concurrency against `max_concurrency`. The feature only tracks concurrency but fails to limit it, which contradicts its name and purpose.</comment>
<file context>
@@ -677,14 +677,59 @@ class NuQ<JobData = any, JobReturnValue = any> {
+ if (this.options.concurrencyLimit === "per-owner") {
+ // Single query: select, update jobs, and update owner concurrency
+ updateQuery = `
+ WITH next AS (
+ ${queryGetNext}
+ ),
</file context>
[internal] Confidence score: 10/10
[internal] Posted by: System Design Agent
|
|
||
| export const scrapeQueue = new NuQ<ScrapeJobData>("nuq.queue_scrape"); | ||
| export const scrapeQueue = new NuQ<ScrapeJobData>("nuq.queue_scrape", { | ||
| concurrencyLimit: "per-owner", |
There was a problem hiding this comment.
The NuQ class is designed as a generic queue, but the concurrencyLimit: "per-owner" feature introduces a tight coupling to a specific database schema that will cause runtime errors for any other queue using this option. The migration in nuq.sql only creates the concurrency table for nuq.queue_scrape, breaking the generic design of the NuQ class.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• GPT-5: Using concurrencyLimit: "per-owner" requires a per-queue owner concurrency table, just as every queue already requires its own backing table. The migration currently creates the table for the only queue that enables this option, so there is no demonstrated runtime bug or unexpected coupling beyond the existing design.
Prompt for AI agents
Address the following comment on apps/api/src/services/worker/nuq.ts at line 1083:
<comment>The `NuQ` class is designed as a generic queue, but the `concurrencyLimit: "per-owner"` feature introduces a tight coupling to a specific database schema that will cause runtime errors for any other queue using this option. The migration in `nuq.sql` only creates the concurrency table for `nuq.queue_scrape`, breaking the generic design of the `NuQ` class.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: Using `concurrencyLimit: "per-owner"` requires a per-queue owner concurrency table, just as every queue already requires its own backing table. The migration currently creates the table for the only queue that enables this option, so there is no demonstrated runtime bug or unexpected coupling beyond the existing design.</comment>
<file context>
@@ -956,7 +1079,9 @@ export async function nuqHealthCheck(): Promise<boolean> {
-export const scrapeQueue = new NuQ<ScrapeJobData>("nuq.queue_scrape");
+export const scrapeQueue = new NuQ<ScrapeJobData>("nuq.queue_scrape", {
+ concurrencyLimit: "per-owner",
+});
</file context>
[internal] Confidence score: 10/10
[internal] Posted by: System Design Agent
|
@cubic-dev-local review this |
@sanxroz I've started the AI code review. It'll take a few minutes to complete. |
There was a problem hiding this comment.
3 issues found across 2 files
Prompt for AI agents (all 3 issues)
Understand the root cause of the following 3 issues and fix them.
<file name="apps/api/src/services/worker/nuq.ts">
<violation number="1" location="apps/api/src/services/worker/nuq.ts:711">
The query to fetch jobs for processing does not enforce the `max_concurrency` limit. It selects jobs without checking the owner's current active job count, and only increments the count after activation. This allows the number of concurrent jobs for an owner to exceed their defined limit, defeating the purpose of the feature.</violation>
<violation number="2" location="apps/api/src/services/worker/nuq.ts:713">
The `max_concurrency` for an owner is hardcoded to 100 when an owner's concurrency record is first created. This prevents setting different concurrency limits for different owners, undermining the 'per-owner' concurrency feature and ignoring the flexibility offered by the `nuq.queue_scrape_owner_concurrency` table schema.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: The INSERT hard-codes 100 only for the initial row creation, but owners can still have their `max_concurrency` adjusted later because the `ON CONFLICT` branch leaves that column untouched. Since the codebase currently has no logic that consumes `max_concurrency`, this is effectively a default value/TODO rather than a proven functional defect, so it doesn’t rise to a high-confidence, high-impact bug.</violation>
<violation number="3" location="apps/api/src/services/worker/nuq.ts:886">
Concurrency decrement logic duplicates `apps/api/src/services/worker/nuq.ts:jobFail()` function in its owner concurrency handling.</violation>
</file>
React with 👍 or 👎 to teach cubic. Mention @cubic-dev-ai to give feedback, ask questions, or re-run the review.
| WHERE id = $1 AND lock = $2 | ||
| RETURNING id, listen_channel_id, owner_id | ||
| ), | ||
| owner_decrement AS ( |
There was a problem hiding this comment.
Concurrency decrement logic duplicates apps/api/src/services/worker/nuq.ts:jobFail() function in its owner concurrency handling.
Prompt for AI agents
Address the following comment on apps/api/src/services/worker/nuq.ts at line 886:
<comment>Concurrency decrement logic duplicates `apps/api/src/services/worker/nuq.ts:jobFail()` function in its owner concurrency handling.</comment>
<file context>
@@ -794,26 +873,48 @@ class NuQ<JobData = any, JobReturnValue = any> {
+ WHERE id = $1 AND lock = $2
+ RETURNING id, listen_channel_id, owner_id
+ ),
+ owner_decrement AS (
+ UPDATE ${this.queueName}_owner_concurrency
+ SET current_concurrency = GREATEST(0, current_concurrency - 1)
</file context>
[internal] Confidence score: 9/10
[internal] Posted by: Duplicate Detection Agent
| WHERE owner_id IS NOT NULL | ||
| GROUP BY owner_id | ||
| ), | ||
| owner_increment AS ( |
There was a problem hiding this comment.
The query to fetch jobs for processing does not enforce the max_concurrency limit. It selects jobs without checking the owner's current active job count, and only increments the count after activation. This allows the number of concurrent jobs for an owner to exceed their defined limit, defeating the purpose of the feature.
Prompt for AI agents
Address the following comment on apps/api/src/services/worker/nuq.ts at line 711:
<comment>The query to fetch jobs for processing does not enforce the `max_concurrency` limit. It selects jobs without checking the owner's current active job count, and only increments the count after activation. This allows the number of concurrent jobs for an owner to exceed their defined limit, defeating the purpose of the feature.</comment>
<file context>
@@ -677,14 +677,59 @@ class NuQ<JobData = any, JobReturnValue = any> {
+ WHERE owner_id IS NOT NULL
+ GROUP BY owner_id
+ ),
+ owner_increment AS (
+ INSERT INTO ${this.queueName}_owner_concurrency (id, current_concurrency, max_concurrency)
+ SELECT owner_id, job_count, ${/* TODO max concurrency */ 100}
</file context>
[internal] Confidence score: 10/10
[internal] Posted by: System Design Agent
| ), | ||
| owner_increment AS ( | ||
| INSERT INTO ${this.queueName}_owner_concurrency (id, current_concurrency, max_concurrency) | ||
| SELECT owner_id, job_count, ${/* TODO max concurrency */ 100} |
There was a problem hiding this comment.
The max_concurrency for an owner is hardcoded to 100 when an owner's concurrency record is first created. This prevents setting different concurrency limits for different owners, undermining the 'per-owner' concurrency feature and ignoring the flexibility offered by the nuq.queue_scrape_owner_concurrency table schema.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• GPT-5: The INSERT hard-codes 100 only for the initial row creation, but owners can still have their max_concurrency adjusted later because the ON CONFLICT branch leaves that column untouched. Since the codebase currently has no logic that consumes max_concurrency, this is effectively a default value/TODO rather than a proven functional defect, so it doesn’t rise to a high-confidence, high-impact bug.
Prompt for AI agents
Address the following comment on apps/api/src/services/worker/nuq.ts at line 713:
<comment>The `max_concurrency` for an owner is hardcoded to 100 when an owner's concurrency record is first created. This prevents setting different concurrency limits for different owners, undermining the 'per-owner' concurrency feature and ignoring the flexibility offered by the `nuq.queue_scrape_owner_concurrency` table schema.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: The INSERT hard-codes 100 only for the initial row creation, but owners can still have their `max_concurrency` adjusted later because the `ON CONFLICT` branch leaves that column untouched. Since the codebase currently has no logic that consumes `max_concurrency`, this is effectively a default value/TODO rather than a proven functional defect, so it doesn’t rise to a high-confidence, high-impact bug.</comment>
<file context>
@@ -677,14 +677,59 @@ class NuQ<JobData = any, JobReturnValue = any> {
+ ),
+ owner_increment AS (
+ INSERT INTO ${this.queueName}_owner_concurrency (id, current_concurrency, max_concurrency)
+ SELECT owner_id, job_count, ${/* TODO max concurrency */ 100}
+ FROM owner_counts
+ ON CONFLICT (id)
</file context>
[internal] Confidence score: 10/10
[internal] Posted by: System Design Agent
|
@cubic-dev-local review this |
@sanxroz I've started the AI code review. It'll take a few minutes to complete. |
|
@cubic-dev-local review this |
@sanxroz I've started the AI code review. It'll take a few minutes to complete. |
There was a problem hiding this comment.
3 issues found across 2 files
Prompt for AI agents (all 3 issues)
Understand the root cause of the following 3 issues and fix them.
<file name="apps/api/src/services/worker/nuq.ts">
<violation number="1" location="apps/api/src/services/worker/nuq.ts:699">
The per-owner concurrency limit is tracked but not enforced during job activation. The SQL query in `prefetchJobs` and `getJobToProcess` selects jobs without checking if the owner has reached their `max_concurrency` limit from the `${this.queueName}_owner_concurrency` table. This defeats the purpose of the `concurrencyLimit: "per-owner"` option, as the system will exceed the intended concurrency limits.</violation>
<violation number="2" location="apps/api/src/services/worker/nuq.ts:713">
When creating a new owner record in the `${this.queueName}_owner_concurrency` table, `max_concurrency` is hardcoded to 100. This value is not configurable and is applied to every new job owner, which is an inflexible design and likely not the intended long-term behavior, as indicated by the `/* TODO max concurrency */` comment.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: The `/* TODO max concurrency */ 100` literal is just a default inserted when a new owner row is created. There is no existing configuration API or requirement in this change set to make the cap dynamic, so this is a conscious placeholder rather than a functional bug. Flagging it would amount to a low-severity design suggestion.</violation>
<violation number="3" location="apps/api/src/services/worker/nuq.ts:1082">
The per-owner concurrency feature increments a counter when a job is activated but only decrements it on explicit completion or failure. There is no mechanism to handle jobs that time out or are left dangling by a crashed worker. This will cause the `current_concurrency` count to become permanently inflated, leading to incorrect concurrency tracking.</violation>
</file>
React with 👍 or 👎 to teach cubic. Mention @cubic-dev-ai to give feedback, ask questions, or re-run the review.
| // === Instances | ||
|
|
||
| export const scrapeQueue = new NuQ<ScrapeJobData>("nuq.queue_scrape"); | ||
| export const scrapeQueue = new NuQ<ScrapeJobData>("nuq.queue_scrape", { |
There was a problem hiding this comment.
The per-owner concurrency feature increments a counter when a job is activated but only decrements it on explicit completion or failure. There is no mechanism to handle jobs that time out or are left dangling by a crashed worker. This will cause the current_concurrency count to become permanently inflated, leading to incorrect concurrency tracking.
Prompt for AI agents
Address the following comment on apps/api/src/services/worker/nuq.ts at line 1082:
<comment>The per-owner concurrency feature increments a counter when a job is activated but only decrements it on explicit completion or failure. There is no mechanism to handle jobs that time out or are left dangling by a crashed worker. This will cause the `current_concurrency` count to become permanently inflated, leading to incorrect concurrency tracking.</comment>
<file context>
@@ -956,7 +1079,9 @@ export async function nuqHealthCheck(): Promise<boolean> {
// === Instances
-export const scrapeQueue = new NuQ<ScrapeJobData>("nuq.queue_scrape");
+export const scrapeQueue = new NuQ<ScrapeJobData>("nuq.queue_scrape", {
+ concurrencyLimit: "per-owner",
+});
</file context>
[internal] Confidence score: 9/10
[internal] Posted by: System Design Agent
| ), | ||
| owner_increment AS ( | ||
| INSERT INTO ${this.queueName}_owner_concurrency (id, current_concurrency, max_concurrency) | ||
| SELECT owner_id, job_count, ${/* TODO max concurrency */ 100} |
There was a problem hiding this comment.
When creating a new owner record in the ${this.queueName}_owner_concurrency table, max_concurrency is hardcoded to 100. This value is not configurable and is applied to every new job owner, which is an inflexible design and likely not the intended long-term behavior, as indicated by the /* TODO max concurrency */ comment.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• GPT-5: The /* TODO max concurrency */ 100 literal is just a default inserted when a new owner row is created. There is no existing configuration API or requirement in this change set to make the cap dynamic, so this is a conscious placeholder rather than a functional bug. Flagging it would amount to a low-severity design suggestion.
Prompt for AI agents
Address the following comment on apps/api/src/services/worker/nuq.ts at line 713:
<comment>When creating a new owner record in the `${this.queueName}_owner_concurrency` table, `max_concurrency` is hardcoded to 100. This value is not configurable and is applied to every new job owner, which is an inflexible design and likely not the intended long-term behavior, as indicated by the `/* TODO max concurrency */` comment.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: The `/* TODO max concurrency */ 100` literal is just a default inserted when a new owner row is created. There is no existing configuration API or requirement in this change set to make the cap dynamic, so this is a conscious placeholder rather than a functional bug. Flagging it would amount to a low-severity design suggestion.</comment>
<file context>
@@ -677,14 +677,59 @@ class NuQ<JobData = any, JobReturnValue = any> {
+ ),
+ owner_increment AS (
+ INSERT INTO ${this.queueName}_owner_concurrency (id, current_concurrency, max_concurrency)
+ SELECT owner_id, job_count, ${/* TODO max concurrency */ 100}
+ FROM owner_counts
+ ON CONFLICT (id)
</file context>
[internal] Confidence score: 10/10
[internal] Posted by: System Design Agent
| if (this.options.concurrencyLimit === "per-owner") { | ||
| // Single query: select, update jobs, and update owner concurrency | ||
| updateQuery = ` | ||
| WITH next AS ( |
There was a problem hiding this comment.
The per-owner concurrency limit is tracked but not enforced during job activation. The SQL query in prefetchJobs and getJobToProcess selects jobs without checking if the owner has reached their max_concurrency limit from the ${this.queueName}_owner_concurrency table. This defeats the purpose of the concurrencyLimit: "per-owner" option, as the system will exceed the intended concurrency limits.
Prompt for AI agents
Address the following comment on apps/api/src/services/worker/nuq.ts at line 699:
<comment>The per-owner concurrency limit is tracked but not enforced during job activation. The SQL query in `prefetchJobs` and `getJobToProcess` selects jobs without checking if the owner has reached their `max_concurrency` limit from the `${this.queueName}_owner_concurrency` table. This defeats the purpose of the `concurrencyLimit: "per-owner"` option, as the system will exceed the intended concurrency limits.</comment>
<file context>
@@ -677,14 +677,59 @@ class NuQ<JobData = any, JobReturnValue = any> {
+ if (this.options.concurrencyLimit === "per-owner") {
+ // Single query: select, update jobs, and update owner concurrency
+ updateQuery = `
+ WITH next AS (
+ ${queryGetNext}
+ ),
</file context>
[internal] Confidence score: 10/10
[internal] Posted by: System Design Agent
|
@cubic-dev-local review this |
@sanxroz I've started the AI code review. It'll take a few minutes to complete. |
cubic is running your AI reviewPipeline
|
There was a problem hiding this comment.
1 issue found across 2 files
Prompt for AI agents (all 1 issues)
Understand the root cause of the following 1 issues and fix them.
<file name="apps/api/src/services/worker/nuq.ts">
<violation number="1" location="apps/api/src/services/worker/nuq.ts:887">
SQL CTE for decrementing owner concurrency duplicates itself within apps/api/src/services/worker/nuq.ts:jobFail() from apps/api/src/services/worker/nuq.ts:jobFinish(). This common logic should be extracted into a shared private helper function within the NuQ class.
*DEV MODE: This violation would have been filtered out by screening filters. Failing filters: typescriptUnderstanding.*</violation>
</file>
React with 👍 or 👎 to teach cubic. Mention @cubic-dev-ai to give feedback, ask questions, or re-run the review.
| RETURNING id, listen_channel_id, owner_id | ||
| ), | ||
| owner_decrement AS ( | ||
| UPDATE ${this.queueName}_owner_concurrency |
There was a problem hiding this comment.
SQL CTE for decrementing owner concurrency duplicates itself within apps/api/src/services/worker/nuq.ts:jobFail() from apps/api/src/services/worker/nuq.ts:jobFinish(). This common logic should be extracted into a shared private helper function within the NuQ class.
DEV MODE: This violation would have been filtered out by screening filters. Failing filters: typescriptUnderstanding.
Prompt for AI agents
Address the following comment on apps/api/src/services/worker/nuq.ts at line 887:
<comment>SQL CTE for decrementing owner concurrency duplicates itself within apps/api/src/services/worker/nuq.ts:jobFail() from apps/api/src/services/worker/nuq.ts:jobFinish(). This common logic should be extracted into a shared private helper function within the NuQ class.
*DEV MODE: This violation would have been filtered out by screening filters. Failing filters: typescriptUnderstanding.*</comment>
<file context>
@@ -794,26 +873,48 @@ class NuQ<JobData = any, JobReturnValue = any> {
+ RETURNING id, listen_channel_id, owner_id
+ ),
+ owner_decrement AS (
+ UPDATE ${this.queueName}_owner_concurrency
+ SET current_concurrency = GREATEST(0, current_concurrency - 1)
+ FROM updated
</file context>
[internal] Confidence score: 9/10
[internal] Posted by: Duplicate Detection Agent
|
@cubic-dev-local review this |
@sanxroz I've started the AI code review. It'll take a few minutes to complete. |
There was a problem hiding this comment.
1 issue found across 2 files
Prompt for AI agents (all 1 issues)
Understand the root cause of the following 1 issues and fix them.
<file name="apps/api/src/services/worker/nuq.ts">
<violation number="1" location="apps/api/src/services/worker/nuq.ts:887">
SQL logic for decrementing owner concurrency duplicates in NuQ.jobFail() (new code) and NuQ.jobFinish() (new code).</violation>
</file>
React with 👍 or 👎 to teach cubic. Mention @cubic-dev-ai to give feedback, ask questions, or re-run the review.
| RETURNING id, listen_channel_id, owner_id | ||
| ), | ||
| owner_decrement AS ( | ||
| UPDATE ${this.queueName}_owner_concurrency |
There was a problem hiding this comment.
SQL logic for decrementing owner concurrency duplicates in NuQ.jobFail() (new code) and NuQ.jobFinish() (new code).
Prompt for AI agents
Address the following comment on apps/api/src/services/worker/nuq.ts at line 887:
<comment>SQL logic for decrementing owner concurrency duplicates in NuQ.jobFail() (new code) and NuQ.jobFinish() (new code).</comment>
<file context>
@@ -794,26 +873,48 @@ class NuQ<JobData = any, JobReturnValue = any> {
+ RETURNING id, listen_channel_id, owner_id
+ ),
+ owner_decrement AS (
+ UPDATE ${this.queueName}_owner_concurrency
+ SET current_concurrency = GREATEST(0, current_concurrency - 1)
+ FROM updated
</file context>
[internal] Confidence score: 9/10
[internal] Posted by: Duplicate Detection Agent
|
@cubic-dev-local review this |
@sanxroz I've started the AI code review. It'll take a few minutes to complete. |
There was a problem hiding this comment.
2 issues found across 2 files
Prompt for AI agents (all 2 issues)
Understand the root cause of the following 2 issues and fix them.
<file name="apps/api/src/services/worker/nuq.ts">
<violation number="1" location="apps/api/src/services/worker/nuq.ts:699">
The per-owner concurrency limit is not enforced when activating jobs. The logic increments the owner's concurrency count *after* a job is selected, but fails to check if the owner is already at their limit *before* selection. This architectural flaw makes the concurrency limit ineffective.</violation>
<violation number="2" location="apps/api/src/services/worker/nuq.ts:712">
The `max_concurrency` limit is hardcoded to 100 for all owners when their first job is processed. This is an inflexible design and is marked by a `TODO` comment, indicating it's an incomplete feature that prevents customized concurrency limits for different users or tiers.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: The code simply seeds new owner rows with a default max_concurrency of 100. Nothing in the implementation prevents updating that value later, and the TODO comment alone is not evidence of a defect. This is a design preference/future enhancement, not a clear correctness issue, so it should not be reported.</violation>
</file>
React with 👍 or 👎 to teach cubic. Mention @cubic-dev-ai to give feedback, ask questions, or re-run the review.
| GROUP BY owner_id | ||
| ), | ||
| owner_increment AS ( | ||
| INSERT INTO ${this.queueName}_owner_concurrency (id, current_concurrency, max_concurrency) |
There was a problem hiding this comment.
The max_concurrency limit is hardcoded to 100 for all owners when their first job is processed. This is an inflexible design and is marked by a TODO comment, indicating it's an incomplete feature that prevents customized concurrency limits for different users or tiers.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• GPT-5: The code simply seeds new owner rows with a default max_concurrency of 100. Nothing in the implementation prevents updating that value later, and the TODO comment alone is not evidence of a defect. This is a design preference/future enhancement, not a clear correctness issue, so it should not be reported.
Prompt for AI agents
Address the following comment on apps/api/src/services/worker/nuq.ts at line 712:
<comment>The `max_concurrency` limit is hardcoded to 100 for all owners when their first job is processed. This is an inflexible design and is marked by a `TODO` comment, indicating it's an incomplete feature that prevents customized concurrency limits for different users or tiers.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: The code simply seeds new owner rows with a default max_concurrency of 100. Nothing in the implementation prevents updating that value later, and the TODO comment alone is not evidence of a defect. This is a design preference/future enhancement, not a clear correctness issue, so it should not be reported.</comment>
<file context>
@@ -677,14 +677,59 @@ class NuQ<JobData = any, JobReturnValue = any> {
+ GROUP BY owner_id
+ ),
+ owner_increment AS (
+ INSERT INTO ${this.queueName}_owner_concurrency (id, current_concurrency, max_concurrency)
+ SELECT owner_id, job_count, ${/* TODO max concurrency */ 100}
+ FROM owner_counts
</file context>
[internal] Confidence score: 10/10
[internal] Posted by: System Design Agent
| if (this.options.concurrencyLimit === "per-owner") { | ||
| // Single query: select, update jobs, and update owner concurrency | ||
| updateQuery = ` | ||
| WITH next AS ( |
There was a problem hiding this comment.
The per-owner concurrency limit is not enforced when activating jobs. The logic increments the owner's concurrency count after a job is selected, but fails to check if the owner is already at their limit before selection. This architectural flaw makes the concurrency limit ineffective.
Prompt for AI agents
Address the following comment on apps/api/src/services/worker/nuq.ts at line 699:
<comment>The per-owner concurrency limit is not enforced when activating jobs. The logic increments the owner's concurrency count *after* a job is selected, but fails to check if the owner is already at their limit *before* selection. This architectural flaw makes the concurrency limit ineffective.</comment>
<file context>
@@ -677,14 +677,59 @@ class NuQ<JobData = any, JobReturnValue = any> {
+ if (this.options.concurrencyLimit === "per-owner") {
+ // Single query: select, update jobs, and update owner concurrency
+ updateQuery = `
+ WITH next AS (
+ ${queryGetNext}
+ ),
</file context>
[internal] Confidence score: 10/10
[internal] Posted by: System Design Agent
|
@cubic-dev-local review this |
@sanxroz I've started the AI code review. It'll take a few minutes to complete. |
There was a problem hiding this comment.
1 issue found across 2 files
Prompt for AI agents (all 1 issues)
Understand the root cause of the following 1 issues and fix them.
<file name="apps/api/src/services/worker/nuq.ts">
<violation number="1" location="apps/api/src/services/worker/nuq.ts:886">
SQL CTE for decrementing owner concurrency duplicates the implementation in jobFail().
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: Although the owner_decrement CTE appears in both jobFinish and jobFail, each query needs to perform the same SQL operation when finishing or failing a job. The small duplication (only a few lines in separate SQL strings) is intentional and low impact, making it unsuitable to flag as a significant maintainability issue.</violation>
</file>
React with 👍 or 👎 to teach cubic. Mention @cubic-dev-ai to give feedback, ask questions, or re-run the review.
| WHERE id = $1 AND lock = $2 | ||
| RETURNING id, listen_channel_id, owner_id | ||
| ), | ||
| owner_decrement AS ( |
There was a problem hiding this comment.
SQL CTE for decrementing owner concurrency duplicates the implementation in jobFail().
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• GPT-5: Although the owner_decrement CTE appears in both jobFinish and jobFail, each query needs to perform the same SQL operation when finishing or failing a job. The small duplication (only a few lines in separate SQL strings) is intentional and low impact, making it unsuitable to flag as a significant maintainability issue.
Prompt for AI agents
Address the following comment on apps/api/src/services/worker/nuq.ts at line 886:
<comment>SQL CTE for decrementing owner concurrency duplicates the implementation in jobFail().
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: Although the owner_decrement CTE appears in both jobFinish and jobFail, each query needs to perform the same SQL operation when finishing or failing a job. The small duplication (only a few lines in separate SQL strings) is intentional and low impact, making it unsuitable to flag as a significant maintainability issue.</comment>
<file context>
@@ -794,26 +873,48 @@ class NuQ<JobData = any, JobReturnValue = any> {
+ WHERE id = $1 AND lock = $2
+ RETURNING id, listen_channel_id, owner_id
+ ),
+ owner_decrement AS (
+ UPDATE ${this.queueName}_owner_concurrency
+ SET current_concurrency = GREATEST(0, current_concurrency - 1)
</file context>
[internal] Confidence score: 10/10
[internal] Posted by: Duplicate Detection Agent
|
@cubic-dev-local review this |
@sanxroz I've started the AI code review. It'll take a few minutes to complete. |
cubic is running your AI reviewPipeline
|
There was a problem hiding this comment.
2 issues found across 2 files
Prompt for AI agents (all 2 issues)
Understand the root cause of the following 2 issues and fix them.
<file name="apps/api/src/services/worker/nuq.ts">
<violation number="1" location="apps/api/src/services/worker/nuq.ts:695">
SQL query construction logic duplicates between `jobFinish()` and `jobFail()` methods in `apps/api/src/services/worker/nuq.ts`. This leads to repeated code for updating job status and decrementing owner concurrency.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: The two SQL snippets share similar structure, but they serve distinct status update flows and the minimal duplication keeps the logic straightforward. Refactoring into a helper would add complexity without clear functional benefit, so this is not a high-impact issue to report.</violation>
<violation number="2" location="apps/api/src/services/worker/nuq.ts:1082">
The per-owner concurrency feature does not enforce the `max_concurrency` limit. It only tracks the number of active jobs, which contradicts the purpose of a `concurrencyLimit` option and the `max_concurrency` DB field, potentially leading to system overload.</violation>
</file>
React with 👍 or 👎 to teach cubic. Mention @cubic-dev-ai to give feedback, ask questions, or re-run the review.
| RETURNING ${this.jobReturning.map(x => `q.${x}`).join(", ")} | ||
| ` | ||
|
|
||
| let updateQuery: string; |
There was a problem hiding this comment.
SQL query construction logic duplicates between jobFinish() and jobFail() methods in apps/api/src/services/worker/nuq.ts. This leads to repeated code for updating job status and decrementing owner concurrency.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• GPT-5: The two SQL snippets share similar structure, but they serve distinct status update flows and the minimal duplication keeps the logic straightforward. Refactoring into a helper would add complexity without clear functional benefit, so this is not a high-impact issue to report.
Prompt for AI agents
Address the following comment on apps/api/src/services/worker/nuq.ts at line 695:
<comment>SQL query construction logic duplicates between `jobFinish()` and `jobFail()` methods in `apps/api/src/services/worker/nuq.ts`. This leads to repeated code for updating job status and decrementing owner concurrency.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: The two SQL snippets share similar structure, but they serve distinct status update flows and the minimal duplication keeps the logic straightforward. Refactoring into a helper would add complexity without clear functional benefit, so this is not a high-impact issue to report.</comment>
<file context>
@@ -677,14 +677,59 @@ class NuQ<JobData = any, JobReturnValue = any> {
+ RETURNING ${this.jobReturning.map(x => `q.${x}`).join(", ")}
+ `
+
+ let updateQuery: string;
+ if (this.options.concurrencyLimit === "per-owner") {
+ // Single query: select, update jobs, and update owner concurrency
</file context>
[internal] Confidence score: 9/10
[internal] Posted by: Duplicate Detection Agent
| // === Instances | ||
|
|
||
| export const scrapeQueue = new NuQ<ScrapeJobData>("nuq.queue_scrape"); | ||
| export const scrapeQueue = new NuQ<ScrapeJobData>("nuq.queue_scrape", { |
There was a problem hiding this comment.
The per-owner concurrency feature does not enforce the max_concurrency limit. It only tracks the number of active jobs, which contradicts the purpose of a concurrencyLimit option and the max_concurrency DB field, potentially leading to system overload.
Prompt for AI agents
Address the following comment on apps/api/src/services/worker/nuq.ts at line 1082:
<comment>The per-owner concurrency feature does not enforce the `max_concurrency` limit. It only tracks the number of active jobs, which contradicts the purpose of a `concurrencyLimit` option and the `max_concurrency` DB field, potentially leading to system overload.</comment>
<file context>
@@ -956,7 +1079,9 @@ export async function nuqHealthCheck(): Promise<boolean> {
// === Instances
-export const scrapeQueue = new NuQ<ScrapeJobData>("nuq.queue_scrape");
+export const scrapeQueue = new NuQ<ScrapeJobData>("nuq.queue_scrape", {
+ concurrencyLimit: "per-owner",
+});
</file context>
[internal] Confidence score: 10/10
[internal] Posted by: System Design Agent
|
@cubic-dev-local review this |
@sanxroz I've started the AI code review. It'll take a few minutes to complete. |
There was a problem hiding this comment.
1 issue found across 2 files
Prompt for AI agents (all 1 issues)
Understand the root cause of the following 1 issues and fix them.
<file name="apps/api/src/services/worker/nuq.ts">
<violation number="1" location="apps/api/src/services/worker/nuq.ts:682">
Selecting queued jobs without checking each owner's current/max concurrency means `concurrencyLimit: "per-owner"` is never enforced. For example, an owner capped at 5 can still have every queued job activated, and the counter just increments past 5. Please join/filter against `${this.queueName}_owner_concurrency` (here and in the similar getJobToProcess query) so owners at their limit are skipped until capacity frees up.</violation>
</file>
React with 👍 or 👎 to teach cubic. Mention @cubic-dev-ai to give feedback, ask questions, or re-run the review.
| ).rows.map(row => this.rowToJob(row)!); | ||
| const queryGetNext = ` | ||
| SELECT ${this.jobReturning.map(x => `${this.queueName}.${x}`).join(", ")} FROM ${this.queueName} | ||
| WHERE ${this.queueName}.status = 'queued'::nuq.job_status |
There was a problem hiding this comment.
Selecting queued jobs without checking each owner's current/max concurrency means concurrencyLimit: "per-owner" is never enforced. For example, an owner capped at 5 can still have every queued job activated, and the counter just increments past 5. Please join/filter against ${this.queueName}_owner_concurrency (here and in the similar getJobToProcess query) so owners at their limit are skipped until capacity frees up.
Prompt for AI agents
Address the following comment on apps/api/src/services/worker/nuq.ts at line 682:
<comment>Selecting queued jobs without checking each owner's current/max concurrency means `concurrencyLimit: "per-owner"` is never enforced. For example, an owner capped at 5 can still have every queued job activated, and the counter just increments past 5. Please join/filter against `${this.queueName}_owner_concurrency` (here and in the similar getJobToProcess query) so owners at their limit are skipped until capacity frees up.</comment>
<file context>
@@ -677,14 +677,59 @@ class NuQ<JobData = any, JobReturnValue = any> {
- ).rows.map(row => this.rowToJob(row)!);
+ const queryGetNext = `
+ SELECT ${this.jobReturning.map(x => `${this.queueName}.${x}`).join(", ")} FROM ${this.queueName}
+ WHERE ${this.queueName}.status = 'queued'::nuq.job_status
+ ORDER BY ${this.queueName}.priority ASC, ${this.queueName}.created_at ASC
+ FOR UPDATE SKIP LOCKED LIMIT 500
</file context>
[internal] Confidence score: 8/10
[internal] Posted by: General AI Review Agent
|
@cubic-dev-local review this |
@sanxroz I've started the AI code review. It'll take a few minutes to complete. |
There was a problem hiding this comment.
2 issues found across 2 files
Prompt for AI agents (all 2 issues)
Understand the root cause of the following 2 issues and fix them.
<file name="apps/api/src/services/worker/nuq.ts">
<violation number="1" location="apps/api/src/services/worker/nuq.ts:698">
The per-owner concurrency logic tracks the number of active jobs but fails to enforce the `max_concurrency` limit. The SQL query that fetches the next jobs (`queryGetNext`) does not check the owner's current concurrency, allowing the system to exceed the specified limit.</violation>
<violation number="2" location="apps/api/src/services/worker/nuq.ts:713">
The `max_concurrency` for an owner is hardcoded to 100. This contradicts the database schema design in `nuq.sql`, which allows for a per-owner `max_concurrency`, and makes the feature inflexible. The `/* TODO max concurrency */` comment indicates this is a known deficiency.</violation>
</file>
React with 👍 or 👎 to teach cubic. Mention @cubic-dev-ai to give feedback, ask questions, or re-run the review.
| ), | ||
| owner_increment AS ( | ||
| INSERT INTO ${this.queueName}_owner_concurrency (id, current_concurrency, max_concurrency) | ||
| SELECT owner_id, job_count, ${/* TODO max concurrency */ 100} |
There was a problem hiding this comment.
The max_concurrency for an owner is hardcoded to 100. This contradicts the database schema design in nuq.sql, which allows for a per-owner max_concurrency, and makes the feature inflexible. The /* TODO max concurrency */ comment indicates this is a known deficiency.
Prompt for AI agents
Address the following comment on apps/api/src/services/worker/nuq.ts at line 713:
<comment>The `max_concurrency` for an owner is hardcoded to 100. This contradicts the database schema design in `nuq.sql`, which allows for a per-owner `max_concurrency`, and makes the feature inflexible. The `/* TODO max concurrency */` comment indicates this is a known deficiency.</comment>
<file context>
@@ -677,14 +677,59 @@ class NuQ<JobData = any, JobReturnValue = any> {
+ ),
+ owner_increment AS (
+ INSERT INTO ${this.queueName}_owner_concurrency (id, current_concurrency, max_concurrency)
+ SELECT owner_id, job_count, ${/* TODO max concurrency */ 100}
+ FROM owner_counts
+ ON CONFLICT (id)
</file context>
[internal] Confidence score: 10/10
[internal] Posted by: System Design Agent
| let updateQuery: string; | ||
| if (this.options.concurrencyLimit === "per-owner") { | ||
| // Single query: select, update jobs, and update owner concurrency | ||
| updateQuery = ` |
There was a problem hiding this comment.
The per-owner concurrency logic tracks the number of active jobs but fails to enforce the max_concurrency limit. The SQL query that fetches the next jobs (queryGetNext) does not check the owner's current concurrency, allowing the system to exceed the specified limit.
Prompt for AI agents
Address the following comment on apps/api/src/services/worker/nuq.ts at line 698:
<comment>The per-owner concurrency logic tracks the number of active jobs but fails to enforce the `max_concurrency` limit. The SQL query that fetches the next jobs (`queryGetNext`) does not check the owner's current concurrency, allowing the system to exceed the specified limit.</comment>
<file context>
@@ -677,14 +677,59 @@ class NuQ<JobData = any, JobReturnValue = any> {
+ let updateQuery: string;
+ if (this.options.concurrencyLimit === "per-owner") {
+ // Single query: select, update jobs, and update owner concurrency
+ updateQuery = `
+ WITH next AS (
+ ${queryGetNext}
</file context>
[internal] Confidence score: 10/10
[internal] Posted by: System Design Agent
|
@cubic-dev-local review this |
@sanxroz I've started the AI code review. It'll take a few minutes to complete. |
There was a problem hiding this comment.
4 issues found across 2 files
Prompt for AI agents (all 4 issues)
Understand the root cause of the following 4 issues and fix them.
<file name="apps/api/src/services/worker/nuq.ts">
<violation number="1" location="apps/api/src/services/worker/nuq.ts:699">
The per-owner concurrency limit is tracked but not enforced. The query that activates new jobs increments the `current_concurrency` but does not check it against `max_concurrency`, allowing an owner's active jobs to exceed their defined limit. This defeats the purpose of `concurrencyLimit: "per-owner"`.</violation>
<violation number="2" location="apps/api/src/services/worker/nuq.ts:712">
SQL logic for incrementing owner concurrency is duplicated between `prefetchJobs()` and `getJobToProcess()`.</violation>
<violation number="3" location="apps/api/src/services/worker/nuq.ts:713">
The `max_concurrency` for an owner is hardcoded to 100. This should be a configurable parameter passed via `NuQOptions` to allow for flexible concurrency management per owner, as implied by the database schema changes.</violation>
<violation number="4" location="apps/api/src/services/worker/nuq.ts:886">
SQL logic for decrementing owner concurrency is duplicated between `jobFinish()` and `jobFail()`.</violation>
</file>
React with 👍 or 👎 to teach cubic. Mention @cubic-dev-ai to give feedback, ask questions, or re-run the review.
| WHERE id = $1 AND lock = $2 | ||
| RETURNING id, listen_channel_id, owner_id | ||
| ), | ||
| owner_decrement AS ( |
There was a problem hiding this comment.
SQL logic for decrementing owner concurrency is duplicated between jobFinish() and jobFail().
Prompt for AI agents
Address the following comment on apps/api/src/services/worker/nuq.ts at line 886:
<comment>SQL logic for decrementing owner concurrency is duplicated between `jobFinish()` and `jobFail()`.</comment>
<file context>
@@ -794,26 +873,48 @@ class NuQ<JobData = any, JobReturnValue = any> {
+ WHERE id = $1 AND lock = $2
+ RETURNING id, listen_channel_id, owner_id
+ ),
+ owner_decrement AS (
+ UPDATE ${this.queueName}_owner_concurrency
+ SET current_concurrency = GREATEST(0, current_concurrency - 1)
</file context>
[internal] Confidence score: 10/10
[internal] Posted by: Duplicate Detection Agent
| GROUP BY owner_id | ||
| ), | ||
| owner_increment AS ( | ||
| INSERT INTO ${this.queueName}_owner_concurrency (id, current_concurrency, max_concurrency) |
There was a problem hiding this comment.
SQL logic for incrementing owner concurrency is duplicated between prefetchJobs() and getJobToProcess().
Prompt for AI agents
Address the following comment on apps/api/src/services/worker/nuq.ts at line 712:
<comment>SQL logic for incrementing owner concurrency is duplicated between `prefetchJobs()` and `getJobToProcess()`.</comment>
<file context>
@@ -677,14 +677,59 @@ class NuQ<JobData = any, JobReturnValue = any> {
+ GROUP BY owner_id
+ ),
+ owner_increment AS (
+ INSERT INTO ${this.queueName}_owner_concurrency (id, current_concurrency, max_concurrency)
+ SELECT owner_id, job_count, ${/* TODO max concurrency */ 100}
+ FROM owner_counts
</file context>
[internal] Confidence score: 9/10
[internal] Posted by: Duplicate Detection Agent
| ), | ||
| owner_increment AS ( | ||
| INSERT INTO ${this.queueName}_owner_concurrency (id, current_concurrency, max_concurrency) | ||
| SELECT owner_id, job_count, ${/* TODO max concurrency */ 100} |
There was a problem hiding this comment.
The max_concurrency for an owner is hardcoded to 100. This should be a configurable parameter passed via NuQOptions to allow for flexible concurrency management per owner, as implied by the database schema changes.
Prompt for AI agents
Address the following comment on apps/api/src/services/worker/nuq.ts at line 713:
<comment>The `max_concurrency` for an owner is hardcoded to 100. This should be a configurable parameter passed via `NuQOptions` to allow for flexible concurrency management per owner, as implied by the database schema changes.</comment>
<file context>
@@ -677,14 +677,59 @@ class NuQ<JobData = any, JobReturnValue = any> {
+ ),
+ owner_increment AS (
+ INSERT INTO ${this.queueName}_owner_concurrency (id, current_concurrency, max_concurrency)
+ SELECT owner_id, job_count, ${/* TODO max concurrency */ 100}
+ FROM owner_counts
+ ON CONFLICT (id)
</file context>
[internal] Confidence score: 10/10
[internal] Posted by: System Design Agent
| if (this.options.concurrencyLimit === "per-owner") { | ||
| // Single query: select, update jobs, and update owner concurrency | ||
| updateQuery = ` | ||
| WITH next AS ( |
There was a problem hiding this comment.
The per-owner concurrency limit is tracked but not enforced. The query that activates new jobs increments the current_concurrency but does not check it against max_concurrency, allowing an owner's active jobs to exceed their defined limit. This defeats the purpose of concurrencyLimit: "per-owner".
Prompt for AI agents
Address the following comment on apps/api/src/services/worker/nuq.ts at line 699:
<comment>The per-owner concurrency limit is tracked but not enforced. The query that activates new jobs increments the `current_concurrency` but does not check it against `max_concurrency`, allowing an owner's active jobs to exceed their defined limit. This defeats the purpose of `concurrencyLimit: "per-owner"`.</comment>
<file context>
@@ -677,14 +677,59 @@ class NuQ<JobData = any, JobReturnValue = any> {
+ if (this.options.concurrencyLimit === "per-owner") {
+ // Single query: select, update jobs, and update owner concurrency
+ updateQuery = `
+ WITH next AS (
+ ${queryGetNext}
+ ),
</file context>
[internal] Confidence score: 10/10
[internal] Posted by: System Design Agent
|
@cubic-dev-local review this |
@sanxroz I've started the AI code review. It'll take a few minutes to complete. |
There was a problem hiding this comment.
5 issues found across 2 files
Prompt for AI agents (all 5 issues)
Understand the root cause of the following 5 issues and fix them.
<file name="apps/api/src/services/worker/nuq.ts">
<violation number="1" location="apps/api/src/services/worker/nuq.ts:681">
SQL Injection risk: unescaped dynamic table/identifier (queueName) directly embedded in SQL.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: `this.queueName` is not user-controlled; the only instantiation is `export const scrapeQueue = new NuQ<ScrapeJobData>("nuq.queue_scrape", ...)`, and the class itself is not exported. Because the queue name is a static, trusted identifier, interpolating it into the query cannot be exploited for SQL injection.</violation>
<violation number="2" location="apps/api/src/services/worker/nuq.ts:886">
The SQL CTE `owner_decrement` for managing owner concurrency is duplicated in `apps/api/src/services/worker/nuq.ts` within the `jobFinish()` and `jobFail()` methods. This exact duplication of complex database logic increases maintenance burden and risk of inconsistencies.</violation>
<violation number="3" location="apps/api/src/services/worker/nuq.ts:907">
SQL Injection: pg_notify channel built via string interpolation instead of parameters.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: The `pg_notify` call interpolates `this.queueName`, but that value is the same static string `"nuq.queue_scrape"` defined when `NuQ` is instantiated. Without any untrusted input reaching `queueName`, the channel name is fixed and cannot enable injection.</violation>
</file>
<file name="apps/nuq-postgres/nuq.sql">
<violation number="1" location="apps/nuq-postgres/nuq.sql:42">
`current_concurrency` needs a default of 0 so new owner rows can be created without explicitly supplying the starting count. Without it, inserts that rely on the counter starting at zero will hit a NOT NULL violation whenever the column is omitted.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: The application code that creates rows in nuq.queue_scrape_owner_concurrency always supplies a current_concurrency value explicitly. For example, apps/api/src/services/worker/nuq.ts inserts (id, current_concurrency, max_concurrency) with a computed job_count or 1, so no insert relies on a database default. Therefore, lack of a default will not cause NOT NULL errors.</violation>
<violation number="2" location="apps/nuq-postgres/nuq.sql:43">
`max_concurrency` should default to the intended baseline (100) so callers can create owner rows without explicitly providing that value each time; otherwise inserts that omit it will violate the NOT NULL constraint.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: All inserts into nuq.queue_scrape_owner_concurrency explicitly set max_concurrency to 100 in the application logic (apps/api/src/services/worker/nuq.ts). Since the column is always provided, the absence of a database default does not lead to NULL inserts or constraint violations.</violation>
</file>
React with 👍 or 👎 to teach cubic. Mention @cubic-dev-ai to give feedback, ask questions, or re-run the review.
| WHERE id = $1 AND lock = $2 | ||
| RETURNING id, listen_channel_id, owner_id | ||
| ), | ||
| owner_decrement AS ( |
There was a problem hiding this comment.
The SQL CTE owner_decrement for managing owner concurrency is duplicated in apps/api/src/services/worker/nuq.ts within the jobFinish() and jobFail() methods. This exact duplication of complex database logic increases maintenance burden and risk of inconsistencies.
Prompt for AI agents
Address the following comment on apps/api/src/services/worker/nuq.ts at line 886:
<comment>The SQL CTE `owner_decrement` for managing owner concurrency is duplicated in `apps/api/src/services/worker/nuq.ts` within the `jobFinish()` and `jobFail()` methods. This exact duplication of complex database logic increases maintenance burden and risk of inconsistencies.</comment>
<file context>
@@ -794,26 +873,48 @@ class NuQ<JobData = any, JobReturnValue = any> {
+ WHERE id = $1 AND lock = $2
+ RETURNING id, listen_channel_id, owner_id
+ ),
+ owner_decrement AS (
+ UPDATE ${this.queueName}_owner_concurrency
+ SET current_concurrency = GREATEST(0, current_concurrency - 1)
</file context>
[internal] Confidence score: 9/10
[internal] Posted by: Duplicate Detection Agent
| CREATE TABLE IF NOT EXISTS nuq.queue_scrape_owner_concurrency ( | ||
| id uuid NOT NULL, | ||
| current_concurrency int8 NOT NULL, | ||
| max_concurrency int8 NOT NULL, |
There was a problem hiding this comment.
max_concurrency should default to the intended baseline (100) so callers can create owner rows without explicitly providing that value each time; otherwise inserts that omit it will violate the NOT NULL constraint.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• GPT-5: All inserts into nuq.queue_scrape_owner_concurrency explicitly set max_concurrency to 100 in the application logic (apps/api/src/services/worker/nuq.ts). Since the column is always provided, the absence of a database default does not lead to NULL inserts or constraint violations.
Prompt for AI agents
Address the following comment on apps/nuq-postgres/nuq.sql at line 43:
<comment>`max_concurrency` should default to the intended baseline (100) so callers can create owner rows without explicitly providing that value each time; otherwise inserts that omit it will violate the NOT NULL constraint.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: All inserts into nuq.queue_scrape_owner_concurrency explicitly set max_concurrency to 100 in the application logic (apps/api/src/services/worker/nuq.ts). Since the column is always provided, the absence of a database default does not lead to NULL inserts or constraint violations.</comment>
<file context>
@@ -37,6 +37,13 @@ CREATE INDEX IF NOT EXISTS nuq_queue_scrape_queued_optimal_2_idx ON nuq.queue_sc
+CREATE TABLE IF NOT EXISTS nuq.queue_scrape_owner_concurrency (
+ id uuid NOT NULL,
+ current_concurrency int8 NOT NULL,
+ max_concurrency int8 NOT NULL,
+ CONSTRAINT queue_scrape_owner_concurrency_pkey PRIMARY KEY (id)
+);
</file context>
[internal] Confidence score: 6/10
[internal] Posted by: General AI Review Agent
| max_concurrency int8 NOT NULL, | |
| max_concurrency int8 NOT NULL DEFAULT 100, |
|
|
||
| CREATE TABLE IF NOT EXISTS nuq.queue_scrape_owner_concurrency ( | ||
| id uuid NOT NULL, | ||
| current_concurrency int8 NOT NULL, |
There was a problem hiding this comment.
current_concurrency needs a default of 0 so new owner rows can be created without explicitly supplying the starting count. Without it, inserts that rely on the counter starting at zero will hit a NOT NULL violation whenever the column is omitted.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• GPT-5: The application code that creates rows in nuq.queue_scrape_owner_concurrency always supplies a current_concurrency value explicitly. For example, apps/api/src/services/worker/nuq.ts inserts (id, current_concurrency, max_concurrency) with a computed job_count or 1, so no insert relies on a database default. Therefore, lack of a default will not cause NOT NULL errors.
Prompt for AI agents
Address the following comment on apps/nuq-postgres/nuq.sql at line 42:
<comment>`current_concurrency` needs a default of 0 so new owner rows can be created without explicitly supplying the starting count. Without it, inserts that rely on the counter starting at zero will hit a NOT NULL violation whenever the column is omitted.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: The application code that creates rows in nuq.queue_scrape_owner_concurrency always supplies a current_concurrency value explicitly. For example, apps/api/src/services/worker/nuq.ts inserts (id, current_concurrency, max_concurrency) with a computed job_count or 1, so no insert relies on a database default. Therefore, lack of a default will not cause NOT NULL errors.</comment>
<file context>
@@ -37,6 +37,13 @@ CREATE INDEX IF NOT EXISTS nuq_queue_scrape_queued_optimal_2_idx ON nuq.queue_sc
+CREATE TABLE IF NOT EXISTS nuq.queue_scrape_owner_concurrency (
+ id uuid NOT NULL,
+ current_concurrency int8 NOT NULL,
+ max_concurrency int8 NOT NULL,
+ CONSTRAINT queue_scrape_owner_concurrency_pkey PRIMARY KEY (id)
</file context>
[internal] Confidence score: 6/10
[internal] Posted by: General AI Review Agent
| current_concurrency int8 NOT NULL, | |
| current_concurrency int8 NOT NULL DEFAULT 0, |
|
|
||
| if (job) { | ||
| if (this.nuqWaitMode === "listen" && !process.env.NUQ_RABBITMQ_URL) { | ||
| await nuqPool.query(`SELECT pg_notify('${this.queueName}', $1);`, [ |
There was a problem hiding this comment.
SQL Injection: pg_notify channel built via string interpolation instead of parameters.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• GPT-5: The pg_notify call interpolates this.queueName, but that value is the same static string "nuq.queue_scrape" defined when NuQ is instantiated. Without any untrusted input reaching queueName, the channel name is fixed and cannot enable injection.
Prompt for AI agents
Address the following comment on apps/api/src/services/worker/nuq.ts at line 907:
<comment>SQL Injection: pg_notify channel built via string interpolation instead of parameters.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: The `pg_notify` call interpolates `this.queueName`, but that value is the same static string `"nuq.queue_scrape"` defined when `NuQ` is instantiated. Without any untrusted input reaching `queueName`, the channel name is fixed and cannot enable injection.</comment>
<file context>
@@ -794,26 +873,48 @@ class NuQ<JobData = any, JobReturnValue = any> {
+
+ if (job) {
+ if (this.nuqWaitMode === "listen" && !process.env.NUQ_RABBITMQ_URL) {
+ await nuqPool.query(`SELECT pg_notify('${this.queueName}', $1);`, [
+ job.id + "|completed",
+ ]);
</file context>
[internal] Confidence score: 9/10
[internal] Posted by: Security Vulnerabilities Agent
| ) | ||
| ).rows.map(row => this.rowToJob(row)!); | ||
| const queryGetNext = ` | ||
| SELECT ${this.jobReturning.map(x => `${this.queueName}.${x}`).join(", ")} FROM ${this.queueName} |
There was a problem hiding this comment.
SQL Injection risk: unescaped dynamic table/identifier (queueName) directly embedded in SQL.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• GPT-5: this.queueName is not user-controlled; the only instantiation is export const scrapeQueue = new NuQ<ScrapeJobData>("nuq.queue_scrape", ...), and the class itself is not exported. Because the queue name is a static, trusted identifier, interpolating it into the query cannot be exploited for SQL injection.
Prompt for AI agents
Address the following comment on apps/api/src/services/worker/nuq.ts at line 681:
<comment>SQL Injection risk: unescaped dynamic table/identifier (queueName) directly embedded in SQL.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: `this.queueName` is not user-controlled; the only instantiation is `export const scrapeQueue = new NuQ<ScrapeJobData>("nuq.queue_scrape", ...)`, and the class itself is not exported. Because the queue name is a static, trusted identifier, interpolating it into the query cannot be exploited for SQL injection.</comment>
<file context>
@@ -677,14 +677,59 @@ class NuQ<JobData = any, JobReturnValue = any> {
- )
- ).rows.map(row => this.rowToJob(row)!);
+ const queryGetNext = `
+ SELECT ${this.jobReturning.map(x => `${this.queueName}.${x}`).join(", ")} FROM ${this.queueName}
+ WHERE ${this.queueName}.status = 'queued'::nuq.job_status
+ ORDER BY ${this.queueName}.priority ASC, ${this.queueName}.created_at ASC
</file context>
[internal] Confidence score: 8/10
[internal] Posted by: Security Vulnerabilities Agent
There was a problem hiding this comment.
4 issues found across 2 files
Prompt for AI agents (all 4 issues)
Understand the root cause of the following 4 issues and fix them.
<file name="apps/api/src/services/worker/nuq.ts">
<violation number="1" location="apps/api/src/services/worker/nuq.ts:711">
Per-owner concurrency limit never enforced; jobs still run even when owner is already at max.</violation>
<violation number="2" location="apps/api/src/services/worker/nuq.ts:711">
Incrementing `current_concurrency` here leaves leaked counts when the lock reaper requeues stalled jobs, because that cron job (see apps/nuq-postgres/nuq.sql) never decrements the per-owner row. Please adjust the activation logic or update the reaper so stalled jobs release their concurrency slot.</violation>
<violation number="3" location="apps/api/src/services/worker/nuq.ts:712">
The logic to construct the owner concurrency table name (`${this.queueName}_owner_concurrency`) is flawed. When the queue name includes a schema (e.g., "nuq.queue_scrape"), the resulting interpolated string is an invalid SQL table identifier, which will cause runtime errors. This prevents the new per-owner concurrency feature from working correctly.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: `this.queueName` is "nuq.queue_scrape", so the interpolated identifier becomes `nuq.queue_scrape_owner_concurrency`, which PostgreSQL interprets as schema `nuq` and table `queue_scrape_owner_concurrency`. The migration in `apps/nuq-postgres/nuq.sql` creates `nuq.queue_scrape_owner_concurrency`, so the generated SQL references the correct table and is valid.</violation>
<violation number="4" location="apps/api/src/services/worker/nuq.ts:1082">
The new per-owner concurrency feature is incomplete because it does not handle stale job cleanup. The existing `cleanupStaleJobs` function deletes timed-out jobs without decrementing the owner's concurrency count. This will lead to a resource leak, eventually blocking affected owners from processing new jobs.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: The reported `cleanupStaleJobs` routine does not exist in `apps/api/src/services/worker/nuq.ts` (no references to "cleanup" or "stale" remain). Without that function, the alleged missing decrement logic cannot be assessed, so the claimed concurrency leak is unfounded.</violation>
</file>
React with 👍 or 👎 to teach cubic. Mention @cubic-dev-ai to give feedback, ask questions, or re-run the review.
| // === Instances | ||
|
|
||
| export const scrapeQueue = new NuQ<ScrapeJobData>("nuq.queue_scrape"); | ||
| export const scrapeQueue = new NuQ<ScrapeJobData>("nuq.queue_scrape", { |
There was a problem hiding this comment.
The new per-owner concurrency feature is incomplete because it does not handle stale job cleanup. The existing cleanupStaleJobs function deletes timed-out jobs without decrementing the owner's concurrency count. This will lead to a resource leak, eventually blocking affected owners from processing new jobs.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• GPT-5: The reported cleanupStaleJobs routine does not exist in apps/api/src/services/worker/nuq.ts (no references to "cleanup" or "stale" remain). Without that function, the alleged missing decrement logic cannot be assessed, so the claimed concurrency leak is unfounded.
Prompt for AI agents
Address the following comment on apps/api/src/services/worker/nuq.ts at line 1082:
<comment>The new per-owner concurrency feature is incomplete because it does not handle stale job cleanup. The existing `cleanupStaleJobs` function deletes timed-out jobs without decrementing the owner's concurrency count. This will lead to a resource leak, eventually blocking affected owners from processing new jobs.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: The reported `cleanupStaleJobs` routine does not exist in `apps/api/src/services/worker/nuq.ts` (no references to "cleanup" or "stale" remain). Without that function, the alleged missing decrement logic cannot be assessed, so the claimed concurrency leak is unfounded.</comment>
<file context>
@@ -956,7 +1079,9 @@ export async function nuqHealthCheck(): Promise<boolean> {
// === Instances
-export const scrapeQueue = new NuQ<ScrapeJobData>("nuq.queue_scrape");
+export const scrapeQueue = new NuQ<ScrapeJobData>("nuq.queue_scrape", {
+ concurrencyLimit: "per-owner",
+});
</file context>
[internal] Confidence score: 9/10
[internal] Posted by: System Design Agent
| GROUP BY owner_id | ||
| ), | ||
| owner_increment AS ( | ||
| INSERT INTO ${this.queueName}_owner_concurrency (id, current_concurrency, max_concurrency) |
There was a problem hiding this comment.
The logic to construct the owner concurrency table name (${this.queueName}_owner_concurrency) is flawed. When the queue name includes a schema (e.g., "nuq.queue_scrape"), the resulting interpolated string is an invalid SQL table identifier, which will cause runtime errors. This prevents the new per-owner concurrency feature from working correctly.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• GPT-5: this.queueName is "nuq.queue_scrape", so the interpolated identifier becomes nuq.queue_scrape_owner_concurrency, which PostgreSQL interprets as schema nuq and table queue_scrape_owner_concurrency. The migration in apps/nuq-postgres/nuq.sql creates nuq.queue_scrape_owner_concurrency, so the generated SQL references the correct table and is valid.
Prompt for AI agents
Address the following comment on apps/api/src/services/worker/nuq.ts at line 712:
<comment>The logic to construct the owner concurrency table name (`${this.queueName}_owner_concurrency`) is flawed. When the queue name includes a schema (e.g., "nuq.queue_scrape"), the resulting interpolated string is an invalid SQL table identifier, which will cause runtime errors. This prevents the new per-owner concurrency feature from working correctly.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: `this.queueName` is "nuq.queue_scrape", so the interpolated identifier becomes `nuq.queue_scrape_owner_concurrency`, which PostgreSQL interprets as schema `nuq` and table `queue_scrape_owner_concurrency`. The migration in `apps/nuq-postgres/nuq.sql` creates `nuq.queue_scrape_owner_concurrency`, so the generated SQL references the correct table and is valid.</comment>
<file context>
@@ -677,14 +677,59 @@ class NuQ<JobData = any, JobReturnValue = any> {
+ GROUP BY owner_id
+ ),
+ owner_increment AS (
+ INSERT INTO ${this.queueName}_owner_concurrency (id, current_concurrency, max_concurrency)
+ SELECT owner_id, job_count, ${/* TODO max concurrency */ 100}
+ FROM owner_counts
</file context>
[internal] Confidence score: 10/10
[internal] Posted by: System Design Agent
| WHERE owner_id IS NOT NULL | ||
| GROUP BY owner_id | ||
| ), | ||
| owner_increment AS ( |
There was a problem hiding this comment.
Per-owner concurrency limit never enforced; jobs still run even when owner is already at max.
Prompt for AI agents
Address the following comment on apps/api/src/services/worker/nuq.ts at line 711:
<comment>Per-owner concurrency limit never enforced; jobs still run even when owner is already at max.</comment>
<file context>
@@ -677,14 +677,59 @@ class NuQ<JobData = any, JobReturnValue = any> {
+ WHERE owner_id IS NOT NULL
+ GROUP BY owner_id
+ ),
+ owner_increment AS (
+ INSERT INTO ${this.queueName}_owner_concurrency (id, current_concurrency, max_concurrency)
+ SELECT owner_id, job_count, ${/* TODO max concurrency */ 100}
</file context>
[internal] Confidence score: 7/10
[internal] Posted by: Functional Bugs Agent
| WHERE owner_id IS NOT NULL | ||
| GROUP BY owner_id | ||
| ), | ||
| owner_increment AS ( |
There was a problem hiding this comment.
Incrementing current_concurrency here leaves leaked counts when the lock reaper requeues stalled jobs, because that cron job (see apps/nuq-postgres/nuq.sql) never decrements the per-owner row. Please adjust the activation logic or update the reaper so stalled jobs release their concurrency slot.
Prompt for AI agents
Address the following comment on apps/api/src/services/worker/nuq.ts at line 711:
<comment>Incrementing `current_concurrency` here leaves leaked counts when the lock reaper requeues stalled jobs, because that cron job (see apps/nuq-postgres/nuq.sql) never decrements the per-owner row. Please adjust the activation logic or update the reaper so stalled jobs release their concurrency slot.</comment>
<file context>
@@ -677,14 +677,59 @@ class NuQ<JobData = any, JobReturnValue = any> {
+ WHERE owner_id IS NOT NULL
+ GROUP BY owner_id
+ ),
+ owner_increment AS (
+ INSERT INTO ${this.queueName}_owner_concurrency (id, current_concurrency, max_concurrency)
+ SELECT owner_id, job_count, ${/* TODO max concurrency */ 100}
</file context>
[internal] Confidence score: 6/10
[internal] Posted by: General AI Review Agent
There was a problem hiding this comment.
1 issue found across 2 files
Prompt for AI agents (all 1 issues)
Understand the root cause of the following 1 issues and fix them.
<file name="apps/api/src/services/worker/nuq.ts">
<violation number="1" location="apps/api/src/services/worker/nuq.ts:696">
When `concurrencyLimit` is "per-owner", this branch still activates every queued job for an owner without first checking `${this.queueName}_owner_concurrency`, so owners can exceed their max concurrency. Please gate `queryGetNext` with the concurrency table (e.g. require `current_concurrency < max_concurrency`) before promoting jobs.</violation>
</file>
React with 👍 or 👎 to teach cubic. Mention @cubic-dev-ai to give feedback, ask questions, or re-run the review.
| ` | ||
|
|
||
| let updateQuery: string; | ||
| if (this.options.concurrencyLimit === "per-owner") { |
There was a problem hiding this comment.
When concurrencyLimit is "per-owner", this branch still activates every queued job for an owner without first checking ${this.queueName}_owner_concurrency, so owners can exceed their max concurrency. Please gate queryGetNext with the concurrency table (e.g. require current_concurrency < max_concurrency) before promoting jobs.
Prompt for AI agents
Address the following comment on apps/api/src/services/worker/nuq.ts at line 696:
<comment>When `concurrencyLimit` is "per-owner", this branch still activates every queued job for an owner without first checking `${this.queueName}_owner_concurrency`, so owners can exceed their max concurrency. Please gate `queryGetNext` with the concurrency table (e.g. require `current_concurrency < max_concurrency`) before promoting jobs.</comment>
<file context>
@@ -677,14 +677,59 @@ class NuQ<JobData = any, JobReturnValue = any> {
+ `
+
+ let updateQuery: string;
+ if (this.options.concurrencyLimit === "per-owner") {
+ // Single query: select, update jobs, and update owner concurrency
+ updateQuery = `
</file context>
[internal] Confidence score: 9/10
[internal] Posted by: General AI Review Agent
Summary by cubic
Adds per-owner concurrency tracking to NuQ and enables it for the scrape queue. Job activation, completion, and failure now atomically update per-owner concurrency counts to reduce race conditions.
New Features
Migration