Repository navigation
fix(packs): cap view writes at the pack quota; review follow-ups to #437 - #444
Conversation
Fails today: the quota is only enforced by the hourly retention pass and keep collections are never evicted, so twenty 15 KB view writes all land in a 100 KB pack. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
The quota was only enforced by the hourly retention pass, and keep collections are never evicted, so a view writing a keep + viewWritable collection could grow a pack without bound (and on an evictable one, push the pass into evicting the pack's own documents). View writes now check the pack's live bytes inside the write transaction and refuse with QuotaExceeded when the write would grow the pack past quotaBytes; rewriting a document no larger than before is still allowed. Script writes keep the eviction-based quota, and can opt in with capAtQuota. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…t a defect Reproduced the review's concern: a 20_000-deep array under the byte cap fails with a typed InvalidInput (the JSON encoder reports it as not serializable); nesting up to ~5_000 is stored and served. No depth cap is needed; this pins the typed failure. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
github-actions
Bot
added
vouch:trusted
Thread transfer impact
This comment will update automatically after the next completed run. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c9a71090ca
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| sql`SELECT COALESCE(SUM(byte_size), 0) AS "total", | ||
| COALESCE(SUM(CASE WHEN collection = ${collection} AND doc_key = ${key} | ||
| THEN byte_size END), 0) AS "current" | ||
| FROM t3team_pack_documents WHERE pack_id = ${packId} AND ${live(now)}`.pipe( |
There was a problem hiding this comment.
When a pack update removes or renames a collection, its persisted rows remain, and the retention pass deliberately counts only currently declared collections (t3team-packDocumentRetention.ts:49-54). This query instead sums every live row for the pack, so inaccessible data from a retired collection can make all writes to a current viewWritable collection fail with QuotaExceeded; because validation prevents accessing the retired collection and retention will not remove it, the pack cannot recover through the store API. Restrict the total to the current configuration's declared collections.
Useful? React with 👍 / 👎.
| const { total, current } = yield* query.usage(collection, key, now); | ||
| const growth = encoded.bytes - current; | ||
| if (growth > 0 && total + growth > quotaBytes) | ||
| return yield* refuse("put", "QuotaExceeded", `${total + growth} > ${quotaBytes} bytes`); |
There was a problem hiding this comment.
When a view submits a stale ifVersion—or ifVersion: 0 for an existing key—with a larger payload while the pack is near its quota, this check returns QuotaExceeded before the conditional update discovers that the CAS lost. The RPC contract and existing store behavior use null for that outcome, so contention is misreported as storage exhaustion and clients cannot follow their normal CAS retry path. Apply the quota check only after establishing that the requested version can be written, or include the version condition in the accounting/write operation.
Useful? React with 👍 / 👎.
Follow-ups from the #437 review, in its priority order.
1. MEDIUM: the quota is now a cap for view writes
Before this change the quota was only enforced by the hourly retention pass, and
keepcollections are never evicted. A view writing akeep+viewWritablecollection could therefore grow a pack without bound. On an evictable collection it could fill the quota within the hour and make the pass evict the pack's own documents.putFromViewnow checks the pack's live bytes inside the write transaction (SUM(byte_size)over unexpired rows) and refuses with the newQuotaExceededreason ("The pack's storage quota is full.") when the write would grow the pack pastquotaBytes. Rewriting a document in place without growing it is still allowed, so a full pack can still be edited and shrunk. Script writes keep the eviction-based quota from the spec. They can opt in through the newputoptioncapAtQuota.Test:
t3team-packDocumentQuota.test.tsloopsputFromView(... doc: "x".repeat(15_000)). It stops after 6 documents in a 100 KB pack with "The pack's storage quota is full.", and rewrites (same size, then smaller) still succeed. It was committed red first: all 20 writes landed. Mutation check: counting the whole new size instead of the growth fails the rewrite assertion.2. LOW: expired documents in live subscriber state, documented
I took the cheaper correct option and documented it. Publishing
removedat the moment of expiry needs a timer or a frequent sweep, andexpires_athas no index for that. The contract comment onT3TeamPackDocumentsStreamEvent(and onexpiresAt) now says:removedarrives when the store deletes the row (the next write to that key, or the hourly pass), so a live subscriber can hold an expired document for up to an hour;expiresAtthemselves.3. LOW: the pack-isolation test for
removednow has a pack-b subscriberIt watches the same collection and prefix. After pack A's removals, pack B's own removal must be its first live event, and the assertion includes
packId. Mutation check: dropping the pack filter in the hub fails it. The first version of this assertion did not catch that mutation, because the summary omittedpackId.4. LOW: deep nesting does not reproduce as a defect
A 20,000-deep array (40 KB, under the cap) already fails as a typed
InvalidInput: the JSON encoder reports it as not serializable. Nesting up to about 5,000 levels is stored, read and serialized fine, and the failures start somewhere between 5,000 and 8,000. The RPC payload decode accepts the deep value too. I added no depth cap, only a regression test that pins the typed refusal throughputFromView.5. Note: views can name another pack's id
Pack web modules run in the host realm (no iframe). A pack view can therefore send any registered pack's
packIdont3team.packStore.put. The server only checks that the id is in the registered set, which matches the spec's security model (views are bound to their registering pack id by the web host; the server re-checks membership). It is the same trust level as any host-realm code, and per-pack isolation for views would need a realm boundary.Verification (local; fork CI skips tests)
Run serially:
vp test runont3team-packDocumentStore(6),…Retention(4),…ViewWrites(5),…Quota(2), and contractst3team-packDocuments(4). All pass. Server typecheck with the Effect checker rules exits 0, and lint and the additive guard are clean.🤖 Generated with Claude Code