Repository navigation
fix(session): serialize archive publication and roll back failed saves - #1334
Conversation
- Hold a checkout-keyed publication lock across archive writes, commits, remote pushes, and pull/merge for session push, migrate --push, and session pull; keep the short index lock inside it. - Stage transcript and metadata before replacing the pair, retain previous files as renameable backups, and restore or remove the pair when the atomic index write fails. Read/repair the index before installation so rebuild cannot adopt a partial save. - Preserve recovery backups and report their paths if rollback itself fails; document local lock waiting, retry behavior, and the lock ordering for maintainers. - Verify 135 focused tests plus real-CLI concurrent remote transactions, failed-index cleanup and retry without duplicates, and JSON migration/archive behavior; type checking, lint, and build pass.
|
No confident findings in the specified PR diff. The PR description satisfies the testing requirement: it records build, type-check, lint, targeted unit tests, and representative real built-CLI verification for concurrent publication and failed-save rollback. It also documents baseline failures and coverage deferred to CI. Review was read-only; I did not run, build, or install anything from the PR. |
SaulMoro
left a comment
SaulMoro
left a comment
There was a problem hiding this comment.
This was generated by AI during triage.
@nateEc Checked 32b2b0a4. Concurrent publication and caught save failures behave as intended. The built-CLI run of session-publication.test.ts and session-archive-lock.test.ts passed all 3 tests. Additional probes confirmed dead-owner lock recovery, JSON success/busy output, and preservation of archive bytes after ordinary write and rename failures.
- [P2 non-blocking] Recovery guidance after a partial rollback names backups already consumed by successful restores. In the injected metadata-restore failure, the transcript is restored and its backup no longer exists, but the error lists both backup paths. Track the backups that remain and name their live destinations. If both restores fail, the original bytes survive in backups, while the index still references missing live files.
- [P2 non-blocking] The ten-second wait covers the entire network publication. With a slow post-receive hook, both commands succeed on base, while this head safely refuses the second as busy. The behavior is documented and retryable, so it does not block this fix.
- [P3 nit] Reuse the existing
withFileLockhelper for publication to avoid another acquisition/release loop. Keep the index lock's ownership/re-entry handling.
Caught exceptions are covered by rollback; process death is outside that guarantee. With an existing valid index, a SIGKILL before index rename leaves an unindexed pair and a suffixed retry on both base and head. A later failed push can also reach the existing directory-wide Git-add fallback and publish retained recovery backups. That requires multiple failures and is optional hardening.
CI review reported no confident findings and accepted the testing record. We agree there is no blocking finding and the record is sufficient; the recovery guidance and contention behavior above are non-blocking qualifications. Current CI jobs passed. Local verification used macOS and local Git remotes; Windows and hosted-provider coverage were not rerun.
This PR owns session archive publication and rollback. #1280, and the branches stacked on it, own workflow publication and will need to resolve their overlapping session-command and dashboard-doc edits when they land.
Summary
Overlapping session pushes could race after releasing the archive index lock, and a failed index write left an unindexed transcript/metadata pair. Serialize each archive-writing command through Git commit/pull/push, including
migrate --pushandsession pull. Stage the pair, retain old files as renameable backups, and restore/remove it when the atomic index write fails.Type of Change
Test Plan
npx tsc --noEmitpassesnpm run lintpassesnpx vitest runpasses entirely (known baseline failures documented below)Added/updated tests for the change
npm run buildpassesnpx vitest run src/__tests__/session-sync.test.ts src/__tests__/session-sync-pull.test.ts src/__tests__/session-cmd.test.ts: 135 passed.Four rollback tests cover a new archive and an existing archive, with either the metadata rename or index rename failing. They preserve file bytes and retry without
_1duplicates. All four fail against clean main.Real built CLI:
session-publication.test.ts+session-archive-lock.test.tsthrough the E2E config: 3 passed. A bare remote's pre-receive gate holds the first push while a second command reaches the same archive; both exit 0, both sessions land remotely, and the checkout is clean. A Node filesystem fault makes the index rename fail, leaves no orphan pair, and the next push succeeds without a suffix. The existing JSON migration/copy/archive flow also passes. The two new E2E cases fail on main.Broad validation: combined type check, lint, build, and 10 critical real-CLI E2E cases passed. The full five-branch unit run took 614.07s: 499 files; 12,244 passed, 36 skipped, 7 failed. Five failures came from the recall acceptance harness reading the old OpenCode argv position; a one-line test-only adapter fix preserves the attribution assertions. All 221 OpenCode/recall tests pass on the final combined snapshot after that correction; runtime source did not change.
The remaining two failures are independently reproduced on clean
6c57c3b7with Apple Git 2.50.1:submodule-checkout.test.tscannot readhook.teamai-post-checkout.commandafter preserving empty legacy Claude/Codex settings (24 passed, 2 failed on clean main). The full-suite checkbox stays unchecked for these baseline failures.Local verification used macOS, Node 22.22.1, and local Git fixtures. GitLab/GitHub provider runs and additional live agent/Windows coverage were not run locally; remote CI cases may skip without credentials.
Related Issues
Related #1325: addresses concurrent publication and failed-save orphan files. The optional nested-wrapper/prototype-key cleanup is outside this fix.
Notes for Reviewers
Lock order is publication, then index. Network work holds the publication lock, never the short index lock; a contender waits up to ten seconds before an explicit busy/retry result. The lock is local to one machine; existing cross-machine merge/rebuild logic remains in use. Rollback failure keeps and names recovery backups. Much of the command diff is indentation of the existing publication bodies inside the lock. Bilingual user docs and the maintainer lock contract are updated.