Repository navigation
fix: update restart with stale service record; silent journal no-op recovery #6649
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -304,6 +304,30 @@ describe("codex-journal", () => { | |
| expect(existsSync(journalPath)).toBe(false); | ||
| }); | ||
|
|
||
| test("reconcileJournal stays silent when a dead-owner journal needs no rewrite", () => { | ||
| const journalPath = join(testDir, "opencodex-journal.json"); | ||
| const original = "# original config\nmodel_provider = \"openai\"\n"; | ||
| writeFileSync(join(testDir, "config.toml"), original, "utf8"); | ||
| writeFileSync(journalPath, JSON.stringify({ | ||
| version: 1, | ||
| originalConfig: Buffer.from(original).toString("base64"), | ||
| originalProfile: null, | ||
| pid: 999999, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win 🔎 Supported by static analysis🏁 Script executed: rg -n -C 5 'pid|kill|reconcileJournal' src/codex/journal.ts
if [ -r /proc/sys/kernel/pid_max ]; then cat /proc/sys/kernel/pid_max; fi
if kill -0 999999 2>/dev/null; then
echo "PID 999999 is live in this environment"
else
echo "PID 999999 is absent in this environment"
fiRepository: lidge-jun/opencodex Length of output: 3616 🏁 Script executed: printf '%s\n' '--- test context ---'
nl -ba tests/codex-integration/codex-journal.test.ts | sed -n '270,345p'
printf '%s\n' '--- reconcile implementation ---'
nl -ba src/codex/journal.ts | sed -n '387,432p'
printf '%s\n' '--- test file setup and relevant helpers ---'
nl -ba tests/codex-integration/codex-journal.test.ts | sed -n '1,90p'
printf '%s\n' '--- CI workflow files ---'
rg --files .github/workflows || test "$?" -eq 1
printf '%s\n' '--- CI OS/runtime declarations ---'
rg -n -i 'runs-on:|matrix:|ubuntu|macos|windows|pid_max|node-version|bun-version' .github/workflows package.json bun.lockb bun.lock 2>/dev/null || test "$?" -eq 1Repository: lidge-jun/opencodex Length of output: 33059 🏁 Script executed: printf '%s\n' '--- test command references ---'
rg -n -F 'codex-journal.test.ts' .github package.json tests || test "$?" -eq 1
rg -n 'bun test|test:unit|test:integration|codex-integration' .github/workflows/ci.yml package.json
printf '%s\n' '--- primary test job ---'
nl -ba .github/workflows/ci.yml | sed -n '470,565p'
printf '%s\n' '--- platform test jobs ---'
nl -ba .github/workflows/ci.yml | sed -n '680,755p'
nl -ba .github/workflows/ci.yml | sed -n '840,960p'Repository: lidge-jun/opencodex Length of output: 17780 Make the dead-owner PID deterministic. If PID 999999 is live, 🐛 Suggested fix const { reconcileJournal } = require("./src/codex/journal");
+ const originalKill = process.kill;
+ process.kill = (pid, signal) => {
+ if (pid === 999999 && signal === 0) {
+ const error = Object.assign(new Error("fixture process is not running"), { code: "ESRCH" });
+ throw error;
+ }
+ return originalKill.call(process, pid, signal);
+ };
const result = reconcileJournal();🤖 Prompt for AI Agents |
||
| timestamp: new Date().toISOString(), | ||
| }), "utf8"); | ||
|
|
||
| const r = runScript(testDir, ` | ||
| const { reconcileJournal } = require("./src/codex/journal"); | ||
| const result = reconcileJournal(); | ||
| console.log(JSON.stringify({ restored: result })); | ||
| `); | ||
| expect(r.status).toBe(0); | ||
| expect(JSON.parse(r.stdout).restored).toBe(false); | ||
| expect(readFileSync(join(testDir, "config.toml"), "utf8")).toBe(original); | ||
| expect(existsSync(journalPath)).toBe(false); | ||
| expect(r.stderr).not.toContain("did not shut down cleanly"); | ||
| }); | ||
|
|
||
| test("reconcileJournal preserves corrupt JSON without authorizing recovery", () => { | ||
| const journalPath = join(testDir, "opencodex-journal.json"); | ||
| writeFileSync(journalPath, "NOT VALID JSON{{{", "utf8"); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
Repository: lidge-jun/opencodex
Length of output: 14075
🏁 Script executed:
Repository: lidge-jun/opencodex
Length of output: 17886
🏁 Script executed:
Repository: lidge-jun/opencodex
Length of output: 7670
Report incomplete journal recovery before the no-rewrite return.
When the config is already original and profile removal fails,
restoreJournalState()leaves the generated profile and journal in place withcompletefalse and both rewrite flags false. Both reconciliation branches then return without warning. Keep the silent return for successful no-op cleanup, but report incomplete recovery first.🐛 Suggested fix
if (restored.unverified) { console.error("⚠️ Codex journal recovery was not verified; current configuration files and the journal were preserved."); return false; } + if (!restored.complete) { + console.error("⚠️ Codex journal recovery was incomplete; check the Codex files and journal."); + return false; + } if (!restored.configRewritten && !restored.profileRewritten) return false;Apply this check in both reconciliation branches.
🤖 Prompt for AI Agents