MemberJunction / MemberJunction/MJ

AutotagPipelineResolver: fire-and-forget progress save can revert the completion write

Open
#4,340 0 comments 0 reactions 0 assignees View on GitHub
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.