fix(engine): restore agent presence lifecycle - #306
Conversation
📝 WalkthroughWalkthroughThe change adds five-minute agent presence leases, stale-status sweeping, atomic registration, and host-aware release handling. Releases dispatch to live hosts, complete locally for eligible deleted agents, or fail with ChangesAgent lifecycle
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant AgentRoute
participant ActionEngine
participant LiveHost
participant EngineDatabase
AgentRoute->>ActionEngine: invoke release with completionDeps
alt live host exists
ActionEngine->>LiveHost: dispatch release
LiveHost-->>ActionEngine: return release result
ActionEngine->>EngineDatabase: apply completion effects
else host is unavailable
ActionEngine->>EngineDatabase: complete delete_agent locally
ActionEngine->>EngineDatabase: record agent_host_unavailable for normal release
end
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/engine/src/engine/action.ts`:
- Around line 692-713: Update completeLocally and the corresponding flow around
the additional release-completion path to execute applyReleaseCompletionEffect
and the invocation status update within a single database transaction,
preserving the existing mutation order and conditions. Defer any external
completion effects until after the transaction successfully commits, so failures
roll back all lifecycle, binding, capacity, and invocation changes together.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4e4d5010-819c-46aa-9385-36ae01f4faae
📒 Files selected for processing (8)
README.mdopenapi.yamlpackages/engine/src/__tests__/conformance/agentLifecycle.test.tspackages/engine/src/adapters/node/index.tspackages/engine/src/engine/action.tspackages/engine/src/engine/agent.tspackages/engine/src/index.tspackages/engine/src/routes/agent.ts
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 8 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/engine/src/engine/agent.ts">
<violation number="1" location="packages/engine/src/engine/agent.ts:129">
P3: `listAgents` now performs a full workspace-wide select and applies the status filter in memory, and it also triggers `sweepStaleAgents` (a DB write) on every roster GET. The old code pushed `status` into the SQL `where` clause, so a `?status=active` request loaded only the matching rows. For workspaces with a large roster this removes the SQL pushdown and adds a write to a hot read path. Consider keeping the dirty-status sweep, but only push the status predicate into SQL when a status filter was actually requested (the derived status only differs from the persisted column for stale `active`/`online` rows).</violation>
<violation number="2" location="packages/engine/src/engine/agent.ts:280">
P2: Updating a stale agent to `active` returns `offline` but broadcasts `agent.status.active`, leaving roster consumers and realtime subscribers with conflicting presence. Fanout should use the effective status (or otherwise renew `lastSeen` before publishing an active event).</violation>
</file>
<file name="packages/engine/src/engine/action.ts">
<violation number="1" location="packages/engine/src/engine/action.ts:758">
P3: The new guard `if (!registry || !nodeId) { throw ... }` placed after the `!hostLive` branch is unreachable. `hostLive` is already false whenever `registry` or `nodeId` is missing (the `!!registry && ... && !!nodeId` conjunction short-circuits), so we only reach this line when both are present and the guard can never fire. It reads like a safety net but adds confusion; consider removing it (or hoisting it above the liveness check if you intend it to run for the non-via_node case).</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| handle: `@${updated.name}`, | ||
| type: updated.type, | ||
| status: updated.status, | ||
| status: effectiveAgentStatus(updated), |
There was a problem hiding this comment.
P2: Updating a stale agent to active returns offline but broadcasts agent.status.active, leaving roster consumers and realtime subscribers with conflicting presence. Fanout should use the effective status (or otherwise renew lastSeen before publishing an active event).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/engine/src/engine/agent.ts, line 280:
<comment>Updating a stale agent to `active` returns `offline` but broadcasts `agent.status.active`, leaving roster consumers and realtime subscribers with conflicting presence. Fanout should use the effective status (or otherwise renew `lastSeen` before publishing an active event).</comment>
<file context>
@@ -266,7 +277,7 @@ export async function updateAgent(
handle: `@${updated.name}`,
type: updated.type,
- status: updated.status,
+ status: effectiveAgentStatus(updated),
persona: updated.persona,
capabilities: updated.capabilities ?? null,
</file context>
There was a problem hiding this comment.
Confirmed still live, and filed as #313 rather than fixed here.
Reproduced the mechanism: updateAgent writes the status column but never renews last_seen, so effectiveAgentStatus(updated) returns offline for a stale row while fanoutAgentStatus(c, updated, body.status) (routes/agent.ts) broadcasts the requested value. Same request, same instant, two different answers.
Not fixed in this PR deliberately: it is presence reporting, not identity. Note the severity dropped during this PR — while the reclaim guard branched on status, this endpoint was a write path into the identity guard, letting a caller re-tighten a row while proving nothing about liveness. The guard now reads last_seen, which updateAgent does not write, so what remains is a reporting divergence rather than an identity one.
| } | ||
| // Keep the durable state aligned as a cleanup side effect, while still | ||
| // deriving below so correctness never depends on a cron/sweep having run. | ||
| await sweepStaleAgents(db, workspaceId); |
There was a problem hiding this comment.
P3: listAgents now performs a full workspace-wide select and applies the status filter in memory, and it also triggers sweepStaleAgents (a DB write) on every roster GET. The old code pushed status into the SQL where clause, so a ?status=active request loaded only the matching rows. For workspaces with a large roster this removes the SQL pushdown and adds a write to a hot read path. Consider keeping the dirty-status sweep, but only push the status predicate into SQL when a status filter was actually requested (the derived status only differs from the persisted column for stale active/online rows).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/engine/src/engine/agent.ts, line 129:
<comment>`listAgents` now performs a full workspace-wide select and applies the status filter in memory, and it also triggers `sweepStaleAgents` (a DB write) on every roster GET. The old code pushed `status` into the SQL `where` clause, so a `?status=active` request loaded only the matching rows. For workspaces with a large roster this removes the SQL pushdown and adds a write to a hot read path. Consider keeping the dirty-status sweep, but only push the status predicate into SQL when a status filter was actually requested (the derived status only differs from the persisted column for stale `active`/`online` rows).</comment>
<file context>
@@ -107,33 +124,27 @@ export async function registerAgent(
- }
+ // Keep the durable state aligned as a cleanup side effect, while still
+ // deriving below so correctness never depends on a cron/sweep having run.
+ await sweepStaleAgents(db, workspaceId);
+ const rows = await db
+ .select()
</file context>
There was a problem hiding this comment.
Confirmed valid on both halves, and filed as #315 rather than fixed here.
Splitting it, because the two halves are not the same severity:
- the lost SQL pushdown is the P3 you rated it. Real, and measured: 877 agent rows in the workspace this was verified against, 20,107 fleet-wide.
- the write on a read path is a design property rather than an optimisation gap, and I would rate it higher.
GET /v1/agentsnow issuesUPDATE agents, unbounded — the first roster read after a quiet period flips every stale row in one statement (308 of 877 here).
Not fixed in this PR because it is the PR's pre-existing design rather than something the last commits introduced, and reworking it means touching the derive-vs-filter logic the whole presence contract rests on. Holding a security fix on a performance refactor is the wrong trade.
One thing #315 records that is worth stating here: this PR removed the identity consequence of reads triggering the sweep — the reclaim guard no longer reads the column the sweep rewrites — but it did not stop reads from writing. Those shared a cause; only one was fixed.
There was a problem hiding this comment.
2 issues found across 3 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/engine/src/engine/action.ts">
<violation number="1" location="packages/engine/src/engine/action.ts:693">
P2: A concurrent bind/rebind can leave capacity permanently inflated: if the binding changes after this snapshot, the transaction deactivates the new binding but decrements only the old `activeNodeIds`, so the new node keeps an occupied `activeAgents` slot despite no active binding. Deriving the node decrement from the bindings inside the same atomic unit (or otherwise serializing the snapshot with binding changes) would keep placement capacity consistent.</violation>
<violation number="2" location="packages/engine/src/engine/action.ts:766">
P3: The local-offline completion branch inside `completeLocally` is unreachable. `completeLocally` is only called when `input.delete_agent === true` (both call sites in `dispatchRelease` use `delete_agent === true ? completeLocally() : failClosed()`), so the `else` branch that marks the agent `offline`/`self_connected`, clears `locationNodeId`, and stamps `metadata.release` can never run. It is dead code that now contradicts the PR contract (non-delete releases with no live host 503 via `failClosed`). Recommend removing the `else` branch (and simplifying the surrounding `if` to unconditional delete) so the local local-release path can't be mistakenly resurrected for non-delete releases.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
Review follow-up complete in f852dec.
No findings were argued against. Validation: turbo build --filter=@relaycast/engine..., engine lint, and agentLifecycle conformance 9/9 passed. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/engine/src/engine/action.ts (1)
771-784: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winA local delete of a legacy agent emits no
agent.exitedevent.The exit effect and
handler_node_idboth useagent.locationNodeId. The follow-up fix made dispatch resolve a host from the active bindings or from the implicit direct node id, precisely because a legacy directly registered row can havelocationType: 'self_connected'and a nulllocationNodeId. The conformance tests at Lines 281-284 and 331-334 create exactly that state.For such an agent,
delete_agentcompletes locally, the agent row is deleted, andagent.locationNodeIdis null. The condition at Line 771 is then false, so noagent.exitedevent reaches the spawn caller, the workspace event log, or the webhook. The response also reportshandler_node_id: null.Use the same resolved node id that liveness detection uses.
nodeIdis initialized at Line 817, beforecompleteLocallyruns at Line 832, so the closure can read it.🐛 Proposed fix
- if (completed.length > 0 && args.completionDeps && agent.locationNodeId) { + const exitNodeId = nodeId ?? agent.locationNodeId; + if (completed.length > 0 && args.completionDeps && exitNodeId) { await emitAgentExitedEffects(args.completionDeps, args.workspaceId, { agentId: agent.id, agentName: agent.name, - nodeId: agent.locationNodeId, + nodeId: exitNodeId, invocationId: fleetInvocationId(agent.metadata), reason: 'released', }); } return { invocation_id: invocation.id, action_name: 'release', handler_agent_id: null, - handler_node_id: agent.locationNodeId, + handler_node_id: exitNodeId,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/engine/src/engine/action.ts` around lines 771 - 784, Update the local release/delete flow around emitAgentExitedEffects and the returned handler_node_id to use the resolved nodeId from the liveness-detection path instead of agent.locationNodeId. Preserve the existing completionDeps and completed checks, but ensure legacy self-connected agents with a null locationNodeId emit agent.exited and report the resolved node identifier.
🧹 Nitpick comments (6)
packages/engine/src/engine/agent.ts (3)
353-364: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider the write cost of sweeping on every read path.
getAgentByName(Line 193) andlistAgentsboth callsweepStaleAgentsbefore reading. Each call issues twoUPDATEstatements even when no row matches. On SQLite and D1 each statement takes the write lock, so agent detail and roster reads now serialize behind the writer.Two options reduce this:
- Run a cheap
SELECTfirst and issue theUPDATEstatements only when a candidate row exists.- Rely on the periodic adapter sweep for durability and keep the request path read-only, since
effectiveAgentStatusalready derives the correct public status.Also consider an index on
(workspace_id, status, last_seen)to keep both predicates from scanning all workspace rows.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/engine/src/engine/agent.ts` around lines 353 - 364, Avoid issuing unconditional UPDATE statements from sweepStaleAgents on read paths used by getAgentByName and listAgents; either perform a lightweight candidate SELECT before updating or remove request-path sweeping and rely on effectiveAgentStatus plus the periodic adapter sweep. If retaining the sweep, add or reuse an index covering workspaceId, status, and lastSeen to avoid full scans.
353-379: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe returned count mixes clamped rows with expired rows.
sweepStaleAgentsreturnsnormalized.length + result.length. A clamped future timestamp is not a stale agent. A caller that reports this number as "stale agents marked offline" reports an incorrect value. Return the two counts separately, or return onlyresult.length.♻️ Proposed refactor
-export async function sweepStaleAgents(db: Db, workspaceId?: string): Promise<number> { +export async function sweepStaleAgents( + db: Db, + workspaceId?: string, +): Promise<{ clamped: number; expired: number }> { @@ - return normalized.length + result.length; + return { clamped: normalized.length, expired: result.length }; }Update the Node adapter sweep call site and any other caller that consumes the number.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/engine/src/engine/agent.ts` around lines 353 - 379, Update sweepStaleAgents to return the count of agents actually marked offline, using result.length rather than combining it with normalized.length; adjust the Node adapter sweep call site and any other consumers to use the corrected stale-agent count.
77-140: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMake the returning-row lookup independent of write order.
results[1]depends on the agents insert staying the second element ofwrites. A future write added before it silently shifts the index, andagentbecomes the wrong row type. Capture the index when you push the statement.♻️ Proposed refactor
let agent; try { + let agentWriteIndex = -1; const results = await runAtomicWrites(db, (writeDb) => { const writes: AtomicWrite[] = [writeDb.insert(nodes).values({ @@ .returning()]; + agentWriteIndex = writes.length - 1; if (generalChannel) { @@ return writes; }); - [agent] = results[1] as (typeof agents.$inferSelect)[]; + [agent] = results[agentWriteIndex] as (typeof agents.$inferSelect)[];🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/engine/src/engine/agent.ts` around lines 77 - 140, In the transaction around runAtomicWrites, capture the array index returned when pushing the agents insert into writes, then use that saved index instead of hard-coded results[1] when assigning agent. Keep the existing write ordering and returned-row type handling unchanged.packages/engine/src/__tests__/conformance/agentLifecycle.test.ts (1)
285-292: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDrop the trigger after the test.
The test creates
fail_local_release_completionand never removes it. If the harness reuses one SQLite handle across tests in this file, the trigger aborts any later local release completion, and failures then depend on declaration order. Add an explicit cleanup so the test does not rely on per-test database disposal.♻️ Proposed change
expect(invocation.status).toBe('pending'); + stack.runtime.handle.sqlite.exec('DROP TRIGGER IF EXISTS fail_local_release_completion'); });Prefer
try/finallyif an assertion can throw before the cleanup runs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/engine/src/__tests__/conformance/agentLifecycle.test.ts` around lines 285 - 292, Update the test creating the fail_local_release_completion trigger to wrap its assertions and execution in a try/finally block, and drop the trigger in the finally cleanup using the same SQLite handle. Ensure cleanup runs even when an assertion throws and preserve the trigger’s existing failure behavior during the test.packages/engine/src/engine/action.ts (2)
767-767: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBoth
runAtomicWritescallers recover typed rows by position.runAtomicWritesreturnsunknown[], and each caller picks a result by index and casts it without a check. If a write is later added or reordered, the cast still compiles and the wrong element is read.
packages/engine/src/engine/action.ts#L767-L767: capture the index of the completion write when you push it, and verify the value is an array before you usecompleted.lengthto gate theagent.exitedemission.packages/engine/src/engine/agent.ts#L138-L140: capture the index of theagentsinsert when you push it, instead of hardcodingresults[1].🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/engine/src/engine/action.ts` at line 767, Update packages/engine/src/engine/action.ts lines 767-767 in the runAtomicWrites caller to capture the completion write’s index when enqueueing it, then retrieve that result by index and verify it is an array before using completed.length to gate agent.exited emission. Update packages/engine/src/engine/agent.ts lines 138-140 in its runAtomicWrites caller to capture the agents insert index when enqueueing it and use that index instead of hardcoded results[1].
808-820: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winOrder the active bindings, and reuse
directNodeIdForAgent.Two points on the resolution chain:
activeBindings[0]?.nodeIdselects an arbitrary row. The query has noorderBy, andagent_node_bindingscarries aprioritycolumn. If an agent holds several active bindings and none matchesagent.locationNodeIdor the implicit direct node, the selected host can differ between attempts. Order the select bypriorityso the choice is deterministic.- The literal
node_direct_${agent.id}appears here and at Line 747.directNodeIdForAgentinpackages/engine/src/engine/node.tsalready produces this id. Import it so the format has one definition.♻️ Proposed refactor
const activeBindings = await args.db .select({ nodeId: agentNodeBindings.nodeId }) .from(agentNodeBindings) .where(and( eq(agentNodeBindings.workspaceId, args.workspaceId), eq(agentNodeBindings.agentId, agent.id), eq(agentNodeBindings.status, 'active'), - )); - const implicitDirectNodeId = `node_direct_${agent.id}`; + )) + .orderBy(desc(agentNodeBindings.priority), agentNodeBindings.createdAt); + const implicitDirectNodeId = directNodeIdForAgent(agent.id);Add the imports:
import { desc } from 'drizzle-orm'; import { directNodeIdForAgent } from './node.js';Apply the same helper at Line 747.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/engine/src/engine/action.ts` around lines 808 - 820, Update the active-bindings query around agent node resolution to order results by the binding priority column, preserving deterministic fallback selection via activeBindings[0]. Replace the local node_direct_${agent.id} construction and the equivalent literal near the other occurrence with the imported directNodeIdForAgent helper, adding the required drizzle-orm and node.js imports.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@packages/engine/src/engine/action.ts`:
- Around line 771-784: Update the local release/delete flow around
emitAgentExitedEffects and the returned handler_node_id to use the resolved
nodeId from the liveness-detection path instead of agent.locationNodeId.
Preserve the existing completionDeps and completed checks, but ensure legacy
self-connected agents with a null locationNodeId emit agent.exited and report
the resolved node identifier.
---
Nitpick comments:
In `@packages/engine/src/__tests__/conformance/agentLifecycle.test.ts`:
- Around line 285-292: Update the test creating the
fail_local_release_completion trigger to wrap its assertions and execution in a
try/finally block, and drop the trigger in the finally cleanup using the same
SQLite handle. Ensure cleanup runs even when an assertion throws and preserve
the trigger’s existing failure behavior during the test.
In `@packages/engine/src/engine/action.ts`:
- Line 767: Update packages/engine/src/engine/action.ts lines 767-767 in the
runAtomicWrites caller to capture the completion write’s index when enqueueing
it, then retrieve that result by index and verify it is an array before using
completed.length to gate agent.exited emission. Update
packages/engine/src/engine/agent.ts lines 138-140 in its runAtomicWrites caller
to capture the agents insert index when enqueueing it and use that index instead
of hardcoded results[1].
- Around line 808-820: Update the active-bindings query around agent node
resolution to order results by the binding priority column, preserving
deterministic fallback selection via activeBindings[0]. Replace the local
node_direct_${agent.id} construction and the equivalent literal near the other
occurrence with the imported directNodeIdForAgent helper, adding the required
drizzle-orm and node.js imports.
In `@packages/engine/src/engine/agent.ts`:
- Around line 353-364: Avoid issuing unconditional UPDATE statements from
sweepStaleAgents on read paths used by getAgentByName and listAgents; either
perform a lightweight candidate SELECT before updating or remove request-path
sweeping and rely on effectiveAgentStatus plus the periodic adapter sweep. If
retaining the sweep, add or reuse an index covering workspaceId, status, and
lastSeen to avoid full scans.
- Around line 353-379: Update sweepStaleAgents to return the count of agents
actually marked offline, using result.length rather than combining it with
normalized.length; adjust the Node adapter sweep call site and any other
consumers to use the corrected stale-agent count.
- Around line 77-140: In the transaction around runAtomicWrites, capture the
array index returned when pushing the agents insert into writes, then use that
saved index instead of hard-coded results[1] when assigning agent. Keep the
existing write ordering and returned-row type handling unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: fc2e5da4-232e-4cea-8351-b97620c6e9fd
📒 Files selected for processing (3)
packages/engine/src/__tests__/conformance/agentLifecycle.test.tspackages/engine/src/engine/action.tspackages/engine/src/engine/agent.ts
|
Additional CodeRabbit major fixed in 7801180: local delete/reap now uses the resolved host node for both handler_node_id and agent.exited. The legacy self_connected/null-location regression now reads back the completed response and durable workspace event, each with node_direct_<agent_id>. Validation remains green: engine build, lint, and agentLifecycle 9/9. |
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
Before this merges: restoring the sweep loosens an identity boundary on the resident-roster registration pathThis is a presence fix with a security side effect that is invisible from inside this issue. Flagging it here rather than in the issues where it was found, because the decision happens on merge of this one. The coupling
setWhere: or(
ne(agents.status, 'active'),
and(
eq(agents.locationType, 'via_node'),
or(
eq(agents.locationNodeId, nodeId),
sql`${agents.locationNodeId} = 'node_direct_' || ${agents.id}`,
),
),
),Read the first disjunct against today's reality. Because the sweep has had no caller, Restoring the sweep inverts that. Every record it flips to That is the AR-448 duplicate-agent/name-takeover class that AgentWorkforce/relay#1438 was written to close on the broker's own registration path. This path is not covered by that gate. I am not arguing the sweep shouldn't be restored — presence being permanently stale is its own serious problem, and the current protection is an accident rather than a design. The ask is that the Two more interactions worth checking before merge1. The 2. "Deriving public Suggested sequencingDecide these together rather than in isolation:
ProvenanceLine references read from relaycast |
Follow-up after reading the diff — one correction to my own comment, and the timing is sharper than I saidI wrote the above from the description. Having now read the diff, two things change. The core concern gets more urgent, and one of my pointers was wrong. 1. The sweep is not a background timer — it fires on every roster read
Measured on a live workspace on 2026-08-07: 305 records currently stored That is a one-shot transition of 305 agent identities from "reclaimable only by their own node" to "reclaimable by anyone", triggered by a read. Worth deciding deliberately. 2. Correction: the local reap does not go through
|
The local reap inlined a bare `DELETE` on `agents` inside the same atomic unit as the binding update and the invocation completion. Four foreign keys reference `agents.id` without `onDelete` — channels.created_by (schema.ts:455), messages.agent_id (:503), files.uploaded_by (:666), webhooks.created_by (:759) — so SQLite refuses the delete for any agent that has ever created a channel, sent a message, uploaded a file, or created a webhook. Because the statement sits inside `runAtomicWrites`, that refusal aborted the whole unit, so the invocation never completed either: a transaction abort rather than a legible error, on exactly the agents the reap exists to clean up. Every existing `delete_agent` fixture registered a fresh agent and released it immediately, so the suite could not observe this. The added fixture posts one message first and reproduced it as `SQLITE_CONSTRAINT_FOREIGNKEY: FOREIGN KEY constraint failed` (HTTP 500) before this change. Cascade is not an alternative — it would delete the agent's message history, which is the thing worth keeping — and `messages.agent_id` is NOT NULL, so `set null` cannot apply. That leaves the tombstone rename proposed in #309: the unique key is `(workspace_id, name)`, so freeing the name only requires the name to stop colliding, not the row to disappear. The released row keeps its id, so every FK target stays valid and every message keeps its sender. It is renamed to `<name>#released-<agentId>`, marked `released`, and stamped with `metadata.release`. Two deliberate choices beyond #309's sketch: - the tombstone is keyed on the agent id rather than a timestamp. It runs inside an atomic batch, where a unique-constraint violation would abort the whole unit — reintroducing the failure being fixed. The id is already unique per workspace, so the name cannot collide and a repeat release is idempotent. The release time is preserved in `metadata.release.releasedAt`. - `token_hash` is rotated to an unheld value. The row survives the release, and `token_hash` is NOT NULL UNIQUE so it cannot be cleared; without the rotation a released agent's old token would keep authenticating. `listAgents` now excludes released rows so `agent list` does not fill with tombstones. Refs #309
Restoring the presence sweep loosened an identity boundary as a side effect, and the trigger was a read. `registerAgentViaNode` reclaims a name via `onConflictDoUpdate` and overwrites `token_hash`, so a permitted reclaim is a full credential handover: the incumbent's token stops working and the claiming node is handed a live `at_live_` token for the same row. That decision was guarded by `setWhere: or(ne(agents.status, 'active'), <owning node>)`. `status` is maintained by `sweepStaleAgents`, which this branch calls synchronously from `listAgents` and `getAgentByName`. So a plain `agent list` rewrote the column, and every record it flipped to 'offline' satisfied the first disjunct and moved from "reclaimable only by its own node" to "reclaimable by any node, on name alone". A read widened who may claim an identity. Presence and identity are different questions and must not share a field. The first disjunct now gates on observed silence — `last_seen` older than `AGENT_RECLAIM_GRACE_MS` — which reads cannot write. The owning-node disjunct is unchanged, so a node restart still re-registers freely. An agent is absent from the roster after 5 minutes of silence and its name is reclaimable by a stranger after 24 hours. Between those points it reads as away and its identity is still its own. The grace value is measured, not guessed (relaycast-cloud, 2026-08-07): of the 1,578 records this governs, silence was <5m: 5, 5m-24h: 9, 1d-7d: 1, >7d: 1,568. At 24h the eligible set is 1,569 against 1,578, so it costs essentially nothing steady-state while protecting the ~14 identities a human would still call live. The reasoning is recorded on the constant. Scope, measured on the same data: this workspace has 8 genuinely loosened records, not the 305 first estimated — 300 of its 308 stale-active rows sit on their implicit direct node and were already reclaimable by any node with no deploy at all. Fleet-wide the split is 1,578 loosened against 14,074 already open. The larger standing hole is the `location_node_id = 'node_direct_' || id` disjunct, which this change does not touch; that is #311's subject. Also fixes the reverse defect: a row stored 'active' but silent for weeks was previously NOT reclaimable, so a name stranded by a dead node stayed stranded. The grace window now expires into recovery.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/engine/src/engine/action.ts (1)
694-807: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftGuard local completion against stale agent state.
completeLocallyuses an agent snapshot taken before the invocation and later updates byagent.idonly. If another node re-registers the agent, or another release completes first, this invocation can tombstone the newer agent state and still mark itselfcompleted.Make the atomic unit compare the current agent ownership state, such as the captured
tokenHashand active binding identity, before it deactivates bindings or completes the invocation. If the comparison fails, do not returncompleted. Reload and route the current state, or fail the stale invocation safely.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/engine/src/engine/action.ts` around lines 694 - 807, The completeLocally atomic workflow must reject stale agent snapshots before mutating state. In completeLocally, require the current agents row to match the captured agent ownership (including tokenHash) and require the expected active binding identity before deactivating bindings, renaming the agent, deleting the direct node, or completing the invocation; if validation fails, avoid returning completed and instead reload and route the current state or safely fail the invocation.
🧹 Nitpick comments (1)
packages/engine/src/engine/agent.ts (1)
210-212: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftMove stale-status persistence off the roster request path.
Line 212 waits for a workspace-wide write before every roster response.
effectiveAgentStatusalready derives the response status. A workspace with many stale agents can block roster reads and concurrent writes during the first sweep.Run durable sweeping in the periodic presence worker, or batch it outside this request path. Keep effective-status derivation in the read path.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/engine/src/engine/agent.ts` around lines 210 - 212, Remove the awaited sweepStaleAgents call from the roster request path in the surrounding agent handler, while preserving effectiveAgentStatus derivation for response correctness. Move durable stale-agent persistence to the periodic presence worker or another batched background flow so roster reads do not perform workspace-wide writes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@packages/engine/src/engine/action.ts`:
- Around line 694-807: The completeLocally atomic workflow must reject stale
agent snapshots before mutating state. In completeLocally, require the current
agents row to match the captured agent ownership (including tokenHash) and
require the expected active binding identity before deactivating bindings,
renaming the agent, deleting the direct node, or completing the invocation; if
validation fails, avoid returning completed and instead reload and route the
current state or safely fail the invocation.
---
Nitpick comments:
In `@packages/engine/src/engine/agent.ts`:
- Around line 210-212: Remove the awaited sweepStaleAgents call from the roster
request path in the surrounding agent handler, while preserving
effectiveAgentStatus derivation for response correctness. Move durable
stale-agent persistence to the periodic presence worker or another batched
background flow so roster reads do not perform workspace-wide writes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 65fbfce7-69bb-4c05-bb6e-766dfba52320
📒 Files selected for processing (5)
packages/engine/src/__tests__/conformance/agentLifecycle.test.tspackages/engine/src/__tests__/conformance/agentNameReclaim.test.tspackages/engine/src/engine/action.tspackages/engine/src/engine/agent.tspackages/engine/src/engine/node.ts
There was a problem hiding this comment.
1 issue found across 5 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/engine/src/engine/agent.ts">
<violation number="1" location="packages/engine/src/engine/agent.ts:218">
P2: The `/v1/agents/presence` response includes released tombstones even though they are no longer roster members. Applying the released-row exclusion in the presence query would keep presence consumers from seeing historical rows as live agents.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| .from(agents) | ||
| // Released rows are tombstones retained only to keep history attributable; | ||
| // they are not roster members, so `agent list` must not fill with them. | ||
| .where(and(eq(agents.workspaceId, workspaceId), ne(agents.status, RELEASED_AGENT_STATUS))); |
There was a problem hiding this comment.
P2: The /v1/agents/presence response includes released tombstones even though they are no longer roster members. Applying the released-row exclusion in the presence query would keep presence consumers from seeing historical rows as live agents.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/engine/src/engine/agent.ts, line 218:
<comment>The `/v1/agents/presence` response includes released tombstones even though they are no longer roster members. Applying the released-row exclusion in the presence query would keep presence consumers from seeing historical rows as live agents.</comment>
<file context>
@@ -170,7 +213,9 @@ export async function listAgents(db: Db, workspaceId: string, status?: string) {
- .where(eq(agents.workspaceId, workspaceId));
+ // Released rows are tombstones retained only to keep history attributable;
+ // they are not roster members, so `agent list` must not fill with them.
+ .where(and(eq(agents.workspaceId, workspaceId), ne(agents.status, RELEASED_AGENT_STATUS)));
const requestedStatus = status === 'online' ? 'active' : status;
</file context>
There was a problem hiding this comment.
Confirmed and fixed. listAgents was filtered but getPresence runs its own query against agents with no status predicate, so releasing a name made it reappear on /v1/agents/presence as a permanently offline agent rather than disappearing.
Filtering one roster surface and not the other is worse than filtering neither — it makes the tombstone look like a real second agent to exactly the consumers watching for liveness.
Fixed in getPresence. Test keeps released tombstones out of the roster and the presence view asserts both surfaces in one go, and asserts both that the original name is gone and that no #released- name is present, so a future surface that leaks the tombstone under its new name is also caught.
That test asserts no new behaviour — it passes before and after the guard change. It pins a decision: gating reclaim on node identity alone was rejected because an agent whose node dies and respawns elsewhere could never reclaim its own name, and a stranded name has no recovery path short of relaycast#309. A test that guards a decision rather than a behaviour reads like a redundant one, and is the first to be deleted by someone simplifying the disjunct it protects. Say so in the test.
…indings
Four findings from cubic on the previous two commits. All verified against
the code before being actioned; one falsifies a claim I made in a comment.
1. The id-keyed tombstone was NOT collision-free, as claimed.
The argument was "the agent id is unique per workspace, so the name cannot
collide". That holds only if nothing else can occupy the namespace, and
agent names are validated as `z.string().min(1)` — arbitrary strings. A
caller could pre-register `<victim>#released-<victimId>`, and the victim's
release would then hit `UNIQUE(workspace_id, name)` inside the atomic batch
and abort the whole unit: exactly the failure the tombstone exists to
avoid, reachable on demand.
Fixed at the root rather than by adding entropy: `#released-` is now a
reserved marker rejected on both registration paths (`registerAgent` and
`registerAgentViaNode`), which is what makes the id-keyed name actually
collision-free. Production has zero existing names containing the marker,
so nothing is grandfathered out.
2. `/v1/agents/presence` still listed released tombstones. `listAgents` was
filtered but `getPresence` runs its own query, so releasing a name made it
reappear as a permanently offline agent instead of disappearing. Filtering
one roster surface and not the other is worse than filtering neither.
3. The local reap discarded the caller's release reason, hardcoding
`reason: 'released'`, while the dispatched path records the supplied one.
Now uses the same `release: { reason, released_at, previous_name }` shape,
so an audit does not have to know which path released the agent.
4. The reclaim guard's comment overclaimed. It said reads do not move
`last_seen`; the sweep does write it, clamping a FUTURE timestamp back to
the server clock. The security conclusion is unchanged — the clamp writes
`now`, and the gate needs `now - AGENT_RECLAIM_GRACE_MS`, so a read still
cannot make a row claimable — but "reads never touch this column" was
false, and a security-sensitive comment that overstates its guarantee is
how the next person justifies a change it does not actually cover.
Each fix has a test that fails without it.
4 participants
Summary
activestatus fromlast_seenso correctness does not depend on cron timing503 agent_host_unavailablewhen no live hosting connection can own it, instead of returning an ownerlesspendinginvocationdelete_agentto reap an undispatchable record locally, freeing the name by tombstone rename rather than byDELETEstatuscolumn, so restoring the sweep does not loosen an identity boundary as a side effectRoot cause
sweepStaleAgentswas originally wired to a 60-secondsetIntervalin commit11ddbf5. The Fly.io -> Cloudflare Workers migration (b9eb241) removed the direct-run server loop, including that timer, but left the sweep function behind. No Worker scheduled replacement was added. The orphaned function was later copied into the portable engine (8aa0bf1) and has had no caller since.The release path independently routed every request through the target location. If the provider send failed,
dispatchReleasereturnedpendingwithdispatched_node_id: null, leaving no owner capable of completing it.Presence contract
activemeans Relaycast observed authenticated activity within the last five minutes;last_seenis authoritative, notmetadata.fleet.nodeId.offline.503 agent_host_unavailableand persists a failed invocation, never pending.delete_agentwith no live host is completed locally: the name is released by tombstone rename, and the active bindings, capacity, and implicit direct node are cleaned up.Identity contract, and why it is in this PR
Restoring the sweep changed who may take an agent's name, as a side effect, and the trigger was a read. That is resolved here rather than after.
registerAgentViaNodereclaims a name viaonConflictDoUpdateand overwritestoken_hash. A permitted reclaim is therefore a full credential handover, not a pointer move — demonstrated against the engine with a two-node probe:node_alpha;node_betasendsagent.registerfor the same nameactive: refused,token_hashunchangedoffline: allowed, and the reply frame returns{"ok":true,"data":{"agent_id":"<same row id>","name":"contested","token":"at_live_<redacted>"}}— same row, working token, incumbent evictedThe guard was
setWhere: or(ne(agents.status, 'active'), <owning node>). Because this branch callssweepStaleAgentssynchronously fromlistAgentsandgetAgentByName, a plainagent listrewritesstatus, and every record flipped toofflinesatisfied that first disjunct — moving from "reclaimable only by its own node" to "reclaimable by any node, on name alone".Presence and identity are different questions and must not share a field. "Should the roster show this agent as here" is cheap to get wrong and self-corrects on the next heartbeat. "May another node take this name and be issued a working token for it" cannot be undone. The guard now reads
last_seen, which a read cannot write, so a roster read can no longer widen who may claim an identity — structurally, not by convention.An agent is absent from the roster after 5 minutes of silence and its name is reclaimable by a stranger after
AGENT_RECLAIM_GRACE_MS(24h). Between those points it reads as away and its identity is still its own. The owning-node disjunct is unchanged, so a node restart or reconnect is never blocked.Why gate on
last_seenrather than simply tightening thestatuscheckBecause
statusis writable through the API, and that makes it unfit to govern identity regardless of what the sweep does.PATCH /v1/agents/:namewith{"status":"active"}writes the column while proving nothing about liveness. So while the guard branched onstatus, that endpoint was a write path into the identity guard: setactiveand a row re-tightens, let the sweep setofflineand it re-opens. Two unrelated callers — a roster read and a metadata PATCH — could both move an identity boundary as a side effect of doing something else.last_seenis only advanced by observed authenticated activity.updateAgentdoes not write it and neither does a roster read. So this change does not merely relocate the coupling — it removes the coupling for both of those paths at once, which tightening thestatuspredicate could not have done.It also closes a stranding, not only a loosening
"Tightening the reclaim guard" reads like it only takes something away. It does not — the two failure modes here are opposite, and this closes both.
Under the old
ne(agents.status, 'active')guard, a row storedactivebut silent for weeks was not reclaimable by another node, because the disjunct was false and the node-identity condition could not be satisfied by a different node. So when a node died uncleanly and never deregistered, its agents' names were stranded — held by a record nothing could revoke, and unrecoverable except by aDELETEthat the foreign keys refuse (see the reaping section, and #309). That is not hypothetical: it is why the fleet currently has names it cannot reuse.Gating on
last_seenmakes the grace window expire into recovery. A genuinely abandoned name becomes reclaimable after 24h instead of never. The testan agent silent beyond the reclaim grace can be reclaimed by another nodefails against the old guard withexpected 'node_alpha' to be 'node_beta'— the old code refusing a reclaim that should always have been allowed.Which class this closes, and which it does not. This fixes stranding on the engine's
registerAgentViaNodepath — agents registered through node-controlagent.register. It does not help a name stranded through the SDK/registerOrRotatepath or a fleet node whose own record is stuck, because those are different registration paths with different guards. Same class of harm, different code. Recovering those still depends on #309 landing.Scope, measured
Against
relaycast-cloudon 2026-08-07 (20,107 agent rows; 15,652 storedactivewithlast_seenpast the TTL):An earlier estimate of "305 loosened in this workspace" was wrong in both directions. It counted every stale-active row, but 300 of those 308 sit on their implicit direct node (
location_node_id = 'node_direct_' || id), which already satisfies the guard from any node — they were open before this PR and remain open after it. The genuinely loosened set here is 8.Fleet-wide, the bulk of what this PR would have opened is 1,563 rows with a NULL
location_node_id, protected today bystatus='active'and nothing else.The larger standing exposure — 14,074 rows open today via the
node_direct_disjunct — is not addressed here. That is #311.Choosing 24h
Silence distribution across the 1,578 records this governs:
<5m: 5,5m-24h: 9,1d-7d: 1,>7d: 1,568. 99.4% have been silent over a week, so a 24h grace costs essentially nothing steady-state (1,569 eligible vs 1,578) while protecting the ~14 identities a human would still call live. The reasoning and these numbers are recorded on the constant; lowering it converts live agents into reclaimable ones.Reaping without destroying history
The local reap issued a bare
DELETEonagentsinside the same atomic unit as the binding update and the invocation completion. Four foreign keys referenceagents.idwithoutonDelete—channels.created_by(schema.ts:455),messages.agent_id(:503),files.uploaded_by(:666),webhooks.created_by(:759) — so the delete is refused for any agent that has ever spoken. InsiderunAtomicWritesthat refusal aborted the whole unit, so the invocation never completed either: a transaction abort rather than a legible error, on exactly the agents the reap exists to clean up. 444 agents in this workspace alone have sent a message.Worth recording: the earlier review fix that moved these writes into
runAtomicWrites(f852dec, correctly closing a real partial-write bug) is what turned this FK refusal from a survivable partial failure into a whole-unit abort. A correctness fix deepened a latent bug underneath it. Neither change is wrong; the interaction was invisible from either one alone.Cascade would delete the agent's message history, and
messages.agent_idisNOT NULLsoset nullcannot apply. This adopts the tombstone rename from #309: the unique key is(workspace_id, name), so freeing the name only requires the name to stop colliding.Two choices beyond #309's sketch:
releasedAtis preserved inmetadata.release.token_hashis rotated on release. The row survives, andtoken_hashisNOT NULL UNIQUEso it cannot be cleared; without the rotation a released agent's old token would keep authenticating.listAgentsexcludes released rows soagent listdoes not fill with tombstones.On #312
This does not close #312.
effectiveAgentStatuscan never return"unknown", and for a fresh stored-activeagent it returns"active"— byte-identical to the previousstatus: a.status. So it cannot change the rows #312 is about (live agents serializing as"unknown"); it only changes stale rows fromactivetooffline. The"unknown"mapping exists in no local tree and was not located in this checkout. #312 remains open and unaddressed by this PR.Known-live, not fixed here
updateAgent(agent.ts) writesstatuswithout renewinglast_seen, soPATCH /v1/agents/:namewith{"status":"active"}returnsofflinein the response body while broadcastingagent.status.activeto realtime subscribers. Reported by cubic on this PR and confirmed. It is presence reporting rather than identity — after this change that path no longer touches the reclaim guard — and it is left for a follow-up rather than growing this PR.Verification
npm run typecheck --workspace=@relaycast/engine— cleannpm run lint --workspace=@relaycast/engine— cleannpm test --workspace=@relaycast/engine— 51 files, 548 tests (541 on the branch before these commits)Both defects were reproduced red before being fixed:
delete_agentfixtures all registered a fresh agent and released it immediately, so the suite could not observe the FK refusal. A fixture that posts one message first fails withSQLITE_CONSTRAINT_FOREIGNKEY: FOREIGN KEY constraint failed(HTTP 500) before the change.ne(agents.status,'active')turnsa roster read does not make a recently-active agent reclaimable by another nodered (expected 'node_beta' to be 'node_alpha').New assertions and what each catches:
status, so it cannot pass by the sweep not running<victim>#released-<victimId>to make the victim's release abortOn the review round that followed
cubic raised four findings against the first two commits and all four were valid, including one that falsified a claim made in this PR: the id-keyed tombstone name was described as collision-free by construction, and it was not. Agent names are
z.string().min(1), so a caller could occupy the tombstone namespace deliberately and turn any release into the whole-unit abort this PR exists to remove. That is now fixed at the root by reserving the marker at registration, rather than by adding entropy or a retry.Recording it because the correction matters more than the fix: the original argument was sound given an assumption about the namespace that nothing in the code enforced.
Neither automated reviewer caught either defect
Worth stating plainly, because this PR was
MERGEABLE/CLEANwith two green checks while carrying both of the above.CodeRabbit and cubic filed ten inline comments between them — atomicity, capacity accounting, unreachable guards, exit-node derivation. Useful review; four of those are verified fixed on this branch and one (
updateAgent) is confirmed still live and filed as a follow-up. But neither mentioned the FK RESTRICT refusal, and neither mentioned the identity loosening. Both checks reported pass throughout.So on this PR, green CI and two passing bot reviewers were both compatible with two merge-blocking defects. Bot review passing is not the same as having been vetted, and it should not be read as sufficient on a change that touches identity or foreign keys.
PR only. Do not merge or deploy without Khaliq approval.