perf: delete expired Firestore states concurrently under a bound - #1170
Conversation
cleanup_old_states() deleted every expired document in a sequential await loop, despite a "# Delete in batch" comment claiming otherwise. Cleanup therefore cost N network round-trips and scaled linearly with the size of the expired backlog. Deletes are now fanned out with asyncio.gather() under a semaphore bounded by CLEANUP_DELETE_CONCURRENCY (16), so a large backlog cannot flood Firestore with unbounded in-flight RPCs. return_exceptions=True keeps a single failing delete from abandoning deletes already in flight, and the returned count now reflects deletes that actually succeeded rather than being lost to a propagating exception. Adds three regression tests, each verified to fail against the previous sequential implementation: - overlap test (old peak in-flight was 1) - boundedness test (peak never exceeds the configured limit) - failure-isolation test (old code propagated and skipped the rest) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedAuto reviews are limited based on label configuration. 🏷️ Required labels (at least one) (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository YAML (base), Repository UI (inherited), Organization UI (inherited) Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughSummary by CodeRabbit
Walkthrough
ChangesFirestore cleanup
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested labels: Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 4❌ Failed checks (1 warning, 3 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Warning Review ran into problems🔥 ProblemsThese MCP integrations need to be re-authenticated in the Integrations settings: Sentry Linked repositories: Public OSS repositories can only analyze public repositories installed in this organization. No linked repositories were analyzed; skipped 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 |
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Snapshot WarningsEnsure that dependencies are being submitted on PR branches and consider enabling retry-on-snapshot-warnings. See the documentation for more information and troubleshooting advice. Scanned FilesNone |
Agent Completion Truth Gate: NOT_APPLICABLEEvidence agrees. Machine-readable verdict{
"details": {},
"reasons": [],
"verdict": "not_applicable"
} |
There was a problem hiding this comment.
Pull request overview
Optimizes Firestore state cleanup by concurrently deleting expired documents while preserving partial-success reporting.
Changes:
- Adds a 16-delete concurrency limit and partial-failure logging.
- Adds regression tests for overlap, bounded RPCs, and failures.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
firestore_state.py |
Implements bounded concurrent deletion. |
test_firestore_state.py |
Tests concurrency and failure behavior. |
|
Automated remediation pass — terminal state: Correctness / red-team review — PASS. The bounded The two red checks are both non-code — nothing in this diff can flip them:
No merge performed — base Generated by Claude Code |
Review correctly identified that asyncio.gather() over a comprehension allocates one task per document *before* the semaphore can gate anything. Since the cleanup query has no limit, a large expired backlog would cost unbounded task and event-loop memory even though only 16 deletes reached Firestore at a time. Replaces the gather-plus-semaphore fan-out with a fixed pool of CLEANUP_DELETE_CONCURRENCY workers pulling from a shared iterator over the documents. Pulling with next() is safe without a lock because the event loop is single-threaded and there is no await between taking a document and using it. Worker count is min(CONCURRENCY, len(docs)), so both in-flight RPCs and allocated tasks are bounded by the same constant. Failure isolation is preserved: each worker tallies its own exceptions instead of relying on return_exceptions, so one bad delete still cannot abandon the remaining backlog. Adds test_cleanup_bounds_allocated_delete_tasks_not_just_rpcs, which measures peak len(asyncio.all_tasks()) during cleanup. Against the previous gather implementation it reports 49 concurrent tasks for 48 documents; with the worker pool it stays within the bound. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
Both review findings were correct and both are now fixed at 1.
|
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
ast-grep outline src/youtube_extension/services/cloud/firestore_state.py --items all
ast-grep outline tests/unit/test_firestore_state.py --items all --match 'cleanup|Cleanup'
rg -n -C 12 'CLEANUP_DELETE_CONCURRENCY|async def cleanup_old_states|def cleanup_old_states' \
src/youtube_extension/services/cloud/firestore_state.py
rg -n -C 10 'test_cleanup_(deletes_overlap|bounds|failure)|all_tasks|cleanup_old_states' \
tests/unit/test_firestore_state.pyLength of output: 6056 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '== Candidate Firestore state test files =='
fd -i 'firestore.*state|state.*firestore' . -t f
printf '%s\n' '== Cleanup test definitions in Python files =='
rg -n -P --type py -C 8 '^\s*(async\s+)?def\s+test_.*cleanup|asyncio\.all_tasks\s*\(' .
printf '%s\n' '== cleanup_old_states method tail =='
sed -n '312,378p' src/youtube_extension/services/cloud/firestore_state.pyLength of output: 874 🏁 Script executed: #!/bin/bash
set -u
printf '%s\n' '== Checked-out commit =='
git rev-parse HEAD
git status --short
printf '%s\n' '== PR `#1170` changed files =='
gh pr view 1170 --repo groupthinking/EventRelay --json headRefOid,baseRefOid,files \
--jq '.headRefOid, .baseRefOid, (.files[] | "\(.path)\t+\(.additions)\t-\(.deletions)")' \
|| true
printf '%s\n' '== Test files available in checkout =='
fd . -t f | rg -n '(^|/)(test|tests?)[^/]*\.py$|_test\.py$' || true
printf '%s\n' '== Complete cleanup_old_states implementation =='
sed -n '312,378p' src/youtube_extension/services/cloud/firestore_state.pyLength of output: 4403 🏁 Script executed: #!/bin/bash
set -euo pipefail
commit="db083b637feb8a2ff1f0c6cfc90e9399059278e5"
test_path="tests/unit/test_firestore_state.py"
printf '%s\n' '== Cleanup-related test definitions and context =='
git show "${commit}:${test_path}" |
rg -n -C 16 '^\s*(async\s+)?def\s+test_.*cleanup|asyncio\.all_tasks\s*\(|CLEANUP_DELETE_CONCURRENCY|cleanup_old_states\s*\('
printf '%s\n' '== PR test-file diff =='
git diff --no-ext-diff --unified=20 "${commit}^" "${commit}" -- "${test_path}"Length of output: 15882
The lock-free The worker pool bounds both resources. The per-worker exception handling preserves failure isolation. A delete that raises an The focused tests cover overlap, in-flight RPC bounds, allocated-task bounds, and continued deletion after an ordinary failure. The task-allocation test would detect the prior ✏️ Learnings added
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/youtube_extension/services/cloud/firestore_state.py`:
- Around line 358-363: Strengthen
test_cleanup_failure_does_not_abandon_remaining_deletes by creating more
documents than CLEANUP_DELETE_CONCURRENCY, or overriding that constant to a
small value, so a worker must continue consuming pending documents after its
delete fails. Arrange a failure and assert that all subsequent documents are
still attempted and deleted, preserving the existing failure-isolation
assertions.
- Around line 30-34: Update the comment above CLEANUP_DELETE_CONCURRENCY to
describe the fixed worker pool and shared-iterator approach used by
cleanup_old_states(), not a semaphore. Replace the hardcoded value with an
os.getenv()-based configuration lookup, preserving a sensible default of 16 and
converting the environment value to an integer for worker-count usage.
- Around line 345-357: Update the _delete_worker function’s
doc.reference.delete() call to pass an explicit timeout value, using the
surrounding cleanup configuration or established timeout constant. Preserve the
existing success counting and exception collection behavior so timed-out deletes
are tallied as failures.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Repository UI (inherited), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 6046d70a-2b0b-48a8-b027-38a1239497a2
⛔ Files ignored due to path filters (1)
tests/unit/test_firestore_state.pyis excluded by!tests/**
📒 Files selected for processing (1)
src/youtube_extension/services/cloud/firestore_state.py
📜 Review details
⏰ Context from checks skipped due to timeout. (5)
- GitHub Check: Generate and Upload Coverage
- GitHub Check: test
- GitHub Check: trivy
- GitHub Check: Security Scan - python
- GitHub Check: Security Scan - javascript
⚠️ CI failures not shown inline (17)
GitHub Actions: Secret Scan / 0_gitleaks (working tree).txt: perf: delete expired Firestore states concurrently under a bound
Conclusion: failure
##[group]Run gitleaks detect --no-git --config .gitleaks.toml --redact --verbose --exit-code 1
�[36;1mgitleaks detect --no-git --config .gitleaks.toml --redact --verbose --exit-code 1�[0m
shell: /usr/bin/bash -e {0}
##[endgroup]
○
│╲
│ ○
○ ░
░ gitleaks
Finding: ...gz", hash = "sha256:�[1;3;mREDACTED�[0m, size = 401824, upl...
***REDACTED_SECRET_ASSIGNMENT***
RuleID: square-access-token
Entropy: 3.884400
File: uv.lock
Line: 5129
Fingerprint: uv.lock:square-access-***REDACTED_SECRET_ASSIGNMENT***
�[90m9:02PM�[0m �[32mINF�[0m scan completed in 5.88s
�[90m9:02PM�[0m �[31mWRN�[0m leaks found: 1
##[error]Process completed with exit code 1.
GitHub Actions: PR Governance / Canonical issue and evidence: perf: delete expired Firestore states concurrently under a bound
Conclusion: failure
##[group]Run actions/github-script@v8
with:
script: const pr = context.payload.pull_request;
const runUrl =
`${context.serverUrl}/${context.repo.owner}/${context.repo.repo}/actions/runs/${context.runId}`;
async function publish(conclusion, title, summary) {
await github.rest.checks.create({
owner: context.repo.owner,
repo: context.repo.repo,
name: "PR Governance",
head_sha: pr.head.sha,
status: "completed",
conclusion,
details_url: runUrl,
output: {
title,
summary: summary.slice(0, 60000)
}
});
if (conclusion === "failure") {
core.setFailed(summary);
}
}
if (pr.draft) {
await publish(
"neutral",
"Governance deferred for draft PR",
`Draft PR #${pr.number} is not enforced. The Check is bound to exact head ${pr.head.sha}.`
);
return;
}
const body = pr.body || "";
function getSectionContent(text, heading) {
const escapedHeading = heading.replace(/[.*+?^${}()|[\]\\]/g, "\\$&");
const pattern = new RegExp(
escapedHeading + "\\s*\\n([\\s\\S]*?)(?=\\n## |$)",
"i"
);
const match = text.match(pattern);
if (!match) return null;
return match[1].replace(/<!--[\s\S]*?-->/g, "").trim();
}
const placeholderPatterns = [
/^Describe the user or operational result this PR produces\.?$/i,
/^List exact automated and manual checks, tied to the current head SHA\.?$/i,
/^Provide the Vercel preview, production deployment, runtime evidence, or state why production evidence is not applicable\.?$/i,
/^-\s*Risk level:\s*low\s*\/\s*medium\s*\/\s*high\s*$/i,
/^-\s*Failure mode:\s*$/i,
/^-\s*Rollback:\s*$/i,
/^-\s*\[\s\]\s*(Focused tests|Required CI|Review threads resolved)\s*$/i,
/^(Closes?|Fix(?:es|ed)?|Resolves?)\s+#\s*$/i
];
function hasMeaningfulContent(content) {
if (content === null) return false;
const meaningfulLines = content
.split(/\r?\n/)
.map(line => line.trim())
.filter(Boolean)
.filter(line => !placeholderPatterns.some(pattern => pattern.test...
GitHub Actions: Secret Scan / gitleaks (working tree): perf: delete expired Firestore states concurrently under a bound
Conclusion: failure
##[group]Run gitleaks detect --no-git --config .gitleaks.toml --redact --verbose --exit-code 1
�[36;1mgitleaks detect --no-git --config .gitleaks.toml --redact --verbose --exit-code 1�[0m
shell: /usr/bin/bash -e {0}
##[endgroup]
○
│╲
│ ○
○ ░
░ gitleaks
Finding: ...gz", hash = "sha256:�[1;3;mREDACTED�[0m, size = 401824, upl...
***REDACTED_SECRET_ASSIGNMENT***
RuleID: square-access-token
Entropy: 3.884400
File: uv.lock
Line: 5129
Fingerprint: uv.lock:square-access-***REDACTED_SECRET_ASSIGNMENT***
�[90m9:02PM�[0m �[32mINF�[0m scan completed in 5.88s
�[90m9:02PM�[0m �[31mWRN�[0m leaks found: 1
##[error]Process completed with exit code 1.
GitHub Actions: PR Governance / 0_Canonical issue and evidence.txt: perf: delete expired Firestore states concurrently under a bound
Conclusion: failure
##[group]Run actions/github-script@v8
with:
script: const pr = context.payload.pull_request;
const runUrl =
`${context.serverUrl}/${context.repo.owner}/${context.repo.repo}/actions/runs/${context.runId}`;
async function publish(conclusion, title, summary) {
await github.rest.checks.create({
owner: context.repo.owner,
repo: context.repo.repo,
name: "PR Governance",
head_sha: pr.head.sha,
status: "completed",
conclusion,
details_url: runUrl,
output: {
title,
summary: summary.slice(0, 60000)
}
});
if (conclusion === "failure") {
core.setFailed(summary);
}
}
if (pr.draft) {
await publish(
"neutral",
"Governance deferred for draft PR",
`Draft PR #${pr.number} is not enforced. The Check is bound to exact head ${pr.head.sha}.`
);
return;
}
const body = pr.body || "";
function getSectionContent(text, heading) {
const escapedHeading = heading.replace(/[.*+?^${}()|[\]\\]/g, "\\$&");
const pattern = new RegExp(
escapedHeading + "\\s*\\n([\\s\\S]*?)(?=\\n## |$)",
"i"
);
const match = text.match(pattern);
if (!match) return null;
return match[1].replace(/<!--[\s\S]*?-->/g, "").trim();
}
const placeholderPatterns = [
/^Describe the user or operational result this PR produces\.?$/i,
/^List exact automated and manual checks, tied to the current head SHA\.?$/i,
/^Provide the Vercel preview, production deployment, runtime evidence, or state why production evidence is not applicable\.?$/i,
/^-\s*Risk level:\s*low\s*\/\s*medium\s*\/\s*high\s*$/i,
/^-\s*Failure mode:\s*$/i,
/^-\s*Rollback:\s*$/i,
/^-\s*\[\s\]\s*(Focused tests|Required CI|Review threads resolved)\s*$/i,
/^(Closes?|Fix(?:es|ed)?|Resolves?)\s+#\s*$/i
];
function hasMeaningfulContent(content) {
if (content === null) return false;
const meaningfulLines = content
.split(/\r?\n/)
.map(line => line.trim())
.filter(Boolean)
.filter(line => !placeholderPatterns.some(pattern => pattern.test...
GitHub Actions: 🔍 Dependency Review / dependency-review: perf: delete expired Firestore states concurrently under a bound
Conclusion: failure
##[group]Run actions/dependency-review-action@v5
with:
fail-on-severity: moderate
allow-ghsas: GHSA-w5hq-g745-h8pq
allow-licenses: MIT, Apache-2.0, BSD-2-Clause, BSD-3-Clause, ISC, Python-2.0, BlueOak-1.0.0, MPL-2.0, CC0-1.0, 0BSD
allow-dependencies-licenses: pkg:npm/caniuse-lite, pkg:npm/@sentry/nextjs, pkg:npm/@sentry/cli, pkg:npm/@sentry/cli-darwin, pkg:npm/@sentry/cli-linux-arm, pkg:npm/@sentry/cli-linux-arm64, pkg:npm/@sentry/cli-linux-i686, pkg:npm/@sentry/cli-linux-x64, pkg:npm/@sentry/cli-win32-arm64, pkg:npm/@sentry/cli-win32-i686, pkg:npm/@sentry/cli-win32-x64, pkg:npm/@sentry/bundler-plugin-core, pkg:npm/@sentry/babel-plugin-component-annotate
comment-summary-in-pr: always
repo-***REDACTED_SECRET_ASSIGNMENT***
##[endgroup]
##[error]Dependency review is not supported on this repository. Please ensure that Dependency graph is enabled, see https://github.com/groupthinking/EventRelay/settings/security_analysis
GitHub Actions: 🔍 Dependency Review / 0_dependency-review.txt: perf: delete expired Firestore states concurrently under a bound
Conclusion: failure
##[group]Run actions/dependency-review-action@v5
with:
fail-on-severity: moderate
allow-ghsas: GHSA-w5hq-g745-h8pq
allow-licenses: MIT, Apache-2.0, BSD-2-Clause, BSD-3-Clause, ISC, Python-2.0, BlueOak-1.0.0, MPL-2.0, CC0-1.0, 0BSD
allow-dependencies-licenses: pkg:npm/caniuse-lite, pkg:npm/@sentry/nextjs, pkg:npm/@sentry/cli, pkg:npm/@sentry/cli-darwin, pkg:npm/@sentry/cli-linux-arm, pkg:npm/@sentry/cli-linux-arm64, pkg:npm/@sentry/cli-linux-i686, pkg:npm/@sentry/cli-linux-x64, pkg:npm/@sentry/cli-win32-arm64, pkg:npm/@sentry/cli-win32-i686, pkg:npm/@sentry/cli-win32-x64, pkg:npm/@sentry/bundler-plugin-core, pkg:npm/@sentry/babel-plugin-component-annotate
comment-summary-in-pr: always
repo-***REDACTED_SECRET_ASSIGNMENT***
##[endgroup]
##[error]Dependency review is not supported on this repository. Please ensure that Dependency graph is enabled, see https://github.com/groupthinking/EventRelay/settings/security_analysis
GitHub Actions: Agent completion enforcement / Agent completion enforcement: perf: delete expired Firestore states concurrently under a bound
Conclusion: failure
##[group]Run actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3
with:
script: const fs = require('fs');
const pull = await github.rest.pulls.get({
owner: context.repo.owner,
repo: context.repo.repo,
pull_number: Number(process.env.PR)
});
let verdict = {
conclusion: 'failure',
reason: 'verifier_did_not_publish',
details: {}
};
try {
verdict = JSON.parse(fs.readFileSync(
'enforcement-verdict.json', 'utf8'
));
} catch (error) {
core.warning(error.message);
}
const conclusion = verdict.conclusion === 'success'
? 'success'
: 'failure';
const summary = JSON.stringify(verdict);
await github.rest.checks.create({
owner: context.repo.owner,
repo: context.repo.repo,
name: 'Agent completion enforcement',
head_sha: pull.data.head.sha,
status: 'completed',
conclusion,
output: {
title: conclusion === 'success'
? 'Trusted evidence verified'
: 'Trusted evidence blocked',
summary: summary.slice(0, 60000)
}
});
if (conclusion !== 'success') {
core.setFailed(verdict.reason || 'trusted evidence blocked');
}
github-***REDACTED_SECRET_ASSIGNMENT***
debug: false
user-agent: actions/github-script
result-encoding: json
retries: 0
retry-exempt-status-codes: 400,401,403,404,422
env:
PR: 1170
##[endgroup]
POST /repos/groupthinking/EventRelay/check-runs - 403 with id 4803:349479:66EAA6E:695BE05:6A6E5F1E in 153ms
RequestError [HttpError]: API rate limit exceeded for installation. If you reach out to GitHub Support for help, please include the request ID 4803:349479:66EAA6E:695BE05:6A6E5F1E and timestamp UTC. For more on scraping GitHub and how it may affect your rights, please review our Terms of Service (https://docs.github.com/en/site-policy/github-terms/github-terms-of-service) - https://docs.github.com/en/rest/using-the-rest-api/getting-started-with-the-rest-api#rate-limiting
##[error]Unhandled error: HttpError: API rate limit exceeded for installation. If you reach out to GitHub Sup...
GitHub Actions: Agent completion enforcement / 0_Agent completion enforcement.txt: perf: delete expired Firestore states concurrently under a bound
Conclusion: failure
##[group]Run actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3
with:
script: const fs = require('fs');
const pull = await github.rest.pulls.get({
owner: context.repo.owner,
repo: context.repo.repo,
pull_number: Number(process.env.PR)
});
let verdict = {
conclusion: 'failure',
reason: 'verifier_did_not_publish',
details: {}
};
try {
verdict = JSON.parse(fs.readFileSync(
'enforcement-verdict.json', 'utf8'
));
} catch (error) {
core.warning(error.message);
}
const conclusion = verdict.conclusion === 'success'
? 'success'
: 'failure';
const summary = JSON.stringify(verdict);
await github.rest.checks.create({
owner: context.repo.owner,
repo: context.repo.repo,
name: 'Agent completion enforcement',
head_sha: pull.data.head.sha,
status: 'completed',
conclusion,
output: {
title: conclusion === 'success'
? 'Trusted evidence verified'
: 'Trusted evidence blocked',
summary: summary.slice(0, 60000)
}
});
if (conclusion !== 'success') {
core.setFailed(verdict.reason || 'trusted evidence blocked');
}
github-***REDACTED_SECRET_ASSIGNMENT***
debug: false
user-agent: actions/github-script
result-encoding: json
retries: 0
retry-exempt-status-codes: 400,401,403,404,422
env:
PR: 1170
##[endgroup]
POST /repos/groupthinking/EventRelay/check-runs - 403 with id 4803:349479:66EAA6E:695BE05:6A6E5F1E in 153ms
RequestError [HttpError]: API rate limit exceeded for installation. If you reach out to GitHub Support for help, please include the request ID 4803:349479:66EAA6E:695BE05:6A6E5F1E and timestamp UTC. For more on scraping GitHub and how it may affect your rights, please review our Terms of Service (https://docs.github.com/en/site-policy/github-terms/github-terms-of-service) - https://docs.github.com/en/rest/using-the-rest-api/getting-started-with-the-rest-api#rate-limiting
##[error]Unhandled error: HttpError: API rate limit exceeded for installation. If you reach out to GitHub Sup...
GitHub Actions: PR Checks / agent-completion_truth-gate: perf: delete expired Firestore states concurrently under a bound
Conclusion: failure
##[group]Run actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3
with:
script: const owner = context.repo.owner;
const repo = context.repo.repo;
const runUrl = context.serverUrl + '/' + owner + '/' + repo +
'/actions/runs/' + context.runId;
let prNumber = Number(process.env.INPUT_PR_NUMBER || 0);
if (!prNumber && context.payload.pull_request) {
prNumber = context.payload.pull_request.number;
}
if (!prNumber) {
core.setOutput('pr_number', '');
core.setOutput('head_sha', '');
core.setOutput('base_sha', '');
return;
}
if (context.payload.pull_request &&
context.payload.pull_request.head) {
core.setOutput('pr_number', String(prNumber));
core.setOutput(
'head_sha',
context.payload.pull_request.head.sha
);
}
const pr = (await github.rest.pulls.get({
owner,
repo,
pull_number: prNumber
})).data;
core.setOutput('pr_number', String(prNumber));
core.setOutput('head_sha', pr.head.sha);
core.setOutput('base_sha', pr.base.sha);
const gateContext =
'agent-completion/truth-gate/pr-' + prNumber;
const statuses = await github.paginate(
github.rest.repos.listCommitStatusesForRef,
{owner, repo, ref: pr.head.sha, per_page: 100}
);
const gateStatuses = statuses.filter(status =>
status.context === gateContext
);
if (gateStatuses.length >= 998) {
core.setOutput('pending_status_id', '');
core.setFailed(
'status_capacity_exhausted: push a new head or complete `#874`'
);
return;
}
const pendingStatus = await github.rest.repos.createCommitStatus({
owner,
repo,
sha: pr.head.sha,
state: 'pending',
context: gateContext,
description: 'collecting repository evidence',
target_url: runUrl
});
core.setOutput(
'pending_status_id',
String(pendingStatus.data.id)
);
github-***REDACTED_SECRET_ASSIGNMENT***
debug: false
user-agent: actions/github-script
result-encoding: json
retries: 0
retry-exempt-status-codes: 400,401,403,404,422
env:
INPUT_PR_NUMBER:
##[endgroup]
GET /repos/groupthinking/Even...
GitHub Actions: PR Checks / agent-completion_truth-gate: perf: delete expired Firestore states concurrently under a bound
Conclusion: failure
##[group]Run actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3
with:
script: const fs = require('fs');
const owner = context.repo.owner;
const repo = context.repo.repo;
const marker = '<!-- agent-completion-truth-gate:v1 -->';
const runUrlPrefix = context.serverUrl + '/' + owner + '/' +
repo + '/actions/runs/';
const runUrl = runUrlPrefix + context.runId;
const gateContext = 'agent-completion/truth-gate/pr-' +
process.env.PR_NUMBER;
function gateStatusDisposition(
status,
expectedPendingId,
currentRunUrl,
targetPrefix
) {
if (!/^\d+$/.test(String(expectedPendingId || '')) ||
!status || !/^\d+$/.test(String(status.id || ''))) {
return 'fail_closed';
}
const target = String(
(status && status.target_url) || ''
);
const expectedId = BigInt(String(expectedPendingId));
const statusId = BigInt(String(status.id));
function validRunTarget(targetUrl) {
const value = String(targetUrl || '');
if (!value.startsWith(targetPrefix)) {
return false;
}
const suffix = value.slice(targetPrefix.length);
return /^\d+$/.test(suffix);
}
function statusOwnerId(candidate) {
if (candidate.state === 'pending') {
return BigInt(String(candidate.id));
}
const owner = String(candidate.description || '').match(
/^gate-owner:(\d+)(?:\s|$)/
);
return owner ? BigInt(owner[1]) : null;
}
if (!validRunTarget(currentRunUrl) ||
!validRunTarget(target)) {
return 'fail_closed';
}
const ownerId = statusOwnerId(status);
if (ownerId === null) {
return 'fail_closed';
}
if (ownerId === expectedId && target === currentRunUrl) {
if (statusId === expectedId &&
status.state === 'pending') {
return 'current_pending';
}
if (['failure', 'error'].includes(status.state)) {
return 'already_failed';
}
if (status.state === 'success') {
return 'already_succeeded';
}
return 'fail_closed';
}
if (target === currentRunUrl) {...
GitHub Actions: PR Checks / agent-completion_truth-gate: perf: delete expired Firestore states concurrently under a bound
Conclusion: failure
##[group]Run actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a
with:
name: agent-completion-verdict-1170
path: gate-input.json
gate-verdict.json
if-no-files-found: error
compression-level: 6
overwrite: false
include-hidden-files: false
archive: true
##[endgroup]
Multiple search paths detected. Calculating the least common ancestor of all paths
The least common ancestor is /home/runner/work/EventRelay/EventRelay. This will be the root directory of the artifact
##[error]No files were found with the provided path: gate-input.json
GitHub Actions: PR Checks / agent-completion_truth-gate: perf: delete expired Firestore states concurrently under a bound
Conclusion: failure
##[group]Run actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3
with:
script: const owner = context.repo.owner;
const repo = context.repo.repo;
const runUrlPrefix = context.serverUrl + '/' + owner + '/' +
repo + '/actions/runs/';
const runUrl = runUrlPrefix + context.runId;
const gateContext = 'agent-completion/truth-gate/pr-' +
process.env.PR_NUMBER;
function gateStatusDisposition(
status,
expectedPendingId,
currentRunUrl,
targetPrefix
) {
if (!/^\d+$/.test(String(expectedPendingId || '')) ||
!status || !/^\d+$/.test(String(status.id || ''))) {
return 'fail_closed';
}
const target = String(
(status && status.target_url) || ''
);
const expectedId = BigInt(String(expectedPendingId));
const statusId = BigInt(String(status.id));
function validRunTarget(targetUrl) {
const value = String(targetUrl || '');
if (!value.startsWith(targetPrefix)) {
return false;
}
const suffix = value.slice(targetPrefix.length);
return /^\d+$/.test(suffix);
}
function statusOwnerId(candidate) {
if (candidate.state === 'pending') {
return BigInt(String(candidate.id));
}
const owner = String(candidate.description || '').match(
/^gate-owner:(\d+)(?:\s|$)/
);
return owner ? BigInt(owner[1]) : null;
}
if (!validRunTarget(currentRunUrl) ||
!validRunTarget(target)) {
return 'fail_closed';
}
const ownerId = statusOwnerId(status);
if (ownerId === null) {
return 'fail_closed';
}
if (ownerId === expectedId && target === currentRunUrl) {
if (statusId === expectedId &&
status.state === 'pending') {
return 'current_pending';
}
if (['failure', 'error'].includes(status.state)) {
return 'already_failed';
}
if (status.state === 'success') {
return 'already_succeeded';
}
return 'fail_closed';
}
if (target === currentRunUrl) {
return 'fail_closed';
}
if (ownerId > expectedId) {
return 'successor';...
GitHub Actions: PR Checks / validate: perf: delete expired Firestore states concurrently under a bound
Conclusion: failure
##[group]Run actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3
with:
script: const pr = context.payload.pull_request;
const findings = [];
if (pr.title.length < 10) {
findings.push('❌ PR title too short (minimum 10 characters)');
}
if (!/^(?:⚡\s*)?(feat|fix|docs|style|refactor|test|chore|perf|ci|build|revert)(\(.+\))?:/i.test(pr.title)) {
findings.push('⚠️ PR title should follow conventional commits format');
}
if (!pr.body || pr.body.length < 20) {
findings.push('❌ PR description is required (minimum 20 characters)');
}
const totalChanges = (pr.additions || 0) + (pr.deletions || 0);
if (totalChanges > 500) {
findings.push('⚠️ Large PR detected (' + totalChanges + ' lines changed)');
}
const marker = '<!-- pr-validation:v1 -->';
const comments = await github.paginate(
github.rest.issues.listComments,
{owner: context.repo.owner, repo: context.repo.repo, issue_number: pr.number, per_page: 100}
);
const existing = comments.find(comment =>
comment.user &&
comment.user.login === 'github-actions[bot]' &&
comment.body && comment.body.includes(marker)
);
if (findings.length === 0) {
if (existing) {
await github.rest.issues.updateComment({
owner: context.repo.owner,
repo: context.repo.repo,
comment_id: existing.id,
body: marker + '\n## 🔍 PR Validation\n\n' +
'✅ Current validation passed.'
});
}
return;
}
const body = marker + '\n## 🔍 PR Validation\n\n' + findings.join('\n');
if (existing) {
await github.rest.issues.updateComment({
owner: context.repo.owner,
repo: context.repo.repo,
comment_id: existing.id,
body
});
} else {
await github.rest.issues.createComment({
owner: context.repo.owner,
repo: context.repo.repo,
issue_number: pr.number,
body
});
}
if (findings.some(finding => finding.startsWith('❌'))) {
core.setFailed('PR validation failed');
}
github-***REDACTED_SECRET_ASSIGNMENT***
debug: false
user-agent: actions/github-script
resu...
GitHub Actions: PR Checks / agent-completion_truth-gate: perf: delete expired Firestore states concurrently under a bound
Conclusion: failure
##[group]Run exit 1
�[36;1mexit 1�[0m
shell: /usr/bin/bash -e {0}
##[endgroup]
##[error]Process completed with exit code 1.
GitHub Actions: PR Checks / 2_validate.txt: perf: delete expired Firestore states concurrently under a bound
Conclusion: failure
##[group]Run actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3
with:
script: const pr = context.payload.pull_request;
const findings = [];
if (pr.title.length < 10) {
findings.push('❌ PR title too short (minimum 10 characters)');
}
if (!/^(?:⚡\s*)?(feat|fix|docs|style|refactor|test|chore|perf|ci|build|revert)(\(.+\))?:/i.test(pr.title)) {
findings.push('⚠️ PR title should follow conventional commits format');
}
if (!pr.body || pr.body.length < 20) {
findings.push('❌ PR description is required (minimum 20 characters)');
}
const totalChanges = (pr.additions || 0) + (pr.deletions || 0);
if (totalChanges > 500) {
findings.push('⚠️ Large PR detected (' + totalChanges + ' lines changed)');
}
const marker = '<!-- pr-validation:v1 -->';
const comments = await github.paginate(
github.rest.issues.listComments,
{owner: context.repo.owner, repo: context.repo.repo, issue_number: pr.number, per_page: 100}
);
const existing = comments.find(comment =>
comment.user &&
comment.user.login === 'github-actions[bot]' &&
comment.body && comment.body.includes(marker)
);
if (findings.length === 0) {
if (existing) {
await github.rest.issues.updateComment({
owner: context.repo.owner,
repo: context.repo.repo,
comment_id: existing.id,
body: marker + '\n## 🔍 PR Validation\n\n' +
'✅ Current validation passed.'
});
}
return;
}
const body = marker + '\n## 🔍 PR Validation\n\n' + findings.join('\n');
if (existing) {
await github.rest.issues.updateComment({
owner: context.repo.owner,
repo: context.repo.repo,
comment_id: existing.id,
body
});
} else {
await github.rest.issues.createComment({
owner: context.repo.owner,
repo: context.repo.repo,
issue_number: pr.number,
body
});
}
if (findings.some(finding => finding.startsWith('❌'))) {
core.setFailed('PR validation failed');
}
github-***REDACTED_SECRET_ASSIGNMENT***
debug: false
user-agent: actions/github-script
resu...
GitHub Actions: PR Checks / 0_agent-completion_truth-gate.txt: perf: delete expired Firestore states concurrently under a bound
Conclusion: failure
##[group]Run actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3
with:
script: const owner = context.repo.owner;
const repo = context.repo.repo;
const runUrl = context.serverUrl + '/' + owner + '/' + repo +
'/actions/runs/' + context.runId;
let prNumber = Number(process.env.INPUT_PR_NUMBER || 0);
if (!prNumber && context.payload.pull_request) {
prNumber = context.payload.pull_request.number;
}
if (!prNumber) {
core.setOutput('pr_number', '');
core.setOutput('head_sha', '');
core.setOutput('base_sha', '');
return;
}
if (context.payload.pull_request &&
context.payload.pull_request.head) {
core.setOutput('pr_number', String(prNumber));
core.setOutput(
'head_sha',
context.payload.pull_request.head.sha
);
}
const pr = (await github.rest.pulls.get({
owner,
repo,
pull_number: prNumber
})).data;
core.setOutput('pr_number', String(prNumber));
core.setOutput('head_sha', pr.head.sha);
core.setOutput('base_sha', pr.base.sha);
const gateContext =
'agent-completion/truth-gate/pr-' + prNumber;
const statuses = await github.paginate(
github.rest.repos.listCommitStatusesForRef,
{owner, repo, ref: pr.head.sha, per_page: 100}
);
const gateStatuses = statuses.filter(status =>
status.context === gateContext
);
if (gateStatuses.length >= 998) {
core.setOutput('pending_status_id', '');
core.setFailed(
'status_capacity_exhausted: push a new head or complete `#874`'
);
return;
}
const pendingStatus = await github.rest.repos.createCommitStatus({
owner,
repo,
sha: pr.head.sha,
state: 'pending',
context: gateContext,
description: 'collecting repository evidence',
target_url: runUrl
});
core.setOutput(
'pending_status_id',
String(pendingStatus.data.id)
);
github-***REDACTED_SECRET_ASSIGNMENT***
debug: false
user-agent: actions/github-script
result-encoding: json
retries: 0
retry-exempt-status-codes: 400,401,403,404,422
env:
INPUT_PR_NUMBER:
##[endgroup]
GET /repos/groupthinking/Even...
Commit Status: Vercel: Vercel
Conclusion: failure
Canceled from the Vercel Dashboard
🧰 Additional context used
📓 Path-based instructions (10)
**/*.{py,js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{py,js,jsx,ts,tsx}: Use Python 3.9+ and Node 18+ for development
Never hardcode API keys, database URLs, or secrets in code
Make minimal, surgical changes and avoid deleting working code unless fixing security issues
Files:
src/youtube_extension/services/cloud/firestore_state.py
**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.py: Always use type hints for Python functions
Use Black formatter with 88 character line length for Python code
Follow PEP 8 conventions for Python code
Use descriptive variable names and add docstrings to all public functions in Python
Group imports in Python: standard library, third-party, local
Use SQLAlchemy ORM and never write raw SQL queries
Use environment variables via os.getenv() or pydantic-settings to access configuration
Wrap database operations in try-except blocks and use context managers or FastAPI dependencies for connection cleanup
Implement comprehensive error handling with proper logging in all functions
Version APIs using /api/v1/ prefix for stability
Use JSON-RPC 2.0 protocol for all MCP communication
Follow the single-flow workflow: YouTube link → context extraction → agent dispatch → outputs
Use context managers for resource management in Python code
Implement comprehensive input validation and sanitize outputs for security
Use parameterized queries and SQLAlchemy ORM to prevent SQL injection
Define API request/response models using Pydantic for FastAPI endpoints
Store the single unified workflow as the only workflow; never introduce alternate flows or manual triggers
Use SQLAlchemy with connection pooling for database connections and manage sessions with context managers
Provide sensible defaults for non-sensitive configuration in Python settings
Maintain backward compatibility and do not break existing API endpoints
Include comprehensive logging for debugging in MCP implementations
Files:
src/youtube_extension/services/cloud/firestore_state.py
⚙️ CodeRabbit configuration file
Python backend code. Check for type hints, proper exception handling, async context manager usage, and potential blocking calls in async functions. Flag any bare except clauses or missing timeout parameters on network calls. CRITICAL: Flag any file that contains placeholder/stub implementations — especially in unified_ai_sdk. Any class or function that says "TODO: Replace with production implementation" or returns mock/fake data must be flagged as a blocking issue. Flag any code generation output that reaches users without AST validation or syntax checking.
Files:
src/youtube_extension/services/cloud/firestore_state.py
**/*.{py,js,ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Maintain >80% code coverage for new features
Files:
src/youtube_extension/services/cloud/firestore_state.py
**/*.{py,ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{py,ts,tsx}: Keep frontend and backend data models synchronized using matching Pydantic (backend) and TypeScript (frontend) interfaces
Use type-safe interfaces for backend-frontend data exchange
**/*.{py,ts,tsx}: Production code must use real behavior only: no mock delays, fake data, or simulated responses.
Maintain strict type safety: mypy strict mode for Python and TypeScript strict mode for the frontend.
Name events using the<domain>.<entity>.<action>format.
Files:
src/youtube_extension/services/cloud/firestore_state.py
**/*
📄 CodeRabbit inference engine (Custom checks)
**/*: Strictly verify that GitHub Copilot has explicitly reviewed and approved the pull request; human approvals alone must not satisfy this check.
Before allowing a merge, require thecopilot-rabbitlabel and AI-generated unit tests committed alongside the code changes; fail the check if either is missing.For Vercel-specific work, include
https://vercel.com/docs/llms-full.txtin the AI assistant context set.
Files:
src/youtube_extension/services/cloud/firestore_state.py
**/*.{py,pyw}
📄 CodeRabbit inference engine (AGENTS.md)
Write Python code to remain compatible with Linux and Windows where possible, including correct handling of
asyncioevent loops.
Files:
src/youtube_extension/services/cloud/firestore_state.py
src/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.py: Format Python code with Black using an 88-character line length.
Sort Python imports with isort using the Black profile.
Run Ruff with E, W, F, I, B, C4, and UP rules; E501 is ignored.
Use mypy strict mode; untyped function definitions are disallowed.
Target Python 3.9 or newer.
Validate backend inputs with Pydantic and sanitize subprocess arguments.
Use Anthropic SDK featuresthinking={"type": "adaptive"}andoutput_config={"effort": "..."}withanthropic>=0.105.0; do not addTypeErrorfallbacks for these parameters.Use the service container dependency-injection pattern in
backend/containers/.
Files:
src/youtube_extension/services/cloud/firestore_state.py
**/*.{py,ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Do not commit secrets; store keys and credentials in gitignored
.envfiles.
Files:
src/youtube_extension/services/cloud/firestore_state.py
**/*.{py,pyi}
📄 CodeRabbit inference engine (GEMINI.md)
**/*.{py,pyi}: Use Black formatting with an 88-character line length for Python code.
Use Ruff with rules E, W, F, I, B, C4, and UP for Python linting.
Use strict mypy type checking; do not define untyped functions.
Target Python 3.9 or newer.
Validate Python inputs with Pydantic.
Sanitize subprocess arguments before execution.
Do not add mock delays or fake data to production Python code; production runs in REAL_MODE_ONLY.
Keep secrets out of Python source code; load keys and credentials from environment variables instead.
Use absolute imports that resolve withPYTHONPATH=srcin the Python backend.
Files:
src/youtube_extension/services/cloud/firestore_state.py
**/*.{py,pyi,ts,tsx}
📄 CodeRabbit inference engine (GEMINI.md)
**/*.{py,pyi,ts,tsx}: Preserve the single workflow: YouTube link → transcript → events → agents → outputs; do not introduce alternative flows or manual triggers that bypass it.
Use event names following<domain>.<entity>.<action>, such asyoutube.video.captured.
Make surgical, precise changes and do not delete working code without justification.
Files:
src/youtube_extension/services/cloud/firestore_state.py
🔍 Remote MCP GitHub Copilot, Linear
Additional review context
- PR
#1170isgroupthinking/EventRelay, headdb083b6, based onmain; it changes onlyfirestore_state.pyand its unit tests. - The linked issue requires bounded concurrent deletes, failure isolation, successful-delete counting, and non-vacuous regression tests. Linear issue
GRV-215remains in Triage. - The final implementation creates at most 16 worker tasks, each issuing one delete at a time. Shared-iterator access occurs without an
await, and ordinary delete exceptions are recorded while workers continue. - Prior review correctly identified that the initial gather-plus-semaphore design bounded RPCs but still allocated one task per document; commit
db083b6replaced it with the worker pool. - Test coverage nuance: the failure-isolation test uses only three documents, so
min(16, 3)workers process them concurrently. It proves a later document is not abandoned, but does not specifically prove that the same worker continues processing after a failure; a backlog larger than 16 or a patched concurrency of 1 would cover that path more directly. - Pre-existing watchpoint: states are written with ISO-format string
created_at, while cleanup comparescreated_atagainst a numeric Unix timestamp. This mismatch exists onmainand is outside this PR’s stated scope. - The constant’s comment still says deletes are fanned out “under a semaphore,” although the final code uses a worker pool.
- At retrieval time, build and Python/frontend lint checks passed, while test/coverage checks were still running; several governance/security-related checks were red. The PR discussion attributes the agent-completion failure to repository trust-policy provisioning rather than code.
🔇 Additional comments (1)
src/youtube_extension/services/cloud/firestore_state.py (1)
10-10: LGTM!
|
Automated remediation pass (re-scan of head Correcting the earlier pass, which reported only 2 red checks — there are 6, and none is caused by this diff. The code is review-clean (both Copilot findings resolved, CodeRabbit's focused re-review found nothing, tests non-vacuous and passing).
What this means: four of the six red checks are the repo's own governance workflows failing on a shared GitHub App-installation API rate-limit (5000/hr exhausted), not on anything in the code. A re-run after the quota resets should clear those four. No merge performed — Generated by Claude Code |
The failure-isolation test used 3 documents against a pool of CLEANUP_DELETE_CONCURRENCY=16, so min(16, 3) = 3 workers each handled exactly one document. The `while True` continuation path was never taken, which made the test vacuous: a worker that returned on its first exception instead of continuing to drain the shared iterator would still have satisfied every assertion. Narrow the pool to a single worker against a 5-document backlog so the worker whose delete raises must keep pulling. Verified non-vacuous: with `return` added to the worker's except branch the test fails with `assert 0 == (5 - 1)`. Also correct the CLEANUP_DELETE_CONCURRENCY comment, which still described a semaphore-gated fan-out rather than the fixed worker pool that replaced it. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
Thanks — finding 1 was a real defect and I've fixed it. Findings 2 and 3 I'm
1. Test was vacuous — you were right, and it was worse than "weak"The test used 3 documents against except Exception as exc:
failures.append(exc)
return # abandons the rest of its queuewould have satisfied every assertion in that test. Fixed by narrowing the pool to a single worker over a 5-document backlog, so the with patch.object(_mod, "CLEANUP_DELETE_CONCURRENCY", 1):
count = await svc.cleanup_old_states(days=7)
assert count == doc_count - 1
for doc in docs:
doc.reference.delete.assert_awaited_once()Verified non-vacuous. With the I picked pool size 1 rather than "pool 2, docs 9" deliberately: with 2+ workers a 2. Comment correctedIt still described the semaphore-gated fan-out from the previous revision. Now 3 & 4. Deferred to #1180 — reasonsEnv-configurable constant. The equivalent constant merged in Explicit delete timeout. Your comment suggested using "the surrounding Both are captured in #1180 with acceptance criteria. Validation at
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '== Checked-out commit =='
git rev-parse HEAD
printf '%s\n' '== Source structure =='
ast-grep outline src/youtube_extension/services/cloud/firestore_state.py --items all --match 'cleanup|CLEANUP'
printf '%s\n' '== Test structure =='
ast-grep outline tests/unit/test_firestore_state.py --items all --match 'cleanup|Cleanup'
printf '%s\n' '== Cleanup implementation =='
rg -n -C 18 'CLEANUP_DELETE_CONCURRENCY|async def cleanup_old_states' \
src/youtube_extension/services/cloud/firestore_state.py
printf '%s\n' '== Relevant cleanup tests =='
rg -n -C 22 'test_cleanup_(failure_does_not_abandon_remaining_deletes|bounds_allocated_delete_tasks_not_just_rpcs|deletes_overlap|bounds)|patch\.object\(_mod, "CLEANUP_DELETE_CONCURRENCY"' \
tests/unit/test_firestore_state.py
printf '%s\n' '== Diff for commit 7558f03 when available =='
git show --no-ext-diff --format=fuller --stat 7558f03
git diff --no-ext-diff --unified=30 7558f03^ 7558f03 -- \
src/youtube_extension/services/cloud/firestore_state.py \
tests/unit/test_firestore_state.pyLength of output: 6719 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '== Candidate Firestore state test files =='
fd -i 'test_firestore_state.py' . -t f
fd -i '*firestore*state*.py' . -t f
printf '%s\n' '== Cleanup test definitions and patch sites =='
rg -n -P --type py -C 24 \
'^\s*async\s+def\s+test_cleanup|patch\.object\(\s*_mod\s*,\s*"CLEANUP_DELETE_CONCURRENCY"|cleanup_old_states\s*\(' \
.
printf '%s\n' '== Commit diff for all Python test files =='
git diff --no-ext-diff --unified=40 7558f03^ 7558f03 -- '*.py'Length of output: 1153 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '== Candidate test files =='
fd -i 'firestore' . -t f | rg -n '(^|/)(test|tests?)[^/]*\.py$|_test\.py$'
fd -i 'state' . -t f | rg -n '(^|/)(test|tests?)[^/]*\.py$|_test\.py$'
printf '%s\n' '== Cleanup test definitions and patch sites =='
rg -n -P --type py -C 28 \
'^\s*async\s+def\s+test_.*cleanup|patch\.object\(\s*_mod\s*,\s*"CLEANUP_DELETE_CONCURRENCY"|cleanup_old_states\s*\(' \
.
printf '%s\n' '== Commit diff for Python files =='
git diff --no-ext-diff --unified=40 7558f03^ 7558f03 -- '*.py'Length of output: 885 🏁 Script executed: #!/bin/bash
set -u
printf '%s\n' '== Python test paths in HEAD =='
git ls-tree -r --name-only HEAD | rg -n '(^|/)(test[^/]*|[^/]*_test)\.py$' || true
printf '%s\n' '== Paths changed by 7558f03 =='
git diff-tree --no-commit-id --name-status -r 7558f03
printf '%s\n' '== All paths containing firestore or cleanup =='
git ls-tree -r --name-only HEAD | rg -in 'firestore|cleanup' || true
printf '%s\n' '== Cleanup-related test source from Git objects =='
while IFS= read -r path; do
printf '\n--- %s ---\n' "$path"
git show "HEAD:$path" | rg -n -C 30 \
'test_cleanup_failure_does_not_abandon_remaining_deletes|CLEANUP_DELETE_CONCURRENCY|cleanup_old_states' || true
done < <(git ls-tree -r --name-only HEAD | rg '(^|/)(test[^/]*|[^/]*_test)\.py$')Length of output: 14168 🏁 Script executed: #!/bin/bash
set -euo pipefail
test_path='tests/unit/test_firestore_state.py'
printf '%s\n' '== Cleanup test blocks at HEAD =='
git show "HEAD:${test_path}" | rg -n -C 38 \
'test_cleanup_failure_does_not_abandon_remaining_deletes|test_cleanup_bounds_allocated_delete_tasks_not_just_rpcs|test_cleanup_deletes_overlap|patch\.object\(_mod, "CLEANUP_DELETE_CONCURRENCY"|cleanup_old_states\s*\('
printf '%s\n' '== Exact test change in 7558f03 =='
git diff --no-ext-diff --unified=60 7558f03^ 7558f03 -- "${test_path}"
printf '%s\n' '== Import and fixture context =='
git show "HEAD:${test_path}" | sed -n '1,140p'Length of output: 24458
The single-worker test fully covers the worker-continuation path. The first delete fails, and the same worker must continue through four remaining documents. A worker that returns from A separate failure test with two or more workers is not required for this guarantee. The worker count does not change the exception handler or loop continuation. The existing tests already cover multi-worker overlap, in-flight RPC bounds, and task-allocation bounds.
No remaining code-level finding. 🐇 ✅ [resolve] ✏️ Learnings added
If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! You are interacting with an AI system. |
|
@coderabbitai review Exact-head independent review requested for |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
expected='7558f0341f8da99452c7ca969b67635358db4662'
actual="$(git rev-parse HEAD)"
printf '%s\n' '== Exact head verification =='
printf 'expected: %s\nactual: %s\n' "$expected" "$actual"
if [ "$actual" != "$expected" ]; then
printf '%s\n' 'ERROR: checkout does not match the requested commit' >&2
exit 1
fi
printf '%s\n' '== Pull request files at exact head =='
git diff-tree --no-commit-id --name-status -r "$actual"
git diff --no-ext-diff --check "${actual}^" "$actual"
printf '%s\n' '== Relevant source and test structure =='
ast-grep outline src/youtube_extension/services/cloud/firestore_state.py --items all --match 'CLEANUP|cleanup'
ast-grep outline tests/unit/test_firestore_state.py --items all --match 'Cleanup|cleanup'
printf '%s\n' '== Exact implementation =='
rg -n -C 28 --type py \
'CLEANUP_DELETE_CONCURRENCY|async def cleanup_old_states|def cleanup_old_states' \
src/youtube_extension/services/cloud/firestore_state.py
printf '%s\n' '== Exact cleanup tests =='
rg -n -C 35 --type py \
'async def test_cleanup|CLEANUP_DELETE_CONCURRENCY|asyncio\.all_tasks|cleanup_old_states\s*\(' \
tests/unit/test_firestore_state.py
printf '%s\n' '== Exact commit diff =='
git diff --no-ext-diff --unified=60 "${actual}^" "$actual" -- \
src/youtube_extension/services/cloud/firestore_state.py \
tests/unit/test_firestore_state.pyLength of output: 8595 🏁 Script executed: #!/bin/bash
set -euo pipefail
commit='7558f0341f8da99452c7ca969b67635358db4662'
printf '%s\n' '== Changed paths at requested commit =='
git diff-tree --no-commit-id --name-status -r "$commit"
printf '%s\n' '== Tracked Firestore cleanup tests =='
git ls-tree -r --name-only "$commit" |
rg -i '(^|/)(test[^/]*firestore[^/]*\.py|[^/]*firestore[^/]*test\.py|test[^/]*state[^/]*\.py|[^/]*state[^/]*test\.py)$'
printf '%s\n' '== Locate all cleanup test definitions in tracked Python tests =='
while IFS= read -r path; do
git show "${commit}:${path}" |
rg -q 'cleanup_old_states|test_cleanup' || continue
printf '\n--- %s ---\n' "$path"
git show "${commit}:${path}" |
rg -n -C 40 \
'async def test_.*cleanup|def test_.*cleanup|CLEANUP_DELETE_CONCURRENCY|asyncio\.all_tasks|cleanup_old_states\s*\('
done < <(git ls-tree -r --name-only "$commit" | rg '(^|/)(test[^/]*|[^/]*_test)\.py$')
printf '%s\n' '== Parent-to-head diff for changed Python tests =='
while IFS= read -r path; do
case "$path" in
*.py)
printf '\n--- %s ---\n' "$path"
git diff --no-ext-diff --unified=60 "${commit}^" "$commit" -- "$path"
;;
esac
done < <(git diff-tree --no-commit-id --name-only -r "$commit")Length of output: 1856 🏁 Script executed: #!/bin/bash
set -euo pipefail
commit='7558f0341f8da99452c7ca969b67635358db4662'
test_path='tests/unit/test_firestore_state.py'
source_path='src/youtube_extension/services/cloud/firestore_state.py'
printf '%s\n' '== Cleanup tests at requested commit =='
git show "${commit}:${test_path}" |
rg -n -C 45 \
'async def test_.*cleanup|def test_.*cleanup|CLEANUP_DELETE_CONCURRENCY|asyncio\.all_tasks|cleanup_old_states\s*\('
printf '%s\n' '== Test module imports and class context =='
git show "${commit}:${test_path}" | sed -n '1,180p'
printf '%s\n' '== Exact parent-to-head test diff =='
git diff --no-ext-diff --unified=80 "${commit}^" "$commit" -- "$test_path"
printf '%s\n' '== Full cleanup implementation from requested commit =='
git show "${commit}:${source_path}" | sed -n '314,376p'
printf '%s\n' '== Parent-to-head source diff =='
git diff --no-ext-diff --unified=80 "${commit}^" "$commit" -- "$source_path"Length of output: 38232 🏁 Script executed: #!/bin/bash
set -euo pipefail
head='7558f0341f8da99452c7ca969b67635358db4662'
base="$(git merge-base "$head" main)"
printf '%s\n' '== Pull request range =='
printf 'base: %s\nhead: %s\n' "$base" "$head"
printf '%s\n' '== All changed files from merge base =='
git diff --name-status "$base" "$head"
git diff --no-ext-diff --check "$base" "$head"
printf '%s\n' '== Full source change from merge base =='
git diff --no-ext-diff --unified=80 "$base" "$head" -- \
src/youtube_extension/services/cloud/firestore_state.py
printf '%s\n' '== Full cleanup-test change from merge base =='
git diff --no-ext-diff --unified=80 "$base" "$head" -- \
tests/unit/test_firestore_state.py
printf '%s\n' '== Python version constraints =='
fd -HI 'pyproject.toml|setup.cfg|setup.py|tox.ini|\.python-version' . -t f -d 3 \
-x sh -c 'printf "\n--- %s ---\n" "$1"; sed -n "1,220p" "$1"' sh {}Length of output: 47624
I found no code-level findings.
I verified the diff statically. The sandbox could not run the test suite. ✅ Action performedReview finished.
|
|
@coderabbitai review Re-review exact head |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
expected='57fc58d137ecb718980ff62b2de59e7aa4edc65b'
actual="$(git rev-parse HEAD)"
printf 'expected: %s\nactual: %s\n' "$expected" "$actual"
test "$actual" = "$expected"
base="$(git merge-base "$actual" main)"
previous='7558f0341f8da99452c7ca969b67635358db4662'
printf '\n== Complete PR range ==\n'
printf 'base: %s\nhead: %s\n' "$base" "$actual"
git diff --no-ext-diff --check "$base" "$actual"
git diff --name-status "$base" "$actual"
printf '\n== Change since prior reviewed head ==\n'
git diff --no-ext-diff --check "$previous" "$actual"
git diff --no-ext-diff --unified=80 "$previous" "$actual" -- \
src/youtube_extension/services/cloud/firestore_state.py \
tests/unit/test_firestore_state.py
printf '\n== Source structure ==\n'
ast-grep outline src/youtube_extension/services/cloud/firestore_state.py --items all --match 'CLEANUP|cleanup|timeout'
printf '\n== Implementation context ==\n'
rg -n -C 32 --type py \
'CLEANUP_DELETE_CONCURRENCY|CLEANUP.*TIMEOUT|async def cleanup_old_states|\.delete\s*\(' \
src/youtube_extension/services/cloud/firestore_state.py
printf '\n== Test structure and relevant tests ==\n'
ast-grep outline tests/unit/test_firestore_state.py --items all --match 'Cleanup|cleanup|timeout'
rg -n -C 38 --type py \
'async def test_.*cleanup|CLEANUP_DELETE_CONCURRENCY|CLEANUP.*TIMEOUT|timeout|cleanup_old_states\s*\(' \
tests/unit/test_firestore_state.py
printf '\n== Environment parsing patterns in repository ==\n'
rg -n -C 5 --type py \
'os\.getenv\s*\(|os\.environ\.get\s*\(|int\s*\(\s*os\.(getenv|environ)' \
src/youtube_extensionLength of output: 28971 🏁 Script executed: #!/bin/bash
set -euo pipefail
commit='57fc58d137ecb718980ff62b2de59e7aa4edc65b'
test_path='tests/unit/test_firestore_state.py'
source_path='src/youtube_extension/services/cloud/firestore_state.py'
printf '%s\n' '== Tracked paths relevant to tests and dependencies =='
git ls-tree -r --name-only "$commit" | rg \
'(^|/)(test_firestore_state\.py|pyproject\.toml|uv\.lock|requirements[^/]*\.txt|setup\.cfg|setup\.py)$'
printf '%s\n' '== Exact configuration and cleanup tests =='
git show "${commit}:${test_path}" | rg -n -C 45 \
'CLEANUP_DELETE_CONCURRENCY|CLEANUP_DELETE_TIMEOUT_SECONDS|cleanup_old_states|timeout|environ|reload|importlib'
printf '%s\n' '== Exact test-file diff from prior reviewed head =='
git diff --no-ext-diff --unified=100 \
'7558f0341f8da99452c7ca969b67635358db4662' "$commit" -- "$test_path"
printf '%s\n' '== Firestore dependency declarations =='
for path in pyproject.toml uv.lock requirements.txt requirements-dev.txt setup.cfg setup.py; do
if git cat-file -e "${commit}:${path}" 2>/dev/null; then
printf '\n--- %s ---\n' "$path"
git show "${commit}:${path}" | rg -n -C 4 'google-cloud-firestore|firestore'
fi
done
printf '%s\n' '== Local package API declaration, if installed =='
python - <<'PY'
import inspect
try:
from google.cloud.firestore_v1.async_document import AsyncDocumentReference
print(inspect.signature(AsyncDocumentReference.delete))
except Exception as exc:
print(f'Firestore package inspection unavailable: {exc!r}')
PYLength of output: 30072
Validate that the parsed timeout is finite and positive before assigning the module constant. Add regression coverage for You are interacting with an AI system. |
CLEANUP_DELETE_TIMEOUT_SECONDS was parsed as `max(0.001, float(os.getenv(...)))`. `float()` accepts `inf`, `-inf` and `nan`, and `max(0.001, inf)` returns `inf` unchanged, so setting CLEANUP_DELETE_TIMEOUT_SECONDS=inf silently removed the per-delete deadline (or would be rejected downstream by gRPC timeout validation). `nan` was equally unsafe: it compares false against every bound, so it was silently swallowed by max() rather than reported. Replace the clamping with two explicit parsers: - `_positive_int_env` - integer >= 1 - `_positive_finite_float_env` - finite float > 0 (math.isfinite) Out-of-range values now raise at import rather than being silently coerced, so an operator typo surfaces at startup instead of quietly changing cleanup behaviour. Blank values (a common artifact of a compose/Helm template rendering an empty string) fall back to the default instead of raising. Adds TestCleanupConfigEnvParsing covering inf/Infinity/-inf/nan/0/ negative/malformed/blank/unset/valid for both parsers, plus an invariant check on the module defaults. Non-vacuity proven by mutation: restoring the `max(0.001, value)` clamping inside the helper fails all 7 non-finite/non-positive cases with "DID NOT RAISE ValueError". Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
@coderabbitai Confirmed and fixed at Verdict
You were right, and the bug was slightly wider than reported. FixReplaced the def _positive_finite_float_env(name: str, default: float) -> float:
raw = os.getenv(name)
if raw is None or not raw.strip():
return default # blank -> default, not ValueError
value = float(raw.strip()) # malformed -> fail fast at import
if not math.isfinite(value) or value <= 0:
raise ValueError(
f"{name} must be a positive, finite number of seconds, got {raw!r}"
)
return value
Deliberate choice: raise rather than clamp. Silently coercing an out-of-range Non-vacuity — proven by behavioural mutationRestoring only the old clamping inside the new helper Source restored → 90 passed. ruff at parity with Coverage added
Two questions before I merge:
If this closes it out, please @coderabbitai review |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
expected='f6c7910e4232403a121e6fd7bd5378b13640eca3'
actual="$(git rev-parse HEAD)"
printf '== Exact head ==\nexpected: %s\nactual: %s\n' "$expected" "$actual"
test "$actual" = "$expected"
previous='57fc58d137ecb718980ff62b2de59e7aa4edc65b'
base="$(git merge-base "$actual" main)"
printf '\n== Changed files: previous review to head ==\n'
git diff --no-ext-diff --check "$previous" "$actual"
git diff --name-status "$previous" "$actual"
git diff --no-ext-diff --unified=100 "$previous" "$actual" -- \
src/youtube_extension/services/cloud/firestore_state.py \
tests/unit/test_firestore_state.py
printf '\n== Complete PR changed files ==\n'
git diff --no-ext-diff --check "$base" "$actual"
git diff --name-status "$base" "$actual"
printf '\n== Source structure ==\n'
ast-grep outline src/youtube_extension/services/cloud/firestore_state.py \
--items all --match 'CLEANUP|positive|cleanup|timeout'
printf '\n== Source implementation ==\n'
rg -n -C 35 --type py \
'def _positive_|CLEANUP_DELETE_|async def cleanup_old_states|\.delete\s*\(|math\.isfinite|os\.getenv' \
src/youtube_extension/services/cloud/firestore_state.py
printf '\n== Test structure ==\n'
ast-grep outline tests/unit/test_firestore_state.py \
--items all --match 'Cleanup|cleanup|Config|config|Env|env|timeout'
printf '\n== Configuration and cleanup tests ==\n'
rg -n -C 45 --type py \
'TestCleanupConfigEnvParsing|positive_finite|positive_int|CLEANUP_DELETE_|cleanup_old_states|timeout|reload|monkeypatch|patch\.dict' \
tests/unit/test_firestore_state.py
printf '\n== All cleanup call sites ==\n'
rg -n -C 5 --type py 'cleanup_old_states\s*\(' src testsLength of output: 39633 🏁 Script executed: #!/bin/bash
set -euo pipefail
head='f6c7910e4232403a121e6fd7bd5378b13640eca3'
previous='57fc58d137ecb718980ff62b2de59e7aa4edc65b'
test_path='tests/unit/test_firestore_state.py'
printf '%s\n' '== Confirm test blob exists at exact head =='
git cat-file -e "${head}:${test_path}"
git ls-tree -r --name-only "$head" | rg -n '^tests/unit/test_firestore_state\.py$'
printf '%s\n' '== Exact parser and timeout test blocks =='
git show "${head}:${test_path}" | rg -n -C 55 \
'class TestCleanupConfigEnvParsing|test_float_env_|test_int_env_|test_module_defaults_are_positive_and_finite|test_cleanup_passes_configured_delete_timeout'
printf '%s\n' '== Exact worker-pool test blocks =='
git show "${head}:${test_path}" | rg -n -C 60 \
'def _tracking_docs|test_cleanup_deletes_overlap|test_cleanup_bounds|test_cleanup_failure_does_not_abandon_remaining_deletes|CLEANUP_DELETE_CONCURRENCY'
printf '%s\n' '== Test changes since prior reviewed revision =='
git diff --no-ext-diff --unified=100 "$previous" "$head" -- "$test_path"
printf '%s\n' '== Cleanup source change since prior reviewed revision =='
git diff --no-ext-diff --unified=100 "$previous" "$head" -- \
src/youtube_extension/services/cloud/firestore_state.pyLength of output: 48645
The Raising during import is the correct behavior. These values are startup configuration. A warning plus fallback would hide an invalid deployment configuration. It could also make cleanup behavior differ from the declared configuration. Blank and unset values already use the default, which handles templated empty values safely. The regression coverage is adequate. It tests non-finite values, malformed values, non-positive values, blank and unset fallback, valid trimmed values, and timeout forwarding. The existing tests still cover concurrent deletion, RPC bounds, task-allocation bounds, and single-worker continuation after a failure. I found no remaining code-level finding in the worker pool or the new configuration parsing. I verified the revision statically. The sandbox did not run the test suite. 🐇 ✅ [resolve] ✏️ Learnings added
You are interacting with an AI system. |
Canonical issue
Closes #1169
Outcome
FirestoreStateService.cleanup_old_states()deleted expired documents in asequential
awaitloop, so every delete waited for the previous one tocomplete — despite a
# Delete in batchcomment claiming otherwise.Deletes are now handled by a fixed pool of
CLEANUP_DELETE_CONCURRENCY = 16workers pulling from a shared iterator over the expired documents.
What this does and does not improve (corrected after review — see
this comment):
ceil(N/16)waves.delete()RPC per document.An earlier revision of this description claimed a reduction in "network
round-trips", which wrongly implied reduced request/quota usage. Only a native
batch write would do that; a bounded fan-out does not. The claim has been
reworded throughout.
Two correctness improvements come with it:
exception propagated out of the loop and every remaining document was skipped.
being lost along with the propagating exception.
Scope
cleanup_old_states(); four regression testswhereclause, caching, singleton lifecycle, the method signatureTwo files changed:
src/youtube_extension/services/cloud/firestore_state.pyandtests/unit/test_firestore_state.py(tests only).Design notes
Why a worker pool rather than
gather+ semaphore? The first revision ofthis PR used
asyncio.gather()over a generator with a semaphore inside eachcoroutine. Review correctly pointed out that
gatherallocates one task perdocument up front, before the semaphore gates anything — so a large backlog
still cost unbounded task and event-loop memory, and the cleanup query has no
limit. The worker pool bounds both in-flight RPCs and allocated tasks by thesame constant. This is measured, not assumed — see the task-allocation test below.
Pulling from the shared iterator with
next()needs no lock: the event loop issingle-threaded and there is no
awaitbetween taking a document and using it.Why not Firestore's native
AsyncWriteBatch? It would genuinely reducerequest count, which this does not. It also caps at 500 operations, so correct
use requires chunking plus a second error path for partial batch failure. Given
that cleanup is a periodic maintenance job, the latency win here is the valuable
part and the smaller diff is the better trade. Native batching is a reasonable
follow-up if cleanup volume grows enough to make quota the binding constraint.
Why a call-scoped pool rather than the per-instance/loop-keyed semaphore in
#1152? That PR needed loop-keyed laziness because
intelligent_cache.pyconstructs its singleton at import time, binding an eagerly-created
asyncio.Semaphoreto the wrong event loop. Here_firestore_serviceis createdlazily inside
async def get_firestore_service(), so there is no import-timeloop hazard, and cleanup is a periodic job rather than a hot concurrent path.
Risk
Low. One behavioural change is intentional: cleanup no longer raises when an
individual delete fails. It logs a warning naming the failure count and the first
error, and returns the number of successful deletes. For a maintenance job this
is strictly better than aborting halfway — the previous behaviour left the
collection partially cleaned and discarded the count.
Delete ordering is no longer deterministic. Deletes of distinct documents are
independent, so this has no semantic consequence.
Verification
At head
f6c7910e4232403a121e6fd7bd5378b13640eca3:Non-vacuity — every new test was run against the implementation it replaced
and confirmed to fail.
Against the original sequential loop:
test_cleanup_deletes_overlap_instead_of_running_sequentiallyAssertionError: deletes never overlapped (peak=1)test_cleanup_bounds_in_flight_deletesAttributeError: ... has no attribute 'CLEANUP_DELETE_CONCURRENCY'test_cleanup_failure_does_not_abandon_remaining_deletesRuntimeError: firestore boompropagated, stranding the remaining deleteAgainst a worker that returns instead of continuing after its own delete
fails -- the defect the first version of this test was too weak to catch, raised
in review and fixed in
7558f03:That test previously used 3 documents against a pool of 16, so
min(16, 3) = 3workers each handled exactly one document and the
while Truecontinuationbranch never ran. It now narrows the pool to a single worker over a 5-document
backlog, which is the only arrangement that isolates that path -- with 2+ workers
a surviving sibling drains the backlog and the assertion passes regardless.
Against the intermediate
gather+ semaphore revision, proving the reviewfinding was real and is now fixed:
The overlap test measures peak concurrent in-flight deletes; the old loop pins it
at exactly 1. The allocation test measures peak
len(asyncio.all_tasks()), whichis the stricter bound the review asked for.
Against the
max(0.001, float(os.getenv(...)))clamping that the configurationcommit originally shipped --
float()acceptsinf, andmax(0.001, inf)isinf, so an operator could silently remove the per-delete deadline. Restoringonly that clamping inside the new parser:
nanfailed in the opposite direction:max(0.001, nan)returns0.001because
nan > 0.001is false, so ananoverride was silently swallowedrather than reported. Both are now rejected by a single
math.isfinitecheck.Production evidence
The module is live, with 4 importers:
cleanup_old_statesis the retention path for thevideo_processing_statecollection, so its latency grows with exactly the backlog it exists to drain —
the sequential loop was slowest precisely when cleanup mattered most.
Agent handoff
@coderabbitai review