fix(webapp,run-store): make run-ops sharding failures visible and unroutable ids a 404 - #4868
Conversation
|
Observability mapAs of 20/100 over 454 measured of 472 entry points (base 20, no change) What this PR changed FIX FIRST
AUDIT 3 of 50 sensitive mutations record an actor. 47 without one. What the score is made ofThe score and findings here are report-only and never gate the merge. Separately, a required test suite keeps this tool's symbol and route lists in sync with the code they name, and can fail a pull request that renames or removes a symbol they reference, or that adds the first route with a segment they anticipate. Each failure names the list to edit. The rules and their reasons: internal-packages/observability-map/README.md. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 10 included reviews per hour; 3 remain after this review. 📜 Recent review details⏰ Context from checks skipped due to timeout. (36)
🧰 Additional context used📓 Path-based instructions (14)New code must target Run Engine V2 through the singleton in `app/v3/runEngine.server.ts`; do not reintroduce V1 execution paths.📄 CodeRabbit inference engine (apps/webapp/CLAUDE.md) Files:
Never use `request.signal` to detect client disconnects.📄 CodeRabbit inference engine (apps/webapp/CLAUDE.md) Files:
We use vitest exclusively.📄 CodeRabbit inference engine (AGENTS.md) Files:
Test files must not import `app/env.server.ts`; pass configuration as options instead.📄 CodeRabbit inference engine (apps/webapp/CLAUDE.md) Files:
For dashboard changes, visually verify the running Remix app with Chrome DevTools MCP, using snapshots, screenshots, interaction, and console-message checks as appropriate.📄 CodeRabbit inference engine (apps/webapp/CLAUDE.md) Files:
**Prefer static imports over dynamic imports.**📄 CodeRabbit inference engine (AGENTS.md) Files:
Add crumbs as you write code — not just when debugging.📄 CodeRabbit inference engine (AGENTS.md) Files:
Use zod for validation in packages/core and apps/webapp📄 CodeRabbit inference engine (.github/copilot-instructions.md) Files:
Do not import `env.server.ts` directly or indirectly into test files; instead pass environment-dependent values through options/parameters to make code testable📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc) Files:
Access environment variables through the `env` export of `env.server.ts` instead of directly accessing `process.env` Use subpath exports from `@trigger.dev/core` package instead of importing from the root `@trigger.dev/core` path📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc) Files:
Use vitest for all tests in the Trigger.dev repository📄 CodeRabbit inference engine (.github/copilot-instructions.md) Files:
Use function declarations instead of default exports📄 CodeRabbit inference engine (.github/copilot-instructions.md) Files:
Use types over interfaces for TypeScript Avoid using enums; prefer string unions or const objects instead📄 CodeRabbit inference engine (.github/copilot-instructions.md) Files:
When creating or editing OTEL metrics (counters, histograms, gauges), ensure metric attributes have low cardinality by using only enums, booleans, bounded error codes, or bounded shard IDs Do not use high-cardinality attributes in OTEL metr...📄 CodeRabbit inference engine (.cursor/rules/otel-metrics.mdc) Files:
🧠 Learnings (1)📚 Learning: 2026-06-16T09:19:47.637ZApplied to files:
🔇 Additional comments (3)
WalkthroughThe pull request adds unroutable-ID handling across API, worker, waitpoint, and run routes. It adds typed replication publication errors and per-source misconfiguration metrics. It reports invalid stored mint-shard configurations while preserving gen-1 fallback behavior. It adds read-through source metrics for shard and legacy stores. It adds per-shard routing metrics with tests that exclude misses, probes, fan-outs, fallback legs, idless operations, and initialization. Merge Risk: 🟡 Moderate · up to The PR improves shard-routing errors, replication diagnostics, and rollout metrics, but new diagnostic paths still lack required breadcrumbs and two tests do not follow repository-mandated integration and placement rules. Merge should wait until these readiness issues are corrected or explicitly accepted. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Description checkExplanation The description gives detailed, relevant context and explains the design and testing coverage. However, it omits the required issue reference, checklist, Testing section with explicit steps, Changelog section, and Screenshots section.
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…outable ids a 404 Four failure modes from a manual pass over a multi-database run-ops setup, three of them silent: - A replication source whose publication carries no tables replicates nothing while boot succeeds and the service looks healthy. The client now raises a typed PublicationMisconfiguredError and the webapp counts it per source, so it alarms. - Completing a wait token with an id naming a database the deployment has no store for answered 500. Every API route builder now answers 404, matching the run routes. - An unparseable stored shard set still degrades to the previous id format, but the parse failure is now reported instead of vanishing, on both the read and write paths. - Per-shard observability, so a gradual rollout can be watched: runops_shard_routed_total and runops_read_through_source_total.
ad9e703 to
ff518b5
Compare
… routes The API route builders cover the routes built on them, but routes that handle their own errors never reach that mapping. Four bare API routes answered 500 for an id naming a database the deployment has no store for, and ten dashboard and resource routes threw straight through to an error page. Each now takes the not-found path it already has for a run that does not exist: the API routes return 404, the dashboard routes redirect or 404 as they already did. A shared helper keeps that uniform, and every one logs first, so a shard key dropped from a config meant to be append-only still alarms rather than reading as an absent run.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…, and count only routed traffic Two defects from review of the previous commit. undefinedOnUnroutableId took the read as a promise, but RoutingRunStore.findRun is not async and resolves the shard before it returns anything, so an unroutable id threw while the argument was still being evaluated and never reached the try block. Every dashboard and resource route it was added to still answered a server error. It now takes a thunk, and a test covers the synchronous-throw case that types cannot. runops_shard_routed_total counted every call through the generic store lookup, which probes, fan-outs and fallback legs also use, so an unrouted miss credited legacy and a waitpoint home-miss counted twice. Counting moved to the two id-routed entry points, so the series means what its name says. Tests cover an unrouted miss and a probe leg.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
An operation with no id falls back to the default store rather than resolving to one, so counting it as id-routed traffic inflated the default series during a ramp. The counter now fires only when an id resolved the key. Also moves the unroutable-id helper test next to its source.
2 participants
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.