Repository navigation
Conversation
MarcZX10
left a comment
There was a problem hiding this comment.
Reviewed exact head b42c09562c218caa6e999205e43a6c08274b7ced. I found a blocking lifetime race in the async completion queue.
[P1] sceHttp2WaitAsync must not retain a deque reference across the condition-variable wait when deletion can erase it. At Export.cpp:349-350, queue refers directly to the mapped deque and the wait releases g_completionsMutex. sceHttp2DeleteRequest can then erase that entry at lines 163-164. A detached operation may subsequently run CompleteAsync, recreate the map entry, and notify; the waiter wakes and evaluates available against the destroyed deque reference (or accesses it at queue.front()), which is a use-after-free. A finite timeout can also wake it after deletion. Separately, the worker can publish a new completion after deletion, so erasing the existing queue does not actually drop pending completions as the PR description says.
Please make request deletion, in-flight operations, and waiters share a stable per-request completion state (including a defined cancelled/deleted wake result), and ensure no completion can be enqueued after that state is retired. Add a regression that blocks a waiter, deletes the request while an operation is still in flight, then releases/completes the operation and verifies there is no hang, stale completion, or invalid access; run it under ASan/TSan where available.
I have not run this PR's tests locally yet; existing PR checks are policy-only for these changed paths and skip product builds. This request is based on the lock and lifetime interleaving above, not on an unrun test claim.
|
Tested with Palworld (PPSA23126) on Linux (CachyOS, GCC 16.2.1) on a local stack that also carries the HTTP/1.x transport, the |
|
@edubox Thanks for finishing these. A few things before this can merge:
|
|
Other open pull requests touch the same code. Merging one will break the other, or both implement the same thing. If this one builds on them, list them in Depends on.
Some comparisons could not be completed because a pull request conflicts with the base branch. Review these shared files manually before concluding that there is no overlap.
|
|
Addressed the completion lifetime and error-handling review at
Verification:
|
|
Independent Linux follow-up on exact A minimal local fixture correction changes that invalid-context case to check exception delivery. It also uses Exact #11630 plus this two-line correction ( Both patches, source pins and negative/positive logs, SHA256 |
MarcZX10
left a comment
There was a problem hiding this comment.
Reviewed current head 2ca67c36eb082641d502227da49d08ca193c9c6b. The completion-lifetime and exception-propagation changes address my earlier request, but the guest_http2 fixture still expects sceHttp2WaitAsync to return a numeric result for a request created by sceHttp2Init(1, 1, ...) without initializing an SSL context. Transport.cpp requires a usable SSL context and throws sceHttp2: SSL context or connection unavailable; the worker now correctly carries that exception to WaitAsync, so this fixture aborts rather than receiving the expected completion. An independent Linux run on this exact head reports the same failure. Please update this case to assert exception delivery, or provide a valid SSL context if a numeric completion is intended.
I built guest_http2_tests locally. ctest -R '^guest_http2$' aborts before reaching the assertion because this Xeon E5-1620 v2 lacks required CPU extensions; this is not a local reproduction of the fixture failure. git diff --check passes. Local conventions report the existing 1200-character debt entry length warning; hosted conventions passed on this head. Product build jobs are skipped by path policy.
2ca67c3 to
1e42382
Compare
|
Pushed
Validation: |
MarcZX10
left a comment
There was a problem hiding this comment.
Reviewed exact head 1e42382d8bcf4e1d1612395577bc5f78b5fd21e5. The updated guest_http2 case now uses the transport's declared-length check to exercise exception delivery, and changing AsyncFailure to Mode::Reset removes its dependence on redirect support. Those address the prior fixture findings.
[P1] Serialize event delivery with request retirement. CompleteAsync checks state->retired and enqueues the completion while holding state->mutex, then releases the lock at line 69 and triggers the user event at line 71. sceHttp2DeleteRequest can retire and erase the request in that gap. The worker then triggers an event after deletion even though the debt entry says later completions are dropped without triggering one. Please make retirement and event delivery mutually ordered (and add a regression for deletion after enqueue but before trigger); the existing stalled-worker deletion test only covers completion arriving after retirement.
[P2] Coordinate the overlapping test/debt edits. The current merge-base check against open #11575, #11586, #11611, and #11617 reports content conflicts in GuestHttp2Transport.cpp (and, for the first three, docs/dev/debt/libSceHttp2.md). CONTRIBUTING.md says to agree with overlapping PR authors on order and declare a prerequisite with Depends on when needed. Please coordinate the order and update the checklist/debt text so the combined landing is explicit. The pair with #11616 merges cleanly.
The updated convention and label checks pass, and the report check passed. The product build jobs are skipped by path policy; I have not independently rerun the reported Release/ASan/TSan tests on this head.
|
Thanks, the request-owned state fixes the lifetime problem, and the zero-length POST in guest_http2 makes JJenkx's case deterministic: it now fails on the body-length mismatch before any connect, so it no longer depends on whether something listens on localhost:443. Three things are left:
|
|
Pushed
Local verification on this head: P2 remains unresolved. Against the current heads ( |
|
A title that reaches this path over HTTP/2: MXGP 26 (PPSA30839 01.003.000). Linux, RTX 4070 Ti (NVIDIA 615.71.09), GCC 15.2, Release libs, warm shader cache, 600 s run limit. Its first network calls stop on three HTTP/2 gaps in a row:
For the stack, the only conflicts with this PR were the AI-assisted: yes (Claude Code) |
|
Follow-up to the MXGP run above: with #11611 in place of #11575 (main abed035 + #11617 + #11611 + this PR), the asynchronous HTTP/1.x request reaches the host lookup. MXGP then exits at 7 s with AI-assisted: yes (Claude Code) |
54fe144 to
20b7a97
Compare
|
Rebased onto Local verification on
P2 is still unresolved because I cannot verify an agreement with the overlapping authors: AI-assisted: yes |
|
@edubox For #11617 (mine, stacked on #11616): either order is fine with me. The conflicts between it and this PR are small. In AI-assisted: yes (Claude Code) |
|
Re-checked head Completion lifetime / event ordering (P1). Fixture assertions. Overlap (P2). The checklist now records the textual overlap and the stacks: #11575 -> #11586 -> #11611 and #11616 -> #11617, keeping both sides for Verification on
Product jobs remain skipped by path policy, so this is the focused local evidence, not a title run. |
20b7a97 to
eba7e4a
Compare
|
Rebased onto current Local verification on
The previous head's review items stand as fixed: |
|
Rebased automatically onto current |
3484886 to
9d7aaab
Compare
|
Rebased automatically onto current |
|
@edubox here's a second title that reaches this code, Crimson Desert (PPSA25286 01.000.568). It uses an asynchronous form this PR still refuses. On On this PR's head
So it uses the asynchronous calls without a kqueue and waits for each one with Everything else this title imports from libSceHttp2 is the template/request setup: If the kqueue-less form is in scope for this PR, I can rerun Crimson Desert on a new head; otherwise it can be a follow-up. In my sandbox I'd expect it to stop next at the DNS lookup, like MXGP. AI-assisted: yes (Claude Code) |
|
Rebased automatically onto current |
3519324 to
f364585
Compare
|
Rebased onto current Resolutions:
On this head: |
|
Hi edubox, thanks for the rebase onto main. One thing from putting
At Please pass the host in the fourth slot again ( |
|
Fixed the async fixtures' listener binding: the five calls that need Tested at AI-assisted: yes. |
|
@edubox a Windows run of the new head
AI-assisted: yes (Claude Code) |
✅ AMD GPU test: passedPR head
Unstable on main (1, these also failed in recent main runs on this machine; not counted against this PR)Failing on main too (4, not counted against this PR)Validation IDs already present on main (8 IDs in 469 tests)Skipped (7)ReproduceAutomated build and test on a contributor's AMD machine; it does not replace upstream CI. A new comment is posted for each new push. Full logs. |
|
The hosted AI-assisted: yes. |
|
The I merged current Checks on the merged head:
The push retriggers |
Apply review request from 2026-11-05: resolve the GuestHttp2Transport Mode enum and libSceHttp2 debt-doc conflicts.
✅ AMD GPU test: passedPR head
Unstable on main (1, these also failed in recent main runs on this machine; not counted against this PR)Failing on main too (4, not counted against this PR)Validation IDs already present on main (4 IDs in 550 tests)Skipped (7)ReproduceAutomated build and test on a contributor's AMD machine; it does not replace upstream CI. A new comment is posted for each new push. Full logs. |
|
Independent Linux qualification of this PR head Built the actual HTTP2 module and selected fixtures, then ran 13/13 HTTP2 cases on main and13/13 with this PR, zero skips. The production NID-patched HTTP2 module was frozen into the same full-main SDK/relinker bank; that module is the only changed native library. Actual title check: PPSA16064, Minecraft DungeonsII01.000.000, fresh private saves/caches in each arm, no controller input:
The latter refusal is the exact provider's Linux7.2.11, GCC16.2.1 Release, Ryzen9 7950X, RX6650XT/RADV26.2.6. Each complete title harness ran through the shared FIFO, with frozen banks/decoded inputs unchanged. Original assets were bound read-only; converted-module outputs used private writable overlays. All mounts released and udmabuf64MiB restored, no harness errors. An initial staging-only readonly-output-identity failure is retained and excluded from native results. Commands, full build/test/JUnit logs, native reports/logs, module hashes and reviewed captures, SHA-256 |
|
Follow-up ready PR #16231 targets the already-completed zero-timeout case, depending on this PR. A private PPSA16064 diagnostic found request6 with completed=1 and retired=0 at the unconditional refusal. The fix checks readiness under the existing mutex and still refuses an empty zero wait; it adds no pending-wait timeout policy. The new real loopback send/read regression fails this parent and passes with the change; all14 focused cases pass. The title clears that wait and next stops on unsupported authentication at41.6s. Its25s readbacks are black, so no visual/gameplay unblock is claimed. Source/commands/logs/captures and the exact parent/source pins are in the follow-up description. The final commit fe20311 is the new change; your commits/authorship are retained. AI-assisted: yes. |
|
Thanks. Confirming the split: #16231 carries the zero-timeout follow-up and this PR stays at |
…letions # Conflicts: # core/libs/tests/GuestHttp2Transport.cpp # docs/dev/debt/libSceHttp2.md
|
Merged
Focused |
WhatAn independent title measurement for this PR on Windows: PPSA23126 on TestedWindows 10 (10.0.19045), MinGW GCC 15.2, NVIDIA GeForce RTX 4080 SUPER, libraries and relinker built fresh from the tree under test. One title, runs set up for 600 s.
AI-assisted (Claude Code). |
…letions # Conflicts: # core/libs/prx/libSceHttp2/Export.cpp
|
Merged current The only conflict was Focused tests: |
What
sceHttp2SendRequestAsync,sceHttp2ReadDataAsyncandsceHttp2WaitAsyncthrew. This completes them over the existing transport:sceHttp2SendRequestAsynccopies the body, runsSendon a worker and queues anHttp2AsyncResultwhoseresultis the transport's result, then triggers the kqueue user event (equeue at 0, event id at 8, user data at 16, the layout inferred from PPSA17221).sceHttp2ReadDataAsyncdoes the same forReadand reports the bytes read inresult.sceHttp2WaitAsyncwaits for the oldest completion. A nulltimeoutwaits indefinitely;*timeout == 0throws because whether zero blocks is unknown (PPSA02195 polls with a zero timeout and aborts the request on an immediate timeout, but the 11.00 kernel path is not in the dump), recorded in the debt entry; a nonzero*timeoutis a microsecond deadline that returns0x817b1079when it expires, which is the 11.00 mapping of the kernel's0x8002003c. A nullresultreturns0x817b1225and a non-null trailing argument or a non-zeroreservedfield of the result returns0x817b11fe. Each request owns a stable completion state; deleting a request or its parent retires that state, wakes waiters with the invalid-id error, and drops queued and later completions without triggering a user event. Publication and retirement share the request state lock, so a completion cannot be delivered after deletion returns.event_typeis 0 for send and 1 for read, confirmed by the 11.00 completion kinds (+0x1600pairs mapped at0x9a4f).*timeoutreceives the remaining microseconds, following the forwarded kernel pointer; whether a zero timeout blocks is still an open kernel question recorded in the debt entry, an unregistered user event is not detected at submission, and a transport exception is rethrown fromsceHttp2WaitAsync.Evidence
The argument and timeout contract comes from the decrypted firmware 11.00
libSceHttp2.sprx:sceHttp2SendRequestAsyncat0x1940/internal0x4a60,sceHttp2ReadDataAsyncat0x1b20/0x4e20andsceHttp2WaitAsyncat0x1c20/0x9a00. The wait forwards the caller's pointer tosceKernelWaitSema(sema, 1, timeout)(0x4790) and remaps the kernel timeout0x8002003cto0x817b1079(0x9a79); the result and trailing-argument checks are at0x1d21,0x1ccaand0x1cd3, and the completion kind at+0x1600selectsevent_typethrough the table at0x271d0. Because the 11.00 submission bodies reject a non-null trailing pointer with0x817b11fewhile PPSA22408 passes one, the trailing pointer stays ignored and the debt entry records the version split.Tested
Linux (CachyOS), GCC 16.2.1.
guest_http2submits an asynchronous request whose body length does not match the declared content length, then checks the user event payload and thatsceHttp2WaitAsyncrethrows the worker's exception.guest_http2_transportdrives a real loopback HTTP/2 send and read through the asynchronous API, plus the async-failure, wait-versus-delete, two-waiter, parent-teardown, deletion-after-publication, optional-argument and expired-wait cases. The expired-wait case pins0x817b1079, and the null-timeout case stalls the response and checks that the wait returns only after the completion. The zero-timeout case is not pinned because its console behaviour is unknown.guest_http2,guest_http2_cookies,guest_http2_transportandguest_http2_early_responsepass in the Release build.Title: PPSA02195 polls
sceHttp2WaitAsyncwith*timeout == 0and aborted the request as soon as it received an immediate timeout; that remains the only observation and the case stays unimplemented until a console measurement settles it. It was run with other local changes on the same tree.Checklist
main; no other open PR implements the same functionsNotImplemented_nid_no_patch); silent stubs are listed under Silent stubs in their library's technical debt file.mdfiles or images added to the repository (attach them to this PR)git merge-treeon the current heads)