MemberJunction / MemberJunction/MJ
AutotagPipelineResolver: fire-and-forget progress save can revert the completion write
- Dominant language
- TSQL
- Stars
- 29
- Forks
- 6
- Avg merge
- 1d 8h
- Merged PRs (30d)
- 308
Description
## Summary
`AutotagPipelineResolver` has the same defect #4251 fixes for credentials: a fire-and-forget full-row `Save()` racing an awaited write to the same row. Found while checking whether #4251's bug was a one-off.
## The race
`packages/MJServer/src/resolvers/AutotagPipelineResolver.ts`
```js
// :107 — called repeatedly through the pipeline
this.updateProcessRunProgress(processRun, processed, total);
// :216-228 — fire-and-forget
private updateProcessRunProgress(processRun, processedItems, totalItems): void {
processRun.ProcessedItems = processedItems;
processRun.TotalItemCount = totalItems;
processRun.LastProcessedOffset = processedItems;
processRun.Save().catch(e => { ... }); // not awaited
}
// :242-248 — the completion path, on the SAME entity instance
processRun.Status = status;
processRun.EndTime = new Date();
if (errorMessage) { processRun.ErrorMessage = errorMessage; processRun.ErrorCount = ... }
const saved = await processRun.Save();
```
`BaseEntity.Save()` writes **every** `IsSPParameter` column, not just dirty ones. So a progress save still in flight when the pipeline completes can commit after the completion save and write its own snapshot over it.
Observable result: **a finished run left showing its old `Status` with a null `EndTime`** — the pipeline reports success while the row says it never ended.
Two aggravating details:
1. **Same entity instance.** Both paths mutate one `processRun` object, so the outcome depends on whether the in-flight save has already generated its SQL. Nondeterministic either way.
2. **Concurrent saves on one instance.** `updateProcessRunProgress` is called in a loop without awaiting, so several saves can be in flight on the same `BaseEntity` simultaneously — independent of the completion race, and not something `BaseEntity` is designed for (`IsSaving` is per-instance state).
## Why this one and not the others
I swept the repo for the pattern. The only other un-awaited `.Save()` is `artifact-viewer-panel.component.ts:1463`, and that one is an **INSERT** — a fresh `GetEntityObject` with fields set, so there is no pre-existing row to clobber. Different risk profile (unbounded concurrent inserts), not this defect. This resolver is the only other instance of the actual lost-update shape I could find.
## Fix
The same shape as #4251 works: serialize writes to a given run through a promise chain so each save loads and commits in order. Cheaper alternative for this specific case: `await` the progress save, or drop progress persistence to a throttled interval — the comment says the intent was "don't slow down the pipeline", and a throttle achieves that without the race.
Whichever, the completion write must not be overtakeable by a progress write.
Related: #4251 (the same defect in credentials, with a live reproduction), #4339 (the cross-process half neither fix addresses).
Contributor guide
Research direction
Start in packages/MJServer/src/resolvers/AutotagPipelineResolver.ts, especially updateProcessRunProgress around lines 216–228 and the completion path around lines 242–248; then read BaseEntity.Save() to confirm its full-row write behavior. Use #4251 as the related precedent and verify that progress writes cannot overtake the completion write, while preserving the pipeline’s progress-saving intent.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- typescript
- Domain
- backend
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Active
- Clarity
- Clearly specified
- Newbie friendliness
- 55/100