Skip to content

Commit 68729ef

Browse files
committed
Run one review at a time, and let a browser watch it
Work item W6: the scheduler, the event bus, the startup hook and the stream that carries a review's progress to a page. Reviews are slow and expensive and share one account's usage, so two at once would race for the same rate limit and make both slower for no gain. One runs and the rest queue. The queue is in memory only: on a restart it is empty and the reviews that were waiting are still drafts, which is honest for a local tool, because nothing was promised to them and nothing was spent on them. Persisting it would mean a crash could start expensive work nobody was watching. The manager announces only what the database already says, and reads the row before it emits. An event arriving before the row it describes would let the screen show a stage as started while the database still said draft, and reloading the page would appear to undo it. A test asserts that from inside the listener: it reads the row on every event and records any disagreement. Cancelling a queued review takes it out of the line without touching its status, because nothing was started and there is nothing to unwind. Cancelling a running one aborts the signal the service already knows how to map. The bus swallows a listener's error. A browser that disconnected mid-write must not stop the other watchers being told, and must not fail the review it was watching. Listeners are copied before iteration, because a done-event listener unsubscribes itself and mutating the set mid-iteration would skip whoever came after it. Two build problems surfaced only in the production build, and both were real rather than configuration noise. Turbopack reads the migrations folder's URL as a module it must resolve, so migrations moved to instrumentation and the path is assembled with join. And Next builds instrumentation for its edge runtime too, where none of this can load, so every import there is now inside register() and behind the runtime check. Six mutations checked and all caught: removing the concurrency cap, a cancel that aborts nothing, skipping orphan recovery at startup, announcing before reading the row, a stream that never closes, and a throwing listener taking the others down with it.
1 parent 2e24270 commit 68729ef

12 files changed

Lines changed: 1322 additions & 2 deletions

File tree

docs/DECISIONS.md

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -539,3 +539,31 @@ verified evidence, in writing, here.
539539
proved the value was recorded and not that it was the value used. Making the
540540
wrong thing unreachable is the stronger answer, and the batching behaviour
541541
itself is already covered by the pipeline's own context-window tests.
542+
- 2026-07-31 DECIDED (W6): the job queue lives in memory only. On a restart it
543+
is empty and the reviews that were waiting are still drafts. That is the
544+
honest behaviour for a local tool: nothing was promised to them and nothing
545+
was spent on them, and persisting a queue would mean a crash could start
546+
expensive work nobody was watching.
547+
- 2026-07-31 DECIDED (W6): the manager announces only what the database already
548+
says, and reads the row before emitting. An event that arrived before the row
549+
it describes would let the screen show a stage as started while the database
550+
still said draft, and reloading the page would appear to undo it.
551+
- 2026-07-31 DECIDED (W6): the bus swallows a listener's error rather than
552+
letting it escape. A browser that disconnected mid-write must not stop the
553+
other watchers being told and must not fail the review it was watching.
554+
Listeners are copied before iteration, because a done-event listener
555+
unsubscribes itself and mutating the set mid-iteration would skip whoever
556+
came after it.
557+
- 2026-07-31 DECIDED (W6): migrations run in `instrumentation.ts`, not in the
558+
manager. Startup migrates, the manager schedules. It also keeps `migrate.ts`
559+
out of the SSE route's import graph, which matters because Turbopack reads
560+
`new URL("../../../drizzle", import.meta.url)` as a module it must resolve
561+
and fails the build over a directory that is only ever read at run time. The
562+
path is now assembled with join for the same reason.
563+
- 2026-07-31 DECIDED (W6): every import in `instrumentation.ts` is inside
564+
`register()` and behind the runtime check. Next builds that file for its edge
565+
runtime too, where none of it can load, and a top-level import of anything
566+
reaching node:path fails the production build.
567+
- 2026-07-31 DECIDED (W6): the SSE stream takes a narrow watcher, not the
568+
manager. It reads a picture and listens for changes; nothing reachable from a
569+
route handler should be able to start or cancel a review.

docs/plans/M2-FINISH-PLAN.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -400,7 +400,7 @@ maintainer at the top of the demo output directory:
400400
| W3 | checkpointing runner and its tests | W1, W2 | DONE |
401401
| W4 | review service, artifact lifecycle, its tests | W1, W2, W3 | DONE |
402402
| W5 | pause/resume/cancel determinism tests | W4 | DONE |
403-
| W6 | bus, manager, instrumentation, SSE route, tests | W4 | |
403+
| W6 | bus, manager, instrumentation, SSE route, tests | W4 | DONE |
404404
| W7 | demo script, tsx alias proof, npm script | W2, W6 | |
405405
| W8 | docs, gate evidence, FG-2 checklist | W7 | |
406406

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
/**
2+
* Live progress for one review.
3+
*
4+
* The Node runtime is required, not preferred: the manager holds an open
5+
* SQLite handle and spawns processes, neither of which the edge runtime can do.
6+
*/
7+
8+
import { jobManager } from "@/server/jobs/manager";
9+
import { reviewEventStream } from "@/server/jobs/stream";
10+
11+
export const runtime = "nodejs";
12+
export const dynamic = "force-dynamic";
13+
14+
export async function GET(
15+
request: Request,
16+
context: { params: Promise<{ id: string }> },
17+
): Promise<Response> {
18+
const { id } = await context.params;
19+
return reviewEventStream(jobManager(), id, request);
20+
}

