Skip to content

Conversation

rescrv
Copy link
Contributor

@rescrv rescrv commented Sep 11, 2025

Description of changes

Once upon a time we didn't have scout logs. So this could fail and
silently fell back on the old behavior. Don't do that anymore.

Test plan

CI

Migration plan

N/A

Observability plan

N/A

Documentation Changes

N/A

Copy link
Contributor

propel-code-bot bot commented Sep 11, 2025

Remove Forked Code Path for Pulling Logs and Simplify FetchLog Logic

This pull request refactors the fetch_log operator in the worker to remove legacy fallback behavior that handled the absence of scout logs. Previously, if pulling logs with scout_logs failed, the code would silently revert to an older, less efficient fetch loop; this fallback has now been eliminated. The operator now requires scout_logs to be implemented and will fail clearly if not available. Additionally, logging and error reporting in the batched pull logic is improved. Minor dependency updates in Cargo.lock (notably for utoipa-swagger-ui and zip crates) are also included. Several unrelated test utilities and info logs receive small improvements, such as adding contextual identifiers to tracing events.

Key Changes

• Removed the legacy fallback code path in rust/worker/src/execution/operators/fetch_log.rs that previously handled missing scout_logs by using an old (loop-based) log fetch strategy.
• Fetch log operator now directly errors (with logging) if scout_logs fails, enforcing that all logs are fetched through the new, single code path.
• Improved error reporting via tracing::error! for failed log fetches.
• Minor improvements to tracing context, e.g., adding collection_id to some log statements in garbage_collector_component.rs and garbage_collector_orchestrator.rs.
• Dependency bumps in Cargo.lock for utoipa-swagger-ui (from 9.0.0 to 9.0.2) and pinning zip package versions for transitive dependencies.
• Best practice suggestions in review for improved error handling (specifically replacing unwrap() on semaphore acquire with explicit error capture) and variable naming.

Affected Areas

rust/worker/src/execution/operators/fetch_log.rs
rust/garbage_collector/src/garbage_collector_component.rs
rust/garbage_collector/src/garbage_collector_orchestrator.rs
Cargo.lock

This summary was automatically generated by @propel-code-bot

Copy link

Reviewer Checklist

Please leverage this checklist to ensure your code review is thorough before approving

Testing, Bugs, Errors, Logs, Documentation

  • Can you think of any use case in which the code does not behave as intended? Have they been tested?
  • Can you think of any inputs or external events that could break the code? Is user input validated and safe? Have they been tested?
  • If appropriate, are there adequate property based tests?
  • If appropriate, are there adequate unit tests?
  • Should any logging, debugging, tracing information be added or removed?
  • Are error messages user-friendly?
  • Have all documentation changes needed been made?
  • Have all non-obvious changes been commented?

System Compatibility

  • Are there any potential impacts on other parts of the system or backward compatibility?
  • Does this change intersect with any items on our roadmap, and if so, is there a plan for fitting them together?

Quality

  • Is this code of a unexpectedly high quality (Readability, Modularity, Intuitiveness)

let start = start as i64;
let sema = Arc::clone(&sema);
async move {
let _permit = sema.acquire().await.unwrap();
Copy link
Contributor

Choose a reason for hiding this comment

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

[BestPractice]

Potential panic from semaphore acquisition: sema.acquire().await.unwrap() will panic if the semaphore is closed, which can happen during async task cancellation or if the semaphore is dropped while permits are being acquired. Replace with proper error handling:

let _permit = sema.acquire().await.map_err(|_| {
    // Convert semaphore error to appropriate error type
    FetchLogError::PullLog(Box::new(/* appropriate error */))
})?;

Or if you want to fail fast on semaphore issues, at least log before panicking.

Context for Agents
[**BestPractice**]

Potential panic from semaphore acquisition: `sema.acquire().await.unwrap()` will panic if the semaphore is closed, which can happen during async task cancellation or if the semaphore is dropped while permits are being acquired. Replace with proper error handling:

```rust
let _permit = sema.acquire().await.map_err(|_| {
    // Convert semaphore error to appropriate error type
    FetchLogError::PullLog(Box::new(/* appropriate error */))
})?;
```

Or if you want to fail fast on semaphore issues, at least log before panicking.

File: rust/worker/src/execution/operators/fetch_log.rs
Line: 108

Once upon a time we didn't have scout logs.  So this could fail and
silently fell back on the old behavior.  Don't do that anymore.
@rescrv rescrv force-pushed the rescrv/no-fork-in-pull-logs branch from 2ac5524 to 400c68a Compare September 11, 2025 20:58
@@ -77,100 +77,61 @@ impl Operator<FetchLogInput, FetchLogOutput> for FetchLogOperator {
);

let mut log_client = self.log_client.clone();
let limit_offset = log_client
let mut limit_offset = log_client
Copy link
Contributor

Choose a reason for hiding this comment

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

[BestPractice]

The variable name limit_offset could be more descriptive. Based on the knowledge base guideline about variable naming, consider renaming it to specify what type of identifier this represents (e.g., limit_log_offset or max_log_offset) to improve readability and maintainability.

Context for Agents
[**BestPractice**]

The variable name `limit_offset` could be more descriptive. Based on the knowledge base guideline about variable naming, consider renaming it to specify what type of identifier this represents (e.g., `limit_log_offset` or `max_log_offset`) to improve readability and maintainability.

File: rust/worker/src/execution/operators/fetch_log.rs
Line: 80

.ok();
.inspect_err(|err| {
tracing::error!("could not pull logs: {err:?}");
})?;
let mut fetched = Vec::new();
let timestamp = SystemTime::now().duration_since(UNIX_EPOCH)?.as_nanos() as i64;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The changes starting at this point are all whitespace.

@blacksmith-sh blacksmith-sh bot deleted a comment from rescrv Sep 15, 2025
@rescrv rescrv requested a review from Sicheng-Pan September 15, 2025 20:04
@rescrv rescrv merged commit 664a3b2 into main Sep 16, 2025
169 of 172 checks passed
@rescrv rescrv deleted the rescrv/no-fork-in-pull-logs branch September 16, 2025 18:18
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.

2 participants