Refactor sdpub access and shorten queue write locks#67
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
Summary by CodeRabbit
WalkthroughThe PR renames Possibly Related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches✨ Simplify code
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/cli/archive.ts (1)
308-313: 💤 Low valueReturn type discards operation result.
The helper declares
Promise<void>but wrapsSpineDigestFile.readDocument<T>()which returnsPromise<T>. While all current callers returnvoid, this signature prevents future callers from returning values without modifying the helper.♻️ Suggested fix to propagate return type
-async function readArchiveDocument<T>( - path: string, - operation: (document: ReadonlyDocument) => Promise<T> | T, -): Promise<void> { - await new SpineDigestFile(path).readDocument(operation); +async function readArchiveDocument<T>( + path: string, + operation: (document: ReadonlyDocument) => Promise<T> | T, +): Promise<T> { + return await new SpineDigestFile(path).readDocument(operation); }🤖 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/cli/archive.ts` around lines 308 - 313, The readArchiveDocument function declares a return type of Promise<void> but wraps SpineDigestFile.readDocument which returns Promise<T>, causing the operation result to be discarded. Change the return type annotation from Promise<void> to Promise<T> and add a return statement to propagate the result from the SpineDigestFile.readDocument call so that future callers can retrieve the operation result without modifying the helper function.src/facade/build-queue.ts (2)
504-506: 💤 Low valueRedundant heartbeat and recovery calls from every slot.
Each of the N concurrent slots calls
heartbeatBuildWorkerandrecoverStaleBuildJobson every iteration. Withconcurrency: 10and a 500ms delay between iterations, this results in up to ~20 heartbeat and recovery operations per second instead of ~2.Consider tracking the last heartbeat/recovery time at the worker level and only executing these when a threshold has passed, or designate a single slot for housekeeping duties.
💡 Sketch for throttled housekeeping
// At worker level (outside runSlot) let lastHousekeepingAt = 0; const HOUSEKEEPING_INTERVAL_MS = 1000; // Inside runSlot, replace direct calls with: const now = Date.now(); if (now - lastHousekeepingAt >= HOUSEKEEPING_INTERVAL_MS) { lastHousekeepingAt = now; await heartbeatBuildWorker(ownerId, state); await recoverStaleBuildJobs(state); }🤖 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/facade/build-queue.ts` around lines 504 - 506, The heartbeatBuildWorker and recoverStaleBuildJobs functions are being called on every iteration of the while(!stopping) loop in each concurrent slot, causing excessive overhead. Implement throttling by maintaining a lastHousekeepingAt timestamp at the worker level (outside the runSlot loop) and wrapping the calls to heartbeatBuildWorker and recoverStaleBuildJobs in a condition that checks if the current time minus lastHousekeepingAt exceeds a threshold (e.g., 1000ms), only executing these housekeeping operations when sufficient time has passed and updating lastHousekeepingAt after execution.
781-791: 💤 Low valueConsider verifying claim ownership after UPDATE for defensive robustness.
The transaction relies on Database-level serialization (
#runSerialized) to ensure the SELECT and UPDATE execute atomically across concurrent slots. This is currently safe becauseacquireBuildWorkerLeaseensures only one worker process, and all slots share the sameDatabaseinstance.However, if the architecture later supports multiple worker processes or database connections, the UPDATE could match 0 rows (job already claimed), yet
requireBuildJobByIdwould still return the job—now owned by another worker.A defensive improvement would verify ownership in the final SELECT:
💡 Defensive ownership verification
- return await requireBuildJobById(state, job.jobId); + const claimed = await state.queryOne( + "SELECT * FROM build_jobs WHERE job_id = ? AND owner_id = ?", + [job.jobId, ownerId], + mapBuildJob, + ); + + return claimed;🤖 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/facade/build-queue.ts` around lines 781 - 791, The UPDATE statement in the build job claiming logic may return 0 affected rows if another worker already claimed the job, but the subsequent call to requireBuildJobById would still return the job object with a different owner. Add defensive verification by checking the number of rows affected by the UPDATE query (the state.run call that sets state to 'running' and updates owner_id). If the UPDATE affects 0 rows, throw an error to indicate the job was already claimed, rather than returning a job owned by another worker. This ensures the code remains safe if the architecture evolves to support multiple worker processes or database connections.
🤖 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.
Nitpick comments:
In `@src/cli/archive.ts`:
- Around line 308-313: The readArchiveDocument function declares a return type
of Promise<void> but wraps SpineDigestFile.readDocument which returns
Promise<T>, causing the operation result to be discarded. Change the return type
annotation from Promise<void> to Promise<T> and add a return statement to
propagate the result from the SpineDigestFile.readDocument call so that future
callers can retrieve the operation result without modifying the helper function.
In `@src/facade/build-queue.ts`:
- Around line 504-506: The heartbeatBuildWorker and recoverStaleBuildJobs
functions are being called on every iteration of the while(!stopping) loop in
each concurrent slot, causing excessive overhead. Implement throttling by
maintaining a lastHousekeepingAt timestamp at the worker level (outside the
runSlot loop) and wrapping the calls to heartbeatBuildWorker and
recoverStaleBuildJobs in a condition that checks if the current time minus
lastHousekeepingAt exceeds a threshold (e.g., 1000ms), only executing these
housekeeping operations when sufficient time has passed and updating
lastHousekeepingAt after execution.
- Around line 781-791: The UPDATE statement in the build job claiming logic may
return 0 affected rows if another worker already claimed the job, but the
subsequent call to requireBuildJobById would still return the job object with a
different owner. Add defensive verification by checking the number of rows
affected by the UPDATE query (the state.run call that sets state to 'running'
and updates owner_id). If the UPDATE affects 0 rows, throw an error to indicate
the job was already claimed, rather than returning a job owned by another
worker. This ensures the code remains safe if the architecture evolves to
support multiple worker processes or database connections.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cfb50173-d0c0-4c9e-a822-b0b881a372bf
📒 Files selected for processing (21)
src/cli/archive-chapter.tssrc/cli/archive-maintenance.tssrc/cli/archive.tssrc/cli/queue.tssrc/facade/app.tssrc/facade/archive-view.tssrc/facade/build-queue.tssrc/facade/chapter-build.tssrc/facade/chapter.tssrc/facade/index.tssrc/facade/sdpub-coordinator.tssrc/facade/spine-digest-file.tssrc/serial.tstest/cli/archive-chapter.test.tstest/cli/archive-maintenance.test.tstest/cli/archive.test.tstest/cli/queue.test.tstest/facade/build-queue.test.tstest/facade/chapter-graph.test.tstest/facade/chapter.test.tstest/facade/spine-digest-file.test.ts
1 participant
Summary
Validation