fix(worker): echo the lease fence on fail and the idempotency key on complete - #14
Conversation
…complete Two fields the claim hands out that the Java worker was dropping. lease_fence on /fail (#12). Without it the engine settles the item but emits no NodeFailed, so the scheduler fold keeps the node `scheduled` and the execution never reaches a terminal state. Every Java tool failure stranded its workflow, including the RCE-gate rejection - the fence is now read before that gate, since a rejection is still a settle. A 409 means the lease was reclaimed and a new worker owns the item, so it is a quiet no-op: reporting our failure would kill work that worker is running. idempotency_key on /complete (#13). The engine records the result against it, so a re-run replays instead of firing the tool a second time. Omitted, nothing lands in tool_effects and every replay re-fires. Both public client methods keep their existing signatures - the old ones delegate - so this is source and binary compatible with 0.4.0. The unfenced failWorkItem is deprecated with the reason. Tests mutation-checked: dropping the fence, dropping the key, and removing the 409 catch each fail their guard. Full suite green. Closes #12 Closes #13
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 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 |
There was a problem hiding this comment.
Pull request overview
This PR updates the Java agent worker/client to correctly echo two engine-provided claim fields (lease_fence on /fail and idempotency_key on /complete) so failures transition executions to terminal states and replays don’t re-fire tools.
Changes:
- Thread
lease_fenceinto/work-items/{id}/failcalls and treat HTTP 409 as a quiet lost-lease outcome. - Thread
idempotency_keyfrom claim responses into/work-items/{id}/completeso the engine can persist and replay tool effects. - Add focused tests that pin request bodies and the worker loop behavior around 409 and missing keys.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
jamjet-agent/src/test/java/dev/jamjet/agent/worker/JavaToolWorkerTest.java |
Adds tests for fencing on fail and idempotency key echoing (including absent-key behavior). |
jamjet-agent/src/main/java/dev/jamjet/agent/worker/JavaToolWorker.java |
Reads/threads lease_fence earlier and centralizes fenced failure handling (409 => lost lease); echoes idempotency key on complete. |
jamjet-agent/src/main/java/dev/jamjet/agent/client/JamjetEngineClient.java |
Adds a completeWorkItem overload that can send idempotency_key; adds fenced failWorkItem overload and deprecates unfenced legacy method. |
jamjet-agent/src/main/java/dev/jamjet/agent/client/ClaimedWorkItem.java |
Extends the claim DTO to include idempotencyKey. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| Long leaseFence | ||
| Long leaseFence, | ||
| String idempotencyKey | ||
| ) {} |
From the Copilot review on #14. A record canonical constructor is public API. Adding the idempotencyKey component widened it, so any caller building a ClaimedWorkItem directly would have broken at both source and binary level - every accessor still resolving is not the same thing, and the PR description claim of 0.4.0 compatibility was wrong as written. Restored the 7-arg signature as an overload delegating null, and pinned it with a test that constructs one the old way. Removing the overload stops that test compiling, which is the point.
|
Good catch, and it made a claim in my description false — fixed in You are right that a record canonical constructor is public API. Adding The 7-arg signature is back as an overload delegating Suite: 83 tests, green. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
jamjet-agent/src/main/java/dev/jamjet/agent/worker/JavaToolWorker.java:295
- This comment says the behavior mirrors a Python "re-raise", but the code now calls fail(...) which catches JamjetHttpException and returns LOST_LEASE rather than rethrowing. Update the comment so it matches the current control flow (attempt to fail; if failing can't be reported, treat as lost lease).
// Any other completion error keeps the fail behavior (mirror Python re-raise).
Two fields the claim hands out that the Java worker was dropping. Closes #12 and #13.
lease_fenceon/fail(#12)Without it the engine settles the item but emits no
NodeFailed, so the scheduler fold keeps the nodescheduledand the execution never reaches a terminal state. Every Java tool failure stranded its workflow — the engine has been able to handle it correctly since jamjet#118; the client just never sent the fence.409means the lease was reclaimed and a new worker owns the item, so it is a quiet no-op. Reporting our failure would kill work that worker is running.idempotency_keyon/complete(#13)The engine records the result against it, so a re-run replays instead of firing the tool a second time. Omitted, nothing lands in
tool_effectsand every replay re-fires. Pairs with jamjet#128, which added the field on the engine side.Compatibility
Both public client methods keep their existing signatures — the old ones delegate — so this is source and binary compatible with 0.4.0. The unfenced
failWorkItem(String, String)is deprecated with the reason. A claim from an older engine carries no key; the worker completes exactly as before.ClaimedWorkItemneeded the same care, and my first version of this PR got it wrong: a record's canonical constructor is public API, so adding theidempotencyKeycomponent widened it and would have broken any caller building one directly — accessors still resolving is not the same thing. Caught by Copilot. The 7-arg signature is back as an overload delegatingnull, pinned bythePreIdempotencyKeyConstructorStillCompiles; deleting the overload stops that test compiling.Tests
Four added, each mutation-checked against the bug it guards:
aFailureReportsTheFenceSoTheNodeIsRescheduledfail()sends no fencetheClaimsIdempotencyKeyIsEchoedOnCompleteaConflictOnFailDoesNotEscapeAndKillTheWorkerrunOnce())aClaimWithoutAKeyStillCompletesThe third pins the escape, not a 409/other distinction — a failure we could not report is
LOST_LEASEeither way, because in both cases we did not settle the item and the reclaimer must have it. Only the log level differs, and the test says so rather than implying more.Full repo suite green.