Skip to content
PR Quorum
Sign inStart free
← Review gallery
denoland/deno #36153openreviewed 6h ago

feat: sqlite web cache api expiration

by CertainLach+239 282 filesreviewed in 50sView PR on GitHub
needs attentionThe panel surfaced issues that deserve attention before merge.3 findings · 2 high+

Reviewer panel

Correctness
deepseek/deepseek-v4-flash
2 found
Security
deepseek/deepseek-v4-flash
0 found
Architecture
deepseek/deepseek-v4-flash
1 found

Findings

01Hhighconf 90%

Missing database schema migration for new column 'last_inserted_at'

ext/cache/sqlite.rs:299

The SELECT query now includes the 'last_inserted_at' column from the request_response_list table, but no ALTER TABLE statement is provided in this PR to add the column. This will cause a SQL error when the cache is used with an existing database that lacks the column. A migration is needed, e.g., 'ALTER TABLE request_response_list ADD COLUMN last_inserted_at INTEGER NOT NULL DEFAULT 0'.

02Hhighconf 80%

New query column `last_inserted_at` may not exist in existing databases

ext/cache/sqlite.rs:299

The SQL query in match_inner now selects the last_inserted_at column which did not appear in the previous query. The diff does not include a database migration or schema update to add this column. If an existing SQLite cache database (created before this PR) is used, the query will fail with a "no such column" error at runtime, breaking all cache lookups.

03Mmedconf 75%

Flaky test `cacheMatchExpiresAfterTtl` due to timing dependency

tests/unit/cache_api_test.ts:419

The test puts a response with max-age=4 and then does a second cache.match after a delay(6000). If the first match (immediately after put) takes more than 4 seconds due to system load or scheduling delays, it will already be expired, causing the first assertion to fail. This can lead to intermittent failures in CI.

Want this on your own pull requests?
The same reviewer panel runs on every PR — one focused review, no noise.
Start freeLive demo