Skip to content

feat(reborn): add concrete TurnRunner worker composition - #3457

Closed
serrrfirat wants to merge 5 commits into
reborn-integrationfrom
reborn/issue-3404-turn-runner-clean
Closed

serrrfirat wants to merge 5 commits into
reborn-integrationfrom
reborn/issue-3404-turn-runner-clean

Conversation

@serrrfirat

Copy link
Copy Markdown
Collaborator

Summary

  • add concrete Reborn TurnRunnerWorker composition on a clean branch from origin/reborn-integration
  • claim one run at a time with stable worker TurnRunnerId and fresh per-claim TurnLeaseToken
  • run wake-driven claim draining plus fallback polling, worker-owned heartbeats, registry driver lookup, per-run host construction, and trusted LoopExit application
  • add evidence-backed LoopExitApplier and recovery mapping for missing drivers, host failures, driver errors/panics, heartbeat lease loss, cancellation, and exit-application failures
  • wire the text-only RebornLoopDriverHostFactory into the runner HostFactory seam

Closes #3404

Supersedes #3446 with a clean branch directly from reborn-integration.

Tests

  • cargo fmt --all -- --check
  • CARGO_TARGET_DIR=/tmp/ironclaw-target-3404-clean cargo test -p ironclaw_reborn
  • CARGO_TARGET_DIR=/tmp/ironclaw-target-3404-clean cargo clippy -p ironclaw_reborn --all-targets --all-features -- -D warnings

@github-actions github-actions Bot added scope: dependencies Dependency updates size: XL 500+ changed lines risk: low Changes to docs, tests, or low-risk modules contributor: core 20+ merged PRs labels May 10, 2026

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request implements the TurnRunnerWorker and LoopExitApplier components to manage turn run execution, including task claiming, lease heartbeating, and exit validation. It also introduces a BlockedProcess status and a require_final_checkpoint policy. Feedback suggests optimizing the worker loop to drain the work queue and configuring the heartbeat timer to skip missed ticks to prevent request bursts.

Comment on lines +227 to +233
if let Err(err) = self.try_claim_and_run().await {
warn!(
runner_id = ?self.runner_id,
error = %err,
"claim-and-run cycle failed"
);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

The worker currently claims and executes only one run per wake signal or poll interval. If multiple runs are queued, the worker will wait for the next poll interval (or another wake signal) before processing the next run. It is more efficient to drain the queue by looping until no more runs are available to claim. This requires updating try_claim_and_run to return a boolean indicating whether a run was claimed.

            while !cancel.is_cancelled() {
                match self.try_claim_and_run().await {
                    Ok(true) => continue,
                    Ok(false) => break,
                    Err(err) => {
                        warn!(
                            runner_id = ?self.runner_id,
                            error = %err,
                            "claim-and-run cycle failed"
                        );
                        break;
                    }
                }
            }
References
  1. Errors should be surfaced via tracing::warn! to ensure reliability in background tasks without failing the entire operation.

}

/// Attempt one claim-and-run cycle.
async fn try_claim_and_run(&self) -> Result<(), TurnRunnerError> {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

Update the signature of try_claim_and_run to return Result<bool, TurnRunnerError> to support draining the work queue in the main loop.

    async fn try_claim_and_run(&self) -> Result<bool, TurnRunnerError> {
References
  1. Create specific error variants for different failure modes (e.g., TurnRunnerError) to provide semantically correct and clear error messages.


let Some(claimed) = claimed else {
debug!(runner_id = ?self.runner_id, "no runs available to claim");
return Ok(());

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

Return false when no runs are available to claim.

            return Ok(false);

);

self.execute_claimed_run(claimed).await;
Ok(())

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

Return true after successfully executing a claimed run.

        Ok(true)

interval: Duration,
cancel: CancellationToken,
) -> Result<(), TurnError> {
let mut tick = tokio::time::interval(interval);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

The heartbeat loop uses the default Burst behavior for tokio::time::interval. If the worker or runtime is under heavy load and ticks are missed, this can lead to a burst of heartbeat requests being sent once the task resumes. For heartbeats, it is generally safer to use MissedTickBehavior::Skip to maintain a steady rate without catching up on missed intervals.

    let mut tick = tokio::time::interval(interval);
    tick.set_missed_tick_behavior(tokio::time::MissedTickBehavior::Skip);
References
  1. Adjusting timing behavior (like MissedTickBehavior) is a pragmatic tradeoff for resource management in non-critical paths like heartbeats.

@serrrfirat

Copy link
Copy Markdown
Collaborator Author

Superseded by the cleaned split PRs: #3446 now covers #3404 TurnRunner worker + host-factory wiring, and #3460 covers #3424 trusted LoopExitApplier as a stacked follow-up. Closing this mixed branch to avoid merging combined/CI-red scope.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

contributor: core 20+ merged PRs risk: low Changes to docs, tests, or low-risk modules scope: dependencies Dependency updates size: XL 500+ changed lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant