Skip to content

fix(knowledge): clear stale error_message on the remaining reprocess paths - #36

Draft
lyingbug wants to merge 2 commits into
mainfrom
cursor/clear-stale-error-message-followup-50d9
Draft

fix(knowledge): clear stale error_message on the remaining reprocess paths#36
lyingbug wants to merge 2 commits into
mainfrom
cursor/clear-stale-error-message-followup-50d9

Conversation

@lyingbug

@lyingbug lyingbug commented Aug 3, 2026

Copy link
Copy Markdown
Owner

Description

Follow-up to Tencent#2488, which cleared error_message in SetFinalizing, FinalizeSubtask and ProcessDocument. Reviewing the surrounding paths turned up two more places where a previous attempt's failure message is still persisted, both reachable without any of the transitions Tencent#2488 fixed:

1. ProcessManualUpdate flips a row back to processing without clearing error_message.

The entry guards skip completed, deleting and cancelled, but deliberately not failed. So a manual knowledge item that failed earlier — for example when GetKnowledgeBaseByID failed and the handler wrote parse_status=failed plus a message, or when Asynq redelivers the task — re-enters processing still carrying that message. The UI then shows a failure on a document it is simultaneously showing as running. This is the exact symptom Tencent#2488 fixed for ProcessDocument, on the manual-knowledge sibling.

2. finalizeIndexedKnowledgeState is the only route to completed that bypasses FinalizeSubtask.

When a document ends up with no text chunks and no pending multimodal work, it is completed in place rather than handed to the post-process pipeline, so the promotion that clears the column in Tencent#2488 never runs. Combined with (1), a manual item that failed once and is then reindexed into an empty result lands on completed with the old failure message still attached. processChunks passes the same *types.Knowledge pointer through from the worker, so the stale in-memory value is what gets persisted.

The processing transition is extracted into markKnowledgeProcessing so ProcessDocument and ProcessManualUpdate share one definition of what starting a new attempt means, rather than having the clear applied at one call site and forgotten at the other. The helper also replaces the "processing" string literal with types.ParseStatusProcessing.

Paths checked and deliberately not changed

Type of Change

  • 🐛 Bug fix

Related Issue

Follow-up to Tencent#2488. No separate issue.

Testing

  • TestMarkKnowledgeProcessingClearsPreviousAttemptError — new, covers the shared helper directly: a failed row with a message must come out processing with the column cleared.
  • TestFinalizeIndexedKnowledgeState — extended: every case now seeds a stale error_message and asserts it is cleared on the completed branch and left in place on the processing branches, where FinalizeSubtask clears it on promotion.
  • Verified the tests actually catch the regression: temporarily reverting the finalizeIndexedKnowledgeState clear makes TestFinalizeIndexedKnowledgeState/empty_indexed_document_is_completed_without_summary_work fail with ErrorMessage = "previous attempt failed", want ""; restoring it passes.
  • go build ./internal/... — clean (needs libsqlite3-dev for the sqlite-vec cgo bindings).
  • go test ./internal/application/service/ ./internal/application/repository/ -count=1 — both pass.
  • go vet ./internal/application/service/ — clean.
  • gofmt -l on both changed files — clean; git diff --check — clean.

Checklist

  • git diff --check origin/main...HEAD passes
  • Changed source files are formatted
  • Targeted tests for the changed packages/components pass
  • Diff-scoped lint passes where applicable — not run: golangci-lint refuses to start in this environment with the Go language version (go1.25) used to build golangci-lint is lower than the targeted Go version (1.26.0). go vet was run instead and is clean.
  • Full-repository checks were run, or any unrelated/environment-dependent failures are documented above
  • Self-reviewed the code
  • Added/updated tests covering the change
  • Updated related documentation (the clearing semantics are documented on both changed functions)
  • Breaking changes are clearly called out in the description above

Screenshots / Recordings

Not applicable — no UI change. The user-visible effect is that an existing document card stops rendering a stale failure message; the frontend already reads error_message as-is.

Open in Web Open in Cursor 

A knowledge item that failed once keeps error_message set; when it is
reprocessed or driven through the finalizing pipeline, the stale message
was never cleared, so the UI kept showing an outdated failure on items
that were processing or had completed successfully.

- SetFinalizing and FinalizeSubtask now clear error_message in the same
  atomic UPDATE that transitions parse_status
- ProcessDocument clears knowledge.ErrorMessage when flipping the row
  to processing for a new attempt

Add a SQLite-backed regression test covering both transitions.
@cursor
cursor Bot force-pushed the cursor/clear-stale-error-message-followup-50d9 branch from f934870 to cf2cb1d Compare August 3, 2026 11:52
…paths

Follow-up to the error_message clearing added for SetFinalizing,
FinalizeSubtask and ProcessDocument. Two routes still persisted a
previous attempt's failure message:

- ProcessManualUpdate flips a row back to 'processing' without clearing
  error_message. A manual knowledge item that failed earlier (for
  example when the knowledge base lookup failed, or after an Asynq
  retry) re-enters processing still carrying that message, so the UI
  shows a failure on a document it also shows as running.

- finalizeIndexedKnowledgeState is the only route to 'completed' that
  bypasses FinalizeSubtask. When a document has no text chunks and no
  pending multimodal work it is completed in place, so the promotion
  that would otherwise clear the column never runs and a successfully
  indexed row keeps reporting the earlier failure.

The 'processing' transition is extracted into markKnowledgeProcessing so
ProcessDocument and ProcessManualUpdate share one definition of what
starting a new attempt means, and a future worker cannot reintroduce the
same omission. The helper also replaces the "processing" string literal
with types.ParseStatusProcessing.

Tests: markKnowledgeProcessing gets direct coverage, and the
finalizeIndexedKnowledgeState table now seeds a stale error_message and
asserts it survives the processing branches (FinalizeSubtask clears it
later) and is cleared on the completed branch.
@cursor
cursor Bot force-pushed the cursor/clear-stale-error-message-followup-50d9 branch from cf2cb1d to e092f35 Compare August 3, 2026 11:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants