Skip to content

Review mirror: phone stale threshold - #38

Closed
geiresx wants to merge 1 commit into
mainfrom
fix/android-worker-stale-threshold
Closed

geiresx wants to merge 1 commit into
mainfrom
fix/android-worker-stale-threshold

Conversation

@geiresx

@geiresx geiresx commented Nov 7, 2026 •

Copy link
Copy Markdown
Owner

Review mirror of GeiserX#644. Findings are fixed on that PR; this one closes after review.

Summary by CodeRabbit

  • Bug Fixes
    • Android workers are now marked offline after 45 minutes without a heartbeat. Other workers retain the existing 3-minute threshold.
    • Offline alerts now display the threshold used for the affected worker.
  • Documentation
    • Updated fleet guidance to describe the different offline thresholds and Android heartbeat interval.

@coderabbitai

coderabbitai Bot commented Nov 7, 2026 •

Copy link
Copy Markdown

Explain Change →

No actionable comments were generated in the recent review.

Walkthrough

The stale-worker check now uses a worker-specific timeout. Android workers use 45 minutes; other workers retain the 180-second timeout. Offline alerts report the selected timeout.

Changes: Worker stale-timeout handling

Layer / File(s) Summary
Timeout selection, stale-worker sweep, and validation
app/main.py, tests/test_android_worker_stale.py, docs/fleet.md
The stale-worker check selects a 45-minute timeout for Android workers and 180 seconds for other workers. Alerts report the selected threshold. Tests cover Android metadata variants, fallback inputs, and Docker-worker behavior. Fleet documentation describes both thresholds.

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Merge Risk: Minimal · up to d8879

Android phones that report every 15 minutes will no longer be marked offline between reports, while other workers keep the 180-second threshold. No concrete merge-blocking risk was identified.

Security architecture risk: Low · up to d8879

The longer timeout affects worker visibility and alert timing, without adding an endpoint or expanding credential authority. Worker identity checks and heartbeat-race protection remain intact. Historical metadata compatibility and production rollout behavior were not fully verified.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The policy applies across Android-classified fleet rows, but a threshold decision operates on one worker’s status and alerts. A key holder can select the longer window through its own metadata; no new ability to change a confirmed sibling worker’s metadata or credentials was established.

Security Findings and Attack Paths

  • inferred — An authenticated worker can report Android metadata and then stop heartbeating, extending its silent-but-online window from three to 45 minutes. This affects monitoring and status-dependent operations, but the same key holder could already keep its row online by continuing heartbeats. The examined path therefore does not establish a new privilege escalation.

Trust Boundaries and Controls

  • observed — The existing heartbeat boundary requires the key bound to the supplied worker identity. Shared-key enrollment and bounded unconfirmed-key reissue remain existing exceptions; a confirmed worker presenting the shared key is rejected. The timeout change does not modify these controls.
Pre-merge checks: Passed 5

Check name Status Explanation
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title identifies the change to the phone stale threshold.
Docstring Coverage Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat…
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage

Explanation

Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 3 functions across 2 files. (1 skipped: 1 unsupported.)

Finishing Touches

Generate docstrings

  • Commit to this branch
  • Create a new PR
Generate unit tests (beta)

  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Recent review info

Run configuration

Commits

Reviewing files that changed from the base of the PR and between 4f339d9 and d8879c7.

Files selected for processing: 3

  • app/main.py
  • docs/fleet.md
  • tests/test_android_worker_stale.py

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 2 remain after this review.

@geiresx geiresx closed this Nov 7, 2026
@geiresx
geiresx deleted the fix/android-worker-stale-threshold branch November 7, 2026 14:59
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