Deep dive needed: MagicMapper's write path — two lookups, an audit label that can disagree with what was written

[rubenvdlinde]

Scoping a proper review of MagicMapper's single-object write path. #2215 fixes the concurrency symptom; this is about what the investigation exposed underneath.

What is already established

saveObjectToRegisterSchemaTable() (~line 3400) does its own lookup and its own insert-or-update decision — independently of the lookup SaveObject::saveObject() already performed. Two layers each decide "create or update" from separate reads.

Consequences observed while debugging #2212:

1. The audit action can disagree with the write. The service decides "create" from lookup (1) and records create in the audit trail; the mapper can then take the UPDATE branch from lookup (2). Measured: 3 audit create entries against a single _id that was inserted once. The audit trail said three objects were created. One was.
2. A re-fetch failure is treated as success. insertObjectEntity() re-reads the row it just wrote and, on DoesNotExistException, logs a warning and returns the in-memory entity as though persisted. A caller cannot distinguish that from a real write.
3. Two lookups per write. Both hit the database for the same question. On the fleet's hottest path that is worth measuring — see the OR CRUD budget work, where repeated resolution rather than the write itself dominated.

Questions the deep dive should answer

• Can the two insert-or-update decisions be collapsed into one, with the mapper as the single decider?
• Should the audit action be derived from what the mapper actually did rather than what the service predicted?
• Is the re-fetch fallback ever correct, or should it fail loudly?
• What does the second lookup cost per write at p50/p95?

Why not fold this into #2215

#2215 is a targeted fix with a measured before/after (12 racers: up to 8x201 before, 1x201 6/6 runs after) and 15,516 tests green. This is a structural review of the same code with a different risk profile, and mixing them would make both harder to review and to revert.

Related: #2212, #2215, hydra#425.

[rubenvdlinde]

Deep dive, first pass — two of four answered by fixing them (#2229), two answered here

✅ "Should the audit action be derived from what the mapper actually did?" — yes, and it now is

MagicMapper::getLastWriteAction() reports create or update, set at all three write branches, and SaveObject records that instead of its own prediction.

⚠️ Worth knowing before anyone else tries this: the obvious fix is a no-op. buildAuditTrail() inferred the action from $action === 'update', which is both the DEFAULT and a legitimate EXPLICIT value — so old: null, action: 'update' was rewritten back to create before it reached the row. I wrote that half first and it changed nothing. The default is now null = "infer"; a named action is used verbatim.

✅ "Is the re-fetch fallback ever correct, or should it fail loudly?" — both, and they were conflated

Two causes shared one outcome:

• the row is there and only the filtered read could not reach it (scoping, an org/owner context still being established). Degraded but honest — the write landed, so use the raw row's id.
• the row is not there. The write did not land, and returning the in-memory entity says it did, with a null id, to a caller that cannot tell.

The second now throws.

🔍 "Can the two insert-or-update decisions be collapsed, with the mapper as the single decider?" — yes, but not cheaply, and here is the specific obstacle

The mapper's decision is already the authoritative one: it runs last, so it matches the row. The service's lookup is not there only to decide insert-vs-update — it also produces $oldObject, which the update path needs for:

• the audit trail's changed diff (old vs new);
• updateObjectEntity(oldEntity: …), which takes it explicitly to avoid a third read;
• the inverse-relations pass, which compares old and new relation sets.

So "delete the service's lookup" is not the change. The change is invert the direction: let the mapper decide and hand the row it found back up, so the service consumes a fact instead of re-deriving one. getLastWriteAction() is the smallest possible version of that — it returns the label. The full version returns the row, and then the service's own lookup can go.

That is worth doing, and it is a different PR: it touches the update path, the audit diff and inverse relations at once, and each of those has its own tests. I would sequence it after the p50/p95 measurement below, because the measurement tells you whether it is a correctness cleanup or also a hot-path win — and the answer changes how aggressively it is worth refactoring.

❌ "What does the second lookup cost per write at p50/p95?" — not measured, deliberately not guessed

This needs instrumentation on a live instance under load, and the CRUD-budget work (#2201) already owns that harness — WritePhaseProbe marks are already threaded through this exact path (sv:pre-insert, sv:INSERT, sv:AUDIT). Adding a mark either side of findObjectInRegisterSchemaTable() in saveObjectToRegisterSchemaTable() would answer it with the tooling that exists.

I am leaving it open rather than reporting a number I did not take. Note the shape of the expected answer: on the object-API performance work the repeated resolution dominated rather than the write itself, so a second lookup per write is a plausible contributor and an implausible dominator — but that is a prior, not a measurement.