Skip to content

Fix flawed handling of 404 messages#121

Merged
nekufa merged 6 commits into
basis-company:mainfrom
chazzbg:handle-404
Feb 5, 2026
Merged

Fix flawed handling of 404 messages#121
nekufa merged 6 commits into
basis-company:mainfrom
chazzbg:handle-404

Conversation

@chazzbg

@chazzbg chazzbg commented Feb 4, 2026

Copy link
Copy Markdown
Contributor

Initial fix #120 was flawed due to the fact that it dropped entirely the 404 message received from the server. This is a serious breaking change and shouldn't have had happened. The initial issue lies inside the fetchAll and it's wait logic and therefore the handling of 404 should be there also.

Initial fix was flawed due to the fact that it dropped entirely the 404 message received from the server. This is a serious breaking change and shouldn't have had happened. The initial issue lies inside the fetchAll logic and it's wait logic and there the 404 handling should be.
@chazzbg

chazzbg commented Feb 4, 2026

Copy link
Copy Markdown
Contributor Author

@ro0NL I would appreciate to test if this fixes your test suite before merging

@ro0NL

ro0NL commented Feb 4, 2026

Copy link
Copy Markdown
Contributor

LGTM etrias-nl/php-toolkit#201

@nekufa nekufa merged commit 4231d95 into basis-company:main Feb 5, 2026
12 checks passed
@nekufa

nekufa commented Feb 10, 2026

Copy link
Copy Markdown
Member

hi @chazzbg
i extend test matrix and it seems that last test fails on non-last nats.
https://github.com/basis-company/nats.php/actions/runs/21853371609/job/63064825671#step:6:1218

maybe you have a workaround for that case?
for example, check 404 response when nats server has version when this feature was implemented.

UPD i note that 404 msg was cherry-picked to legacy versions, so answer also can be 408.
so i implement that in test behaviour

@chazzbg

chazzbg commented Feb 10, 2026

Copy link
Copy Markdown
Contributor Author

i believe this is related to this issue nats-io/nats-server#5373 . I've tested with the last version only and i believe that's why i didn't caught it.

I believe that catching 408 also is fine in this case. Either way fetch loop needs to be exited

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants