[jessepeterson]

Summary

A workflow step enqueued to many enrollments is completed by whichever of them reports last. Deciding which one that is happened inside each enrollment's completion transaction, against rows the others owned, so the whole fan-out serialized on the shared steps row and held it until commit. Enrollments that lost the race inside innodb_lock_wait_timeout were rolled back and their command result discarded — after the device had already reported it, and MDM does not redeliver.

This moves the step collection after the completion transaction commits: a non-locking reference check, then a FOR UPDATE SKIP LOCKED claim and point deletes. Completing a step now touches only the reporting enrollment's own id_commands rows.

One step fanned out to 300 enrollments all reporting at once, at READ-COMMITTED with a 2s lock timeout: the current code fails 104 of 300 with ERROR 1205, strands 104 id_commands rows, and takes 5.2s. This change completes all 300, strands nothing, in 0.26s. Measurements, reproduction and limitations are in a comment below.

Three further defects are fixed along the way, all latent. No schema change. Other engine storage backends are untouched.

Problem

GetIDCommandsByStepIDAndLock joined steps under FOR UPDATE. Because ic.step_id = s.id AND s.id = ? is just ic.step_id = ?, that join selected nothing and filtered nothing — its only effect was an exclusive lock on the one steps row the whole fan-out shares, taken early in the transaction and held to commit. That is the contention.

It was needed because of what came at the end of the same transaction: deciding whether the step could now be deleted meant anti-joining id_commands, a question about rows every other enrollment owns.

CancelSteps then finished by scanning for any step anywhere with no commands left, and any step_commands row anywhere whose command was gone — O(total steps) on every call, to cancel one enrollment's.

Three further defects. Each needs a state the normal lifecycle does not reach, so all three are latent rather than live, but the SQL is wrong on its own terms and the storage should not depend on guarantees it neither states nor enforces:

• The same query's LEFT JOIN step_commands read no column and could not narrow the match, but repeated each result once per raw command held for the step — a three-command step returned seven results.
• The step was then deleted while those step_commands rows still referenced it, which the foreign key rejects (Error 1451); schemas without the foreign key orphan them silently instead.
• Completion was decided by CountOutstandingIDWorkflowStepCommands, read outside the transaction that acted on it. MDM reports an enrollment's command results one at a time, so no sibling response can make that count stale — but that guarantee comes from the caller, and the worker does not inherit it: RetrieveTimedOutSteps deletes the same rows on a timer, in a transaction of its own.

Change

Collection moves after the commit. Once the completion transaction has committed, SelectUnreferencedStepIDs asks — no transaction, no locks — whether anything still refers to the step. If nothing does, a second short transaction claims it with FOR UPDATE SKIP LOCKED and deletes the step and its raw commands by key. Asked afterwards, with this enrollment's deletes already visible, the question needs no lock on another enrollment's rows and none on the step they share. This is the same split as nanomdm's command deleter (d7b3ac5), applied to the same shape of problem.

Two things make it safe. "Unreferenced" is terminal: id_commands rows are only ever inserted by StoreStep, which always creates a new steps row first, so no existing step can gain a reference. And the check must run in autocommit — inside a transaction, REPEATABLE READ would serve it from a snapshot predating the peers' deletes, so every enrollment would see the others' rows still present and none would ever collect.

Completion is decided inside the transaction. The locking read that gathers the sibling results now also counts what is outstanding, so the decision is made from rows the transaction holds. Only this enrollment's own rows are locked, so it costs nothing in contention.

Both joins are dropped; raw commands are deleted with the step that owns them, before it, satisfying the foreign key; and CancelSteps notes the affected step IDs before orphaning them and collects exactly those. No DELETE in query.sql joins any more — every one is now a plain single-table DELETE FROM, in place of MySQL’s proprietary DELETE <alias> FROM <joined tables> form.

A logger, so the collection is not silent. Its error must not be returned — engine.go:468 discards a non-nil step result if the storage errors, and engine.go:621 skips the ClearWorkflowStatus that follows a cancel — so in both cases returning it would abandon work that succeeded in order to report a row that was not reclaimed. Both are logged instead, via a new WithLogger option following the same pattern as engine.WithLogger and defaulting to log.NopLogger. Additive: no existing caller changes.

[jessepeterson]

Supporting detail for the description above: the query as it stood, how this was measured, the numbers, and what was not covered.

The query before

-- GetIDCommandsByStepIDAndLock
SELECT ic.command_uuid, ic.request_type, ic.result
FROM id_commands ic
  INNER JOIN steps s          ON ic.step_id = s.id
  LEFT  JOIN step_commands sc ON sc.step_id = s.id
