B2 Phase 1 follow-up: plug lease orphan gap in advanceTaskStatus + scope interruptStaleRuns cleanup
Linus review T1 surfaced three real bugs in the B2 Phase 1 shadow
dual-write surface. Fix them in one pass.
1. advanceTaskStatus left task_leases behind on terminal (done /
cancelled / blocked) and "return to queue" (todo / backlog)
transitions. The status->phase trigger advanced tasks.phase but the
lease row lingered as an orphan. dev-mode verifyPhaseConsistency
could see the divergence; production silently drifted. Wrap the
status UPDATE and an explicit DELETE FROM task_leases in a single
transaction so no caller observes the intermediate state.
2. The no-restricted-syntax guard in eslint.config.mjs only matched
literals shaped "UPDATE tasks SET status" -- advanceTaskStatus's
dynamic `UPDATE tasks SET ${timestampSets.join(", ")}` slipped past,
and so did saveTask / recoverStaleLocks (status not first column).
Tighten the selectors to (a) match any status-write within UPDATE
tasks regardless of column order, and (b) flag template literals
whose head ends with `SET ` so dynamic helpers must carry an
explicit eslint-disable with rationale. Five grandfathered sites
now carry matching disables.
3. interruptStaleRuns scoped its task+lease cleanup with
`WHERE finished_at = ?` against the freshly stamped timestamp --
safe in isolation, vulnerable to ISO-millisecond ties with other
already-interrupted runs. Capture the stale run ids before
mutating anything and drive every downstream statement from that
explicit id list. Original semantics preserved; scoping is exact.
Unit tests added in tests/unit/tasks-phase.test.ts cover the
advanceTaskStatus lease cleanup across done / cancelled / blocked /
todo, and pin the in_review case as "lease stays".
This commit is contained in:
1 parent
ded707d960
commit
54a97f67ac
5 files changed
+215
-25
No files matched your search
@@ -70,6 +70,7 @@ export function buildTaskCrudMethods(
|
||||
if (existing) {
|
||||
getDb()
|
||||
.prepare(
|
||||
// eslint-disable-next-line no-restricted-syntax -- B2 Phase 1 grandfathered: saveTask upsert path writes the full row (including status) as a single atomic UPDATE. The status→phase trigger keeps the shadow phase column in sync; no lease is ever held by saveTask callers (lease writes live behind checkoutTask).
|
||||
"update tasks set project_id = ?, goal_id = ?, parent_id = ?, title = ?, description = ?, assignee_agent_id = ?, workspace_id = ?, priority = ?, status = ?, updated_at = ? where id = ? and company_id = ?",
|
||||
)
|
||||
.run(
|
||||
|
||||
@@ -43,12 +43,26 @@ function writeCheckoutLease(
|
||||
export function buildTaskStatusMethods(
|
||||
getDb: () => DatabaseSync,
|
||||
readAll: ReadAll,
|
||||
_transaction: Transaction,
|
||||
transaction: Transaction,
|
||||
deps: {
|
||||
addComment: (input: { companyId: string; taskId: string; authorAgentId: string | null; authorName: string; body: string }) => void;
|
||||
addActivity: (input: { companyId: string; actor: string; action: string; entityType: string; entityId: string; detail: string }) => void;
|
||||
},
|
||||
) {
|
||||
// Terminal phases and "returned to queue" phases release any existing
|
||||
// lease: once a task is done/cancelled/blocked or bounced back to
|
||||
// todo/backlog the execution grant no longer has a referent. We mirror
|
||||
// that in `task_leases` explicitly — the status→phase trigger only
|
||||
// touches the phase column and leaves the lease row behind, producing
|
||||
// an orphan that the dev-mode consistency probe surfaces as a
|
||||
// divergence. Production would silently drift instead.
|
||||
const STATUSES_THAT_CLEAR_LEASE = new Set<string>([
|
||||
"done",
|
||||
"cancelled",
|
||||
"blocked",
|
||||
"todo",
|
||||
"backlog",
|
||||
]);
|
||||
function incrementActiveSprintVelocity(companyId: string, delta: number): void {
|
||||
const ts = nowIso();
|
||||
const today = ts.slice(0, 10);
|
||||
@@ -150,9 +164,27 @@ export function buildTaskStatusMethods(
|
||||
timestampSets.push("active_run_id = NULL");
|
||||
}
|
||||
|
||||
getDb()
|
||||
.prepare(`UPDATE tasks SET ${timestampSets.join(", ")} WHERE id = ?`)
|
||||
.run(...timestampValues, taskId);
|
||||
const shouldClearLease = STATUSES_THAT_CLEAR_LEASE.has(newStatus);
|
||||
transaction(() => {
|
||||
getDb()
|
||||
.prepare(
|
||||
// eslint-disable-next-line no-restricted-syntax -- B2 Phase 1 grandfathered: advanceTaskStatus is the sanctioned dynamic-SQL helper; timestampSets[0] is always the literal "status = ?". Status→phase trigger plus the explicit DELETE FROM task_leases below keep the shadow surface coherent.
|
||||
`UPDATE tasks SET ${timestampSets.join(", ")} WHERE id = ?`,
|
||||
)
|
||||
.run(...timestampValues, taskId);
|
||||
if (shouldClearLease) {
|
||||
// Drop any live lease row in the same transaction so no
|
||||
// caller observes the intermediate state of "task in terminal
|
||||
// or backlog phase, lease still present". `releaseTaskLock`
|
||||
// clears the lease on the happy path (normal run completion),
|
||||
// but terminal transitions can come from review timeouts,
|
||||
// retries, automation rules, etc. without ever going through
|
||||
// that helper.
|
||||
getDb()
|
||||
.prepare("DELETE FROM task_leases WHERE task_id = ?")
|
||||
.run(taskId);
|
||||
}
|
||||
});
|
||||
|
||||
if (comment) {
|
||||
deps.addComment({
|
||||
@@ -205,9 +237,9 @@ export function buildTaskStatusMethods(
|
||||
// The `active_run_id IS NULL` guard preserves the original CAS
|
||||
// contract for the busy case: a task whose active_run_id still
|
||||
// points at a live run is refused here, exactly as before.
|
||||
|
||||
const result = getDb()
|
||||
.prepare(
|
||||
// eslint-disable-next-line no-restricted-syntax -- B2 Phase 1 grandfathered: checkoutTask CAS writes status='in_progress' as part of the atomic lock acquisition. The status→phase trigger mirrors to the new phase column and writeCheckoutLease below writes the shadow lease in the same call.
|
||||
`UPDATE tasks
|
||||
SET checkout_run_id = ?, execution_locked_at = ?, status = 'in_progress', updated_at = ?
|
||||
WHERE id = ?
|
||||
|
||||
Reference in new issue
Block a user