feat: implement task_id for build triggers#1902
Conversation
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: de4815cf8f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if let Err(e) = BuildTriggerService::handle_git_push_event( | ||
| self.storage.clone(), | ||
| self.git_object_cache.clone(), | ||
| self.bellatrix.clone(), | ||
| event, | ||
| ) | ||
| .await?; | ||
| .await | ||
| { | ||
| tracing::error!("Failed to trigger CI pipeline: {}", e); | ||
| } |
There was a problem hiding this comment.
This change swallows errors from BuildTriggerService::handle_git_push_event and allows post_cl_operation to return success even when CI trigger creation fails (for example, Orion request failure or task ID parsing failure). Before this commit, the ? propagated the error and the caller (post_receive_pack) failed the operation, so callers could detect that no build was started; now pushes can appear successful while silently skipping CI, which is a functional regression for build-trigger reliability.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
We prioritize code persistence (Git push) over CI triggering to ensures that core Git operations are never blocked by CI system (Orion) unavailability.
There was a problem hiding this comment.
Pull request overview
This PR implements explicit task_id tracking for build triggers as part of the effort to transition from implicit to explicit build triggering (issue #1830). The change enables the system to capture and persist the task_id returned by the Orion build system when triggers are created, establishing a direct link between trigger records and their corresponding build tasks.
Changes:
- Modified the Orion client to parse and return task_id from build trigger responses
- Updated the build trigger storage to support optional task_id during insertion
- Changed the dispatcher from asynchronous (fire-and-forget) to synchronous execution to capture task_id immediately
- Modified error handling in Git push operations to log build trigger failures without blocking the push
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| orion-server/bellatrix/src/orion_client/mod.rs | Added TaskResponse struct and modified trigger_build to return task_id as String |
| orion-server/bellatrix/src/lib.rs | Updated on_post_receive to return task_id instead of unit type |
| jupiter/src/storage/build_trigger_storage.rs | Added task_id parameter to insert method; added update_task_id method for future use |
| ceres/src/pack/monorepo.rs | Changed error handling to log build trigger failures without propagating errors |
| ceres/src/build_trigger/dispatcher.rs | Refactored from async dispatch to synchronous execution, capturing task_id before database insertion |
| .await?; | ||
| .await | ||
| { | ||
| tracing::error!("Failed to trigger CI pipeline: {}", e); |
There was a problem hiding this comment.
The error handling behavior has changed from propagating errors to silently logging them. Previously, if build triggering failed, the entire post_cl_operation would fail. Now the error is only logged and execution continues. This behavioral change should be intentional and documented, or the error should be propagated to maintain the original behavior. Consider whether build trigger failures should block CL operations, especially since check_reg.run_checks failures are still propagated on line 1070.
| tracing::error!("Failed to trigger CI pipeline: {}", e); | |
| tracing::error!("Failed to trigger CI pipeline: {}", e); | |
| return Err(e); |
| #[tokio::test] | ||
| async fn test_insert_and_get() { | ||
| let temp_dir = tempdir().unwrap(); | ||
| let storage = test_storage(temp_dir.path()).await; | ||
|
|
||
| let payload = serde_json::json!({ | ||
| "type": "git_push", | ||
| "repo": "/test", | ||
| "commit_hash": "abc123", | ||
| "cl_link": "test_link", | ||
| "builds": [] | ||
| }); | ||
|
|
||
| let inserted = storage | ||
| .build_trigger_storage() | ||
| .insert("git_push".to_string(), "user".to_string(), payload) | ||
| .insert("git_push".to_string(), "user".to_string(), payload, None) | ||
| .await | ||
| .unwrap(); | ||
|
|
There was a problem hiding this comment.
Tests were updated to pass None for task_id, but there's no test coverage for the new functionality of inserting triggers with a task_id. Consider adding a test case that verifies inserting a trigger with a provided task_id works correctly and that the task_id is properly persisted and retrievable.
| changes: info.changes.into_iter().map(|s| s.into()).collect(), | ||
| }) | ||
| .collect(); | ||
|
|
There was a problem hiding this comment.
Debug logging that showed the number of changes per build was removed. While this is just debug-level logging, it was useful for troubleshooting build dispatching issues. Consider keeping this logging or adding equivalent logging at the appropriate level to maintain observability of the build dispatch process.
| let num_builds = bellatrix_builds.len(); | |
| let total_changes: usize = bellatrix_builds | |
| .iter() | |
| .map(|build| build.changes.len()) | |
| .sum(); | |
| tracing::debug!( | |
| num_builds, | |
| total_changes, | |
| "BuildDispatcher: Dispatching builds to Bellatrix" | |
| ); |
| #[derive(Deserialize, Debug)] | ||
| pub struct TaskResponse { | ||
| pub task_id: String, | ||
| } | ||
|
|
There was a problem hiding this comment.
The TaskResponse deserialization does not validate that task_id is non-empty. If the Orion server returns an empty string for task_id, it will be accepted and later fail UUID parsing with a confusing error. Consider adding validation to ensure task_id is not empty, or using a non-empty string type to catch this at deserialization time.
| #[derive(Deserialize, Debug)] | |
| pub struct TaskResponse { | |
| pub task_id: String, | |
| } | |
| #[derive(Debug)] | |
| pub struct TaskResponse { | |
| pub task_id: String, | |
| } | |
| impl<'de> serde::Deserialize<'de> for TaskResponse { | |
| fn deserialize<D>(deserializer: D) -> Result<Self, D::Error> | |
| where | |
| D: serde::Deserializer<'de>, | |
| { | |
| #[derive(serde::Deserialize)] | |
| struct Inner { | |
| task_id: String, | |
| } | |
| let inner = Inner::deserialize(deserializer)?; | |
| if inner.task_id.is_empty() { | |
| return Err(serde::de::Error::custom( | |
| "task_id must not be empty in Orion TaskResponse", | |
| )); | |
| } | |
| Ok(TaskResponse { | |
| task_id: inner.task_id, | |
| }) | |
| } | |
| } |
|
|
||
| pub async fn update_task_id( | ||
| &self, | ||
| trigger_id: i64, | ||
| task_id: uuid::Uuid, | ||
| ) -> Result<(), MegaError> { | ||
| let trigger = build_triggers::Entity::find_by_id(trigger_id) | ||
| .one(self.base.get_connection()) | ||
| .await | ||
| .map_err(MegaError::Db)?; | ||
|
|
||
| if let Some(trigger_model) = trigger { | ||
| let mut active_model: build_triggers::ActiveModel = trigger_model.into(); | ||
| active_model.task_id = ActiveValue::Set(Some(task_id)); | ||
| active_model.updated_at = ActiveValue::Set(chrono::Utc::now().naive_utc()); | ||
|
|
||
| active_model | ||
| .update(self.base.get_connection()) | ||
| .await | ||
| .map_err(MegaError::Db)?; | ||
|
|
||
| Ok(()) | ||
| } else { | ||
| Err(MegaError::Other(format!( | ||
| "Trigger {} not found", | ||
| trigger_id | ||
| ))) | ||
| } | ||
| } |
There was a problem hiding this comment.
The update_task_id method is now unused since the implementation was changed to insert triggers with task_id in a single operation. This method was previously intended for the async pattern where the task_id would be updated after the trigger was created. Since it's no longer needed in the new synchronous approach, consider removing it to reduce code maintenance burden and avoid confusion.
| pub async fn update_task_id( | |
| &self, | |
| trigger_id: i64, | |
| task_id: uuid::Uuid, | |
| ) -> Result<(), MegaError> { | |
| let trigger = build_triggers::Entity::find_by_id(trigger_id) | |
| .one(self.base.get_connection()) | |
| .await | |
| .map_err(MegaError::Db)?; | |
| if let Some(trigger_model) = trigger { | |
| let mut active_model: build_triggers::ActiveModel = trigger_model.into(); | |
| active_model.task_id = ActiveValue::Set(Some(task_id)); | |
| active_model.updated_at = ActiveValue::Set(chrono::Utc::now().naive_utc()); | |
| active_model | |
| .update(self.base.get_connection()) | |
| .await | |
| .map_err(MegaError::Db)?; | |
| Ok(()) | |
| } else { | |
| Err(MegaError::Other(format!( | |
| "Trigger {} not found", | |
| trigger_id | |
| ))) | |
| } | |
| } |
| } else { | ||
| tracing::error!("Failed to trigger build: {}", res.status()); | ||
| Err(anyhow::anyhow!("Failed to trigger build: {}", res.status())) |
There was a problem hiding this comment.
The error message only includes the HTTP status code but not the response body. If the Orion server returns error details in the response body (e.g., validation errors, detailed failure reasons), this information is lost. Consider reading and including the response body in the error message to aid debugging when build triggers fail.
| // Insert trigger record with task_id (complete record in one operation) | ||
| let db_record = self | ||
| .storage | ||
| .build_trigger_storage() | ||
| .insert( | ||
| trigger.trigger_type.to_string(), | ||
| trigger.trigger_source.to_string(), | ||
| trigger_payload, | ||
| task_id, | ||
| ) | ||
| .await?; |
There was a problem hiding this comment.
If the Orion call succeeds but the database insertion fails, a build will be triggered without a corresponding trigger record in the database. This orphaned build cannot be tracked or managed through the UI/API. Consider implementing a compensation mechanism, such as canceling the build task if database insertion fails, or implementing a reconciliation process to handle such inconsistencies.
Signed-off-by: allure <1550220889@qq.com>
AllureCurtain
force-pushed
the
feat/sync-dispatch
branch
from
de4815c to
f96f289
Compare
link #1830