Skip to content

Commit 18b5a33

Browse files
committed
fix(plugin-approvals): the display backfill re-reads each request before writing it
A live writer (the boot SLA / dead-run sweep this plugin starts without awaiting, or another node) can write a pending request between the fill's page read and its row write. The request pass now re-reads the row by id, patches only columns still null there, writes the waiting-on only while the row is still pending on the slate the names were read for, and passes back the re-read updated_at — the one-engine-call window the comment-parent-pair backfill accepts. No conditional where. Claude-Session: https://claude.ai/code/session_01LZZjeuwVdnwn6dFDPLwTBu Co-authored-by: Claude <noreply@anthropic.com>
1 parent f8efc94 commit 18b5a33

3 files changed

Lines changed: 120 additions & 26 deletions

File tree

‎.changeset/23889-approval-list-display-labels.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,7 @@ The inbox door's `process_label` / `step_label` now read the new columns first.
2525

2626
**Read security.** Every new value is either a column of the request the row's readers can already read, or a name the approvals door already serves those readers for that request: `pending_approver_names`, and the timeline's names. The names come from the same resolver, under the same system read. The generic data door serves both objects only to the callers the approvals door would serve.
2727

28-
**Rows written before this release are filled at boot.** A new boot-time pass runs on `kernel:ready`, after the action-slot repair. It fills the new columns on rows that hold null, writing only into null columns, so it never overwrites a value a live writer stored. Every row it fills keeps its own `updated_at`: the write passes the stored value back under `preserveAudit`, so the request lists, which sort on `updated_at`, keep their order. The action-slot repair's writes keep it the same way. Where `plugin-audit` is composed, the first boot after the upgrade writes one system audit entry per row it fills. A second boot writes nothing.
28+
**Rows written before this release are filled at boot.** A new boot-time pass runs on `kernel:ready`, after the action-slot repair. It fills the new columns on rows that hold null. Each request is re-read just before it is written, and the write names only the columns that are still null on that re-read row. Whom a request waits on is written only while the request is still pending on the slate its names were read for. A value a live writer stored, such as the boot sweep or another node, is therefore kept. The only window left is the single engine call between the re-read and the write. Every row it fills keeps its own `updated_at`: the write passes the stored value back under `preserveAudit`, so the request lists, which sort on `updated_at`, keep their order. The action-slot repair's writes keep it the same way. Where `plugin-audit` is composed, the first boot after the upgrade writes one system entry in the audit log and one in the record's activity timeline for each row it fills. A second boot writes nothing.
2929

3030
- On every request, the flow and step labels come from the chain the approvals door serves: the authored labels the config snapshot carries, else the humanized machine name. The list and the inbox therefore show the same label for the same row, and no request is left blank.
3131
- On a pending request, whom it waits on is filled.

‎packages/plugins/plugin-approvals/src/display-label-backfill.integration.test.ts‎

Lines changed: 67 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,10 @@
2828
* (f) an empty authored label is stored as the humanized machine name, never
2929
* `''`, at open;
3030
* (g) every row a boot fills keeps its own `updated_at` exactly — the request
31-
* lists sort on it, and a derived display column is not an edit.
31+
* lists sort on it, and a derived display column is not an edit;
32+
* (h) a live write landing between a page's read and its row writes — a
33+
* recall, an SLA-style slate change — is kept exactly as it left the row,
34+
* and the rest of the page is still filled with its stamps kept.
3235
*/
3336

3437
import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest';
@@ -199,21 +202,27 @@ describe('#23889 — the boot-time display-label backfill', () => {
199202

200203
it('(c) never overwrites a column a live writer filled; an orphaned action keeps a null pointer and its own step name', async () => {
201204
await engine.update('sys_approval_request', { id: 'areq_open', process_label: 'Live label' }, { context: SYSTEM } as any);
205+
// A second pending request on the same page, so one labeller call per page
206+
// and one per row give different calls.
207+
await engine.insert('sys_approval_request', legacyRequest('open2', {
208+
status: 'pending', pending_approvers: 'u_mail', node_config_json: JSON.stringify({ approvers: [] }),
209+
}), { context: SYSTEM } as any);
202210
// An action whose step label and acting-as label a writer already stored,
203211
// different from what its request would give: the fill copies only its
204212
// null pointer columns and leaves both labels as stored.
205213
await engine.update('sys_approval_action', {
206214
id: 'a_vote', step_label: 'Recorded step', acted_as_label: 'Recorded name',
207215
}, { context: SYSTEM } as any);
208216
const svc = new ApprovalService({ engine: engine as any, logger: quiet as any });
209-
// The labeller is asked once per page, never once per row: here one request
210-
// page (the pending slate) and one action page (two acted_as slots).
217+
// The labeller is asked once per page, never once per row: one request page
218+
// (two pending slates) and one action page.
211219
const source = slotLabelSourceOf(svc);
212220
const asked: string[][] = [];
213221
const counting = { slotLabels: (slots: readonly string[]) => { asked.push([...slots]); return source.slotLabels(slots); } };
214222
const out = await backfillApprovalDisplayLabels(engine as any, counting);
215-
expect(asked).toEqual([['u_alice', 'position:legal'], ['position:legal']]);
216-
expect(out.requestsFilled).toBe(2);
223+
expect(asked).toEqual([['u_alice', 'position:legal', 'u_mail'], ['position:legal']]);
224+
expect(out.requestsFilled).toBe(3);
225+
expect((await row('sys_approval_request', 'areq_open2')).pending_approver_labels).toBe('Mia Mail');
217226

218227
expect((await row('sys_approval_request', 'areq_open')).process_label).toBe('Live label');
219228
expect(await row('sys_approval_action', 'a_vote')).toMatchObject({
@@ -320,4 +329,57 @@ describe('#23889 — the boot-time display-label backfill', () => {
320329
expect(await row('sys_approval_action', id), id).toMatchObject({ object_name: 'crm_deal', updated_at: BACKDATED });
321330
}
322331
});
332+
333+
it('(h) a live write between the page read and the row writes is kept as it left the row', async () => {
334+
// Two more pending requests on the page: one a sweep recalls, one it moves to another slate.
335+
await engine.insert('sys_approval_request', legacyRequest('recalled', {
336+
status: 'pending', pending_approvers: 'u_alice', node_config_json: JSON.stringify({ approvers: [] }),
337+
}), { context: SYSTEM } as any);
338+
await engine.insert('sys_approval_request', legacyRequest('moved', {
339+
status: 'pending', pending_approvers: 'u_alice', node_config_json: JSON.stringify({ approvers: [] }),
340+
}), { context: SYSTEM } as any);
341+
for (const id of ['areq_done', 'areq_open', 'areq_recalled', 'areq_moved']) await backdate('sys_approval_request', id);
342+
343+
const svc = new ApprovalService({ engine: engine as any, logger: quiet as any });
344+
const source = slotLabelSourceOf(svc);
345+
let left: Record<string, any> | undefined;
346+
// The labeller is asked once the request page has been read and before any
347+
// of its rows is written — the window. The live writes land there, as the
348+
// sweep's do: plain system writes that stamp now.
349+
const racing = {
350+
slotLabels: async (slots: readonly string[]) => {
351+
if (!left) {
352+
await engine.update('sys_approval_request', {
353+
id: 'areq_recalled', status: 'recalled', pending_approvers: null, pending_approver_labels: null,
354+
completed_at: '2026-10-01T00:00:00.000Z',
355+
}, { context: SYSTEM } as any);
356+
await engine.update('sys_approval_request', {
357+
id: 'areq_moved', pending_approvers: 'u_mail', pending_approver_labels: 'Mia Mail',
358+
}, { context: SYSTEM } as any);
359+
left = {
360+
recalled: await row('sys_approval_request', 'areq_recalled'),
361+
moved: await row('sys_approval_request', 'areq_moved'),
362+
};
363+
}
364+
return source.slotLabels(slots);
365+
},
366+
};
367+
await backfillApprovalDisplayLabels(engine as any, racing);
368+
369+
const pick = (r: any) => ({
370+
status: r.status, pending_approvers: r.pending_approvers,
371+
pending_approver_labels: r.pending_approver_labels, updated_at: r.updated_at,
372+
});
373+
expect(left!.recalled.updated_at).not.toBe(BACKDATED);
374+
expect(pick(await row('sys_approval_request', 'areq_recalled'))).toEqual(pick(left!.recalled));
375+
expect(pick(await row('sys_approval_request', 'areq_moved'))).toEqual(pick(left!.moved));
376+
// Their null label columns are still filled, under the stamp the live write left.
377+
expect((await row('sys_approval_request', 'areq_recalled')).process_label).toBe('Deal Review');
378+
379+
// The untouched rows on the page are filled, their stamps kept.
380+
expect(await row('sys_approval_request', 'areq_open')).toMatchObject({
381+
pending_approver_labels: 'Alice, position:legal', updated_at: BACKDATED,
382+
});
383+
expect(await row('sys_approval_request', 'areq_done')).toMatchObject({ process_label: 'Deal Review', updated_at: BACKDATED });
384+
});
323385
});

‎packages/plugins/plugin-approvals/src/display-label-backfill.ts‎

Lines changed: 52 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -36,17 +36,25 @@
3636
*
3737
* ## Each row keeps its `updated_at`
3838
*
39-
* Every write passes the read row's `updated_at` back under `preserveAudit`
40-
* ({@link PRESERVING_CONTEXT}), so a filled request keeps its place in the
41-
* lists that sort on it. Where `plugin-audit` is composed, each filled row still
42-
* gets one system audit entry for the write.
39+
* Every write passes the row's `updated_at` back under `preserveAudit`
40+
* ({@link PRESERVING_CONTEXT}) — on a request, the value the row holds when it
41+
* is re-read just before the write — so a filled request keeps its place in
42+
* the lists that sort on it. A row with no stored `updated_at` takes the write's
43+
* instant: no update can keep it null. Where `plugin-audit` is composed, the
44+
* write still records one system entry in the audit log and one in the record's
45+
* activity timeline for each row it fills.
4346
*
4447
* ## Idempotent, null-only, and run at every boot
4548
*
46-
* Each write names only columns that were null on the row it read, so the
47-
* pass can never overwrite what a live writer wrote, and a second run over an
48-
* unchanged database writes nothing (`display-label-backfill.integration.test.ts`
49-
* asserts both). A label the labeller could not produce — a failed `sys_user`
49+
* Each write names only columns that are null on the row as it stands just
50+
* before the write: a request is re-read by id first, and its waiting-on is
51+
* written only while it is still pending on the slate the names were read for.
52+
* So a value a live writer stored before that re-read is never overwritten;
53+
* what remains is the one engine call between the re-read and the write, the
54+
* window `plugin-audit`'s comment-parent-pair backfill accepts. No live writer
55+
* updates an action row, so the action pass writes from its page read. A
56+
* second run over an unchanged database writes nothing
57+
* (`display-label-backfill.integration.test.ts` asserts both). A label the labeller could not produce — a failed `sys_user`
5058
* read, which `slotLabels` reports as null rather than freezing a raw address
5159
* — stays null and is retried on the next boot. Both walks are seek walks over
5260
* the predicate "a display column is null", so a row this pass fills leaves the
@@ -99,6 +107,12 @@ const SYSTEM_CONTEXT = { isSystem: true, positions: [], permissions: [] };
99107
const PRESERVING_CONTEXT = { ...SYSTEM_CONTEXT, preserveAudit: true };
100108
const PAGE = 500;
101109

110+
/** What the request pass reads — on the page, and again on the row just before its write. */
111+
const REQUEST_FIELDS = [
112+
'id', 'status', 'process_name', 'current_step', 'node_config_json', 'pending_approvers',
113+
'process_label', 'current_step_label', 'pending_approver_labels', 'updated_at',
114+
];
115+
102116
/** The read row's own `updated_at`, to hand back on the write. */
103117
function keptStamp(row: Record<string, unknown>): Record<string, unknown> {
104118
return row.updated_at !== undefined && row.updated_at !== null ? { updated_at: row.updated_at } : {};
@@ -140,10 +154,7 @@ export async function backfillApprovalDisplayLabels(
140154
const requests = keysetWalk<Record<string, unknown>>(
141155
(q) => engine.find('sys_approval_request', {
142156
...q,
143-
fields: [
144-
'id', 'status', 'process_name', 'current_step', 'node_config_json', 'pending_approvers',
145-
'process_label', 'current_step_label', 'pending_approver_labels', 'updated_at',
146-
],
157+
fields: REQUEST_FIELDS,
147158
context: SYSTEM_CONTEXT,
148159
}) as Promise<Record<string, unknown>[]>,
149160
{
@@ -162,22 +173,43 @@ export async function backfillApprovalDisplayLabels(
162173
.filter((row) => row.status === 'pending' && !nonEmpty(row.pending_approver_labels))
163174
.flatMap((row) => slateOf(row.pending_approvers));
164175
const named = await pageLabels(labels, waitingOnSlots);
165-
for (const row of rows) {
166-
const id = nonEmpty(row.id);
176+
for (const paged of rows) {
177+
const id = nonEmpty(paged.id);
167178
if (!id) continue;
179+
// The page is a snapshot; live writers keep writing pending requests while
180+
// this pass runs — the boot SLA / dead-run sweep this plugin starts itself
181+
// (not awaited), and every other node. So the row is RE-READ by id
182+
// immediately before its write, and the patch is computed from that row:
183+
// only columns still null there, the stamp it holds now, and the
184+
// waiting-on only for the slate the names were read for. That narrows the
185+
// read-then-write race to the one engine call, as `plugin-audit`'s
186+
// comment-parent-pair backfill does. ⛔ No conditional `where` on the
187+
// update: that would put the write on the predicate path.
188+
const reread = ((await engine.find('sys_approval_request', {
189+
where: { id }, fields: REQUEST_FIELDS, limit: 1, context: SYSTEM_CONTEXT,
190+
})) as Record<string, unknown>[])[0];
191+
if (!reread) continue;
168192
const patch: Record<string, unknown> = {};
169-
const chain = requestDisplayLabels(row);
170-
if (!nonEmpty(row.process_label) && chain.process_label) patch.process_label = chain.process_label;
171-
if (!nonEmpty(row.current_step_label) && chain.current_step_label) {
193+
const chain = requestDisplayLabels(reread);
194+
if (!nonEmpty(reread.process_label) && chain.process_label) patch.process_label = chain.process_label;
195+
if (!nonEmpty(reread.current_step_label) && chain.current_step_label) {
172196
patch.current_step_label = chain.current_step_label;
173197
}
174-
if (row.status === 'pending' && !nonEmpty(row.pending_approver_labels)) {
175-
const slate = slateOf(row.pending_approvers);
198+
// Whom it waits on: only while it is still pending on the very slate the
199+
// page's names were read for. A slate that moved was moved by a live
200+
// writer, which wrote that slate's own labels (or cleared them).
201+
if (
202+
reread.status === 'pending'
203+
&& !nonEmpty(reread.pending_approver_labels)
204+
&& paged.status === 'pending'
205+
&& reread.pending_approvers === paged.pending_approvers
206+
) {
207+
const slate = slateOf(reread.pending_approvers);
176208
const waitingOn = slate.length ? slateLabelOf(slate.map((slot) => named.get(slot) ?? null)) : null;
177209
if (waitingOn) patch.pending_approver_labels = waitingOn;
178210
}
179211
if (!Object.keys(patch).length) continue;
180-
await engine.update('sys_approval_request', { id, ...patch, ...keptStamp(row) }, { context: PRESERVING_CONTEXT });
212+
await engine.update('sys_approval_request', { id, ...patch, ...keptStamp(reread) }, { context: PRESERVING_CONTEXT });
181213
requestsFilled++;
182214
}
183215
}

0 commit comments

Comments
 (0)