fix(test): bound the native-profile teardown and stop parsing partial JSON (#1061) - #1071
Open
lidge-jun wants to merge 1 commit into
Open
fix(test): bound the native-profile teardown and stop parsing partial JSON (#1061)#1071lidge-jun wants to merge 1 commit into
lidge-jun wants to merge 1 commit into
Conversation
… JSON (#1061) The teardown awaited child.exited with no deadline. A child stalled in server.stop(true) hung the run until CI killed the job - a 30-minute wait for a test that had already finished its work. The waits are bounded now, including the ones after a signal: an ignored SIGTERM would otherwise reproduce the same hang one layer down, so it escalates to SIGKILL and observes the reap before throwing. Second half, same issue: waitFor proves a file exists, which is not the precondition a caller that immediately parses it needs. A half-written document satisfied the wait and then threw Unexpected EOF. The settled file is now published through atomicWriteFile, and the reader waits for something parseable rather than something present. Both paths are driven, not just present. An opt-in test-only stall makes the child hang exactly where the reported failure occurred, and the teardown has to give up and reap it; removing the bound makes that test time out at 30s instead of passing in 1.2s. The partial-write race is reproduced directly.
|
✅ Deterministic PR hygiene checks passed. |
Contributor
📝 WalkthroughWalkthroughThe startup child now publishes settled results atomically and can stall during shutdown. Recovery tests wait for parseable JSON and use bounded shutdown with ChangesNative profile crash-boundary tests
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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}
Contributor
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 `@tests/native-profile-crash-boundaries.test.ts`:
- Around line 130-135: The final child-reap wait in stopStartup must remain
bounded and its result must be checked; update
tests/native-profile-crash-boundaries.test.ts:128-133 to store the Promise.race
result and throw explicitly when it is null. In cleanup at
tests/native-profile-crash-boundaries.test.ts:309-313, replace the unbounded
child.exited await with the same bounded reap wait used by stopStartup.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: d6cb39a0-e1ee-46e6-9edc-d0d081554080
📒 Files selected for processing (2)
tests/helpers/native-profile-startup-child.tstests/native-profile-crash-boundaries.test.ts
Comment on lines
+130
to
+135
| child.kill("SIGKILL"); | ||
| // Observe the escalation before throwing, so a caller asserting on | ||
| // `exitCode` is not racing the reap. | ||
| await Promise.race([child.exited, Bun.sleep(KILL_GRACE_MS).then(() => null)]); | ||
| } | ||
| throw new Error("startup child did not stop"); |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file outline =="
ast-grep outline tests/native-profile-crash-boundaries.test.ts || true
echo "== relevant lines 100-150 =="
sed -n '100,150p' tests/native-profile-crash-boundaries.test.ts
echo "== relevant lines 280-330 =="
sed -n '280,330p' tests/native-profile-crash-boundaries.test.ts
echo "== occurrences of native-profile test helpers / exported interfaces =="
rg -n "stopStartup|child\\.exited|KILL_GRACE_MS|SIGKILL|exited" tests/native-profile-crash-boundaries.test.ts src || trueRepository: lidge-jun/opencodex
Length of output: 11802
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== surrounding stopStartup callers in test =="
sed -n '200,230p' tests/native-profile-crash-boundaries.test.ts
sed -n '240,290p' tests/native-profile-crash-boundaries.test.ts
sed -n '298,360p' tests/native-profile-crash-boundaries.test.ts
echo "== static control-flow probe for stopStartup unbounded awaits =="
python3 - <<'PY'
from pathlib import Path
p = Path("tests/native-profile-crash-boundaries.test.ts")
text = p.read_text()
sig = "async function stopStartup("
start = text.find(sig)
end = text.find("\nfunction spawnSwitch") if "function spawnSwitch" in text[start:] else start + 2000
body = text[start:end]
lines = body.splitlines()
for i, line in enumerate(lines, 1):
j = start + sum(len(x)+1 for x in lines[:i-1])
if "await Promise.race([child.exited" in line or "await child.exited" in line:
print(f"{p}:{i+j}: {line.strip()}")
PY
echo "== unbounded child.exited in target test sites =="
python3 - <<'PY'
from pathlib import Path
p = Path("tests/native-profile-crash-boundaries.test.ts")
for needle, line in [(133, "tests/native-profile-crash-boundaries.test.ts:133"), (312, "tests/native-profile-crash-boundaries.test.ts:312")]:
print(needle + " contains Promise.race:", "Promise.race" in p.read_text().splitlines()[needle-1])
PYRepository: lidge-jun/opencodex
Length of output: 7128
Keep final child-reap waits bounded and check their result.
tests/native-profile-crash-boundaries.test.ts:109-126 documents that this suite avoids unbounded await child.exited after signals, but line 133 still discards the final Promise.race result after SIGKILL. If the child is unreaped, stopStartup throws, and the expected-failure cleanup at line 312 then uses an unbounded await child.exited. That can restore the teardown hang this test adds coverage for.
tests/native-profile-crash-boundaries.test.ts:128-133: store the final race result and throw explicitly when it isnull.tests/native-profile-crash-boundaries.test.ts:309-312: replace the finalawait child.exitedincleanupwith the same bounded reap wait used instopStartup.
📍 Affects 1 file
tests/native-profile-crash-boundaries.test.ts#L130-L135(this comment)tests/native-profile-crash-boundaries.test.ts#L309-L313
🤖 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 `@tests/native-profile-crash-boundaries.test.ts` around lines 130 - 135, The
final child-reap wait in stopStartup must remain bounded and its result must be
checked; update tests/native-profile-crash-boundaries.test.ts:128-133 to store
the Promise.race result and throw explicitly when it is null. In cleanup at
tests/native-profile-crash-boundaries.test.ts:309-313, replace the unbounded
child.exited await with the same bounded reap wait used by stopStartup.
3 tasks
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
Two defects in the same test, both reported in #1061.
The teardown had no deadline. It did
expect(await restart.exited).toBe(0), so a child stalled inserver.stop(true)hung the run until CI killed the job — a 30-minute wait for a test that had already finished its work.Every wait is bounded now, including the ones after a signal. An ignored SIGTERM would otherwise reproduce the same hang one layer down, so it escalates to SIGKILL and observes the reap before throwing.
The settled file was parsed the moment it existed.
waitForproves a path exists, which is not the precondition a caller that immediately callsJSON.parseactually needs. The child wrote that file non-atomically, so a partially written document satisfied the wait and then threwUnexpected EOF— the failure the issue reports.The child now publishes through
atomicWriteFile, which the repository already uses precisely because it guarantees a reader sees either nothing or the whole document. The reader waits for something parseable rather than something present.Both fixes reuse mechanisms already in the tree: the bounded-wait shape comes from the sibling
native-profile-startup.test.ts, the deadline fromtests/helpers/test-budget.ts, and the atomic writer fromsrc/config.ts.Closes #1061.
Verification
Both paths are driven, not merely present — a green suite says nothing about a timeout branch nobody exercises:
OCX_TEST_STALL_ON_STOP=1) makes the child hang exactly where the reported failure occurs, and the teardown must give up and reap it. Removing the bound makes that test time out at 30s instead of passing in 1.2s.One honest limit: a child that ignored SIGKILL is not testable and not a real case, so the escalation branch itself stays unproven.
Full suite on this branch is unaffected; the only failure anywhere is
jawcode-metadata-sync, which fails identically onorigin/dev.Checklist
Summary by CodeRabbit