fix: resolve ConfigCache.has() stat inflation and hook system errors#582
Open
oalanicolas wants to merge 2 commits intomainfrom
Open
fix: resolve ConfigCache.has() stat inflation and hook system errors#582oalanicolas wants to merge 2 commits intomainfrom
oalanicolas wants to merge 2 commits intomainfrom
Conversation
…497, #528) #497: ConfigCache.has() delegated to get(), inflating hit/miss stats. Reimplemented has() to check Map directly without touching metrics. Applied to both core/config/ and infrastructure/scripts/ copies. #528: precompact-session-digest.cjs failed in installed projects because runner path didn't account for node_modules layout. Added path resolution with fallback candidates. Also fixed: - synapse-engine.cjs: replaced process.exit() with process.exitCode for clean stdout flush - hook-runtime.js: create session file on first prompt when loadSession returns null (prevents silent updateSession failures) Updated tests to match new behavior (process.exitCode, createSession export). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
github-actions
bot
added
area: agents
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughConfigCache.has() now checks expiration without calling get(); hook runner lookup for precompact-runner.js is dynamic and exits gracefully if missing; session initialization uses createSession when loadSession returns null; process exit handling switched to exitCode-based flush; manifest hashes and timestamp updated. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
Contributor
📊 Coverage ReportCoverage report not available
Generated by PR Automation (Story 6.1) |
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
.aiox-core/core/config/config-cache.js (1)
74-85: Extract the expiry/eviction check into one helper.
get(),has(), andclearExpired()now all carry their own TTL logic here, and the same behavior is mirrored again in.aiox-core/infrastructure/scripts/config-cache.js. Centralizing the “is entry still valid, otherwise evict it” path would reduce drift on this core module.♻️ Possible refactor
+ _hasLiveEntry(key, now = Date.now()) { + if (!this.cache.has(key)) { + return false; + } + + const timestamp = this.timestamps.get(key); + if (now - timestamp > this.ttl) { + this.cache.delete(key); + this.timestamps.delete(key); + return false; + } + + return true; + } + get(key) { - if (!this.cache.has(key)) { + if (!this._hasLiveEntry(key)) { this.misses++; return null; } - - const timestamp = this.timestamps.get(key); - const now = Date.now(); - - // Check if expired - if (now - timestamp > this.ttl) { - this.cache.delete(key); - this.timestamps.delete(key); - this.misses++; - return null; - } this.hits++; return this.cache.get(key); } has(key) { - if (!this.cache.has(key)) { - return false; - } - const timestamp = this.timestamps.get(key); - if (Date.now() - timestamp > this.ttl) { - this.cache.delete(key); - this.timestamps.delete(key); - return false; - } - return true; + return this._hasLiveEntry(key); }As per coding guidelines, "Ensure backwards compatibility — core modules are consumed by all agents."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.aiox-core/core/config/config-cache.js around lines 74 - 85, The TTL/eviction logic is duplicated across has(), get(), and clearExpired() (and mirrored in the script version); extract that logic into a single helper method (e.g., isExpiredAndEvict(key) or ensureNotExpired(key)) that checks timestamps.get(key) vs Date.now(), deletes from this.cache and this.timestamps when expired, and returns a boolean indicating validity; update has(), get(), and clearExpired() to call this helper so behavior remains identical and keep method name/signature stable to preserve backwards compatibility.tests/synapse/hook-entry.test.js (1)
337-343: Please cover the successfulcreateSession()path too.These assertions only exercise
loadSession() === nullpluscreateSession() === null, so the new happy path inresolveHookRuntime()is still untested. Add one case wherecreateSession()returns a real session object and mirror it in the in-process variant below.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/synapse/hook-entry.test.js` around lines 337 - 343, Add a new test case (or extend the existing one) so that when loadSession() returns null but createSession() returns a real session object, resolveHookRuntime() follows the happy path and returns/uses that session; update both the existing in-process test and its in-process variant to mock createSession() to return a real session object (e.g., an object with prompt_count and any other expected fields) and add assertions that the resolved session equals that object and that any derived values (such as prompt_count) match; target the functions loadSession, createSession, and resolveHookRuntime in tests/synapse/hook-entry.test.js to ensure coverage of the successful createSession path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.claude/hooks/precompact-session-digest.cjs:
- Around line 73-75: The hook currently calls resolveRunnerPath() then executes
the runner which uses setImmediate(async => ...) in-process, keeping the event
loop alive; instead spawn a detached child process to run
.aiox-core/hooks/unified/runners/precompact-runner.js so the digest is
fire-and-forget. Modify the code that uses resolveRunnerPath() / runnerPath to
fork or spawn a detached process (child_process.fork or spawn) with the runner
path as the module/entry, set detached: true, stdio: 'ignore', and call
child.unref() so the parent can exit immediately; ensure any args or env needed
by the runner are forwarded and preserve the existing early return when
!runnerPath.
In @.claude/hooks/synapse-engine.cjs:
- Around line 90-94: The timeout callback in run() currently sets
process.exitCode = 0 which doesn't force termination; change the callback to
call process.exit(0) so the 5s limit is enforced (i.e., replace the body of the
setTimeout in run() to invoke process.exit(0)); keep the existing timer handling
(including timer.unref() if present) but ensure the timeout uses process.exit(0)
to forcibly terminate when HOOK_TIMEOUT_MS elapses.
---
Nitpick comments:
In @.aiox-core/core/config/config-cache.js:
- Around line 74-85: The TTL/eviction logic is duplicated across has(), get(),
and clearExpired() (and mirrored in the script version); extract that logic into
a single helper method (e.g., isExpiredAndEvict(key) or ensureNotExpired(key))
that checks timestamps.get(key) vs Date.now(), deletes from this.cache and
this.timestamps when expired, and returns a boolean indicating validity; update
has(), get(), and clearExpired() to call this helper so behavior remains
identical and keep method name/signature stable to preserve backwards
compatibility.
In `@tests/synapse/hook-entry.test.js`:
- Around line 337-343: Add a new test case (or extend the existing one) so that
when loadSession() returns null but createSession() returns a real session
object, resolveHookRuntime() follows the happy path and returns/uses that
session; update both the existing in-process test and its in-process variant to
mock createSession() to return a real session object (e.g., an object with
prompt_count and any other expected fields) and add assertions that the resolved
session equals that object and that any derived values (such as prompt_count)
match; target the functions loadSession, createSession, and resolveHookRuntime
in tests/synapse/hook-entry.test.js to ensure coverage of the successful
createSession path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 184d4eef-e5d1-4164-8b21-7711fa2e342b
📒 Files selected for processing (7)
.aiox-core/core/config/config-cache.js.aiox-core/core/synapse/runtime/hook-runtime.js.aiox-core/infrastructure/scripts/config-cache.js.aiox-core/install-manifest.yaml.claude/hooks/precompact-session-digest.cjs.claude/hooks/synapse-engine.cjstests/synapse/hook-entry.test.js
Comment on lines
+73
to
75
| const runnerPath = resolveRunnerPath(); | ||
| if (!runnerPath) return; // Runner not available — silent exit | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
python - <<'PY'
import subprocess, textwrap, time
script = textwrap.dedent("""
async function onPreCompact() {
setImmediate(async () => {
await new Promise((resolve) => setTimeout(resolve, 2000));
});
}
onPreCompact().then(() => {
process.exitCode = 0;
});
""")
start = time.time()
subprocess.run(['node', '-e', script], check=True)
print(f'elapsed={time.time() - start:.2f}s')
PYRepository: SynkraAI/aiox-core
Length of output: 75
Move the digest work to a detached child process or worker.
Now that installed projects can resolve the runner, the hook executes .aiox-core/hooks/unified/runners/precompact-runner.js, which queues the digest via setImmediate(async () => ...) inside the current process. This keeps the event loop alive until the async work completes, causing the compaction to block or wait for the 9-second timeout instead of remaining fire-and-forget.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.claude/hooks/precompact-session-digest.cjs around lines 73 - 75, The hook
currently calls resolveRunnerPath() then executes the runner which uses
setImmediate(async => ...) in-process, keeping the event loop alive; instead
spawn a detached child process to run
.aiox-core/hooks/unified/runners/precompact-runner.js so the digest is
fire-and-forget. Modify the code that uses resolveRunnerPath() / runnerPath to
fork or spawn a detached process (child_process.fork or spawn) with the runner
path as the module/entry, set detached: true, stdio: 'ignore', and call
child.unref() so the parent can exit immediately; ensure any args or env needed
by the runner are forwarded and preserve the existing early return when
!runnerPath.
Comment on lines
+90
to
+94
| /** Entry point runner — lets Node exit naturally after stdout flush. */ | ||
| function run() { | ||
| const timer = setTimeout(() => safeExit(0), HOOK_TIMEOUT_MS); | ||
| const timer = setTimeout(() => { | ||
| process.exitCode = 0; | ||
| }, HOOK_TIMEOUT_MS); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
python - <<'PY'
import subprocess, textwrap, time
script = textwrap.dedent("""
setInterval(() => {}, 1000);
setTimeout(() => {
process.exitCode = 0;
}, 1000);
""")
start = time.time()
proc = subprocess.Popen(['node', '-e', script], stdout=subprocess.PIPE, stderr=subprocess.PIPE, text=True)
try:
proc.wait(timeout=3)
print(f'exited_in={time.time() - start:.2f}s code={proc.returncode}')
except subprocess.TimeoutExpired:
proc.kill()
print('still_running_after_3s')
PYRepository: SynkraAI/aiox-core
Length of output: 84
🏁 Script executed:
cat -n .claude/hooks/synapse-engine.cjs | sed -n '80,110p'Repository: SynkraAI/aiox-core
Length of output: 1033
The timeout callback must use process.exit() to enforce the 5s limit.
process.exitCode = 0 only sets the eventual exit status without terminating the process. If main() hangs on stdout backpressure or other active handles, the timeout callback will not stop execution; the process will wait indefinitely for those handles to close. The timer.unref() call prevents the timer itself from blocking Node, but does not help if other handles remain open.
Suggested fix
/** Entry point runner — lets Node exit naturally after stdout flush. */
function run() {
const timer = setTimeout(() => {
- process.exitCode = 0;
+ process.exit(0);
}, HOOK_TIMEOUT_MS);📝 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.
Suggested change
| /** Entry point runner — lets Node exit naturally after stdout flush. */ | |
| function run() { | |
| const timer = setTimeout(() => safeExit(0), HOOK_TIMEOUT_MS); | |
| const timer = setTimeout(() => { | |
| process.exitCode = 0; | |
| }, HOOK_TIMEOUT_MS); | |
| /** Entry point runner — lets Node exit naturally after stdout flush. */ | |
| function run() { | |
| const timer = setTimeout(() => { | |
| process.exit(0); | |
| }, HOOK_TIMEOUT_MS); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.claude/hooks/synapse-engine.cjs around lines 90 - 94, The timeout callback
in run() currently sets process.exitCode = 0 which doesn't force termination;
change the callback to call process.exit(0) so the 5s limit is enforced (i.e.,
replace the body of the setTimeout in run() to invoke process.exit(0)); keep the
existing timer handling (including timer.unref() if present) but ensure the
timeout uses process.exit(0) to forcibly terminate when HOOK_TIMEOUT_MS elapses.
Contributor
|
Heads up @Pedrovaleriolopez — esse fix do ConfigCache.has() já está no nosso PR #498 (aberto 24/fev, há mais de 2 semanas). Mesmo bug, mesma solução. O #498 já tá rebased e pronto pra review. |
2 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
2 participants
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
Fixes two reported bugs with correct implementations:
bug: ConfigCache.has() inflates hit/miss stats by calling get() internally #497 —
ConfigCache.has()delegated toget(), inflatinghits/missesstats on every existence check. Reimplementedhas()to check the Map directly with TTL expiration logic, without touching metrics. Applied to bothcore/config/config-cache.jsandinfrastructure/scripts/config-cache.js.bug: install registers precompact-session-digest hook in UserPromptSubmit with missing runner #528 —
precompact-session-digest.cjsfailed in installed projects because the runner path didn't account fornode_modules/layout. Added multi-path resolution with fallback. Also fixed:synapse-engine.cjs: replacedprocess.exit()withprocess.exitCodefor clean stdout flush (prevents truncated hook output)hook-runtime.js: callcreateSession()whenloadSession()returns null on first prompt (fixes silentupdateSessionfailures)Related PRs
Test plan
npm run lint— 0 errorshook-entry.test.jsupdated to match new behavior (process.exitCode, createSession mock)config-cachehas() no longer inflates stats when calledprecompact-session-digestresolves runner path in both framework-dev and installed-project modesCloses #497
Closes #528
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Refactor
Tests
Chores