fix: authorizationPolicyResetAll task never reaches COMPLETED; error path inverts it (#6310) - #6323
Draft
hero101 wants to merge 9 commits into
Draft
fix: authorizationPolicyResetAll task never reaches COMPLETED; error path inverts it (#6310)#6323hero101 wants to merge 9 commits into
hero101 wants to merge 9 commits into
Conversation
`updateTaskErrors` omitted the truthiness guard that `updateTaskResults` had, so for a task created without an `itemsCount` the check read `undefined === undefined` and flipped the task to COMPLETED on its very first error — while a fully successful run never completed at all. Both update paths now share one `isFullyAccountedFor()` guard: a task is only terminal when it is *counted* and its counter has reached the target. A run that lost items settles as ERRORED, not COMPLETED, on either path, and a terminal task gets its `end` timestamp like `complete()` sets one. Also replaces the obscure `itemsDone: itemsCount && 0` (which returned the falsy left operand) with an explicit undefined check, and completes a zero-item task at creation — no item update would ever arrive to move it out of IN_PROGRESS. Refs #6310 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`publishResetAll` created its task with no `itemsCount`, so the id handed back by `authorizationPolicyResetAll` could never reach a terminal status — useless as the completion signal our migration notes tell operators to poll. The sibling publishers already passed a count when invoked standalone; only the aggregate path was countless. The three entity id sets are now fetched ONCE up front and the total (accounts x2 + organizations x2 + users, for the authorization AND licence resets) is set on the task before the first message is emitted. Fetching once rather than counting separately is what makes the count trustworthy: the ids counted are exactly the ids emitted, so a row created or deleted between a COUNT and a SELECT cannot leave the task one item short forever. It is also cheaper — 3 id-only selects instead of 5. The platform and AI-server resets are deliberately excluded: they carry no task id, so the subscriber never accounts for them. Fixes #6310 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
📊 PR Metrics SummaryTitle: fix: authorizationPolicyResetAll task never reaches COMPLETED; error path inverts it (#6310)
Flags
Thresholds{
"critical_loc": 200,
"simple_loc": 100,
"file_count": 10
} |
|
Schema Diff Summary: No blocking changes
Baseline branch: develop |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes task progress tracking for long-running operations (notably authorizationPolicyResetAll) by ensuring tasks created for batch work are properly counted, and by aligning the “success” and “error” update paths so a task only becomes terminal when all counted items are accounted for.
Changes:
- Fix
TaskService.create()and task terminal-state logic so uncounted tasks never auto-complete, counted tasks complete/errored correctly, and zero-item counted tasks start asCOMPLETED. - Update
AuthResetService.publishResetAll()to prefetch entity IDs once, compute a trustworthyitemsCount, and reuse those IDs to emit exactly the tracked messages. - Add regression tests covering the original inversion/hanging behavior and ensuring counts match emitted work.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
src/services/task/task.service.ts |
Fixes counted vs uncounted task semantics, adds shared terminal guard, ensures terminal tasks get end timestamp, and handles overshoot/zero-item cases. |
src/services/task/task.service.spec.ts |
Adds regression coverage for terminal status behavior (issue #6310) and updates expectations for error-terminal outcomes. |
src/services/auth-reset/publisher/auth-reset.service.ts |
Prefetches IDs once, computes itemsCount, creates a counted task up front, and reuses IDs to keep emitted-message count consistent with tracking. |
src/services/auth-reset/publisher/auth-reset.service.spec.ts |
Adds regression coverage to verify itemsCount correctness, message tracking alignment, and single-fetch behavior. |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughTask progress now uses Redis-backed atomic counters, item deduplication, and derived terminal status. Aggregate auth-reset publishing prefetches entity IDs, computes tracked item counts, and passes stable item keys through event handling. ChangesTask accounting and terminal states
Aggregate reset publishing and event tracking
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
hero101
marked this pull request as ready for review
July 31, 2026 12:58
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/services/task/task.service.ts (1)
137-186: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftConcurrent writes to
itemsDonecan silently lose increments, causing tasks to hang.Both
updateTaskResultsandupdateTaskErrorsread the task from cache, incrementitemsDonein memory, and write the whole task back. This read-modify-write sequence is not atomic.The auth-reset worker runs as competing consumers across multiple pods, each pulling messages from the same RabbitMQ queue. Although
prefetchCount: 1limits each pod to one unacked message at a time, it does not prevent concurrent processing of the same task across different pods. When two pods process messages for the same task simultaneously, both can read the sameitemsDonevalue before either writes back. The last write wins, silently discarding the increment from the other pod.If
itemsDonelosses accumulate, the task never satisfiesisFullyAccountedFor(), and the task remains inIN_PROGRESSindefinitely. The>=guard on line 118 defends only against over-count; it does not prevent this under-count failure.Verify whether the task service must support concurrent updates for the same task across pods. If concurrent updates occur, implement atomic increment using Redis
INCRor application-level locking. If tasks are guaranteed single-pod processing (e.g., per-task affinity in the queue), document this assumption.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/services/task/task.service.ts` around lines 137 - 186, Ensure updateTaskResults and updateTaskErrors update itemsDone atomically when concurrent processing of the same task can occur across pods, using Redis INCR or the service’s locking mechanism before persisting the task. If per-task queue affinity guarantees single-pod processing instead, document that contract and preserve the existing counters and completion logic.
🧹 Nitpick comments (3)
src/services/task/task.service.ts (1)
90-93: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse
finish()for the zero-item completion path.The zero-item branch duplicates what
finish()does. If terminal-state handling changes later, this call site can drift fromfinish()since it does not go through it.♻️ Proposed refactor
- if (itemsCount === 0) { - task.status = TaskStatus.COMPLETED; - task.end = now; - } + if (itemsCount === 0) { + this.finish(task, TaskStatus.COMPLETED); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/services/task/task.service.ts` around lines 90 - 93, Update the zero-item branch in the task completion flow to call the existing finish() method instead of directly assigning task.status and task.end. Preserve the current behavior by passing the appropriate completion timestamp or arguments expected by finish(), and remove the duplicated terminal-state assignments from that branch.src/services/auth-reset/publisher/auth-reset.service.ts (2)
91-100: 🚀 Performance & Scalability | 🔵 TrivialConsider the memory footprint of full-table ID fetches at platform scale.
findAllIdsloads every id for an entity into memory at once. For very largeAccount,Organization, orUsertables, this could become a sizable in-memory array during a full platform reset. If dataset sizes are expected to grow substantially, consider streaming with.createQueryBuilder().stream()or paginated batches foremitResetEventsinstead of one large upfront array.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/services/auth-reset/publisher/auth-reset.service.ts` around lines 91 - 100, Update findAllIds and its emitResetEvents caller to avoid loading every entity ID into one in-memory array during a platform reset; use paginated batches or a query-streaming approach to process and emit reset events incrementally while preserving the existing ID coverage and reset behavior.
123-216: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the repeated publish pattern into one generic helper.
publishAuthorizationResetAllAccounts,publishLicenseResetAllAccounts,publishLicenseResetAllOrganizations,publishAuthorizationResetAllUsers, andpublishAuthorizationResetAllOrganizationsshare the same body, varying only by entity and event type. Consolidating reduces duplication and the risk of the branches drifting from each other.♻️ Proposed refactor
+ private async publishReset<T extends ObjectLiteral & { id: string }>( + entity: EntityTarget<T>, + type: RESET_EVENT_TYPE, + taskId?: string, + ids?: string[] + ) { + const resolvedIds = ids ?? (await this.findAllIds(entity)); + + const task = taskId + ? { id: taskId } + : await this.taskService.create(resolvedIds.length); + + this.emitResetEvents(resolvedIds, type, task.id); + + return task.id; + } + public async publishAuthorizationResetAllAccounts( taskId?: string, accountIds?: string[] ) { - const ids = accountIds ?? (await this.findAllIds(Account)); - - const task = taskId - ? { id: taskId } - : await this.taskService.create(ids.length); - - this.emitResetEvents( - ids, - RESET_EVENT_TYPE.AUTHORIZATION_RESET_ACCOUNT, - task.id - ); - - return task.id; + return this.publishReset( + Account, + RESET_EVENT_TYPE.AUTHORIZATION_RESET_ACCOUNT, + taskId, + accountIds + ); }Apply the same pattern to the remaining four methods.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/services/auth-reset/publisher/auth-reset.service.ts` around lines 123 - 216, Extract the shared task creation and reset-event publishing flow from publishAuthorizationResetAllAccounts, publishLicenseResetAllAccounts, publishLicenseResetAllOrganizations, publishAuthorizationResetAllUsers, and publishAuthorizationResetAllOrganizations into one generic helper parameterized by the entity, optional IDs, and RESET_EVENT_TYPE. Update each public method to delegate to that helper while preserving its current entity and event-type mapping and returned task ID.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/services/auth-reset/publisher/auth-reset.service.ts`:
- Around line 82-88: Update the catch block in the authorization reset
initialization flow to keep the message static and pass the caught error as the
fourth BaseException argument via an ExceptionDetails object using the
originalException field.
---
Outside diff comments:
In `@src/services/task/task.service.ts`:
- Around line 137-186: Ensure updateTaskResults and updateTaskErrors update
itemsDone atomically when concurrent processing of the same task can occur
across pods, using Redis INCR or the service’s locking mechanism before
persisting the task. If per-task queue affinity guarantees single-pod processing
instead, document that contract and preserve the existing counters and
completion logic.
---
Nitpick comments:
In `@src/services/auth-reset/publisher/auth-reset.service.ts`:
- Around line 91-100: Update findAllIds and its emitResetEvents caller to avoid
loading every entity ID into one in-memory array during a platform reset; use
paginated batches or a query-streaming approach to process and emit reset events
incrementally while preserving the existing ID coverage and reset behavior.
- Around line 123-216: Extract the shared task creation and reset-event
publishing flow from publishAuthorizationResetAllAccounts,
publishLicenseResetAllAccounts, publishLicenseResetAllOrganizations,
publishAuthorizationResetAllUsers, and publishAuthorizationResetAllOrganizations
into one generic helper parameterized by the entity, optional IDs, and
RESET_EVENT_TYPE. Update each public method to delegate to that helper while
preserving its current entity and event-type mapping and returned task ID.
In `@src/services/task/task.service.ts`:
- Around line 90-93: Update the zero-item branch in the task completion flow to
call the existing finish() method instead of directly assigning task.status and
task.end. Preserve the current behavior by passing the appropriate completion
timestamp or arguments expected by finish(), and remove the duplicated
terminal-state assignments from that branch.
🪄 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: Pro
Run ID: 5d489311-dd59-4197-8e48-32770da57789
📒 Files selected for processing (4)
src/services/auth-reset/publisher/auth-reset.service.spec.tssrc/services/auth-reset/publisher/auth-reset.service.tssrc/services/task/task.service.spec.tssrc/services/task/task.service.ts
… consumers The #6310 fix made terminal status depend on itemsDone reaching itemsCount, but the counter was a read-modify-write on a shared Redis entry. The auth-reset queue runs as competing consumers across 10 pods, so increments were lost and the task would never reach its target — reinstating the hang it fixed. - itemsDone/errorCount move to atomic Redis INCR keys - get() derives terminal status from the counters, so a stale whole-object write cannot clobber COMPLETED back to IN_PROGRESS - publishResetAll: task hoisted out of the try so a partial publish fails the task instead of orphaning it IN_PROGRESS - setItemsCount() lets a caller-supplied task id receive the computed total - both update paths ignore items arriving after the task has settled Tests fail with `expected 1 to be 10` when the atomic path is disabled. Refs #6310 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
Suppressed comments (4)
src/services/auth-reset/publisher/auth-reset.service.ts:117
- The thrown
BaseExceptionmessage currently interpolates the raw caughterror, which can leak sensitive runtime details into GraphQL responses and is often unhelpful when it stringifies. Keep the message stable and attach the cause viadetails.
throw new BaseException(
`Error while initializing authorization reset: ${error}`,
LogContext.AUTH,
AlkemioErrorStatus.AUTHORIZATION_RESET
);
src/services/task/task.service.ts:386
setItemsCount()always resetsitemsDoneto 0 and overwrites the cached task state. If it is accidentally called on an already-counted (or already-started) task, it will silently corrupt progress and terminal status. Consider guarding against calling this on tasks that already have counters set.
public async setItemsCount(id: string, itemsCount: number) {
const existing = await this.getOrFail(id);
// `itemsCount` is readonly on the interface, so rebuild rather than cast
// the guarantee away.
src/services/auth-reset/publisher/auth-reset.service.ts:103
completeWithError()stores the provided string on the task and is likely operator/UI-visible; interpolating the raw caughterrorcan leak internal details (and may stringify as "[object Object]"). Prefer a generic message here and rely on server logs / the thrown exception details for diagnostics.
This issue also appears on line 113 of the same file.
await this.taskService.completeWithError(
task.id,
`Reset publishing failed before all events were emitted: ${error}`
);
src/services/auth-reset/publisher/auth-reset.service.ts:109
logger.erroris being called withundefinedfor the stacktrace argument; this drops the most useful debugging context. Pass the stack when available.
this.logger.error(
`Failed to mark task '${task.id}' as errored: ${completionError}`,
undefined,
LogContext.AUTH
);
- BaseException carries the caught error as structured `details.originalException` rather than interpolating it into the message (repo convention; 9 other sites) - completeWithError stores a stable, operator-facing string; diagnostics go to the log line, not to the task the `task` query exposes - logger.error now receives the stack instead of undefined - create() and setItemsCount() route their zero-item completion through finish(), so every terminal transition stamps `end` identically - setItemsCount refuses to re-count an already-counted task, which would reset itemsDone under consumers that have already reported progress Refs #6310 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/services/task/task.service.ts (2)
381-393: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winReset the Redis counters when you assign the count.
itemsDone: 0is written to the cached object only.withAuthoritativeCountersreadstask:{id}:itemsDoneandtask:{id}:errorCount, so a pre-existing counter value survives this reset and overrides the zero.This is reachable today:
updateTaskErrorsincrementserrorsKeyeven for an uncounted task. If a caller reports an error on its own task before handing the id topublishResetAll, the staleerrorCountmakes the derived status ERRORED after a clean run.Clear both keys here. This needs a
deldeclaration added toRedisClientLikeinsrc/common/interfaces/redis.interfaces.ts.♻️ Sketch of the counter reset
In
src/common/interfaces/redis.interfaces.ts:expire( key: string, seconds: number, callback?: (err: Error | null, res: number) => void ): void; + del( + key: string | string[], + callback?: (err: Error | null, res: number) => void + ): void; }In
src/services/task/task.service.ts:public async setItemsCount(id: string, itemsCount: number) { const existing = await this.getOrFail(id); + // The cached `itemsDone: 0` below is not authoritative — the Redis keys + // are. Clear them so a stale tally cannot decide the terminal status. + await this.resetCounters(id); + // `itemsCount` is readonly on the interface, so rebuild rather than cast // the guarantee away. const task: Task = {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/services/task/task.service.ts` around lines 381 - 393, The setItemsCount method resets itemsDone to 0 in the cached object but does not clear the corresponding Redis counter keys, allowing stale values from task:{id}:itemsDone and task:{id}:errorCount to persist and override the reset. First, add a del method declaration to the RedisClientLike interface in src/common/interfaces/redis.interfaces.ts to support deletion of Redis keys. Then, in the setItemsCount method after the task object is created, use the Redis client to delete both the task:{id}:itemsDone and task:{id}:errorCount keys alongside the cache update to ensure counters are properly cleared when the count is reset.
105-123: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueOptional: batch the counter reads.
Each
getissues two Redis GET commands.getAllcallsgetfor every task id, so a list of N tasks costs 2N round trips. If the task list grows, consider a singleMGETof both keys per task, or oneMGETfor the whole list. This needs an added method onRedisClientLike.Also applies to: 157-161
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/services/task/task.service.ts` around lines 105 - 123, The readCounter method issues individual Redis GET commands for each counter key, and when getAll iterates through multiple task IDs calling readCounter for each one, this creates unnecessary round trips. Replace the per-task readCounter calls with a batched approach using Redis MGET to fetch multiple counter keys in a single operation. Add a new method to RedisClientLike that accepts multiple keys and returns their values in one batch operation, then update getAll to collect all needed counter keys first and fetch them together rather than calling readCounter repeatedly for each task ID.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/services/task/task.service.ts`:
- Around line 309-334: Make task accounting idempotent per item in
updateTaskResults and updateTaskErrors by adding an item identity parameter and
tracking it in a Redis seen-items set; increment doneKey and record the
result/error only when adding the item identity is new. Update every caller,
including auth-reset.controller.ts and search.ingest.service.ts, to pass the
identity, and preserve settlement until all unique items are accounted for.
---
Nitpick comments:
In `@src/services/task/task.service.ts`:
- Around line 381-393: The setItemsCount method resets itemsDone to 0 in the
cached object but does not clear the corresponding Redis counter keys, allowing
stale values from task:{id}:itemsDone and task:{id}:errorCount to persist and
override the reset. First, add a del method declaration to the RedisClientLike
interface in src/common/interfaces/redis.interfaces.ts to support deletion of
Redis keys. Then, in the setItemsCount method after the task object is created,
use the Redis client to delete both the task:{id}:itemsDone and
task:{id}:errorCount keys alongside the cache update to ensure counters are
properly cleared when the count is reset.
- Around line 105-123: The readCounter method issues individual Redis GET
commands for each counter key, and when getAll iterates through multiple task
IDs calling readCounter for each one, this creates unnecessary round trips.
Replace the per-task readCounter calls with a batched approach using Redis MGET
to fetch multiple counter keys in a single operation. Add a new method to
RedisClientLike that accepts multiple keys and returns their values in one batch
operation, then update getAll to collect all needed counter keys first and fetch
them together rather than calling readCounter repeatedly for each task ID.
🪄 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: Pro
Run ID: 4a1e0d9b-a1ce-4741-958a-662b2c62c9b5
📒 Files selected for processing (6)
src/common/interfaces/redis.interfaces.tssrc/services/auth-reset/publisher/auth-reset.service.spec.tssrc/services/auth-reset/publisher/auth-reset.service.tssrc/services/task/task.service.spec.tssrc/services/task/task.service.tstest/mocks/task.service.mock.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/services/auth-reset/publisher/auth-reset.service.ts
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Suppressed comments (2)
src/services/task/task.service.ts:168
getOrFailthrows an Error message containing the task id. Repo guidance is to keep exception messages stable (no dynamic IDs) and put dynamic data in structured details/logging instead.
This issue also appears on line 389 of the same file.
if (!task) {
throw new Error(`Task '${id}' not found`);
}
src/services/task/task.service.ts:392
setItemsCountthrows an Error message that includes the task id and existing count. Repo guidance is to keep exception messages free of dynamic data; prefer a stable message and put identifiers/counts into structured details or logs.
if (existing.itemsCount !== undefined) {
throw new Error(
`Task '${id}' already has an itemsCount (${existing.itemsCount}); refusing to re-count it`
);
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/services/task/task.service.spec.ts (1)
703-715: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAssert that rejected re-counting preserves progress.
The test verifies that
itemsCountremains5, but it does not verify the progress value that re-counting could reset. AfterupdateTaskResults, assert thatitemsDoneremains1aftersetItemsCountrejects.Proposed test improvement
- expect((await service.getOrFail(task.id)).itemsCount).toBe(5); + const persistedTask = await service.getOrFail(task.id); + expect(persistedTask.itemsCount).toBe(5); + expect(persistedTask.itemsDone).toBe(1);As per path instructions: “Risk-based testing approach: add tests when they deliver real signal.” This assertion adds signal for the stated progress-reset regression.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/services/task/task.service.spec.ts` around lines 703 - 715, Extend the test around setItemsCount in the “refuses to re-count a task that already has a count” case to also assert that itemsDone remains 1 after the rejected call. Keep the existing itemsCount assertion and rejection expectation unchanged.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.
Nitpick comments:
In `@src/services/task/task.service.spec.ts`:
- Around line 703-715: Extend the test around setItemsCount in the “refuses to
re-count a task that already has a count” case to also assert that itemsDone
remains 1 after the rejected call. Keep the existing itemsCount assertion and
rejection expectation unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 903fd51a-851f-40f4-847f-8c161ee4b66b
📒 Files selected for processing (3)
src/services/auth-reset/publisher/auth-reset.service.tssrc/services/task/task.service.spec.tssrc/services/task/task.service.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/services/auth-reset/publisher/auth-reset.service.ts
- src/services/task/task.service.ts
Two review findings, both real:
- Duplicate RMQ deliveries could settle a counted task early. handleReset
re-publishes its retry before acking the original, so redelivery is
guaranteed on pod failure; counting it twice reached itemsCount while real
items were still queued, and the terminal guard then dropped every genuine
update after it — a run that later failed an item still reported COMPLETED.
Items are now claimed with SADD task:{id}:seen, keyed on the payload's
type + entity id, and only a new claim advances the counter.
- The in-object fallback could regress the counter. If Redis was briefly
unreachable, a reconnected INCR starts below what the task already recorded;
a plain assignment undercounted and stranded the task IN_PROGRESS. Both the
write path and the read overlay are now monotonic.
Only the auth-reset controller supplies an item identity; the search-ingest
call sites run on uncounted tasks, which have no target to settle early
against.
Refs #6310
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
Suppressed comments (2)
src/services/task/task.service.ts:453
- Same risk as in updateTaskResults(): writing the whole task back can clear a previously-stamped
endif another consumer finished the task while this one was still processing. Since reads don’t reconstructend, it can remain missing. Preserve an existing cachedendvalue before writing (or moveendstamping into an atomic Redis key).
await this.cacheManager.set(task.id, task, {
ttl: TTL,
});
src/services/task/task.service.ts:413
- Because each consumer overwrites the entire cached Task, a slower consumer can clobber a terminal task’s
endtimestamp by writing back a stale copy whereendis stillundefined.withAuthoritativeCounters()intentionally does not deriveend, so callers may observe COMPLETED/ERRORED tasks with no end time. Consider preserving an existingendvalue from the cache before writing back updates (or storingendin Redis similarly to counters).
This issue also appears on line 451 of the same file.
await this.cacheManager.set(task.id, task, {
ttl: TTL,
});
Every consumer writes the whole Task object back, so a slower one holding a
copy taken before the task settled would write `end: undefined` over the
stamp — leaving a COMPLETED task with no end time, since the read overlay
deliberately does not invent one.
`end` is now recorded with SETNX on task:{id}:end (first stamp wins) and
restored by the read overlay, so the clobber is no longer observable.
Refs #6310
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
Suppressed comments (4)
src/services/task/task.service.ts:244
- Exception messages should avoid embedding dynamic data (task ids) per repo guidelines.
getOrFailcurrently throws an Error whose message includes the task id; prefer a stable message and attach the id via adetailsproperty instead.
This issue also appears on line 511 of the same file.
public async getOrFail(id: string) {
const task = await this.get(id);
if (!task) {
throw new Error(`Task '${id}' not found`);
}
src/services/task/task.service.ts:515
- Exception messages should avoid embedding dynamic data. This error includes both the task id and the existing count in the message; consider keeping the message stable and attaching
taskId/itemsCountvia adetailsproperty instead.
if (existing.itemsCount !== undefined) {
throw new Error(
`Task '${id}' already has an itemsCount (${existing.itemsCount}); refusing to re-count it`
);
}
src/services/task/task.service.ts:153
- The JSDoc block starting with "Claim an item for this task" is immediately followed by another JSDoc block, so the first one becomes an orphan comment (not attached to any symbol). This is confusing for readers and for generated docs; consider moving that block to sit directly above claimItem(), and keep only the stampEnd() doc above stampEnd().
/**
* Claim an item for this task, exactly once.
*
* RabbitMQ is at-least-once, and `handleReset` re-publishes its retry BEFORE
* acking the original — so a pod dying between those two lines guarantees a
* redelivery. Counting that twice would push `itemsDone` to `itemsCount`
* while real items are still queued: the task settles early, and every
* genuine update after it is dropped by the terminal guard, so a run that
* later fails an item can still report COMPLETED.
*
* `SADD` returns 1 only for a member that was not already in the set, which
* makes it the claim. Returns true when the caller owns this item and should
* account for it, false when it is a duplicate.
*
* Without Redis (or without an item identity) there is nothing to dedupe
* against and every call is treated as a fresh item — the previous
* behaviour.
*/
/**
* Record the task's terminal timestamp where it cannot be clobbered.
*
* `end` lives on the Task object, and every consumer writes that whole
* object back — so a slower consumer holding a copy taken before the task
* settled will write `end: undefined` straight over the stamp. The result is
* a COMPLETED task with no end time. SETNX keeps the FIRST stamp, and the
* read overlay restores it, so the clobber becomes invisible.
*/
src/services/auth-reset/publisher/auth-reset.service.ts:127
BaseExceptionincludesdetailsin GraphQL error extensions; passing the raw caughterrorasoriginalExceptionrisks leaking internal exception data to API consumers and may not serialize cleanly. Consider omitting the raw exception fromdetails(you already log stacktrace), or include only a stable, non-sensitive cause string.
throw new BaseException(
'Error while initializing authorization reset',
LogContext.AUTH,
AlkemioErrorStatus.AUTHORIZATION_RESET,
{ originalException: error }
);
stampEnd was inserted between claimItem's doc block and claimItem itself, leaving two stacked JSDoc comments above stampEnd and the claim semantics documented on the wrong method. Refs #6310 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
complete()/completeWithError() set the terminal status on the cached object, but a consumer that read the task just before them still holds an IN_PROGRESS copy and writes it straight back. For a task failed *because publishing threw* this is unrecoverable: the remaining events were never emitted, so the counter can never reach itemsCount and the counter-derived path has nothing to repair it with — the task hangs, which is #6310 again by another route. The terminal status is now stamped with SETNX on task:{id}:terminal (first stamp wins) and reapplied by the read overlay, alongside the existing end stamp. The overlay returns the task by identity when there is no Redis, so single-process callers keep mutating the instance they write back. Refs #6310 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
setItemsCount accepted any number. A fractional target can never be reached by an integer counter, and a negative one satisfies `itemsDone >= itemsCount` on the first read — so the task would report finished having processed nothing. Refs #6310 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/services/task/task.service.ts:452
accountForItem()assumesatomic === undefinedonly happens when there is no Redis client, butincrementCounter()also resolvesundefinedon Redis INCR/EXPIRE errors. That makes the surrounding comment inaccurate and can mislead future maintainers about when the non-atomic fallback path can occur (including in multi-pod/competing-consumer scenarios).
// `atomic` is undefined only when there is no Redis to be atomic against —
// a single-process environment, where the in-object increment is exact.
//
// Math.max, not a plain assignment: if Redis was briefly unreachable some
// increments landed in-object only, so a reconnected INCR starts from a
hero101
marked this pull request as draft
July 31, 2026 15:23
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #6310
Root cause
Two independent defects compounded into an unusable task handle:
publishResetAll()created a countless task. It calledtaskService.create()with noitemsCount, soitemsDonestayedundefinedtoo. The sibling publishers (publishAuthorizationResetAllAccountsetc.) already pass a count when invoked standalone — only the aggregate path, which creates one countless task and passes its id down to all of them, was broken.The two update paths disagreed on when a task is done.
updateTaskErrorsomitted the truthiness guard thatupdateTaskResultshad:Net effect: a reset that failed got marked
COMPLETED, one that succeeded stayedIN_PROGRESSforever. Any caller polling the returned task id hung indefinitely.Confirmed pre-existing, not a regression — and not limited to auth reset:
adminSearchIngestFromScratchalso creates a countless task and callsupdateTaskErrorsmid-run, so it was silently flipping toCOMPLETEDon its first error too. Both are fixed by the same guard.What changed
src/services/task/task.service.tsisFullyAccountedFor()guard — a task is terminal only when it is counted and its counter has reached the target. An uncounted task is never auto-terminated; it is finished explicitly viacomplete()/completeWithError(), which is exactly how the search-ingest caller uses it.ERRORED, notCOMPLETED, on either path (the success path checkserrors.length), and a terminal task now gets itsendtimestamp likecomplete()sets one.>=rather than===so an over-count cannot step over the equality and hang.itemsDone: itemsCount && 0→ an explicitundefinedcheck. Behaviourally identical (&&returned the falsy left operand, correct only by accident) but readable, and it keeps the documented "no count ⇒ untracked" semantics the existing specs already encode.IN_PROGRESS.src/services/auth-reset/publisher/auth-reset.service.tspublishResetAllfetches the three entity id sets once up front, computesaccounts × 2 + organizations × 2 + users(both the authorization and licence resets), and creates the task with that count before the first message is emitted.Design note: prefetch rather than
COUNT(*)The issue suggested computing the total up front. A separate
COUNT(*)would drift from the subsequentSELECT(a row created or deleted in between), leaving the task permanently one item short — i.e. hanging, resurrecting the exact bug under a race. Fetching the ids once and counting those makes the count structurally trustworthy. It is also strictly cheaper: 3 id-only selects instead of the 5 the publishers each ran on their own.The extra parameter is confined:
publishResetAllis the only publisher method with a non-test caller (admin.authorization.resolver.mutations.ts), and it is called with no arguments.Regression tests
src/services/task/task.service.spec.ts— newterminal status (issue #6310)block:COMPLETEDwhenitemsDonereachesitemsCountIN_PROGRESS, and the run endsERRORED— covering the case where the last update lands on the success pathERROREDCOMPLETEDsrc/services/auth-reset/publisher/auth-reset.service.spec.ts— newitemsCount (issue #6310)block:create(14))itemsCountequals exactly the number of emitted messages that carry the task idOne pre-existing assertion was intentionally inverted:
updateTaskErrors › should set status to COMPLETED when all items are done→should set status to ERRORED. That test encoded the bug.Verified non-vacuous: with the two source files reverted to
developand the new specs kept, 12 tests fail for the right reasons (expected 'completed' to be 'in-progress',expected "vi.fn()" to be called with arguments: [ 14 ],expected find to be called 3 times, but got 5).Gate results
pnpm test:ci:no:coveragepnpm buildnest build, no output)pnpm lint(tsc --noEmit+biome check src/)The full suite also ran green in the pre-commit hook on both commits.
Scope
No GraphQL schema change, no migration, no DDL —
Taskis a cache-backed object and both fields already existed on the DTO. Server-only; no cross-repo contract touched. Unrelated to the in-flightworkspace#027-platform-role-redesignbranch (which does not touchsrc/services/task/orsrc/services/auth-reset/), which is why it ships separately.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes