Use Duration parameter instead of seconds in wait_or_shutdown

Description

Improve call-site clarity and prevent unit mistakes by accepting Duration directly instead of u64 seconds in the wait_or_shutdown function.

Problem

The current API takes secs: u64 which requires implicit unit conversion and is prone to unit mistakes. Using Duration is more explicit and type-safe.

Solution

Change the function signature and update call sites:

// Function signature
pub async fn wait_or_shutdown(duration: Duration, shutdown: &mut watch::Receiver<()>) {
    tokio::select\! {
        biased;
        _ = shutdown.changed() => {},
        _ = tokio::time::sleep(duration) => {},
    }
}

// Call site update
WorkerHooks::wait_or_shutdown(Duration::from_secs(config.cooldown_period_seconds), shutdown).await;

Location

File: crates/comenqd/src/worker.rs
Lines: 122-127
Additional call sites need updating

References

PR: Document wait_or_shutdown helper #85
Comment: Document wait_or_shutdown helper #85 (comment)

@coderabbitai Please provide an assessment of the issue criticality (critical, high, medium or low), based on risk of the issue impacting the user and the anticipated blast radius.

Once a decision has been made, apply the appropriate label.

If the issue is documentation ONLY (i.e., no code changes needed), apply the documentation label in addition to the appropriate criticality label.

@leynos

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

I'll assess the criticality of this issue by examining the codebase and the specific changes involved.

Let me also check the GitHub issue details:

Now I'll apply the appropriate criticality label based on my assessment:

@leynos, I've assessed this issue as low criticality based on the following analysis:

Risk Assessment:

User Impact: Minimal - The current implementation works correctly; this is a type safety enhancement rather than a bug fix
Blast Radius: Limited - Changes are confined to one function and its call sites, with straightforward refactoring required

Rationale for Low Criticality:

No Broken Functionality: The existing wait_or_shutdown function with u64 seconds parameter works as intended
Enhancement Nature: This is a code quality improvement to prevent potential unit conversion mistakes and improve API clarity
Internal API: The change affects internal worker functionality, not user-facing features
Straightforward Implementation: The refactoring is well-defined with clear before/after examples

The issue represents good engineering practice for type safety but doesn't address any immediate operational concerns or user-impacting bugs. It can be prioritized alongside other technical debt and code quality improvements.

I've applied the low label to the issue accordingly.

🏁 Script executed:

#!/bin/bash

# First, let's examine the current implementation of wait_or_shutdown function
echo "=== Current wait_or_shutdown function implementation ==="
rg -A 10 -B 5 "wait_or_shutdown" crates/comenqd/src/worker.rs

echo -e "\n=== Find all call sites of wait_or_shutdown ==="
rg -n "wait_or_shutdown" --type rust

echo -e "\n=== Check if this is purely a type safety improvement ==="
rg -A 5 -B 5 "secs.*u64" crates/comenqd/src/worker.rs

Length of output: 3097

🏁 Script executed:

#!/bin/bash

# Get the issue details
gh issue view 88 --json title,body,labels,state,assignees

Length of output: 1224

🏁 Script executed:

#!/bin/bash

# Apply the "low" criticality label to the issue
gh issue edit 88 --add-label "low"

Length of output: 42

Perfect! I've successfully applied the low criticality label to issue #88.

My assessment is confirmed by the code analysis:

Current Implementation:

Function signature: wait_or_shutdown(secs: u64, shutdown: &mut watch::Receiver<()>)
Internally converts to Duration: tokio::time::sleep(Duration::from_secs(secs))
Only 2 actual call sites that need updating (lines 189 and 220)

Why This is Low Criticality:

Functional Correctness: The current implementation works perfectly - no bugs or broken behavior
Limited Scope: Only affects 2 call sites plus documentation examples
Type Safety Enhancement: This is about improving developer experience and preventing potential mistakes, not fixing existing issues
No User Impact: Internal API change with no effect on end-user functionality

The issue represents good engineering practice for API design but doesn't address any immediate operational risks or user-facing problems. It's the type of technical debt/improvement that can be scheduled alongside other low-priority enhancements.

@CodeRabbit

@CodeRabbit plan

Coding Plan

Summary

Map the obsolete wait_or_shutdown reference to the current seconds-based wait helper, wait_for_scheduled_entry.
Change that private helper to accept Duration. Convert from seconds once at the run_worker call site.
Keep the configuration, store/queue, metrics and public API contracts in seconds. Leave the retry path unchanged because it already uses Duration/Instant.
Preserve shutdown priority and the notification-registration order. Validate with the repository's warning-denying make targets.

Design Choices

