fix(worker): do not use an ephemeral container ID as the worker identity - #134
Conversation
|
Warning Review limit reached
Next review available in: 27 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe worker API now avoids using ephemeral Docker container IDs as migrated client identities. It tracks consecutive authenticated heartbeat failures, resets the count after success, and reports recovery guidance after three 401 responses. Documentation and regression tests cover identity persistence and recovery. ChangesWorker identity recovery
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #134 +/- ##
==========================================
- Coverage 93.88% 93.88% -0.01%
==========================================
Files 35 35
Lines 4102 4118 +16
==========================================
+ Hits 3851 3866 +15
- Misses 251 252 +1
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/worker_api.py`:
- Around line 164-173: Update the recovery warning in the worker identity
handling around _active_key() to remove the claim that the worker will enroll as
a new worker. State instead that heartbeats will be rejected until the persisted
client_id is restored, while preserving the existing remediation instructions
and placeholders.
- Line 218: Update the heartbeat handling function around the global
_consecutive_auth_failures state so both the non-401 HTTPStatusError branch and
the general exception branch reset the counter to zero before handling the
outcome. Preserve the existing increment behavior for 401 responses and reset on
successful heartbeats, and add sequence tests covering 401 failures interrupted
by timeout, other HTTP errors, or general exceptions.
In `@docs/fleet.md`:
- Line 155: Update the CASHPILOT_WORKER_NAME entry in the environment-variable
table to describe it strictly as a display-name setting, stating that
configuring it keeps the worker’s display name stable. Remove the claim that
leaving it unset makes identity fall back to the Docker container hostname, and
retain the legacy recovery behavior details in the Worker identity section.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 46112d17-2fd5-4064-8867-ccd31c69038f
📒 Files selected for processing (3)
app/worker_api.pydocs/fleet.mdtests/test_worker_keys.py
|
All three addressed — two of them caught my own text being factually wrong:
1321 tests green, ruff clean. |
A worker keys its UI row and its per-worker fleet key on a client_id. When no client_id was persisted yet, an already-enrolled worker reused WORKER_NAME, which defaults to socket.gethostname() — inside Docker that is the first 12 hex characters of the container ID, regenerated on every recreate. So every image bump minted a new identity. The worker then presented its still valid per-worker key under a client_id the UI had never enrolled, the UI correctly refused it, and every heartbeat returned 401 while the service containers kept running and earning. Nothing else surfaced the problem — the fleet just silently lost the worker. Hit in production upgrading 1.0.0 -> 1.4.1. The migration itself is worth keeping: on bare metal or a VM the hostname is stable and reusing it preserves the row. Only the Docker container-ID shape is rejected, and only when actually running inside a container, so existing non-container workers migrate exactly as before. Also: after three consecutive 401s while holding our own key, log the concrete remediation (which client_id we are sending, and the file to write the expected one into) instead of repeating a generic warning forever.
…secutive Three review points: - The warning claimed the worker would 'enroll as a NEW worker'. It cannot: it still authenticates with its existing per-worker key, which the UI refuses under an id it never enrolled, so heartbeats are simply rejected until the id is restored by hand. Says that now, at error level. - Only a successful heartbeat reset the 401 counter, so 401 -> timeout -> 401 -> 500 -> 401 tripped the alarm without three consecutive rejections. Any non-401 outcome now breaks the run. - The env-var table described identity falling back to the container hostname, which this change removed. It now says what the variable actually does: keeps the display name stable, and gives pre-existing workers something to migrate on.
4c639b8 to
6433b04
Compare
…ity (#134) * fix(worker): do not use an ephemeral container ID as the worker identity A worker keys its UI row and its per-worker fleet key on a client_id. When no client_id was persisted yet, an already-enrolled worker reused WORKER_NAME, which defaults to socket.gethostname() — inside Docker that is the first 12 hex characters of the container ID, regenerated on every recreate. So every image bump minted a new identity. The worker then presented its still valid per-worker key under a client_id the UI had never enrolled, the UI correctly refused it, and every heartbeat returned 401 while the service containers kept running and earning. Nothing else surfaced the problem — the fleet just silently lost the worker. Hit in production upgrading 1.0.0 -> 1.4.1. The migration itself is worth keeping: on bare metal or a VM the hostname is stable and reusing it preserves the row. Only the Docker container-ID shape is rejected, and only when actually running inside a container, so existing non-container workers migrate exactly as before. Also: after three consecutive 401s while holding our own key, log the concrete remediation (which client_id we are sending, and the file to write the expected one into) instead of repeating a generic warning forever. * fix(worker): correct the lockout warning and make the alarm truly consecutive Three review points: - The warning claimed the worker would 'enroll as a NEW worker'. It cannot: it still authenticates with its existing per-worker key, which the UI refuses under an id it never enrolled, so heartbeats are simply rejected until the id is restored by hand. Says that now, at error level. - Only a successful heartbeat reset the 401 counter, so 401 -> timeout -> 401 -> 500 -> 401 tripped the alarm without three consecutive rejections. Any non-401 outcome now breaks the run. - The env-var table described identity falling back to the container hostname, which this change removed. It now says what the variable actually does: keeps the display name stable, and gives pre-existing workers something to migrate on.
1 participant
The bug
A worker keys its UI row — and its per-worker fleet key — on a
client_id. When none was persisted yet, an already-enrolled worker fell back toWORKER_NAME, which defaults tosocket.gethostname(). Inside Docker that is the first 12 hex chars of the container ID, regenerated on every recreate.So every image bump minted a new identity. The worker presented its still-valid per-worker key under a
client_idthe UI had never enrolled, the UI correctly refused it, and every heartbeat returned401 Unauthorized.The failure is silent. Service containers keep running and earning, so nothing looks broken — the fleet just loses the worker. Hit in production upgrading a live 3-worker fleet from 1.0.0 to 1.4.1:
The UI listed the worker as
159d39365879; the recreated container had invented515ccbc46cd9.The fix
Narrow deliberately. The migration is worth keeping — on bare metal or a VM the hostname is stable, and reusing it preserves the row. Only the Docker container-ID shape is rejected, and only when
/.dockerenvconfirms we are in a container, so existing non-container workers migrate exactly as before (their test still passes unchanged).Also: after three consecutive 401s while holding our own key, log the concrete remediation — which
client_idwe are sending and the file to write the expected one into — instead of repeating a generic warning forever.Verification
ruff check+ruff format --checkclean200 OKwithkey_confirmed=1, and pre-seeding/data/.worker_idon the other two servers upgraded them with zero 401sDocs: new Worker identity section in
docs/fleet.mdwith the symptom and the recovery steps, andCASHPILOT_WORKER_NAMEnow flagged as one to set on Docker workers.Summary by CodeRabbit
Bug Fixes
Documentation
Tests