decrating: physical removal of shipper-events and shipper-state#73
Conversation
PR #60 landed shim absorption (re-exports from crate::state::events and crate::state::execution_state pointing at standalone crates) because shipper-store (PR #57) and shipper-engine-parallel (PR #64) had trait- signature deps on them. Both blockers landed via merge train round 2, unblocking this physical removal. Move 2821 LOC (events) + 2725 LOC (state) from standalone crates into crates/shipper/src/state/events/ and crates/shipper/src/state/execution_state/ as absorbed modules. Events split into three files: - mod.rs (production: EventLog, EVENTS_FILE, events_path) - tests.rs (unit + insta snapshot tests) - proptests.rs (property-based tests) Execution-state split into two files: - mod.rs (production: atomic write, migration, encrypted I/O) - tests.rs (unit + snapshot tests; nested proptests + proptests_extended submodules preserved) Snapshots relocated and renamed for the new module path (shipper__state__events__tests__* and shipper__state__execution_state__tests__*). Integration test moved to crates/shipper/tests/state_integration.rs (was crates/shipper-state/tests/state_integration.rs) with imports rewritten to use shipper::state::execution_state::*. shipper-events fuzz target (fuzz/fuzz_targets/event_log_roundtrip.rs) repointed to shipper::state::events::EventLog; its shipper-events dep removed from fuzz/Cargo.toml. Internal shipper imports rewritten: - shipper_events → crate::state::events - shipper_state → crate::state::execution_state - shipper_environment::collect_environment_fingerprint → crate::runtime::environment::collect_environment_fingerprint (also already absorbed; execution_state no longer needs the external dep) Delete crates/shipper-events and crates/shipper-state. Remove from workspace members and shipper's deps. CLAUDE.md files updated to reflect the now- physically-absorbed state. docs/architecture.md updated to remove dangling standalone-crate edges. Per docs/decrating-plan.md §6 Phase 2 follow-up + §6.A R-PR-1..R-PR-5. Validation: cargo check --workspace, cargo test -p shipper (1640 lib + integration tests), cargo test -p shipper-cli, cargo clippy --workspace --all-targets --all-features -- -D warnings, cargo fmt --all -- --check — all green.
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 18 minutes and 24 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (51)
📒 Files selected for processing (36)
✨ 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 absorbs the shipper-events and shipper-state microcrates into the main library, consolidating event logging and state persistence logic within the crate::state module. Feedback identifies a bug in EventLog::write_to_file where using append mode with the full in-memory history leads to data duplication. Further improvements are suggested to enhance the durability of atomic writes by propagating sync_all errors and to refine receipt migration logic to avoid misleading audit trails caused by using current environment fingerprints for legacy records.
| .create(true) | ||
| .append(true) | ||
| .open(path) | ||
| .with_context(|| format!("failed to open events file {}", path.display()))?; | ||
|
|
There was a problem hiding this comment.
The combination of append(true) and iterating over the entire self.events vector (lines 104-107) will cause duplication in the log file if write_to_file is called multiple times on the same instance, or if the log was previously populated from the same file via read_from_file.
Since EventLog maintains the full history in memory, it is safer to overwrite the file to ensure the on-disk state exactly matches the in-memory state. If you intend to maintain a persistent log across multiple runs, you should load the existing log first (which read_from_file does), and then overwrite the file with the updated history.
| .create(true) | |
| .append(true) | |
| .open(path) | |
| .with_context(|| format!("failed to open events file {}", path.display()))?; | |
| let file = OpenOptions::new() | |
| .create(true) | |
| .write(true) | |
| .truncate(true) | |
| .open(path) | |
| .with_context(|| format!("failed to open events file {}", path.display()))?; |
| .with_context(|| format!("failed to create tmp file {}", tmp.display()))?; | ||
| f.write_all(&data) | ||
| .with_context(|| format!("failed to write tmp file {}", tmp.display()))?; | ||
| f.sync_all().ok(); |
There was a problem hiding this comment.
Ignoring the result of sync_all() with .ok() undermines the durability of the atomic write. If the OS fails to flush the data to physical storage (e.g., due to an I/O error or disk full), the subsequent rename might replace a valid state file with a corrupted or empty one. It is better to propagate the error to ensure the caller knows the persistence failed.
| f.sync_all().ok(); | |
| f.sync_all().with_context(|| format!("failed to sync tmp file {}", tmp.display()))?; |
| // Add environment: default EnvironmentFingerprint if not present | ||
| if receipt.get("environment").is_none() { | ||
| let environment = collect_environment_fingerprint(); | ||
| receipt["environment"] = serde_json::to_value(environment) | ||
| .context("failed to serialize environment fingerprint")?; | ||
| } |
There was a problem hiding this comment.
Filling the environment field with the current environment fingerprint during migration of a v1 receipt to v2 may be misleading for audit purposes. A receipt is an audit record of a past publish operation; attributing the current tool versions and OS info to a past event reduces the reliability of the audit trail.
Consider using a placeholder indicating the information was missing in the original v1 record, or documenting that this field reflects the environment at the time of migration rather than the time of publication.
Summary
Completes Phase 2 decrating by physically moving the
shipper-events(2821LOC) and
shipper-state(2725 LOC) source intoshipper/src/state/events/and
shipper/src/state/execution_state/and deleting both standalone crates.PR #60 landed the shim absorption (re-exports from
crate::state::eventsandcrate::state::execution_statepointing at the standalone crates) becauseshipper-store(PR #57) andshipper-engine-parallel(PR #64) still hadtrait-signature deps on those crates. Both blockers landed via merge train
round 2, unblocking this physical removal.
Module structure
crate::state::events/mod.rs— production (EventLog,EVENTS_FILE,events_path)tests.rs— unit + insta snapshot testsproptests.rs— property-based testssnapshots/— 31 snapshots renamedshipper__state__events__tests__*.snapcrate::state::execution_state/mod.rs— production (atomic write, encrypted I/O, schema migration)tests.rs— unit + snapshot tests with two nested proptest modulespreserved (
proptests,proptests_extended)snapshots/— 16 snapshots renamedshipper__state__execution_state__tests__*.snapIntegration test moved to
crates/shipper/tests/state_integration.rs(wascrates/shipper-state/tests/state_integration.rs); its 3 snapshots moved tocrates/shipper/tests/snapshots/.Import rewrites inside
shippershipper_events→crate::state::eventsshipper_state→crate::state::execution_stateshipper_environment::collect_environment_fingerprint→crate::runtime::environment::collect_environment_fingerprint(execution_state no longer needs the external dep —
shipper-environmentwas already absorbed into
crate::runtime::environmentin PR decrating: absorb shipper-environment into shipper::runtime::environment #65)Fuzz target
fuzz/fuzz_targets/event_log_roundtrip.rsrepointed toshipper::state::events::EventLog;shipper-eventsremoved fromfuzz/Cargo.toml.Docs
CLAUDE.mdfiles updated to remove the "transitional shim"note and document the now-physically-absorbed state
docs/architecture.md: removed dangling standalone-crate edges from thedependency graph; renamed the
shipper-state/shipper-eventsentries inthe state-layer table to point at the absorbed module paths
Validation (all green)
cargo check --workspacecargo test -p shipper— 1640 lib tests + 26 integration tests passcargo test -p shipper-cli— all passcargo build -p shipper-clicargo clippy --workspace --all-targets --all-features -- -D warningscargo fmt --all -- --checkPer
docs/decrating-plan.md§6 Phase 2 follow-up + §6.A R-PR-1..R-PR-5.Test plan
cargo checkpassescargo test -p shipper state::events— 34 tests passcargo test -p shipper state::execution_state— 89 tests passcargo test -p shipper(full) — 1640 lib tests passcargo test -p shipper-cli— all passcargo clippy --workspace --all-targets --all-features -- -D warningscargo fmt --all -- --check