Design Choice 1: Select the function to change because `wait_or_shutdown` does not exist

Options Considered:

Close the ticket as obsolete with no code change.
Change wait_for_scheduled_entry to accept a Duration, and convert at the single call site in run_worker.
Reintroduce a wait_or_shutdown helper (for example on WorkerHooks) that matches the ticket snippet.

Chosen Option: 2

Rationale: It applies the ticket's type-safety intent to the only remaining seconds-based wait parameter, and it adds no new abstraction and changes no public API.

Design Choice 2: Select the location for the seconds-to-`Duration` conversion

Options Considered:

Convert at the run_worker call site, and keep the store, queue and metrics contracts in seconds.
Change claim_next_due and the store scheduling methods to return Duration.

Chosen Option: 1

Rationale: It matches the ticket's call-site example (Duration::from_secs(...) at the caller), and it keeps the change local to the worker.

💡 User Tips

Regenerate the plan with different choices with @coderabbitai <feedback>.

Implementation Steps

Phase 1: Move the wait helper boundary to `Duration`

Change the private scheduled-entry wait helper to accept a Duration. Move the unit conversion to the caller. Complete both tasks together in the same module so that the crate compiles.

Apply the ticket reference from the earlier revision (PR #85) to the current helper. The current checkout has no wait_or_shutdown and no WorkerHooks::wait_or_shutdown. The retry path already uses Duration/Instant at this boundary. Only the scheduled-entry wait still accepts raw seconds.

Keep the store/queue contract as (StoredEntry, u64). Keep metrics in seconds.

Task 1: Change the `wait_for_scheduled_entry` signature

Make the helper unit-explicit.

Update crates/comenqd/src/worker.rs (wait_for_scheduled_entry, around lines 159–188). Replace the wait_seconds: u64 parameter with a Duration parameter (for example wait: Duration).
Pass the Duration directly to tokio::time::sleep. Remove the internal Duration::from_secs call.
Preserve every behavioural detail. Keep the biased; ordering with shutdown first. Keep the changed.as_mut().enable() registration before the waiting readiness future. Keep the true-only-on-shutdown return contract. Keep the cfg(test/test-support) notify_waiting branch.
Update the helper's doc comment. State that the argument is the time left until the head entry becomes due. Remove wording that implies seconds.
Keep the existing std::time::Duration import. Do not add a new import.

Task 2: Update the call site in `run_worker`

Convert seconds to Duration at the boundary where the value enters the wait.

Update the Some((entry, wait_seconds)) branch of run_worker in crates/comenqd/src/worker.rs (around lines 315–330). Pass Duration::from_secs(wait_seconds) to wait_for_scheduled_entry.
Keep metrics::record_cooldown_wait(wait_seconds) in seconds. Keep the wait_seconds > 0 check and the continue/break control flow unchanged.
Leave the retry path unchanged. retry_deadline(Duration::from_secs(config.cooldown_period_seconds)) and wait_for_retry_deadline(deadline, ...) already satisfy the ticket's intent.
Do not change SharedQueue::claim_next_due, the store scheduling methods, the configuration fields or metrics::record_cooldown_wait.

🤖 Prompt for AI agents

Change the private scheduled-entry wait boundary to `Duration` in
`crates/comenqd/src/worker.rs`. Update the helper and its single caller together
so that the crate compiles.

- In `wait_for_scheduled_entry` (around lines 159–188), replace `wait_seconds:
u64` with a `Duration` parameter, for example `wait: Duration`.
- Pass the parameter directly to `tokio::time::sleep`. Remove the internal
`Duration::from_secs` conversion. Reuse the existing `std::time::Duration`
import.
- Preserve the `biased;` select order with shutdown first. Preserve
`changed.as_mut().enable()` before the `waiting` readiness future. Preserve the
`cfg(test/test-support)` `notify_waiting` branch and the true-only-on-shutdown
return contract.
- Update the doc comment to describe the time left until the head entry becomes
due. Remove wording that describes the helper argument as seconds.
- In the `Some((entry, wait_seconds))` branch of `run_worker` (around lines
315–330), pass `Duration::from_secs(wait_seconds)` to the helper.
- Keep `metrics::record_cooldown_wait(wait_seconds)`, the `wait_seconds > 0`
check and the `continue`/`break` control flow unchanged.
- Leave `retry_deadline(Duration::from_secs(config.cooldown_period_seconds))`
and `wait_for_retry_deadline(deadline, ...)` unchanged. Do not change store
scheduling, `SharedQueue::claim_next_due`, configuration or metrics contracts.
- Do not reintroduce `wait_or_shutdown` or add a new abstraction. Limit changes
to `crates/comenqd/src/worker.rs`.

Phase 2: Confirm references and validate

Check that no stale seconds-based references remain. Run the repository's prescribed checks.

Task 1: Check for stale references

Make sure that comments and docs match the new signature.

Search crates/comenqd/src/, docs/ and tests/ for wait_for_scheduled_entry and wait_seconds. Update any comment that describes the helper's argument as seconds.
Leave docs/users-guide.md and docs/comenq-design.md unchanged. They describe the cooldown_period_seconds configuration unit. That unit does not change.
Do not add new tests. Existing paused-time tests already exercise the helper through run_worker, for example timer_expiry_posts_first_and_successive_entries_at_flutter_etas and bump_bust_and_del_wake_a_worker_waiting_for_a_deferred_entry.

Task 2: Run validation

Prove that behaviour does not change and that no warnings appear.

Run the focused worker tests: RUSTFLAGS="-D warnings" cargo nextest run -p comenqd --lib --all-features worker::tests::.
Run make fmt, make lint and make test. Fix any warnings. Do not suppress them.
Run cargo test -p comenq-lib --test public_api to confirm that the public contract does not change.

🤖 Prompt for AI agents

Check references against the new helper signature and validate unchanged
behaviour. Use the existing tests and warning-denying repository checks.

- Search `crates/comenqd/src/`, `docs/` and `tests/` for
`wait_for_scheduled_entry` and `wait_seconds`. Update comments that describe the
helper argument as seconds.
- Leave `docs/users-guide.md` and `docs/comenq-design.md` unchanged because
`cooldown_period_seconds` remains in seconds. Keep file changes limited to
`crates/comenqd/src/worker.rs`.
- Do not add tests. Use the existing paused-time coverage through `run_worker`,
including `timer_expiry_posts_first_and_successive_entries_at_flutter_etas` and
`bump_bust_and_del_wake_a_worker_waiting_for_a_deferred_entry`.
- Run `RUSTFLAGS="-D warnings" cargo nextest run -p comenqd --lib --all-features
worker::tests::`.
- Run `make fmt`, `make lint` and `make test`. Fix warnings without suppressing
them.
- Run `cargo test -p comenq-lib --test public_api` to confirm that the public
contract remains unchanged.

Research

Ticket Summary

Replace the raw u64 seconds argument of the worker's shutdown-aware wait helper with std::time::Duration. Make call sites clearer and reduce unit mistakes. The ticket names wait_or_shutdown, but that function does not exist in the current checkout. Apply the ticket's intent to the current equivalent helper, wait_for_scheduled_entry, in crates/comenqd/src/worker.rs.

Codebase Summary

The comenqd crate runs the run_worker worker loop in crates/comenqd/src/worker.rs. The loop uses two private wait helpers:

wait_for_scheduled_entry(queue, wait_seconds: u64, control) -> bool waits for shutdown, a queue change or a timer. It converts seconds to Duration internally. It uses biased; select ordering to give shutdown priority. It registers the change notification with enable() before it fires the test-only notify_waiting hook.
wait_for_retry_deadline(deadline: Instant, changed, shutdown) -> bool already takes a deadline. retry_deadline(Duration) builds that deadline with overflow protection.

run_worker gets wait_seconds: u64 from SharedQueue::claim_next_due. It records metrics::record_cooldown_wait(wait_seconds) and then calls wait_for_scheduled_entry. std::time::Duration is already imported in worker.rs. Both wait helpers are private. No tests, behavioural steps, docs or public API contracts call them directly. Seconds remain the correct unit at the configuration, store/queue scheduling and metrics boundaries. Validation uses make fmt, make lint and make test. All three deny warnings.

File-level Change Summary and Follow-up Agent Prompt

Impacted files:

crates/comenqd/src/worker.rs:
- wait_for_scheduled_entry: change the parameter from u64 seconds to Duration, and update its doc comment.
- run_worker: wrap the argument as Duration::from_secs(wait_seconds) at the call site.
Change no other files. Keep seconds-based units in the store, queue, configuration, metrics, tests, behavioural steps and docs.

Follow-up agent prompt:

In crates/comenqd/src/worker.rs, change the private helper
`wait_for_scheduled_entry` so it takes a `std::time::Duration` instead of
`wait_seconds: u64`. Pass that Duration straight to `tokio::time::sleep` and
delete the internal `Duration::from_secs`. Keep the `biased;` select order
(shutdown first), the `changed.as_mut().enable()` call before the test-only
`waiting` hook, and the "returns true only on shutdown" contract exactly as
they are. Update the helper's doc comment so it names the argument as the
remaining time until the entry is due, not seconds.

In `run_worker`, update the single call site to pass
`Duration::from_secs(wait_seconds)`. Keep `metrics::record_cooldown_wait(wait_seconds)`
in seconds. Do not touch `wait_for_retry_deadline`, `retry_deadline`,
`SharedQueue::claim_next_due`, store scheduling, config, or metrics.

Note: the ticket names `wait_or_shutdown`, which no longer exists. Do not
reintroduce it. Then run `make fmt`, `make lint`, and `make test`, and fix any
warnings without suppressing them.

🚀 Next Steps

🤖 All AI agent prompts combined

Task: 1

Change the private scheduled-entry wait boundary to `Duration` in
`crates/comenqd/src/worker.rs`. Update the helper and its single caller together
so that the crate compiles.

- In `wait_for_scheduled_entry` (around lines 159–188), replace `wait_seconds:
u64` with a `Duration` parameter, for example `wait: Duration`.
- Pass the parameter directly to `tokio::time::sleep`. Remove the internal
`Duration::from_secs` conversion. Reuse the existing `std::time::Duration`
import.
- Preserve the `biased;` select order with shutdown first. Preserve
`changed.as_mut().enable()` before the `waiting` readiness future. Preserve the
`cfg(test/test-support)` `notify_waiting` branch and the true-only-on-shutdown
return contract.
- Update the doc comment to describe the time left until the head entry becomes
due. Remove wording that describes the helper argument as seconds.
- In the `Some((entry, wait_seconds))` branch of `run_worker` (around lines
315–330), pass `Duration::from_secs(wait_seconds)` to the helper.
- Keep `metrics::record_cooldown_wait(wait_seconds)`, the `wait_seconds > 0`
check and the `continue`/`break` control flow unchanged.
- Leave `retry_deadline(Duration::from_secs(config.cooldown_period_seconds))`
and `wait_for_retry_deadline(deadline, ...)` unchanged. Do not change store
scheduling, `SharedQueue::claim_next_due`, configuration or metrics contracts.
- Do not reintroduce `wait_or_shutdown` or add a new abstraction. Limit changes
to `crates/comenqd/src/worker.rs`.
===============================================================================

Task: 2

Check references against the new helper signature and validate unchanged
behaviour. Use the existing tests and warning-denying repository checks.

- Search `crates/comenqd/src/`, `docs/` and `tests/` for
`wait_for_scheduled_entry` and `wait_seconds`. Update comments that describe the
helper argument as seconds.
- Leave `docs/users-guide.md` and `docs/comenq-design.md` unchanged because
`cooldown_period_seconds` remains in seconds. Keep file changes limited to
`crates/comenqd/src/worker.rs`.
- Do not add tests. Use the existing paused-time coverage through `run_worker`,
including `timer_expiry_posts_first_and_successive_entries_at_flutter_etas` and
`bump_bust_and_del_wake_a_worker_waiting_for_a_deferred_entry`.
- Run `RUSTFLAGS="-D warnings" cargo nextest run -p comenqd --lib --all-features
worker::tests::`.
- Run `make fmt`, `make lint` and `make test`. Fix warnings without suppressing
them.
- Run `cargo test -p comenq-lib --test public_api` to confirm that the public
contract remains unchanged.

💡 Iterate on the plan with: @coderabbitai <feedback>

Example Feedback
- @coderabbitai You can skip phase 3. Add a simple unit test case for phase 2.
- @coderabbitai For design choice 1 go ahead with option 3 and replan.

Start Coding →

Navigation Menu

Repository navigation

Use Duration parameter instead of seconds in wait_or_shutdown #88

Description

Problem

Solution

Location

References

Coding Plan

Summary

Implementation Steps

Phase 1: Move the wait helper boundary to `Duration`

Phase 2: Confirm references and validate

Ticket Summary

Codebase Summary

File-level Change Summary and Follow-up Agent Prompt

🚀 Next Steps

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Use Duration parameter instead of seconds in wait_or_shutdown #88

Description

Description

Problem

Solution

Location

References

Activity

leynos commented on Sep 11, 2025

coderabbitai commented on Sep 11, 2025

leynos commented on Oct 20, 2026

coderabbitai commented on Oct 20, 2026

Coding Plan

Summary

Implementation Steps

Phase 1: Move the wait helper boundary to Duration

Phase 2: Confirm references and validate

Ticket Summary

Codebase Summary

File-level Change Summary and Follow-up Agent Prompt

🚀 Next Steps

Metadata

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Issue actions

Phase 1: Move the wait helper boundary to `Duration`