Feature - Implemented Prevent multiple instances of kolmafia from accessing the same account

Sometimes in a fortnight I misclick and accidentally log into an account that's running turns on my second instance of mafia. This obviously does not go well.

We have a session lock, I'm not sure if its possible to make another session lock for a username.

So when you have an account selected in your login menu, it should have a greyed out login button. If you try to log in with a script, perhaps it fails "The account 'Irrat' has a session lock"

My vague recollection, from reading stuff from before I started even using KoLmafia...

The reason for two instances being the limit, instead of one, is that sometimes the session lock files weren't cleaned up properly, for whatever reason. So there wasn't actually an expectation that two instances would be run at once, it was just meant to reduce the chance that one would be unable to open even a single instance.

If the plan is to continue with that initial spirit, then anything working to support multiple instances doesn't make sense.

If that isn't the plan, then maybe changes to the session lock system could be discussed more generally. Does two instances make sense, instead of something like 5? Should any limit even exist still? Only 10 ports are checked for relay browser use, a limit that someone might care about if there is no instance limit.

I have no opinion about how things should be done, I'm just throwing in some historical context.

I've changed my copy to support four and I regularly run three (ascender, nonascender, easyfax). I haven't had any problems that I'm aware of due to running so many.

The problem I do have is typing in one instance (or somewhere else) while a second instance is launching and it grabs my text input and I hit enter causing it to log onto two account simultaneously. I consider that to be entirely user error.

An original motivation and implementation philosophy for KoLmafia was to reduce server hits on Kingdomofloathing.com. So multiple simultaneous instances were not really encouraged because that produced more hits. Bots, especially mallbots were not welcomed. However that is not absolute as the community benefits of faxbots and buffbots are acknowledged.

I remember the session lock files as a way to restrict simultaneous sessions and we have two because of some implementation issues with Windows. Windows often failed to delete a file and it was easier to allow two than it was assist frustrated users when they couldn't run because the lock had not been deleted. If we, and Windows, has been smart enough to reliably use a file as a session lock there would be one session.

In practice the philosophical limit is easy to bypass. Anyone who is willing to build their own version can trivially change the limit. Someone who is not willing can still run multiple instances of mafia from separate local directories.

The OP's issue can be addressed by changing the session limit to one, or partially addressed by disabling the ability to select an account from a dropdown and making the user always log in and type the name of the character. There are also several possibilities that essentially require a robust way to communicate between processes and that will need some design work to work across all platforms. Java can definitely support that but it strikes me as a lot of work for something that could also be addressed by telling the user "Don't Do That". Given that there is conversation about a KoLmafia tool that updates and then launches the jar it is possible the ability to detect and prevent multiple logins into the same character might be a side-effect of that work if it happens.

Changing the session limit can be discussed as a philosophical issue. I do expect there will need to be some additional implementation work. While there are plenty of anecdotal evidence that multiple sessions can coexist and play nicely with the file system, there are also anecdotes about the GLOBAL settings files getting trashed in multiple session scenarios. I figure it happens about every six months for me but I have not felt it worth the time or effort to try and reproduce.

The original change from supporting 5 simultaneous users to only 2 was hard for me to accept. I liked the ability to run several users at once, and never seemed to encounter any problems that seemed, to me at least, to be related to it. So I soon reverted the change on my personal build and continued to run more that 2 users at once. Without any factual basis to support it, I adopted a personal policy of always completely closing each instance before starting the next. As I say, I have no real reason to believe that made any real difference, but I didn't seem to encounter any problems with this method of operation. And thus I continued to run this way until about the beginning of this year. At that time I bought a new computer that was supposed to be a lot more powerful than my previous one, so I decided to kick it up another notch and increased the number of simultaneous users from 5 to 6. This also seemed to work fine and is the way I've operated for most of this year.

My typical day is to start up a batch of 6 chars and let them run to completion as I wander off and do other things. At some point, I come back and clean things up, restarting any that chars that stopped running for whatever reason, rinse, lather, and repeat. This works fine for my needs and I'd like to see mafia restored to actually officially supporting more than 2 simultaneous users. But as I say, this seems to meet my needs and I can continue running my local build if the mafia devs feel this is not the way to go.

For the record, I'm retired and basically have no life. After starting to play KoL in Dec. 2005, I soon had what seemed to be the standard configuration of mulits at that time: 1 softcore, 1 hardcore, and 1 hardcore oxy. I played that way for some time, then one manic day I decided I wanted a char of each class and proceeded to create 6 new multis in one day. That went ok for a while, but then things started bogging down and I looked for a way to speed things up. That was when I discovered mafia and start using it. It was a lot of fun and a way to dabble in some programming which I missed after retiring. Mainly ASH programming, but some poking around at the mafia innards and thus java. Since them, I've resisted to urge to create any more chars, but have adopted a few orphans when friends who had a character with some some nice, but untradable items like familiars and bound items of the month, decided to quit the game.

