feat(app-hosting): idle reaper and runtime lifecycle (ships dark) - #2503
Conversation
|
Warning Review limit reachedNext included review available in 17 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (27)
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 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a770b1cc6f
ℹ️ 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".
…cap a door out Two flag-flip landmines from the review of #2503, both of which would only have appeared the day APP_HOSTING_ENABLED was turned on. F1 — a reaped app came back unmetered and unreapable. `stopped` is servable, and Fly's proxy auto-starts a stopped target the moment a replay reaches it. Nothing wrote `running`, so the heartbeat never billed the machine (it lists `running` rows) and the reaper never stopped it (so does it): every app would have run free, forever, from its first visit after any idle stop. The router now wakes a stopped app through `wakePublishedApp` before replaying — which is where the credit gate was always meant to consume its hold — and refuses to replay when the wake does not succeed, because a replay IS the unmetered start. The wake is serialized per app so a cold page's twenty asset requests produce one Fly start rather than twenty against a ~1/s rate limit; the losers replay, since the machine they wanted is already starting. F2 — cap-parking was a one-way door. The counter resets at midnight UTC and the status does not, and nothing in the system ever wrote `parked -> stopped` for that reason, so one busy day took an app off the internet permanently. The reaper's tick now also releases apps whose budget has rolled over (matched on the `lastError` the park wrote, so an insolvency park is never released by a clock), and a release that fails is loud for the opposite reason a failed stop is. F4 — the day counter keyed off `billedThrough`, which on the repair path is a mirrored boundary that can sit in a previous UTC day: the CASE then zeroed today's accrual and stamped a stale day, so the cap failed OPEN on exactly the broken-lifecycle rows it exists to catch. Keyed off the tick's clock now, the same one the heartbeat's cap projection uses. Also: an integration case for the stop path's counter against real Postgres (F5, including the repair-boundary divergence), the three runtime knobs declared in env-validation.ts and .env.example (F6), and a parked-page copy that tells a daily-cap visitor to come back tomorrow rather than to buy credits. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QE7viA8wZh5x4e5pgyYxD1
…ck idleness under the lock Two findings from the automated review pass on #2503. P1 — a park after a FAILED watermark advance double-charges. `advanceSettledWatermark` caught the write's error and returned normally, so the park went ahead, and `stopPublishedApp` then re-read the unchanged watermark and settled the same span a second time. The double charge landed exactly in the `settledButUnadvanced` case, which is the one already known to be going wrong. The advance now reports what happened and a throw blocks the park — for the insolvency park as much as the new cap park. `superseded` still parks: that row's watermark is AHEAD of ours, so the stop has nothing of ours left to re-bill, and refusing there would leave an insolvent payer's machine awake on a technicality. P2 — the reaper could stop a machine that had just become active. The scan and the stop are minutes apart on a large fleet and the router stamps `lastHitAt` in between, so `planIdleStop` was approving a stale snapshot. The stop now takes the reaper's own cutoff and re-checks recency against the row AS IT IS under the lock, refusing as `became_active` — which the reaper counts as active, because that is what it is. The residual window is the milliseconds between that read and the Fly call, and its cost is one cold start, not a mis-billing. The third finding (a cap park being a one-way door) was already closed by the unpark sweep in the previous commit. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QE7viA8wZh5x4e5pgyYxD1
…tion mirror (F2)
Stripe does not order webhook deliveries. A customer.subscription.updated carrying
`active` can arrive AFTER the `deleted` that ended the subscription — a redelivery,
a retry after a timeout, or two events simply racing. Written blindly that late
event re-entitles the app, and it re-entitles it FOREVER: nothing further will
arrive to correct it, because the subscription is already dead. The result is an
always-on machine nobody is paying for and nothing that knows.
BOTH GUARDS SHIP, because neither closes the case alone:
- Terminal statuses are ABSORBING (load-bearing). Once a row is canceled /
unpaid / incomplete_expired, only a DIFFERENT stripeSubscriptionId — an actual
new purchase — may re-entitle that app. No clock involved, and it matches the
purchase model: re-buying always mints a new subscription.
- Monotonic `stripeEventCreated` stamps (general). These order everything the
terminal rule says nothing about — a stale `past_due` overwriting a fresh
`active`, two ordinary updates arriving backwards.
The stamp cannot replace the terminal rule: `event.created` has ONE-SECOND
resolution, so a `deleted` and an `updated` emitted a few hundred milliseconds
apart compare as equal and the stamp admits the later-arriving one either way.
That case is covered by a dedicated test.
The mirror write is now a locked read plus a decision plus an upsert in one
transaction — the ordering decision must not itself be subject to the reordering
it exists to refuse — and it returns the AUTHORITATIVE row on every outcome,
including refusals. `syncAppTierToSubscription` now takes that row instead of an
event's status, which makes the real hazard unexpressible: syncing from the event
would have let the write be refused and the tier move anyway, which is worse than
no guard because it looks defended.
Each half is mutation-proved separately: remove the terminal rule and 2 go red
(including the same-second case); remove the stamp and 1 goes red.
Migration regenerated in place (unreleased); renumber to 0276 follows the rebase
onto post-#2503 master.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NTETMrmm8ukvBZP7xN4TSp
Published machines run with `autostop: "off"` so that every awake boundary is an API call we made at an instant we know exactly. The unavoidable consequence is that nothing stops an app unless we do — without this, "scale to zero" is a claim rather than a behaviour and every app anybody has ever visited bills awake-seconds forever. This adds the thing that stops them, the abuse bounds around it, and the lock that stops a stop from double-pricing a heartbeat's window. - Idle reaper: an advisory-locked 5-minute cron stops every published app that has gone quiet. It prices NOTHING itself — it decides which apps are idle and hands each to `stopPublishedApp`, so the stop stays the single settle boundary. - Recency: the router had no record that an app was served (a replayed response never passes back through us), so `published_apps.lastHitAt` is added and stamped on the replay path only, throttled to one write per app per minute. Idle is the later of `lastHitAt` and `lastWakeAt`, judged by a pure planner. - BINDING (PR #2493 review): `stopPublishedApp` now runs its whole sequence under the `meter-published-apps-awake` advisory lock, so a stop-settle can no longer price the same window a heartbeat is pricing. The weekly reconcile's `over_billed` signal should be unreachable for this cause. The heartbeat's own park passes a pass-through serializer — re-acquiring from inside the meter's locked region would answer lock_busy and silently skip the park. - Per-app daily awake cap: a counter advanced in the SAME statement as every watermark, judged at the wake gate and at each heartbeat. Exceeding it stops and PARKS the app (metered only, through the status machine's legal edge). - Insolvency parking landed in #2493 and is NOT duplicated here. Ships dark behind APP_HOSTING_ENABLED: both crons report `disabled` as a green 200 and read nothing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QE7viA8wZh5x4e5pgyYxD1
…epo typecheck The pool doubles' `query` was inferred as a one-parameter function, so reading the lock key out of `mock.calls[n][1]` failed `tsc` while passing vitest — the gate CI runs is the monorepo one, and a per-file test run does not measure it. Also documents why the reaper's scan is sequential and uncapped: a top-N would silently leave the rest of the fleet awake and billing while the counters reported a clean tick. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QE7viA8wZh5x4e5pgyYxD1
…cap a door out Two flag-flip landmines from the review of #2503, both of which would only have appeared the day APP_HOSTING_ENABLED was turned on. F1 — a reaped app came back unmetered and unreapable. `stopped` is servable, and Fly's proxy auto-starts a stopped target the moment a replay reaches it. Nothing wrote `running`, so the heartbeat never billed the machine (it lists `running` rows) and the reaper never stopped it (so does it): every app would have run free, forever, from its first visit after any idle stop. The router now wakes a stopped app through `wakePublishedApp` before replaying — which is where the credit gate was always meant to consume its hold — and refuses to replay when the wake does not succeed, because a replay IS the unmetered start. The wake is serialized per app so a cold page's twenty asset requests produce one Fly start rather than twenty against a ~1/s rate limit; the losers replay, since the machine they wanted is already starting. F2 — cap-parking was a one-way door. The counter resets at midnight UTC and the status does not, and nothing in the system ever wrote `parked -> stopped` for that reason, so one busy day took an app off the internet permanently. The reaper's tick now also releases apps whose budget has rolled over (matched on the `lastError` the park wrote, so an insolvency park is never released by a clock), and a release that fails is loud for the opposite reason a failed stop is. F4 — the day counter keyed off `billedThrough`, which on the repair path is a mirrored boundary that can sit in a previous UTC day: the CASE then zeroed today's accrual and stamped a stale day, so the cap failed OPEN on exactly the broken-lifecycle rows it exists to catch. Keyed off the tick's clock now, the same one the heartbeat's cap projection uses. Also: an integration case for the stop path's counter against real Postgres (F5, including the repair-boundary divergence), the three runtime knobs declared in env-validation.ts and .env.example (F6), and a parked-page copy that tells a daily-cap visitor to come back tomorrow rather than to buy credits. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QE7viA8wZh5x4e5pgyYxD1
…ck idleness under the lock Two findings from the automated review pass on #2503. P1 — a park after a FAILED watermark advance double-charges. `advanceSettledWatermark` caught the write's error and returned normally, so the park went ahead, and `stopPublishedApp` then re-read the unchanged watermark and settled the same span a second time. The double charge landed exactly in the `settledButUnadvanced` case, which is the one already known to be going wrong. The advance now reports what happened and a throw blocks the park — for the insolvency park as much as the new cap park. `superseded` still parks: that row's watermark is AHEAD of ours, so the stop has nothing of ours left to re-bill, and refusing there would leave an insolvent payer's machine awake on a technicality. P2 — the reaper could stop a machine that had just become active. The scan and the stop are minutes apart on a large fleet and the router stamps `lastHitAt` in between, so `planIdleStop` was approving a stale snapshot. The stop now takes the reaper's own cutoff and re-checks recency against the row AS IT IS under the lock, refusing as `became_active` — which the reaper counts as active, because that is what it is. The residual window is the milliseconds between that read and the Fly call, and its cost is one cold start, not a mis-billing. The third finding (a cap park being a one-way door) was already closed by the unpark sweep in the previous commit. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QE7viA8wZh5x4e5pgyYxD1
…s persisted guard Rebased onto #2502, which changed `trackUsage` from `Promise<void>` to a reported outcome: a resolved settle is no longer a settled one, and only `persisted: true` means an `ai_usage_logs` row exists. Everything this PR added downstream of a settle now sits behind that guard, which matters in both directions. The daily-cap projection must not count seconds that were never charged — parking an app for spend that does not exist takes it off the internet for nothing — and the watermark advance must not forgive a span nobody billed. The heartbeat's early return covers both, and the final settle's counter patch rides on `billedSeconds`, which #2502 leaves at 0 on a lost charge. Verified by two more mutations: ignoring the persisted guard in the meter turns the new cap case red alongside #2502's own, and counting the day on an unpersisted final settle turns the new integration case red. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QE7viA8wZh5x4e5pgyYxD1
bfb9b5c to
216908c
Compare
…tion mirror (F2)
Stripe does not order webhook deliveries. A customer.subscription.updated carrying
`active` can arrive AFTER the `deleted` that ended the subscription — a redelivery,
a retry after a timeout, or two events simply racing. Written blindly that late
event re-entitles the app, and it re-entitles it FOREVER: nothing further will
arrive to correct it, because the subscription is already dead. The result is an
always-on machine nobody is paying for and nothing that knows.
BOTH GUARDS SHIP, because neither closes the case alone:
- Terminal statuses are ABSORBING (load-bearing). Once a row is canceled /
unpaid / incomplete_expired, only a DIFFERENT stripeSubscriptionId — an actual
new purchase — may re-entitle that app. No clock involved, and it matches the
purchase model: re-buying always mints a new subscription.
- Monotonic `stripeEventCreated` stamps (general). These order everything the
terminal rule says nothing about — a stale `past_due` overwriting a fresh
`active`, two ordinary updates arriving backwards.
The stamp cannot replace the terminal rule: `event.created` has ONE-SECOND
resolution, so a `deleted` and an `updated` emitted a few hundred milliseconds
apart compare as equal and the stamp admits the later-arriving one either way.
That case is covered by a dedicated test.
The mirror write is now a locked read plus a decision plus an upsert in one
transaction — the ordering decision must not itself be subject to the reordering
it exists to refuse — and it returns the AUTHORITATIVE row on every outcome,
including refusals. `syncAppTierToSubscription` now takes that row instead of an
event's status, which makes the real hazard unexpressible: syncing from the event
would have let the write be refused and the tier move anyway, which is worse than
no guard because it looks defended.
Each half is mutation-proved separately: remove the terminal rule and 2 go red
(including the same-second case); remove the stamp and 1 goes red.
Migration regenerated in place (unreleased); renumber to 0276 follows the rebase
onto post-#2503 master.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NTETMrmm8ukvBZP7xN4TSp
…he reaper exemption once MIGRATION (5). My 0275 is regenerated as 0276 on top of #2503's 0275, same prevId chain, `drizzle-kit check` clean. The regenerated snapshot carries #2503's columns (lastHitAt, awakeSecondsDay/Today) alongside this branch's table, and the published-apps constraints block now holds both sides: #2503's counter CHECKs and this branch's guest-preset pair. REAPER REWIRE (6). I land second, so the duplicated rule is mine to collapse. `IDLE_REAPER_EXEMPT_TIERS` is now the single source: `isIdleReaperExempt` tests against it, `planDailyAwakeCap` asks through that predicate instead of an inline `!== 'metered'`, and the reaper's candidate query builds its `notInArray` from the same array — SQL cannot call the function, so it reads the constant rather than spelling the rule a third time. `DailyAwakeCapInput.tier` is narrowed from `string` to `PublishedAppTier` so the predicate could be used at all; "any string" was the wrong domain for a question about whether an app may be switched off. Proved rather than performed: empty that array and exactly three tests go red — the reaper's candidate source (against a real Postgres), the daily cap, and the predicate itself. SURVIVAL CHECKS (7). Both double-charge predicates survived the merge and are re-verified against a real database: `listRunningApps` and `listPublishedAppRootfs` still filter `tier = 'metered'`. The wake seam kept BOTH sides of its conflict — #2503's daily-cap check, which parks before the ledger is touched, and this branch's metered-only gate — and the `gate.holdId` → `holdId` refactor compiles clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NTETMrmm8ukvBZP7xN4TSp
…tion mirror (F2)
Stripe does not order webhook deliveries. A customer.subscription.updated carrying
`active` can arrive AFTER the `deleted` that ended the subscription — a redelivery,
a retry after a timeout, or two events simply racing. Written blindly that late
event re-entitles the app, and it re-entitles it FOREVER: nothing further will
arrive to correct it, because the subscription is already dead. The result is an
always-on machine nobody is paying for and nothing that knows.
BOTH GUARDS SHIP, because neither closes the case alone:
- Terminal statuses are ABSORBING (load-bearing). Once a row is canceled /
unpaid / incomplete_expired, only a DIFFERENT stripeSubscriptionId — an actual
new purchase — may re-entitle that app. No clock involved, and it matches the
purchase model: re-buying always mints a new subscription.
- Monotonic `stripeEventCreated` stamps (general). These order everything the
terminal rule says nothing about — a stale `past_due` overwriting a fresh
`active`, two ordinary updates arriving backwards.
The stamp cannot replace the terminal rule: `event.created` has ONE-SECOND
resolution, so a `deleted` and an `updated` emitted a few hundred milliseconds
apart compare as equal and the stamp admits the later-arriving one either way.
That case is covered by a dedicated test.
The mirror write is now a locked read plus a decision plus an upsert in one
transaction — the ordering decision must not itself be subject to the reordering
it exists to refuse — and it returns the AUTHORITATIVE row on every outcome,
including refusals. `syncAppTierToSubscription` now takes that row instead of an
event's status, which makes the real hazard unexpressible: syncing from the event
would have let the write be refused and the tier move anyway, which is worse than
no guard because it looks defended.
Each half is mutation-proved separately: remove the terminal rule and 2 go red
(including the same-second case); remove the stamp and 1 goes red.
Migration regenerated in place (unreleased); renumber to 0276 follows the rebase
onto post-#2503 master.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NTETMrmm8ukvBZP7xN4TSp
…he reaper exemption once MIGRATION (5). My 0275 is regenerated as 0276 on top of #2503's 0275, same prevId chain, `drizzle-kit check` clean. The regenerated snapshot carries #2503's columns (lastHitAt, awakeSecondsDay/Today) alongside this branch's table, and the published-apps constraints block now holds both sides: #2503's counter CHECKs and this branch's guest-preset pair. REAPER REWIRE (6). I land second, so the duplicated rule is mine to collapse. `IDLE_REAPER_EXEMPT_TIERS` is now the single source: `isIdleReaperExempt` tests against it, `planDailyAwakeCap` asks through that predicate instead of an inline `!== 'metered'`, and the reaper's candidate query builds its `notInArray` from the same array — SQL cannot call the function, so it reads the constant rather than spelling the rule a third time. `DailyAwakeCapInput.tier` is narrowed from `string` to `PublishedAppTier` so the predicate could be used at all; "any string" was the wrong domain for a question about whether an app may be switched off. Proved rather than performed: empty that array and exactly three tests go red — the reaper's candidate source (against a real Postgres), the daily cap, and the predicate itself. SURVIVAL CHECKS (7). Both double-charge predicates survived the merge and are re-verified against a real database: `listRunningApps` and `listPublishedAppRootfs` still filter `tier = 'metered'`. The wake seam kept BOTH sides of its conflict — #2503's daily-cap check, which parks before the ledger is touched, and this branch's metered-only gate — and the `gate.holdId` → `holdId` refactor compiles clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NTETMrmm8ukvBZP7xN4TSp
Published machines run with
autostop: "off"so that every awake boundary is an API call we made, at an instant we know exactly. The unavoidable consequence is that nothing stops an app unless we do — until this PR, "scale to zero" was a claim rather than a behaviour, and every app anybody had ever visited would bill awake-seconds forever. This adds the thing that stops them, the abuse bounds around it, and the lock that stops a stop from double-pricing a heartbeat's window.Ships dark behind
APP_HOSTING_ENABLED. Both crons reportdisabledas a green 200 and read nothing.What landed
1. The idle reaper (
services/app-hosting/idle-reaper.ts+/api/cron/reap-idle-apps, every 5 min)An advisory-locked cron that stops every published app that has gone quiet. It prices nothing itself: it decides which apps are idle and hands each to
stopPublishedApp, so the stop stays the single settle boundary. A parallel settle here would be a second answer to "what does this window owe", which for money is a wrong one.2. The recency the reaper reads (
published_apps.lastHitAt)#2491's router records nothing — a replayed response goes straight from the target app to the client and never passes back through us, and Fly's machine events record starts and stops, not traffic. So the router now stampslastHitAton the replay path only, throttled to one write per app per minute (PUBLISHED_APP_HIT_STAMP_INTERVAL_SECONDS): the hot path pays an indexed statement that matches nothing, all but once a minute. A refusal is not demand and stamps nothing — otherwise a parked app's crawler traffic would keep it looking busy forever.Idle is
now - GREATEST(lastHitAt, lastWakeAt) > PUBLISHED_APP_IDLE_STOP_SECONDS(default 900s), judged by a pure planner.lastHitAtalone would reap an app between the moment it was woken and the moment its first request is routed;lastWakeAtalone would reap a busy app 15 minutes after it woke. A row with neither stamp is left alone and counted — the honest response to a live machine we have no boundary for is not to stop it, and the heartbeat back-fills one on its next tick.3. BINDING (from #2493's review): the stop takes the meter's lock
stopPublishedAppnow runs its whole sequence — the read, the Fly stop, the settle — undermeter-published-apps-awake. The double-charge is created by the READ (a stop and a heartbeat pricing the same window from two snapshots), so a lock taken after the read would protect nothing. The weekly reconcile'sover_billedsignal should now be unreachable for this cause.The heartbeat's own park passes
passThroughSettleLock, because it runs inside the meter's locked region: re-acquiring would take a fresh connection, see the lock held by us, answerlock_busyand silently skip the park while the tick reported success.stopPublishedAppgains alock_busyoutcome, handled explicitly by the reaper (the app is stillrunning, so the next tick reaps it).4. Per-app daily awake cap (
PUBLISHED_APP_DAILY_AWAKE_SECONDS_CAP, default 43,200s = 12h, 0 disables)The per-app analog of
dailyExposureCapForTier, which bounds a payer — a single runaway app can exhaust that and take down every other app its owner has. This bounds the app that caused it. Implemented as a counter (awakeSecondsDay+awakeSecondsToday) advanced in the same statement as every watermark, because seconds that were charged but not counted are seconds the cap cannot see. Exceeding it stops and parks the app, at the wake gate and at each heartbeat, metered tier only (parked_is_metered_onlymakes that structural).5. Insolvency parking is NOT duplicated — it landed in #2493.
Decisions worth reviewing
createNotificationonly takes a member of theNotificationTypepg enum, so a first-class notification means an enum migration + a union member + a list renderer + an email-preference row — user-visible UI work, in a PR whose safety property is that it ships dark, for a state no user can reach yet. Instead:published_apps.lastErrorcarries a plain reason (the column the publish surface already reads to answer "why is my app not serving"), the crons audit acappedParkedcounter, and the meter route raises one Sentry warning fingerprinted on the cause. Not a TODO — adding the enum value later costs one line at the call site.listIdleCandidates; the sibling dedicated-tier task is addingisIdleReaperExempt(tier)and whichever PR lands second rewires that line (flagged on the questions page — neither branch can import the other today).Verification
idle-reaper(14),app-metering-core(+18 forplanIdleStop/planDailyAwakeCap/utcDayOf),app-lifecycle-metering(+13),awake-meter(+6),router(+4),app-hosting-env(+13), and the new cron route's contract tests.lastHitAtalone; stamp recency before the routing decision.make_interval, the day-resetCASE, the nested guarded counterCASE).knip:checkclean; monorepotypecheck/lint/test:unitgreen.