Add TrueFoundryAgentStore for agents - #560
Conversation
🦋 Changeset detectedLatest commit: 2b721e6 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
1a5b818 to
45b8a9e
Compare
45b8a9e to
72c6a3f
Compare
45b880d to
2dcbc86
Compare
4af175c to
c9270c5
Compare
| import { toPutRemoteAgentPayload } from './toPutRemoteAgentPayload'; | ||
| import { TrueFoundryServiceFoundryServerClient } from './TrueFoundryServiceFoundryServerClient'; | ||
|
|
||
| function asError(value: unknown): Error { |
There was a problem hiding this comment.
Can you ask Cursor to check if this can be a common pattern across TrueForge?
There was a problem hiding this comment.
Not a common pattern
| constructor(input: { | ||
| inner: IAgentStore<TTransaction>; | ||
| client: TrueFoundryServiceFoundryServerClient; | ||
| accessToken: string; |
There was a problem hiding this comment.
@chiragjn I feel this should always be a callback.
Later we can implement refresh logic here.
| throw new AgentNameConflictError({ tenant_id: input.tenant_id, name: input.name }); | ||
| } | ||
|
|
||
| const { remoteAgentId } = await this.#client.putRemoteAgent({ |
There was a problem hiding this comment.
Looking at the code, it is not clear to me whether all this is happening within the transaction or not.
| // Race: peer create won the name and owns this remote (1:1) — do not delete it. | ||
| if (!(error instanceof AgentNameConflictError)) { | ||
| try { | ||
| await this.#client.deleteRemoteAgent({ accessToken: this.#accessToken, remoteAgentId }); |
There was a problem hiding this comment.
Is this worth it? How do you plan to roll back if we have an issue at commit time?
| throw new AgentNameConflictError({ tenant_id: input.tenant_id, name: input.name }); | ||
| } | ||
|
|
||
| const { remoteAgentId } = await this.#client.putRemoteAgent({ |
There was a problem hiding this comment.
I am a bit worried about what happens in a race situation. How do we know which put call won here?
There was a problem hiding this comment.
It is possible,
- Agent A (mcp: a, b) and Agent A (mcp: c) arrive. Note that the name is same.
- In TrueFoundry, we save (mcp: c)
- In TrueForge, we save (mcp: a, b).
| assertAgentNameNotReserved(input.name); | ||
|
|
||
| // SF PUT upserts by name — skip if local name exists (avoids overwrite/delete of e.g. research→sf-1). | ||
| const existing = await this.#inner.getAgent({ tenant_id: input.tenant_id, name: input.name }, transaction); |
There was a problem hiding this comment.
Do you not need a lock here to protect the system properly?
There was a problem hiding this comment.
Check out advisory locks in Postgres once and see if you can come up with a solution. Think about contention as well.
This is a non-issue in standalone mode.
There was a problem hiding this comment.
This is for Create:
Approach A — Insert first
- create:
createDB(external_id=null) → putRemote → updateDB(external_id) | on put/update fail → deleteDB (+ deleteRemote if put ok) | cleanup+fail → AggregateError - Pros:
- Fixes same-name MCP desync (loser never calls SF)
- No DB lock while waiting on SF
- Works in Postgres and SQLite
- No name-check read needed — unique insert picks the winner (0
getAgentreads on the happy path)
- Cons:
- Two DB writes per create
- Short window where
external_idis null - Rollback means deleting the local row (and remote if put already ran)
Approach B — Advisory lock
- create:
lock(name) → check name → putRemote → createDB(external_id) → unlock | on DB fail → deleteRemote | both fail → AggregateError - Pros:
- Fixes same-name desync if the lock covers the SF call
- One DB insert;
external_idset immediately - Clear “one create at a time” per name
- Only 1 SF put for a same-name race (waiter re-checks under lock and skips put)
- Cons:
- Lock held across SF HTTP (can wait seconds) → contention on same name
- More wiring (txn + lock; SQLite needs a no-op path)
- Extra name-check read under the lock (winner and waiter each do 1
getAgent)
Preferring A for create same race fixed, no lock across the network, Two DB writes per create agents & Short window where external_id is null
There was a problem hiding this comment.
This is for Update:
Approach A — DB first
- update:
get → updateDB(manifest) → putRemote → updateDB(external_id if changed) | on put fail → updateDB(previous) | cleanup+fail → AggregateError - Pros:
- DB is updated before SF; SF is not called if the local write fails
- No DB lock while waiting on SF
- Works in Postgres and SQLite
- Same local-first shape as create
- Cons:
- Does not fix concurrent update MCP desync (both updates can succeed on the same row, then both call SF)
- Often two DB writes when
external_idchanges (manifest + id); restore needs another write on put failure - Short window where DB is ahead of SF
- Rollback means rewriting the previous manifest (row is kept)
Approach B — Advisory lock
- update:
lock(name|id) → get → putRemote → updateDB → unlock | on DB fail → putRemote(old) | both fail → AggregateError
(orlock → get → updateDB → putRemote → unlock) - Pros:
- Fixes concurrent update desync if the lock covers the SF call
- Clear “one update at a time” per agent
- Only 1 SF put for a contended update (waiter runs after unlock with a fresh get)
- Cons:
- Lock held across SF HTTP (can wait seconds) → contention on hot agents
- More wiring (txn + lock; SQLite needs a no-op path)
- Extra get under the lock for waiters
Prefer B here
There was a problem hiding this comment.
Alternative using Redis:
- Use Redis as the mutex for (tenant_id, agent_id). Postgres is only used for short reads/writes. HTTP never runs under a PG txn or advisory lock.
- Redis — update / delete (by agent id)
- Mutex key:
tf:agent-update:{tenant_id}:{id}
- Mutex key:
SET key <token> NX EX 30 ← acquire (retry until 30s)
getAgent ← short PG read (auto-commit / caller txn only if passed)
putRemote / deleteRemote ← SF HTTP; no open PG txn, no pool pin
updateAgent / deleteAgent ← short PG write
EVAL compare-and-del(token) ← release only if we still own the key
Flow with redis lock:
- create: createDB(null) → lock → putRemote → updateDB(external_id) | on put/update fail → deleteDB (+ deleteRemote if put ok)
- update: lock → get → putRemote(new) → updateDB | on DB fail → putRemote(old) | both fail → AggregateError
- delete: lock → get → deleteRemote(404 ok) → deleteDB
8102487 to
bb53fda
Compare
74f67ba to
67459f4
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 67459f4. Configure here.
| const key = `tf:agent:${input.tenant_id}:${input.id}`; | ||
| await sql`SELECT pg_advisory_xact_lock(hashtext(${key}))`.execute(trx); | ||
| return fn(trx); | ||
| }); |
There was a problem hiding this comment.
Lock holds txn during HTTP
Medium Severity
createPostgresAgentUpdateLock opens a Postgres transaction and updateAgent awaits putRemoteAgent (and restore) inside that callback, so ServiceFoundry HTTP runs while the txn and pg_advisory_xact_lock stay held. That violates the store rule that transaction callbacks do only local DB work. A pool connection can sit idle for the 10s SF timeout, and a commit/rollback after a successful put leaves ServiceFoundry and the DB out of sync.
Additional Locations (1)
Triggered by project rule: @truefoundry/trueforge review rules
Reviewed by Cursor Bugbot for commit 67459f4. Configure here.
There was a problem hiding this comment.


Summary
Wire agent create/update/delete to ServiceFoundry in TrueFoundry mode via
TrueFoundryAgentStore(SF-first, remote id inexternal_id). Standalone stays DB-only.Linear: AGE-2064
Changes
TrueFoundryAgentStore+putRemoteAgent/deleteRemoteAgent/toPutRemoteAgentPayloadresolveAgentStore(same pattern as models/MCP)map*/to*convention insrc/truefoundry/AGENTS.mdHow was this tested?
tests/unit/truefoundry/TrueFoundryAgentStore.test.ts(create/update/delete + failure paths)resolveAgentStoretsc --noEmitfor@truefoundry/trueforgeChecklist
pnpm build,pnpm test,pnpm typecheck,pnpm lint:ci, andpnpm format:checkpass locallypackages/trueforge-sdk,.github/fern/openapi/openapi.json,docs/openapi.json) — fork PRs omit SDK regen; maintainers regenerate after merge.env.exampleupdated if configuration or behavior changed (N/A — no new env;AGENTS.mdmapper convention only)Note
Medium Risk
Dual-write orchestration between the database and ServiceFoundry on every mutating agent path can leave partial state if cleanup fails; Postgres locking mitigates concurrency but distributed failures remain possible.
Overview
In TrueFoundry mode, agent CRUD goes through a new
TrueFoundryAgentStoredecorator: local DB first on create, thenPUT /internal/tfg/agents, persisting the returned id inexternal_id; manifest updates and deletes sync remotely with rollback/cleanup on failure. Postgres uses a transaction-scoped advisory lock so concurrent update/delete cannot desync ServiceFoundry and the DB.Wiring matches models/MCP:
resolveAgentStore(request token → decorator; scheduler/no context → DB only), a shared ServiceFoundry client, and 10s HTTP timeouts on all ServiceFoundry calls (refactored via a shared JSON request helper).listAgentsnow accepts optionalexternal_idsfiltering in SQLite/Postgres stores.tfgandtrueforgeare reserved agent names on create (AgentNameReservedError→ HTTP 400). Routers, tests, and OpenAPI bootstrap are updated forresolveAgentStore.Reviewed by Cursor Bugbot for commit 2b721e6. Bugbot is set up for automated code reviews on this repo. Configure here.