fix: harden durable job submission - #20
Conversation
|
A remediação foi implementada e validada localmente e no CI disponível. Solicito nova revisão sobre segurança, isolamento de tenant, idempotência, contratos, migrações e interoperabilidade entre módulos.\n\n@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6772d4bb79
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| @@ -0,0 +1,28 @@ | |||
| datasource db { | |||
| provider = "postgresql" | |||
| url = env("DATABASE_URL") | |||
There was a problem hiding this comment.
With the new environment split, .env.example no longer defines DATABASE_URL and instead documents WORKBENCH_MIGRATION_DATABASE_URL for privileged migrations, but this datasource still reads only DATABASE_URL. In deployments following the new contract, pnpm prisma:migrate will fail before creating the workbench schema/roles, or it will accidentally run with a runtime URL if an old DATABASE_URL remains. Point the Prisma datasource/script at WORKBENCH_MIGRATION_DATABASE_URL or explicitly bridge it in the migrate command.
Useful? React with 👍 / 👎.
| correlation_id: correlationId, | ||
| request: input.request, | ||
| })); | ||
| const jobId = `job_${digest(`${input.tenantId}:create-job:${keyDigest}`).slice(0, 48)}`; |
There was a problem hiding this comment.
Because the queue job ID is derived only from tenant and idempotency key, a key reused after its receipt expires gets the same BullMQ jobId. The cleanup path deletes only the Postgres receipt, while JobStoreService.enqueue keeps completed and failed BullMQ jobs (removeOnComplete/removeOnFail are false), so BullMQ will treat the new submission as a duplicate and return the old job instead of enqueueing fresh work. Include a per-receipt component in the job ID or expire/remove the queue job on the same retention schedule.
Useful? React with 👍 / 👎.
| } | ||
|
|
||
| async submit(input: SubmissionInput, enqueue: (jobId: string) => Promise<WorkerJob>): Promise<SubmissionResult> { | ||
| const idempotencyKey = this.requiredHeader(input.idempotencyKey, 'Idempotency-Key', 16, 128); |
There was a problem hiding this comment.
The public OpenAPI parameter now used by /api/jobs still allows Idempotency-Key values with length 1–255 and no character pattern, but this code rejects anything shorter than 16, longer than 128, or outside [A-Za-z0-9._:-]. Clients generated from the published contract, and the legacy endpoints that share that parameter, can therefore send documented keys that job submission rejects with 400. Either tighten the OpenAPI parameter for this endpoint or accept the documented range here.
Useful? React with 👍 / 👎.
| const receipt = this.pool | ||
| ? await this.reservePostgres(input.tenantId, keyDigest, requestHash, jobId, correlationId, input.actorId) | ||
| : this.reserveMemory(input.tenantId, keyDigest, requestHash, jobId); |
There was a problem hiding this comment.
The new public contract advertises 503 for durable receipt storage failures, but only the queue enqueue is translated to ServiceUnavailableException; Postgres errors from reserving or completing the receipt escape this call path as generic errors and Nest will surface them as 500s. During a workbench DB outage, missing function, or permission regression, clients lose the intended retryable 503 signal for exactly the durable storage case this endpoint adds.
Useful? React with 👍 / 👎.
| ) { | ||
| return this.translate(async () => { | ||
| this.requiredHeaders(idempotencyKey, correlationId); | ||
| this.requireLegacyQueue(); |
There was a problem hiding this comment.
When WORKBENCH_QUEUES temporarily excludes legacy, this check now runs before the legacy manager can look up an existing receipt for the same idempotency key. That means a retry/replay of an already accepted export (and the same pattern on imports) returns 409 The legacy processing queue is not enabled even though no new enqueue is needed, breaking the idempotency contract during queue maintenance or partial rollouts. Move the queue-enabled check to only the branch that is about to enqueue a newly QUEUED receipt.
Useful? React with 👍 / 👎.
1 participant
Summary
Validation
pnpm guardian:checkpnpm openapi:checkpnpm buildpnpm test(27 tests)Notes
Queue execution, retries, schedules, status, and metrics remain covered by the existing worker contract.