Skip to content

Worker: mark permanently malformed jobs as failed instead of retrying forever - #2

Open
ZIJ wants to merge 1 commit into
mainfrom
fix/oncall-worker-6f1192b4206f
Open

ZIJ wants to merge 1 commit into
mainfrom
fix/oncall-worker-6f1192b4206f

Conversation

@ZIJ

@ZIJ ZIJ commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

Cause: runWorkerBatch selects the oldest pending job each step. When generateReport throws (e.g. job.rows is not an array, as in job-101 from incident 6f1192b4206f4cf5ace82c733d24537f), the catch block recorded job.lastError but left job.status as "pending". That same malformed job was reselected on every subsequent step, consuming the whole bounded batch and starving healthy jobs queued behind it (job-102, job-103 never got attempted).

Fix: in app/worker.mjs's catch block, set job.status = "failed" alongside job.lastError, so a permanently malformed job is recorded as failed and not reselected by the pending-job filter. Healthy jobs behind it can then be attempted and completed within the same bounded batch.

Regression test: app/test/worker.regression.test.mjs reproduces the exact incident shape (job-101 with rows: null followed by two healthy jobs) and asserts the required behavior: the bad job fails once and is marked "failed", and both healthy jobs are attempted and completed with correct CSV output within the batch. This test fails on the original source (the malformed job is retried 4 times, both healthy jobs remain pending/unattempted) and passes after the fix.

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

Tested against commit 04b1e334a737824d719341f674d7048afa69401c. 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.450301
  type: 'test'
  ...
# Subtest: unknown reports and unmatched routes return 404
ok 2 - unknown reports and unmatched routes return 404
  ---
  duration_ms: 1.416865
  type: 'test'
  ...
# Subtest: report writes are rejected
ok 3 - report writes are rejected
  ---
  duration_ms: 0.945056
  type: 'test'
  ...
# Subtest: a permanently malformed job does not starve healthy jobs behind it
not ok 4 - a permanently malformed job does not starve healthy jobs behind it
  ---
  duration_ms: 20.054329
  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.156362
  type: 'test'
  ...
# Subtest: CSV output preserves commas, quotes, and embedded newlines
ok 6 - CSV output preserves commas, quotes, and embedded newlines
  ---
  duration_ms: 0.330604
  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.508817
  type: 'test'
  ...
1..7
# tests 7
# suites 0
# pass 6
# fail 1
# cancelled 0
# skipped 0
# todo 0
# duration_ms 638.72421

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.949947
  type: 'test'
  ...
# Subtest: unknown reports and unmatched routes return 404
ok 2 - unknown reports and unmatched routes return 404
  ---
  duration_ms: 1.569703
  type: 'test'
  ...
# Subtest: report writes are rejected
ok 3 - report writes are rejected
  ---
  duration_ms: 0.953179
  type: 'test'
  ...
# Subtest: a permanently malformed job does not starve healthy jobs behind it
ok 4 - a permanently malformed job does not starve healthy jobs behind it
  ---
  duration_ms: 16.357831
  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.058835
  type: 'test'
  ...
# Subtest: CSV output preserves commas, quotes, and embedded newlines
ok 6 - CSV output preserves commas, quotes, and embedded newlines
  ---
  duration_ms: 0.330758
  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.500768
  type: 'test'
  ...
1..7
# tests 7
# suites 0
# pass 7
# fail 0
# cancelled 0
# skipped 0
# todo 0
# duration_ms 635.868046

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