Skip to content

fix(run-engine): don't mislabel DB errors as UnclassifiableWaitpointId in completeWaitpoint - #8

Open
anurag6569201 wants to merge 1 commit into
qa/agent-triggerdotdev-trigger-dev/pr-15-4259/basefrom
qa/agent-triggerdotdev-trigger-dev/pr-15-4259/head
Open

fix(run-engine): don't mislabel DB errors as UnclassifiableWaitpointId in completeWaitpoint#8
anurag6569201 wants to merge 1 commit into
qa/agent-triggerdotdev-trigger-dev/pr-15-4259/basefrom
qa/agent-triggerdotdev-trigger-dev/pr-15-4259/head

Conversation

@anurag6569201

Copy link
Copy Markdown

Summary

The waitpoint completion path wrapped every error from the store-resolution step (runStore.forWaitpointCompletion) as UnclassifiableWaitpointId. That step probes the database to find the owning store, so a transient connection failure surfaced as a misleading "unclassifiable waitpointId" error, hiding the real cause and losing the underlying error's type, retryability and grouping. During a brief database failover this mislabel sent an incident investigation down a false trail before the real connection error was found underneath.

Fix

The catch is narrowed: only a genuine UnclassifiableRunId (the documented classification-failure signal from RunStore.forWaitpointCompletion) becomes UnclassifiableWaitpointId. Every other error, including database connectivity failures, is rethrown unchanged.

With the default classifier this also turns UnclassifiableWaitpointId into a clean signal: it no longer fires on infra noise, so any occurrence indicates a real id-routing defect worth alerting on. Recovery does not depend on this change, since lost completion side effects are re-delivered by the finalization guard from #4849; this PR is about diagnosing failures correctly.

A hermetic unit test locks both behaviors: a database error bubbles up unchanged, and a classification failure is wrapped with the original error as cause.

Source merge-base: 755feb2b50ad1639730d8d7435143b144ea15303
Source head: d793ed534bc1e6b7424fe3cbaa35779901412691

@shipwright-agent

Copy link
Copy Markdown

✅ Shipwright · Approve

Recommendation: approve PR #8 · Tier T1
Checks: 0 total · 0 needing attention

Next step: ready to merge.

Findings (3)

  • HIGH The test file uses 'vi.fn()' without importing 'vi' from vitest. · internal-packages/run-engine/src/engine/tests/completeWaitpointErrorClassification.test.ts:14
    • Fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
  • HIGH The classification check relies on 'error.name === "UnclassifiableRunId"' to detect foreign module instances. · internal-packages/run-engine/src/engine/waitpointCoordinator/legacyPostgresCoordinator.ts:130
    • Fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
  • HIGH The error handling now logs the full 'error' object for store-resolution failures. · internal-packages/run-engine/src/engine/waitpointCoordinator/legacyPostgresCoordinator.ts:140
    • Fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.

Fireworks usage: 8,192 input · 427 output · 8,619 total tokens · $0.0021 · 10s · 0 fix iteration(s)

Open the Shipwright check for full evidence and the audit bundle. Use /shipwright rerun to verify again.

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