Finish scheduled maintenance safety checks#1880
Conversation
📝 WalkthroughWalkthroughThe PR updates Deno Deploy configuration, schema validation, environment-variable retrieval, revision polling, and hostname generation. It also restructures maintenance startup checks around allowance budgets, instruments child-site database reads, and supports unversioned legacy site-data blobs. ChangesDeno Deploy integration
Maintenance startup budgeting
Site data compatibility
Child site database accounting
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant deployCode
participant DenoDeployAPI
participant waitForRevision
deployCode->>DenoDeployAPI: submit deployment
DenoDeployAPI-->>deployCode: revision id and status
deployCode->>waitForRevision: poll revision
waitForRevision->>DenoDeployAPI: fetch revision status
DenoDeployAPI-->>waitForRevision: pending or terminal status
waitForRevision-->>deployCode: success or deployment error
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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.
Actionable comments posted: 2
🤖 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/config/env.test.ts`:
- Around line 231-236: Restore the direct getDenoDeployOrgId regression
assertion in the required environment getter tests, rather than replacing it
with only getDenoDeployOrgSlug coverage. Keep both organization-ID and
organization-slug cases, using their respective environment keys and expected
values.
In `@test/shared/site-db.test.ts`:
- Around line 124-136: Update the test around readSiteSetting to capture its
returned value and assert the expected successful setting value before
validating getSubrequestUsage(). Keep the existing budget-counter assertions,
ensuring the test fails if the database read increments usage but execute does
not succeed.
🪄 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: f7583c58-c9c5-4eb3-8bf3-d6620ca0225a
📒 Files selected for processing (21)
AGENTS.mdscripts/mutation/equivalent-mutants.txtsrc/shared/config.tssrc/shared/db/built-sites/blob.tssrc/shared/deno-deploy-api.tssrc/shared/deno-deploy-schema.tssrc/shared/maintenance/definition.tssrc/shared/maintenance/registry.tssrc/shared/maintenance/runner.tssrc/shared/site-db.tstest/features/admin/builder/server.test.tstest/shared/builder.test.tstest/shared/config/env.test.tstest/shared/db/built-sites/blob.test.tstest/shared/deno-deploy-api.test.tstest/shared/deno-deploy-schema.test.tstest/shared/maintenance/definition.test.tstest/shared/maintenance/fixtures.tstest/shared/maintenance/registry.test.tstest/shared/maintenance/runner.test.tstest/shared/site-db.test.ts
| expectRequiredEnvGetter( | ||
| "getDenoDeployOrgSlug", | ||
| getDenoDeployOrgSlug, | ||
| "DENO_DEPLOY_ORG_SLUG", | ||
| "deploy-org", | ||
| ); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Keep the organization-ID getter regression case.
This replaces the direct getDenoDeployOrgId() contract test rather than adding slug coverage beside it. Restore the organization-ID assertion so both required configuration inputs are covered.
🤖 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/config/env.test.ts` around lines 231 - 236, Restore the direct
getDenoDeployOrgId regression assertion in the required environment getter
tests, rather than replacing it with only getDenoDeployOrgSlug coverage. Keep
both organization-ID and organization-slug cases, using their respective
environment keys and expected values.
Source: Coding guidelines
| test("counts a child database read against the shared request budget", async () => { | ||
| await runWithSubrequestBudget(async () => { | ||
| await readSiteSetting( | ||
| { dbToken: "t", dbUrl: "libsql://u" }, | ||
| "current_script_version", | ||
| ); | ||
| expect(getSubrequestUsage()).toEqual({ | ||
| database: 1, | ||
| external: 0, | ||
| total: 1, | ||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Assert that the read actually succeeded.
This test ignores readSiteSetting’s result, so it can pass when execute fails after usage is incremented. Capture the result and assert the expected successful value before checking the budget counters.
As per coding guidelines, tests must test observable behavior and use strong mutation-resistant assertions.
🤖 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/site-db.test.ts` around lines 124 - 136, Update the test around
readSiteSetting to capture its returned value and assert the expected successful
setting value before validating getSubrequestUsage(). Keep the existing
budget-counter assertions, ensuring the test fails if the database read
increments usage but execute does not succeed.
Source: Coding guidelines
![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: 43942f7c0c
ℹ️ 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".
| return okResult(undefined); | ||
| }; | ||
|
|
||
| const REVISION_POLL_BACKOFF_MS = Array<number>(20).fill(1_000); |
There was a problem hiding this comment.
When Deno accepts a revision but leaves it queued or building for more than 20 seconds, this hard timeout makes publishSite return { ok: false } even though the revision can still finish and route afterwards. In the builder flow the site has already been retained before publishSite runs, so operators can see a failed build with a recorded site that may later go live; either avoid treating an accepted pending revision as a synchronous failure or move the wait to a background status path.
Useful? React with 👍 / 👎.
| failure_reason: v.optional( | ||
| v.nullable(v.picklist(["error", "cancelled", "timed_out", "skipped"])), | ||
| null, | ||
| ), |
There was a problem hiding this comment.
For Deno revision responses, failure_reason is a documented field that should be present as null on non-failures, but this schema accepts it missing and invents null. If the API response loses that field, deploy failure handling continues with a fabricated value instead of failing at the boundary, which is exactly the structured external-data case this repo requires us to parse rather than default.
Useful? React with 👍 / 👎.
| const database = | ||
| 1 + | ||
| settingsRead + | ||
| tasks.reduce((sum, task) => sum + task.check.maxDatabaseCalls, 0); |
There was a problem hiding this comment.
For a maintenance task whose activation check has no settings keys, this counts only one database startup call, but runWithinAllowance still calls settings.loadKeys([]), which probes settings_version, before syncMaintenanceTaskRows does its own write. Outside an already-prepared request where that version probe is not cached, combinedAllowance: 1 passes this preflight and then spends two database calls before any task runs, throwing instead of skipping the scheduler bookkeeping the new safety check is meant to avoid.
Useful? React with 👍 / 👎.
1 participant
What changed
Why
These fixes address the five final review findings on #1862, which merged while the fixes were being prepared. The contracts make future maintenance tasks, Deno status changes, stored data, and child database reads fail clearly instead of drifting silently.
Configuration
Deno site builders now require
DENO_DEPLOY_ORG_SLUGas well as the existing organization ID and token. This is used for the managed<app>.<organization>.deno.netdomain.Verification
nix develop -c deno task precommitSummary by CodeRabbit
New Features
Bug Fixes
Documentation