Skip to content

fix: expire every build key at write time - #1

Merged
prsimp merged 2 commits into
fix-exit-code-v0.66.0from
pe-3857-expire-build-keys-at-write-time
Sep 22, 2026
Merged

prsimp merged 2 commits into
fix-exit-code-v0.66.0from
pe-3857-expire-build-keys-at-write-time

Conversation

@prsimp

@prsimp prsimp commented Sep 10, 2026

Copy link
Copy Markdown
Member

Why

tools-redis-001, the ElastiCache node behind CI_QUEUE_URL, was pinned at 100% memory with 469,496 keys of which only 1,484 had a TTL. Under its volatile-lru policy the only evictable keys were ci-queue's live queue bookkeeping, so builds lost total / master-status mid-run and ruby spec lanes went red with no failing spec (Total tests in queue: 0, or CI::Queue::Redis::LostMaster).

The permanent keys were ci-queue's own. Measured on the node before cleanup:

Key Count Memory
processed ~3,900 4.62 GiB
worker:<n>:queue ~337,500 3.82 GiB
requeues-count ~112,300 0.99 GiB
owners / running / warnings ~11,100 0.09 GiB

That accounts for the whole 9.7 GiB.

What changed

TTLs are now set at write time. The EXPIRE for processed and worker:<id>:queue used to live at the tail of Worker#poll, inside a method whose bottom is rescue *CONNECTION_ERRORS. A worker that is SIGKILLed, cancelled, OOMs, or loses its Redis connection never reaches that line, so the key leaked forever. running, owners, requeues-count, warnings, test_failed_count and created-at never got an expiry at all.

The TTL is threaded through the Lua scripts as an argument, and through the spawned heartbeat monitor process. No key's lifetime now depends on a clean shutdown.

rspec-queue --report no longer certifies a vanished queue. An evicted queue reads as exhausted and its error-report hash reads as empty, so --report printed "No errors found" and exited 0 for a run whose results were gone. It now refuses to report a result when total is 0 or no progress was recorded, and says explicitly that this indicates eviction rather than a test failure.

Verification

Driving reserve/requeue/acknowledge by hand so the end-of-poll EXPIRE never runs — the leak path:

  • Before: 9 of 13 keys written with ttl=-1 (created-at, owners, processed, requeues-count, running, test_failed_count, warnings, worker:1:queue, worker:2:queue).
  • After: all 13 carry a TTL.

Covered by a new regression test, test_every_build_key_has_a_ttl, which fails against the previous Lua scripts naming exactly the leaked keys. Full redis_test.rb suite: 27 tests, 75 assertions, 0 failures.

For the report guard, against a real rspec-queue --report process with a completed queue:

Case Before After
healthy queue exit 0 exit 0
total evicted exit 0, "No errors found" exit 1, eviction explained
master-status evicted exit 1 exit 1

The middle row is the false green this closes.

Notes

Upstream Shopify/ci-queue has the same missing expires in reserve.lua, so this is an inherited bug rather than a fork regression. An upstream PR should follow separately.

Running the test suite locally on Ruby 3.3 also needs rexml in the gem's Gemfile; that is not included here to keep the diff scoped.

PE-3857

The EXPIRE for `processed` and `worker:<id>:queue` lived at the tail of
Worker#poll, and `running`, `owners`, `requeues-count`, `warnings`,
`test_failed_count` and `created-at` never got one at all. Any worker that is
SIGKILLed, cancelled, OOMs or loses its Redis connection never reaches the end
of poll, so those keys leaked permanently.

On Kajabi's ci-queue node that reached 464k permanent keys holding 9.5 GiB:
`processed` 4.6 GiB, `worker:<n>:queue` 3.8 GiB, `requeues-count` 1.0 GiB.
Because the node runs `volatile-lru`, the leaked keys were the only ones
ineligible for eviction, so memory pressure fell entirely on the live queue
keys of in-flight builds -- producing red spec lanes with no failing spec.

Every key is now expired at the point of write, so no key's lifetime depends on
a clean shutdown. The TTL is threaded through the Lua scripts as an argument
and through the spawned heartbeat monitor process.

Also: `rspec-queue --report` exited 0 against a queue whose keys were gone. An
evicted queue reads as exhausted and its error-report hash reads as empty, so
the report certified a run whose results it no longer had. It now refuses to
report a result when `total` is 0 or no progress was recorded, and explains
that this is eviction rather than a test failure.
This fork carries Kajabi-only patches on top of upstream 0.66.0 without
bumping the version (see b304463), and the monolith's supply-chain cooldown
check fails closed on a version it cannot find on rubygems. The Gemfile.lock
revision is the real identifier for a git-sourced gem.
@prsimp
prsimp requested review from a team and rwc9u September 11, 2026 02:12
@prsimp
prsimp merged commit 476bc19 into fix-exit-code-v0.66.0 Sep 22, 2026
12 of 25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants