Explorar o código

fix(queue): chain the Timeline in the scheduler's order, not queue position (issue #3043)

    Turning Shortest Job First on reordered the pending list and the
    scheduler, and left the Timeline drawing the pre-SJF queue for good. It
    chained each swimlane's bars by queue position alone and was never told
    the setting existed -- so the one view whose whole job is to say when
    each print will run was the one view answering for an order the
    scheduler had no intention of using.

    Bars now chain in the order the scheduler will dispatch: jumped items
    first, then shortest print time with an unknown duration last, then
    position. The starvation guard is visible there too, so a long print
    that has finally come up reads as next rather than staying buried behind
    every short job on the lane.

    Three surfaces claim to show queue order -- the pending list, the "if
    started now" ETA, and the Timeline -- and each had its own copy of the
    comparator, which is how one of them came to be missing a whole clause.
    They now share one, written against the scheduler's ORDER BY.

    Grouping came along with it: the pending list folded a model name down
    to its first character to build a lane key, so Any X1C and Any X2D both
    landed on -88 (as did Any P1S and Any P1P) and the two lanes interleaved
    into a single run of rows.

    Backend untouched. The scheduler was dispatching correctly the whole
    time; only the drawing of it was wrong.
maziggy hai 5 días
pai
achega
2b8caa8bbe

+ 116 - 0
frontend/src/__tests__/components/QueueTimelineView.test.tsx

@@ -0,0 +1,116 @@
+/**
+ * The timeline draws each lane's pending jobs chained end to end, so the order
+ * it chains them in is a claim about what the scheduler will dispatch next.
+ *
+ * It used to chain by queue position alone, which meant turning
+ * Shortest-Job-First on changed the scheduler and the pending list but left
+ * the timeline drawing the pre-SJF queue forever (#3043).
+ */
+
+import { describe, it, expect } from 'vitest';
+import { render, screen } from '@testing-library/react';
+import { QueueTimelineView } from '../../components/QueueTimelineView';
+import type { PrintQueueItem, Printer } from '../../api/client';
+
+const HOUR = 3600;
+
+const printers = [{ id: 1, name: 'Workshop X1C', model: 'X1C' }] as unknown as Printer[];
+
+// Something has to be running for a lane to forecast at all: an idle printer's
+// ASAP queue has no committed anchor to chain from, so the view drops it.
+const running = {
+  id: 100,
+  printer_id: 1,
+  status: 'printing',
+  position: 0,
+  archive_name: 'Running now',
+  print_time_seconds: HOUR,
+  started_at: new Date(Date.now() - 30 * 60 * 1000).toISOString(),
+} as unknown as PrintQueueItem;
+
+const pending = (
+  id: number,
+  name: string,
+  position: number,
+  printTimeSeconds: number,
+  over: Partial<PrintQueueItem> = {},
+) =>
+  ({
+    id,
+    printer_id: 1,
+    status: 'pending',
+    position,
+    archive_name: name,
+    print_time_seconds: printTimeSeconds,
+    scheduled_time: null,
+    manual_start: false,
+    waiting_reason: null,
+    ...over,
+  }) as unknown as PrintQueueItem;
+
+const queueItems = [
+  running,
+  pending(1, 'Long job', 1, 2 * HOUR),
+  pending(2, 'Short job', 2, HOUR / 2),
+  pending(3, 'Medium job', 3, HOUR),
+];
+
+function renderTimeline(sjfEnabled: boolean) {
+  render(
+    <QueueTimelineView
+      queueItems={queueItems}
+      printers={printers}
+      printerStatuses={{ 1: { progress: 50, remaining_time: 30, state: 'RUNNING' } }}
+      sjfEnabled={sjfEnabled}
+      onItemClick={() => {}}
+      t={(key: string) => key}
+    />,
+  );
+}
+
+/** Bar names, left to right. Bars are absolutely positioned, so the rendered
+ *  offset is what the user reads -- not DOM order. The tooltip leads with the
+ *  display name, which is cleaner to match on than the bar's own text (name
+ *  and duration sit in adjacent divs with no separator between them). */
+function barsLeftToRight(): string[] {
+  return screen
+    .getAllByRole('button')
+    .filter(el => el.style.left.endsWith('%'))
+    .sort((a, b) => parseFloat(a.style.left) - parseFloat(b.style.left))
+    .map(el => (el.getAttribute('title') ?? '').split(' \u00b7 ')[0]);
+}
+
+describe('QueueTimelineView job ordering', () => {
+  it('chains by queue position when SJF is off', () => {
+    renderTimeline(false);
+    expect(barsLeftToRight()).toEqual(['Running now', 'Long job', 'Short job', 'Medium job']);
+  });
+
+  it('chains shortest-first when SJF is on', () => {
+    renderTimeline(true);
+    expect(barsLeftToRight()).toEqual(['Running now', 'Short job', 'Medium job', 'Long job']);
+  });
+
+  it('keeps a jumped item ahead of a shorter one', () => {
+    // The starvation guard is part of the scheduler's order too, so the
+    // timeline has to draw it or it lies about the long job that is finally
+    // about to run.
+    render(
+      <QueueTimelineView
+        queueItems={[
+          running,
+          pending(1, 'Short job', 1, HOUR / 2),
+          pending(2, 'Long job', 2, 2 * HOUR, { been_jumped: true }),
+        ]}
+        printers={printers}
+        printerStatuses={{ 1: { progress: 50, remaining_time: 30, state: 'RUNNING' } }}
+        sjfEnabled
+        onItemClick={() => {}}
+        t={(key: string) => key}
+      />,
+    );
+    // Position order would put the short job first, so this only passes if
+    // been_jumped is actually consulted.
+    expect(barsLeftToRight()).toEqual(['Running now', 'Long job', 'Short job']);
+  });
+});

+ 133 - 0
frontend/src/__tests__/utils/queueOrder.test.ts

@@ -0,0 +1,133 @@
+/**
+ * The comparator every "queue order" surface shares (#3043).
+ *
+ * It has to agree with the scheduler's ORDER BY in
+ * `print_scheduler.check_queue`, because a timeline or a pending list that
+ * disagrees is telling the user the queue will run in an order it won't.
+ */
+
+import { describe, it, expect } from 'vitest';
+import {
+  compareQueueOrder,
+  compareQueueOrderAcrossLanes,
+  queueLaneKey,
+} from '../../utils/queueOrder';
+
+interface Item {
+  id: number;
+  printer_id?: number | null;
+  target_model?: string | null;
+  been_jumped?: boolean;
+  print_time_seconds?: number | null;
+  position: number;
+}
+
+const item = (id: number, over: Partial<Item> = {}): Item => ({
+  id,
+  printer_id: 1,
+  position: id,
+  print_time_seconds: 3600,
+  ...over,
+});
+
+const idsInOrder = (items: Item[], sjf: boolean) =>
+  [...items].sort((a, b) => compareQueueOrder(a, b, sjf)).map(i => i.id);
+
+describe('compareQueueOrder', () => {
+  it('leaves the queue in position order when SJF is off', () => {
+    const items = [
+      item(1, { position: 3, print_time_seconds: 60 }),
+      item(2, { position: 1, print_time_seconds: 99999 }),
+      item(3, { position: 2, print_time_seconds: 600 }),
+    ];
+    expect(idsInOrder(items, false)).toEqual([2, 3, 1]);
+  });
+
+  it('puts the shortest print first when SJF is on', () => {
+    const items = [
+      item(1, { position: 1, print_time_seconds: 7200 }),
+      item(2, { position: 2, print_time_seconds: 600 }),
+      item(3, { position: 3, print_time_seconds: 3600 }),
+    ];
+    expect(idsInOrder(items, true)).toEqual([2, 3, 1]);
+  });
+
+  it('sorts an item with no known duration last, not first', () => {
+    // Treating a missing duration as 0 would make an unsliced job win every
+    // comparison it entered -- the SQL says NULLS LAST for the same reason.
+    const items = [
+      item(1, { position: 1, print_time_seconds: null }),
+      item(2, { position: 2, print_time_seconds: 7200 }),
+      item(3, { position: 3, print_time_seconds: undefined }),
+    ];
+    expect(idsInOrder(items, true)).toEqual([2, 1, 3]);
+  });
+
+  it('promotes a jumped item ahead of a shorter one', () => {
+    // Starvation guard: a long print that keeps being overtaken has to run
+    // eventually, so once it has been jumped it outranks print time.
+    const items = [
+      item(1, { position: 1, print_time_seconds: 36000, been_jumped: true }),
+      item(2, { position: 2, print_time_seconds: 300 }),
+    ];
+    expect(idsInOrder(items, true)).toEqual([1, 2]);
+  });
+
+  it('falls back to position when two prints are the same length', () => {
+    const items = [
+      item(1, { position: 5, print_time_seconds: 3600 }),
+      item(2, { position: 2, print_time_seconds: 3600 }),
+    ];
+    expect(idsInOrder(items, true)).toEqual([2, 1]);
+  });
+});
+
+describe('queueLaneKey', () => {
+  it('names a lane after the printer, the model, or neither', () => {
+    expect(queueLaneKey(item(1, { printer_id: 4 }))).toBe('printer:4');
+    expect(queueLaneKey(item(2, { printer_id: null, target_model: 'X1C' }))).toBe('model:X1C');
+    expect(queueLaneKey(item(3, { printer_id: null, target_model: null }))).toBe('unassigned');
+  });
+
+  it('keeps two models apart that share a first letter', () => {
+    // The flat pending list used to fold the model down to -charCodeAt(0),
+    // which gave X1C and X2D the same key and interleaved the two lanes.
+    expect(queueLaneKey(item(1, { printer_id: null, target_model: 'X1C' }))).not.toBe(
+      queueLaneKey(item(2, { printer_id: null, target_model: 'X2D' })),
+    );
+  });
+});
+
+describe('compareQueueOrderAcrossLanes', () => {
+  const sortIds = (items: Item[], sjf: boolean) =>
+    [...items].sort((a, b) => compareQueueOrderAcrossLanes(a, b, sjf)).map(i => i.id);
+
+  it('keeps each lane contiguous and sorted within itself', () => {
+    const items = [
+      item(1, { printer_id: 2, position: 1, print_time_seconds: 7200 }),
+      item(2, { printer_id: 1, position: 2, print_time_seconds: 7200 }),
+      item(3, { printer_id: 2, position: 3, print_time_seconds: 600 }),
+      item(4, { printer_id: 1, position: 4, print_time_seconds: 600 }),
+    ];
+    expect(sortIds(items, true)).toEqual([4, 2, 3, 1]);
+  });
+
+  it('does not interleave two model lanes with the same initial', () => {
+    const items = [
+      item(1, { printer_id: null, target_model: 'X1C', position: 1, print_time_seconds: 7200 }),
+      item(2, { printer_id: null, target_model: 'X2D', position: 2, print_time_seconds: 600 }),
+      item(3, { printer_id: null, target_model: 'X1C', position: 3, print_time_seconds: 300 }),
+    ];
+    expect(sortIds(items, true)).toEqual([3, 1, 2]);
+  });
+
+  it('orders printers numerically, then model lanes, then unassigned', () => {
+    const items = [
+      item(1, { printer_id: null, target_model: null, position: 1 }),
+      item(2, { printer_id: null, target_model: 'P1S', position: 2 }),
+      item(3, { printer_id: 10, position: 3 }),
+      item(4, { printer_id: 2, position: 4 }),
+    ];
+    expect(sortIds(items, false)).toEqual([4, 3, 2, 1]);
+  });
+});

+ 10 - 10
frontend/src/components/QueueTimelineView.tsx

@@ -1,6 +1,7 @@
 import { useState, useMemo, useEffect, useRef } from 'react';
 import { ChevronLeft, ChevronRight, Clock, Layers, Printer as PrinterIcon } from 'lucide-react';
 import { formatDuration, parseUTCDate } from '../utils/date';
+import { compareQueueOrder, queueLaneKey } from '../utils/queueOrder';
 import type { PrintQueueItem, Printer } from '../api/client';
 import { api } from '../api/client';
 import { Button } from './Button';
@@ -32,6 +33,10 @@ interface QueueTimelineViewProps {
   queueItems: PrintQueueItem[];
   printers: Printer[];
   printerStatuses: Record<number, { progress?: number; remaining_time?: number; state?: string }>;
+  /** `queue_shortest_first`. The bars chain in dispatch order, and dispatch
+   *  order depends on it, so without it the timeline drew a queue the
+   *  scheduler had no intention of running (#3043). */
+  sjfEnabled: boolean;
   onItemClick: (item: PrintQueueItem) => void;
   t: (key: string, options?: Record<string, unknown>) => string;
 }
@@ -57,6 +62,7 @@ export function QueueTimelineView({
   queueItems,
   printers,
   printerStatuses,
+  sjfEnabled,
   onItemClick,
   t,
 }: QueueTimelineViewProps) {
@@ -97,12 +103,6 @@ export function QueueTimelineView({
     // Chain-end timestamp per lane (where the next pending item's bar starts).
     const chainEndByLane = new Map<string, number>();
 
-    const laneKeyOf = (item: PrintQueueItem): string => {
-      if (item.printer_id != null) return `printer:${item.printer_id}`;
-      if (item.target_model) return `model:${item.target_model}`;
-      return 'unassigned';
-    };
-
     for (const item of queueItems) {
       if (item.status === 'printing') {
         const status = item.printer_id != null ? printerStatuses[item.printer_id] : undefined;
@@ -124,7 +124,7 @@ export function QueueTimelineView({
           progress: status?.progress ?? undefined,
           type: 'printing',
         });
-        const lk = laneKeyOf(item);
+        const lk = queueLaneKey(item);
         lanesWithActive.add(lk);
         chainEndByLane.set(lk, Math.max(chainEndByLane.get(lk) ?? nowMs, endTime.getTime()));
       } else if (item.status === 'pending') {
@@ -132,7 +132,7 @@ export function QueueTimelineView({
         // won't auto-dispatch, so a bar would lie.
         if (item.manual_start) continue;
         if (item.waiting_reason) continue;
-        const lk = laneKeyOf(item);
+        const lk = queueLaneKey(item);
         if (!pendingByLaneKey.has(lk)) pendingByLaneKey.set(lk, []);
         pendingByLaneKey.get(lk)!.push(item);
       }
@@ -140,7 +140,7 @@ export function QueueTimelineView({
 
     const sixMonthsFromNow = Date.now() + 180 * 24 * HOUR_MS;
     for (const [lk, items] of pendingByLaneKey) {
-      items.sort((a, b) => a.position - b.position);
+      items.sort((a, b) => compareQueueOrder(a, b, sjfEnabled));
       const hasActive = lanesWithActive.has(lk);
       // A lane is timelineable when EITHER it has an active print (chain
       // forecast off its end) OR its first pending item is scheduled (a
@@ -167,7 +167,7 @@ export function QueueTimelineView({
       }
     }
     return result;
-  }, [queueItems, printerStatuses, nowMs]);
+  }, [queueItems, printerStatuses, nowMs, sjfEnabled]);
 
   // Lanes: every printer + every distinct target_model with queue activity
   // + an "unassigned" lane if needed. Printers that have NO events queued

+ 7 - 27
frontend/src/pages/QueuePage.tsx

@@ -77,6 +77,7 @@ import { useAuth } from '../contexts/AuthContext';
 import { QueueStatsBar } from '../components/QueueStatsBar';
 import { CompactHistoryRow } from '../components/CompactHistoryRow';
 import { QueueTimelineView } from '../components/QueueTimelineView';
+import { compareQueueOrder, compareQueueOrderAcrossLanes } from '../utils/queueOrder';
 import { BatchOrdersView } from '../components/BatchOrdersView';
 
 function formatWeight(g: number, useKg = false): string {
@@ -1784,22 +1785,7 @@ export function QueuePage() {
 
     // When SJF is enabled, override sort to match scheduler order
     if (settings?.queue_shortest_first) {
-      return [...items].sort((a, b) => {
-        // Group by printer first (nulls = model-based, grouped by target_model)
-        const aPrinter = a.printer_id ?? -(a.target_model?.charCodeAt(0) ?? 0);
-        const bPrinter = b.printer_id ?? -(b.target_model?.charCodeAt(0) ?? 0);
-        if (aPrinter !== bPrinter) return aPrinter - bPrinter;
-        // Within same printer/model: jumped items first (starvation guard)
-        const aJumped = a.been_jumped ? 1 : 0;
-        const bJumped = b.been_jumped ? 1 : 0;
-        if (aJumped !== bJumped) return bJumped - aJumped;
-        // Shortest print time next (nulls last)
-        const aTime = a.print_time_seconds ?? Infinity;
-        const bTime = b.print_time_seconds ?? Infinity;
-        if (aTime !== bTime) return aTime - bTime;
-        // Position as tiebreaker
-        return a.position - b.position;
-      });
+      return [...items].sort((a, b) => compareQueueOrderAcrossLanes(a, b, true));
     }
 
     return [...items].sort((a, b) => {
@@ -1863,17 +1849,10 @@ export function QueuePage() {
 
     // Mirrors the scheduler's own ordering so "next up" here means the item the
     // scheduler would actually dispatch next, not whatever the user sorted by.
-    const schedulerOrder = (a: PrintQueueItem, b: PrintQueueItem): number => {
-      if (settings?.queue_shortest_first) {
-        const aJumped = a.been_jumped ? 1 : 0;
-        const bJumped = b.been_jumped ? 1 : 0;
-        if (aJumped !== bJumped) return bJumped - aJumped;
-        const aTime = a.print_time_seconds ?? Infinity;
-        const bTime = b.print_time_seconds ?? Infinity;
-        if (aTime !== bTime) return aTime - bTime;
-      }
-      return a.position - b.position;
-    };
+    // Bucketed by printer immediately below, so the within-lane comparator is
+    // the right one -- no cross-lane grouping needed.
+    const schedulerOrder = (a: PrintQueueItem, b: PrintQueueItem): number =>
+      compareQueueOrder(a, b, settings?.queue_shortest_first ?? false);
 
     // Claimants for each printer, in the order the scheduler would take them.
     // Staged and future-scheduled items are excluded: the scheduler skips both
@@ -2557,6 +2536,7 @@ export function QueuePage() {
           queueItems={queue || []}
           printers={printers || []}
           printerStatuses={printerStatusMap}
+          sjfEnabled={settings?.queue_shortest_first ?? false}
           onItemClick={(item) => {
             if (['completed', 'failed', 'skipped', 'cancelled'].includes(item.status)) {
               setRequeueItem(item);

+ 102 - 0
frontend/src/utils/queueOrder.ts

@@ -0,0 +1,102 @@
+/**
+ * The order the scheduler will actually dispatch pending queue items in.
+ *
+ * The backend decides this in SQL (`print_scheduler.check_queue`):
+ *
+ *     ORDER BY printer_id, target_model,
+ *              been_jumped DESC,
+ *              print_time_seconds ASC NULLS LAST,
+ *              position
+ *
+ * with the first two columns acting purely as a grouping — a row carries
+ * either a `printer_id` or a `target_model`, never both — and the rest
+ * deciding who goes first inside that group. Every UI surface that claims to
+ * show queue order has to reproduce it, and each one that reproduced it
+ * privately drifted: the timeline sorted by `position` alone and so ignored
+ * Shortest-Job-First entirely (#3043). One comparator, three callers.
+ */
+
+interface OrderableQueueItem {
+  printer_id?: number | null;
+  target_model?: string | null;
+  been_jumped?: boolean;
+  print_time_seconds?: number | null;
+  position: number;
+}
+
+/**
+ * Which dispatch group an item belongs to: a named printer, a printer model,
+ * or neither. Items only compete with others in their own group, so this is
+ * both the timeline's swimlane and the outer sort key of a flat pending list.
+ *
+ * Returned as a string rather than a number because the group is a name, not
+ * a magnitude. The flat list used to fold `target_model` down to
+ * `-charCodeAt(0)`, which gave `X1C` and `X2D` (and `P1S` and `P1P`) the same
+ * key and interleaved two lanes into one.
+ */
+export function queueLaneKey(item: OrderableQueueItem): string {
+  if (item.printer_id != null) return `printer:${item.printer_id}`;
+  if (item.target_model) return `model:${item.target_model}`;
+  return 'unassigned';
+}
+
+/**
+ * Order two items competing for the same printer or model.
+ *
+ * @param sjfEnabled the `queue_shortest_first` setting. When off, the
+ *                   scheduler orders by position alone and so does this.
+ */
+export function compareQueueOrder(
+  a: OrderableQueueItem,
+  b: OrderableQueueItem,
+  sjfEnabled: boolean,
+): number {
+  if (sjfEnabled) {
+    // Starvation guard: an item something else was allowed to jump ahead of
+    // goes first next time, whatever the print times say.
+    const aJumped = a.been_jumped ? 1 : 0;
+    const bJumped = b.been_jumped ? 1 : 0;
+    if (aJumped !== bJumped) return bJumped - aJumped;
+
+    // Shortest first, and an item whose duration we don't know yet sorts last
+    // rather than winning by looking like a zero-second print (NULLS LAST).
+    const aTime = a.print_time_seconds ?? Infinity;
+    const bTime = b.print_time_seconds ?? Infinity;
+    if (aTime !== bTime) return aTime - bTime;
+  }
+
+  return a.position - b.position;
+}
+
+/**
+ * Order a flat list that spans several groups -- a pending list rather than a
+ * per-lane one. Groups stay contiguous; within each, the scheduler's own order
+ * applies.
+ *
+ * Groups themselves are ordered for reading, not to mirror the backend: named
+ * printers by id, then model lanes by name, then unassigned. The backend's own
+ * answer here is `ORDER BY printer_id` with a NULL in it, which SQLite sorts
+ * first and PostgreSQL sorts last -- nothing worth reproducing.
+ */
+export function compareQueueOrderAcrossLanes(
+  a: OrderableQueueItem,
+  b: OrderableQueueItem,
+  sjfEnabled: boolean,
+): number {
+  const aLane = queueLaneKey(a);
+  const bLane = queueLaneKey(b);
+  if (aLane !== bLane) {
+    if (a.printer_id != null && b.printer_id != null) return a.printer_id - b.printer_id;
+    const aRank = laneRank(a);
+    const bRank = laneRank(b);
+    if (aRank !== bRank) return aRank - bRank;
+    return aLane < bLane ? -1 : 1;
+  }
+  return compareQueueOrder(a, b, sjfEnabled);
+}
+
+function laneRank(item: OrderableQueueItem): number {
+  if (item.printer_id != null) return 0;
+  if (item.target_model) return 1;
+  return 2;
+}

A diferenza do arquivo foi suprimida porque é demasiado grande
+ 0 - 0
static/assets/index-DgrwpFp7.js


+ 1 - 1
static/index.html

@@ -26,7 +26,7 @@
 
     <!-- Splash screens for iOS -->
     <link rel="apple-touch-startup-image" href="/img/android-chrome-512x512.png" />
-    <script type="module" crossorigin src="/assets/index-BbLrhGQ7.js"></script>
+    <script type="module" crossorigin src="/assets/index-DgrwpFp7.js"></script>
     <link rel="stylesheet" crossorigin href="/assets/index-ChscM3lF.css">
   </head>
   <body>

Algúns arquivos non se mostraron porque demasiados arquivos cambiaron neste cambio