Run scheduled maintenance securely on every site#1862
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR replaces request-triggered pruning with durable scheduled maintenance, adds authenticated ChangesScheduled maintenance platform
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/shared/maintenance/runner.ts (1)
32-37: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUnexplained magic number
3in budget headroom check.
taskCalls(task) <= remaining.total - 3reserves 3 calls with no named constant or comment explaining what they cover (e.g., claim + finish + margin). In a budget-critical scheduler this is easy to get subtly wrong on a future refactor.♻️ Suggested fix
+/** Reserved subrequest headroom for claim + finish bookkeeping calls. */ +const TASK_BOOKKEEPING_RESERVE = 3; + const taskFits = ( task: MaintenanceTaskDeclaration, remaining: { database: number; external: number; total: number }, ): boolean => task.maxExternalCalls <= remaining.external && - taskCalls(task) <= remaining.total - 3; + taskCalls(task) <= remaining.total - TASK_BOOKKEEPING_RESERVE;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/shared/maintenance/runner.ts` around lines 32 - 37, Replace the unexplained literal 3 in taskFits with a clearly named constant describing the reserved call headroom, and use that constant in the remaining.total calculation. Define it near the related scheduler budget logic and document what the reserved calls cover, preserving the existing capacity behavior.src/shared/db/built-sites.ts (1)
448-466: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftFile keeps growing past the ~400-line guideline with mixed concerns.
This PR adds more concerns (scheduler-key fields, revision tracking, schema v2 blob validation) into
src/shared/db/built-sites.ts, which per the change details already spans past line 650. As per coding guidelines, "Keep files under roughly 400 lines, split overloaded concepts into focused modules, and separate pure logic from I/O." Consider splitting blob schema/validation, plain/blob column mapping, and CRUD wiring into separate focused modules.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/shared/db/built-sites.ts` around lines 448 - 466, The built-sites module has grown beyond the file-size guideline and mixes schema validation, field mapping, and CRUD concerns. Split the logic around mapPlainFields and toRawInput into focused modules for blob schema/validation, plain/blob column mapping, and CRUD wiring, preserving existing exports and behavior while keeping each module narrowly scoped.Source: Coding guidelines
src/shared/storage.ts (1)
359-377: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winKeep the explicit upload content type Bunny’s upload API defaults to a generic binary MIME type when
contentTypeisn’t passed, so these objects may be served with different headers from before. If downstream consumers expect direct CDN serving, restore the explicitcontentTypehere.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/shared/storage.ts` around lines 359 - 377, Update uploadRaw to pass the explicit upload contentType expected by the existing storage behavior when calling sdk.file.upload, while preserving the current stream construction and local-storage path. Use the established content-type value or constant from the surrounding storage implementation rather than introducing a new one.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/shared/db/built-sites.ts`:
- Around line 448-466: The built-sites module has grown beyond the file-size
guideline and mixes schema validation, field mapping, and CRUD concerns. Split
the logic around mapPlainFields and toRawInput into focused modules for blob
schema/validation, plain/blob column mapping, and CRUD wiring, preserving
existing exports and behavior while keeping each module narrowly scoped.
In `@src/shared/maintenance/runner.ts`:
- Around line 32-37: Replace the unexplained literal 3 in taskFits with a
clearly named constant describing the reserved call headroom, and use that
constant in the remaining.total calculation. Define it near the related
scheduler budget logic and document what the reserved calls cover, preserving
the existing capacity behavior.
In `@src/shared/storage.ts`:
- Around line 359-377: Update uploadRaw to pass the explicit upload contentType
expected by the existing storage behavior when calling sdk.file.upload, while
preserving the current stream construction and local-storage path. Use the
established content-type value or constant from the surrounding storage
implementation rather than introducing a new one.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b19f433f-8bd2-4d1b-b451-456518baeece
📒 Files selected for processing (75)
scripts/mutation/equivalent-mutants.txtsrc/features/admin/settings-page.tssrc/features/app/read-only.tssrc/features/app/request.tssrc/features/app/rules.tssrc/features/scheduled.tssrc/shared/db/built-sites.tssrc/shared/db/prune.tssrc/shared/limits.tssrc/shared/maintenance/runner.tssrc/shared/storage.tssrc/ui/templates/admin/settings-advanced.tsxsrc/ui/templates/admin/settings.tsxsrc/ui/templates/admin/settings/domain-payment-warning.tsxtest/features/admin/builder/server.test.tstest/features/admin/built-site-page/scheduler.test.tstest/features/admin/debug/sentry.test.tstest/features/admin/settings-page/page.test.tstest/features/app/read-only.test.tstest/features/app/request/organic-maintenance.test.tstest/features/app/routes.test.tstest/features/app/routes/messages.test.tstest/features/app/rules.test.tstest/features/middleware.test.tstest/features/scheduled/server.test.tstest/lib/server-settings/domains.test.tstest/shared/builder/access.test.tstest/shared/builder/host-secrets.test.tstest/shared/crypto/encryption.test.tstest/shared/db/activity-log-backfill.test.tstest/shared/db/built-site-scheduler.test.tstest/shared/db/built-sites.test.tstest/shared/db/built-sites/assignment-renewal.test.tstest/shared/db/built-sites/core.test.tstest/shared/db/built-sites/crud.test.tstest/shared/db/built-sites/fixtures.tstest/shared/db/built-sites/update-tier.test.tstest/shared/db/login-attempts.test.tstest/shared/db/migrations/2026-06-19_built_sites_last_pruned.test.tstest/shared/db/migrations/2026-07-18_drop_built_sites_last_pruned.test.tstest/shared/db/migrations/2026-07-18_maintenance_tasks.test.tstest/shared/db/migrations/schema/tables-core.test.tstest/shared/db/migrations/schema/tables-questions.test.tstest/shared/db/migrations/schema/version/guard.test.tstest/shared/db/prune/invites.test.tstest/shared/db/prune/tables.test.tstest/shared/db/query-log.test.tstest/shared/db/settings/public-api.test.tstest/shared/db/users/contracts.test.tstest/shared/fetch.test.tstest/shared/limits.test.tstest/shared/limits/registry.test.tstest/shared/maintenance/runner.test.tstest/shared/sentry-sdk.test.tstest/shared/sentry.test.tstest/shared/settings/keys.test.tstest/shared/site-assignment/contracts.test.tstest/shared/site-build.test.tstest/shared/site-secrets.test.tstest/shared/site-secrets/host-infra.test.tstest/shared/storage.test.tstest/shared/storage/bunny.test.tstest/shared/storage/config.test.tstest/shared/storage/deletion-attachments.test.tstest/shared/storage/fixtures.tstest/shared/storage/images.test.tstest/shared/storage/local.test.tstest/test-utils/hash.tstest/test-utils/secure-admin.tstest/test-utils/user-owned-records.tstest/ui/templates/admin/built-sites/panels.test.tsxtest/ui/templates/admin/debug/rendering.test.tsxtest/ui/templates/admin/settings-advanced/page.test.tstest/ui/templates/admin/settings-advanced/rendering.test.tstest/ui/templates/admin/settings-advanced/state.ts
💤 Files with no reviewable changes (6)
- src/shared/limits.ts
- test/shared/storage.test.ts
- test/shared/site-secrets.test.ts
- test/shared/sentry.test.ts
- test/shared/db/built-site-scheduler.test.ts
- test/shared/db/built-sites.test.ts
![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: 61ea10dd51
ℹ️ 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".
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/shared/db/built-sites.ts (1)
620-631: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winValidate scheduled keys before persisting them.
This public API accepts any string and writes it into
sk, butparseSiteDataBloblater rejects values failingisScheduledTaskKey. An invalid caller input therefore creates a row that cannot be read/decrypted by normal built-site flows. Validate centrally before blob serialization and add a regression test that asserts an invalid key is rejected before insertion.Proposed fix
+const validateScheduledTaskKey = ( + scheduledTaskKey: string | null | undefined, +): void => { + if ( + scheduledTaskKey !== null && + scheduledTaskKey !== undefined && + !isScheduledTaskKey(scheduledTaskKey) + ) { + throw new Error("Invalid scheduled task key"); + } +}; + const buildSiteDataBlobFromInput = ( input: Partial<BuiltSiteBlobInput>, ): string => { + validateScheduledTaskKey(input.scheduledTaskKey); const blob = Object.fromEntries([As per coding guidelines, “Every bug fix must include a regression test that reproduces the original failure.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/shared/db/built-sites.ts` around lines 620 - 631, Validate the scheduledTaskKey parameter in the built-site insertion function before calling toRawInput or persisting the row, using the existing isScheduledTaskKey validator and rejecting invalid non-null values. Preserve the null/valid-key behavior, and add a regression test proving an invalid key is rejected before builtSites.table.insert executes.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/features/app/request.ts`:
- Line 287: Update the request handler around runOrganicMaintenanceAfterResponse
so maintenance is scheduled through the server’s deferred/background lifecycle
instead of being awaited, allowing the already-created Response to resolve
immediately. Add a regression test that keeps deferred maintenance pending and
verifies the handler completes before runOrganicMaintenanceAfterResponse
finishes.
In `@src/shared/site-scheduler.ts`:
- Around line 43-70: Update provisionSiteScheduler and the maintenance panel’s
active-state logic to track child acceptance separately from scheduledTaskKey
generation. Persist or expose a distinct verified/accepted flag only after
setSecrets succeeds and verifyScheduledKey confirms the key; have the panel use
that flag instead of scheduledTaskKey presence, while preserving key generation
and failure handling.
In `@test/shared/db/built-site-scheduler.test.ts`:
- Around line 52-56: Replace both encoded-length assertions in
test/shared/db/built-site-scheduler.test.ts at lines 52-56 and 67-69 with
isScheduledTaskKey() validation, while preserving the existing equality and
persistence checks. In test/shared/scheduled-keys.test.ts lines 43-47,
interpolate SCHEDULED_KEY_BYTES into the expected error message instead of
hard-coding 32.
---
Outside diff comments:
In `@src/shared/db/built-sites.ts`:
- Around line 620-631: Validate the scheduledTaskKey parameter in the built-site
insertion function before calling toRawInput or persisting the row, using the
existing isScheduledTaskKey validator and rejecting invalid non-null values.
Preserve the null/valid-key behavior, and add a regression test proving an
invalid key is rejected before builtSites.table.insert executes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 99b80d8b-9f5f-4d25-87f2-280fa8730da5
📒 Files selected for processing (44)
TODO.mddeno.jsondocs/scheduled-maintenance.mdscripts/find-unused-src.tsscripts/mutation/equivalent-mutants.txtsrc/features/admin/built-sites.tssrc/features/app/request.tssrc/locales/en/built-sites.jsonsrc/shared/admin-surface/routes/built-sites.tssrc/shared/boot-checks.tssrc/shared/bunny-cdn.tssrc/shared/db/built-site-scheduler.tssrc/shared/db/built-sites.tssrc/shared/deno-deploy-api.tssrc/shared/provider-types.tssrc/shared/scheduled-access.tssrc/shared/scheduled-keys.tssrc/shared/site-scheduler.tssrc/shared/update.tssrc/ui/templates/admin/built-sites/panels.tsxtest/features/admin/built-site-page/scheduler.test.tstest/features/admin/built-sites/scheduler.test.tstest/features/app/request.test.tstest/features/index.test.tstest/features/scheduled/server.test.tstest/scripts/bundle-composition.test.tstest/scripts/unit-tests-report-imports.test.tstest/shared/admin-surface/routes/built-sites.test.tstest/shared/bunny-cdn/edge-script.test.tstest/shared/bunny-cdn/provider.test.tstest/shared/db/built-site-scheduler.test.tstest/shared/db/built-sites/core.test.tstest/shared/db/built-sites/crud.test.tstest/shared/deno-deploy-api.test.tstest/shared/scheduled-access.test.tstest/shared/scheduled-keys.test.tstest/shared/site-build.test.tstest/shared/site-scheduler.test.tstest/shared/site-update.test.tstest/shared/update.test.tstest/test-utils/factories.tstest/test-utils/scheduled.tstest/ui/templates/admin/built-sites/panels.test.tsxtsconfig.json
💤 Files with no reviewable changes (14)
- test/shared/site-build.test.ts
- src/shared/admin-surface/routes/built-sites.ts
- src/locales/en/built-sites.json
- test/test-utils/scheduled.ts
- scripts/find-unused-src.ts
- test/shared/bunny-cdn/provider.test.ts
- test/features/index.test.ts
- test/features/scheduled/server.test.ts
- test/test-utils/factories.ts
- test/shared/db/built-sites/crud.test.ts
- src/shared/provider-types.ts
- src/shared/bunny-cdn.ts
- test/shared/bunny-cdn/edge-script.test.ts
- test/shared/deno-deploy-api.test.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fb18539a02
ℹ️ 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".
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/shared/db/prune.ts (2)
132-150: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftBound the encrypted invite scan.
This loads and decrypts every candidate before applying the batch cap. A large current-invite population can exhaust the 15-second maintenance window and prevent pruning. Page through candidates with durable progress (or persist a queryable expiry), stop after collecting a batch, and add a regression test for a large current-invite prefix.
As per coding guidelines, “Every bug fix must include a regression test that reproduces the original failure and passes only after the fix.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/shared/db/prune.ts` around lines 132 - 150, The expiredInviteIds function decrypts every invite candidate before applying the batch limit, causing large current-invite prefixes to exceed the maintenance window. Change its candidate scan to page with durable id progress, decrypting each page and stopping once MAINTENANCE_PRUNE_BATCH expired IDs are collected; preserve ordered processing and avoid rescanning candidates. Add a regression test covering a large current-invite prefix that verifies pruning remains bounded and collects the batch.Source: Coding guidelines
132-150: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRevalidate invite state in the delete batch.
inviteStatements()only keys onid, so a user who accepts the invite afterexpiredInviteIds()runs can still have their child rows and user row deleted. Add the unactivated marker to each DELETE or rerun the lookup inside the batch.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/shared/db/prune.ts` around lines 132 - 150, Update the pruning flow centered on expiredInviteIds and inviteStatements so deletion revalidates that each user is still unactivated, not merely matched by id. Add the existing unactivated-state predicates to every DELETE statement in inviteStatements, or rerun the expired-invite lookup immediately before the batch, ensuring accepted invites and their child/user rows are not deleted.test/ui/templates/admin/built-sites/panels.test.tsx (1)
1-18: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRemove the file-level
beforeAllhook.It shares mutable encryption/CSRF setup across tests, making isolation depend on hook ordering. Set up the state explicitly per test or use an isolated test helper instead. As per coding guidelines, “Do not use global BDD hooks, rely on virgin isolates, or share mutable state without resetting and pinning it.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/ui/templates/admin/built-sites/panels.test.tsx` around lines 1 - 18, Remove the file-level beforeAll hook from the built site maintenance panel test suite. Move setupTestEncryptionKey and signCsrfToken into each affected test, or replace them with an existing isolated per-test helper, ensuring every test initializes its own encryption and CSRF state without shared mutable setup.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/shared/db/prune/tables.test.ts`:
- Around line 41-53: Update the test around runDatabasePruning so it does not
require the specific `prune-backlog-${MAINTENANCE_PRUNE_BATCH}` row to survive
when all seeded rows share the same `created` value. Assert deterministically
that exactly one stale row remains, or introduce a deterministic ordering key
and retain the specific-row assertion.
In `@test/shared/maintenance/runner.test.ts`:
- Around line 129-145: Make the timing assertions in the “a successful task can
request an early follow-up” test deterministic by controlling the clock or
capturing the comparison timestamp after database setup and runner bookkeeping
complete. Preserve validation of the minimum interval and the expected
upper-bound window without relying on elapsed real time before
maintenance.run(tasks).
---
Outside diff comments:
In `@src/shared/db/prune.ts`:
- Around line 132-150: The expiredInviteIds function decrypts every invite
candidate before applying the batch limit, causing large current-invite prefixes
to exceed the maintenance window. Change its candidate scan to page with durable
id progress, decrypting each page and stopping once MAINTENANCE_PRUNE_BATCH
expired IDs are collected; preserve ordered processing and avoid rescanning
candidates. Add a regression test covering a large current-invite prefix that
verifies pruning remains bounded and collects the batch.
- Around line 132-150: Update the pruning flow centered on expiredInviteIds and
inviteStatements so deletion revalidates that each user is still unactivated,
not merely matched by id. Add the existing unactivated-state predicates to every
DELETE statement in inviteStatements, or rerun the expired-invite lookup
immediately before the batch, ensuring accepted invites and their child/user
rows are not deleted.
In `@test/ui/templates/admin/built-sites/panels.test.tsx`:
- Around line 1-18: Remove the file-level beforeAll hook from the built site
maintenance panel test suite. Move setupTestEncryptionKey and signCsrfToken into
each affected test, or replace them with an existing isolated per-test helper,
ensuring every test initializes its own encryption and CSRF state without shared
mutable setup.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 1459c695-e5b0-493a-97d8-f146394fc46d
📒 Files selected for processing (40)
README.mddeno.jsonscripts/build-site.tsscripts/mutation/equivalent-mutants.txtsrc/features/admin/built-sites.tssrc/features/request-scopes.tssrc/locales/en/built-sites.jsonsrc/shared/db/address-cache.tssrc/shared/db/built-site-scheduler.tssrc/shared/db/built-sites.tssrc/shared/db/prune.tssrc/shared/db/users.tssrc/shared/maintenance/definition.tssrc/shared/maintenance/registry.tssrc/shared/maintenance/runner.tssrc/shared/site-build.tssrc/shared/update.tssrc/ui/templates/admin/built-sites/panels.tsxtest/features/admin/built-sites/actions.test.tstest/features/admin/built-sites/scheduler.test.tstest/features/admin/built-sites/update.test.tstest/features/request-scopes.test.tstest/shared/builder/host-secrets.test.tstest/shared/db/address-cache.test.tstest/shared/db/built-site-scheduler.test.tstest/shared/db/built-sites/assignment-renewal.test.tstest/shared/db/built-sites/crud.test.tstest/shared/db/prune/helpers.tstest/shared/db/prune/invites.test.tstest/shared/db/prune/tables.test.tstest/shared/maintenance/registry.test.tstest/shared/maintenance/runner.test.tstest/shared/scheduled-keys.test.tstest/shared/site-build.test.tstest/shared/storage/local.test.tstest/shared/update.test.tstest/test-utils/activity-log.tstest/ui/templates/admin/built-sites/panels.test.tsxtest/ui/templates/admin/settings-advanced/page.test.tstest/ui/templates/admin/settings-state.ts
| test("reports a full bounded batch when stale rows remain", async () => { | ||
| const old = new Date( | ||
| nowMs() - PRUNE_UNUSED_STRINGS_RETENTION_MS - 60_000, | ||
| ).toISOString(); | ||
| await insertStrings("prune-backlog", old, MAINTENANCE_PRUNE_BATCH + 1); | ||
|
|
||
| const result = await runDatabasePruning(); | ||
|
|
||
| expect(result).toEqual({ fullBatch: true }); | ||
| expect(await stringExists(`prune-backlog-${MAINTENANCE_PRUNE_BATCH}`)).toBe( | ||
| true, | ||
| ); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Do not depend on implicit batch deletion order.
Every seeded row has the same created value, but line 50 requires one particular row to survive. A correct bounded prune can remove a different equal-age row. Assert that one row remains, or seed a deterministic ordering key.
As per coding guidelines, “Maintain 100% deterministic test coverage.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/shared/db/prune/tables.test.ts` around lines 41 - 53, Update the test
around runDatabasePruning so it does not require the specific
`prune-backlog-${MAINTENANCE_PRUNE_BATCH}` row to survive when all seeded rows
share the same `created` value. Assert deterministically that exactly one stale
row remains, or introduce a deterministic ordering key and retain the
specific-row assertion.
Source: Coding guidelines
| test("a successful task can request an early follow-up", async () => { | ||
| const tasks = defineMaintenanceTasks([ | ||
| declaration("follow_up", ({ requestFollowUp }) => requestFollowUp(), { | ||
| intervalMs: 300_000, | ||
| }), | ||
| ]); | ||
| const before = Date.now(); | ||
|
|
||
| await runWithSubrequestBudget(() => maintenance.run(tasks)); | ||
|
|
||
| expect(await nextRunAt("follow_up")).toBeGreaterThanOrEqual( | ||
| before + MAINTENANCE_MIN_INTERVAL_MS, | ||
| ); | ||
| expect(await nextRunAt("follow_up")).toBeLessThan( | ||
| before + MAINTENANCE_MIN_INTERVAL_MS + 1_000, | ||
| ); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Make the follow-up timing assertion deterministic.
before is captured before database setup and runner bookkeeping. A valid run taking over one second fails line 142 despite scheduling correctly. Use controlled time, or assert relative to a timestamp captured after the run.
As per coding guidelines, “Maintain 100% deterministic test coverage.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/shared/maintenance/runner.test.ts` around lines 129 - 145, Make the
timing assertions in the “a successful task can request an early follow-up” test
deterministic by controlling the clock or capturing the comparison timestamp
after database setup and runner bookkeeping complete. Preserve validation of the
minimum interval and the expected upper-bound window without relying on elapsed
real time before maintenance.run(tasks).
Source: Coding guidelines
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ad11154b26
ℹ️ 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".
| const existing = await findBuiltSiteById(id); | ||
| if (!existing) return { value: null }; |
There was a problem hiding this comment.
When this runs against Turso/read-replica DBs, findBuiltSiteById uses the normal findById read before writing. Callers added in this change update a just-retained site immediately after insertBuiltSite (for example auto-built assignable sites and the admin builder assignable toggle), so a lagging replica can miss the new row and return null, leaving the provider site built but not marked assignable. Use the primary/read-your-writes path for this read-before-write update or carry the assignable state in the retaining write.
Useful? React with 👍 / 👎.
| toRawInput({ | ||
| ...existing, | ||
| ...changes, | ||
| siteDataRevision: nextRevision, | ||
| }), |
There was a problem hiding this comment.
When this helper runs while a checkout is assigning a child site, the full-row input here carries the assignment fields from the earlier snapshot. assignBuiltSite still writes assignable/assigned_* through builtSites.table.update without bumping site_data_revision, so this later revision-fenced write can pass and put stale assignable: true/null assignment values back, making the same child available for another buyer. Restrict this update to changed plain columns or make assignment participate in the same revision fence.
Useful? React with 👍 / 👎.
| sql: `DELETE FROM maintenance_tasks | ||
| WHERE name IN (${inPlaceholders(disabledNames)})`, |
There was a problem hiding this comment.
When one runner is finishing the last activity_log_backfill batch, another wake can see hasLegacyActivityLog() return false and reach this unconditional delete while the first row is still leased. The first runner then fails finishMaintenanceTask with Lost maintenance lease, so the scheduled endpoint reports 503 even though the work succeeded. Keep leased rows, or mark disabled without deleting them, until their lease is released.
Useful? React with 👍 / 👎.
| request: Pick<Request, "method" | "url" | "headers">, | ||
| key: string | undefined, | ||
| ): ScheduledAccess => { | ||
| if (new URL(request.url).pathname !== SCHEDULED_PATH) { |
There was a problem hiding this comment.
When a monitor is configured with /scheduled/, this pre-router check compares the raw pathname before parseRequest can apply the app's normal trailing-slash normalization. The request is treated as not_scheduled, falls through to the app router where the scheduled route was removed, and returns 404 even with a valid bearer key, so that site never runs maintenance. Normalize the path here or accept the trailing slash.
Useful? React with 👍 / 👎.
| await initialize(); | ||
| if (scheduledAccess.kind === "authorized") { | ||
| const { handleScheduledRequest } = await import("#routes/scheduled.ts"); | ||
| return await handleScheduledRequest(request); |
There was a problem hiding this comment.
For authenticated /scheduled calls this branch bypasses handleRequest, so it also skips seedEffectiveDomainHost() and the later loadEffectiveDomain() that normal requests run before any error is logged. If initDb, setup loading, or a maintenance task fails with NTFY_URL configured, reportMaintenanceFailure queues an ntfy alert using getEffectiveDomain() while it is still localhost (or a previous request's host), making multi-site maintenance failures point at the wrong site. Seed/load the effective domain in the scheduled path before reporting failures.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/mutation/equivalent-mutants.txt (1)
1-1: 🩺 Stability & Availability | 🔵 TrivialResolve the merge conflict before merging.
The pipeline reports a merge conflict with
origin/main(CONFLICT (content)). This must be resolved and the result committed before the PR can merge.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/mutation/equivalent-mutants.txt` at line 1, Resolve the content merge conflict between scripts/mutation/equivalent-mutants.txt and origin/main, preserving the intended known-equivalent-mutants content. Remove all conflict markers, verify the merged file is correct, and commit the resolved result.Source: Pipeline failures
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@scripts/mutation/equivalent-mutants.txt`:
- Line 1: Resolve the content merge conflict between
scripts/mutation/equivalent-mutants.txt and origin/main, preserving the intended
known-equivalent-mutants content. Remove all conflict markers, verify the merged
file is correct, and commit the resolved result.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 0f19272d-bb7e-4e2b-aa5e-932a8edc98ac
📒 Files selected for processing (3)
scripts/mutation/equivalent-mutants.txttest/shared/db/users/contracts.test.tstest/shared/deno-deploy-api.test.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 74ede5c389
ℹ️ 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".
| ); | ||
|
|
||
| const result = await builderApi.buildSite({ code, siteName }); | ||
| const { result } = await buildRetainedSite(siteName, { code, siteName }); |
There was a problem hiding this comment.
After this call switched the one-off build script to buildRetainedSite, the CLI now opens the builder database and encrypts a built_sites row during the retain callback. When an operator runs the documented local command with only BUNNY_API_KEY and no builder DB/encryption environment, the Bunny site has already been prepared before the retain step throws, so the command reports failure and leaves an orphan or unpublished child site. Validate the builder DB setup before starting provider provisioning, or keep this CLI on a non-retaining path.
Useful? React with 👍 / 👎.
| getSecretNames: (hostingId) => denoDeployApi.getEnvVarNames(hostingId), | ||
| prepareSite: prepareDenoSiteImpl, | ||
| publishSite: async (hostingId, code) => { | ||
| const result = await denoDeployApi.deployCode(hostingId, code); |
There was a problem hiding this comment.
When the builder uses Deno Deploy, this newly split publishSite step now runs after the child has already been inserted into built_sites, but it delegates to deployCode, which still posts to /v2/apps/{app}/deployments. Deno's v2 OpenAPI documents the revision creation endpoint as POST /v2/apps/{app}/deploy (https://console.deno.com/api/v2/openapi.json), so a Deno build can create the app, set env vars, and retain the scheduled key before publish fails. Point deployCodeImpl at the documented endpoint, or publish before retaining, so failed Deno builds do not leave retained-but-undeployed children.
Useful? React with 👍 / 👎.
…nance # Conflicts: # scripts/mutation/equivalent-mutants.txt
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 390b36144b
ℹ️ 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 (!site.scheduledTaskKey) { | ||
| const listed = await provider.getSecretNames(access.hostingId); | ||
| if (!listed.ok) return listed; | ||
| if (listed.names.includes(SCHEDULED_TASK_KEY_ENV)) { |
There was a problem hiding this comment.
For Deno-hosted children that already have SCHEDULED_TASK_KEY set but have no builder-stored key, this check does not catch the existing secret: I checked Deno's v2 OpenAPI, and app env_vars are returned as an array of env-var objects, while denoHostingProvider.getSecretNames currently maps Object.keys over that value, yielding array indexes rather than names. The provision action then falls through and overwrites the child's live key, so any already-configured monitor starts getting 401s.
Useful? React with 👍 / 👎.
1 participant
What changed
POST /scheduledbefore setup, routing, or database work.#routesdirectly at the scoped application request handler and remove the extra entry module.Key rotation
Coordinated one-click key rotation is intentionally deferred to the upcoming Uptime Kuma integration. That work will update the child site and its monitor together. Until then, a host operator can manually replace a compromised key on the child and in the monitor.
This PR does not include a second key slot, staged keys, promotion actions, or provider-specific rotation APIs.
Why
Quiet sites still need cleanup, but the old public endpoint could trigger database work and builder fan-out. Each site now has a small, bounded maintenance endpoint that a monitor can call safely.
Checks
44dfa370.44dfa370.