fix(daemon): break the session-finalize retry loop that pinned the CPU - #744
Conversation
📝 WalkthroughWalkthroughThe PR adds ledger-scoped anti-entropy leases with platform-specific implementations. Detection now coordinates ownership and aggregates queue rejections. Session finalization now preserves failed work, synthesizes metadata, reports push failures, and converges after recovery. ChangesAgentwork reliability
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Manager
participant LedgerLease
participant HandlerScan
participant WorkQueue
participant SessionFinalizer
Manager->>LedgerLease: acquire ledger-scoped lease
LedgerLease-->>Manager: grant or deny ownership
Manager->>HandlerScan: scan handlers when ownership is granted
HandlerScan->>WorkQueue: enqueue detected work
WorkQueue-->>Manager: return cycle rejection count
Manager->>SessionFinalizer: process upload-only session
SessionFinalizer-->>Manager: report success or push failure
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
f97ea4b to
7ce450a
Compare
![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 (3)
internal/daemon/agentwork/session_finalize_convergence_test.go (1)
140-145: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the repeated skip guards into one helper.
The same
testing.Short()andexec.LookPath("git")guards appear in all three tests. A single helper removes the duplication.♻️ Proposed helper
func requireGit(t *testing.T) { t.Helper() if testing.Short() { t.Skip("short: real git operations") } if _, err := exec.LookPath("git"); err != nil { t.Skip("git not available") } }Then replace each guard block:
- if testing.Short() { - t.Skip("short: real git operations") - } - if _, err := exec.LookPath("git"); err != nil { - t.Skip("git not available") - } + requireGit(t)Also applies to: 191-196
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/daemon/agentwork/session_finalize_convergence_test.go` around lines 140 - 145, Extract the duplicated testing.Short and exec.LookPath("git") skip logic into a requireGit helper in the test file, marking it with t.Helper and preserving the existing skip messages. Replace the guard blocks in all three tests with calls to requireGit.internal/daemon/agentwork/ledger_lease_test.go (1)
9-41: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRename this test; it does not verify denial.
TestLedgerLease_SecondHolderIsDeniedcallsfirst.Release()at line 29 before acquiringsecondat line 33. The test therefore only verifies that a lease is reacquirable after an explicit release, not that a second holder is denied. The final assertion message,"lease not reacquirable after release", confirms this is the actual intent.The NOTE at lines 25-28 states a second acquire "legitimately succeeds" in-process — per the Linux
flock(2)man page, a same-process acquire through a different file descriptor "may be denied by a lock that the calling process has already placed via another file descriptor," which contradicts this claim. Since the test never actually attempts a concurrent second acquire (it releases first), this comment does not describe anything the test exercises and should be removed or corrected.Rename the test to reflect what it verifies, for example
TestLedgerLease_ReacquireAfterRelease.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/daemon/agentwork/ledger_lease_test.go` around lines 9 - 41, Rename TestLedgerLease_SecondHolderIsDenied to TestLedgerLease_ReacquireAfterRelease, since it only verifies reacquisition after first.Release(). Remove the misleading NOTE about same-process flock behavior, as the test does not perform a concurrent second acquire; preserve the existing release and reacquisition assertions.internal/daemon/agentwork/ledger_lease.go (1)
42-53: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the canonical ledger cache helper.
paths.LedgerSessionCacheBasealready returns the ledger.sageox/cachedirectory. If customledgerPathvalues must remain supported, add a path-based helper ininternal/pathsand use it here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/daemon/agentwork/ledger_lease.go` around lines 42 - 53, Update ledgerLeasePath to use the canonical paths.LedgerSessionCacheBase helper for resolving the ledger .sageox/cache directory instead of rebuilding the path locally. If that helper only supports the default ledger location, add a path-based equivalent in internal/paths that preserves custom ledgerPath support, then use it from ledgerLeasePath while retaining directory creation and error handling.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/daemon/agentwork/intent_test.go`:
- Around line 132-241: Extract the duplicated helper-process startup and
readiness polling from TestOwnsLedgerAntiEntropy_DeniedWhileAnotherProcessHolds
and TestOwnsLedgerAntiEntropy_TakesOverAfterHolderExits into a shared
startLeaseHolder(t *testing.T, ledger string) *exec.Cmd helper. Preserve the
existing command, environment, timeout, readiness deadline, polling, and failure
behavior; leave each test’s distinct cleanup and process-kill handling in its
own body.
In `@internal/daemon/agentwork/ledger_lease.go`:
- Around line 76-86: Update ledgerLease.Release so a platformReleaseLedgerLease
failure does not restore held to 1 after the underlying file descriptor has been
closed; leave the lease marked released and return the wrapped error. Preserve
the existing idempotent nil/already-released behavior and successful release
path.
---
Nitpick comments:
In `@internal/daemon/agentwork/ledger_lease_test.go`:
- Around line 9-41: Rename TestLedgerLease_SecondHolderIsDenied to
TestLedgerLease_ReacquireAfterRelease, since it only verifies reacquisition
after first.Release(). Remove the misleading NOTE about same-process flock
behavior, as the test does not perform a concurrent second acquire; preserve the
existing release and reacquisition assertions.
In `@internal/daemon/agentwork/ledger_lease.go`:
- Around line 42-53: Update ledgerLeasePath to use the canonical
paths.LedgerSessionCacheBase helper for resolving the ledger .sageox/cache
directory instead of rebuilding the path locally. If that helper only supports
the default ledger location, add a path-based equivalent in internal/paths that
preserves custom ledgerPath support, then use it from ledgerLeasePath while
retaining directory creation and error handling.
In `@internal/daemon/agentwork/session_finalize_convergence_test.go`:
- Around line 140-145: Extract the duplicated testing.Short and
exec.LookPath("git") skip logic into a requireGit helper in the test file,
marking it with t.Helper and preserving the existing skip messages. Replace the
guard blocks in all three tests with calls to requireGit.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 67e033fb-1e24-4a6e-86a3-b6229d5a1940
📒 Files selected for processing (6)
internal/daemon/agentwork/intent_test.gointernal/daemon/agentwork/ledger_lease.gointernal/daemon/agentwork/ledger_lease_test.gointernal/daemon/agentwork/ledger_lease_unix.gointernal/daemon/agentwork/ledger_lease_windows.gointernal/daemon/agentwork/session_finalize_convergence_test.go
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/daemon/agentwork/session_finalize.go (1)
1544-1626: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winScope
hasStagedChangesto the session's own path, not the whole index.
hasStagedChangesrunsgit diff --cached --name-onlyagainst the entire repository index, not justrelDirfor the session being processed.git add --sparse relDir/only stages this session's files, but the code has nogit reseton a failed commit elsewhere in the retry loop. If an earlierWorkItemfor a different session staged content viagit addand then itsgit commitfailed for a reason other than "nothing to commit" (leaving that content staged), the next session processed in the same run will seestaged=truebecause of the unrelated leftover content, and will commit it under the wrong session's message ("finalize session %s").Scope the check to
relDirso it answers "did THIS session'sgit addproduce a diff", not "is anything staged anywhere in the repo".🐛 Proposed fix
- staged, err := h.hasStagedChanges(ledgerPath) + staged, err := h.hasStagedChanges(ledgerPath, relDir) if err != nil { h.logger.Warn("could not inspect staged changes", "err", err) return false }-func (h *SessionFinalizeHandler) hasStagedChanges(repoPath string) (bool, error) { - cmd := exec.Command("git", "-C", repoPath, "diff", "--cached", "--name-only") +func (h *SessionFinalizeHandler) hasStagedChanges(repoPath, relDir string) (bool, error) { + cmd := exec.Command("git", "-C", repoPath, "diff", "--cached", "--name-only", "--", relDir) cmd.Env = append(os.Environ(), "GIT_TERMINAL_PROMPT=0")Also applies to: 1717-1728
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/daemon/agentwork/session_finalize.go` around lines 1544 - 1626, Update the hasStagedChanges call in gitCommitAndPush to scope its staged-file inspection to relDir, ensuring it only detects changes added for the current session rather than unrelated repository index content. Adjust the helper signature or invocation as needed while preserving the existing error handling and commit flow.
🧹 Nitpick comments (3)
internal/daemon/agentwork/ledger_lease_test.go (2)
9-41: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRename this test to match its assertion.
The test releases
firstbefore it acquiressecond. It verifies lease reacquisition after release. It does not verify that a second holder is denied.Rename it to
TestLedgerLease_ReacquirableAfterRelease. Keep the cross-process denial assertion ininternal/daemon/agentwork/intent_test.go.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/daemon/agentwork/ledger_lease_test.go` around lines 9 - 41, Rename the test function from TestLedgerLease_SecondHolderIsDenied to TestLedgerLease_ReacquirableAfterRelease so its name matches the release-and-reacquire behavior it asserts. Leave the existing lease setup and assertions unchanged, and keep cross-process denial coverage in intent_test.go.
62-70: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest lease-evaluation failure separately.
An empty
ledgerPathreturns beforeacquireLedgerLeaseruns. This test does not verify the documented fail-open behavior when lease acquisition returns a filesystem error.Add a test that makes lease acquisition fail and asserts that
ownsLedgerAntiEntropy()still returnstrue. As per coding guidelines, “Tests that execute Git commands must setcmd.Dirto a temporary directory and must never modify the real repository's Git identity.” The applicable requirement here is to “test error paths.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/daemon/agentwork/ledger_lease_test.go` around lines 62 - 70, Add a separate test alongside TestOwnsLedgerAntiEntropy_FailsOpenWithoutLedger that preserves a valid ledgerPath, forces acquireLedgerLease to return a filesystem error, and asserts ownsLedgerAntiEntropy() returns true. Configure any Git-command runner used by the test with cmd.Dir set to a temporary directory and avoid changing the real repository’s Git identity.Source: Coding guidelines
internal/daemon/agentwork/session_finalize.go (1)
1492-1505: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
errors.Is(err, os.ErrNotExist)instead ofos.IsNotExist(err).Line 1498 checks
os.Stat(metaPath)withos.IsNotExist(err).os.Staterrors are not wrapped here, so this works functionally, but the repo has a documented preference forerrors.Is(err, os.ErrNotExist)for consistency and forward-compatibility with wrapped errors.Based on learnings, "In this repo, when checking for file-not-found errors in Go, prefer
errors.Is(err, os.ErrNotExist)overos.IsNotExist(err)." As per coding guidelines, "Useerrors.Is()anderrors.As()for error inspection and wrap errors with contextual information."♻️ Proposed fix
- if _, err := os.Stat(metaPath); os.IsNotExist(err) { + if _, err := os.Stat(metaPath); errors.Is(err, os.ErrNotExist) { meta := h.loadOrSynthesizeMeta(payload.SessionDir, sessionName)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/daemon/agentwork/session_finalize.go` around lines 1492 - 1505, Update the meta.json existence check in the session-finalization flow to use errors.Is(err, os.ErrNotExist) instead of os.IsNotExist(err), preserving the existing synthesis and write behavior when the file is missing. Ensure the required errors package is imported.Sources: Coding guidelines, Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/daemon/agentwork/manager.go`:
- Around line 395-401: Update the lease cleanup flow around m.ledgerLease and
ledgerLease.Release so m.ledgerLease is cleared only after Release succeeds;
retain the reference when release fails, preserving the existing warning log so
later calls can retry the release.
- Around line 295-300: Update the disabled-configuration path in the Manager’s
ledger anti-entropy flow to call releaseLedgerAntiEntropy() before returning.
Preserve the existing ownsLedgerAntiEntropy() guard and detection behavior when
the configuration remains enabled.
In `@internal/daemon/agentwork/session_finalize_upload_test.go`:
- Around line 489-556: Update
TestProcessResult_UploadOnly_AlreadyCommitted_PrunesCache to pre-seed a valid
meta.json in ledgerDir before the initial git commit, using content that
loadOrSynthesizeMeta will produce; keep meta.json absent from cacheDir so
stageSessionInLedger does not overwrite the committed file. Ensure the resulting
ProcessResult staging diff is empty and the test exercises the hasStagedChanges
false/"nothing to commit" path while preserving the existing success and
cache-pruned assertions.
In `@internal/daemon/agentwork/session_finalize.go`:
- Around line 1682-1715: Update loadOrSynthesizeMeta to retain the raw session
ID by capturing stored.Meta.SessionID when reading raw.jsonl and passing it as
the startMinted argument to session.ResolveOrMintSessionID. Preserve minting
behavior only when no stored session ID is available, and keep the existing
metadata synthesis flow unchanged.
---
Outside diff comments:
In `@internal/daemon/agentwork/session_finalize.go`:
- Around line 1544-1626: Update the hasStagedChanges call in gitCommitAndPush to
scope its staged-file inspection to relDir, ensuring it only detects changes
added for the current session rather than unrelated repository index content.
Adjust the helper signature or invocation as needed while preserving the
existing error handling and commit flow.
---
Nitpick comments:
In `@internal/daemon/agentwork/ledger_lease_test.go`:
- Around line 9-41: Rename the test function from
TestLedgerLease_SecondHolderIsDenied to TestLedgerLease_ReacquirableAfterRelease
so its name matches the release-and-reacquire behavior it asserts. Leave the
existing lease setup and assertions unchanged, and keep cross-process denial
coverage in intent_test.go.
- Around line 62-70: Add a separate test alongside
TestOwnsLedgerAntiEntropy_FailsOpenWithoutLedger that preserves a valid
ledgerPath, forces acquireLedgerLease to return a filesystem error, and asserts
ownsLedgerAntiEntropy() returns true. Configure any Git-command runner used by
the test with cmd.Dir set to a temporary directory and avoid changing the real
repository’s Git identity.
In `@internal/daemon/agentwork/session_finalize.go`:
- Around line 1492-1505: Update the meta.json existence check in the
session-finalization flow to use errors.Is(err, os.ErrNotExist) instead of
os.IsNotExist(err), preserving the existing synthesis and write behavior when
the file is missing. Ensure the required errors package is imported.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: ab4c89f8-4143-46ac-a34f-b017bfffa527
📒 Files selected for processing (10)
internal/daemon/agentwork/intent_test.gointernal/daemon/agentwork/ledger_lease.gointernal/daemon/agentwork/ledger_lease_test.gointernal/daemon/agentwork/ledger_lease_unix.gointernal/daemon/agentwork/ledger_lease_windows.gointernal/daemon/agentwork/manager.gointernal/daemon/agentwork/queue.gointernal/daemon/agentwork/session_finalize.gointernal/daemon/agentwork/session_finalize_convergence_test.gointernal/daemon/agentwork/session_finalize_upload_test.go
🚧 Files skipped from review as they are similar to previous changes (5)
- internal/daemon/agentwork/ledger_lease_windows.go
- internal/daemon/agentwork/ledger_lease.go
- internal/daemon/agentwork/session_finalize_convergence_test.go
- internal/daemon/agentwork/ledger_lease_unix.go
- internal/daemon/agentwork/intent_test.go
A session whose files already match HEAD produces a zero-delta stage, so
`git commit` exits 1 with "nothing to commit". gitCommitAndPush read that
as failure and returned false, which made processUploadOnly skip the cache
prune. The cache dir surviving is exactly what Detect() keys on, so the
same session was re-queued five minutes later. Forever.
Measured on one machine before the fix (sageox-monorepo ledger, one day):
898 "agent work complete" vs 897 "git commit failed"
43,996 "enqueue skipped: queue full" (1,039,714 lifetime)
299 distinct sessions re-detected 114 times each
5,396 cache dirs vs 2,320 tracked sessions
511MB daemon log, 2.88M lines
four daemons spiking to 100-176% CPU on every 5-minute doctor tick
Four defects, each independently sufficient to keep the loop alive:
1. Zero-delta commits read as failure. Fixed by asking git what is staged
(`git diff --cached --name-only`) instead of parsing the commit message
— the wording varies with the rest of the tree ("working tree clean" vs
"untracked files present"), so one stray file would have resurrected
the loop. Message matching survives only as a fallback for the
concurrent-committer race, which is real when two daemons share a
ledger.
2. Failures reported as successes. processUploadOnly returned nil even
when nothing was committed or pushed, so the manager logged
"agent work complete status=success duration=29µs" and no counter
moved. It now returns an error, and the manager records a failure.
3. The detector's completion marker was unreliable. Detect() treated
sessions/<name>/meta.json as proof a session had landed, but nothing
on the upload path created one unless it already existed, and the
write is not tied to a successful push. Completion is now the cache
prune, which happens only after a verified commit and push.
processUploadOnly also synthesizes a meta.json when absent, carrying
identity and provenance only — never a fabricated title or summary.
4. Duplicate daemons multiplied the work. Two processes for one repo each
ran the full scan and each enforced its own 60-invocations/hour ceiling.
A per-ledger flock now serializes anti-entropy while leaving the cheap
per-repo work untouched. It fails open: a ledger that stops self-healing
is worse than one scanned twice.
Also: a saturated queue logged one WARN per rejected item. Now one INFO
summary per detection cycle.
Why the suite stayed green through all of this:
- Detect() and ProcessResult() were tested as separate units. Nothing
asserted the system CONVERGED, and the loop is only visible across
cycles. TestAntiEntropy_ConvergesAfterTransientPushFailure runs three
cycles and is the test whose absence let this run for months.
- Fixture bias. Every upload fixture wrote meta.json by hand, so the
tests always satisfied a detector that production never did.
- TestProcessResult_UploadOnly_CachePreservedOnPushFail asserted
ProcessResult returns nil on push failure — it encoded the silent
success this bug rode on. Its real invariant (no data loss) is
unchanged and still enforced.
- TestDetect_FullyFinalizedInCache_AlreadyPushed asserted that a cache
dir plus a ledger meta.json means "already pushed, skip" — the exact
state ~3,000 stranded sessions were in, and the rule that made them
invisible to anti-entropy.
New tests assert intent, not mechanics, and each was verified to fail
against the pre-fix behavior: convergence across cycles, meta.json
synthesis without fabricated content, failure surfacing through the real
handler into daemon status, cross-process lease exclusion (driven by a
re-executed helper process, because flock is per-file-description and an
in-process test would pass while the real case stayed broken), and
one-log-line-per-cycle under a saturated queue.
Refs: ox-zmjc.18, ox-zmjc.19, ox-zmjc.20, ox-4qvn, ox-2gki
Co-Authored-By: SageOx <ox@sageox.ai>
Greptile SummaryThis change improves session-finalization convergence, metadata recovery, upload outcome handling, and per-ledger coordination. A focused Git reproduction verified that the pointer-rewrite commit can include an unrelated session file that was already staged, publishing it under the current session's pointerization commit. This should not merge until that commit is scoped to the intended pointer paths or uses an isolated index. Confidence Score: 2/5
 T-Rex LogsWhat T-Rex did
|
…g meta.json Rebased onto main, which landed GH #710 (locked read-modify-write for meta.json). That fix and this one collided directly: #710 exists because rebuilding meta.json from a fresh builder strips every field the writer does not set, and the ledger auto-resolves sessions/ conflicts to the local side, so a stripped copy wins the next rebase and erases those fields from shared history. This branch's meta.json seeding did exactly that. Both writes now go through lfs.MutateSessionMeta and seed ONLY when the file is absent, where there is nothing to strip. Review fixes: - synthesizeMeta minted a fresh ses_ ID on every call by passing empty arguments to ResolveOrMintSessionID, rotating the identity of a recording that already had one. That function's own contract exists to make this impossible ("a call site can never accidentally skip past a preserved or header-carried ID"). Now reads the header-carried ID, with session.ReadHeaderSessionID as the crash-safe fallback. - ledgerLease.Release restored held=1 on a failed release so the caller could retry. Both platform backends close the file before returning that error, and closing the fd is what drops the kernel's flock — so the retry could only fail with EBADF, and in the meantime the process claimed a lease the kernel had already handed on. It now reports the error without reasserting ownership. Manager.releaseLedgerAntiEntropy clears its reference unconditionally to match, and no-ops when unheld. - detectAndEnqueue returned while still holding the lease when the worker was disabled, blocking every other daemon on that ledger until this process exited. It now hands the lease back first. - TestProcessResult_UploadOnly_AlreadyCommitted_PrunesCache no longer exercised the zero-delta path it guards: with no meta.json in the ledger copy, synthesizeMeta produced a genuinely new file, so the stage was non-empty and the ordinary commit path ran. Both assertions still passed, so a regression in the no-op handling would have gone unnoticed. Fixed by pre-seeding meta.json into the ledger copy only, and — more importantly — the test now asserts HEAD is unchanged, which is only true on the branch under test. Verified: removing the pre-seed now fails with "a commit was created ... does not test what it claims" instead of passing silently. - Extracted the duplicated subprocess spawn-and-wait into startLeaseHolder. Refs: ox-zmjc.18, ox-zmjc.19, ox-zmjc.20, ox-4qvn, ox-2gki Co-Authored-By: SageOx <ox@sageox.ai>
7ce450a to
c7f2f68
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/daemon/agentwork/session_finalize.go (1)
1266-1367: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winScore cleanup can permanently delete data that was never persisted.
session.ReadSageoxScore(agentID)at Line 1271 discards its error (score, _ = session.ReadSageoxScore(agentID)). If the read fails for any reason other than "no such file" (corrupt file, transient I/O error),scorestaysnil, sonext.SageoxScore*fields are never populated in this write.
session.CleanupSageoxScore(agentID)at Line 1364-1366 then runs unconditionally wheneveragentID != "", regardless of whether the read succeeded. On a real (non-not-found) read error, this deletes the score file after its data was silently dropped frommeta.json, with no retry path — the score is lost permanently.Track the read error and skip cleanup unless the read succeeded or the file was genuinely absent.
🛠️ Proposed fix to avoid deleting an unread score
var score *session.ScoreFile + scoreReadOK := true if agentID != "" { - score, _ = session.ReadSageoxScore(agentID) + var scoreErr error + score, scoreErr = session.ReadSageoxScore(agentID) + if scoreErr != nil && !errors.Is(scoreErr, os.ErrNotExist) { + h.logger.Warn("failed to read sageox score, preserving for retry", + "agent_id", agentID, "err", scoreErr) + scoreReadOK = false + } }- if agentID != "" { + if agentID != "" && scoreReadOK { _ = session.CleanupSageoxScore(agentID) }Based on learnings, this repo prefers
errors.Is(err, os.ErrNotExist)overos.IsNotExist(err)for file-not-found checks.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/daemon/agentwork/session_finalize.go` around lines 1266 - 1367, Update the Sageox score handling around ReadSageoxScore and CleanupSageoxScore to retain the read error instead of discarding it. Treat nil errors and errors matching os.ErrNotExist as safe to clean up, but skip CleanupSageoxScore for all other read errors so unread score data is preserved; use errors.Is for the not-found check and keep existing score population behavior unchanged.Source: Learnings
🧹 Nitpick comments (1)
internal/daemon/agentwork/session_finalize_git_test.go (1)
8-8: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCapture stderr for easier failure diagnosis.
gitOutputusescmd.Output()and reports onlyerron failure (Line 199-201), so at.Fatalfmessage shows just something like "exit status 128" without the git error text. The neighboringrunGitCmdincludes command output in its failure message. Capture stderr here too, so a failing convergence or no-op-commit test in CI is diagnosable without a local repro.🛠️ Proposed fix to include stderr in test failures
func gitOutput(t *testing.T, dir string, args ...string) string { t.Helper() cmd := exec.Command("git", args...) cmd.Dir = dir - out, err := cmd.Output() + var stderr bytes.Buffer + cmd.Stderr = &stderr + out, err := cmd.Output() if err != nil { - t.Fatalf("git %v in %s failed: %s", args, dir, err) + t.Fatalf("git %v in %s failed: %s\n%s", args, dir, err, stderr.String()) } return strings.TrimSpace(string(out)) }Requires adding the
bytesimport.Also applies to: 191-203
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/daemon/agentwork/session_finalize_git_test.go` at line 8, Update the gitOutput helper to capture command stderr by adding the bytes import and wiring a buffer to the command before execution. When the command fails, include the captured git error output alongside the existing error in the t.Fatalf message, matching runGitCmd’s diagnostic behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/daemon/agentwork/session_finalize.go`:
- Around line 1628-1661: The staged-changes check currently inspects the entire
Git index, allowing leftover files from another session to be committed under
the current session’s message. Update hasStagedChanges to accept relDir and pass
it as the pathspec to git diff --cached, then update both call sites in session
finalization to provide the current session directory so commit decisions are
scoped to that session.
- Around line 1813-1823: Update the git invocation used by runGit to set
LC_ALL=C and LANG=C before executing Git, ensuring isNothingToCommit receives
stable English error text for its existing message checks.
---
Outside diff comments:
In `@internal/daemon/agentwork/session_finalize.go`:
- Around line 1266-1367: Update the Sageox score handling around ReadSageoxScore
and CleanupSageoxScore to retain the read error instead of discarding it. Treat
nil errors and errors matching os.ErrNotExist as safe to clean up, but skip
CleanupSageoxScore for all other read errors so unread score data is preserved;
use errors.Is for the not-found check and keep existing score population
behavior unchanged.
---
Nitpick comments:
In `@internal/daemon/agentwork/session_finalize_git_test.go`:
- Line 8: Update the gitOutput helper to capture command stderr by adding the
bytes import and wiring a buffer to the command before execution. When the
command fails, include the captured git error output alongside the existing
error in the t.Fatalf message, matching runGitCmd’s diagnostic behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: d8afd0d2-f825-4090-bfb7-a0c567b722c1
📒 Files selected for processing (11)
internal/daemon/agentwork/intent_test.gointernal/daemon/agentwork/ledger_lease.gointernal/daemon/agentwork/ledger_lease_test.gointernal/daemon/agentwork/ledger_lease_unix.gointernal/daemon/agentwork/ledger_lease_windows.gointernal/daemon/agentwork/manager.gointernal/daemon/agentwork/queue.gointernal/daemon/agentwork/session_finalize.gointernal/daemon/agentwork/session_finalize_convergence_test.gointernal/daemon/agentwork/session_finalize_git_test.gointernal/daemon/agentwork/session_finalize_upload_test.go
🚧 Files skipped from review as they are similar to previous changes (9)
- internal/daemon/agentwork/ledger_lease_windows.go
- internal/daemon/agentwork/ledger_lease_unix.go
- internal/daemon/agentwork/ledger_lease.go
- internal/daemon/agentwork/ledger_lease_test.go
- internal/daemon/agentwork/session_finalize_upload_test.go
- internal/daemon/agentwork/session_finalize_convergence_test.go
- internal/daemon/agentwork/queue.go
- internal/daemon/agentwork/manager.go
- internal/daemon/agentwork/intent_test.go
Two more review findings, both real. The git index is shared across every session a process finalizes. hasStagedChanges ran `git diff --cached --name-only` over the WHOLE index, so if an earlier finalize staged another session's files and then failed to commit for any reason other than an empty stage, this session would see those files, take the ordinary commit path, and fold them into a commit titled "finalize session <this one>". Nothing is lost — the other session's next cycle finds nothing staged and pushes — but the history attributes files to the wrong session. Now scoped to the session's own path. TestGitCommitAndPush_IgnoresOtherSessionsStagedFiles reproduces it: with the pathspec removed the commit swallows the other session's file and empties its stage; with it, both stay untouched. runGit also inherited the host locale while isNothingToCommit matches git's English wording. That match is only the fallback for the concurrent-committer race, but under a translated locale it would silently never fire, so runGit now pins LC_ALL=C / LANG=C. Refs: ox-zmjc.18, ox-zmjc.19, ox-zmjc.20, ox-4qvn, ox-2gki Co-Authored-By: SageOx <ox@sageox.ai>
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Scoping hasStagedChanges closed half the hole. `git commit -m` writes the WHOLE index, so when the target session does have staged changes, any files another session left staged after a failed finalize are still committed under this session's "finalize session <name>" message and pushed. The check decides WHETHER to commit; it never constrained WHAT got committed. The previous test passed only because it exercised the zero-delta path, where no commit runs at all. TestGitCommitAndPush_CommitExcludesOtherSessions WhenStaged covers the case that was missing — target session with real staged changes plus a stray staged file from another session — and fails without the pathspec with "OxSTRAY's files rode along in OxBOTH's commit". Refs: ox-zmjc.18, ox-zmjc.19, ox-zmjc.20, ox-4qvn, ox-2gki Co-Authored-By: SageOx <ox@sageox.ai>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/daemon/agentwork/session_finalize_convergence_test.go (1)
281-292: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the boolean result of
gitCommitAndPush.
gitCommitAndPushreturnsbool, wheretrueindicates success. Capture the result and fail when it isfalse; the current file-state assertions can still pass after a push failure.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/daemon/agentwork/session_finalize_convergence_test.go` around lines 281 - 292, Capture the boolean returned by newGitBackedHandler().gitCommitAndPush in the session finalization test and fail the test when it is false, before the existing committed and staged file-state assertions. Keep those assertions unchanged to continue validating repository state after a successful operation.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@internal/daemon/agentwork/session_finalize_convergence_test.go`:
- Around line 281-292: Capture the boolean returned by
newGitBackedHandler().gitCommitAndPush in the session finalization test and fail
the test when it is false, before the existing committed and staged file-state
assertions. Keep those assertions unchanged to continue validating repository
state after a successful operation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 4ce7b5a4-d0fb-42fc-9628-c419709861ec
📒 Files selected for processing (2)
internal/daemon/agentwork/session_finalize.gointernal/daemon/agentwork/session_finalize_convergence_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/daemon/agentwork/session_finalize.go
1 participant
What was happening
Four
oxdaemons were spiking to 100–176% CPU on every 5-minute doctor tick, indefinitely. The session-finalize anti-entropy loop could not converge: it re-did the same work on the same sessions forever, and reported every failure as a success.Measured on one machine before the fix (sageox-monorepo ledger, single day):
agent work completegit commit failedenqueue skipped: queue fullRoot cause
A session whose files already match
HEADproduces a zero-delta stage, sogit commitexits 1 withnothing to commit.gitCommitAndPushread that as failure and returnedfalse, soprocessUploadOnlyskipped the cache prune — and the surviving cache dir is exactly whatDetect()keys on.flowchart LR D["Detect: session in cache"] --> S["stage: copy cache to sessions/"] S --> C["git commit"] C -->|"zero delta, exit 1"| F["read as FAILURE"] F --> P["cache prune SKIPPED"] P -->|"5 minutes later"| D F --> L["logs status=success"]Verified on disk for one session: all five files present in
git ls-files,git statusclean, cache dir still there, nometa.jsonanywhere.The four defects
Zero-delta commits read as failure. Fixed by asking git what is staged (
git diff --cached --name-only) rather than parsing the commit message — the wording varies with the rest of the tree (working tree cleanvsuntracked files present), so a single stray file would have resurrected the loop. Message matching survives only as a fallback for the concurrent-committer race, which is real when two daemons share a ledger.Failures reported as successes.
processUploadOnlyreturnednileven when nothing was committed or pushed, so the manager loggedagent work complete status=success duration=29µsand no counter ever moved. It now returns an error and the manager records a failure.The detector's completion marker was unreliable.
Detect()treatedsessions/<name>/meta.jsonas proof a session had landed, but nothing on the upload path created one unless it already existed, and the write is not tied to a successful push. Completion is now the cache prune, which happens only after a verified commit and push.processUploadOnlyalso synthesizes ameta.jsonwhen absent — identity and provenance only, never a fabricated title or summary.Duplicate daemons multiplied the work. Two processes for one repo each ran the full scan and each enforced its own 60-invocations/hour ceiling. A per-ledger
flocknow serializes anti-entropy while leaving cheap per-repo work untouched. It fails open: a ledger that stops self-healing is worse than one scanned twice.Also: a saturated queue logged one
WARNper rejected item. Now oneINFOsummary per detection cycle.Why the suite stayed green through all of this
This is the part worth reviewing closely.
Detect()andProcessResult()were tested as separate units. Nothing asserted the system converged, and the loop is only visible across cycles.meta.jsonby hand, so the tests always satisfied a detector that production never did.TestProcessResult_UploadOnly_CachePreservedOnPushFailassertedProcessResultreturnsnilon push failure — the silent success this bug rode on. Its real invariant (no data loss) is unchanged and still enforced.TestDetect_FullyFinalizedInCache_AlreadyPushedasserted that a cache dir plus a ledgermeta.jsonmeans "already pushed, skip" — the exact state ~3,000 stranded sessions were in, and the rule that made them invisible to anti-entropy. Retargeted, with the reasoning recorded in the test.New tests — each verified to fail against the pre-fix behavior
TestAntiEntropy_ConvergesAfterTransientPushFailuresession_finalize.go, confirmed FAILTestProcessResult_UploadOnly_AlreadyCommitted_PrunesCacheTestProcessUploadOnly_WritesMetaWhenAbsentmeta.jsonsynthesized, with no fabricated title/summaryTestManager_UnpushableSession_SurfacesAsFailureStats.Failuresandstatus=failed, driven through the real handlerStats.Successes = 1, want 0TestOwnsLedgerAntiEntropy_DeniedWhileAnotherProcessHoldsTestDetectAndEnqueue_FullQueueLogsOncePerCycleTwo notes on test design, since both were mistakes I made first and corrected:
flockis per-open-file-description, so two acquires inside one process both succeed. It now re-executes the test binary as a lease holder.The convergence test also caught a regression I was about to introduce: writing
meta.jsonbefore the push satisfied the detector, which would have made an unpushed session stop being retried. That is what drove defect 3's redesign.Verification
go test ./internal/daemon/...— all four packages passgo vetclean;golangci-lintreports 8errcheckissues, all pre-existing, none in changed linescmd/ox(TestLoadBearingCommentPresent_LocalRecall,TestDiscoverTeamContextWithFallback_RefreshesExistingLocalCopy,TestConsultRoutes_NoDriftWithSkill) are unrelated and pre-existing — they pass in isolation and fail only under whole-package parallel runs on shared XDG state. Related: ox-y7n8.Not fixed here
Backfilling the ~3,000 already-stranded sessions is left to the daemon's own anti-entropy: each gets one more pass with the fixed binary, writes its
meta.json, commits, and prunes. No separate migration.Refs: ox-zmjc.18, ox-zmjc.19, ox-zmjc.20, ox-4qvn, ox-2gki
Summary by CodeRabbit