Follow the update job, and diff from what actually published - #553
Open
galshubeli wants to merge 1 commit into
Open
Follow the update job, and diff from what actually published#553galshubeli wants to merge 1 commit into
galshubeli wants to merge 1 commit into
Conversation
Two halves of the same problem, both needing GraphRAG-Server#357 deployed. **Diff from what published, not from the push event.** `github.event.before` is "what main used to be"; it says nothing about whether the run for that commit succeeded. When one fails, the next push diffs from the failed run's head and the files in between are never sent again — which is why #545's one-file change is missing from the live graph with nothing to re-send it. The workflow now asks `GET /api/admin/update-graph/state` for `last_ingested_sha` and diffs from that, falling back to `event.before` when the graph has never published or the commit was rewritten away. If neither is usable the payload builder refuses rather than diffing from the empty tree, which would re-send the whole repo as additions. **Follow the job instead of holding a connection.** The endpoint is called with `?wait=false`, which answers 202 with a job id, and the workflow polls it every 30s, printing each phase as it changes. A 190-file diff took 2h23m against a 300s connection ceiling, so the old shape could not report the outcome of a large run at all: it went red while the work published two hours later. - 409 (another run holds the graph's lease) fails the job with an explanation rather than retrying — its result changes what this diff should have been computed against - an `attached: true` response means this commit was already in flight, so the workflow follows that job instead of starting a second ingest - 55 minutes without a terminal state fails, saying explicitly that this is *not* a failure: the run continues and publishes atomically. Job timeout raised to 60 minutes to match The step summary now names the commit it diffed from and the job it followed, so a red run says which of the three it was: refused, failed, or still going. 5 tests for the base resolution (published sha preferred, unreachable one falls back, empty one falls back, neither is refused, all-zero still means the empty tree). 20 tests total in the file.
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
Contributor
There was a problem hiding this comment.
Pull request overview
Updates the docs-to-GraphRAG ingestion pipeline to diff against what’s actually published (server-reported last_ingested_sha) and to treat the ingestion as an asynchronous job that the workflow follows to completion, improving correctness after failed runs and observability for long ingests.
Changes:
- Update
update-graph.ymlto query the server for the last published SHA, start ingestion with?wait=false, then poll the returned job until completion/failure/timeout. - Add
_resolve_base()inbuild_diff_payload.pyto select a safe diff base (published → push parent → refuse) and adjust base handling inmain(). - Add 5 tests covering base selection scenarios.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| .github/workflows/update-graph.yml | Switches to “diff from published SHA” and job-following semantics with longer timeout and richer step summary. |
| .github/scripts/build_diff_payload.py | Introduces base resolution logic to safely choose a diff base (or refuse) instead of relying solely on github.event.before. |
| .github/scripts/test_build_diff_payload.py | Adds unit tests for base resolution behavior and related edge cases. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
42
to
+45
| env: | ||
| GRAPH_ID: docs_benchmark | ||
| GRAPHRAG_UI_URL: ${{ vars.GRAPHRAG_UI_URL }} | ||
| UPDATE_GRAPH_TOKEN: ${{ secrets.UPDATE_GRAPH_TOKEN }} |
Comment on lines
+115
to
+122
| job=$(jq -r '.job_id' start.json) | ||
| attached=$(jq -r '.attached' start.json) | ||
| echo "job=$job" >> "$GITHUB_OUTPUT" | ||
| if [ "$attached" = "true" ]; then | ||
| echo "::notice::Attached to in-flight job $job for this same commit" | ||
| else | ||
| echo "::notice::Started job $job" | ||
| fi |
| echo "| | |" | ||
| echo "|---|---|" | ||
| echo "| Commit | \`${GITHUB_SHA}\` |" | ||
| echo "| Diffed from | \`${{ steps.state.outputs.published }}\` |" |
Comment on lines
+268
to
+273
| base = _git(repo, "rev-parse", "HEAD") | ||
| (repo / "guides" / "new.mdx").write_text("new\n", encoding="utf-8") | ||
| head = _commit(repo, "add a page") | ||
| monkeypatch.chdir(repo) | ||
|
|
||
| assert bdp._resolve_base(base, head) == base |
Comment on lines
222
to
+226
| head = os.environ["HEAD_SHA"] | ||
|
|
||
| if set(base) == {"0"}: | ||
| # First push to a brand-new branch: everything in the tree is new. | ||
| base = EMPTY_TREE_SHA | ||
| elif not _commit_exists(base): | ||
| base = _resolve_base( | ||
| os.environ.get("BASE_SHA", ""), os.environ.get("BASE_SHA_FALLBACK", ""), | ||
| ) | ||
| if base is None: |
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.
Why
Stacked on #552 — review that one first; this PR's diff against it is one workflow file, one function, and five tests.
Two problems left over from the pipeline review, both needing FalkorDB/GraphRAG-Server#357 deployed before this can merge.
A failed run's files are lost forever. The workflow diffs from
github.event.before, which is "what main used to be" — it says nothing about whether the run for that commit succeeded. When one fails, the next push diffs from the failed run's head and everything in between is never sent again. That is why #545's one-file change is missing from the live graph with nothing to re-send it.A large run cannot report its own outcome. A 190-file diff took 2h23m against a 300s connection ceiling. CI went red at five minutes; the work published at 13:33 with nobody watching.
What changed
Diff from what actually published. The workflow asks
GET /api/admin/update-graph/stateforlast_ingested_sha— written by the compare-and-set that publishes, so it means "the commit whose content is live", not "the last commit CI mentioned". Diffing from it picks up everything since the last run that landed, including the work of runs that didn't.Fallbacks, in order: the published sha → the push event's parent → refuse.
_resolve_basehandles a graph that has never published (no sha yet) and a sha rewritten out of history. If neither is usable it fails, because the remaining option is the empty tree, and that would re-send the entire repo as additions.Follow the job.
?wait=falseanswers 202 with a job id; the workflow polls every 30s and prints each phase as it changes (copying,ingesting,reconciling,smoke,promoting,flipping).202+attached: false202+attached: true409The summary says which of the three it was. A red run now names the commit it diffed from, the job it followed, and whether it was refused, failed, or still going. Previously every one of those looked the same.
Testing
5 new tests for base resolution — published sha preferred, unreachable one falls back, empty one falls back, neither is refused, all-zero still means the empty tree. 20 in the file, run by the
check-payloadjob from #552.The workflow shell itself is not unit-testable here; it is deliberately boring —
set -euo pipefail, explicit status checks,jq -rwith defaults, and no--retryanywhere.Merge order
/stateand?wait=false)Merging this before step 1 breaks the workflow:
/statewould 404 and the POST would answer 200 instead of 202.