Skip to content

Record SNOS timing in job metadata - #1194

Open
heemankv wants to merge 4 commits into
mainfrom
orchestrator-snos-timing-metadata
Open

Record SNOS timing in job metadata#1194
heemankv wants to merge 4 commits into
mainfrom
orchestrator-snos-timing-metadata

Conversation

@heemankv

Copy link
Copy Markdown
Contributor

Summary

  • pin SNOS/generate-pie to keep-starknet-strange/snos commit b284015497ee980b4129d1c72d622b94965ace98 from PR Refactor CI and Release process #527
  • add optional SNOS timing fields to SnosMetadata
  • persist total SNOS processing time, RPC wait time, and execution/local processing time after successful SNOS runs

Notes

Testing

  • cargo fmt --all
  • git diff --check
  • Local cargo check was intentionally skipped per request; CI/manual image build will validate with the full Cairo toolchain.

@heemankv

heemankv commented Jun 24, 2026

Copy link
Copy Markdown
Contributor Author

Triggered manual Orchestrator Docker image build for this branch: https://github.com/madara-alliance/madara/actions/runs/28082160388

Expected image tag after successful publish: ghcr.io/madara-alliance/orchestrator:manual-99b2a14

@greptile-apps

greptile-apps Bot commented Jun 24, 2026

Copy link
Copy Markdown

Confidence Score: 4/5

Safe to merge; changes are additive and isolated to SNOS job metadata, with no behavioral changes to the processing path itself.

The only finding is a pre-existing inconsistency: snos_n_steps lacks #[serde(default)] while the three new timing fields added in this PR correctly have it. Any SNOS job record persisted before snos_n_steps was introduced will fail to deserialize. The PR touches the same struct and would be a natural place to fix it.

orchestrator/src/types/jobs/metadata/mod.rssnos_n_steps consistency with #[serde(default)].

Reviews (1): Last reviewed commit: "Record SNOS timing in job metadata" | Re-trigger Greptile

Comment on lines 216 to 217
/// SNOS total steps taken
pub snos_n_steps: Option<usize>,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 snos_n_steps missing #[serde(default)]

The existing snos_n_steps: Option<usize> field (added in a prior PR) has no #[serde(default)] annotation, while the three new timing fields added here correctly do. Without #[serde(default)], serde will return a "missing field" error when deserializing any persisted SNOS job record that was created before snos_n_steps was introduced — same class of backward-compat issue the new fields guard against. Worth adding it here while the struct is being touched.

@github-actions

github-actions Bot commented Aug 2, 2026

Copy link
Copy Markdown
Contributor

There hasn't been any activity on this pull request recently, and in order to prioritize active work, it has been marked as stale.
This PR will be closed and locked in 7 days if no further activity occurs.
Thank you for your contributions!

@github-actions github-actions Bot added the stale No activity for a long period label Aug 2, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

stale No activity for a long period

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

1 participant