fix(tasks): validate owner identity and brand session ids
This commit is contained in:
@@ -883,7 +883,7 @@ export const TYPE_API: readonly TypeApiEntry[] = [
|
||||
},
|
||||
{
|
||||
name: 'TaskSnapshot',
|
||||
declaration: 'export interface TaskSnapshot {\n id: TaskId;\n kind: string;\n label: string;\n ownerSession?: string;\n status: TaskStatus;\n detail?: string;\n startedAt: number;\n finishedAt?: number;\n reported: boolean;\n}',
|
||||
declaration: 'export interface TaskSnapshot {\n id: TaskId;\n kind: string;\n label: string;\n ownerSession?: SessionId;\n status: TaskStatus;\n detail?: string;\n startedAt: number;\n finishedAt?: number;\n reported: boolean;\n}',
|
||||
},
|
||||
{
|
||||
name: 'TaskStart',
|
||||
|
||||
@@ -17,7 +17,7 @@ Every read/kill/wait/get compares the task's owner session (`owner.session.heade
|
||||
## Lifecycle
|
||||
|
||||
- Registrations are NOT effect-scoped to the registering fiber: tasks belong to their owning agent + producing backend, so producer/surface HMR reloads never touch them.
|
||||
- An owned task attaches (once per owner) an awaited cleanup via `ctx.agents.onCleanup`: on owner disposal the registry cancels live tasks, awaits contract-compliant producers to quiescence, and drops their snapshots. If a teardown cancel throws, it force-fails the record and logs that the underlying work may be orphaned rather than deadlocking `AgentHandle.dispose()`.
|
||||
- An owned task must name the exact live `Agent` instance currently registered under its id (stale objects are rejected after id reuse), then attaches once per owner an awaited cleanup via `ctx.agents.onCleanup`: on owner disposal the registry cancels live tasks, awaits contract-compliant producers to quiescence, and drops their snapshots. If a teardown cancel throws, it force-fails the record and logs that the underlying work may be orphaned rather than deadlocking `AgentHandle.dispose()`.
|
||||
- Service disposal closes the listener registry first (late teardown settlements stay silent), then applies the same cancellation rule to every live task and awaits terminal records.
|
||||
- A producer whose `cancel` returns but never causes `done` to settle remains indistinguishable from a slow stop and can stall teardown; solving that residual requires an explicit bounded-lifetime or forced-disposal design.
|
||||
|
||||
|
||||
@@ -24,6 +24,7 @@
|
||||
"peerDependencies": {
|
||||
"@deepseek-ai/dsh-agent": "^0.0.1",
|
||||
"@deepseek-ai/dsh-brand": "^0.0.1",
|
||||
"@deepseek-ai/dsh-session": "^0.0.1",
|
||||
"@deepseek-ai/dsh-timeout": "^0.0.1",
|
||||
"cordis": "^4.0.0-rc.6"
|
||||
},
|
||||
|
||||
@@ -32,6 +32,7 @@
|
||||
|
||||
import { Context, Service } from 'cordis'
|
||||
import type { Agent, AgentId } from '@deepseek-ai/dsh-agent'
|
||||
import type { SessionId } from '@deepseek-ai/dsh-session'
|
||||
import { deadline, timeoutOf } from '@deepseek-ai/dsh-timeout'
|
||||
import { TaskId } from './types.ts'
|
||||
import type { TaskDoneListener, TaskOutcome, TaskRead, TaskSnapshot, TaskStart, TaskStatus } from './types.ts'
|
||||
@@ -67,7 +68,7 @@ interface TrackedTask {
|
||||
kind: string
|
||||
label: string
|
||||
/** The owner's session id (`session.header.id`), or undefined for an unowned task. */
|
||||
ownerSession: string | undefined
|
||||
ownerSession: SessionId | undefined
|
||||
cancel: (reason?: string) => void
|
||||
readOutput: (() => string) | undefined
|
||||
status: TaskStatus
|
||||
@@ -121,8 +122,9 @@ export class TaskService extends Service {
|
||||
* task id (`<kind>-N`, per-kind counter). Every check that can fail — the
|
||||
* control-surface fence ({@link attachSurface}; a task the model could
|
||||
* never read or stop must fail loud before it exists), kind/label
|
||||
* validation, and the owner's awaited disposal-cleanup attach (once per
|
||||
* owner agent, through `ctx.agents.onCleanup`) — runs BEFORE
|
||||
* validation, exact live owner-instance identity, and the owner's awaited
|
||||
* disposal-cleanup attach (once per owner agent, through
|
||||
* `ctx.agents.onCleanup`) — runs BEFORE
|
||||
* `spec.run()` starts the actual work, and nothing in the runtime can fail
|
||||
* after it returns: "work started but never got a collectable id" is
|
||||
* structurally impossible, not a producer rollback obligation. The runtime
|
||||
@@ -450,16 +452,21 @@ export class TaskService extends Service {
|
||||
* fiber. A narrow race remains if new work starts on an agent already being
|
||||
* drained: before this callback clears the owner entry, that start can reuse
|
||||
* the in-flight cleanup after its task snapshot was taken.
|
||||
* Fails loud when no agent registry is mounted — an owned background task
|
||||
* without the cleanup seam would outlive its owner silently.
|
||||
* Fails loud when no agent registry is mounted or when `owner` is not the
|
||||
* exact live instance currently registered under its id — accepting a stale
|
||||
* object after id reuse would attach its session's task to another agent's
|
||||
* lifecycle.
|
||||
*/
|
||||
private ensureOwnerCleanup(owner: Agent): void {
|
||||
const ownerId = owner.id
|
||||
if (this.ownerCleanups.has(ownerId)) return
|
||||
const agents = this.selfCtx.get('agents')
|
||||
if (agents === undefined) {
|
||||
throw new Error('background task ownership requires the agent registry (load @deepseek-ai/dsh-agent)')
|
||||
}
|
||||
if (agents.get(ownerId) !== owner) {
|
||||
throw new Error(`agent "${ownerId}" is not the registered agent instance (background task owner must be live)`)
|
||||
}
|
||||
if (this.ownerCleanups.has(ownerId)) return
|
||||
const ownerSession = owner.session.header.id
|
||||
// Attach FIRST, record after: onCleanup throws for an unregistered agent,
|
||||
// and marking the owner as covered before that would make every later
|
||||
@@ -476,7 +483,7 @@ export class TaskService extends Service {
|
||||
}
|
||||
|
||||
/** Cancel, await terminal records, and drop every task owned by one session. */
|
||||
private async disposeOwned(ownerSession: string): Promise<void> {
|
||||
private async disposeOwned(ownerSession: SessionId): Promise<void> {
|
||||
const owned = [...this.store.values()].filter(task => task.ownerSession === ownerSession)
|
||||
this.cancelForTeardown(owned, 'owner disposed')
|
||||
await Promise.all(owned.map(task => task.settled))
|
||||
|
||||
@@ -9,6 +9,7 @@
|
||||
|
||||
import type { Branded } from '@deepseek-ai/dsh-brand'
|
||||
import type { Agent } from '@deepseek-ai/dsh-agent'
|
||||
import type { SessionId } from '@deepseek-ai/dsh-session'
|
||||
|
||||
/**
|
||||
* Identifies one background task in the runtime-global registry. Generated by
|
||||
@@ -71,7 +72,9 @@ export interface TaskStart {
|
||||
/**
|
||||
* The spawning agent. Its `session.header.id` becomes the task's owner
|
||||
* token (read/kill/wait/list are fenced to that session), and its disposal
|
||||
* cancels and awaits the task through the `ctx.agents.onCleanup` seam.
|
||||
* cancels and awaits the task through the `ctx.agents.onCleanup` seam. It
|
||||
* must be the exact live instance currently registered under its agent id;
|
||||
* a stale object whose id has been reused is rejected before work starts.
|
||||
* `undefined` starts an UNOWNED task: open to any caller, alive until the
|
||||
* tasks service disposes.
|
||||
*/
|
||||
@@ -136,9 +139,10 @@ export interface TaskSnapshot {
|
||||
* The owner's session id (`session.header.id`), for surfaces that must
|
||||
* reach the owning agent (the completion-notice injector); absent for
|
||||
* unowned tasks. Session ids are runtime-shared identifiers, not secrets —
|
||||
* the read/kill/wait/list FENCE is what isolation rests on.
|
||||
* the read/kill/wait/list FENCE is what isolation rests on. The shared
|
||||
* {@link SessionId} brand is preserved across this package boundary.
|
||||
*/
|
||||
ownerSession?: string
|
||||
ownerSession?: SessionId
|
||||
/** Current lifecycle state. */
|
||||
status: TaskStatus
|
||||
/** Kind-specific status detail, present once the producer supplied one (usually terminal). */
|
||||
|
||||
@@ -1,4 +1,4 @@
|
||||
import { describe, expect, it, vi } from 'vitest'
|
||||
import { describe, expect, expectTypeOf, it, vi } from 'vitest'
|
||||
import { Context } from 'cordis'
|
||||
import { Session, SessionId } from '@deepseek-ai/dsh-session'
|
||||
import AgentRegistry, { AgentId } from '@deepseek-ai/dsh-agent'
|
||||
@@ -6,12 +6,12 @@ import type { Agent } from '@deepseek-ai/dsh-agent'
|
||||
import TaskService, { TaskId } from '@deepseek-ai/dsh-tasks'
|
||||
import type { TaskHooks, TaskOutcome, TaskSnapshot, TaskStart } from '@deepseek-ai/dsh-tasks'
|
||||
|
||||
function stubAgent(rawId: string): Agent {
|
||||
function stubAgent(rawId: string, rawSessionId = `${rawId}-session`): Agent {
|
||||
const id = AgentId(rawId)
|
||||
return {
|
||||
id,
|
||||
options: {},
|
||||
session: new Session(SessionId(`${id}-session`)),
|
||||
session: new Session(SessionId(rawSessionId)),
|
||||
status: 'idle',
|
||||
send() {},
|
||||
steer() {},
|
||||
@@ -48,6 +48,10 @@ async function harness() {
|
||||
const tick = () => new Promise<void>(r => setTimeout(r, 0))
|
||||
|
||||
describe('TaskService.start', () => {
|
||||
it('preserves the SessionId brand on public owner snapshots', () => {
|
||||
expectTypeOf<TaskSnapshot['ownerSession']>().toEqualTypeOf<SessionId | undefined>()
|
||||
})
|
||||
|
||||
it('refuses to register while no control surface is attached', async () => {
|
||||
const ctx = new Context()
|
||||
await ctx.plugin(TaskService)
|
||||
@@ -365,9 +369,10 @@ describe('TaskService owner isolation', () => {
|
||||
const ctx = await harness()
|
||||
const ghost = stubAgent('ghost') // never registered in ctx.agents
|
||||
|
||||
// onCleanup rejects the unregistered agent BEFORE any registry mutation.
|
||||
// Exact-instance preflight rejects the unregistered agent BEFORE any
|
||||
// registry mutation or owner-cleanup attachment.
|
||||
expect(() => ctx.tasks.start(producer({ owner: ghost }).spec))
|
||||
.toThrow('is not registered')
|
||||
.toThrow('is not the registered agent instance')
|
||||
expect(ctx.tasks.list(ghost)).toEqual([])
|
||||
|
||||
// Once the agent actually exists, the same owner gets a WORKING cleanup —
|
||||
@@ -389,6 +394,30 @@ describe('TaskService owner isolation', () => {
|
||||
expect(cancels).toEqual(['owner disposed'])
|
||||
expect(ctx.tasks.list(ghost)).toEqual([])
|
||||
})
|
||||
|
||||
it('rejects a stale owner instance after another agent reuses its id', async () => {
|
||||
const ctx = await harness()
|
||||
const staleOwner = stubAgent('owner', 'stale-session')
|
||||
const unregisterStale = ctx.agents.register(staleOwner)
|
||||
unregisterStale()
|
||||
|
||||
const currentOwner = stubAgent('owner', 'current-session')
|
||||
ctx.agents.register(currentOwner)
|
||||
const current = producer({ owner: currentOwner })
|
||||
ctx.tasks.start(current.spec) // Attach the current owner's cleanup first.
|
||||
|
||||
const stale = producer({ owner: staleOwner })
|
||||
const staleRun = vi.fn(() => stale.spec.run())
|
||||
expect(() => ctx.tasks.start({ ...stale.spec, run: staleRun }))
|
||||
.toThrow('is not the registered agent instance')
|
||||
expect(staleRun).not.toHaveBeenCalled()
|
||||
expect(ctx.tasks.list(staleOwner)).toEqual([])
|
||||
expect(ctx.tasks.list(currentOwner)).toHaveLength(1)
|
||||
|
||||
current.settle({ status: 'completed' })
|
||||
await tick()
|
||||
await ctx.agents.drainCleanups(currentOwner.id)
|
||||
})
|
||||
})
|
||||
|
||||
describe('TaskService owner cleanup', () => {
|
||||
|
||||
@@ -20,6 +20,9 @@
|
||||
{
|
||||
"path": "../../core/agent"
|
||||
},
|
||||
{
|
||||
"path": "../../core/session"
|
||||
},
|
||||
{
|
||||
"path": "../../util/timeout"
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user