fix(rest)!: an async import job whose every row failed ends failed, not succeeded

.changeset/27676-import-job-every-row-failed.md

@@ -0,0 +1,19 @@
+---
+'@objectstack/rest': patch
+---
+
+fix(rest)!: an async import job whose every row failed ends `failed`, not `succeeded` (#27676)
+
+Clause-②: no (narrowing) — the async import job's `succeeded` status is no longer emitted for a job that imported nothing; it now reads `failed`. No value, key or shape is added.
+Metadata-protocol: none
+
+<!-- adr-0087: not-required (no-migration-prescription) No key of any schema is added, removed, renamed or re-shaped, and no export or type changes: one already-declared status value (`failed`) is now emitted for a job that created, updated and skipped no row. There is nothing for a migration to rewrite and no tombstone. -->
+
+**BREAKING** a published status value changes meaning at one REST door (`@objectstack/rest`), per maintainer ruling B (record 6509036166).
+
+- **Before.** A job of `POST /data/:object/import/jobs` that was not cancelled and did not throw ended `succeeded`, even when every row failed and nothing was written. The only sign was `errors` equal to `total`.
+- **Now.** A job that was not cancelled, had at least one row, and created, updated or skipped none of them ends `failed`. Its `error` says every row failed and points at the job's `results`, which still carry each row's reason. A dry run follows the same rule: a dry run whose every row is invalid ends `failed`.
+- **Unchanged, on purpose.** A job whose rows partly failed still ends `succeeded`. So does a job whose every row was skipped (for example `writeMode: 'update'` with no match), and a job with no rows. A cancel still wins: a cancelled job ends `cancelled` whatever its rows did. Counts and `results` are written as before.
+- **Who sees it.** A client that polls an import job and branches on `status === 'succeeded'` is now told `failed` for a job that imported nothing. Undo is unaffected: such a job wrote nothing, so it was never undoable.
+
+**The remedy.** Read `failed` as "this job imported nothing"; read `errors` and `results` for why. A client that wants to treat such a job as finished can treat `succeeded` and `failed` alike as terminal, as before.

packages/qa/dogfood/test/import-reference-exposure.dogfood.test.ts

@@ -83,7 +83,12 @@ describe('[#22739] an import matches a lookup cell only against a target whose d
       const { jobId } = (await res.json()) as { jobId: string };
       for (let i = 0; i < 200; i++) {
         const body: any = await (await stack.apiAs(who, 'GET', `/data/import/jobs/${jobId}/results`)).json();
-        if (body.status === 'succeeded') return body.results;
+        // A job whose every row failed ends `failed`, one with any row ok
+        // ends `succeeded` (maintainer ruling B, record 6509036166).
+        if (body.status === 'succeeded' || body.status === 'failed') {
+          expect(body.status, JSON.stringify(body)).toBe((body.results as Row[]).some((r) => r.ok) ? 'succeeded' : 'failed');
+          return body.results;
+        }
         expect(['pending', 'running'], JSON.stringify(body)).toContain(body.status);
         await new Promise((r) => setTimeout(r, 25));
       }

packages/rest/src/data-action-routes.ts

@@ -179,6 +179,14 @@ function describeNonFieldMap(value: unknown): string {
     return `a ${typeof value}`;
 }
 
+/**
+ * The `error` of an async import job that ended `failed` because every row
+ * failed (maintainer ruling B, record 6509036166): the per-row reasons are the
+ * job's `results`, which this sentence points the reader at.
+ */
+const IMPORT_JOB_EVERY_ROW_FAILED =
+    'Every row of this import failed and nothing was imported; each row\'s reason is in the job\'s results.';
+
 /** [#27500] The formats `GET /data/:object/export` serves, csv first as the default. */
 const EXPORT_FORMATS = ['csv', 'json', 'xlsx'] as const;
 type ExportFormatServed = typeof EXPORT_FORMATS[number];
@@ -762,16 +770,28 @@ export function registerDataActionEndpoints(this: DataActionRouteHost, basePath:
                         // the durable row 'cancelled', and a late 'succeeded' here
                         // would silently overwrite it (framework#2824). Counts stay
                         // truthful either way — they reflect what was written.
-                        let finalStatus = summary.cancelled ? 'cancelled' : 'succeeded';
+                        let finalStatus: 'cancelled' | 'succeeded' | 'failed' = summary.cancelled ? 'cancelled' : 'succeeded';
                         if (finalStatus === 'succeeded' && this.cancelledImportJobs.has(jobId)) finalStatus = 'cancelled';
                         if (finalStatus === 'succeeded') {
                             try {
                                 const row = await loadImportJob(p, jobId, environmentId, context);
                                 if (String(row?.status ?? '') === 'cancelled') finalStatus = 'cancelled';
                             } catch { /* keep succeeded */ }
                         }
+                        // A job that was not cancelled, had rows, and created,
+                        // updated or skipped none of them accomplished nothing: it
+                        // ends `failed`, not `succeeded` (maintainer ruling B,
+                        // record 6509036166). Decided after every cancel re-check
+                        // above, so a cancel still wins. `ok` is not the test: a
+                        // skipped row is not counted in it. A zero-row job, an
+                        // all-skipped job and a partly-failed job stay `succeeded`
+                        // on purpose; a dry run follows the same rule.
+                        const everyRowFailed = finalStatus === 'succeeded' && summary.total > 0
+                            && summary.created + summary.updated + summary.skipped === 0;
+                        if (everyRowFailed) finalStatus = 'failed';
                         await patch({
                             status: finalStatus,
+                            ...(everyRowFailed ? { error: IMPORT_JOB_EVERY_ROW_FAILED } : {}),
                             processed_rows: summary.processed,
                             created_count: summary.created,
                             updated_count: summary.updated,

packages/rest/src/import-job-integration.test.ts

@@ -508,3 +508,123 @@ describe('async import job — an unregistered object is refused before a job ex
     expect(rows[0]).toMatchObject({ id: res._json.jobId, object_name: 'task' });
   });
 });
+
+/**
+ * The terminal status of a job that wrote nothing (maintainer ruling B, record
+ * 6509036166). A job that was not cancelled, had at least one row, and ended
+ * with no row created, updated or skipped ends `failed`, with `error` naming
+ * the outcome and pointing at `results`. Kept `succeeded` on purpose: a job
+ * whose rows partly failed (the partial pin in the first describe above), a job
+ * whose every row was skipped, and a job with no rows. Cancellation still wins.
+ * A dry run follows the same rule.
+ */
+describe('async import job — a job whose every row failed ends failed', () => {
+  let ctx: Awaited<ReturnType<typeof boot>>;
+  beforeEach(async () => {
+    ctx = await boot();
+    expect(ctx.create && ctx.progress && ctx.results && ctx.undo).toBeTruthy();
+  });
+
+  const badRows = (prefix: string) => [
+    { id: `${prefix}1`, title: 'one', score: 'not-a-number' },
+    { id: `${prefix}2`, title: 'two', score: 'nope' },
+    { id: `${prefix}3`, title: 'three', score: 'x' },
+  ];
+
+  it('every row fails: the job ends failed, errors === total, and error is set', async () => {
+    const res = await callCreate(ctx.create, { format: 'json', rows: badRows('af') });
+    expect(res._status).toBe(201);
+    const done = await waitForTerminal(ctx.progress, res._json.jobId);
+    expect(done).toMatchObject({ status: 'failed', total: 3, processed: 3, created: 0, updated: 0, skipped: 0, errors: 3 });
+    expect(done.errors).toBe(done.total);
+    expect(typeof done.error).toBe('string');
+    expect(done.error).toMatch(/results/);
+    // The per-row report is still delivered beside the status.
+    const results = await callJob(ctx.results, res._json.jobId);
+    expect(results._json.results).toHaveLength(3);
+    expect(results._json.results.every((r: any) => r.ok === false)).toBe(true);
+    // Nothing landed, and the undo reader answers as it did for this job before.
+    expect(await ctx.engine.find('task', { where: {} })).toHaveLength(0);
+    expect(done.undoable).toBe(false);
+    const u = await callJob(ctx.undo, res._json.jobId);
+    expect(u._status).toBe(422);
+    expect(u._json.code).toBe('NOT_UNDOABLE');
+    // The history filter sees it under its real status.
+    const failedOnly = await callJob(ctx.list, '', { status: 'failed' });
+    expect(failedOnly._json.jobs.map((j: any) => j.jobId)).toEqual([res._json.jobId]);
+  });
+
+  it('control: every row skipped keeps succeeded', async () => {
+    const res = await callCreate(ctx.create, {
+      format: 'json', writeMode: 'update', matchFields: ['id'],
+      rows: [{ id: 'sk1', title: 'a' }, { id: 'sk2', title: 'b' }],
+    });
+    expect(res._status).toBe(201);
+    const done = await waitForTerminal(ctx.progress, res._json.jobId);
+    expect(done).toMatchObject({ status: 'succeeded', total: 2, created: 0, updated: 0, skipped: 2, errors: 0 });
+    expect(done.error).toBeUndefined();
+  });
+
+  it('control: a job with no rows keeps succeeded', async () => {
+    const res = await callCreate(ctx.create, { format: 'csv', csv: 'id,title\n' });
+    expect(res._status).toBe(201);
+    expect(res._json.total).toBe(0);
+    const done = await waitForTerminal(ctx.progress, res._json.jobId);
+    expect(done).toMatchObject({ status: 'succeeded', total: 0, errors: 0 });
+    expect(done.error).toBeUndefined();
+  });
+
+  it('every row of a dry run fails: the job ends failed', async () => {
+    const res = await callCreate(ctx.create, { format: 'json', dryRun: true, rows: badRows('dr') });
+    expect(res._status).toBe(201);
+    const done = await waitForTerminal(ctx.progress, res._json.jobId);
+    expect(done).toMatchObject({ status: 'failed', dryRun: true, total: 3, created: 0, updated: 0, skipped: 0, errors: 3 });
+    expect(typeof done.error).toBe('string');
+  });
+
+  it('control: a valid dry run keeps succeeded', async () => {
+    const res = await callCreate(ctx.create, { format: 'json', dryRun: true, rows: [{ id: 'dv1', title: 'ok' }] });
+    const done = await waitForTerminal(ctx.progress, res._json.jobId);
+    expect(done).toMatchObject({ status: 'succeeded', dryRun: true, created: 1, errors: 0 });
+  });
+
+  it('control: a cancel that lands after the last row still wins over an all-failed outcome', async () => {
+    // Every row collides with a seeded primary key, so each fails at the
+    // driver; the slowed writes let the cancel land while the job runs, and
+    // the 3-row job has no mid-loop checkpoint, so the loop finishes first.
+    const slowCtx = await boot((driver) => {
+      const bulkCreate = driver.bulkCreate.bind(driver);
+      driver.bulkCreate = async (o: string, rows2: any[], ...rest: any[]) => {
+        if (o === 'task') await new Promise((r) => setImmediate(r));
+        return bulkCreate(o, rows2, ...rest);
+      };
+      const create = driver.create.bind(driver);
+      driver.create = async (o: string, data: any, ...rest: any[]) => {
+        if (o === 'task') await new Promise((r) => setImmediate(r));
+        return create(o, data, ...rest);
+      };
+    });
+    for (const id of ['cf1', 'cf2', 'cf3']) {
+      await slowCtx.protocol.createData({ object: 'task', data: { id, title: 'seeded' } });
+    }
+    const created = await callCreate(slowCtx.create, {
+      format: 'json',
+      rows: [{ id: 'cf1', title: 'a' }, { id: 'cf2', title: 'b' }, { id: 'cf3', title: 'c' }],
+    });
+    const jobId = created._json.jobId;
+    const c = await callJob(slowCtx.cancel, jobId);
+    expect(c._json).toMatchObject({ success: true });
+
+    await waitForTerminal(slowCtx.progress, jobId);
+    // The cancel route writes `cancelled` first; wait until the worker's own
+    // final patch has landed (it is the only write of `results`), then read the row.
+    for (let i = 0; i < 400; i++) {
+      const r = await callJob(slowCtx.results, jobId);
+      if ((r._json?.results ?? []).length === 3) break;
+      await new Promise((rr) => setTimeout(rr, 5));
+    }
+    const after = await callJob(slowCtx.progress, jobId);
+    expect(after._json.status).toBe('cancelled');
+    expect(after._json).toMatchObject({ processed: 3, created: 0, updated: 0, skipped: 0, errors: 3 });
+  });
+});

packages/rest/src/import-row-cell-errors.test.ts

@@ -106,8 +106,12 @@ async function boot() {
     return r.body.results as any[];
   };
 
-  /** The async job's per-row report, read off the results route once the job is terminal. */
-  const jobRows = async (rows: Array<Record<string, unknown>>, dryRun: boolean) => {
+  /**
+   * The async job's per-row report, read off the results route once the job is terminal.
+   * A job whose every row failed ends `failed` (maintainer ruling B, record 6509036166);
+   * one with any row created, updated or skipped ends `succeeded`.
+   */
+  const jobRows = async (rows: Array<Record<string, unknown>>, dryRun: boolean, terminal: 'succeeded' | 'failed' = 'succeeded') => {
     const created = await call('POST', '/api/v1/data/:object/import/jobs', { format: 'json', rows, dryRun });
     expect(created.status, JSON.stringify(created.body)).toBe(201);
     const jobId = String(created.body.jobId);
@@ -117,7 +121,7 @@ async function boot() {
       if (['succeeded', 'failed', 'cancelled'].includes(progress.body?.status)) break;
       await new Promise((r) => setTimeout(r, 5));
     }
-    expect(progress?.body?.status, JSON.stringify(progress?.body)).toBe('succeeded');
+    expect(progress?.body?.status, JSON.stringify(progress?.body)).toBe(terminal);
     const results = await call('GET', '/api/v1/data/import/jobs/:jobId/results', undefined, { jobId });
     return [...(results.body.results as any[])].sort((a, b) => a.row - b.row);
   };
@@ -174,15 +178,15 @@ describe('[#25582] a row that failed for another reason carries no `errors`', ()
 
   it('the dry run\'s validate verdict (sync and async)', async () => {
     const b = await boot();
-    for (const rows of [await b.syncRows([NO_NAME], true), await b.jobRows([NO_NAME], true)]) {
+    for (const rows of [await b.syncRows([NO_NAME], true), await b.jobRows([NO_NAME], true, 'failed')]) {
       expect(rows[0], JSON.stringify(rows[0])).toMatchObject({ row: 1, ok: false, action: 'failed', field: 'name', code: 'required' });
       expect(rows[0]).not.toHaveProperty('errors');
     }
   });
 
   it('the commit\'s refused write (sync and async)', async () => {
     const b = await boot();
-    for (const rows of [await b.syncRows([NO_NAME], false), await b.jobRows([NO_NAME], false)]) {
+    for (const rows of [await b.syncRows([NO_NAME], false), await b.jobRows([NO_NAME], false, 'failed')]) {
       expect(rows[0], JSON.stringify(rows[0])).toMatchObject({ row: 1, ok: false, action: 'failed', field: 'name', code: 'required' });
       expect(rows[0]).not.toHaveProperty('errors');
     }