Deliver jobs before stale broker messages, so a self-hosted job cannot stall - #39
Merged
Merged
Conversation
GitHub pushes RunnerRefreshConfig to every runner about once a day. The proxy queued it per target with no worker session there to receive it, so the next worker's first poll returned the refresh instead of its job. The runner then rewrote its config and restarted its session, the second /session call found no pending target, and the job sat queued until the app was restarted. - Deliver the job first; only jobs and cancellations are queued at all - Resolve /session to the target whose runner the request names, falling back to the positional pending queue - Queue a cancellation only for a job that is queued or running here; GitHub redelivers them for a while after a job ends - Log the runner's own /acknowledge request; the upstream ack still 400s and its expected shape is unknown - Cap request bodies the proxy reads at 64 KB Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
A live run showed the runner's /session body: sessionId, ownerName, and
agent {id, name, version, osDescription}. Drop the speculative agentName
fallback and stop logging the body now that its shape is known.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
A listener spawned ahead of any job runs in the generic sandbox; the worker spawned for a job carries the repository's approved policy. Binding every named session let the idle listener win the next job and fail it: cargo could not read ~/.rustup. A named session now takes only its own target's pending entry and stays unbound otherwise. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Changes recommended
isJobLive can treat queued cancellation messages as evidence a job is live, which may cause repeated cancellation redeliveries to be queued indefinitely.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR hardens the broker proxy’s message routing to prevent a self-hosted runner from stalling when stale broker housekeeping messages (e.g., RunnerRefreshConfig) get delivered ahead of a queued job, and improves session↔target binding using the runner’s agent.name.
Changes:
- Drop non-job, non-cancellation broker messages instead of queuing them, and prefer delivering queued job messages ahead of stale messages.
- Bind sessions to targets by runner
agent.nameand only when a pending assignment exists for that target, avoiding speculative listeners consuming assignments. - Add bounded request-body reading (413 on oversized bodies) and expand routing tests to cover the new behaviors.
File summaries
| File | Description |
|---|---|
| src/main/broker-proxy-service.ts | Implements job-first delivery, drops non-forwardable messages, improves session→target binding, and adds bounded request-body reading. |
| src/main/broker-proxy-service.test.ts | Adds a new “message routing” test suite validating stale-message handling, cancellation behavior, acknowledge handling, body size limits, and session binding. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Copilot review on #39. isJobLive scanned the queues for any message naming the job, and a queued JobCancellation names it too - so a cancellation was evidence of its own liveness. GitHub redelivers a cancellation while the job is unfinished, and each redelivery found the previous one still queued and added another: unbounded growth, and the next worker to poll handed a cancellation instead of a job. That is the stall this file exists to stop, reintroduced by the check meant to bound it. Only a job assignment counts now. isJobAssignmentMessage already drew that line for the message that arrives; it draws it for the queue too. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XVXbqWE6zBi1HV7b4uobKh
There was a problem hiding this comment.
🟡 Changes recommended
A jobless session can still be handed a queued cancellation when no job is present due to the new queue selection logic, which can misroute cancellations and contradict the intended delivery ordering.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
src/main/broker-proxy-service.ts:1242
/acknowledgelogs the request body at info level. This can be very noisy (one log line per runner ack) and increases the chance of leaking unexpected payload data into normal user logs; debug-level is a safer default for request-body logging.
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
Copilot review on #39, second finding. getMessageForTarget took the head of the queue whenever no job assignment was queued - Math.max(jobIndex, 0) turns "no job found" into index 0 - so a worker holding no job was handed somebody else's JobCancellation. It stole the message from the worker actually running that job, and markSessionWithJob then stamped its session with a job id it never held, which in turn made isJobLive count that session as live. The comment above it already described the intended rule ("a cancellation queued ahead of it is for a job this worker doesn't hold yet"); the code only honoured it while a job happened to be queued too. Now a job assignment is delivered first as before, and with none queued a cancellation goes only to the session whose currentJobId it names. The first test asserts state rather than awaiting a response: with nothing deliverable the handler long-polls, which is precisely the behaviour under test. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XVXbqWE6zBi1HV7b4uobKh
There was a problem hiding this comment.
🟡 Changes recommended
It introduces a new log path that can allow log injection via unescaped request-body content, and one new test leaves a long-poll request pending without cleanup which can cause open-handle flakiness.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
src/main/broker-proxy-service.ts:1242
- The /acknowledge handler logs raw request-body content, which can contain newlines. Because log-file writes messages verbatim, this allows log injection (a request can forge additional log lines) and can make logs misleading. Consider escaping CR/LF (or logging a parsed/structured summary) before writing it to the log.
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
Two more from Copilot on #39. The test I added for cancellation routing started a long-poll and never ended it. handleMessagePoll schedules real timers until a 50s timeout, so the pending request and its timer outlived the test - open handles, and flaky or hanging under stricter Jest settings. It now ends the poll the way one really ends, by pushing the job the worker was waiting for, which cleans the request up and makes a stronger assertion besides: the queued cancellation was skipped rather than consumed, and is still there afterwards. Separately, /acknowledge logged the raw request body at info. log-file writes messages verbatim, so a body containing CR/LF could forge log lines, and it is one line per message a runner receives. It is JSON-encoded now, which escapes newlines, and at debug. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XVXbqWE6zBi1HV7b4uobKh
There was a problem hiding this comment.
🔵 Needs a closer look
There’s a concrete type/robustness issue in jobIdFromMessage() and some newly introduced unreachable routing logic that should be corrected to avoid subtle message-key mismatches and misleading control flow in this high-impact path.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
src/main/broker-proxy-service.ts:202
jobIdFromMessage()is typed to returnstring | undefined, but it returnsinnerBody?.jobId || innerBody?.runner_request_idwithout validating/coercing the value. If GitHub ever sends a numericjobId, this can introduce subtle key mismatches (e.g., Map lookups with string IDs vs number IDs) and break equality checks.
src/main/broker-proxy-service.ts:1430- In
getMessageForTarget(), the post-jobIndexblock attempts to route cancellations bysession.currentJobId, but this function only runs in the branch wheresession.currentJobIdis falsy (the truthy case returns earlier). As written, this cancellation-routing code is unreachable and the surrounding comments are misleading.
- Files reviewed: 2/2 changed files
- Comments generated: 0 new
- Review effort level: Lite
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
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.
Rebases the never-merged
fix/broker-proxy-stale-message-stallwork onto currentmain.Why now
This fix has been deployed on the developer machine as a packaged build since 2026-09-02, but it was never merged. Rebuilding the app from
maintherefore silently reverts the runner to the stalling build — which is exactly what happened while working on #38: three self-hosted CI jobs hung for ten minutes each and reported failure, while the GitHub-hosted leg of the same commit passed.The fingerprint is
Returning message to workerappearing in the app log before the runner'sListening for Jobs, with noWorker_*.logever created: the runner starts, is handed a stale message instead of its job, restarts its session, and polls forever.The bug
processMessagequeued every non-job broker message per target —RunnerRefreshConfig,AgentRefresh,JobCancellation— with no worker session there to receive it. GitHub sendsRunnerRefreshConfigto all instances roughly daily.handleMessagePolldid a barequeue.shift(), so the next worker's first poll got the stale refresh rather than its job; the runner then wrote.runner_migrated, restarted its session, and the secondPOST /sessionfound an emptypendingTargetAssignments, so it had no target and polled indefinitely. One stale message, one stalled job.The change
agent.name, instead of consuming whichever pending assignment happened to be first.Rebase note
Only
broker-proxy-service.test.tsconflicted, and only becausemainand this branch each append a different top-leveldescribeto the same file (extractGitHubJobInfofrom #35,message routinghere). Both suites are kept in full; the implementation rebased cleanly.Verified on the rebase: 983 tests pass, tsc and eslint clean.
🤖 Generated with Claude Code
https://claude.ai/code/session_01XVXbqWE6zBi1HV7b4uobKh