feat: add isolated qpk-vnext N2 filesystem store#257
Conversation
Co-Authored-By: Codex <noreply@openai.com>
🤖 Codex PR Review🚫 Merge blocked: 2 serious issue(s) found in high-risk files 🚫 Blocking IssuesThese issues must be fixed before this PR can be merged: 1. 🔴 [CRITICAL] Security in
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cfa5022298
ℹ️ 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".
| handle.write(payload) | ||
| handle.flush() | ||
| os.fsync(handle.fileno()) | ||
| os.replace(temporary, path) |
There was a problem hiding this comment.
When two writers create the same identity key at the same time (for example the same key with a different computed_at, which N1 excludes from identity), both can pass the earlier path.exists() check; this os.replace then overwrites the file created by the other writer and returns created instead of failing as a conflict. That violates the write-once/conflict-fail-closed contract and can silently change persisted result metadata under concurrent orchestrators; the final install needs an exclusive-create pattern rather than a replacing rename.
Useful? React with 👍 / 👎.
| for path in base.rglob("*.json"): | ||
| try: | ||
| key = path.resolve().relative_to(self.root).as_posix() | ||
| if f"/{timing}/" in key: |
There was a problem hiding this comment.
When strategy_id or run_id equals a valid timing such as next_open, a next_close record's key still contains /next_open/ before the actual timing segment, so list_keys(..., timing="next_open") returns that next_close key. This mixes result buckets even for valid contracts; compare the fixed timing component of the relative path instead of substring-searching the whole key.
Useful? React with 👍 / 👎.
| def list_keys(self, *, domain: str, profile: str, timing: str) -> tuple[str, ...]: | ||
| """List exact selector matches; no implicit latest or legacy scan.""" | ||
| prefix = f"{NAMESPACE}/{domain}/{profile}/" | ||
| base = (self.root / NAMESPACE / domain / profile).resolve() |
There was a problem hiding this comment.
When domain or profile contains .., this resolved base path is only checked to stay under the store root, not under qpk-vnext/result/v2; for example domain="../../../legacy" makes list_keys walk root/legacy/... and return matching JSON paths. That breaks the isolated namespace/no-legacy-scan guarantee for invalid selector input, so the listing selectors should be validated as single key segments or the listed keys should be constrained to the namespace prefix.
Useful? React with 👍 / 👎.
| os.fsync(handle.fileno()) | ||
| os.replace(temporary, path) | ||
| temporary = None | ||
| return "created" |
There was a problem hiding this comment.
On POSIX filesystems, when the process or host crashes after this rename, fsyncing only the file data does not guarantee the new directory entry is durable. Because put() returns created immediately afterward for a persist_mode="durable" contract, callers can believe a result was safely committed even though the key may disappear after crash recovery; fsync the parent directory after the rename before returning success.
Useful? React with 👍 / 👎.
Co-Authored-By: Codex <noreply@openai.com>
|
Closure batch (head |
|
PERMANENT_FREEZE: closure exhausted; fresh N2 reslice design required for descriptor-relative containment and full ancestor durability. Frozen evidence retained. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 94c46a08f0
ℹ️ 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".
| path.parent.mkdir(parents=True, exist_ok=True) | ||
| self._sync_directory(path.parent) |
There was a problem hiding this comment.
When the store is first used for a new root/domain/profile, mkdir(parents=True) creates several directory entries but this only fsyncs the leaf path.parent; the entries for ancestors such as qpk-vnext, result, or v2 live in their own parent directories and can be lost on a POSIX crash even after put() returns created for persist_mode="durable". Fresh evidence: the updated code now fsyncs after the create-only link, but it still never fsyncs the parents whose directory entries were created by this mkdir call.
Useful? React with 👍 / 👎.
| if (len(parts) == 11 and key.startswith(prefix) | ||
| and parts[7] == timing and self._path_for_key(key) == path.resolve()): | ||
| found.append(key) |
There was a problem hiding this comment.
If any stray or corrupt *.json file exists under a selector path, this predicate returns it as a key as long as the path has 11 segments and the timing segment matches; it does not verify the N1 key shape/digest or decode the wire record. For example qpk-vnext/result/v2/us_equity/SOXL/bad!/run/next_open/oops/p1/notadigest.json is listed even though get() will fail, so callers using list_keys() can receive unusable non-contract keys from the isolated namespace.
Useful? React with 👍 / 👎.
| if existing == payload: | ||
| return "idempotent" |
There was a problem hiding this comment.
When two writers race, the loser can hit this FileExistsError path after the winner has linked the destination but before the winner reaches the directory fsync below; returning idempotent here lets a durable put() report success even though no caller has yet made the new directory entry crash-durable. This branch should fsync the parent (or otherwise wait for the winner's durable commit) before returning success.
Useful? React with 👍 / 👎.
| payload = self._bytes(contract) | ||
| path = self._path_for_key(contract.key) |
There was a problem hiding this comment.
Because put() accepts any ResultContract subclass, a subclass that overrides key can pass the N1 encode/decode validation here while causing the canonical payload to be written under a different path than the payload's own identity key. In that case put() returns created, but get(contract.key) fails because the on-disk wire decodes to a contract whose key does not match the path; derive the path from the decoded contract or explicitly compare checked.key to the requested key before writing.
Useful? React with 👍 / 👎.
1 participant
QPK-N2 isolated vNext local store
Fresh base:
origin/main@a06a99f74566ce60ae91fc39317cdfc16558e325(merged N1 only).This PR adds a minimal filesystem-only store for the new
qpk-vnext/result/v2namespace. It consumes only the merged N1ResultContract/wire/key contract and never reads legacy PerformanceStore, legacy indexes/codecs, or fallback paths.Validation
5 passed619 passed, 1 skippedNon-goals: N3 caller/orchestrator/exporter, legacy compatibility, network/S3, live/broker/account, merge automation.