mirror of
https://github.com/mvschwarz/openrig.git
synced 2026-11-07 05:56:21 -10:00
fix: events cascade-delete + atomic rig.deleted event (MEDIUM)
Schema (Fix 7):
events.rig_id changed from FK with ON DELETE CASCADE to plain TEXT.
Events are append-only history — they now survive rig deletion,
matching the existing node_id treatment.
Route (Fix 6):
DELETE /api/rigs/:id now persists rig.deleted event atomically with
the rig delete inside a db.transaction(). If event insert fails,
the rig survives (rollback). notifySubscribers called after commit.
Nonexistent rig -> 204 with no event.
Invariant:
createApp() now asserts rigRepo.db === eventBus.db === sessionRegistry.db
at construction time. Mismatched handles throw immediately.
Tests:
Schema: events survive rig deletion, orphan rig_id allowed
Route: event persisted on delete, no event for nonexistent,
sabotaged events -> rig survives + no partial row
Server: mismatched db handles rejected
Addresses R1-F06, R1-F12.
Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
This commit is contained in:
1 parent
9f6c2d8433
commit
750dff9e19
6 files changed
+136
-10
No files matched your search
@@ -5,11 +5,12 @@ export const eventsSchema: Migration = {
|
||||
sql: `
|
||||
CREATE TABLE events (
|
||||
seq INTEGER PRIMARY KEY AUTOINCREMENT,
|
||||
rig_id TEXT REFERENCES rigs(id) ON DELETE CASCADE,
|
||||
-- INTENTIONALLY NOT AN FK to rigs(id).
|
||||
-- Events are an append-only history log. They must survive rig deletion
|
||||
-- so the full timeline is preserved for replay and audit.
|
||||
rig_id TEXT,
|
||||
-- INTENTIONALLY NOT AN FK to nodes(id).
|
||||
-- Events are an append-only history log. They must survive node deletion
|
||||
-- so the full timeline is preserved for replay and audit. An FK with
|
||||
-- CASCADE would destroy history; SET NULL would lose the node reference.
|
||||
-- Same rationale: events must survive node deletion.
|
||||
node_id TEXT,
|
||||
type TEXT NOT NULL,
|
||||
payload TEXT NOT NULL,
|
||||
|
||||
@@ -1,6 +1,7 @@
|
||||
import { Hono } from "hono";
|
||||
import type { RigRepository } from "../domain/rig-repository.js";
|
||||
import type { SessionRegistry } from "../domain/session-registry.js";
|
||||
import type { EventBus } from "../domain/event-bus.js";
|
||||
import { projectRigToGraph } from "../domain/graph-projection.js";
|
||||
|
||||
export const rigsRoutes = new Hono();
|
||||
@@ -46,6 +47,32 @@ rigsRoutes.get("/:id/graph", (c) => {
|
||||
});
|
||||
|
||||
rigsRoutes.delete("/:id", (c) => {
|
||||
getRepo(c).deleteRig(c.req.param("id"));
|
||||
return c.body(null, 204);
|
||||
const rigId = c.req.param("id");
|
||||
const repo = getRepo(c);
|
||||
const eventBus = c.get("eventBus" as never) as EventBus;
|
||||
|
||||
// Only emit event + delete if rig exists
|
||||
const rig = repo.getRig(rigId);
|
||||
if (!rig) {
|
||||
return c.body(null, 204);
|
||||
}
|
||||
|
||||
// Atomic: event persist + rig delete in one transaction
|
||||
// Uses eventBus.db (same handle as rigRepo.db — enforced by shared AppDeps)
|
||||
const txn = eventBus.db.transaction(() => {
|
||||
const persisted = eventBus.persistWithinTransaction({
|
||||
type: "rig.deleted",
|
||||
rigId,
|
||||
});
|
||||
repo.deleteRig(rigId);
|
||||
return persisted;
|
||||
});
|
||||
|
||||
try {
|
||||
const persistedEvent = txn();
|
||||
eventBus.notifySubscribers(persistedEvent);
|
||||
return c.body(null, 204);
|
||||
} catch (err) {
|
||||
return c.json({ error: "delete failed" }, 500);
|
||||
}
|
||||
});
|
||||
@@ -20,6 +20,15 @@ export interface AppDeps {
|
||||
}
|
||||
|
||||
export function createApp(deps: AppDeps): Hono {
|
||||
// Hard runtime invariant: all domain services must share the same db handle.
|
||||
// Routes that use atomic transactions across services depend on this.
|
||||
if (deps.rigRepo.db !== deps.eventBus.db) {
|
||||
throw new Error("createApp: rigRepo and eventBus must share the same db handle");
|
||||
}
|
||||
if (deps.rigRepo.db !== deps.sessionRegistry.db) {
|
||||
throw new Error("createApp: rigRepo and sessionRegistry must share the same db handle");
|
||||
}
|
||||
|
||||
const app = new Hono();
|
||||
|
||||
// Inject dependencies into context for all routes
|
||||
|
||||
@@ -89,9 +89,49 @@ describe("Rig CRUD routes", () => {
|
||||
expect(repo.getRig(rig.id)).toBeNull();
|
||||
});
|
||||
|
||||
it("DELETE /api/rigs/:id with nonexistent id -> 204 (idempotent)", async () => {
|
||||
it("DELETE /api/rigs/:id -> rig.deleted event row in DB", async () => {
|
||||
const rig = repo.createRig("test-rig");
|
||||
|
||||
const res = await app.request(`/api/rigs/${rig.id}`, { method: "DELETE" });
|
||||
expect(res.status).toBe(204);
|
||||
|
||||
const events = db
|
||||
.prepare("SELECT type, payload FROM events WHERE type = 'rig.deleted'")
|
||||
.all() as { type: string; payload: string }[];
|
||||
expect(events).toHaveLength(1);
|
||||
const payload = JSON.parse(events[0]!.payload);
|
||||
expect(payload.rigId).toBe(rig.id);
|
||||
});
|
||||
|
||||
it("DELETE /api/rigs/:id with nonexistent id -> 204 + no rig.deleted event", async () => {
|
||||
const res = await app.request("/api/rigs/nonexistent", { method: "DELETE" });
|
||||
expect(res.status).toBe(204);
|
||||
|
||||
const events = db
|
||||
.prepare("SELECT * FROM events WHERE type = 'rig.deleted'")
|
||||
.all();
|
||||
expect(events).toHaveLength(0);
|
||||
});
|
||||
|
||||
it("DELETE /api/rigs/:id with sabotaged events -> rig still exists + no event row", async () => {
|
||||
const rig = repo.createRig("test-rig");
|
||||
|
||||
// Sabotage events table so event insert fails inside the transaction
|
||||
db.exec("DROP TABLE events");
|
||||
db.exec(
|
||||
"CREATE TABLE events (seq INTEGER PRIMARY KEY AUTOINCREMENT, rig_id TEXT, node_id TEXT, type TEXT NOT NULL, payload TEXT NOT NULL, created_at TEXT NOT NULL DEFAULT (datetime('now')), CONSTRAINT force_fail CHECK(length(type) < 1))"
|
||||
);
|
||||
|
||||
const res = await app.request(`/api/rigs/${rig.id}`, { method: "DELETE" });
|
||||
// Should fail (transaction rolled back)
|
||||
expect(res.status).toBe(500);
|
||||
|
||||
// Rig still exists (rollback)
|
||||
expect(repo.getRig(rig.id)).not.toBeNull();
|
||||
|
||||
// No partial event row
|
||||
const events = db.prepare("SELECT * FROM events").all();
|
||||
expect(events).toHaveLength(0);
|
||||
});
|
||||
|
||||
it("POST /api/rigs with invalid body -> 400", async () => {
|
||||
|
||||
@@ -173,13 +173,37 @@ describe("003_events", () => {
|
||||
expect(events[1]!.type).toBe("session.status_changed");
|
||||
});
|
||||
|
||||
it("cascades on rig delete", () => {
|
||||
it("events survive rig deletion (audit trail preserved)", () => {
|
||||
db.prepare(
|
||||
"INSERT INTO events (rig_id, type, payload) VALUES (?, ?, ?)"
|
||||
).run("rig-1", "event.a", "{}");
|
||||
db.prepare(
|
||||
"INSERT INTO events (rig_id, type, payload) VALUES (?, ?, ?)"
|
||||
).run("rig-1", "event.b", "{}");
|
||||
|
||||
db.prepare("DELETE FROM rigs WHERE id = ?").run("rig-1");
|
||||
const events = db.prepare("SELECT * FROM events").all();
|
||||
expect(events).toHaveLength(0);
|
||||
|
||||
// Events must still exist with original rig_id intact
|
||||
const events = db
|
||||
.prepare("SELECT rig_id, type FROM events ORDER BY seq")
|
||||
.all() as { rig_id: string; type: string }[];
|
||||
expect(events).toHaveLength(2);
|
||||
expect(events[0]!.rig_id).toBe("rig-1");
|
||||
expect(events[1]!.rig_id).toBe("rig-1");
|
||||
});
|
||||
|
||||
it("allows event with rig_id referencing nonexistent rig (orphan refs)", () => {
|
||||
expect(() =>
|
||||
db
|
||||
.prepare(
|
||||
"INSERT INTO events (rig_id, type, payload) VALUES (?, ?, ?)"
|
||||
)
|
||||
.run("nonexistent-rig", "rig.created", "{}")
|
||||
).not.toThrow();
|
||||
|
||||
const event = db
|
||||
.prepare("SELECT rig_id FROM events WHERE rig_id = ?")
|
||||
.get("nonexistent-rig") as { rig_id: string };
|
||||
expect(event.rig_id).toBe("nonexistent-rig");
|
||||
});
|
||||
});
|
||||
@@ -1,5 +1,11 @@
|
||||
import { describe, it, expect } from "vitest";
|
||||
import { createFullTestDb, createTestApp } from "./helpers/test-app.js";
|
||||
import { RigRepository } from "../src/domain/rig-repository.js";
|
||||
import { SessionRegistry } from "../src/domain/session-registry.js";
|
||||
import { EventBus } from "../src/domain/event-bus.js";
|
||||
import { NodeLauncher } from "../src/domain/node-launcher.js";
|
||||
import { createApp } from "../src/server.js";
|
||||
import { mockTmuxAdapter, unavailableCmuxAdapter } from "./helpers/test-app.js";
|
||||
|
||||
describe("Hono server (production app)", () => {
|
||||
it("GET /healthz returns 200 with status ok", async () => {
|
||||
@@ -29,4 +35,23 @@ describe("Hono server (production app)", () => {
|
||||
expect(Array.isArray(body)).toBe(true);
|
||||
db.close();
|
||||
});
|
||||
|
||||
it("createApp throws if rigRepo and eventBus use different db handles", () => {
|
||||
const db1 = createFullTestDb();
|
||||
const db2 = createFullTestDb();
|
||||
|
||||
const rigRepo = new RigRepository(db1);
|
||||
const sessionRegistry = new SessionRegistry(db1);
|
||||
const eventBus = new EventBus(db2); // different handle
|
||||
const tmux = mockTmuxAdapter();
|
||||
const cmux = unavailableCmuxAdapter();
|
||||
const nodeLauncher = new NodeLauncher({ db: db1, rigRepo, sessionRegistry, eventBus: new EventBus(db1), tmuxAdapter: tmux });
|
||||
|
||||
expect(() =>
|
||||
createApp({ rigRepo, sessionRegistry, eventBus, nodeLauncher, tmuxAdapter: tmux, cmuxAdapter: cmux })
|
||||
).toThrow(/same db handle/);
|
||||
|
||||
db1.close();
|
||||
db2.close();
|
||||
});
|
||||
});
|
||||
Reference in new issue
Block a user