groups: exploring venter-like pattern

[arthyn]

Summary

Addresses TLON-4432. There's lots to be said here.

• One of the main problems that the client has is that we have to listen and guess when a subscription fact is associated with a particular action we took. This is far from ideal and means we have some brittle machinery to be able to do what should be a simple operation.
• Another problem is that the client has no way of knowing if a foreign operation errored. This was just not built into our API, but now we have that capability for these agents
• Knowing the "status" of an operation, truly this is somewhat unexplored because it requires some UI/UX thinking of how to display in-progress actions, but this new API allows us to do something like this.
• Being able to use HTTP requests is just plain easier and more ergonomic, especially if we can get a success response right away while also reducing dependency on an active SSE connection.

This does not create a library to facilitate these things yet. Instead we've manually added handling for request/response to both %groups and %channels. This does not implement venter in the way laid out by by ~niblyx-malnus here: https://github.com/niblyx-malnus/venter-pattern but it does use the basic principles.

The major thing still in progress is writing backend tests. May want to use new aqua testing not sure yet.

Finishing up:

• some cleanup of commented out code in various places
• request cleanup in channels-server
• new author types that were never realized
• random examples of showing errors, not sure we want this in all the places I put it
• cleanup of random logs both hoon and client side

Things left undone for follow-up PRs:

• modifying the message component to actually use delivery failure reasons
• converting all past usages of actions to use new actions
• using for group joins
• implementing in other agents
• trying to standardize the pattern such that it could be used as a library

Changes

• added new types in %groups and %channels to facilitate request/response
• modified agents to keep track of requests and also cleanup after requests have finished
• modified client to use new request/response paradigm in APIs with new types
• fixed click scripts newline parsing
• made a groups commit script for AI tools to be able to compile hoon themselves
• CLAUDE.md modifications to enable claude code to self compile and read errors

How did I test?

Manually through the UI trying different actions in various configurations between two ships.

Risks and impact

• Safe to rollback without consulting PR author? No
• Affects important code area:
  • Onboarding
  • State / providers
  • Message sync
  • Channel display
  • Notifications
  • Other:

Rollback plan

Rollback would require protocol shenanigans and migrations

Screenshots / videos

[mikolajpp]

Looking very nice so far. Some nits in the comments.

[mikolajpp]

This comment line is a bit confusing: what means we should give a response? If request-id is present?

[mikolajpp]

It is wild we have to do this in this way. I think if %create was not there, flag it could still be referenced without dispatching on the type...

[mikolajpp]

The use of %not-authorized here is confusing (we use it both for authorized ships who try to access privileged commands, and for an unauthorized ship trying to join the group). I think we should make a semantic distinction and introduce something like HTTP 400 Bad Request, here and elesewhere.

[mikolajpp]

This comment is not clear to me: what does it refer to? Since we are in +go-core, everything we receive are responses to our own requests.

[mikolajpp]

This can be a +weld less.

Suggested change

	=/ =path :(weld /v1 go-area /request/(scot %uv id))
	=/ =path (weld [~.v1 go-area] /request/(scot %uv id))

[Fang-]

