ci: disable stale cron workflows and remove their matching controllers#52
Open
colinwilliams91 wants to merge 1 commit into
Open
Conversation
devflowinc#50) Two scheduled cron workflows (check-queues every 5 min, clean-before-24h-complete-jobs every 30 min) have been failing on every run since BULL_AUTH_KEY is not configured as a repo secret AND the curl target api.firecrawl.dev is upstream Mendable's SaaS, not this fork's deployment -- the workflows were copied verbatim when the fork diverged and never adapted. See issue devflowinc#50 for run-log receipts. Even if these were re-pointed at a fork-owned deployment with the secret set, the operational benefit would be nil: check-queues calls checkAlerts() which is a no-op by default (guarded by ENV=production AND ALERT_NUM_ACTIVE_JOBS AND ALERT_NUM_WAITING_JOBS -- none plumbed in .env.example, compose, or docs; Slack output was stripped in b024f08); clean-before-24h-complete-jobs is largely redundant with BullMQ's removeOnComplete.age=90000 (25h) auto-eviction. Upstream firecrawl/firecrawl has fully removed both workflows AND the matching controllers/v0/admin/queue.ts and services/alerts.ts. Changes: - Move .github/workflows/check-queues.yml and clean-before-24h-complete-jobs.yml into .github/archive/ with on: [] (matches the existing archive convention for the 6 SDK workflows). - Remove checkQueuesController and cleanBefore24hCompleteJobsController from controllers/v0/admin/queue.ts and their route registrations from routes/admin.ts (the only callers of those endpoints were the disabled crons). Keep queuesController (JSON health endpoint, still useful to self-hosters per SELF_HOST.md:86) and redisHealthController (upstream also kept it). - Delete the now-dead apps/api/src/services/alerts/ directory (its only consumer was checkQueuesController). Unchanged: BULL_AUTH_KEY stays in apps/api/.env.example and docker-compose.yaml for self-hosters to protect their own admin endpoints to their own deployments. README.md, CONTRIBUTING.md, and SELF_HOST.md untouched -- the stale SELF_HOST.md:86 'Bull Queue Manager UI' reference is tracked separately in issue devflowinc#51. A latent admin-guard bug in admin.ts (routes mount unconditionally at /admin/undefined/... when BULL_AUTH_KEY unset; upstream gates behind if (config.BULL_AUTH_KEY)) is flagged in the PR body for a separate follow-up. Verified: pnpm run build (tsc) passes clean with no orphan-import errors. pnpm test runs -- 7 of 11 unit suites pass (74/89 tests); the 4 failing suites (parseTable, urlValidation, html-to-markdown, crawl) are pre-existing environmental issues (Cheerio load, native Go html-to-markdown module, services/idempotency/validate missing, test-expectation drift) unrelated to anything removed here. No test file references any of the removed symbols (grep across *.test.ts).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #50. Cross-reference: #49 (compose fix) flagged these failing crons in its "Out of scope" section -- this PR is the follow-up. Also references follow-up issue #51 (stale
SELF_HOST.md:86"Bull Queue Manager UI" doc reference).What & why
Two scheduled GitHub Actions workflows have been failing on every single run (~336 runs/day, ~10,000 failed runs/month):
Check Queues--.github/workflows/check-queues.yml, cron*/5 * * * *Clean Every 30 Minutes Before 24h Completed Jobs--.github/workflows/clean-before-24h-complete-jobs.yml, cron30 * * * *Root cause (from the run logs)
From run
29581536093:BULL_AUTH_KEYis not configured as a repo secret, so${{ secrets.BULL_AUTH_KEY }}resolves to empty, the URL becomesadmin//check-queues, and the request returns HTTP 404.api.firecrawl.devis upstream firecrawl's (Mendable's) own SaaS, not this fork's deployment. This repo is a fork offirecrawl/firecrawl; thefirecrawlorg ownsapi.firecrawl.dev. These workflows were copied verbatim when the fork diverged and were never adapted to a fork-owned endpoint (and no fork-owned prod endpoint is documented in this repo).Why disabling (and removing the matching controllers) is safe
Verified against the code. Even if these were re-pointed at a fork-owned deployment with the secret set, the operational benefit would be nil:
check-queuescallscheckAlerts()(apps/api/src/services/alerts/index.ts), which is a no-op by default. Guarded byprocess.env.ENV === "production"ANDprocess.env.ALERT_NUM_ACTIVE_JOBSANDprocess.env.ALERT_NUM_WAITING_JOBS-- none of which appear in.env.example,docker-compose.yaml,README.md,SELF_HOST.md, orCONTRIBUTING.md. When unset, the controller just logs "Initializing alerts" and returns 200. Slack output was previously stripped (commitb024f08 cleanup: remove SLACK_WEBHOOK_URL env and code); zeroSLACK_*env reads remain in the repo. The only consumer ofcheckAlerts()wascheckQueuesController. So a correctly-pointedcheck-queuescron would emit only localLogger.info/warnlines that nobody reads.clean-before-24h-complete-jobsis largely redundant. BullMQ's queue is created withremoveOnComplete: { age: 90000 }(25h) andremoveOnFail: { age: 90000 }(apps/api/src/services/queue-service.ts:18-23), so completed/failed jobs auto-evict after ~25h anyway. The controller also caps at 9 batches x 10 = ~90 jobs per call (apps/api/src/controllers/v0/admin/queue.ts:16-27), inadequate for any real backlog..github/workflows/check-queues.yml,.github/workflows/clean-before-24h-complete-jobs.yml,apps/api/src/controllers/v0/admin/queue.ts, andapps/api/src/services/alerts.tsare all GONE fromfirecrawl/firecrawl. This fork is carrying cold ash from before upstream's queue/admin refactor.Cost note (correction)
The repo is public (
visibility: public), and GitHub Actions minutes are free and unlimited for public repos -- so these failing crons are not directly costing money today. The real harms are:build-docker-images.yml.GITHUB_TOKENwithContents: write,PullRequests: write, etc. -- small but unnecessary exposure surface on every run.Changes
.github/workflows/check-queues.yml->.github/archive/check-queues.yml,on: []..github/workflows/clean-before-24h-complete-jobs.yml->.github/archive/clean-before-24h-complete-jobs.yml,on: [].checkQueuesControllerandcleanBefore24hCompleteJobsControllerfromapps/api/src/controllers/v0/admin/queue.ts(the only callers of those endpoints were the disabled crons); keepqueuesController./admin/<KEY>/check-queuesand/admin/<KEY>/clean-before-24h-complete-jobsroute registrations fromapps/api/src/routes/admin.ts; keepredis-healthandqueues(both still useful to self-hosters, and upstream also keptredis-health).apps/api/src/services/alerts/directory (its only consumer wascheckQueuesController).Why move +
on: []rather than juston: []in place: matches the established repo precedent. The 6 SDK workflows already in.github/archive/all useon: []. Files outside.github/workflows/are not eligible to be triggered by GitHub; theon: []is defense-in-depth for the case where someone moves them back and forgets to set the trigger.Why also remove the matching controllers (not just the crons): the only HTTP callers of
checkQueuesControllerandcleanBefore24hCompleteJobsControllerwere the crons being disabled.checkQueuesControllerwas a no-op by default;cleanBefore24hCompleteJobsControllerduplicated BullMQ's built-in 25h auto-eviction at far smaller scale. Both were also fully removed upstream. Mirroring upstream's evolution (Option B) keeps this fork from carrying dead code whose only client is itself dead code.What is NOT changed
apps/api/src/controllers/v0/admin/redis-health.ts-- kept (upstream also kept it).queuesController(the/admin/<KEY>/queuesJSON health endpoint) -- kept; still usable by self-hosters. Note:SELF_HOST.md:86describes this as a "Bull Queue Manager UI," which is stale (bull-board was removed in commitsc344bc0/003fb21). That stale doc line is tracked separately in issue SELF_HOST.md references a 'Bull Queue Manager UI' that was removed with bull-board #51 -- not addressed here.BULL_AUTH_KEYinapps/api/.env.example,docker-compose.yaml, README, SELF_HOST.md, CONTRIBUTING.md -- kept. Self-hosters actively use it to protect their own admin endpoints to their own deployments. Disabling the crons does not affect self-hoster admin access.apps/api/src/services/queue-service.ts,queue-worker.ts,queue-jobs.ts,rate-limiter.ts-- untouched.README.md,CONTRIBUTING.md,SELF_HOST.md-- untouched, to avoid creating stale docs. Issue SELF_HOST.md references a 'Bull Queue Manager UI' that was removed with bull-board #51 tracks theSELF_HOST.md:86cleanup separately.Latent bug (separate follow-up, not addressed here)
apps/api/src/routes/admin.ts:7-12mounts the admin routes unconditionally. IfBULL_AUTH_KEYis unset, routes register at literal/admin/undefined/...paths -- trivially guessable. Upstream gates the whole block behindif (config.BULL_AUTH_KEY). This fork has not backported that guard. Worth a separate issue; flagged here only.Verification
pnpm run build(tsc) inapps/api/-- passes clean with no orphan-import errors.pnpm testinapps/api/-- 7 of 11 unit suites pass (74/89 tests). The 4 failing suites (parseTable,urlValidation,html-to-markdown,crawl) are pre-existing environmental issues unrelated to anything removed here:parseTable.test.ts--TypeError: Cannot read properties of undefined (reading 'load')(Cheerio load issue, library/environment-related).urlValidation.test.ts-- expects "social media scraping" throw but gets a Zod error string instead (test-expectation drift on URL validation logic).html-to-markdown.test.ts-- markdown equality mismatch (needs the Go native module that's not built in this scratch install).crawl.test.ts--Cannot find module '../../services/idempotency/validate' from 'src/controllers/__tests__/crawl.test.ts'(a pre-existing missing module I did not touch).*.test.tsforadmin | redis-health | /queues | alerts | checkQueues | cleanBefore24h | services/alerts-- zero matches. No test file references any of the removed symbols.Re-enable path (for maintainers)
If a
devflowinc/firecrawl-simpledeployment is ever stood up at a known URL with aBULL_AUTH_KEY:git mvboth files back to.github/workflows/and restoreon: schedule: [...].BULL_AUTH_KEYas a repo secret.https://api.firecrawl.dev/...URL in eachcurlto point at the actual deployment.checkQueuesControllerandcleanBefore24hCompleteJobsControllerfrom git history (git checkout HEAD~ -- apps/api/src/controllers/v0/admin/queue.ts apps/api/src/services/alerts/index.ts) and re-add the two route registrations toadmin.ts.check-queuesis wanted as a real alert, also plumbENV=production,ALERT_NUM_ACTIVE_JOBS,ALERT_NUM_WAITING_JOBSinto.env.exampleand compose, and wire up an output sink (Slack or a webhook) since Slack was stripped.Closes #50.