Repository navigation
fix(openclaw-tps-mail): the aged-nack release re-checks the record is still the aged debt; a held record keeps its receipt - #534
Conversation
…ld record keeps its receipt The aged owed-nack release re-read the record but checked only its state, so a record rewritten to a fresh debt between the sweep's read and the release had its fresh debt cleared. It now releases only while the re-read record is still an owed nack whose last transition is still older than the window the sweep selected it by. A record held for its owed nack was added to the receipt sweep's terminal set before the hold, so its metadata receipt was deleted while the obligation was still retained. A held record now marks its obligation live, so the receipt stays until the obligation itself is removed. Restores the sweep docstring and the index.ts call-site comment that state the age bound. Closes #533
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (5)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughObligation and receipt operations now use the agent-store lock. The retention sweep applies a configurable hold to owed NACKs and preserves receipts for held, ambiguous, or unageable obligations. ChangesObligation locking and NACK retention
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix Sequence Diagram(s)sequenceDiagram
participant RetentionSweep
participant AgentStoreLock
participant ObligationRecords
participant ReceiptFiles
RetentionSweep->>AgentStoreLock: Acquire lock without waiting
AgentStoreLock-->>RetentionSweep: Grant lock or report busy
RetentionSweep->>ObligationRecords: Read and classify records
RetentionSweep->>ObligationRecords: Release aged owed NACK with cutoff
RetentionSweep->>ReceiptFiles: Remove aged receipts not protected by live or ambiguous IDs
RetentionSweep->>AgentStoreLock: Release lock
Suggested reviewers: Merge Risk: ⚪ Minimal · up to No actionable merge-blocking issue remains; the change is ready for normal merge checks. 🚥 Pre-merge checks | ✅ 3 | ❌ 1 | ❓ 1❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
Full details: Linked Issues checkExplanation Issue Full details: Docstring CoverageExplanation Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 16 functions across 3 files. (2 skipped: 2 unsupported.)
✨ Finishing Touches 💡 1📝 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 |
|
Prose sweep — every sentence this PR adds or changes, checked against the code (deliverable, not a repository file). Scope of the sweep:
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
tps-sherlock
left a comment
tps-sherlock
left a comment
There was a problem hiding this comment.
APPROVE — reviewed head 04ea7a83 (all three commits). Author is a tps-* agent, so this was a read+run review, not a read-only one. Repo tpsdev-ai/cli is PUBLIC (checked repos/tpsdev-ai/cli → .visibility: "public", .private: false). Nothing below is an unpatched exposure that is not already readable in this public diff; the findings are about lock semantics and test coverage.
Verification. Worktree ~/work/review-534-sherlock @ 04ea7a83; bun install --frozen-lockfile && bun run build (agent+cli) and bun run build in the plugin; plugin suite through its own launcher (scripts/run-tests.mjs): 339 pass / 0 fail across 20 files. (First run showed 2 node-load failures — a missing-plugin-dist artifact, not the PR; after building the plugin they pass.) I also ran 5 targeted mutations against test/aged-nack-recheck-and-receipt.test.ts + test/cur-record-write.test.ts.
The fix is correct, and the new tests bite.
- Same lock as the sweep's read (Kern #1).
sweepTerminalObligationstakes the lock (obligations.ts:563→lock = acquireMailLockSync(resolve(mailDir, agent), { timeoutMs: 0 })) and then callssweepTerminalObligationsLocked, which callsabandonOwedNackLockedwith no re-acquire. Every public writer takes the same root lock viawithObligationLock(obligations.ts:232). Mutation M5 (replace the sweep lock with a no-op) failsskips a locked store while a competing writer refreshes one record— the lock is what closes #533, and the CLI'supdateExistingRecordholds the same root (dirname(dirname(path))=<mailDir>/<agent>), which is what lets that test reproduce the interleaving. - Same cutoff (Kern #2).
nackCutoff = nowMs - nackHoldDays * 24*60*60*1000is computed once in the sweep and passed straight into the release, which re-checksowedSince === null || owedSince >= nackCutoffMs(obligations.ts:404-406). - The receipt bug (the headline). The held-for-nack record now marks its obligation LIVE, not terminal:
if (!nackAbandoned && nackOwed(record)) { if (typeof snapshotObligationId === "string") liveObligationIds.add(snapshotObligationId); res.heldForNack++; … }(previouslyterminalObligationIds.add(...)before the hold, so the receipt was deleted while the record was retained — #533). Mutation M1 (revert toterminalObligationIds) fails 7 tests, incl.is kept while the record is held, and goes when the obligation is removed. - Adversarial records (Sherlock). The new inbound/filename equality in the shape check (
typeof inbound !== "string" ||${inbound}.json!== name) stops the release from being handed an inbound id that reads a different record; M3 fails 3 tests. TheObject.hasOwn(r, "lastTransitionAt")change (obligations.ts:490) stops a presentnullfrom being aged by the sender-suppliedinboundTimestamp; M4 fails thelastTransitionAt nullcase. A duplicateobligationId(or an unreadable record) keeps its receipt viaambiguousObligationIds/liveObligationIds— no cross-deletion. No path makes an aged debt skip its release.
Findings
[obligations.ts:402-407]The re-check is untested as a difference. Deleting the entireif (typeof nackCutoffMs === "number") { const owedSince = obligationLastTransitionMs(current); if (owedSince === null || owedSince >= nackCutoffMs) return false; }block leaves the suite green (mutation M2: 47/47 pass). Once the sweep holds the lock, the re-read cannot differ from the sweep's own read, and the sweep already pre-checksowedSince < nackCutoffbefore calling — so the property "a record rewritten to a fresh debt is kept" is guaranteed by the lock (which is tested), not by this re-check, even though the commit message credits the re-check. Harmless defence-in-depth, but it is an untested property claim; either add a test that fails without it or label it as belt-and-suspenders in the comment.[obligations.ts:232-234]withObligationLockthrowsnew Error("obligation store is busy")after the default 2 s lock budget, sowriteObligation/createObligation/transitionObligation/writeReceiptnow throw where they previously did not. Three call sites call these unguarded inside their function:ackObligation(index.ts:989),armDeadline(index.ts:1361) — reached fromretryObligationRead's timer callback (index.ts:930) — andreconcileObligation(index.ts:1515). In the timer callback an uncaught throw escapes the event loop; at 1515 it aborts the re-arm. Low severity (needs >2 s contention, i.e. a stalled holder), but it is a new failure mode, and I could not establish an upper bound on how long the sweep holds the lock (it is O(store) synchronous fs), so I cannot promise the 2 s budget is always enough.{ timeoutMs: 0 }+ skip/retry, or a guard at these sites, would remove it.[openclaw.plugin.json:44-47]obligationNackHoldMultipleis added with nodescription, unlike its siblingobligationRetentionDays. Minor.- The PR body/commit says it "restores … the index.ts call-site comment … state the age bound", but
index.tsis not in the diff (byte-identical toorigin/main); that comment andresolveObligationNackHoldDaysare already on main. What the manifest change actually contributes is declaring theobligationNackHoldMultiplekey thatindex.tsalready read — a real, good fix worth naming accurately.
Could not see / did not run: no launchd or darwin integration lane (host rule — production Flair is a launchd job in this user's domain); the CLI package suite (out of scope); and, as noted, no bound on the sweep's lock-hold time.
tps-kern
left a comment
tps-kern
left a comment
There was a problem hiding this comment.
Verdict: APPROVE on head 04ea7a83. Repo visibility checked before writing: tpsdev-ai/cli is PUBLIC (via repos/<owner>/<repo> .visibility); everything below describes behavior the public diff already discloses.
Focus verification
1. The re-check and the write are under the same lock as the sweep's read. A new withObligationLock(mailDir, agent, run) wraps every mutation path — writeObligation, createObligation (read+write inside), transitionObligation (read+write inside), markNackSent (read+write inside), abandonOwedNack (read+write inside), writeReceipt, and sweepTerminalObligations itself, which acquires the per-agent mail lock via acquireMailLockSync(resolve(mailDir, agent), { timeoutMs: 0 }) and holds it for the entire pass [obligations.ts:561-585]. The sweep calls abandonOwedNackLocked — the lock-free internal — inside that lock, so the sweep's read, the abandon's re-read, and the abandon's write are all atomic against any competing writer (which must acquire the same lock). The non-reentrant lock is pinned by "uses the mailbox lock for every obligation writer" (each write API throws "nested acquisition" when the lock is already held, and the file is left unchanged), and the sweep's non-blocking skip is pinned by "skips a locked store while a competing writer refreshes one record" — a real child process holds the lock and refreshes the record's lastTransitionAt + nackPending; the sweep's timeoutMs: 0 acquisition returns null, the sweep skips with a log, abandonedForNack is 0, and the refreshed debt survives with nackOwed still true.
2. The cutoff passed is the same one the sweep selected by. The sweep computes nackCutoff = nowMs - nackHoldDays * 24*60*60*1000 and passes it as the sixth argument to abandonOwedNackLocked [obligations.ts:653]. Inside, the re-check is owedSince === null || owedSince >= nackCutoffMs → return false [obligations.ts:393-396] — the same nackCutoff value, so a record rewritten to a fresh debt between the sweep's read and the abandon's re-read has a lastTransitionAt that is now ≥ the cutoff, and the release is refused. A concurrent re-owe cannot make the sweep clear a fresh debt.
3. No race-induced path now holds a record forever. The #528 defect this follow-up closes: a concurrent re-owe between the sweep's read and abandonOwedNack's write could clear nackPending on the fresh debt, dropping a nack still owed — the sender's obligation stayed alive forever because the nack was never sent. The re-check under the lock closes that: the fresh debt's nack is not cleared; it is eventually sent (the startup retry fires while nackOwed is true) or aged-abandoned on a later sweep (the fresh lastTransitionAt advances, and the next sweep after the window re-attempts). The unresolved-inbound + aged-nack path (the #528 reorder) abandons first, then falls through to normal terminal retention — deletable. The unageable-forever case (an unparseable or absent lastTransitionAt) is #528's deliberate "retained on principle" posture, now documented in the updated docstring ("Unparseable timestamps and failed release writes keep the debt") and pinned by five lastTransitionAt malformation rows plus the "failed release write" row — the debt and its receipt are kept, nackOwed stays true, and the retry fires every startup. That is not a silent forever-hold; it is a loud, pinned retention of a corrupt record whose timestamp cannot be trusted to age it.
What else the lock and the tightened shape checks cover
- Receipts for held or ambiguous obligations. A record held for its owed nack adds its
obligationIdtoliveObligationIds[obligations.ts:680-682], so the receipt loop'sif (live || ambiguousObligationIds.has(obligationId)) continuekeeps its receipt. A receipt shared by a held and a terminal record is kept (pinned: "keeps a receipt shared by a held record and a terminal record"). Two records claiming the sameobligationIdare ambiguous, and neither's receipt is deleted (pinned: "keeps an ambiguous receipt when both claimants are terminal"). A record whoseinboundIddiffers from its filename is unreadable + live, and its receipt is kept (pinned: "keeps a record whose inbound id differs from its filename"). A malformed or adversarial record cannot orphan another obligation's receipt. - The
obligationLastTransitionMsfix. A present-but-nulllastTransitionAtis now unageable (theObject.hasOwn(r, "lastTransitionAt")check distinguishes present from absent); an absent field falls back toinboundTimestamp. Pinned by "ages debt from inboundTimestamp when lastTransitionAt is absent" (abandoned + removed) vs the fivenull|123|false|{}|[]rows (kept). - The configurable hold.
obligationNackHoldMultiplein the plugin manifest, Ajv-validated (minimum 1; 0.5 rejected), wired throughresolveObligationNackHoldDays— pinned by the Ajv + configured-hold test.
Verification basis
- Worktree
~/work/review-534-kernat04ea7a83: agent + cli built, pluginnpm ci+npm run build, then the plugin's isolated launcher (node scripts/run-tests.mjs) with sandboxedHOMEandTMPDIR=/tmp. 339 pass / 0 fail across 20 files in 134 s — including the concurrent-writer-skip test (a real child process with the lock), the lock-nesting test (all write APIs refuse under a held lock), the ambiguous/malformed receipt tests, the five unageable-timestamp rows, and the cur-record-write EISDIR fault rows. - No process from my run remains. No CI status characterized.
4 participants
Closes #533.
What changed
In the
openclaw-tps-mailplugin's obligation store:obligationNackHoldMultiplevalues of at least 1.Tests
test/aged-nack-recheck-and-receipt.test.tsuses real temporary files:Verification
Results measured on 04ea7a8:
bun run build: passed.bun run lint:ci: passed.npm run build, tsc): passed.node scripts/changelog-fragments.mjs check: passed.Changelog:
.changelog/unreleased/fixed-533-aged-nack-recheck.md.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes