Repository navigation
[sprites 6-1] Storage metering: bytes written, not allocation - #1990
Conversation
📝 WalkthroughWalkthroughTerminal storage billing now relies on opportunistic filesystem measurements persisted on terminal sessions. Sandbox operations trigger best-effort measurement, reconciliation bills measured footprints without waking machines, and cron responses report stale measurement counts. ChangesTerminal storage metering
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant SandboxOperation
participant StorageMeasurement
participant Machine
participant TerminalSessions
participant ReconcileCron
SandboxOperation->>StorageMeasurement: trigger best-effort measurement
StorageMeasurement->>Machine: execute du on active sandbox
Machine-->>StorageMeasurement: return filesystem byte output
StorageMeasurement->>TerminalSessions: persist bytes and timestamp
ReconcileCron->>TerminalSessions: read measured storage and watermark
ReconcileCron-->>SandboxOperation: charge measured GB-months and report stale count
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![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: 0e3d4c94ee
ℹ️ 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".
| if (costDollars <= 0) { | ||
| if (elapsedMs > 0) { | ||
| await deps.advanceWatermark({ pageId: machine.pageId, billedThrough: now }); |
There was a problem hiding this comment.
For machines used only through the realtime PTY or machine APIs, this branch can run every cron tick with measuredBytes still null/zero and then advance storageLastBilledAt, permanently forgiving that storage window. I checked repo-wide (rg measureMachineStorageOpportunistically|refreshStorageMeasurement|storageMeasured): the only production measurement wiring is the agent tool runtime, while apps/realtime/src/index.ts and apps/web/src/lib/machines/* acquire/wake terminal sandboxes without persisting storageMeasured*. A user who writes data in the terminal but never invokes an agent tool will therefore be billed $0 forever after this migration.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in d046ffe3a. Wired the terminal-CONNECT wake: apps/realtime/src/index.ts buildMachineSandbox.acquire() now calls measureMachineStorageOpportunistically after acquireTerminalSandbox succeeds, so PTY-only machines (no agent tool ops) are measured on connect instead of staying never-measured. The network attach is lazy — measureMachineStorageOpportunistically gained a resolveHandle thunk it invokes only after the in-process throttle + terminal_sessions row-existence checks pass, so a throttled connect pays no attach. This closes the "$0 forever" path for the interactive-terminal population. (The never-measured watermark advance is retained deliberately to bound the first-measurement retroactive charge to ≤ one cron interval; with connect-time measurement the never-measured window is now transient, not permanent.) Leaving this open for your verification.
| if (acquired.pageId && deps.measureStorage) { | ||
| const pageId = acquired.pageId; | ||
| void deps.measureStorage({ sandbox, pageId }).catch((error) => { |
There was a problem hiding this comment.
This fires the storage measurement as soon as openSession reconnects, before runBashInSandbox executes the command and before writeSandboxFile/editSandboxFile mutate the filesystem. For a first operation that writes a lot of data, the persisted measurement can be the old/zero footprint with a fresh storageMeasuredAt; the one-hour throttle then suppresses a post-write measurement, and if the reconcile runs before another wake it advances the watermark using the stale footprint, permanently dropping that interval from storage billing.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in d046ffe3a. Moved the measurement out of openSession (before the op) into the session release thunk, which all four runners call in finally AFTER the op completes. It therefore observes any bytes the op just wrote and stamps storageMeasuredAt post-write, so the throttle no longer suppresses the write from being reflected. As a bonus this also removes the concurrent-exec contention (the measurement now runs sequentially after the op, not alongside it). Leaving this open for your verification.
Proactive review round (high-effort adversarial review) — fixes pushed (
|
ca01de0 to
0c0c689
Compare
…ned allocation (#890 / sprites 6-1) The storage reconcile billed every machine a flat 5 GB-months accrual (SANDBOX_RESOURCE_CAPS.storageGB) on the false premise that allocation is what costs. The platform bills the bytes a machine has ACTUALLY written (TRIM-friendly), not the 100GB-style allocation (docs.sprites.dev/concepts/lifecycle) — so a machine that wrote 200MB was metered at 5GB. - Bill from the last PERSISTED measured footprint, never the provisioned cap, and NEVER wake a sprite to measure (the cron has no sprite handle by construction). New pure fns: computeElapsedGbMonths({measuredGB,elapsedMs}) and pickBillableGB({lastMeasuredGB,lastMeasuredAt,awake,now})->{gb,stale}. - Opportunistic measurement (terminal-storage-measure.ts): pure df parser + bytes→GB + throttle decision, plus a DI'd shell that runs one cheap `df` through an ALREADY-awake machine handle and persists {bytes,at}. Wired into the agent tool-runner's session-open seam (real work = sprite already awake), throttled and fully best-effort. - Never-measured machines bill a conservative 0 floor (not the cap) and still advance their watermark, so a later measurement never bills the un-measured span retroactively — clean cutover from allocation-billing, idempotency preserved. - Schema: terminal_sessions.storageMeasuredBytes/At (migration 0194). - staleMeasurements health counter surfaced in the cron log + audit. - Updated the docstrings that asserted the allocation premise; noted HOT/COLD are infra layers, not billing tiers (per-tier split is a follow-up). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HarN68Tuc5judwB1V7qL4w
…tree, keep sub-cent residuals, throttle in-process Adversarial code-review pass (high effort) on the measured-bytes cutover surfaced several billing-correctness issues. Fixes: - Measure the workspace SUBTREE via `du -sbx /workspace`, not `df` of the whole filesystem. `df` would also count the read-only OS/base-image bytes if the workspace is a dir on the root overlay rather than a dedicated mount — over- billing every machine by the base-image size, the very "bill allocation not bytes-written" bug this leaf removes. `du` counts only what the workload wrote. (Trade-off: bytes written outside the workspace aren't counted — a deliberate conservative under-count, consistent with the never-measured 0 floor.) - Don't discard sub-cent residuals: the zero-charge watermark advance now applies ONLY to never-measured rows (the cutover case). A MEASURED tiny footprint whose per-window cost rounds to $0 no longer advances the watermark, so the residual accrues across windows until it crosses the pricing floor and actually bills — previously a small machine on a frequent cron billed $0 forever. - In-process per-page throttle cache in measureMachineStorageOpportunistically: the tool-runner calls it on every op, but a machine needs measuring once per window — the cache short-circuits before the DB read, so a 30-tool-call turn does one measurement attempt, not 30 wasted SELECTs. Persisted measuredAt is still the authoritative throttle (survives restart / multi-instance). - Documented the bounded (≤ one cron interval, one-time) retroactive charge when a first measurement lands, and the "never wake to measure" trade-offs (PTY-only coverage gap → 0 floor; shrink-lag billing last-known, surfaced by staleMeasurements) instead of over-claiming "never retroactive". Not changed (scope): realtime-PTY-connect measurement lives in the lane-shared agent-terminal-handler.ts (out of this leaf); the agent-run path is wired and the helper is ready for other wake paths to call. Concurrent `du` is a separate spawned process on the VM (sprite.spawn), so it does not serialize with the primary op. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HarN68Tuc5judwB1V7qL4w
…p (review threads) Two reviewer findings on the measured-bytes cutover: - P1 — PTY-only machines were never measured (measurement was wired only at the agent tool-runner), so a machine used solely through the interactive terminal stayed never-measured and the reconcile advanced its watermark every tick, billing $0 forever. Wire the terminal-CONNECT wake in apps/realtime's buildMachineSandbox.acquire() to measure opportunistically. The network attach is lazy — measureMachineStorageOpportunistically now takes a `resolveHandle` thunk it calls ONLY after the in-process throttle + row-existence checks pass, so a throttled connect pays no attach. - P2 — the agent-path measurement fired in openSession BEFORE the op ran, so it captured the pre-write footprint and the throttle then suppressed the post-write measurement. Move it into the session `release` thunk, which the runners call in `finally` AFTER the op, so it observes what the op wrote and runs sequentially (not concurrently) with it. Tests: resolveHandle lazy-attach path (attaches only when due + row exists; not on a throttled repeat or a missing row); existing tool-runner seam tests still green (measurement fires via release). typecheck lib/web/realtime + lint clean; sandbox suite 446/446. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HarN68Tuc5judwB1V7qL4w
…ttle too Follow-up to the terminal-connect wiring: check the persisted storageMeasuredAt throttle BEFORE calling resolveHandle, so a caller with a cold in-process cache (e.g. a freshly-restarted realtime node) doesn't pay a wasted network attach when another instance already measured the page within the window. refreshStorageMeasurement still re-checks; this purely gates the attach. Adds a regression test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HarN68Tuc5judwB1V7qL4w
…ion review A second high-effort adversarial review of the review-fix diff found 5 confirmed billing-correctness bugs I introduced; all fixed: - Watermark freeze → retroactive over-bill (the worst): my previous change stopped advancing storageLastBilledAt for measured sub-cent footprints. A machine at a tiny footprint that later grew (e.g. to 100GB) would then be billed the new size across the ENTIRE frozen span. Revert to advancing the watermark on every zero-cost window (measured or not): the residual lost is a sub-cent for sub-2.4MB footprints that genuinely cost ~$0, whereas freezing over-charges the payer for storage they didn't hold. Added a regression test proving a grown footprint bills only the post-growth window. - du apparent-size over-bill: `du -sbx` (-b = apparent size) billed a sparse 100GB file occupying 1GB of real blocks as 100GB. Switched to `du -sxB1` (actual allocated bytes), matching the real disk footprint the platform bills. - Partial du under-bill: parsing du output regardless of exit code persisted an under-counted total when du couldn't read part of the tree. Restored the exit-code guard (skip + retry next window on non-zero). - Transient-failure lockout: the in-process throttle stamped BEFORE the exec, so a transient measure failure suppressed re-measurement for the whole window on that instance. Now stamp only on a definitive outcome (measured success / no-row / already-fresh); transient failures stay retryable. Added a regression test. - Unbounded Map memory leak: the in-process throttle Map grew per distinct pageId forever. Bounded to 10k entries with oldest-first eviction. Tests: sandbox suite 449/449; typecheck + lint clean (lib/web/realtime). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HarN68Tuc5judwB1V7qL4w
… (review round 3) A third adversarial review showed the round-2 du exit-code guard over-corrected, and flagged a lost concurrent-dedup. Fixes: - Persist any PARSEABLE du total, even on non-zero exit. `du -s` always prints a valid cumulative total of what it read; a non-zero exit only means it skipped unreadable entries, so the total is a conservative LOWER BOUND, never garbage. The prior "reject non-zero" guard meant a permanently-unreadable subtree (chmod 000 / root-owned path) billed the 0 floor or a stale value FOREVER and — since the caller only caches on measured:true — re-walked the whole workspace with `du` on every tool op for the entire window. Persisting the lower bound is the right conservative trade-off (self-corrects when readable) and caches, so no re-walk storm. True failures (exec threw / no numeric total) still return not-measured and stay retryable. This resolves the round-1/2/3 flip-flop. - Restore synchronous concurrent-dedup: the window clock is stamped only after the awaits, so a burst of N parallel ops for one page all passed the gate and each spawned a DB read + attach + du walk. Added an in-flight Set added-to before the first await and cleared in finally, so a burst collapses to one measurement. (Kept off the persisted-throttle path so a transient failure still retries — the round-2 fix.) - Added __resetStorageMeasurementCachesForTests seam so the in-process caches are cleared between tests (no cross-case bleed); regression tests for the non-zero-exit persist and the 5-parallel-calls→1-du dedup. Cleanup notes (bounded-LRU vs quota.ts's TTL sweep) left as-is: the LRU is correct and bounded; aligning the two in-process maps is a separate cleanup. Tests: sandbox suite 450/450; typecheck + lint clean (lib/web/realtime). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HarN68Tuc5judwB1V7qL4w
0c0c689 to
7d3661b
Compare
1 participant
Summary
The storage reconcile billed every machine a flat 5 GB-months accrual (
SANDBOX_RESOURCE_CAPS.storageGB) on the premise that Sprites' provisioned volume is what costs. It isn't: the platform bills the bytes a machine has actually written (TRIM-friendly — deleting files lowers the bill), not the allocation (docs.sprites.dev/concepts/lifecycle — "you pay for the bytes you actually write, not the full 100 GB"). A machine that wrote 200MB was being metered at 5GB. This fixes the meter to measured usage, and never wakes a sprite to measure it.Requirements → how satisfied
Bill from last persisted measured bytes, never
SANDBOX_RESOURCE_CAPS.storageGB, and never wake a sprite (zero sprite calls from the cron).computeElapsedGbMonths({ measuredGB, elapsedMs })(wasstorageGB) and newpickBillableGB({ lastMeasuredGB, lastMeasuredAt, awake, now }) -> { gb, stale }.ReconcileTerminalStorageDepsno longer has astorageGBfield at all — the cron's deps expose no sprite handle, so it cannot exec against a machine (compile-time guarantee, asserted by test). It reads only persistedstorageMeasuredBytes/storageMeasuredAtoffterminal_sessions.Sprite awake for real work → opportunistically refresh measurement (cheap exec, throttled — pure throttle decision).
terminal-storage-measure.ts: pureparseDuBytes(du -sbxof the workspace subtree),bytesToGB(decimal GB),shouldRefreshMeasurement({ lastMeasuredAt, now, throttleMs }), plus a DI'drefreshStorageMeasurementshell that runs onedu -sbx /workspacethrough an already-awakeMachineHandle.execand persists. (Measures the workspace subtree, not the whole filesystem, so it never counts the OS/base-image bytes — see the review-round comment.)measureStorage?seam onSandboxRunDeps(mirrors the existingnotifyTerminalActivity?/billing?seams), invoked inopenSession— a sprite is awake there because real work is happening on it.ExecutableSandbox.runCommandsharesMachineHandle.exec's signature. Throttled to 1 measure/machine/hour (TERMINAL_STORAGE_MEASURE_THROTTLE_MS), fire-and-forget, never blocks or fails the op.Never-measured machine → conservative documented floor.
pickBillableGB(null) -> { gb: 0, stale: true }. Documented in the schema column, the reconcile module doc, andpickBillableGB.Existing accrual rows transition cleanly (no double-billing at cutover; idempotency preserved).
Also
terminal_sessions.storageMeasuredBytes(bigint) +storageMeasuredAt(timestamp), nullable — migration0194_yielding_the_leader.sql(generated, not hand-edited).staleMeasurementshealth counter (measured-but-stale rows) surfaced in the cron log line + auditdetails.terminal-storage-reconcile.ts, the cron route,TERMINAL_STORAGE_USD_PER_GB_MONTH); noted HOT/COLD are infra layers not billing tiers (per-tier split = follow-up, per Out-of-scope).Out of scope (untouched)
The credit price constant's value (kept env-tunable); the HOT/COLD tier split.
Test evidence
bun run typecheck(db + lib + web): clean. Lint (lib + web): clean (only pre-existing unrelated warnings).Full sandbox suite:
New/updated files specifically:
For the orchestrator to verify
measureStorage?seam toSandboxRunDeps+ a call inopenSession(packages/lib/src/services/sandbox/tool-runners.ts) and its composition inapps/web/.../sandbox-tools-runtime.ts. This is the shared agent tool-runner path — additive only (no signature changes to existing seams), but flag if another sprites leaf is mid-edit there.🤖 Generated with Claude Code
Update — proactive review round (
f6fd166c3)A high-effort adversarial review surfaced 7 billing-correctness findings. Fixed in code: (1) measure workspace subtree via
du -sbxnot whole-FSdf(avoids counting base-image bytes); (2) sub-cent residuals no longer discarded — zero-charge watermark advance now applies only to never-measured rows; (3) in-process per-page throttle cache removes the per-tool-op DB SELECT; (4) honest docs on the bounded (≤ one cron interval, one-time) retroactive charge. Acknowledged/scoped: PTY-only measurement lives in the lane-shared realtime handler (follow-up); billing-last-known-after-shrink is inherent to "never wake"; concurrentduis a separatesprite.spawnprocess. Full detail in the review-round comment.Update — reviewer threads addressed (
d046ffe3a)measureMachineStorageOpportunisticallyintoapps/realtime/src/index.tsbuildMachineSandbox.acquire()so interactive-PTY-only machines are measured on connect (were never-measured → billed the 0 floor forever). The attach is lazy via a newresolveHandlethunk, gated behind the throttle + row-existence checks.openSession(pre-op) into the sessionreleasethunk (called infinally, post-op) so it captures what the op wrote and runs sequentially rather than concurrently with it.Update — verification-review corrections (
9e495b9b7)A second adversarial review of the review-fix diff caught 5 billing-correctness bugs I'd introduced; all fixed: (1) reverted the measured-sub-cent watermark freeze (it would retroactively over-bill a footprint that later grows) — the reconcile now advances the watermark on every zero-cost window, losing only a negligible sub-cent residual; (2)
du -sbx(apparent size) →du -sxB1(actual allocated bytes) so sparse files aren't over-billed; (3) restored the du exit-code guard so a partial "cannot-read" walk isn't persisted as an under-count; (4) the in-process throttle now stamps only on a definitive outcome so a transient measure failure doesn't lock out re-measurement for the window; (5) bounded the throttle Map (10k entries, oldest-first eviction) to prevent a long-lived-process memory leak. Regression tests added for the no-retroactive-over-bill and transient-retry cases.Update — review round 3 (
0a5de6f37)Resolved the
duexit-code flip-flop definitively and restored concurrent-dedup: (1) persist any parseabledutotal even on non-zero exit — it's a valid conservative lower bound (only unreadable entries omitted), so a permanently-unreadable subtree no longer bills $0 forever nor re-walks the workspace every op; true failures (no numeric output) still retry; (2) synchronous in-flightSetcollapses a burst of N parallel ops for one page to a single measurement (the window clock only stamps after the awaits); (3) added a test-reset seam for the in-process caches. Regression tests for both.Update — rebased onto master's TERMINAL→MACHINE rename (
ca01de0e1)Rebased onto master (post-#1992
PageType.TERMINAL→MACHINE+ FKterminalId→machineIdrename). All 6-1 storage-metering work is intact — this was a pure rename/rebase adaptation, no scope dropped:0194_yielding_the_leader→0196_yielding_the_leader(after master's new 0194 + 0195);_journal.jsongets a matching idx-196 tail entry and a fresh0196_snapshot.jsonchained off 0195 (prevId= 0195 snapshot id) carrying the two new nullableterminal_sessionscolumns. Master's 0194/0195 are untouched; the rename was not regenerated.machineIdlocal. My storage-billing files (terminal-storage-*.ts,credit-pricing.ts) key onpageId/machineIdand needed no field renames.buildAgentTerminalSessionKey({ terminalId })shorthand →{ terminalId: machineId }, andPageType.TERMINAL→MACHINEinterminal-keepalive.ts+TerminalKeepAliveHost.tsx.Validation after rebase:
bun run typecheck→ 16/16 packages, 0 errors; sandbox suite 519/519;turbo build --filter=@pagespace/db --filter=@pagespace/libgreen; lint clean. No merge conflicts with master.Update — rebased again onto master (
7d3661bf6; #1991 Phase 3 + #1993 rename-fix)Master advanced to
d357a6741(Phase 3 ClickHouse #1991 + #1993 which fixed the same three rename-misses I had). Re-rebased:0196_yielding_the_leader→0197_yielding_the_leader(after master's new0196_married_blink);_journal.jsonidx-197 tail + fresh0197_snapshot.jsonchained off 0196 (prevId= 0196 id).bun installpicked up master's new@clickhouse/clientdep.Re-validated:
bun run typecheck16/16, 0 errors; sandbox suite 519/519; db+lib build green; lint clean. No conflicts.Summary by CodeRabbit