fix(#517): root-cause mechanism 2 — non-deterministic key order in childContractsSignature - #535
Merged
Merged
Conversation
…ildContractsSignature Root cause: childContractsSignature built its per-task signature object by mutating a SHARED object from inside concurrent async callbacks (`sig[taskId] = perFile`) — the resulting key INSERTION ORDER followed I/O completion order, not tasks.json's own task order. Promise.all resolves its result array positionally regardless of which callback's stat() actually finishes first, but a direct assignment inside the callback body races on completion instead. sigEqual is a plain JSON.stringify comparison (key-order-sensitive), so two structurally IDENTICAL signatures could serialize to different strings purely from timing jitter. Invisible in isolation — fast, low-contention stat() calls tend to resolve in call order — but real under full-suite CPU/IO load, where resolution order shuffles. This forced an unnecessary re-parse of an unchanged run, losing `fromIndex` and failing dashboard-command-pages.test.js's "index-served lite runs keep the persisted rollup..." assertion — mechanism 2 from #517, left undiagnosed after PR #532 fixed mechanism 1 (the ENOTEMPTY teardown race). Live-reproduced the mechanism directly (not just reasoned about it): a standalone script confirmed up to 3! = 6 distinct key orderings from Promise-resolution-order variance alone. Fixed by collecting `[taskId, perFile]` tuples and rebuilding via Object.fromEntries, whose insertion order is always the ARRAY order Promise.all guarantees (tasks.json's own order), independent of resolution timing. New tests/hub-rollup-signature-order-517.test.js stresses the actual mechanism via syncRootRuns with an injected jittery `stat` (randomized 0-8ms latency per call, no sleep-based flake-chasing) — 40 repeated sync-pairs, asserting the run is served from the index every time. Mutation-tested: reverting to the shared-object-mutation pattern reproduces the failure directly (8/40 served from index instead of 40/40). Verified: 5 consecutive clean full-suite runs (1916/1916 each), typecheck/ lint/security/validate all clean. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Strix is installed on this repository, but we couldn't run this PR security review because this workspace's trial has ended. Add a card to resume code reviews here. |
2 tasks
Contributor
|
Warning Review limit reached
Next review available in: 59 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
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 |
PR Summary by QodoFix non-deterministic rollup signature ordering causing index cache misses (#517)
AI Description
Diagram
High-Level Assessment
Files changed (2)
|
Code Review by Qodo
Context used✅ Compliance rules (platform):
300 rules✅ Skills:
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+35
to
+38
| stat: async (p) => { | ||
| await new Promise((resolve) => setTimeout(resolve, Math.random() * 8)); | ||
| return realStat(p); | ||
| }, |
There was a problem hiding this comment.
1. stat(p) uses ambiguous name 📜 Skill insight ⚙ Maintainability
The new test defines stat: async (p) using a single-letter parameter name, reducing readability and violating the descriptive naming requirement. This makes the test harder to maintain and review.
Agent Prompt
## Issue description
A single-letter variable name (`p`) is used for the path argument in the new test IO shim, which is not descriptive.
## Issue Context
Compliance requires JavaScript variables to have descriptive names.
## Fix Focus Areas
- tests/hub-rollup-signature-order-517.test.js[35-38]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+42
to
+85
| function seedFixture() { | ||
| const projectRoot = mkdtempSync(join(tmpdir(), 'rstack-sig-order-517-')); | ||
| const runId = '2026-07-06T12-00-00-000Z-sig-order-fixture'; | ||
| const runDir = join(projectRoot, '.rstack', 'runs', runId); | ||
| const taskIds = ['003-architecture', '004-implementation', '005-testing']; | ||
| for (const taskId of taskIds) { | ||
| mkdirSync(join(runDir, 'tasks', taskId), { recursive: true }); | ||
| writeFileSync(join(runDir, 'tasks', taskId, 'builder.json'), '{}'); | ||
| } | ||
| writeFileSync(join(runDir, 'manifest.json'), JSON.stringify({ | ||
| run_id: runId, schema_version: 2, goal: 'sig-order fixture', status: 'IN_PROGRESS', | ||
| created_at: '2026-07-06T12:00:00.000Z', | ||
| })); | ||
| writeFileSync(join(runDir, 'tasks.json'), JSON.stringify({ | ||
| tasks: taskIds.map((id) => ({ id, status: 'PASS' })), | ||
| })); | ||
| // Far enough in the past that statusFromEntry always classifies this as | ||
| // 'stalled' (never 'active'), isolating the test to the signature-equality | ||
| // mechanism rather than the separate status-classification branch. | ||
| writeFileSync(join(runDir, 'events.jsonl'), JSON.stringify({ | ||
| ts: '2026-07-06T12:01:00.000Z', type: 'task_started', task_id: '003-architecture', | ||
| }) + '\n'); | ||
| return { projectRoot, runId }; | ||
| } | ||
|
|
||
| test('#517: an unchanged run stays index-served across repeated syncs under I/O timing jitter', async () => { | ||
| const { projectRoot, runId } = seedFixture(); | ||
| const io = jitteryIo(); | ||
| const attempts = 40; | ||
| let servedFromIndex = 0; | ||
|
|
||
| for (let i = 0; i < attempts; i++) { | ||
| await syncRootRuns(projectRoot, { io, now: Date.now() }); | ||
| const { runs } = await syncRootRuns(projectRoot, { io, now: Date.now() }); | ||
| const run = runs.find((entry) => entry.runId === runId); | ||
| assert.ok(run, `fixture run present on attempt ${i}`); | ||
| if (run.fromIndex === true) servedFromIndex++; | ||
| } | ||
|
|
||
| assert.equal( | ||
| servedFromIndex, attempts, | ||
| `an unchanged run must be served from the index every time regardless of stat() timing (${servedFromIndex}/${attempts} were)`, | ||
| ); | ||
| }); |
There was a problem hiding this comment.
5. Temp fixture not cleaned 📜 Skill insight ▣ Testability
The new regression test creates a temporary project directory and fixture files (including .rstack fixture data) but never removes them, leaving filesystem state behind across runs. This can accumulate temporary directories, cause flaky behavior, and create avoidable disk/inode pressure in CI and on developer machines.
Agent Prompt
## Issue description
The regression test allocates a temporary `projectRoot` directory with `mkdtempSync()` and writes fixture files, but it does not delete the directory after the test completes, leaking temp directories and leaving `.rstack` fixture data behind.
## Issue Context
`seedFixture()` creates a temp directory under the OS temp folder and writes multiple files into it. PR Compliance ID 1400495 requires tests that create filesystem data to also clean it up via `afterEach`/`afterAll` (or equivalent) or a `try/finally` pattern. Other tests in the repo that use mkdtemp-based fixtures typically clean up with `rmSync(..., { recursive: true, force: true })` in a `finally` block (or via `t.after`).
## Fix Focus Areas
- tests/hub-rollup-signature-order-517.test.js[24-85]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+33
to
+39
| function jitteryIo() { | ||
| return { | ||
| stat: async (p) => { | ||
| await new Promise((resolve) => setTimeout(resolve, Math.random() * 8)); | ||
| return realStat(p); | ||
| }, | ||
| }; |
There was a problem hiding this comment.
7. Unseeded random test jitter 🐞 Bug ⚙ Maintainability
The new regression test injects stat() latency using Math.random(), so if a regression causes intermittent failures they may be hard to reproduce locally due to a different random delay schedule. This reduces diagnosability and can make the test’s regression-catching effectiveness dependent on chance permutations of delays.
Agent Prompt
## Issue description
The test uses `Math.random()` to introduce timing jitter. While the assertion should be stable when the fix is correct, regressions may reproduce intermittently depending on the random schedule, making failures harder to debug.
## Issue Context
This test is intended to stress ordering variance; you can keep the variance while making it reproducible by using a deterministic delay schedule or a seeded PRNG.
## Fix Focus Areas
- tests/hub-rollup-signature-order-517.test.js[33-39]
## Suggested change
Replace `Math.random()` with one of:
- A deterministic per-call delay sequence (e.g., a counter cycling 0..7ms).
- A deterministic delay derived from the stat path (e.g., stable hash(path) % 8).
- A small seeded PRNG local to the test (fixed seed constant) so a failure can be reproduced exactly.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
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
1 participant
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.
Summary
Diagnoses and fixes the second, previously-undiagnosed mechanism from #517 — an intermittent full-suite-only flake where
dashboard-command-pages.test.js's "index-served lite runs keep the persisted rollup..." sawrun.fromIndex === undefined, meaning an unchanged run paid an unnecessary full re-parse instead of being served from the cache.Root cause:
childContractsSignaturebuilt its per-task signature object by mutating a SHARED object from inside concurrent async callbacks (sig[taskId] = perFile) — the resulting key INSERTION ORDER followed I/O completion order, nottasks.json's own task order.Promise.allresolves its result array positionally regardless of which callback'sstat()actually finishes first, but a direct assignment inside the callback body races on completion instead.sigEqualis a plainJSON.stringifycomparison (key-order-sensitive), so two structurally IDENTICAL signatures could serialize to different strings purely from timing jitter — invisible in isolation (fast, low-contentionstat()calls tend to resolve in call order) but real under full-suite CPU/IO load, where resolution order shuffles.This matches every clue from the issue thread: deterministic in isolation (20/20 clean per the prior diagnosis), the index file genuinely is written (waiting on it didn't help — the bug isn't about missing data), and the signature covers mtimes so "a recompute difference under load" was the right instinct — it was just a key-ordering difference in an otherwise-identical recompute, not a real value change.
Verification
tests/hub-rollup-signature-order-517.test.jsstresses the actual mechanism viasyncRootRunswith an injected jitterystat(randomized 0-8ms latency per call — no sleep-based flake-chasing), 40 repeated sync-pairs, asserting the run is served from the index every time.[taskId, perFile]tuples and rebuilding viaObject.fromEntries, whose insertion order is always the array orderPromise.allguarantees (tasks.json's own order), independent of resolution timing.Test plan
npm test— 1916/1916 pass, ×5 consecutive full-suite runsnpm run typecheck— 0 errorsnpm run lint— 0 errorsnode scripts/security-audit.mjs— cleannpm run validate— clean🤖 Generated with Claude Code