feat: version runtime observability snapshots#379
feat: version runtime observability snapshots#379ndycode wants to merge 1 commit intofeat/harden-request-reliabilityfrom
Conversation
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 28 minutes and 21 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
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 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| describe("runtime observability snapshot versioning", () => { | ||
| afterEach(() => { | ||
| readFileMock.mockReset(); | ||
| }); |
There was a problem hiding this comment.
vi.resetModules()
both tests use dynamic await import(...) inside their it() bodies but there is no vi.resetModules() call between them. in vitest's ESM mode the second test reuses the already-cached module instance, including the module-level snapshotState and pendingWrite singletons. the current tests pass only because they both call loadPersistedRuntimeObservabilitySnapshot, which is stateless with respect to those singletons. any future test that calls mutateRuntimeObservabilitySnapshot will see leftover state and produce intermittent failures depending on test order.
add a beforeEach reset:
| describe("runtime observability snapshot versioning", () => { | |
| afterEach(() => { | |
| readFileMock.mockReset(); | |
| }); | |
| describe("runtime observability snapshot versioning", () => { | |
| beforeEach(() => { | |
| vi.resetModules(); | |
| }); | |
| afterEach(() => { | |
| readFileMock.mockReset(); | |
| }); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: test/runtime-observability.test.ts
Line: 19-22
Comment:
**module cache shared across tests — missing `vi.resetModules()`**
both tests use dynamic `await import(...)` inside their `it()` bodies but there is no `vi.resetModules()` call between them. in vitest's ESM mode the second test reuses the already-cached module instance, including the module-level `snapshotState` and `pendingWrite` singletons. the current tests pass only because they both call `loadPersistedRuntimeObservabilitySnapshot`, which is stateless with respect to those singletons. any future test that calls `mutateRuntimeObservabilitySnapshot` will see leftover state and produce intermittent failures depending on test order.
add a `beforeEach` reset:
```suggestion
describe("runtime observability snapshot versioning", () => {
beforeEach(() => {
vi.resetModules();
});
afterEach(() => {
readFileMock.mockReset();
});
```
How can I resolve this? If you propose a fix, please make it concise.
|
Superseded by #387, which rebuilds the full open PR stack onto one reviewed integration branch. |
|
Closing in favor of #387. |
1 participant
Summary
Validation
note: greptile review for oc-chatgpt-multi-auth. cite files like
lib/foo.ts:123. confirm regression tests + windows concurrency/token redaction coverage.Greptile Summary
this pr adds a
versionfield to persisted runtime observability snapshots and implements two migration behaviors: legacy unversioned snapshots are normalized onto the current schema via a safe spread-merge, and unknown future versions are silently rejected. the approach is consistent with howstorage/migrations.tshandles v1/v2→v3 upgrades elsewhere in the codebase.key concerns:
writeSnapshotcallsfs.renamewithout EBUSY/EPERM retry logic, violating the project's explicit anti-pattern. the failure is then silently swallowed by thependingWritechain's.catch(() => undefined), making the data loss invisible on windows.await import(...)withoutvi.resetModules()in abeforeEach, sharing the module-levelsnapshotStatesingleton across tests — currently harmless but fragile as the suite grows.existsSync=false, JSON-parse-error, and non-object guard branches inloadPersistedRuntimeObservabilitySnapshotare untested; the 80% threshold may be at risk.Confidence Score: 3/5
safe to merge on linux/mac; windows rename path silently loses snapshot data without retry logic
core versioning logic is correct and well-scoped; the windows filesystem safety gap is a real concern per project conventions but non-blocking for non-windows environments; missing test isolation is fragile but not currently failing
lib/runtime/runtime-observability.ts (writeSnapshot rename retry), test/runtime-observability.test.ts (vi.resetModules + branch coverage)
Important Files Changed
Sequence Diagram
sequenceDiagram participant Caller participant mutate as mutateRuntimeObservabilitySnapshot participant chain as pendingWrite chain participant write as writeSnapshot participant fs as node:fs Caller->>mutate: mutator fn mutate->>mutate: apply mutator, stamp updatedAt mutate->>chain: .then(writeSnapshot(clone)) chain->>write: writeSnapshot(snapshot) write->>fs: mkdir(dir, recursive) write->>fs: writeFile(tempPath) write->>fs: rename(tempPath → path) note over fs: ⚠ no EBUSY/EPERM retry on Windows fs-->>write: ok / EBUSY throws write-->>chain: rejects note over chain: next .catch(() => undefined) silently drops error Caller->>+write: loadPersistedRuntimeObservabilitySnapshot() write->>fs: existsSync(path) alt file missing write-->>Caller: null else file present write->>fs: readFile(path) write->>write: parse + check version alt version undefined — legacy write-->>Caller: spread-merged snapshot (version=1) else version === 1 — current write-->>Caller: spread-merged snapshot else version > 1 — unknown future write-->>Caller: null end end deactivate writeComments Outside Diff (1)
lib/runtime/runtime-observability.ts, line 79-81 (link)fs.renameon windows can fail withEBUSYorEPERMwhen antivirus has the destination file open. perAGENTS.md, bare filesystem writes without retry logic are an explicit anti-pattern ("do not use bare fs.rm without retry logic — Windows antivirus locks"). the failure propagates intowriteSnapshotwhich throws, themovedflag stays false, the temp file is cleaned up — but thependingWritechain's next.catch(() => undefined)silently swallows the rejection. windows users will lose snapshot data with zero diagnostic signal.Prompt To Fix With AI
Prompt To Fix All With AI
Reviews (1): Last reviewed commit: "feat: version runtime observability snap..." | Re-trigger Greptile
Context used: