Skip to content

fix(cost): report an inaccessible session log once, not on every poll - #970

Open
feiiiiii5 wants to merge 1 commit into
openai:mainfrom
feiiiiii5:fix/cost-quarantine-inaccessible-session-log
Open

feiiiiii5 wants to merge 1 commit into
openai:mainfrom
feiiiiii5:fix/cost-quarantine-inaccessible-session-log

Conversation

@feiiiiii5

Copy link
Copy Markdown

Summary

An inaccessible session log under $CODEX_HOME/sessions fails on every cost poll for the rest of the scan, and can fail a scan that already completed.

readSessionUsage has two failure exits and only one of them records that the session is bad. A log it cannot parse sets session.unreadable, so the guard at the top of the function skips that file on later polls: one error, then the tracker moves on. A log it cannot open records nothing, so the next poll opens the same file and fails identically every COST_POLL_INTERVAL_MS.

The failure also escapes the thread-tree filter in #readSessions. A deferral is filtered by included only when the thread id is known, and an unopenable log never produced one, so if (session.threadId === null) throw error; rethrows instead of deferring — for a file that can never be attributed to this scan.

Because stop() awaits refresh() unguarded, the final poll can reject after the turn already completed, discarding the caller's fallbackUsage and reporting a successful scan as failed.

Reproduced on main at 70d5b2ed with a real chmod 0o000 rollout file: four onError reports within 300 ms, and stop() rejecting with EACCES: permission denied, open '.../sessions/2026/07/26/rollout-prior-thread.jsonl' from src/cost.ts:191.

Closes #223. #224 proposed a fix for this and was closed by its author on 2026-09-01 with no maintainer decision recorded; this is an independent implementation against current main, and it differs in the respect described under Changes.

Changes

  • readSessionUsage now quarantines the session when open() fails with an access refusal the process cannot clear by retrying (EACCES, EPERM), mirroring the existing parse-failure exit. The error is still thrown, so it is still reported — once.
  • Both failure exits now share a quarantineSession() helper. The parse-failure path is unchanged in behavior.
  • Every other open failure keeps the current retry-and-report behavior. A process-wide shortage such as EMFILE clears on its own, and retiring a session for it would stop observing usage that could still be read, which would weaken the --max-cost enforcement the tracker exists for. No attempt counter or other new limit is introduced.

Not changed, deliberately: stop() still awaits refresh() unguarded, so a failure that first appears exactly at the final refresh would still discard fallbackUsage. With inaccessible logs quarantined, the reported failure mode no longer reaches that path. Hardening stop() against unrelated refresh failures is a separate question and is left out of this change.

Testing

From sdk/typescript on macOS arm64, node 24.19.0, bun 1.3.14, pnpm 11.11.0:

  • bun test --timeout 30000 tests-ts/cost.test.ts -t inaccessible at 70d5b2ed without the src/cost.ts change: 2 failexpect(received).toHaveLength(expected) / Expected length: 1 / Received length: 4, and stop() rejecting with EACCES: permission denied, open '/…/rollout-prior-thread.jsonl' at async stop (src/cost.ts:191:16).
  • The same command with the change: 2 pass.
  • bun test --timeout 30000 tests-ts/cost.test.ts: 74 pass, 0 fail (72 before the two new tests).
  • Mutation check: deleting only the new quarantineSession(session) call from the open-failure path makes both new tests fail again; restoring it makes them pass. So the tests pin this change rather than the surrounding code.
  • bun test --timeout 30000 tests-ts/cost-context.test.ts tests-ts/cost-model.property.test.ts tests-ts/cli-show-cost.test.ts: 24 pass, 0 fail.
  • pnpm run types rc=0, pnpm run lint rc=0, pnpm run format rc=0 (All matched files use Prettier code style!).
  • bun test --timeout 30000 tests-ts/api.test.ts: 54 pass, 4 skip, 109 fail, identical with and without this change — the same three counts measured at base and at head. Those failures are ModuleNotFoundError: No module named 'workbench_scan_usage' from the generated _bundled_plugin payload, which I could not build here: pnpm run build:plugin stops at plugins/codex-security/native/prebuilt/COPYRIGHT-library.html, and preparing that payload needs the pinned Rust toolchain. For the same reason pnpm run test was not run locally; the focused module suites above were run instead.

The two new tests are gated test.skipIf(process.platform === "win32" || process.getuid?.() === 0), following the existing permission fixture in tests-ts/security-policy-helper.test.ts:203, because the fixture is a POSIX permission bit. Both restore the mode in a finally block so the shared temporary-directory cleanup can remove the fixture.

Risk and rollout

No public CLI or SDK surface changes, and no new limit, flag, or configuration. Behavior changes only for a session log that cannot be opened: it is reported once and then skipped, instead of being reported on every poll for the life of the scan. A quarantined session is not re-read if it later becomes accessible — that is the contract the parse-failure path already has. Missing files (ENOENT) are unchanged, and transient open errors keep retrying as before. No release, migration, or rollout ordering impact.

Public disclosure review

  • No customer, partner, prospect, or user identities, data, or identifying details are included.
  • No credentials, personal data, private source, scan findings, or nonpublic links or tickets are included.
  • I reviewed the branch name, title, description, commits, changes, comments, logs, screenshots, attachments, and links for public disclosure.

readSessionUsage quarantines a log it cannot parse, so the guard at the top
of the function skips it on later polls, but a log it cannot open records
nothing. The next poll opens the same file and fails identically for the life
of the scan, and because an unopenable log never produced a thread id, the
failure also escapes the thread-tree filter in #readSessions. stop() awaits
refresh() unguarded, so the final poll can reject after the turn completed and
discard the caller's usage.

Quarantine the session on an access refusal the process cannot clear by
retrying, mirroring the parse-failure exit. Other open failures keep retrying:
a process-wide shortage such as EMFILE clears on its own, and retiring a
session for it would stop observing usage that could still be read.
@github-actions github-actions Bot added the bug Something isn't working label Sep 18, 2026
@feiiiiii5

Copy link
Copy Markdown
Author

Flagging an overlap I should have caught before opening this.

Staff PR #465fix(cost): enforce verifiable scan spending limits, open since 2026-08-15 — already covers this root cause. Its sdk/typescript/src/cost.ts diff adds the same predicate and the same helper this PR introduces:

function isSessionAccessDenied(error: unknown): boolean {
  return (
    isRecord(error) && (error["code"] === "EACCES" || error["code"] === "EPERM")
  );
}

and calls quarantineSession(session, error) at both sites this PR touches: the open() failure inside readSessionUsage (if (session.threadId === null && isSessionAccessDenied(error))), and the unreadable loop in #readSessions (else if (isSessionAccessDenied(error))). It also reworks session.unreadable from a boolean into { error } and adds requireReadableSessions / readFailures, which looks like it reaches the stop() interaction I listed under "Not changed, deliberately". I have not traced that path end to end, so treat that last part as unverified.

To be precise about the current state: main as of writing has no quarantineSession, no isSessionAccessDenied and no EACCES handling in sdk/typescript/src/cost.ts, so #223 is still unfixed and this PR is not stale today.

If #465 lands first, though, this one is redundant and should be closed rather than rebased. I have not closed it myself because #465 is still open, so the fix is not on main yet. Happy to close on a maintainer's word, or to narrow this to whatever #465 leaves uncovered if that is more useful than closing it.

The note in the description about #224 stands: it was closed by its author on 2026-09-01 with no maintainer decision recorded.

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

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

An unreadable Codex session log fails every cost poll for the rest of the scan, and can fail a scan that succeeded

1 participant