[AIG-854] De-flake consensus expiry test with injectable clock/fake timers + CI repeat guard - #52
Conversation
…t guard The expireCheckpoints tests depended on the real wall-clock: expiry is computed from Date.now() in the SQLite store and new Date() in the service, so a slow CI runner could let the timeout elapse between create and approve, flipping an approved checkpoint to 'expired' (observed once in the coverage job). - Freeze time with vi.useFakeTimers()/setSystemTime() around both expireCheckpoints tests and advance the clock explicitly to cross the expiry boundary, making the create -> approve -> expire sequence fully deterministic. No business logic changed. - Add a dedicated 'consensus-flake-guard' CI job that runs only the consensus suite 50x as a permanent guard against future flakiness, without slowing the full test suite (vitest 2.x has no --repeat CLI flag, so the run is looped at the shell level).
📝 WalkthroughWalkthroughThis PR improves consensus service test reliability by introducing deterministic time handling with Vitest fake timers and adds a CI job that runs those tests 50 times to detect intermittent failures. The test refactor centralizes timeout configuration and replaces time-based mocking with controlled time advancement, while the new CI job provides early signal of flaky test behavior. ChangesConsensus Service Test Reliability
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
tests/unit/consensus-service.test.tsParsing error: "parserOptions.project" has been provided for 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 @.github/workflows/ci.yml:
- Around line 72-98: Update the "consensus-flake-guard" job (and apply
file-wide) to pin third-party actions, restrict permissions, and disable
credential persistence: replace uses: actions/checkout@v4 and uses:
actions/setup-node@v4 with commit-pinned references (use the specific commit SHA
for actions/checkout and actions/setup-node), add an explicit permissions: block
granting least privilege (e.g., read-only for contents or only the permissions
actually needed by the job) at the job level, and set persist-credentials: false
on the checkout step; ensure these changes are applied similarly wherever
actions/checkout and actions/setup-node are used across the workflow.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6c37e09b-1fc0-47fc-8a41-39bfaf5da8b5
📒 Files selected for processing (2)
.github/workflows/ci.ymltests/unit/consensus-service.test.ts
| consensus-flake-guard: | ||
| name: Consensus Flake Guard | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - name: Checkout | ||
| uses: actions/checkout@v4 | ||
|
|
||
| - name: Setup Node.js | ||
| uses: actions/setup-node@v4 | ||
| with: | ||
| node-version: 22 | ||
| cache: 'npm' | ||
|
|
||
| - name: Install dependencies | ||
| run: npm ci | ||
|
|
||
| # vitest 2.x exposes `repeats` only as a per-test option, not a CLI flag, | ||
| # so we loop the run 50x at the shell level. The whole loop fails fast if | ||
| # any single run fails (set -e + explicit exit on non-zero). | ||
| - name: Repeat consensus tests (50x) | ||
| run: | | ||
| set -euo pipefail | ||
| for i in $(seq 1 50); do | ||
| echo "::group::consensus run $i/50" | ||
| npx vitest run tests/unit/consensus-service.test.ts | ||
| echo "::endgroup::" | ||
| done |
There was a problem hiding this comment.
Security: Pin action versions, restrict permissions, disable credential persistence.
The static analysis tool (zizmor) flagged three security concerns with this job that also apply to all other jobs in this workflow:
- Unpinned actions (flagged as error):
actions/checkout@v4andactions/setup-node@v4use semantic version tags instead of commit hashes, creating a supply chain attack vector if the tags are moved. - Overly broad permissions (flagged as warning): No
permissions:block means the job inherits default workflow permissions (read/write), violating least privilege. - Credential persistence (flagged as warning): The checkout step doesn't set
persist-credentials: false, allowing subsequent steps to use the GitHub token.
These are pre-existing patterns across all jobs in this file—the new job correctly follows the existing structure. Consider addressing these file-wide in a follow-up PR.
🔒 Recommended fixes (apply to all jobs)
1. Pin actions to commit hashes:
- - name: Checkout
- uses: actions/checkout@v4
+ - name: Checkout
+ uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2
- - name: Setup Node.js
- uses: actions/setup-node@v4
+ - name: Setup Node.js
+ uses: actions/setup-node@39370e3970a6d050c480ffad4ff0ed4d3fdee5af # v4.1.02. Add permissions block to each job:
consensus-flake-guard:
name: Consensus Flake Guard
runs-on: ubuntu-latest
+ permissions:
+ contents: read
steps:3. Disable credential persistence:
- name: Checkout
- uses: actions/checkout@v4
+ uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2
+ with:
+ persist-credentials: false📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| consensus-flake-guard: | |
| name: Consensus Flake Guard | |
| runs-on: ubuntu-latest | |
| steps: | |
| - name: Checkout | |
| uses: actions/checkout@v4 | |
| - name: Setup Node.js | |
| uses: actions/setup-node@v4 | |
| with: | |
| node-version: 22 | |
| cache: 'npm' | |
| - name: Install dependencies | |
| run: npm ci | |
| # vitest 2.x exposes `repeats` only as a per-test option, not a CLI flag, | |
| # so we loop the run 50x at the shell level. The whole loop fails fast if | |
| # any single run fails (set -e + explicit exit on non-zero). | |
| - name: Repeat consensus tests (50x) | |
| run: | | |
| set -euo pipefail | |
| for i in $(seq 1 50); do | |
| echo "::group::consensus run $i/50" | |
| npx vitest run tests/unit/consensus-service.test.ts | |
| echo "::endgroup::" | |
| done | |
| consensus-flake-guard: | |
| name: Consensus Flake Guard | |
| runs-on: ubuntu-latest | |
| permissions: | |
| contents: read | |
| steps: | |
| - name: Checkout | |
| uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 | |
| with: | |
| persist-credentials: false | |
| - name: Setup Node.js | |
| uses: actions/setup-node@39370e3970a6d050c480ffad4ff0ed4d3fdee5af # v4.1.0 | |
| with: | |
| node-version: 22 | |
| cache: 'npm' | |
| - name: Install dependencies | |
| run: npm ci | |
| # vitest 2.x exposes `repeats` only as a per-test option, not a CLI flag, | |
| # so we loop the run 50x at the shell level. The whole loop fails fast if | |
| # any single run fails (set -e + explicit exit on non-zero). | |
| - name: Repeat consensus tests (50x) | |
| run: | | |
| set -euo pipefail | |
| for i in $(seq 1 50); do | |
| echo "::group::consensus run $i/50" | |
| npx vitest run tests/unit/consensus-service.test.ts | |
| echo "::endgroup::" | |
| done |
🧰 Tools
🪛 zizmor (1.25.2)
[warning] 76-77: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[warning] 72-98: overly broad permissions (excessive-permissions): default permissions used due to no permissions: block
(excessive-permissions)
[error] 77-77: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 80-80: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 Prompt for 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.
In @.github/workflows/ci.yml around lines 72 - 98, Update the
"consensus-flake-guard" job (and apply file-wide) to pin third-party actions,
restrict permissions, and disable credential persistence: replace uses:
actions/checkout@v4 and uses: actions/setup-node@v4 with commit-pinned
references (use the specific commit SHA for actions/checkout and
actions/setup-node), add an explicit permissions: block granting least privilege
(e.g., read-only for contents or only the permissions actually needed by the
job) at the job level, and set persist-credentials: false on the checkout step;
ensure these changes are applied similarly wherever actions/checkout and
actions/setup-node are used across the workflow.
Source: Linters/SAST tools
1 participant
Cosa cambia
Fix della flakiness time-based nei test
expireCheckpointsditests/unit/consensus-service.test.ts+ nuovo guard CI.1. Determinismo del tempo (fake timers)
I due test
expireCheckpointsdipendevano dal wall-clock reale:Date.now()(createConsensusCheckpoint,expireOldCheckpoints)new Date() > checkpoint.expiresAtinsubmitDecisionSu un runner CI lento il timeout poteva trascorrere tra
createCheckpointeapproveCheckpoint, facendo finire un checkpoint approvato in statoexpired(AssertionError: expected 'expired' to be 'approved', gia osservato 1 volta nel jobcoverage).Fix: congelo il tempo con
vi.useFakeTimers()+vi.setSystemTime()attorno a entrambi i test e avanzo il clock esplicitamente per superare la soglia di scadenza. I fake timers di vitest intercettano siaDate.now()(store) sianew Date()(service), quindi l'intera sequenza create -> approve -> expire diventa deterministica. Sostituiti gliUPDATE ... SET expires_at = Date.now() - 1(fonte della corsa) con avanzamenti di tempo controllati.2. Guard CI permanente (repeat 50x)
Nuovo job
consensus-flake-guardin.github/workflows/ci.ymlche esegue solo il file consensus, ripetuto 50 volte, come guard contro future regressioni di flakiness. Veloce: gira solotests/unit/consensus-service.test.ts, non l'intera suite, e non rallenta gli altri job.Scope file
tests/unit/consensus-service.test.ts— fake timers sui testexpireCheckpoints.github/workflows/ci.yml— nuovo jobconsensus-flake-guard(repeat 50x, solo consensus)Nessuna modifica alla logica di business.
Summary by CodeRabbit
Tests
Chores