Skip to content

Commit 330c366

Browse files
committed
fix: address CodeRabbit review on #4418
- waiting-run diagnosis: 'unknown' with concurrency evidence in hand no longer claims the evidence is missing; an elapsed delay says 'not yet enqueued' instead of hiding behind time-from-creation - queue metrics route: drop the double decode that 500ed on names with a literal percent sign - evidence schema: kind must match the URI's own kind - seed-queue-metrics: default-binding imports like the other seeders
1 parent 65a4942 commit 330c366

5 files changed

Lines changed: 56 additions & 20 deletions

File tree

apps/webapp/app/presenters/v3/waitingRun/waitingRunDiagnosis.ts

Lines changed: 14 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -238,6 +238,12 @@ function describeRun(run: WaitingRunRow, now: Date): WaitingRunDiagnosis["run"]
238238
// A delayed run isn't in the queue yet — this is a schedule, NOT queue latency.
239239
waitingLabel = `scheduled to start at ${run.delayUntil.toISOString()}`;
240240
waitingBasis = "delay_until";
241+
} else if (run.delayUntil && run.queuedAt === null) {
242+
// The delay elapsed but the run hasn't been enqueued yet — say that, not "time from
243+
// creation", which would hide that the schedule already passed.
244+
const ms = Math.max(0, now.getTime() - run.delayUntil.getTime());
245+
waitingLabel = `delay elapsed ${formatMs(ms)} ago, not yet enqueued`;
246+
waitingBasis = "delay_until";
241247
} else {
242248
// No queuedAt to measure from. Report creation age and SAY that's what it is.
243249
const ms = Math.max(0, (run.startedAt ?? now).getTime() - run.createdAt.getTime());
@@ -393,9 +399,14 @@ function deriveCause(
393399
if (!backlogged || (observedRate !== null && observedRate > 0)) {
394400
return { cause: "draining_normally", evidence };
395401
}
396-
// Backlogged with nothing moving, but no capacity evidence to say whether that's a limit or a
397-
// stall. flow.ts's lesson: don't name a concurrency cause without the evidence for it.
398-
return { cause: "unknown", evidence: { ...evidence, missing: "env_concurrency" } };
402+
// Backlogged with nothing conclusive. Only claim evidence is MISSING when it actually is —
403+
// with concurrency evidence in hand this is "inconclusive", not "blind" (flow.ts's lesson:
404+
// don't name a concurrency cause without the evidence for it, and don't mislabel evidence
405+
// that's present).
406+
return {
407+
cause: "unknown",
408+
evidence: { ...evidence, missing: hasConcurrencyEvidence ? null : "env_concurrency" },
409+
};
399410
}
400411

401412
// ---------------------------------------------------------------------------

apps/webapp/app/routes/api.v1.queues.$queueParam.metrics.ts

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -66,7 +66,9 @@ export const loader = createLoaderApiRoute(
6666
},
6767
},
6868
async ({ params, searchParams, authentication }) => {
69-
const name = decodeURIComponent(params.queueParam).replace(/%2F/g, "/");
69+
// Remix already URL-decoded the param and the schema's transform unescaped %2F —
70+
// decoding again would throw a 500 on queue names containing a literal "%".
71+
const name = params.queueParam;
7072
const queue = searchParams.type === "task" && !name.startsWith("task/") ? `task/${name}` : name;
7173

7274
const windowMs = periodMs(searchParams.period);

apps/webapp/seed-queue-metrics.mts

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,16 @@
1-
import { prisma } from "./app/db.server";
2-
import { createOrganization } from "./app/models/organization.server";
3-
import { createProject } from "./app/models/project.server";
41
import { ClickHouse } from "@internal/clickhouse";
52
import type { QueueMetricsRawV1Input } from "@internal/clickhouse";
6-
import { generateFriendlyId } from "./app/v3/friendlyIdentifiers";
3+
// App modules compile to CommonJS under tsx (webapp has no "type": "module"), so
4+
// import them as default bindings and destructure — same pattern as seed-agent-examples.
5+
import dbServer from "./app/db.server";
6+
import organizationServer from "./app/models/organization.server";
7+
import projectServer from "./app/models/project.server";
8+
import friendlyIdentifiers from "./app/v3/friendlyIdentifiers";
9+
10+
const { prisma } = dbServer;
11+
const { createOrganization } = organizationServer;
12+
const { createProject } = projectServer;
13+
const { generateFriendlyId } = friendlyIdentifiers;
714

815
// Queue metrics simulator: writes realistic raw rows into a synthetic tenant's
916
// queue_metrics_raw_v1 and lets the MV build queue_metrics_v1 (the same path the real

apps/webapp/test/waitingRunDiagnosis.test.ts

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -224,7 +224,10 @@ describe("computeWaitingRunDiagnosis", () => {
224224
run({ status: "PENDING", queuedAt: null, delayUntil: new Date(NOW.getTime() - 60_000) }),
225225
signals()
226226
);
227-
expect(result?.run.waitingBasis).toBe("created_at");
227+
// The delay already passed but the run wasn't enqueued — the label says the
228+
// delay elapsed rather than hiding it behind "time from creation".
229+
expect(result?.run.waitingBasis).toBe("delay_until");
230+
expect(result?.run.waitingLabel).toContain("not yet enqueued");
228231
});
229232
});
230233

internal-packages/dashboard-agent-contracts/src/evidence.ts

Lines changed: 24 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -7,18 +7,31 @@
77
* validated, and it rots. If it can't be expressed as a URI, it isn't evidence.
88
*/
99
import { z } from "zod";
10-
import { triggerUriKindSchema, triggerUriSchema } from "./trigger-uri.js";
10+
import { safeParseTriggerUri, triggerUriKindSchema, triggerUriSchema } from "./trigger-uri.js";
1111

12-
export const evidenceSchema = z.object({
13-
/** What kind of resource this cites. Mirrors the URI's resource kinds. */
14-
kind: triggerUriKindSchema,
15-
/** The resource itself. */
16-
uri: triggerUriSchema,
17-
/** Short human label, e.g. "run_abc123 failed span" or "processOrder.ts:42". */
18-
label: z.string(),
19-
/** Optional verbatim snippet (error message, log line, source lines). */
20-
excerpt: z.string().optional(),
21-
});
12+
export const evidenceSchema = z
13+
.object({
14+
/** What kind of resource this cites. Mirrors the URI's resource kinds. */
15+
kind: triggerUriKindSchema,
16+
/** The resource itself. */
17+
uri: triggerUriSchema,
18+
/** Short human label, e.g. "run_abc123 failed span" or "processOrder.ts:42". */
19+
label: z.string(),
20+
/** Optional verbatim snippet (error message, log line, source lines). */
21+
excerpt: z.string().optional(),
22+
})
23+
// A citation must not claim one resource type while pointing at another — the
24+
// renderer picks its icon and label from `kind`, so a mismatch is a lie.
25+
.superRefine((evidence, ctx) => {
26+
const parsed = safeParseTriggerUri(evidence.uri);
27+
if (parsed.success && parsed.data.kind !== evidence.kind) {
28+
ctx.addIssue({
29+
code: z.ZodIssueCode.custom,
30+
path: ["kind"],
31+
message: `kind "${evidence.kind}" does not match the URI's kind "${parsed.data.kind}"`,
32+
});
33+
}
34+
});
2235

2336
export type Evidence = z.infer<typeof evidenceSchema>;
2437
export type EvidenceKind = Evidence["kind"];

0 commit comments

Comments
 (0)