Skip to content

Worker: mark permanently invalid jobs failed instead of retrying forever - #4

Open
ZIJ wants to merge 1 commit into
mainfrom
fix/oncall-worker-3dcf3f2686e5
Open

ZIJ wants to merge 1 commit into
mainfrom
fix/oncall-worker-3dcf3f2686e5

Conversation

@ZIJ

@ZIJ ZIJ commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

Cause: runWorkerBatch always selects the earliest job with status "pending". generateReport() throws for jobs whose rows aren't an array (e.g. rows: null), but the catch block in runWorkerBatch only recorded job.lastError and pushed a "failed" attempt — it never changed job.status away from "pending". So a permanently invalid job (job-101 in the captured incident) was re-selected and re-failed on every subsequent step, consuming the whole bounded batch and starving healthy jobs behind it (job-102, job-103) which never got a chance to run, even though their rows were valid.

Fix: in the catch branch of runWorkerBatch (app/worker.mjs), set job.status = "failed" alongside recording job.lastError, so a job that failed generateReport is removed from the pending queue and the batch proceeds to the next pending job. Completed-job handling and CSV generation are unchanged.

Regression test: app/test/worker.regression.test.mjs derives a 3-job snapshot from the captured incident (job-101 with rows: null ahead of two healthy pending jobs). It asserts the batch attempts exactly one failed attempt for job-101 followed by completed attempts for job-102 and job-103, that job-101 ends in status "failed" with attempts: 1, and that job-102/job-103 produce their expected CSV outputs. This test fails on the original source (job-101 is retried on every step, healthy jobs never run) and passes after the fix.

Sentry incident: event. This is the deliberately faulty on-call example app.

Tested against commit b664e075ca7c0173b06f344d47a8cbcfa6e426fa. The same regression command failed before the fix and passed afterward:

node --test --test-reporter=tap --test-concurrency=1 app/test/api.test.mjs app/test/worker.regression.test.mjs app/test/worker.test.mjs
Exit 1
TAP version 13
# Subtest: GET returns a report with its timestamp in the configured timezone
ok 1 - GET returns a report with its timestamp in the configured timezone
  ---
  duration_ms: 84.25222
  type: 'test'
  ...
# Subtest: unknown reports and unmatched routes return 404
ok 2 - unknown reports and unmatched routes return 404
  ---
  duration_ms: 1.416666
  type: 'test'
  ...
# Subtest: report writes are rejected
ok 3 - report writes are rejected
  ---
  duration_ms: 0.970515
  type: 'test'
  ...
# Subtest: a permanently invalid job fails once and does not starve healthy jobs behind it
not ok 4 - a permanently invalid job fails once and does not starve healthy jobs behind it
  ---
  duration_ms: 20.095247
  type: 'test'
  location: '<verification>/before/app/test/worker.regression.test.mjs:39:1'
  failureType: 'testCodeFailure'
  error: |-
    Expected values to be strictly deep-equal:
    + actual - expected
    
      [
        {
          jobId: 'job-101',
          outcome: 'failed'
        },
        {
    +     jobId: 'job-101',
    +     outcome: 'failed'
    -     jobId: 'job-102',
    -     outcome: 'completed'
        },
        {
    +     jobId: 'job-101',
    +     outcome: 'failed'
    -     jobId: 'job-103',
    -     outcome: 'completed'
        },
    +   {
    +     jobId: 'job-101',
    +     outcome: 'failed'
    +   }
      ]
    
  code: 'ERR_ASSERTION'
  name: 'AssertionError'
  expected:
    0:
      jobId: 'job-101'
      outcome: 'failed'
    1:
      jobId: 'job-102'
      outcome: 'completed'
    2:
      jobId: 'job-103'
      outcome: 'completed'
  actual:
    0:
      jobId: 'job-101'
      outcome: 'failed'
    1:
      jobId: 'job-101'
      outcome: 'failed'
    2:
      jobId: 'job-101'
      outcome: 'failed'
    3:
      jobId: 'job-101'
      outcome: 'failed'
  operator: 'deepStrictEqual'
  stack: |-
    TestContext.<anonymous> (file://<verification>/before/app/test/worker.regression.test.mjs:45:10)
    Test.runInAsyncScope (node:async_hooks:214:14)
    Test.run (node:internal/test_runner/test:1047:25)
    Test.start (node:internal/test_runner/test:944:17)
    startSubtestAfterBootstrap (node:internal/test_runner/harness:296:17)
  ...
# Subtest: a bounded batch processes healthy jobs in creation order
ok 5 - a bounded batch processes healthy jobs in creation order
  ---
  duration_ms: 16.150798
  type: 'test'
  ...
# Subtest: CSV output preserves commas, quotes, and embedded newlines
ok 6 - CSV output preserves commas, quotes, and embedded newlines
  ---
  duration_ms: 0.327944
  type: 'test'
  ...
# Subtest: completed jobs are not repeated when another batch runs
ok 7 - completed jobs are not repeated when another batch runs
  ---
  duration_ms: 0.497934
  type: 'test'
  ...
1..7
# tests 7
# suites 0
# pass 6
# fail 1
# cancelled 0
# skipped 0
# todo 0
# duration_ms 637.566628

node --test --test-reporter=tap --test-concurrency=1 app/test/api.test.mjs app/test/worker.regression.test.mjs app/test/worker.test.mjs
Exit 0
TAP version 13
# Subtest: GET returns a report with its timestamp in the configured timezone
ok 1 - GET returns a report with its timestamp in the configured timezone
  ---
  duration_ms: 84.563106
  type: 'test'
  ...
# Subtest: unknown reports and unmatched routes return 404
ok 2 - unknown reports and unmatched routes return 404
  ---
  duration_ms: 1.476022
  type: 'test'
  ...
# Subtest: report writes are rejected
ok 3 - report writes are rejected
  ---
  duration_ms: 0.952891
  type: 'test'
  ...
# Subtest: a permanently invalid job fails once and does not starve healthy jobs behind it
ok 4 - a permanently invalid job fails once and does not starve healthy jobs behind it
  ---
  duration_ms: 16.559125
  type: 'test'
  ...
# Subtest: a bounded batch processes healthy jobs in creation order
ok 5 - a bounded batch processes healthy jobs in creation order
  ---
  duration_ms: 16.273132
  type: 'test'
  ...
# Subtest: CSV output preserves commas, quotes, and embedded newlines
ok 6 - CSV output preserves commas, quotes, and embedded newlines
  ---
  duration_ms: 0.347289
  type: 'test'
  ...
# Subtest: completed jobs are not repeated when another batch runs
ok 7 - completed jobs are not repeated when another batch runs
  ---
  duration_ms: 0.508464
  type: 'test'
  ...
1..7
# tests 7
# suites 0
# pass 7
# fail 0
# cancelled 0
# skipped 0
# todo 0
# duration_ms 634.739615

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