Skip to content

feat(daemon): account for source cache objects - #2766

Open
Harbor404 wants to merge 1 commit into
agentconnect-md:mainfrom
Harbor404:feat/source-cache-accounting
Open

Harbor404 wants to merge 1 commit into
agentconnect-md:mainfrom
Harbor404:feat/source-cache-accounting

Conversation

@Harbor404

Copy link
Copy Markdown
Contributor

Part of #2732. Implements only CP1.2 — Source Cache data-plane accounting.

Changes

  • Add the source_cache_object ledger and per-org source_cache_usage lock row on both store drivers, including Postgres canonicalColumns entries and schema v35 migration coverage.
  • Reserve objects in one transaction under the org usage-row lock, counting committed bytes plus unexpired pending reservations against the quota.
  • Add commit, pending expiry/release, last-read, usage, and object read APIs with cross-org isolation and rollback-safe failure behavior.
  • Add a cross-driver contract suite covering quota boundaries, concurrent reservations, pending expiry, commit/last-read, org isolation, and rollback.

Verification

  • pnpm --filter @agentconnect.md/daemon exec vitest run test/source-cache-accounting.test.ts test/local-store.test.ts test/postgres-dialect.test.ts test/local-store-sql-portability.test.ts
  • pnpm --filter @agentconnect.md/daemon typecheck
  • pnpm exec eslint on changed source/test files
  • PostgreSQL store project is wired to run the new contract suite in CI; local execution is unavailable because this host has no container runtime.
@Harbor404

Copy link
Copy Markdown
Contributor Author

@agentconnect-md-test

@agentconnect-md-test

Copy link
Copy Markdown
Contributor

This agent responds to requests from this repository's maintainers and trusted contributors. A maintainer can mention it on this thread to have it take a look.

@agentconnect-md-test agentconnect-md-test Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Architecture review: CP1.2 Source Cache accounting (#2732)

This PR mostly matches the plan in docs/designs/source-cache.md §§9–10. The ledger is a pool-store table on both drivers, with a v35 migration and canonicalColumns entries. Reservations happen under the per-org usage-row lock. The quota counts committed bytes plus unexpired pending rows. A late commit loses to expiry. The bucket is never listed. The PR body says it covers CP1.2 only, and CP1.1 and CP1.3–1.8 are clearly left for later PRs. That scoping is right.

Blocking: the expiry primitive reverses the sweep contract

expireSourceCacheReservations deletes the expired pending rows first and only then hands them to the caller "so their objects can be swept". The design and CP1.7 require the opposite order:

  • §9 Abandoned uploads: the sweep "HEADs its key, deletes the object if present, and deletes the row, which releases the reservation."
  • §10: "a member that dies mid-pass leaves rows the next pass finishes."

With rows deleted first:

  • If a member crashes between the row delete and the object delete, the database loses track of the object. Only the 2-day pending lifecycle rule ever collects it. That rule is meant as a backstop for lost rows, not as the normal crash path.
  • The org's quota is freed while the object is still in the bucket, so new reservations can push real storage past the 20 GiB cap.
  • CP1.7 would have to build on this primitive or route around it. It is cheaper to get the shape right now, before anything calls it.

Suggested shape, without going into implementation:

  1. A claim step returns due pending rows and does not delete them. Per CP1.7 this is FOR UPDATE SKIP LOCKED per row, not serialization on the org lock, so members can sweep in parallel.
  2. A release step for one (orgId, key) deletes the row and refreshes usage under the org lock. The sweep calls it only after the object step succeeds, and it must be safe to repeat.

The sweep will also need a way to find which orgs have due rows. Today the API only works for an org it is given.

Non-blocking

  • repositoryUrlHash column name. §4 now keys a cred entry by <provider>:<id> from CodeHostRepository, not by a URL hash. The "repository URL hash" wording in §10 is left over from before the access-class change. Once v35 ships, renaming the column costs a migration. Consider a neutral name such as repositoryKey, or a hash of the <repo> key segment, and fix the §10 wording in the same change.
  • Pointer rows. kind = 'pointer' has to pass through pending with an expiresAt. In §9 the daemon writes the pointer itself with a conditional write, and nothing reserves it. Confirm in CP1.6 whether pointer rows are ledger entries that count toward the quota at all. If not, drop pointer from the reservation path.
  • Comment style. The new JSDoc blocks are multi-line. CLAUDE.md asks for one-line comments.

Once the claim and release steps follow the design's order, this is ready to approve.

sent by architect (Claude Agent · default) · open in session

* Delete expired pending rows and return them to the caller so their objects can be swept.
* The delete and the usage snapshot share the org lock with reservations.
*/
async expireSourceCacheReservations(orgId: string, now: number): Promise<SourceCacheObjectRow[]> {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking: this deletes the row, which releases the reservation, before the object is deleted. §9/§10 and CP1.7 need the reverse: claim due rows (FOR UPDATE SKIP LOCKED, no delete), delete the object, then release the row in a repeatable step. A crash in between must leave the row for the next pass. Otherwise only the 2-day lifecycle backstop collects the object, and the quota is freed while its bytes are still in the bucket.

kind TEXT NOT NULL CHECK (kind IN ('bundle', 'pointer')),
state TEXT NOT NULL CHECK (state IN ('pending', 'committed')),
bytes INTEGER NOT NULL CHECK (bytes >= 0),
repositoryUrlHash TEXT NOT NULL,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-blocking: a cred entry's repository identity is <provider>:<id> (§4), not a URL hash. Renaming this column after v35 ships costs a migration, so consider a neutral name now and fix the stale §10 wording to match.

@agentconnect-md-test agentconnect-md-test Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed CP1.2 at aba922d. The schema migration, per-org quota lock and reservation/commit accounting look consistent across SQLite and PostgreSQL. Approved with one non-blocking follow-up for CP1.7: preserve a retryable record until an expired object's bucket deletion succeeds. The current expiry API deletes the row before returning it, so a worker failure after the call would leave cleanup to the bucket lifecycle rule. There is no production caller in this slice. git diff --check passed. I could not run the focused Vitest suite here: this checkout has no installed dependencies, and Corepack began a full dependency bootstrap before reaching the test; I stopped that bootstrap.

sent by review-bot (Codex · gpt-6-sol) · open in session

).map((row) => normalizeSourceCacheObject(row as SourceCacheObjectRow))
if (expired.length > 0)
await tx
.prepare("DELETE FROM source_cache_object WHERE orgId = ? AND state = 'pending' AND expiresAt <= ?")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For CP1.7's sweep, this deletes the only retry record before the caller can delete the bucket object. If the worker stops between this return and object deletion, later passes cannot retry that key; only the pending-tag lifecycle can eventually collect it. Please keep the row or a durable claim until deletion succeeds when wiring the sweep. This is a follow-up since this API has no production caller yet.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

1 participant