Skip to content

fix: adapt queue metrics to CDS v10 scheduling changes#435

Draft
mariayord wants to merge 1 commit into
mainfrom
fix/cds10-queue-metrics-compat
Draft

fix: adapt queue metrics to CDS v10 scheduling changes#435
mariayord wants to merge 1 commit into
mainfrom
fix/cds10-queue-metrics-compat

Conversation

@mariayord

Copy link
Copy Markdown

CDS v10 enables scheduling by default, which changes how outbox messages are written and processed:

  • Scheduler.scheduleTask now uses UPSERT instead of INSERT, so db.after 'CREATE' never fires for new messages; listen for both 'CREATE' and 'UPSERT' to stay compatible with CDS 9 and 10
  • Scheduler also UPSERTs internal flush markers (service:'scheduling'); skip these to avoid counting them as incoming messages
  • Retries fire UPSERT with attempts>0; guard incomingMessages increment to only count initial inserts (attempts==null)
  • Update maxAttempts fallback from 20 to 10 (CDS v10 default)

Also add db.pool.max:1 to the test bookshop config: @cap-js/sqlite@3 switched from better-sqlite3 to node:sqlite where each :memory: connection is an isolated database, so without a pool cap of 1 the deploy and queue processor can land on different connections and the table created by deploy is invisible to the processor.

CDS v10 enables scheduling by default, which changes how outbox messages
are written and processed:

- Scheduler.scheduleTask now uses UPSERT instead of INSERT, so db.after
  'CREATE' never fires for new messages; listen for both 'CREATE' and
  'UPSERT' to stay compatible with CDS 9 and 10
- Scheduler also UPSERTs internal flush markers (service:'scheduling');
  skip these to avoid counting them as incoming messages
- Retries fire UPSERT with attempts>0; guard incomingMessages increment
  to only count initial inserts (attempts==null)
- Update maxAttempts fallback from 20 to 10 (CDS v10 default)

Also add db.pool.max:1 to the test bookshop config: @cap-js/sqlite@3
switched from better-sqlite3 to node:sqlite where each :memory:
connection is an isolated database, so without a pool cap of 1 the
deploy and queue processor can land on different connections and the
table created by deploy is invisible to the processor.
@hyperspace-insights

Copy link
Copy Markdown
Contributor

Summary

The following content is AI-generated and provides a summary of the pull request:


Fix: Adapt Queue Metrics to CDS v10 Scheduling Changes

Bug Fix

🐛 Updated queue metrics collection to remain compatible with both CDS v9 and CDS v10, where the scheduler now uses UPSERT instead of INSERT for outbox messages by default.

Changes

  • lib/metrics/queue.js:

    • Updated the database event listener from 'CREATE' to ['CREATE', 'UPSERT'] to handle both CDS v9 (INSERT) and CDS v10 (UPSERT via Scheduler.scheduleTask).
    • Added a guard to skip internal scheduler flush markers (service: 'scheduling') to prevent them from being counted as incoming messages.
    • Added a check on attempts == null to ensure only initial inserts increment the incomingMessages counter, not retries (which also fire UPSERT with attempts > 0 in CDS v10).
    • Updated the maxAttempts fallback value from 20 to 10 to align with the CDS v10 default.
  • test/bookshop/package.json: Added db.pool.max: 1 to the test bookshop config to ensure the deploy and queue processor share the same in-memory SQLite connection. This is required because @cap-js/sqlite@3 switched from better-sqlite3 to node:sqlite, where each :memory: connection is an isolated database.


  • 🔄 Regenerate and Update Summary
  • ✏️ Insert as PR Description (deletes this comment)
  • 🗑️ Delete comment
PR Bot Information

Version: 1.24.0

  • File Content Strategy: Full file content
  • Correlation ID: bcb5674a-18ef-4aa3-9942-16730edf944e
  • Output Template: Default Template
  • LLM: anthropic--claude-4.6-sonnet
  • Event Trigger: pull_request.opened
  • Summary Prompt: Default Prompt

@hyperspace-insights hyperspace-insights Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR looks reasonable overall — the adaptation to CDS v10 scheduling semantics is well-reasoned and the db.pool.max:1 test fix is correct. The one concern worth addressing is that the attempts == null guard on line 206 may silently stop counting initial messages on CDS 9 if the INSERT payload includes an explicit attempts: 0 value; using (createTaskReq.data.attempts ?? 0) === 0 would be more robust across both CDS versions.

PR Bot Information

Version: 1.24.0

  • Correlation ID: bcb5674a-18ef-4aa3-9942-16730edf944e
  • LLM: anthropic--claude-4.6-sonnet
  • Event Trigger: pull_request.opened
  • File Content Strategy: Full file content

Comment thread lib/metrics/queue.js
cds.db.after(['CREATE', 'UPSERT'], queueEntity, async (_, createTaskReq) => {
const tenant = cds.context?.tenant

const queuedServiceName = JSON.parse(createTaskReq.data.msg || '{}').service ?? createTaskReq.data.target

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: JSON.parse(createTaskReq.data.msg || '{}') will throw a SyntaxError if msg is a non-empty but malformed JSON string — the || '{}' fallback only guards against falsy values, not invalid JSON.

Suggested change
const queuedServiceName = JSON.parse(createTaskReq.data.msg || '{}').service ?? createTaskReq.data.target
const queuedServiceName = (() => { try { return JSON.parse(createTaskReq.data.msg || '{}') } catch { return {} } })().service ?? createTaskReq.data.target

Double-check suggestion before committing. Edit this comment for amendments.


Please provide feedback on the review comment by checking the appropriate box:

  • 🌟 Awesome comment, a human might have missed that.
  • ✅ Helpful comment
  • 🤷 Neutral
  • ❌ This comment is not helpful

Comment thread lib/metrics/queue.js
// > a new task is created in the persistent queue
tenantQueueStastics(statistics, tenant, queuedServiceName).incomingMessages += 1
// In CDS >=10 (scheduling:true), retries also fire UPSERT with attempts > 0 — skip those
if (createTaskReq.data.attempts == null) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Logic Error: The guard createTaskReq.data.attempts == null assumes that a brand-new message on the CDS 9 INSERT path always has attempts as undefined/null in the request data. If CDS 9 includes attempts: 0 explicitly in the INSERT payload, this condition evaluates to false and the initial message is never counted in incomingMessages. Consider guarding with !createTaskReq.data.attempts (falsy check covering both null/undefined and 0) or (createTaskReq.data.attempts ?? 0) === 0.

Suggested change
if (createTaskReq.data.attempts == null) {
if ((createTaskReq.data.attempts ?? 0) === 0) {

Double-check suggestion before committing. Edit this comment for amendments.


Please provide feedback on the review comment by checking the appropriate box:

  • 🌟 Awesome comment, a human might have missed that.
  • ✅ Helpful comment
  • 🤷 Neutral
  • ❌ This comment is not helpful

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant