Repository navigation
fix: preserve daemon lifetime lock on same-process checks - #31
Conversation
posix_lifetime_lock_probe opened a throwaway fd on the lock file to confirm an already-claimed lock, then closed it. fcntl(2) record locks are scoped to (process, inode), so that close silently released the real lock too, even though the reservation's own fd stayed open. Check the in-process claim before opening anything and return early; that branch only ever needed to trust the registry. Signed-off-by: Amir Fathi <[email protected]>
posix_lifetime_lock_try_acquire opened a throwaway fd on the lock file to confirm an already-claimed lock, then closed it, in both the probe path and the try-acquire-again path. fcntl(2) record locks are scoped to (process, inode), so that close silently released the real lock too, even though the reservation's own fd stayed open. Check the in-process claim before opening anything and return early in both places; neither branch ever needed more than the registry. Also hardens the child-process verdict in the regression test: it now reports held only when open() succeeded and F_SETLK failed with EACCES or EAGAIN, so a wrong path or an unrelated errno can no longer read as a pass. Signed-off-by: Amir Fathi <[email protected]>
Adapt upstream merge c80a256 (DeusData#2207). Concurrent probe and acquisition still require review.
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 59 minutes. View limit details
Tip 🎯 Customize your risk assessments
✅ Automatic approvals, with your rules
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. |
|
Review update (2026-11-02): commit |
|
Re-review at |
|
Implementation follow-up: committed |
License gate step 3/3 ran `npm ci` only when graph-ui/node_modules was missing. Jenkins main build #32 (commit 2093ca0) reused a node_modules tree left in the persistent workspace by build #31 (2026-10-06), which was installed from a lockfile that predates the @types/node devDependency. The install was skipped, `tsc -b` failed with TS2688 "Cannot find type definition file for 'node'", and every later stage was skipped. A stale tree is also a correctness problem for the gate itself: the npm license check would inspect packages that are not the ones this candidate ships. The gate now reuses an existing tree only when `npm ls --all` confirms it matches package.json and package-lock.json. Otherwise it runs `npm ci --ignore-scripts`, which replaces the tree. A fresh `npm ci` of the current lockfile passes `npm ls --all`, so a matching tree does not trigger a reinstall on every run. Since 9f26f16 the Jenkinsfile also cleans the workspace after each build. This change keeps the gate correct for local runs and for any workspace the cleanup misses.
Draft pending concurrency review and Jenkins qualification. POSIX record locks are released when this process closes any descriptor for the lock inode. A same-process lifetime probe or repeat acquisition used to open and close a second descriptor, potentially dropping the existing reservation. This applies upstream DeusData#2207, merge
c80a2569995c44d7ad8522ea97e5944c4b5265be, to fork base27b731cec45e2353303738ac6d8e77acbef42b7c; original fix96e44e2a87492e4e2ac66ad5c39b6a889192f995has the same stable patch ID as the upstream first-parent diff. Merged48aa2de47bef6b5aa04a25373c778cb72b31cc1preserves the upstream feature ancestry: its parents are fork fix712a032c32e68e233871065152535abd4aa1a5bfand upstream feature tip8bd9d37b156641ecf1c82b16b0ec0a8246e8c201, bringing upstream commits3aec07ea3705700a1eba71321f1eb76c435a4d1dand8bd9d37b156641ecf1c82b16b0ec0a8246e8c201into ancestry. The merge tree is byte-identical to712a032c32e68e233871065152535abd4aa1a5bf; relative to current upstream it reduces the upstream-only count from 926 to 924. The later upstream PR mergec80a2569995c44d7ad8522ea97e5944c4b5265becarries 888 additional upstream commits and remains for the checkpoint sequence. No dependency on unmerged PRs.The existing-claim paths return before opening a second descriptor; two upstream regressions check that a child cannot acquire the lock after repeated probes or a repeat acquisition. Linux
scripts/test.sh --suites daemon_ipc: 57 passed under ASan/UBSan, rerun after commit. Actions policy checker passed; Jenkins remains the sole automatic CI authority. No Jenkins result at this SHA or native Windows/macOS qualification is claimed. No schema or index migration is introduced.Open correctness question: the process registry check and the later probe open/close are not guarded as one operation; a concurrent acquisition between them may still allow the close to release the new lock. Verify or eliminate this interleaving before review-ready. The originating Agent Runs were orphaned before completion and have not been evaluated or accepted; the parent inspected the patch and ran the suite. No MCP functionality is introduced.
2026-11-02 synchronization follow-up
Commit
712a032c32e68e233871065152535abd4aa1a5bfserializes process-local lifetime-lock probe/acquire/release. The earlier open correctness question is addressed in code but not yet independently validated. No tests or Jenkins result are claimed at this new head. Required: concurrent interleaving regression, fork/mutex safety review, Linux daemon_ipc sanitizer rerun and Jenkins exact-head gate. Remains draft.Jenkins build #2 passed 57 daemon IPC tests and production build at
712a032c32e68e233871065152535abd4aa1a5bf. The new merged48aa2de47bef6b5aa04a25373c778cb72b31cc1has an identical tree but has not been independently tested at that SHA. Required deterministic race regression and fork review remain outstanding.