Skip to content

Fix/1149 timeline prune active session - #1185

Open
MayurK-cmd wants to merge 2 commits into
Nano-Collective:mainfrom
MayurK-cmd:fix/1149-timeline-prune-active-session
Open

Fix/1149 timeline prune active session#1185
MayurK-cmd wants to merge 2 commits into
Nano-Collective:mainfrom
MayurK-cmd:fix/1149-timeline-prune-active-session

Conversation

@MayurK-cmd

Copy link
Copy Markdown

Description

Closes #1149

Adds a per-process lockfile at .nanocoder/timeline/<sessionId>/.lock so pruneStaleSessions no longer wipes a session directory that another in-flight process is still writing into.

A long-running session mid-tool-call used to be vulnerable: its session directory's mtimeMs lagged behind now - MAX_TIMELINE_SESSION_AGE_MS whenever no new entry was being captured, and pruneStaleSessions could fs.rm the entire directory, breaking the active tool call.

Pruning is now gated on a liveness probe of the lockfile:

  • Live PIDs (lock present, holder process is alive, purpose tag matches) → skip the prune.
  • Dead PIDs (process gone) → reap the lock, then remove the directory.
  • Malformed or wrong-purpose locks → reap the lock, then remove the directory.
  • No lockfile at all → treat as abandoned, remove the directory (preserves the existing "abandoned sessions" behaviour).

The session directory's mtime is also refreshed on every ensureDir via fs.utimes so an active session naturally bubbles to the top of the count cap and out of the age-based cutoff.

How it works

New module: source/services/timeline-lock.ts

Self-contained, mirrors daemon/lockfile.ts:

  • acquireTimelineLock(sessionDir, {pid, startedAt}) — creates a temporary lockfile using exclusive creation (O_EXCL), then atomically renames it into place. Returns false (does not throw) on contention so the chat never blocks.
  • releaseTimelineLock(sessionDir) — idempotent unlink; ENOENT is ignored.
  • isTimelineLockLive(sessionDir) — reads the lock, validates the purpose tag, probes the holder's PID with process.kill(pid, 0), and reaps stale locks as a side effect.
  • isProcessAlive(pid) is duplicated from daemon/lockfile.ts for now; a follow-up can lift it into a shared util.

The lock payload is {pid, startedAt, purpose: 'session-active'}. The purpose tag allows isTimelineLockLive to distinguish a real timeline lock from a random JSON file another tool may have dropped in the session directory.

TimelineManager changes

  • Acquires the lock on the first ensureDir call (best-effort: failure is logged, never thrown).
  • Idempotent: a second ensureDir does not flip lockHeld back to false by racing its own on-disk lockfile.
  • Refreshes the session directory's mtime on every ensureDir via fs.utimes so an active session bubbles to the top of the count cap and out of the age-based cutoff.
  • Exposes async dispose() that releases the lock and clears the lockHeld flag. Idempotent and safe to call on managers that never acquired.

pruneStaleSessions changes

Probes the lockfile for every stale entry. If isTimelineLockLive reports the lock as held by a live process, the entry is skipped. A dead, malformed, or wrong-purpose lock is reaped before fs.rm runs, so the directory leaves the timeline root in a clean state.

Why no lifecycle wiring?

The lock remains useful without an explicit dispose() call:

  • A process exiting does not automatically remove the lockfile from the filesystem.
  • isTimelineLockLive() detects lockfiles whose recorded PID is no longer alive and reaps them during pruning.
  • This means an abandoned lockfile cannot permanently prevent cleanup.
  • The regression in [Bug] timeline-manager.ts mtime-based prune can delete a session that's mid-tool-call #1149 is fixed as long as active sessions hold the lock for the duration of the chat.
  • dispose() provides prompt cleanup when lifecycle wiring is available, but it is not required for correctness.

A follow-up can plumb dispose() into the App / AcpSession teardown so production code releases the lock promptly. This is out of scope for this fix.

Type of Change

  • Bug fix
  • New feature
  • Breaking change
  • Documentation update

Changeset

  • Added a changeset (pnpm changeset) describing this change for the changelog

.changeset/fix-1149-timeline-prune-lock.md is a patch changeset naming @nanocollective/nanocoder and explaining the lockfile and live-skip behaviour.

Validated locally with node scripts/validate-changesets.js (73 changesets checked, all resolve).

Docs-only or internal chores need no changeset (or run pnpm changeset --empty to note that intentionally).

Testing

Automated Tests

  • Regression coverage added for the stale-session pruning bug
  • Tests cover both success and error scenarios
  • All existing tests pass (pnpm test:all completes successfully)
  • Type checks pass
  • Lint passes

Verified locally:

  • pnpm test:ava source/services/timeline-lock.spec.ts10/10 passed
  • pnpm test:ava source/services/timeline-manager.spec.ts21/21 passed (17 existing + 4 new)
  • pnpm test:types → clean
  • pnpm test:lint → 517 files, 0 fixes

New tests cover:

In timeline-lock.spec.ts:

  • live lock acquire + live probe
  • double-acquire returns false
  • idempotent release
  • stale-PID reap (dead PID)
  • malformed-payload reap
  • wrong-purpose-tag reap
  • no-file path
  • isProcessAlive for current process and invalid PIDs
  • strict acquire path on a stale lock

In timeline-manager.spec.ts:

  • TimelineManager prunes abandoned sessions whose lockfile points to a dead process — proves the lock reaper and fs.rm cooperate.
  • TimelineManager skips pruning a stale-but-live session (issue #1149) — proves the regression this PR fixes.
  • TimelineManager.dispose releases the session lock — proves the lifecycle.
  • TimelineManager.dispose does not throw when the lock was never acquired — proves the idempotency.

Success and error scenarios are both covered: the live-skip and dead-reap tests exercise the two paths in pruneStaleSessions, while the malformed, wrong-purpose, and missing-lock tests cover the stale outcomes for isTimelineLockLive.

Manual Testing

  • Tested with Ollama
  • Tested with OpenRouter
  • Tested with OpenAI-compatible API
  • Tested MCP integration (if applicable)

This change is filesystem-only and provider-agnostic. The lockfile lives under .nanocoder/timeline/<sessionId>/.lock and is read/written via node:fs/promises with no LLM or network calls.

Manual end-to-end testing against a real provider was not done as part of this PR; the change was validated via the unit/integration tests listed above.

To manually reproduce the original bug and verify the fix, the steps from the issue are: start a long-running session, then in a second shell run a flood of nanocoder invocations that each create a new session so the count cap is hit, and observe that the long-running session's directory is no longer removed.

Checklist

  • If this was for an open issue, I was assigned to it
  • Code follows project style guidelines
  • Self-review completed
  • Documentation updated (if needed)
  • No breaking changes (or clearly documented)
  • Appropriate logging added using structured logging (see CONTRIBUTING.md#logging)

No documentation update included in this PR — the lockfile is an internal implementation detail of TimelineManager; the user-visible behaviour (pruning) is unchanged for the normal case. Happy to add a docs page if reviewers want one.

No breaking changes: the public surface of TimelineManager only grew by the new dispose() method. The constructor signature and all existing method signatures remain unchanged, and the file layout under .nanocoder/timeline/<sessionId>/ is unchanged. The tryAcquireSessionLock, touchSessionDir, and lock-related fields remain private.

Logging: the lock acquisition and release paths use the existing logWarning helper from source/utils/message-queue.ts with structured context (sessionId, error), matching the project's logging conventions. No additional getLogger().info calls were added because the happy path is silent, matching the rest of the timeline subsystem; only the failure path logs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:tui Terminal UI

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] timeline-manager.ts mtime-based prune can delete a session that's mid-tool-call

1 participant