fix: stabilize Uploaded, timeouts, and evidence redaction#3
Conversation
Summary of ChangesHello @EffortlessSteven, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the robustness and correctness of the package publishing process. It addresses critical issues related to command execution timeouts, ensuring that process output is always captured and timeout status is accurately reported. Furthermore, it refines sensitive data redaction for improved security and corrects the handling of the 'Uploaded' package state, preventing false positives and ensuring proper registry reconciliation. These changes collectively lead to a more reliable and secure publishing workflow. Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Summary by CodeRabbit
WalkthroughThe changes add timeout tracking to cargo publish operations with enhanced output redaction, while tightening publish success verification logic in the engine by eliminating Uploaded as a final success state and introducing post-loop registry visibility checks when packages reach the Uploaded state. Changes
Sequence Diagram(s)sequenceDiagram
participant Engine as Engine
participant Package as Package<br/>(State)
participant Registry as Registry
participant Result as Result<br/>(Outcome)
Note over Engine,Result: Publish Loop Completes
Engine->>Package: Check state
alt Package in Uploaded state
Engine->>Registry: Query: version exists?
alt Version exists
Registry-->>Engine: Yes
Engine->>Package: Transition to Published
Engine->>Result: Return success receipt
else Version not found
Registry-->>Engine: No
Engine->>Result: Set error (Ambiguous visibility)
Engine->>Package: Transition to Failed
end
else Package in other final state
Engine->>Result: Return based on state
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRsBased on the provided data, no additional related PRs were identified beyond the current changes. Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 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 |
There was a problem hiding this comment.
Code Review
This pull request introduces several important fixes for stability and correctness. The timeout handling in cargo_publish is now more robust, capturing output even on timeouts. The redaction logic is safer against Unicode-related panics by using to_ascii_lowercase. The semantics of the Uploaded state have been clarified, with fallback checks to prevent packages from getting stuck. The changes are well-reasoned and include corresponding tests. I have a few suggestions to further improve maintainability and error handling in the new logic.
| .get(&key) | ||
| .map(|p| p.state.clone()); | ||
| if matches!(current_state, Some(PackageState::Uploaded)) { | ||
| if reg.version_exists(&p.name, &p.version).unwrap_or(false) { |
There was a problem hiding this comment.
Using unwrap_or(false) here suppresses potential errors from version_exists, such as network issues or unexpected API responses. This could lead to incorrectly reporting a package as not visible when the check itself failed. It would be better to propagate the error using the ? operator, similar to the logic in the serial engine (crates/shipper/src/engine.rs). The function signature of publish_package already supports returning a Result.
| if reg.version_exists(&p.name, &p.version).unwrap_or(false) { | |
| if reg.version_exists(&p.name, &p.version)? { |
| let mut stdout_bytes = Vec::new(); | ||
| let mut stderr_bytes = Vec::new(); | ||
| if let Some(mut out) = child.stdout.take() { | ||
| let _ = out.read_to_end(&mut stdout_bytes); | ||
| } | ||
| if let Some(mut err) = child.stderr.take() { | ||
| let _ = err.read_to_end(&mut stderr_bytes); | ||
| } |
There was a problem hiding this comment.
There's duplicated logic for reading stdout and stderr pipes here and in the timeout handling block (lines 158-165). To improve maintainability, you could extract this logic into a local helper function or closure.
Additionally, the Result from read_to_end is being ignored. It would be good practice to handle this, even if it's just to log a warning on failure, as an I/O error could occur while reading the pipes.
| { | ||
| let mut state = st.lock().unwrap(); | ||
| update_state_locked(&mut state, &key, PackageState::Published); | ||
| let _ = state::save_state(state_dir, &state); |
There was a problem hiding this comment.
The result of state::save_state is being ignored. If saving the state fails (e.g., due to file system permissions or I/O errors), the error will be silently dropped. This could lead to an inconsistent state. It's better to propagate the error using the ? operator.
| let _ = state::save_state(state_dir, &state); | |
| state::save_state(state_dir, &state)?; |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/shipper/src/engine.rs (1)
700-710:⚠️ Potential issue | 🟠 MajorInconsistency: serial path still treats
Uploadedas success, contradicting the PR's stated semantic change.The parallel path (line 323) correctly excludes
Uploadedfrom the success predicate:PackageState::Published | PackageState::Skipped { .. }But the serial path here still includes
Uploaded:PackageState::Published | PackageState::Uploaded | PackageState::Skipped { .. }While the new post-loop reconciliation (lines 619–632) should ensure no receipt reaches this point in
Uploadedstate, keepingUploadedin the success predicate contradicts the PR's objective of treating it as an intermediate state, and silently masks any regression where the reconciliation logic is bypassed.Proposed fix: align serial path with parallel path
let exec_result = if receipts.iter().all(|r| { matches!( r.state, - PackageState::Published | PackageState::Uploaded | PackageState::Skipped { .. } + PackageState::Published | PackageState::Skipped { .. } ) }) {crates/shipper/src/engine_parallel.rs (1)
427-456:⚠️ Potential issue | 🟡 MinorEvent log reports hardcoded
ErrorClass::Ambiguousregardless of original error class.At line 444, the
PackageFailedevent always usesErrorClass::Ambiguous, even thoughclass(fromlast_err) could be a different variant (e.g.,Retryable). The state update at lines 428–435 correctly uses the originalclass, but the event log entry diverges.Proposed fix: use the actual error class in the event
let mut log = event_log.lock().unwrap(); log.record(PublishEvent { timestamp: Utc::now(), event_type: EventType::PackageFailed { - class: ErrorClass::Ambiguous, + class: class.clone(), message: msg.clone(), }, package: pkg_label,Note: this requires keeping
classalive past theupdate_state_lockedcall. You'll need to clone it before the state update or restructure the borrows.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 662e51998f
ℹ️ 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 Some(mut out) = child.stdout.take() { | ||
| let _ = out.read_to_end(&mut stdout_bytes); | ||
| } | ||
| if let Some(mut err) = child.stderr.take() { | ||
| let _ = err.read_to_end(&mut stderr_bytes); |
There was a problem hiding this comment.
Avoid blocking timeout path on pipe drain
After timing out, this branch kills and waits for cargo, then does a blocking read_to_end on the captured pipes. If cargo publish spawned children that inherited stdout/stderr and outlived the killed parent (a realistic case in build/publish flows), EOF is delayed until those descendants exit, so the timeout handler can hang far past per_package_timeout and stall parallel workers.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR fixes three related issues: timeout handling in cargo_publish, Unicode-safe redaction, and the semantics of the Uploaded state.
Changes:
- Fixed timeout handling to avoid double-consuming stdout/stderr pipes and to propagate timeout status via the
timed_outfield inCargoOutput - Changed redaction from
to_lowercase()toto_ascii_lowercase()to preserve byte offsets when slicing strings, preventing panics with non-ASCII characters - Added fallback registry checks in both serial and parallel engines to reconcile packages stuck in
Uploadedstate, and removedUploadedfrom the parallel mode success predicate
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| crates/shipper/src/cargo.rs | Added timed_out field to CargoOutput; fixed timeout path to read pipes manually after killing process; switched redaction to to_ascii_lowercase() for Unicode safety; added tests for mixed-case and non-ASCII patterns |
| crates/shipper/src/engine.rs | Removed Uploaded from parallel mode success check (line 323); added fallback version_exists check after retry loop for packages in Uploaded state (lines 619-632); updated test expectations |
| crates/shipper/src/engine_parallel.rs | Added timeout check to success condition (line 222); added fallback version_exists check for packages in Uploaded state (lines 371-393) |
Summary
cargo_publishtimeout path — avoidtry_wait+wait_with_outputdouble-consuming stdout/stderr pipes; read pipes manually aftertry_waitsucceeds; on timeout, capture output before killing the process. Propagate timeout viaCargoOutput.timed_outso parallel mode still performs registry reconciliation.to_ascii_lowercase()instead ofto_lowercase()to avoid Unicode offset mismatches during case-insensitive redaction. Remove deadval_offsetvariable.Uploadedis intermediate, not success — remove it from success predicates. Add fallbackversion_existscheck after the retry loop in both serial and parallel engines so that packages stuck in theUploadedstate are reconciled against the registry.Tests
cargo test --workspace— 296 tests pass (263 lib + 13 CLI + 20 e2e)redact_mixed_case_authorization,redact_mixed_case_token,redact_non_ascii_near_sensitive_pattern_no_panic