fix(agent-jobs): prevent terminal status resurrection
This commit is contained in:
@@ -0,0 +1,66 @@
|
|||||||
|
# Task 4 Report: Terminal agent-job status guard
|
||||||
|
|
||||||
|
## Status
|
||||||
|
|
||||||
|
Implemented Phase 1 Task 4. `updateStatus` now treats terminal Convex job state as authoritative and ignores later worker status writes instead of resurrecting a completed or cancelled job.
|
||||||
|
|
||||||
|
## Implementation
|
||||||
|
|
||||||
|
- Added a shared terminal-status set and predicate for `failed`, `cancelled`, `timed_out`, and `draft_pr_opened`.
|
||||||
|
- Reused the predicate in the existing terminal-job helper.
|
||||||
|
- Added an early successful no-op in `updateStatus` after worker claim validation.
|
||||||
|
- Added coverage proving cancelled jobs remain cancelled and running jobs still accept `checks_running`.
|
||||||
|
|
||||||
|
## Files
|
||||||
|
|
||||||
|
- `packages/backend/convex/agentJobs.ts`
|
||||||
|
- `packages/backend/tests/unit/update-status.test.ts`
|
||||||
|
|
||||||
|
## TDD Evidence
|
||||||
|
|
||||||
|
### RED
|
||||||
|
|
||||||
|
Command:
|
||||||
|
|
||||||
|
```sh
|
||||||
|
cd packages/backend && bun run test:unit -- update-status
|
||||||
|
```
|
||||||
|
|
||||||
|
Result: exit 1. One of two tests failed. The cancelled-job case received `{ success: true }` instead of `{ success: true, ignored: true }`, while the non-terminal transition test passed. This demonstrated that `updateStatus` had no terminal-state guard.
|
||||||
|
|
||||||
|
### GREEN
|
||||||
|
|
||||||
|
Command:
|
||||||
|
|
||||||
|
```sh
|
||||||
|
cd packages/backend && bun run test:unit -- update-status
|
||||||
|
```
|
||||||
|
|
||||||
|
Result: exit 0. One test file passed; both tests passed.
|
||||||
|
|
||||||
|
## Verification
|
||||||
|
|
||||||
|
Commands:
|
||||||
|
|
||||||
|
```sh
|
||||||
|
cd packages/backend && bun run test:unit
|
||||||
|
cd packages/backend && bun run lint
|
||||||
|
cd packages/backend && bun run typecheck
|
||||||
|
```
|
||||||
|
|
||||||
|
Results: all exited 0. The backend unit suite passed 2 files and 16 tests; ESLint and `tsc --noEmit` emitted no diagnostics.
|
||||||
|
|
||||||
|
## Codegen Environment
|
||||||
|
|
||||||
|
`bun codegen:convex` was attempted and exited 1 before tests because neither `CONVEX_SELF_HOSTED_URL` plus `CONVEX_SELF_HOSTED_ADMIN_KEY` nor `CONVEX_DEPLOYMENT` is available. To run `convex-test` without modifying generated files, the worktree temporarily used the existing generated snapshot from the primary checkout via a symlink. The symlink was removed before staging and committing.
|
||||||
|
|
||||||
|
## Self-review
|
||||||
|
|
||||||
|
- Confirmed claim ownership is still validated before terminal writes are ignored.
|
||||||
|
- Confirmed all four specified terminal statuses share the guard.
|
||||||
|
- Confirmed non-terminal worker transitions retain their prior behavior.
|
||||||
|
- Confirmed no generated Convex files were edited or committed.
|
||||||
|
|
||||||
|
## Concerns
|
||||||
|
|
||||||
|
Full root-level validation remains unavailable without a configured Convex codegen environment. Focused and package-level backend checks pass against the existing generated snapshot.
|
||||||
@@ -240,10 +240,18 @@ const isDeletableWorkspace = (job: Doc<'agentJobs'>) =>
|
|||||||
['failed', 'cancelled', 'timed_out'].includes(job.status) ||
|
['failed', 'cancelled', 'timed_out'].includes(job.status) ||
|
||||||
['stopped', 'expired', 'failed'].includes(job.workspaceStatus ?? '');
|
['stopped', 'expired', 'failed'].includes(job.workspaceStatus ?? '');
|
||||||
|
|
||||||
|
const TERMINAL_STATUSES = new Set([
|
||||||
|
'failed',
|
||||||
|
'cancelled',
|
||||||
|
'timed_out',
|
||||||
|
'draft_pr_opened',
|
||||||
|
]);
|
||||||
|
|
||||||
|
const isTerminalStatus = (status: string) => TERMINAL_STATUSES.has(status);
|
||||||
|
|
||||||
const isTerminalJob = (job: Doc<'agentJobs'>) =>
|
const isTerminalJob = (job: Doc<'agentJobs'>) =>
|
||||||
['failed', 'cancelled', 'timed_out', 'draft_pr_opened'].includes(
|
isTerminalStatus(job.status) ||
|
||||||
job.status,
|
['stopped', 'expired', 'failed'].includes(job.workspaceStatus ?? '');
|
||||||
) || ['stopped', 'expired', 'failed'].includes(job.workspaceStatus ?? '');
|
|
||||||
|
|
||||||
const deleteWorkspaceRows = async (ctx: MutationCtx, job: Doc<'agentJobs'>) => {
|
const deleteWorkspaceRows = async (ctx: MutationCtx, job: Doc<'agentJobs'>) => {
|
||||||
const messages = await ctx.db
|
const messages = await ctx.db
|
||||||
@@ -1120,6 +1128,9 @@ export const updateStatus = mutation({
|
|||||||
if (job?.claimedBy !== args.workerId) {
|
if (job?.claimedBy !== args.workerId) {
|
||||||
throw new ConvexError('Agent job not claimed by this worker.');
|
throw new ConvexError('Agent job not claimed by this worker.');
|
||||||
}
|
}
|
||||||
|
if (isTerminalStatus(job.status)) {
|
||||||
|
return { success: true, ignored: true as const };
|
||||||
|
}
|
||||||
const now = Date.now();
|
const now = Date.now();
|
||||||
const patch: Partial<Doc<'agentJobs'>> = {
|
const patch: Partial<Doc<'agentJobs'>> = {
|
||||||
status: args.status,
|
status: args.status,
|
||||||
|
|||||||
@@ -0,0 +1,109 @@
|
|||||||
|
import { convexTest } from 'convex-test';
|
||||||
|
import { beforeAll, describe, expect, test } from 'vitest';
|
||||||
|
|
||||||
|
import { api } from '../../convex/_generated/api.js';
|
||||||
|
import schema from '../../convex/schema';
|
||||||
|
|
||||||
|
const modules = import.meta.glob('../../convex/**/*.*s');
|
||||||
|
|
||||||
|
const spoonInput = {
|
||||||
|
name: 'Editor Spoon',
|
||||||
|
provider: 'gitea' as const,
|
||||||
|
upstreamOwner: 'upstream',
|
||||||
|
upstreamRepo: 'editor',
|
||||||
|
upstreamDefaultBranch: 'main',
|
||||||
|
upstreamUrl: 'https://git.example.com/upstream/editor',
|
||||||
|
forkOwner: 'team',
|
||||||
|
forkRepo: 'editor-spoon',
|
||||||
|
forkUrl: 'https://git.example.com/team/editor-spoon',
|
||||||
|
visibility: 'private' as const,
|
||||||
|
maintenanceMode: 'watch' as const,
|
||||||
|
syncCadence: 'daily' as const,
|
||||||
|
productionRefStrategy: 'default_branch' as const,
|
||||||
|
};
|
||||||
|
|
||||||
|
const seedJob = async (
|
||||||
|
t: ReturnType<typeof convexTest>,
|
||||||
|
status: 'cancelled' | 'running',
|
||||||
|
) =>
|
||||||
|
await t.mutation(async (ctx) => {
|
||||||
|
const now = Date.now();
|
||||||
|
const ownerId = await ctx.db.insert('users', {
|
||||||
|
email: 'a@b.c',
|
||||||
|
name: 'A',
|
||||||
|
});
|
||||||
|
const spoonId = await ctx.db.insert('spoons', {
|
||||||
|
...spoonInput,
|
||||||
|
ownerId,
|
||||||
|
status: 'active',
|
||||||
|
createdAt: now,
|
||||||
|
updatedAt: now,
|
||||||
|
});
|
||||||
|
const requestId = await ctx.db.insert('agentRequests', {
|
||||||
|
spoonId,
|
||||||
|
ownerId,
|
||||||
|
prompt: 'x',
|
||||||
|
status: 'running',
|
||||||
|
createdAt: now,
|
||||||
|
updatedAt: now,
|
||||||
|
});
|
||||||
|
return await ctx.db.insert('agentJobs', {
|
||||||
|
spoonId,
|
||||||
|
ownerId,
|
||||||
|
agentRequestId: requestId,
|
||||||
|
status,
|
||||||
|
prompt: 'x',
|
||||||
|
runtime: 'opencode',
|
||||||
|
baseBranch: 'main',
|
||||||
|
workBranch: 'spoon/x',
|
||||||
|
forkOwner: 'team',
|
||||||
|
forkRepo: 'editor-spoon',
|
||||||
|
forkUrl: 'https://git.example.com/team/editor-spoon',
|
||||||
|
upstreamOwner: 'upstream',
|
||||||
|
upstreamRepo: 'editor',
|
||||||
|
selectedSecretIds: [],
|
||||||
|
model: '',
|
||||||
|
reasoningEffort: 'medium',
|
||||||
|
claimedBy: 'worker-1',
|
||||||
|
createdAt: now,
|
||||||
|
updatedAt: now,
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|
||||||
|
beforeAll(() => {
|
||||||
|
process.env.SPOON_WORKER_TOKEN = 'test-worker-token';
|
||||||
|
});
|
||||||
|
|
||||||
|
describe('updateStatus terminal guard', () => {
|
||||||
|
test('ignores writes to a cancelled job', async () => {
|
||||||
|
const t = convexTest(schema, modules);
|
||||||
|
const jobId = await seedJob(t, 'cancelled');
|
||||||
|
|
||||||
|
const result = await t.mutation(api.agentJobs.updateStatus, {
|
||||||
|
workerToken: 'test-worker-token',
|
||||||
|
workerId: 'worker-1',
|
||||||
|
jobId,
|
||||||
|
status: 'running',
|
||||||
|
});
|
||||||
|
|
||||||
|
expect(result).toEqual({ success: true, ignored: true });
|
||||||
|
const job = await t.run(async (ctx) => await ctx.db.get(jobId));
|
||||||
|
expect(job?.status).toBe('cancelled');
|
||||||
|
});
|
||||||
|
|
||||||
|
test('accepts writes to a running job', async () => {
|
||||||
|
const t = convexTest(schema, modules);
|
||||||
|
const jobId = await seedJob(t, 'running');
|
||||||
|
|
||||||
|
const result = await t.mutation(api.agentJobs.updateStatus, {
|
||||||
|
workerToken: 'test-worker-token',
|
||||||
|
workerId: 'worker-1',
|
||||||
|
jobId,
|
||||||
|
status: 'checks_running',
|
||||||
|
});
|
||||||
|
|
||||||
|
expect(result).toEqual({ success: true });
|
||||||
|
const job = await t.run(async (ctx) => await ctx.db.get(jobId));
|
||||||
|
expect(job?.status).toBe('checks_running');
|
||||||
|
});
|
||||||
|
});
|
||||||
Reference in New Issue
Block a user