fix(cost): quarantine unreadable session logs instead of failing every poll

[rohanpoudel2]

Fixes #223

Problem

readSessionUsage has two failure exits and only one of them records that the session is bad:

  try {
    file = await open(path, "r");
  } catch (error) {
    if (isMissingFile(error)) return;
    throw error;                      // <-- nothing recorded
  }
  ...
      } catch (error) {
        session.unreadable = true;    // <-- quarantined
        session.pendingLine = [];
        session.pendingLineBytes = 0;
        throw error;
      }

The oversized-event exit sets session.unreadable, so the if (session.unreadable) return; guard skips that file on every later poll — one error, then the tracker moves on. The open() exit rethrows without recording anything, so the next poll reopens the same file and fails identically, for the life of the scan.

#readSessions amplifies it: a failure is only deferred to the included thread-tree filter when session.threadId !== null. An unopenable session never got a thread id, so if (session.threadId === null) throw error; fires and the error escapes the filter that exists precisely to ignore unrelated sessions. A root-owned rollout from one sudo codex run then fails every poll of every later scan.

stop() had the mirror-image problem:

    await this.refresh();
    if (this.#snapshot.usage !== null) return this.#snapshot;
    const cost = estimateScanCost(this.#options.model, fallbackUsage);

The unguarded refresh rejected, which discarded the caller's fallbackUsage — the completed turn's own authoritative usage — and reported a scan that had already succeeded as failed. Swallowing that rejection alone is not enough, because the non-null check then returns the pre-completion snapshot. In api.ts that stale snapshot is what onFinalize records through complete-scan --cost-json, and since no new onCost fires, maxCostUsd is never compared against the completed turn:

        onFinalize: async (usage) => {
          const snapshot = await tracker.stop(usage);
          throwIfAborted(signal, scanDir);   // onCost is what aborts; it never fired
          ...
          completionCost = snapshot.cost;    // the pre-completion figure

A scan whose completed turn exceeded --max-cost therefore completes, recorded below its limit.

The suite already pins the tolerant half of the first defect — "ignores oversized events from unrelated prior credential sessions". This PR makes the unopenable case behave the same way, and makes the completed turn reachable.

Change

src/cost.ts, +67/−5.

1. The three quarantine lines move into quarantineSession(session), called from both failure exits, so an open failure is handled like a parse failure.
2. That quarantine is classified. PERMANENT_ACCESS_ERROR_CODES — EACCES, EPERM, EISDIR, ELOOP, ENAMETOOLONG, ENOTDIR — retires a session on its first failure. Every other code is retried, up to MAX_SESSION_OPEN_ATTEMPTS (5) consecutive failures, after which the session is retired regardless of code. session.openFailures resets on any successful open.
3. stop() records whether its final refresh succeeded. When it did not, the completed turn's fallbackUsage is costed and wins whenever it charges more than the stale snapshot, which reports a fresh onCost.

isMissingFile handling is untouched: a file that vanished between the directory walk and the open is still skipped silently, with no quarantine. A successful refresh still returns the snapshot unconditionally, so precedence on the healthy path is unchanged.

Why not reject from stop() when the final refresh fails

That is the behaviour being removed, and it deserves scrutiny because weakening cost handling could weaken --max-cost. It does not:

• Enforcement is prospective and lives entirely in the polling path (start() → refresh() → onCost → abort). stop() runs after the turn is over; there is nothing left to abort and rejecting cannot recover spend that already happened.
• The error is still reported where it matters. Under a cost limit, polling already surfaced it through onError.
• Rejecting destroys authoritative data. fallbackUsage is the completed turn's own usage — better than the log-scraped snapshot, not worse.
• Routing it through onError would be self-defeating. In api.ts, onError aborts costAbortController and onFinalize calls throwIfAborted(signal, scanDir) immediately after stop(), so reporting from stop() would reconstruct the exact failure this fixes.
• It matches existing intent. api.ts already does await costTracker?.stop().catch(() => null) on the failure path.

Why not always prefer fallbackUsage when the refresh fails

Because #snapshot sums the scan thread and its delegated worker sessions, while fallbackUsage is the completed turn of the main thread alone. On a scan with workers the snapshot is legitimately the larger figure, so unconditionally preferring the fallback would trade a stale under-report for a structural one. Taking whichever charges more is conservative in both directions, and it is the only comparison added — a real, current snapshot still wins outright.

Why not quarantine every open failure

A process-wide shortage such as EMFILE or ENFILE clears on its own. Retiring a session on that first failure means every later refresh short-circuits at the session.unreadable guard, so the tracker never observes that session's usage again even after descriptors free up. That matters for enforcement, not just reporting: a delegated worker whose parent chain is not yet visible sits outside included, so its failure is swallowed by the thread-tree filter, and if it were retired there it would still be quarantined when the tree later grows to include it — under-counting a session the budget is supposed to cover.

Why not classify by errno alone

An allowlist on its own moves the defect rather than removing it. Any retryable code that never actually clears — EBUSY for a rollout another process holds open without sharing on Windows, EIO on failing media — would be reopened on every 100 ms poll for the life of the scan, which is exactly what this PR exists to stop, relocated to the codes nobody enumerated. So the allowlist is the fast path and the retry budget is the floor: known-permanent codes never spend a retry, and everything else is retired after five consecutive failures. The reset on success keeps two separate outages from adding up to a retirement.

Impact, stated plainly

• A rollout the process cannot open is reported once and then skipped, instead of failing every poll for the rest of the scan.
• A scan whose turn completed is no longer reported as failed because its session logs became unreadable.
• A completed turn that exceeds --max-cost is now enforced even when the final refresh fails. Before this change that scan completed with the pre-completion cost recorded; there is an api-level test below that resolves successfully without the fix and rejects with it.
• A momentary descriptor shortage no longer costs the tracker a session permanently, and a persistent failure of any code still stops after five attempts.
• Not addressed: #readSessions still rethrows when the failing session has no thread id yet, so under --max-cost the first such error still reaches onError and aborts. That is issue An unreadable Codex session log fails every cost poll for the rest of the scan, and can fail a scan that succeeded #223's stated expected behaviour ("reported once"), and changing it would weaken the path that enforces the limit.

Verification

Nine new tests: eight in tests-ts/cost.test.ts, one in tests-ts/api.test.ts. The POSIX permission fixtures use the repository's testPosix pattern and restore 0o600 in a finally so afterEach teardown can still delete them; the errno cases mock node:fs/promises's open for one path and count attempts, which is how EMFILE/EBUSY are reachable in a test at all.

The permanence test is deliberately order-independent: whichever file readdir yields first, the first refresh() rejects and the second must succeed.

Full suite: 747 pass / 5 skip / 0 fail (752 tests, 34 files).

With src/cost.ts reverted to this PR's base and all the tests kept: 740 pass / 5 skip / 7 fail.

The halves are independently load-bearing. Reverting one piece at a time, against tests-ts/cost.test.ts (23 tests):

Reverted	Result	Failing test
stop() fallback only	22 pass / 1 fail	"prefers the completed turn when the final refresh fails" (and, in api.test.ts, "enforces the cost limit on a completed turn the final refresh cannot read" — Expected promise that rejects / Received promise that resolved)
classification only (quarantine every open failure)	20 pass / 3 fail	"retries a session log after a transient open failure", "retires a session log that keeps failing to open", "counts open failures per outage rather than for the whole scan"
retry budget only (allowlist kept)	22 pass / 1 fail	"retires a session log that keeps failing to open"
the counter's reset only	22 pass / 1 fail	"counts open failures per outage rather than for the whole scan"

pnpm run types and pnpm run format are clean.

Deliberately out of scope

• sessionFiles has the same shape at directory level — a non-ENOENT readdir error has no per-directory state to quarantine and would also repeat every poll. It is outside this trigger and needs new state rather than reuse of the existing flag. Noted in An unreadable Codex session log fails every cost poll for the rest of the scan, and can fail a scan that succeeded #223.
• api.ts calls stop() twice on the failure path (onFinalize, then the catch). The second call re-reads the now-quarantined session and can recompute a lower cost than the fallback the first call adopted, so fail-scan --cost-json can record less than the limit-exceeded message states. That is pre-existing — before this PR the same path recorded the same lower figure, with a worse error attached — and making stop() terminal is a behavioural change to a call site this PR does not otherwise touch.

[rohanpoudel2]

Note on overlap: #177 and #198 also change src/cost.ts and tests-ts/cost.test.ts, but they address a different defect (coalescing overlapping cost refreshes, #31) in refresh/start, whereas this PR changes readSessionUsage's failure exits and the stop() fallback. I test-merged this branch onto both and each combination merges cleanly, so whichever lands first should not disturb the other.

[mldangelo-oai]

@codex review exact head fdc1be5

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: fdc1be55f4

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Security review completed. No security issues were found in this pull request.

Reviewed commit: fdc1be55f4

View security finding report

Only the user who started this review can view the report in Codex.

ℹ️ About Codex security reviews in GitHub

This is an experimental Codex feature. Security reviews are triggered when:

• You comment "@codex security review"
• A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review

Once complete, Codex will leave suggestions, or a comment if no findings are found.