revert: fix: close bug-hunter stale-state paths - #142
Conversation
This reverts commit 0d00daf.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:13126af0bd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| insertedDocumentOwnerId = null; | ||
| throw new Error(jobError.message); | ||
| } | ||
| if (jobError) throw new Error(jobError.message); |
There was a problem hiding this comment.
When the storage upload and documents insert succeed but ingestion_jobs.insert() fails, this now throws without deleting the document row. The catch block still removes the uploaded storage object, so the user is left with a queued document whose storage_path points to a missing file; because duplicate detection runs by content_hash, retrying the same upload can also be treated as an existing duplicate and never queue a job.
Useful? React with 👍 / 👎.
| } | ||
| throw new Error(jobError.message); | ||
| } | ||
| if (jobError) throw new Error(jobError.message); |
There was a problem hiding this comment.
For non-indexed full reindexes, the document is first changed to status: "queued" with page/chunk/image counts set to zero, but if the subsequent job insert fails this line returns an error with no job queued and no rollback. That leaves the document looking actively queued and emptied even though no worker can pick it up, so the previous indexed/failed state is lost until someone repairs the row manually.
Useful? React with 👍 / 👎.
| } | ||
| throw new Error(jobError.message); | ||
| } | ||
| if (jobError) throw new Error(jobError.message); |
There was a problem hiding this comment.
In the bulk full/retry path, each document is mutated to queued and its counts are zeroed before inserting the ingestion job; if this insert fails, the per-document result reports failure but the document row is left in the queued/empty state with no pending job. This makes bulk reindex failures leave selected documents stuck as active work that no worker can process.
Useful? React with 👍 / 👎.
| } | ||
| throw new Error(documentError.message); | ||
| } | ||
| if (documentError) throw new Error(documentError.message); |
There was a problem hiding this comment.
If the guarded job update succeeds but the following document update returns an error, the API now reports failure while leaving the ingestion job reset to pending. claim_ingestion_jobs claims by job status, so a worker can retry the job even though the document status/error message was not synchronized and the user was told retry failed; restore the prior job snapshot when the document update cannot be committed.
Useful? React with 👍 / 👎.
| } catch { | ||
| // Fall through to the legacy selection path for restricted browser contexts. | ||
| } | ||
| await navigator.clipboard.writeText(value); |
There was a problem hiding this comment.
In browsers or embedded contexts where navigator.clipboard.writeText exists but rejects because permission or context is restricted, this throws immediately and copyValue displays "Copy failed" without trying the legacy textarea selection path. Those contexts can still allow document.execCommand("copy"), so the copy action regresses for users who previously had the fallback.
Useful? React with 👍 / 👎.
Codex was right, and the finding is worth stating plainly: the earlier rows argued that because the sibling `documentScrollTop` assertion did not fail, the scroll position held and scroll-restoration causes were ruled out. Playwright aborts a test at the first failing `expect`. Once the `anchorTop` assertion threw, the `documentScrollTop` assertion never executed at all — its absence from the failure output is evidence of nothing. The narrowing is withdrawn and that class of cause is explicitly not ruled out. The capture itself is unaffected: the Services viewport-anchor failure is real, intermittent on byte-identical code (pass / pass / fail / pass-on-rerun), and distinct from #127. A superseding ledger record carries the correction rather than editing the earlier rows, per the append-only rule.
…ild-skip gate gap Records three items that surfaced during the /mockups/warning-consolidation study (PR #1437) and were not tracked anywhere: #141 P2 task — adopt a consolidated answer-home notice block; the study landed but nothing adopts it, and adoption moves the single site-wide APP-5 line across three composers #142 P2 issue — answer mode ships no verify-before-use caveat while every other clinical mode does #143 P2 issue — verify:pr-local exits 0 when guard-next-build refuses to run, reporting green with the build never executed Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NPyFcMfn1jMmphr6AqiWBg
Reverts #137 because downstream checks failed after merge.
This PR reverts commit 0d00daf.