I'll add my anecdotal statistics to this discussion. Since probably 2007 when I first started using mafia, I've run two concurrent instances of mafia. I run my main on one instance while running a handful of multis on the the other, since I always play my main manually and it takes more time. The multis are mostly automated, to the point where unless I want to do anything special (like ascend one of them, or participate in a Kingdom event) I can type "routine" on the first one and mafia will automate all of them sequentially.

It is my hope to be able to continue doing as I have and would like to request that the session limit not be set to 1. I have had little to no issues with corrupt settings files aside from when I would play a character remotely without shutting down mafia on my home system, which I would also consider user error. (These days if I find myself wanting to play on a machine other than my home computer, I will simply use a remote access tool to play my characters on my home machine.) In fact, increasing the limit would be helpful for people who have more than two accounts and find themselves an hour or two from rollover without having played any turns yet today.

zarqon said:

I'll add my anecdotal statistics to this discussion. Since probably 2007 when I first started using mafia, I've run two concurrent instances of mafia. I run my main on one instance while running a handful of multis on the the other, since I always play my main manually and it takes more time. The multis are mostly automated, to the point where unless I want to do anything special (like ascend one of them, or participate in a Kingdom event) I can type "routine" on the first one and mafia will automate all of them sequentially.

It is my hope to be able to continue doing as I have and would like to request that the session limit not be set to 1. I have had little to no issues with corrupt settings files aside from when I would play a character remotely without shutting down mafia on my home system, which I would also consider user error. (These days if I find myself wanting to play on a machine other than my home computer, I will simply use a remote access tool to play my characters on my home machine.) In fact, increasing the limit would be helpful for people who have more than two accounts and find themselves an hour or two from rollover without having played any turns yet today.

Click to expand...

Just to clarify because I think there's some confusion going on.

I'm not talking about raising or lowering the instance limit.

Just a sanity check to prevent one account being logged into with multiple instances. So the instances are not fighting over who is logged in.

Irrat said:

Just to clarify because I think there's some confusion going on.

I'm not talking about raising or lowering the instance limit.

Just a sanity check to prevent one account being logged into with multiple instances. So the instances are not fighting over who is logged in.

Click to expand...

We veered because of history lessons. That said, I did say reducing the session count to a max of one would prevent your problem although I certainly would not advocate doing that.

I also think that if we trusted the file locking and deconflicted simultaneous access we could then trivially address your problem by keeping track of logged in characters in a file and checking the file at login. But there is more motivation for me to personally undertake that effort if we choose to raise the session limit.

In general, the biggest potential for corruption / data loss is in editing shared resources (namely: datafiles / global prefs, exacerbated by settings like saveSettingsOnSet -- global preferences in particular have lots of opportunity to clash with each other). If scripts are consistently well-behaved by only touching per-character datafiles and not updating global preferences, then this is less of a concern.

One simple solution to the specific request is to change the locks to incorporate the current character's name (possibly normalized, e.g. all lowercase, spaces replaced with underscores). This has the added bonus of being human-readable, and simultaneously removes the artificial limit of 2 simultaneous logins.

https://github.com/kolmafia/kolmafia/pull/1134 if anyone wants to try it out. I make no promises about the integrity of your datafiles.

User preferences are OK.

GLOBAL preferences are at risk.
So are mallprices.

Anything else that is written/updated that is shared?

Theoretically any custom datafiles, especially those auto-updated by scripts.

Any chance for svn / git clashes, e.g. when updating scripts? Those should be locked, but I'm not convinced all the operations we do are atomic.

Git doesn't lock, but I think at the moment everything is atomic: one side will succeed, the other will crash.

After I do the "allow local changes that might conflict" thing, that will change.

We need to share data between the instances, so I think using global preferences is the right move. Or some other file on the filesystem.

My concern re: global preferences is that today we read in GLOBAL_prefs.txt on process startup (i.e. in a static { } block), and then never again. And, when we write to file, we clobber the entire existing file. It's very easy to edit global preferences in one instance, only to have them overridden by another. Bad things may happen if both instances try to edit the same file at the same time, as we don't use any filesystem-wide locking. Even if we did, one instance would win arbitrarily. You still have data loss.

There was some dev discussion a while back about using databases (whether fully-featured SQL, or something simpler like a key-value store). That would be an obvious solution here. As a very significant consequence, though, prefs files would no longer be easily human-editable / -readable. We could take that further, and transform data files to this new mechanism. It'd also fix mallprices.txt races and other custom script datafiles (I'm thinking anything that updates its map files via zarqon's website). Same drawback.

Ryo_Sangnoir said:

Git doesn't lock, but I think at the moment everything is atomic: one side will succeed, the other will crash.

Click to expand...

That doesn't totally line up with my notion of atomicity, although I suppose it is compatible. I'm most worried about interleaving of operations that leaves the repository in a state that's inconsistent with the system model maintained by either session.

