fix: use synchronous transaction callbacks for better-sqlite3#200
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR fixes transaction handling in schedule and settings routers by converting async transaction callbacks to synchronous-style handlers and explicitly calling Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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.
Pull request overview
This PR fixes schedule and device-settings write endpoints by removing async transaction callbacks, which are not supported by Drizzle’s better-sqlite3 driver (and currently cause “Transaction function cannot return a promise” 500s).
Changes:
- Convert schedule create/update/delete mutations to use synchronous
db.transaction((tx) => ...)callbacks. - Convert device settings update mutation to use a synchronous transaction callback.
- Keep scheduler reloads occurring after the transaction scope (intended to run post-commit).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/server/routers/schedules.ts | Reworks all schedule write transactions to be synchronous. |
| src/server/routers/settings.ts | Reworks device settings update transaction to be synchronous. |
Comments suppressed due to low confidence (8)
src/server/routers/settings.ts:101
- Within a better-sqlite3 synchronous transaction callback,
tx.select()should be executed with.all()/.get()(see other in-repo uses of.all()in transactions).const [current] = tx.select()...limit(1)risks destructuring a query object rather than rows.
const updated = db.transaction((tx) => {
// Fetch current settings to validate final computed state
const [current] = tx
.select()
.from(deviceSettings)
.where(eq(deviceSettings.id, 1))
.limit(1)
src/server/routers/schedules.ts:155
- Same issue for updates: in a sync transaction callback you can't
await, so you need a synchronous execution method.tx.update(...).returning()should be executed via.returning().get()/.all()before destructuring, otherwise[result]may throw or be undefined.
const updated = db.transaction((tx) => {
const [result] = tx
.update(temperatureSchedules)
.set({ ...updates, updatedAt: new Date() })
.where(eq(temperatureSchedules.id, id))
.returning()
if (!result) {
src/server/routers/schedules.ts:342
- Same execution issue for the update:
tx.update(...).returning()needs.returning().get()/.all()in a sync transaction callback; otherwise[result]may fail or not reflect the DB write.
const [result] = tx
.update(powerSchedules)
.set({ ...updates, updatedAt: new Date() })
.where(eq(powerSchedules.id, id))
.returning()
if (!result) {
src/server/routers/schedules.ts:388
- Delete path has the same issue:
tx.delete(...).returning()should be executed synchronously via.returning().get()/.all()(or use.run()plus a separate select) so the NOT_FOUND check is based on real results.
db.transaction((tx) => {
const [deleted] = tx
.delete(powerSchedules)
.where(eq(powerSchedules.id, input.id))
.returning()
if (!deleted) {
src/server/routers/schedules.ts:537
tx.delete(alarmSchedules)...returning()should be executed via.returning().get()/.all()in this sync transaction callback; without that, the NOT_FOUND logic may not run against actual DB results.
db.transaction((tx) => {
const [deleted] = tx
.delete(alarmSchedules)
.where(eq(alarmSchedules.id, input.id))
.returning()
if (!deleted) {
src/server/routers/settings.ts:134
tx.update(deviceSettings)...returning()inside this sync transaction callback needs an execution step (.returning().get()/.all()). Otherwise[result]may not be a real row and could throw at runtime.
const [result] = tx
.update(deviceSettings)
.set({ ...input, updatedAt: new Date() })
.where(eq(deviceSettings.id, 1))
.returning()
src/server/routers/schedules.ts:318
- This select is inside a synchronous transaction callback; without
awaityou should execute it with.all()/.get(). As written,const [existing] = tx.select(...).limit(1)may attempt to destructure a query object rather than actual rows.
const [existing] = tx
.select({ onTime: powerSchedules.onTime, offTime: powerSchedules.offTime })
.from(powerSchedules)
.where(eq(powerSchedules.id, id))
.limit(1)
src/server/routers/schedules.ts:491
tx.update(alarmSchedules)...returning()needs a synchronous execution call (.returning().get()/.all()) inside this sync transaction callback; otherwise[result]may be invalid at runtime.
const updated = db.transaction((tx) => {
const [result] = tx
.update(alarmSchedules)
.set({ ...updates, updatedAt: new Date() })
.where(eq(alarmSchedules.id, id))
.returning()
if (!result) {
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| db.transaction((tx) => { | ||
| const [deleted] = tx | ||
| .delete(temperatureSchedules) | ||
| .where(eq(temperatureSchedules.id, input.id)) | ||
| .returning() |
| const created = await db.transaction(async (tx) => { | ||
| const [result] = await tx.insert(alarmSchedules).values(input).returning() | ||
| const created = db.transaction((tx) => { | ||
| const [result] = tx.insert(alarmSchedules).values(input).returning() |
| const created = await db.transaction(async (tx) => { | ||
| const [result] = await tx.insert(temperatureSchedules).values(input).returning() | ||
| const created = db.transaction((tx) => { | ||
| const [result] = tx.insert(temperatureSchedules).values(input).returning() |
| const created = await db.transaction(async (tx) => { | ||
| const [result] = await tx.insert(powerSchedules).values(input).returning() | ||
| const created = db.transaction((tx) => { | ||
| const [result] = tx.insert(powerSchedules).values(input).returning() |
better-sqlite3 transactions are synchronous and cannot accept async callbacks. Remove async/await from all db.transaction() calls and add .all() for sync query execution in schedules and settings routers. Closes #199 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
ng
force-pushed
the
fix/sync-transactions-199
branch
from
ad1a546 to
d71abf0
Compare
ng
left a comment
ng
left a comment
There was a problem hiding this comment.
Adversarial Review — Standard Depth (Sonnet Optimizer + Sonnet Skeptic)
Recommendation: Approve ✅
The core fix is correct, complete, and mechanically verified. All 10 broken db.transaction(async ...) calls converted to synchronous callbacks with .all() terminators. TypeScript, lint, and all 400 tests pass.
Confirmed findings (no blockers)
| Severity | Finding | Status |
|---|---|---|
| 🟢 Minor | reloadScheduler() inside try — if scheduler reload fails after committed write, caller gets misleading 500 |
Deferred |
| 🟢 Minor | Delete handlers silently drop db.transaction() return value |
Deferred |
| 🟢 Minor | createPowerSchedule/updatePowerSchedule missing .meta() and .output() — unreachable via REST |
Deferred |
| ⚪ Nit | No comment explaining better-sqlite3 rollback-on-throw guarantee | Noted |
Pre-existing issues surfaced
| Finding |
|---|
Promise.all on sync better-sqlite3 queries — no actual parallelism |
settings.ts other mutations still use await pattern (works but inconsistent) |
getJobManager() singleton race on failed loadSchedules() |
validateTimeRange rejects midnight-crossing schedules |
Full review artifacts: .claude/reviews/fix-sync-transactions-199/
🤖 adversarial-review — standard depth
- Isolate reloadScheduler() in its own try/catch so scheduler failures don't surface as misleading mutation errors after committed writes - Add void cast to delete transaction calls for clarity - Add missing .meta() and .output() to createPowerSchedule and updatePowerSchedule for OpenAPI/REST exposure Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
🎉 This PR is included in version 1.1.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Summary
db.transaction(async (tx) => { await tx... })calls to synchronousdb.transaction((tx) => { tx... })— better-sqlite3 doesn't support async transaction callbacksschedules.ts(9 transactions: create/update/delete for temperature, power, alarm)settings.ts(1 transaction: update device settings)Closes #199
Test plan
POST /api/trpc/schedules.createTemperatureSchedulereturns 200 instead of 500createPowerScheduleandcreateAlarmSchedulealso work🤖 Generated with Claude Code
Summary by CodeRabbit