From 079fc02b7b77b7f2224196b6395d3dce31a5779b Mon Sep 17 00:00:00 2001 From: Makisuo Date: Fri, 6 Nov 2026 18:11:43 +0100 Subject: [PATCH 01/11] feat(backend): shared guarded status transition helper transitionRow runs a caller-built UPDATE guarded on the states the writer read and fails with TransitionConflictError when the row already moved. --- .../src/platform/transition-row.test.ts | 89 +++++++++++++++++++ .../backend/src/platform/transition-row.ts | 54 +++++++++++ 2 files changed, 143 insertions(+) create mode 100644 packages/backend/src/platform/transition-row.test.ts create mode 100644 packages/backend/src/platform/transition-row.ts diff --git a/packages/backend/src/platform/transition-row.test.ts b/packages/backend/src/platform/transition-row.test.ts new file mode 100644 index 0000000000..67076deca6 --- /dev/null +++ b/packages/backend/src/platform/transition-row.test.ts @@ -0,0 +1,89 @@ +import { afterEach, assert, describe, it } from "@effect/vitest" +import * as PG from "@maple-dev/effect-orm/postgres" +import { Effect, Schema } from "effect" +import { Investigations } from "@maple/db/tables" +import { + InvestigationFreeformSubject, + InvestigationId, + type InvestigationStatus, + OrgId, +} from "@maple/domain/http" +import { Database } from "./DatabaseLive" +import { cleanupTestDbs, createTestDb, type TestDb } from "./test-pglite" +import { transitionRow } from "./transition-row" + +const createdDbs: TestDb[] = [] +afterEach(() => cleanupTestDbs(createdDbs)) + +const ORG = Schema.decodeUnknownSync(OrgId)("org_transition_row_test") +const ID = Schema.decodeUnknownSync(InvestigationId)("00000000-0000-4000-8000-000000000001") +const NOW = Date.UTC(2026, 10, 6, 12, 0) + +const seed = Effect.gen(function* () { + const database = yield* Database + yield* database.execute((db) => + db.run( + PG.insertInto(Investigations).values({ + id: ID, + orgId: ORG, + status: "investigating", + seededBy: "system", + subjectJson: new InvestigationFreeformSubject({ + type: "freeform", + title: "seed", + prompt: "seed", + contextRefs: [], + }), + createdAt: NOW, + updatedAt: NOW, + }), + ), + ) +}) + +const move = (from: InvestigationStatus, to: InvestigationStatus) => + Effect.gen(function* () { + const database = yield* Database + return yield* database.execute((db) => + transitionRow({ table: "investigations", id: ID, from: [from], to }, (expected) => + db.run( + PG.update(Investigations) + .set({ status: to, updatedAt: NOW }) + .where(($) => [$.orgId.eq(ORG), $.id.eq(ID), $.status.in_(...expected)]) + .returning("id", "status"), + ), + ), + ) + }) + +const statusNow = Effect.gen(function* () { + const database = yield* Database + const rows = yield* database.execute((db) => + db.run(PG.from(Investigations).select("status").where(($) => [$.orgId.eq(ORG), $.id.eq(ID)])), + ) + return rows[0]?.status +}) + +describe("transitionRow", () => { + it.effect("returns the transitioned row when the state still matches", () => + Effect.gen(function* () { + yield* seed + const row = yield* move("investigating", "diagnosed") + assert.strictEqual(row.status, "diagnosed") + }).pipe(Effect.provide(createTestDb(createdDbs).layer)), + ) + + it.effect("fails the writer that read a state another writer already moved", () => + Effect.gen(function* () { + yield* seed + // Both writers read `investigating`; the first commits, the second must not overwrite it. + yield* move("investigating", "failed") + const conflict = yield* Effect.flip(move("investigating", "diagnosed")) + + assert.strictEqual(conflict._tag, "@maple/backend/TransitionConflictError") + assert.deepStrictEqual(conflict.expected, ["investigating"]) + assert.strictEqual(conflict.target, "diagnosed") + assert.strictEqual(yield* statusNow, "failed") + }).pipe(Effect.provide(createTestDb(createdDbs).layer)), + ) +}) diff --git a/packages/backend/src/platform/transition-row.ts b/packages/backend/src/platform/transition-row.ts new file mode 100644 index 0000000000..832e77c618 --- /dev/null +++ b/packages/backend/src/platform/transition-row.ts @@ -0,0 +1,54 @@ +/** + * Guarded status transitions for the implicit state machines in Postgres. + * + * A writer that read a row's state earlier and then wrote `UPDATE ... WHERE id = ?` + * silently overwrote any transition that landed in between. The fix is to guard + * the UPDATE on the state the writer read and treat "no row came back" as a lost race. + */ +import { Array as Arr, Effect, Schema } from "effect" + +export class TransitionConflictError extends Schema.TaggedError()( + "@maple/backend/TransitionConflictError", + { + message: Schema.String, + table: Schema.String, + id: Schema.String, + expected: Schema.Array(Schema.String), + target: Schema.String, + }, +) {} + +export interface TransitionRowOptions { + /** Table name, for the error and logs. */ + readonly table: string + readonly id: string + /** The states the writer read; the UPDATE only applies while the row is still in one of them. */ + readonly from: Arr.NonEmptyReadonlyArray + readonly to: S +} + +/** + * Run a guarded transition. `write` receives the expected states and must put them in its + * `WHERE IN (...)` alongside the org and id, with a RETURNING clause: the returned row + * is the transitioned one, and an empty result fails with {@link TransitionConflictError}. + * + * Callers either remap the conflict (an API caller's 409) or catch it as a no-op where losing + * is fine (a sweep, a late writer whose result no longer applies). + */ +export const transitionRow = ( + options: TransitionRowOptions, + write: (from: Arr.NonEmptyReadonlyArray) => Effect.Effect, E, R>, +): Effect.Effect => + Effect.flatMap(write(options.from), (rows) => { + const [row] = rows + if (row !== undefined) return Effect.succeed(row) + return Effect.fail( + new TransitionConflictError({ + message: `${options.table} '${options.id}' is no longer in ${Arr.join(options.from, " or ")}; it did not move to '${options.to}'.`, + table: options.table, + id: options.id, + expected: options.from, + target: options.to, + }), + ) + }) From 8109576fad12670359d95e29cf6bcc42e948df46 Mon Sep 17 00:00:00 2001 From: Makisuo Date: Fri, 6 Nov 2026 18:16:17 +0100 Subject: [PATCH 02/11] fix(backend): guard error issue and investigation status transitions Writers updated status columns by id alone, so one that read state earlier silently overwrote a concurrent transition. Every writer now guards its UPDATE on the state it decided from, through transitionRow. Error issues: applyTransition returns a 409 when it loses the race; the tick only regresses rows still done; alert and integration reopens land in regressed with the tick's bookkeeping. Investigations: late diagnoses on failed or resolved runs are dropped; manual status changes may only resolve a finished run or reopen it to its own verdict (MANUAL_INVESTIGATION_TRANSITIONS, 409 otherwise); restart refuses a running pass; one heartbeat-aware stale sweep; anomaly triage settles only once. --- docs/error-issue-lifecycle.md | 7 + .../src/platform/transition-row.test.ts | 10 +- .../errors/ErrorIssueWorkflowService.test.ts | 37 ++++ .../errors/ErrorIssueWorkflowService.ts | 31 +++- .../src/services/errors/ErrorsService.ts | 4 +- .../errors/InvestigationService.test.ts | 107 +++++++++++- .../services/errors/InvestigationService.ts | 163 ++++++++++++------ .../src/services/errors/apply-diagnosis.ts | 158 +++++++++++------ .../services/errors/error-tick-persistence.ts | 83 +++++---- .../error-tick-regression-guard.test.ts | 69 ++++++++ .../errors/investigation-stale.test.ts | 5 +- .../services/errors/investigation-stale.ts | 62 ++++--- .../src/services/errors/issue-hub.test.ts | 7 +- .../backend/src/services/errors/issue-hub.ts | 36 ++-- .../src/services/errors/issue-reopen.ts | 69 ++++++++ .../planetscale/webhook-events.test.ts | 4 +- .../planetscale/webhook-events.ts | 38 ++-- .../anticipated-error-identifiers.ts | 1 + packages/domain/src/http/investigations.ts | 49 ++++++ packages/domain/src/http/v2/investigations.ts | 19 +- 20 files changed, 737 insertions(+), 222 deletions(-) create mode 100644 packages/backend/src/services/errors/error-tick-regression-guard.test.ts create mode 100644 packages/backend/src/services/errors/issue-reopen.ts diff --git a/docs/error-issue-lifecycle.md b/docs/error-issue-lifecycle.md index c75401079c..3437fb6d5a 100644 --- a/docs/error-issue-lifecycle.md +++ b/docs/error-issue-lifecycle.md @@ -194,6 +194,13 @@ loop in verification without a human ever seeing it. ## Rules worth not re-deriving +- **Every state write is guarded on the state it was decided from.** `applyTransition`, the + errors tick's regression and the alert/integration reopen all go through `transitionRow` + (`packages/backend/src/platform/transition-row.ts`): the UPDATE carries + `workflow_state IN ()`, so a concurrent move wins and the late writer gets a 409 + (or, for a tick, skips the row). A fixed alert or integration issue that fires again lands in + `regressed` too, never `triage`. + - **Never make the claim a step an agent has to remember.** Any new tool that starts work on an issue takes the lease itself. See above for what happened when it did not. - **An issue with a linked PR should not be closed by hand.** The merge opens the verification diff --git a/packages/backend/src/platform/transition-row.test.ts b/packages/backend/src/platform/transition-row.test.ts index 67076deca6..eae4ab29fc 100644 --- a/packages/backend/src/platform/transition-row.test.ts +++ b/packages/backend/src/platform/transition-row.test.ts @@ -15,8 +15,8 @@ import { transitionRow } from "./transition-row" const createdDbs: TestDb[] = [] afterEach(() => cleanupTestDbs(createdDbs)) -const ORG = Schema.decodeUnknownSync(OrgId)("org_transition_row_test") -const ID = Schema.decodeUnknownSync(InvestigationId)("00000000-0000-4000-8000-000000000001") +const ORG = Schema.decodeSync(OrgId)("org_transition_row_test") +const ID = Schema.decodeSync(InvestigationId)("00000000-0000-4000-8000-000000000001") const NOW = Date.UTC(2026, 10, 6, 12, 0) const seed = Effect.gen(function* () { @@ -59,7 +59,11 @@ const move = (from: InvestigationStatus, to: InvestigationStatus) => const statusNow = Effect.gen(function* () { const database = yield* Database const rows = yield* database.execute((db) => - db.run(PG.from(Investigations).select("status").where(($) => [$.orgId.eq(ORG), $.id.eq(ID)])), + db.run( + PG.from(Investigations) + .select("status") + .where(($) => [$.orgId.eq(ORG), $.id.eq(ID)]), + ), ) return rows[0]?.status }) diff --git a/packages/backend/src/services/errors/ErrorIssueWorkflowService.test.ts b/packages/backend/src/services/errors/ErrorIssueWorkflowService.test.ts index e332bb0b57..c36e305643 100644 --- a/packages/backend/src/services/errors/ErrorIssueWorkflowService.test.ts +++ b/packages/backend/src/services/errors/ErrorIssueWorkflowService.test.ts @@ -413,6 +413,43 @@ describe("ErrorIssueWorkflowService", () => { }).pipe(Effect.provide(makeLayer())), ) + it.effect("gives the conflict to the second of two writers that read the same state", () => + Effect.gen(function* () { + const workflow = yield* ErrorIssueWorkflowService + const database = yield* Database + const issueId = asIssueId(randomUUID()) + yield* seedIssue(issueId, { workflowState: "triage" }) + // Both callers read `triage` before either wrote. + const seenByBoth = yield* workflow.requireIssue(ORG, issueId) + + yield* workflow.applyTransition(ORG, null, seenByBoth, "cancelled") + const loser = yield* Effect.flip(workflow.applyTransition(ORG, null, seenByBoth, "done")) + assert.strictEqual(loser._tag, "@maple/http/errors/ErrorIssueTransitionError") + + const [after] = yield* database.execute((db) => + db.run( + PG.from(ErrorIssues) + .select() + .where(($) => [$.id.eq(issueId)]), + ), + ) + assert.strictEqual(after?.workflowState, "cancelled") + assert.isNull(after?.resolvedAt) + // The loser's timeline event rolled back with its UPDATE. + const events = yield* database.execute((db) => + db.run( + PG.from(ErrorIssueEvents) + .select() + .where(($) => [$.issueId.eq(issueId)]), + ), + ) + assert.deepStrictEqual( + Arr.map(events, (event) => event.toState), + ["cancelled"], + ) + }).pipe(Effect.provide(makeLayer())), + ) + // The lease used to advance only via an explicit `heartbeat` call, which agents // never made: `claim` was used 11 times in 8 days, `heartbeat` zero. Issues were // claimed, worked, and silently dropped back to `todo` when the lease lapsed. diff --git a/packages/backend/src/services/errors/ErrorIssueWorkflowService.ts b/packages/backend/src/services/errors/ErrorIssueWorkflowService.ts index dac8fb2a98..84c61a1e85 100644 --- a/packages/backend/src/services/errors/ErrorIssueWorkflowService.ts +++ b/packages/backend/src/services/errors/ErrorIssueWorkflowService.ts @@ -42,6 +42,7 @@ import { AuditLogService } from "@maple/backend/services/audit/AuditLogService" import { CurrentAuditActor } from "@maple/backend/services/auth/audit-actor" import { SYSTEM_ERRORS_AGENT_NAME } from "@maple/backend/services/auth/system-actors" import { currentTxid, readTxid } from "@maple/backend/platform/electric-txid" +import { transitionRow } from "@maple/backend/platform/transition-row" import { ErrorActorsService } from "./ErrorActorsService" import { makeErrorDatabaseExecute } from "./error-persistence" import { issueFingerprintHashes } from "./issue-fingerprints" @@ -627,13 +628,24 @@ const make: Effect.Effect< // One transaction, because a `done` that committed the issue but lost the // incident resolution or its timeline event could never be repaired: a // retry sees `fromState === toState` and returns before reaching them. + // Guarded on the state that was validated: a concurrent transition that landed first + // makes this one a 409 instead of a silent overwrite, and rolls the transaction back. yield* dbExecute((db) => db.transaction( Effect.gen(function* () { - yield* db.run( - PG.update(ErrorIssues) - .set(update) - .where(($) => [$.orgId.eq(orgId), $.id.eq(row.id)]), + yield* transitionRow( + { table: "error_issues", id: row.id, from: [fromState], to: toState }, + (from) => + db.run( + PG.update(ErrorIssues) + .set(update) + .where(($) => [ + $.orgId.eq(orgId), + $.id.eq(row.id), + $.workflowState.in_(...from), + ]) + .returning("id"), + ), ) if (toState === "done") { yield* db.run( @@ -650,6 +662,17 @@ const make: Effect.Effect< yield* db.run(PG.insertInto(ErrorIssueEvents).values(eventInsert)) }), ), + ).pipe( + Effect.catchTag("@maple/backend/TransitionConflictError", () => + Effect.fail( + new ErrorIssueTransitionError({ + message: `The issue moved out of '${fromState}' while this change to '${toState}' was in flight. Re-read it and retry.`, + issueId: row.id, + fromState, + toState, + }), + ), + ), ) if (actorId) { yield* recordEventAudit(orgId, row.id, actorId, "state_change", { fromState, toState }) diff --git a/packages/backend/src/services/errors/ErrorsService.ts b/packages/backend/src/services/errors/ErrorsService.ts index e4d49fac47..32549735da 100644 --- a/packages/backend/src/services/errors/ErrorsService.ts +++ b/packages/backend/src/services/errors/ErrorsService.ts @@ -1946,7 +1946,9 @@ const make: Effect.Effect< // One statement for every org, outside the per-org loop: an abandoned run is // rare and the matching set is almost always empty, so paying a round-trip per // org to discover that would cost more than the sweep saves. - const investigationsAbandoned = yield* dbExecute((db) => sweepAbandonedInvestigations(db, nowMs)) + const investigationsAbandoned = yield* dbExecute((db) => + sweepAbandonedInvestigations(db, nowMs), + ).pipe(Effect.map((rows) => rows.length)) if (investigationsAbandoned > 0) { yield* Effect.logWarning("Failed investigations whose agent pass never returned").pipe( Effect.annotateLogs({ count: investigationsAbandoned, budgetMs: STALE_MS }), diff --git a/packages/backend/src/services/errors/InvestigationService.test.ts b/packages/backend/src/services/errors/InvestigationService.test.ts index a122775d1a..a358ebc5a0 100644 --- a/packages/backend/src/services/errors/InvestigationService.test.ts +++ b/packages/backend/src/services/errors/InvestigationService.test.ts @@ -1,6 +1,7 @@ import { randomUUID } from "node:crypto" import { afterEach, assert, describe, it } from "@effect/vitest" import { Cause, ConfigProvider, Effect, Exit, Layer, Schema, Array as Arr, Clock } from "effect" +import { TestClock } from "effect/testing" import { AiTriageEvidence, AiTriageResult, @@ -13,6 +14,8 @@ import { InvestigationSubjectSnapshot, InvestigationAgentUnavailableError, InvestigationStartFailedError, + InvestigationTransitionError, + type InvestigationProgress, OrgId, SubmitDiagnosisRequest, } from "@maple/domain/http" @@ -33,6 +36,7 @@ import { Env } from "@maple/backend/platform/Env" import { Database } from "@maple/backend/platform/DatabaseLive" import { cleanupTestDbs, createTestDb, type TestDb } from "@maple/backend/platform/test-pglite" import { InvestigationService } from "./InvestigationService" +import { STALE_MS } from "./investigation-stale" const createdDbs: TestDb[] = [] afterEach(() => cleanupTestDbs(createdDbs)) @@ -294,6 +298,11 @@ describe("InvestigationService", () => { const service = yield* InvestigationService const created = yield* service.createInvestigation(ORG, null, freeformRequest("status flow")) assert.strictEqual(created.status, "investigating") + yield* service.submitDiagnosis( + ORG, + created.id, + new SubmitDiagnosisRequest({ report: sampleReport() }), + ) const updated = yield* service.updateStatus(ORG, created.id, "resolved") assert.strictEqual(updated.status, "resolved") @@ -318,6 +327,95 @@ describe("InvestigationService", () => { }).pipe(Effect.provide(makeLayer())), ) + it.effect("updateStatus refuses moves a person may not make", () => + Effect.gen(function* () { + const service = yield* InvestigationService + const running = yield* service.createInvestigation(ORG, null, freeformRequest("still running")) + const refused = yield* Effect.flip(service.updateStatus(ORG, running.id, "resolved")) + assert.instanceOf(refused, InvestigationTransitionError) + assert.strictEqual((yield* service.getInvestigation(ORG, running.id)).status, "investigating") + + const diagnosed = yield* service.createInvestigation(ORG, null, freeformRequest("diagnosed")) + yield* service.submitDiagnosis( + ORG, + diagnosed.id, + new SubmitDiagnosisRequest({ report: sampleReport() }), + ) + const relabel = yield* Effect.flip(service.updateStatus(ORG, diagnosed.id, "failed")) + assert.instanceOf(relabel, InvestigationTransitionError) + + // Reopening lands back on the run's own verdict, and only there. + yield* service.updateStatus(ORG, diagnosed.id, "resolved") + const wrongReopen = yield* Effect.flip(service.updateStatus(ORG, diagnosed.id, "inconclusive")) + assert.instanceOf(wrongReopen, InvestigationTransitionError) + const reopened = yield* service.updateStatus(ORG, diagnosed.id, "diagnosed") + assert.strictEqual(reopened.status, "diagnosed") + }).pipe(Effect.provide(makeLayer())), + ) + + it.effect("drops a diagnosis that arrives after the run failed", () => + Effect.gen(function* () { + const service = yield* InvestigationService + const created = yield* service.createInvestigation(ORG, null, freeformRequest("late report")) + yield* service.failInvestigation(ORG, created.id, "diagnosis_timeout: swept") + + const doc = yield* service.submitDiagnosis( + ORG, + created.id, + new SubmitDiagnosisRequest({ report: sampleReport() }), + ) + assert.strictEqual(doc.status, "failed") + assert.isNull(doc.report) + const partial = yield* service.submitDiagnosis( + ORG, + created.id, + new SubmitDiagnosisRequest({ report: sampleReport(), partial: true }), + ) + assert.strictEqual(partial.status, "failed") + }).pipe(Effect.provide(makeLayer())), + ) + + it.effect("refuses to restart a run that is still in flight", () => { + const chat = chatSessionHarness() + const harness = makeHarness(chat.env, chat.chatSessions) + return Effect.gen(function* () { + const service = yield* InvestigationService + const started = yield* service.createAndStartInvestigation( + ORG, + null, + freeformRequest("in flight"), + ) + const refused = yield* Effect.flip(service.restartInvestigation(ORG, started.id)) + assert.instanceOf(refused, InvestigationTransitionError) + assert.lengthOf(chat.beginTurns, 1) + }).pipe(Effect.provide(harness.layer)) + }) + + it.effect("the read-path sweep fails an abandoned run but spares one still recording steps", () => + Effect.gen(function* () { + const now = Date.UTC(2026, 10, 6, 12, 0) + yield* TestClock.setTime(now) + const database = yield* Database + const service = yield* InvestigationService + const alive = yield* service.createInvestigation(ORG, null, freeformRequest("long close-out")) + const dead = yield* service.createInvestigation(ORG, null, freeformRequest("abandoned")) + const age = (id: InvestigationId, progressJson: InvestigationProgress | null) => + database.execute((db) => + db.run( + PG.update(Investigations) + .set({ startedAt: now - STALE_MS - 60_000, progressJson }) + .where(($) => [$.id.eq(id)]), + ), + ) + yield* age(alive.id, { stepCount: 4, steps: [], updatedAt: now - 8_000 }) + yield* age(dead.id, null) + + // Reading the abandoned run sweeps the org; the old sweep ignored the heartbeat. + assert.strictEqual((yield* service.getInvestigation(ORG, dead.id)).status, "failed") + assert.strictEqual((yield* service.getInvestigation(ORG, alive.id)).status, "investigating") + }).pipe(Effect.provide(makeLayer())), + ) + /** * Stub `ChatSession` namespace. The autonomous turn now claims the turn on the ChatSession * Durable Object in process instead of POSTing back to chat-flue over a service binding, so the @@ -460,6 +558,7 @@ describe("InvestigationService", () => { criticalIncidentRequest("err_over_budget"), ) assert.strictEqual(started.status, "investigating") + yield* service.failInvestigation(ORG, started.id, "no_diagnosis: the pass ended in prose") const restarted = yield* service.restartInvestigation(ORG, started.id) assert.strictEqual(restarted.status, "investigating") @@ -567,18 +666,20 @@ describe("InvestigationService", () => { const failed = yield* triage(FAILED_ID) yield* service.failInvestigation(ORG, failed.id, "no_diagnosis: the pass ended in prose") const manual = yield* triage(MANUAL_ID) + yield* service.failInvestigation(ORG, manual.id, "no_diagnosis: the pass ended in prose") + // Resolving by hand afterwards does not rewrite a triage that already settled. yield* service.updateStatus(ORG, manual.id, "resolved") - assert.deepStrictEqual(yield* statuses, ["completed", "skipped", "completed"]) + assert.deepStrictEqual(yield* statuses, ["completed", "skipped", "skipped"]) // A retry is triaging again, and its diagnosis completes it. yield* service.restartInvestigation(ORG, failed.id) - assert.deepStrictEqual(yield* statuses, ["completed", "pending", "completed"]) + assert.deepStrictEqual(yield* statuses, ["completed", "pending", "skipped"]) yield* service.submitDiagnosis( ORG, failed.id, new SubmitDiagnosisRequest({ report: sampleReport() }), ) - assert.deepStrictEqual(yield* statuses, ["completed", "completed", "completed"]) + assert.deepStrictEqual(yield* statuses, ["completed", "completed", "skipped"]) }).pipe(Effect.provide(harness.layer)) }) diff --git a/packages/backend/src/services/errors/InvestigationService.ts b/packages/backend/src/services/errors/InvestigationService.ts index b927f57645..3f47c78182 100644 --- a/packages/backend/src/services/errors/InvestigationService.ts +++ b/packages/backend/src/services/errors/InvestigationService.ts @@ -10,6 +10,7 @@ import { InvestigationNotFoundError, InvestigationPersistenceError, InvestigationStartFailedError, + InvestigationTransitionError, InvestigationSnapshotFact, InvestigationSubjectSnapshot, InvestigationsListResponse, @@ -17,6 +18,9 @@ import { type InvestigationStatus, InvestigationSubject, type AnomalyTriageStatus, + MANUAL_INVESTIGATION_TRANSITIONS, + RESTARTABLE_INVESTIGATION_STATUSES, + reopenedInvestigationStatus, type OrgId, type SubmitDiagnosisRequest, type UserId, @@ -33,14 +37,14 @@ import { } from "@maple/backend/services/errors/apply-diagnosis" import { startInvestigationTurn } from "@maple/backend/services/errors/investigation-start" import { - STALE_MS, isInvestigationStale, - staleTimeoutMessage, storedProgressJson, + sweepAbandonedInvestigations, } from "@maple/backend/services/errors/investigation-stale" import { ChatSessions } from "@maple/backend/platform/bindings" import { Database } from "@maple/backend/platform/DatabaseLive" import { makeDbExecute, makePersistenceErrorMapper } from "@maple/backend/platform/db-execute" +import { transitionRow } from "@maple/backend/platform/transition-row" const decodeIdSync = Schema.decodeUnknownSync(InvestigationId) const decodeIsoSync = Schema.decodeUnknownSync(InvestigationDocument.fields.createdAt) @@ -129,6 +133,7 @@ export interface InvestigationServiceApi { InvestigationDocument, | InvestigationPersistenceError | InvestigationNotFoundError + | InvestigationTransitionError | InvestigationAgentUnavailableError | InvestigationStartFailedError | InvestigationDataCorruptionError @@ -139,7 +144,10 @@ export interface InvestigationServiceApi { status: InvestigationStatus, ) => Effect.Effect< InvestigationDocument, - InvestigationPersistenceError | InvestigationNotFoundError | InvestigationDataCorruptionError + | InvestigationPersistenceError + | InvestigationNotFoundError + | InvestigationTransitionError + | InvestigationDataCorruptionError > readonly submitDiagnosis: ( orgId: OrgId, @@ -370,32 +378,25 @@ export class InvestigationService extends Context.Service db.run( PG.update(AnomalyIncidents) .set({ triageStatus, updatedAt: nowMs }) - .where(($) => [$.orgId.eq(orgId), $.id.in_(first, ...rest)]), + .where(($) => [ + $.orgId.eq(orgId), + $.id.in_(first, ...rest), + triageStatus === "pending" + ? undefined + : $.triageStatus.in_("none", "pending"), + ]), ), ).pipe(Effect.ignore({ log: "Warn", message: "Failed to settle anomaly triage status" })) } const failStaleInvestigations = Effect.fnUntraced(function* (orgId: OrgId, nowMs: number) { - const failed = yield* dbExecute((db) => - db.run( - PG.update(Investigations) - .set({ - status: "failed", - error: staleTimeoutMessage(STALE_MS), - updatedAt: nowMs, - }) - .where(($) => [ - $.orgId.eq(orgId), - $.status.eq("investigating"), - $.startedAt.lt(nowMs - STALE_MS), - ]) - .returning("incidentKind", "incidentId"), - ), - ) + const failed = yield* dbExecute((db) => sweepAbandonedInvestigations(db, nowMs, orgId)) yield* settleAnomalyTriage(orgId, failed, "failed", nowMs) }) @@ -616,22 +617,46 @@ export class InvestigationService extends Context.Service - db.run( - PG.update(Investigations) - .set(($) => ({ - status: "investigating", - error: null, - startedAt: nowMs, - autonomousTurns: $.autonomousTurns.add(1), - updatedAt: nowMs, - })) - .where(($) => [$.orgId.eq(orgId), $.id.eq(id)]) - .returning("incidentKind", "incidentId"), + // A run in flight is not restarted underneath itself; a stale one was already + // failed by the read above, so it restarts like any other settled run. + const restarted = yield* dbExecute((db) => + transitionRow( + { + table: "investigations", + id, + from: RESTARTABLE_INVESTIGATION_STATUSES, + to: "investigating", + }, + (from) => + db.run( + PG.update(Investigations) + .set(($) => ({ + status: "investigating", + error: null, + startedAt: nowMs, + autonomousTurns: $.autonomousTurns.add(1), + updatedAt: nowMs, + })) + .where(($) => [$.orgId.eq(orgId), $.id.eq(id), $.status.in_(...from)]) + .returning("incidentKind", "incidentId"), + ), ), ).pipe( - Effect.tap((restarted) => settleAnomalyTriage(orgId, restarted, "investigating", nowMs)), + Effect.catchTag("@maple/backend/TransitionConflictError", () => + Effect.fail( + new InvestigationTransitionError({ + message: + existing.status === "investigating" + ? "This investigation is still running; wait for it to finish before restarting it." + : "The investigation changed while restarting it. Reload and retry.", + investigationId: id, + fromStatus: existing.status, + toStatus: "investigating", + }), + ), + ), ) + yield* settleAnomalyTriage(orgId, [restarted], "investigating", nowMs) const restarting = new InvestigationDocument({ ...existing, status: "investigating", @@ -651,20 +676,56 @@ export class InvestigationService extends Context.Service - db.run( - PG.update(Investigations) - .set({ status, updatedAt: nowMs }) - .where(($) => [$.orgId.eq(orgId), $.id.eq(id)]) - .returning("id", "incidentKind", "incidentId"), - ), - ) - if (updated.length === 0) { + const current = yield* loadRow(orgId, id) + if (!current) { return yield* new InvestigationNotFoundError({ message: `No such investigation: '${id}'`, }) } - yield* settleAnomalyTriage(orgId, updated, status, nowMs) + const fromStatus = current.status + if (fromStatus === status) return yield* documentFor(orgId, current) + const refuse = (message: string) => + new InvestigationTransitionError({ + message, + investigationId: id, + fromStatus, + toStatus: status, + }) + const allowed: ReadonlyArray = + MANUAL_INVESTIGATION_TRANSITIONS[fromStatus] + if (!allowed.includes(status)) { + return yield* refuse( + fromStatus === "investigating" + ? "This investigation is still running; its status settles when the run ends." + : `An investigation cannot move from '${fromStatus}' to '${status}' by hand. From '${fromStatus}' it can go to: ${Arr.join(allowed, ", ")}.`, + ) + } + if (fromStatus === "resolved") { + const reopened = reopenedInvestigationStatus({ + diagnosed: current.diagnosedAt !== null, + hasReport: current.reportJson !== null, + }) + if (reopened !== status) { + return yield* refuse( + `Reopening this investigation returns it to '${reopened}', the verdict its run reached.`, + ) + } + } + const updated = yield* dbExecute((db) => + transitionRow({ table: "investigations", id, from: [fromStatus], to: status }, (from) => + db.run( + PG.update(Investigations) + .set({ status, updatedAt: nowMs }) + .where(($) => [$.orgId.eq(orgId), $.id.eq(id), $.status.in_(...from)]) + .returning("id", "incidentKind", "incidentId"), + ), + ), + ).pipe( + Effect.catchTag("@maple/backend/TransitionConflictError", () => + Effect.fail(refuse("The investigation changed while updating it. Reload and retry.")), + ), + ) + yield* settleAnomalyTriage(orgId, [updated], status, nowMs) const row = yield* loadRow(orgId, id) if (!row) { return yield* new InvestigationNotFoundError({ @@ -726,16 +787,18 @@ export class InvestigationService extends Context.Service } +/** Whether a diagnosis or partial actually reached the row. */ +export type DiagnosisOutcome = "applied" | "dropped" + +/** + * A diagnosis lands on a run still going, or supersedes an earlier verdict (an attended + * follow-up). A run already `failed` (the stale sweep gave up on it) or `resolved` by a person + * keeps its state: the late report is dropped, not written. + */ +const DIAGNOSABLE_STATUSES = ["investigating", "diagnosed", "inconclusive"] as const + +/** A partial only ever closes out the autonomous pass that is still running. */ +const PARTIAL_STATUSES = ["investigating"] as const + +const dropLateWrite = ( + investigationId: InvestigationId, + to: InvestigationStatus, + from: Arr.NonEmptyReadonlyArray, + write: ( + from: Arr.NonEmptyReadonlyArray, + ) => Effect.Effect, MapleDbError>, +): Effect.Effect => + transitionRow({ table: "investigations", id: investigationId, from, to }, write).pipe( + Effect.as("applied"), + Effect.catchTag("@maple/backend/TransitionConflictError", (conflict) => + Effect.gen(function* () { + yield* Effect.annotateCurrentSpan({ "maple.investigation.late_write_dropped": to }) + yield* Effect.logWarning("Dropped a late investigation write").pipe( + Effect.annotateLogs({ investigationId, to, message: conflict.message }), + ) + return "dropped" as const + }), + ), + ) + /** * Flip the row to `diagnosed` with the report attached, then — if an issue is * linked — apply the severity and record the timeline event atomically. The @@ -113,29 +149,41 @@ const writeDiagnosis = (orm: MapleDb, input: ApplyDiagnosisInput) => const confidence: InvestigationConfidence = input.report.confidence const now = input.nowMs - yield* orm.run( - PG.update(Investigations) - .set({ - status: "diagnosed", - reportJson: input.report, - // Explicit null rather than `undefined`: an unassessed report must write - // "no severity" onto the row, not silently omit the column from the UPDATE - // and leave a stale one standing. - severity: input.report.severityAssessment ?? null, - confidence, - model: input.model, - inputTokens: input.inputTokens, - outputTokens: input.outputTokens, - error: null, - diagnosedAt: now, - updatedAt: now, - }) - .where(($) => [$.orgId.eq(input.orgId), $.id.eq(input.investigationId)]), + const landed = yield* dropLateWrite( + input.investigationId, + "diagnosed", + DIAGNOSABLE_STATUSES, + (from) => + orm.run( + PG.update(Investigations) + .set({ + status: "diagnosed", + reportJson: input.report, + // Explicit null rather than `undefined`: an unassessed report must write + // "no severity" onto the row, not silently omit the column from the UPDATE + // and leave a stale one standing. + severity: input.report.severityAssessment ?? null, + confidence, + model: input.model, + inputTokens: input.inputTokens, + outputTokens: input.outputTokens, + error: null, + diagnosedAt: now, + updatedAt: now, + }) + .where(($) => [ + $.orgId.eq(input.orgId), + $.id.eq(input.investigationId), + $.status.in_(...from), + ]) + .returning("id"), + ), ) + if (landed === "dropped") return landed - if (!input.issueId) return + if (!input.issueId) return landed // See `subjectType` above: a verification's report must not re-rank the issue. - if (Option.contains(input.subjectType, "fix_verification")) return + if (Option.contains(input.subjectType, "fix_verification")) return landed const decodedIssueId = decodeIssueId(input.issueId) // Keyed by the turn: a follow-up's superseding diagnosis records its own events, a retry // does not. Without a turn, one set per investigation, as before. @@ -174,6 +222,7 @@ const writeDiagnosis = (orm: MapleDb, input: ApplyDiagnosisInput) => ) }), ) + return landed }) /** @@ -195,11 +244,11 @@ const writeDiagnosis = (orm: MapleDb, input: ApplyDiagnosisInput) => */ export const applyDiagnosisWrites: ( input: ApplyDiagnosisInput, -) => Effect.Effect = Effect.fn( +) => Effect.Effect = Effect.fn( "ApplyDiagnosis.applyDiagnosisWrites", )(function* (input) { const database = yield* Database - yield* makeDbExecute(database, "applyDiagnosisWrites", identity)((db) => writeDiagnosis(db, input)) + return yield* makeDbExecute(database, "applyDiagnosisWrites", identity)((db) => writeDiagnosis(db, input)) }) export interface ApplyInconclusiveInput { @@ -236,24 +285,31 @@ export interface ApplyInconclusiveInput { * render in a destructive box. The report replaces it. */ const writeInconclusive = (orm: MapleDb, input: ApplyInconclusiveInput) => - orm.run( - PG.update(Investigations) - .set({ - status: "inconclusive", - reportJson: input.report, - severity: null, - // Always low, whatever the report says. Nothing was established, and a - // partial that claims medium confidence in a lead is a promotion nobody - // made, reintroduced one layer down. - confidence: "low", - model: input.model, - inputTokens: input.inputTokens, - outputTokens: input.outputTokens, - error: null, - diagnosedAt: null, - updatedAt: input.nowMs, - }) - .where(($) => [$.orgId.eq(input.orgId), $.id.eq(input.investigationId)]), + dropLateWrite(input.investigationId, "inconclusive", PARTIAL_STATUSES, (from) => + orm.run( + PG.update(Investigations) + .set({ + status: "inconclusive", + reportJson: input.report, + severity: null, + // Always low, whatever the report says. Nothing was established, and a + // partial that claims medium confidence in a lead is a promotion nobody + // made, reintroduced one layer down. + confidence: "low", + model: input.model, + inputTokens: input.inputTokens, + outputTokens: input.outputTokens, + error: null, + diagnosedAt: null, + updatedAt: input.nowMs, + }) + .where(($) => [ + $.orgId.eq(input.orgId), + $.id.eq(input.investigationId), + $.status.in_(...from), + ]) + .returning("id"), + ), ) /** @@ -263,13 +319,13 @@ const writeInconclusive = (orm: MapleDb, input: ApplyInconclusiveInput) => */ export const applyInconclusiveWrites: ( input: ApplyInconclusiveInput, -) => Effect.Effect = Effect.fn("ApplyDiagnosis.applyInconclusiveWrites")( - function* (input) { - const database = yield* Database - yield* makeDbExecute( - database, - "applyInconclusiveWrites", - identity, - )((db) => writeInconclusive(db, input)) - }, -) +) => Effect.Effect = Effect.fn( + "ApplyDiagnosis.applyInconclusiveWrites", +)(function* (input) { + const database = yield* Database + return yield* makeDbExecute( + database, + "applyInconclusiveWrites", + identity, + )((db) => writeInconclusive(db, input)) +}) diff --git a/packages/backend/src/services/errors/error-tick-persistence.ts b/packages/backend/src/services/errors/error-tick-persistence.ts index 6706e56b65..83858ae025 100644 --- a/packages/backend/src/services/errors/error-tick-persistence.ts +++ b/packages/backend/src/services/errors/error-tick-persistence.ts @@ -597,6 +597,44 @@ const buildNotificationRows = (input: PersistErrorTickWindowInput, payload: Noti })) } +/** + * Reopen fixed issues as `regressed`, guarded on `done`: the scan read the state with a plain + * SELECT, so a human transition committed since then (a re-triage, a `cancelled`) wins and the + * row is left alone. Returns the ids that actually reopened. + */ +export const applyRegressions = ( + tx: MapleDb, + input: Pick, + issueIds: ReadonlyArray, +): Effect.Effect, MapleDbError> => + Effect.gen(function* () { + const reopened = yield* inChunks(issueIds, (chunk) => + tx.run( + PG.update(ErrorIssues) + .set(($) => ({ + workflowState: "regressed", + resolvedAt: null, + resolvedByActorId: null, + lastRegressedAt: input.windowEndMs, + regressionCount: $.regressionCount.add(1), + // A regression on an issue retention already archived must be seen. + archivedAt: null, + updatedAt: input.windowEndMs, + })) + .where(($) => [$.orgId.eq(input.orgId), $.id.in_(...chunk), $.workflowState.eq("done")]) + .returning("id"), + ), + ) + const lost = issueIds.length - reopened.length + if (lost > 0) { + yield* Effect.annotateCurrentSpan({ "maple.error_tick.regressions_lost": lost }) + yield* Effect.logInfo("Regression lost to a concurrent transition").pipe( + Effect.annotateLogs({ orgId: input.orgId, lost }), + ) + } + return new Set(Arr.map(reopened, (row) => row.id)) + }) + /** * Apply one claimed, half-open evaluator window atomically. The durable cursor * advances only if every issue, incident, event, and notification-outbox write @@ -804,6 +842,19 @@ export const persistErrorTickWindow = ( ) }) + // Apply the reopen decided above. Reopening lands in `regressed`, not + // `triage`: an issue that was fixed and came back is not the same thing + // as one nobody has looked at, and flattening the two is what let the + // same bug be picked up and fixed again from scratch. + const regressedIds = yield* applyRegressions( + tx, + input, + Arr.map( + Arr.filter(resolvedRows, (entry) => entry.regression), + (entry) => entry.issueId, + ), + ) + for (const { row, prior, regression, suppressed, issueId } of resolvedRows) { if (!prior) { events.push( @@ -818,7 +869,9 @@ export const persistErrorTickWindow = ( ) } - const wasRegression = regression + // Only a reopen that actually applied is a regression; one that lost to a + // concurrent human transition is an ordinary occurrence on the new state. + const wasRegression = regression && regressedIds.has(issueId) if (wasRegression) { events.push( buildEvent(input, issueId, "state_change", { @@ -845,34 +898,6 @@ export const persistErrorTickWindow = ( }) } - // Apply the reopen decided above. Reopening lands in `regressed`, not - // `triage`: an issue that was fixed and came back is not the same thing - // as one nobody has looked at, and flattening the two is what let the - // same bug be picked up and fixed again from scratch. - const regressedIds = Arr.map( - Arr.filter(observed, (entry) => entry.wasRegression), - (entry) => entry.issueId, - ) - yield* Effect.forEach( - chunks(regressedIds), - (chunk) => - tx.run( - PG.update(ErrorIssues) - .set(($) => ({ - workflowState: "regressed", - resolvedAt: null, - resolvedByActorId: null, - lastRegressedAt: windowEnd, - regressionCount: $.regressionCount.add(1), - // A regression on an issue retention already archived must be seen. - archivedAt: null, - updatedAt: windowEnd, - })) - .where(($) => [$.orgId.eq(input.orgId), $.id.in_(...chunk)]), - ), - { discard: true }, - ) - // Only retention archives an issue, on the premise that it stopped occurring. // One that occurs again was retired too early: a fingerprint bump whose new // hashes are not live yet (a self-hosted warehouse that has not applied the diff --git a/packages/backend/src/services/errors/error-tick-regression-guard.test.ts b/packages/backend/src/services/errors/error-tick-regression-guard.test.ts new file mode 100644 index 0000000000..60c92eb7c9 --- /dev/null +++ b/packages/backend/src/services/errors/error-tick-regression-guard.test.ts @@ -0,0 +1,69 @@ +import { afterEach, assert, describe, it } from "@effect/vitest" +import * as PG from "@maple-dev/effect-orm/postgres" +import { Effect, Schema } from "effect" +import { ErrorIssues } from "@maple/db/tables" +import { ErrorIssueId, OrgId } from "@maple/domain/primitives" +import { Database } from "@maple/backend/platform/DatabaseLive" +import { cleanupTestDbs, createTestDb, type TestDb } from "@maple/backend/platform/test-pglite" +import { applyRegressions } from "./error-tick-persistence" + +const createdDbs: TestDb[] = [] +afterEach(() => cleanupTestDbs(createdDbs)) + +const ORG = Schema.decodeSync(OrgId)("org_error_tick_regression_guard") +const DONE = Schema.decodeSync(ErrorIssueId)("00000000-0000-4000-8000-0000000000d1") +const CANCELLED = Schema.decodeSync(ErrorIssueId)("00000000-0000-4000-8000-0000000000c1") +const NOW = Date.UTC(2026, 10, 6, 12, 0) + +const seed = (id: ErrorIssueId, workflowState: "done" | "cancelled") => + Effect.gen(function* () { + const database = yield* Database + yield* database.execute((db) => + db.run( + PG.insertInto(ErrorIssues).values({ + id, + orgId: ORG, + fingerprintHash: `fp-${id}`, + serviceName: "checkout-api", + exceptionType: "TimeoutError", + exceptionMessage: "upstream timed out", + topFrame: "handler.ts:42", + workflowState, + resolvedAt: workflowState === "done" ? NOW - 60_000 : null, + firstSeenAt: NOW, + lastSeenAt: NOW, + createdAt: NOW, + updatedAt: NOW, + }), + ), + ) + }) + +describe("applyRegressions", () => { + it.effect("reopens a done issue but not one a human cancelled after the scan read it", () => + Effect.gen(function* () { + yield* seed(DONE, "done") + // The scan saw both as `done`; a person cancelled this one before the tick wrote. + yield* seed(CANCELLED, "cancelled") + const database = yield* Database + + const reopened = yield* database.execute((db) => + applyRegressions(db, { orgId: ORG, windowEndMs: NOW }, [DONE, CANCELLED]), + ) + assert.deepStrictEqual([...reopened], [DONE]) + + const rows = yield* database.execute((db) => + db.run( + PG.from(ErrorIssues) + .select("id", "workflowState", "regressionCount", "resolvedAt") + .where(($) => [$.orgId.eq(ORG)]) + .orderBy(["id", "asc"]), + ), + ) + assert.deepStrictEqual(rows, [ + { id: CANCELLED, workflowState: "cancelled", regressionCount: 0, resolvedAt: null }, + { id: DONE, workflowState: "regressed", regressionCount: 1, resolvedAt: null }, + ]) + }).pipe(Effect.provide(createTestDb(createdDbs).layer)), + ) +}) diff --git a/packages/backend/src/services/errors/investigation-stale.test.ts b/packages/backend/src/services/errors/investigation-stale.test.ts index bd98fdd6f1..179e9aa5c2 100644 --- a/packages/backend/src/services/errors/investigation-stale.test.ts +++ b/packages/backend/src/services/errors/investigation-stale.test.ts @@ -156,7 +156,7 @@ describe("sweepAbandonedInvestigations", () => { const database = yield* Database const moved = yield* database.execute((db) => sweepAbandonedInvestigations(db, NOW)) - assert.strictEqual(moved, 2) + assert.strictEqual(moved.length, 2) assert.strictEqual(yield* statusOf("1"), "failed") assert.strictEqual(yield* statusOf("2"), "investigating") assert.strictEqual(yield* statusOf("3"), "failed") @@ -172,7 +172,8 @@ describe("sweepAbandonedInvestigations", () => { progress: { stepCount: 1, steps: [] }, }) const database = yield* Database - assert.strictEqual(yield* database.execute((db) => sweepAbandonedInvestigations(db, NOW)), 1) + const swept = yield* database.execute((db) => sweepAbandonedInvestigations(db, NOW)) + assert.strictEqual(swept.length, 1) assert.strictEqual(yield* statusOf("5"), "failed") }).pipe(Effect.provide(makeLayer())), ) diff --git a/packages/backend/src/services/errors/investigation-stale.ts b/packages/backend/src/services/errors/investigation-stale.ts index 898689ccea..4a4cedbff1 100644 --- a/packages/backend/src/services/errors/investigation-stale.ts +++ b/packages/backend/src/services/errors/investigation-stale.ts @@ -6,7 +6,8 @@ */ import * as PG from "@maple-dev/effect-orm/postgres" import type { MapleDb, MapleDbError } from "@maple/db/client" -import { Investigations } from "@maple/db/tables" +import { Investigations, type InvestigationRow } from "@maple/db/tables" +import type { OrgId } from "@maple/domain/http" import { Effect, Option, Schema } from "effect" /** One agent pass. If it has not answered in this long, it is not going to. */ @@ -58,7 +59,9 @@ export const isInvestigationStale = ( } /** - * Fail every abandoned run, across every org, in one statement. + * Fail abandoned runs: every org in one statement for the tick, or one org when a read finds a + * stale row on its page. The only sweep; there used to be a second, per-org one that ignored + * the heartbeat and failed live passes in their close-out. * * Until this existed the check ran only when something happened to read the row: a detail page * load, or a second incident on the same subject re-enqueueing. An autonomous run nobody opens is @@ -66,40 +69,35 @@ export const isInvestigationStale = ( * `investigating` indefinitely and the hub filled with investigations that were never coming back. * Prod, 2026-09-19: 78 of 100 starts left no trace at all. * - * Org-agnostic and unbatched on purpose. The per-org form would be one round-trip per org per tick - * against a table where the matching set is almost always empty; this is one statement for the - * whole tick, and `status` narrows it before `started_at` does. - * * Mirrors {@link isInvestigationStale}, heartbeat included: a run still recording steps is alive - * however long it has been going, and a sweep that ran every tick rather than on a read would - * otherwise fail a pass in its close-out. `coalesce` to zero so a row whose progress somehow - * carries no `updatedAt` reads as no heartbeat and is still swept, rather than a NULL comparison - * quietly excluding it forever. + * however long it has been going. `coalesce` to zero so a row whose progress somehow carries no + * `updatedAt` reads as no heartbeat and is still swept, rather than a NULL comparison quietly + * excluding it forever. Guarded on `investigating`, so a run that settled meanwhile is untouched. * - * Returns how many rows it moved so the tick can report a number that should normally be zero. + * Returns the rows it moved, so callers can settle their anomaly triage and the tick can report + * a count that should normally be zero. */ export const sweepAbandonedInvestigations = ( orm: MapleDb, nowMs: number, -): Effect.Effect => - Effect.map( - orm.run( - PG.update(Investigations) - .set({ - status: "failed", - error: staleTimeoutMessage(STALE_MS), - updatedAt: nowMs, - }) - .where(($) => [ - $.status.eq("investigating"), - $.startedAt.lt(nowMs - STALE_MS), - PG.or( - $.progressJson.isNull(), - PG.sql - .cond`coalesce((${$.progressJson}->>'updatedAt')::bigint, 0) < ${nowMs - PROGRESS_HEARTBEAT_STALE_MS}`, - ), - ]) - .returning("id"), - ), - (rows) => rows.length, + orgId?: OrgId, +): Effect.Effect>, MapleDbError> => + orm.run( + PG.update(Investigations) + .set({ + status: "failed", + error: staleTimeoutMessage(STALE_MS), + updatedAt: nowMs, + }) + .where(($) => [ + orgId === undefined ? undefined : $.orgId.eq(orgId), + $.status.eq("investigating"), + $.startedAt.lt(nowMs - STALE_MS), + PG.or( + $.progressJson.isNull(), + PG.sql + .cond`coalesce((${$.progressJson}->>'updatedAt')::bigint, 0) < ${nowMs - PROGRESS_HEARTBEAT_STALE_MS}`, + ), + ]) + .returning("incidentKind", "incidentId"), ) diff --git a/packages/backend/src/services/errors/issue-hub.test.ts b/packages/backend/src/services/errors/issue-hub.test.ts index 2f7b501797..6e92ea28df 100644 --- a/packages/backend/src/services/errors/issue-hub.test.ts +++ b/packages/backend/src/services/errors/issue-hub.test.ts @@ -223,7 +223,7 @@ describe("upsertAlertIssue", () => { }).pipe(Effect.provide(makeLayer())), ) - it.effect("reopens a done issue to triage with regression events", () => + it.effect("reopens a done issue as regressed with regression events", () => Effect.gen(function* () { yield* TestClock.setTime(T0) const database = yield* Database @@ -242,8 +242,11 @@ describe("upsertAlertIssue", () => { assert.strictEqual(second.action, "reopened") const issues = yield* loadIssue - assert.strictEqual(issues[0]?.workflowState, "triage") + // A fixed issue that fires again lands in `regressed`, as the errors tick does. + assert.strictEqual(issues[0]?.workflowState, "regressed") assert.isNull(issues[0]?.resolvedAt) + assert.strictEqual(issues[0]?.regressionCount, 1) + assert.strictEqual(issues[0]?.lastRegressedAt, T0 + 100_000) const events = yield* database.execute((db) => db.run( diff --git a/packages/backend/src/services/errors/issue-hub.ts b/packages/backend/src/services/errors/issue-hub.ts index 6800cb8878..1291db40ec 100644 --- a/packages/backend/src/services/errors/issue-hub.ts +++ b/packages/backend/src/services/errors/issue-hub.ts @@ -13,6 +13,7 @@ import { DateTime, Clock, Effect, Schema } from "effect" import { Database } from "@maple/backend/platform/DatabaseLive" import type { MaybeEnqueueTriageInput } from "./ai-triage-enqueue" import { issueSeverityFromAlert } from "./severity-map" +import { applyIssueReopen, issueReopenFor } from "./issue-reopen" import { SYSTEM_ALERTS_AGENT_NAME } from "@maple/backend/services/auth/system-actors" import { summarizeCause } from "@maple/backend/platform/describe-cause" @@ -284,29 +285,24 @@ export const upsertAlertIssue: ( .where(($) => [$.orgId.eq(input.orgId), $.id.eq(prior.id), $.severity.isNull()]), ), ) - const reopenFrom: WorkflowState | null = - prior.workflowState === "done" || prior.workflowState === "wontfix" - ? prior.workflowState - : null - if (reopenFrom !== null) { + const reopen = issueReopenFor(prior, input.timestamp) + const reopened = + reopen !== null && + (yield* database + .execute((db) => applyIssueReopen(db, input.orgId, prior.id, reopen, input.timestamp)) + .pipe( + Effect.as(true), + // A concurrent transition (a human re-triage, a renewed snooze) won. + Effect.catchTag("@maple/backend/TransitionConflictError", () => + Effect.succeed(false), + ), + )) + if (reopen !== null && reopened) { action = "reopened" - yield* database.execute((db) => - db.run( - PG.update(ErrorIssues) - .set({ - workflowState: "triage", - resolvedAt: null, - resolvedByActorId: null, - snoozeUntil: null, - updatedAt: input.timestamp, - }) - .where(($) => [$.orgId.eq(input.orgId), $.id.eq(prior.id)]), - ), - ) const actorId = yield* ensureSystemAlertsActor(input.orgId) yield* recordIssueEvent(input.orgId, issueId, actorId, "state_change", { - fromState: reopenFrom, - toState: "triage", + fromState: reopen.fromState, + toState: reopen.toState, payload: { viaRegression: true, incidentId: input.incidentId }, timestamp: input.timestamp, }) diff --git a/packages/backend/src/services/errors/issue-reopen.ts b/packages/backend/src/services/errors/issue-reopen.ts new file mode 100644 index 0000000000..625463e526 --- /dev/null +++ b/packages/backend/src/services/errors/issue-reopen.ts @@ -0,0 +1,69 @@ +import * as PG from "@maple-dev/effect-orm/postgres" +import type { MapleDb, MapleDbError } from "@maple/db/client" +import { ErrorIssues, type ErrorIssueRow } from "@maple/db/tables" +import type { OrgId, WorkflowState } from "@maple/domain/http" +import { transitionRow } from "@maple/backend/platform/transition-row" + +/** + * A closed issue that fires again: a fixed (`done`) one lands in `regressed` with the same + * bookkeeping the errors tick does, and a `wontfix` whose snooze expired wakes into `triage`. + */ +export type IssueReopen = + | { readonly fromState: "done"; readonly toState: "regressed" } + | { readonly fromState: "wontfix"; readonly toState: "triage" } + +export const issueReopenFor = ( + prior: Pick, + timestamp: number, +): IssueReopen | null => { + if (prior.workflowState === "done") return { fromState: "done", toState: "regressed" } + if (prior.workflowState === "wontfix" && prior.snoozeUntil !== null && prior.snoozeUntil <= timestamp) { + return { fromState: "wontfix", toState: "triage" } + } + return null +} + +/** + * Apply a reopen, guarded on the state it was decided from. Fails with + * `TransitionConflictError` when a concurrent transition moved the issue first (or re-snoozed + * it), so the caller records no regression for a reopen that did not happen. + */ +export const applyIssueReopen = ( + db: MapleDb, + orgId: OrgId, + issueId: ErrorIssueRow["id"], + reopen: IssueReopen, + timestamp: number, +) => + transitionRow( + { table: "error_issues", id: issueId, from: [reopen.fromState], to: reopen.toState }, + (from) => + reopen.toState === "regressed" + ? db.run( + PG.update(ErrorIssues) + .set(($) => ({ + workflowState: "regressed", + resolvedAt: null, + resolvedByActorId: null, + lastRegressedAt: timestamp, + regressionCount: $.regressionCount.add(1), + // A regression on an issue retention already archived must be seen. + archivedAt: null, + updatedAt: timestamp, + })) + .where(($) => [$.orgId.eq(orgId), $.id.eq(issueId), $.workflowState.in_(...from)]) + .returning("id"), + ) + : db.run( + PG.update(ErrorIssues) + .set({ workflowState: "triage", snoozeUntil: null, updatedAt: timestamp }) + .where(($) => [ + $.orgId.eq(orgId), + $.id.eq(issueId), + $.workflowState.in_(...from), + // A snooze renewed since the read keeps the issue asleep. + $.snoozeUntil.lte(timestamp), + ]) + .returning("id"), + ), + ) diff --git a/packages/backend/src/services/integrations/planetscale/webhook-events.test.ts b/packages/backend/src/services/integrations/planetscale/webhook-events.test.ts index 94fa947915..323b9654a1 100644 --- a/packages/backend/src/services/integrations/planetscale/webhook-events.test.ts +++ b/packages/backend/src/services/integrations/planetscale/webhook-events.test.ts @@ -327,7 +327,7 @@ describe("upsertPlanetScaleIssue", () => { [first.issueId], ), ) - assert.strictEqual(reopened?.workflow_state, "triage") + assert.strictEqual(reopened?.workflow_state, "regressed") assert.strictEqual(reopened?.occurrence_count, 3) }).pipe(Effect.provide(testDb.layer)) }) @@ -414,7 +414,7 @@ describe("upsertPlanetScaleIssue", () => { [initial.issueId], ), ) - assert.strictEqual(aggregate?.workflow_state, "triage") + assert.strictEqual(aggregate?.workflow_state, "regressed") assert.strictEqual(aggregate?.occurrence_count, 3) assert.strictEqual(aggregate?.state_changes, 1) assert.strictEqual(aggregate?.regressions, 1) diff --git a/packages/backend/src/services/integrations/planetscale/webhook-events.ts b/packages/backend/src/services/integrations/planetscale/webhook-events.ts index eb214ad1aa..8fae87a1d6 100644 --- a/packages/backend/src/services/integrations/planetscale/webhook-events.ts +++ b/packages/backend/src/services/integrations/planetscale/webhook-events.ts @@ -26,6 +26,7 @@ import { Clock, Effect, Option, Result, Schema, String as Str } from "effect" import { timestampMs } from "@maple/backend/platform/time" import { Database, DatabaseError } from "@maple/backend/platform/DatabaseLive" import { secretsEqual } from "@maple/backend/services/auth/secret-equal" +import { applyIssueReopen, issueReopenFor } from "@maple/backend/services/errors/issue-reopen" /** * PlanetScale webhook event handling: signature verification, payload decode, @@ -762,27 +763,28 @@ export const upsertPlanetScaleIssue: ( .where(($) => [$.orgId.eq(input.orgId), $.id.eq(prior.id)]), ) - const reopenFrom: WorkflowState | null = - prior.workflowState === "done" || prior.workflowState === "wontfix" - ? prior.workflowState - : null - if (reopenFrom === null) return { issueId, action: "refreshed" as const } - - yield* db.run( - PG.update(ErrorIssues) - .set({ - workflowState: "triage", - resolvedAt: null, - resolvedByActorId: null, - snoozeUntil: null, - updatedAt: input.timestamp, - }) - .where(($) => [$.orgId.eq(input.orgId), $.id.eq(prior.id)]), + const reopen = issueReopenFor(prior, input.timestamp) + if (reopen === null) return { issueId, action: "refreshed" as const } + + // The row is locked FOR UPDATE above, so this guard only ever trips on a + // writer that skipped the lock; a lost reopen is then an ordinary refresh. + const reopened = yield* applyIssueReopen( + db, + input.orgId, + prior.id, + reopen, + input.timestamp, + ).pipe( + Effect.as(true), + Effect.catchTag("@maple/backend/TransitionConflictError", () => + Effect.succeed(false), + ), ) + if (!reopened) return { issueId, action: "refreshed" as const } const actorId = yield* ensureActor yield* recordEvent(issueId, actorId, "state_change", { - fromState: reopenFrom, - toState: "triage", + fromState: reopen.fromState, + toState: reopen.toState, payload: { viaRegression: true, event: input.payload.event }, }) yield* recordEvent(issueId, actorId, "regression", { diff --git a/packages/domain/src/generated/anticipated-error-identifiers.ts b/packages/domain/src/generated/anticipated-error-identifiers.ts index 4eb36e380e..a629b0f37c 100644 --- a/packages/domain/src/generated/anticipated-error-identifiers.ts +++ b/packages/domain/src/generated/anticipated-error-identifiers.ts @@ -139,6 +139,7 @@ export const ANTICIPATED_ERROR_IDENTIFIER_LIST: ReadonlyArray = [ "@maple/http/errors/WarehouseValidationError", "@maple/http/investigations/InvestigationNotFoundError", "@maple/http/investigations/InvestigationQuotaError", + "@maple/http/investigations/InvestigationTransitionError", "@maple/http/investigations/InvestigationValidationError", "@maple/http/openrouter-key/OpenRouterKeyAddOnRequiredError", "@maple/http/openrouter-key/OpenRouterKeyForbiddenError", diff --git a/packages/domain/src/http/investigations.ts b/packages/domain/src/http/investigations.ts index a10d7e412e..c57399c8b3 100644 --- a/packages/domain/src/http/investigations.ts +++ b/packages/domain/src/http/investigations.ts @@ -36,6 +36,36 @@ export const InvestigationStatus = Schema.Literals([ }) export type InvestigationStatus = Schema.Schema.Type +/** + * The status moves a person may make by hand: resolve a finished run, or reopen a resolved one. + * The autonomous pass owns everything else (`investigating` and the verdict it lands on); starting + * a new pass is a restart, not a status change. + */ +export const MANUAL_INVESTIGATION_TRANSITIONS = { + investigating: [], + diagnosed: ["resolved"], + inconclusive: ["resolved"], + failed: ["resolved"], + resolved: ["diagnosed", "inconclusive", "failed"], +} as const satisfies Record> + +/** + * Where reopening a resolved investigation lands: back on the verdict its run reached, so a + * reopen cannot claim a diagnosis that never happened. + */ +export const reopenedInvestigationStatus = (run: { + readonly diagnosed: boolean + readonly hasReport: boolean +}): InvestigationStatus => (run.diagnosed ? "diagnosed" : run.hasReport ? "inconclusive" : "failed") + +/** A run in flight cannot be restarted underneath itself; every settled one can. */ +export const RESTARTABLE_INVESTIGATION_STATUSES = [ + "diagnosed", + "inconclusive", + "failed", + "resolved", +] as const satisfies Arr.NonEmptyReadonlyArray + /** Who opened the investigation: a person (attended) or an incident-open trigger. */ export const InvestigationSeededBy = Schema.Literals(["user", "system"]).annotate({ identifier: "@maple/InvestigationSeededBy", @@ -427,6 +457,25 @@ export class InvestigationRejectedError extends HttpTaggedError()( + "@maple/http/investigations/InvestigationTransitionError", + { + message: Schema.String, + investigationId: InvestigationId, + fromStatus: InvestigationStatus, + toStatus: InvestigationStatus, + }, + { + status: 409, + code: "investigation_status_conflict", + title: "Investigation status cannot change", + retry: "never", + recovery: "refresh", + exposure: "public_message", + }, +) {} + /** Stored investigation data no longer decodes into its current public schema. */ export class InvestigationDataCorruptionError extends HttpTaggedError()( "@maple/http/investigations/InvestigationDataCorruptionError", diff --git a/packages/domain/src/http/v2/investigations.ts b/packages/domain/src/http/v2/investigations.ts index 75ada5dad6..a1d0730ed9 100644 --- a/packages/domain/src/http/v2/investigations.ts +++ b/packages/domain/src/http/v2/investigations.ts @@ -10,6 +10,7 @@ import { InvestigationPersistenceError, InvestigationSeededBy, InvestigationStartFailedError, + InvestigationTransitionError, InvestigationStatus, } from "../investigations" import { TraceId, UserId } from "../../primitives" @@ -427,7 +428,8 @@ export type V2InvestigationCreateParams = Schema.Schema.Type Date: Fri, 6 Nov 2026 18:23:17 +0100 Subject: [PATCH 03/11] fix(backend): close the remaining unguarded investigation and tick writers The enqueue path's stale sweep now uses the shared heartbeat-aware sweep, markFailed is guarded on investigating, restart rescues a run whose start never claimed it, and the errors tick skips a wake or lease expiry that lost to a concurrent move (a renewed snooze included) instead of failing the org's tick. The investigation detail page disables resolve and restart on a run in flight, matching the server's rules. --- .../detail/investigation.scene.test.ts | 13 +++++++ .../src/page/investigations/detail/view.ts | 32 ++++++++++++++--- .../backend/src/platform/transition-row.ts | 15 +++----- .../errors/ErrorIssueWorkflowService.ts | 5 +++ .../src/services/errors/ErrorsService.ts | 35 ++++++++++++++++--- .../errors/InvestigationService.test.ts | 13 +++++++ .../services/errors/InvestigationService.ts | 18 ++++++---- .../src/services/errors/ai-triage-enqueue.ts | 20 ++--------- .../services/errors/investigation-stale.ts | 4 +-- .../services/errors/investigation-start.ts | 6 +++- 10 files changed, 114 insertions(+), 47 deletions(-) diff --git a/apps/web/src/page/investigations/detail/investigation.scene.test.ts b/apps/web/src/page/investigations/detail/investigation.scene.test.ts index 95240e0c63..592179d30d 100644 --- a/apps/web/src/page/investigations/detail/investigation.scene.test.ts +++ b/apps/web/src/page/investigations/detail/investigation.scene.test.ts @@ -241,6 +241,19 @@ describe("investigation detail", () => { expect(failed.model.busy).toBeNull() }) + test("a run still in flight offers no resolve, which the server would refuse", () => { + const ready = synced( + entered(), + investigationOf({ status: "investigating", report_json: null, diagnosed_at: null }), + ).model + scene( + { update: pageUpdate, view }, + given(ready), + ...canvas(), + expectView(role("button", { name: "Resolve" })).toBeDisabled(), + ) + }) + test("a failed run's primary action restarts it", () => { const ready = synced( entered(), diff --git a/apps/web/src/page/investigations/detail/view.ts b/apps/web/src/page/investigations/detail/view.ts index 890a6f8861..6090cb6632 100644 --- a/apps/web/src/page/investigations/detail/view.ts +++ b/apps/web/src/page/investigations/detail/view.ts @@ -1,4 +1,9 @@ -import type { IssueSeverity } from "@maple/domain/http" +import { + type InvestigationStatus, + type IssueSeverity, + MANUAL_INVESTIGATION_TRANSITIONS, + RESTARTABLE_INVESTIGATION_STATUSES, +} from "@maple/domain/http" import type { V2Investigation } from "@maple/domain/http/v2" import { investigationHeadline, investigationScope } from "@maple/web-data/investigations/display" import { Mount, Submodel } from "foldkit" @@ -81,6 +86,11 @@ const titleContent = (h: H, investigation: V2Investigation): Html => { const headerActions = (h: H, model: Model, investigation: V2Investigation): Html => { const status = investigation.status const isBusy = model.busy !== null + // The server's own rules: a run in flight can be neither resolved nor restarted. + const manualMoves: ReadonlyArray = MANUAL_INVESTIGATION_TRANSITIONS[status] + const restartable: ReadonlyArray = RESTARTABLE_INVESTIGATION_STATUSES + const canResolve = manualMoves.includes("resolved") + const canRestart = restartable.includes(status) const primary = status === "failed" ? { label: "Retry", variant: "default" as const } @@ -89,11 +99,25 @@ const headerActions = (h: H, model: Model, investigation: V2Investigation): Html : status === "resolved" ? { label: "Reopen", variant: "outline" as const } : { label: "Resolve", variant: "default" as const } + const primaryAllowed = status === "diagnosed" || status === "investigating" ? canResolve : canRestart const entries: ReadonlyArray = [ - { _tag: "Item", value: "restart", label: "Run again", icon: "ArrowPathIcon", isDisabled: isBusy }, + { + _tag: "Item", + value: "restart", + label: "Run again", + icon: "ArrowPathIcon", + isDisabled: isBusy || !canRestart, + }, ...(status === "resolved" ? [] - : [{ _tag: "Item" as const, value: "resolve", label: "Mark resolved", isDisabled: isBusy }]), + : [ + { + _tag: "Item" as const, + value: "resolve", + label: "Mark resolved", + isDisabled: isBusy || !canResolve, + }, + ]), ] return detailActions(h, [ hstack(h, { gap: "2" }, [ @@ -102,7 +126,7 @@ const headerActions = (h: H, model: Model, investigation: V2Investigation): Html { size: "sm", variant: primary.variant, - isDisabled: isBusy, + isDisabled: isBusy || !primaryAllowed, attributes: [h.OnClick(Message.ClickedPrimary())], }, [primary.label], diff --git a/packages/backend/src/platform/transition-row.ts b/packages/backend/src/platform/transition-row.ts index 832e77c618..9b3b0f3c2f 100644 --- a/packages/backend/src/platform/transition-row.ts +++ b/packages/backend/src/platform/transition-row.ts @@ -1,9 +1,6 @@ /** - * Guarded status transitions for the implicit state machines in Postgres. - * - * A writer that read a row's state earlier and then wrote `UPDATE ... WHERE id = ?` - * silently overwrote any transition that landed in between. The fix is to guard - * the UPDATE on the state the writer read and treat "no row came back" as a lost race. + * Guarded status transitions: the UPDATE carries the state the writer read, so a concurrent + * transition wins and the late writer sees a typed conflict instead of overwriting it. */ import { Array as Arr, Effect, Schema } from "effect" @@ -28,12 +25,8 @@ export interface TransitionRowOptions { } /** - * Run a guarded transition. `write` receives the expected states and must put them in its - * `WHERE IN (...)` alongside the org and id, with a RETURNING clause: the returned row - * is the transitioned one, and an empty result fails with {@link TransitionConflictError}. - * - * Callers either remap the conflict (an API caller's 409) or catch it as a no-op where losing - * is fine (a sweep, a late writer whose result no longer applies). + * `write` must guard on ` IN (from)` plus org and id, with RETURNING. No row back fails + * with {@link TransitionConflictError}; callers remap it (a 409) or catch it as a no-op. */ export const transitionRow = ( options: TransitionRowOptions, diff --git a/packages/backend/src/services/errors/ErrorIssueWorkflowService.ts b/packages/backend/src/services/errors/ErrorIssueWorkflowService.ts index 84c61a1e85..266b62db61 100644 --- a/packages/backend/src/services/errors/ErrorIssueWorkflowService.ts +++ b/packages/backend/src/services/errors/ErrorIssueWorkflowService.ts @@ -197,6 +197,8 @@ export interface ErrorIssueWorkflowServiceApi extends ErrorIssueWorkflowPublicAp readonly snoozeUntilMs?: number | null readonly timestamp?: number readonly payload?: StoredJsonRecord + /** Only apply while the snooze has elapsed by this time, so a renewed snooze wins. */ + readonly snoozeElapsedBy?: number }, ) => Effect.Effect< ErrorIssueRow, @@ -643,6 +645,9 @@ const make: Effect.Effect< $.orgId.eq(orgId), $.id.eq(row.id), $.workflowState.in_(...from), + opts.snoozeElapsedBy === undefined + ? undefined + : $.snoozeUntil.lte(opts.snoozeElapsedBy), ]) .returning("id"), ), diff --git a/packages/backend/src/services/errors/ErrorsService.ts b/packages/backend/src/services/errors/ErrorsService.ts index 32549735da..8c6bd215e6 100644 --- a/packages/backend/src/services/errors/ErrorsService.ts +++ b/packages/backend/src/services/errors/ErrorsService.ts @@ -1043,6 +1043,27 @@ const make: Effect.Effect< ) }) + /** + * Skip a system transition that lost to a concurrent move (wontfix to triage and + * in_progress to todo are always legal, so the refusal is the race), and keep ticking. + */ + const skipLostTransition = + (issueId: ErrorIssueId, via: string) => + ( + effect: Effect.Effect< + A, + ErrorPersistenceError | ErrorIssueNotFoundError | ErrorIssueTransitionError + >, + ) => + effect.pipe( + Effect.catchTag("@maple/http/errors/ErrorIssueTransitionError", (error) => + Effect.logInfo("Skipped a tick transition another writer beat").pipe( + Effect.annotateLogs({ issueId, via, message: error.message }), + Effect.as(undefined), + ), + ), + ) + const expireLeasesForOrg = Effect.fn("ErrorsService.expireLeases")(function* ( orgId: OrgId, nowMs: number, @@ -1075,7 +1096,7 @@ const make: Effect.Effect< yield* applyTransition(orgId, systemActor.id, refreshed, "todo", { payload: { viaLeaseExpiry: true }, timestamp: nowMs, - }) + }).pipe(skipLostTransition(row.id, "lease_expiry")) } }), ) @@ -1190,14 +1211,18 @@ const make: Effect.Effect< systemActor, ).pipe(Effect.tapError(() => releaseTickClaim(orgId, tickWindow.claimToken, nowMs))) - const wakeCandidates = preamble.wakeCandidates - yield* Effect.forEach(wakeCandidates, (row) => + const woken = yield* Effect.forEach(preamble.wakeCandidates, (row) => applyTransition(orgId, systemActor.id, row, "triage", { payload: { viaSnoozeWakeup: true }, timestamp: nowMs, - }), + snoozeElapsedBy: nowMs, + }).pipe( + Effect.as(true), + skipLostTransition(row.id, "snooze_wakeup"), + Effect.map((applied) => applied === true), + ), ).pipe(Effect.tapError(() => releaseTickClaim(orgId, tickWindow.claimToken, nowMs))) - const issuesReopened = wakeCandidates.length + const issuesReopened = Arr.filter(woken, (applied) => applied).length const scanWindow = (endMs: number) => Effect.gen(function* () { diff --git a/packages/backend/src/services/errors/InvestigationService.test.ts b/packages/backend/src/services/errors/InvestigationService.test.ts index a358ebc5a0..43a02a6a2c 100644 --- a/packages/backend/src/services/errors/InvestigationService.test.ts +++ b/packages/backend/src/services/errors/InvestigationService.test.ts @@ -391,6 +391,19 @@ describe("InvestigationService", () => { }).pipe(Effect.provide(harness.layer)) }) + it.effect("restarts a run whose start never claimed it", () => { + const chat = chatSessionHarness() + const harness = makeHarness(chat.env, chat.chatSessions) + return Effect.gen(function* () { + const service = yield* InvestigationService + // Created but never started: `investigating` with no `startedAt`, so nothing is running. + const orphan = yield* service.createInvestigation(ORG, null, freeformRequest("orphan")) + const restarted = yield* service.restartInvestigation(ORG, orphan.id) + assert.strictEqual(restarted.status, "investigating") + assert.lengthOf(chat.beginTurns, 1) + }).pipe(Effect.provide(harness.layer)) + }) + it.effect("the read-path sweep fails an abandoned run but spares one still recording steps", () => Effect.gen(function* () { const now = Date.UTC(2026, 10, 6, 12, 0) diff --git a/packages/backend/src/services/errors/InvestigationService.ts b/packages/backend/src/services/errors/InvestigationService.ts index 3f47c78182..d691496f50 100644 --- a/packages/backend/src/services/errors/InvestigationService.ts +++ b/packages/backend/src/services/errors/InvestigationService.ts @@ -360,9 +360,8 @@ export class InvestigationService extends Context.Service isInvestigationStale(row, nowMs) - // An anomaly's AI triage is an investigation anchored on the incident, so - // every status change of that investigation (start, restart, end, manual) - // sets the incident's triage status. Best-effort: it is display-only. + // An anomaly's AI triage is an investigation anchored on the incident: a (re)start + // re-arms it, and the run's first settled status settles it once. Display-only. const settleAnomalyTriage = ( orgId: OrgId, runs: ReadonlyArray>, @@ -617,8 +616,8 @@ export class InvestigationService extends Context.Service transitionRow( { @@ -637,7 +636,14 @@ export class InvestigationService extends Context.Service [$.orgId.eq(orgId), $.id.eq(id), $.status.in_(...from)]) + .where(($) => [ + $.orgId.eq(orgId), + $.id.eq(id), + PG.or( + $.status.in_(...from), + PG.and($.status.eq("investigating"), $.startedAt.isNull()), + ), + ]) .returning("incidentKind", "incidentId"), ), ), diff --git a/packages/backend/src/services/errors/ai-triage-enqueue.ts b/packages/backend/src/services/errors/ai-triage-enqueue.ts index ca74320fc7..e5001ee2d1 100644 --- a/packages/backend/src/services/errors/ai-triage-enqueue.ts +++ b/packages/backend/src/services/errors/ai-triage-enqueue.ts @@ -36,10 +36,9 @@ import { import { applyClassifierSeverity } from "@maple/backend/services/errors/issue-severity" import { startInvestigationTurn } from "@maple/backend/services/errors/investigation-start" import { - STALE_MS, isInvestigationStale, - staleTimeoutMessage, storedProgressJson, + sweepAbandonedInvestigations, } from "@maple/backend/services/errors/investigation-stale" import { summarizeCause } from "@maple/backend/platform/describe-cause" @@ -405,22 +404,7 @@ export const maybeEnqueueTriage: ( const existing = existingRows[0] if (existing !== undefined) { if (isInvestigationStale(existing, nowMs)) { - const budget = STALE_MS - yield* database.execute((db) => - db.run( - PG.update(Investigations) - .set({ - status: "failed", - error: staleTimeoutMessage(budget), - updatedAt: nowMs, - }) - .where(($) => [ - $.orgId.eq(input.orgId), - $.id.eq(existing.id), - $.startedAt.lt(nowMs - budget), - ]), - ), - ) + yield* database.execute((db) => sweepAbandonedInvestigations(db, nowMs, input.orgId)) } return { enqueued: false, investigationId: existing.id, reason: "duplicate" as const } } diff --git a/packages/backend/src/services/errors/investigation-stale.ts b/packages/backend/src/services/errors/investigation-stale.ts index 4a4cedbff1..78f4e8eaf3 100644 --- a/packages/backend/src/services/errors/investigation-stale.ts +++ b/packages/backend/src/services/errors/investigation-stale.ts @@ -74,8 +74,8 @@ export const isInvestigationStale = ( * `updatedAt` reads as no heartbeat and is still swept, rather than a NULL comparison quietly * excluding it forever. Guarded on `investigating`, so a run that settled meanwhile is untouched. * - * Returns the rows it moved, so callers can settle their anomaly triage and the tick can report - * a count that should normally be zero. + * Returns the rows it moved: the per-org read path settles their anomaly triage, and the tick + * reports the count, which should normally be zero. */ export const sweepAbandonedInvestigations = ( orm: MapleDb, diff --git a/packages/backend/src/services/errors/investigation-start.ts b/packages/backend/src/services/errors/investigation-start.ts index 8025a0665d..df412f50bf 100644 --- a/packages/backend/src/services/errors/investigation-start.ts +++ b/packages/backend/src/services/errors/investigation-start.ts @@ -70,7 +70,11 @@ export const startInvestigationTurn: ( db.run( PG.update(Investigations) .set({ status: "failed", error, updatedAt: nowMs }) - .where(($) => [$.id.eq(investigationId)]), + .where(($) => [ + $.orgId.eq(orgId), + $.id.eq(investigationId), + $.status.eq("investigating"), + ]), ), ) .pipe(Effect.asVoid) From cd7872cb1c42c55add47a26a723cc1965a2b2678 Mon Sep 17 00:00:00 2001 From: Makisuo Date: Fri, 6 Nov 2026 18:27:00 +0100 Subject: [PATCH 04/11] fix(backend): lease expiry keeps a holder's concurrent move and heartbeat The lease clear is guarded on still being expired, and only an issue still in_progress drops to todo. The detail page lets an unclaimed run be run again, and a renewed snooze is covered by a test. --- .../src/page/investigations/detail/view.ts | 4 ++- .../errors/ErrorIssueWorkflowService.test.ts | 27 +++++++++++++++++++ .../src/services/errors/ErrorsService.ts | 17 +++++++----- 3 files changed, 41 insertions(+), 7 deletions(-) diff --git a/apps/web/src/page/investigations/detail/view.ts b/apps/web/src/page/investigations/detail/view.ts index 6090cb6632..34acbcadbd 100644 --- a/apps/web/src/page/investigations/detail/view.ts +++ b/apps/web/src/page/investigations/detail/view.ts @@ -90,7 +90,9 @@ const headerActions = (h: H, model: Model, investigation: V2Investigation): Html const manualMoves: ReadonlyArray = MANUAL_INVESTIGATION_TRANSITIONS[status] const restartable: ReadonlyArray = RESTARTABLE_INVESTIGATION_STATUSES const canResolve = manualMoves.includes("resolved") - const canRestart = restartable.includes(status) + // A run whose start never claimed it (no `started_at`) is not in flight and can be restarted. + const canRestart = + restartable.includes(status) || (status === "investigating" && investigation.started_at === null) const primary = status === "failed" ? { label: "Retry", variant: "default" as const } diff --git a/packages/backend/src/services/errors/ErrorIssueWorkflowService.test.ts b/packages/backend/src/services/errors/ErrorIssueWorkflowService.test.ts index c36e305643..cfe26b0df5 100644 --- a/packages/backend/src/services/errors/ErrorIssueWorkflowService.test.ts +++ b/packages/backend/src/services/errors/ErrorIssueWorkflowService.test.ts @@ -450,6 +450,33 @@ describe("ErrorIssueWorkflowService", () => { }).pipe(Effect.provide(makeLayer())), ) + it.effect("a snooze wakeup loses to a snooze renewed after it was read", () => + Effect.gen(function* () { + const workflow = yield* ErrorIssueWorkflowService + const database = yield* Database + const now = yield* Clock.currentTimeMillis + const issueId = asIssueId(randomUUID()) + yield* seedIssue(issueId, { workflowState: "wontfix", snoozeUntil: now - 1_000 }) + const readByTick = yield* workflow.requireIssue(ORG, issueId) + // A person re-snoozes it before the tick writes. + yield* database.execute((db) => + db.run( + PG.update(ErrorIssues) + .set({ snoozeUntil: now + 3_600_000 }) + .where(($) => [$.id.eq(issueId)]), + ), + ) + + const lost = yield* Effect.flip( + workflow.applyTransition(ORG, null, readByTick, "triage", { snoozeElapsedBy: now }), + ) + assert.strictEqual(lost._tag, "@maple/http/errors/ErrorIssueTransitionError") + const after = yield* workflow.requireIssue(ORG, issueId) + assert.strictEqual(after.workflowState, "wontfix") + assert.strictEqual(after.snoozeUntil, now + 3_600_000) + }).pipe(Effect.provide(makeLayer())), + ) + // The lease used to advance only via an explicit `heartbeat` call, which agents // never made: `claim` was used 11 times in 8 days, `heartbeat` zero. Issues were // claimed, worked, and silently dropped back to `todo` when the lease lapsed. diff --git a/packages/backend/src/services/errors/ErrorsService.ts b/packages/backend/src/services/errors/ErrorsService.ts index 8c6bd215e6..bc7e2f1b36 100644 --- a/packages/backend/src/services/errors/ErrorsService.ts +++ b/packages/backend/src/services/errors/ErrorsService.ts @@ -1044,8 +1044,8 @@ const make: Effect.Effect< }) /** - * Skip a system transition that lost to a concurrent move (wontfix to triage and - * in_progress to todo are always legal, so the refusal is the race), and keep ticking. + * Skip a system transition that lost to a concurrent move (it is only ever attempted from a + * state where the move is legal, so a refusal is the race), and keep ticking. */ const skipLostTransition = (issueId: ErrorIssueId, via: string) => @@ -1075,7 +1075,8 @@ const make: Effect.Effect< yield* Effect.forEach(expired, (row) => Effect.gen(function* () { const prevActorId = row.leaseHolderActorId - yield* dbExecute((db) => + // Still expired: a heartbeat committed since the preamble read keeps the lease. + const cleared = yield* dbExecute((db) => db.run( PG.update(ErrorIssues) .set({ @@ -1084,15 +1085,19 @@ const make: Effect.Effect< claimedAt: null, updatedAt: nowMs, }) - .where(($) => [$.id.eq(row.id)]), + .where(($) => [$.orgId.eq(orgId), $.id.eq(row.id), $.leaseExpiresAt.lt(nowMs)]) + .returning("id"), ), ) + if (cleared.length === 0) return yield* recordEvent(orgId, row.id, systemActor.id, "lease_expired", { payload: { previousHolderActorId: prevActorId }, timestamp: nowMs, }) - if (row.workflowState === "in_progress") { - const refreshed = yield* requireIssue(orgId, row.id) + // Only an issue still `in_progress` drops back to `todo`; one the holder already moved + // on (to review, verifying, triage) keeps its new state. + const refreshed = yield* requireIssue(orgId, row.id) + if (refreshed.workflowState === "in_progress") { yield* applyTransition(orgId, systemActor.id, refreshed, "todo", { payload: { viaLeaseExpiry: true }, timestamp: nowMs, From be0a9aa2596007eac8c806de76fd451876193289 Mon Sep 17 00:00:00 2001 From: Makisuo Date: Fri, 6 Nov 2026 18:49:09 +0100 Subject: [PATCH 05/11] fix(api): fence schema-apply progress on the claiming run id and heartbeat long steps --- .../ClickHouseSchemaApplyWorkflow.run.test.ts | 79 ++++++- .../ClickHouseSchemaApplyWorkflow.run.ts | 204 ++++++++++++++---- packages/backend/src/platform/bindings.ts | 2 +- .../org/OrgClickHouseSettingsService.ts | 19 +- 4 files changed, 254 insertions(+), 50 deletions(-) diff --git a/apps/api/src/workflows/ClickHouseSchemaApplyWorkflow.run.test.ts b/apps/api/src/workflows/ClickHouseSchemaApplyWorkflow.run.test.ts index 1bdf1a37b7..bae03f4826 100644 --- a/apps/api/src/workflows/ClickHouseSchemaApplyWorkflow.run.test.ts +++ b/apps/api/src/workflows/ClickHouseSchemaApplyWorkflow.run.test.ts @@ -85,8 +85,8 @@ describe("runClickHouseSchemaApply failure bookkeeping", () => { const ENCRYPTION_KEY = Buffer.alloc(32, 5).toString("base64") /** The run as the alchemy class provides it (env as `ConfigProvider`), minus the connection scope and telemetry. */ - const runFor = (testDb: TestDb, env: Record, orgId: string) => - runClickHouseSchemaApply({ orgId }).pipe( + const runFor = (testDb: TestDb, env: Record, orgId: string, runId = "run-1") => + runClickHouseSchemaApply({ orgId, runId }).pipe( Effect.provide([ testDb.layer, Layer.succeed(Cloudflare.WorkflowStep, inlineStep), @@ -97,25 +97,90 @@ describe("runClickHouseSchemaApply failure bookkeeping", () => { Effect.exit, ) - const seedQueuedRun = (testDb: TestDb, orgId: string) => + const seedQueuedRun = (testDb: TestDb, orgId: string, runId = "run-1", status = "queued") => Effect.promise(() => executeSql( testDb, - `INSERT INTO org_clickhouse_schema_apply_runs (org_id, status, phase, created_at, updated_at) - VALUES ($1, 'queued', 'queued', now(), now())`, + `INSERT INTO org_clickhouse_schema_apply_runs (org_id, workflow_instance_id, status, phase, created_at, updated_at) + VALUES ($1, $2, $3, 'queued', now(), now())`, + [orgId, runId, status], + ), + ) + + /** A settings row loadConfig accepts: no password, plain HTTP to a port nothing listens on. */ + const seedSettings = (testDb: TestDb, orgId: string) => + Effect.promise(() => + executeSql( + testDb, + `INSERT INTO org_clickhouse_settings (org_id, ch_url, ch_user, ch_database, sync_status, created_at, updated_at, created_by, updated_by) + VALUES ($1, 'http://127.0.0.1:9', 'default', 'default', 'connected', now(), now(), 'u', 'u')`, [orgId], ), ) const readRun = (testDb: TestDb, orgId: string) => Effect.promise(() => - queryFirstRow<{ status: string; error_message: string | null }>( + queryFirstRow<{ + status: string + error_message: string | null + workflow_instance_id: string | null + phase: string | null + }>( + testDb, + "SELECT status, error_message, workflow_instance_id, phase FROM org_clickhouse_schema_apply_runs WHERE org_id = $1", + [orgId], + ), + ) + + const readSyncStatus = (testDb: TestDb, orgId: string) => + Effect.promise(() => + queryFirstRow<{ sync_status: string }>( testDb, - "SELECT status, error_message FROM org_clickhouse_schema_apply_runs WHERE org_id = $1", + "SELECT sync_status FROM org_clickhouse_settings WHERE org_id = $1", [orgId], ), ) + it.effect("a reclaimed instance stops before touching the cluster and leaves the new run alone", () => + Effect.gen(function* () { + const testDb = createTestDb(trackedDbs) + // A newer applySchema reclaimed the row for run-new; run-old wakes up afterwards. + yield* seedQueuedRun(testDb, "org_wf_stale", "run-new", "running") + yield* seedSettings(testDb, "org_wf_stale") + + const exit = yield* runFor( + testDb, + { MAPLE_INGEST_KEY_ENCRYPTION_KEY: ENCRYPTION_KEY }, + "org_wf_stale", + "run-old", + ) + assert.deepStrictEqual(exit, Exit.succeed({ status: "superseded", appliedVersions: [] })) + + const row = yield* readRun(testDb, "org_wf_stale") + assert.strictEqual(row?.workflow_instance_id, "run-new") + assert.strictEqual(row?.status, "running") + assert.strictEqual(row?.phase, "queued") + assert.strictEqual((yield* readSyncStatus(testDb, "org_wf_stale"))?.sync_status, "connected") + }), + ) + + it.effect("a reclaimed instance that fails cannot mark the new run failed", () => + Effect.gen(function* () { + const testDb = createTestDb(trackedDbs) + yield* seedQueuedRun(testDb, "org_wf_stale_fail", "run-new") + yield* seedSettings(testDb, "org_wf_stale_fail") + + // No encryption key: run-old fails before its first step and takes the failure path. + const exit = yield* runFor(testDb, {}, "org_wf_stale_fail", "run-old") + assert.include(failureMessage(exit), "MAPLE_INGEST_KEY_ENCRYPTION_KEY") + + const row = yield* readRun(testDb, "org_wf_stale_fail") + assert.strictEqual(row?.status, "queued") + assert.strictEqual(row?.error_message, null) + assert.strictEqual((yield* readSyncStatus(testDb, "org_wf_stale_fail"))?.sync_status, "connected") + }), + ) + /** A step that spent its retries and a typed failure outside the steps both stay in the error channel. */ const failureMessage = (exit: Exit.Exit): string => { assert.isTrue(Exit.isFailure(exit)) diff --git a/apps/api/src/workflows/ClickHouseSchemaApplyWorkflow.run.ts b/apps/api/src/workflows/ClickHouseSchemaApplyWorkflow.run.ts index 52dfa7dd89..1f29ce2bc6 100644 --- a/apps/api/src/workflows/ClickHouseSchemaApplyWorkflow.run.ts +++ b/apps/api/src/workflows/ClickHouseSchemaApplyWorkflow.run.ts @@ -32,7 +32,18 @@ import { } from "@maple/domain/clickhouse" import { OrgId } from "@maple/domain/http" import * as Cloudflare from "@alchemy.run/cloudflare" -import { Cause, Clock, Config, Effect, Option, Redacted, Schema, Array as Arr, String as Str } from "effect" +import { + Cause, + Clock, + Config, + Effect, + Option, + Predicate, + Redacted, + Schema, + Array as Arr, + String as Str, +} from "effect" import { FetchHttpClient, HttpClient, HttpClientRequest } from "effect/http" import { EdgeCacheService } from "@maple/cache" import { EdgeCacheServiceLive } from "@maple/backend/platform/CacheBackendLive" @@ -42,6 +53,7 @@ import { ORG_CH_CONFIG_CACHE_BUCKET, } from "@maple/backend/services/org/OrgClickHouseSettingsService" import { durableStep, type DurableStepConfig } from "@maple/backend/platform/durable-step" +import { transitionRow } from "@maple/backend/platform/transition-row" /** * Bust the cached runtime config after the workflow writes to @@ -73,10 +85,12 @@ const bustRuntimeConfigCache = (orgId: OrgId): Effect.Effect => export interface SchemaApplyWorkflowPayload { readonly orgId: string + /** The claim's run id (`workflow_instance_id`); every progress write is fenced on it. */ + readonly runId: string } export interface SchemaApplyWorkflowResult { - readonly status: "succeeded" | "failed" + readonly status: "succeeded" | "superseded" readonly appliedVersions: ReadonlyArray } @@ -106,6 +120,15 @@ const causeMessage = (cause: Cause.Cause): string => { return error instanceof Error ? error.message : String(error) } +/** An optional step may be skipped, but a lost fence must still stop the run. */ +const isSkippable = (cause: Cause.Cause): boolean => + !Arr.some( + cause.reasons, + (reason) => + Cause.isFailReason(reason) && + Predicate.isTagged(reason.error, "@maple/api/workflows/SchemaApplyRunSupersededError"), + ) + /** * Optional feature metadata must never decide whether the correctness schema * is ready for ingest. Keeping this boundary explicit also makes the workflow @@ -300,6 +323,12 @@ export class SchemaApplyPayloadError extends Schema.TaggedError()( + "@maple/api/workflows/SchemaApplyRunSupersededError", + { message: Schema.String, orgId: OrgId, runId: Schema.String }, +) {} + /** A settings row that cannot be turned into a usable ClickHouse target. */ export class SchemaApplyConfigError extends Schema.TaggedError()( "@maple/api/workflows/SchemaApplyConfigError", @@ -393,18 +422,80 @@ type RunPatch = Partial<{ finishedAt: number | null }> -/** Stamps `updatedAt` from the clock, so it only ever runs inside a step (or on the failure path). */ -const updateRun = (database: DatabaseApi, orgId: OrgId, patch: RunPatch) => +type RunStatus = "queued" | "running" | "succeeded" | "failed" + +/** The run row this instance owns: the org's row while it still carries the claim's run id. */ +interface RunFence { + readonly database: DatabaseApi + readonly orgId: OrgId + readonly runId: string +} + +/** + * Stamps `updatedAt` from the clock, so it only ever runs inside a step (or on the failure path). + * Fenced on the run id and a live status: a reclaimed or settled run fails it as superseded. + */ +const updateRun = (fence: RunFence, patch: RunPatch) => Effect.gen(function* () { const now = yield* Clock.currentTimeMillis - yield* database.execute((db) => - db.run( - PG.update(OrgClickHouseSchemaApplyRuns) - .set({ ...patch, updatedAt: now }) - .where(($) => [$.orgId.eq(orgId)]), - ), + yield* transitionRow( + { + table: "org_clickhouse_schema_apply_runs", + id: fence.orgId, + from: ["queued", "running"], + to: patch.status ?? "running", + }, + (from) => + fence.database.execute((db) => + db.run( + PG.update(OrgClickHouseSchemaApplyRuns) + .set({ ...patch, updatedAt: now }) + .where(($) => [ + $.orgId.eq(fence.orgId), + $.workflowInstanceId.eq(fence.runId), + $.status.in_(...from), + ]) + .returning("orgId"), + ), + ), ) - }) + }).pipe( + Effect.catchTag("@maple/backend/TransitionConflictError", (conflict) => + Effect.fail( + new SchemaApplyRunSupersededError({ + message: conflict.message, + orgId: fence.orgId, + runId: fence.runId, + }), + ), + ), + ) + +/** Under the 30 minute staleness window the service reclaims a silent run after. */ +const RUN_HEARTBEAT = "5 minutes" + +/** + * Runs one long statement (a backfill chunk) while touching the run row, so a live run never + * reads as stale. Losing the fence ends the statement; a failed touch is retried next beat. + */ +const withRunHeartbeat = (fence: RunFence, statement: Effect.Effect) => + Effect.raceFirst( + statement, + Effect.sleep(RUN_HEARTBEAT).pipe( + Effect.andThen( + updateRun(fence, {}).pipe( + Effect.catchTag("@maple/backend/lib/DatabaseError", (error) => + Effect.logWarning("Schema apply heartbeat failed", error.message), + ), + ), + ), + Effect.forever, + ), + ) + +/** Claims the step for this run, then runs its statement under the heartbeat. */ +const fencedExec = (fence: RunFence, cfg: ChConfig, patch: RunPatch, sql: string) => + updateRun(fence, patch).pipe(Effect.andThen(withRunHeartbeat(fence, execClickHouse(cfg, sql)))) // --- snapshot-diff additive (mirror of OrgClickHouseSettingsService) -------- @@ -515,7 +606,14 @@ export const runClickHouseSchemaApply = ( (error) => new SchemaApplyPayloadError({ message: error.message, rawOrgId: payload.orgId }), ), ) - const database = yield* Database + if (typeof payload.runId !== "string" || payload.runId.length === 0) { + return yield* new SchemaApplyPayloadError({ + message: "Schema apply payload has no run id", + rawOrgId: payload.orgId, + }) + } + const fence: RunFence = { database: yield* Database, orgId, runId: payload.runId } + const { database } = fence // Inside the protected region below: a config-load failure (settings row // deleted, missing encryption key, decrypt failure, invalid URL) must still @@ -525,11 +623,23 @@ export const runClickHouseSchemaApply = ( Effect.gen(function* () { const finishedAt = yield* Clock.currentTimeMillis const message = causeMessage(cause) - yield* updateRun(database, orgId, { + // Only the run that still owns the row may report the org's sync error. + const owned = yield* updateRun(fence, { status: "failed", errorMessage: message, finishedAt, - }).pipe(Effect.ignore({ log: true, message: "Failed to mark schema apply run failed" })) + }).pipe( + Effect.as(true), + Effect.catchTag("@maple/api/workflows/SchemaApplyRunSupersededError", () => + Effect.succeed(false), + ), + Effect.catch((error) => + Effect.logWarning("Failed to mark schema apply run failed", error.message).pipe( + Effect.as(true), + ), + ), + ) + if (!owned) return yield* database .execute((db) => db.run( @@ -551,13 +661,18 @@ export const runClickHouseSchemaApply = ( yield* bustRuntimeConfigCache(orgId) }) - return yield* applySchema(database, orgId).pipe( + return yield* applySchema(fence).pipe( + // A reclaimed run ends quietly: the newer run owns the row and the org's sync status. + Effect.catchTag("@maple/api/workflows/SchemaApplyRunSupersededError", () => + Effect.succeed({ status: "superseded", appliedVersions: [] }), + ), Effect.catchCause((cause) => markFailed(cause).pipe(Effect.andThen(Effect.failCause(cause)))), ) }) -const applySchema = (database: DatabaseApi, orgId: OrgId) => +const applySchema = (fence: RunFence) => Effect.gen(function* () { + const { database, orgId } = fence const encryptionKeyText = yield* Config.Redacted(ENCRYPTION_KEY_CONFIG).pipe( Effect.mapError( (error) => @@ -581,7 +696,7 @@ const applySchema = (database: DatabaseApi, orgId: OrgId) => Effect.gen(function* () { const c = yield* loadConfig(database, orgId, encryptionKey) const startedAt = yield* Clock.currentTimeMillis - yield* updateRun(database, orgId, { + yield* updateRun(fence, { status: "running", phase: "connecting", errorMessage: null, @@ -615,12 +730,17 @@ const applySchema = (database: DatabaseApi, orgId: OrgId) => // would repeat, and a clock read there would differ per replay. durableStep( `m${migration.version}:${s.name}`, - updateRun(database, orgId, { - phase: `migration ${migration.version} · ${s.name}`, - currentMigration: migration.version, - stepsTotal: steps.length, - stepsDone: index, - }).pipe(Effect.andThen(execClickHouse(cfg, s.sql))), + fencedExec( + fence, + cfg, + { + phase: `migration ${migration.version} · ${s.name}`, + currentMigration: migration.version, + stepsTotal: steps.length, + stepsDone: index, + }, + s.sql, + ), STEP, ), { discard: true }, @@ -639,8 +759,8 @@ const applySchema = (database: DatabaseApi, orgId: OrgId) => // cluster + add missing columns (metadata-only, fits a step easily). yield* durableStep( "snapshot-diff", - updateRun(database, orgId, { phase: "reconciling schema snapshot" }).pipe( - Effect.andThen(reconcileSchemaSnapshot(cfg)), + updateRun(fence, { phase: "reconciling schema snapshot" }).pipe( + Effect.andThen(withRunHeartbeat(fence, reconcileSchemaSnapshot(cfg))), ), STEP, ) @@ -686,12 +806,17 @@ const applySchema = (database: DatabaseApi, orgId: OrgId) => (migrationStep, index) => durableStep( `performance-m${migration.version}:${migrationStep.name}`, - updateRun(database, orgId, { - phase: `performance migration ${migration.version} · ${index + 1}/${steps.length}`, - currentMigration: migration.version, - stepsTotal: steps.length, - stepsDone: index, - }).pipe(Effect.andThen(execClickHouse(cfg, migrationStep.sql))), + fencedExec( + fence, + cfg, + { + phase: `performance migration ${migration.version} · ${index + 1}/${steps.length}`, + currentMigration: migration.version, + stepsTotal: steps.length, + stepsDone: index, + }, + migrationStep.sql, + ), STEP, ), { discard: true }, @@ -706,7 +831,7 @@ const applySchema = (database: DatabaseApi, orgId: OrgId) => }) // A performance migration that cannot be installed is skipped, never fatal. return yield* applyPerformanceMigration.pipe( - Effect.catchCause((cause) => + Effect.catchCauseIf(isSkippable, (cause) => Effect.sync(() => { skippedFeatures.push({ id: `migration_${migration.version}`, @@ -779,9 +904,14 @@ const applySchema = (database: DatabaseApi, orgId: OrgId) => (statement, index) => durableStep( `feature-${feature.id}:${index + 1}`, - updateRun(database, orgId, { - phase: `feature ${feature.id} · ${index + 1}/${feature.statements.length}`, - }).pipe(Effect.andThen(execClickHouse(cfg, statement))), + fencedExec( + fence, + cfg, + { + phase: `feature ${feature.id} · ${index + 1}/${feature.statements.length}`, + }, + statement, + ), STEP, ), { discard: true }, @@ -794,7 +924,7 @@ const applySchema = (database: DatabaseApi, orgId: OrgId) => ) }) return yield* applyFeature.pipe( - Effect.catchCause((cause) => + Effect.catchCauseIf(isSkippable, (cause) => Effect.sync(() => { skippedFeatures.push({ id: feature.id, reason: causeMessage(cause) }) }), @@ -809,7 +939,7 @@ const applySchema = (database: DatabaseApi, orgId: OrgId) => "finalize", Effect.gen(function* () { const finishedAt = yield* Clock.currentTimeMillis - yield* updateRun(database, orgId, { + yield* updateRun(fence, { status: "succeeded", phase: "done", currentMigration: null, diff --git a/packages/backend/src/platform/bindings.ts b/packages/backend/src/platform/bindings.ts index 8d9836152c..c76c7653f1 100644 --- a/packages/backend/src/platform/bindings.ts +++ b/packages/backend/src/platform/bindings.ts @@ -147,7 +147,7 @@ export interface WorkflowStarter { /** The per-org ClickHouse schema apply, hosted by the api Worker. */ export class SchemaApplyWorkflow extends Context.Service< SchemaApplyWorkflow, - WorkflowStarter<{ readonly orgId: string }> + WorkflowStarter<{ readonly orgId: string; readonly runId: string }> >()("@maple/backend/platform/SchemaApplyWorkflow") {} export class WorkflowCancelError extends Schema.TaggedError()( diff --git a/packages/backend/src/services/org/OrgClickHouseSettingsService.ts b/packages/backend/src/services/org/OrgClickHouseSettingsService.ts index 503cea21ad..9ccf041e3c 100644 --- a/packages/backend/src/services/org/OrgClickHouseSettingsService.ts +++ b/packages/backend/src/services/org/OrgClickHouseSettingsService.ts @@ -1,5 +1,6 @@ // SAFETY-FILE: JSON rows here come from fixed internal formats and are validated before domain use. // BOUNDARY: This module intentionally carries opaque values; callers decode them before domain use. +import { randomUUID } from "node:crypto" import * as Policy from "@maple/domain/policy" import { IsoDateTimeString, @@ -1301,15 +1302,18 @@ export class OrgClickHouseSettingsService extends Context.Service< // concurrent applySchema calls wins (interleaved workflow instances // would race destructive DROP/TRUNCATE/backfill migrations). A // queued/running row that stopped updating for STALE_APPLY_RUN_MS is - // reclaimable — the workflow stamps progress on every step, so a silent - // stall that long means the instance died without reaching its catch. + // reclaimable — the workflow stamps progress on every step and heartbeats + // long ones, so a silent stall that long means the instance died. const now = yield* Clock.currentTimeMillis + // The run id fences every progress write: an instance reclaimed as stale finds it + // replaced and stops instead of writing over (or migrating under) the new run. + const runId = yield* Effect.sync(() => randomUUID()) const claimed = yield* dbExecute((db) => db.run( PG.insertInto(OrgClickHouseSchemaApplyRuns) .values({ orgId, - workflowInstanceId: null, + workflowInstanceId: runId, status: "queued", phase: "queued", currentMigration: null, @@ -1331,6 +1335,7 @@ export class OrgClickHouseSettingsService extends Context.Service< $.updatedAt.lt(now - STALE_APPLY_RUN_MS), ), set: { + workflowInstanceId: runId, status: "queued", phase: "queued", currentMigration: null, @@ -1351,7 +1356,7 @@ export class OrgClickHouseSettingsService extends Context.Service< return new OrgClickHouseApplySchemaStarted({ status: "already_running" }) } - yield* workflow.create({ orgId }).pipe( + yield* workflow.create({ orgId, runId }).pipe( Effect.mapError( (error) => new OrgClickHouseSettingsPersistenceError({ @@ -1372,7 +1377,11 @@ export class OrgClickHouseSettingsService extends Context.Service< finishedAt: now, updatedAt: now, }) - .where(($) => [$.orgId.eq(orgId), $.status.eq("queued")]), + .where(($) => [ + $.orgId.eq(orgId), + $.workflowInstanceId.eq(runId), + $.status.eq("queued"), + ]), ), ) .pipe(Effect.ignore), From b5ae7a4083f780a805609f27e12cba093ffb5a4e Mon Sep 17 00:00:00 2001 From: Makisuo Date: Fri, 6 Nov 2026 18:50:03 +0100 Subject: [PATCH 06/11] fix(backend): fence the post-merge settle on the examination's lease --- .../PrReviewPostMergeService.test.ts | 29 +++++++++ .../pr-review/PrReviewPostMergeService.ts | 61 +++++++++++++------ 2 files changed, 73 insertions(+), 17 deletions(-) diff --git a/packages/backend/src/services/pr-review/PrReviewPostMergeService.test.ts b/packages/backend/src/services/pr-review/PrReviewPostMergeService.test.ts index 9b413dee28..bec1dbe958 100644 --- a/packages/backend/src/services/pr-review/PrReviewPostMergeService.test.ts +++ b/packages/backend/src/services/pr-review/PrReviewPostMergeService.test.ts @@ -306,3 +306,32 @@ describe("PrReviewPostMergeService.runTick, lease", () => { }).pipe(Effect.provide(layerFor(testDb, posted, { failOperations: false }, racing))) }) }) + +describe("PrReviewPostMergeService.runTick, settle", () => { + it.effect("does not overwrite a report a later tick made while this one read the deploys", () => { + const testDb = createTestDb(trackedDbs) + const posted: Array = [] + // A later tick, holding the row's next lease, reports while this one waits on the warehouse. + const reportElsewhere = Effect.gen(function* () { + const db = yield* Database + yield* db.execute((client) => + client.run( + PG.update(PrReviews) + .set({ postMergeStatus: "reported", postMergeAfter: null }) + .where(($) => [$.id.eq(reviewId)]), + ), + ) + }).pipe(Effect.provide(testDb.layer), Effect.orDie) + const noDeployYet: PrReviewTelemetryServiceApi = { + ...telemetryFake({ failOperations: false }), + deploymentsSince: () => reportElsewhere.pipe(Effect.as([])), + } + return Effect.gen(function* () { + yield* seedDueReview + yield* TestClock.setTime(DEPLOYED_AT + 70 * 60_000) + const result = yield* (yield* PrReviewPostMergeService).runTick + assert.equal(result.waiting, 0) + assert.deepStrictEqual(yield* rowState, { status: "reported", after: null }) + }).pipe(Effect.provide(layerFor(testDb, posted, { failOperations: false }, noDeployYet))) + }) +}) diff --git a/packages/backend/src/services/pr-review/PrReviewPostMergeService.ts b/packages/backend/src/services/pr-review/PrReviewPostMergeService.ts index 7b0a5eeca4..57b6f46fa6 100644 --- a/packages/backend/src/services/pr-review/PrReviewPostMergeService.ts +++ b/packages/backend/src/services/pr-review/PrReviewPostMergeService.ts @@ -21,6 +21,7 @@ import { PrReviews, type PrReviewRow } from "@maple/db/tables" import { Context, DateTime, Duration, Effect, Layer, Option, Schema } from "effect" import { Database } from "@maple/backend/platform/DatabaseLive" import { summarizeCause } from "@maple/backend/platform/describe-cause" +import { transitionRow } from "@maple/backend/platform/transition-row" import { VcsProviderRegistry } from "@maple/backend/services/integrations/vcs/VcsProviderRegistry" import { VcsRepository } from "@maple/backend/services/integrations/vcs/VcsRepository" import { PrReviewTelemetryService } from "./telemetry/PrReviewTelemetryService" @@ -78,23 +79,41 @@ export class PrReviewPostMergeService extends Context.Service< const repositories = yield* VcsRepository const providers = yield* VcsProviderRegistry + /** + * Applies only while the row is still `waiting` under the lease this examination took: a + * look that outlived it lost the row to a later tick, which may already have reported. + */ const settle = ( orgId: OrgId, id: PrReviewId, + leaseUntil: DateTime.Utc, status: PrReviewPostMergeStatus, postMergeAfter: number | null, now: DateTime.Utc, ) => - database.execute((db) => - db.run( - PG.update(PrReviews) - .set({ postMergeStatus: status, updatedAt: toMs(now), postMergeAfter }) - .where(($) => [$.orgId.eq(orgId), $.id.eq(id)]), + transitionRow({ table: "pr_reviews", id, from: ["waiting"], to: status }, (from) => + database.execute((db) => + db.run( + PG.update(PrReviews) + .set({ postMergeStatus: status, updatedAt: toMs(now), postMergeAfter }) + .where(($) => [ + $.orgId.eq(orgId), + $.id.eq(id), + $.postMergeStatus.in_(...from), + $.postMergeAfter.eq(toMs(leaseUntil)), + ]) + .returning("id"), + ), ), ) - const later = (orgId: OrgId, id: PrReviewId, at: DateTime.Utc, now: DateTime.Utc) => - settle(orgId, id, "waiting", toMs(at), now) + const later = ( + orgId: OrgId, + id: PrReviewId, + leaseUntil: DateTime.Utc, + at: DateTime.Utc, + now: DateTime.Utc, + ) => settle(orgId, id, leaseUntil, "waiting", toMs(at), now) const examine = Effect.fn("PrReviewPostMergeService.examine")(function* ( row: DueRow, @@ -112,7 +131,7 @@ export class PrReviewPostMergeService extends Context.Service< const breaksOnly = facts !== undefined && services.length === 0 && openContractBreaks(facts).length > 0 if (facts === undefined || (services.length === 0 && !breaksOnly)) { - yield* settle(orgId, row.id, "no_traffic", null, now) + yield* settle(orgId, row.id, leaseUntil, "no_traffic", null, now) return "gave_up" satisfies Outcome } const versions = yield* telemetry.deploymentsSince(orgId, services, mergedAt, now) @@ -131,10 +150,10 @@ export class PrReviewPostMergeService extends Context.Service< }) if (picked === undefined) { if (elapsed(mergedAt, now, POST_MERGE_GIVE_UP)) { - yield* settle(orgId, row.id, "no_deploy", null, now) + yield* settle(orgId, row.id, leaseUntil, "no_deploy", null, now) return "gave_up" satisfies Outcome } - yield* later(orgId, row.id, DateTime.addDuration(now, POST_MERGE_RETRY), now) + yield* later(orgId, row.id, leaseUntil, DateTime.addDuration(now, POST_MERGE_RETRY), now) return "waiting" satisfies Outcome } const deployAt = picked.deploy.firstSeen @@ -143,7 +162,7 @@ export class PrReviewPostMergeService extends Context.Service< // A full window of traffic after the deploy, and its rollups landed. const readable = DateTime.addDuration(windowEnd, ROLLUP_LAG) if (DateTime.isLessThan(now, readable)) { - yield* later(orgId, row.id, readable, now) + yield* later(orgId, row.id, leaseUntil, readable, now) return "waiting" satisfies Outcome } @@ -233,6 +252,7 @@ export class PrReviewPostMergeService extends Context.Service< updatedAt: toMs(now), }) .where(($) => [ + $.orgId.eq(orgId), $.id.eq(row.id), $.postMergeStatus.eq("waiting"), $.postMergeAfter.eq(toMs(leaseUntil)), @@ -289,9 +309,9 @@ export class PrReviewPostMergeService extends Context.Service< ) const outcomes = yield* Effect.forEach( due, - (row) => - Effect.gen(function* () { - const leaseUntil = DateTime.addDuration(now, CLAIM) + (row) => { + const leaseUntil = DateTime.addDuration(now, CLAIM) + return Effect.gen(function* () { // Claimed first: an overlapping tick that read the same row finds it leased and // skips it, so the follow-up is posted once. const claimed = yield* database.execute((db) => @@ -307,7 +327,12 @@ export class PrReviewPostMergeService extends Context.Service< ), ) if (claimed.length === 0) return "skipped" as const - return yield* examine(row, now, leaseUntil) + return yield* examine(row, now, leaseUntil).pipe( + // A later tick took the row over; it settles or reports it instead. + Effect.catchTag("@maple/backend/TransitionConflictError", () => + Effect.succeed("skipped" as const), + ), + ) }).pipe( Effect.map((outcome): Outcome | "failed" | "skipped" => outcome), Effect.catchCause((cause) => { @@ -324,10 +349,11 @@ export class PrReviewPostMergeService extends Context.Service< }), Effect.andThen( (giveUp - ? settle(row.orgId, row.id, "failed", null, now) + ? settle(row.orgId, row.id, leaseUntil, "failed", null, now) : later( row.orgId, row.id, + leaseUntil, DateTime.addDuration(now, POST_MERGE_RETRY), now, ) @@ -336,7 +362,8 @@ export class PrReviewPostMergeService extends Context.Service< Effect.as("failed" as const), ) }), - ), + ) + }, { concurrency: 2 }, ) const count = (outcome: Outcome | "failed" | "skipped") => From df31aadf1205a1cd79d1b6fcbb90f47b71c36cb7 Mon Sep 17 00:00:00 2001 From: Makisuo Date: Fri, 6 Nov 2026 18:51:23 +0100 Subject: [PATCH 07/11] fix(backend): fence the onboarding reward finalize and release on the caller's reservation --- .../org/OnboardingChecklistService.ts | 6 +- .../services/org/OnboardingService.test.ts | 91 +++++++++++++++++++ .../src/services/org/OnboardingService.ts | 70 ++++++++++---- 3 files changed, 144 insertions(+), 23 deletions(-) create mode 100644 packages/backend/src/services/org/OnboardingService.test.ts diff --git a/packages/backend/src/services/org/OnboardingChecklistService.ts b/packages/backend/src/services/org/OnboardingChecklistService.ts index a20dbc26da..9759e2aafa 100644 --- a/packages/backend/src/services/org/OnboardingChecklistService.ts +++ b/packages/backend/src/services/org/OnboardingChecklistService.ts @@ -280,7 +280,7 @@ const make = Effect.gen(function* () { "reserveRewardClaim", onboarding.reserveRewardClaim(orgId, CLAIM_LEASE_MS), ) - if (!reserved) { + if (Option.isNone(reserved)) { return yield* new OnboardingRewardNotClaimableError({ message: "Another admin is claiming the reward right now. Try again in a moment.", reason: "in_progress", @@ -288,7 +288,7 @@ const make = Effect.gen(function* () { } const release = onboarding - .releaseRewardClaim(orgId) + .releaseRewardClaim(orgId, reserved.value) .pipe( Effect.catchCause((cause) => Effect.logError("Onboarding reward lease could not be released").pipe( @@ -350,7 +350,7 @@ const make = Effect.gen(function* () { ).pipe(Effect.annotateLogs({ orgId, error }), Effect.andThen(Effect.fail(error))), }), ) - yield* unavailable("finalizeRewardClaim", onboarding.finalizeRewardClaim(orgId)) + yield* unavailable("finalizeRewardClaim", onboarding.finalizeRewardClaim(orgId, reserved.value)) yield* edgeCache.invalidate({ bucket: CUSTOMER_CACHE_BUCKET, key: orgId }) yield* Effect.annotateCurrentSpan({ orgId, "maple.onboarding.reward_claimed": true }) diff --git a/packages/backend/src/services/org/OnboardingService.test.ts b/packages/backend/src/services/org/OnboardingService.test.ts new file mode 100644 index 0000000000..e0687a9511 --- /dev/null +++ b/packages/backend/src/services/org/OnboardingService.test.ts @@ -0,0 +1,91 @@ +import { afterEach, assert, describe, it } from "@effect/vitest" +import { Effect, Layer, Option, Schema } from "effect" +import { TestClock } from "effect/testing" +import { OrgId } from "@maple/domain/http" +import { + cleanupTestDbs, + createTestDb, + executeSql, + queryFirstRow, + type TestDb, +} from "@maple/backend/platform/test-pglite" +import { OnboardingService } from "@maple/backend/services/org/OnboardingService" + +const trackedDbs: TestDb[] = [] +afterEach(() => cleanupTestDbs(trackedDbs)) + +const ORG = Schema.decodeSync(OrgId)("org_reward_lease") +const LEASE_MS = 60_000 + +const layerFor = (testDb: TestDb) => OnboardingService.layer.pipe(Layer.provideMerge(testDb.layer)) + +const seedRow = (testDb: TestDb) => + Effect.promise(() => + executeSql( + testDb, + `INSERT INTO org_onboarding_state (org_id, demo_data_requested, created_at, updated_at) + VALUES ($1, false, now(), now())`, + [ORG], + ), + ) + +const rewardState = (testDb: TestDb) => + Effect.promise(() => + queryFirstRow<{ reward_reserved_at: Date | null; reward_claimed_at: Date | null }>( + testDb, + "SELECT reward_reserved_at, reward_claimed_at FROM org_onboarding_state WHERE org_id = $1", + [ORG], + ), + ) + +/** Holder A's lease lapses and holder B takes it over; returns both reservations. */ +const lapseAndTakeOver = Effect.gen(function* () { + const onboarding = yield* OnboardingService + const first = yield* onboarding.reserveRewardClaim(ORG, LEASE_MS) + yield* TestClock.adjust(`${LEASE_MS + 1} millis`) + const second = yield* onboarding.reserveRewardClaim(ORG, LEASE_MS) + return { first: Option.getOrThrow(first), second: Option.getOrThrow(second) } +}) + +describe("OnboardingService reward lease", () => { + it.effect("a holder whose lease lapsed cannot release the next holder's reservation", () => { + const testDb = createTestDb(trackedDbs) + return Effect.gen(function* () { + yield* seedRow(testDb) + const { first, second } = yield* lapseAndTakeOver + yield* (yield* OnboardingService).releaseRewardClaim(ORG, first) + + const row = yield* rewardState(testDb) + assert.strictEqual(row?.reward_reserved_at?.getTime(), second) + // The live reservation still blocks a third claimant. + assert.isTrue(Option.isNone(yield* (yield* OnboardingService).reserveRewardClaim(ORG, LEASE_MS))) + }).pipe(Effect.provide(layerFor(testDb))) + }) + + it.effect("a holder whose lease lapsed records its claim but keeps the next holder's lease", () => { + const testDb = createTestDb(trackedDbs) + return Effect.gen(function* () { + yield* seedRow(testDb) + const { first, second } = yield* lapseAndTakeOver + yield* (yield* OnboardingService).finalizeRewardClaim(ORG, first) + + const row = yield* rewardState(testDb) + assert.strictEqual(row?.reward_reserved_at?.getTime(), second) + assert.isNotNull(row?.reward_claimed_at) + }).pipe(Effect.provide(layerFor(testDb))) + }) + + it.effect("the current holder finalizes and releases its own lease", () => { + const testDb = createTestDb(trackedDbs) + return Effect.gen(function* () { + yield* seedRow(testDb) + const onboarding = yield* OnboardingService + const held = Option.getOrThrow(yield* onboarding.reserveRewardClaim(ORG, LEASE_MS)) + yield* onboarding.finalizeRewardClaim(ORG, held) + + const row = yield* rewardState(testDb) + assert.strictEqual(row?.reward_reserved_at, null) + assert.strictEqual(row?.reward_claimed_at?.getTime(), held) + }).pipe(Effect.provide(layerFor(testDb))) + }) +}) diff --git a/packages/backend/src/services/org/OnboardingService.ts b/packages/backend/src/services/org/OnboardingService.ts index 21b9ab0aa1..935c2e96d2 100644 --- a/packages/backend/src/services/org/OnboardingService.ts +++ b/packages/backend/src/services/org/OnboardingService.ts @@ -5,6 +5,7 @@ import type { OrgId } from "@maple/domain/http" import { Clock, Context, Effect, Layer, Option } from "effect" import { Database } from "@maple/backend/platform/DatabaseLive" import { makeDbExecute } from "@maple/backend/platform/db-execute" +import { transitionRow } from "@maple/backend/platform/transition-row" const toPersistenceError = (error: unknown) => new OnboardingPersistenceError({ @@ -204,7 +205,8 @@ export class OnboardingService extends Context.Service()( /** * Take the reward claim lease: stamps `rewardReservedAt` if the reward is not * claimed and no live reservation exists (a reservation older than `leaseMs` - * is a crashed attempt and may be taken over). Reports whether this call won. + * is a crashed attempt and may be taken over). Returns the reservation this call + * holds, or `None` when another holder has it. */ const reserveRewardClaim = Effect.fn("OnboardingService.reserveRewardClaim")(function* ( orgId: OrgId, @@ -223,39 +225,67 @@ export class OnboardingService extends Context.Service()( .returning(($) => ({ id: $.orgId })), ), ) - return result.length > 0 + return result.length > 0 ? Option.some(now) : Option.none() }) - /** Billing confirmed the credit: the claim is final and the lease is released. */ + /** A write that applies only while the org's lease is still the reservation the caller took. */ + const whileReserved = ( + orgId: OrgId, + reservedAt: number, + to: "claimed" | "released", + set: Partial, + ) => + transitionRow({ table: "org_onboarding_state", id: orgId, from: ["reserved"], to }, () => + dbExecute((db) => + db.run( + PG.update(OrgOnboardingState) + .set(set) + .where(($) => [$.orgId.eq(orgId), $.rewardReservedAt.eq(reservedAt)]) + .returning(($) => ({ id: $.orgId })), + ), + ), + ) + + /** + * Billing confirmed the credit: the claim is final and the caller's lease is released. + * A lease that lapsed under the call still records the claim, but leaves the next + * holder's reservation alone. + */ const finalizeRewardClaim = Effect.fn("OnboardingService.finalizeRewardClaim")(function* ( orgId: OrgId, + reservedAt: number, ) { const now = yield* Clock.currentTimeMillis - yield* dbExecute((db) => - db.run( - PG.update(OrgOnboardingState) - .set({ - rewardClaimedAt: now, - rewardReservedAt: null, - updatedAt: now, - }) - .where(($) => [$.orgId.eq(orgId)]), + yield* whileReserved(orgId, reservedAt, "claimed", { + rewardClaimedAt: now, + rewardReservedAt: null, + updatedAt: now, + }).pipe( + Effect.catchTag("@maple/backend/TransitionConflictError", () => + dbExecute((db) => + db.run( + PG.update(OrgOnboardingState) + .set({ rewardClaimedAt: now, updatedAt: now }) + .where(($) => [$.orgId.eq(orgId), $.rewardClaimedAt.isNull()]), + ), + ), ), ) }) - /** Billing refused before anything was applied: give the lease back so the org can retry now. */ + /** + * Billing refused before anything was applied: give the lease back so the org can retry + * now. A lease another holder already took over is theirs, and stays. + */ const releaseRewardClaim = Effect.fn("OnboardingService.releaseRewardClaim")(function* ( orgId: OrgId, + reservedAt: number, ) { const now = yield* Clock.currentTimeMillis - yield* dbExecute((db) => - db.run( - PG.update(OrgOnboardingState) - .set({ rewardReservedAt: null, updatedAt: now }) - .where(($) => [$.orgId.eq(orgId)]), - ), - ) + yield* whileReserved(orgId, reservedAt, "released", { + rewardReservedAt: null, + updatedAt: now, + }).pipe(Effect.catchTag("@maple/backend/TransitionConflictError", () => Effect.void)) }) const markEmailSent = Effect.fn("OnboardingService.markEmailSent")(function* ( From 16f441caf83debceb3839d99a375b5fca0ec4056 Mon Sep 17 00:00:00 2001 From: Makisuo Date: Fri, 6 Nov 2026 18:53:44 +0100 Subject: [PATCH 08/11] fix(backend): guard repository sync status writes on the expected states and an active repo --- .../integrations/vcs/VcsRepository.ts | 74 +++++++++++-------- .../integrations/vcs/VcsSyncService.ts | 22 +++++- .../integrations/vcs/__tests__/vcs.test.ts | 48 +++++++++++- 3 files changed, 108 insertions(+), 36 deletions(-) diff --git a/packages/backend/src/services/integrations/vcs/VcsRepository.ts b/packages/backend/src/services/integrations/vcs/VcsRepository.ts index 55be41cba6..42273c1c4b 100644 --- a/packages/backend/src/services/integrations/vcs/VcsRepository.ts +++ b/packages/backend/src/services/integrations/vcs/VcsRepository.ts @@ -44,6 +44,7 @@ import { import { Array as Arr, Clock, Context, Effect, Layer, Option, Schema } from "effect" import { Database, type DatabaseError } from "@maple/backend/platform/DatabaseLive" import { makeDbExecute } from "@maple/backend/platform/db-execute" +import { transitionRow } from "@maple/backend/platform/transition-row" import { deleteSourcesOf, upsertWithSource, @@ -215,10 +216,20 @@ interface UpsertInstallationInput { interface RepoSyncStatusUpdate { readonly status: VcsRepoSyncStatus + /** The states this writer expects; a repo another sync moved since is left alone. */ + readonly from: Arr.NonEmptyReadonlyArray readonly error?: string | null readonly syncedAt?: number | null } +/** Every sync status: a fresh sync start, or a provider verdict that holds whatever the walk state. */ +export const ANY_REPO_SYNC_STATUS: Arr.NonEmptyReadonlyArray = [ + "pending", + "backfilling", + "ready", + "error", +] + /** * Read scope for repository queries — a required argument so every caller must * consciously decide whether provider-removed repos are in scope. `"active"` @@ -784,51 +795,54 @@ export class VcsRepository extends Context.Service()( return true }) - // Write a sync-status transition. `last_synced_at` is touched ONLY when the - // caller passes `syncedAt` (i.e. a sync actually completed) — so marking a repo - // "backfilling" at the start of / mid a run never stamps a misleading sync time; - // it stays whatever the last *successful* sync left. + // Write a sync-status transition, guarded on the states the caller expects and on the + // repo still being active. `last_synced_at` is touched ONLY when the caller passes + // `syncedAt` (a sync actually completed). Reports whether the write applied. const updateRepoSyncStatus = Effect.fn("VcsRepository.updateRepoSyncStatus")(function* ( repositoryId: VcsRepositoryId, update: RepoSyncStatusUpdate, ) { const now = yield* Clock.currentTimeMillis - yield* dbExecute((db) => - db.run( - PG.update(VcsRepositories) - .set({ - syncStatus: update.status, - lastSyncError: update.error ?? null, - ...("syncedAt" in update - ? { lastSyncedAt: update.syncedAt ?? null } - : undefined), - updatedAt: now, - }) - .where(($) => [$.id.eq(repositoryId)]), - ), + const applied = yield* transitionRow( + { table: "vcs_repositories", id: repositoryId, from: update.from, to: update.status }, + (from) => + dbExecute((db) => + db.run( + PG.update(VcsRepositories) + .set({ + syncStatus: update.status, + lastSyncError: update.error ?? null, + ...("syncedAt" in update + ? { lastSyncedAt: update.syncedAt ?? null } + : undefined), + updatedAt: now, + }) + .where(($) => [ + $.id.eq(repositoryId), + $.status.eq("active"), + $.syncStatus.in_(...from), + ]) + .returning("id"), + ), + ), + ).pipe( + Effect.as(true), + Effect.catchTag("@maple/backend/TransitionConflictError", () => Effect.succeed(false)), ) // Only a settled sync speaks for the installation; a start or a backfill step does not. - if (update.status === "ready" || update.status === "error") { + if (applied && (update.status === "ready" || update.status === "error")) { yield* reportInstallationSync(repositoryId, now) } + return applied }) // Flag a single repo's sync as errored without touching its cursor / // last-synced time (a failed fetch must not wipe prior progress). - const markRepoSyncError = Effect.fn("VcsRepository.markRepoSyncError")(function* ( + const markRepoSyncError = ( repositoryId: VcsRepositoryId, message: string, - ) { - const now = yield* Clock.currentTimeMillis - yield* dbExecute((db) => - db.run( - PG.update(VcsRepositories) - .set({ syncStatus: "error", lastSyncError: message, updatedAt: now }) - .where(($) => [$.id.eq(repositoryId)]), - ), - ) - yield* reportInstallationSync(repositoryId, now) - }) + from: Arr.NonEmptyReadonlyArray, + ) => updateRepoSyncStatus(repositoryId, { status: "error", from, error: message }) // Persist commits for an already-resolved repository. Commits belong to the // repo only (no commit↔branch link — a repo tracks one branch at a time). diff --git a/packages/backend/src/services/integrations/vcs/VcsSyncService.ts b/packages/backend/src/services/integrations/vcs/VcsSyncService.ts index 06be1d8399..630162a8fa 100644 --- a/packages/backend/src/services/integrations/vcs/VcsSyncService.ts +++ b/packages/backend/src/services/integrations/vcs/VcsSyncService.ts @@ -38,7 +38,7 @@ import { PullRequestEventSink, } from "./PullRequestEventSink" import { VcsProviderRegistry } from "./VcsProviderRegistry" -import { VcsRepository } from "./VcsRepository" +import { ANY_REPO_SYNC_STATUS, VcsRepository } from "./VcsRepository" import { VcsSyncQueue } from "./VcsSyncQueue" import { WorkflowEventSink } from "./WorkflowEventSink" import { summarizeCause } from "@maple/backend/platform/describe-cause" @@ -197,7 +197,9 @@ export class VcsSyncService extends Context.Service Effect.logWarning("[VCS] Repository unavailable — backfill skipped").pipe( Effect.annotateLogs({ @@ -1277,6 +1292,7 @@ export class VcsSyncService extends Context.Service { const a = yield* repoFor(repo, orgId, "7") const b = yield* repoFor(repo, orgId, "8") - yield* repo.updateRepoSyncStatus(a.id, { status: "ready", syncedAt: 1 }) + yield* repo.updateRepoSyncStatus(a.id, { + status: "ready", + from: ANY_REPO_SYNC_STATUS, + syncedAt: 1, + }) const ok = yield* sourceState assert.isNotNull(ok.last_poll_ok_at) assert.isNull(ok.last_poll_error) assert.deepStrictEqual(ok.poll_resources, { total: 2, unhealthy: 0 }) - yield* repo.markRepoSyncError(b.id, "upstream unavailable") + yield* repo.markRepoSyncError(b.id, "upstream unavailable", ANY_REPO_SYNC_STATUS) const failed = yield* sourceState assert.strictEqual(failed.last_poll_error, "octo/b: upstream unavailable") assert.deepStrictEqual(failed.poll_resources, { total: 2, unhealthy: 1 }) @@ -1751,6 +1755,44 @@ describe("VcsRepository", () => { }).pipe(Effect.provide(repoLayer(testDb))) }) + it.effect("an overlapping sync cannot regress a settled repo or write a removed one", () => { + const testDb = createTestDb(trackedDbs) + return Effect.gen(function* () { + const repo = yield* VcsRepository + const orgId = asOrgId("org_sync_race") + yield* repo.upsertInstallation({ orgId, ...installationSeed("42", "100") }) + yield* upsertReposFor(repo, "42", [ + repoFixture({ externalRepoId: "7", fullName: "octo/a", name: "a" }), + repoFixture({ externalRepoId: "8", fullName: "octo/b", name: "b" }), + ]) + const a = yield* repoFor(repo, orgId, "7") + const b = yield* repoFor(repo, orgId, "8") + + // Walks A and B both start; B finishes first. + yield* repo.updateRepoSyncStatus(a.id, { status: "backfilling", from: ANY_REPO_SYNC_STATUS }) + yield* repo.updateRepoSyncStatus(a.id, { status: "ready", from: ["backfilling"], syncedAt: 5 }) + // A's continuation checkpoint and its exhausted-retries error both find the repo settled. + const checkpoint = yield* repo.updateRepoSyncStatus(a.id, { + status: "backfilling", + from: ["backfilling"], + }) + yield* repo.markRepoSyncError(a.id, "backfill failed", ["pending", "backfilling"]) + assert.isFalse(checkpoint) + const settled = yield* repoFor(repo, orgId, "7") + assert.strictEqual(settled.syncStatus, "ready") + assert.strictEqual(settled.lastSyncError, null) + + yield* repo.markRepositoryRemoved(b.id) + const written = yield* repo.updateRepoSyncStatus(b.id, { + status: "backfilling", + from: ANY_REPO_SYNC_STATUS, + }) + assert.isFalse(written) + const removed = Option.getOrThrow(yield* repo.getRepositoryById(orgId, b.id)) + assert.strictEqual(removed.syncStatus, "pending") + }).pipe(Effect.provide(repoLayer(testDb))) + }) + // A SHA persisted in any case is normalized to lowercase (the GitCommitSha decode // lowercases on the write path), so an uppercase push and a lowercase lookup never // split into two rows. From bb7b3c850bd353f8f2a59d412060d636f2a675f4 Mon Sep 17 00:00:00 2001 From: Makisuo Date: Fri, 6 Nov 2026 18:56:39 +0100 Subject: [PATCH 09/11] fix(backend): guard recommendation reconcile on the status it read; dismiss stamps resolvedAt --- .../errors/RecommendationIssueService.test.ts | 112 ++++++++++++++++++ .../errors/RecommendationIssueService.ts | 28 +++-- .../src/tables/org-recommendation-issues.ts | 3 +- 3 files changed, 134 insertions(+), 9 deletions(-) create mode 100644 packages/backend/src/services/errors/RecommendationIssueService.test.ts diff --git a/packages/backend/src/services/errors/RecommendationIssueService.test.ts b/packages/backend/src/services/errors/RecommendationIssueService.test.ts new file mode 100644 index 0000000000..fab3a5e0e7 --- /dev/null +++ b/packages/backend/src/services/errors/RecommendationIssueService.test.ts @@ -0,0 +1,112 @@ +import { afterEach, assert, describe, it } from "@effect/vitest" +import { Effect, Layer, Schema } from "effect" +import { OrgId, RecommendationIssueId, UserId } from "@maple/domain/http" +import { Database } from "@maple/backend/platform/DatabaseLive" +import { + cleanupTestDbs, + createTestDb, + executeSql, + queryFirstRow, + type TestDb, +} from "@maple/backend/platform/test-pglite" +import type { TenantContext } from "@maple/backend/services/auth/AuthService" +import { WarehouseQueryService } from "@maple/backend/services/warehouse/WarehouseQueryService" +import { makeWarehouseServiceStub } from "@maple/backend/testing/warehouse-test-support" +import { RecommendationIssueService } from "./RecommendationIssueService" + +const trackedDbs: TestDb[] = [] +afterEach(() => cleanupTestDbs(trackedDbs)) + +const ORG = Schema.decodeSync(OrgId)("org_recommendations") +const ISSUE = Schema.decodeSync(RecommendationIssueId)("3f9a1b2c-0d4e-4f5a-8b6c-7d8e9f0a1b2c") +const tenant: TenantContext = { + orgId: ORG, + userId: Schema.decodeSync(UserId)("user_recommendations"), + roles: ["org:admin"], + authMode: "self_hosted", +} + +// The key is no longer seen in span telemetry, so reconcile plans the open issue to `resolved`. +const noSpanKeys = makeWarehouseServiceStub({ compiledQuery: () => Effect.succeed([]) }) + +/** + * The real PGlite database, except that `onCall` runs just before the given `execute` call: + * a write that lands between reconcile's read and its updates. + */ +const racingDatabase = (testDb: TestDb, call: number, onCall: Effect.Effect) => + Layer.effect( + Database, + Effect.gen(function* () { + const inner = yield* Database + let calls = 0 + return { + execute: (fn) => + Effect.suspend(() => { + calls += 1 + return calls === call + ? onCall.pipe(Effect.andThen(inner.execute(fn))) + : inner.execute(fn) + }), + } satisfies typeof inner + }), + ).pipe(Layer.provide(testDb.layer)) + +const layerFor = (testDb: TestDb, database: Layer.Layer) => + Layer.effect(RecommendationIssueService, RecommendationIssueService.make).pipe( + Layer.provide(database), + Layer.provide(Layer.succeed(WarehouseQueryService, noSpanKeys)), + ) + +const seedOpenIssue = (testDb: TestDb) => + Effect.promise(() => + executeSql( + testDb, + `INSERT INTO org_recommendation_issues (id, org_id, number, recommendation_key, kind, source_key, canonical_key, status, usage_count) + VALUES ($1, $2, 1, 'rename:http.status_code', 'rename', 'http.status_code', 'http.response.status_code', 'open', 10)`, + [ISSUE, ORG], + ), + ) + +const issueState = (testDb: TestDb) => + Effect.promise(() => + queryFirstRow<{ status: string; resolved_at: Date | null }>( + testDb, + "SELECT status, resolved_at FROM org_recommendation_issues WHERE id = $1", + [ISSUE], + ), + ) + +describe("RecommendationIssueService reconcile", () => { + it.effect("a dismissal that lands after reconcile read the issue stays dismissed", () => { + const testDb = createTestDb(trackedDbs) + const dismissMidReconcile = Effect.promise(() => + executeSql(testDb, "UPDATE org_recommendation_issues SET status = 'dismissed' WHERE id = $1", [ + ISSUE, + ]), + ).pipe(Effect.asVoid) + // Calls: the mappings read, the issues read, then the planned status update. + const database = racingDatabase(testDb, 3, dismissMidReconcile) + return Effect.gen(function* () { + yield* seedOpenIssue(testDb) + const list = yield* (yield* RecommendationIssueService).listReconciled(tenant) + assert.strictEqual(list.issues[0]?.status, "dismissed") + assert.strictEqual((yield* issueState(testDb))?.status, "dismissed") + }).pipe(Effect.provide(layerFor(testDb, database))) + }) + + it.effect("dismiss closes the issue with a resolvedAt, like reconcile's resolve", () => { + const testDb = createTestDb(trackedDbs) + return Effect.gen(function* () { + yield* seedOpenIssue(testDb) + const service = yield* RecommendationIssueService + yield* service.dismiss(tenant, ISSUE) + assert.isNotNull((yield* issueState(testDb))?.resolved_at) + + yield* service.reopen(tenant, ISSUE) + yield* service.listReconciled(tenant) + const resolved = yield* issueState(testDb) + assert.strictEqual(resolved?.status, "resolved") + assert.isNotNull(resolved?.resolved_at) + }).pipe(Effect.provide(layerFor(testDb, testDb.layer))) + }) +}) diff --git a/packages/backend/src/services/errors/RecommendationIssueService.ts b/packages/backend/src/services/errors/RecommendationIssueService.ts index 0aa5663da4..f72800eb31 100644 --- a/packages/backend/src/services/errors/RecommendationIssueService.ts +++ b/packages/backend/src/services/errors/RecommendationIssueService.ts @@ -8,7 +8,6 @@ import { RecommendationIssueNotFoundError, RecommendationIssuePersistenceError, RecommendationIssuesListResponse, - RecommendationIssueStatus, } from "@maple/domain/http" import { canonicalKeysToProbe, @@ -54,7 +53,6 @@ export interface RecommendationIssueServiceApi { const decodeIssueIdSync = Schema.decodeUnknownSync(RecommendationIssueId) const decodeKindSync = Schema.decodeUnknownSync(RecommendationIssueKind) -const decodeStatusSync = Schema.decodeUnknownSync(RecommendationIssueStatus) const decodeIsoSync = Schema.decodeUnknownSync(IsoDateTimeString) const toPersistenceError = (error: DatabaseError) => @@ -68,7 +66,7 @@ const rowToIssue = (row: IssueRow): RecommendationIssue => kind: decodeKindSync(row.kind), sourceKey: row.sourceKey, ...(row.canonicalKey != null ? { canonicalKey: row.canonicalKey } : undefined), - status: decodeStatusSync(row.status), + status: row.status, usageCount: row.usageCount, openedAt: decodeIsoSync(new Date(row.openedAt).toISOString()), updatedAt: decodeIsoSync(new Date(row.updatedAt).toISOString()), @@ -185,14 +183,15 @@ export class RecommendationIssueService extends Context.Service< number: row.number, recommendationKey: row.recommendationKey, sourceKey: row.sourceKey, - status: decodeStatusSync(row.status), + status: row.status, })) const plan = planReconcileIssues(detected, existingLike, mappingSourceKeys) + const readStatus = new Map(existing.map((row) => [row.id, row.status] as const)) const now = yield* Clock.currentTimeMillis if (plan.inserts.length > 0) { - const rows = plan.inserts.map((insert) => ({ + const rows = plan.inserts.map((insert): OrgRecommendationIssueInsert => ({ id: decodeIssueIdSync(randomUUID()), orgId, number: insert.number, @@ -206,7 +205,10 @@ export class RecommendationIssueService extends Context.Service< updatedAt: now, resolvedAt: null, })) - yield* dbExecute((db) => db.run(PG.insertInto(OrgRecommendationIssues).values(rows))) + // A concurrent list already opened the same key; its row stands. + yield* dbExecute((db) => + db.run(PG.insertInto(OrgRecommendationIssues).values(rows).onConflictDoNothing()), + ) } yield* Effect.forEach( @@ -222,11 +224,18 @@ export class RecommendationIssueService extends Context.Service< } : undefined), } + // A status move applies only from the status the plan read: a dismissal or reopen + // that landed since is the user's, and stands. + const from = update.nextStatus === undefined ? undefined : readStatus.get(update.id) return dbExecute((db) => db.run( PG.update(OrgRecommendationIssues) .set(fields) - .where(($) => [$.orgId.eq(orgId), $.id.eq(update.id)]), + .where(($) => [ + $.orgId.eq(orgId), + $.id.eq(update.id), + ...(from === undefined ? [] : [$.status.eq(from)]), + ]), ), ) }, @@ -272,8 +281,11 @@ export class RecommendationIssueService extends Context.Service< return yield* listResponse(orgId) }) + // Dismissal closes the issue like reconcile's applied/resolved do, so it stamps resolvedAt too. const dismiss = (tenant: TenantContext, id: RecommendationIssueId) => - setStatus(tenant, id, { status: "dismissed" }) + Clock.currentTimeMillis.pipe( + Effect.flatMap((now) => setStatus(tenant, id, { status: "dismissed", resolvedAt: now })), + ) const reopen = (tenant: TenantContext, id: RecommendationIssueId) => setStatus(tenant, id, { status: "open", resolvedAt: null }) diff --git a/packages/db/src/tables/org-recommendation-issues.ts b/packages/db/src/tables/org-recommendation-issues.ts index 479a043493..2fd0660a48 100644 --- a/packages/db/src/tables/org-recommendation-issues.ts +++ b/packages/db/src/tables/org-recommendation-issues.ts @@ -1,4 +1,5 @@ import * as PG from "@maple-dev/effect-orm/postgres" +import { RecommendationIssueStatus } from "@maple/domain/http" import { OrgId } from "@maple/domain/primitives" // Durable, numbered attribute-recommendation issues (PlanetScale-style). Recommendations are @@ -21,7 +22,7 @@ export const OrgRecommendationIssues = PG.table("org_recommendation_issues", { kind: PG.text, sourceKey: PG.column(PG.text, { name: "source_key" }), canonicalKey: PG.column(PG.nullable(PG.text), { name: "canonical_key" }), - status: PG.column(PG.text, { default: "open" }), + status: PG.column(PG.brand(PG.text, RecommendationIssueStatus), { default: "open" }), usageCount: PG.column(PG.int4, { name: "usage_count", default: 0 }), openedAt: PG.column(PG.timestamptzMillis, { name: "opened_at", defaultExpr: "now()" }), updatedAt: PG.column(PG.timestamptzMillis, { name: "updated_at", defaultExpr: "now()" }), From 0b0242fdcb0102a0ffbddac6913e879635cbeb6b Mon Sep 17 00:00:00 2001 From: Makisuo Date: Fri, 6 Nov 2026 18:58:10 +0100 Subject: [PATCH 10/11] fix(backend): settle dependency scans abandoned in running as failed --- .../dependency-scan/DependencyScanStore.ts | 32 +++++++++++++++++++ .../dependency-scan/DependencyScanner.test.ts | 26 +++++++++++++++ .../dependency-scan/DependencyScanner.ts | 18 +++++++++++ 3 files changed, 76 insertions(+) diff --git a/packages/backend/src/services/dependency-scan/DependencyScanStore.ts b/packages/backend/src/services/dependency-scan/DependencyScanStore.ts index cf2d64636d..fdda390aec 100644 --- a/packages/backend/src/services/dependency-scan/DependencyScanStore.ts +++ b/packages/backend/src/services/dependency-scan/DependencyScanStore.ts @@ -106,6 +106,8 @@ export interface SetFindingStatusInput { readonly actor: UserId } +const ABANDONED_SCAN_ERROR = "The scan stopped before it finished" + export interface DependencyScanStoreApi { readonly startScan: ( input: StartScanInput, @@ -117,6 +119,15 @@ export interface DependencyScanStoreApi { DependencyScanRow, DependencyScanPersistenceError | DependencyScanNotFoundError | DependencyScanNotRunningError > + /** + * Settles a repository's scans still `running` past `startedBefore` as failed: their worker + * died. A scan that settled first is left as it is. Returns how many were failed. + */ + readonly failAbandonedScans: (input: { + readonly orgId: OrgId + readonly repositoryId: VcsRepositoryId + readonly startedBefore: number + }) => Effect.Effect readonly failScan: (input: { readonly orgId: OrgId readonly scanId: DependencyScanId @@ -589,6 +600,26 @@ export class DependencyScanStore extends Context.Service + db.run( + PG.update(DependencyScans) + .set({ status: "failed", error: ABANDONED_SCAN_ERROR, finishedAt: now }) + .where(($) => [ + $.orgId.eq(input.orgId), + $.repositoryId.eq(input.repositoryId), + $.status.eq("running"), + $.startedAt.lt(input.startedBefore), + ]) + .returning("id"), + ), + ) + return failed.length + }) + const failScan: DependencyScanStoreApi["failScan"] = Effect.fn("DependencyScanStore.failScan")( function* (input) { const now = yield* Clock.currentTimeMillis @@ -818,6 +849,7 @@ export class DependencyScanStore extends Context.Service { }).pipe(Effect.provide(scannerFor(state))) }) + it.effect("settles a scan abandoned in running as failed when the repository is scanned again", () => { + const state = world({ "package-lock.json": packageLock("4.17.15") }) + return Effect.gen(function* () { + const scanner = yield* DependencyScanner + const store = yield* DependencyScanStore + // A worker that died mid-scan: nothing ever settled its row. + const abandoned = yield* store.startScan({ + orgId: repository.orgId, + repositoryId: repository.id, + commitSha: sha("9"), + ref: "main", + trigger: "push", + defaultBranchHead: true, + }) + yield* TestClock.adjust(`${RESCAN_AFTER_MS + 1} millis`) + yield* scanner.scan(installation, repository, { trigger: "schedule" }) + + const scans = yield* store.listScans({ orgId: repository.orgId, limit: 10 }) + const settled = Arr.findFirst(scans, (scan) => scan.id === abandoned.id) + assert.strictEqual(Option.getOrUndefined(settled)?.status, "failed") + assert.isFalse(Arr.some(scans, (scan) => scan.status === "running")) + }).pipe(Effect.provide(scannerFor(state))) + }) + it.effect( "fails a scan whose tree listing was cut short instead of fixing the unlisted lockfiles' findings", () => { diff --git a/packages/backend/src/services/dependency-scan/DependencyScanner.ts b/packages/backend/src/services/dependency-scan/DependencyScanner.ts index d38e8ce0f1..16f1b3fcf5 100644 --- a/packages/backend/src/services/dependency-scan/DependencyScanner.ts +++ b/packages/backend/src/services/dependency-scan/DependencyScanner.ts @@ -306,6 +306,24 @@ export class DependencyScanner extends Context.Service (cause: unknown) => new DependencyScanError({ message, repositoryId: repository.id, cause }) + // A scan whose worker died stays `running`; each new scan of the repo settles those. + const startedAt = yield* Clock.currentTimeMillis + yield* store + .failAbandonedScans({ + orgId, + repositoryId: repository.id, + startedBefore: startedAt - RESCAN_AFTER_MS, + }) + .pipe( + Effect.catchTag( + "@maple/dependency-scan/DependencyScanPersistenceError", + (error) => + Effect.logWarning( + "Could not settle abandoned dependency scans", + error.message, + ), + ), + ) const provider = yield* sources .resolve(repository.provider) .pipe(Effect.mapError(scanError("Unknown VCS provider"))) From f352be6355b116e05f538c6192bd68e4577293f3 Mon Sep 17 00:00:00 2001 From: Makisuo Date: Fri, 6 Nov 2026 19:03:56 +0100 Subject: [PATCH 11/11] fix: fence the schema-version stamp, test the heartbeat, tighten review findings --- .../ClickHouseSchemaApplyWorkflow.run.test.ts | 61 ++++++++++++++++++- .../ClickHouseSchemaApplyWorkflow.run.ts | 9 ++- .../errors/RecommendationIssueService.test.ts | 19 ++++-- .../src/services/org/OnboardingService.ts | 49 ++++++--------- .../org/OrgClickHouseSettingsService.ts | 2 +- 5 files changed, 98 insertions(+), 42 deletions(-) diff --git a/apps/api/src/workflows/ClickHouseSchemaApplyWorkflow.run.test.ts b/apps/api/src/workflows/ClickHouseSchemaApplyWorkflow.run.test.ts index bae03f4826..c9d03837b5 100644 --- a/apps/api/src/workflows/ClickHouseSchemaApplyWorkflow.run.test.ts +++ b/apps/api/src/workflows/ClickHouseSchemaApplyWorkflow.run.test.ts @@ -1,6 +1,9 @@ import { afterEach, assert, describe, it } from "@effect/vitest" import * as Cloudflare from "@alchemy.run/cloudflare" -import { Cause, Effect, Exit, Layer, Schema, Array as Arr } from "effect" +import { Cause, Effect, Exit, Fiber, Layer, Schema, Array as Arr } from "effect" +import { TestClock } from "effect/testing" +import { Database } from "@maple/backend/platform/DatabaseLive" +import { OrgId } from "@maple/domain/http" import { FetchHttpClient } from "effect/http" import { @@ -12,7 +15,11 @@ import { } from "@maple/backend/platform/test-pglite" import { durableStep } from "@maple/backend/platform/durable-step" import { workerEnvLayer } from "@maple/infra/worker-runtime" -import { loadOptionalFeatureState, runClickHouseSchemaApply } from "./ClickHouseSchemaApplyWorkflow.run" +import { + loadOptionalFeatureState, + runClickHouseSchemaApply, + withRunHeartbeat, +} from "./ClickHouseSchemaApplyWorkflow.run" /** The step's Effect as `task` hands it over, with the body context already provided. */ const stepEffect = (options: Cloudflare.WorkflowTaskOptions): Effect.Effect => @@ -226,3 +233,53 @@ describe("runClickHouseSchemaApply failure bookkeeping", () => { }), ) }) + +describe("withRunHeartbeat", () => { + const trackedDbs: TestDb[] = [] + afterEach(() => cleanupTestDbs(trackedDbs)) + const ORG = Schema.decodeSync(OrgId)("org_wf_heartbeat") + + const updatedAt = (testDb: TestDb) => + Effect.promise(() => + queryFirstRow<{ updated_at: Date }>( + testDb, + "SELECT updated_at FROM org_clickhouse_schema_apply_runs WHERE org_id = $1", + [ORG], + ), + ).pipe(Effect.map((row) => row?.updated_at.getTime())) + + it.effect("keeps a long statement's run fresh, and abandons it once the run is reclaimed", () => { + const testDb = createTestDb(trackedDbs) + return Effect.gen(function* () { + yield* Effect.promise(() => + executeSql( + testDb, + `INSERT INTO org_clickhouse_schema_apply_runs (org_id, workflow_instance_id, status, created_at, updated_at) + VALUES ($1, 'run-1', 'running', to_timestamp(0), to_timestamp(0))`, + [ORG], + ), + ) + const fence = { database: yield* Database, orgId: ORG, runId: "run-1" } + // A backfill chunk that outlasts the staleness window. + const fiber = yield* withRunHeartbeat(fence, Effect.never).pipe(Effect.forkChild) + + yield* TestClock.adjust("5 minutes") + assert.strictEqual(yield* updatedAt(testDb), 5 * 60_000) + + yield* Effect.promise(() => + executeSql( + testDb, + "UPDATE org_clickhouse_schema_apply_runs SET workflow_instance_id = 'run-2' WHERE org_id = $1", + [ORG], + ), + ) + yield* TestClock.adjust("5 minutes") + const exit = yield* Fiber.await(fiber) + assert.isTrue(Exit.isFailure(exit)) + if (Exit.isFailure(exit)) { + assert.include(String(Cause.squash(exit.cause)), "SchemaApplyRunSuperseded") + } + assert.strictEqual(yield* updatedAt(testDb), 5 * 60_000) + }).pipe(Effect.provide(testDb.layer)) + }) +}) diff --git a/apps/api/src/workflows/ClickHouseSchemaApplyWorkflow.run.ts b/apps/api/src/workflows/ClickHouseSchemaApplyWorkflow.run.ts index 1f29ce2bc6..750d2d4014 100644 --- a/apps/api/src/workflows/ClickHouseSchemaApplyWorkflow.run.ts +++ b/apps/api/src/workflows/ClickHouseSchemaApplyWorkflow.run.ts @@ -122,6 +122,7 @@ const causeMessage = (cause: Cause.Cause): string => { /** An optional step may be skipped, but a lost fence must still stop the run. */ const isSkippable = (cause: Cause.Cause): boolean => + !Cause.hasInterrupts(cause) && !Arr.some( cause.reasons, (reason) => @@ -425,7 +426,7 @@ type RunPatch = Partial<{ type RunStatus = "queued" | "running" | "succeeded" | "failed" /** The run row this instance owns: the org's row while it still carries the claim's run id. */ -interface RunFence { +export interface RunFence { readonly database: DatabaseApi readonly orgId: OrgId readonly runId: string @@ -476,9 +477,9 @@ const RUN_HEARTBEAT = "5 minutes" /** * Runs one long statement (a backfill chunk) while touching the run row, so a live run never - * reads as stale. Losing the fence ends the statement; a failed touch is retried next beat. + * reads as stale. Losing the fence abandons the request (the server may still finish it). */ -const withRunHeartbeat = (fence: RunFence, statement: Effect.Effect) => +export const withRunHeartbeat = (fence: RunFence, statement: Effect.Effect) => Effect.raceFirst( statement, Effect.sleep(RUN_HEARTBEAT).pipe( @@ -771,6 +772,8 @@ const applySchema = (fence: RunFence) => yield* durableStep( "stamp-correctness", Effect.gen(function* () { + // Only the run that still owns the row speaks for the org's settings. + yield* updateRun(fence, { phase: "stamping schema version" }) const stampedAt = yield* Clock.currentTimeMillis yield* database.execute((db) => db.run( diff --git a/packages/backend/src/services/errors/RecommendationIssueService.test.ts b/packages/backend/src/services/errors/RecommendationIssueService.test.ts index fab3a5e0e7..196993880f 100644 --- a/packages/backend/src/services/errors/RecommendationIssueService.test.ts +++ b/packages/backend/src/services/errors/RecommendationIssueService.test.ts @@ -79,16 +79,25 @@ const issueState = (testDb: TestDb) => describe("RecommendationIssueService reconcile", () => { it.effect("a dismissal that lands after reconcile read the issue stays dismissed", () => { const testDb = createTestDb(trackedDbs) - const dismissMidReconcile = Effect.promise(() => - executeSql(testDb, "UPDATE org_recommendation_issues SET status = 'dismissed' WHERE id = $1", [ - ISSUE, - ]), - ).pipe(Effect.asVoid) + // Reconcile has read the issue and not yet written it: it must still be open here. + const dismissMidReconcile = Effect.gen(function* () { + assert.strictEqual((yield* issueState(testDb))?.status, "open") + yield* Effect.promise(() => + executeSql( + testDb, + "UPDATE org_recommendation_issues SET status = 'dismissed' WHERE id = $1", + [ISSUE], + ), + ) + injected = true + }) + let injected = false // Calls: the mappings read, the issues read, then the planned status update. const database = racingDatabase(testDb, 3, dismissMidReconcile) return Effect.gen(function* () { yield* seedOpenIssue(testDb) const list = yield* (yield* RecommendationIssueService).listReconciled(tenant) + assert.isTrue(injected) assert.strictEqual(list.issues[0]?.status, "dismissed") assert.strictEqual((yield* issueState(testDb))?.status, "dismissed") }).pipe(Effect.provide(layerFor(testDb, database))) diff --git a/packages/backend/src/services/org/OnboardingService.ts b/packages/backend/src/services/org/OnboardingService.ts index 935c2e96d2..0820b08d67 100644 --- a/packages/backend/src/services/org/OnboardingService.ts +++ b/packages/backend/src/services/org/OnboardingService.ts @@ -5,7 +5,6 @@ import type { OrgId } from "@maple/domain/http" import { Clock, Context, Effect, Layer, Option } from "effect" import { Database } from "@maple/backend/platform/DatabaseLive" import { makeDbExecute } from "@maple/backend/platform/db-execute" -import { transitionRow } from "@maple/backend/platform/transition-row" const toPersistenceError = (error: unknown) => new OnboardingPersistenceError({ @@ -228,23 +227,16 @@ export class OnboardingService extends Context.Service()( return result.length > 0 ? Option.some(now) : Option.none() }) - /** A write that applies only while the org's lease is still the reservation the caller took. */ - const whileReserved = ( - orgId: OrgId, - reservedAt: number, - to: "claimed" | "released", - set: Partial, - ) => - transitionRow({ table: "org_onboarding_state", id: orgId, from: ["reserved"], to }, () => - dbExecute((db) => - db.run( - PG.update(OrgOnboardingState) - .set(set) - .where(($) => [$.orgId.eq(orgId), $.rewardReservedAt.eq(reservedAt)]) - .returning(($) => ({ id: $.orgId })), - ), + /** Applies `set` only while the org's lease is still the reservation the caller took. */ + const whileReserved = (orgId: OrgId, reservedAt: number, set: Partial) => + dbExecute((db) => + db.run( + PG.update(OrgOnboardingState) + .set(set) + .where(($) => [$.orgId.eq(orgId), $.rewardReservedAt.eq(reservedAt)]) + .returning(($) => ({ id: $.orgId })), ), - ) + ).pipe(Effect.map((rows) => rows.length > 0)) /** * Billing confirmed the credit: the claim is final and the caller's lease is released. @@ -256,19 +248,17 @@ export class OnboardingService extends Context.Service()( reservedAt: number, ) { const now = yield* Clock.currentTimeMillis - yield* whileReserved(orgId, reservedAt, "claimed", { + const held = yield* whileReserved(orgId, reservedAt, { rewardClaimedAt: now, rewardReservedAt: null, updatedAt: now, - }).pipe( - Effect.catchTag("@maple/backend/TransitionConflictError", () => - dbExecute((db) => - db.run( - PG.update(OrgOnboardingState) - .set({ rewardClaimedAt: now, updatedAt: now }) - .where(($) => [$.orgId.eq(orgId), $.rewardClaimedAt.isNull()]), - ), - ), + }) + if (held) return + yield* dbExecute((db) => + db.run( + PG.update(OrgOnboardingState) + .set({ rewardClaimedAt: now, updatedAt: now }) + .where(($) => [$.orgId.eq(orgId), $.rewardClaimedAt.isNull()]), ), ) }) @@ -282,10 +272,7 @@ export class OnboardingService extends Context.Service()( reservedAt: number, ) { const now = yield* Clock.currentTimeMillis - yield* whileReserved(orgId, reservedAt, "released", { - rewardReservedAt: null, - updatedAt: now, - }).pipe(Effect.catchTag("@maple/backend/TransitionConflictError", () => Effect.void)) + yield* whileReserved(orgId, reservedAt, { rewardReservedAt: null, updatedAt: now }) }) const markEmailSent = Effect.fn("OnboardingService.markEmailSent")(function* ( diff --git a/packages/backend/src/services/org/OrgClickHouseSettingsService.ts b/packages/backend/src/services/org/OrgClickHouseSettingsService.ts index 9ccf041e3c..ded2f4e9e9 100644 --- a/packages/backend/src/services/org/OrgClickHouseSettingsService.ts +++ b/packages/backend/src/services/org/OrgClickHouseSettingsService.ts @@ -1302,7 +1302,7 @@ export class OrgClickHouseSettingsService extends Context.Service< // concurrent applySchema calls wins (interleaved workflow instances // would race destructive DROP/TRUNCATE/backfill migrations). A // queued/running row that stopped updating for STALE_APPLY_RUN_MS is - // reclaimable — the workflow stamps progress on every step and heartbeats + // reclaimable: the workflow stamps progress on every step and heartbeats // long ones, so a silent stall that long means the instance died. const now = yield* Clock.currentTimeMillis // The run id fences every progress write: an instance reclaimed as stale finds it