WHERE ic.enrollment_id = ? AND s.id = ? AND ic.completed != 0
FOR UPDATE;

and the cleanup at the end of the same transaction:

-- DeleteWorkflowStepHavingNoCommandsByStepID
DELETE s FROM steps s
  LEFT JOIN id_commands c ON s.id = c.step_id
WHERE c.step_id IS NULL AND s.id = ?;

The anti-join is the question that made the shared lock necessary; the steps join in the read above is what took it.

On reachability of the two step_commands defects: those rows exist only for steps enqueued with a NotUntil, and the worker deletes them (RemoveStepCommandsByProcessID) in the transaction that sends the delayed commands, so none normally survives to completion. They are fixed here because the SQL is wrong regardless and the collection path has to delete these rows anyway.

How this was tested

MySQL 8.0.46 in a container, schema from engine/storage/mysql/schema.sql, driven entirely through the exported storage methods (StoreStep, StoreCommandResponseAndRetrieveCompletedStep, CancelSteps) rather than by inserting rows directly. Three server configurations:

	settings
CI defaults	REPEATABLE-READ, innodb_lock_wait_timeout=50
stricter	READ-COMMITTED, innodb_lock_wait_timeout=2 — surfaces contention the defaults hide
no foreign keys	as stricter, both FKs dropped

Reproduce with a MySQL server like the CI job's server:

gofmt -s, go vet, make test and go test -race ./engine/storage/mysql/... pass in all three configurations.

Three tests are added:

• TestStepFanOutCompletes — the contention case.
• TestNotUntilStepCompletes — completes a step whose raw commands are still present, which fails on the foreign key before this change. A storage invariant rather than a live regression; see the reachability note above.
• TestConcurrentCommandsOfOneEnrollment — overlapping responses for one enrollment. Also an invariant rather than a regression: MDM does not report an enrollment's commands concurrently, so this drives a state the protocol does not produce. It strands most of its steps before this change.

Results

Fan-out contention

One step enqueued to N enrollments, all reporting simultaneously, stricter server, connection pool capped at 128. The control gives each enrollment its own step — same work, nothing shared — and so bounds what the shared case could possibly achieve.

N	current, shared	current, errors	this change, shared	this change, errors	control (current / this change)
25	498 ms	0	62 ms	0	42 ms / 65 ms
100	2,330 ms	0	154 ms	0	66 ms / 116 ms
300	5,203 ms	104, 104 rows stranded	264 ms	0, none stranded	184 ms / 374 ms

The shared case now lands within the same range as the unshared control, where it used to be roughly 8–35x worse. Raising the pool to 256 raises the current code's failures to 165 of 300 and leaves this change at zero.

Cancel against completion

Twenty enrollments cancelled while fifty others complete a shared step, with forty unrelated steps present for the unscoped sweeps to scan. Three runs each:

	canceller errors
current	18, 19, 18 of 20 — all ERROR 1213 on delete workflow step having no commands ()
this change	0, 0, 0

Concurrent commands of one enrollment

Forty enrollments, each answering its four commands at once. Five runs, across both isolation levels:

	steps completed	steps stranded	errors
current	2–6 of 40	34–38	0
this change	40 of 40	0	0

Cost on the uncontended path

Collecting the step is a second transaction where completing it used to fold the cleanup in, so a workload with nothing to contend over pays for the extra commit. Fifty enrollments answering serially, each step going to one enrollment:

	current	this change
1 command each	49 ms	133 ms
4 commands each	135 ms	212 ms

Roughly 0.5–1.5 ms per completed step, across this and the control column above. This container's commits are fsync-bound, so treat these as pessimistic.

Limitations

• Only MySQL 8.0.46 was exercised.
• query_worker.sql is untouched and was not exercised under contention. Its three joined DELETE/UPDATE statements remain.
• The SKIP LOCKED decline path was not directly exercised. Ten consecutive fan-out runs at N=100 left zero uncollected steps, which means every run took the benign path — collectors racing each other — and says nothing about the case where a claim is refused. The crash-between-phases leak was likewise not exercised.
• The CancelSteps complexity change is argued structurally and measured only at small table sizes; no large-table benchmark was run.
• Timings come from a single container with no tuning and vary run to run by tens of percent. The orders of magnitude are the claim, not the digits.
• None of the three secondary defects was observed in the wild, and the tests construct each state directly rather than reproducing it.