fix(webapp,run-store): make run-ops sharding failures visible and unroutable ids a 404 - #12
Conversation
…outable ids a 404 Source PR: triggerdotdev#4868 Source head: 1e352d0
⛔ Shipwright · BlockedRecommendation: do not merge PR #12 · Tier
Findings (10)
Fireworks usage: 52,890 input · 1,595 output · 54,485 total tokens · $0.0127 · 25s · 0 fix iteration(s) Open the Shipwright check for full evidence and the audit bundle. Use |
![shipwright-agent[bot]](https://avatars.githubusercontent.com/u/140162738?s=60&v=4){kind=small}
| // | ||
| // A thunk, not a promise: `RoutingRunStore.findRun` is not async and routes before returning, so | ||
| // an unroutable id throws while the argument is still being evaluated. | ||
| export async function undefinedOnUnroutableId<T>( |
There was a problem hiding this comment.
Shipwright · HIGH
The 'undefinedOnUnroutableId' helper is async and returns 'Promise<T | undefined>', but the comment claims 'RoutingRunStore.findRun' is not async and routes before returning.
Impact: The 'undefinedOnUnroutableId' helper is async and returns 'Promise<T | undefined>', but the comment claims 'RoutingRunStore.findRun' is not async and routes before returning. The thunk pattern obscures whether synchronous throws are actually caught and makes the fallback chain harder to reason about for a newcomer.
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| const shardRouted = new Counter({ | ||
| name: "runops_shard_routed_total", | ||
| help: "Operations routed to a shard by an id that resolves to it alone. Fan-outs, probes and fallback legs resolve no single shard and are excluded.", | ||
| labelNames: ["shard"], |
There was a problem hiding this comment.
Shipwright · HIGH
The new 'shardRoutedChildren' map caches prom-client label children by 'shardKey' with no enforcement of the claimed bounded cardinality.
Impact: The new 'shardRoutedChildren' map caches prom-client label children by 'shardKey' with no enforcement of the claimed bounded cardinality. If a shard key can be derived from caller-controlled id input, this becomes an unbounded label/memory growth vector on a hot path.
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
|
|
||
| const children = new Map<string, { inc: (value?: number) => void }>(); | ||
|
|
||
| return (source) => { |
There was a problem hiding this comment.
Shipwright · MEDIUM
In 'readThroughSourceMetric.server.ts', the 'children' map caches label children by 'source' string.
Impact: In 'readThroughSourceMetric.server.ts', the 'children' map caches label children by 'source' string. The 'source' values include shard keys from 'ReadThroughSource', which may be unbounded if shard keys are dynamically generated or user-controlled. The comment claims cardinality is bounded by the shard alphabet, but the code does not enforce this bound. If a shard key is derived from an arbitrary id, this can cause…
Suggested fix: Fix the review finding before release.
| const shardRouted = new Counter({ | ||
| name: "runops_shard_routed_total", | ||
| help: "Operations routed to a shard by an id that resolves to it alone. Fan-outs, probes and fallback legs resolve no single shard and are excluded.", | ||
| labelNames: ["shard"], |
There was a problem hiding this comment.
Shipwright · MEDIUM
In 'runStore.server.ts', the new 'shardRoutedChildren' map caches label children by 'shardKey' without any bound.
Impact: In 'runStore.server.ts', the new 'shardRoutedChildren' map caches label children by 'shardKey' without any bound. The comment claims cardinality is bounded by the shard alphabet plus two reserved keys, but the code does not validate or enforce this. If 'shardKey' can be an arbitrary string from an id, this can cause unbounded metric label cardinality and memory growth.
Suggested fix: Fix the review finding before release.
| // | ||
| // A thunk, not a promise: `RoutingRunStore.findRun` is not async and routes before returning, so | ||
| // an unroutable id throws while the argument is still being evaluated. | ||
| export async function undefinedOnUnroutableId<T>( |
There was a problem hiding this comment.
Shipwright · LOW
The 'undefinedOnUnroutableId' helper is declared 'async' and awaits the thunk, but the comment claims 'RoutingRunStore.findRun' is not async and routes before returning.
Impact: The 'undefinedOnUnroutableId' helper is declared 'async' and awaits the thunk, but the comment claims 'RoutingRunStore.findRun' is not async and routes before returning. If 'findRun' can throw synchronously during argument evaluation, the thunk is evaluated inside the 'try' block, so the synchronous throw is still caught. However, the helper's 'async' wrapper means any synchronous throw from 'read()' is converted in…
Suggested fix: Fix the review finding before release.
| } | ||
|
|
||
| // Counted separately from #shardStore, which probes, fan-outs and fallback legs also go through. | ||
| // Only a key an id resolved to on its own is traffic for that shard. |
There was a problem hiding this comment.
Shipwright · LOW
In 'runOpsStore.ts', the new method '#shardStoreRoutedById' calls '#shardStore(key)' and then 'this.#metrics.recordShardRouted(key)'.
Impact: In 'runOpsStore.ts', the new method '#shardStoreRoutedById' calls '#shardStore(key)' and then 'this.#metrics.recordShardRouted(key)'. The '#shardStore' method may throw 'UnknownShardKey' for an unconfigured shard. In that case, the metric is not recorded, which is likely intended. However, the metric is recorded after the store is successfully retrieved, so if '#shardStore' has side effects or returns a store that l…
Suggested fix: Fix the review finding before release.
| ), | ||
| })); | ||
|
|
||
| vi.mock("~/services/routeBuilders/apiBuilder.server", () => ({ |
There was a problem hiding this comment.
Shipwright · LOW
In 'waitpointTokenUnroutableId.test.ts', the test uses 'WaitpointId.toFriendlyId(UNCONFIGURED_SHARD_WAITPOINT_ID)' to create a friendly id.
Impact: In 'waitpointTokenUnroutableId.test.ts', the test uses 'WaitpointId.toFriendlyId(UNCONFIGURED_SHARD_WAITPOINT_ID)' to create a friendly id. The constant 'UNCONFIGURED_SHARD_WAITPOINT_ID' is ''waitpoint_' + 'c'.repeat(24) + 'z2''. The comment says the shard char is 'z' and version '2', but the actual string has 24 'c's, then 'z', then '2'. This is a well-formed gen-2 id with an unconfigured shard 'z'. The test is cor…
Suggested fix: Fix the review finding before release.
| describe("publication with no tables", () => { | ||
| postgresAndRedisTest( | ||
| "emits a typed PublicationMisconfiguredError rather than a bare client error", | ||
| async ({ postgresContainer, prisma, redisOptions }) => { |
There was a problem hiding this comment.
Shipwright · LOW
In 'client.publicationMisconfigured.test.ts', the test creates a publication with no tables and expects a 'PublicationMisconfiguredError'.
Impact: In 'client.publicationMisconfigured.test.ts', the test creates a publication with no tables and expects a 'PublicationMisconfiguredError'. The test uses 'prisma.$executeRawUnsafe' to create the publication. This is a test file and the raw SQL is safe because it is a fixed string. No defect introduced.
Suggested fix: Fix the review finding before release.
1 participant
Summary
Four failure modes found while exercising a multi-database run-ops setup by hand. None stopped the service: each was unalertable, unreported, or answered with the wrong status.
Design
Publication misconfiguration. The replication client already detected an empty publication and emitted a
LogicalReplicationClientErroron every reconnect, which the service logged. That is visible but not alertable, and indistinguishable from any other client error. It now emits aPublicationMisconfiguredErrorsubclass of that error, counted per source asruns_replication_publication_misconfigured_total. No boot-time check was added: it would need a live query per source before the service exists, so a transient blip would fail closed and refuse to boot, which is worse than the problem being solved. The counter increments on every retry, so a nonzero rate is the alarm and it clears when the publication is repaired.Repairing the publication restores replication going forward but does not backfill rows written while it was empty, so catching this early is the point.
Unroutable IDs. All seven API route builders now map this to a 404, including the two worker builders that previously fell through to a 500. Both new call sites log before answering, so a shard key dropped from a config meant to be append-only still alarms rather than turning every live token on it into a quiet not-found.
Routes that handle their own errors never reach a builder, so a sweep covers those too: four bare API routes that answered 500 (run result, run tags, reschedule, batch results) and ten dashboard and resource routes that threw straight through to an error page. Each takes the not-found path it already had for a run that does not exist, and logs first.
Shard-set parse failures. The parsing module is pure by design and its tests depend on that, so it reports through a callback and the caller logs, matching the existing operator reports. This covers the mint path, which every process re-reads within one cache TTL, so a bad set alarms across the fleet. Saving one is still silent at save time: the stamping helper takes the same callback but its caller does not yet pass it.
Observability.
runops_shard_routed_total{shard}counts ID-routed store resolutions, andrunops_read_through_source_total{source,shard}records which store served a read-through. The first is deliberately a relative ramp signal rather than a request count: fan-outs and probe hits resolve no single shard, so they are not counted, and the help text says so. Label children are cached rather than hashed per call, since both sit on hot read paths.Each fix has a test that fails if the corresponding production change is reverted, with two exceptions worth naming: the lines binding the metric recorders into the production service and read-through defaults are module-scope wiring that the tests inject around.
Source merge-base:
fe9470086dea372d172e2a9e72dc714a918f6283Source head:
1e352d0866157a44cbe40d68326f953441b76219