Action and command obviously overlay cleanly onto our existing "seven things" model, but response conflicts. We can understand the addition of venter to move us to a "nine things" model, right? Would be good to explicate in the comment at the top of the sur file. Something like the following. (Forgive the name suggestions, I can't help myself, but we should probably resolve the conflict somehow to aid conversation.)

::        --action-->     --command-->
::       <··status··      <··result··
::    client       subscriber       publisher
::      <--response--     <--update--

[arthyn]

@Fang- I think response was probably the wrong term to use back in the 7 things model, since most of the time its not directly responses from things we've done but sometimes what others have done. maybe event would have been better? idk

[arthyn]

maybe revision would fit?

[mikolajpp]

On the other hand, I don't think response was ever being predicated on being generated in response to our action? The explainer in /sur/channels.hoon merely stipulates that an action, whether local or remote, may become a response, which I think is a good enough mental model.

[Fang-]

Mostly looked at server side so far, haven't done a full pass over the client agents yet. Some comments/questions below.

[Fang-]

Probably shouldn't be committing this amount of verbosity. (;

[Fang-]

Kinda spooky how, if we forget to +ca-init-req, the request-id can just be the bunt and we wouldn't know. Certainly weird to give out a fact in that case, don't you think?

Of course there's limited entry points that want to set it, but doesn't that suggest we want those (just +ca-create and +ca-c-channel?) to take a request-id argument, instead of having a separate arm for setting it?

[Fang-]

Also, the assumption with putting src.bowl in the path here instead of remembering the source that originated the request in state, is that presently all requests get handled synchronously, right? Probably want to ::NOTE that assumption here.

But if they get handled synchronously, why do we need to store them in requests? (During %channel-command and +watch handling.)

[Fang-]

Also also, we never write into requests beyond the %sending state we gut:by during poke handling. Don't we want to update the request state here to contain the body?

[Fang-]

The handling of requests here is identical to the %channel-command poke case. That doesn't seem right. I don't think we should invent a request based on a subscription for it coming in. Additionally, we probably want to send an initial fact if it did already exist.

[mikolajpp]

Noting that this is also the case in groups.

[Fang-]

Presently we never clean these up/out, but we should periodically check all entries' final-at for that, right?

Right, that's one of the remaining tasks you noted:

request cleanup in channels-server

[Fang-]

Given that a %channel-command is always a new request, not clear to me why we gut:by here. We'd want to just overwrite whatever's there, right?

[Fang-]

There's a mismatch here, where the subscription path is per source ship per request-id, but we only store them by request-id. Doesn't this let people clobber each other's request statuses?

[Fang-]

If we don't care to remember the requests, shouldn't we just keep sending plain unidentified actions? Alternatively, just pass *@uv or something similar that makes it obvious we don't care and won't use it.

[Fang-]

Since these are basically identical between the agents, and we expect them to remain that way, could be worth factoring out into a /sur/vent or what have you. The result=(unit response-body) will be different, but easy to parameterize with a |$ type constructor.

[Fang-]

Why the reduced expiration time for successful requests? Is the assumption that in the success case, the result was near-instant and/or the delivery of the result most likely?

[mikolajpp]

Reviewed the groups agent. This line of work looks quite promising! Left quite some comments, most of them minor.

[mikolajpp]

I am curious to know why this is better?

[mikolajpp]

Suggested change

	(pure:m !>(%ok))
	(pure:m !>(&))

Noting in passing (also for myself, to fix this in the backend test runner) that this makes for a weird terminal output from click: it will show the numeric value of %ok, rather than the term itself.

[mikolajpp]

We must be careful here when merging to bump the version number.

Edit: this is because subscription lifecycle PR, which likely will go in before this PR, is currently at v11. v10 was already merged with some other PR.

[mikolajpp]

Suggested change

	++ response
	++ response

[mikolajpp]

Suggested change

	%ok (r-group r-group.body)
	%error (error +.body)
	%pending s+status.body
	%ok (r-group r-group.body)
	%error (error +.body)
	%pending s+status.body

[mikolajpp]

This is different from the logic introduced in +go-send-command. Should +go-send-command be used here? Though I guess after issuing +leave-group we are usually going to delete the group altogether, so perhaps this is fine.

[mikolajpp]

Can we not have a request entry for an issued command with venter? If this is unexpected we should log at least a warning or even an error here.

[mikolajpp]

We have just put a modified request into requests on L3986. We can just preserve it for use here.

[mikolajpp]

Will the server not kick us after fulfilling request? If so, we should probably condition the resubscription on the request status.

[mikolajpp]

Using entropy for id means that we could overwrite requests if we poke more than once in an event. I am not sure if this would be a problem for internally issued actions, so perhaps this is fine?

Or is entropy different on each agent activation. A cursory look at gall does not make me think this is the case. CC @Fang-

[Fang-]

Each agent activation has a fresh eny, yes.

It's generally considered good practice to hash or otherwise tweak the entropy, rather than taking raw bytes from eny directly. Here, for example, you might consider using the +mug of the action as a salt, to avoid the case you describe, a la (end 3^8 (shas (mug action) eny.bowl)).

[mikolajpp]

Some more comments. On the way to finish the review: only channels and channels-server remain.

[mikolajpp]

The drag on productivity for agents due to unbearably slow compilation is going to be even worse that it is for us. (That is, unless they can one-shot Hoon) Is it usable at this point for any non-trivial changes?

[mikolajpp]

A check is missing here.

Suggested change

	=+ ship=(slav %p ship.pole)
	=+ ship=(slav %p ship.pole)
	?> =(our.bowl ship)

[mikolajpp]

Suggested change

	:: disseminated to clients we ourselves are using.
	:: disseminated to client subscribers.

[mikolajpp]

This is confusing: there are in fact two different agents, unlike with groups.

Suggested change

	:: actions are requests to the agent as a channel client. most actions
	:: actions are requests to the %channels agent. most actions

[mikolajpp]

Suggested change

	:: commands are requests to the agent as a channel host. they are checked
	:: for permissions.
	:: commands are requests to the %channels-server agent. they are checked
	:: for permissions.

[mikolajpp]

Suggested change

	:: $a-channels: channels actions
	:: $a-channels: channel actions

[mikolajpp]

Noting that this is also the case in groups.

[mikolajpp]

With recent type versions in groups I used a convention where: (1) we document the reason for type version change, such us "X depends on Y", or "X modified ...". (2) for new types we include the original comment.
See /sur/groups-ver.hoon for examples.

This creates a paper-trail accesible right in the code, and is really helpful for understanding changes between versions.

[mikolajpp]

Since we shield this type with lib-negotiate, it seems like this should not be versioned? Types that are in any way tied up with the client interface should not leak beyond the channels agent.

(Also see similar comments in groups)

[mikolajpp]

Given that venter is supposed to give us an ability to return response errors back to the requester, we should probably crash, if that would be returned to the requester as a response error.

[mikolajpp]

Another batch of comments: reviewed %channels.

[mikolajpp]

This change is probably a mistake, it would allow remote ships to hide our own posts.

[mikolajpp]

This is at odds with behavior in groups: there we use 0v0 as the id, which in particular means repeated requests will overwrite.

[mikolajpp]

We don't mutate request in scope here, so we should either define an alias or simply use it as is.

Suggested change

	=/ new-req (~(got by requests) request-id)
	=. requests
	(~(put by requests) request-id new-req(fetched &))
	=. requests
	(~(put by requests) request-id u.request(fetched &))

[mikolajpp]

I understand we only ever receive timeouts on this wire, but I think it could be worth listening on /request/$id/timeout wire for legibility and to make any future changes easier.

[mikolajpp]

This says "if we have a request-id", but there is no conditional. Is there something missing here?

[mikolajpp]

We don't set the status as %nacked unless we don't actually have the request. Is this correct? (We do the same in groups)

[mikolajpp]

Saying "(...) for the request timout" would make it clearer. Also should be fine feels like we are not sure.
It would be better if it says something like "but we safely ignore it", and make sure that is true.

[mikolajpp]

Interestingly, we preemptively update our local state without waiting for the fact. Does this mean each time a subscriber sends an action he will apply the same update twice, first from the response, second from the channel update fact?

I think we are guaranteed that these two updates will arrive one after another, without any intervening facts in between?

CC @Fang-

[Fang-]

I don't think we have any hard ordering guarantees here, the facts will travel on two separate ames flows. (Because they happen on two different subscriptions with two different wires.)

So, probably theoretically possible to hear other stuff in between the two. (But, you obviously won't hear two facts from the same subscription out of order.)

[mikolajpp]

Then the following scenario is possible:
If Alice's action A caused update fact B (with update B* arriving as request response), and a subsequent Kevin's action K caused fact update L, it is possible that Alice is going to hear the facts in either of the orders:

1. B* < B < L
2. B < L < B*

While we are guaranteed that B < L, because they arrive on the same flow, in case (2) we apply L followed by B* out of order, which can cause problems. While it is possible that our update operation is idempotent (though this needs a careful check – it is not a given. creating a resource twice, for instance, could cause crashes), they are definitely not commutative.

For instance, in channels, if B = create a post, and L = delete a post, in scenario (2) Alice ends up recreating the post wrongly in her local state. In groups, on the other hand, (2) will cause a resync, which triggers on updates arriving out of order.

These problems could be avoided in groups by checking timestamps in those two cases to avoid applying duplicated update fact B in (1), and in (2) to drop the out-of-order update B*. It seems like channels receive updates out of order by design, so this solution might not apply straight away.

[mikolajpp]

I am not quite sure what are we doing here: I think the comment should be attached to the branch where we do handle the creation, because here we actually handle a few different things.

[mikolajpp]

I think handling creation in a separate arm, like we used to, would prove to be less confusing. Are there good reasons why we decided to unify those two arms?

[mikolajpp]

Last batch of comments regarding channels-server!
Found a few things which looked like could be an issue, other than that mostly nits.

[mikolajpp]

But this still leaves the question whether we should not perhaps just overwrite it, as @Fang- noted in channels-server.

[mikolajpp]

Very good changes!

[mikolajpp]

It is hard to understand what this comment refers to without some more context. Are we saying a possible error was already generated somewhere else, for example in +ca-c-post?

[mikolajpp]

Ideally this comment should be attached to the place it refers to: it is enough to attach it to the first conditional below.

[mikolajpp]

The ordering of arguments here is different from the one in groups. We should choose one and stick to it. (I prefer this ordering to the one from groups, as it groups channel-related data together)

[mikolajpp]

Isn't this the same error as on L1155 above? We should either combine these two conditionals into one, or unify (alternatively, meaningfully distinguish) the error messages

[mikolajpp]

Before this change there was only one assertion (corresponding to L1169 after change), now there are two. Are we fixing some bug at the same time?

[mikolajpp]

Is there any reason we +slog here? Given the logging facilities we have now, I think we should stop using +slog on principle and prefer to log with appropriate priority.

[mikolajpp]

I recall that above in +ca-c-post arm there were two conditionals rather than one, one comparing with the author of the existing reply, another with the author of the reply command. Does not this mean here we can modify our own reply to appear as originating from another user?

[mikolajpp]

Let's log rather than slog :-)

[mikolajpp]

Slog we should not, let's log! 🎵