Skip to content

[Graphite MQ] Draft PR GROUP:spec_c48c80 (PRs 4181)#4221

Closed
graphite-app[bot] wants to merge 1 commit intomainfrom
gtmq_spec_c48c80_1771381580854-c7f7066b-a62e-4a04-b2b8-1f25fbc32356
Closed

[Graphite MQ] Draft PR GROUP:spec_c48c80 (PRs 4181)#4221
graphite-app[bot] wants to merge 1 commit intomainfrom
gtmq_spec_c48c80_1771381580854-c7f7066b-a62e-4a04-b2b8-1f25fbc32356

Conversation

@graphite-app
Copy link
Contributor

@graphite-app graphite-app bot commented Feb 18, 2026

This draft PR was created by the Graphite merge queue.
Trunk will be fast forwarded to the HEAD of this PR when CI passes, and the original PRs will be closed.

The following PRs are included in this draft PR:

# Description

Please include a summary of the changes and the related issue. Please also include relevant motivation and context.

## Type of change

- [ ] Bug fix (non-breaking change which fixes an issue)
- [ ] New feature (non-breaking change which adds functionality)
- [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected)
- [ ] This change requires a documentation update

## How Has This Been Tested?

Please describe the tests that you ran to verify your changes.

## Checklist:

- [ ] My code follows the style guidelines of this project
- [ ] I have performed a self-review of my code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have made corresponding changes to the documentation
- [ ] My changes generate no new warnings
- [ ] I have added tests that prove my fix is effective or that my feature works
- [ ] New and existing unit tests pass locally with my changes
@railway-app
Copy link

railway-app bot commented Feb 18, 2026

This PR was not deployed automatically as @graphite-app[bot] does not have access to the Railway project.

In order to get automatic PR deploys, please add @graphite-app[bot] to your workspace on Railway.

@graphite-app graphite-app bot closed this Feb 18, 2026
@graphite-app graphite-app bot deleted the gtmq_spec_c48c80_1771381580854-c7f7066b-a62e-4a04-b2b8-1f25fbc32356 branch February 18, 2026 02:27
@claude
Copy link

claude bot commented Feb 18, 2026

PR Review

This is a Graphite merge queue draft for PR #4181 ("fix: docker cache issues, dashboards"). The changes are small and focused. Here is my review:


engine/packages/gasoline/src/db/kv/mod.rs — Divide-by-zero fix (line 1177)

Good fix. The change from active_worker_ids.len() as u64 to active_worker_ids.len().max(1) as u64 correctly guards against a panic when active_worker_ids is empty and the result is used as a modulus denominator (lines 1254, 1259).

One observation: when the list is empty the worker-idx comparison at line 1162 will have already exited early, so the .max(1) is a defensive safeguard rather than a semantic choice. Consider adding a short comment explaining why .max(1) is used, since a future reader may wonder whether the 1 has semantic meaning or is just a guard:

// Use max(1) to guard against an empty worker list; the modulus result
// is unused in that case because wf_worker_idx will never match this worker.
let active_worker_count = active_worker_ids.len().max(1) as u64;

engine/packages/util/build.rs — Incremental build fix (line 12)

Correct fix. Without cargo:rerun-if-env-changed=OVERRIDE_GIT_SHA, incremental builds would silently ignore a changed OVERRIDE_GIT_SHA, baking in stale git SHAs into binaries. The placement (before the if let Ok check) is correct per Cargo's build script protocol.


engine/docker/universal/Dockerfile — Debug echo

Minor concern. The echo "Copying binary" line appears to be a temporary debug statement left in. If there's intent to keep it as a permanent progress indicator, it should be a more informative message (e.g. echo "Copying rivet-engine binary to /app/dist/"). If it was just for debugging, it should be removed.

Also noted (pre-existing): the LABEL org.opencontainers.image.source in the engine-full stage still references https://github.com/rivet-gg/rivet. Per project conventions the correct repo URL is https://github.com/rivet-dev/rivet.


Grafana dashboards

Dashboard changes look reasonable overall:

  • Extending the default time window from 5 m to 30 m is a sensible operational default.
  • Replacing pull-duration panels with dispatch-rate panels (rivet_gasoline_workflow_dispatched_total) gives better throughput visibility.
  • The pegboard legend fix ({{workflow_name}}{{rivet_datacenter}}) and the query join on rivet_gasoline_worker_last_metrics_publish to filter stale workers are both correct improvements.
  • Verify: The pegboard dashboard default project variable was changed from prod to staging. If this dashboard is deployed to production Grafana instances, this will silently show staging data by default.

Summary

File Verdict
gasoline/src/db/kv/mod.rs ✅ Good bug fix; consider a clarifying comment
util/build.rs ✅ Correct incremental build fix
docker/universal/Dockerfile ⚠️ Remove or improve the debug echo; stale rivet-gg label (pre-existing)
grafana-dashboards/gasoline.json ✅ Looks good
grafana-dashboards/pegboard.json ⚠️ Verify the staging default is intentional for production deployments

The Rust fixes are solid. The main items to address before merging are the leftover debug echo in the Dockerfile and confirming the pegboard dashboard default project.

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