Expose safe dead-letter diagnostics for the indexer - #24
Conversation
randomblocker
left a comment
There was a problem hiding this comment.
Reviewed and validated independently: based on current main (post-#22), npm run typecheck clean, 49/49 tests, production build passes.
This is well-designed and nearly ready. It solves the actual ask in #20 — a publisher with a dead-lettered artifact can now look up their locator via /api/dacs/status?locator=stor-… and get attempts, timestamps, classification, and a remediation code instead of a bare count.
What holds up under scrutiny:
- Information-disclosure design is right: the public payload never serializes the stored
error_message.publicFailure()maps codes through a fixed allowlist and collapses everything else to a stableINDEXER_REJECTED;byCodegroups by the post-mapping code so odd internal codes cannot leak via group keys;publicKind()regex-gates kinds. The test asserts this directly — it plants "secret upstream hostname" / "secret stack trace" / a path-bearing error code in the DB and asserts the serialized diagnostics match none of them. That is the right kind of test. - Input handling: locator strictly
^stor-[0-9a-f]{40}$bound as a prepared-statement parameter, limit clamped 1–100 (default 20),hasMorevia limit+1 probe, supporting index added. - Queue-truthfulness fixes beyond the ask: successful observation transactionally deletes the dead-letter row and resets
retry_countso a later failure starts a fresh lifecycle (tested); the active count joins againstartifacts.status='dead-letter'so stale rows don't inflate it; failure kind no longer downgrades a known kind to "unknown"; both write paths are transactions.
One should-fix before leaving draft (inline below): lastRun.error still returns raw exception text through the same endpoint this PR is hardening — inconsistent with the PR's own "never return raw exceptions or internal URLs" invariant.
Nits, non-blocking: deadLetterDiagnostics.total duplicates the sibling deadLetters count (harmless); the OpenAPI catalogStatusSchema doesn't pin lastRun's shape — which is where the should-fix lives anyway, so fixing one can fix both.
| const limit = Number.isSafeInteger(requestedLimit) ? Math.max(1, Math.min(100, requestedLimit)) : 20; | ||
| const locator = options.deadLetterLocator; | ||
| const artifacts = db.prepare("SELECT status, COUNT(*) count FROM artifacts GROUP BY status").all() as Array<{ status: string; count: number }>; | ||
| const lastRun = db.prepare("SELECT * FROM scan_runs ORDER BY id DESC LIMIT 1").get() as Record<string, unknown> | undefined; |
There was a problem hiding this comment.
Should-fix: lastRun.error leaks raw exception text through the endpoint this PR is hardening.
SELECT * includes the error column, and finishScanRun stores raw messages (reindexCore.ts:63 passes error.message from failed scans; fetch failures can embed the RPC host, e.g. connect ECONNREFUSED <host>). On a deployment with a private DEMOS_RPC, that publishes the internal RPC endpoint via /api/dacs/status — exactly the class of leak the PR's Safety section rules out ("never return raw exceptions, payloads, internal URLs").
It's pre-existing (since #13), but this PR states the invariant, so it should hold across the whole response. Two easy options:
- Project only safe columns here (
status, timestamps, counts) and keeperroroperator-side, or - Map
errorthrough the samepublicFailure-style collapse (SCAN_FAILED+ generic remediation text).
Either way, extending the existing leak-assertion test to plant a hostile string in scan_runs.error would lock the invariant for this field too.
| artifacts: Object.fromEntries(artifacts.map((row) => [row.status, row.count])), | ||
| deadLetters, | ||
| deadLetterDiagnostics: { | ||
| scope: "storage-read", total: deadLetters, byCode, byKind, |
There was a problem hiding this comment.
Nit: total here always equals the sibling top-level deadLetters (same joined count). Harmless duplication — arguably self-describing for consumers that only read deadLetterDiagnostics — just noting it's intentional redundancy, not two different counts.
Summary
Safety
Validation
Closes #20