Avoid truncating event.json while other steps read it - #4675
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The new atomic replace can throw (notably on Windows) if event.json is open, and WriteWebhookPayload() exceptions are not handled by callers, risking intermittent step failures.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR updates how the runner writes event.json (the webhook payload surfaced via GITHUB_EVENT_PATH) to avoid readers observing a truncated/partially-written file when steps overlap.
Changes:
- Switch from in-place writes to writing a sibling temp file and atomically renaming it over
event.json. - Add a dedicated helper (
WriteWebhookPayloadFile) documenting the concurrency/truncation failure mode and implementing the atomic replace.
File summaries
| File | Description |
|---|---|
src/Runner.Worker/ExecutionContext.cs |
Writes the webhook payload via temp-file + atomic rename to prevent concurrent readers from seeing truncated JSON. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // Every step writes the very same webhook payload to the very same file before it runs. | ||
| // Steps can run concurrently (parallel/background steps), and writing in place truncates the | ||
| // file first, so a concurrently starting action can read an empty or partially written | ||
| // payload, which surfaces as `SyntaxError: Unexpected end of JSON input` inside the action. | ||
| // Write to a sibling temporary file and rename it over the target instead, so a reader |
f5a6074 to
a65feba
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The new helper currently does unnecessary temp writes/exception-driven control flow after the file exists, and the cleanup finally can throw and mask the real error, risking step failures and performance regressions.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
src/Runner.Worker/ExecutionContext.cs:1333
- This change materially alters how
event.jsonis published (create-once, atomic publish) but there’s no L0 coverage asserting the key behavior (e.g., a second call does not overwrite/partially-write the file, and does not throw when the file already exists). Adding a focused unit test would help prevent regressions in this concurrency-sensitive path.
WriteWebhookPayloadFile(workflowFile, gitHubEvent);
- Files reviewed: 1/1 changed files
- Comments generated: 2
- Review effort level: Lite
a65feba to
b437ccd
Compare
|
IDK who is the maintainer here, but I'm suffering from this bug multiple times a day. Maybe @TingluoHuang or @salmanmkc could take a look? The change is quite simple. |
Write the payload to a sibling temporary file and move it into place without allowing an overwrite. That move is atomic and create-only -
link()+unlink()on Unix,MoveFileon Windows - so it either publishes the complete payload in one step or fails withEEXIST/ERROR_ALREADY_EXISTSbecause another step already published it. Since every step writes identical bytes, that failure means the work is already done and is ignored.event.jsonis therefore created exactly once per job and never replaced, so no action can ever read it mid-write.Fixes actions/checkout#2484
Fixes actions/cache#1793