feat(flow): Add idempotent operation-run task submission#3372
Conversation
WalkthroughTask submissions now accept stable idempotency keys, persist them with uniqueness enforcement, reuse existing tasks on duplicate requests, and derive consistent keys for operation-run target retries. ChangesTask idempotency contracts and schema
Create-or-get persistence path
Task manager duplicate handling
Stable dispatcher keys
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Dispatcher
participant TaskManager
participant PostgresStore
participant TaskModel
Dispatcher->>TaskManager: Submit request with target-derived IdempotencyKey
TaskManager->>PostgresStore: Lock key and load existing task
PostgresStore->>TaskModel: GetTaskByIdempotencyKey
TaskModel-->>PostgresStore: Existing task or no row
PostgresStore-->>TaskManager: Existing task or create path
TaskManager-->>Dispatcher: Existing task ID or newly scheduled task
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
🔍 Container Scan Summary
Per-CVE detail lives in the per-service |
🔐 TruffleHog Secret Scan✅ No secrets or credentials found! Your code has been scanned for 700+ types of secrets and credentials. All clear! 🎉 🕐 Last updated: 2026-07-10 17:43:23 UTC | Commit: 207a97b |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rest-api/flow/internal/task/manager/manager.go (1)
310-363: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftIdempotent retries are blocked by the rack conflict check
HasConflictruns beforeCreateTaskOrGetByIdempotencyKey, so a retry whose original task is still active can be rejected withErrRackConflictbefore the existing row is reused. Handle the idempotency key first, or make conflict detection ignore the same key.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rest-api/flow/internal/task/manager/manager.go` around lines 310 - 363, Update the transaction callback in the task creation flow so idempotency lookup occurs before rack conflict validation, allowing retries to return the existing task even when it remains active. Refactor the logic around CreateTaskOrGetByIdempotencyKey to reuse an existing row immediately, and only run HasConflict and create a new task when no matching idempotency key exists.Source: Path instructions
🧹 Nitpick comments (2)
rest-api/flow/internal/task/manager/manager.go (1)
371-375: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winEmit a log line on the idempotent short-circuit for traceability.
The duplicate-detection early return silently returns the existing task ID. Given the flow module's emphasis on observability for stuck or repeated operations, a structured debug/info log recording the reused task ID and idempotency key would materially aid diagnosis of retried submissions.
♻️ Suggested log on the dedupe path
// A duplicate idempotent submission found a task whose workflow was // already scheduled; return that task instead of starting it again. if !inserted && task.ExecutionID != "" { + log.Info(). + Str("task_id", task.ID.String()). + Str("idempotency_key", task.IdempotencyKey). + Msg("idempotent duplicate: returning existing scheduled task") return task.ID, nil }As per path instructions: "observability for stuck or failed operations."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rest-api/flow/internal/task/manager/manager.go` around lines 371 - 375, Log a structured debug or info message before the early return in the duplicate idempotent submission branch of the task manager, including the reused task ID and idempotency key. Use the existing logger and conventions in the surrounding submission method, then return the existing task ID unchanged.Source: Path instructions
rest-api/flow/internal/operationrun/manager/dispatcher/dispatcher_test.go (1)
155-190: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCoverage note: the retry test exercises key propagation, not real dedupe.
Because
fakeTaskManagerrecords requests without running the actual conflict/create-or-get logic, this test proves the dispatcher emits a stable key but does not cover the manager-side interaction between conflict rejection and idempotent reuse called out inmanager.go. Consider a manager-level test where a retry with the same key and an active rack task returns the existing task ID rather thanErrRackConflict.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rest-api/flow/internal/operationrun/manager/dispatcher/dispatcher_test.go` around lines 155 - 190, Add a manager-level test covering retry behavior beyond dispatcher key propagation: configure an active rack task, submit a retry with the same idempotency key, and verify the manager returns the existing task ID instead of ErrRackConflict. Place the test alongside the manager logic exercised by manager.go, using the relevant task submission method and fake stores to validate conflict rejection followed by idempotent reuse.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@rest-api/flow/internal/task/manager/manager.go`:
- Around line 310-363: Update the transaction callback in the task creation flow
so idempotency lookup occurs before rack conflict validation, allowing retries
to return the existing task even when it remains active. Refactor the logic
around CreateTaskOrGetByIdempotencyKey to reuse an existing row immediately, and
only run HasConflict and create a new task when no matching idempotency key
exists.
---
Nitpick comments:
In `@rest-api/flow/internal/operationrun/manager/dispatcher/dispatcher_test.go`:
- Around line 155-190: Add a manager-level test covering retry behavior beyond
dispatcher key propagation: configure an active rack task, submit a retry with
the same idempotency key, and verify the manager returns the existing task ID
instead of ErrRackConflict. Place the test alongside the manager logic exercised
by manager.go, using the relevant task submission method and fake stores to
validate conflict rejection followed by idempotent reuse.
In `@rest-api/flow/internal/task/manager/manager.go`:
- Around line 371-375: Log a structured debug or info message before the early
return in the duplicate idempotent submission branch of the task manager,
including the reused task ID and idempotency key. Use the existing logger and
conventions in the surrounding submission method, then return the existing task
ID unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: e720ebf6-2a1b-4802-9ddc-7992c99c3000
📒 Files selected for processing (13)
rest-api/flow/internal/converter/dao/converter.gorest-api/flow/internal/db/migrations/20260709120000_add_task_idempotency_key.down.sqlrest-api/flow/internal/db/migrations/20260709120000_add_task_idempotency_key.up.sqlrest-api/flow/internal/db/model/task.gorest-api/flow/internal/operation/request.gorest-api/flow/internal/operationrun/manager/dispatcher/dispatcher_test.gorest-api/flow/internal/operationrun/manager/dispatcher/execution.gorest-api/flow/internal/scheduler/taskschedule/dispatcher_test.gorest-api/flow/internal/task/conflict/store_mock_test.gorest-api/flow/internal/task/manager/manager.gorest-api/flow/internal/task/store/postgres.gorest-api/flow/internal/task/store/store.gorest-api/flow/internal/task/task/task.go
jw-nvidia
force-pushed
the
feat/idempotent-task-submission
branch
from
207a97b to
0ce8ea3
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rest-api/flow/internal/task/manager/manager.go (1)
308-394: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftSerialize the idempotent resume path.
rest-api/flow/internal/task/manager/manager.go:315-405When a task already exists withExecutionID == "", concurrent retries can both read that row and both fall through toresolveAndExecuteTask, starting the same workflow twice. Acquire an advisory lock keyed by the idempotency key, or persist an intermediate scheduling state inside the transaction, before leaving the transaction.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rest-api/flow/internal/task/manager/manager.go` around lines 308 - 394, The idempotent duplicate path can concurrently resume an existing task with an empty ExecutionID, causing duplicate workflow starts. Update the transaction in the task creation flow and the subsequent resolveAndExecuteTask path to serialize this case, using an advisory lock keyed by the idempotency key or persisting an intermediate scheduling state before the transaction returns; ensure only one retry can proceed to schedule the workflow.Source: Path instructions
🧹 Nitpick comments (2)
rest-api/flow/internal/task/manager/manager.go (1)
371-374: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor: simplify the redundant assignment.
inserted = wasInsertedis alwaysfalsehere since the branch is gated on!wasInserted;inserted = falsewould be more direct.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rest-api/flow/internal/task/manager/manager.go` around lines 371 - 374, In the conditional branch of the task manager’s persistence logic, replace the redundant `inserted = wasInserted` assignment with `inserted = false`, since the branch is only entered when `wasInserted` is false.rest-api/flow/internal/task/manager/manager_test.go (1)
24-91: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the empty-
ExecutionIDresume path.This test asserts the "already scheduled" short-circuit (existing task with
ExecutionIDset), but the riskier branch — a persisted duplicate withExecutionID == ""that falls through toresolveAndExecuteTask— has no test here. Given that branch is the one without concurrency protection (see the correspondingmanager.gocomment), a focused test asserting its current single-threaded behavior would help pin down intended semantics before hardening it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rest-api/flow/internal/task/manager/manager_test.go` around lines 24 - 91, Add a focused test alongside TestCreateAndExecuteTaskReturnsExistingIdempotentTaskBeforeRackConflict covering an existing idempotent task with ExecutionID set to an empty string. Configure the store and manager so createAndExecuteTask falls through to resolveAndExecuteTask, then assert the current single-threaded behavior and relevant call counts/results, documenting the intended semantics of this unprotected resume path.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@rest-api/flow/internal/task/manager/manager.go`:
- Around line 308-394: The idempotent duplicate path can concurrently resume an
existing task with an empty ExecutionID, causing duplicate workflow starts.
Update the transaction in the task creation flow and the subsequent
resolveAndExecuteTask path to serialize this case, using an advisory lock keyed
by the idempotency key or persisting an intermediate scheduling state before the
transaction returns; ensure only one retry can proceed to schedule the workflow.
---
Nitpick comments:
In `@rest-api/flow/internal/task/manager/manager_test.go`:
- Around line 24-91: Add a focused test alongside
TestCreateAndExecuteTaskReturnsExistingIdempotentTaskBeforeRackConflict covering
an existing idempotent task with ExecutionID set to an empty string. Configure
the store and manager so createAndExecuteTask falls through to
resolveAndExecuteTask, then assert the current single-threaded behavior and
relevant call counts/results, documenting the intended semantics of this
unprotected resume path.
In `@rest-api/flow/internal/task/manager/manager.go`:
- Around line 371-374: In the conditional branch of the task manager’s
persistence logic, replace the redundant `inserted = wasInserted` assignment
with `inserted = false`, since the branch is only entered when `wasInserted` is
false.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 6f28fb5d-86de-4456-8605-252cdf3115d1
📒 Files selected for processing (14)
rest-api/flow/internal/converter/dao/converter.gorest-api/flow/internal/db/migrations/20260709120000_add_task_idempotency_key.down.sqlrest-api/flow/internal/db/migrations/20260709120000_add_task_idempotency_key.up.sqlrest-api/flow/internal/db/model/task.gorest-api/flow/internal/operation/request.gorest-api/flow/internal/operationrun/manager/dispatcher/dispatcher_test.gorest-api/flow/internal/operationrun/manager/dispatcher/execution.gorest-api/flow/internal/scheduler/taskschedule/dispatcher_test.gorest-api/flow/internal/task/conflict/store_mock_test.gorest-api/flow/internal/task/manager/manager.gorest-api/flow/internal/task/manager/manager_test.gorest-api/flow/internal/task/store/postgres.gorest-api/flow/internal/task/store/store.gorest-api/flow/internal/task/task/task.go
🚧 Files skipped from review as they are similar to previous changes (8)
- rest-api/flow/internal/converter/dao/converter.go
- rest-api/flow/internal/task/store/store.go
- rest-api/flow/internal/task/task/task.go
- rest-api/flow/internal/operation/request.go
- rest-api/flow/internal/scheduler/taskschedule/dispatcher_test.go
- rest-api/flow/internal/db/model/task.go
- rest-api/flow/internal/operationrun/manager/dispatcher/execution.go
- rest-api/flow/internal/operationrun/manager/dispatcher/dispatcher_test.go
spydaNVIDIA
left a comment
spydaNVIDIA
left a comment
There was a problem hiding this comment.
lgtm -- maybe worth adding a bit more details in the commit message as to why we want to allow idempotent task submission
jw-nvidia
force-pushed
the
feat/idempotent-task-submission
branch
from
0ce8ea3 to
9c90469
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@rest-api/flow/internal/db/migrations/20260709120000_add_task_idempotency_key.up.sql`:
- Around line 4-8: Update the migration adding task.idempotency_key and
task_idempotency_key_unique so IF NOT EXISTS cannot silently accept incompatible
existing objects. Remove the clauses and let conflicts fail, or explicitly
validate the existing column type and index uniqueness, columns, and partial
predicate through the catalog before proceeding; ensure unexpected schema drift
aborts the migration.
In `@rest-api/flow/internal/task/manager/manager_test.go`:
- Around line 124-129: The execution-path test must initialize the nil rule
resolver dependency. In the ManagerImpl setup, add a ruleResolver implementation
that returns the expected rule for the reused unscheduled task, so
resolveAndExecuteTask can proceed to the executor and assertions.
In `@rest-api/flow/internal/task/manager/manager.go`:
- Around line 379-380: The task is being executed inside the creation
transaction, allowing external work and failures to roll back task persistence
while holding the transaction open. In the task creation flow around
resolveAndExecuteTask, commit the create-or-get/admission transaction before
invoking the executor, and add a durable scheduling claim or state transition so
concurrent duplicates cannot schedule the same persisted task; execute only
after the transaction has closed.
In `@rest-api/flow/internal/task/store/postgres.go`:
- Around line 76-90: Namespace advisory-lock inputs by domain before hashing to
prevent rack and idempotency locks from colliding. Update LockRack and
LockIdempotencyKey to pass distinct prefixed keys to lockAdvisoryKey, preserving
existing validation and ensuring swapped rack/idempotency values cannot share
the same PostgreSQL advisory lock.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: bd7c856b-6b91-4619-a193-0c990464e551
📒 Files selected for processing (14)
rest-api/flow/internal/converter/dao/converter.gorest-api/flow/internal/db/migrations/20260709120000_add_task_idempotency_key.down.sqlrest-api/flow/internal/db/migrations/20260709120000_add_task_idempotency_key.up.sqlrest-api/flow/internal/db/model/task.gorest-api/flow/internal/operation/request.gorest-api/flow/internal/operationrun/manager/dispatcher/dispatcher_test.gorest-api/flow/internal/operationrun/manager/dispatcher/execution.gorest-api/flow/internal/scheduler/taskschedule/dispatcher_test.gorest-api/flow/internal/task/conflict/store_mock_test.gorest-api/flow/internal/task/manager/manager.gorest-api/flow/internal/task/manager/manager_test.gorest-api/flow/internal/task/store/postgres.gorest-api/flow/internal/task/store/store.gorest-api/flow/internal/task/task/task.go
✅ Files skipped from review due to trivial changes (1)
- rest-api/flow/internal/operationrun/manager/dispatcher/dispatcher_test.go
🚧 Files skipped from review as they are similar to previous changes (5)
- rest-api/flow/internal/converter/dao/converter.go
- rest-api/flow/internal/operation/request.go
- rest-api/flow/internal/task/task/task.go
- rest-api/flow/internal/db/model/task.go
- rest-api/flow/internal/operationrun/manager/dispatcher/execution.go
| ALTER TABLE task ADD COLUMN IF NOT EXISTS idempotency_key text; | ||
|
|
||
| CREATE UNIQUE INDEX IF NOT EXISTS task_idempotency_key_unique | ||
| ON task (idempotency_key) | ||
| WHERE idempotency_key IS NOT NULL; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Do not let IF NOT EXISTS mask a broken uniqueness contract.
These clauses check only object names, not definitions. A pre-existing column with an incompatible type or an index with the same name but different/non-unique semantics would make the migration succeed without enforcing idempotency. Let unexpected schema drift fail, or explicitly validate existing catalog definitions before continuing.
🧰 Tools
🪛 Squawk (2.59.0)
[warning] 6-8: During normal index creation, table updates are blocked, but reads are still allowed. Use concurrently to avoid blocking writes.
(require-concurrent-index-creation)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@rest-api/flow/internal/db/migrations/20260709120000_add_task_idempotency_key.up.sql`
around lines 4 - 8, Update the migration adding task.idempotency_key and
task_idempotency_key_unique so IF NOT EXISTS cannot silently accept incompatible
existing objects. Remove the clauses and let conflicts fail, or explicitly
validate the existing column type and index uniqueness, columns, and partial
predicate through the catalog before proceeding; ensure unexpected schema drift
aborts the migration.
| manager := &ManagerImpl{ | ||
| taskStore: store, | ||
| executor: executor, | ||
| maxWaitingPerRack: defaultMaxWaitingPerRack, | ||
| defaultQueueTimeout: defaultQueueTimeout, | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Initialize ruleResolver for the execution-path test.
This test reuses an unscheduled task, so it reaches resolveAndExecuteTask, which dereferences m.ruleResolver before calling the executor. The nil dependency panics before the assertions run. Provide a resolver returning the expected rule.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rest-api/flow/internal/task/manager/manager_test.go` around lines 124 - 129,
The execution-path test must initialize the nil rule resolver dependency. In the
ManagerImpl setup, add a ruleResolver implementation that returns the expected
rule for the reused unscheduled task, so resolveAndExecuteTask can proceed to
the executor and assertions.
| // Resolve and execute the task. | ||
| return m.resolveAndExecuteTask(txCtx, &task, targetRack) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Do not execute the task inside the creation transaction.
resolveAndExecuteTask calls the external executor and returns its error after attempting UpdateTaskStatus. That error aborts this transaction, rolling back both the new task row and its failed status. A retry then creates another task, defeating idempotency; the transaction also remains open for the external call.
Commit the create-or-get/admission result first, then execute outside the transaction. Use a durable scheduling claim/state transition so a concurrent duplicate cannot schedule the same persisted task. As per coding guidelines, “do not hold a transaction open while doing long-running work.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rest-api/flow/internal/task/manager/manager.go` around lines 379 - 380, The
task is being executed inside the creation transaction, allowing external work
and failures to roll back task persistence while holding the transaction open.
In the task creation flow around resolveAndExecuteTask, commit the
create-or-get/admission transaction before invoking the executor, and add a
durable scheduling claim or state transition so concurrent duplicates cannot
schedule the same persisted task; execute only after the transaction has closed.
Source: Coding guidelines
| func (s *PostgresStore) LockRack(ctx context.Context, rackID uuid.UUID) error { | ||
| if rackID == uuid.Nil { | ||
| return errors.GRPCErrorInvalidArgument("rack ID is required") | ||
| } | ||
| return s.lockAdvisoryKey(ctx, rackID.String()) | ||
| } | ||
|
|
||
| // LockIdempotencyKey takes a transaction-scoped advisory lock for stable | ||
| // submission lookup and resume. The lock is released automatically when the | ||
| // transaction ends. | ||
| func (s *PostgresStore) LockIdempotencyKey(ctx context.Context, key string) error { | ||
| if key == "" { | ||
| return errors.GRPCErrorInvalidArgument("idempotency key is required") | ||
| } | ||
| return s.lockAdvisoryKey(ctx, key) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Namespace the advisory-lock keys.
A non-empty idempotency key equal to a rack UUID string locks the same PostgreSQL advisory key as that rack. Two submissions with swapped rack-ID keys can therefore deadlock; prefix the domains before hashing.
Proposed fix
- return s.lockAdvisoryKey(ctx, rackID.String())
+ return s.lockAdvisoryKey(ctx, "rack:"+rackID.String())
...
- return s.lockAdvisoryKey(ctx, key)
+ return s.lockAdvisoryKey(ctx, "idempotency:"+key)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (s *PostgresStore) LockRack(ctx context.Context, rackID uuid.UUID) error { | |
| if rackID == uuid.Nil { | |
| return errors.GRPCErrorInvalidArgument("rack ID is required") | |
| } | |
| return s.lockAdvisoryKey(ctx, rackID.String()) | |
| } | |
| // LockIdempotencyKey takes a transaction-scoped advisory lock for stable | |
| // submission lookup and resume. The lock is released automatically when the | |
| // transaction ends. | |
| func (s *PostgresStore) LockIdempotencyKey(ctx context.Context, key string) error { | |
| if key == "" { | |
| return errors.GRPCErrorInvalidArgument("idempotency key is required") | |
| } | |
| return s.lockAdvisoryKey(ctx, key) | |
| func (s *PostgresStore) LockRack(ctx context.Context, rackID uuid.UUID) error { | |
| if rackID == uuid.Nil { | |
| return errors.GRPCErrorInvalidArgument("rack ID is required") | |
| } | |
| return s.lockAdvisoryKey(ctx, "rack:"+rackID.String()) | |
| } | |
| // LockIdempotencyKey takes a transaction-scoped advisory lock for stable | |
| // submission lookup and resume. The lock is released automatically when the | |
| // transaction ends. | |
| func (s *PostgresStore) LockIdempotencyKey(ctx context.Context, key string) error { | |
| if key == "" { | |
| return errors.GRPCErrorInvalidArgument("idempotency key is required") | |
| } | |
| return s.lockAdvisoryKey(ctx, "idempotency:"+key) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rest-api/flow/internal/task/store/postgres.go` around lines 76 - 90,
Namespace advisory-lock inputs by domain before hashing to prevent rack and
idempotency locks from colliding. Update LockRack and LockIdempotencyKey to pass
distinct prefixed keys to lockAdvisoryKey, preserving existing validation and
ensuring swapped rack/idempotency values cannot share the same PostgreSQL
advisory lock.
9c90469 to
da5b334
Compare
- Operation-run dispatch can retry target submission after the service restarts or after a transient failure. Without a stable task idempotency key, those retries may create duplicate tasks for the same operation-run target or hit rack-conflict rejection instead of returning the task that was already persisted. - Persist an optional task idempotency key, have operation-run targets submit with a stable key, and return or resume the existing task when a retry uses the same key. - Serialize idempotent lookup/resume with an idempotency-key advisory lock, and serialize rack-level conflict admission with a rack advisory lock so concurrent submissions cannot both pass conflict checks.
jw-nvidia
force-pushed
the
feat/idempotent-task-submission
branch
from
da5b334 to
9e40d68
Compare
|
🌿 Preview your docs: https://nvidia-preview-pull-request-3372.docs.buildwithfern.com/infra-controller |
2 participants
Operation-run dispatch can retry target submission after the service
restarts or after a transient failure. Without a stable task idempotency
key, those retries may create duplicate tasks for the same operation-run
target or hit rack-conflict rejection instead of returning the task that
was already persisted.
Persist an optional task idempotency key, have operation-run targets submit
with a stable key, and return or resume the existing task when a retry uses
the same key.
Serialize idempotent lookup/resume with an idempotency-key advisory lock,
and serialize rack-level conflict admission with a rack advisory lock so
concurrent submissions cannot both pass conflict checks.
Related issues
Type of Change
Breaking Changes
Testing
Additional Notes