src/instrumentation.ts

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,29 @@
1+
/**
2+
* What runs once, when the server starts.
3+
*
4+
* Next calls `register()` a single time per server process, which is the only
5+
* hook that fits work that must not run per request: opening the database,
6+
* migrating it, and recovering reviews that a previous process left marked as
7+
* running. A review in that state cannot be running, because nothing survived
8+
* the restart that could be running it, and until it is recovered it can
9+
* neither be started nor cancelled.
10+
*
11+
* Every import is inside the function and behind the runtime check. Next builds
12+
* this file for its edge runtime as well, where none of it can load, and a
13+
* top-level import of anything touching node:path fails that build.
14+
*/
15+
16+
export async function register(): Promise<void> {
17+
if (process.env.NEXT_RUNTIME !== "nodejs") return;
18+
19+
const { homedir } = await import("node:os");
20+
const { dbPath, resolveDataDir } = await import("@/lib/paths");
21+
const { createDb } = await import("@/server/db/client");
22+
const { runMigrations } = await import("@/server/db/migrate");
23+
const { jobManager } = await import("@/server/jobs/manager");
24+
25+
const dataDir = resolveDataDir(process.env, homedir());
26+
const db = createDb(dbPath(dataDir));
27+
runMigrations(db);
28+
jobManager().init({ db, dataDir });
29+
}

src/server/db/migrate.ts

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,11 +6,21 @@
66
* anyone running a generator first.
77
*/
88

9+
import { dirname, join } from "node:path";
910
import { fileURLToPath } from "node:url";
1011
import { migrate } from "drizzle-orm/better-sqlite3/migrator";
1112
import type { Db } from "./client";
1213

13-
const MIGRATIONS_FOLDER = fileURLToPath(new URL("../../../drizzle", import.meta.url));
14+
// Assembled from parts rather than written as one URL literal, because a
15+
// bundler reads `new URL("../../../drizzle", import.meta.url)` as a module it
16+
// should resolve and fails the build. This is a directory read at run time.
17+
const MIGRATIONS_FOLDER = join(
18+
dirname(fileURLToPath(import.meta.url)),
19+
"..",
20+
"..",
21+
"..",
22+
"drizzle",
23+
);
1424

1525
export function runMigrations(db: Db, migrationsFolder: string = MIGRATIONS_FOLDER): void {
1626
migrate(db, { migrationsFolder });

src/server/jobs/bus.ts

Lines changed: 91 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,91 @@
1+
/**
2+
* What a running review tells anyone watching it.
3+
*
4+
* The bus carries no state a restart would need. Every durable fact, the
5+
* status, the stage rows, the run notes, the usage, is written to the database
6+
* by the service and the checkpointing runner before it is announced here, so
7+
* a listener that joins late or reconnects after a restart can read the whole
8+
* truth from the snapshot and lose nothing. That ordering is the point: an
9+
* event that arrived before the row it describes would let the UI show a stage
10+
* as finished while the database still says it is running, and reloading the
11+
* page would appear to undo it.
12+
*
13+
* Listeners are untrusted with respect to each other. One that throws must not
14+
* stop the others from being told, and must not fail the review it is watching.
15+
*/
16+
17+
import type { ReviewStage, StageErrorClass } from "@/lib/domain/enums";
18+
import type { ReviewStatus } from "@/lib/domain/state-machines";
19+
import type { RunNote } from "../db/repositories/reviews";
20+
21+
export interface StageUsage {
22+
inputTokens: number;
23+
outputTokens: number;
24+
costEquivalentUsd: number;
25+
}
26+
27+
export type ReviewEvent =
28+
| { kind: "status"; status: ReviewStatus; pausedReason?: string | undefined }
29+
| {
30+
kind: "stage";
31+
stage: ReviewStage;
32+
/** `replayed` costs nothing and spawns nothing; it is not a live call. */
33+
phase: "started" | "replayed" | "finished" | "failed";
34+
attempt?: number | undefined;
35+
usage?: StageUsage | undefined;
36+
errorClass?: StageErrorClass | undefined;
37+
}
38+
| { kind: "engine"; stage: ReviewStage; event: "tool-use" | "text"; detail: string }
39+
| { kind: "rate-limit"; status: string; resetsAt?: number | undefined }
40+
| { kind: "note"; note: RunNote }
41+
| {
42+
kind: "done";
43+
outcome: "completed" | "paused" | "cancelled" | "failed";
44+
reason?: string | undefined;
45+
};
46+
47+
export type ReviewListener = (event: ReviewEvent) => void;
48+
49+
export interface ReviewBus {
50+
emit: (reviewId: string, event: ReviewEvent) => void;
51+
subscribe: (reviewId: string, listener: ReviewListener) => () => void;
52+
/** How many listeners a review has, so a test can prove unsubscribe worked. */
53+
listenerCount: (reviewId: string) => number;
54+
}
55+
56+
export function createReviewBus(onListenerError: (error: unknown) => void = () => {}): ReviewBus {
57+
const listeners = new Map<string, Set<ReviewListener>>();
58+
59+
return {
60+
emit(reviewId, event) {
61+
// Copied before iterating: a listener may unsubscribe itself in response
62+
// to a done event, and mutating the set mid-iteration would skip whoever
63+
// came after it.
64+
for (const listener of [...(listeners.get(reviewId) ?? [])]) {
65+
try {
66+
listener(event);
67+
} catch (error) {
68+
// A browser that disconnected mid-write must not fail the review.
69+
onListenerError(error);
70+
}
71+
}
72+
},
73+
74+
subscribe(reviewId, listener) {
75+
const forReview = listeners.get(reviewId) ?? new Set<ReviewListener>();
76+
forReview.add(listener);
77+
listeners.set(reviewId, forReview);
78+
79+
return () => {
80+
const current = listeners.get(reviewId);
81+
if (!current) return;
82+
current.delete(listener);
83+
if (current.size === 0) listeners.delete(reviewId);
84+
};
85+
},
86+
87+
listenerCount(reviewId) {
88+
return listeners.get(reviewId)?.size ?? 0;
89+
},
90+
};
91+
}

0 commit comments

Comments
 (0)