fix(raw-weights): advance past sealed epochs so pushes can be acknowledged - #69
Merged
Conversation
Challenge clients were inventing epochs from wall-clock buckets while the sealer force-advances past sealed identities. A sealed-epoch retry loop meant no raw-weight push was ever acknowledged. Add a read-only internal endpoint that returns COALESCE(MAX(sealed), 0) + 1 so clients target an epoch master will still accept.
Prefer GET /internal/v1/aggregation/target-epoch over local wall-clock. On sealed/stale/conflict 409, clear pending and retry with a bounded epoch or revision advance so the same sealed identity cannot loop. Verify the ACK path writes last_epoch, last_snapshot_id, and acknowledged_at.
… on 409 Mirror Prism: master-authoritative target epoch, bounded 409 identity advance, and tests that a successful push records last_epoch plus acknowledged_at. Stops the production sealed-epoch 409 loop that left the ledger never acknowledged.
📝 WalkthroughWalkthroughChangesRaw weight push coordination
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant RawWeightPushClient
participant TargetEpochEndpoint
participant RawWeightIngressService
participant DurableCursor
RawWeightPushClient->>TargetEpochEndpoint: Fetch authoritative target_epoch
TargetEpochEndpoint->>RawWeightIngressService: Authorize challenge and compute epoch
RawWeightIngressService-->>TargetEpochEndpoint: Return TargetEpochInfo
TargetEpochEndpoint-->>RawWeightPushClient: Return target_epoch
RawWeightPushClient->>RawWeightPushClient: POST raw-weight snapshot
RawWeightPushClient->>DurableCursor: Persist acknowledged epoch and snapshot_id
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/base/master/raw_weight_ingress.py (1)
1100-1129: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDeclare a typed
response_modelfor the new endpoint.
-> dict[str, Any]gives the generated OpenAPI an empty/unconstrained response schema, so the target-epoch contract clients now depend on is undocumented. A small Pydantic model (or reusingTargetEpochInfofields) keeps the generated spec authoritative.As per coding guidelines, "Treat OpenAPI generated by the code as the API source of truth".
♻️ Sketch
+class TargetEpochResponse(BaseModel): + target_epoch: int + highest_sealed_epoch: int | None + max_future_epoch_ahead: int + `@router.get`( "/internal/v1/aggregation/target-epoch", + response_model=TargetEpochResponse, responses={ }, )🤖 Prompt for 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. In `@src/base/master/raw_weight_ingress.py` around lines 1100 - 1129, Define a typed Pydantic response model for the target-epoch payload and register it as the response_model on get_aggregation_target_epoch. Replace the unconstrained dict[str, Any] return annotation with that model, preserving the existing target_epoch, highest_sealed_epoch, and max_future_epoch_ahead fields so OpenAPI documents the endpoint contract.Source: Coding guidelines
packages/challenges/prism/src/prism_challenge/raw_weight_push.py (1)
456-475: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winBoth clients fall back to the wall-clock epoch with no diagnostic when the lookup fails.
_fetch_master_target_epochreturnsNonefor any non-200 or exception without logging, so a misconfigured token or unreachable master silently restores the stale-bucket behavior this PR fixes.
packages/challenges/prism/src/prism_challenge/raw_weight_push.py#L456-L475: log status code / exception type (never the token) before returningNone.packages/challenges/agent-challenge/src/agent_challenge/evaluation/raw_weight_push.py#L446-L465: apply the same logging (diff provided in the per-site comment there).🤖 Prompt for 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. In `@packages/challenges/prism/src/prism_challenge/raw_weight_push.py` around lines 456 - 475, Update _fetch_master_target_epoch in packages/challenges/prism/src/prism_challenge/raw_weight_push.py (lines 456-475) and its sibling in packages/challenges/agent-challenge/src/agent_challenge/evaluation/raw_weight_push.py (lines 446-465) to log diagnostic context before returning None: include the non-200 response status code or exception type, and never include the shared token or other secret headers. Preserve the existing fallback behavior for invalid responses and failures.
🤖 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 `@packages/challenges/agent-challenge/tests/test_ac_raw_weight_push.py`:
- Around line 439-501: Update the sealed-epoch tests in
packages/challenges/agent-challenge/tests/test_ac_raw_weight_push.py:439-501 and
packages/challenges/prism/tests/test_raw_weight_push.py:561-615 so the mocked
GET returns epoch 49 on the initial query and 50 on the re-query after the 409;
then assert the exact POST sequence is [49, 50], replacing vacuous count-based
assertions while preserving the acknowledged result and final cursor checks.
---
Nitpick comments:
In `@packages/challenges/prism/src/prism_challenge/raw_weight_push.py`:
- Around line 456-475: Update _fetch_master_target_epoch in
packages/challenges/prism/src/prism_challenge/raw_weight_push.py (lines 456-475)
and its sibling in
packages/challenges/agent-challenge/src/agent_challenge/evaluation/raw_weight_push.py
(lines 446-465) to log diagnostic context before returning None: include the
non-200 response status code or exception type, and never include the shared
token or other secret headers. Preserve the existing fallback behavior for
invalid responses and failures.
In `@src/base/master/raw_weight_ingress.py`:
- Around line 1100-1129: Define a typed Pydantic response model for the
target-epoch payload and register it as the response_model on
get_aggregation_target_epoch. Replace the unconstrained dict[str, Any] return
annotation with that model, preserving the existing target_epoch,
highest_sealed_epoch, and max_future_epoch_ahead fields so OpenAPI documents the
endpoint contract.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4f4e7d54-53fa-4145-914a-494f809bcfe7
📒 Files selected for processing (6)
packages/challenges/agent-challenge/src/agent_challenge/evaluation/raw_weight_push.pypackages/challenges/agent-challenge/tests/test_ac_raw_weight_push.pypackages/challenges/prism/src/prism_challenge/raw_weight_push.pypackages/challenges/prism/tests/test_raw_weight_push.pysrc/base/master/raw_weight_ingress.pytests/unit/test_raw_weight_ingress.py
Comment on lines
+439
to
+501
| @pytest.mark.asyncio | ||
| async def test_sealed_epoch_409_advances_to_master_target(database: Database) -> None: | ||
| """Sealed-epoch 409 must not infinite-retry the same epoch; advance via master.""" | ||
|
|
||
| clock = FakeClock() | ||
| post_epochs: list[int] = [] | ||
|
|
||
| def handler(request: httpx.Request) -> httpx.Response: | ||
| if request.method == "GET": | ||
| # Master says next unsealed is 50 after first sealed rejection path. | ||
| return httpx.Response( | ||
| 200, | ||
| json={ | ||
| "target_epoch": 50, | ||
| "highest_sealed_epoch": 49, | ||
| "max_future_epoch_ahead": 2, | ||
| }, | ||
| ) | ||
| parsed = RawWeightPushRequest.model_validate_json(request.content) | ||
| post_epochs.append(parsed.epoch) | ||
| if parsed.epoch == 49: | ||
| return httpx.Response( | ||
| 409, json={"detail": "epoch is sealed; revision rejected"} | ||
| ) | ||
| return httpx.Response( | ||
| 200, | ||
| json={ | ||
| "protocol_version": "1.0", | ||
| "challenge_slug": SLUG, | ||
| "epoch": parsed.epoch, | ||
| "revision": parsed.revision, | ||
| "snapshot_id": f"snap-{parsed.epoch}", | ||
| "payload_digest": parsed.payload_digest, | ||
| "accepted": True, | ||
| "idempotent": False, | ||
| }, | ||
| ) | ||
|
|
||
| http = httpx.AsyncClient( | ||
| transport=httpx.MockTransport(handler), | ||
| base_url="http://master.test", | ||
| ) | ||
| client = RawWeightPushClient( | ||
| database=database, | ||
| challenge_slug=SLUG, | ||
| master_base_url="http://master.test", | ||
| shared_token=TOKEN, | ||
| now_fn=clock.now, | ||
| http_client=http, | ||
| # Stale local clock would stick on sealed 49 forever without master. | ||
| epoch_fn=lambda: 49, | ||
| ) | ||
| await client.init() | ||
| with activate_role(Role.CHALLENGE): | ||
| result = await client.push_once(weights={WINNER: 1.0}) | ||
| assert result.status == "acknowledged" | ||
| assert result.epoch == 50 | ||
| assert 49 in post_epochs or post_epochs == [50] | ||
| # Must not hammer the same sealed epoch repeatedly in one push_once. | ||
| assert post_epochs.count(49) <= 1 | ||
| assert await client.store.get_cursor() is not None | ||
| assert (await client.store.get_cursor()).epoch == 50 | ||
| await http.aclose() |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
The sealed-epoch tests never trigger a 409, so the advance branch is untested.
In both tests the mocked GET returns target_epoch: 50, and _resolve_target_epoch prefers it over epoch_fn, so the very first POST is already epoch 50 and the 409 handler is never entered. post_epochs.count(49) <= 1 (and 49 in post_epochs or post_epochs == [50]) then pass vacuously. Make the first GET return the stale/sealed epoch (or 409 the first POST at 50 once) so the identity-advance path is genuinely covered.
packages/challenges/agent-challenge/tests/test_ac_raw_weight_push.py#L439-L501: script the GET to return 49 first and 50 on the re-query, then assertpost_epochs == [49, 50].packages/challenges/prism/tests/test_raw_weight_push.py#L561-L615: same scripting, and assert the exact posted epoch sequence rather than acount(...) <= 1bound.
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] 478-478: Configuring an LLM/agent client endpoint over http:// sends prompts and responses (and often API keys) in cleartext, exposing them to interception. Use https for the base_url.
Context: base_url="http://master.test"
Note: [CWE-319] Cleartext Transmission of Sensitive Information.
(llm-client-insecure-http-python)
[warning] 478-478: Do not make http calls without encryption
Context: "http://master.test"
Note: [CWE-319] Cleartext Transmission of Sensitive Information.
(requests-http)
[warning] 483-483: Do not make http calls without encryption
Context: "http://master.test"
Note: [CWE-319] Cleartext Transmission of Sensitive Information.
(requests-http)
📍 Affects 2 files
packages/challenges/agent-challenge/tests/test_ac_raw_weight_push.py#L439-L501(this comment)packages/challenges/prism/tests/test_raw_weight_push.py#L561-L615
🤖 Prompt for 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.
In `@packages/challenges/agent-challenge/tests/test_ac_raw_weight_push.py` around
lines 439 - 501, Update the sealed-epoch tests in
packages/challenges/agent-challenge/tests/test_ac_raw_weight_push.py:439-501 and
packages/challenges/prism/tests/test_raw_weight_push.py:561-615 so the mocked
GET returns epoch 49 on the initial query and 50 on the re-query after the 409;
then assert the exact POST sequence is [49, 50], replacing vacuous count-based
assertions while preserving the acknowledged result and final cursor checks.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
409 conflicting raw weight payload/ sealed loop.last_epochandacknowledged_atstayed null for both challenges.Root cause
Clients chose epochs with local wall-clock buckets (
timestamp // interval). Master sealing force-advances past sealed identities (_next_seal_epoch). When pending expired, rebuild reused the sealed bucket with revision 1 and a new digest. Master rejected with 409; the client cleared pending and repeated. The ACK write path never ran.The working hotpatch semantic is
COALESCE(MAX(epoch),0)+1from sealedaggregation_epochs. That is now exposed as a read-only master endpoint and consumed by both challenge clients.Changes
GET /internal/v1/aggregation/target-epoch(challenge bearer + slug) returnstarget_epoch,highest_sealed_epoch,max_future_epoch_ahead.push_once: prefer master target epoch; on 409 sealed/stale/conflict, clear pending and bounded-retry with advanced epoch or revision.last_epoch/last_snapshot_id/acknowledged_at), sealed 409 advance, conflict revision bump, and master-over-local-clock preference.Test plan
pytest tests/unit/test_raw_weight_ingress.py packages/challenges/prism/tests/test_raw_weight_push.py packages/challenges/agent-challenge/tests/test_ac_raw_weight_push.py— 48 passedCI
Prod deploy confirmation
keepalive-weights.shorburn-weights-5gzi-48huntil real clients show ACKs.SELECT last_epoch, last_snapshot_id, acknowledged_at, pending_epoch FROM raw_weight_push_ledger WHERE id = 1;acknowledged_atandlast_epochnon-null after one successful push interval.curl -sS -H "Authorization: Bearer <challenge-token>" -H "X-Base-Challenge-Slug: prism" http://localhost:<master>/internal/v1/aggregation/target-epochshould returntarget_epoch = highest_sealed + 1.Notes
epoch_fnremains a fallback only when the master target-epoch GET is unreachable.Summary by CodeRabbit
New Features
Bug Fixes