I was reminded of this bug when I browsed the list of open PRs and saw my proposed fix.

This is now r27042.

I'm getting an error in r27096.

Scenario: I left a long-running script to run while at work and didn't come back until after rollover.

I came back to see how much meat I had earned and to do a few tasks before restarting. My session had expired, so I pressed "Refresh Status"


I never run multiple sessions and while I would like to reject even a 2nd concurrent login, I also think that we need to handle cases where there's an abandoned session more robustly. I think file locks are released if the application crashes, but not if it hangs or (in my case) the server logs the end-user out for timeouts.

Do we have a way of identifying the locking process? If it's us, then we should clear the lock.

In some less-Java-y environments, I'd expect a PIDfile with the PID of the lock in /tmp or some similar place and I could check if the process that acquired the lock was actually running.

We might also write the PID to the GLOBALS.pref, since we already have a file.

Another option: if we already have the lock for a given character (we store the locked file in KoLmafia.SESSION_FILE), don't bother trying to reacquire the lock.

@heeheehee, I don't want to fix this since it's your code and you may already be planning to fix it (in part because I don't really understand why you're doing it the way you're doing it), but I think breaking sleeping through rollover is a bad side effect.

I'm not sure why you're putting the version string in the lock file and not the PID, so we could identify if a live process or our own main thread has the lock. Java 9 gives us access to ProcessHandle.current().pid() .

I would expect that if we want to acquire a lock, we'd either check to see if it was our lock or try to delete the lock. If we can do either of those, we can get or assume we have the lock. Or maybe when we log in, after inactivity, we try releasing our lock before acquiring it, or checking the PID.

I'm also concerned about the things I am reading about file locks being advisory on some OSes and enforced on others and making sure we have a design that makes sense across the platforms people are using.

MCroft said:

@heeheehee, I don't want to fix this since it's your code and you plan to fix it (in part because I don't really understand why you're doing it the way you're doing it), but I think breaking sleeping through rollover is a bad side effect.

I'm not sure why you're putting the version string in the lock file and not the PID, so we could identify if a live process or our own main thread has the lock. Java 9 gives us access to ProcessHandle.current().pid() .

I would expect that if we want to acquire a lock, we'd either check to see if it was our lock or try to delete the lock. If we can do either of those, we can get or assume we have the lock. Or maybe when we log in, after inactivity, we try releasing our lock before acquiring it, or checking the PID.

I'm also concerned about the things I am reading about file locks being advisory on some OSes and enforced on others and making sure we have a design that makes sense across the platforms people are using.

Click to expand...

I think what is written to the lock file is arbitrary. I don't find a place where it is being read. Writing the version was done in 2021. I didn't check whether it was newly added or change what was written.

The code doing the NIO channel locking goes back to 2007 so we have lived with concerns about what the OS does for at least a decade. My research says that the OS behavior is not a factor if all the processes that might access the lock are in agreement about how to do so and what failures mean. So if the lock is to protect one instance of mafia from another then it is working as expected, regardless of OS.

I think the PR exposed a logic flaw elsewhere rather than broke waking up. The old code did not fail unless it failed to acquire lock 1 and lock 2. My hypothesis is that your instance had lock 1. After rollover it tried to get lock 1, failed, but succeeded in getting lock 2 so you did log back in anyway. Someone with motivation could try and prove/disprove this by using a version before this PR and looking at the contents of the session directory. It will be a 24+ hour test, however.

I do think you are on the right track with the PID.

If the file doesn't exist, create it, associate it with the current PID and obtain the lock.

If the file exists, check the PID. If it is the same PID, release and reacquire the channel lock. If it is not the same PID then abort with a can't get lock failure.

If the username and the PID uniquely define a filename then it is possible that the channel level locking is no longer needed.

MCroft said:

@heeheehee, I don't want to fix this since it's your code and you may already be planning to fix it (in part because I don't really understand why you're doing it the way you're doing it), but I think breaking sleeping through rollover is a bad side effect.

Click to expand...

Sorry, I forgot about this bug. Thanks for bringing it back to my attention. I agree that the current state is bad.

MCroft said:

I'm not sure why you're putting the version string in the lock file and not the PID

Click to expand...

existing code, dates back to 2007 :/

My change didn't really touch the implementation of acquireFileLock, other than changing its visibility.

MCroft said:

I would expect that if we want to acquire a lock, we'd either check to see if it was our lock or try to delete the lock. If we can do either of those, we can get or assume we have the lock. Or maybe when we log in, after inactivity, we try releasing our lock before acquiring it, or checking the PID.

Click to expand...

Another way to view the bug is that releaseFileLock is not called in all the scenarios we call acquireFileLock. Tying releaseFileLock to LogoutManager is not correct if LogoutManager is not the only way a player can be logged out.