feat: bind TQQQ snapshots to successor calendar - #196
Conversation
Co-Authored-By: Codex <noreply@openai.com>
![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: daa6a4318c
ℹ️ 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".
| manifest = { | ||
| "contract_version": CONTRACT_VERSION, | ||
| "symbols": list(SYMBOLS), | ||
| "requested_lower_bound": REQUESTED_LOWER_BOUND, | ||
| "price_field": PRICE_FIELD, | ||
| "plugin": PLUGIN, | ||
| "mode": MODE, | ||
| "size": 0, | ||
| "row_count": len(prices), | ||
| "prices_sha256": _sha256(staging / "prices.csv"), | ||
| } |
There was a problem hiding this comment.
The digest-addressed artifact is derived only from the price bytes and generic metadata; expected_calendar_sha256 and the successor session never enter the manifest. Consequently, two accepted calendar artifacts with identical session dates but different close/early-close metadata produce the same manifest hash and publication directory, and the resulting snapshot can later be verified against either calendar by changing the external arguments. Include the trusted calendar digest and successor identity in the hashed artifact so its claimed provenance is immutable.
Useful? React with 👍 / 👎.
| if (pd.Timestamp(successor) - pd.Timestamp(dates[-2])).days not in (1, 3): | ||
| _invalid("calendar successor is not adjacent") |
There was a problem hiding this comment.
Restricting adjacent sessions to calendar-day gaps of 1 or 3 rejects valid XNYS calendars around holidays. For example, a Friday completed session followed by Tuesday after a Monday market holiday has a four-day gap, while a Wednesday-to-Friday sequence around a Thursday holiday has a two-day gap, so calendar-bound materialization fails despite the supplied calendar hash and session sequence being valid.
Useful? React with 👍 / 👎.
| except OSError as exc: | ||
| if attempt == 0 and _owned_directory_cleanup(destination, identity): | ||
| continue | ||
| raise SnapshotValidationError("immutable publication failed") from exc |
There was a problem hiding this comment.
If an OSError occurs during the second publication attempt, this branch raises without calling _owned_directory_cleanup. That leaves an incomplete digest-named directory under the output root, and every subsequent retry fails immediately with immutable output already exists, requiring manual repair after a transient second write or fsync failure.
Useful? React with 👍 / 👎.
| output_root.mkdir(parents=True, exist_ok=True) | ||
| destination = output_root / manifest_sha256 | ||
| for attempt in range(2): | ||
| try: | ||
| os.mkdir(destination, 0o700) |
There was a problem hiding this comment.
When the requested output root already exists as a symlink to a directory, mkdir(..., exist_ok=True) accepts it and the subsequent destination creation follows that symlink. In a shared writable parent, another user can therefore pre-create the root symlink and redirect publication to an arbitrary directory accessible to the pipeline process; verification still succeeds because the digest child itself is not a symlink. Reject the root symlink or anchor creation to a verified directory descriptor before publishing.
Useful? React with 👍 / 👎.
| for name in OUTPUT_FILENAMES: | ||
| _write_exclusive(destination / name, payloads[name]) | ||
| _write_exclusive(destination / "COMPLETE", f"{manifest_sha256}\n".encode("ascii")) | ||
| result = verify_tqqq_r1_snapshot(destination, expected_manifest_sha256=manifest_sha256, calendar_path=calendar_path, expected_calendar_sha256=expected_calendar_sha256) |
There was a problem hiding this comment.
Although _write_exclusive fsyncs each file, publishing COMPLETE and returning without fsyncing the destination directory or its parent does not durably commit the new directory entries. After a power loss or kernel crash, the function may already have reported success while the digest directory, one of its members, or the completion marker disappears on recovery, defeating the marker's immutable-completion guarantee.
Useful? React with 👍 / 👎.
🤖 Codex PR Review✅ Merge allowed: 1 finding(s) reported but none are blocking ℹ️ Other Findings1. 🟡 [MEDIUM] Logic in
|
Co-Authored-By: Codex <noreply@openai.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 465d82ec93
ℹ️ 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".
| destination = output_root / manifest_sha256 | ||
| for attempt in range(2): | ||
| try: | ||
| os.mkdir(destination, 0o700) |
There was a problem hiding this comment.
When the output root is in a concurrently writable location, an attacker can rename it and install a symlink after the checks at lines 417–424; this pathname-based os.mkdir and every subsequent member write then follow the replacement root. The final identity check covers only the redirected digest directory, so publication can report success outside the requested root. Fresh evidence after the earlier symlink fix is that destination creation still resolves the root by pathname rather than through a verified directory descriptor.
Useful? React with 👍 / 👎.
| if output_root.exists(): | ||
| if not output_root.is_dir(): | ||
| _invalid("calendar-bound output root must be a directory") | ||
| else: | ||
| os.mkdir(output_root, 0o700) | ||
| _fsync_directory(output_root.parent) |
There was a problem hiding this comment.
If a first call creates output_root but its parent fsync raises, the root remains visible and a retry enters this existing-root branch. That retry can publish successfully while never retrying the parent fsync; syncing the root itself does not durably commit the root's directory entry in its parent, so a crash can still remove the supposedly completed publication. The new fsync calls therefore leave this failure-and-retry path uncovered.
Useful? React with 👍 / 👎.
| os.mkdir(destination, 0o700) | ||
| except FileExistsError as exc: | ||
| raise SnapshotValidationError(f"immutable output already exists: {destination}") from exc | ||
| created = os.lstat(destination) |
There was a problem hiding this comment.
If os.mkdir(destination) succeeds but the immediately following lstat raises—for example due to a transient filesystem error—the lookup is outside the cleanup-protected try, leaving an empty digest-named directory behind. Every later publication attempt then fails with immutable output already exists, so this remaining failure point still defeats automatic recovery and requires manual deletion.
Useful? React with 👍 / 👎.
| if observed > _utc_now(): | ||
| _invalid("future E4 endpoint observation") |
There was a problem hiding this comment.
When validation runs after the successor session has closed, an endpoint_observed_at_utc at or after successor_close_utc passes as long as it is not later than the wall clock. Such evidence cannot establish that prices correctly end at completed_last_session, because the successor is already complete and should now be represented; this allows a stale snapshot cutoff to pass the E4 freshness check. Parse the bound successor close and require the observation to precede it.
Useful? React with 👍 / 👎.
1 participant
Scope
Verification
PYTHONPATH=src python3 -m pytest -q tests/test_tqqq_r1_snapshot.py(24 passed)python3 -m ruff check src/us_equity_snapshot_pipelines/tqqq_r1_snapshot.py tests/test_tqqq_r1_snapshot.pypython3 -m py_compile src/us_equity_snapshot_pipelines/tqqq_r1_snapshot.pypython3 -m compileall -q srcNo provider, price-data, E3/E4 generation, merge, or live action.