Conversation
|
@agentconnect-md-test |
|
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. |
There was a problem hiding this comment.
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
pendinglifecycle 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:
- A claim step returns due
pendingrows and does not delete them. Per CP1.7 this isFOR UPDATE SKIP LOCKEDper row, not serialization on the org lock, so members can sweep in parallel. - 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
repositoryUrlHashcolumn name. §4 now keys acredentry by<provider>:<id>fromCodeHostRepository, 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 asrepositoryKey, or a hash of the<repo>key segment, and fix the §10 wording in the same change.- Pointer rows.
kind = 'pointer'has to pass throughpendingwith anexpiresAt. 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, droppointerfrom 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[]> { |
There was a problem hiding this comment.
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, |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 <= ?") |
There was a problem hiding this comment.
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.
Part of #2732. Implements only CP1.2 — Source Cache data-plane accounting.
Changes
source_cache_objectledger and per-orgsource_cache_usagelock row on both store drivers, including PostgrescanonicalColumnsentries and schema v35 migration coverage.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.tspnpm --filter @agentconnect.md/daemon typecheckpnpm exec eslinton changed source/test files