Skip to content

fix: bound native scan claim queue reads - #3662

Draft
Patrick-Erichsen wants to merge 1 commit into
mainfrom
codex/clawscan-claim-read-budget
Draft

fix: bound native scan claim queue reads#3662
Patrick-Erichsen wants to merge 1 commit into
mainfrom
codex/clawscan-claim-read-budget

Conversation

@Patrick-Erichsen

Copy link
Copy Markdown
Collaborator

Native ClawScan workers requesting one job currently fetch up to 512 ready queue rows when generic GitHub rollout is disabled. Start each source page at the worker's remaining capacity, expanding to the existing bounded window only after encountering blocked legacy GitHub jobs. Preserve priority ordering, rollout restrictions, catalog admission and lease behavior.

Before/after proof on a disposable local Convex backend (http://127.0.0.1:3318) with 600 queued native bulk-rescan jobs:

Claim size Queue rows before → after (regression test) Real Convex document budget Before → after
1 512 → 1 14 Read-limit error → 1 claimed
4 512 → 4 26 Read-limit error → 4 claimed
16 512 → 16 74 Read-limit error → 16 claimed

The existing 513-blocked-GitHub-job fallback still reaches the allowed NVIDIA job in at most three source pages. All 116 security-scan tests pass. Temporary seed/proof functions were deployed only locally, excluded from this PR, and the disposable backend was stopped.

This is a read-efficiency fix, not a demonstrated scanner throughput fix. Local burst tests with 128 one-job requests at concurrency 16 and 32 still encountered heavy OCC contention before and after; there was no consistent throughput improvement. Production workers remain at concurrency four. Production claim errors are sanitized and backend diagnostics require a Convex login, so the production root cause remains unverified. Keep this draft while that investigation continues; no deployment or capacity increase is part of this PR.

Validation:

  • bunx vitest run convex/securityScan.test.ts — 116 passed; new regression cases fail on the original code.
  • bun run ci:unit — 6,613 passed, 3 skipped; coverage gate passed.
  • bun run ci:static — passed.
  • bun run ci:types-build — passed.
  • bunx tsc -p convex/tsconfig.json --noEmit — passed.
  • Repository autoreview — no actionable findings.

@clawsweeper

clawsweeper Bot commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

@vercel

vercel Bot commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated
clawhub Ready Ready Preview Sep 11, 2026 12:51am UTC

Request Review

@clawsweeper

clawsweeper Bot commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

Codex review: blocked before merge. Reviewed September 10, 2026, 8:51 PM ET / September 11, 2026, 00:51 UTC.

ClawSweeper review

What this changes

Reduce ClawScan worker queue reads to the requested claim capacity, expanding pagination only when blocked legacy jobs require it, with regression tests and a documented invariant.

Merge readiness

Blocked before merge - 3 items remain

Keep open: current main still overfetches native queue rows, and this focused patch has no blocking findings. The author explicitly requests retaining draft status during the production investigation.

Priority: P2
Reviewed head: 18ddd2c08febde9bd0dcbb38af9a2e0a0f658c64
Owner decision: Required. See Decision needed.

Review scores

Measure Result What it means
Overall readiness 🐚 platinum hermit (4/6) A focused implementation with useful regression coverage and reported local runtime validation; the explicit draft hold does not lower patch quality.
Proof confidence 🌊 off-meta tidepool Not applicable: The collaborator-authored PR is exempt from the external-contributor proof gate, and this patch does not materially change authority. Its body reports real local Convex claim-budget comparisons supporting the changed claim handler, but does not establish production contention recovery.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Not applicable Not applicable: The collaborator-authored PR is exempt from the external-contributor proof gate, and this patch does not materially change authority. Its body reports real local Convex claim-budget comparisons supporting the changed claim handler, but does not establish production contention recovery.
Evidence reviewed 8 items Current main still contains the overfetch: The main-branch claim helper requests 512 returned rows whenever GitHub Skill Sync is disabled, regardless of remaining worker capacity. The branch changes that initial page size without replacing the eligibility or leasing logic.
Production caller and security boundaries: The worker calls claimCodexScanJobLeases, which authenticates its token before invoking the changed mutation. Source ordering, rollout eligibility, catalog admission, final job validation, and lease issuance remain intact; hydration additionally checks worker identity and lease token.
Focused regression coverage: Added cases assert returned queue rows equal claim sizes 1, 4, and 16. The existing legacy-GitHub scenario now also limits pagination to three publish-source queries before reaching the allowed NVIDIA job. These use mocked database pagination and are supplemental validation.
Findings None None.
Security None None.

How this fits together

ClawScan workers request queued security scans from Convex. The claim handler selects eligible jobs in priority order and issues leases before workers retrieve artifacts and run scans.

flowchart TD
  A[Worker requests capacity] --> B[Authenticate worker]
  B --> C[Read ready queue by source]
  C --> D{Job allowed by rollout?}
  D -->|No| E[Expand next page]
  E --> C
  D -->|Yes| F[Validate job and catalog admission]
  F --> G[Issue scan leases]
Loading

Decision needed

Question Recommendation
Should this read-efficiency fix remain draft during the production investigation, or be released independently of the unresolved contention problem? Honor the investigation hold: Keep the PR draft until the owner concludes the production investigation or explicitly releases the hold.

Why: The author explicitly requested a draft hold; source review cannot override that investigation boundary.

Before merge

  • Resolve merge risk (P1) - The production claim-failure cause remains unverified, and the reported burst experiments do not establish a throughput improvement.
  • Complete next step (P2) - Obtain the owner’s explicit release of the draft hold before merging this read-efficiency fix.
  • Resolve maintainer decision - Resolve the maintainer decision shown above before merge.
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Change size +45/-4 across 3 files; production net +5, tests +32, specification +4 Production growth is limited to adaptive pagination and is justified by the demonstrated overfetch.
Claim-size regression cases 3 sizes: 1, 4, and 16 jobs Coverage checks that small native claims no longer return a fixed 512-row page.

Merge-risk options

Maintainer options:

  1. Decide the mitigation before merge
    Retain capacity-sized native pagination and the existing legacy/catalog exceptions, with production contention treated as a separate diagnostic question.
  2. Pause or close
    Do not merge this PR until maintainers decide whether the risk is worth taking.

Technical review

Best possible solution:

Retain capacity-sized native pagination and the existing legacy/catalog exceptions, with production contention treated as a separate diagnostic question.

Do we have a high-confidence way to reproduce the issue?

Yes: current main requests 512 queue rows for a one-job native claim with GitHub rollout disabled; the supplied local experiment reports corresponding read-budget failures. This review verified the source path without running tests or backend code.

Is this the best way to solve the issue?

Yes, for native queue overfetch: changing page size in the shared claim helper preserves existing callers and eligibility rules. Raising transaction budgets would retain unnecessary reads, while changing indexes or rollout policy would broaden the fix without supporting evidence.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning medium; reviewed against 385669695b1a.

Labels

Label changes:

  • add P2: This is a bounded backend read-efficiency fix; an urgent production failure attributable to this mechanism is not established.
  • add rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🌊 off-meta tidepool and patch quality is 🐚 platinum hermit.

Label justifications:

  • P2: This is a bounded backend read-efficiency fix; an urgent production failure attributable to this mechanism is not established.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🌊 off-meta tidepool and patch quality is 🐚 platinum hermit.

Evidence

What I checked:

  • Current main still contains the overfetch: The main-branch claim helper requests 512 returned rows whenever GitHub Skill Sync is disabled, regardless of remaining worker capacity. The branch changes that initial page size without replacing the eligibility or leasing logic. (convex/securityScan.ts:3108, 385669695b1a)
  • Production caller and security boundaries: The worker calls claimCodexScanJobLeases, which authenticates its token before invoking the changed mutation. Source ordering, rollout eligibility, catalog admission, final job validation, and lease issuance remain intact; hydration additionally checks worker identity and lease token. (convex/securityScan.ts:3908, 18ddd2c08feb)
  • Focused regression coverage: Added cases assert returned queue rows equal claim sizes 1, 4, and 16. The existing legacy-GitHub scenario now also limits pagination to three publish-source queries before reaching the allowed NVIDIA job. These use mocked database pagination and are supplemental validation. (convex/securityScan.test.ts:4226, 18ddd2c08feb)
  • Captured runtime evidence and explicit hold: The supplied complete PR body reports a disposable local Convex run with 600 native jobs: document budgets 14, 26, and 74 changed from read-limit failures to successful claims of 1, 4, and 16 jobs. It separately reports persistent OCC contention and no consistent throughput improvement, and explicitly requests keeping the PR draft. These are contributor-reported results; this review did not execute them. (18ddd2c08feb)
  • Convex contract and durable intent: The changed handler directly consumes Convex pagination through listReadySourceJobsForClaimHandler. Repository-generated Convex guidance describes numItems as a page-size target and distinguishes it from scan limits; the retained query filter can still scan excluded rows. The new specification appropriately documents normal native claims and the legacy/catalog exceptions. (convex/_generated/ai/guidelines.md:141, 18ddd2c08feb)
  • Feature-history routing: Main history repeatedly identifies Patrick Erichsen on securityScan.ts, including rollout and recent bulk-rescan work. GitHub commit metadata maps the rollout commit to Patrick-Erichsen. Local blame and deeper content history encountered unavailable objects, so no verified source-line introduction attribution is claimed. (convex/securityScan.ts, fe8eff20eeba)

Likely related people:

  • Patrick-Erichsen: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

@blacksmith-sh

blacksmith-sh Bot commented Sep 11, 2026

Copy link
Copy Markdown

Found 2 test failures on Blacksmith runners:

Failures

Test View Logs
[chromium] › e2e/local-auth/
malicious-skill-ban-flow.pw.test.ts:102:1 › malicious prepublication retries keep the c
lean latest visible
View Logs
e2e/local-auth/malicious-skill-ban-flow.pw.test.ts/
malicious prepublication retries keep the clean latest visible
View Logs

Fix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need.

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.

1 participant